[CFX-7754] Reject browser-forged API keys during dr auth login

[chasdr]

Summary

During dr auth login the CLI runs a local listener on localhost:51164 for 5 minutes, waiting for the browser to hand back your API key. It accepted any request on that port, so a web page you had open could plant an attacker's key with <img src="http://localhost:51164/?key=ATTACKER_KEY"> and the CLI would store it as your credential, so every later dr command ran against the attacker's account. The callback now refuses the forgeable browser requests.

Notes for review

The gate is Sec-Fetch-Dest present and not document. Absent must stay accepted: the CLI-to-CLI port handover uses a Go http client that sends no fetch metadata, and rejecting absent would deadlock two concurrent logins. It does not gate on Sec-Fetch-Site, since the real callback is legitimately cross-site.

Partial by design. It does not stop a page that forces a full-page navigation (a real document request, visible in the tab) or browsers older than Safari 16.4. The complete fix is a callback nonce, tracked separately since it needs a web app change.

Blocking manual gate, a real login must still work on:

• Chrome (macOS): pass
• Firefox: pending
• Safari: pending
• Edge: pending

Output

$ curl -s -o /dev/null -w "%{http_code}\n" -H "Sec-Fetch-Dest: image" "http://localhost:51164/?key=PLANTED-BY-ATTACKER"
403
$ curl -s -o /dev/null -w "%{http_code}\n" -H "Sec-Fetch-Dest: document" "http://localhost:51164/?key=REAL-KEY"
200

Technical Changes

• browserflow.go: refuse when Sec-Fetch-Dest is present and not document; success page only for a keyed request, 204 for the keyless handover sentinel.
• browserflow_test.go: image/empty refused, document accepted, headerless accepted, keyless image no longer interrupts, keyless headerless still hands over.
• auth docs: corrected the security note, added the editing rule for the gate.

Breakdown

• code: +17 / -5
• tests: +103 / -3
• docs: +15 / -5

---

Note

High Risk
Touches the login callback that accepts and stores API keys. The gate is partial by design (no state/nonce), so mistakes here can still accept attacker credentials or break concurrent logins.

Overview
Hardens dr auth login so a page cannot plant an API key via a forgeable request (<img> / fetch) to the localhost callback while login is in flight.

handleCallback now returns 403 when Sec-Fetch-Dest is present and not document. An absent header is still accepted so CLI-to-CLI port handover (Go http.Client, no fetch metadata) does not deadlock. Keyless handover gets 204 instead of the success HTML; a keyless browser probe (e.g. favicon) is refused and no longer aborts login.

This is a partial mitigation: full-page navigation and clients that can set headers are still in scope. Docs and tests cover the gate, genuine document callbacks, and the handover sentinel.

^{Reviewed by Cursor Bugbot for commit 385eb4a. Configure here.}

[datarobot-pr-review-router]

🎫 Jira: CFX-7754 — Auth callback listener accepts any browser request, so a page can plant an API key during login

[datarobot-pr-review-router]

Code Ownership

Cli Maintainers

• docs/commands/auth.md
• docs/development/authentication.md
• internal/auth/browserflow.go
• internal/auth/browserflow_test.go

Review requested from the teams above. Labels will be removed automatically upon approval.

[ajalon1]

I thought I had reviewed this already.

[ajalon1]

Reviewed from four angles (security/STRIDE, correctness & concurrency, test quality, docs & code style) using parallel review passes against upstream/main.

Verdict: approve. The Sec-Fetch-Dest gate is a correct, low-risk defense-in-depth improvement and the 204 sentinel change fixes a real latent bug (favicon probes no longer abort an in-flight login). The handler is HTTP-correct (header ordering, 204 with no body, 403 early-return before the channel send), concurrency-safe (buffered-1 keyCh with non-blocking select, closeOnce on Close), and the tests pass clean under -race -count 5 with golangci-lint reporting 0 issues.

