From 403352357b67aac8de41dff995333903fe63142c Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 19:29:02 -0400 Subject: [PATCH] test(auth): make the revocation fixture a real form-urldecoder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #2222 reported that `core/auth/revocation.ts` breaks a client secret containing `+`, on the basis that `encodeURIComponent` leaves `+` bare. It does not — it escapes it as `%2B`. The characters it leaves bare are exactly `!'()*-._~` and alphanumerics, every one of which a form- urldecoder passes through unchanged, so its output decodes identically under both algorithms; verified over every code point up to U+2FFF. Switching to `URLSearchParams` would be the literal algorithm §2.3.1 names and a small regression: it encodes a space as `+`, which a lenient server decoding with `decodeURIComponent` alone reads as a literal `+`, while `%20` is understood by both. The encoder is therefore unchanged and the reasoning is recorded on it. The report's second half stands and is the real defect: the fixture decoded with `decodeURIComponent`, the encoder's own inverse, so the round trip succeeded for every input by construction and no test could have failed on an encoding mistake. `/oauth/revoke` now runs a genuine RFC 6749 Appendix B decode, with unit cases for `+`, a space, `:` and `%` (asserted on the wire and through an independent decoder), an end-to-end client whose secret is base64 with a `+` in it, and a guard that an unencoded credential is refused — the case that separates the two decoders. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BhVaufrEZAfFAXcg6456hK Signed-off-by: cliffhall --- .../web/src/test/core/auth/revocation.test.ts | 68 ++++++++++++++- .../integration/auth/revocation-e2e.test.ts | 85 ++++++++++++++++--- core/auth/revocation.ts | 20 +++++ test-servers/src/test-server-oauth.ts | 33 ++++++- 4 files changed, 193 insertions(+), 13 deletions(-) diff --git a/clients/web/src/test/core/auth/revocation.test.ts b/clients/web/src/test/core/auth/revocation.test.ts index d0dd7ae15..fe5c29f15 100644 --- a/clients/web/src/test/core/auth/revocation.test.ts +++ b/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( + 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(), + fetchFn, }); + expect(fetchFn).not.toHaveBeenCalled(); expect(outcome).toMatchObject({ status: "failed", endpoint: REVOKE_URL }); }); diff --git a/clients/web/src/test/integration/auth/revocation-e2e.test.ts b/clients/web/src/test/integration/auth/revocation-e2e.test.ts index 0e3b33730..dfbd4d0c1 100644 --- a/clients/web/src/test/integration/auth/revocation-e2e.test.ts +++ b/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_secret: CLIENT_SECRET, + client_id: clientId, + client_secret: clientSecret, code_verifier: verifier, }), }); @@ -146,10 +164,14 @@ describe("OAuth token revocation (RFC 7009)", () => { return response.status !== 401; } - async function seededStorage(tokens: { - access_token: string; - refresh_token?: string; - }): Promise { + async function seededStorage( + tokens: { + access_token: string; + refresh_token?: string; + }, + clientId: string = CLIENT_ID, + clientSecret: string = CLIENT_SECRET, + ): Promise { 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_secret: CLIENT_SECRET, + client_id: clientId, + 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); + }); }); diff --git a/core/auth/revocation.ts b/core/auth/revocation.ts index 8a438ccdc..3514e4a43 100644 --- a/core/auth/revocation.ts +++ b/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) { diff --git a/test-servers/src/test-server-oauth.ts b/test-servers/src/test-server-oauth.ts index 6d06c8dac..e73e0e7a1 100644 --- a/test-servers/src/test-server-oauth.ts +++ b/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)); - clientSecret = decodeURIComponent(decoded.slice(separator + 1)); + clientId = formUrlDecode(decoded.slice(0, separator)); + 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 */