fix(oauth): read code and state from a redirect URL fragment

devlog/_plan/260825_oauth_login_ux/040_wp5_paste_normalization.md

@@ -1,8 +1,28 @@
-# 040 — WP5: a pasted redirect that looks right must not be silently refused
+# 040 — WP5: read a redirect's fragment, not only its query
 
-**Issue:** bug report. **PR base:** `dev`. **Screenshot:** not required.
+**Issue:** feature proposal (parser hardening — not a reported failure).
+**PR base:** `dev`. **Screenshot:** not required.
 
-## The defect
+## Honest classification, first
+
+**No provider in this repository can currently produce the input this fixes.**
+That was checked, not assumed: every `OAuthCallbackFlow` subclass — ChatGPT,
+xAI, Antigravity, Anthropic — requests `response_type=code` and none sets
+`response_mode=fragment`, so an authorization-code response lands in the
+query. Cursor, Kiro, Copilot, Kimi and Nous are not this class at all (poll,
+device, or token-paste flows). Anthropic's copyable `code#state` is the *raw*
+branch, not a URL fragment, which is why `exchangeToken` still splits on
+`#`.
+
+So this is **defensive parser hardening**, not a fix for a failure users are
+hitting today. The first draft of this doc told a story about an operator
+pasting their address bar and being told it contained no code. That story is
+not reachable with the current provider set, and shipping it as a bug report
+would have been a small lie in a changelog. The change is still worth making —
+the cost is four lines and the parser is the one place a future
+fragment-returning provider would land — but it ships described as what it is.
+
+## The gap
 
 `parseCallbackInput` (`callback-server.ts:273-300`) tries three shapes in
 order: a parseable URL, a string containing `code=`, then a raw code with an
@@ -19,20 +39,17 @@ return {
 };
 ```
 
-A redirect that returns its parameters in the **fragment** —
-`http://localhost:1455/callback#code=abc&state=xyz` — parses as a valid URL,
+A redirect that returned its parameters in the **fragment** —
+`http://127.0.0.1:<port>/callback#code=abc&state=xyz` — parses as a valid URL,
 yields no `code`, and is rejected by `submitManualLoginCode:1345` with
-"no authorization code found in input".
+"no authorization code found in input". The hint text asks the operator to
+"copy the full URL from its address bar", so that rejection would be
+particularly hard to act on if a provider ever did this.
 
-From the operator's chair this is the worst possible failure: they pasted the
-entire address bar, exactly as the hint text instructed
-(`prov.pasteRedirectHint`: "copy the full URL from its address bar"), and were
-told their paste contains no code.
-
-Note the asymmetry that makes this a bug rather than a limitation: the **raw**
-branch already understands `code#state`, and the **query** branch already
-strips a leading `#` (`value.replace(/^[?#]/, "")`). Fragments are understood
-everywhere except the one shape most likely to be pasted.
+Note the asymmetry that makes it worth closing: the **raw** branch already
+understands `code#state`, and the **query** branch already strips a leading
+`#` (`value.replace(/^[?#]/, "")`). Fragments are understood everywhere
+except in a full URL.
 
 ## The change
 
@@ -95,7 +112,15 @@ supported. Add it.
 
 ## Acceptance
 
-- A fragment-carried redirect URL completes a login.
-- A fragment-carried redirect with a bad state is refused, with the specific
-  state-mismatch message.
+- `parseCallbackInput` reads `code` and `state` from a URL fragment when the
+  query does not carry them, and keeps `kind: "url"` so state stays mandatory.
+- A fragment-carried paste with a missing or mismatched state is refused
+  end-to-end through `submitManualLoginCode`, with the same messages a
+  query-carried one gets. This is the assertion that proves the convenience did
+  not become a CSRF hole.
+- A token fragment yields no code.
+- No existing accepted paste changes meaning: query wins when both are present.
 - `bun run typecheck`, `bun run test` green.
+
+Note what is deliberately **not** claimed: that a real login was failing. See
+the classification at the top of this doc.

src/oauth/callback-server.ts

