fix(auth): only rebuild the response when an override is actually applied

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 <cliff@futurescale.com>

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(

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)),