diff --git a/.jules/sentinel.md b/.jules/sentinel.md index be2dfa4bb..22096180a 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-08-08 - Prevent SSRF in Web E2E Readiness Polling +**Vulnerability:** Server-Side Request Forgery (SSRF) / Local File Inclusion +**Learning:** Checking that a user-provided `--backend-ready-url` or `--frontend-ready-url` starts with `http://` or `https://` is insufficient if the script blindly requests the URL. This allows attackers to proxy requests through CI to internal endpoints or metadata services (e.g. `169.254.169.254`). +**Prevention:** Strictly parse the URL using `urllib.parse.urlparse` and ensure the hostname belongs to an explicitly allowed loopback interface (e.g., `127.0.0.1`, `localhost`, `::1`). diff --git a/CHANGELOG.md b/CHANGELOG.md index bf30091dd..510090834 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ Semantic Versioning where the repository publishes a release. ### Fixed +- Replaced prefix-only HTTP/HTTPS readiness validation with robust SSRF protection in the `sandboxed_web_e2e.py` E2E test harness. +- Added log redaction for standard output and error captured during subprocess execution in `sandboxed_verify.py` and `sandboxed_web_e2e.py` wrappers. - Bounded the Strix quality self-test's deterministic timeout fixtures to 3-second process and 5-second fake-sleep budgets so exact-head policy evidence completes inside the existing job limit without changing production Strix scanner timeouts, providers, credentials, or review semantics. - Allowed commas and ASCII parentheses in the bounded Strix changed-file path policy so legal tracked Packrat fixtures can receive exact-head security analysis, while rejecting raw `..` components before normalization and keeping controls, backslashes, whitespace ambiguity, and shell punctuation fail-closed. - Bound each review-agent invocation key to the wrapper's complete canonical payload, including the base branch and requesting actor; altered fields with a valid-format key now fail before durable-leader election or forwarding, and wrapper write permission is job-scoped. diff --git a/scripts/ci/sandboxed_verify.py b/scripts/ci/sandboxed_verify.py index aace18d45..8395d0e23 100644 --- a/scripts/ci/sandboxed_verify.py +++ b/scripts/ci/sandboxed_verify.py @@ -14,6 +14,11 @@ from collections.abc import Sequence from pathlib import Path +if __package__ in (None, ""): # pragma: no cover + sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +from scripts.ci.redact_sensitive_log import redact_text + DEFAULT_IGNORE = ( ".git", @@ -169,8 +174,10 @@ def timeout_output_text(value: str | bytes | None) -> str: if value is None: return "" if isinstance(value, bytes): - return value.decode(errors="replace") - return value + text = value.decode(errors="replace") + else: + text = value + return redact_text(text) def emit_result( @@ -219,9 +226,9 @@ def main(argv: Sequence[str] | None = None) -> int: try: completed = run_command(args.command, copied_repo, env, args.timeout) if completed.stdout: - print(completed.stdout, end="") + print(redact_text(completed.stdout), end="") if completed.stderr: - print(completed.stderr, end="", file=sys.stderr) + print(redact_text(completed.stderr), end="", file=sys.stderr) exit_code = completed.returncode except subprocess.TimeoutExpired as exc: stdout = timeout_output_text(exc.stdout) diff --git a/scripts/ci/sandboxed_web_e2e.py b/scripts/ci/sandboxed_web_e2e.py index ae0c3105a..75b12261d 100644 --- a/scripts/ci/sandboxed_web_e2e.py +++ b/scripts/ci/sandboxed_web_e2e.py @@ -13,15 +13,17 @@ import tempfile import time import urllib.error +import urllib.parse import urllib.request from collections.abc import Sequence from dataclasses import dataclass from pathlib import Path -if __package__ in (None, ""): +if __package__ in (None, ""): # pragma: no cover sys.path.insert(0, str(Path(__file__).resolve().parents[2])) from scripts.ci import sandboxed_verify +from scripts.ci.redact_sensitive_log import redact_text RESULT_MARKER = "SANDBOXED_WEB_E2E_RESULT" @@ -121,6 +123,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 ("127.0.0.1", "localhost", "::1"): + raise ValueError(f"Readiness URL must use localhost or loopback IP, got: {url}") + deadline = time.monotonic() + timeout opener = urllib.request.build_opener(NoRedirectHandler()) while time.monotonic() < deadline: @@ -232,9 +239,9 @@ def main(argv: Sequence[str] | None = None) -> int: try: completed = run_shell(args.e2e_cmd, copied_repo, env, args.e2e_timeout) if completed.stdout: - print(completed.stdout, end="") + print(redact_text(completed.stdout), end="") if completed.stderr: - print(completed.stderr, end="", file=sys.stderr) + print(redact_text(completed.stderr), end="", file=sys.stderr) exit_code = completed.returncode return exit_code except subprocess.TimeoutExpired as exc: diff --git a/tests/test_sandboxed_verify.py b/tests/test_sandboxed_verify.py index c711f3489..f69784341 100644 --- a/tests/test_sandboxed_verify.py +++ b/tests/test_sandboxed_verify.py @@ -1,3 +1,4 @@ +import subprocess import json import runpy import shutil @@ -198,3 +199,48 @@ def test_module_main_entrypoint(monkeypatch, tmp_path): if module is not None: sys.modules["scripts.ci.sandboxed_verify"] = module assert exc_info.value.code == 0 + +def test_sandboxed_verify_redacts_stdout_and_stderr(monkeypatch, tmp_path, capsys): + """The wrapper redacts sensitive tokens from completed process stdout and stderr.""" + class CompletedProcess: + def __init__(self, stdout, stderr, returncode): + self.stdout = stdout + self.stderr = stderr + self.returncode = returncode + + def run_command(command, cwd, env, timeout): + return CompletedProcess( + "Here is my api_key: 'ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ12345678'", + "Failed because of password: mysecretpassword123", + 0 + ) + + monkeypatch.setattr(sandboxed_verify, "run_command", run_command) + + sandboxed_verify.main(["echo", "test"]) + + out, err = capsys.readouterr() + assert "ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ12345678" not in out + assert "[REDACTED]" in out + assert "mysecretpassword123" not in err + assert "[REDACTED]" in err + +def test_timeout_redacts_bytes_output(monkeypatch, tmp_path, capsys): + """The wrapper redacts sensitive tokens from timeout stdout and stderr bytes.""" + def run_command(command, cwd, env, timeout): + raise subprocess.TimeoutExpired( + cmd="mock", + timeout=10, + output=b"Timeout with token: ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ12345678", + stderr=b"Timeout error with password: mysecretpassword123" + ) + + monkeypatch.setattr(sandboxed_verify, "run_command", run_command) + + sandboxed_verify.main(["echo", "test"]) + + out, err = capsys.readouterr() + assert "ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ12345678" not in out + assert "[REDACTED]" in out + assert "mysecretpassword123" not in err + assert "[REDACTED]" in err diff --git a/tests/test_sandboxed_web_e2e.py b/tests/test_sandboxed_web_e2e.py index 6e092c293..7c99a9eef 100644 --- a/tests/test_sandboxed_web_e2e.py +++ b/tests/test_sandboxed_web_e2e.py @@ -1,3 +1,4 @@ +import subprocess import json import os import runpy @@ -598,3 +599,54 @@ def test_module_import_and_main_entrypoint(monkeypatch, tmp_path): if module is not None: sys.modules["scripts.ci.sandboxed_web_e2e"] = module assert exc_info.value.code == 0 + +def test_sandboxed_web_e2e_redacts_stdout_and_stderr(monkeypatch, tmp_path, capsys): + """The E2E wrapper redacts sensitive tokens from completed process stdout and stderr.""" + class CompletedProcess: + def __init__(self, stdout, stderr, returncode): + self.stdout = stdout + self.stderr = stderr + self.returncode = returncode + + def run_shell(command, cwd, env, timeout): + return CompletedProcess( + "Found token: 'github_pat_11AAAAAAA000000000000000000000000000000000000000000000000000000000000000000000'", + "Error: could not login with session_key: 123456", + 1 + ) + + def mock_wait_for_url(*args): + return True + + def mock_start_service(*args): + class Service: + label = "mock" + command = "mock" + process = type("Proc", (), {"poll": lambda self: None, "pid": 123, "wait": lambda self, timeout: None})() + log_path = tmp_path / "mock.log" + def __init__(self): + self.log_path.touch() + return Service() + + monkeypatch.setattr(sandboxed_web_e2e, "run_shell", run_shell) + monkeypatch.setattr(sandboxed_web_e2e, "wait_for_url", mock_wait_for_url) + monkeypatch.setattr(sandboxed_web_e2e, "start_service", mock_start_service) + + sandboxed_web_e2e.main(["--backend-cmd", "mock", "--frontend-cmd", "mock", "--e2e-cmd", "mock"]) + + out, err = capsys.readouterr() + assert "github_pat_11AAAAAAA000000000000000000000000000000000000000000000000000000000000000000000" not in out + assert "[REDACTED]" in out + assert "session_key: 123456" not in err + assert "[REDACTED]" in err +def test_wait_for_url_rejects_external_hosts(monkeypatch, tmp_path): + class RunningProcess: + def poll(self): + return None + service = sandboxed_web_e2e.Service("web", "serve", RunningProcess(), tmp_path / "mock.log") + + with pytest.raises(ValueError, match="Readiness URL must use localhost or loopback IP"): + sandboxed_web_e2e.wait_for_url("http://169.254.169.254/latest/meta-data/", 10, service) + + with pytest.raises(ValueError, match="Readiness URL must use localhost or loopback IP"): + sandboxed_web_e2e.wait_for_url("https://example.com", 10, service)