fix(sandbox): redact credential-shaped subprocess evidence - #842
fix(sandbox): redact credential-shaped subprocess evidence#842seonghobae wants to merge 11 commits into
Conversation
|
Warning Review limit reached
Next review available in: 58 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review Please review the exact current head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base='6eb06cdd08c79a06f7b390069d4ffa49e2eb7dba'
head='19405408ab832fcc507d84cf75689d8cc7b9c80d'
printf '%s\n' '== Exact commits and ancestry =='
git rev-parse "$base" "$head"
git merge-base --is-ancestor "$base" "$head" && echo 'base is an ancestor of head'
git log --oneline --reverse "${base}..${head}"
printf '%s\n' '== Changed paths and change summary =='
git diff --name-status "$base" "$head"
git diff --stat "$base" "$head"
printf '%s\n' '== Production diff =='
git diff --unified=80 "$base" "$head" -- \
scripts/ci/sandboxed_verify.py \
scripts/ci/sandboxed_web_e2e.py \
scripts/ci/redact_sensitive_log.py
printf '%s\n' '== Regression-test diff =='
git diff --unified=40 "$base" "$head" -- \
tests/test_sandboxed_log_redaction_regression.py
printf '%s\n' '== Affected source outlines =='
ast-grep outline scripts/ci/sandboxed_verify.py --items all
ast-grep outline scripts/ci/sandboxed_web_e2e.py --items all
ast-grep outline scripts/ci/redact_sensitive_log.py --items all
ast-grep outline tests/test_sandboxed_log_redaction_regression.py --items all
printf '%s\n' '== Evidence-output call sites in affected helpers =='
rg -n -C 8 -e 'timeout_output_text\s*\(' -e 'tail_text\s*\(' -e '\bprint\s*\(' \
scripts/ci/sandboxed_verify.py scripts/ci/sandboxed_web_e2e.py
printf '%s\n' '== Full focused regression test =='
cat -n tests/test_sandboxed_log_redaction_regression.pyLength of output: 63799 🏁 Script executed: #!/bin/bash
set -euo pipefail
base='6eb06cdd08c79a06f7b390069d4ffa49e2eb7dba'
red='f33b37d882ddb8ab0ef8ffd4e843cb5edce9adc9'
head='19405408ab832fcc507d84cf75689d8cc7b9c80d'
printf '%s\n' '== RED commit changed paths =='
git diff --name-status "$base" "$red"
git diff --unified=20 "$base" "$red" -- tests/test_sandboxed_log_redaction_regression.py
printf '%s\n' '== Shared redactor implementation =='
cat -n scripts/ci/redact_sensitive_log.py
printf '%s\n' '== Dedicated quality workflow =='
cat -n .github/workflows/sandbox-log-redaction-quality-ci.yml
printf '%s\n' '== Doctoring and changelog changes =='
git diff --unified=30 "$base" "$head" -- \
docs/doctoring/sandbox-log-redaction.md \
CHANGELOG.md
printf '%s\n' '== All changed diff lines that can affect excluded scope =='
git diff --unified=5 "$base" "$head" | rg -n -i -C 3 \
'(^[+-].*(ready|url|redirect|urllib|ssrf|provider|model|credential|token|secret|permission|permissions|workflow|shell|process|timeout|redact))' || true
printf '%s\n' '== Existing shared-redactor tests and direct sandbox helper tests =='
fd -t f -e py . tests scripts | sort | while read -r file; do
if rg -q 'redact_text|timeout_output_text|tail_text|sandboxed_verify|sandboxed_web_e2e' "$file"; then
echo "--- $file"
rg -n -C 2 'redact_text|timeout_output_text|tail_text|sandboxed_verify|sandboxed_web_e2e' "$file"
fi
doneLength of output: 85587 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo='ContextualWisdomLab/.github'
pr='842'
head='19405408ab832fcc507d84cf75689d8cc7b9c80d'
printf '%s\n' '== Pull request head and base =='
gh pr view "$pr" --repo "$repo" \
--json number,state,isDraft,baseRefOid,headRefOid,statusCheckRollup,reviews \
--jq '{number, state, isDraft, baseRefOid, headRefOid, reviews: [.reviews[] | {author: .author.login, state, submittedAt}]}'
printf '%s\n' '== Check runs attached to requested head =='
gh api "repos/${repo}/commits/${head}/check-runs?per_page=100" \
--jq '.check_runs[] | {name, status, conclusion, details_url: .details_url, started_at, completed_at}'
printf '%s\n' '== Commit statuses attached to requested head =='
gh api "repos/${repo}/commits/${head}/status" \
--jq '{state, statuses: [.statuses[] | {context, state, target_url}]}'Length of output: 22395
The reviewed head is a descendant of the specified protected base. The RED commit changes only the regression test. The final code applies
The implementation preserves the reviewed process behavior. It does not change argv, I found no production changes for readiness URL handling, SSRF, provider routing, credentials, or existing workflow permissions. The new quality workflow uses only The exact PR base and head match the requested OIDs. A prior exact-head redaction-contract run and Strix run succeeded. At review time, the newest Strix, pip-audit, and CodeQL runs were still in progress. The PR is also not Draft and has no human approval. Therefore, the source review is clear, but the stated merge-readiness acceptance is not complete yet.
|
|
@coderabbitai review Please perform a fresh full review of unchanged exact head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo='ContextualWisdomLab/.github'
base='6eb06cdd08c79a06f7b390069d4ffa49e2eb7dba'
head='19405408ab832fcc507d84cf75689d8cc7b9c80d'
red='f33b37d882ddb8ab0ef8ffd4e843cb5edce9adc9'
git fetch --no-tags origin "$base" "$head" "$red"
printf '%s\n' '== Exact object identities =='
git show -s --format='commit=%H%ntree=%T%nparents=%P%nsubject=%s' "$base" "$red" "$head"
git merge-base --is-ancestor "$base" "$head"
printf '%s\n' 'base_is_ancestor_of_head=yes'
printf '%s\n' '== Commit lineage =='
git log --oneline --reverse "${base}..${head}"
printf '%s\n' '== Changed paths =='
git diff --name-status "$base" "$head"
printf '%s\n' '== Focused source outlines =='
for file in \
scripts/ci/sandboxed_verify.py \
scripts/ci/sandboxed_web_e2e.py \
scripts/ci/redact_sensitive_log.py \
tests/test_sandboxed_log_redaction_regression.py \
docs/doctoring/sandbox-log-redaction.md \
CHANGELOG.md
do
if test -f "$file"; then
echo "--- $file"
ast-grep outline "$file" --items all || true
fi
done
printf '%s\n' '== RED test-only diff =='
git diff --unified=35 "$base" "$red" -- tests/test_sandboxed_log_redaction_regression.py
printf '%s\n' '== GREEN implementation and evidence diff =='
git diff --unified=70 "$base" "$head" -- \
scripts/ci/sandboxed_verify.py \
scripts/ci/sandboxed_web_e2e.py \
scripts/ci/redact_sensitive_log.py \
tests/test_sandboxed_log_redaction_regression.py \
docs/doctoring/sandbox-log-redaction.md \
CHANGELOG.md
printf '%s\n' '== Evidence-output and process-related call sites =='
rg -n -C 10 \
-e '\b(redact_text|timeout_output_text|tail_text)\s*\(' \
-e '\b(print|subprocess\.run|Popen)\s*\(' \
-e '\b(shell|start_new_session|timeout|returncode)\b' \
scripts/ci/sandboxed_verify.py scripts/ci/sandboxed_web_e2e.py
printf '%s\n' '== Excluded-scope diff indicators =='
git diff --unified=3 "$base" "$head" | rg -n -i -C 2 \
'^[+-].*\b(ssrf|url|urllib|redirect|provider|model|credential|permission|permissions|workflow)\b' || true
printf '%s\n' '== PR identity =='
gh pr view 842 --repo "$repo" --json baseRefOid,headRefOid,isDraft,state \
--jq '{baseRefOid,headRefOid,isDraft,state}'Length of output: 91246 🏁 Script executed: #!/bin/bash
set -euo pipefail
base='6eb06cdd08c79a06f7b390069d4ffa49e2eb7dba'
head='19405408ab832fcc507d84cf75689d8cc7b9c80d'
printf '%s\n' '== New quality workflow =='
cat -n .github/workflows/sandbox-log-redaction-quality-ci.yml
printf '%s\n' '== Shared redactor contract =='
cat -n scripts/ci/redact_sensitive_log.py
printf '%s\n' '== Shared timeout-helper references =='
rg -n -C 6 '\btimeout_output_text\s*\(' .
printf '%s\n' '== Existing direct sandbox tests =='
rg -n -C 4 \
-e '\b(run_command|run_shell|timeout_output_text|tail_text|start_service|stop_service)\s*\(' \
tests scripts/ci \
-g '*.py'
printf '%s\n' '== Exact implementation-only delta =='
git diff --word-diff=plain --unified=12 "$base" "$head" -- \
scripts/ci/sandboxed_verify.py scripts/ci/sandboxed_web_e2e.py
printf '%s\n' '== Changed workflow permission and checkout settings =='
git diff --unified=20 "$base" "$head" -- .github/workflows/sandbox-log-redaction-quality-ci.yml | \
rg -n -C 8 'permissions|persist-credentials|ref:|checkout|pull_request|workflow_dispatch|contents:' || trueLength of output: 41226
I re-fetched and verified the requested identities. The pull request base is I found no current source-backed findings in the requested scope. The RED→GREEN lineage is visible. Commit The repair applies
The regression tests cover credential-shaped values, ordinary diagnostics, byte-valued timeout output, absent timeout streams, and empty completed streams. The production diff does not change subprocess argv, The changed workflow is restricted to this contract. It grants only
|
|
@coderabbitai review Please perform one fresh full review of unchanged exact head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo='ContextualWisdomLab/.github'
base='6eb06cdd08c79a06f7b390069d4ffa49e2eb7dba'
head='19405408ab832fcc507d84cf75689d8cc7b9c80d'
red='f33b37d882ddb8ab0ef8ffd4e843cb5edce9adc9'
git fetch --no-tags origin "$base" "$red" "$head"
printf '%s\n' '== Exact commit identities and ancestry =='
git show -s --format='commit=%H%ntree=%T%nparents=%P%nsubject=%s' "$base" "$red" "$head"
git merge-base --is-ancestor "$base" "$head"
echo 'base_is_ancestor_of_head=yes'
printf '%s\n' '== Pull request identity =='
gh pr view 842 --repo "$repo" --json baseRefOid,headRefOid,state,isDraft \
--jq '{baseRefOid,headRefOid,state,isDraft}'
printf '%s\n' '== Commit lineage and changed paths =='
git log --oneline --reverse "${base}..${head}"
git diff --name-status "$base" "$head"
printf '%s\n' '== Scoped file outlines =='
for file in \
scripts/ci/sandboxed_verify.py \
scripts/ci/sandboxed_web_e2e.py \
scripts/ci/redact_sensitive_log.py \
tests/test_sandboxed_log_redaction_regression.py \
docs/doctoring/sandbox-log-redaction.md \
CHANGELOG.md \
.github/workflows/sandbox-log-redaction-quality-ci.yml
do
if test -f "$file"; then
echo "--- $file"
ast-grep outline "$file" --items all || true
fi
done
printf '%s\n' '== RED test-only delta =='
git diff --unified=45 "$base" "$red" -- tests/test_sandboxed_log_redaction_regression.py
printf '%s\n' '== Scoped GREEN delta =='
git diff --unified=90 "$base" "$head" -- \
scripts/ci/sandboxed_verify.py \
scripts/ci/sandboxed_web_e2e.py \
scripts/ci/redact_sensitive_log.py \
tests/test_sandboxed_log_redaction_regression.py \
docs/doctoring/sandbox-log-redaction.md \
CHANGELOG.md \
.github/workflows/sandbox-log-redaction-quality-ci.yml
printf '%s\n' '== Evidence-output and subprocess call sites =='
rg -n -C 12 \
-e '\b(redact_text|timeout_output_text|tail_text)\s*\(' \
-e '\b(print|subprocess\.run|Popen)\s*\(' \
-e '\b(shell|start_new_session|timeout|returncode|killpg|terminate|wait)\b' \
scripts/ci/sandboxed_verify.py scripts/ci/sandboxed_web_e2e.py
printf '%s\n' '== Excluded-scope diff indicators =='
git diff --unified=5 "$base" "$head" | rg -n -i -C 3 \
'^[+-].*\b(ssrf|url|urllib|redirect|provider|model|credential|permission|permissions|workflow|network|socket)\b' || true
printf '%s\n' '== Current exact-head checks =='
gh api "repos/${repo}/commits/${head}/check-runs?per_page=100" \
--jq '.check_runs[] | {name,status,conclusion}' | sortLength of output: 106088
I re-fetched the requested commits. The pull request base is I found no current source-backed findings in the requested scope. The RED→GREEN lineage is visible. Commit The production code redacts all reviewed evidence paths:
The tests cover credential-shaped values, ordinary diagnostic text, byte-valued timeout output, missing timeout streams, and empty completed streams. The source diff does not change subprocess arguments, The diff does not add provider, SSRF, or credential behavior changes. The added workflow grants only Exact-head checks include successful redaction-contract, CodeQL, Semgrep, pip-audit, secret-scan, OSV, SBOM, and Strix runs. These results support the review. They do not replace source review or the required counted non-author human approval.
|
🎯 **What:** Aborted the code health refactoring of `scripts/ci/sandboxed_verify.py` and reverted changes. 💡 **Why:** As noted in code review, the requested extraction of the printing logic into a helper function overlaps unsafely with an active security fix (PR #842) which introduces a redaction boundary. Merging this would cause conflicts and potentially reintroduce a pre-redaction vulnerability. ✅ **Verification:** Verified that the working tree is clean and all tests, including interrogator and bandit, pass. ✨ **Result:** No changes made, task closed safely.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| end_private_key = "-----END " + "PRIVATE KEY-----" | ||
| begin_pgp_private_key = "-----BEGIN PGP " + "PRIVATE KEY BLOCK-----" | ||
| end_pgp_private_key = "-----END PGP " + "PRIVATE KEY BLOCK-----" |
| source = ( | ||
| f"PRIVATE_KEY={begin_private_key}\n" | ||
| "MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSj\n" |
| assert cleaned["token_expires_at"] == "2026-08-09T00:00:00Z" | ||
| assert cleaned["token_type"] == "Bearer" | ||
| assert cleaned["token_usage"] == {"input": 256, "output": 128} | ||
|
|
| unterminated_cleaned = redactor.redact_text(unterminated) | ||
| assert opaque_secret not in unterminated_cleaned | ||
| assert unterminated_cleaned.count("\n") == unterminated.count("\n") | ||
|
|
|
Superseded by clean-history replacement #888. Exact evidence on replacement head
The source repair is preserved; only the unsafe branch-history reachability is replaced. This PR is closed without merge or history rewriting. #888 remains subject to its exact-head queued checks, current review, qualifying counted non-author approval, and protected-main runtime acceptance. |
2 participants
Purpose
Create a clean current-main, test-first replacement for the sandbox output-redaction portion of #841 without importing its unrelated SSRF slice or rewriting Jules history.
RCA
Protected
maincaptures and prints subprocess stdout/stderr inscripts/ci/sandboxed_verify.pyandscripts/ci/sandboxed_web_e2e.py, decodes timeout stdout/stderr throughtimeout_output_text(), and prints backend/frontend service log tails throughtail_text()without passing those evidence paths through the matureredact_sensitive_log.redact_textboundary. A credential-shaped value emitted by a repository test or service can therefore become CI/review evidence even though the sandbox scrubs ambient secret environment variables.This is an evidence-redaction defect, not shell injection, provider routing, SSRF, reviewer identity, or branch-protection failure.
Feasibility decision
RED state
Exact base:
6eb06cdd08c79a06f7b390069d4ffa49e2eb7dba.Initial test-only head:
f33b37d882ddb8ab0ef8ffd4e843cb5edce9adc9.The only changed file at this RED head is
tests/test_sandboxed_log_redaction_regression.py. It requires all CI-facing evidence paths to redact representative credential-shaped values while preserving ordinary diagnostic text:sandboxed_verifystdout/stderr;sandboxed_verifytimeout stdout/stderr bytes;sandboxed_web_e2estdout/stderr;Protected main does not satisfy these contracts, so this head is intentionally not merge-ready. No production file has been changed yet.
Required GREEN implementation
After hosted RED evidence confirms the intended failure, make only the smallest production repair:
redact_sensitive_log.redact_textat the sandbox evidence-output boundary;shell=False, process-group, timeout, readiness, network, or cleanup behavior;Do not add readiness-URL SSRF changes, provider/model changes, credential changes, workflow-permission changes, branch-protection changes, or output-resource-limit work owned by #766.
Acceptance
Keep Draft until the RED→GREEN lineage is visible, focused sandbox tests and the complete central suite pass, changed production statement/branch coverage and public docstrings are 100%, Strix/security/supply-chain exact-head gates are green, automated review is current-head, zero valid unresolved findings remain, and a qualifying counted non-author human approval exists. Protected-main operational evidence remains required before closing any incident tied to real log disclosure.
Supersedes only the redaction slice of #841 after this replacement proves its own exact-head evidence; preserve #841 history unchanged.
Follow-up exhaustive boundary review (current head)
Exact follow-up head:
9d24ea60ac9a281b7ac9d855ad4c86e3b68db182(tree21891b5aad2bb6e86007ee05c8b4b1a7b727b48a), still based directly on19405408ab832fcc507d84cf75689d8cc7b9c80d.The initial GREEN exposed five narrower defects in the shared boundary: opaque JSON strings were not scanned, benign metadata keys were over-redacted, terminal controls could split signatures, explicit
--allow-envvalues were not supplied to the redactor, and assignment parsing was quadratic. Consumer-path review then added fail-first coverage for result-marker integrity, credential-bearing JSON keys, authorization headers, URL userinfo, private-key blocks, escaped/multiline literals, service-tail truncation order, terminal overwrite/default-ignorable controls, pathological JSON depth, separated credential options, setup/launch exceptions, and fixed-evidence collisions.The bounded follow-up:
redact_sensitive_log.py;Current source-tree acceptance:
git diff --check: PASS;Output-memory and service-file quotas remain explicitly out of scope under #766. Hosted checks must prove this exact head, and the branch must still satisfy the live ruleset's two approvals, last-push approval, and resolved-thread requirements before merge.