From 84c7eabfce39f6c4be916a5e914c1c4f25ee74ad Mon Sep 17 00:00:00 2001 From: Alexandre Teixeira Date: Fri, 30 Oct 2026 23:13:46 +0000 Subject: [PATCH] fix(ci): sanitize diagnostics fail-closed and purge malformed trace records --- .github/scripts/verify_sanitized_artifacts.py | 206 +++++++++++++++++ .github/workflows/native-test-suites.yml | 8 + docs/maintainers/native-test-coverage.md | 4 + scripts/sanitize_trace_artifacts.py | 37 ++-- tests/test_trace_artifact_sanitizer.py | 69 ++++++ tests/test_verify_sanitized_artifacts.py | 209 ++++++++++++++++++ 6 files changed, 518 insertions(+), 15 deletions(-) create mode 100755 .github/scripts/verify_sanitized_artifacts.py create mode 100644 tests/test_verify_sanitized_artifacts.py diff --git a/.github/scripts/verify_sanitized_artifacts.py b/.github/scripts/verify_sanitized_artifacts.py new file mode 100755 index 000000000..58fc83ddb --- /dev/null +++ b/.github/scripts/verify_sanitized_artifacts.py @@ -0,0 +1,206 @@ +#!/usr/bin/env python3 +"""Fail-closed independent verification and sanitization gate for test diagnostics. + +Runs after test suite completion or abnormal runner termination (e.g. failures, timeouts, crashes). +Ensures that no unsanitized traces, raw credentials, temporary archives, or unauthorized +diagnostics can ever be published as CI artifacts, while preserving safe JUnit XML reporting. +""" +from __future__ import annotations + +import argparse +import json +import logging +import os +import sys +import tempfile +import xml.etree.ElementTree as ET +from pathlib import Path + +# Add repository root to Python path to import sanitizer +REPO_ROOT = Path(__file__).resolve().parents[2] +if str(REPO_ROOT) not in sys.path: + sys.path.insert(0, str(REPO_ROOT)) + +try: + from scripts.sanitize_trace_artifacts import sanitize_trace_archive +except ImportError: + # If import fails, define fail-closed fallback + def sanitize_trace_archive(zp: str) -> bool: + if os.path.exists(zp): + try: + os.remove(zp) + except OSError: + pass + return False + +logging.basicConfig(level=logging.INFO, format="%(levelname)s: %(message)s") +logger = logging.getLogger("verify_sanitized_artifacts") + +PNG_MAGIC = b"\x89PNG\r\n\x1a\n" + + +def is_valid_disposable_ci_fixture() -> bool: + """Verify that execution ran inside an authorized disposable synthetic fixture.""" + is_ci = bool(os.environ.get("CI") or os.environ.get("GITHUB_ACTIONS")) + if not is_ci: + # Local runs require explicit trace opt-in + trace_opt_in = os.environ.get("PLAYWRIGHT_ENABLE_TRACE", "").lower() in ("1", "true") + return trace_opt_in + + if os.environ.get("ODYSSEUS_E2E_DISPOSABLE_ARTIFACTS") != "1": + return False + if os.environ.get("PHOTO_EDITOR_E2E_AUTH") == "0": + return False + + data_dir = os.environ.get("PHOTO_EDITOR_E2E_DATA_DIR") + if not data_dir: + return False + resolved = os.path.abspath(data_dir) + base_name = os.path.basename(resolved) + if not base_name.startswith("odysseus-photo-editor-e2e-"): + return False + + allowed_tmp = [os.path.abspath(p) for p in (tempfile.gettempdir(), "/tmp", "/var/tmp")] + if not any(resolved.startswith(b + os.sep) or resolved == b for b in allowed_tmp): + return False + + db_path = os.environ.get("PHOTO_EDITOR_E2E_DB_PATH") + if db_path != os.path.join(resolved, "app.db"): + return False + + marker_path = os.path.join(resolved, ".disposable-ci-fixture.json") + if not os.path.exists(marker_path): + return False + + try: + with open(marker_path, "r", encoding="utf-8") as f: + fixture = json.load(f) + return ( + fixture.get("synthetic") is True + and fixture.get("authenticated") is True + and fixture.get("database") == os.path.join(resolved, "app.db") + ) + except Exception: + return False + + +def verify_and_sanitize_artifacts(target_dir: str) -> bool: + """Independently verify and sanitize diagnostics fail-closed, preserving safe JUnit XML. + + Returns True if verification succeeded, False if fail-closed purge was required. + """ + if not os.path.exists(target_dir): + logger.info("Target directory %s does not exist; nothing to sanitize.", target_dir) + return True + + disposable_ci = is_valid_disposable_ci_fixture() + preserved_xmls = 0 + sanitized_archives = 0 + purged_files = 0 + + try: + for root, _, files in os.walk(target_dir): + for filename in files: + filepath = os.path.join(root, filename) + name_lower = filename.lower() + + # 1. Safe JUnit reporting: preserve valid XML reports + if name_lower.endswith(".xml"): + try: + ET.parse(filepath) + preserved_xmls += 1 + continue + except Exception as exc: + logger.warning("Corrupted XML report %s: %s; purging fail-closed", filepath, exc) + try: + os.remove(filepath) + purged_files += 1 + except OSError: + pass + continue + + # 2. Check if diagnostics are authorized (disposable synthetic fixture) + if not disposable_ci: + # In CI without disposable fixture, or local without opt-in: purge all diagnostics + logger.info("Purging diagnostic artifact %s (disposable CI fixture not verified)", filepath) + try: + os.remove(filepath) + purged_files += 1 + except OSError: + pass + continue + + # 3. Disposable CI is authorized: enforce strict sanitization + if name_lower.endswith(".zip"): + ok = sanitize_trace_archive(filepath) + if ok and os.path.exists(filepath): + sanitized_archives += 1 + else: + logger.warning("Trace archive %s failed sanitization; purged fail-closed", filepath) + if os.path.exists(filepath): + try: + os.remove(filepath) + except OSError: + pass + purged_files += 1 + + elif name_lower.endswith(".png"): + # Validate PNG magic bytes to ensure not a secret payload disguised as an image + try: + with open(filepath, "rb") as pf: + header = pf.read(8) + if header == PNG_MAGIC: + continue + except Exception: + pass + logger.warning("Invalid or suspicious screenshot %s; purged fail-closed", filepath) + try: + os.remove(filepath) + purged_files += 1 + except OSError: + pass + + else: + # Purge any .tmp, .har, .db, .sqlite, .log or unrecognized files + logger.info("Purging unauthorized diagnostic file %s", filepath) + try: + os.remove(filepath) + purged_files += 1 + except OSError: + pass + + logger.info( + "Artifact verification complete for %s: preserved %d XML reports, sanitized %d archives, purged %d files.", + target_dir, + preserved_xmls, + sanitized_archives, + purged_files, + ) + return True + + except Exception as exc: + logger.error("Unexpected error during artifact verification: %s; purging all non-XML files fail-closed", exc) + try: + for p in Path(target_dir).rglob("*"): + if p.is_file() and not p.name.lower().endswith(".xml"): + try: + p.unlink(missing_ok=True) + except OSError: + pass + except Exception: + pass + return False + + +def main() -> None: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("target_dir", nargs="?", default="test-results", help="Directory containing test results") + args = parser.parse_args() + + ok = verify_and_sanitize_artifacts(args.target_dir) + # Always exit 0 so upload-artifact can safely publish verified reports + sys.exit(0 if ok else 0) + + +if __name__ == "__main__": + main() diff --git a/.github/workflows/native-test-suites.yml b/.github/workflows/native-test-suites.yml index e85e9d43d..a7a16726d 100644 --- a/.github/workflows/native-test-suites.yml +++ b/.github/workflows/native-test-suites.yml @@ -39,6 +39,10 @@ jobs: run: | node .github/scripts/run_native_tests.mjs python .github/scripts/check_test_execution.py test-results/native-node/junit.xml --minimum 43 + - name: Verify and sanitize native test results fail-closed + if: always() + run: | + python .github/scripts/verify_sanitized_artifacts.py test-results/native-node/ - name: Save native test identities and results if: always() uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 @@ -95,6 +99,10 @@ jobs: run: | npm run test:photo-editor -- --reporter=line,junit python .github/scripts/check_test_execution.py test-results/browser-junit.xml --minimum 137 + - name: Verify and sanitize failure diagnostics fail-closed + if: always() + run: | + python .github/scripts/verify_sanitized_artifacts.py test-results/ - name: Save per-test results and sanitized failure diagnostics if: always() uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 diff --git a/docs/maintainers/native-test-coverage.md b/docs/maintainers/native-test-coverage.md index 5f7a2c575..6a6d36323 100644 --- a/docs/maintainers/native-test-coverage.md +++ b/docs/maintainers/native-test-coverage.md @@ -25,6 +25,10 @@ Default CI trace suppression remains intact. Only this job's explicit opt-in plu a newly generated authenticated synthetic fixture marker permits failure traces. The existing structured sanitizer runs before artifacts are uploaded; invalid ownership, failed sanitization and missing tooling purge sensitive artifacts. +Unsupported or malformed sensitive records cause the affected archive to be removed +fail-closed. An independent fail-closed verification step (.github/scripts/verify_sanitized_artifacts.py) +runs after test suite execution or abnormal runner termination to prevent publishing +unsanitized diagnostics while preserving safe JUnit reporting. Never upload the fixture directory, database or browser storage state. The stroke budget now excludes measured synthetic event-delivery pacing and also diff --git a/scripts/sanitize_trace_artifacts.py b/scripts/sanitize_trace_artifacts.py index ea975b3b5..43151aa95 100644 --- a/scripts/sanitize_trace_artifacts.py +++ b/scripts/sanitize_trace_artifacts.py @@ -77,13 +77,14 @@ def sanitize_body_text(text: str) -> str: # 1. Try JSON trimmed = text.strip() - if (trimmed.startswith("{") and trimmed.endswith("}")) or (trimmed.startswith("[") and trimmed.endswith("]")): + if trimmed.startswith(("{", "[")): try: parsed = json.loads(trimmed) sanitized = sanitize_structured_obj(parsed) return json.dumps(sanitized, ensure_ascii=False) - except Exception: - pass + except Exception as exc: + if SENSITIVE_KEY_PATTERN.search(trimmed) or any(p in trimmed.lower() for p in SENSITIVE_PARAM_NAMES): + raise ValueError(f"Malformed sensitive JSON request body: {exc}") from exc # 2. Try URL-encoded form data if "=" in trimmed and not trimmed.startswith("<"): @@ -98,8 +99,13 @@ def sanitize_body_text(text: str) -> str: else: out_pairs.append((k, v)) return urllib.parse.urlencode(out_pairs) - except Exception: - pass + except Exception as exc: + if SENSITIVE_KEY_PATTERN.search(trimmed): + raise ValueError(f"Malformed sensitive form request body: {exc}") from exc + + # If unparsed body text contains sensitive keys or tokens, fail closed + if SENSITIVE_KEY_PATTERN.search(trimmed) or any(p in trimmed.lower() for p in SENSITIVE_PARAM_NAMES): + raise ValueError("Malformed or unsupported sensitive request body in trace entry") return text @@ -207,7 +213,11 @@ def sanitize_structured_obj(obj: Any) -> Any: def sanitize_jsonl_content(data: bytes) -> bytes: - """Sanitize newline-delimited JSON Lines (.trace, .network, .stacks).""" + """Sanitize newline-delimited JSON Lines (.trace, .network, .stacks). + + If any non-empty record cannot be decoded as valid UTF-8 or parsed as valid JSON, + or is malformed, raise ValueError fail-closed so the affected archive is permanently removed. + """ lines = data.split(b"\n") out_lines = [] for line in lines: @@ -216,10 +226,8 @@ def sanitize_jsonl_content(data: bytes) -> bytes: continue try: parsed = json.loads(line.decode("utf-8")) - except (json.JSONDecodeError, UnicodeDecodeError): - # Not valid JSON, keep as-is - out_lines.append(line) - continue + except (json.JSONDecodeError, UnicodeDecodeError) as err: + raise ValueError(f"Malformed or unsupported JSONL trace record: {err}") sanitized = sanitize_structured_obj(parsed) out_lines.append(json.dumps(sanitized, ensure_ascii=False).encode("utf-8")) return b"\n".join(out_lines) @@ -275,11 +283,10 @@ def sanitize_trace_archive(zp: str) -> bool: elif filename_lower.endswith((".json", ".har")): try: parsed = json.loads(content.decode("utf-8")) - sanitized = sanitize_structured_obj(parsed) - zout.writestr(item, json.dumps(sanitized, ensure_ascii=False).encode("utf-8")) - except Exception: - # If invalid JSON, treat as text line-by-line - zout.writestr(item, sanitize_jsonl_content(content)) + except (json.JSONDecodeError, UnicodeDecodeError) as exc: + raise ValueError(f"Malformed JSON in archive member {item.filename}: {exc}") from exc + sanitized = sanitize_structured_obj(parsed) + zout.writestr(item, json.dumps(sanitized, ensure_ascii=False).encode("utf-8")) else: # Unknown binary or text member: exclude to fail closed continue diff --git a/tests/test_trace_artifact_sanitizer.py b/tests/test_trace_artifact_sanitizer.py index 2964ce4a2..1a6d76c3b 100644 --- a/tests/test_trace_artifact_sanitizer.py +++ b/tests/test_trace_artifact_sanitizer.py @@ -242,3 +242,72 @@ def test_multiple_trace_archives_in_directory(tmp_path): with zipfile.ZipFile(z2, "r") as z: content = json.loads(z.read("0.trace").decode().strip()) assert content["params"]["value"] == "[REDACTED]" + + +def test_fail_closed_malformed_jsonl_record(tmp_path): + """Malformed JSONL lines in .trace archives fail closed and delete the archive.""" + zp = str(tmp_path / "malformed_jsonl.zip") + with zipfile.ZipFile(zp, "w") as z: + z.writestr( + "0-trace.trace", + b'{"type": "action", "params": {"value": "secret123"}}\n' + b'this is invalid json {"password": "leak_raw_pass"}\n' + ) + ok = sanitize_trace_archive(zp) + assert ok is False + assert not os.path.exists(zp) + assert not os.path.exists(zp + ".tmp") + + +def test_fail_closed_malformed_jsonl_truncated(tmp_path): + """Truncated JSONL records in .network archives fail closed and delete the archive.""" + zp = str(tmp_path / "truncated_jsonl.zip") + with zipfile.ZipFile(zp, "w") as z: + z.writestr( + "0-trace.network", + b'{"type": "request", "request": {"url": "https://example.com/api/auth", "headers": [{"name": "Authorization", "value": "Bearer secret_tok"' + ) + ok = sanitize_trace_archive(zp) + assert ok is False + assert not os.path.exists(zp) + assert not os.path.exists(zp + ".tmp") + + +def test_fail_closed_malformed_sensitive_body(tmp_path): + """Malformed sensitive request body in trace archives fails closed and deletes archive.""" + zp = str(tmp_path / "malformed_sensitive_body.zip") + with zipfile.ZipFile(zp, "w") as z: + record = { + "type": "request", + "request": { + "url": "https://example.com/api/login", + "postData": '{"password": "super_secret_pw", "corrupt_trailing": ', + }, + } + z.writestr("0-trace.network", json.dumps(record).encode() + b"\n") + ok = sanitize_trace_archive(zp) + assert ok is False + assert not os.path.exists(zp) + assert not os.path.exists(zp + ".tmp") + + +def test_fail_closed_malformed_har_member(tmp_path): + """Malformed JSON in .har archives fails closed and deletes the archive.""" + zp = str(tmp_path / "malformed_har.zip") + with zipfile.ZipFile(zp, "w") as z: + z.writestr("network.har", b'{"log": {"entries": [ malformed JSON content') + ok = sanitize_trace_archive(zp) + assert ok is False + assert not os.path.exists(zp) + assert not os.path.exists(zp + ".tmp") + + +def test_fail_closed_invalid_utf8_jsonl_record(tmp_path): + """Non-UTF8 byte sequences in JSONL trace records fail closed and delete the archive.""" + zp = str(tmp_path / "invalid_utf8.zip") + with zipfile.ZipFile(zp, "w") as z: + z.writestr("0-trace.trace", b"\xff\xfe\x00\x00 invalid utf8 raw bytes\n") + ok = sanitize_trace_archive(zp) + assert ok is False + assert not os.path.exists(zp) + assert not os.path.exists(zp + ".tmp") diff --git a/tests/test_verify_sanitized_artifacts.py b/tests/test_verify_sanitized_artifacts.py new file mode 100644 index 000000000..4e626d073 --- /dev/null +++ b/tests/test_verify_sanitized_artifacts.py @@ -0,0 +1,209 @@ +"""Tests for the fail-closed artifact verification step (.github/scripts/verify_sanitized_artifacts.py). + +Validates that: +1. Safe JUnit XML reporting is preserved across all scenarios. +2. Abnormal runner termination without verified disposable CI fixture purges all diagnostics. +3. Valid disposable CI environments sanitize trace archives fail-closed. +4. Malformed trace archives are deleted fail-closed without corrupting or deleting JUnit XML reports. +5. In-progress temporary files, HAR files, and database dumps are purged unconditionally. +6. Top-level exceptions fail closed to purge all non-XML diagnostics. +""" +from __future__ import annotations + +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] +VERIFY_SCRIPT = REPO_ROOT / ".github" / "scripts" / "verify_sanitized_artifacts.py" + +import importlib.util + +spec = importlib.util.spec_from_file_location("verify_sanitized_artifacts", str(VERIFY_SCRIPT)) +verify_module = importlib.util.module_from_spec(spec) +sys.modules["verify_sanitized_artifacts"] = verify_module +spec.loader.exec_module(verify_module) + +is_valid_disposable_ci_fixture = verify_module.is_valid_disposable_ci_fixture +verify_and_sanitize_artifacts = verify_module.verify_and_sanitize_artifacts + + +def _setup_disposable_ci(tmp_path: Path) -> dict[str, str]: + data_dir = tmp_path / "odysseus-photo-editor-e2e-fixture" + data_dir.mkdir(parents=True, exist_ok=True) + db_path = data_dir / "app.db" + db_path.write_bytes(b"SQLite format 3\x00") + marker_file = data_dir / ".disposable-ci-fixture.json" + marker_file.write_text( + json.dumps({ + "synthetic": True, + "authenticated": True, + "database": str(db_path), + }) + ) + return { + "CI": "true", + "GITHUB_ACTIONS": "true", + "ODYSSEUS_E2E_DISPOSABLE_ARTIFACTS": "1", + "PHOTO_EDITOR_E2E_DATA_DIR": str(data_dir), + "PHOTO_EDITOR_E2E_DB_PATH": str(db_path), + "PHOTO_EDITOR_E2E_AUTH": "1", + "ODYSSEUS_TEST_PYTHON": sys.executable, + } + + +def test_preserves_valid_junit_xml_cleanly(tmp_path): + """Valid JUnit XML files are preserved intact.""" + results_dir = tmp_path / "test-results" + results_dir.mkdir() + junit = results_dir / "browser-junit.xml" + junit.write_text('') + + ok = verify_and_sanitize_artifacts(str(results_dir)) + assert ok is True + assert junit.exists() + assert "tests=\"137\"" in junit.read_text() + + +def test_fail_closed_abnormal_termination_purges_all_diagnostics(tmp_path, monkeypatch): + """Abnormal runner termination without disposable fixture purges all diagnostics, keeping JUnit.""" + # Simulate CI without disposable fixture + monkeypatch.setenv("CI", "true") + monkeypatch.setenv("GITHUB_ACTIONS", "true") + monkeypatch.delenv("ODYSSEUS_E2E_DISPOSABLE_ARTIFACTS", raising=False) + monkeypatch.delenv("PHOTO_EDITOR_E2E_DATA_DIR", raising=False) + + results_dir = tmp_path / "test-results" + results_dir.mkdir() + junit = results_dir / "browser-junit.xml" + junit.write_text('') + + trace = results_dir / "trace.zip" + trace.write_bytes(b"PK0304 sensitive trace archive") + har = results_dir / "session.har" + har.write_text('{"log": {"entries": ["secret"]}}') + png = results_dir / "failure.png" + png.write_bytes(b"\x89PNG\r\n\x1a\nfake screenshot") + tmp = results_dir / "temp.tmp" + tmp.write_text("in-progress trace") + db = results_dir / "app.db" + db.write_bytes(b"SQLite format 3 database") + + ok = verify_and_sanitize_artifacts(str(results_dir)) + assert ok is True + + # Safe JUnit reporting MUST be preserved + assert junit.exists() + assert "failures=\"1\"" in junit.read_text() + + # All sensitive diagnostics MUST be purged + assert not trace.exists() + assert not har.exists() + assert not png.exists() + assert not tmp.exists() + assert not db.exists() + + +def test_sanitizes_trace_archive_when_disposable_ci_verified(tmp_path, monkeypatch): + """When running inside a verified disposable CI fixture, trace archives are sanitized fail-closed.""" + env = _setup_disposable_ci(tmp_path) + for k, v in env.items(): + monkeypatch.setenv(k, v) + + results_dir = tmp_path / "test-results" + results_dir.mkdir() + junit = results_dir / "browser-junit.xml" + junit.write_text('') + + trace = results_dir / "trace.zip" + with zipfile.ZipFile(trace, "w") as z: + action = {"type": "action", "name": "fill", "params": {"value": "super_secret_password"}} + z.writestr("0-trace.trace", json.dumps(action).encode() + b"\n") + + png = results_dir / "failure.png" + png.write_bytes(b"\x89PNG\r\n\x1a\nvalid screenshot payload") + + ok = verify_and_sanitize_artifacts(str(results_dir)) + assert ok is True + + assert junit.exists() + assert trace.exists() + assert png.exists() + + with zipfile.ZipFile(trace, "r") as z: + content = json.loads(z.read("0-trace.trace").decode().strip()) + assert content["params"]["value"] == "[REDACTED]" + + +def test_malformed_trace_archive_purged_preserving_junit_xml(tmp_path, monkeypatch): + """Malformed trace archive is purged fail-closed without affecting JUnit XML.""" + env = _setup_disposable_ci(tmp_path) + for k, v in env.items(): + monkeypatch.setenv(k, v) + + results_dir = tmp_path / "test-results" + results_dir.mkdir() + junit = results_dir / "browser-junit.xml" + junit.write_text('') + + # Create trace archive with malformed JSONL line + malformed_trace = results_dir / "trace.zip" + with zipfile.ZipFile(malformed_trace, "w") as z: + z.writestr("0-trace.trace", b'{"type": "action"}\nmalformed not json line\n') + + ok = verify_and_sanitize_artifacts(str(results_dir)) + assert ok is True + + # JUnit XML is preserved + assert junit.exists() + # Malformed archive is purged! + assert not malformed_trace.exists() + + +def test_unhandled_exception_fails_closed_purges_all_non_xml(tmp_path, monkeypatch): + """Unexpected exception during verification triggers emergency fail-closed purge of non-XML files.""" + env = _setup_disposable_ci(tmp_path) + for k, v in env.items(): + monkeypatch.setenv(k, v) + + results_dir = tmp_path / "test-results" + results_dir.mkdir() + junit = results_dir / "browser-junit.xml" + junit.write_text('') + + trace = results_dir / "trace.zip" + trace.write_bytes(b"PK0304 trace") + + def broken_sanitize(zp): + raise RuntimeError("Simulated unexpected crash in sanitizer") + + monkeypatch.setattr(verify_module, "sanitize_trace_archive", broken_sanitize) + + ok = verify_and_sanitize_artifacts(str(results_dir)) + assert ok is False + # Safe JUnit reporting is preserved + assert junit.exists() + # Sensitive non-XML file is purged fail-closed! + assert not trace.exists() + + +def test_cli_invocation_returns_cleanly(tmp_path): + """Running the script as a CLI entrypoint succeeds with exit code 0.""" + results_dir = tmp_path / "test-results" + results_dir.mkdir() + junit = results_dir / "junit.xml" + junit.write_text('') + + res = subprocess.run( + [sys.executable, str(VERIFY_SCRIPT), str(results_dir)], + capture_output=True, + text=True, + ) + assert res.returncode == 0 + assert junit.exists()