From ac8a08e42f9efe24a6f234a363c223b93c72bfde Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 21:30:32 -0400 Subject: [PATCH] fix(auth): only rebuild the response when an override is actually applied MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings on the fetch wrapper: Reading the body with `response.text()` meant every successful JSON response was rebuilt while overrides were configured, even when it was not a metadata document. That dropped native properties a synthesized `Response` cannot carry (`url`, `redirected`, `type`) from responses the wrapper has no business altering, and threw outright on a body-less 204/205, whose status Fetch forbids a body on — so an unrelated transport response could fail. It now parses a clone and returns the caller's own response unless a document is actually patched. `new URL` also accepts embedded credentials while Fetch rejects a request URL carrying them, so `https://user:pass@host/token` passed both the form and the runtime and then failed deep in the OAuth exchange. `oauthEndpointUrlError` now rejects a username or password. Signed-off-by: cliffhall --- .../test/core/auth/endpointOverrides.test.ts | 62 ++++++++++++++++--- core/auth/endpointOverrides.ts | 26 +++++--- 2 files changed, 70 insertions(+), 18 deletions(-) diff --git a/clients/web/src/test/core/auth/endpointOverrides.test.ts b/clients/web/src/test/core/auth/endpointOverrides.test.ts index 7403ce97..0d9d214e 100644 --- a/clients/web/src/test/core/auth/endpointOverrides.test.ts +++ b/clients/web/src/test/core/auth/endpointOverrides.test.ts @@ -55,6 +55,17 @@ describe("oauthEndpointUrlError", () => { /not an http\(s\) URL/, ); }); + + // `new URL` accepts these, but Fetch rejects a request URL carrying them — + // so without this check the value passes validation and fails mid-flow. + it("rejects embedded credentials", () => { + expect( + oauthEndpointUrlError("https://user:pass@as.example.com/token"), + ).toMatch(/username or password/); + expect(oauthEndpointUrlError("https://user@as.example.com/token")).toMatch( + /username or password/, + ); + }); }); describe("normalizeOAuthEndpointOverrides", () => { @@ -90,6 +101,18 @@ describe("normalizeOAuthEndpointOverrides", () => { ); }); + it("drops a URL with embedded credentials", () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + expect( + normalizeOAuthEndpointOverrides({ + tokenUrl: "https://user:pass@as.example.com/token", + }), + ).toBeUndefined(); + expect(warn).toHaveBeenCalledWith( + expect.stringContaining("username or password"), + ); + }); + it("returns undefined when every configured value is malformed", () => { vi.spyOn(console, "warn").mockImplementation(() => {}); expect( @@ -236,24 +259,43 @@ describe("withOAuthEndpointOverrides", () => { await expect(wrapped("https://as.example.com/x")).resolves.toBe(original); }); - it("preserves a JSON body that is not metadata, and stays readable", async () => { - const wrapped = withOAuthEndpointOverrides( - passThrough(jsonResponse({ access_token: "abc" })), - () => ({ tokenUrl: STAGING_TOKEN }), - ); + // Everything that is not a metadata document comes back as the caller's own + // Response — identity, not an equivalent copy — so native properties a + // synthesized Response cannot carry (`url`, `redirected`, `type`) survive. + it("returns the original response for a JSON body that is not metadata", async () => { + const original = jsonResponse({ access_token: "abc" }); + const wrapped = withOAuthEndpointOverrides(passThrough(original), () => ({ + tokenUrl: STAGING_TOKEN, + })); const response = await wrapped("https://as.example.com/token"); + expect(response).toBe(original); await expect(response.json()).resolves.toEqual({ access_token: "abc" }); }); - it("hands back a readable response when the body is not valid JSON", async () => { - const wrapped = withOAuthEndpointOverrides( - passThrough(jsonResponse("not json at all")), - () => ({ tokenUrl: STAGING_TOKEN }), - ); + it("returns the original response when the body is not valid JSON", async () => { + const original = jsonResponse("not json at all"); + const wrapped = withOAuthEndpointOverrides(passThrough(original), () => ({ + tokenUrl: STAGING_TOKEN, + })); const response = await wrapped("https://as.example.com/x"); + expect(response).toBe(original); await expect(response.text()).resolves.toBe("not json at all"); }); + // Fetch forbids a body on 204/205, so rebuilding one would throw. A JSON + // content-type on an empty response is unusual but legal, and it must not + // take down an unrelated transport request. + it("passes a body-less 204 through untouched", async () => { + const original = new Response(null, { + status: 204, + headers: { "content-type": "application/json" }, + }); + const wrapped = withOAuthEndpointOverrides(passThrough(original), () => ({ + tokenUrl: STAGING_TOKEN, + })); + await expect(wrapped("https://as.example.com/x")).resolves.toBe(original); + }); + it("drops the stale content-length and content-encoding of a rewritten body", async () => { const body = JSON.stringify(METADATA); const wrapped = withOAuthEndpointOverrides( diff --git a/core/auth/endpointOverrides.ts b/core/auth/endpointOverrides.ts index 7ae07155..64534294 100644 --- a/core/auth/endpointOverrides.ts +++ b/core/auth/endpointOverrides.ts @@ -85,6 +85,14 @@ export function oauthEndpointUrlError(value: string): string | undefined { if (parsed.protocol !== "http:" && parsed.protocol !== "https:") { return `"${trimmed}" is not an http(s) URL.`; } + // `new URL` accepts embedded credentials but Fetch rejects an HTTP(S) request + // URL that carries them, so without this the value would pass both the form + // and the runtime and then fail deep in the OAuth exchange — the failure mode + // this validation exists to prevent. (Sending credentials to an endpoint the + // flow discovered is not something to support quietly, either.) + if (parsed.username || parsed.password) { + return "URL must not contain a username or password."; + } return undefined; } @@ -232,18 +240,20 @@ export function withOAuthEndpointOverrides( const contentType = response.headers.get("content-type") ?? ""; if (!contentType.toLowerCase().includes("json")) return response; - const body = await response.text(); + // Read through a clone so every path that does NOT patch can return the + // caller's own `Response`, untouched. Rebuilding unconditionally would drop + // native properties a synthesized `Response` cannot carry (`url`, + // `redirected`, `type`) from responses this wrapper has no business + // altering — and would throw outright on a body-less 204/205, whose status + // Fetch forbids a body on. A clone also makes a non-JSON body a plain + // parse failure with nothing consumed. let parsed: unknown; try { - parsed = JSON.parse(body); + parsed = await response.clone().json(); } catch { - // Not JSON despite the header. The body is already consumed, so hand back - // an equivalent response rather than the drained original. - return responseWithBody(response, body); - } - if (!isAuthorizationServerMetadata(parsed)) { - return responseWithBody(response, body); + return response; } + if (!isAuthorizationServerMetadata(parsed)) return response; return responseWithBody( response, JSON.stringify(applyOAuthEndpointOverrides(parsed, overrides)),