ci(#5221): redact behaviour debug artifacts before upload - #6167
Conversation
Authorized PR-head code can populate behaviour-artifacts with downloaded workflow output and attacker-controlled files. Scan and redact text, nested archives, and job logs before upload; stub encrypted blobs and binary files that embed known secrets. Run redaction from a base-branch script PRs cannot override. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Behaviour debug artifacts are text and JSON only; keeping unscannable binary/media files allows exfiltration via disguised formats (e.g. base64 in a fake .gif). Stub all binary, media, and gzip-with-NUL payloads. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 8:40 AM UTC · Completed 8:59 AM UTC Commit: |
PR Summary by QodoRedact behaviour debug artifacts with base-branch script before upload
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Site previewPreview: https://583ddf35-site.fullsend-ai.workers.dev Commit: |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Labels: PR modifies CI workflows (.github/workflows/e2e.yml) and adds CI scripts (.github/scripts/), addressing a security issue in e2e behaviour test infrastructure. Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
component/ci
Use safe Python archive extraction with pre-extract size limits and path validation, fix GNU sed case-insensitive flag, build test literals at runtime for secret scanners, gate artifact upload on redaction success, and expand GitHub token pattern coverage. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
ifireball
changed the title
|
🤖 Review · Commit: |
…s manifest CollectVendoredAssets auto-includes .github/scripts/*; enumerateVendoredPaths must list the new redaction scripts so scaffold tests stay in sync. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · ❌ Terminated · Started 9:27 AM UTC · Ended 9:47 AM UTC Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Superseded by updated review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 9:27 AM UTC · Completed 9:47 AM UTC Commit: |
…tization Pass literal secrets to awk via ENVIRON so backslashes are not escaped, extend _sanitize_log_path for URL-encoded newlines and ANSI sequences, and add a backslash literal-secret regression test. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Multi-agent review squad pass (3 agents — Claude + Grok — findings cross-verified against source). Posting the unique medium-and-above findings; everything from the earlier qodo/bot round is already fixed or declared out of scope and is not re-raised. 2 critical, 2 high, 12 medium follow as inline comments.
Two notes: (1) the two critical fixes and the E2E_GCP_SERVICE_ACCOUNT env addition touch .github/workflows/e2e.yml — actionable by you as author, but the automated /fs-fix code agent can't edit workflow files, so those need a manual hand. (2) The base-ref script checkout, fail-closed upload gating, and safe archive traversal/size guards are genuinely well done — the findings below are about the execution environment and the redaction internals, not that design.
…review Use shell: bash with pipefail for behaviour-test tee, run redaction in a clean env -i with pinned PATH, harden the redaction script (multiline PEM lines, symlinks, capped gzip, archive limits, per-file error handling), add adversarial tests, and document residual scanning limitations. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 7:14 PM UTC · Completed 7:29 PM UTC Commit: |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Approving after re-review of 280af014. All findings from the earlier multi-agent squad pass are resolved and verified locally.
Verified locally:
redact-behaviour-artifacts-test.sh— 19/19 pass (11 original + 8 new adversarial: multi-line PEM body, one-line JSON PEM, top-level symlink, relativeARTIFACT_DIRzip, unreadable file, NUL-byte.log, gzip bomb)shellcheck 0.11.0— clean on both scriptsgo test ./internal/scaffold/...— pass (vendor manifest addition consistent with embedded files)
Findings resolved:
- Critical —
teemasking failures fixed viashell: bash(pipefail); redaction now runs underenv -iwith pinnedPATH,unset LD_PRELOAD/BASH_ENV, absolute interpreter — interpreter/PATH hijack closed, residual same-job race documented honestly. - High — multi-line PEM literals now redacted line-by-line; symlinks stubbed before upload (
_remove_symlinks). - Medium — one-line PEM no longer sets sticky
in_pem; archive size guards count bytes actually read (matches Go reference); gzip capped decompress;E2E_GCP_SERVICE_ACCOUNTadded; O(n²) loop removed;inherit_errexit+ stub-on-failure; NUL check moved ahead of extension classification;ARTIFACT_DIRcanonicalized to absolute; unreadable files stubbed instead of aborting. - Docs — overclaim corrected to "cannot modify the checked-in script contents" + a Residual Limitations section covering the encoding/obfuscation and same-job-race gaps.
Redaction is a defense-in-depth layer, and the residual limitations (content-scan can't catch every encoding; same-job race) are now documented rather than claimed away — reasonable for this change. Nice work.
Match auth header tokens until the next whitespace so base64 characters like + and / are covered; add regression test. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 7:36 PM UTC · Completed 8:15 PM UTC Commit: |
|
🤖 Finished Retro · ✅ Success · Started 8:01 PM UTC · Completed 8:17 PM UTC Commit: |
|
Review skipped — this PR is already merged. The Posted by fullsend post-review check |
Retro: PR #6167 — Redact behaviour debug artifacts before uploadWorkflow shape: Human-authored PR (with Cursor) fixing issue #5221. Triage ran on the issue; 5 review agent runs across the PR lifecycle; no code or fix agent involvement (triage correctly noted the code agent cannot modify Review quality gap — the central finding. The human reviewer (waynesun09) conducted a multi-agent review squad pass (3 agents cross-verified) and posted 2 critical, 2 high, 12 medium findings. The fullsend review bot posted 7 low-severity findings across 3 review passes. The quality gap is dramatic: the human found that the entire redaction feature was dead code (critical: Notably, the agent's findings were shallow variants of deeper issues the human caught:
The human also verified findings empirically (e.g., Evidence for existing issues
Proposal filedOne proposal filed below for a genuinely novel gap: the review agent's security sub-agent does not analyze GitHub Actions step-execution context (shell defaults, env persistence, step ordering) when reviewing CI workflow changes — the category that produced both critical findings on this PR. No existing open issue covers this specific pattern. Proposals filed |
2 participants
Summary
make behaviour-testoutput intobehaviour-test.logfor the same pass..gif).Test plan
bash .github/scripts/redact-behaviour-artifacts-test.shscript-test/ lint workflow on PR.github/workflows/e2e.ymlredaction step wiringFixes #5221
Made with Cursor