From a5a9d4fe15e3d5a8a22dade096ed0046d0c7ca12 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 11:16:51 -0400 Subject: [PATCH] fix: the classifier does not survive structuredClone, and the test now proves it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since round 1 the doc claimed the `name` fallback covers "a structured clone or a JSON hop". Verified, and it is false for half of that: structuredClone(new InsecureTokenEndpointError(url)) -> name: "Error", tokenEndpoint: undefined, constructor: Error structuredClone normalizes a custom Error subclass back to Error, so neither arm has anything left to match. A caller routing this error through structuredClone or postMessage would silently get the generic retryable handling back — the exact defect this PR removes. The test could not have caught it: it hand-built a look-alike plain object, so it asserted what I believed the boundary does rather than what it does. It now performs a real JSON round trip (asserting the prototype is gone), and a second case pins that structuredClone is NOT recognized — turning a false claim into a tested limitation. The doc comment now scopes the contract to JSON and states the structuredClone limit with the remedy: serialize the fields explicitly across such a boundary. Also removes an orphaned JSDoc block the round-4 refactor stranded above the report helper, and gives showInsecureTokenEndpointNotice its own doc back. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju Signed-off-by: cliffhall --- .../src/lib/insecureTokenEndpointNotice.ts | 16 +++++--- .../core/auth/insecureTokenEndpoint.test.ts | 39 ++++++++++++++----- core/auth/insecureTokenEndpoint.ts | 16 ++++++-- 3 files changed, 52 insertions(+), 19 deletions(-) diff --git a/clients/web/src/lib/insecureTokenEndpointNotice.ts b/clients/web/src/lib/insecureTokenEndpointNotice.ts index 2049df08..2d8acb7f 100644 --- a/clients/web/src/lib/insecureTokenEndpointNotice.ts +++ b/clients/web/src/lib/insecureTokenEndpointNotice.ts @@ -31,12 +31,6 @@ import { insecureTokenEndpointTitle, } from "../utils/oauthUx"; -/** - * Show the terminal notice when `err` is the SDK's `InsecureTokenEndpointError`. - * - * @returns `true` when it was handled (the caller should stop), `false` when - * `err` is some other failure and the caller's normal handling applies. - */ /** * Report the refusal **and** clear the re-auth banner for that server. * @@ -79,6 +73,16 @@ export function reportTerminalInsecureTokenEndpoint< return true; } +/** + * Show the notice alone, without touching the banner. + * + * Prefer {@link reportTerminalInsecureTokenEndpoint} — the two are one + * invariant. This stays exported for the one arm that has already cleared the + * banner itself before starting. + * + * @returns `true` when it was handled (the caller should stop), `false` when + * `err` is some other failure and the caller's normal handling applies. + */ export function showInsecureTokenEndpointNotice( err: unknown, serverName?: string, diff --git a/clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts b/clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts index b89aa680..ed855d61 100644 --- a/clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts +++ b/clients/web/src/test/core/auth/insecureTokenEndpoint.test.ts @@ -43,16 +43,37 @@ describe("findInsecureTokenEndpoint", () => { ).toMatchObject({ tokenEndpoint: ENDPOINT }); }); - it("recognizes a serialized copy by `name`, where the prototype is gone", () => { - // The fallback arm: a structured clone or JSON hop drops the prototype and - // the brand set but keeps `name`. - expect( - findInsecureTokenEndpoint({ - name: "InsecureTokenEndpointError", - message: "Refusing to send credentials…", - tokenEndpoint: ENDPOINT, + it("recognizes a JSON round trip, where the prototype is gone", () => { + // An ACTUAL round trip, not a hand-built look-alike: constructing the + // object by hand asserts what I believed the boundary does rather than what + // it does. A JSON hop drops the prototype and the brand set while keeping + // `name` and `tokenEndpoint`, which is the case this fallback exists for. + const err = new InsecureTokenEndpointError(ENDPOINT); + const hopped: unknown = JSON.parse( + JSON.stringify({ + name: err.name, + message: err.message, + tokenEndpoint: err.tokenEndpoint, }), - ).toMatchObject({ tokenEndpoint: ENDPOINT }); + ); + expect(Object.getPrototypeOf(hopped)).toBe(Object.prototype); + expect(findInsecureTokenEndpoint(hopped)).toMatchObject({ + tokenEndpoint: ENDPOINT, + }); + }); + + it("does NOT survive structuredClone, and this pins that limit", () => { + // Verified, not assumed: structuredClone normalizes a custom Error subclass + // back to `Error`, so `name` becomes "Error" and `tokenEndpoint` is dropped + // — nothing is left for either arm to match. An earlier revision of the doc + // comment claimed this boundary worked; it does not, and a caller relying on + // it would silently get the generic retryable handling back. + const cloned = structuredClone(new InsecureTokenEndpointError(ENDPOINT)); + expect(cloned.name).toBe("Error"); + expect( + (cloned as { tokenEndpoint?: unknown }).tokenEndpoint, + ).toBeUndefined(); + expect(findInsecureTokenEndpoint(cloned)).toBeUndefined(); }); it("rejects a look-alike carrying the endpoint but not the identity", () => { diff --git a/core/auth/insecureTokenEndpoint.ts b/core/auth/insecureTokenEndpoint.ts index 0374a54a..b943c6a4 100644 --- a/core/auth/insecureTokenEndpoint.ts +++ b/core/auth/insecureTokenEndpoint.ts @@ -36,10 +36,18 @@ export interface InsecureTokenEndpointShape { * on an instance — don't check that property.) * * The `name` comparison is the same deliberate serialization fallback - * `isAuthorizationServerMismatchShape` carries in `issuerBinding.ts`: today the - * web client runs `auth()` in the browser, so no boundary is crossed, but the - * prototype and brand set are the first things a structured clone or a JSON hop - * would drop, and `name` survives both. + * `isAuthorizationServerMismatchShape` carries in `issuerBinding.ts`. Today the + * web client runs `auth()` in the browser so no boundary is crossed, but a JSON + * hop drops the prototype and the brand set while preserving `name` and + * `tokenEndpoint`, and that is the case this arm exists for. + * + * ⚠️ **It does not cover `structuredClone`, and cannot.** That algorithm + * normalizes a custom `Error` subclass back to `Error` — `name` becomes + * `"Error"` and own properties like `tokenEndpoint` are dropped entirely — so + * nothing survives for either arm to match on. A caller who routes this error + * through `structuredClone` (or `postMessage`, which uses it) will silently get + * the generic retryable handling back. Serialize the fields explicitly across + * such a boundary rather than relying on this classifier. */ function isInsecureTokenEndpointShape( err: unknown,