From 87e570abfe63b41defc848b7ded45fbca96a6d37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Mon, 15 Jun 2026 00:09:22 +0200 Subject: [PATCH] fix(polar): lift invalid_grant to reauth and harden the credentials surface classifyPolarError now lifts a 400 carrying invalid_grant on the token endpoint from persistent to reauth_required, mirroring Oura via the shared isOAuthInvalidGrant helper. A stale or replayed authorization code at connect time now surfaces the reconnect prompt instead of silently parking after 24 h. The BYO-credentials DELETE now audit-logs and parks the integration ledger at disconnected for parity with the dedicated disconnect route, and the PUT audit-logs the credential write. Refresh the connect-route comment that still described Polar as env-only OAuth; it has resolved BYO-first since v1.17.1. Add the invalid_grant classification test plus the previously-missing callback (happy path, CSRF mismatch, cross-user, off-spec token), connect, and credentials route tests. --- .../polar/callback/__tests__/route.test.ts | 102 ++++++++++++++++++ .../api/polar/connect/__tests__/route.test.ts | 69 ++++++++++++ src/app/api/polar/connect/route.ts | 11 +- .../polar/credentials/__tests__/route.test.ts | 84 +++++++++++++++ src/app/api/polar/credentials/route.ts | 10 ++ .../__tests__/response-classifier.test.ts | 64 +++++++++++ src/lib/polar/response-classifier.ts | 13 +++ 7 files changed, 348 insertions(+), 5 deletions(-) create mode 100644 src/app/api/polar/callback/__tests__/route.test.ts create mode 100644 src/app/api/polar/connect/__tests__/route.test.ts create mode 100644 src/app/api/polar/credentials/__tests__/route.test.ts create mode 100644 src/lib/polar/__tests__/response-classifier.test.ts diff --git a/src/app/api/polar/callback/__tests__/route.test.ts b/src/app/api/polar/callback/__tests__/route.test.ts new file mode 100644 index 000000000..c290363ad --- /dev/null +++ b/src/app/api/polar/callback/__tests__/route.test.ts @@ -0,0 +1,102 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +const { + stateMatchesCookieMock, + verifySignedStateMock, + getSessionMock, + exchangeCodeMock, + registerUserMock, + getCredsMock, + markReconnectedMock, +} = vi.hoisted(() => ({ + stateMatchesCookieMock: vi.fn(), + verifySignedStateMock: vi.fn(), + getSessionMock: vi.fn(), + exchangeCodeMock: vi.fn(), + registerUserMock: vi.fn(), + getCredsMock: vi.fn(), + markReconnectedMock: vi.fn(), +})); + +vi.mock("@/lib/api-handler", () => ({ + apiHandler: unknown>(fn: T) => fn, +})); +vi.mock("@/lib/db", () => ({ prisma: { user: { update: vi.fn() } } })); +vi.mock("@/lib/logging/context", () => ({ + annotate: vi.fn(), + getEvent: () => ({ setAuth: vi.fn(), setError: vi.fn() }), +})); +vi.mock("@/lib/auth/audit", () => ({ auditLog: vi.fn() })); +vi.mock("@/lib/crypto", () => ({ encrypt: (s: string) => `enc:${s}` })); +vi.mock("@/lib/auth/session", () => ({ getSession: getSessionMock })); +vi.mock("@/lib/polar/client", () => ({ + exchangeCode: exchangeCodeMock, + registerUser: registerUserMock, +})); +vi.mock("@/lib/polar/credentials", () => ({ + getPolarClientCredentials: getCredsMock, +})); +vi.mock("@/lib/oauth/signed-state", () => ({ + oauthStateCookieName: () => "polar_oauth_state", + stateMatchesCookie: stateMatchesCookieMock, + verifySignedState: verifySignedStateMock, +})); +vi.mock("@/lib/integrations/status", () => ({ + markReconnected: markReconnectedMock, +})); + +import { GET } from "../route"; + +process.env.NEXT_PUBLIC_APP_URL = "https://app.example"; + +function makeReq(): Parameters[0] { + return { + url: "https://app.example/api/polar/callback?code=abc&state=s", + cookies: { get: () => ({ value: "s" }) }, + } as unknown as Parameters[0]; +} + +const run = GET as unknown as (req: ReturnType) => Promise<{ + status: number; + headers: { get(k: string): string | null }; +}>; + +beforeEach(() => { + vi.clearAllMocks(); + stateMatchesCookieMock.mockReturnValue(true); + verifySignedStateMock.mockReturnValue({ userId: "u1" }); + getSessionMock.mockResolvedValue(null); + getCredsMock.mockResolvedValue({ clientId: "c", clientSecret: "s" }); + exchangeCodeMock.mockResolvedValue({ access_token: "tok", x_user_id: 42 }); + registerUserMock.mockResolvedValue(undefined); +}); + +describe("GET /api/polar/callback", () => { + it("completes the handshake and redirects to connected", async () => { + const res = await run(makeReq()); + expect(res.status).toBe(307); + expect(res.headers.get("location")).toContain("polar=connected"); + expect(markReconnectedMock).toHaveBeenCalledWith("u1", "polar"); + }); + + it("rejects a CSRF cookie/param mismatch before exchanging the code", async () => { + stateMatchesCookieMock.mockReturnValue(false); + const res = await run(makeReq()); + expect(res.headers.get("location")).toContain("reason=csrf1"); + expect(exchangeCodeMock).not.toHaveBeenCalled(); + }); + + it("rejects a cross-user session completing another user's handshake", async () => { + getSessionMock.mockResolvedValue({ user: { id: "other" } }); + const res = await run(makeReq()); + expect(res.headers.get("location")).toContain("reason=cross_user"); + expect(exchangeCodeMock).not.toHaveBeenCalled(); + }); + + it("rejects an off-spec token body missing x_user_id", async () => { + exchangeCodeMock.mockResolvedValue({ access_token: "tok" }); + const res = await run(makeReq()); + expect(res.headers.get("location")).toContain("reason=token"); + expect(registerUserMock).not.toHaveBeenCalled(); + }); +}); diff --git a/src/app/api/polar/connect/__tests__/route.test.ts b/src/app/api/polar/connect/__tests__/route.test.ts new file mode 100644 index 000000000..87511d54e --- /dev/null +++ b/src/app/api/polar/connect/__tests__/route.test.ts @@ -0,0 +1,69 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +const { rateLimitMock, getCredsMock, mintMock, authUrlMock } = vi.hoisted( + () => ({ + rateLimitMock: vi.fn(), + getCredsMock: vi.fn(), + mintMock: vi.fn(() => "signed-state"), + authUrlMock: vi.fn(() => "https://flow.polar.com/oauth2/authorization?x=1"), + }), +); + +vi.mock("@/lib/api-handler", () => ({ + apiHandler: unknown>(fn: T) => fn, + requireAuth: vi.fn(async () => ({ user: { id: "u1" } })), +})); +vi.mock("@/lib/api-response", () => ({ + apiError: (error: string, status: number) => ({ data: null, error, status }), +})); +vi.mock("@/lib/logging/context", () => ({ annotate: vi.fn() })); +vi.mock("@/lib/auth/secure-cookie", () => ({ + shouldEmitSecureCookie: () => true, +})); +vi.mock("@/lib/rate-limit", () => ({ checkRateLimit: rateLimitMock })); +vi.mock("@/lib/polar/client", () => ({ getAuthorizationUrl: authUrlMock })); +vi.mock("@/lib/polar/credentials", () => ({ + getPolarClientCredentials: getCredsMock, +})); +vi.mock("@/lib/oauth/signed-state", () => ({ + OAUTH_STATE_TTL_MS: 600_000, + mintSignedState: mintMock, + oauthStateCookieName: () => "polar_oauth_state", +})); + +import { GET } from "../route"; + +process.env.NEXT_PUBLIC_APP_URL = "https://app.example"; + +const run = GET as unknown as () => Promise<{ + status: number; + headers: { get(k: string): string | null }; + cookies: { get(k: string): { value: string } | undefined }; +}>; + +beforeEach(() => { + vi.clearAllMocks(); + rateLimitMock.mockResolvedValue({ allowed: true }); + getCredsMock.mockResolvedValue({ clientId: "c", clientSecret: "s" }); +}); + +describe("GET /api/polar/connect", () => { + it("redirects to the consent screen and sets the state cookie", async () => { + const res = await run(); + expect(res.headers.get("location")).toContain("flow.polar.com"); + expect(res.cookies.get("polar_oauth_state")?.value).toBe("signed-state"); + }); + + it("redirects to an error when the bucket is exhausted", async () => { + rateLimitMock.mockResolvedValue({ allowed: false }); + const res = await run(); + expect(res.headers.get("location")).toContain("reason=rate_limited"); + expect(getCredsMock).not.toHaveBeenCalled(); + }); + + it("returns 400 when neither BYO nor env credentials resolve", async () => { + getCredsMock.mockResolvedValue(null); + const res = (await run()) as unknown as { status: number }; + expect(res.status).toBe(400); + }); +}); diff --git a/src/app/api/polar/connect/route.ts b/src/app/api/polar/connect/route.ts index 65d94e708..f92c68f35 100644 --- a/src/app/api/polar/connect/route.ts +++ b/src/app/api/polar/connect/route.ts @@ -15,11 +15,12 @@ import { NextResponse } from "next/server"; /** * v1.17.0 (F4) — redirect the user to the Polar AccessLink consent screen. * - * Full OAuth from env: the shared client id/secret come from `POLAR_CLIENT_ID` - * / `POLAR_CLIENT_SECRET` (not per-user BYO). The CSRF defence is a stateless - * signed state (`src/lib/oauth/signed-state.ts`): the same token is the `state` - * URL param AND an httpOnly cookie, and the callback enforces both signature - * validity and a byte-exact cookie match. + * BYO-key OAuth: the client id/secret resolve DB-first then env via + * `getPolarClientCredentials` — a user's own AccessLink app when stored, + * otherwise the shared `POLAR_CLIENT_ID` / `POLAR_CLIENT_SECRET`. The CSRF + * defence is a stateless signed state (`src/lib/oauth/signed-state.ts`): the + * same token is the `state` URL param AND an httpOnly cookie, and the callback + * enforces both signature validity and a byte-exact cookie match. * * Rate-limited per user so a session can't spam the consent redirect. */ diff --git a/src/app/api/polar/credentials/__tests__/route.test.ts b/src/app/api/polar/credentials/__tests__/route.test.ts new file mode 100644 index 000000000..3b744bc78 --- /dev/null +++ b/src/app/api/polar/credentials/__tests__/route.test.ts @@ -0,0 +1,84 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +const { storeMock, clearMock, auditMock, markDisconnectedMock } = vi.hoisted( + () => ({ + storeMock: vi.fn(), + clearMock: vi.fn(), + auditMock: vi.fn(), + markDisconnectedMock: vi.fn(), + }), +); + +vi.mock("@/lib/api-handler", () => ({ + apiHandler: unknown>(fn: T) => fn, + requireAuth: vi.fn(async () => ({ user: { id: "u1" } })), +})); +vi.mock("@/lib/db", () => ({ + prisma: { user: { findUnique: vi.fn(), update: vi.fn() } }, +})); +vi.mock("@/lib/logging/context", () => ({ annotate: vi.fn() })); +vi.mock("@/lib/auth/audit", () => ({ auditLog: auditMock })); +vi.mock("@/lib/integrations/status", () => ({ + markDisconnected: markDisconnectedMock, +})); +vi.mock("@/lib/polar/credentials", () => ({ + storePolarClientCredentials: storeMock, + clearPolarClientCredentials: clearMock, +})); +vi.mock("@/lib/api-response", () => ({ + apiSuccess: (data: unknown) => ({ data, error: null, status: 200 }), + apiError: (error: string, status: number) => ({ data: null, error, status }), + safeJson: vi.fn(async (req: { _body: unknown }) => ({ + data: req._body, + error: null, + })), +})); + +import { PUT, DELETE } from "../route"; +import { prisma } from "@/lib/db"; + +type RouteResult = { data: unknown; error: string | null; status: number }; +const userUpdate = prisma.user.update as ReturnType; + +beforeEach(() => { + vi.clearAllMocks(); +}); + +describe("PUT /api/polar/credentials", () => { + it("stores credentials and audit-logs the mutation", async () => { + const req = { _body: { clientId: "cid", clientSecret: "csecret" } }; + const put = PUT as unknown as (r: typeof req) => Promise; + const res = await put(req); + expect(res.status).toBe(200); + expect(storeMock).toHaveBeenCalledWith("u1", "cid", "csecret"); + expect(auditMock).toHaveBeenCalledWith("polar.credentials.update", { + userId: "u1", + }); + }); + + it("rejects an invalid body with a 422", async () => { + const req = { _body: { clientId: "" } }; + const put = PUT as unknown as (r: typeof req) => Promise; + const res = await put(req); + expect(res.status).toBe(422); + expect(storeMock).not.toHaveBeenCalled(); + }); +}); + +describe("DELETE /api/polar/credentials", () => { + it("clears creds + token and parks the ledger at disconnected", async () => { + const del = DELETE as unknown as () => Promise; + const res = await del(); + expect(res.status).toBe(200); + // The active token + member id are nulled. + expect(userUpdate).toHaveBeenCalledWith({ + where: { id: "u1" }, + data: { polarAccessTokenEncrypted: null, polarUserIdEncrypted: null }, + }); + expect(clearMock).toHaveBeenCalledWith("u1"); + expect(auditMock).toHaveBeenCalledWith("polar.credentials.delete", { + userId: "u1", + }); + expect(markDisconnectedMock).toHaveBeenCalledWith("u1", "polar"); + }); +}); diff --git a/src/app/api/polar/credentials/route.ts b/src/app/api/polar/credentials/route.ts index 9a92d467d..7dbab091b 100644 --- a/src/app/api/polar/credentials/route.ts +++ b/src/app/api/polar/credentials/route.ts @@ -1,7 +1,9 @@ import { prisma } from "@/lib/db"; import { apiHandler, requireAuth } from "@/lib/api-handler"; +import { auditLog } from "@/lib/auth/audit"; import { apiSuccess, apiError, safeJson } from "@/lib/api-response"; import { annotate } from "@/lib/logging/context"; +import { markDisconnected } from "@/lib/integrations/status"; import { storePolarClientCredentials, clearPolarClientCredentials, @@ -57,12 +59,18 @@ export const PUT = apiHandler(async (request: NextRequest) => { result.data.clientId, result.data.clientSecret, ); + await auditLog("polar.credentials.update", { userId: user.id }); return apiSuccess({ updated: true }); }); /** * Delete Polar credentials and the active connection. + * + * Removing the BYO client id/secret also drops the live grant: a token minted + * under the user's own AccessLink app would be orphaned once the keys are gone. + * Parks the integration ledger at `disconnected` for parity with the dedicated + * disconnect route so the status snapshot does not linger at a stale state. */ export const DELETE = apiHandler(async () => { const { user } = await requireAuth(); @@ -76,6 +84,8 @@ export const DELETE = apiHandler(async () => { }, }); await clearPolarClientCredentials(user.id); + await auditLog("polar.credentials.delete", { userId: user.id }); + await markDisconnected(user.id, "polar"); return apiSuccess({ deleted: true }); }); diff --git a/src/lib/polar/__tests__/response-classifier.test.ts b/src/lib/polar/__tests__/response-classifier.test.ts new file mode 100644 index 000000000..6e9655872 --- /dev/null +++ b/src/lib/polar/__tests__/response-classifier.test.ts @@ -0,0 +1,64 @@ +import { describe, expect, it } from "vitest"; + +import { + PolarApiError, + classifyPolarError, + isInvalidGrant, +} from "../response-classifier"; + +describe("classifyPolarError — invalid_grant reauth lift (H-1)", () => { + it("lifts a 400 invalid_grant on the token endpoint to reauth_required", () => { + const err = new PolarApiError({ + verb: "exchangeCode", + classification: "persistent", + httpStatus: 400, + reason: "http_400", + upstreamError: "invalid_grant", + }); + expect(isInvalidGrant(err)).toBe(true); + expect(classifyPolarError(err)).toBe("reauth_required"); + }); + + it("recovers the verdict from the message shape after a lost prototype", () => { + // A pg-boss retry can strip the PolarApiError prototype; the message-shape + // fallback must still recognise the invalid_grant signal. + const bare = new Error("Polar exchangeCode error: 400 - invalid_grant"); + expect(isInvalidGrant(bare)).toBe(true); + expect(classifyPolarError(bare)).toBe("reauth_required"); + }); + + it("leaves a non-invalid_grant 400 as persistent", () => { + const err = new PolarApiError({ + verb: "exchangeCode", + classification: "persistent", + httpStatus: 400, + reason: "http_400", + upstreamError: "invalid_client", + }); + expect(isInvalidGrant(err)).toBe(false); + expect(classifyPolarError(err)).toBe("persistent"); + }); + + it("keeps a 401 reauth and a 500 transient classification", () => { + expect( + classifyPolarError( + new PolarApiError({ + verb: "fetchSleeps", + classification: "reauth_required", + httpStatus: 401, + reason: "http_401", + }), + ), + ).toBe("reauth_required"); + expect( + classifyPolarError( + new PolarApiError({ + verb: "fetchSleeps", + classification: "transient", + httpStatus: 500, + reason: "http_500", + }), + ), + ).toBe("transient"); + }); +}); diff --git a/src/lib/polar/response-classifier.ts b/src/lib/polar/response-classifier.ts index 30297e60b..41eec2a8c 100644 --- a/src/lib/polar/response-classifier.ts +++ b/src/lib/polar/response-classifier.ts @@ -20,6 +20,7 @@ import { classifyHttpStatus, classifyIntegrationError, IntegrationApiError, + isOAuthInvalidGrant, type ClassifiedHttpResponse, type IntegrationClassification, } from "@/lib/integrations/http-status-classifier"; @@ -48,6 +49,17 @@ export class PolarApiError extends IntegrationApiError { } } +/** + * True when a caught error is an OAuth `invalid_grant` on the token endpoint — + * a stale/replayed authorization code on Polar's only token call + * (`exchangeCode`). Lifts a 400 carrying `invalid_grant` from `persistent` to + * `reauth_required` so the connection surfaces the reconnect prompt instead of + * silently parking after 24 h. + */ +export function isInvalidGrant(err: unknown): boolean { + return isOAuthInvalidGrant(err, "Polar"); +} + /** * Read the classification from any caught error. A non-`PolarApiError` defaults * to `transient` so a plain `Error` retries rather than disabling the @@ -55,5 +67,6 @@ export class PolarApiError extends IntegrationApiError { * caller that lost the prototype across a pg-boss retry. */ export function classifyPolarError(err: unknown): PolarClassification { + if (isInvalidGrant(err)) return "reauth_required"; return classifyIntegrationError(err, "Polar"); }