diff --git a/.github/workflows/sandbox-log-redaction-quality-ci.yml b/.github/workflows/sandbox-log-redaction-quality-ci.yml new file mode 100644 index 000000000..36ba60bc3 --- /dev/null +++ b/.github/workflows/sandbox-log-redaction-quality-ci.yml @@ -0,0 +1,105 @@ +name: Sandbox Log Redaction Quality CI + +on: + pull_request: + branches: [main] + paths: + - ".github/workflows/sandbox-log-redaction-quality-ci.yml" + - "CHANGELOG.md" + - "docs/doctoring/sandbox-log-redaction.md" + - "scripts/ci/redact_sensitive_log.py" + - "scripts/ci/sandboxed_verify.py" + - "scripts/ci/sandboxed_web_e2e.py" + - "tests/test_opencode_security_boundaries.py" + - "tests/test_sandboxed_verify.py" + - "tests/test_sandboxed_web_e2e.py" + - "tests/test_sandboxed_log_redaction_regression.py" + +permissions: + contents: read + +concurrency: + group: sandbox-log-redaction-quality-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +jobs: + exact-head-redaction-contract: + if: github.event_name != 'pull_request' || github.event.action != 'closed' + runs-on: ubuntu-24.04 + timeout-minutes: 10 + steps: + - name: Checkout exact source revision + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + ref: ${{ github.event.pull_request.head.sha || github.sha }} + persist-credentials: false + + - name: Set up Python + uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + with: + python-version: "3.14" + + - name: Install exact hash-verified test dependencies + env: + PIP_DISABLE_PIP_VERSION_CHECK: "1" + PIP_NO_INPUT: "1" + shell: bash --noprofile --norc -e -o pipefail {0} + run: | + cat >"${RUNNER_TEMP}/sandbox-redaction-quality-requirements.txt" <<'EOF' + coverage==7.15.2 --hash=sha256:b9a6367e4aff723e8ee8190836836124284e8fcd4265e307c844010cfa074f3f + iniconfig==2.1.0 --hash=sha256:9deba5723312380e77435581c6bf4935c94cbfab9b1ed33ef8d238ea168eb760 + packaging==26.2 --hash=sha256:5fc45236b9446107ff2415ce77c807cee2862cb6fac22b8a73826d0693b0980e + pluggy==1.6.0 --hash=sha256:e920276dd6813095e9377c0bc5566d94c932c33b27a3e3945d8389c374dd4746 + pygments==2.20.0 --hash=sha256:81a9e26dd42fd28a23a2d169d86d7ac03b46e2f8b59ed4698fb4785f946d0176 + pytest==9.1.1 --hash=sha256:37a86b45efb9a47a61a36449063e8e18d0cab3161329fc099eb21783169c4f0c + EOF + python -m pip install \ + --only-binary=:all: \ + --require-hashes \ + -r "${RUNNER_TEMP}/sandbox-redaction-quality-requirements.txt" + + - name: Verify fail-closed sandbox redaction contract + env: + STRIX_TEST_PROCESS_TIMEOUT_SECONDS: "3" + STRIX_TEST_FAKE_SLEEP_SECONDS: "5" + shell: bash --noprofile --norc -e -o pipefail {0} + run: | + test "$(git rev-parse HEAD)" = "${{ github.event.pull_request.head.sha || github.sha }}" + python -m coverage run --branch -m pytest \ + tests/test_opencode_security_boundaries.py \ + tests/test_sandboxed_verify.py \ + tests/test_sandboxed_web_e2e.py \ + tests/test_sandboxed_log_redaction_regression.py \ + -q + python -m coverage report \ + --include='scripts/ci/redact_sensitive_log.py,scripts/ci/sandboxed_verify.py,scripts/ci/sandboxed_web_e2e.py' \ + --fail-under=100 + python - <<'PY' + import ast + from pathlib import Path + + missing = [] + for filename in ( + "scripts/ci/redact_sensitive_log.py", + "scripts/ci/sandboxed_verify.py", + "scripts/ci/sandboxed_web_e2e.py", + ): + tree = ast.parse(Path(filename).read_text(encoding="utf-8"), filename=filename) + for node in tree.body: + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)): + if not node.name.startswith("_") and ast.get_docstring(node) is None: + missing.append(f"{filename}:{node.lineno}:{node.name}") + if missing: + raise SystemExit("public docstrings missing: " + ", ".join(missing)) + PY + python -m pytest tests -q + bash scripts/ci/test_strix_quick_gate.sh + python -m compileall -q \ + scripts/ci/redact_sensitive_log.py \ + scripts/ci/sandboxed_verify.py \ + scripts/ci/sandboxed_web_e2e.py \ + tests/test_opencode_security_boundaries.py \ + tests/test_sandboxed_verify.py \ + tests/test_sandboxed_web_e2e.py \ + tests/test_sandboxed_log_redaction_regression.py + git diff --exit-code diff --git a/CHANGELOG.md b/CHANGELOG.md index bf30091dd..aa8f0be97 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ Semantic Versioning where the repository publishes a release. ### Fixed +- Redacted credential-shaped stdout, stderr, timeout evidence, and bounded backend/frontend service-log tails emitted by sandboxed verification and web-E2E subprocesses before those values become CI or review evidence. The boundary now canonicalizes safe ANSI styling and fails closed on single- or multiline rendering controls, scans JSON keys and opaque string values without corrupting scalar types or result schemas, preserves benign metadata and diagnostic domains, removes authorization headers, URL userinfo, and private-key blocks, protects raw and escaped explicitly allowed values before setup and tail selection, handles separated credential options, preserves markers and line boundaries, falls back safely on excessive JSON nesting, and parses high-volume diagnostics within bounded time. Normal executed argv, child exit, timeout, readiness, and network behavior remains unchanged; ambiguous short or fixed-evidence-colliding allowed values and setup/launch exceptions return redacted code `126` evidence before a raw traceback can escape. - Bounded the Strix quality self-test's deterministic timeout fixtures to 3-second process and 5-second fake-sleep budgets so exact-head policy evidence completes inside the existing job limit without changing production Strix scanner timeouts, providers, credentials, or review semantics. - Allowed commas and ASCII parentheses in the bounded Strix changed-file path policy so legal tracked Packrat fixtures can receive exact-head security analysis, while rejecting raw `..` components before normalization and keeping controls, backslashes, whitespace ambiguity, and shell punctuation fail-closed. - Bound each review-agent invocation key to the wrapper's complete canonical payload, including the base branch and requesting actor; altered fields with a valid-format key now fail before durable-leader election or forwarding, and wrapper write permission is job-scoped. diff --git a/docs/doctoring/sandbox-log-redaction.md b/docs/doctoring/sandbox-log-redaction.md new file mode 100644 index 000000000..b151df1fa --- /dev/null +++ b/docs/doctoring/sandbox-log-redaction.md @@ -0,0 +1,94 @@ +# Sandbox subprocess evidence redaction + +## Incident boundary + +The organization sandbox wrappers already removed ambient secret-bearing environment variables before launching pull-request verification commands. That control did not cover a different disclosure path: child processes and long-running services can emit credential-shaped values to stdout, stderr, timeout evidence, or service log files. `sandboxed_verify.py` and `sandboxed_web_e2e.py` forwarded those captured values into GitHub Actions/review evidence without passing them through the existing `redact_sensitive_log.redact_text` boundary. + +This is an evidence-handling defect. It is not a shell-injection defect, a reason to change subprocess argv/process-group semantics, a provider-routing problem, or a reason to broaden/revoke repository credentials. + +## RCA + +The causal chain is: + +1. a verification command or local web service emits text controlled by the repository-under-review; +2. the sandbox correctly captures that text through `subprocess.PIPE`, `TimeoutExpired`, or a service log file; +3. the wrapper prints the captured text into CI/review evidence; +4. the mature central log redactor was not invoked on this output boundary; and +5. therefore a token/password/session-key-shaped value can cross the sandbox boundary even though the corresponding ambient environment variable was scrubbed. + +Python documents `subprocess.run(..., shell=False, stdout=PIPE, stderr=PIPE, timeout=...)` and `TimeoutExpired` as normal captured-output mechanisms. The repair therefore leaves process execution semantics unchanged and treats the captured text as untrusted evidence that requires redaction before publication. + +## Feasibility analysis + +The following candidates were evaluated: + +- **Change or remove subprocess execution. Rejected.** The defect occurs after capture, so changing argv, shell mode, process groups, or timeouts would not address the disclosure mechanism and would add unrelated behavioral risk. +- **Rely only on GitHub Actions secret masking. Rejected.** GitHub recommends masking sensitive values and notes that log redaction is not a complete substitute for avoiding sensitive output; child output may contain transformed or non-registered sensitive data. Repository-under-review output must therefore cross the product's own deterministic redaction boundary. +- **Import the mixed sentinel #841. Rejected.** That branch combined this defect with unrelated readiness-URL hardening, production changes preceded its tests, and the external writer reported that its narrowed result could not be published. Rewriting or manually reconstructing unpublished UI state would weaken provenance. +- **Apply the existing redactor at the evidence-output boundary. Accepted.** This is the smallest reversible change, requires no new credential or permission, preserves process semantics, and is directly testable with credential-shaped fixtures. + +The first GREEN implementation exposed five narrower defects in that shared boundary during an exhaustive diff review: + +- valid JSON took a structured branch that redacted only sensitive key names and never scanned string values under opaque keys; +- substring key matching hid benign diagnostics such as `token_count` and `password_policy`; +- terminal ANSI sequences could split a sensitive key or provider-token signature before detection; +- explicitly allowed environment values were not supplied to the redactor, so an opaque value printed without a recognizable key or provider prefix survived; and +- `_redact_assignments()` restarted a suffix scan at every character in a long key-like line, producing observed quadratic growth even though its contract claimed linear parsing. + +The root cause was not missing calls in the two wrappers anymore. It was an incomplete normalization and value-provenance contract in the shared redactor, plus repeated suffix scanning. The follow-up repair canonicalizes terminal evidence before matching, recursively scans JSON keys and string values, classifies credential fields by semantic words while exempting explicit diagnostic-metadata endings, passes explicitly allowed environment values through a validated literal-sensitive path, and advances the assignment parser by complete key spans. + +A consumer-path review then found additional integration hazards: post-processing the prefixed result marker could corrupt otherwise valid JSON, selecting a service tail before literal redaction could expose a clipped multiline value, a permissive three-segment pattern hid the `api.deepseek.com` failure signal as if it were a JWT, excessive valid JSON nesting could abort the central failed-check collector, literal values such as `true` could corrupt JSON types, terminal overwrite/format controls could reconstruct rendered secrets, separated CLI options could leave their following credential visible, and setup, launch, or cleanup exceptions could escape the boundary. The bounded repair now redacts trusted result schemas before serialization, protects existing markers with a single-pass matcher, redacts complete service logs before tail selection, verifies decoded JOSE headers, preserves JSON scalar types and every supported line separator, handles separated credential options, fails closed across multiline or unterminated terminal controls, falls back safely on excessive JSON recursion, and captures explicitly allowed values before fallible setup. It does not implement the separate output-memory and service-file quotas tracked by #766. + +## Test-first evidence + +A clean branch was created from protected `main` `6eb06cdd08c79a06f7b390069d4ffa49e2eb7dba`. + +The first commit `f33b37d882ddb8ab0ef8ffd4e843cb5edce9adc9` changed only `tests/test_sandboxed_log_redaction_regression.py`. Hosted exact-head quality run `31291746435`, job `93190018774`, then produced the intended RED result: **4 failed, 23 passed**. Each new failure exposed credential-shaped text crossing one of the required output boundaries. + +Only after that hosted RED evidence did production change. The implementation: + +- redacts completed `sandboxed_verify` stdout/stderr; +- decodes and redacts `sandboxed_verify` timeout stdout/stderr, including byte-valued `TimeoutExpired` evidence; +- redacts completed web-E2E stdout/stderr; +- reuses the redacted timeout helper for web-E2E timeout evidence; and +- redacts complete backend/frontend service-log text before selecting and printing the bounded tail. + +Normal child-command exit codes, readiness URL, redirect, executed subprocess argv, process-group, timeout, provider/model, workflow permission, and branch-protection behavior are unchanged. Workspace-setup and process-launch exceptions now return fail-closed code `126` with a redacted diagnostic instead of propagating a raw traceback. A non-empty explicitly allowed environment value shorter than eight characters, or one identical to fixed evidence text that must remain machine-readable, also returns `126` before child execution because it cannot be redacted unambiguously. Other values, including whitespace-bearing credentials, are passed to the child unchanged while their raw and escaped evidence representations are suppressed. + +Exact-head focused acceptance on `9dd2ab31a8eab9ef7b4572e37c68023df6a0f16e` reports **32 passed** and exact **100% statement and branch coverage** for both owned production modules (`265` statements and `82` branches total). The permanent quality workflow also enforces public callable docstrings, exact-head checkout, a complete repository suite, the central Strix quick gate, compilation, and a clean worktree before Ready status is permitted. + +The follow-up fail-first contract independently reproduced eight initial failures: opaque JSON values, benign JSON metadata over-redaction, ANSI-split evidence, quadratic long-line processing, and opaque `--allow-env` values across completed/timeout verification plus completed/timeout/service-tail E2E paths. Consumer review added fail-first cases for marker integrity, truncation order, credential-bearing JSON keys, joined and suffixed credential fields, authorization headers, URL userinfo, private-key blocks, raw and escaped explicit values, all supported line separators, JSON scalar types, diagnostic domains, pathological JSON depth, high line/value counts, separated credential options, short or fixed-evidence-colliding allowed values, terminal overwrite and multiline/unterminated control sequences, and pre-copy/backend/frontend/E2E launch and cleanup exceptions. After the bounded repair, the focused suite reports **102 passed** and exact **100% statement and branch coverage** across `redact_sensitive_log.py`, `sandboxed_verify.py`, and `sandboxed_web_e2e.py` (`605` statements and `206` branches). The exact-head quality workflow now owns all three modules and the shared redactor security tests, so a wrapper-only coverage result cannot promote a redactor regression. + +## Security and privacy interpretation + +Redaction is defense in depth, not authorization. It does not make arbitrary sensitive material safe to publish and it does not authorize repositories to pass secrets into sandbox commands. The existing environment minimization remains the primary ingress control; deterministic output redaction limits accidental disclosure if a child process or service emits sensitive-looking evidence anyway. + +ANSI styling canonicalization preserves visible diagnostic text and line separators, while cursor movement, backspace, multiline/unterminated control payloads, and invisible Unicode format controls fail closed for every affected evidence line or value. Structured JSON retains benign failure, policy, count, status, expiry, type, and usage metadata, while credential-denoting keys, credential material used as a key, credential-shaped string values, authorization headers, URL userinfo, and multiline private-key blocks are replaced without changing boolean or numeric types. Result markers remain one-line valid JSON with stable trusted keys after redaction, and the collector's literal `api.deepseek.com` classification signal remains visible. Literal protection is limited to non-empty values of names explicitly passed through `--allow-env`; values shorter than eight characters or colliding with fixed evidence text are rejected before execution, while whitespace-bearing values remain supported. The ordinary safe environment allowlist is not treated as secret, avoiding blanket removal of paths, locale data, or other useful diagnostics. + +The redactor operates on CI-facing text only. It does not mutate files in the copied repository, service log files on disk, subprocess input, or successful child-process status and lifetime. The wrappers separately apply the documented pre-execution and exception code `126` policy. Operators should therefore interpret `[REDACTED]` as evidence suppression, not as successful removal of sensitive data from the source system that produced it. + +## Rollback + +If the redaction integration causes a demonstrated diagnostic incompatibility, revert the production redaction commits while retaining the fail-first regression and this doctoring record. Do not disable the regression, weaken the credential-shaped fixtures, or replace the deterministic boundary with blanket omission of all stdout/stderr. A rollback is incomplete until the resulting protected-main behavior is reassessed for disclosure risk. + +## Operational acceptance + +PR checks prove the code path, not protected-main operation. After protected integration, run one bounded sandbox verification fixture that emits synthetic credential-shaped stdout/stderr and one bounded web-E2E fixture that emits a synthetic service-log credential. Accept the repair only if protected-main workflow evidence shows the synthetic value absent, `[REDACTED]` present, ordinary diagnostic text preserved, and the expected exit code/process cleanup behavior unchanged. + +## References + +GitHub. (n.d.). *Secure use reference*. GitHub Docs. Retrieved August 9, 2026, from https://docs.github.com/en/actions/reference/security/secure-use + +GitHub. (n.d.). *Using secrets in GitHub Actions*. GitHub Docs. Retrieved August 9, 2026, from https://docs.github.com/en/actions/how-tos/write-workflows/choose-what-workflows-do/use-secrets + +MITRE Corporation. (n.d.). *CWE-180: Incorrect behavior order: Validate before canonicalize*. CWE. Retrieved August 9, 2026, from https://cwe.mitre.org/data/definitions/180.html + +MITRE Corporation. (n.d.). *CWE-407: Inefficient algorithmic complexity*. CWE. Retrieved August 9, 2026, from https://cwe.mitre.org/data/definitions/407.html + +MITRE Corporation. (n.d.). *CWE-532: Insertion of sensitive information into log file*. CWE. Retrieved August 9, 2026, from https://cwe.mitre.org/data/definitions/532.html + +National Institute of Standards and Technology. (2022). *Secure software development framework (SSDF) version 1.1: Recommendations for mitigating the risk of software vulnerabilities* (NIST SP 800-218). https://doi.org/10.6028/NIST.SP.800-218 + +OWASP Foundation. (n.d.). *Logging cheat sheet*. OWASP Cheat Sheet Series. Retrieved August 9, 2026, from https://cheatsheetseries.owasp.org/cheatsheets/Logging_Cheat_Sheet.html + +Python Software Foundation. (2026). *subprocess — Subprocess management* (Python 3.14.6 documentation). https://docs.python.org/3.14/library/subprocess.html diff --git a/scripts/ci/redact_sensitive_log.py b/scripts/ci/redact_sensitive_log.py index 16e89f264..1eb684086 100644 --- a/scripts/ci/redact_sensitive_log.py +++ b/scripts/ci/redact_sensitive_log.py @@ -3,27 +3,129 @@ from __future__ import annotations +import base64 import json import re +import shlex import sys +import unicodedata +from collections.abc import Sequence from typing import Any REDACTED = "[REDACTED]" KEY_CHARS = frozenset("ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_.-") -SENSITIVE_KEY_RE = re.compile( - r"(?:token|secret|password|passwd|credential|authorization|jwt|" - r"api[_-]?key|private[_-]?key|access[_-]?key|session[_-]?key)", - re.IGNORECASE, +ANSI_ESCAPE_RE = re.compile( + r"(?:\x1b\[|\x9b)[0-?]*[ -/]*[@-~]" + r"|(?:\x1b\]|\x9d)[^\x1b\x07\x9c]*(?:\x07|\x9c|\x1b\\)" + r"|(?:\x1b[PX^_]|\x90|\x98|\x9e|\x9f)[^\x1b\x9c]*(?:\x9c|\x1b\\)" + r"|(?:\x1b\]|\x1b[PX^_]|\x90|\x98|\x9d|\x9e|\x9f)[\s\S]*\Z" + r"|\x1b[ -/]*[0-~]" + r"|[\x80-\x84\x86-\x8f\x91-\x9a\x9c]" +) +SGR_ESCAPE_RE = re.compile(r"(?:\x1b\[|\x9b)[0-9:;]*m") +UNSAFE_INLINE_CONTROL_RE = re.compile( + r"[\x00-\x08\x0b\x0c\x0e-\x1f\x7f-\x84\x86-\x9f]" +) +LINE_SEPARATOR_RE = re.compile(r"\r\n|[\n\r\v\f\x1c-\x1e\x85\u2028\u2029]") +LINE_SEPARATOR_END_RE = re.compile( + r"(?:\r\n|[\n\r\v\f\x1c-\x1e\x85\u2028\u2029])\Z" +) +CAMEL_ACRONYM_BOUNDARY_RE = re.compile(r"(?<=[A-Z])(?=[A-Z][a-z])") +CAMEL_WORD_BOUNDARY_RE = re.compile(r"(?<=[a-z0-9])(?=[A-Z])") +NON_KEY_WORD_RE = re.compile(r"[^A-Za-z0-9]+") +SENSITIVE_KEY_TERMS = frozenset( + { + "auth", + "authorization", + "credential", + "credentials", + "jwt", + "passwd", + "password", + "secret", + "token", + } +) +SENSITIVE_KEY_PAIRS = frozenset( + { + ("access", "key"), + ("api", "key"), + ("connection", "string"), + ("database", "url"), + ("encryption", "key"), + ("private", "key"), + ("secret", "key"), + ("session", "key"), + ("signing", "key"), + } +) +SENSITIVE_JOINED_KEY_TERMS = frozenset( + "".join(pair) for pair in SENSITIVE_KEY_PAIRS +) +BENIGN_JOINED_KEY_TERMS = frozenset({"notsecret", "retoken"}) +SAFE_METADATA_SUFFIXES = frozenset( + { + ("budget",), + ("count",), + ("decode", "error"), + ("expires", "at"), + ("failure", "reason"), + ("policy",), + ("policy", "status"), + ("rotation", "status"), + ("scan", "count"), + ("status",), + ("type",), + ("usage",), + } ) JWT_RE = re.compile( r"(?\b(?:proxy-)?authorization\s*:\s*)[^\r\n]+", + re.IGNORECASE, +) BEARER_RE = re.compile( - r"(?P\b(?:authorization\s*:\s*)?(?:bearer|basic)\s+)" + r"(?P\b(?:bearer|basic)\s+)" r"[^\s\"'\\]+", re.IGNORECASE, ) +URL_CREDENTIAL_RE = re.compile( + r"(?P\b[a-z][a-z0-9+.-]*://)[^/\s:@]*:[^@\s/]+@", + re.IGNORECASE, +) +PRIVATE_KEY_PEM_RE = re.compile( + r"-----BEGIN (?P