Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 63 additions & 55 deletions actions/setup/js/create_pull_request.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs");
const { buildWorkflowRunUrl } = require("./workflow_metadata_helpers.cjs");
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 { withGitHubHostToken } = require("./git_auth_helpers.cjs");
const { COPILOT_REVIEWER_BOT, FAQ_CREATE_PR_PERMISSIONS_URL } = require("./constants.cjs");
const { isStagedMode } = require("./safe_output_helpers.cjs");
const { normalizeCommitSHA } = require("./commit_sha_helpers.cjs");
Expand Down Expand Up @@ -567,27 +567,18 @@ function enforcePullRequestLimits(patchContent, maxFiles = MAX_FILES) {
* @param {string} [options.repo] - Repository name for the deleteRef call.
* @param {string} [options.remoteTarget] - Remote name or URL used for remote branch existence checks.
* @param {string} [options.remoteToken] - Optional token used for authenticated remote branch checks.
* @param {string} [options.cwd] - Optional working directory for git operations; scopes git config overrides to the correct checkout.
* @returns {Promise<string>} The (possibly renamed) branch name to use going forward.
*/
async function handleRemoteBranchCollision(branchName, preserveBranchName, options = {}) {
const cwd = options.cwd;
let remoteBranchExists = false;
try {
const remoteTarget = options.remoteTarget || "origin";
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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const githubServerUrl = (process.env.GITHUB_SERVER_URL || "https://github.com").replace(/\/+$/, "");
let previousExtraheaders = [];
let overrideApplied = false;
try {
previousExtraheaders = await overridePersistedExtraheader(githubServerUrl, options.remoteToken);
overrideApplied = true;
checkResult = await checkRemoteBranch();
} finally {
if (overrideApplied) {
await restorePersistedExtraheader(githubServerUrl, previousExtraheaders);
}
}
checkResult = await withGitHubHostToken(options.remoteToken, checkRemoteBranch, cwd);
} else {
checkResult = await checkRemoteBranch();
}
Expand Down Expand Up @@ -659,7 +650,7 @@ async function handleRemoteBranchCollision(branchName, preserveBranchName, optio
const oldBranch = branchName;
const renamedBranch = `${branchName}-${extraHex}`;
// Rename local branch
await exec.exec("git", ["branch", "-m", oldBranch, renamedBranch]);
await exec.exec("git", ["branch", "-m", oldBranch, renamedBranch], cwd ? { cwd } : {});
core.info(`Renamed branch to ${renamedBranch}`);
return renamedBranch;
}
Expand Down Expand Up @@ -1638,14 +1629,16 @@ async function main(config = {}) {
// fallback issue can include a compare URL. Genuine push failures are handled in
// the catch block below.
{
try {
const forkCwd = process.cwd();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const runBundlePush = async () => {
branchName = await handleRemoteBranchCollision(branchName, preserveBranchName, {
recreateRef,
githubClient: pushGithubClient,
owner: pushRepoParts.owner,
repo: pushRepoParts.repo,
remoteTarget: pushRemoteUrl || "origin",
remoteToken: headGitHubToken,
cwd: forkCwd,
});

await pushSignedCommits({
Expand All @@ -1654,14 +1647,17 @@ async function main(config = {}) {
repo: pushRepoParts.repo,
branch: branchName,
baseRef: `origin/${baseBranch}`,
cwd: process.cwd(),
cwd: forkCwd,
pushRemoteUrl,
pushToken: headGitHubToken,
signedCommits,
resolvedTemporaryIds,
currentRepo: itemRepo,
validationConfig: config,
});
};
try {
await runBundlePush();
core.info("Changes pushed to branch (from bundle)");

// Count new commits on PR branch relative to base
Expand All @@ -1683,20 +1679,22 @@ async function main(config = {}) {
core.warning("Signed push rejected merge commit topology from bundle; rewriting branch and retrying signed push");
try {
await rewriteBundleBranchAsSingleCommit(baseBranch, exec);
await pushSignedCommits({
githubClient: pushGithubClient,
owner: pushRepoParts.owner,
repo: pushRepoParts.repo,
branch: branchName,
baseRef: `origin/${baseBranch}`,
cwd: process.cwd(),
pushRemoteUrl,
pushToken: headGitHubToken,
signedCommits,
resolvedTemporaryIds,
currentRepo: itemRepo,
validationConfig: config,
});
const runRetryPush = async () =>
pushSignedCommits({
githubClient: pushGithubClient,
owner: pushRepoParts.owner,
repo: pushRepoParts.repo,
branch: branchName,
baseRef: `origin/${baseBranch}`,
cwd: forkCwd,
pushRemoteUrl,
pushToken: headGitHubToken,
signedCommits,
resolvedTemporaryIds,
currentRepo: itemRepo,
validationConfig: config,
});
await runRetryPush();
core.info("Changes pushed to branch after bundle rewrite retry");

try {
Expand Down Expand Up @@ -2025,14 +2023,16 @@ gh pr create --title '${title}' --base ${baseBranch} --head ${getPullRequestHead
// fallback issue can include a compare URL. Genuine push failures are handled in
// the catch block below.
{
try {
const forkCwd = process.cwd();
const runPatchPush = async () => {
branchName = await handleRemoteBranchCollision(branchName, preserveBranchName, {
recreateRef,
githubClient: pushGithubClient,
owner: pushRepoParts.owner,
repo: pushRepoParts.repo,
remoteTarget: pushRemoteUrl || "origin",
remoteToken: headGitHubToken,
cwd: forkCwd,
});

await pushSignedCommits({
Expand All @@ -2041,14 +2041,17 @@ gh pr create --title '${title}' --base ${baseBranch} --head ${getPullRequestHead
repo: pushRepoParts.repo,
branch: branchName,
baseRef: `origin/${baseBranch}`,
cwd: process.cwd(),
cwd: forkCwd,
pushRemoteUrl,
pushToken: headGitHubToken,
signedCommits,
resolvedTemporaryIds,
currentRepo: itemRepo,
validationConfig: config,
});
};
try {
await runPatchPush();
core.info("Changes pushed to branch");

// Count new commits on PR branch relative to base, used to restrict
Expand Down Expand Up @@ -2196,29 +2199,34 @@ ${patchPreview}`;
await exec.exec(`git commit --allow-empty -m "Initialize"`);
core.info("Created empty commit");

branchName = await handleRemoteBranchCollision(branchName, preserveBranchName, {
recreateRef,
githubClient: pushGithubClient,
owner: pushRepoParts.owner,
repo: pushRepoParts.repo,
remoteTarget: pushRemoteUrl || "origin",
remoteToken: headGitHubToken,
});
const forkCwd = process.cwd();
const runEmptyPush = async () => {
branchName = await handleRemoteBranchCollision(branchName, preserveBranchName, {
recreateRef,
githubClient: pushGithubClient,
owner: pushRepoParts.owner,
repo: pushRepoParts.repo,
remoteTarget: pushRemoteUrl || "origin",
remoteToken: headGitHubToken,
cwd: forkCwd,
});

await pushSignedCommits({
githubClient: pushGithubClient,
owner: pushRepoParts.owner,
repo: pushRepoParts.repo,
branch: branchName,
baseRef: `origin/${baseBranch}`,
cwd: process.cwd(),
pushRemoteUrl,
pushToken: headGitHubToken,
signedCommits,
resolvedTemporaryIds,
currentRepo: itemRepo,
validationConfig: config,
});
await pushSignedCommits({
githubClient: pushGithubClient,
owner: pushRepoParts.owner,
repo: pushRepoParts.repo,
branch: branchName,
baseRef: `origin/${baseBranch}`,
cwd: forkCwd,
pushRemoteUrl,
pushToken: headGitHubToken,
signedCommits,
resolvedTemporaryIds,
currentRepo: itemRepo,
validationConfig: config,
});
};
await runEmptyPush();
core.info("Empty branch pushed successfully");

// Count new commits (will be 1 from the Initialize commit)
Expand Down
33 changes: 33 additions & 0 deletions actions/setup/js/git_auth_helpers.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,41 @@ async function restorePersistedExtraheader(serverUrl, previousValues, cwd) {
core.info(`git_auth_helpers: extraheader restored`);
}

/**
* Temporarily override the persisted GitHub extraheader for remote git operations.
*
* Saves the current extraheader value(s), replaces them with the fork token for the
* duration of the callback, then restores the original value(s). This ensures that
* only one Authorization source is active at a time, preventing duplicate-header HTTP 400s
* when a fork token and the checkout-persisted upstream token both apply to the same host.
*
* @template T
* @param {string} token
* @param {() => Promise<T>} callback
* @param {string} [cwd] - Optional working directory; scopes the git config override to the correct checkout
* @returns {Promise<T>}
*/
async function withGitHubHostToken(token, callback, cwd) {
if (!token) {
return callback();
}
const githubServerUrl = (process.env.GITHUB_SERVER_URL || "https://github.com").replace(/\/+$/, "");
let previousExtraheaders = [];
let overrideApplied = false;
try {
previousExtraheaders = await overridePersistedExtraheader(githubServerUrl, token, cwd);
overrideApplied = true;
return await callback();
} finally {
if (overrideApplied) {
await restorePersistedExtraheader(githubServerUrl, previousExtraheaders, cwd);
}
}
}

module.exports = {
checkoutHasPersistedExtraheader,
overridePersistedExtraheader,
restorePersistedExtraheader,
withGitHubHostToken,
};
108 changes: 107 additions & 1 deletion actions/setup/js/git_auth_helpers.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ describe("git_auth_helpers.cjs", () => {
let checkoutHasPersistedExtraheader;
let overridePersistedExtraheader;
let restorePersistedExtraheader;
let withGitHubHostToken;

const SERVER_URL = "https://github.com";
const EXTRAHEADER_KEY = "http.https://github.com/.extraheader";
Expand All @@ -25,7 +26,7 @@ describe("git_auth_helpers.cjs", () => {
global.exec = mockExec;

delete require.cache[require.resolve("./git_auth_helpers.cjs")];
({ checkoutHasPersistedExtraheader, overridePersistedExtraheader, restorePersistedExtraheader } = require("./git_auth_helpers.cjs"));
({ checkoutHasPersistedExtraheader, overridePersistedExtraheader, restorePersistedExtraheader, withGitHubHostToken } = require("./git_auth_helpers.cjs"));
});

afterEach(() => {
Expand Down Expand Up @@ -223,4 +224,109 @@ describe("git_auth_helpers.cjs", () => {
await expect(restorePersistedExtraheader(SERVER_URL, null)).resolves.toBeUndefined();
});
});

// ──────────────────────────────────────────────────────
// withGitHubHostToken
// ──────────────────────────────────────────────────────

describe("withGitHubHostToken", () => {
it("should call the callback without any git config changes when token is empty", async () => {
let callbackCalled = false;
await withGitHubHostToken("", async () => {
callbackCalled = true;
});
expect(callbackCalled).toBe(true);
expect(mockExec.exec).not.toHaveBeenCalled();
});

it("should call the callback without any git config changes when token is undefined", async () => {
let callbackCalled = false;
// @ts-expect-error intentional undefined test
await withGitHubHostToken(undefined, async () => {
callbackCalled = true;
});
expect(callbackCalled).toBe(true);
expect(mockExec.exec).not.toHaveBeenCalled();
});

it("should override extraheader with fork token before calling callback", async () => {
const token = "fork-token";
const expectedHeader = `Authorization: basic ${Buffer.from(`x-access-token:${token}`).toString("base64")}`;
const execCalls = [];

mockExec.exec.mockImplementation(async (_cmd, args) => {
execCalls.push(args);
return 0;
});

await withGitHubHostToken(token, async () => {
// Inside callback the override should already be applied
const overrideCall = execCalls.find(a => a[1] === "--replace-all");
expect(overrideCall).toBeDefined();
expect(overrideCall[3]).toBe(expectedHeader);
});
});

it("should restore the previous extraheader after the callback completes", async () => {
const upstreamHeader = `Authorization: basic ${Buffer.from("x-access-token:upstream").toString("base64")}`;
mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: upstreamHeader + "\n", stderr: "" });

await withGitHubHostToken("fork-token", async () => {});

// After callback, the last --replace-all should restore the upstream header
const replaceAllCalls = mockExec.exec.mock.calls.filter(c => c[1][1] === "--replace-all");
expect(replaceAllCalls.length).toBeGreaterThanOrEqual(2);
const restoreCall = replaceAllCalls[replaceAllCalls.length - 1];
expect(restoreCall[1][3]).toBe(upstreamHeader);
});

it("should restore extraheader even when the callback throws", async () => {
const upstreamHeader = `Authorization: basic ${Buffer.from("x-access-token:upstream").toString("base64")}`;
mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: upstreamHeader + "\n", stderr: "" });

const callbackError = new Error("push failed");
await expect(
withGitHubHostToken("fork-token", async () => {
throw callbackError;
})
).rejects.toThrow(callbackError);

// Restore must still run after the callback throws — the last --replace-all is the restore
const replaceAllCalls = mockExec.exec.mock.calls.filter(c => c[1][1] === "--replace-all");
expect(replaceAllCalls.length).toBeGreaterThanOrEqual(2);
const restoreCall = replaceAllCalls[replaceAllCalls.length - 1];
expect(restoreCall[1][3]).toBe(upstreamHeader);
});

it("should unset extraheader after callback when no previous value existed", async () => {
mockExec.getExecOutput.mockResolvedValue({ exitCode: 1, stdout: "", stderr: "" });

await withGitHubHostToken("fork-token", async () => {});

const unsetCall = mockExec.exec.mock.calls.find(c => c[1][1] === "--unset-all");
expect(unsetCall).toBeDefined();
expect(unsetCall[1][2]).toBe(EXTRAHEADER_KEY);
});

it("should return the callback's return value", async () => {
const result = await withGitHubHostToken("fork-token", async () => "expected-result");
expect(result).toBe("expected-result");
});

it("should pass cwd to overridePersistedExtraheader and restorePersistedExtraheader", async () => {
const cwd = "/some/repo";
const upstreamHeader = `Authorization: basic ${Buffer.from("x-access-token:upstream").toString("base64")}`;
mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: upstreamHeader + "\n", stderr: "" });

await withGitHubHostToken("fork-token", async () => {}, cwd);

// All git config calls should include cwd
for (const call of mockExec.exec.mock.calls) {
expect(call[2]).toMatchObject({ cwd });
}
for (const call of mockExec.getExecOutput.mock.calls) {
expect(call[2]).toMatchObject({ cwd });
}
});
});
});
Loading
Loading