Inline comments below cover the actionable findings (2 Medium, 4 Low). A few additional optional items that didn't warrant inline comments:

• Test coverage (optional): Sec-Fetch-Dest: "document" with an empty key (the intersection of the two new behaviors) is untested — expected 204 + ErrLoginInterrupted. Looping the forged request 2-3x before the genuine callback would catch per-request state leaks. Adding "script" to the forged-dest table documents the gate's breadth.
• Minor UX (informational): A user manually navigating to localhost:51164/ with no ?key= now gets a blank 204 instead of the success page. The interrupt behavior is unchanged; only the rendered body differs. Could render a small "this URL is for the CLI login" page for that case if desired.
• Docs (informational): The note could parenthetically acknowledge browsers that don't send Fetch Metadata headers aren't protected, though all modern browsers do.

None of the above are blocking. Nice fix.

[ajalon1]

[Medium] Lead claim is overstated and self-contradicted. The first sentence says "a web page you have open cannot plant a key" but the very next sentence says "a page that forces a full-page navigation, can deliver one." A top-level navigation carries Sec-Fetch-Dest: document, which the gate explicitly accepts, so the bypass is real.

Suggest scoping the claim to the vectors actually blocked, e.g. "cannot plant a key via a hidden <img> or background fetch." Also, the bypass list omits <iframe>, <link rel="prefetch" as="document">, and window.open — all set Sec-Fetch-Dest: document and bypass the gate. Worth enumerating so the gate's limits aren't overstated.

[ajalon1]

[Low] Doc comment omits the present-and-document accept case. The comment covers "present and not document refuses; absent accepts" but is silent on the third branch — present and document is accepted (the genuine callback). A future editor could misread "present … refuses" as "any present value refuses."

Suggest appending: present-and-"document" is the real callback; absent accepts (port handover sends none).

[ajalon1]

[Low] http.Error body is a bit cryptic. Per the repo's error-message rules (AGENTS.md: "specialize messages"; .cursor/bugbot-errors.md: "avoid cryptic error messages"), the bare "forbidden" is the kind of string those rules target. It's what a browser renders if the guard ever fires on a legitimate request (e.g. a browser quirk sending an unexpected dest).

Consider "forbidden: request type not allowed". Minor since this is primarily an attack/blocked path.

[ajalon1]

[Low] Odd-length headers is silently dropped. If a future caller writes waitForCallback(t, url, "Sec-Fetch-Dest") (name without a value), i+1 < len(headers) is false on the first iteration, so the header is silently dropped and the request is sent with no Sec-Fetch-Dest — exercising the ungated path instead of the intended one, producing a false-green test.

Suggest adding at the top of the helper: require.Equal(t, 0, len(headers)%2, "headers must be alternating name/value pairs").

[ajalon1]

[Low] context.Background() vs t.Context(). The repo is on Go 1.26, so t.Context() (available since 1.24) is usable. context.Background() is never cancelled, so an in-flight Do isn't torn down on test timeout/failure. Bounded by the server's ReadHeaderTimeout: 10s, so not a correctness bug — just nicer to use t.Context() so requests are cancelled automatically.

[ajalon1]

[Medium] Tests are fully sequential — the real attack is concurrent. Both RefusesForgedBrowserCallbacks and KeylessBrowserProbeDoesNotInterrupt issue the forged request, wait for its response, then issue the genuine one. The real threat model is a page firing many <img>/fetch probes concurrently with the legitimate top-level navigation, where ordering isn't guaranteed.

The non-blocking select on the buffered-1 keyCh (browserflow.go ~line 209) isn't exercised under interleaved contention, which is where a subtle ordering bug (e.g., a 403 path that accidentally touched the channel) would hide.

Suggest a subtest that fires N forged probes concurrently (e.g. 10 goroutines with Sec-Fetch-Dest: image), waits for all to return 403, then sends the genuine callback and asserts <-keyCh == "real-key" and <-errCh is nil.