[WIP] Fix duplicate Authorization headers from checkout@v7 - #50377
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #50377 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
Addresses duplicate Authorization headers caused by checkout v7’s included credential files.
Changes:
- Detects temp-directory
includeIf.gitdirconfig files. - Clears extraheaders from included files.
- Adds unit and real-git integration tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/git_auth_helpers.cjs |
Adds included-config discovery and cleanup. |
actions/setup/js/git_auth_helpers.test.cjs |
Adds unit coverage for discovery and safety checks. |
actions/setup/js/git_auth_helpers.integration.test.cjs |
Tests checkout v7-style Git configuration. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Suppressed comments (1)
actions/setup/js/git_auth_helpers.cjs:110
- An unexpected write failure here leaves the included checkout Authorization header active, but the override continues and adds the fork header locally, recreating the duplicate-header failure. Treat this like the global/local unsets and abort before running any authenticated operation.
if (result.exitCode !== 0 && result.exitCode !== 5) {
core.warning(`git_auth_helpers: git config --file ${file} --unset-all ${key} failed (exit ${result.exitCode}): ${result.stderr.trim()}`);
}
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
| for (const file of includedFiles) { | ||
| const result = await exec.getExecOutput("git", ["config", "--file", file, "--unset-all", key], { silent: true, ignoreReturnCode: true }); |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics (50 tests)
(Representative sample — 50 tests analyzed total)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Fix duplicate Authorization headers from checkout@v7
The approach is correct and well-reasoned. The includeIf.gitdir-referenced credential file path is identified by querying the local config, validated against known-safe temp roots (RUNNER_TEMP / OS tmpdir), and then cleared via git config --file ... --unset-all. Integration and unit test coverage is solid.
Two non-blocking suggestions (see inline comments):
require('path')/require('os')inside function body – move these to module-level.- Dangling
includeIf.gitdirdirective – after clearing the credential file, consider also unsetting the referencingincludeIf.gitdir*.pathkey in.git/configto leave the local config clean.
Neither blocks merging; the security-critical path (safe-root guard + exit-code 5 tolerance) is correct.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 28.8 AIC · ⌖ 13.4 AIC · ⊞ 5.4K
| } | ||
|
|
||
| const path = require("path"); | ||
| const os = require("os"); |
There was a problem hiding this comment.
Non-blocking: require('path') and require('os') are called inside the function body on every invocation. These should be module-level requires at the top of the file alongside the existing core and exec references.
const path = require('path');
const os = require('os');While Node.js caches require calls, putting them inside the function obscures the module's dependencies and is inconsistent with the rest of the file.
@copilot please address this.
| } | ||
| for (const file of includedFiles) { | ||
| const result = await exec.getExecOutput("git", ["config", "--file", file, "--unset-all", key], { silent: true, ignoreReturnCode: true }); | ||
| if (result.exitCode !== 0 && result.exitCode !== 5) { |
There was a problem hiding this comment.
Non-blocking – missing cleanup: After unsetting the extraheader key from the included credential file, the includeIf.gitdir:<path>.path entry in .git/config itself is not removed. The dangling include directive is harmless once the file's extraheader is cleared, but leaving it adds noise and could confuse future reads. Consider also running:
await exec.getExecOutput('git', ['config', '--local', '--unset-all', matchingKey], opts);...for each matched includeIf.gitdir*.path key so the local config is left in a clean state.
@copilot please address this.
There was a problem hiding this comment.
Blocking issues found in the new includeIf.gitdir credential-clearing logic — see inline comments.
Themes
- Error-handling asymmetry (high): the scope-unset loop throws on unexpected git exit codes, but the new includeIf-file-unset loop only warns for the same failure class — this can silently leave the checkout@v7 credentials file with a stale
extraheader, reproducing the exact bug this PR fixes. - Safe-root check bypass via symlinks (medium):
path.resolvedoes not dereference symlinks, so a symlink under RUNNER_TEMP pointing outside it defeats the safety check meant to restrict which files get mutated. - Fragile key/value parsing (medium): splitting
--get-regexpoutput on the first space breaks if the gitdir path (embedded in the key) contains a space, silently dropping or mis-locating the credentials file. - Test coverage gap (low-medium): the new failure-warning path for file unsets has no test, unlike its scope-unset counterpart.
Given this is marked WIP, these are all fixable before merge — none require redesigning the approach, but the error-handling asymmetry in particular could quietly undermine the bug fix this PR is meant to deliver.
🔎 Code quality review by PR Code Quality Reviewer · auto · 72 AIC · ⌖ 4.33 AIC · ⊞ 7.9K
Comment /review to run again
| for (const file of includedFiles) { | ||
| const result = await exec.getExecOutput("git", ["config", "--file", file, "--unset-all", key], { silent: true, ignoreReturnCode: true }); | ||
| if (result.exitCode !== 0 && result.exitCode !== 5) { | ||
| core.warning(`git_auth_helpers: git config --file ${file} --unset-all ${key} failed (exit ${result.exitCode}): ${result.stderr.trim()}`); |
There was a problem hiding this comment.
Failures here are only warned about, while the identical failure class in the scope-unset loop above throws — this asymmetry can silently leave the checkout@v7 credentials file with a stale extraheader, reproducing the exact duplicate-header bug this PR is meant to fix.
Details
The --global/--local loop throws on any exit code other than 0 or 5, telling the caller the credential may still be effective. The new includeIf-file loop only logs a warning for the same failure class (permission denied, lock contention, corrupt file, etc.) and continues silently. Since this is exactly the file actions/checkout@v7 uses to persist the credential, a failed unset here is at least as consequential as a failed scope unset, yet the caller gets no signal cleanup did not succeed.
Suggested fix: throw (or aggregate and re-throw) on unexpected exit codes here too, or surface failure via a return value so callers can detect the credential may still be active.
| const filePath = (spaceIdx === -1 ? "" : trimmed.slice(spaceIdx + 1)).trim(); | ||
| if (!filePath) continue; | ||
| const resolved = path.resolve(cwd || process.cwd(), filePath); | ||
| if (safeRoots.some(root => resolved === root || resolved.startsWith(root + path.sep))) { |
There was a problem hiding this comment.
The safe-root check uses path.resolve, which never dereferences symlinks, so a symlink placed inside RUNNER_TEMP that points outside it will pass this check and then have git config --file <path> --unset-all run against it.
Details
path.resolve only normalizes ./.. segments and joins with cwd; it does not call fs.realpath. If an attacker (or a stray leftover from a prior job on a shared/self-hosted runner) can place a symlink under $RUNNER_TEMP pointing at an arbitrary file, resolved.startsWith(root + path.sep) still passes because the symlink's own path is inside the safe root, even though it dereferences elsewhere. The subsequent git config --file <resolved> --unset-all call would then operate on the symlink target, not the safe temp file — exactly the class of write the comment above this function claims to prevent.
Fix: resolve with fs.realpathSync(resolved) (wrapped in try/catch for missing files) before the safe-root comparison, and re-validate the real path against safeRoots.
| for (const line of result.stdout.split("\n")) { | ||
| const trimmed = line.trim(); | ||
| if (!trimmed) continue; | ||
| const spaceIdx = trimmed.indexOf(" "); |
There was a problem hiding this comment.
Splitting on the first space to separate the includeIf key from its file-path value will silently misparse if the gitdir path (embedded in the key itself) contains a space.
Details
git config --get-regexp prints lines as <key> <value>, and here the key is includeif.gitdir:<gitdir-path>.path. If <gitdir-path> contains a space (plausible for checkout paths under custom working directories, or on Windows runners), trimmed.indexOf(" ") finds a space inside the key rather than the true key/value separator, producing a garbage filePath. That garbage path then either fails the safe-root check (silently dropped, leaving the credentials file uncleared) or, worse, resolves to something unexpected.
Consider using git config --get-regexp --null (NUL-separated key/value pairs) to avoid ambiguity, or explicitly locating the last space rather than the first, given .path keys generally have simple values (but not always, if the value itself contains a space).
| }); | ||
|
|
||
| it("should also unset the key from includeIf.gitdir-referenced config files (checkout v7 case)", async () => { | ||
| process.env.RUNNER_TEMP = "/home/runner/work/_temp"; |
There was a problem hiding this comment.
No test covers the file-unset failure path (non-0/non-5 exit code) for the new includeIf-referenced config file loop, even though the equivalent scope-unset failure path is tested just above.
Details
The test at line ~100 (should throw when a scope unset fails...) verifies the throw behavior for --global/--local unsets. There is no equivalent test asserting what happens when git config --file <path> --unset-all returns an exit code other than 0 or 5 (e.g. 4, permission denied) — currently that path only calls core.warning and swallows the error (see the related comment on line 109). Add a test that mocks such a failure and asserts the current (or fixed) behavior, so any future change to this error-handling asymmetry is caught by the suite.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes on two small but real issues.
📋 Key Themes & Highlights
Issues Found
RUNNER_TEMPenv var leak — theunsetExtraheaderAllScopesunit test mutatesprocess.env.RUNNER_TEMPwithouttry/finally; a mid-test throw would corrupt the environment for subsequent tests.- Inline
requirecalls —pathandosare required insidefindIncludedExtraheaderConfigFileson every call instead of at module top level, contrary to the existing file style. - Regex coverage note — minor: the
--get-regexppattern is fine as-is, but a clarifying comment and an extra unit test for thegitdir/slash variant would guard against unintentional future changes.
Positive Highlights
- ✅ Excellent root-cause diagnosis: the bug is the
includeIfindirection that bypasses scope-targeted unsets — not a superficial symptom fix. - ✅ Safety guard (RUNNER_TEMP / OS tmp dir allowlist) is the right defence-in-depth approach.
- ✅ Both unit and integration test coverage, including a real-git reproduction of the checkout v7 scenario.
- ✅ Exit-code 5 ("key not found") is correctly treated as a non-error in
--unset-allcalls.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 40.3 AIC · ⌖ 7.49 AIC · ⊞ 7.1K
Comment /matt to run again
| }); | ||
|
|
||
| it("should also unset the key from includeIf.gitdir-referenced config files (checkout v7 case)", async () => { | ||
| process.env.RUNNER_TEMP = "/home/runner/work/_temp"; |
There was a problem hiding this comment.
[/tdd] RUNNER_TEMP is mutated without try/finally — if the test throws before the delete on line 120, it leaks into subsequent tests.
💡 Suggested fix
Save and restore RUNNER_TEMP with the same try/finally pattern used in the findIncludedExtraheaderConfigFiles describe block:
const orig = process.env.RUNNER_TEMP;
try {
process.env.RUNNER_TEMP = "/home/runner/work/_temp";
// ... rest of test body ...
} finally {
if (orig === undefined) delete process.env.RUNNER_TEMP;
else process.env.RUNNER_TEMP = orig;
}@copilot please address this.
| return []; | ||
| } | ||
|
|
||
| const path = require("path"); |
There was a problem hiding this comment.
[/codebase-design] path and os are required inside the function on every call. These are core Node.js built-ins that should be required at the top of the file alongside the existing core and exec references.
💡 Suggested fix
Move to the top of the file with the other requires:
const path = require("path");
const os = require("os");Then remove the inline require calls from findIncludedExtraheaderConfigFiles.
@copilot please address this.
| */ | ||
| async function findIncludedExtraheaderConfigFiles(cwd) { | ||
| const opts = { silent: true, ignoreReturnCode: true, ...(cwd ? { cwd } : {}) }; | ||
| const result = await exec.getExecOutput("git", ["config", "--local", "--get-regexp", "^includeif\\.gitdir.*\\.path$"], opts); |
There was a problem hiding this comment.
[/diagnosing-bugs] The regex ^includeif\.gitdir.*\.path$ is case-insensitive in git config output (git lowercases all section names in --get-regexp output), but the pattern itself uses a literal lowercase match — which is fine for the current output format. However, it is worth a brief comment explaining why lowercase is sufficient so a future reader does not "fix" it unnecessarily.
Also consider whether includeIf.gitdir/ (forward-slash variant, which git also supports) is covered by the .* wildcard — it is, but a unit test for that path value would make this explicit.
@copilot please address this.
|
@copilot Please address the remaining blocking review feedback on this PR, rerun the failing checks, refresh the branch if needed, then run the Open review feedback to close out:
Failed checks:
Please resolve addressed review threads once the reviewer feedback has been handled.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all review feedback in commit
|
|
@copilot Please address the remaining review feedback on this PR, refresh the branch if GitHub allows it, and then run the Open review feedback to close out (newest first):
|
findIncludedExtraheaderConfigFilesto detect config files referenced viaincludeIf.gitdir*.path(as written byactions/checkout@v7)unsetExtraheaderAllScopesto also unset the extraheader key in those included files, with a safety check restricting file targets toRUNNER_TEMP/OS tmp dirincludeIf.gitdirscenario and confirming the value is cleared