fix: wrap fork push sequences in withGitHubHostToken to prevent duplicate Authorization (HTTP 400)#48595
Conversation
…TTP 400) - Move withGitHubHostToken from push_to_pull_request_branch.cjs into git_auth_helpers.cjs and export it, so it can be shared - Update push_to_pull_request_branch.cjs to import withGitHubHostToken from git_auth_helpers.cjs instead of defining it locally - Add cwd support to handleRemoteBranchCollision in create_pull_request.cjs so git ls-remote and git branch -m use the correct working directory - Wrap all three fork push sequences (bundle, patch, allow-empty) in withGitHubHostToken so only the fork token is active for the entire fork operation, preventing duplicate Authorization headers (HTTP 400) - Add withGitHubHostToken unit tests to git_auth_helpers.test.cjs Fixes #48414 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #48595 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Fixes #48414 by centralizing temporary GitHub credential overrides and applying them to fork-backed PR pushes.
Changes:
- Extracts and tests
withGitHubHostToken. - Adds working-directory propagation for collision handling.
- Wraps bundle, patch, retry, and empty fork pushes with fork credentials.
Show a summary per file
| File | Description |
|---|---|
create_pull_request.cjs |
Applies fork authentication to PR push paths. |
git_auth_helpers.cjs |
Exports the shared credential override helper. |
git_auth_helpers.test.cjs |
Tests override, restoration, errors, and cwd. |
push_to_pull_request_branch.cjs |
Uses the shared helper. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (3)
actions/setup/js/create_pull_request.cjs:1697
- The retry has the same checkout-wide credential regression:
pushSignedCommitscan fetch missing objects from upstreamorigin, but this wrapper makes those requests use the fork-only head token. Invoke the retry directly; the fork-targetedls-remoteand push operations already applypushTokeninternally.
await (headGitHubToken && pushRemoteUrl ? withGitHubHostToken(headGitHubToken, runRetryPush, forkCwd) : runRetryPush());
actions/setup/js/create_pull_request.cjs:2054
- Wrapping all of
runPatchPushreplaces the upstream checkout credential whilepushSignedCommitsmay perform on-demand or recovery fetches againstorigin. Separate fork credentials commonly cannot read a private upstream repository, so partial-clone patch pushes can now fail before reaching the fork. Leave the outer credential unchanged and rely on the already-scoped fork wrappers in the collision check andpushSignedCommits.
await (headGitHubToken && pushRemoteUrl ? withGitHubHostToken(headGitHubToken, runPatchPush, forkCwd) : runPatchPush());
actions/setup/js/create_pull_request.cjs:2229
- This also applies the fork token to every Git operation performed by
pushSignedCommits, including possible partial-clone object fetches from upstreamorigin. A fork-scoped token is not a substitute for the target repository's persisted credential; call the sequence directly and let the fork-specific operations applypushTokenlocally.
await (headGitHubToken && pushRemoteUrl ? withGitHubHostToken(headGitHubToken, runEmptyPush, forkCwd) : runEmptyPush());
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Medium
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
The refactor looks correct and well-tested. withGitHubHostToken is properly extracted into git_auth_helpers.cjs and all three fork push paths (bundle, patch, allow-empty) are wrapped to eliminate the duplicate-Authorization HTTP 400.
Minor observation (non-blocking): In the bundle, patch, and allow-empty paths, handleRemoteBranchCollision is called with remoteToken: headGitHubToken inside the runBundlePush/runPatchPush/runEmptyPush closure that is already wrapped by an outer withGitHubHostToken(headGitHubToken, ...). The inner override saves the currently-active fork header, replaces it with the same token, and restores it — functionally a no-op but slightly redundant. Not blocking.
The 8 new unit tests cover all important cases: no-op on empty/undefined token, override before callback, restore after success and on throw, return value propagation, cwd propagation.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 19.8 AIC · ⌖ 5.26 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report
📊 Metrics (8 tests)
Per-Test Classification
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 80/100. 25% implementation tests (threshold: 30%). 75% design-level tests verify critical behavioral contracts (override sequencing, error recovery, cwd scoping). Test inflation ratio of 3.24:1 is justified by function complexity.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — requesting changes for dead imports left behind by the refactor.
📋 Key Themes & Highlights
Issues Found
- Dead imports (2 files): After moving
withGitHubHostTokenintogit_auth_helpers.cjs, bothcreate_pull_request.cjsandpush_to_pull_request_branch.cjsstill importoverridePersistedExtraheaderandrestorePersistedExtraheadereven though they are no longer used directly. These should be removed.
Positive Highlights
- ✅ Clean extraction of
withGitHubHostTokeninto the shared helper — good single-responsibility improvement - ✅ All three fork push paths (bundle, patch, allow-empty) are consistently wrapped — no partial coverage
- ✅ Excellent test suite: 8 targeted tests covering no-op, override, restore-on-throw, unset, return value, and
cwdpropagation - ✅ The
forkCwd = process.cwd()capture before entering the async callback prevents any potential cwd drift - ✅ JSDoc updated on
handleRemoteBranchCollisionto document the newcwdoption
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 27.8 AIC · ⌖ 4.74 AIC · ⊞ 6.7K
Comment /matt to run again
| const { checkFileProtection, checkFileProtectionPostApply } = require("./manifest_file_helpers.cjs"); | ||
| const { renderTemplateFromFile, renderFilesList, buildProtectedFileList, getPromptPath } = require("./messages_core.cjs"); | ||
| const { overridePersistedExtraheader, restorePersistedExtraheader } = require("./git_auth_helpers.cjs"); | ||
| const { overridePersistedExtraheader, restorePersistedExtraheader, withGitHubHostToken } = require("./git_auth_helpers.cjs"); |
There was a problem hiding this comment.
[/diagnosing-bugs] Same dead-import issue: overridePersistedExtraheader and restorePersistedExtraheader are imported but no longer called in this file after the refactor.
Consider trimming to:
const { withGitHubHostToken } = require('./git_auth_helpers.cjs');@copilot please address this.
| const { buildWorkflowRunUrl } = require("./workflow_metadata_helpers.cjs"); | ||
| const { renderTemplateFromFile, buildProtectedFileList, getPromptPath } = require("./messages_core.cjs"); | ||
| const { overridePersistedExtraheader, restorePersistedExtraheader } = require("./git_auth_helpers.cjs"); | ||
| const { overridePersistedExtraheader, restorePersistedExtraheader, withGitHubHostToken } = require("./git_auth_helpers.cjs"); |
There was a problem hiding this comment.
[/diagnosing-bugs] Dead imports: overridePersistedExtraheader and restorePersistedExtraheader are still imported but no longer used after the refactor — this will cause linter warnings and mislead future readers. Consider reducing to just withGitHubHostToken.
@copilot please address this.
|
@copilot run pr-finisher skill |
There was a problem hiding this comment.
COMMENT — non-blocking maintainability concerns
The core fix (wrapping fork push sequences in withGitHubHostToken to avoid duplicate Authorization headers) is logically sound: it saves/restores the extraheader around each push callback, cwd is correctly threaded through to scope git config changes, and item processing is strictly sequential (for...of loops), so there is no concurrent-access risk to the mutated git config state. Tests for the new shared helper look reasonable.
Themes from this review
- Nested-wrap fragility:
handleRemoteBranchCollision's internalwithGitHubHostToken(options.remoteToken, ...)call is now always nested inside an outerwithGitHubHostToken(headGitHubToken, ...)wrap at every call site, and both currently use the same token. This coincidence makes the nesting a safe no-op today, but it is an unenforced invariant with no test coverage — a future change that diverges the two tokens would silently misapply/restore the wrong Authorization header. See inline comment for details and suggested hardening. - Duplicated wrapping boilerplate: the same
forkCwd+ closure + conditionalwithGitHubHostTokenpattern is copy-pasted across four call sites (bundle push, bundle retry-after-rewrite, patch push, empty-commit push). This is exactly the kind of duplication that lets fixes land inconsistently, as nearly happened with the retry path in this same PR. A small shared helper would reduce that risk going forward.
A grumpy-coder sub-agent independently mined for issues; its findings on nesting and duplication were confirmed and incorporated. Its concern about concurrent parallel processing was checked directly against the codebase and ruled out — create_pull_request.cjs processes items via sequential for loops in safe_output_handler_manager.cjs, so there is no concurrent mutation of the shared git config extraheader.
No correctness, security, or crash-level bugs were found in the diff; the two inline comments are recommendations to harden an implicit invariant and reduce duplication, not blockers.
🔎 Code quality review by PR Code Quality Reviewer · sonnet50 · 162.1 AIC · ⌖ 4.64 AIC · ⊞ 7.4K
Comment /review to run again
| const checkRemoteBranch = async () => exec.getExecOutput("git", ["ls-remote", "--heads", remoteTarget, branchName]); | ||
| const checkRemoteBranch = async () => exec.getExecOutput("git", ["ls-remote", "--heads", remoteTarget, branchName], cwd ? { cwd } : {}); | ||
| let checkResult; | ||
| if (options.remoteToken) { |
There was a problem hiding this comment.
The inner withGitHubHostToken call here is nested inside the outer wrap added around runBundlePush/runPatchPush/runEmptyPush, and both currently use the identical token — but nothing enforces that invariant.
💡 Why this matters
handleRemoteBranchCollision is now always invoked from inside an outer withGitHubHostToken(headGitHubToken, ...) block (see lines ~1632-1660, 2026-2054, 2202-2229). Since options.remoteToken is always headGitHubToken — the same value the outer wrap already applied — this inner call reads the outer override as its "previous" value and then restores that same override value when it finishes. That happens to be a harmless no-op today only because the two tokens coincide.
If a future change ever passes a different token into remoteToken (e.g. a per-call scoped token) while the outer wrap still uses headGitHubToken, the inner restorePersistedExtraheader would put the outer's overridden header back rather than the true pre-override upstream header, and the subsequent push (which runs after this inner call returns, still inside the outer callback) would silently run under the wrong or already-restored auth state — reintroducing the exact duplicate/incorrect-Authorization class of bug this PR fixes.
Suggested fix: either (a) have handleRemoteBranchCollision skip its own token override entirely when called from within an already-active outer wrap (e.g. accept a flag or detect via a shared "active token" marker), or (b) add an explicit code comment + a unit test asserting nested calls with the same token are safe, so a future refactor that diverges the tokens fails a test instead of silently breaking auth.
| // the catch block below. | ||
| { | ||
| try { | ||
| const forkCwd = process.cwd(); |
There was a problem hiding this comment.
This wrap-a-closure-in-withGitHubHostToken pattern is copy-pasted three times (bundle, patch, and empty-commit push paths), increasing the odds a future fix lands in only one of them.
💡 Why this matters
Each of the three push blocks (~1632-1660, 2026-2054, 2202-2229) repeats the same shape: capture forkCwd = process.cwd(), build an async closure, then conditionally call withGitHubHostToken(headGitHubToken, closure, forkCwd) vs calling the closure directly when there is no fork token/remote. The retry-after-rewrite path even needed a fourth near-duplicate of this pattern added separately (line ~1697), which is exactly the kind of drift that duplicated logic invites — it would be easy for a future auth fix or logging addition to be applied to one call site and missed in the others.
Suggested refactor: extract a small helper, e.g. runForkPush(token, remoteUrl, cwd, fn) that encapsulates the token && remoteUrl ? withGitHubHostToken(token, fn, cwd) : fn() ternary, and call it from all four sites. This removes ~4 lines of duplication per call site and centralizes the wrapping decision so any future change to the gating condition only needs to happen once.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented a pr-finisher pass in 70365c6: removed the checkout-wide fork-token wrappers around create_pull_request push sequences and trimmed stale auth-helper imports; local validation passed ( |
Fork-backed
create_pull_requestfails with HTTP 400 on every git operation againsthead-repowhenhead-github-appsupplies a separate fork token. The checkout-persisted upstream extraheader and the fork token both reach the fork remote, causing git to send duplicateAuthorizationheaders.This is the
create_pull_requestequivalent of thepush_to_pull_request_branchfix in #40280.Changes
git_auth_helpers.cjs— promotewithGitHubHostTokento shared helperwithGitHubHostToken(token, callback, cwd)out ofpush_to_pull_request_branch.cjsand intogit_auth_helpers.cjsas an exported functionAuthorizationsource is active at a timepush_to_pull_request_branch.cjs— no behavior changewithGitHubHostTokenfromgit_auth_helpers.cjs; removes the now-redundant local definitioncreate_pull_request.cjs— fix duplicate-auth on fork pushhandleRemoteBranchCollisiongains acwdoption; all git commands (ls-remote,branch -m) and the credential override now use the caller-supplied working directoryhandleRemoteBranchCollisionto usewithGitHubHostTokenwithGitHubHostToken(headGitHubToken, …, forkCwd)so only the fork token is active for the entire sequence:git_auth_helpers.test.cjs— unit tests forwithGitHubHostTokencwdpropagation