fix(spotify): collapse concurrent OAuth refresh + TTL/cap PKCE verifiers [S4.3]

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<T>() {
+  let resolve!: (v: T) => void;
+  let reject!: (e: unknown) => void;
+  const promise = new Promise<T>((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<any>();
+    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-"));

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<string, string>();
+  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<string | null> | 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<boolean> {
-    const verifier = this.pendingVerifiers.get(state);
+    const entry = this.pendingVerifiers.get(state);
-    if (!verifier) return false; // unknown/expired state -> CSRF guard
+    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<string | null> {
@@ -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,
     };
   }