diff --git a/.changeset/consume-verification-rejects-expired.md b/.changeset/consume-verification-rejects-expired.md new file mode 100644 index 0000000000..02e128a271 --- /dev/null +++ b/.changeset/consume-verification-rejects-expired.md @@ -0,0 +1,6 @@ +--- +"better-auth": patch +"@better-auth/oauth-provider": patch +--- + +Expired magic-link tokens and OAuth authorization codes are now reliably rejected. Magic-link verify redirects to `?error=INVALID_TOKEN` for expired tokens (was `?error=EXPIRED_TOKEN`). The OIDC, MCP, and `@better-auth/oauth-provider` `/token` endpoints return `error_description: "invalid code"` for expired codes (was `"code expired"`). The OAuth `error` value stays `invalid_grant`. diff --git a/packages/better-auth/src/db/internal-adapter.test.ts b/packages/better-auth/src/db/internal-adapter.test.ts index 1fecfa2f84..edb707ef49 100644 --- a/packages/better-auth/src/db/internal-adapter.test.ts +++ b/packages/better-auth/src/db/internal-adapter.test.ts @@ -1546,6 +1546,23 @@ describe("internal adapter test", async () => { expect(result).toBeNull(); }); + it("returns null when the row exists but has already expired", async () => { + const adapter = await makeAdapter(); + await adapter.createVerificationValue({ + identifier: "consume:expired", + value: "user-expired", + expiresAt: new Date(Date.now() - 1_000), + }); + + const result = await adapter.consumeVerificationValue("consume:expired"); + expect(result).toBeNull(); + + // The expired row must still be invalidated so a later replay cannot + // consume it after a cleanup pass. + const replay = await adapter.findVerificationValue("consume:expired"); + expect(replay).toBeNull(); + }); + it("aborts the consume when a delete.before hook returns false", async () => { const veto = vi.fn().mockReturnValue(false); const adapter = await makeAdapter({ @@ -1678,6 +1695,113 @@ describe("internal adapter test", async () => { expect(store.has("verification:consume:secondary")).toBe(false); }); + it("returns null when the secondary storage row has already expired", async () => { + const store = new Map(); + const adapter = await makeAdapter({ + verification: { storeInDatabase: false }, + secondaryStorage: { + set(key, value) { + store.set(key, value); + }, + get(key) { + return store.get(key) ?? null; + }, + delete(key) { + store.delete(key); + }, + }, + }); + + // Bypass `createVerificationValue`'s TTL gate by writing directly + // with an `expiresAt` already in the past. This mirrors a row that + // was valid when written but reached the consume call after expiry. + store.set( + "verification:consume:secondary-expired", + JSON.stringify({ + id: "row-id", + identifier: "consume:secondary-expired", + value: "secondary-expired-user", + expiresAt: new Date(Date.now() - 1_000), + createdAt: new Date(), + updatedAt: new Date(), + }), + ); + + const result = await adapter.consumeVerificationValue( + "consume:secondary-expired", + ); + expect(result).toBeNull(); + // The expired row is still consumed (deleted) so it cannot be + // replayed later. + expect(store.has("verification:consume:secondary-expired")).toBe(false); + }); + + it("rehydrates string `expiresAt` from secondary storage JSON", async () => { + const store = new Map(); + const adapter = await makeAdapter({ + verification: { storeInDatabase: false }, + secondaryStorage: { + set(key, value) { + store.set(key, value); + }, + get(key) { + return store.get(key) ?? null; + }, + delete(key) { + store.delete(key); + }, + }, + }); + await adapter.createVerificationValue({ + identifier: "consume:secondary-hydrate", + value: "hydrate-user", + expiresAt: new Date(Date.now() + 60_000), + }); + + const result = await adapter.consumeVerificationValue( + "consume:secondary-hydrate", + ); + expect(result).not.toBeNull(); + expect(result!.value).toBe("hydrate-user"); + expect(result!.expiresAt).toBeInstanceOf(Date); + expect(result!.expiresAt.getTime()).toBeGreaterThan(Date.now()); + }); + + it("returns null when secondary storage `expiresAt` cannot be parsed", async () => { + const store = new Map(); + const adapter = await makeAdapter({ + verification: { storeInDatabase: false }, + secondaryStorage: { + set(key, value) { + store.set(key, value); + }, + get(key) { + return store.get(key) ?? null; + }, + delete(key) { + store.delete(key); + }, + }, + }); + + store.set( + "verification:consume:secondary-invalid-date", + JSON.stringify({ + id: "row-id", + identifier: "consume:secondary-invalid-date", + value: "bad-row", + expiresAt: "not-a-date", + createdAt: new Date(), + updatedAt: new Date(), + }), + ); + + const result = await adapter.consumeVerificationValue( + "consume:secondary-invalid-date", + ); + expect(result).toBeNull(); + }); + it("serializes the secondary storage compatibility fallback within one process", async () => { const store = new Map(); const adapter = await makeAdapter({ diff --git a/packages/better-auth/src/db/internal-adapter.ts b/packages/better-auth/src/db/internal-adapter.ts index 4df84a053c..af65bf66d1 100644 --- a/packages/better-auth/src/db/internal-adapter.ts +++ b/packages/better-auth/src/db/internal-adapter.ts @@ -1230,9 +1230,9 @@ export const createInternalAdapter = ( * return value, because consuming one row invalidates the whole * identifier and stale rows cannot be replayed. * - * Expiry is intentionally not checked here. Callers retain their - * existing expiry handling so each call site can surface its own - * error code (for example `token_expired` vs `invalid_grant`). + * Rows past their `expiresAt` are treated as already invalid: the row + * is still deleted (so it cannot be replayed later) but `null` is + * returned. Callers do not need their own `expiresAt` gate. * * The secondary-storage-only path (`storeInDatabase: false`) is atomic * only when the configured storage implements `getAndDelete`; otherwise @@ -1259,30 +1259,34 @@ export const createInternalAdapter = ( ? [storedIdentifier, identifier] : [storedIdentifier]; + // After a JSON round-trip `expiresAt` arrives as a string, so coerce + // it back to a valid `Date` to match what the DB adapter returns. + const hydrateCachedVerification = (raw: unknown): Verification | null => { + if (!raw) return null; + const candidate = + typeof raw === "string" + ? safeJSONParse(raw) + : typeof raw === "object" + ? (raw as Verification) + : null; + if (!candidate) return null; + const expiresAt = new Date(candidate.expiresAt); + if (!Number.isFinite(expiresAt.getTime())) return null; + return { ...candidate, expiresAt }; + }; + + let consumed: Verification | null = null; + if (secondaryStorage && !options.verification?.storeInDatabase) { - const parseCachedVerification = (raw: unknown) => { - if (!raw) return null; - // Secondary storage wrappers can return either a JSON string - // (the default codec) or a pre-parsed object (some Redis client - // integrations do this). Accept both shapes; only call - // safeJSONParse when we actually have a string. - if (typeof raw === "string") { - return safeJSONParse(raw); - } - if (typeof raw === "object") { - return raw as Verification; - } - return null; - }; const consumeCacheKey = async (key: string) => { if (secondaryStorage.getAndDelete) { - return parseCachedVerification( + return hydrateCachedVerification( await secondaryStorage.getAndDelete(key), ); } return withVerificationConsumeLock(key, async () => { const raw = await secondaryStorage.get(key); - const parsed = parseCachedVerification(raw); + const parsed = hydrateCachedVerification(raw); if (!parsed) return null; await secondaryStorage.delete(key); return parsed; @@ -1300,67 +1304,67 @@ export const createInternalAdapter = ( secondaryStorage.delete(`verification:${candidate}`), ), ); - return cached; + consumed = cached; + break; } - return null; - } + } else { + const consumeByIdentifier = async ( + id: string, + ): Promise => + withVerificationConsumeLock(`verification:${id}`, () => + runWithTransaction(adapter, async () => { + const txAdapter = await getCurrentAdapter(adapter); + const where = [{ field: "identifier", value: id }]; + const rows = await txAdapter.findMany({ + model: "verification", + where, + sortBy: { field: "createdAt", direction: "desc" }, + limit: 1, + }); + const latest = rows[0] ?? null; + if (!latest) return null; - async function consumeByIdentifier( - id: string, - ): Promise { - const where = [{ field: "identifier", value: id }]; - return withVerificationConsumeLock(`verification:${id}`, () => - runWithTransaction(adapter, async () => { - const txAdapter = await getCurrentAdapter(adapter); - const rows = await txAdapter.findMany({ - model: "verification", - where, - sortBy: { field: "createdAt", direction: "desc" }, - limit: 1, - }); - const latest = rows[0] ?? null; - if (!latest) return null; + // FIXME(consume-identifier-atomic): add an adapter primitive that + // deletes all rows for an identifier and returns the latest row in + // one operation. Until then, consume the latest row as the race gate + // and invalidate stale rows inside the same transaction/local lock. + return consumeOneWithHooks( + "verification", + [{ field: "id", value: latest.id }], + async () => { + const row = await txAdapter.consumeOne({ + model: "verification", + where: [{ field: "id", value: latest.id }], + }); + if (!row) return null; + await txAdapter.deleteMany({ + model: "verification", + where, + }); + return row; + }, + latest, + ); + }), + ); - // FIXME(consume-identifier-atomic): add an adapter primitive that - // deletes all rows for an identifier and returns the latest row in - // one operation. Until then, consume the latest row as the race gate - // and invalidate stale rows inside the same transaction/local lock. - const hookWhere = [{ field: "id", value: latest.id }]; - return consumeOneWithHooks( - "verification", - hookWhere, - async () => { - const consumed = await txAdapter.consumeOne({ - model: "verification", - where: hookWhere, - }); - if (!consumed) return null; - await txAdapter.deleteMany({ - model: "verification", - where, - }); - return consumed; - }, - latest, - ); - }), - ); - } + for (const stored of identifiersToTry) { + consumed = await consumeByIdentifier(stored); + if (consumed) break; + } - let consumed: Verification | null = null; - for (const stored of identifiersToTry) { - consumed = await consumeByIdentifier(stored); - if (consumed) break; - } - - if (consumed && secondaryStorage) { - await Promise.all( - identifiersToTry.map((stored) => - secondaryStorage.delete(`verification:${stored}`), - ), - ); + if (consumed && secondaryStorage) { + await Promise.all( + identifiersToTry.map((stored) => + secondaryStorage.delete(`verification:${stored}`), + ), + ); + } } + // Single expiry gate. A row past its `expiresAt` is treated as already + // invalid, so callers can rely on a non-null return meaning "valid". + if (!consumed || consumed.expiresAt < new Date()) return null; return consumed; }, updateVerificationByIdentifier: async ( diff --git a/packages/better-auth/src/plugins/magic-link/index.ts b/packages/better-auth/src/plugins/magic-link/index.ts index e49c5c6f06..f85cd046f5 100644 --- a/packages/better-auth/src/plugins/magic-link/index.ts +++ b/packages/better-auth/src/plugins/magic-link/index.ts @@ -376,9 +376,6 @@ export const magicLink = (options: MagicLinkOptions) => { if (!tokenValue) { redirectWithError("INVALID_TOKEN"); } - if (tokenValue.expiresAt < new Date()) { - redirectWithError("EXPIRED_TOKEN"); - } const { email, name } = JSON.parse(tokenValue.value) as { email: string; name?: string | undefined; diff --git a/packages/better-auth/src/plugins/magic-link/magic-link.test.ts b/packages/better-auth/src/plugins/magic-link/magic-link.test.ts index 10b4c18b19..90aa1ec819 100644 --- a/packages/better-auth/src/plugins/magic-link/magic-link.test.ts +++ b/packages/better-auth/src/plugins/magic-link/magic-link.test.ts @@ -119,7 +119,7 @@ describe("magic link", async () => { onError(context) { expect(context.response.status).toBe(302); const location = context.response.headers.get("location"); - expect(location).toContain("?error=EXPIRED_TOKEN"); + expect(location).toContain("?error=INVALID_TOKEN"); }, }, ); @@ -555,7 +555,7 @@ describe("magic link storeToken", async () => { * Magic-link tokens are consumed atomically on the first verification call that * finds the token: the row is deleted before any subsequent success checks * (signup gates, session creation, etc.), so even a verify that ends in - * `EXPIRED_TOKEN`, `new_user_signup_disabled`, or `failed_to_create_session` + * `INVALID_TOKEN`, `new_user_signup_disabled`, or `failed_to_create_session` * still burns the token. `allowedAttempts` is retained on the options type for * backward compatibility but does not multiply redemptions; a token mints at * most one session regardless of the value. diff --git a/packages/better-auth/src/plugins/mcp/index.ts b/packages/better-auth/src/plugins/mcp/index.ts index 7b1b65b61d..9d985fe451 100644 --- a/packages/better-auth/src/plugins/mcp/index.ts +++ b/packages/better-auth/src/plugins/mcp/index.ts @@ -551,12 +551,6 @@ export const mcp = (options: MCPOptions) => { error: "invalid_grant", }); } - if (verificationValue.expiresAt < new Date()) { - throw new APIError("UNAUTHORIZED", { - error_description: "code expired", - error: "invalid_grant", - }); - } if (!client_id) { throw new APIError("UNAUTHORIZED", { diff --git a/packages/better-auth/src/plugins/oidc-provider/index.ts b/packages/better-auth/src/plugins/oidc-provider/index.ts index 1c3550a066..3d45c7c62b 100644 --- a/packages/better-auth/src/plugins/oidc-provider/index.ts +++ b/packages/better-auth/src/plugins/oidc-provider/index.ts @@ -884,12 +884,6 @@ export const oidcProvider = (options: OIDCOptions) => { error: "invalid_grant", }); } - if (verificationValue.expiresAt < new Date()) { - throw new APIError("UNAUTHORIZED", { - error_description: "code expired", - error: "invalid_grant", - }); - } if (!client_id) { throw new APIError("UNAUTHORIZED", { error_description: "client_id is required", diff --git a/packages/core/src/types/context.ts b/packages/core/src/types/context.ts index 471471f700..c939e30279 100644 --- a/packages/core/src/types/context.ts +++ b/packages/core/src/types/context.ts @@ -220,9 +220,11 @@ export interface InternalAdapter< * Atomically consume a single-use verification row by `identifier` and * return it. Only the first concurrent caller receives the latest row; * subsequent callers receive `null`. Consuming one row invalidates the - * whole identifier so stale rows cannot be replayed. Callers MUST gate any - * state change (issue session, mint token, change password) on a non-null - * result. + * whole identifier so stale rows cannot be replayed. Rows past their + * `expiresAt` are treated as already invalid: the row is deleted but + * `null` is returned, so callers do not need to gate on `expiresAt` + * themselves. Callers MUST gate any state change (issue session, mint + * token, change password) on a non-null result. * * Replaces the racy `findVerificationValue` + `deleteVerificationByIdentifier` * pair at single-use credential consumption sites. diff --git a/packages/oauth-provider/src/token.ts b/packages/oauth-provider/src/token.ts index 9bf9cc7b6b..2862d49fc7 100644 --- a/packages/oauth-provider/src/token.ts +++ b/packages/oauth-provider/src/token.ts @@ -646,14 +646,7 @@ async function checkVerificationValue( if (!verification) { throw new APIError("UNAUTHORIZED", { - error_description: "Invalid code", - error: "invalid_grant", - }); - } - - if (!verification.expiresAt || verification.expiresAt < new Date()) { - throw new APIError("UNAUTHORIZED", { - error_description: "code expired", + error_description: "invalid code", error: "invalid_grant", }); } diff --git a/test/unit/magic-link-secondary-storage.test.ts b/test/unit/magic-link-secondary-storage.test.ts index 87531243fc..14e0867d43 100644 --- a/test/unit/magic-link-secondary-storage.test.ts +++ b/test/unit/magic-link-secondary-storage.test.ts @@ -217,7 +217,7 @@ describe("magic link with secondary storage (string return)", async () => { onError(context) { expect(context.response.status).toBe(302); const location = context.response.headers.get("location"); - expect(location).toContain("?error=EXPIRED_TOKEN"); + expect(location).toContain("?error=INVALID_TOKEN"); }, }, );