Merge branch 'v2/main' into v2/feat/2204-chained-skill-eval

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

@@ -207,6 +207,62 @@ describe("buildRevocationRequest", () => {
     expect(body(init).has("client_secret")).toBe(false);
   });
 
+  // #2222 asked whether `encodeURIComponent` is safe here, since §2.3.1 names
+  // the form-urlencoded algorithm (RFC 6749 Appendix B) and this is not it. It
+  // is safe, and these cases are what say so: each credential is checked on the
+  // wire *and* round-tripped through a real form-urldecoder, which is the
+  // decoder a compliant authorization server runs. Before this, no case in
+  // either suite could distinguish the two algorithms — the fixture decoded
+  // with `decodeURIComponent`, the encoder's own inverse, so every input passed
+  // by construction.
+  describe.each([
+    // The character the report turned on, and the answer to it:
+    // `encodeURIComponent` escapes `+` as `%2B` rather than leaving it bare, so
+    // a form-urldecoder never gets the chance to read it as a space. Base64
+    // secrets contain `+` routinely, which is what makes this the case worth
+    // pinning rather than reasoning about.
+    { what: "a plus", id: "cid", secret: "ab+cd", wire: "cid:ab%2Bcd" },
+    // A space is the one place the two algorithms visibly differ — `%20` here,
+    // `+` under `URLSearchParams`. Both decode to a space at a compliant
+    // server, and only `%20` also survives a server that decodes with
+    // `decodeURIComponent` alone, which is why the encoder was left as it is.
+    { what: "a space", id: "cid", secret: "ab cd", wire: "cid:ab%20cd" },
+    // The case the original encoding change was made for: an unencoded `:` in
+    // the id would move the separator and split the credential in the wrong
+    // place. It must keep working.
+    { what: "a colon", id: "c:id", secret: "sec", wire: "c%3Aid:sec" },
+    // `%` is what makes a raw credential undecodable rather than merely
+    // mis-decoded — an unescaped one starts an escape sequence that isn't.
+    { what: "a percent", id: "cid", secret: "s%ec", wire: "cid:s%25ec" },
+  ])("a credential containing $what", ({ id, secret, wire }) => {
+    const basic = (): string => {
+      const { init } = buildRevocationRequest({
+        endpoint: REVOKE_URL,
+        token: "r",
+        tokenTypeHint: "refresh_token",
+        clientInformation: { client_id: id, client_secret: secret },
+        supportedAuthMethods: ["client_secret_basic"],
+      });
+      return String(headerOf(init, "Authorization")).slice("Basic ".length);
+    };
+
+    it("is percent-encoded on the wire", () => {
+      expect(atob(basic())).toBe(wire);
+    });
+
+    it("survives a compliant server's form-urldecode", () => {
+      const decoded = atob(basic());
+      const separator = decoded.indexOf(":");
+      // `+` to space *before* percent-decoding, exactly as `test-servers`'
+      // `/oauth/revoke` now does. Written out here rather than imported so the
+      // assertion does not lean on the fixture it exists to corroborate.
+      const formUrlDecode = (v: string): string =>
+        decodeURIComponent(v.replace(/\+/g, "%20"));
+      expect(formUrlDecode(decoded.slice(0, separator))).toBe(id);
+      expect(formUrlDecode(decoded.slice(separator + 1))).toBe(secret);
+    });
+  });
+
   it("sends the secret in the body for client_secret_post", () => {
     const { init } = buildRevocationRequest({
       endpoint: REVOKE_URL,
@@ -291,7 +347,16 @@ describe("revokeToken", () => {
   // persisted client id or secret and makes `encodeURIComponent` throw. Every
   // caller has already cleared its local state by the time this runs, so a
   // rejection here would break the documented best-effort guarantee.
+  //
+  // The stub is a real `Response` rather than a bare `vi.fn()`: an unencodable
+  // credential must fail *before* the request goes out, and against a stub that
+  // returns nothing the assertion would hold either way — reading `.ok` off
+  // `undefined` throws into the same `catch`. Asserting the fetch was never
+  // called is what makes this about the encoder (#2222).
   it("reports an unencodable credential as failed rather than throwing", async () => {
+    const fetchFn = vi.fn<typeof fetch>(
+      async () => new Response(null, { status: 200 }),
+    );
     const outcome = await revokeToken({
       endpoint: REVOKE_URL,
       token: "r",
@@ -302,9 +367,10 @@ describe("revokeToken", () => {
         client_secret: `bad${String.fromCharCode(0xd800)}`,
       },
       supportedAuthMethods: ["client_secret_basic"],
-      fetchFn: vi.fn<typeof fetch>(),
+      fetchFn,
     });
 
+    expect(fetchFn).not.toHaveBeenCalled();
     expect(outcome).toMatchObject({ status: "failed", endpoint: REVOKE_URL });
   });
 

clients/web/src/test/integration/auth/revocation-e2e.test.ts

@@ -39,6 +39,16 @@ const REDIRECT_URL = "http://localhost:3000/oauth/callback";
 /** A second registered client, used to prove tokens are not cross-revocable. */
 const OTHER_CLIENT_ID = "test-2144-other";
 const OTHER_CLIENT_SECRET = "test-2144-other-secret";
+/**
+ * A third client whose secret carries the character #2222 was filed about — a
+ * `+`, which the two encoding algorithms treat differently and which base64
+ * secrets (the kind an authorization server most often hands out) contain
+ * routinely. The Inspector's encoding turns out to handle it, but nothing here
+ * demonstrated that before, so this client is what makes the claim testable
+ * rather than argued.
+ */
+const PLUS_CLIENT_ID = "test-2222-plus";
+const PLUS_CLIENT_SECRET = "aG9sZA+bXk/beer=";
 
 function base64Url(buffer: Buffer): string {
   return buffer
@@ -71,6 +81,11 @@ describe("OAuth token revocation (RFC 7009)", () => {
             clientSecret: OTHER_CLIENT_SECRET,
             redirectUris: [REDIRECT_URL],
           },
+          {
+            clientId: PLUS_CLIENT_ID,
+            clientSecret: PLUS_CLIENT_SECRET,
+            redirectUris: [REDIRECT_URL],
+          },
         ],
       }),
     });
@@ -93,7 +108,10 @@ describe("OAuth token revocation (RFC 7009)", () => {
   }, 30_000);
 
   /** Run a real authorization-code exchange and return the issued tokens. */
-  async function authorize(): Promise<{
+  async function authorize(
+    clientId: string = CLIENT_ID,
+    clientSecret: string = CLIENT_SECRET,
+  ): Promise<{
     access_token: string;
     refresh_token: string;
   }> {
@@ -105,7 +123,7 @@ describe("OAuth token revocation (RFC 7009)", () => {
       headers: { "Content-Type": "application/x-www-form-urlencoded" },
       redirect: "manual",
       body: new URLSearchParams({
-        client_id: CLIENT_ID,
+        client_id: clientId,
         redirect_uri: REDIRECT_URL,
         response_type: "code",
         scope: "mcp",
@@ -125,8 +143,8 @@ describe("OAuth token revocation (RFC 7009)", () => {
         grant_type: "authorization_code",
         code: code!,
         redirect_uri: REDIRECT_URL,
-        client_id: CLIENT_ID,
+        client_id: clientId,
-        client_secret: CLIENT_SECRET,
+        client_secret: clientSecret,
         code_verifier: verifier,
       }),
     });
@@ -146,10 +164,14 @@ describe("OAuth token revocation (RFC 7009)", () => {
     return response.status !== 401;
   }
 
-  async function seededStorage(tokens: {
+  async function seededStorage(
-    access_token: string;
+    tokens: {
-    refresh_token?: string;
+      access_token: string;
-  }): Promise<NodeOAuthStorage> {
+      refresh_token?: string;
+    },
+    clientId: string = CLIENT_ID,
+    clientSecret: string = CLIENT_SECRET,
+  ): Promise<NodeOAuthStorage> {
     const storage = new NodeOAuthStorage(join(storageDir, "oauth.json"));
     await storage.clear(serverUrl);
     // Issuer-bound and matching the discovered metadata: an unkeyed grant
@@ -164,8 +186,8 @@ describe("OAuth token revocation (RFC 7009)", () => {
     // server with `oauth.clientId` uses. That is the slot the revocation path
     // must read first, or a confidential client sends no authentication at all.
     await storage.savePreregisteredClientInformation(serverUrl, {
-      client_id: CLIENT_ID,
+      client_id: clientId,
-      client_secret: CLIENT_SECRET,
+      client_secret: clientSecret,
     });
     return storage;
   }
@@ -269,4 +291,47 @@ describe("OAuth token revocation (RFC 7009)", () => {
       status: "revoked",
     });
   });
+
+  // #2222. The fixture used to decode with `decodeURIComponent`, the exact
+  // inverse of the encoder under test, so the round trip succeeded for every
+  // input and no case in this file could tell a correct encoder from a wrong
+  // one. `/oauth/revoke` now runs the form-urldecode a compliant authorization
+  // server runs, which is what gives this case teeth: it passes only because
+  // `encodeURIComponent` escapes `+` as `%2B`, and would fail against an
+  // encoder that emitted a bare one.
+  it("revokes a grant whose client secret contains a plus", async () => {
+    const tokens = await authorize(PLUS_CLIENT_ID, PLUS_CLIENT_SECRET);
+    expect(await tokenAccepted(tokens.access_token)).toBe(true);
+
+    const outcome = await clearAndRevoke(
+      await seededStorage(tokens, PLUS_CLIENT_ID, PLUS_CLIENT_SECRET),
+    );
+
+    expect(outcome).toMatchObject({
+      status: "revoked",
+      tokenTypeHint: "refresh_token",
+    });
+    expect(await tokenAccepted(tokens.access_token)).toBe(false);
+  });
+
+  // The guard on the fixture itself, and the one case that separates the two
+  // decoders. An *unencoded* credential is what the SDK's `applyBasicAuth`
+  // sends, and a compliant server form-urldecodes it anyway — so a `+` in the
+  // secret comes back as a space and the credential is refused. Under the old
+  // `decodeURIComponent` fixture this same request was accepted (a string with
+  // no `%` in it decodes to itself), which is precisely why that fixture could
+  // not fail on an encoding mistake. Revert `/oauth/revoke` and this test goes
+  // red.
+  it("refuses an unencoded Basic credential whose secret contains a plus", async () => {
+    const raw = `${PLUS_CLIENT_ID}:${PLUS_CLIENT_SECRET}`;
+    const response = await fetch(`${serverUrl}/oauth/revoke`, {
+      method: "POST",
+      headers: {
+        "Content-Type": "application/x-www-form-urlencoded",
+        Authorization: `Basic ${Buffer.from(raw).toString("base64")}`,
+      },
+      body: new URLSearchParams({ token: "anything" }),
+    });
+    expect(response.status).toBe(401);
+  });
 });

core/auth/revocation.ts

@@ -163,6 +163,26 @@ export function buildRevocationRequest(params: RevocationRequestParams): {
       // there is no precedent to match, and the raw form is ambiguous for a
       // client id containing `:` and makes `btoa` throw outright on a
       // non-Latin-1 secret. Encoding is what the server decodes.
+      //
+      // ⚠️ `encodeURIComponent` is **not** the form-urlencoded algorithm §2.3.1
+      // names (RFC 6749 Appendix B), and #2222 was filed on the assumption that
+      // the difference breaks a secret containing `+`. It does not, and the
+      // reasoning is worth keeping because the next reader will have the same
+      // doubt: `encodeURIComponent` **escapes** `+` as `%2B`. The characters it
+      // leaves bare are exactly `!'()*-._~` plus alphanumerics, and a
+      // form-urldecoder passes every one of them through unchanged. It can
+      // therefore never emit the one character the two algorithms disagree
+      // about, so its output decodes identically under both — verified over
+      // every code point up to U+2FFF, and pinned by the round-trip cases in
+      // `revocation.test.ts`.
+      //
+      // Switching to `URLSearchParams` would be the literal algorithm and a
+      // small *regression*: it encodes a space as `+`, which a compliant server
+      // reads back as a space but a lenient one — decoding with
+      // `decodeURIComponent` alone, having never implemented the `+` rule —
+      // reads as a literal `+`. `%20` is understood by both. So the encoding
+      // here is the one that survives either server, which matters more than
+      // matching the spec's wording for a header no server sees twice.
       const credentials = `${encodeURIComponent(client.client_id)}:${encodeURIComponent(client.client_secret)}`;
       headers.Authorization = `Basic ${base64Encode(credentials)}`;
     } else if (method === "client_secret_post" && client.client_secret) {

test-servers/src/test-server-oauth.ts

@@ -819,8 +819,8 @@ async function authenticateRevocationClient(
     // turn into a 500 — so a bad credential would be reported as a server
     // fault rather than as the `invalid_client` 401 this endpoint means.
     try {
-      clientId = decodeURIComponent(decoded.slice(0, separator));
+      clientId = formUrlDecode(decoded.slice(0, separator));
-      clientSecret = decodeURIComponent(decoded.slice(separator + 1));
+      clientSecret = formUrlDecode(decoded.slice(separator + 1));
     } catch {
       return null;
     }
@@ -841,6 +841,35 @@ async function authenticateRevocationClient(
   return ok ? clientId : null;
 }
 
+/**
+ * Decode one half of a Basic credential the way a compliant authorization
+ * server does — the `application/x-www-form-urlencoded` algorithm RFC 6749
+ * §2.3.1 names, not `decodeURIComponent`.
+ *
+ * The distinction is the whole point of this helper, and #2222 is what it cost
+ * to learn: this fixture used to decode with `decodeURIComponent`, the exact
+ * inverse of the encoder it was testing. The round trip then succeeded for
+ * **every** input — so no test here could have failed on an encoding mistake,
+ * and the suite's apparent coverage of client authentication was really a
+ * statement that the encoder is self-consistent. (The encoder was in fact
+ * fine; that was established by reasoning and a sweep over the code-point
+ * space, not by anything this fixture asserted, which is the gap being
+ * closed.)
+ *
+ * A form-urldecoder reads a bare `+` as a space, so that substitution happens
+ * **before** percent-decoding; doing it after would turn a legitimate escaped
+ * `%2B` into a space too. `%20` still decodes to a space, which is why the
+ * Inspector's `encodeURIComponent` output — which escapes `+` and spaces both,
+ * and never emits a bare `+` — round-trips through this decoder unchanged.
+ *
+ * `decodeURIComponent` remains the right primitive for the percent half, and it
+ * still throws on a malformed escape — which the caller catches, keeping a bad
+ * credential a 401 rather than an Express 500.
+ */
+function formUrlDecode(value: string): string {
+  return decodeURIComponent(value.replace(/\+/g, "%20"));
+}
+
 /**
  * Set up Dynamic Client Registration endpoint
  */