From acf825973a2ba9d027d05899182dc07f3a4dbab3 Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Fri, 24 Jul 2026 19:33:16 +0800 Subject: [PATCH] fix(auth): stop auditing routine device-flow polls as denied, mark access_denied correctly (#8378) --- src/auth/github-oauth.ts | 17 ++++++++++++----- test/unit/auth.test.ts | 28 ++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/src/auth/github-oauth.ts b/src/auth/github-oauth.ts index 5072a72c57..99d129a6e4 100644 --- a/src/auth/github-oauth.ts +++ b/src/auth/github-oauth.ts @@ -83,11 +83,18 @@ export async function pollGitHubDeviceFlow(env: Env, deviceCode: string) { }); const tokenPayload = (await tokenResponse.json().catch(() => ({}))) as GitHubAccessTokenResponse; if ("error" in tokenPayload) { - await recordAuditEvent(env, { - eventType: "auth.github_device_poll", - outcome: tokenPayload.error === "authorization_pending" || tokenPayload.error === "slow_down" ? "denied" : "error", - detail: tokenPayload.error, - }); + // #8378: `authorization_pending`/`slow_down` are RFC 8628's routine "still waiting on the browser step" + // responses — a single successful login polls this dozens of times — so they are not audit-worthy at all + // (auditing them as `denied` buried the one row that matters under ~180 rows of in-progress noise). + // `access_denied` IS the genuine user rejection and is the only `denied` here, matching how the sibling + // web-OAuth callback (routes.ts) treats an OAuth `error` param; every other terminal code stays `error`. + if (tokenPayload.error !== "authorization_pending" && tokenPayload.error !== "slow_down") { + await recordAuditEvent(env, { + eventType: "auth.github_device_poll", + outcome: tokenPayload.error === "access_denied" ? "denied" : "error", + detail: tokenPayload.error, + }); + } return { status: tokenPayload.error, message: tokenPayload.error_description, diff --git a/test/unit/auth.test.ts b/test/unit/auth.test.ts index df86220c3f..0045830a06 100644 --- a/test/unit/auth.test.ts +++ b/test/unit/auth.test.ts @@ -1199,6 +1199,34 @@ describe("private-beta auth and rate limiting", () => { await expect(pollGitHubDeviceFlow(createTestEnv(), "device-code")).rejects.toThrow(/not_configured/); }); + // #8378: routine polling used to write an `outcome: "denied"` audit row on EVERY poll (~180 per successful + // login), while the one genuine user rejection (`access_denied`) was filed as a generic `error`. + it("REGRESSION (#8378): audits only terminal device-poll errors, with access_denied as the sole 'denied'", async () => { + const env = createTestEnv({ GITHUB_OAUTH_CLIENT_ID: "client-id" }); + const pollWith = async (error: string) => { + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => + input.toString().includes("access_token") ? Response.json({ error, error_description: `${error} desc` }) : Response.json({}), + ); + return pollGitHubDeviceFlow(env, "device-code"); + }; + const auditRows = async () => + (await env.DB.prepare("select detail, outcome from audit_events where event_type = ?").bind("auth.github_device_poll").all<{ detail: string; outcome: string }>()).results; + + // Routine, non-terminal polling states write NO audit row at all... + for (const routine of ["authorization_pending", "slow_down"]) { + await expect(pollWith(routine)).resolves.toMatchObject({ status: routine, message: `${routine} desc` }); + } + expect(await auditRows()).toEqual([]); + + // ...the user actually declining is the one genuine `denied`... + await expect(pollWith("access_denied")).resolves.toMatchObject({ status: "access_denied", message: "access_denied desc" }); + expect(await auditRows()).toEqual([{ detail: "access_denied", outcome: "denied" }]); + + // ...and every other terminal code stays `error`, unchanged. + await expect(pollWith("expired_token")).resolves.toMatchObject({ status: "expired_token" }); + expect(await auditRows()).toContainEqual({ detail: "expired_token", outcome: "error" }); + }); + it("rejects invalid GitHub tokens when creating sessions", async () => { const env = createTestEnv(); vi.stubGlobal("fetch", async () => Response.json({ message: "bad credentials" }, { status: 401 }));