From 08c71ab4b3126273e45f35ebb82b3f0940ab14ad Mon Sep 17 00:00:00 2001 From: Santhil Kherwal <72144927+santhil-cyber@users.noreply.github.com> Date: Mon, 22 Jun 2026 11:50:34 +0530 Subject: [PATCH] fix: harden OAuth token-response parsing and refresh-error classification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The token endpoint responses were read with plain `as string` casts and a `(d.expires_in as number) || 3600` fallback. That mostly works, but leaves a few real gaps: - A truthy non-string access_token (a number, an object) passes the presence check and gets mistyped as a string, surfacing later as a confusing downstream failure instead of a clear parse error. - A non-numeric or negative expires_in (e.g. "soon", -5) flows into `Date.now() + expiresIn * 1000`, producing NaN and a RangeError when we build the expiry date. Both token responses are now validated with Zod (the same .safeParse pattern used in auth-config.ts) and expires_in is coerced safely: numeric strings are accepted, anything non-finite or non-positive falls back to the default lifetime. The existing "Token response missing required fields" error and the omitted-expires_in default are preserved. Separately, classifyTerminalRefreshError required both an invalid_grant code and a specific phrase in the error description to flag a dead refresh token. Per RFC 6749 §5.2, invalid_grant on a refresh request already means the token is expired/revoked/reused, so a backend rewording its description would silently break detection. It now classifies on the structured code, keeping the substring checks as a fallback for servers that don't send a clean code. Closes #180 --- src/services/auth-service.test.ts | 108 ++++++++++++++++++++++++++++++ src/services/auth-service.ts | 80 +++++++++++++++++----- 2 files changed, 171 insertions(+), 17 deletions(-) diff --git a/src/services/auth-service.test.ts b/src/services/auth-service.test.ts index ff4ef650..75c8e27b 100644 --- a/src/services/auth-service.test.ts +++ b/src/services/auth-service.test.ts @@ -185,6 +185,114 @@ describe("AuthServiceImpl", () => { ); } }); + + it("coerces a numeric-string expires_in to a number", async () => { + const fetchMock = mock(() => + Promise.resolve( + new Response( + JSON.stringify({ + access_token: "new-access-token", + expires_in: "120", + }), + { status: 200 }, + ), + ), + ); + globalThis.fetch = fetchMock as unknown as typeof fetch; + + const result = await service.refreshAccessToken({ + tokenEndpoint: "https://auth.example.com/oauth/token", + clientId: "client-id", + clientSecret: "client-secret", + refreshToken: "refresh-token", + }); + + expect(result.expiresIn).toBe(120); + }); + + it("falls back to the default lifetime for non-positive or non-numeric expires_in", async () => { + for (const bad of [0, -5, "soon", null]) { + const fetchMock = mock(() => + Promise.resolve( + new Response( + JSON.stringify({ + access_token: "new-access-token", + expires_in: bad, + }), + { status: 200 }, + ), + ), + ); + globalThis.fetch = fetchMock as unknown as typeof fetch; + + const result = await service.refreshAccessToken({ + tokenEndpoint: "https://auth.example.com/oauth/token", + clientId: "client-id", + clientSecret: "client-secret", + refreshToken: "refresh-token", + }); + + expect(result.expiresIn).toBe(3600); + } + }); + + it("rejects responses whose access_token is not a non-empty string", async () => { + for (const bad of [123, "", null, { token: "x" }]) { + const fetchMock = mock(() => + Promise.resolve( + new Response(JSON.stringify({ access_token: bad }), { + status: 200, + }), + ), + ); + globalThis.fetch = fetchMock as unknown as typeof fetch; + + await expect( + service.refreshAccessToken({ + tokenEndpoint: "https://auth.example.com/oauth/token", + clientId: "client-id", + clientSecret: "client-secret", + refreshToken: "refresh-token", + }), + ).rejects.toThrow("Token response missing required fields"); + } + }); + }); + + describe("classifyTerminalRefreshError", () => { + it("classifies invalid_grant regardless of description wording", () => { + const error = new TokenRefreshError( + 400, + JSON.stringify({ + error: "invalid_grant", + error_description: "the token is no longer recognized by the server", + }), + ); + expect(classifyTerminalRefreshError(error)).toBe("invalid_refresh_token"); + }); + + it("classifies invalid_client from the structured code", () => { + const error = new TokenRefreshError( + 401, + JSON.stringify({ error: "invalid_client" }), + ); + expect(classifyTerminalRefreshError(error)).toBe("invalid_client"); + }); + + it("returns undefined for non-terminal OAuth errors", () => { + const error = new TokenRefreshError( + 400, + JSON.stringify({ + error: "invalid_request", + error_description: "missing parameter", + }), + ); + expect(classifyTerminalRefreshError(error)).toBeUndefined(); + }); + + it("returns undefined for non-TokenRefreshError values", () => { + expect(classifyTerminalRefreshError(new Error("boom"))).toBeUndefined(); + }); }); describe("evaluateCallback", () => { diff --git a/src/services/auth-service.ts b/src/services/auth-service.ts index 666f2aac..25c344eb 100644 --- a/src/services/auth-service.ts +++ b/src/services/auth-service.ts @@ -6,6 +6,7 @@ import { generateCodeVerifier, generateState, } from "@githits/core-internal"; +import { z } from "zod"; /** * OAuth Authorization Server metadata from .well-known endpoint. @@ -449,13 +450,20 @@ export function classifyTerminalRefreshError( return "invalid_client"; } + // RFC 6749 §5.2: for the refresh_token grant, `invalid_grant` means the + // refresh token is expired, revoked, or otherwise no longer valid. Trust the + // structured code on its own; the substring checks below are a fallback for + // servers that report the same condition without a clean error code. + if (oauthError === "invalid_grant") { + return "invalid_refresh_token"; + } + if ( - oauthError === "invalid_grant" && - (text.includes("invalid refresh token") || - text.includes("refresh token already used") || - text.includes("already used") || - text.includes("session expired") || - text.includes("session not found")) + text.includes("invalid refresh token") || + text.includes("refresh token already used") || + text.includes("already used") || + text.includes("session expired") || + text.includes("session not found") ) { return "invalid_refresh_token"; } @@ -489,28 +497,66 @@ function stringField(value: unknown): string | undefined { return typeof value === "string" && value.trim() ? value : undefined; } +/** + * Default token lifetime (seconds) applied when the OAuth server omits + * `expires_in` or returns a value that is not a finite positive number. + */ +const DEFAULT_TOKEN_EXPIRES_IN_SECONDS = 3600; + +const nonEmptyString = z.string().min(1); + +/** + * Coerces `expires_in` to a usable lifetime. OAuth servers should send a JSON + * number per RFC 6749, but some emit numeric strings, and a missing, zero, + * negative, or non-numeric value would otherwise corrupt the computed expiry + * (e.g. `new Date(Date.now() + NaN)` throws). Any non-positive or non-finite + * value falls back to the default lifetime. + */ +const expiresInSchema = z + .union([z.number(), z.string()]) + .nullish() + .transform((value) => { + const numeric = typeof value === "string" ? Number(value) : value; + return typeof numeric === "number" && + Number.isFinite(numeric) && + numeric > 0 + ? numeric + : DEFAULT_TOKEN_EXPIRES_IN_SECONDS; + }); + +const TokenResponseSchema = z.object({ + access_token: nonEmptyString, + refresh_token: nonEmptyString, + expires_in: expiresInSchema, +}); + +const RefreshTokenResponseSchema = z.object({ + access_token: nonEmptyString, + refresh_token: nonEmptyString.optional(), + expires_in: expiresInSchema, +}); + function parseTokenResponse(data: unknown): TokenResponse { - const d = data as Record; - if (!d.access_token || !d.refresh_token) { + const parsed = TokenResponseSchema.safeParse(data); + if (!parsed.success) { throw new Error("Token response missing required fields"); } return { - accessToken: d.access_token as string, - refreshToken: d.refresh_token as string, - expiresIn: (d.expires_in as number) || 3600, + accessToken: parsed.data.access_token, + refreshToken: parsed.data.refresh_token, + expiresIn: parsed.data.expires_in, }; } function parseRefreshTokenResponse(data: unknown): RefreshTokenResponse { - const d = data as Record; - if (!d.access_token) { + const parsed = RefreshTokenResponseSchema.safeParse(data); + if (!parsed.success) { throw new Error("Token response missing required fields"); } return { - accessToken: d.access_token as string, - refreshToken: - typeof d.refresh_token === "string" ? d.refresh_token : undefined, - expiresIn: (d.expires_in as number) || 3600, + accessToken: parsed.data.access_token, + refreshToken: parsed.data.refresh_token, + expiresIn: parsed.data.expires_in, }; }