Repository navigation
Prevent secrets from bypassing redaction in patch and bundle artifacts - #50778
Conversation
…ifacts Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. No ADR enforcement needed: PR #50778 does not have the 'implementation' label and has only 10 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Test Quality Sentinel completed test quality analysis. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
There was a problem hiding this comment.
Pull request overview
Hardens artifact secret handling by redacting patch files and excluding unscannable git bundles.
Changes:
- Adds
.patchsecret scanning and regression tests. - Excludes binary bundles from unified artifacts.
- Aligns compiler validation with text-scanning extensions.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/redact_secrets.cjs |
Adds patch scanning. |
actions/setup/js/redact_secrets.test.cjs |
Tests patch redaction. |
pkg/workflow/compiler_yaml_post_agent.go |
Excludes bundle artifacts. |
pkg/workflow/compiler_artifacts_test.go |
Verifies bundle exclusion. |
pkg/workflow/step_order_validation.go |
Updates scanned extensions. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
|
|
||
| // Find all target files in /tmp/gh-aw and ${RUNNER_TEMP}/gh-aw directories | ||
| const targetExtensions = [".txt", ".json", ".log", ".md", ".mdx", ".yml", ".jsonl"]; | ||
| const targetExtensions = [".txt", ".json", ".log", ".md", ".mdx", ".yml", ".jsonl", ".patch"]; |
| // Bundle files are binary git bundles that cannot be safely text-scanned for | ||
| // secrets. They are excluded from the downloadable artifact. | ||
| // The artifact upload step already sets if-no-files-found: ignore, so excluding | ||
| // the bundle glob here is safe even when no bundle files exist. |
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Excellent work! 🎉 This PR prevents a critical vulnerability where secrets in git patches could bypass redaction during artifact uploads. What looks great:
The PR is ready for maintainer review. All checks pass, tests cover the new functionality, and the implementation is sound.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — two observations raised as inline comments; overall the fix is well-scoped and correct.
📋 Key Themes & Highlights
Key Themes
- Extension list drift risk —
step_order_validation.gonow adds.md/.mdx/.ymltoscannedExtensionswithout documenting the rationale; the comment pointing toredact_secrets.cjshelps but a brief "why" would seal it. - Fragile test harness — the
eval()+string-replace approach for the.patchtests can silently no-op if the source string drifts; worth a guard assertion.
Positive Highlights
- ✅ The core fix is minimal and precise: one character class added to
targetExtensions. - ✅ Binary
.bundlefiles are now correctly excluded from artifact uploads with clear reasoning in comments. - ✅ Two meaningful regression tests added for the exact vulnerability scenario (exact-value secret + GitHub PAT pattern).
- ✅ Compile-time guarantee in
step_order_validation.gonow accurately reflects the runtime contract. - ✅ Existing test flipped cleanly to assert the corrected behaviour.
| scannedExtensions := []string{".txt", ".json", ".log", ".md", ".mdx", ".yml", ".jsonl", ".patch"} | ||
| if slices.Contains(scannedExtensions, ext) { | ||
| return true | ||
| } |
There was a problem hiding this comment.
[/diagnosing-bugs] The new scannedExtensions silently adds .md, .mdx, and .yml — absent from the old list — changing what isPathScannedBySecretRedaction returns for those extensions. If any caller uses this function to reject certain paths, this is a silent behavioural change worth documenting.
💡 Suggestion
Add a brief comment explaining why .md/.mdx/.yml were added (they already appear in redact_secrets.cjs) to prevent future reviewers from removing them. Verify all callers of isPathScannedBySecretRedaction to ensure the additions are safe.
@copilot please address this.
| const redacted = fs.readFileSync(patchFile, "utf8"); | ||
| expect(redacted).not.toContain("my-patch-secret-value"); | ||
| expect(redacted).toContain("***REDACTED***"); | ||
| }), |
There was a problem hiding this comment.
[/tdd] Both .patch tests use eval() to monkey-patch findFiles by string-replacing the source. This is fragile: any whitespace change in redact_secrets.cjs will silently break the replacement and the test will pass vacuously (no files found, nothing to redact).
💡 Suggestion
Consider asserting that at least one file was processed by checking a side-effect (e.g. mockCore.info called with the file path) so the test fails loudly if findFiles substitution breaks. Alternatively, extract findFiles as an injectable dependency so tests can pass a test double without string replacement.
@copilot please address this.
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Review: Prevent secrets from bypassing redaction in patch and bundle artifacts
The core security fix is correct and well-structured: adding .patch to targetExtensions closes a real redaction gap, removing .bundle from artifacts is the right call for binary-safe text scanning, and the tests are a solid addition.
One non-blocking issue flagged inline:
threat_detection_steps.go still copies aw-*.bundle files into /tmp/gh-aw/threat-detection/ (lines 196–200). The PR's rationale — bundles are binary, can't be safely UTF-8 decoded — applies equally here. If the threat detection LLM reader ingests those files as text, it receives garbled binary content. This should either remove the bundle-copy loop or add a comment explaining why binary bundles are intentionally forwarded to the detection context.
Everything else (extension list sync in step_order_validation.go, flipped test assertion in compiler_artifacts_test.go, new JS tests) looks correct.> 🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 43.8 AIC · ⊞ 5.3K
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (3 tests)
|
|
@copilot bundle files are required downstream for pull requests so keep those |
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
REQUEST_CHANGES — a critical functional regression: excluding .bundle files from the unified artifact appears to silently break the default patch-format: bundle transport for push_to_pull_request_branch/create_pull_request whenever the safe_outputs job needs to apply changes (the bundle file is generated in the agent job and can only reach the downstream job via this artifact).
Themes
- The
.patchredaction fix and the.bundlescanned-extensions sync instep_order_validation.goare correct and well-targeted. - The bundle-artifact removal in
compiler_yaml_post_agent.gosolves a real security gap (binary files can't be safely UTF-8-scanned) but does so by silently dropping a feature (bundle transport) rather than fixing the scanning gap or explicitly deprecating/warning about bundle-format in cross-job scenarios. - Test coverage for the bundle removal is purely negative (asserts the glob is absent) and does not verify the end-to-end behavior of bundle-format workflows post-change.
| // Bundle files are binary git bundles that cannot be safely text-scanned for | ||
| // secrets. They are excluded from the downloadable artifact. | ||
| // The artifact upload step already sets if-no-files-found: ignore, so excluding | ||
| // the bundle glob here is safe even when no bundle files exist. |
There was a problem hiding this comment.
Removing the bundle glob from the unified artifact breaks git-bundle transport for non-staged push_to_pull_request_branch/create_pull_request — the bundle is only produced in the agent job and consumed via this artifact by a separate job.
| // Bundle files are binary git bundles that cannot be safely text-scanned for | ||
| // secrets. They are excluded from the downloadable artifact. | ||
| // The artifact upload step already sets if-no-files-found: ignore, so excluding | ||
| // the bundle glob here is safe even when no bundle files exist. |
There was a problem hiding this comment.
This removes the only mechanism shipping git-bundle files to the safe_outputs/detection jobs, silently breaking the default patch-format: bundle transport whenever a checkout is actually needed in a separate job.
💡 Details and fix
generate_git_bundle.cjs writes /tmp/gh-aw/aw-*.bundle inside the agent job (see safe_outputs_handlers.cjs calling generateGitBundle for both create_pull_request and push_to_pull_request_branch). The safe_outputs job runs as a separate GitHub Actions job and only gets access to agent-job files by downloading the unified agent artifact (buildAgentOutputDownloadSteps / patch artifact download in compiler_safe_outputs_job.go). push_to_pull_request_branch.cjs and create_pull_request.cjs both look for the bundle at that path and, since patch-format: bundle is the default, will silently fall back to patch transport (losing merge topology / per-commit metadata) any time the bundle actually exists but was never uploaded — which, after this change, is always, because TmpAwBundleGlob was dropped from collectArtifactPaths.
The stated rationale ("binary bundles cannot be safely text-scanned") is a real constraint, but the fix removes the feature instead of solving the scanning problem. Options:
- Keep the bundle glob in the artifact but exclude it from the redact_secrets text scan (a binary file cannot be treated as UTF-8 text), while adding a compensating control such as base64/binary-safe secret scanning, or
- Explicitly disable/deprecate
patch-format: bundlefor non-staged flows and document that onlyam/patch format is supported cross-job, with a clear compile-time warning/error.
As written, this is a silent functional regression with no test coverage — compiler_artifacts_test.go only asserts the bundle glob is absent, not that bundle-format cross-job pushes still work end-to-end.
| if !strings.Contains(uploadSection, "/tmp/gh-aw/aw-*.bundle") { | ||
| t.Error("Expected '/tmp/gh-aw/aw-*.bundle' in unified artifact upload when threat detection is enabled with staged push-to-pull-request-branch") | ||
| if strings.Contains(uploadSection, "/tmp/gh-aw/aw-*.bundle") { | ||
| t.Error("Expected '/tmp/gh-aw/aw-*.bundle' to be excluded from unified artifact upload (binary bundles cannot be safely text-scanned for secrets)") |
There was a problem hiding this comment.
This assertion only proves the bundle glob is absent — it does not cover whether bundle-format transport (the default) still functions correctly across jobs, so it cannot catch the regression flagged above.
💡 Details
Flipping !strings.Contains(...) to strings.Contains(...) correctly encodes the new intended behavior (bundle glob excluded), but it's a purely negative assertion. There's no test verifying that a workflow configured with patch-format: bundle and a non-staged push_to_pull_request_branch/create_pull_request actually delivers the bundle file to the safe_outputs job (or that it now correctly falls back to am/patch format, if that's the intended new behavior). Consider adding an integration-style test that compiles a bundle-format workflow and asserts the resulting job graph either (a) still ships the bundle via some other channel, or (b) explicitly forces patch-format to am / emits a compile warning when checkout is required outside the agent job.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
| // .bundle files are binary git bundles produced when patch-format: bundle is | ||
| // configured. They cannot be safely scanned as UTF-8 text by redact_secrets, but | ||
| // they are required downstream to apply changes while preserving merge topology, | ||
| // so they are intentionally allowed through artifact uploads unscanned. |
There was a problem hiding this comment.
@copilot they are generated from the same git as the patch so the patch scanning is enough
There was a problem hiding this comment.
Updated the comments to reflect this: bundles are built from the same git commit range as the accompanying .patch (via generateGitBundle calling generateGitPatch), so the .patch scan already covers the underlying diff content and no separate bundle scan is needed.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Restored the |
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot Thanks for the update. Please run the pr-finisher skill to address the remaining review feedback before maintainer pickup:
Run: https://github.com/github/gh-aw/actions/runs/31076105335
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Both items were already addressed in the latest commits before this comment:
Verified with targeted Go tests ( |
|
🎉 This pull request is included in a new release. Release: |
redact_secrets.cjsdid not scan.patchfiles, so secrets written into git patches could be uploaded unredacted.step_order_validation.goincorrectly asserted both.patchand.bundlewere runtime-covered, creating a false compile-time guarantee. Binary.bundlefiles were also included in downloadable artifacts despite being unsafe to text-scan.Changes
redact_secrets.cjs— Add.patchtotargetExtensionsso patch files are redacted before upload alongside other text formats.compiler_yaml_post_agent.go— RemoveTmpAwBundleGlobfrom artifact collection. Binary git bundles cannot be safely scanned as UTF-8 and must not be uploaded as downloadable artifacts.step_order_validation.go— SyncscannedExtensionswith the actual runtime contract: add.md,.mdx,.yml; drop.bundle. Add a comment noting this list must tracktargetExtensionsinredact_secrets.cjs.compiler_artifacts_test.go— Flip the bundle assertion to verify.bundleis not included in artifact uploads.redact_secrets.test.cjs— Add two new tests for.patchfile redaction: exact-value secret replacement and built-in credential pattern (GitHub PAT) detection.Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.