diff --git a/src/github/pr-actions.ts b/src/github/pr-actions.ts index e24a3bc906..eb1a44465f 100644 --- a/src/github/pr-actions.ts +++ b/src/github/pr-actions.ts @@ -119,11 +119,16 @@ export async function getLastCloserLogin(env: Env, installationId: number, repoF const { owner, repo } = splitRepo(repoFullName); const token = await createInstallationToken(env, installationId); const octokit = new Octokit({ auth: token }); - // issue-events are returned oldest-first, so the LAST `closed` entry is the most recent close. - const response = await octokit.request("GET /repos/{owner}/{repo}/issues/{issue_number}/events", { owner, repo, issue_number: issueNumber, per_page: 100 }); - const events = response.data as Array<{ event?: string; actor?: { login?: string | null } | null }>; - const closes = events.filter((entry) => entry.event === "closed"); - return closes.length > 0 ? (closes[closes.length - 1]?.actor?.login ?? null) : null; + let lastCloser: string | null = null; + for (let page = 1; ; page += 1) { + // issue-events are returned oldest-first; walk every page so the final `closed` entry is truly the latest. + const response = await octokit.request("GET /repos/{owner}/{repo}/issues/{issue_number}/events", { owner, repo, issue_number: issueNumber, per_page: 100, page }); + const events = response.data as Array<{ event?: string; actor?: { login?: string | null } | null }>; + for (const entry of events) { + if (entry.event === "closed") lastCloser = entry.actor?.login ?? null; + } + if (events.length < 100) return lastCloser; + } } catch { return null; } diff --git a/src/queue/processors.ts b/src/queue/processors.ts index cafb20547b..9721cf05a2 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -3112,12 +3112,16 @@ async function maybeRecloseDisallowedReopen( if (reopener === botLogin) return false; // the bot's own nightly re-review reopen is allowed const repoOwner = repoFullName.includes("/") ? repoFullName.slice(0, repoFullName.indexOf("/")).toLowerCase() : ""; const admins = (env.ADMIN_GITHUB_LOGINS ?? "").split(",").map((s) => s.trim().toLowerCase()).filter(Boolean); - const isMaintainer = (login: string): boolean => login === repoOwner || admins.includes(login); - if (isMaintainer(reopener)) return false; // owner / admin may reopen + const hasMaintainerPermission = async (login: string): Promise => { + if (login === repoOwner || admins.includes(login)) return true; + const permission = await getRepositoryCollaboratorPermission(env, installationId, repoFullName, login).catch(() => null); + return permission === "admin" || permission === "maintain" || permission === "write"; + }; + if (await hasMaintainerPermission(reopener)) return false; // owner / admin / write collaborators may reopen // A non-maintainer reopened: re-close ONLY if gittensory or a maintainer closed it (one-shot). A contributor // reopening a PR they closed themselves is allowed (fail-open on an unknown closer). const closer = (await getLastCloserLogin(env, installationId, repoFullName, pr.number))?.toLowerCase() ?? null; - if (!closer || !(closer === botLogin || isMaintainer(closer))) return false; + if (!closer || !(closer === botLogin || (await hasMaintainerPermission(closer)))) return false; await createIssueComment( env, installationId, diff --git a/test/unit/github-pr-actions.test.ts b/test/unit/github-pr-actions.test.ts index 0c1654ed38..8f31010e2c 100644 --- a/test/unit/github-pr-actions.test.ts +++ b/test/unit/github-pr-actions.test.ts @@ -1,6 +1,6 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { generateKeyPairSync } from "node:crypto"; -import { closePullRequest, createIssueComment, createPullRequestReview, mergePullRequest } from "../../src/github/pr-actions"; +import { closePullRequest, createIssueComment, createPullRequestReview, getLastCloserLogin, mergePullRequest } from "../../src/github/pr-actions"; import { createTestEnv } from "../helpers/d1"; function envWithKey() { @@ -86,6 +86,47 @@ describe("GitHub PR action primitives (#778)", () => { expect(calls[0]).toMatchObject({ method: "POST", body: { body: "hello" } }); expect(calls[0]?.url).toMatch(/\/repos\/owner\/repo\/issues\/7\/comments$/); }); + + it("walks paginated issue events to find the true most recent closer", async () => { + const calls: string[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + calls.push(url); + if (url.includes("/access_tokens")) return Response.json({ token: "t" }); + if (url.includes("/issues/17/events")) { + const page = new URL(url).searchParams.get("page"); + if (page === "1") { + return Response.json([ + ...Array.from({ length: 99 }, (_, index) => ({ event: "labeled", actor: { login: `labeler-${index}` } })), + { event: "closed", actor: { login: "contributor" } }, + ]); + } + if (page === "2") return Response.json([{ event: "closed", actor: { login: "maintainer" } }]); + } + return new Response("unexpected", { status: 500 }); + }); + + await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 17)).resolves.toBe("maintainer"); + expect(calls.some((url) => url.includes("per_page=100") && url.includes("page=1"))).toBe(true); + expect(calls.some((url) => url.includes("per_page=100") && url.includes("page=2"))).toBe(true); + }); + + it("returns null when the events API throws (catch path)", async () => { + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + if (input.toString().includes("/access_tokens")) return Response.json({ token: "t" }); + throw new Error("network failure"); + }); + await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 18)).resolves.toBeNull(); + }); + + it("records null lastCloser when the closed event has a null actor", async () => { + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + if (input.toString().includes("/access_tokens")) return Response.json({ token: "t" }); + if (input.toString().includes("/issues/19/events")) return Response.json([{ event: "closed", actor: null }]); + return new Response("not found", { status: 404 }); + }); + await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 19)).resolves.toBeNull(); + }); }); function generateRsaPrivateKeyPem(): string { diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 51ad518b62..997b38b014 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -1,4 +1,5 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { generateKeyPairSync } from "node:crypto"; import { clearInstallationTokenCacheForTest } from "../../src/github/app"; import { listCollisionEdges, @@ -6005,3 +6006,152 @@ describe("changedPathsForGuardrail", () => { expect(changedPathsForGuardrail(files)).toEqual(["src/a.ts", "src/b.ts", "src/old-b.ts"]); }); }); + +describe("one-shot reopen prevention", () => { + beforeEach(() => { + clearInstallationTokenCacheForTest(); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("re-closes contributor reopens after a write collaborator closed the PR", async () => { + const calls: Array<{ url: string; method: string }> = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + calls.push({ url, method }); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/collaborators/contributor/permission")) return Response.json({ permission: "read" }); + if (url.endsWith("/collaborators/maintainer/permission")) return Response.json({ permission: "write" }); + if (url.includes("/issues/42/events")) return Response.json([{ event: "closed", actor: { login: "maintainer" } }]); + if (url.endsWith("/issues/42/comments")) return Response.json({ id: 99 }, { status: 201 }); + if (url.endsWith("/pulls/42") && method === "PATCH") return Response.json({ state: "closed" }); + return new Response("not found", { status: 404 }); + }); + + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "reopen-write-collab-close", + eventName: "pull_request", + payload: reopenedPayload("contributor"), + }); + + expect(calls.some((call) => call.url.endsWith("/collaborators/contributor/permission"))).toBe(true); + expect(calls.some((call) => call.url.endsWith("/collaborators/maintainer/permission"))).toBe(true); + expect(calls.some((call) => call.method === "POST" && call.url.endsWith("/issues/42/comments"))).toBe(true); + expect(calls.some((call) => call.method === "PATCH" && call.url.endsWith("/pulls/42"))).toBe(true); + const audit = await env.DB.prepare("select detail from audit_events where event_type = ?").bind("github_app.reopen_reclosed").first<{ detail: string }>(); + expect(audit?.detail).toContain("originally closed by maintainer"); + }); + + it("allows an admin reopener to reopen without reclosing (fast-path hasMaintainerPermission)", async () => { + const calls: Array<{ url: string; method: string }> = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + calls.push({ url, method: init?.method ?? "GET" }); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory", ADMIN_GITHUB_LOGINS: "admin-user" }); + await processJob(env, { type: "github-webhook", deliveryId: "admin-reopen", eventName: "pull_request", payload: reopenedPayload("admin-user") }); + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); + }); + + it("allows reopen when the closer is unknown (null lastCloser)", async () => { + const calls: Array<{ url: string; method: string }> = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + calls.push({ url, method: init?.method ?? "GET" }); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/collaborators/contributor/permission")) return Response.json({ permission: "read" }); + if (url.includes("/issues/42/events")) return Response.json([]); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }); + await processJob(env, { type: "github-webhook", deliveryId: "unknown-closer", eventName: "pull_request", payload: reopenedPayload("contributor") }); + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); + }); + + it("re-closes when the bot itself was the last closer", async () => { + const calls: Array<{ url: string; method: string }> = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + calls.push({ url, method }); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/collaborators/contributor/permission")) return Response.json({ permission: "read" }); + if (url.includes("/issues/42/events")) return Response.json([{ event: "closed", actor: { login: "gittensory[bot]" } }]); + if (url.endsWith("/issues/42/comments")) return Response.json({ id: 99 }, { status: 201 }); + if (url.endsWith("/pulls/42") && method === "PATCH") return Response.json({ state: "closed" }); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }); + await processJob(env, { type: "github-webhook", deliveryId: "bot-closer-reclose", eventName: "pull_request", payload: reopenedPayload("contributor") }); + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(true); + }); + + it("allows reopen when a contributor self-closed (non-maintainer, non-bot closer)", async () => { + const calls: Array<{ url: string; method: string }> = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + calls.push({ url, method: init?.method ?? "GET" }); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/collaborators/contributor/permission")) return Response.json({ permission: "read" }); + if (url.includes("/issues/42/events")) return Response.json([{ event: "closed", actor: { login: "contributor" } }]); + if (url.endsWith("/collaborators/contributor/permission")) return Response.json({ permission: "read" }); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }); + await processJob(env, { type: "github-webhook", deliveryId: "self-close-reopen", eventName: "pull_request", payload: reopenedPayload("contributor") }); + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); + }); + + it("treats permission API errors as non-maintainer (catch path returns null)", async () => { + const calls: Array<{ url: string; method: string }> = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + calls.push({ url, method: init?.method ?? "GET" }); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/collaborators/") && url.endsWith("/permission")) throw new Error("permission API down"); + if (url.includes("/issues/42/events")) return Response.json([{ event: "closed", actor: { login: "contributor" } }]); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }); + await processJob(env, { type: "github-webhook", deliveryId: "perm-api-error", eventName: "pull_request", payload: reopenedPayload("contributor") }); + // permission API threw → null → non-maintainer reopener + non-maintainer closer → no reclose. + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); + }); +}); + +function generateRsaPrivateKeyPem(): string { + const { privateKey } = generateKeyPairSync("rsa", { modulusLength: 2048 }); + return privateKey.export({ type: "pkcs1", format: "pem" }).toString(); +} + +function reopenedPayload(sender: string): any { + return { + action: "reopened", + installation: { id: 123 }, + repository: { id: 1, name: "gittensory", full_name: "JSONbored/gittensory", private: false, default_branch: "main", owner: { login: "JSONbored" } }, + sender: { login: sender, type: "User" }, + pull_request: { + id: 4242, + number: 42, + state: "open", + title: "Fix queued guard", + body: "Fixes the queued guard.", + user: { login: "contributor" }, + head: { sha: "abc123", ref: "fix", repo: { full_name: "contributor/gittensory", owner: { login: "contributor" } } }, + base: { sha: "base123", ref: "main", repo: { full_name: "JSONbored/gittensory", owner: { login: "JSONbored" } } }, + draft: false, + merged: false, + mergeable_state: "clean", + created_at: "2026-05-27T00:00:00Z", + updated_at: "2026-05-27T00:00:00Z", + }, + }; +}