diff --git a/src/github_repo_auditor/serve/routes.py b/src/github_repo_auditor/serve/routes.py index 58e6df6..c016b1c 100644 --- a/src/github_repo_auditor/serve/routes.py +++ b/src/github_repo_auditor/serve/routes.py @@ -407,6 +407,7 @@ def _render_action_row(packet_id: str, idx: int, action: dict[str, Any]) -> str: target = _escape(action.get("target") or "—") rationale = _escape(action.get("rationale") or "—") safe_packet_id = _escape(packet_id) + safe_idx = _escape(idx) if state == "approved": state_cell = '✓ Approved' @@ -422,16 +423,16 @@ def _render_action_row(packet_id: str, idx: int, action: dict[str, Any]) -> str: state_cell = 'Pending' buttons = ( f' ' f'' ) row_class = "campaign-plan__row--pending" return ( - f'' + f'' f"{repo}" f'{action_type}' f"{target}" @@ -469,8 +470,6 @@ async def approve_campaign_action(request: Request, packet_id: str, idx: int) -> except Exception: action_dict = {"state": "approved"} - # Dynamic values are escaped in _render_action_row before fragment emission. - # codeql[py/reflective-xss] return HTMLResponse(_render_action_row(packet_id, idx, action_dict)) @@ -505,8 +504,6 @@ async def reject_campaign_action( except Exception: action_dict = {"state": "rejected"} - # Dynamic values are escaped in _render_action_row before fragment emission. - # codeql[py/reflective-xss] return HTMLResponse(_render_action_row(packet_id, idx, action_dict)) diff --git a/src/github_repo_auditor/serve/runner.py b/src/github_repo_auditor/serve/runner.py index d65f7e7..b2b16cb 100644 --- a/src/github_repo_auditor/serve/runner.py +++ b/src/github_repo_auditor/serve/runner.py @@ -2,6 +2,7 @@ from __future__ import annotations +import json import re import subprocess import sys @@ -43,9 +44,13 @@ class RunSession: """Holds state for one spawned audit subprocess.""" - def __init__(self, run_id: str, cmd: list[str]) -> None: + def __init__(self, run_id: str, username: str, flag_args: list[str]) -> None: self.run_id = run_id - self.cmd = cmd + # Keep the OS command line constant. Form-derived values are supplied + # to the worker over stdin and become parser arguments only inside the + # child process; they never participate in process creation. + self.cmd = (sys.executable, "-m", "github_repo_auditor.serve.worker") + self._request = {"username": username, "flag_args": flag_args} self._lines: deque[str] = deque(maxlen=_MAX_LINES) self._lock = threading.Lock() self._done = threading.Event() @@ -68,15 +73,16 @@ def _stream(self) -> None: def start(self) -> None: self._proc = subprocess.Popen( - # Command shape is fixed in spawn_run: sys.executable, module name, - # validated GitHub owner, and allowlisted flags; shell remains off. - # codeql[py/command-line-injection] self.cmd, + stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True, shell=False, # never shell=True ) + assert self._proc.stdin is not None + self._proc.stdin.write(json.dumps(self._request)) + self._proc.stdin.close() t = threading.Thread(target=self._stream, daemon=True) t.start() @@ -145,8 +151,7 @@ def spawn_run(username: str, flags: dict[str, str | bool], output_dir: Path) -> safe_username = validate_username(username) flag_args = validate_flags(flags) run_id = uuid.uuid4().hex - cmd = [sys.executable, "-m", "github_repo_auditor.cli", safe_username, *flag_args] - session = RunSession(run_id=run_id, cmd=cmd) + session = RunSession(run_id=run_id, username=safe_username, flag_args=flag_args) with _registry_lock: _registry[run_id] = session session.start() diff --git a/src/github_repo_auditor/serve/worker.py b/src/github_repo_auditor/serve/worker.py new file mode 100644 index 0000000..9fff44c --- /dev/null +++ b/src/github_repo_auditor/serve/worker.py @@ -0,0 +1,37 @@ +"""Fixed-command worker for server-triggered local audits.""" + +from __future__ import annotations + +import json +import sys +from typing import Any + +from github_repo_auditor.serve.runner import validate_username + + +def _load_request() -> tuple[str, list[str]]: + payload: Any = json.load(sys.stdin) + if not isinstance(payload, dict): + raise ValueError("run request must be an object") + + username = payload.get("username") + flag_args = payload.get("flag_args") + if not isinstance(username, str) or not isinstance(flag_args, list): + raise ValueError("run request has an invalid shape") + if not all(isinstance(value, str) for value in flag_args): + raise ValueError("run request flags must be strings") + return validate_username(username), flag_args + + +def main() -> None: + username, flag_args = _load_request() + # The child invokes the Python entry point directly. The values are parser + # arguments here, not an OS command line supplied to subprocess.Popen. + sys.argv = ["audit", username, *flag_args] + from github_repo_auditor.cli import main as cli_main + + cli_main() + + +if __name__ == "__main__": + main() diff --git a/tests/test_serve.py b/tests/test_serve.py index 1ea24c7..097041c 100644 --- a/tests/test_serve.py +++ b/tests/test_serve.py @@ -4,6 +4,7 @@ import json import sqlite3 +import sys import time from datetime import datetime, timezone from pathlib import Path @@ -536,6 +537,60 @@ def test_stream_happy_path(self, client: TestClient, output_dir: Path) -> None: assert "text/event-stream" in resp.headers["content-type"] +class TestRunnerCommandBoundary: + def test_form_values_are_not_embedded_in_worker_command(self, output_dir: Path) -> None: + from github_repo_auditor.serve import runner as runner_mod + from github_repo_auditor.serve.runner import spawn_run + + with patch.object(runner_mod.RunSession, "start"): + run_id = spawn_run( + username="octo-org", + flags={"output-dir": "user-controlled-output"}, + output_dir=output_dir, + ) + + session = runner_mod.get_session(run_id) + assert session is not None + assert session.cmd == ( + sys.executable, + "-m", + "github_repo_auditor.serve.worker", + ) + assert "octo-org" not in session.cmd + assert "user-controlled-output" not in session.cmd + + def test_worker_passes_payload_to_cli_inside_child_process(self, monkeypatch) -> None: + from io import StringIO + + import github_repo_auditor.cli as cli + from github_repo_auditor.serve import worker + + monkeypatch.setattr( + worker.sys, + "stdin", + StringIO( + json.dumps( + { + "username": "octo-org", + "flag_args": ["--portfolio-truth", "--output-dir", "safe-output"], + } + ) + ), + ) + captured: dict[str, list[str]] = {} + monkeypatch.setattr(cli, "main", lambda: captured.setdefault("argv", list(sys.argv))) + + worker.main() + + assert captured["argv"] == [ + "audit", + "octo-org", + "--portfolio-truth", + "--output-dir", + "safe-output", + ] + + # --------------------------------------------------------------------------- # Runner unit tests # ---------------------------------------------------------------------------