@@ -276,10 +276,23 @@ export function parseCallbackInput(input: string): { kind: "url" | "query" | "ra
 
   try {
     const url = new URL(value);
+    // Also read the fragment. No provider configured here returns one — every
+    // OAuthCallbackFlow asks for response_type=code without response_mode, so
+    // the parameters land in the query — but a full URL whose parameters sit in
+    // the hash parses as a valid URL with no code, and reading only the query
+    // rejects it as "no authorization code found in input". This is the one
+    // place a fragment-returning provider would land, and the raw branch below
+    // already understands `code#state`.
+    //
+    // Query wins when both are present: it is the authorization-code response
+    // location, so no paste that works today changes meaning. Only `code` and
+    // `state` are read — never a token. This repo does not implement the
+    // implicit grant and a paste field must not become the place it appears.
+    const fragment = new URLSearchParams(url.hash.replace(/^#/, ""));
     return {
       kind: "url",
-      code: url.searchParams.get("code") ?? undefined,
-      state: url.searchParams.get("state") ?? undefined,
+      code: url.searchParams.get("code") ?? fragment.get("code") ?? undefined,
+      state: url.searchParams.get("state") ?? fragment.get("state") ?? undefined,
     };
   } catch {
     // Not a URL - check for query string format

tests/oauth-manual-code.test.ts

@@ -49,6 +49,44 @@ describe("parseCallbackInput kinds", () => {
   test("raw authorization code -> kind raw", () => {
     expect(parseCallbackInput("  raw-auth-code  ")).toEqual({ kind: "raw", code: "raw-auth-code", state: undefined });
   });
+
+  test("code#state in a raw paste keeps the state alongside the code", () => {
+    // Supported since the branch was written, never asserted.
+    expect(parseCallbackInput("raw-auth-code#xyz")).toEqual({ kind: "raw", code: "raw-auth-code", state: "xyz" });
+  });
+
+  test("a redirect URL carrying code/state in the FRAGMENT is read, not rejected", () => {
+    // Defensive: no provider configured here returns a fragment response. A full
+    // URL with hash parameters is nonetheless a valid URL with no query code,
+    // and used to come back as "no authorization code found in input".
+    expect(parseCallbackInput("http://127.0.0.1:56121/callback#code=abc&state=xyz")).toEqual({
+      kind: "url", code: "abc", state: "xyz",
+    });
+  });
+
+  test("the query wins when both query and fragment carry a code", () => {
+    // The query is the authorization-code response location, so no paste that
+    // works today changes meaning.
+    expect(parseCallbackInput("http://127.0.0.1:56121/callback?code=q&state=qs#code=f&state=fs")).toEqual({
+      kind: "url", code: "q", state: "qs",
+    });
+  });
+
+  test("a fragment code without state stays kind url, so state stays mandatory", () => {
+    // kind must NOT degrade to raw: that is what would exempt it from the CSRF
+    // check and turn a convenience into a hole.
+    expect(parseCallbackInput("http://127.0.0.1:56121/callback#code=abc")).toEqual({
+      kind: "url", code: "abc", state: undefined,
+    });
+  });
+
+  test("a token fragment is not an authorization response", () => {
+    // This repo does not implement the implicit grant, and a paste field must
+    // not become the place it appears.
+    expect(parseCallbackInput("http://127.0.0.1:56121/callback#access_token=t&token_type=bearer")).toEqual({
+      kind: "url", code: undefined, state: undefined,
+    });
+  });
 });
 
 describe("OAuth manual login code fallback", () => {
@@ -156,6 +194,16 @@ describe("OAuth manual login code fallback", () => {
       expect(missingState.ok).toBe(false);
       if (!missingState.ok) expect(missingState.error).toContain("missing the state");
 
+      // A FRAGMENT-carried response gets the same CSRF treatment: reading the
+      // fragment must not have opened a hole beside the query it copies.
+      const fragmentMismatch = submitManualLoginCode("xai", `${redirectUri}#code=evil&state=WRONG`);
+      expect(fragmentMismatch.ok).toBe(false);
+      if (!fragmentMismatch.ok) expect(fragmentMismatch.error).toContain("state mismatch");
+
+      const fragmentNoState = submitManualLoginCode("xai", `${redirectUri}#code=abc`);
+      expect(fragmentNoState.ok).toBe(false);
+      if (!fragmentNoState.ok) expect(fragmentNoState.error).toContain("missing the state");
+
       // Correct paste: matching state completes the login via the original verifier.
       const goodSubmit = submitManualLoginCode("xai", `${redirectUri}?code=pasted-auth-code&state=${state}`);
       expect(goodSubmit).toEqual({ ok: true });

tests/update-stop-first.test.ts

@@ -21,11 +21,20 @@ function freePort(): Promise<number> {
 }
 
 async function waitForProxy(port: number): Promise<boolean> {
-  const deadline = Date.now() + 15_000;
+  // A cold detached proxy takes ~2s locally, but this test runs inside a CI
+  // batch of twelve files on a shared runner, where the same boot has been
+  // observed to blow a 15s budget and fail the whole shard. The test's own
+  // Bun timeout is 60s, so the readiness wait may use most of that: this
+  // deadline exists to stop a hung proxy, not to assert a boot deadline the
+  // suite never intended to enforce.
+  const deadline = Date.now() + 45_000;
   while (Date.now() < deadline) {
     try {
       const response = await fetch(`http://127.0.0.1:${port}/healthz`, {
-        signal: AbortSignal.timeout(500),
+        // A loaded runner can exceed 500ms on the very first connection while
+        // the process is still binding; a short per-probe timeout there reads
+        // as "not ready" for a proxy that is merely slow to accept.
+        signal: AbortSignal.timeout(2_000),
       });
       if (response.ok) return true;
     } catch { /* detached proxy is still starting */ }