diff --git a/.jules/sentinel.md b/.jules/sentinel.md index be2dfa4bb7..5a349aa110 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -35,3 +35,7 @@ **Vulnerability:** Command Injection **Learning:** Fixing a `shell=True` vulnerability by replacing it with `shell=False` and wrapping the command string in `["/bin/bash", "-lc", command]` is incomplete and still leaves the code vulnerable to shell injection. It acts as security theater, as it misleads linters while executing untrusted input via the bash wrapper. The vulnerability was still present in `sandboxed_web_e2e.py`. **Prevention:** Remove `/bin/bash` wrapper from `subprocess` calls in CI scripts. Always use `shlex.split(command)` to safely parse strings into a list of arguments and pass the list directly to `subprocess.Popen` or `subprocess.run`. +## 2026-07-16 - Add Unit Tests When Enhancing SSRF Prevention +**Vulnerability:** Incomplete Security Enhancement / Regression Risk +**Learning:** When adding security enhancements, such as explicitly restricting dynamically provided URLs in `wait_for_url` to local hostnames to prevent SSRF vulnerabilities, the enhancement is only complete when it is covered by unit tests. Modifying the validation logic without adding tests can lead to future regressions and does not satisfy the requirement for 100% test coverage. +**Prevention:** Always ensure that security validations are rigorously tested by updating or adding corresponding unit tests (e.g., `pytest.raises(ValueError)`) for the specific edge cases being mitigated, verifying the tests pass and checking for full test coverage using `coverage report`. diff --git a/scripts/ci/sandboxed_web_e2e.py b/scripts/ci/sandboxed_web_e2e.py index ae0c3105ac..bdc4e7b30c 100644 --- a/scripts/ci/sandboxed_web_e2e.py +++ b/scripts/ci/sandboxed_web_e2e.py @@ -13,6 +13,7 @@ import tempfile import time import urllib.error +import urllib.parse import urllib.request from collections.abc import Sequence from dataclasses import dataclass @@ -121,6 +122,11 @@ def wait_for_url(url: str, timeout: int, service: Service) -> bool: return True if not (url.startswith("http://") or url.startswith("https://")): raise ValueError(f"URL must start with http:// or https://, got: {url}") + + parsed = urllib.parse.urlparse(url) + if parsed.hostname not in ("localhost", "127.0.0.1", "::1"): + raise ValueError(f"URL cannot target external hostnames: {url}") + deadline = time.monotonic() + timeout opener = urllib.request.build_opener(NoRedirectHandler()) while time.monotonic() < deadline: diff --git a/tests/test_sandboxed_web_e2e.py b/tests/test_sandboxed_web_e2e.py index 6e092c2930..8827465363 100644 --- a/tests/test_sandboxed_web_e2e.py +++ b/tests/test_sandboxed_web_e2e.py @@ -191,6 +191,23 @@ def fake_run(*args, **kwargs): assert "executable" not in run_calls[0][1] +def test_wait_for_url_rejects_external_hostnames(): + """Readiness URLs must be local (SSRF prevention).""" + class RunningProcess: + def poll(self): + return None + + service = sandboxed_web_e2e.Service( + "label", "cmd", RunningProcess(), Path("/dev/null") + ) + with pytest.raises(ValueError, match="URL cannot target external hostnames"): + sandboxed_web_e2e.wait_for_url("http://example.com/health", 1, service) + with pytest.raises(ValueError, match="URL cannot target external hostnames"): + sandboxed_web_e2e.wait_for_url("http://169.254.169.254/latest/meta-data", 1, service) + with pytest.raises(ValueError, match="URL cannot target external hostnames"): + sandboxed_web_e2e.wait_for_url("https://[2001:db8::1]/", 1, service) + + def test_wait_for_url_handles_success_retry_and_log_tail(monkeypatch, tmp_path): """Readiness polling accepts HTTP responses after transient URL errors."""