fix(auth): keep a static client's SEP-2352 issuer stamp across saves and restarts

clients/web/src/test/core/auth/providers.test.ts

@@ -6,6 +6,10 @@ import {
   BaseOAuthClientProvider,
   type OAuthProviderConfig,
 } from "@inspector/core/auth/providers.js";
+import {
+  auth,
+  type StoredOAuthClientInformation,
+} from "@modelcontextprotocol/client";
 import type { OAuthStorage } from "@inspector/core/auth/storage.js";
 import { OAuthStorageBase } from "@inspector/core/auth/oauth-storage.js";
 import { OAuthMemoryStore } from "@inspector/core/auth/store.js";
@@ -636,6 +640,259 @@ describe("OAuthNavigation", () => {
         );
       });
 
+      // #2751: the static client's stamp used to land in a dynamic slot the
+      // preregistered read never looks at, and re-seeding dropped it anyway.
+      describe("static client issuer stamp", () => {
+        const ISSUER = "https://as.example.com";
+        const STATIC = { client_id: "static", client_secret: "s3cret" };
+
+        function makeStaticStorage(
+          preregistered: StoredOAuthClientInformation | undefined,
+        ): OAuthStorage {
+          const storage = makeStorage();
+          vi.mocked(storage.getClientInformation).mockImplementation(
+            async (_url: string, isPreregistered?: boolean) =>
+              isPreregistered ? preregistered : undefined,
+          );
+          return storage;
+        }
+
+        it("writes the SDK back-stamp of the static client to the preregistered slot", async () => {
+          const storage = makeStaticStorage(STATIC);
+          const provider = makeProvider(storage);
+
+          await provider.saveClientInformation(
+            { ...STATIC, issuer: ISSUER },
+            { issuer: ISSUER },
+          );
+
+          expect(
+            storage.savePreregisteredClientInformation,
+          ).toHaveBeenCalledWith(SERVER, { ...STATIC, issuer: ISSUER });
+          expect(storage.saveClientInformation).not.toHaveBeenCalled();
+        });
+
+        it("keeps a different client_id in its registration slot", async () => {
+          const storage = makeStaticStorage(STATIC);
+          const provider = makeProvider(storage);
+
+          await provider.saveClientInformation(
+            { client_id: "dyn" },
+            { issuer: ISSUER },
+          );
+
+          expect(
+            storage.savePreregisteredClientInformation,
+          ).not.toHaveBeenCalled();
+          expect(storage.saveClientInformation).toHaveBeenCalledWith(
+            SERVER,
+            { client_id: "dyn" },
+            { registrationKind: "dcr", issuer: ISSUER },
+          );
+        });
+
+        it("leaves our own saves alone even when the client_id matches", async () => {
+          const storage = makeStaticStorage(STATIC);
+          const provider = makeProvider(storage);
+
+          await provider.saveClientInformation(STATIC, {
+            registrationKind: "cimd",
+            issuer: ISSUER,
+          });
+
+          expect(
+            storage.savePreregisteredClientInformation,
+          ).not.toHaveBeenCalled();
+          expect(storage.getClientInformation).not.toHaveBeenCalled();
+        });
+
+        it("returns the static client with a matching stamp as stored", async () => {
+          const storage = makeStaticStorage({ ...STATIC, issuer: ISSUER });
+          const provider = makeProvider(storage);
+
+          expect(await provider.clientInformation({ issuer: ISSUER })).toEqual({
+            ...STATIC,
+            issuer: ISSUER,
+          });
+          expect(await provider.clientInformation()).toEqual({
+            ...STATIC,
+            issuer: ISSUER,
+          });
+        });
+
+        it("returns the static client unstamped when the stamp names another issuer", async () => {
+          const storage = makeStaticStorage({ ...STATIC, issuer: ISSUER });
+          const provider = makeProvider(storage);
+
+          expect(
+            await provider.clientInformation({
+              issuer: "https://other-as.example.com",
+            }),
+          ).toEqual({ ...STATIC, issuer: undefined });
+        });
+
+        it("keeps the stamp when re-seeding the same static client", async () => {
+          const storage = makeStaticStorage({ ...STATIC, issuer: ISSUER });
+          const provider = makeProvider(storage);
+
+          await provider.savePreregisteredClientInformation(STATIC);
+
+          expect(
+            storage.savePreregisteredClientInformation,
+          ).toHaveBeenCalledWith(SERVER, { ...STATIC, issuer: ISSUER });
+        });
+
+        it.each([
+          ["client_id", { ...STATIC, client_id: "rotated" }],
+          ["client_secret", { ...STATIC, client_secret: "rotated" }],
+        ])(
+          "drops the stamp when re-seeding with a changed %s",
+          async (_field, changed) => {
+            const storage = makeStaticStorage({ ...STATIC, issuer: ISSUER });
+            const provider = makeProvider(storage);
+
+            await provider.savePreregisteredClientInformation(changed);
+
+            expect(
+              storage.savePreregisteredClientInformation,
+            ).toHaveBeenCalledWith(SERVER, changed);
+          },
+        );
+
+        // The regression end to end: real storage, the real startup seed and
+        // the SDK's own `auth()`, whose unstamped-credential warning is what
+        // repeated on every run.
+        describe("across SDK auth() runs", () => {
+          const ISSUER_B = "https://as-b.example.com";
+
+          /**
+           * A persisted store that outlives any one process: each
+           * {@link startProcess} reloads it into fresh storage and memory, so a
+           * stamp that does not survive serialization is lost between runs.
+           */
+          function makeDisk() {
+            let saved: string | null = null;
+            const backend: OAuthPersistBackend = {
+              read: async () => (saved === null ? null : JSON.parse(saved)),
+              write: async (snapshot) => {
+                saved = JSON.stringify(snapshot);
+              },
+            };
+            return {
+              startProcess: (): OAuthStorage =>
+                new OAuthStorageBase(new OAuthMemoryStore(), backend),
+            };
+          }
+
+          /** Discovery naming `issuer` as the resource's authorization server. */
+          function discoveryFetch(issuer: string): typeof fetch {
+            return async (input: RequestInfo | URL) => {
+              const url = String(input);
+              if (url.includes("/.well-known/oauth-protected-resource")) {
+                return new Response(
+                  JSON.stringify({
+                    resource: SERVER,
+                    authorization_servers: [issuer],
+                  }),
+                );
+              }
+              if (url.startsWith(issuer)) {
+                return new Response(
+                  JSON.stringify({
+                    issuer,
+                    authorization_endpoint: `${issuer}/authorize`,
+                    token_endpoint: `${issuer}/token`,
+                    response_types_supported: ["code"],
+                  }),
+                );
+              }
+              throw new Error(`unexpected fetch: ${url}`);
+            };
+          }
+
+          /** One process lifetime: seed from config, then let the SDK authorize. */
+          async function startAndAuthorize(
+            storage: OAuthStorage,
+            issuer: string,
+          ) {
+            const redirectUrlProvider = new MutableRedirectUrlProvider();
+            redirectUrlProvider.redirectUrl = "http://localhost/oauth/callback";
+            const provider = makeProvider(storage, vi.fn(), {
+              redirectUrlProvider,
+            });
+            await provider.savePreregisteredClientInformation(STATIC);
+            expect(
+              await auth(provider, {
+                serverUrl: SERVER,
+                fetchFn: discoveryFetch(issuer),
+              }),
+            ).toBe("REDIRECT");
+          }
+
+          let warn: ReturnType<typeof vi.spyOn>;
+          beforeEach(() => {
+            warn = vi.spyOn(console, "warn").mockImplementation(() => {});
+          });
+          afterEach(() => {
+            warn.mockRestore();
+          });
+
+          function sep2352Warnings(): unknown[][] {
+            return warn.mock.calls.filter((args: unknown[]) =>
+              String(args[0]).includes("SEP-2352"),
+            );
+          }
+
+          it("keeps the stamp across a restart, so the SDK warns only once", async () => {
+            const disk = makeDisk();
+
+            await startAndAuthorize(disk.startProcess(), ISSUER);
+            // First use: nothing was stamped yet, so the SDK warns and stamps.
+            expect(sep2352Warnings()).toHaveLength(1);
+            warn.mockClear();
+
+            const storage = disk.startProcess();
+            await startAndAuthorize(storage, ISSUER);
+
+            expect(sep2352Warnings()).toHaveLength(0);
+            expect(await storage.getClientInformation(SERVER, true)).toEqual({
+              ...STATIC,
+              issuer: ISSUER,
+            });
+            expect(
+              await storage.getClientInformation(SERVER, false, ISSUER),
+            ).toBeUndefined();
+          });
+
+          it("re-binds the static client when discovery resolves another issuer", async () => {
+            const disk = makeDisk();
+            const first = disk.startProcess();
+            await startAndAuthorize(first, ISSUER);
+            // SDK auth() reuses the cached AS; drop it so discovery re-resolves.
+            await first.clearDiscoveryState(SERVER);
+
+            const storage = disk.startProcess();
+            await startAndAuthorize(storage, ISSUER_B);
+
+            expect(await storage.getClientInformation(SERVER, true)).toEqual({
+              ...STATIC,
+              issuer: ISSUER_B,
+            });
+          });
+        });
+
+        it("seeds an unstamped static client as given", async () => {
+          const storage = makeStaticStorage({ ...STATIC });
+          const provider = makeProvider(storage);
+
+          await provider.savePreregisteredClientInformation(STATIC);
+
+          expect(
+            storage.savePreregisteredClientInformation,
+          ).toHaveBeenCalledWith(SERVER, STATIC);
+        });
+      });
+
       // #2242: the SDK binds an existing registration to its issuer by calling
       // `saveClientInformation(info, { issuer })` with no registration kind.
       // Treating every such save as DCR relabeled a CIMD registration
