Skip to content

fix(review): verify PR head before E2E test commits #4359

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JSONbored merged 2 commits into from
Jul 10, 2026
Merged

fix(review): verify PR head before E2E test commits #4359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
75fc37e
fix(review): verify PR head before E2E test commits
JSONbored Jul 9, 2026
5bd8615
Merge branch 'main' into codex/propose-fix-for-fork-pr-vulnerability
JSONbored Jul 10, 2026
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
9 changes: 9 additions & 0 deletions src/github/e2e-test-commit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,15 @@ export async function commitE2eTestToPrBranch(
return await withInstallationTokenRetry(env, args.installationId, async (token) => {
const octokit = makeInstallationOctokit(env, token, args.mode, githubRateLimitAdmissionKeyForInstallation(args.installationId));

const livePr = await octokit.request("GET /repos/{owner}/{repo}/pulls/{pull_number}", { owner, repo, pull_number: args.prNumber });
const liveHead = (livePr.data as { head?: { ref?: string | null; sha?: string | null; repo?: { full_name?: string | null } | null } }).head;
if (liveHead?.repo?.full_name !== args.repoFullName) {
return { status: "declined", reason: "commit delivery is only supported for same-repository PR branches" };
}
if (liveHead.ref !== args.headRef || liveHead.sha !== args.headSha) {
return { status: "declined", reason: "the live PR head no longer matches the cached branch/commit — try the command again" };
}

const headCommit = await octokit.request("GET /repos/{owner}/{repo}/git/commits/{commit_sha}", { owner, repo, commit_sha: args.headSha });
const baseTreeSha = (headCommit.data as { tree: { sha: string } }).tree.sha;

Expand Down
42 changes: 40 additions & 2 deletions test/unit/e2e-test-commit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ describe("commitE2eTestToPrBranch (#4197)", () => {
const url = input.toString();
if (TOKEN_URL.test(url)) return Response.json({ token: "t" });
const method = init?.method ?? "GET";
if (url.endsWith("/pulls/42") && method === "GET") return Response.json({ head: { ref: "feature/my-branch", sha: "head-commit-sha", repo: { full_name: REPO } } });
calls.push({ method, url, body: init?.body ? JSON.parse(String(init.body)) : {} });
if (url.endsWith("/git/commits/head-commit-sha") && method === "GET") return Response.json({ tree: { sha: "base-tree-sha" } });
if (url.endsWith("/git/trees") && method === "POST") return Response.json({ sha: "new-tree-sha" });
Expand Down Expand Up @@ -86,6 +87,7 @@ describe("commitE2eTestToPrBranch (#4197)", () => {
const url = input.toString();
if (TOKEN_URL.test(url)) return Response.json({ token: "t" });
const method = init?.method ?? "GET";
if (url.endsWith("/pulls/42") && method === "GET") return Response.json({ head: { ref: "feature/my-branch", sha: "head-commit-sha", repo: { full_name: REPO } } });
if (url.endsWith("/git/commits/head-commit-sha") && method === "GET") return Response.json({ tree: { sha: "base-tree-sha" } });
if (url.endsWith("/git/trees") && method === "POST") {
treeBody = init?.body ? JSON.parse(String(init.body)) : {};
Expand All @@ -101,12 +103,47 @@ describe("commitE2eTestToPrBranch (#4197)", () => {
expect((treeBody.tree as Array<{ path: string }>)[0]?.path).toBe("test/e2e/custom.spec.ts");
});

it("declines fork PR commit delivery before any git write", async () => {
const env = envWithKey();
const calls: Array<{ method: string; url: string }> = [];
vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {
const url = input.toString();
if (TOKEN_URL.test(url)) return Response.json({ token: "t" });
const method = init?.method ?? "GET";
calls.push({ method, url });
if (url.endsWith("/pulls/42") && method === "GET") return Response.json({ head: { ref: "feature/my-branch", sha: "head-commit-sha", repo: { full_name: "fork/widgets" } } });
return new Response("unexpected", { status: 500 });
});

const result = await commitE2eTestToPrBranch(env, baseArgs);

expect(result).toEqual({ status: "declined", reason: "commit delivery is only supported for same-repository PR branches" });
expect(calls.some((call) => call.url.includes("/git/"))).toBe(false);
});

it("declines when the live PR head no longer matches the cached branch and sha", async () => {
const env = envWithKey();
vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {
const url = input.toString();
if (TOKEN_URL.test(url)) return Response.json({ token: "t" });
const method = init?.method ?? "GET";
if (url.endsWith("/pulls/42") && method === "GET") return Response.json({ head: { ref: "feature/new", sha: "new-head-sha", repo: { full_name: REPO } } });
return new Response("unexpected", { status: 500 });
});

const result = await commitE2eTestToPrBranch(env, baseArgs);

expect(result).toMatchObject({ status: "declined" });
if (result.status !== "declined") throw new Error("unreachable");
expect(result.reason).toContain("live PR head no longer matches");
});

it("declines with a clear reason on a 403/404 (no write access -- fork without maintainer edits)", async () => {
const env = envWithKey();
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
const url = input.toString();
if (TOKEN_URL.test(url)) return Response.json({ token: "t" });
if (url.endsWith("/git/commits/head-commit-sha")) return new Response("forbidden", { status: 403 });
if (url.endsWith("/pulls/42")) return new Response("forbidden", { status: 403 });
return new Response("unexpected", { status: 500 });
});

Expand All @@ -122,6 +159,7 @@ describe("commitE2eTestToPrBranch (#4197)", () => {
const url = input.toString();
if (TOKEN_URL.test(url)) return Response.json({ token: "t" });
const method = init?.method ?? "GET";
if (url.endsWith("/pulls/42") && method === "GET") return Response.json({ head: { ref: "feature/my-branch", sha: "head-commit-sha", repo: { full_name: REPO } } });
if (url.endsWith("/git/commits/head-commit-sha") && method === "GET") return Response.json({ tree: { sha: "base-tree-sha" } });
if (url.endsWith("/git/trees") && method === "POST") return Response.json({ sha: "new-tree-sha" });
if (url.endsWith("/git/commits") && method === "POST") return Response.json({ sha: "new-commit-sha" });
Expand All @@ -140,7 +178,7 @@ describe("commitE2eTestToPrBranch (#4197)", () => {
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
const url = input.toString();
if (TOKEN_URL.test(url)) return Response.json({ token: "t" });
if (url.endsWith("/git/commits/head-commit-sha")) return new Response("server exploded", { status: 500 });
if (url.endsWith("/pulls/42")) return new Response("server exploded", { status: 500 });
return new Response("unexpected", { status: 500 });
});

Expand Down
6 changes: 4 additions & 2 deletions test/unit/queue.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25999,6 +25999,7 @@ describe("queue processors", () => {
const method = init?.method ?? "GET";
if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" });
if (url.includes("/collaborators/maintainer/permission")) return Response.json({ permission: "admin" });
if (url.endsWith("/pulls/4207") && method === "GET") return Response.json({ head: { ref: "feature/checkout-retry", sha: "commit-ok-head-sha", repo: { full_name: repoFullName } } });
if (url.endsWith("/git/commits/commit-ok-head-sha") && method === "GET") return Response.json({ tree: { sha: "base-tree-sha" } });
if (url.endsWith("/git/trees") && method === "POST") return Response.json({ sha: "new-tree-sha" });
if (url.endsWith("/git/commits") && method === "POST") return Response.json({ sha: "committed-sha-123" });
Expand Down Expand Up @@ -26066,7 +26067,7 @@ describe("queue processors", () => {
const method = init?.method ?? "GET";
if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" });
if (url.includes("/collaborators/maintainer/permission")) return Response.json({ permission: "admin" });
if (url.endsWith("/git/commits/declined-head-sha") && method === "GET") return new Response("forbidden", { status: 403 });
if (url.endsWith("/pulls/4209") && method === "GET") return new Response("forbidden", { status: 403 });
if (url.includes("/issues/4209/comments") && method === "GET") return Response.json([]);
if (url.includes("/issues/4209/comments") && method === "POST") { postedBody = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? ""); return Response.json({ id: 42090 }); }
return new Response("not found", { status: 404 });
Expand Down Expand Up @@ -26132,7 +26133,7 @@ describe("queue processors", () => {
if (url.includes("/collaborators/maintainer/permission")) return Response.json({ permission: "admin" });
// Neither a 403/404 (no write access) nor a 422/409 (branch moved) -- a genuinely unexpected 500,
// which commitE2eTestToPrBranch maps to status: "error" rather than "declined".
if (url.endsWith("/git/commits/error-mapped-head-sha") && method === "GET") return new Response("server exploded", { status: 500 });
if (url.endsWith("/pulls/4213") && method === "GET") return new Response("server exploded", { status: 500 });
if (url.includes("/issues/4213/comments") && method === "GET") return Response.json([]);
if (url.includes("/issues/4213/comments") && method === "POST") { postedBody = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? ""); return Response.json({ id: 42130 }); }
return new Response("not found", { status: 404 });
Expand Down Expand Up @@ -26170,6 +26171,7 @@ describe("queue processors", () => {
const method = init?.method ?? "GET";
if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" });
if (url.includes("/collaborators/maintainer/permission")) return Response.json({ permission: "admin" });
if (url.endsWith("/pulls/4214") && method === "GET") return Response.json({ head: { ref: "feature/checkout-retry", sha: "no-author-head-sha", repo: { full_name: repoFullName } } });
if (url.endsWith("/git/commits/no-author-head-sha") && method === "GET") return Response.json({ tree: { sha: "base-tree-sha" } });
if (url.endsWith("/git/trees") && method === "POST") return Response.json({ sha: "new-tree-sha" });
if (url.endsWith("/git/commits") && method === "POST") return Response.json({ sha: "no-author-commit-sha" });
Expand Down