fix: DPoP nonce retry race issue (#2580)

Author: nandan-bhat

src/server/auth-client-provider.test.ts

@@ -503,105 +503,6 @@ describe("AuthClientProvider", () => {
     });
   });
 
-  describe("proxy fetchers", () => {
-    it("should cache proxy fetcher by key", async () => {
-      const provider = new AuthClientProvider({
-        domain: "example.auth0.com",
-        createAuthClient: createAuthClientMock
-      });
-
-      const fetcher1 = { mock: "fetcher1" };
-      const fetcher2 = { mock: "fetcher2" };
-
-      const factory1 = vi.fn().mockResolvedValue(fetcher1);
-      const factory2 = vi.fn().mockResolvedValue(fetcher2);
-
-      const result1a = await provider.getProxyFetcher("key1", factory1);
-      const result1b = await provider.getProxyFetcher("key1", factory1);
-      const result2 = await provider.getProxyFetcher("key2", factory2);
-
-      expect(result1a).toBe(fetcher1);
-      expect(result1b).toBe(fetcher1); // Same fetcher from cache
-      expect(result1a).toBe(result1b); // From cache
-      expect(result2).toBe(fetcher2);
-      expect(factory1).toHaveBeenCalledTimes(1); // Only called once
-      expect(factory2).toHaveBeenCalledTimes(1);
-    });
-
-    it("should not call factory if fetcher is cached", async () => {
-      const provider = new AuthClientProvider({
-        domain: "example.auth0.com",
-        createAuthClient: createAuthClientMock
-      });
-
-      const factory = vi.fn().mockResolvedValue({ mock: "fetcher" });
-
-      await provider.getProxyFetcher("key", factory);
-      await provider.getProxyFetcher("key", factory);
-      await provider.getProxyFetcher("key", factory);
-
-      expect(factory).toHaveBeenCalledTimes(1);
-    });
-
-    it("should use LRU behavior for proxy fetchers", async () => {
-      const provider = new AuthClientProvider({
-        domain: "example.auth0.com",
-        createAuthClient: createAuthClientMock
-      });
-
-      // Fill cache to near capacity (MAX_PROXY_FETCHERS is 100)
-      const factories: { [key: string]: any } = {};
-      const fetchers: { [key: string]: any } = {};
-
-      // Create 99 fetchers to fill most of the cache
-      for (let i = 0; i < 99; i++) {
-        const key = `key${i}`;
-        const factory = vi.fn().mockResolvedValue({ id: `fetcher-${i}` });
-        factories[key] = factory;
-        fetchers[key] = await provider.getProxyFetcher(key, factory);
-      }
-
-      // Now we have 99 cached fetchers
-      // Add fetcher A (will be candidate for eviction if we exceed limit)
-      const factoryA = vi.fn().mockResolvedValue({ id: "fetcher-a" });
-      const fetcherA = await provider.getProxyFetcher("keyA", factoryA);
-      expect(fetcherA.id).toBe("fetcher-a");
-      expect(factoryA).toHaveBeenCalledTimes(1);
-
-      // We now have 100 cached fetchers (at max)
-
-      // Access A again - should promote it to end
-      const fetcherAAgain = await provider.getProxyFetcher("keyA", factoryA);
-      expect(fetcherAAgain).toBe(fetcherA);
-      expect(factoryA).toHaveBeenCalledTimes(1); // Still 1 call
-
-      // Add 2 more fetchers to trigger eviction
-      // This should evict the oldest entries (key0 and key1)
-      const factoryB = vi.fn().mockResolvedValue({ id: "fetcher-b" });
-      const factoryC = vi.fn().mockResolvedValue({ id: "fetcher-c" });
-      await provider.getProxyFetcher("keyB", factoryB);
-      await provider.getProxyFetcher("keyC", factoryC);
-
-      // Now key0 should be evicted (it was the oldest)
-      const factory0New = vi.fn().mockResolvedValue({ id: "fetcher-0-new" });
-      const _fetcher0Again = await provider.getProxyFetcher(
-        "key0",
-        factory0New
-      );
-      // key0 should have been evicted and factory should be called
-      expect(factory0New).toHaveBeenCalledTimes(1);
-
-      // A should still be cached (it was promoted)
-      const factoryANew = vi.fn().mockResolvedValue({ id: "fetcher-a-new" });
-      const fetcherAAgain2 = await provider.getProxyFetcher(
-        "keyA",
-        factoryANew
-      );
-      expect(fetcherAAgain2).toBe(fetcherA); // Same instance
-      expect(factoryANew).not.toHaveBeenCalled(); // Factory not called - was cached
-    });
-  });
-
   describe("mode detection", () => {
     it("should detect static mode", () => {
       const provider = new AuthClientProvider({

src/server/auth-client-provider.ts

@@ -34,13 +34,6 @@ interface AuthClientProviderOptions {
  */
 const MAX_DOMAIN_CLIENTS = 100;
 
-/**
- * Maximum number of proxy fetchers to cache.
- * This prevents unbounded memory growth when many unique audience/domain
- * combinations are used. Uses LRU eviction matching the domain clients pattern.
- */
-const MAX_PROXY_FETCHERS = 100;
-
 /**
  * AuthClientProvider manages creating and caching AuthClient instances for MCD mode.
  *
@@ -61,7 +54,6 @@ export class AuthClientProvider {
   private domainClients: LruMap<string, AuthClient>;
 
   private createAuthClientFactory: (domain: string) => AuthClient;
-  private proxyFetchers: LruMap<string, any>;
 
   /**
    * Creates a new AuthClientProvider instance.
@@ -76,7 +68,6 @@ export class AuthClientProvider {
 
     // Initialize LRU caches
     this.domainClients = new LruMap(MAX_DOMAIN_CLIENTS);
-    this.proxyFetchers = new LruMap(MAX_PROXY_FETCHERS);
 
     // Detect mode and validate configuration
     if (typeof options.domain === "string") {
@@ -192,33 +183,6 @@ export class AuthClientProvider {
     return newClient;
   }
 
-  /**
-   * Gets a proxy fetcher from cache or creates one via the provided factory.
-   *
-   * Proxy fetchers are cached per key to avoid creating multiple instances
-   * for the same audience or configuration.
-   *
-   * @param key - A unique key for the fetcher (e.g., "domain:audience")
-   * @param factory - Factory function to create the fetcher if not cached
-   * @returns The cached or newly created fetcher
-   *
-   * @internal
-   */
-  async getProxyFetcher(
-    key: string,
-    factory: () => Promise<any>
-  ): Promise<any> {
-    // Check cache first (LruMap.get() handles promotion)
-    const fetcher = this.proxyFetchers.get(key);
-    if (fetcher) {
-      return fetcher;
-    }
-    // Create new fetcher (LruMap.set() handles eviction)
-    const newFetcher = await factory();
-    this.proxyFetchers.set(key, newFetcher);
-    return newFetcher;
-  }
-
   /**
    * Resolves the domain from request headers using the resolver function.
    *

src/server/auth-client.test.ts

@@ -10039,6 +10039,66 @@ ykwV8CV22wKDubrDje1vchfTL/ygX6p27RKpJm8eAH7k3EwVeg3NDfNVzQ==
       expect(authClient["clientMetadata"][oauth.clockSkew]).toBe(15);
       expect(authClient["clientMetadata"][oauth.clockTolerance]).toBe(30);
     });
+
+    it("should ignore a provided dpopHandle when DPoP is disabled on the client", async () => {
+      const secret = await generateSecret(32);
+      const transactionStore = new TransactionStore({ secret });
+      const sessionStore = new StatelessSessionStore({ secret });
+      const dpopHandle = { privateKey: "test", publicKey: "test" } as any;
+
+      const authClient = new AuthClient({
+        transactionStore,
+        sessionStore,
+        domain: DEFAULT.domain,
+        clientId: DEFAULT.clientId,
+        clientSecret: DEFAULT.clientSecret,
+        secret,
+        appBaseUrl: DEFAULT.appBaseUrl,
+        routes: getDefaultRoutes(),
+        useDPoP: false,
+        fetch: getMockAuthorizationServer()
+      });
+
+      const fetcher = await authClient.fetcherFactory({
+        getAccessToken: vi.fn().mockResolvedValue("at_123"),
+        dpopHandle
+      });
+
+      expect((fetcher as any).config.dpopHandle).toBeUndefined();
+      expect((fetcher as any).hooks.isDpopEnabled()).toBe(false);
+    });
+
+    it("should ignore a provided dpopHandle when DPoP is disabled for the fetcher", async () => {
+      const secret = await generateSecret(32);
+      const transactionStore = new TransactionStore({ secret });
+      const sessionStore = new StatelessSessionStore({ secret });
+      const { generateDpopKeyPair } = await import("../utils/dpopRetry.js");
+      const dpopKeyPair = await generateDpopKeyPair();
+      const dpopHandle = { privateKey: "test", publicKey: "test" } as any;
+
+      const authClient = new AuthClient({
+        transactionStore,
+        sessionStore,
+        domain: DEFAULT.domain,
+        clientId: DEFAULT.clientId,
+        clientSecret: DEFAULT.clientSecret,
+        secret,
+        appBaseUrl: DEFAULT.appBaseUrl,
+        routes: getDefaultRoutes(),
+        useDPoP: true,
+        dpopKeyPair,
+        fetch: getMockAuthorizationServer()
+      });
+
+      const fetcher = await authClient.fetcherFactory({
+        getAccessToken: vi.fn().mockResolvedValue("at_123"),
+        useDPoP: false,
+        dpopHandle
+      });
+
+      expect((fetcher as any).config.dpopHandle).toBeUndefined();
+      expect((fetcher as any).hooks.isDpopEnabled()).toBe(false);
+    });
   });
 });
 

src/server/auth-client.ts

@@ -331,7 +331,7 @@ export class AuthClient {
 
   private readonly mfaTokenTtl: number;
 
-  private proxyFetchers: { [audience: string]: Fetcher<Response> } = {};
+  private proxyDpopHandles: { [audience: string]: oauth.DPoPHandle } = {};
 
   /**
    * Maximum allowed response body size (1 MB). Responses exceeding this limit
@@ -2948,12 +2948,14 @@ export class AuthClient {
       throw discoveryError;
     }
 
+    const shouldUseDpop = this.useDPoP && (options.useDPoP ?? true);
+
     const fetcherConfig: FetcherConfig<TOutput> = {
       // Fetcher-scoped DPoP handle and nonce management
-      dpopHandle:
-        this.useDPoP && (options.useDPoP ?? true)
-          ? oauth.DPoP(this.clientMetadata, this.dpopKeyPair!)
-          : undefined,
+      dpopHandle: shouldUseDpop
+        ? (options.dpopHandle ??
+          oauth.DPoP(this.clientMetadata, this.dpopKeyPair!))
+        : undefined,
       httpOptions: this.httpOptions,
       allowInsecureRequests: this.allowInsecureRequests,
       retryConfig: this.dpopOptions?.retry,
@@ -2964,7 +2966,7 @@ export class AuthClient {
 
     const fetcherHooks: FetcherHooks = {
       getAccessToken: options.getAccessToken,
-      isDpopEnabled: () => options.useDPoP ?? this.useDPoP ?? false
+      isDpopEnabled: () => shouldUseDpop
     };
 
     return new Fetcher<TOutput>(fetcherConfig, fetcherHooks);
@@ -3508,38 +3510,24 @@ export class AuthClient {
       return tokenSetResponse.tokenSet;
     };
 
-    // Get/create fetcher instance with domain-aware caching
-    // In MCD mode, use provider.getProxyFetcher for shared caching across AuthClient instances
-    // In static mode, use local proxyFetchers cache
-    const cacheKey = this.provider
-      ? `${this.domain}:${options.audience}`
-      : options.audience;
-
-    let fetcher: Fetcher<Response>;
-    if (this.provider) {
-      fetcher = await this.provider.getProxyFetcher(cacheKey, async () => {
-        return this.fetcherFactory({
-          useDPoP: this.useDPoP,
-          fetch: this.fetch,
-          getAccessToken: getAccessToken
-        });
-      });
-    } else {
-      // Static mode or no provider: use local cache
-      fetcher = this.proxyFetchers[options.audience];
-      if (!fetcher) {
-        fetcher = await this.fetcherFactory({
-          useDPoP: this.useDPoP,
-          fetch: this.fetch,
-          getAccessToken: getAccessToken
-        });
-        this.proxyFetchers[options.audience] = fetcher;
-      }
-    }
+    // Cache only the DPoP handle so nonce state is shared across proxied
+    // requests for the same audience on this AuthClient instance. Always create
+    // a fresh request-bound fetcher so token resolution remains scoped to the
+    // current session instead of being shared or mutated.
+    const dpopHandle =
+      this.useDPoP && this.dpopKeyPair
+        ? (this.proxyDpopHandles[options.audience] ??= oauth.DPoP(
+            this.clientMetadata,
+            this.dpopKeyPair
+          ))
+        : undefined;
 
-    // Override getAccessToken for the current request
-    // @ts-expect-error Override fetcher's getAccessToken to capture token set side effects
-    fetcher.getAccessToken = getAccessToken;
+    const fetcher = await this.fetcherFactory({
+      useDPoP: this.useDPoP,
+      fetch: this.fetch,
+      getAccessToken,
+      dpopHandle
+    });
 
     try {
       const response = await fetcher.fetchWithAuth(
@@ -3891,6 +3879,7 @@ type GetTokenSetResponse = {
 export type FetcherFactoryOptions<TOutput extends Response> = {
   useDPoP?: boolean;
   getAccessToken: AccessTokenFactory;
+  dpopHandle?: oauth.DPoPHandle;
 } & FetcherMinimalConfig<TOutput>;
 
 /**

src/server/mcd.integration.test.ts

@@ -732,57 +732,6 @@ describe("MCD Integration Tests (Units 6-12)", () => {
 
       expect(client.domain).toBe("example.com");
     });
-
-    it("U11-9: Proxy fetcher cached by key", async () => {
-      const createAuthClient = vi.fn(
-        () =>
-          ({
-            domain: "example.com"
-          }) as any
-      );
-
-      const provider = new AuthClientProvider({
-        domain: "example.com",
-        createAuthClient
-      });
-
-      const factory = vi.fn(async () => ({ fetch: vi.fn() }) as any);
-      const fetcher1 = await provider.getProxyFetcher(
-        "domain1:audience1",
-        factory
-      );
-      const fetcher2 = await provider.getProxyFetcher(
-        "domain1:audience1",
-        factory
-      );
-
-      expect(fetcher1).toBe(fetcher2);
-      expect(factory).toHaveBeenCalledTimes(1);
-    });
-
-    it("U11-10: Different keys have separate fetchers", async () => {
-      const createAuthClient = vi.fn(
-        () =>
-          ({
-            domain: "example.com"
-          }) as any
-      );
-
-      const provider = new AuthClientProvider({
-        domain: "example.com",
-        createAuthClient
-      });
-
-      const factory1 = vi.fn(async () => ({ fetch: "fetcher1" }) as any);
-      const factory2 = vi.fn(async () => ({ fetch: "fetcher2" }) as any);
-
-      const fetcher1 = await provider.getProxyFetcher("key1", factory1);
-      const fetcher2 = await provider.getProxyFetcher("key2", factory2);
-
-      expect(fetcher1).not.toBe(fetcher2);
-      expect(factory1).toHaveBeenCalledTimes(1);
-      expect(factory2).toHaveBeenCalledTimes(1);
-    });
   });
 
   // ===== Unit 12 Tests: openid Scope Enforcement =====