diff --git a/src/music/spotify/spotify-oauth.test.ts b/src/music/spotify/spotify-oauth.test.ts index 481d3e9..9b6b7ff 100644 --- a/src/music/spotify/spotify-oauth.test.ts +++ b/src/music/spotify/spotify-oauth.test.ts @@ -37,6 +37,17 @@ function memStore( return s; } +/** A manually-settled promise, to hold a token POST "in flight" during a test. */ +function deferred() { + let resolve!: (v: T) => void; + let reject!: (e: unknown) => void; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + return { promise, resolve, reject }; +} + describe("PKCE helpers", () => { it("generateCodeVerifier returns 64 chars from the unreserved set", () => { const v = generateCodeVerifier(); @@ -273,6 +284,131 @@ describe("SpotifyOAuth.getAccessToken", () => { }); }); +// S4.3: refresh-token rotation makes two concurrent refreshes race — the second +// would POST with a token the first already invalidated. Collapse them into one. +describe("SpotifyOAuth.getAccessToken (in-flight refresh, S4.3)", () => { + it("collapses concurrent refreshes into a single token POST", async () => { + let t = 0; + const now = () => t; + const store = memStore({ + accessToken: "old", + refreshToken: "r1", + expiresAt: 0, // now()=0 is not < 0 -> expired -> must refresh + scope: "s", + }); + const d = deferred(); + const http = { post: vi.fn().mockReturnValue(d.promise) } as any; + const oauth = new SpotifyOAuth({ + clientId: CLIENT_ID, + store, + deps: { http, now }, + }); + + // Fire two calls while the POST is still pending. + const p1 = oauth.getAccessToken(); + const p2 = oauth.getAccessToken(); + expect(http.post).toHaveBeenCalledTimes(1); // collapsed to ONE POST + + d.resolve({ + data: { access_token: "a2", refresh_token: "r2", expires_in: 3600 }, + }); + expect(await p1).toBe("a2"); + expect(await p2).toBe("a2"); + expect(http.post).toHaveBeenCalledTimes(1); + expect(store.value?.refreshToken).toBe("r2"); // rotated once, not twice + }); + + it("clears the in-flight refresh after it settles, allowing a later refresh", async () => { + let t = 0; + const now = () => t; + const store = memStore({ + accessToken: "old", + refreshToken: "r1", + expiresAt: 0, + scope: "s", + }); + const http = { + post: vi + .fn() + .mockResolvedValueOnce({ + data: { access_token: "a2", refresh_token: "r2", expires_in: 3600 }, + }) + .mockResolvedValueOnce({ + data: { access_token: "a3", refresh_token: "r3", expires_in: 3600 }, + }), + } as any; + const oauth = new SpotifyOAuth({ + clientId: CLIENT_ID, + store, + deps: { http, now }, + }); + + expect(await oauth.getAccessToken()).toBe("a2"); + expect(http.post).toHaveBeenCalledTimes(1); + + // toTokens now uses this.now(): saved expiresAt = 0 + 3600s - 30s skew. + // Still valid at t=0 -> cached, no new POST. + expect(await oauth.getAccessToken()).toBe("a2"); + expect(http.post).toHaveBeenCalledTimes(1); + + // Advance past the newly-saved expiry -> in-flight was cleared, so a fresh + // refresh fires (proves .finally() reset refreshInFlight). + t = 3600 * 1000; + expect(await oauth.getAccessToken()).toBe("a3"); + expect(http.post).toHaveBeenCalledTimes(2); + expect(store.value?.refreshToken).toBe("r3"); + }); +}); + +// S4.3: bound the PKCE verifier map so abandoned logins can't accumulate. +describe("SpotifyOAuth PKCE verifier TTL + cap (S4.3)", () => { + it("expires a pending verifier after the TTL (handleCallback returns false)", async () => { + let t = 0; + const now = () => t; + const http = { post: vi.fn() } as any; + const oauth = new SpotifyOAuth({ + clientId: CLIENT_ID, + redirectUri: REDIRECT_URI, + store: memStore(), + deps: { http, now }, + }); + const { state } = oauth.buildAuthorizeUrl(); + + t = 10 * 60 * 1000 + 1; // VERIFIER_TTL_MS + 1 + expect(await oauth.handleCallback("CODE", state)).toBe(false); + expect(http.post).not.toHaveBeenCalled(); // never reached the token step + }); + + it("caps the verifier map, evicting the oldest state (behavioral)", async () => { + let t = 0; + const now = () => t; + const http = { + post: vi.fn().mockResolvedValue({ + data: { access_token: "a1", refresh_token: "r1", expires_in: 3600 }, + }), + } as any; + const oauth = new SpotifyOAuth({ + clientId: CLIENT_ID, + redirectUri: REDIRECT_URI, + store: memStore(), + deps: { http, now }, + }); + + // First (oldest) state, then enough more to exceed VERIFIER_MAX (32). + const first = oauth.buildAuthorizeUrl().state; + let last = first; + for (let i = 0; i < 32; i++) last = oauth.buildAuthorizeUrl().state; + + // The oldest was evicted -> unknown state -> CSRF guard, no token POST. + expect(await oauth.handleCallback("CODE", first)).toBe(false); + expect(http.post).not.toHaveBeenCalled(); + + // A still-pending (newest) state DOES resolve -> only the oldest was dropped. + expect(await oauth.handleCallback("CODE", last)).toBe(true); + expect(http.post).toHaveBeenCalledTimes(1); + }); +}); + describe("createFileOAuthTokenStore", () => { it("round-trips save/load and clear() removes it", () => { const dir = mkdtempSync(join(tmpdir(), "sp-oauth-")); diff --git a/src/music/spotify/spotify-oauth.ts b/src/music/spotify/spotify-oauth.ts index b77232c..b574a8d 100644 --- a/src/music/spotify/spotify-oauth.ts +++ b/src/music/spotify/spotify-oauth.ts @@ -51,7 +51,7 @@ export interface SpotifyOAuthOptions { */ redirectUri?: string; store: OAuthTokenStore; - deps?: { http?: AxiosInstance }; + deps?: { http?: AxiosInstance; now?: () => number }; } /** 64 random chars from the PKCE unreserved set (43-128 allowed by the spec). */ @@ -111,8 +111,20 @@ export class SpotifyOAuth { private redirectUri: string; private store: OAuthTokenStore; private http: AxiosInstance; + // Injectable clock (tests drive a mutable now); defaults to Date.now. + private now: () => number; // Pending PKCE verifiers keyed by state, awaiting the loopback redirect back. - private pendingVerifiers = new Map(); + private pendingVerifiers = new Map< + string, + { verifier: string; expiresAt: number } + >(); + // A verifier is abandoned if the redirect never returns; drop it after TTL and + // cap the map so parallel logins can't grow it without bound. + private static readonly VERIFIER_TTL_MS = 10 * 60 * 1000; + private static readonly VERIFIER_MAX = 32; + // Collapse concurrent refreshes into one POST (rotation invalidates the token + // a second in-flight refresh would send); cleared in .finally(). + private refreshInFlight: Promise | null = null; constructor(o: SpotifyOAuthOptions) { this.clientId = o.clientId ?? ""; @@ -120,6 +132,20 @@ export class SpotifyOAuth { this.store = o.store; this.http = o.deps?.http ?? axios.create({ baseURL: ACCOUNTS_BASE, timeout: 15_000 }); + this.now = o.deps?.now ?? (() => Date.now()); + } + + private evictStaleVerifiers(): void { + const t = this.now(); + for (const [state, e] of this.pendingVerifiers) { + if (e.expiresAt < t) this.pendingVerifiers.delete(state); + } + // Bound memory even if all are unexpired: drop oldest (insertion order). + while (this.pendingVerifiers.size >= SpotifyOAuth.VERIFIER_MAX) { + const oldest = this.pendingVerifiers.keys().next().value; + if (oldest === undefined) break; + this.pendingVerifiers.delete(oldest); + } } getClientId(): string { @@ -142,7 +168,11 @@ export class SpotifyOAuth { } const state = randomBytes(16).toString("hex"); const verifier = generateCodeVerifier(); - this.pendingVerifiers.set(state, verifier); + this.evictStaleVerifiers(); + this.pendingVerifiers.set(state, { + verifier, + expiresAt: this.now() + SpotifyOAuth.VERIFIER_TTL_MS, + }); const params = new URLSearchParams({ client_id: this.clientId, response_type: "code", @@ -156,8 +186,13 @@ export class SpotifyOAuth { } async handleCallback(code: string, state: string): Promise { - const verifier = this.pendingVerifiers.get(state); - if (!verifier) return false; // unknown/expired state -> CSRF guard + const entry = this.pendingVerifiers.get(state); + if (!entry || entry.expiresAt < this.now()) { + // unknown/expired state -> CSRF guard (drop any stale entry too) + this.pendingVerifiers.delete(state); + return false; + } + const verifier = entry.verifier; // C3.7: drop the state->verifier entry on EVERY terminal path (success, // rejected token exchange, or throw) so a failed login can't leak/replay it. try { @@ -185,10 +220,16 @@ export class SpotifyOAuth { if (!this.clientId) return null; // C3.2: no app => nothing to mint against const tokens = this.store.load(); if (!tokens?.refreshToken) return null; // unauthorized - if (tokens.accessToken && Date.now() < tokens.expiresAt) { + if (tokens.accessToken && this.now() < tokens.expiresAt) { return tokens.accessToken; } - return this.refresh(tokens); + // Collapse concurrent refreshes: rotation makes a second in-flight refresh + // use a refresh token the first one already invalidated. + if (this.refreshInFlight) return this.refreshInFlight; + this.refreshInFlight = this.refresh(tokens).finally(() => { + this.refreshInFlight = null; + }); + return this.refreshInFlight; } private async refresh(current: OAuthTokens): Promise { @@ -218,7 +259,7 @@ export class SpotifyOAuth { return { accessToken: data.access_token, refreshToken, - expiresAt: Date.now() + (data.expires_in ?? 3600) * 1000 - EXPIRY_SKEW_MS, + expiresAt: this.now() + (data.expires_in ?? 3600) * 1000 - EXPIRY_SKEW_MS, scope: scope ?? SPOTIFY_CONTROL_SCOPES, }; }