Prevent safe-output artifacts from capturing base64 CI trigger token#50636
Conversation
|
Hey 👋 — thanks for getting started on the critical security fix for CI trigger token exposure in safe-output artifacts! This is well-scoped and addresses a real vulnerability. Since this is still a WIP with no changes yet, here are the next steps to move it forward:
The PR is on the right track. Once changes are committed and tests are in place, this should be ready for review. Keep pushing! 🚀
|
… header Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #50636 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — approving with one minor test coverage suggestion.
📋 Key Themes & Highlights
Key Themes
- Root cause properly addressed:
silent: trueprevents the credential-bearing command line from reaching stdout and uploaded artifacts — not just masking symptoms - Defense-in-depth:
core.setSecret(tokenBase64)registers the value for runner-side redaction as a secondary guard - Comprehensive branch coverage: both
cwdand non-cwdbranches in bothoverridePersistedExtraheaderandrestorePersistedExtraheaderare silenced
Positive Highlights
- ✅ Clear PR description explaining primary vs. secondary fix and why
setSecretalone is insufficient - ✅ New focused test verifies both
setSecretis called with the correct base64 value and that all exec calls are silenced - ✅ Existing test assertions updated to enforce
silent: truecontract going forward - ✅ No consumer regeneration required — clean deploy path via runtime file copy
Minor Gap (inline comment)
The "all exec calls silenced" loop in the new test only runs inside overridePersistedExtraheader scope. A parallel assertion for restorePersistedExtraheader would close the regression window on the restore path. See inline comment for a suggested test.
Comments that could not be inline-anchored
actions/setup/js/git_auth_helpers.test.cjs:332
[/tdd] The "never echo via exec" loop only covers overridePersistedExtraheader call sites — the restorePersistedExtraheader exec calls are not included in this sweep.
<details>
<summary>💡 Suggested addition</summary>
Add a parallel assertion in the restorePersistedExtraheader describe block:
it('should silence all exec calls during restore', async () => {
const prevHeader = `Authorization: basic ${Buffer.from('x-access-token:ghp_prev').toString('base64')}`;
await restore…
</details>There was a problem hiding this comment.
Pull request overview
Prevents credential-bearing Git commands from leaking into safe-output artifacts.
Changes:
- Silences Git configuration commands containing authorization headers.
- Adds masking registration and related tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/git_auth_helpers.cjs |
Silences credential-bearing Git commands and adds masking. |
actions/setup/js/git_auth_helpers.test.cjs |
Updates assertions for silent execution and masking. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
| // masking redacts it if it ever leaks. This is defense-in-depth; the silent | ||
| // exec calls below prevent the credential-bearing command line from being | ||
| // written to stdout (and thus to uploaded artifacts) in the first place. | ||
| core.setSecret(tokenBase64); |
| await overridePersistedExtraheader(SERVER_URL, token); | ||
|
|
||
| // The base64 header is registered for runner-side masking. | ||
| expect(mockCore.setSecret).toHaveBeenCalledWith(tokenBase64); |
There was a problem hiding this comment.
The fix correctly addresses the credential-leakage path. Both the primary mitigation (silent: true on all credential-bearing exec.exec calls) and the defense-in-depth layer (core.setSecret) are in place and well-tested. No blocking issues.> 🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 11.2 AIC · ⌖ 7.51 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Verdict: sound, targeted fix — one non-blocking diagnosability regression noted
The core fix is correct and minimal: silencing the credential-bearing git config exec calls addresses the actual leak vector (the echoed command line in uploaded stdout/stderr artifacts), and core.setSecret adds reasonable defense-in-depth. No other unsilenced code paths in this file carry the same credential (the read paths in getExtraheaderValues, unsetExtraheaderAllScopes, and findIncludedExtraheaderConfigFiles were already silent: true before this PR). Tests are updated consistently.
💡 Themes / notes
- One flagged issue:
silent: truesuppresses stderr as well as stdout, so genuinegit configfailures (lock contention, permission errors) will now only surface as a generic exit-code error rather than git's actual diagnostic text, in both the override and restore paths. This is a reasonable trade-off given the security fix's goal, but alisteners.stderrbuffer could preserve diagnosability without reintroducing the leak — left as a suggestion, not a blocker. - Grumpy-coder sub-agent's other two findings (ordering of
setSecretvs. exec calls in the new test) were reviewed and judged too minor/speculative to warrant a comment —silent: trueis the actual mitigation,setSecretis explicitly documented as secondary defense-in-depth, so ordering has no real security impact. - No correctness, concurrency, or completeness issues found; the fix covers both write (
overridePersistedExtraheader) and restore (restorePersistedExtraheader) paths, matching the PR's stated scope.
| // where it would otherwise be captured in uploaded safe-output artifacts. | ||
| if (cwd) { | ||
| await exec.exec("git", ["config", "--local", "--replace-all", `http.${normalizedUrl}/.extraheader`, authHeader], { cwd }); | ||
| await exec.exec("git", ["config", "--local", "--replace-all", `http.${normalizedUrl}/.extraheader`, authHeader], { cwd, silent: true }); |
There was a problem hiding this comment.
silent: true also suppresses stderr on failure, so a future git config failure here (e.g. lock file contention, permission denied) will surface only a generic "process exited with code N" instead of git's actual error text.
💡 Details
@actions/exec's silent option suppresses both stdout and stderr echoing to the console; none of these four exec.exec calls (this one, line 209, and the two/four calls in restorePersistedExtraheader at lines 250/252/255/257) register listeners.stderr or listeners.errline to capture output before it's silenced. Previously, a failed --replace-all/--add would print git's real stderr (e.g. permission denied, index.lock present) to the workflow log, aiding debugging. Now the thrown error from exec.exec will just be The process exited with code N, and restorePersistedExtraheader's catch block only logs getErrorMessage(err) — which won't contain the underlying git diagnostic. This is an acceptable trade-off for the security fix, but consider adding a listeners.stderr buffer (safe, since it captures raw output for programmatic use rather than echoing it) so the actual git error can still be logged via core.warning on failure, without reintroducing the leak.
let stderrBuf = "";
await exec.exec("git", ["config", ...], {
cwd,
silent: true,
listeners: { stderr: (data) => { stderrBuf += data.toString(); } },
});
// on catch: core.warning(`git config failed: ${stderrBuf}`)Rationale: security fix silences the echoed command line (the actual leak vector), but as written it also silently discards diagnostic stderr that never contained the secret itself — that data loss is avoidable.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (1 new/modified test)
AnalysisThe PR adds one focused security test in
This is a high-value design test: if deleted, the exact credential-leak bug this PR fixes would silently regress with no CI signal. The test verifies a user-visible security invariant, not an internal implementation detail. Several existing tests were also updated to include Inflation: 21 added test lines vs. 16 production lines (1.31×) — within threshold. Verdict
|
PR Triage: #50636Category: bug | Risk: high | Priority Score: 80/100 (impact 35, urgency 25, quality 20)
|
|
@copilot please follow up on the latest review feedback on this PR:
|
…mask command Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Removed |
Safe-output jobs upload raw
process-safe-outputs.stdout/stderr.log. On create/push safe-output paths usingGH_AW_CI_TRIGGER_TOKEN, the credential-bearinggit configcommand line was echoed to stdout by@actions/exec(defaultsilent: false), teed into those artifacts, and reversible viabase64 -d.Changes (
actions/setup/js/git_auth_helpers.cjs){ silent: true }to everyexec.exec("git", ["config", ...])that writes theAuthorization: basic <base64>extraheader, in bothoverridePersistedExtraheader(write path) andrestorePersistedExtraheader(restore path, whosepreviousValuesmay also carry checkout credentials). This is the primary fix: the header no longer reaches stdout or the uploaded files.core.setSecret(tokenBase64). Note this alone is insufficient, since the artifacts are written before runner-side masking; the silent flag is what prevents capture.Tests (
git_auth_helpers.test.cjs)setSecretis called with the base64 value and that allexec.execcalls are silenced.Notes
.cjsfiles are copied to the runner at execution time from theactions/setupcomposite action (not inlined into.lock.yml);make recompileproduces no lock diff, so no consumer regeneration is required in this repo. External consumers pick up the fix once released.