fix(security): preserve exact CI evidence while redacting provider secrets - #1242
fix(security): preserve exact CI evidence while redacting provider secrets#1242seonghobae wants to merge 17 commits into
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
seonghobae
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![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 could not approve from deterministic current-head evidence because GitHub Checks have failed.
Findings
1. HIGH Current-head GitHub Checks - Fix failed required checks before approval
- Problem: Failed same-head checks remain for
9bdfcbdaf4d079de3b346e1584dd505c5043afd3. - Root cause: The model-unavailable evidence fallback is allowed only when peer GitHub Checks are complete and clean.
- Fix: Read and fix the failed check logs below, then rerun the current-head checks.
- Regression test: Keep the model-unavailable fallback gated on an empty failed-check rollup.
Failed checks:
- Strix Security Scan/strix: FAILURE (https://github.com/ContextualWisdomLab/.github/actions/runs/32645130529/job/97208123150)
- Strix Security Scan/strix: failure (https://github.com/ContextualWisdomLab/.github/actions/runs/32645130529/job/97208123150)
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file: CHANGELOG.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file: CHANGELOG.md"]
R1 --> V1["required checks"]
Evidence --> S2["Docs: ci-log-evidence-redaction.md"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs: ci-log-evidence-redaction.md"]
R2 --> V2["docs review"]
Evidence --> S3["CI script: redact_sensitive_log.py"]
S3 --> I3["review and security gate shell path"]
I3 --> R3["Review risk: CI script: redact_sensitive_log.py"]
R3 --> V3["bash -n plus Strix self-test"]
Evidence --> S4["Test: test_opencode_security_boundaries.py"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test: test_opencode_security_boundaries.py"]
R4 --> V4["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode could not approve from deterministic current-head evidence because GitHub Checks have failed. Findings1. HIGH Current-head GitHub Checks - Fix failed required checks before approval
Failed checks:
Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file: CHANGELOG.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file: CHANGELOG.md"]
R1 --> V1["required checks"]
Evidence --> S2["Docs: ci-log-evidence-redaction.md"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs: ci-log-evidence-redaction.md"]
R2 --> V2["docs review"]
Evidence --> S3["CI script: redact_sensitive_log.py"]
S3 --> I3["review and security gate shell path"]
I3 --> R3["Review risk: CI script: redact_sensitive_log.py"]
R3 --> V3["bash -n plus Strix self-test"]
Evidence --> S4["Test: test_opencode_security_boundaries.py"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test: test_opencode_security_boundaries.py"]
R4 --> V4["targeted test run"]
|
…ization-9726133996050986471 # Conflicts: # CHANGELOG.md
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Resolves the sole conflict, in CHANGELOG.md's "### Fixed" section: this PR's bullet (Stripe secret-key redaction while preserving 40-character commit SHAs and other fixed-length CI evidence) and main's independently added bullets (job-scoped github.token for same-repo OpenCode status publication, coverage-placeholder precedence fix, and coverage-cleared re-dispatch) both landed at the same list position. Both are additive, unrelated changelog entries describing separate work, so both are kept, this PR's entry first. All other files (main added/renamed/removed dozens of workflows, tests, and docs since this branch was opened, including a full hourly-review- repair caller consolidation) merged automatically with no conflicts, since this PR's actual changes are confined to scripts/ci/redact_sensitive_log.py, docs/doctoring/ci-log-evidence-redaction.md, and tests/test_opencode_security_boundaries.py, none of which main touched. Verified post-merge with Python 3.13 per requirements-opencode-review-ci-hashes.txt: - coverage run -m pytest tests: 2576 passed, 1 skipped, 2 failed (the 2 failures are tests/test_opencode_live_draft_state_regression.py shell-script subprocess tests that require the `gh` CLI binary, which this sandbox does not have installed; reproduced identically on a pristine origin/main worktree with no PR changes involved, confirming they are a pre-existing environment gap, not a regression from this merge or this PR's changes) - coverage report --show-missing: 100% (11,866 statements, 4,840 branches), matching pyproject.toml's fail_under = 100 - interrogate: 100% docstring coverage, matching fail-under = 100 - python -m py_compile on every tracked .py file: clean - git diff --check origin/main: no whitespace/conflict-marker issues Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
seonghobae
added
area: ci-cd
ChatGPT Codex Connector
![cwl-noema-review[bot]](https://avatars.githubusercontent.com/u/295022177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Noema LLM review
The PR corrects a critical regression where generic 40- and 88-character strings (including Git commit SHAs) were being redacted, destroying CI evidence. It shifts the redaction strategy for AWS and Azure keys to a context-aware assignment/JSON-key parser while adding specific, documented prefixes for Stripe keys. The changes are verified by new regression tests ensuring that unlabeled fixed-length evidence is preserved while labeled secrets are still redacted.
Reviewed changed lines
scripts/ci/redact_sensitive_log.py:15 (RIGHT): Added 'storage[_-]?key' to SENSITIVE_KEY_RE to ensure Azure storage keys are caught by the context-aware parser.scripts/ci/redact_sensitive_log.py:29 (RIGHT): Consolidated provider tokens into a single regex and added the Stripe 'sk_(?:test|live)_' prefix, removing the overbroad length-only patterns.tests/test_opencode_security_boundaries.py:137 (RIGHT): Added a critical regression test verifying that 40-char SHAs and 88-char blobs are preserved when unlabeled, but redacted when associated with sensitive keys.
Adversarial validation
scripts/ci/redact_sensitive_log.py:29 (RIGHT)falsified: A 40-character Git commit SHA will be redacted if it appears without a label. — The PROVIDER_TOKEN_RE no longer contains [A-Za-z0-9/+]{40}. The test 'test_sensitive_log_redaction_requires_context_for_fixed_length_secrets' explicitly asserts that 'a' * 40 remains in the output when unlabeled.scripts/ci/redact_sensitive_log.py:15 (RIGHT)falsified: An Azure storage key assigned via 'AZURE_STORAGE_KEY=' will survive redaction. — SENSITIVE_KEY_RE now includes 'storage[_-]?key', which triggers _consume_sensitive_assignment to redact the value. Verified by test line 143.- Residual risk: Low. While some provider secrets might survive if they lack both a documented prefix and a sensitive key label in a structured/assignment format, this is a necessary trade-off to prevent the destruction of audit evidence (commit SHAs).
Findings
- No blocking findings.
- Result: APPROVE
- Head SHA:
91bc70b8336acde093949fc304ad9af85f25da47 - Reviewer credential:
noema-review-github-app-refresh - Actor:
cwl-noema-review[bot]
…on-9726133996050986471
|
Rebased onto current Root cause of the two failing checks ( This was pure staleness: Fixed simply by picking up that later Verification on new head
Pushed non-force to Generated by Claude Code |
테스트 실행 시 |
Devin flagged (PR #1242, unresolved) two gaps in redact_sensitive_log.py left over from the provider-token consolidation: 1. _redact_json only ever checked dict *keys* against SENSITIVE_KEY_RE. A provider-token-shaped secret (ghp_..., sk-..., a Bearer header, a JWT) sitting in a string *value* under an innocuous key -- e.g. {"message": "leaked ghp_AAAA... during the run"} -- survived redaction unchanged whenever the log line happened to be valid JSON, even though the exact same text would be scrubbed by _redact_unstructured() if the line were not JSON. Factored the existing bearer/JWT/provider-token scrubbing into a shared _redact_token_patterns() helper and now apply it to every JSON string value, not just non-JSON text. 2. The new storage[_-]?key sensitive-key pattern matched as a bare substring, so storage_key_count (an ordinary diagnostic metric) was redacted along with the intended AZURE_STORAGE_KEY-shaped assignments and JSON keys. Added a negative lookahead so the match only fires when "key" ends the field name. Added regression coverage for both: a provider token embedded in a JSON string value under a harmless key, and storage_key_count staying visible alongside AZURE_STORAGE_KEY still being redacted in both JSON and assignment-text forms. Verified on this exact tree (Python 3.11, two files skipped -- see this PR's own prior comment for the pre-existing Python 3.11-vs-3.12+ f-string/backslash sandbox limitation, unrelated to this change): coverage run -m pytest tests -q -> 2633 passed, 1 skipped, 21 subtests passed; coverage report -- scripts/ci 100% (12030 statements / 4890 branches, 0 missing; redact_sensitive_log.py itself 124/124 stmts, 62/62 branches); interrogate -- 100%; git diff --check clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
|
Fixed two unresolved Devin findings that were still open against exact head 🟥 "Provider secrets survive structured logs": 🟡 "Storage metrics lose their values": the Added regression tests for both ( Verified on the pushed head Pushed non-force. No merge performed — leaving that to the scheduler per this org's governance model. Generated by Claude Code |
There was a problem hiding this comment.
Noema LLM review
The PR corrects a previous overbroad length-only redaction rule that destroyed legitimate CI evidence such as 40-character commit SHAs and 88-character opaque diagnostics. The consolidated provider-token regex only matches documented provider-specific prefixes, while opaque AWS and Azure values are redacted only through the existing sensitive assignment/JSON-key parser. The recursive JSON redactor now scrubs token-shaped values inside string values under innocuous keys. Matching regression tests verify that Stripe prefixes and labeled AWS/Azure secrets are removed while unlabeled fixed-length evidence remains visible, and that diagnostic names like storage_key_count are not over-redacted. No blocking issues were found.
Reviewed changed lines
scripts/ci/redact_sensitive_log.py:32 (RIGHT): The new string branch in_redact_jsoncalls_redact_token_patterns, ensuring provider tokens, bearer headers, and JWTs hidden under innocuous JSON keys are scrubbed. This closes the original structured-log leak.scripts/ci/redact_sensitive_log.py:33 (RIGHT): The negative lookahead narrowsstorage[_-]?keyso it only matches whenkeyends the field name. This prevents over-redacting metrics likestorage_key_countwhile still matching exact sensitive labels.scripts/ci/redact_sensitive_log.py:37 (RIGHT): The consolidated provider-token regex removes the earlier flawed 40/88-character fallback. Unlabeled fixed-length values such as commit SHAs remain visible, while documented provider prefixes are still redacted.tests/test_opencode_security_boundaries.py:137 (RIGHT): This regression test exercises the structured JSON value path and demonstrates that a provider token under an innocuousmessagekey is redacted inline while unrelated text is preserved.tests/test_opencode_security_boundaries.py:169 (RIGHT): This regression test proves unlabeled 40- and 88-character values remain visible while labeled AWS/Azure assignments are redacted, accurately reflecting the new evidence-preserving boundary.
Adversarial validation
scripts/ci/redact_sensitive_log.py:32 (RIGHT)falsified: Structured JSON strings bypass all provider-token redaction when the key is innocuous. — The new call to_redact_token_patternsin_redact_jsonprocesses every string value. The corresponding test assertsghp_is absent and only the token portion becomes[REDACTED].scripts/ci/redact_sensitive_log.py:37 (RIGHT)falsified: The reintroduction of generic fixed-length token patterns would again redact commit SHAs and opaque evidence. — The consolidatedPROVIDER_TOKEN_REcontains only documented prefixes and no length-only alternatives. The regression test at line 169 confirms the unlabeled values remain unchanged while labeled secrets are redacted.- Residual risk: Opaque provider secrets that appear without a documented prefix or a sensitive label remain visible by design. This is an intentional tradeoff favoring audit evidence preservation, documented in the new guidance file; it is not a regression from the pre-overbroad-pattern behavior and matches the stated OWASP-informed boundary.
Findings
- No blocking findings.
- Result: APPROVE
- Head SHA:
3f2a1661dbe18d26dbb405a15b5df5e099a0cdc4 - Reviewer credential:
noema-review-github-app-refresh - Actor:
cwl-noema-review[bot]
2 participants
What\n\n- consolidate provider-prefixed token redaction into one compiled expression\n- redact documented Stripe
sk_test_andsk_live_secret prefixes\n- redact opaque AWS/Azure values only when a sensitive assignment or JSON key supplies context\n- preserve unlabeled 40-character commit SHAs and other fixed-length audit evidence\n- remove the generatedpr_body.txtartifact and unsupported performance claim\n\n## Why\n\nLength-only 40/88-character patterns erased exact Git identities and other legitimate CI evidence. That weakens protected-review traceability while still failing to distinguish opaque credentials reliably.\n\n## Verification\n\nExact head9bdfcbdaf4d079de3b346e1584dd505c5043afd3:\n\n- full Python: 1,394 passed, 1 skipped, 16 subtests\n- statement/branch coverage: 100% (8,345statements,3,212branches)\n- production docstrings: 100%\n- focused security boundary: 37 passed\n- Python compilation andgit diff --check: pass\n\n## Standards and evidence\n\nSeedocs/doctoring/ci-log-evidence-redaction.mdfor the OWASP, AWS, Azure, and Stripe source-backed boundary and APA 7th references.\n\nNo real credential or organization data is stored in fixtures or documentation.