From 6d102a213f5e37fe0af61cdd8af3581732189e14 Mon Sep 17 00:00:00 2001 From: Alexandre Teixeira <111787685+alteixeira20@users.noreply.github.com> Date: Wed, 28 Oct 2026 21:14:43 +0000 Subject: [PATCH] fix(security): confine teardown paths and avoid interpolating exception data in logs - routes/gallery/gallery_routes.py: use stable diagnostic log message without interpolating raw exception, resolver details, tokens, or destination strings. - tests/e2e/teardown.js: remove environment-variable path overrides from production code, strictly confine test options and temp directories, and prevent arbitrary directory cleanup. - Add regression tests covering non-leakage of synthetic tokens/queries/exceptions into logs and responses, and path confinement during teardown. --- routes/gallery/gallery_routes.py | 7 +- tests/e2e/teardown.js | 57 ++++++- tests/test_gallery_endpoint_enforcement.py | 25 ++- tests/test_playwright_teardown_security.py | 177 +++++++++++++-------- 4 files changed, 184 insertions(+), 82 deletions(-) diff --git a/routes/gallery/gallery_routes.py b/routes/gallery/gallery_routes.py index 0825a0815..690c22044 100644 --- a/routes/gallery/gallery_routes.py +++ b/routes/gallery/gallery_routes.py @@ -2195,10 +2195,9 @@ def setup_gallery_routes() -> APIRouter: ok, reason = check_outbound_url(base, block_private=block_private) if not ok: # 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) + # exception details to clients or logs. Use a stable diagnostic message + # without interpolating untrusted exception content or destination strings. + logger.warning("edit-progress endpoint rejected: outbound URL policy check failed") return {"available": False, "reason": "endpoint URL rejected"} if not base.endswith("/v1"): diff --git a/tests/e2e/teardown.js b/tests/e2e/teardown.js index baa8fc382..b3bad62e0 100644 --- a/tests/e2e/teardown.js +++ b/tests/e2e/teardown.js @@ -1,20 +1,60 @@ const fs = require('node:fs'); +const os = require('node:os'); const path = require('node:path'); const { execFileSync } = require('node:child_process'); -module.exports = async () => { +const repoRoot = path.resolve(__dirname, '..', '..'); +const defaultResultsDir = path.join(repoRoot, 'test-results'); +const defaultReportDir = path.join(repoRoot, 'playwright-report'); +const defaultSanitizerScript = path.join(repoRoot, 'scripts', 'sanitize_trace_artifacts.py'); + +/** + * Confine dynamic paths to explicitly allowed test directories. + */ +function validateTestPath(targetPath, allowedBases, label) { + if (!targetPath || typeof targetPath !== 'string') return null; + const resolved = path.resolve(targetPath); + const isAllowed = allowedBases.some((base) => { + const resolvedBase = path.resolve(base); + return resolved.startsWith(resolvedBase + path.sep) || resolved === resolvedBase; + }); + if (!isAllowed) { + throw new Error(`Security rejection: ${label} path "${targetPath}" is outside allowed test directories`); + } + return resolved; +} + +module.exports = async (options = {}) => { + // If options is a Playwright FullConfig object, ignore it and use safe defaults + const isPlaywrightConfig = options && (options.configFile || options.rootDir); + const testOpts = (!isPlaywrightConfig && typeof options === 'object') ? options : {}; + + // Confine cleanup of PHOTO_EDITOR_E2E_DATA_DIR: + // Strictly require it to be within an OS temporary directory and match the expected test prefix const dataDirectory = process.env.PHOTO_EDITOR_E2E_DATA_DIR; - if (dataDirectory) { + if (dataDirectory && typeof dataDirectory === 'string') { try { - fs.rmSync(dataDirectory, { recursive: true, force: true }); + const resolvedData = path.resolve(dataDirectory); + const allowedTmpBases = [os.tmpdir(), '/var/tmp', '/tmp'].map((b) => path.resolve(b)); + const isTmp = allowedTmpBases.some((base) => resolvedData.startsWith(base + path.sep)); + const baseName = path.basename(resolvedData); + if (isTmp && baseName.startsWith('odysseus-photo-editor-e2e-')) { + fs.rmSync(resolvedData, { recursive: true, force: true }); + } } catch (_) {} } // 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 = process.env.ODYSSEUS_TEST_RESULTS_DIR || path.join(__dirname, '..', '..', 'test-results'); - const reportDir = process.env.ODYSSEUS_TEST_REPORT_DIR || path.join(__dirname, '..', '..', 'playwright-report'); + const allowedBases = [defaultResultsDir, defaultReportDir, os.tmpdir(), '/var/tmp', '/tmp']; + const resultsDir = testOpts.resultsDir + ? validateTestPath(testOpts.resultsDir, allowedBases, 'resultsDir') + : defaultResultsDir; + const reportDir = testOpts.reportDir + ? validateTestPath(testOpts.reportDir, allowedBases, 'reportDir') + : defaultReportDir; + if (fs.existsSync(reportDir)) { try { fs.rmSync(reportDir, { recursive: true, force: true }); @@ -65,8 +105,11 @@ module.exports = async () => { purgeSensitiveArtifacts(resultsDir); } else { const python = process.env.ODYSSEUS_TEST_PYTHON || 'python'; - 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; + const sanitizerScript = testOpts.sanitizerScript + ? validateTestPath(testOpts.sanitizerScript, [path.join(repoRoot, 'scripts'), os.tmpdir(), '/var/tmp', '/tmp'], 'sanitizerScript') + : defaultSanitizerScript; + const rawTimeout = Number(testOpts.timeout) || 30_000; + const timeout = Math.max(100, Math.min(rawTimeout, 120_000)); let sanitizerSucceeded = false; try { execFileSync(python, [sanitizerScript, resultsDir], { diff --git a/tests/test_gallery_endpoint_enforcement.py b/tests/test_gallery_endpoint_enforcement.py index 5d1873100..099ec8a66 100644 --- a/tests/test_gallery_endpoint_enforcement.py +++ b/tests/test_gallery_endpoint_enforcement.py @@ -230,17 +230,19 @@ def test_edit_progress_blocks_private_ip_when_policy_enabled(mock_provider_liste 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.""" + """CodeQL #1203: Outbound URL rejection must never leak exception strings, tokens or internal destinations into responses or logs.""" db = SessionLocal() ep_id = "ep-test-sensitive-exception" synthetic_secret = "synthetic-resolver-secret-marker-42" + synthetic_token = "synthetic-token-xyz-999" + synthetic_query_secret = "synthetic-query-key-777" 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", + base_url=f"http://user:{synthetic_secret}@{internal_host}:8000?api_key={synthetic_query_secret}", model_type="image", is_enabled=True, owner="alice", @@ -261,7 +263,7 @@ def test_edit_progress_rejection_does_not_expose_exception_details_or_secrets(mo synthetic_rejection_reason = ( f"host does not resolve: [Errno -2] Name or service not known: " - f"http://user:{synthetic_secret}@{internal_host} -> {internal_ip}" + f"http://custom_user:custom_pass@{internal_host} -> {internal_ip} with token={synthetic_token}" ) with patch("routes.gallery.gallery_routes.require_privilege", return_value="alice"): @@ -277,18 +279,27 @@ def test_edit_progress_rejection_does_not_expose_exception_details_or_secrets(mo # 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 synthetic_token not in resp_str + assert synthetic_query_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 + assert "custom_pass" 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 + # 4. Server-side log uses a stable message without interpolating untrusted exception or destination strings + log_text = " ".join(rec.message for rec in caplog.records) + assert "edit-progress endpoint rejected: outbound URL policy check failed" in log_text + assert synthetic_secret not in log_text + assert synthetic_token not in log_text + assert synthetic_query_secret not in log_text + assert internal_host not in log_text + assert internal_ip not in log_text + assert "Errno" not in log_text + assert "custom_pass" not in log_text finally: db.query(ModelEndpoint).filter(ModelEndpoint.id == ep_id).delete() db.commit() diff --git a/tests/test_playwright_teardown_security.py b/tests/test_playwright_teardown_security.py index ec88d38d8..35623bbb2 100644 --- a/tests/test_playwright_teardown_security.py +++ b/tests/test_playwright_teardown_security.py @@ -1,12 +1,14 @@ -"""Regression tests for Playwright global teardown security hardening (CodeQL #1202). +"""Regression tests for Playwright global teardown security hardening (CodeQL #1202 & Filesystem Safety). Validates that: -1. Environment variables and paths passed to teardown cannot trigger shell command execution. +1. Shell command execution is prevented (execFileSync with shell: false and separate argument arrays). 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. +7. Filesystem confinement: teardown ignores arbitrary directory environment variables and confines + cleanup to strictly validated test-owned locations. """ import json @@ -23,13 +25,16 @@ 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.""" +def _run_teardown(env: dict = None, options: dict = None) -> subprocess.CompletedProcess: + """Run teardown.js in an isolated Node process with custom environment and options.""" full_env = os.environ.copy() - full_env.update(env) + if env: + full_env.update(env) + opts_json = json.dumps(options or {}) script = ( "const teardown = require('./tests/e2e/teardown.js');\n" - "teardown().then(() => process.exit(0)).catch((err) => {\n" + f"const opts = {opts_json};\n" + "teardown(opts).then(() => process.exit(0)).catch((err) => {\n" " console.error(err);\n" " process.exit(1);\n" "});\n" @@ -52,12 +57,14 @@ def test_teardown_unconditional_ci_suppression(tmp_path): (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), - }) + res = _run_teardown( + env={ + "CI": "true", + "GITHUB_ACTIONS": "true", + "PLAYWRIGHT_ENABLE_TRACE": "1", # Even with opt-in, CI must purge unconditionally + }, + options={"resultsDir": str(results_dir)}, + ) assert res.returncode == 0 # All sensitive artifacts must be purged assert not (results_dir / "trace.zip").exists() @@ -75,12 +82,14 @@ def test_teardown_local_default_purges_without_opt_in(tmp_path): (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), - }) + res = _run_teardown( + env={ + "CI": "", + "GITHUB_ACTIONS": "", + "PLAYWRIGHT_ENABLE_TRACE": "0", # No opt-in + }, + options={"resultsDir": str(results_dir)}, + ) assert res.returncode == 0 assert not (results_dir / "trace.zip").exists() assert not (results_dir / "session.har").exists() @@ -97,13 +106,15 @@ def test_teardown_sanitizer_success_with_opt_in(tmp_path): 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), - }) + res = _run_teardown( + env={ + "CI": "", + "GITHUB_ACTIONS": "", + "PLAYWRIGHT_ENABLE_TRACE": "1", + "ODYSSEUS_TEST_PYTHON": sys.executable, + }, + options={"resultsDir": str(results_dir)}, + ) assert res.returncode == 0 # Archive is retained because sanitizer succeeded assert trace_file.exists() @@ -121,13 +132,15 @@ def test_teardown_missing_python_executable_fails_closed(tmp_path): 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), - }) + res = _run_teardown( + env={ + "CI": "", + "GITHUB_ACTIONS": "", + "PLAYWRIGHT_ENABLE_TRACE": "1", + "ODYSSEUS_TEST_PYTHON": "/usr/bin/nonexistent_python_binary_xyz_404", + }, + options={"resultsDir": str(results_dir)}, + ) assert res.returncode == 0 # Fail closed: archives must be purged assert not trace_file.exists() @@ -146,14 +159,18 @@ def test_teardown_nonzero_sanitizer_exit_fails_closed(tmp_path): 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), - }) + res = _run_teardown( + env={ + "CI": "", + "GITHUB_ACTIONS": "", + "PLAYWRIGHT_ENABLE_TRACE": "1", + "ODYSSEUS_TEST_PYTHON": sys.executable, + }, + options={ + "resultsDir": str(results_dir), + "sanitizerScript": str(failing_script), + }, + ) assert res.returncode == 0 assert not trace_file.exists() assert not tmp_file.exists() @@ -171,15 +188,19 @@ def test_teardown_sanitizer_timeout_fails_closed(tmp_path): 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), - }) + res = _run_teardown( + env={ + "CI": "", + "GITHUB_ACTIONS": "", + "PLAYWRIGHT_ENABLE_TRACE": "1", + "ODYSSEUS_TEST_PYTHON": sys.executable, + }, + options={ + "resultsDir": str(results_dir), + "sanitizerScript": str(hanging_script), + "timeout": 300, # 300ms timeout + }, + ) assert res.returncode == 0 # Fail closed: timeout killed the process and purged archives assert not trace_file.exists() @@ -205,14 +226,18 @@ def test_teardown_handles_paths_containing_spaces(tmp_path): "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), - }) + res = _run_teardown( + env={ + "CI": "", + "GITHUB_ACTIONS": "", + "PLAYWRIGHT_ENABLE_TRACE": "1", + "ODYSSEUS_TEST_PYTHON": sys.executable, + }, + options={ + "resultsDir": str(spaced_root), + "sanitizerScript": str(spaced_script), + }, + ) assert res.returncode == 0 assert (spaced_root / "executed.marker").exists() @@ -242,15 +267,39 @@ def test_teardown_synthetic_command_substitution_and_metacharacters(tmp_path, pa 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), - }) + res = _run_teardown( + env={ + "CI": "", + "GITHUB_ACTIONS": "", + "PLAYWRIGHT_ENABLE_TRACE": "1", + "ODYSSEUS_TEST_PYTHON": formatted_payload, + }, + options={"resultsDir": 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() + + +def test_teardown_rejects_unconfined_data_directory_environment_variable(tmp_path): + """Untrusted PHOTO_EDITOR_E2E_DATA_DIR pointing outside OS temp or lacking expected prefix is ignored.""" + protected_file = tmp_path / "important_data.txt" + protected_file.write_text("do not delete") + + # Target outside tmp or missing odysseus-photo-editor-e2e- prefix + res = _run_teardown(env={ + "PHOTO_EDITOR_E2E_DATA_DIR": str(tmp_path), + }) + assert res.returncode == 0 + # Must NOT have been deleted + assert protected_file.exists(), "Security failure: unconfined data directory was deleted!" + + +def test_teardown_rejects_unconfined_programmatic_paths(): + """Programmatic options pointing outside allowed test roots are rejected fail-closed.""" + # Attempting to target /etc or home root + res = _run_teardown(options={"resultsDir": "/etc"}) + assert res.returncode == 1 + assert "Security rejection" in res.stderr