diff --git a/docs/service-contract.md b/docs/service-contract.md index 2a8a47f..fdcc48f 100644 --- a/docs/service-contract.md +++ b/docs/service-contract.md @@ -493,7 +493,8 @@ Important behavior: processes sharing one database. Within one process, concurrent refreshes of one identity share a single attempt and its outcome (success or failure), so nobody re-presents a token behind a failed holder in that burst. Residual - risk, accepted: the guard is one presentation per process per burst, so N + risk, tracked as follow-up `joystick:follow_up:cross-process-failed-refresh-marker`: + the guard is one presentation per process per burst, so N Heimdall processes can present one token N times after a transient failure. The provider call is bounded at 10 seconds - a provider whose refresh tokens rotate (Joystick) fails closed: a refresh @@ -508,10 +509,14 @@ Important behavior: - `502 {"error":"managed_credential_refresh_failed","provider":...}`: the refresh failed for any reason that is not a grant verdict (invalid client, a 401 or 5xx whatever its body says, timeout, a 2xx without a string - `access_token` or carrying an `error`, an empty or non-JSON body). Stored - tokens are untouched and a later request may succeed. An answer without - `expires_in` stores the expiry as unknown, and an unknown expiry is never - due, so it does not trigger another refresh + `access_token` or carrying an `error`, a `refresh_token` that is present but + not a string, an empty or non-JSON body). Stored tokens are untouched and a + later request may succeed. An empty-string `refresh_token` counts as missing, + so a rotating provider revokes (`rotation_missing`). A token answer without a + usable `expires_in` (absent, non-numeric, or not a positive number or + positive-integer string) stores the expiry as now plus the provider's default + access-token lifetime (3600 seconds), so an expiry is never unknown and the + credential always comes due for refresh - app-local binding, diagnostics, subscription setup, and runtime behavior stay app-owned diff --git a/src/managed-credential.ts b/src/managed-credential.ts index 617a897..7d15e2c 100644 --- a/src/managed-credential.ts +++ b/src/managed-credential.ts @@ -150,9 +150,9 @@ async function refreshUnderLock( refreshTokenEncrypted = locked.refreshTokenEncrypted!; } - // An answer without expires_in leaves the expiry unknown. The old expiry is - // already due, so keeping it would refresh again on every resolve; an - // unknown expiry is never due (dueForRefresh). + // The token set always carries an expiry (the provider's, or the + // descriptor's default lifetime): the old one is already due, and an + // unknown one would never come due (dueForRefresh). const tokenExpiresAt = refreshed.expiresAt; return { kind: "rotate", diff --git a/src/oauth.ts b/src/oauth.ts index 39dd2c0..53f0f85 100644 --- a/src/oauth.ts +++ b/src/oauth.ts @@ -196,12 +196,19 @@ export async function requestProviderTokens( } const tokenResponse = answer as { access_token: string; - refresh_token?: string; + refresh_token?: unknown; token_type?: string; - expires_in?: number; + expires_in?: unknown; scope?: string | string[]; }; + // A refresh token that is present must be a string: anything else is a + // malformed grant answer, refused here as a typed failure rather than + // reaching custody as a TypeError. + if (tokenResponse.refresh_token !== undefined && typeof tokenResponse.refresh_token !== "string") { + throw new ProviderResponseError(response.status, "provider_body_not_token_set", failure); + } + const tokenSet: OAuthTokenSet = { accessToken: tokenResponse.access_token, tokenType: tokenResponse.token_type ?? "Bearer", @@ -211,12 +218,19 @@ export async function requestProviderTokens( if (tokenResponse.refresh_token) { tokenSet.refreshToken = tokenResponse.refresh_token; } - if (typeof tokenResponse.expires_in === "number" && tokenResponse.expires_in > 0) { - tokenSet.expiresAt = new Date(Date.now() + tokenResponse.expires_in * 1000).toISOString(); - } + // Nothing issued here has an unknown expiry: an answer without a usable + // expires_in takes the descriptor's default lifetime. + const lifetimeSeconds = usableExpiresIn(tokenResponse.expires_in) ?? descriptor.defaultAccessTokenLifetimeSeconds; + tokenSet.expiresAt = new Date(Date.now() + lifetimeSeconds * 1000).toISOString(); return tokenSet; } +/** A positive finite number, or a string that is a positive integer. */ +function usableExpiresIn(value: unknown): number | undefined { + const seconds = typeof value === "string" && /^\s*\d+\s*$/.test(value) ? Number(value) : value; + return typeof seconds === "number" && Number.isFinite(seconds) && seconds > 0 ? seconds : undefined; +} + function sharedRuntimeExchange(provider: Provider): OAuthProviderRuntime["exchangeAuthorizationCode"] { return (options) => requestProviderTokens(provider, options.config, { diff --git a/src/providers.ts b/src/providers.ts index 9b139ab..77eebf4 100644 --- a/src/providers.ts +++ b/src/providers.ts @@ -11,6 +11,12 @@ export interface ProviderDescriptor { tokenEndpointAuthMethod: "client_secret_basic" | "client_secret_post"; /** True when a refresh response replaces the refresh token and the old one dies. */ rotatesRefreshTokens: boolean; + /** + * Seconds an access token lives when a token answer carries no usable + * `expires_in` (RFC 6749 5.1 makes it RECOMMENDED, not required). An unknown + * expiry is never stored: the credential would never come due for refresh. + */ + defaultAccessTokenLifetimeSeconds: number; defaultScopes: string[]; roles: ProviderRole[]; clientIdEnv: string; @@ -26,6 +32,7 @@ export const providerCatalog: Record = { tokenEndpoint: "https://discord.com/api/v10/oauth2/token", tokenEndpointAuthMethod: "client_secret_basic", rotatesRefreshTokens: false, + defaultAccessTokenLifetimeSeconds: 3600, defaultScopes: ["identify", "guilds.members.read"], roles: ["identity", "entitlement"], clientIdEnv: "GC_ACCESS_PROVIDER_DISCORD_CLIENT_ID", @@ -38,6 +45,7 @@ export const providerCatalog: Record = { tokenEndpoint: "https://www.patreon.com/api/oauth2/token", tokenEndpointAuthMethod: "client_secret_post", rotatesRefreshTokens: false, + defaultAccessTokenLifetimeSeconds: 3600, defaultScopes: ["identity", "identity[email]"], roles: ["identity", "entitlement"], clientIdEnv: "GC_ACCESS_PROVIDER_PATREON_CLIENT_ID", @@ -50,6 +58,7 @@ export const providerCatalog: Record = { tokenEndpoint: "https://github.com/login/oauth/access_token", tokenEndpointAuthMethod: "client_secret_post", rotatesRefreshTokens: false, + defaultAccessTokenLifetimeSeconds: 3600, defaultScopes: ["read:user", "user:email"], roles: ["identity", "managed_credential"], clientIdEnv: "GC_ACCESS_PROVIDER_GITHUB_CLIENT_ID", @@ -62,6 +71,7 @@ export const providerCatalog: Record = { tokenEndpoint: "https://id.twitch.tv/oauth2/token", tokenEndpointAuthMethod: "client_secret_post", rotatesRefreshTokens: false, + defaultAccessTokenLifetimeSeconds: 3600, defaultScopes: ["user:read:email"], roles: ["identity", "managed_credential"], clientIdEnv: "GC_ACCESS_PROVIDER_TWITCH_CLIENT_ID", @@ -74,6 +84,7 @@ export const providerCatalog: Record = { tokenEndpoint: "https://oauth2.googleapis.com/token", tokenEndpointAuthMethod: "client_secret_post", rotatesRefreshTokens: false, + defaultAccessTokenLifetimeSeconds: 3600, defaultScopes: [ "openid", "email", @@ -96,6 +107,7 @@ export const providerCatalog: Record = { tokenEndpoint: "https://accounts.spotify.com/api/token", tokenEndpointAuthMethod: "client_secret_basic", rotatesRefreshTokens: false, + defaultAccessTokenLifetimeSeconds: 3600, defaultScopes: [ "user-read-currently-playing", "user-read-playback-state", @@ -112,6 +124,7 @@ export const providerCatalog: Record = { tokenEndpoint: "https://api.joystick.tv/api/oauth/token", tokenEndpointAuthMethod: "client_secret_basic", rotatesRefreshTokens: true, + defaultAccessTokenLifetimeSeconds: 3600, defaultScopes: ["identity:read", "stream:read", "chat:read", "followers:read", "subscribers:read"], roles: ["identity", "managed_credential"], clientIdEnv: "GC_ACCESS_PROVIDER_JOYSTICK_CLIENT_ID", diff --git a/tests/managed-credential.test.ts b/tests/managed-credential.test.ts index 8a0d4d2..931f7ef 100644 --- a/tests/managed-credential.test.ts +++ b/tests/managed-credential.test.ts @@ -53,7 +53,23 @@ function testConfig(): HeimdallConfig { } as unknown as HeimdallConfig; } -type Mode = "normal" | "invalid_grant" | "invalid_client" | "server_error" | "not_json" | "no_refresh_token" | "hang"; +type Mode = + | "normal" + | "invalid_grant" + | "invalid_client" + | "server_error" + | "not_json" + | "no_refresh_token" + | "empty_refresh_token" + | "bad_refresh_token" + | "no_expires_in" + | "unusable_expires_in" + | "string_expires_in" + | "hang"; + +function expiresInFor(mode: Mode): unknown { + return mode === "unusable_expires_in" ? "soon" : mode === "string_expires_in" ? "7200" : 3600; +} /** * A provider token endpoint with single-use refresh tokens. Presenting a @@ -95,7 +111,18 @@ function installTokenEndpoint(provider: Provider, firstRefreshToken: string) { return new Response(JSON.stringify({ access_token: "access-norefresh", token_type: "Bearer", expires_in: 3600 }), { status: 200, }); - case "normal": { + case "empty_refresh_token": + return new Response(JSON.stringify({ access_token: "access-empty", refresh_token: "", token_type: "Bearer", expires_in: 3600 }), { + status: 200, + }); + case "bad_refresh_token": + return new Response(JSON.stringify({ access_token: "access-bad", refresh_token: 42, token_type: "Bearer", expires_in: 3600 }), { + status: 200, + }); + case "normal": + case "no_expires_in": + case "unusable_expires_in": + case "string_expires_in": { if (state.installationRevoked || presented !== state.live) { state.installationRevoked = true; return new Response(JSON.stringify({ error: "invalid_grant", error_description: "reused CANARY" }), { status: 400 }); @@ -107,7 +134,7 @@ function installTokenEndpoint(provider: Provider, firstRefreshToken: string) { access_token: `access-${state.issued}`, refresh_token: state.live, token_type: "Bearer", - expires_in: 3600, + ...(state.mode === "no_expires_in" ? {} : { expires_in: expiresInFor(state.mode) }), }), { status: 200 } ); @@ -329,7 +356,7 @@ for (const { kind, live } of rigKinds) describe.skipIf(live && !adminUrl)(`manag expect(first.body + second.body + JSON.stringify(events)).not.toContain("CANARY"); }); - it.each(["invalid_client", "server_error", "not_json", "hang"] as const)( + it.each(["invalid_client", "server_error", "not_json", "bad_refresh_token", "hang"] as const)( "transient failure (%s) keeps custody: 502, tokens byte-identical, nothing leaked", timeout, async (mode) => { @@ -422,15 +449,16 @@ for (const { kind, live } of rigKinds) describe.skipIf(live && !adminUrl)(`manag }); if (providerCatalog[provider].rotatesRefreshTokens) { - it("rotation-fails-closed: a response without a refresh token revokes, 410, and the old token is never presented again", timeout, async () => { + it.each(["no_refresh_token", "empty_refresh_token"] as const)("rotation-fails-closed: a response with %s revokes, 410, and the old token is never presented again", timeout, async (mode) => { const rig = await makeRig(provider); const endpoint = installTokenEndpoint(provider, "refresh-0"); - endpoint.mode = "no_refresh_token"; + endpoint.mode = mode; const first = await rig.resolve(); expect(first.statusCode).toBe(410); expect(first.json).toEqual({ error: "managed_credential_revoked", provider }); expect(first.body).not.toContain("access-norefresh"); + expect(first.body).not.toContain("access-empty"); const row = await rig.row(); expect(row.accessTokenEncrypted).toBeUndefined(); expect(row.refreshTokenEncrypted).toBeUndefined(); @@ -444,17 +472,56 @@ for (const { kind, live } of rigKinds) describe.skipIf(live && !adminUrl)(`manag ]); }); } else { - it("a response without a refresh token keeps the old one for a non-rotating provider", timeout, async () => { + it.each(["no_refresh_token", "empty_refresh_token"] as const)("a response with %s keeps the old one for a non-rotating provider", timeout, async (mode) => { const rig = await makeRig(provider); const endpoint = installTokenEndpoint(provider, "refresh-0"); - endpoint.mode = "no_refresh_token"; + endpoint.mode = mode; const answer = await rig.resolve(); expect(answer.statusCode).toBe(200); - expect(answer.json.accessToken).toBe("access-norefresh"); + expect(answer.json.accessToken).toBe(mode === "no_refresh_token" ? "access-norefresh" : "access-empty"); expect(rig.custody.decrypt((await rig.row()).refreshTokenEncrypted!)).toBe("refresh-0"); }); } + it.each([ + ["no expires_in", "no_expires_in", 3600], + ["an unusable expires_in", "unusable_expires_in", 3600], + ["a numeric-string expires_in", "string_expires_in", 7200], + ] as const)( + "refresh-once-per-token: an answer with %s stores now plus %s s, not the old due expiry, and resolve refreshes again only once it passes", + timeout, + async (_name, mode, seconds) => { + const rig = await makeRig(provider); + const endpoint = installTokenEndpoint(provider, "refresh-0"); + endpoint.mode = mode; + + const before = Date.now(); + expect((await rig.resolve()).statusCode).toBe(200); + expect(endpoint.calls).toBe(1); + const stored = Date.parse((await rig.row()).tokenExpiresAt!); + expect(stored - before).toBeGreaterThanOrEqual(seconds * 1000 - 1000); + expect(stored - before).toBeLessThan(seconds * 1000 + 10_000); + + expect((await rig.resolve()).statusCode).toBe(200); + expect(endpoint.calls).toBe(1); + + // The stored expiry passes: the next resolve refreshes. + const past = new Date(Date.now() - 1000).toISOString(); + await rig.stores[0]!.updateLinkedIdentityCredential(rig.identityId, async (locked) => ({ + kind: "rotate", + accessTokenEncrypted: locked.accessTokenEncrypted!, + refreshTokenEncrypted: locked.refreshTokenEncrypted!, + tokenExpiresAt: past, + scopes: locked.scopes, + updatedAt: past, + })); + expect((await rig.resolve()).statusCode).toBe(200); + expect(endpoint.calls).toBe(2); + const again = Date.parse((await rig.row()).tokenExpiresAt!); + expect(again).toBeGreaterThan(Date.now() + MANAGED_CREDENTIAL_REFRESH_SKEW_MS); + } + ); + it("a row with no access token resolves as revoked (410), a missing row as not found (404)", timeout, async () => { const rig = await makeRig(provider, { accessToken: null, refreshToken: null }); const endpoint = installTokenEndpoint(provider, "refresh-0"); @@ -882,6 +949,8 @@ describe("refresh answer classification", () => { ["a 2xx with JSON null", "null", 200], ["a 2xx with a JSON array", "[]", 200], ["a 2xx with a non-string access token", JSON.stringify({ access_token: 7, refresh_token: "r" }), 200], + ["a 2xx with a numeric refresh token", JSON.stringify({ access_token: "a", refresh_token: 42 }), 200], + ["a 2xx with an object refresh token", JSON.stringify({ access_token: "a", refresh_token: { r: 1 } }), 200], ["a 2xx with an empty access token", JSON.stringify({ access_token: "", refresh_token: "r" }), 200], ["a 2xx that is not JSON", "", 200], ["a 5xx page", "upstream exploded", 502], @@ -924,27 +993,50 @@ describe("refresh answer classification", () => { }); }); - it("a rotating provider's well-formed answer without a refresh token still revokes (rotation_missing)", async () => { + it.each([ + ["without a refresh token", { access_token: "a", expires_in: 3600 }], + ["with an empty refresh token", { access_token: "a", refresh_token: "", expires_in: 3600 }], + ])("a rotating provider's well-formed answer %s still revokes (rotation_missing)", async (_n, body) => { const store = new InMemoryStore(); const row = await seedRow(store, "joystick"); - answer(JSON.stringify({ access_token: "a", expires_in: 3600 }), 200); + answer(JSON.stringify(body), 200); expect((await refreshManagedCredential(ctxFor(store), row)).kind).toBe("revoked"); expect((await store.findStoredLinkedIdentity("joystick", row.providerUserId))!.refreshTokenEncrypted).toBeUndefined(); }); - it("an answer without expires_in stores the expiry as unknown, and the next resolve does not refresh again", async () => { + it.each([ + ["no expires_in", {}], + ["a non-numeric string expires_in", { expires_in: "soon" }], + ])("an answer with %s stores now plus the default lifetime; resolve refreshes once it passes", async (_n, extra) => { const store = new InMemoryStore(); - const row = await seedRow(store, "twitch"); - const calls = answer(JSON.stringify({ access_token: "access-1", refresh_token: "refresh-1" }), 200); - const first = await refreshManagedCredential(ctxFor(store), row); + const seeded = await seedRow(store, "twitch"); + // Due against the real clock, which the token answer's expiry is computed from. + const row = (await store.updateLinkedIdentityCredential(seeded.id, async (locked) => ({ + kind: "rotate", + accessTokenEncrypted: locked.accessTokenEncrypted!, + refreshTokenEncrypted: locked.refreshTokenEncrypted!, + tokenExpiresAt: new Date().toISOString(), + scopes: locked.scopes, + updatedAt: new Date().toISOString(), + })))!; + const calls = answer(JSON.stringify({ access_token: "access-1", refresh_token: "refresh-1", ...extra }), 200); + let skew = 0; + const ctx = { ...ctxFor(store), clock: () => Date.now() + skew }; + const before = Date.now(); + const first = await refreshManagedCredential(ctx, row); expect(first.kind).toBe("current"); - expect(first.identity.tokenExpiresAt).toBeUndefined(); const stored = (await store.findStoredLinkedIdentity("twitch", row.providerUserId))!; - expect(stored.tokenExpiresAt).toBeUndefined(); + const expiry = Date.parse(stored.tokenExpiresAt!); + expect(expiry - before).toBeGreaterThanOrEqual(providerCatalog.twitch.defaultAccessTokenLifetimeSeconds * 1000); + expect(expiry - before).toBeLessThan(providerCatalog.twitch.defaultAccessTokenLifetimeSeconds * 1000 + 5_000); expect(custody.decrypt(stored.accessTokenEncrypted!)).toBe("access-1"); - await refreshManagedCredential(ctxFor(store), stored); + await refreshManagedCredential(ctx, stored); expect(calls).toEqual(["refresh-0"]); + + skew = providerCatalog.twitch.defaultAccessTokenLifetimeSeconds * 1000; + await refreshManagedCredential(ctx, stored); + expect(calls).toEqual(["refresh-0", "refresh-1"]); }); it("a non-rotating provider keeps the locked row's refresh token, not the snapshot's", async () => { diff --git a/tests/provider-token-request.test.ts b/tests/provider-token-request.test.ts index 501a6e3..ff415fd 100644 --- a/tests/provider-token-request.test.ts +++ b/tests/provider-token-request.test.ts @@ -90,14 +90,70 @@ describe("requestProviderTokens follows the provider descriptor", () => { expect(calls[1]!.body.has("code")).toBe(false); }); + const everyProvider = Object.keys(providerCatalog) as Provider[]; + + it("gives every provider a default access-token lifetime of 3600 seconds", () => { + for (const provider of everyProvider) { + expect(providerCatalog[provider].defaultAccessTokenLifetimeSeconds).toBe(3600); + } + }); + + // expires_in is RECOMMENDED (RFC 6749 5.1): unusable means the default, never an unknown expiry. it.each([ - ["no expires_in", { access_token: "a" }], - ["a zero expires_in", { access_token: "a", expires_in: 0 }], - ["a string expires_in", { access_token: "a", expires_in: "3600" }], - ])("sets no expiry and no refresh token for %s", async (_n, body) => { - capture(() => new Response(JSON.stringify(body), { status: 200 })); + ["no expires_in", undefined, 3600], + ["a zero expires_in", 0, 3600], + ["a negative expires_in", -5, 3600], + ["a null expires_in", null, 3600], + ["an object expires_in", {}, 3600], + ["a non-numeric string", "abc", 3600], + ["an empty string", "", 3600], + ["a negative string", "-1", 3600], + ["a zero string", "0", 3600], + ["a fractional string", "12.5", 3600], + ["an exponent string", "1e3", 3600], + ["a numeric string", "7200", 7200], + ["a padded numeric string", " 7200 ", 7200], + ["a number", 7200, 7200], + ])("stores now plus the right lifetime for %s, on every provider and grant", async (_n, expiresIn, seconds) => { + for (const provider of everyProvider) { + for (const grant of [ + { grant_type: "authorization_code", code: "c", redirect_uri: "u" } as const, + { grant_type: "refresh_token", refresh_token: "R" } as const, + ]) { + capture(() => new Response(JSON.stringify({ access_token: "a", ...(expiresIn === undefined ? {} : { expires_in: expiresIn }) }), { status: 200 })); + const before = Date.now(); + const tokens = await requestProviderTokens(provider, configFor(), grant); + const stored = Date.parse(tokens.expiresAt ?? ""); + expect(stored - before).toBeGreaterThanOrEqual(seconds * 1000); + expect(stored - before).toBeLessThan(seconds * 1000 + 5_000); + expect(tokens.refreshToken).toBeUndefined(); + } + } + }); + + it("takes the default lifetime for an expires_in too large to be a date", async () => { + capture(() => new Response('{"access_token":"a","expires_in":1e999}', { status: 200 })); + const before = Date.now(); + const tokens = await requestProviderTokens("twitch", configFor(), { grant_type: "refresh_token", refresh_token: "R" }); + expect(Date.parse(tokens.expiresAt ?? "") - before).toBeLessThan(3600 * 1000 + 5_000); + }); + + it.each([ + ["a number", 42], + ["an object", { a: 1 }], + ["an array", ["r"]], + ["true", true], + ["null", null], + ])("refuses a 2xx whose refresh_token is %s as a malformed token set", async (_n, refreshToken) => { + capture(() => new Response(JSON.stringify({ access_token: "a", refresh_token: refreshToken, expires_in: 3600 }), { status: 200 })); + await expect( + requestProviderTokens("twitch", configFor(), { grant_type: "refresh_token", refresh_token: "R" }) + ).rejects.toMatchObject({ code: "provider_body_not_token_set" }); + }); + + it("reads an empty-string refresh_token as no refresh token", async () => { + capture(() => new Response(JSON.stringify({ access_token: "a", refresh_token: "", expires_in: 3600 }), { status: 200 })); const tokens = await requestProviderTokens("twitch", configFor(), { grant_type: "refresh_token", refresh_token: "R" }); - expect(tokens.expiresAt).toBeUndefined(); expect(tokens.refreshToken).toBeUndefined(); });