Skip to content

Fix two bugs in the OAuth browser-flow#8

Merged
pimfeltkamp merged 1 commit intomainfrom
fix-oauth-state-and-timeout
Apr 28, 2026
Merged

Fix two bugs in the OAuth browser-flow#8
pimfeltkamp merged 1 commit intomainfrom
fix-oauth-state-and-timeout

Conversation

@pimfeltkamp
Copy link
Copy Markdown
Contributor

Why

Iter-36 audit of src/auth/browser-flow.ts found two issues. Neither has bitten in normal use because the CLI is alpha and few people have hit the edge cases — but they're both real, one is security-adjacent, and the other is the same body-read total-deadline class we already fixed in the Node SDK (iter 10) and Ruby SDK (iter 19).

Bug 1: state validation runs after the listener responds

Old flow on a state mismatch (e.g. CSRF attempt):

[browser hits /callback?code=X&state=WRONG]
  -> listener sends 200 successPage("✓ Logged in")
  -> listener resolves with { code, receivedState }
[runBrowserFlow]
  -> if (receivedState !== state) throw "state mismatch"

So the user (or attacker) sees "✓ Logged in" in the browser while the terminal simultaneously aborts with state mismatch. Contradictory, and on a real CSRF attempt the attacker sees a success page they shouldn't see.

Fix

Validate state inside the listener handler, before writing the response:

[listener accepts callback]
  -> validate code+state present
  -> validate state matches expected     ← MOVED IN
  -> on mismatch: 400 errorPage("CSRF…") + reject
  -> only on full success: 200 successPage + resolve

Browser and terminal now agree.

Bug 2: token exchange has no deadline

The 5-minute DEFAULT_TIMEOUT_MS only covers listenForCallback. The moment the callback arrives, that timer clears. exchangeCodeForToken then does:

res = await fetch(args.tokenUrl, { method: "POST",});
const text = await res.text();

No signal, no Request.timeout. If /oauth2/token stalls (slow network, blackholed TCP, Cryptohopper edge issue), the CLI hangs forever. Same class of bug as the body-read deadline gaps we fixed in the Node and Ruby SDKs.

Fix

  • Add TOKEN_EXCHANGE_TIMEOUT_MS = 30_000 (matches the SDK's default request timeout).
  • Wrap the fetch and the res.text() in a single AbortController whose timer brackets both.
  • On abort, throw a clear "timed out reading the response after 30s" error so the user knows what happened (not a generic AbortError).
  • clearTimeout lives in a finally so the timer never leaks on the success path.

Test plan

  • State-mismatch end-to-end check: hit the local listener with ?state=wrong-state, confirm the CLI rejects with the right error message (verified via tsx reproduction).
  • npm run typecheck clean.
  • npm run build clean.
  • Manual: stand up an HTTP server that accepts the POST then never sends a body, point CRYPTOHOPPER_WEB_URL at it, run cryptohopper login --token (paste-flow doesn't hit the token endpoint — would need a real OAuth flow against a slow staging endpoint to verify the timeout end-to-end).
  • Manual: real OAuth flow against staging still works on the happy path.

Bug 1: state validation ran AFTER the listener responded.
  The callback handler sent the success page to the browser
  unconditionally on a code+state pair, then runBrowserFlow
  validated state on the CLI side and threw if it didn't match.
  On a forged callback (CSRF attempt) the browser showed
  "✓ Logged in" while the terminal showed "✗ state mismatch" —
  contradictory UX, and counter-productive against the threat
  the state nonce is supposed to defend against.

  Move state validation INSIDE the listener: on mismatch, the
  listener now responds with the same 400 errorPage every other
  malformed callback gets, and the CLI rejects with the same
  error message as before. Browser and terminal now agree.

Bug 2: exchangeCodeForToken had no timeout.
  The 5-minute listenForCallback timer fires only until the OAuth
  callback arrives. Once it does, the timer is cleared and the
  CLI proceeds to POST /oauth2/token with no deadline of its own.
  If the token endpoint stalls (slow network, blackholed TCP),
  the CLI hangs with no Ctrl-C affordance hint.

  Wrap the fetch + body read in an AbortController with a
  30-second total deadline. Same body-read total-deadline class
  as the iter-10 (Node) and iter-19 (Ruby) SDK fixes — the body
  read can hang independently of the response headers, so the
  fix has to bracket both.

Both fixes verified end-to-end: state-mismatch test confirms
the CLI rejects with the right error after the listener serves
a 400 errorPage; typecheck and build are clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@pimfeltkamp pimfeltkamp merged commit a348597 into main Apr 28, 2026
1 check passed
pimfeltkamp added a commit that referenced this pull request Apr 28, 2026
The 0.6.0-alpha.2 entry only mentioned the auth-fix dep bump from #9.
PRs #7 (cryptohopper upgrade version-comparison) and #8 (OAuth
browser-flow state validation + token-exchange timeout) also landed
on main without their own CHANGELOG lines. Folding all four bug
fixes under the same release since none has shipped yet.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant