diff --git a/routes/gallery/gallery_routes.py b/routes/gallery/gallery_routes.py index 0aed9c1a9..0825a0815 100644 --- a/routes/gallery/gallery_routes.py +++ b/routes/gallery/gallery_routes.py @@ -2194,7 +2194,12 @@ def setup_gallery_routes() -> APIRouter: block_private = os.getenv("IMAGE_BLOCK_PRIVATE_IPS", "false").lower() == "true" ok, reason = check_outbound_url(base, block_private=block_private) if not ok: - return {"available": False, "reason": f"endpoint URL rejected: {reason}"} + # CodeQL #1203: Never expose internal addresses, resolver errors or raw + # exception details to clients. Log useful diagnostic server-side without + # printing credentials or raw destination URLs, and return a stable generic response. + safe_reason = re.sub(r"://([^:@\s]+):([^@\s]+)@", r"://\1:[REDACTED]@", str(reason)) + logger.warning("edit-progress endpoint rejected: %s", safe_reason) + return {"available": False, "reason": "endpoint URL rejected"} if not base.endswith("/v1"): base += "/v1" diff --git a/tests/e2e/teardown.js b/tests/e2e/teardown.js index 4ee3dfa67..baa8fc382 100644 --- a/tests/e2e/teardown.js +++ b/tests/e2e/teardown.js @@ -1,6 +1,6 @@ const fs = require('node:fs'); const path = require('node:path'); -const { execSync } = require('node:child_process'); +const { execFileSync } = require('node:child_process'); module.exports = async () => { const dataDirectory = process.env.PHOTO_EDITOR_E2E_DATA_DIR; @@ -13,8 +13,8 @@ module.exports = async () => { // Secure default tracing: never publish trace archives in CI or without explicit opt-in. // When traces are not opted-in, purge all archives unconditionally. // When local opt-in tracing is active, run the strict fail-closed structured sanitizer. - const resultsDir = path.join(__dirname, '..', '..', 'test-results'); - const reportDir = path.join(__dirname, '..', '..', 'playwright-report'); + const resultsDir = process.env.ODYSSEUS_TEST_RESULTS_DIR || path.join(__dirname, '..', '..', 'test-results'); + const reportDir = process.env.ODYSSEUS_TEST_REPORT_DIR || path.join(__dirname, '..', '..', 'playwright-report'); if (fs.existsSync(reportDir)) { try { fs.rmSync(reportDir, { recursive: true, force: true }); @@ -65,12 +65,14 @@ module.exports = async () => { purgeSensitiveArtifacts(resultsDir); } else { const python = process.env.ODYSSEUS_TEST_PYTHON || 'python'; - const sanitizerScript = path.join(__dirname, '..', '..', 'scripts', 'sanitize_trace_artifacts.py'); + const sanitizerScript = process.env.ODYSSEUS_TEST_SANITIZER_SCRIPT || path.join(__dirname, '..', '..', 'scripts', 'sanitize_trace_artifacts.py'); + const timeout = Number(process.env.ODYSSEUS_TEST_SANITIZER_TIMEOUT_MS) || 30_000; let sanitizerSucceeded = false; try { - execSync(`${JSON.stringify(python)} ${JSON.stringify(sanitizerScript)} ${JSON.stringify(resultsDir)}`, { + execFileSync(python, [sanitizerScript, resultsDir], { stdio: 'ignore', - timeout: 30_000, + timeout, + shell: false, }); sanitizerSucceeded = true; } catch (_) { diff --git a/tests/test_gallery_endpoint_enforcement.py b/tests/test_gallery_endpoint_enforcement.py index 0ee87d723..5d1873100 100644 --- a/tests/test_gallery_endpoint_enforcement.py +++ b/tests/test_gallery_endpoint_enforcement.py @@ -220,7 +220,7 @@ def test_edit_progress_blocks_private_ip_when_policy_enabled(mock_provider_liste assert isinstance(resp, dict) assert resp["available"] is False - assert "rejected" in str(resp.get("reason")) + assert resp.get("reason") == "endpoint URL rejected" # Zero outbound polling because private IP was blocked by policy! assert len(TrackingPollHandler.requests_received) == 0 finally: @@ -229,6 +229,116 @@ def test_edit_progress_blocks_private_ip_when_policy_enabled(mock_provider_liste db.close() +def test_edit_progress_rejection_does_not_expose_exception_details_or_secrets(mock_provider_listener, caplog): + """CodeQL #1203: Outbound URL rejection must never leak exception strings or internal destinations.""" + db = SessionLocal() + ep_id = "ep-test-sensitive-exception" + synthetic_secret = "synthetic-resolver-secret-marker-42" + internal_host = "cluster-internal-worker.prod.corp" + internal_ip = "10.128.0.42" + try: + ep = ModelEndpoint( + id=ep_id, + name="Internal Worker Endpoint", + base_url=f"http://user:{synthetic_secret}@{internal_host}:8000", + model_type="image", + is_enabled=True, + owner="alice", + cached_models=json.dumps(["sd-v1"]), + ) + db.merge(ep) + db.commit() + + router = setup_gallery_routes() + route = next(r for r in router.routes if r.path == "/api/image/edit-progress") + + req = create_gallery_request( + "GET", + "/api/image/edit-progress", + query_params={"request_id": "req-safe-test", "endpoint_id": ep_id}, + user="alice", + ) + + synthetic_rejection_reason = ( + f"host does not resolve: [Errno -2] Name or service not known: " + f"http://user:{synthetic_secret}@{internal_host} -> {internal_ip}" + ) + + with patch("routes.gallery.gallery_routes.require_privilege", return_value="alice"): + with patch("src.url_safety.check_outbound_url", return_value=(False, synthetic_rejection_reason)): + TrackingPollHandler.requests_received = [] + resp = asyncio.run(route.endpoint(req)) + + # 1. Operation is rejected with a stable generic reason + assert isinstance(resp, dict) + assert resp["available"] is False + assert resp.get("reason") == "endpoint URL rejected" + + # 2. No synthetic secrets or internal host details exposed to API client + resp_str = json.dumps(resp) + assert synthetic_secret not in resp_str + assert internal_host not in resp_str + assert internal_ip not in resp_str + assert "Errno" not in resp_str + assert "resolve" not in resp_str + + # 3. No outbound HTTP request occurred + assert len(TrackingPollHandler.requests_received) == 0 + + # 4. Server-side log recorded rejection diagnostic without raw credentials + assert any("edit-progress endpoint rejected" in rec.message for rec in caplog.records) + for rec in caplog.records: + assert synthetic_secret not in rec.message + finally: + db.query(ModelEndpoint).filter(ModelEndpoint.id == ep_id).delete() + db.commit() + db.close() + + +def test_edit_progress_valid_endpoint_succeeds(mock_provider_listener, monkeypatch): + """Valid authorized endpoint allows progress polling and returns provider payload.""" + monkeypatch.setenv("IMAGE_BLOCK_PRIVATE_IPS", "false") + db = SessionLocal() + ep_id = "ep-test-valid-progress" + try: + ep = ModelEndpoint( + id=ep_id, + name="Valid SD Provider", + base_url=mock_provider_listener, + model_type="image", + is_enabled=True, + owner="alice", + cached_models=json.dumps(["sd-v1"]), + ) + db.merge(ep) + db.commit() + + router = setup_gallery_routes() + route = next(r for r in router.routes if r.path == "/api/image/edit-progress") + + req = create_gallery_request( + "GET", + "/api/image/edit-progress", + query_params={"request_id": "valid-progress-req", "endpoint_id": ep_id}, + user="alice", + ) + + with patch("routes.gallery.gallery_routes.require_privilege", return_value="alice"): + TrackingPollHandler.requests_received = [] + resp = asyncio.run(route.endpoint(req)) + + assert isinstance(resp, dict) + assert resp["available"] is True + assert resp.get("progress") == 50 + # Exactly one outbound request to the provider progress route + assert len(TrackingPollHandler.requests_received) == 1 + assert TrackingPollHandler.requests_received[0].endswith("/images/progress/valid-progress-req") + finally: + db.query(ModelEndpoint).filter(ModelEndpoint.id == ep_id).delete() + db.commit() + db.close() + + def test_harmonize_rejects_chat_only_endpoint(mock_provider_listener): """POST /api/image/harmonize rejects a chat-only endpoint that has no image capability.""" db = SessionLocal() diff --git a/tests/test_playwright_teardown_security.py b/tests/test_playwright_teardown_security.py new file mode 100644 index 000000000..ec88d38d8 --- /dev/null +++ b/tests/test_playwright_teardown_security.py @@ -0,0 +1,256 @@ +"""Regression tests for Playwright global teardown security hardening (CodeQL #1202). + +Validates that: +1. Environment variables and paths passed to teardown cannot trigger shell command execution. +2. Synthetic command substitutions and shell metacharacters fail safely without invoking a shell. +3. Paths containing spaces and non-standard characters are preserved without word-splitting. +4. Process errors (missing executable, nonzero exit, timeout) fail closed by purging artifacts. +5. Secure default tracing (unconditional CI suppression and local default purging) remains enforced. +6. Opt-in local runs execute the sanitizer and preserve sanitized archives. +""" + +import json +import os +import subprocess +import sys +import tempfile +import zipfile +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[1] +TEARDOWN_PATH = REPO_ROOT / "tests" / "e2e" / "teardown.js" + + +def _run_teardown(env: dict) -> subprocess.CompletedProcess: + """Run teardown.js in an isolated Node process with custom environment.""" + full_env = os.environ.copy() + full_env.update(env) + script = ( + "const teardown = require('./tests/e2e/teardown.js');\n" + "teardown().then(() => process.exit(0)).catch((err) => {\n" + " console.error(err);\n" + " process.exit(1);\n" + "});\n" + ) + return subprocess.run( + ["node", "-e", script], + cwd=REPO_ROOT, + env=full_env, + capture_output=True, + text=True, + ) + + +def test_teardown_unconditional_ci_suppression(tmp_path): + """In CI environments, all sensitive trace artifacts are purged unconditionally.""" + results_dir = tmp_path / "test-results" + results_dir.mkdir() + (results_dir / "trace.zip").write_bytes(b"PK0304 sensitive trace archive") + (results_dir / "session.har").write_text('{"log": {"entries": []}}') + (results_dir / "failure.png").write_bytes(b"\x89PNG sensitive screenshot") + (results_dir / "temp.tmp").write_text("in-progress trace data") + + res = _run_teardown({ + "CI": "true", + "GITHUB_ACTIONS": "true", + "PLAYWRIGHT_ENABLE_TRACE": "1", # Even with opt-in, CI must purge unconditionally + "ODYSSEUS_TEST_RESULTS_DIR": str(results_dir), + }) + assert res.returncode == 0 + # All sensitive artifacts must be purged + assert not (results_dir / "trace.zip").exists() + assert not (results_dir / "session.har").exists() + assert not (results_dir / "failure.png").exists() + assert not (results_dir / "temp.tmp").exists() + + +def test_teardown_local_default_purges_without_opt_in(tmp_path): + """Local runs without explicit PLAYWRIGHT_ENABLE_TRACE=1 purge artifacts.""" + results_dir = tmp_path / "test-results" + results_dir.mkdir() + (results_dir / "trace.zip").write_bytes(b"PK0304 trace") + (results_dir / "session.har").write_text('{"log": {}}') + (results_dir / "failure.png").write_bytes(b"\x89PNG") + (results_dir / "temp.tmp").write_text("temp") + + res = _run_teardown({ + "CI": "", + "GITHUB_ACTIONS": "", + "PLAYWRIGHT_ENABLE_TRACE": "0", # No opt-in + "ODYSSEUS_TEST_RESULTS_DIR": str(results_dir), + }) + assert res.returncode == 0 + assert not (results_dir / "trace.zip").exists() + assert not (results_dir / "session.har").exists() + assert not (results_dir / "failure.png").exists() + assert not (results_dir / "temp.tmp").exists() + + +def test_teardown_sanitizer_success_with_opt_in(tmp_path): + """Opt-in local runs execute the Python sanitizer and retain sanitized archives.""" + results_dir = tmp_path / "test-results" + results_dir.mkdir() + trace_file = results_dir / "trace.zip" + with zipfile.ZipFile(trace_file, "w") as z: + action = {"type": "action", "name": "fill", "params": {"value": "secret_password"}} + z.writestr("0-trace.trace", json.dumps(action).encode() + b"\n") + + res = _run_teardown({ + "CI": "", + "GITHUB_ACTIONS": "", + "PLAYWRIGHT_ENABLE_TRACE": "1", + "ODYSSEUS_TEST_PYTHON": sys.executable, + "ODYSSEUS_TEST_RESULTS_DIR": str(results_dir), + }) + assert res.returncode == 0 + # Archive is retained because sanitizer succeeded + assert trace_file.exists() + with zipfile.ZipFile(trace_file, "r") as z: + content = json.loads(z.read("0-trace.trace").decode()) + assert content["params"]["value"] == "[REDACTED]" + + +def test_teardown_missing_python_executable_fails_closed(tmp_path): + """If the python executable is missing, teardown fails closed and purges archives.""" + results_dir = tmp_path / "test-results" + results_dir.mkdir() + trace_file = results_dir / "trace.zip" + trace_file.write_bytes(b"PK0304 trace") + tmp_file = results_dir / "trace.tmp" + tmp_file.write_text("trace tmp") + + res = _run_teardown({ + "CI": "", + "GITHUB_ACTIONS": "", + "PLAYWRIGHT_ENABLE_TRACE": "1", + "ODYSSEUS_TEST_PYTHON": "/usr/bin/nonexistent_python_binary_xyz_404", + "ODYSSEUS_TEST_RESULTS_DIR": str(results_dir), + }) + assert res.returncode == 0 + # Fail closed: archives must be purged + assert not trace_file.exists() + assert not tmp_file.exists() + + +def test_teardown_nonzero_sanitizer_exit_fails_closed(tmp_path): + """If the sanitizer exits nonzero, teardown fails closed and purges archives.""" + failing_script = tmp_path / "fail_sanitizer.py" + failing_script.write_text("import sys; sys.exit(2)\n") + + results_dir = tmp_path / "test-results" + results_dir.mkdir() + trace_file = results_dir / "trace.zip" + trace_file.write_bytes(b"PK0304 trace") + tmp_file = results_dir / "trace.tmp" + tmp_file.write_text("trace tmp") + + res = _run_teardown({ + "CI": "", + "GITHUB_ACTIONS": "", + "PLAYWRIGHT_ENABLE_TRACE": "1", + "ODYSSEUS_TEST_PYTHON": sys.executable, + "ODYSSEUS_TEST_SANITIZER_SCRIPT": str(failing_script), + "ODYSSEUS_TEST_RESULTS_DIR": str(results_dir), + }) + assert res.returncode == 0 + assert not trace_file.exists() + assert not tmp_file.exists() + + +def test_teardown_sanitizer_timeout_fails_closed(tmp_path): + """If the sanitizer hangs or exceeds timeout, teardown fails closed and purges archives.""" + hanging_script = tmp_path / "hang_sanitizer.py" + hanging_script.write_text("import time; time.sleep(5)\n") + + results_dir = tmp_path / "test-results" + results_dir.mkdir() + trace_file = results_dir / "trace.zip" + trace_file.write_bytes(b"PK0304 trace") + tmp_file = results_dir / "trace.tmp" + tmp_file.write_text("trace tmp") + + res = _run_teardown({ + "CI": "", + "GITHUB_ACTIONS": "", + "PLAYWRIGHT_ENABLE_TRACE": "1", + "ODYSSEUS_TEST_PYTHON": sys.executable, + "ODYSSEUS_TEST_SANITIZER_SCRIPT": str(hanging_script), + "ODYSSEUS_TEST_SANITIZER_TIMEOUT_MS": "300", # 300ms timeout + "ODYSSEUS_TEST_RESULTS_DIR": str(results_dir), + }) + assert res.returncode == 0 + # Fail closed: timeout killed the process and purged archives + assert not trace_file.exists() + assert not tmp_file.exists() + + +def test_teardown_handles_paths_containing_spaces(tmp_path): + """Paths with spaces in results directory and sanitizer script execute cleanly.""" + spaced_root = tmp_path / "space dir test" / "sub dir results" + spaced_root.mkdir(parents=True) + trace_file = spaced_root / "trace.zip" + with zipfile.ZipFile(trace_file, "w") as z: + action = {"type": "action", "params": {"value": "secret123"}} + z.writestr("0.trace", json.dumps(action).encode() + b"\n") + + spaced_script = tmp_path / "sanitizer tools" / "run sanitizer.py" + spaced_script.parent.mkdir(parents=True) + spaced_script.write_text( + "import sys, json, zipfile, os\n" + "target = sys.argv[1]\n" + "assert os.path.exists(target)\n" + "marker = os.path.join(target, 'executed.marker')\n" + "with open(marker, 'w') as f: f.write('ok')\n" + ) + + res = _run_teardown({ + "CI": "", + "GITHUB_ACTIONS": "", + "PLAYWRIGHT_ENABLE_TRACE": "1", + "ODYSSEUS_TEST_PYTHON": sys.executable, + "ODYSSEUS_TEST_SANITIZER_SCRIPT": str(spaced_script), + "ODYSSEUS_TEST_RESULTS_DIR": str(spaced_root), + }) + assert res.returncode == 0 + assert (spaced_root / "executed.marker").exists() + + +@pytest.mark.parametrize("payload", [ + '$(touch "{marker}")', + '`touch "{marker}"`', + '; touch "{marker}"', + '&& touch "{marker}"', + '|| touch "{marker}"', + '| touch "{marker}"', + '& touch "{marker}"', + '> "{marker}"', +]) +def test_teardown_synthetic_command_substitution_and_metacharacters(tmp_path, payload): + """Untrusted environment strings in ODYSSEUS_TEST_PYTHON cannot execute shell commands. + + With execFileSync and shell: false, any shell metacharacters and command substitutions + are treated strictly as literal filename characters for execve, resulting in ENOENT + without invoking any subshell or creating the marker file. + """ + marker = tmp_path / "shell_pwned_marker.txt" + results_dir = tmp_path / "results" + results_dir.mkdir() + trace_file = results_dir / "trace.zip" + trace_file.write_bytes(b"PK0304 trace") + + formatted_payload = f"python {payload.format(marker=str(marker))}" + + res = _run_teardown({ + "CI": "", + "GITHUB_ACTIONS": "", + "PLAYWRIGHT_ENABLE_TRACE": "1", + "ODYSSEUS_TEST_PYTHON": formatted_payload, + "ODYSSEUS_TEST_RESULTS_DIR": str(results_dir), + }) + assert res.returncode == 0 + # The shell marker MUST NOT have been created! + assert not marker.exists(), f"Security failure: shell command executed for payload {payload}!" + # Fail closed: trace must be purged because python execution failed + assert not trace_file.exists()