fix(auth): read a DCR secret stamped for another client as absent (#2768)

clients/web/src/test/core/auth/oauth-secrets.test.ts

@@ -506,6 +506,93 @@ describe("client-credential owner stamps (#2734)", () => {
     expect(stampedOwner(stamp, "ics")).toBeUndefined();
   });
 
+  describe("join reads a value stamped for another client as absent (#2768)", () => {
+    const REG = issuerRegistrationTokenField(issuer);
+    const clientOf = (
+      residue: ServerOAuthState,
+      secrets: Record<string, string>,
+    ) =>
+      joinServerOAuthState(residue, secrets).byIssuer![issuer]!
+        .clientInformation;
+
+    it("drops a secret and registration token whose stamps name another client", () => {
+      expect(
+        clientOf(dcr("client-b"), {
+          [SECRET]: "secret-a",
+          [STAMP]: clientCredentialStamp("client-a", "secret-a"),
+          [REG]: "rat-a",
+          [clientCredentialStampField(REG)]: clientCredentialStamp(
+            "client-a",
+            "rat-a",
+          ),
+        }),
+      ).toEqual({ client_id: "client-b" });
+    });
+
+    it("drops only the mismatched field", () => {
+      expect(
+        clientOf(dcr("client-b"), {
+          [SECRET]: "secret-a",
+          [STAMP]: clientCredentialStamp("client-a", "secret-a"),
+          [REG]: "rat-b",
+          [clientCredentialStampField(REG)]: clientCredentialStamp(
+            "client-b",
+            "rat-b",
+          ),
+        }),
+      ).toEqual({ client_id: "client-b", registration_access_token: "rat-b" });
+    });
+
+    it.each([
+      ["no stamp", undefined],
+      ["a stamp for another value", clientCredentialStamp("client-a", "other")],
+    ])("still serves a value with %s", (_label, stamp) => {
+      expect(
+        clientOf(dcr("client-b"), {
+          [SECRET]: "secret-a",
+          ...(stamp !== undefined && { [STAMP]: stamp }),
+        }),
+      ).toEqual({ client_id: "client-b", client_secret: "secret-a" });
+    });
+
+    it("applies to the legacy unkeyed slot, but not to a pre-registered client", () => {
+      const joined = joinServerOAuthState(
+        {
+          clientInformation: { client_id: "client-b" },
+          preregisteredClientInformation: { client_id: "pre-b" },
+        },
+        {
+          [LEGACY_CLIENT_SECRET_FIELD]: "secret-a",
+          [clientCredentialStampField(LEGACY_CLIENT_SECRET_FIELD)]:
+            clientCredentialStamp("client-a", "secret-a"),
+          [PREREG_CLIENT_SECRET_FIELD]: "pre-secret",
+          [clientCredentialStampField(PREREG_CLIENT_SECRET_FIELD)]:
+            clientCredentialStamp("pre-a", "pre-secret"),
+        },
+      );
+      expect(joined.clientInformation).toEqual({ client_id: "client-b" });
+      expect(joined.preregisteredClientInformation).toEqual({
+        client_id: "pre-b",
+        client_secret: "pre-secret",
+      });
+    });
+
+    it("serves a stamped value to a slot with no client_id", () => {
+      expect(
+        joinServerOAuthState(
+          {
+            clientInformation: {} as ServerOAuthState["clientInformation"],
+          },
+          {
+            [LEGACY_CLIENT_SECRET_FIELD]: "secret-a",
+            [clientCredentialStampField(LEGACY_CLIENT_SECRET_FIELD)]:
+              clientCredentialStamp("client-a", "secret-a"),
+          },
+        ).clientInformation,
+      ).toEqual({ client_secret: "secret-a" });
+    });
+  });
+
   it("restamps only what no stamp vouches for, keeping a valid one whatever client it names", () => {
     const valid = clientCredentialStamp("earlier", "ics");
     expect(

clients/web/src/test/integration/auth/node/oauth-secrets-namespace.test.ts

@@ -1306,15 +1306,14 @@ describe("a stripped stamp re-adopted on another backend keeps what only the old
   });
 
   /**
-   * The issue's (#2734) steps 1–4 and the strip of step 5: client A saved
-   * to `secrets.json` and absorbed, client B registered in the file under
-   * the same stamp, absorbed again — the keychain keeps A's value — then
-   * the stamp stripped and the file re-adopted on a memory backend.
+   * The issue's (#2734) steps 1–4: client A saved to `secrets.json` and
+   * absorbed, then (with `reregister`) client B registered in the file
+   * under the same stamp, and absorbed again — the keychain keeps A's value.
    */
-  async function repeatedHandOff(
+  async function handOffTwice(
     keyring: SecretStore,
     reregister: boolean,
-  ): Promise<{ ns1: string; ns2: string }> {
+  ): Promise<void> {
     const secretsFile = join(tempDir, "secrets.json");
     const fileStore = () =>
       new FileSecretStore({ filePath: secretsFile, passphrase: "" });
@@ -1345,6 +1344,17 @@ describe("a stripped stamp re-adopted on another backend keeps what only the old
       if (savedFileEnv === undefined) delete process.env[SECRET_FILE_ENV];
       else process.env[SECRET_FILE_ENV] = savedFileEnv;
     }
+  }
+
+  /**
+   * {@link handOffTwice}, then the strip of the issue's step 5: the stamp
+   * stripped and the file re-adopted on a memory backend.
+   */
+  async function repeatedHandOff(
+    keyring: SecretStore,
+    reregister: boolean,
+  ): Promise<{ ns1: string; ns2: string }> {
+    await handOffTwice(keyring, reregister);
     const ns1 = namespaceOf(fileA);
     await stripStamp(fileA);
     await writeOAuthSections(
@@ -1357,6 +1367,29 @@ describe("a stripped stamp re-adopted on another backend keeps what only the old
     return { ns1, ns2: namespaceOf(fileA) };
   }
 
+  it("an ordinary read does not pair the secret a repeated hand-off kept with the client registered since (#2768)", async () => {
+    const keyring = new KeyringSecretStore();
+    await handOffTwice(keyring, true);
+    const ns = namespaceOf(fileA);
+    // The keychain kept A's value under the namespace the file still holds,
+    // and its stamp correctly names A; the file says B.
+    expect(
+      await keyring.get(oauthSecretServerId(SERVER, ns), SECRET_FIELD),
+    ).toBe("secret-a");
+    expect(clientOf(await readOAuthStore(fileA, keyring))).toEqual({
+      client_id: "client-b",
+    });
+  });
+
+  it("an ordinary read still serves the secret a repeated hand-off kept for its own client (#2768)", async () => {
+    const keyring = new KeyringSecretStore();
+    await handOffTwice(keyring, false);
+    expect(clientOf(await readOAuthStore(fileA, keyring))).toEqual({
+      client_id: "client-a",
+      client_secret: "secret-a",
+    });
+  });
+
   it("does not pair a secret a repeated hand-off kept with the client the file registered since (#2734)", async () => {
     const keyring = new KeyringSecretStore();
     const { ns1, ns2 } = await repeatedHandOff(keyring, true);

core/auth/node/oauth-secrets.ts

@@ -357,14 +357,37 @@ function splitClientInformation<T extends ClientInfoSecretKeys>(
  * Overlay a slot's stored bearer-grade keys back onto its residue
  * (store-wins). Only called when the residue slot exists — a secret with no
  * slot was cleared, and rejoining it would resurrect the cleared value.
+ *
+ * A DCR value whose stamp vouches for it but names a different client is
+ * read as absent (#2768): the file-to-keychain hand-off keeps an existing
+ * keychain value, so after a re-registration the keychain can still hold
+ * the previous client's secret, and pairing it with the new `client_id`
+ * fails only at the token endpoint. A value with no valid stamp (legacy, or
+ * rewritten by an older Inspector) is served as before — its owner is
+ * unknown, and an unknown owner is never guessed.
  */
 function joinClientInformation<T extends ClientInfoSecretKeys>(
   info: T,
   fields: ClientInfoFields,
   secrets: OAuthSecretValues,
 ): T {
-  const clientSecret = secrets[fields.clientSecret];
-  const registrationToken = secrets[fields.registrationToken];
+  const owned = (field: string): string | undefined => {
+    const value = secrets[field];
+    if (
+      value === undefined ||
+      !fields.stamped ||
+      typeof info.client_id !== "string"
+    ) {
+      return value;
+    }
+    const owner = stampedOwner(
+      secrets[clientCredentialStampField(field)],
+      value,
+    );
+    return owner === undefined || owner === info.client_id ? value : undefined;
+  };
+  const clientSecret = owned(fields.clientSecret);
+  const registrationToken = owned(fields.registrationToken);
   if (clientSecret === undefined && registrationToken === undefined) {
     return info;
   }