jpmschweitzer/odysseus
1
0
Fork 0
mirror of https://github.com/pewdiepie-archdaemon/odysseus.git synced 2026-11-05 20:52:22 +01:00
Code Issues Packages Projects Releases Wiki Activity

fix(security): confine teardown paths and avoid interpolating exception data in logs

Browse Source 
- 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.
This commit is contained in:
Alexandre Teixeira
2026-10-28 21:14:43 +00:00
parent de8f151632
commit 6d102a213f
4 changed files with 184 additions and 82 deletions
Download Patch File Download Diff File Expand all files Collapse all files
routes/gallery/gallery_routes.py
+3 -4
View File
@@ -2195,10 +2195,9 @@ def setup_gallery_routes() -> APIRouter:
ok, reason = check_outbound_url(base, block_private=block_private) ok, reason = check_outbound_url(base, block_private=block_private)
if not ok: if not ok:
# CodeQL #1203: Never expose internal addresses, resolver errors or raw # CodeQL #1203: Never expose internal addresses, resolver errors or raw
# exception details to clients. Log useful diagnostic server-side without # exception details to clients or logs. Use a stable diagnostic message
# printing credentials or raw destination URLs, and return a stable generic response. # without interpolating untrusted exception content or destination strings.
safe_reason = re.sub(r"://([^:@\s]+):([^@\s]+)@", r"://\1:[REDACTED]@", str(reason)) logger.warning("edit-progress endpoint rejected: outbound URL policy check failed")
logger.warning("edit-progress endpoint rejected: %s", safe_reason)
return {"available": False, "reason": "endpoint URL rejected"} return {"available": False, "reason": "endpoint URL rejected"}
if not base.endswith("/v1"): if not base.endswith("/v1"):
tests/e2e/teardown.js
+50 -7
View File
@@ -1,20 +1,60 @@
const fs = require('node:fs'); const fs = require('node:fs');
const os = require('node:os');
const path = require('node:path'); const path = require('node:path');
const { execFileSync } = require('node:child_process'); 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; const dataDirectory = process.env.PHOTO_EDITOR_E2E_DATA_DIR;
if (dataDirectory) { if (dataDirectory && typeof dataDirectory === 'string') {
try { 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 (_) {} } catch (_) {}
} }
// Secure default tracing: never publish trace archives in CI or without explicit opt-in. // 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 traces are not opted-in, purge all archives unconditionally.
// When local opt-in tracing is active, run the strict fail-closed structured sanitizer. // 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 allowedBases = [defaultResultsDir, defaultReportDir, os.tmpdir(), '/var/tmp', '/tmp'];
const reportDir = process.env.ODYSSEUS_TEST_REPORT_DIR || path.join(__dirname, '..', '..', 'playwright-report'); const resultsDir = testOpts.resultsDir
? validateTestPath(testOpts.resultsDir, allowedBases, 'resultsDir')
: defaultResultsDir;
const reportDir = testOpts.reportDir
? validateTestPath(testOpts.reportDir, allowedBases, 'reportDir')
: defaultReportDir;
if (fs.existsSync(reportDir)) { if (fs.existsSync(reportDir)) {
try { try {
fs.rmSync(reportDir, { recursive: true, force: true }); fs.rmSync(reportDir, { recursive: true, force: true });
@@ -65,8 +105,11 @@ module.exports = async () => {
purgeSensitiveArtifacts(resultsDir); purgeSensitiveArtifacts(resultsDir);
} else { } else {
const python = process.env.ODYSSEUS_TEST_PYTHON || 'python'; 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 sanitizerScript = testOpts.sanitizerScript
const timeout = Number(process.env.ODYSSEUS_TEST_SANITIZER_TIMEOUT_MS) || 30_000; ? 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; let sanitizerSucceeded = false;
try { try {
execFileSync(python, [sanitizerScript, resultsDir], { execFileSync(python, [sanitizerScript, resultsDir], {
tests/test_gallery_endpoint_enforcement.py
+18 -7
View File
@@ -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): 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() db = SessionLocal()
ep_id = "ep-test-sensitive-exception" ep_id = "ep-test-sensitive-exception"
synthetic_secret = "synthetic-resolver-secret-marker-42" 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_host = "cluster-internal-worker.prod.corp"
internal_ip = "10.128.0.42" internal_ip = "10.128.0.42"
try: try:
ep = ModelEndpoint( ep = ModelEndpoint(
id=ep_id, id=ep_id,
name="Internal Worker Endpoint", 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", model_type="image",
is_enabled=True, is_enabled=True,
owner="alice", owner="alice",
@@ -261,7 +263,7 @@ def test_edit_progress_rejection_does_not_expose_exception_details_or_secrets(mo
synthetic_rejection_reason = ( synthetic_rejection_reason = (
f"host does not resolve: [Errno -2] Name or service not known: " 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"): 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 # 2. No synthetic secrets or internal host details exposed to API client
resp_str = json.dumps(resp) resp_str = json.dumps(resp)
assert synthetic_secret not in resp_str 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_host not in resp_str
assert internal_ip not in resp_str assert internal_ip not in resp_str
assert "Errno" not in resp_str assert "Errno" not in resp_str
assert "resolve" not in resp_str assert "resolve" not in resp_str
assert "custom_pass" not in resp_str
# 3. No outbound HTTP request occurred # 3. No outbound HTTP request occurred
assert len(TrackingPollHandler.requests_received) == 0 assert len(TrackingPollHandler.requests_received) == 0
# 4. Server-side log recorded rejection diagnostic without raw credentials # 4. Server-side log uses a stable message without interpolating untrusted exception or destination strings
assert any("edit-progress endpoint rejected" in rec.message for rec in caplog.records) log_text = " ".join(rec.message for rec in caplog.records)
for rec in caplog.records: assert "edit-progress endpoint rejected: outbound URL policy check failed" in log_text
assert synthetic_secret not in rec.message 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: finally:
db.query(ModelEndpoint).filter(ModelEndpoint.id == ep_id).delete() db.query(ModelEndpoint).filter(ModelEndpoint.id == ep_id).delete()
db.commit() db.commit()
tests/test_playwright_teardown_security.py
+113 -64
View File
@@ -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: 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. 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. 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. 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. 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. 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 import json
@@ -23,13 +25,16 @@ REPO_ROOT = Path(__file__).resolve().parents[1]
TEARDOWN_PATH = REPO_ROOT / "tests" / "e2e" / "teardown.js" TEARDOWN_PATH = REPO_ROOT / "tests" / "e2e" / "teardown.js"
def _run_teardown(env: dict) -> subprocess.CompletedProcess: def _run_teardown(env: dict = None, options: dict = None) -> subprocess.CompletedProcess:
"""Run teardown.js in an isolated Node process with custom environment.""" """Run teardown.js in an isolated Node process with custom environment and options."""
full_env = os.environ.copy() full_env = os.environ.copy()
full_env.update(env) if env:
full_env.update(env)
opts_json = json.dumps(options or {})
script = ( script = (
"const teardown = require('./tests/e2e/teardown.js');\n" "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" " console.error(err);\n"
" process.exit(1);\n" " process.exit(1);\n"
"});\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 / "failure.png").write_bytes(b"\x89PNG sensitive screenshot")
(results_dir / "temp.tmp").write_text("in-progress trace data") (results_dir / "temp.tmp").write_text("in-progress trace data")
res = _run_teardown({ res = _run_teardown(
"CI": "true", env={
"GITHUB_ACTIONS": "true", "CI": "true",
"PLAYWRIGHT_ENABLE_TRACE": "1", # Even with opt-in, CI must purge unconditionally "GITHUB_ACTIONS": "true",
"ODYSSEUS_TEST_RESULTS_DIR": str(results_dir), "PLAYWRIGHT_ENABLE_TRACE": "1", # Even with opt-in, CI must purge unconditionally
}) },
options={"resultsDir": str(results_dir)},
)
assert res.returncode == 0 assert res.returncode == 0
# All sensitive artifacts must be purged # All sensitive artifacts must be purged
assert not (results_dir / "trace.zip").exists() 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 / "failure.png").write_bytes(b"\x89PNG")
(results_dir / "temp.tmp").write_text("temp") (results_dir / "temp.tmp").write_text("temp")
res = _run_teardown({ res = _run_teardown(
"CI": "", env={
"GITHUB_ACTIONS": "", "CI": "",
"PLAYWRIGHT_ENABLE_TRACE": "0", # No opt-in "GITHUB_ACTIONS": "",
"ODYSSEUS_TEST_RESULTS_DIR": str(results_dir), "PLAYWRIGHT_ENABLE_TRACE": "0", # No opt-in
}) },
options={"resultsDir": str(results_dir)},
)
assert res.returncode == 0 assert res.returncode == 0
assert not (results_dir / "trace.zip").exists() assert not (results_dir / "trace.zip").exists()
assert not (results_dir / "session.har").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"}} action = {"type": "action", "name": "fill", "params": {"value": "secret_password"}}
z.writestr("0-trace.trace", json.dumps(action).encode() + b"\n") z.writestr("0-trace.trace", json.dumps(action).encode() + b"\n")
res = _run_teardown({ res = _run_teardown(
"CI": "", env={
"GITHUB_ACTIONS": "", "CI": "",
"PLAYWRIGHT_ENABLE_TRACE": "1", "GITHUB_ACTIONS": "",
"ODYSSEUS_TEST_PYTHON": sys.executable, "PLAYWRIGHT_ENABLE_TRACE": "1",
"ODYSSEUS_TEST_RESULTS_DIR": str(results_dir), "ODYSSEUS_TEST_PYTHON": sys.executable,
}) },
options={"resultsDir": str(results_dir)},
)
assert res.returncode == 0 assert res.returncode == 0
# Archive is retained because sanitizer succeeded # Archive is retained because sanitizer succeeded
assert trace_file.exists() 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 = results_dir / "trace.tmp"
tmp_file.write_text("trace tmp") tmp_file.write_text("trace tmp")
res = _run_teardown({ res = _run_teardown(
"CI": "", env={
"GITHUB_ACTIONS": "", "CI": "",
"PLAYWRIGHT_ENABLE_TRACE": "1", "GITHUB_ACTIONS": "",
"ODYSSEUS_TEST_PYTHON": "/usr/bin/nonexistent_python_binary_xyz_404", "PLAYWRIGHT_ENABLE_TRACE": "1",
"ODYSSEUS_TEST_RESULTS_DIR": str(results_dir), "ODYSSEUS_TEST_PYTHON": "/usr/bin/nonexistent_python_binary_xyz_404",
}) },
options={"resultsDir": str(results_dir)},
)
assert res.returncode == 0 assert res.returncode == 0
# Fail closed: archives must be purged # Fail closed: archives must be purged
assert not trace_file.exists() 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 = results_dir / "trace.tmp"
tmp_file.write_text("trace tmp") tmp_file.write_text("trace tmp")
res = _run_teardown({ res = _run_teardown(
"CI": "", env={
"GITHUB_ACTIONS": "", "CI": "",
"PLAYWRIGHT_ENABLE_TRACE": "1", "GITHUB_ACTIONS": "",
"ODYSSEUS_TEST_PYTHON": sys.executable, "PLAYWRIGHT_ENABLE_TRACE": "1",
"ODYSSEUS_TEST_SANITIZER_SCRIPT": str(failing_script), "ODYSSEUS_TEST_PYTHON": sys.executable,
"ODYSSEUS_TEST_RESULTS_DIR": str(results_dir), },
}) options={
"resultsDir": str(results_dir),
"sanitizerScript": str(failing_script),
},
)
assert res.returncode == 0 assert res.returncode == 0
assert not trace_file.exists() assert not trace_file.exists()
assert not tmp_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 = results_dir / "trace.tmp"
tmp_file.write_text("trace tmp") tmp_file.write_text("trace tmp")
res = _run_teardown({ res = _run_teardown(
"CI": "", env={
"GITHUB_ACTIONS": "", "CI": "",
"PLAYWRIGHT_ENABLE_TRACE": "1", "GITHUB_ACTIONS": "",
"ODYSSEUS_TEST_PYTHON": sys.executable, "PLAYWRIGHT_ENABLE_TRACE": "1",
"ODYSSEUS_TEST_SANITIZER_SCRIPT": str(hanging_script), "ODYSSEUS_TEST_PYTHON": sys.executable,
"ODYSSEUS_TEST_SANITIZER_TIMEOUT_MS": "300", # 300ms timeout },
"ODYSSEUS_TEST_RESULTS_DIR": str(results_dir), options={
}) "resultsDir": str(results_dir),
"sanitizerScript": str(hanging_script),
"timeout": 300, # 300ms timeout
},
)
assert res.returncode == 0 assert res.returncode == 0
# Fail closed: timeout killed the process and purged archives # Fail closed: timeout killed the process and purged archives
assert not trace_file.exists() 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" "with open(marker, 'w') as f: f.write('ok')\n"
) )
res = _run_teardown({ res = _run_teardown(
"CI": "", env={
"GITHUB_ACTIONS": "", "CI": "",
"PLAYWRIGHT_ENABLE_TRACE": "1", "GITHUB_ACTIONS": "",
"ODYSSEUS_TEST_PYTHON": sys.executable, "PLAYWRIGHT_ENABLE_TRACE": "1",
"ODYSSEUS_TEST_SANITIZER_SCRIPT": str(spaced_script), "ODYSSEUS_TEST_PYTHON": sys.executable,
"ODYSSEUS_TEST_RESULTS_DIR": str(spaced_root), },
}) options={
"resultsDir": str(spaced_root),
"sanitizerScript": str(spaced_script),
},
)
assert res.returncode == 0 assert res.returncode == 0
assert (spaced_root / "executed.marker").exists() 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))}" formatted_payload = f"python {payload.format(marker=str(marker))}"
res = _run_teardown({ res = _run_teardown(
"CI": "", env={
"GITHUB_ACTIONS": "", "CI": "",
"PLAYWRIGHT_ENABLE_TRACE": "1", "GITHUB_ACTIONS": "",
"ODYSSEUS_TEST_PYTHON": formatted_payload, "PLAYWRIGHT_ENABLE_TRACE": "1",
"ODYSSEUS_TEST_RESULTS_DIR": str(results_dir), "ODYSSEUS_TEST_PYTHON": formatted_payload,
}) },
options={"resultsDir": str(results_dir)},
)
assert res.returncode == 0 assert res.returncode == 0
# The shell marker MUST NOT have been created! # The shell marker MUST NOT have been created!
assert not marker.exists(), f"Security failure: shell command executed for payload {payload}!" assert not marker.exists(), f"Security failure: shell command executed for payload {payload}!"
# Fail closed: trace must be purged because python execution failed # Fail closed: trace must be purged because python execution failed
assert not trace_file.exists() 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