diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 92e3ed89cd..f46a174ddc 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -11254,17 +11254,10 @@ async function recloseDisallowedReopenIfNeeded( pr.number, ); const latestReopenerLogin = latestReopener.login?.toLowerCase() ?? null; - // A bounded scan that did NOT cover every page and found no reopened event is attacker-controllable: the - // original contributor can pad newer issue events until their reopen falls outside the inspected window. Treat - // only a visible different reopener (or a fully covered no-match) as superseding; an incomplete unknown result - // keeps enforcing the one-shot close. A read error remains distinct because it proves no timeline facts. (#2369) - const latestReopenerUnknownInPartialWindow = - latestReopenerLogin === null && - !latestReopener.coveredAllPages && - !latestReopener.errored; - const reopenerSuperseded = - latestReopener.errored || - (!latestReopenerUnknownInPartialWindow && latestReopenerLogin !== reopener); + // A bounded scan that did NOT cover every page and found no reopened event is attacker-controllable: padding can + // hide either the original contributor reopen or a later maintainer-authorized reopen. If the current reopener is + // not visible and confirmed to still be the webhook sender, fail safe by denying the GitHub write. (#2369) + const reopenerSuperseded = latestReopener.errored || latestReopenerLogin !== reopener; if (reopenerSuperseded) { await recordAuditEvent(env, { eventType: "github_app.reopen_reclosed", diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 4eaa7612a0..5f77aad56d 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -23132,7 +23132,7 @@ describe("one-shot reopen prevention", () => { expect(audit?.outcome).toBe("completed"); }); - it("REGRESSION: re-closes when padding hides the contributor reopen beyond the inspected event window", async () => { + it("REGRESSION: denies when padding makes the latest reopener ambiguous beyond the inspected event window", async () => { const calls: Array<{ url: string; method: string }> = []; const eventPages: number[] = []; vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { @@ -23163,11 +23163,11 @@ describe("one-shot reopen prevention", () => { expect(eventPages).toContain(22); expect(eventPages).not.toContain(12); - expect(calls.some((c) => c.method === "POST" && c.url.endsWith("/issues/42/comments"))).toBe(true); - expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(true); + expect(calls.some((c) => c.method === "POST" && c.url.endsWith("/issues/42/comments"))).toBe(false); + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); const audit = await env.DB.prepare("select outcome, detail from audit_events where event_type = ?").bind("github_app.reopen_reclosed").first<{ outcome: string; detail: string }>(); - expect(audit?.outcome).toBe("completed"); - expect(audit?.detail).toContain("re-closed a disallowed reopen by contributor"); + expect(audit?.outcome).toBe("denied"); + expect(audit?.detail).toContain("the current reopener is now unknown, not contributor"); }); it("REGRESSION: fails CLOSED (denies the re-close) when the reopener-timeline read errors (#2369)", async () => {