fix: scrub secret values from CI log artifacts before upload - #488
Conversation
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (8 passed)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/responses-vllm-maas.yml (1)
139-158: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winSame ordering issue: scrub runs after test-report publish.
dorny/test-reporter(139-147) publishes the raw XML to the Checks UI before the scrub step (149-158) redactsVLLM_API_TOKEN/VLLM_EMBEDDING_API_TOKEN/TAVILY_SEARCH_API_KEY. Secrets embedded in captured pytest output are already exposed by the time redaction runs.Reorder so scrubbing precedes "Publish test report" for consistency with the other two responses workflows.
🤖 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 @.github/workflows/responses-vllm-maas.yml around lines 139 - 158, The workflow order in the test results section is wrong: `dorny/test-reporter` publishes raw XML before `scrub_secrets.sh` redacts sensitive values. Move the “Scrub secrets from test results” step to run before “Publish test report” in the responses-vllm-maas workflow, keeping the same secret env vars and test-results path so the redacted XML is what `test-reporter` consumes..github/workflows/responses-vertexai.yml (1)
151-169: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winSame ordering issue: scrub runs after test-report publish.
dorny/test-reporter(151-159) consumes the raw XML before the scrub step (161-169) redactsVERTEX_AI_PROJECT/TAVILY_SEARCH_API_KEY. Any secret leaked into captured pytest output is already exposed in the Checks UI before redaction occurs.Reorder so the scrub step precedes "Publish test report".
🤖 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 @.github/workflows/responses-vertexai.yml around lines 151 - 169, The workflow order in the test-results publishing section is wrong: the raw XML is sent to dorny/test-reporter in Publish test report before Scrub secrets from test results runs. Move the Scrub secrets from test results step so it executes first, then publish the report afterward, keeping the existing step names and the same secret-redaction inputs for VERTEX_AI_PROJECT and TAVILY_SEARCH_API_KEY..github/workflows/responses-openai.yml (1)
135-153: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winScrub step runs after the test report is already published — too late to prevent UI exposure.
dorny/test-reporter(135-143) reads/tmp/test-results/*.xmland renders it in the Checks UI before the new scrub step (145-153) redacts secrets from those same files. IfOPENAI_API_KEY/TAVILY_SEARCH_API_KEYend up in captured stdout/tracebacks (verbose pytest-s -v), they're exposed in the PR checks tab regardless of the later file-level scrub (CWE-532).Move the scrub step before "Publish test report" so redaction happens prior to any consumption of the file content.
🤖 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 @.github/workflows/responses-openai.yml around lines 135 - 153, The test results are being published before secrets are scrubbed, so sensitive values can still appear in the Checks UI. Move the Scrub secrets from test results step to run before the Publish test report step in the workflow, ensuring the /tmp/test-results files are redacted before dorny/test-reporter reads them; keep the existing scrub script and secret env names, and preserve the report step’s current behavior afterward.
🧹 Nitpick comments (1)
tests/check_secret_scrub_list.sh (1)
24-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
grep -A10window is brittle.If the
scrub_secrets.shinvocation's argument list in the workflow grows beyond 10 lines, later var names will silently drop out ofscrub_vars, weakening the drift check without any visible failure.🤖 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/check_secret_scrub_list.sh` around lines 24 - 27, The secret variable extraction in check_secret_scrub_list.sh is using a fixed grep -A10 window around the scrub_secrets.sh invocation, which can miss variables if the workflow arguments expand. Update the logic in the scrub_vars pipeline to avoid relying on a hardcoded line window and instead parse the full relevant scrub_secrets.sh block so all referenced uppercase secret vars are captured consistently, even as the workflow changes.
🤖 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/scrub_secrets.sh`:
- Around line 29-38: The scrub step in scrub_secrets.sh currently only skips
IsADirectoryError, so any other read/write failure can stop redaction partway
through while later upload-artifact steps still proceed. Update the scrub
workflow to fail fast on any per-file scrub error, and make the artifact upload
condition depend on scrub success instead of only using always()/!cancelled(),
referencing the scrub logic in scrub_secrets.sh and the upload-artifact steps in
the workflow.
---
Outside diff comments:
In @.github/workflows/responses-openai.yml:
- Around line 135-153: The test results are being published before secrets are
scrubbed, so sensitive values can still appear in the Checks UI. Move the Scrub
secrets from test results step to run before the Publish test report step in the
workflow, ensuring the /tmp/test-results files are redacted before
dorny/test-reporter reads them; keep the existing scrub script and secret env
names, and preserve the report step’s current behavior afterward.
In @.github/workflows/responses-vertexai.yml:
- Around line 151-169: The workflow order in the test-results publishing section
is wrong: the raw XML is sent to dorny/test-reporter in Publish test report
before Scrub secrets from test results runs. Move the Scrub secrets from test
results step so it executes first, then publish the report afterward, keeping
the existing step names and the same secret-redaction inputs for
VERTEX_AI_PROJECT and TAVILY_SEARCH_API_KEY.
In @.github/workflows/responses-vllm-maas.yml:
- Around line 139-158: The workflow order in the test results section is wrong:
`dorny/test-reporter` publishes raw XML before `scrub_secrets.sh` redacts
sensitive values. Move the “Scrub secrets from test results” step to run before
“Publish test report” in the responses-vllm-maas workflow, keeping the same
secret env vars and test-results path so the redacted XML is what
`test-reporter` consumes.
---
Nitpick comments:
In `@tests/check_secret_scrub_list.sh`:
- Around line 24-27: The secret variable extraction in
check_secret_scrub_list.sh is using a fixed grep -A10 window around the
scrub_secrets.sh invocation, which can miss variables if the workflow arguments
expand. Update the logic in the scrub_vars pipeline to avoid relying on a
hardcoded line window and instead parse the full relevant scrub_secrets.sh block
so all referenced uppercase secret vars are captured consistently, even as the
workflow changes.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8c2e625a-1da9-4ade-8482-dc310431dc26
📒 Files selected for processing (7)
.github/workflows/redhat-distro-container.yml.github/workflows/responses-openai.yml.github/workflows/responses-vertexai.yml.github/workflows/responses-vllm-maas.yml.pre-commit-config.yamltests/check_secret_scrub_list.shtests/scrub_secrets.sh
| for f in glob.glob(sys.argv[0]): | ||
| try: | ||
| with open(f, 'r', errors='replace') as fh: | ||
| content = fh.read() | ||
| for s in secrets: | ||
| content = content.replace(s, '***REDACTED***') | ||
| with open(f, 'w') as fh: | ||
| fh.write(content) | ||
| except IsADirectoryError: | ||
| pass |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files | rg -n 'scrub_secrets\.sh|artifact|upload|workflow|\.yml$|\.yaml$'Repository: opendatahub-io/ogx-distribution
Length of output: 1318
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the scrub script and workflow callers, then inspect relevant snippets.
git ls-files | rg '(^|/)tests/scrub_secrets\.sh$|(^|/)\.github/workflows/.*\.(yml|yaml)$'
printf '\n--- scrub_secrets.sh ---\n'
wc -l tests/scrub_secrets.sh
cat -n tests/scrub_secrets.sh
printf '\n--- workflow references ---\n'
rg -n 'scrub_secrets\.sh|upload-artifact|if:\s*(always\(\)|!\s*cancelled\(\))' .github/workflows tests -g '*.yml' -g '*.yaml' -g '*.sh'Repository: opendatahub-io/ogx-distribution
Length of output: 4831
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact workflow steps around the scrub and upload actions.
rg -n -A4 -B6 'scrub_secrets\.sh|upload-artifact|if:\s*(always\(\)|!\s*cancelled\(\))' .github/workflows -g '*.yml' -g '*.yaml'Repository: opendatahub-io/ogx-distribution
Length of output: 18461
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- tests/scrub_secrets.sh ---'
cat -n tests/scrub_secrets.sh
printf '\n%s\n' '--- workflow upload gates ---'
rg -n -A3 -B6 'scrub_secrets\.sh|upload-artifact|if:\s*(always\(\)|!\s*cancelled\(\))' .github/workflows -g '*.yml' -g '*.yaml'Repository: opendatahub-io/ogx-distribution
Length of output: 18310
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- tests/scrub_secrets.sh ---'
nl -ba tests/scrub_secrets.sh | sed -n '1,220p'
echo
echo '--- workflow references ---'
rg -n -A5 -B8 'scrub_secrets\.sh|upload-artifact|if:\s*(always\(\)|!\s*cancelled\(\))' .github/workflows -g '*.yml' -g '*.yaml'Repository: opendatahub-io/ogx-distribution
Length of output: 240
Failing scrub can still leak secrets in uploaded artifacts (CWE-532/CWE-703).
tests/scrub_secrets.sh only ignores IsADirectoryError; any other per-file I/O failure aborts redaction mid-stream, and the downstream upload-artifact steps still run under if: !cancelled()/always(). Fail the workflow on scrub errors and gate artifact upload on scrub success.
🤖 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/scrub_secrets.sh` around lines 29 - 38, The scrub step in
scrub_secrets.sh currently only skips IsADirectoryError, so any other read/write
failure can stop redaction partway through while later upload-artifact steps
still proceed. Update the scrub workflow to fail fast on any per-file scrub
error, and make the artifact upload condition depend on scrub success instead of
only using always()/!cancelled(), referencing the scrub logic in
scrub_secrets.sh and the upload-artifact steps in the workflow.
|
Tick the box to add this pull request to the merge queue (same as
|
7f9e740 to
c66076f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/check_secret_scrub_list.sh (1)
24-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
grep -A10may truncate long scrub invocations.If the
scrub_secrets.shinvocation in the workflow lists variables across more than 10 lines, variables beyond that window won't be extracted intoscrub_vars, producing false "missing" reports. Consider usinggrep -Awith a larger count or a different delimiter-based approach (e.g.,sedbetween invocation markers).🤖 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/check_secret_scrub_list.sh` around lines 24 - 27, Update the scrub_vars extraction around the scrub_secrets.sh invocation so it captures every referenced variable, even when the invocation spans more than 10 lines. Replace the fixed grep -A10 window with a delimiter-based extraction between the invocation and its end, or use a sufficiently robust range that cannot truncate valid entries; preserve the existing filtering and sorting behavior.
🤖 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.
Nitpick comments:
In `@tests/check_secret_scrub_list.sh`:
- Around line 24-27: Update the scrub_vars extraction around the
scrub_secrets.sh invocation so it captures every referenced variable, even when
the invocation spans more than 10 lines. Replace the fixed grep -A10 window with
a delimiter-based extraction between the invocation and its end, or use a
sufficiently robust range that cannot truncate valid entries; preserve the
existing filtering and sorting behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6afaf03e-7c62-48ba-8e2a-e46e48838a61
📒 Files selected for processing (8)
.github/workflows/redhat-distro-container.yml.github/workflows/responses-openai.yml.github/workflows/responses-vertexai.yml.github/workflows/responses-vllm-maas.yml.pre-commit-config.yamldistribution/requirements-lock.txttests/check_secret_scrub_list.shtests/scrub_secrets.sh
🚧 Files skipped from review as they are similar to previous changes (6)
- .pre-commit-config.yaml
- .github/workflows/redhat-distro-container.yml
- .github/workflows/responses-openai.yml
- .github/workflows/responses-vllm-maas.yml
- .github/workflows/responses-vertexai.yml
- tests/scrub_secrets.sh
b6dd7b0 to
3276e3c
Compare
Upstream OGX redacts known field names (api_key, api_token, password) but any provider whose secret uses a different field name would appear in plaintext in the uploaded log artifacts. Add a defense-in-depth scrub step that replaces actual secret env-var values with ***REDACTED*** in all log files before the upload-artifact step. Also add a pre-commit hook (check-secret-scrub) that ensures the scrub list stays in sync with smoke.sh — it greps for secret-looking env vars (KEY, TOKEN, PASSWORD, SECRET, CREDENTIAL) passed to the container and fails if any are missing from the workflow's scrub list. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Nathan Weinberg <nweinber@redhat.com>
Add scrub steps before upload-artifact in the three responses workflows (OpenAI, Vertex AI, vLLM MaaS) to match the pattern in redhat-distro-container.yml. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move the inline Python scrubber into tests/scrub_secrets.sh and call it from all workflows that upload artifacts. Each workflow passes its own glob pattern and secret env var names as arguments, eliminating the duplicated Python snippet. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
8325c51 to
dfd9265
Compare
|
@nathan-weinberg: The following test has Succeeded: OCI Artifact Browser URLInspecting Test Artifacts ManuallyTo inspect your test artifacts manually, follow these steps:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/opendatahub/odh-ci-artifacts:ogx-core-functional-its-krg7q |
5 participants
Summary
***REDACTED***in all log files before theupload-artifactstepcheck-secret-scrubpre-commit hook that ensures the scrub list stays in sync withsmoke.sh— greps for secret-looking env vars (KEY,TOKEN,PASSWORD,SECRET,CREDENTIAL) passed to the container and fails if any are missing from the workflow's scrub listTest plan
pre-commit run check-secret-scrub --all-filespasses🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes