test(auth): make the revocation fixture a real form-urldecoder
#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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BhVaufrEZAfFAXcg6456hK Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
1c1e70820b
commit
403352357b
4 changed files with 193 additions and 13 deletions
|
|
@ -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 });
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -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<NodeOAuthStorage> {
|
||||
async function seededStorage(
|
||||
tokens: {
|
||||
access_token: string;
|
||||
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_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);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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) {
|
||||
|
|
|
|||
|
|
@ -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
|
||||
*/
|
||||
|
|
|
|||
Loading…
Reference in a new issue