fix(security): redact sandboxed CI evidence without secret-bearing history - #711
fix(security): redact sandboxed CI evidence without secret-bearing history#711seonghobae wants to merge 6 commits into
Conversation
📝 WalkthroughWalkthrough두 샌드박스 스크립트가 실행 증거와 subprocess 출력을 Changes샌드박스 출력 마스킹
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review Please review the exact current head. Focus on fail-closed redactor import, subprocess output and timeout redaction, service-log and evidence-note redaction, and scanner-safe regression fixtures. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== CURRENT HEAD ==='
git rev-parse HEAD
git status --short
echo '=== PR DIFF STAT ==='
git diff --stat origin/main...HEAD
echo '=== CHANGED FILES ==='
git diff --name-status origin/main...HEAD
echo '=== CHANGED PYTHON/TEST OUTLINES ==='
git diff --name-only origin/main...HEAD \
| rg '\.(py|pyi)$' \
| while IFS= read -r file; do
echo "--- $file"
ast-grep outline "$file" --items all || true
done
echo '=== SECURITY-RELATED DIFF ==='
git diff --unified=80 origin/main...HEAD -- \
'*.py' '*test*' '*Test*'Length of output: 32516 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== SHARED REDACTOR OUTLINE ==='
ast-grep outline scripts/ci/redact_sensitive_log.py --items all
echo '=== SHARED REDACTOR SOURCE ==='
cat -n scripts/ci/redact_sensitive_log.py
echo '=== ALL OUTPUT AND RAISE SITES IN WRAPPERS ==='
rg -n -C 4 \
'(?:\bprint\s*\(|\braise\s+|except\b|json\.dumps|subprocess\.(?:run|Popen)|read_text|open\()' \
scripts/ci/sandboxed_verify.py scripts/ci/sandboxed_web_e2e.py
echo '=== COMPLETE CHANGED WRAPPER SOURCE WITH LINE NUMBERS ==='
cat -n scripts/ci/sandboxed_verify.py
cat -n scripts/ci/sandboxed_web_e2e.py
echo '=== RELATED REDACTION TESTS AND FIXTURE VALUES ==='
rg -n -C 3 \
'(?:redact_text|timeout|TimeoutExpired|SANDBOXED_(?:VERIFY|WEB_E2E)_RESULT|mock_(?:token|session|password)|api_key|session_key|password)' \
tests scripts/ciLength of output: 201165 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== SHARED REDACTOR ==='
cat -n scripts/ci/redact_sensitive_log.py
echo '=== VERIFY TIMEOUT TESTS ==='
rg -n -A45 -B8 \
'def test_(?:main_reports_stubbed.*timeout|sandboxed_verify.*timeout|.*timeout.*redact)' \
tests/test_sandboxed_verify.py tests/test_sandboxed_verify_redaction.py 2>/dev/null || true
echo '=== WEB E2E TIMEOUT TESTS ==='
rg -n -A55 -B8 \
'def test_(?:main_reports_stubbed.*timeout|sandboxed_web_e2e_reports_e2e_timeout|.*timeout.*redact)' \
tests/test_sandboxed_web_e2e.py tests/test_sandboxed_web_e2e_redaction.py 2>/dev/null || true
echo '=== DIRECT SCRIPT/WORKFLOW CALLERS ==='
rg -n -C 5 \
'(?:sandboxed_verify\.py|sandboxed_web_e2e\.py|sandboxed_verify|sandboxed_web_e2e)' \
.github scripts tests \
-g '!scripts/ci/sandboxed_verify.py' \
-g '!scripts/ci/sandboxed_web_e2e.py'
echo '=== URL REDACTION PATTERN CHECK ==='
rg -n -C 4 \
'(?:URL|url|Bearer|token|secret|password|api[_-]?key|assignment|PATTERN|re\.compile)' \
scripts/ci/redact_sensitive_log.pyLength of output: 78615
P1 — Test the actual timeout publication paths
Add a focused test for each wrapper that raises The fail-closed import is correctly ordered after repository-root setup. The inspected normal subprocess output, evidence fields, and service-log tail routes apply the shared redactor. ✅ Action performedReview finished.
|
|
Addressed the current-head blocking regression-coverage finding in 1048987. Both wrappers now have focused tests that drive their actual |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head3920b2356c78d6ded4f757bcb3af5e9693d95a64. -
Head SHA:
3920b2356c78d6ded4f757bcb3af5e9693d95a64 -
Workflow run: 30801773674
-
Workflow attempt: 1
Coverage evidence
Coverage Decision
- Result: FAIL
- Test evidence: not proven passing
- Docstring evidence: not proven passing when configured
- Failure count: 1
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["CI script (2 files)"]
S1 --> I1["review and security gate shell path"]
I1 --> R1["Review risk: CI script (2 files)"]
R1 --> V1["bash -n plus Strix self-test"]
Evidence --> S2["Test (2 files)"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test (2 files)"]
R2 --> V2["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage Decision
Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["CI script (2 files)"]
S1 --> I1["review and security gate shell path"]
I1 --> R1["Review risk: CI script (2 files)"]
R1 --> V1["bash -n plus Strix self-test"]
Evidence --> S2["Test (2 files)"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test (2 files)"]
R2 --> V2["targeted test run"]
|
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head3920b2356c78d6ded4f757bcb3af5e9693d95a64. -
Head SHA:
3920b2356c78d6ded4f757bcb3af5e9693d95a64 -
Workflow run: 30801773674
-
Workflow attempt: 2
Coverage evidence
Coverage Decision
- Result: FAIL
- Test evidence: not proven passing
- Docstring evidence: not proven passing when configured
- Failure count: 1
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["CI script (2 files)"]
S1 --> I1["review and security gate shell path"]
I1 --> R1["Review risk: CI script (2 files)"]
R1 --> V1["bash -n plus Strix self-test"]
Evidence --> S2["Test (2 files)"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test (2 files)"]
R2 --> V2["targeted test run"]
![pr-auto-rebase[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
3920b23 to
07e3e0a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_sandboxed_web_e2e_redaction.py`:
- Around line 23-52: Update test_emit_result_redacts_payload_fields so
FakeArgs.evidence_note contains a sensitive fixture value such as api_key, then
assert that the original value is absent from captured.out while the redaction
marker remains present. Keep the existing command, path, and other
sensitive-value assertions intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 79d1f630-8b66-44b0-9151-ae43e6545cc6
📒 Files selected for processing (4)
scripts/ci/sandboxed_verify.pyscripts/ci/sandboxed_web_e2e.pytests/test_sandboxed_verify_redaction.pytests/test_sandboxed_web_e2e_redaction.py
| def test_emit_result_redacts_payload_fields(capsys, tmp_path) -> None: | ||
| """Machine-readable web evidence redacts commands and paths.""" | ||
| api_key = _api_key_fixture() | ||
| session_key = _session_key_fixture() | ||
| password = _password_fixture() | ||
|
|
||
| class FakeArgs: | ||
| backend_cmd = f"echo {api_key}" | ||
| frontend_cmd = f"echo {session_key}" | ||
| e2e_cmd = f"echo {password}" | ||
| allow_env: list[str] = [] | ||
| evidence_note = "used nothing_sensitive" | ||
| network = "default" | ||
| keep_sandbox = True | ||
|
|
||
| sandboxed_web_e2e.emit_result( | ||
| args=FakeArgs(), | ||
| copied_repo=tmp_path / session_key, | ||
| sandbox_root=tmp_path / "sandbox_test_root", | ||
| backend_ready=True, | ||
| frontend_ready=True, | ||
| exit_code=0, | ||
| elapsed_seconds=1.0, | ||
| ) | ||
| captured = capsys.readouterr() | ||
| assert "[REDACTED]" in captured.out | ||
| assert "mock_token_string" not in captured.out | ||
| assert "mock_session_value" not in captured.out | ||
| assert "mock_password_value" not in captured.out | ||
| assert "sandbox_test_root" in captured.out |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
민감한 evidence_note 값을 사용해 회귀 테스트를 추가하십시오.
Line 34의 값은 민감하지 않습니다. evidence_note의 redact_text 호출이 제거되어도 이 테스트는 통과합니다. evidence_note에 api_key 같은 fixture를 넣고 원문 값이 출력에 없는지 확인하십시오.
수정 예시
- evidence_note = "used nothing_sensitive"
+ evidence_note = f"used {api_key}"As per coding guidelines, scripts/ci/ 코드의 변경은 100% 테스트 커버리지를 유지해야 합니다.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_emit_result_redacts_payload_fields(capsys, tmp_path) -> None: | |
| """Machine-readable web evidence redacts commands and paths.""" | |
| api_key = _api_key_fixture() | |
| session_key = _session_key_fixture() | |
| password = _password_fixture() | |
| class FakeArgs: | |
| backend_cmd = f"echo {api_key}" | |
| frontend_cmd = f"echo {session_key}" | |
| e2e_cmd = f"echo {password}" | |
| allow_env: list[str] = [] | |
| evidence_note = "used nothing_sensitive" | |
| network = "default" | |
| keep_sandbox = True | |
| sandboxed_web_e2e.emit_result( | |
| args=FakeArgs(), | |
| copied_repo=tmp_path / session_key, | |
| sandbox_root=tmp_path / "sandbox_test_root", | |
| backend_ready=True, | |
| frontend_ready=True, | |
| exit_code=0, | |
| elapsed_seconds=1.0, | |
| ) | |
| captured = capsys.readouterr() | |
| assert "[REDACTED]" in captured.out | |
| assert "mock_token_string" not in captured.out | |
| assert "mock_session_value" not in captured.out | |
| assert "mock_password_value" not in captured.out | |
| assert "sandbox_test_root" in captured.out | |
| def test_emit_result_redacts_payload_fields(capsys, tmp_path) -> None: | |
| """Machine-readable web evidence redacts commands and paths.""" | |
| api_key = _api_key_fixture() | |
| session_key = _session_key_fixture() | |
| password = _password_fixture() | |
| class FakeArgs: | |
| backend_cmd = f"echo {api_key}" | |
| frontend_cmd = f"echo {session_key}" | |
| e2e_cmd = f"echo {password}" | |
| allow_env: list[str] = [] | |
| evidence_note = f"used {api_key}" | |
| network = "default" | |
| keep_sandbox = True | |
| sandboxed_web_e2e.emit_result( | |
| args=FakeArgs(), | |
| copied_repo=tmp_path / session_key, | |
| sandbox_root=tmp_path / "sandbox_test_root", | |
| backend_ready=True, | |
| frontend_ready=True, | |
| exit_code=0, | |
| elapsed_seconds=1.0, | |
| ) | |
| captured = capsys.readouterr() | |
| assert "[REDACTED]" in captured.out | |
| assert "mock_token_string" not in captured.out | |
| assert "mock_session_value" not in captured.out | |
| assert "mock_password_value" not in captured.out | |
| assert "sandbox_test_root" in captured.out |
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 33-33: Mutable default value for class attribute
(RUF012)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_sandboxed_web_e2e_redaction.py` around lines 23 - 52, Update
test_emit_result_redacts_payload_fields so FakeArgs.evidence_note contains a
sensitive fixture value such as api_key, then assert that the original value is
absent from captured.out while the redaction marker remains present. Keep the
existing command, path, and other sensitive-value assertions intact.
Source: Coding guidelines
1 participant
Security fix
sys.path.Replacement rationale
This supersedes #631. The prior branch contained credential-shaped fixtures in discarded commits; GitHub Advanced Security correctly continued to flag those commits even after the working tree was cleaned. This branch was rebuilt from current
mainand contains only scanner-safe fixtures, so the security gate can validate the actual final change without retaining the obsolete secret-bearing history.Validation
Current-head GitHub Actions are authoritative. The focused regression tests are included in this PR and no policy or security gate is bypassed.
Summary by CodeRabbit
보안 개선
[REDACTED]처리됩니다.테스트