@@ -795,8 +1052,13 @@ describe("OAuthNavigation", () => {
             { issuer: ISSUER },
           );
 
-          // The id is not the metadata URL, so nothing is read at all.
-          expect(storage.getClientInformation).not.toHaveBeenCalled();
+          // The id is not the metadata URL, so no registration slot is read —
+          // only the preregistered one, to rule out a static back-stamp (#2751).
+          expect(storage.getClientInformation).not.toHaveBeenCalledWith(
+            SERVER,
+            false,
+            ISSUER,
+          );
           expect(storage.getDiscoveryState).not.toHaveBeenCalled();
           expect(storage.saveClientInformation).toHaveBeenCalledWith(
             SERVER,

core/auth/oauth-storage.ts

@@ -1,5 +1,6 @@
 import type {
   OAuthClientInformation,
+  StoredOAuthClientInformation as SdkStoredOAuthClientInformation,
   OAuthTokens,
   OAuthMetadata,
   OAuthDiscoveryState,
@@ -36,6 +37,17 @@ function withIssuer<T extends object>(value: T, issuer: string | undefined): T {
   return issuer === undefined ? value : { ...value, issuer };
 }
 
+/**
+ * The `issuer` stamp a preregistered client was saved with. A static client
+ * has no `byIssuer` key to recover its stamp from, so it carries the stamp on
+ * the stored value itself (#2751).
+ */
+function storedIssuerStamp(value: object): string | undefined {
+  return "issuer" in value && typeof value.issuer === "string"
+    ? value.issuer
+    : undefined;
+}
+
 /**
  * Load/persist coordination for one logical store: the load-once latch and
  * the write-serializing persist queue. Instances that share an
@@ -198,7 +210,10 @@ export class OAuthStorageBase implements OAuthStorage {
     if (isPreregistered) {
       const prereg = state.preregisteredClientInformation;
       return prereg
-        ? await OAuthClientInformationSchema.parseAsync(prereg)
+        ? withIssuer(
+            await OAuthClientInformationSchema.parseAsync(prereg),
+            storedIssuerStamp(prereg),
+          )
         : undefined;
     }
 
@@ -261,7 +276,7 @@ export class OAuthStorageBase implements OAuthStorage {
 
   async savePreregisteredClientInformation(
     serverUrl: string,
-    clientInformation: OAuthClientInformation,
+    clientInformation: SdkStoredOAuthClientInformation,
   ): Promise<void> {
     await this.ensureLoaded();
     this.memory.getState().setServerState(serverUrl, {