Skip to content

fix(auth) [BRNS-DESK-029]: reuse one oauth client instead of registering per login - #85

Open
sebastian-ssvlabs wants to merge 1 commit into
mainfrom
fix/desk-029-oauth-client-reuse
Open

fix(auth) [BRNS-DESK-029]: reuse one oauth client instead of registering per login#85
sebastian-ssvlabs wants to merge 1 commit into
mainfrom
fix/desk-029-oauth-client-reuse

Conversation

@sebastian-ssvlabs

Copy link
Copy Markdown

Summary

  • Every brains sign-in performed a fresh OAuth Dynamic Client Registration and threw the client_id away. Each login therefore left behind another orphaned OAuth client and another still-valid long-lived bearer — independently usable, invisible to the user, and accumulating with every re-login and every QA cycle. Logout only scrubbed locally, so the token it "removed" stayed live forever.
  • The registration is now performed once, persisted in the keychain, and reused; the bearer we abandon is revoked server-side before we lose our handle on it.
  • Closes BRNS-DESK-029 (P3, effort M, onboarding / desktop / auth).

Mechanism

Two independent halves — stop minting new clients, and stop leaving old credentials live. Either one alone still leaks.

1. Persist + reuse. register_client() writes {client_id, redirect_port} to the keychain (service ai.mybrains.desktop.v2, new account brains-oauth-client-id) next to the token. brains_oauth_login() now loads that record first and bind_login_listener() tries to bind the exact port it was registered with; on success the stored client_id is reused and no /register call happens at all. Keychain rather than ~/.brains because the data dir is wiped or relocated in exactly the scenarios where client identity has to survive (BRNS-DESK-025). A corrupt or hollow record reads as absent rather than erroring, so a bad keychain item can never break sign-in.

2. Revoke. revoke_token() runs in two places: before logout's local scrub (new brains_oauth_revoke command, called from logout() in +page.svelte ahead of clearBrainsToken()), and before a replacement registration orphans the previous one. It reads the token from the keychain itself so the secret does not cross the IPC boundary again.

The redirect-URI design decision

Registration binds redirect_uris, but the loopback port was random per login — so a reused client_id presented from a different port is a redirect_uri the server never saw, and may be rejected. RFC 8252 §7.3 says an authorization server should match native-app loopback redirects port-agnostically, but that is the server's option, not a guarantee, and probing the live server to find out would have created yet another orphaned registration — the very thing this ticket is about.

So: the port is stored with the client_id and re-bound on later logins, making the redirect_uri byte-identical to the registered one. Correct whether or not brains implements §7.3. If that port is occupied at login time we register afresh and overwrite the stored pair — the leak reappears only in the rare collision case, not on every login.

Revocation endpoint: discovered, not invented

The desktop only knows the two authorization-server endpoints it already calls (/register, /token) — nothing in the code names a revocation endpoint, so none is hardcoded. Instead revoke_token() reads revocation_endpoint from the AS's RFC 8414 metadata at {mcp}/.well-known/oauth-authorization-server (the mcp base is the AS, since /register and /token live there) and posts RFC 7009 token + token_type_hint=access_token + client_id.

If brains advertises no revocation_endpoint, this is a silent no-op and server-side revocation remains an unshipped capability. I could not check which it is without hitting the live server. Two guards: the bearer is never sent in cleartext to a non-loopback host however the metadata advertises it, and the whole discover-plus-revoke round trip is capped at 4s (vs the shared client's 60s) and swallows every failure — logout completes locally regardless.

Deliberately out of scope

Server-side work in ssvlabs/brains, not fixable from the desktop client:

  • capping / de-duplicating registrations per (user, client_name) — this PR stops the desktop creating them but cannot clean up the ones already accumulated;
  • a token list / revoke-all surface in the web app, without which the user still cannot see or revoke the credentials earlier builds minted;
  • exposing an RFC 7009 revocation_endpoint in the AS metadata, if there isn't one — this PR's revocation is inert until then.

Precedent: BRNS-SEC-041, where CLI logout was likewise found to be local-only.

Verification

  • cargo fmt --manifest-path src-tauri/Cargo.toml --check — clean.
  • npx prettier --check + npx eslint on both changed frontend files — 0 errors (35 pre-existing no-unused-vars warnings in +page.svelte / chat/+page.svelte, untouched here).
  • npx vitest run src/lib/brains-api.test.ts — 2 passed. node scripts/i18n-check.mjs — 0 errors (no new UI strings).
  • The pre-commit hook's svelte-check ran and passed, so the new TS is type-clean.

Not run locally, no local build env: cargo test, cargo clippy, cargo build, npm run build. Nine new Rust tests were written to compile by inspection against existing idioms in the same crate — CI is the first thing to actually execute them:

  • reuse decision: stored port free → reuse the client_id; port occupied → no reuse, different port; nothing stored → no reuse.
  • revocation: posts token/token_type_hint/client_id to the advertised endpoint (asserted against a loopback AS that records the body — this is the "revocation fires with the right token" test); no revocation_endpoint advertised → no request at all; cleartext non-loopback endpoint refused; nothing listening → returns false instead of erroring.
  • keychain record: {client_id, port} round-trips through the codec; empty / bare-id / port-0 records read as absent.

Only a real device and the live server can prove:

  1. Two consecutive sign-ins reuse one client_id — the acceptance test. There is no injectable registration seam in brains_oauth_login (one monolithic async fn over the shared HTTP client plus a browser launch), and I did not restructure the module just to mock it. The tests pin the decision (bind_login_listener), not the absence of the HTTP call; confirm on-device by signing in twice and checking only one /register occurs.
  2. The reused client_id + re-bound port is actually accepted by brains' /authorize and /token. This is the design gate above, and the one thing reasoning cannot settle.
  3. Whether the old token is genuinely rejected after logout — i.e. whether brains advertises a revocation_endpoint at all. Check {mcp}/.well-known/oauth-authorization-server, then try the pre-logout bearer against /mcp afterwards.
  4. Re-login after logout yields a working session (the stored registration survives logout by design — it is a client identity, not a user credential).
  5. A revocation failure still completes local logout — unit-tested against a dead port; worth confirming once on-device with the network off.

No live OAuth flow was run and no sign-in was performed while writing this: registering is exactly what the ticket is about, and a probe would have created another orphaned credential. The protocol behaviour above is reasoned from the code and the RFCs only.

Note for a follow-up, not addressed: brains.rs is now 950 lines, inside the 1024 cap but close — the OAuth login flow is the natural thing to split out, and doing it here would have buried this diff.

…ing per login

Every brains sign-in POSTed /register (Dynamic Client Registration) and threw
the client_id away, so each login left another orphaned client and another
still-valid long-lived token behind, invisible to the user.

- persist the client_id in the keychain (service ai.mybrains.desktop.v2, account
  brains-oauth-client-id) together with the loopback port it was registered for,
  and reuse both on later logins; a taken port falls back to a fresh
  registration and overwrites the stored pair
- revoke the abandoned bearer server-side before logout scrubs it locally, and
  before a replacement registration orphans it — RFC 7009, with the endpoint
  read from the server's RFC 8414 metadata rather than guessed, capped at 4s and
  never able to fail the local scrub

@nir-ssvlabs nir-ssvlabs left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both halves are the right design, and the write-up is unusually honest about what it cannot prove — storing the port with the client_id so the reused redirect_uri is byte-identical is the correct read of RFC 8252 §7.3 rather than betting on the server implementing it, and discovering the endpoint through RFC 8414 instead of hardcoding one is right.

🟠 High · the revocation endpoint is scheme-checked but not origin-bound, so a live bearer can be POSTed off-origin (blocking): @sebastian-ssvlabs revoke_token_inner takes revocation_endpoint straight from the metadata and only refuses it for being cleartext non-loopbackhttps://anywhere/collect passes. The metadata itself comes from {mcp}/.well-known/oauth-authorization-server with no scheme requirement on mcp, and mcp is brainsEndpoint(), which reads localStorage["brains.endpoint"]. So an http:// base makes the discovery document MITM-able, and whoever answers it names where the still-valid token gets sent. RFC 8414 expects the issuer to be validated, not just the transport. Two lines fix it: require endpoint.origin() == Url::parse(mcp)?.origin(), and require mcp itself to be https unless loopback before trusting its metadata. The default (https://mcp.mybrains.ai) is safe, which is what keeps this off 🔴 — but this PR introduces the outbound send, so the egress path is new.

  • 🔵 Six open PRs (#78, #80#84) all append to the end of messages/en.json and zh-CN.json, and four also edit +page.svelte; this one edits +page.svelte too. No duplicate keys, so the conflicts are textual rather than semantic — but whoever merges after the first will hit them in every locale file. Worth landing them in a deliberate order rather than discovering it six times.

Checked: the loopback/scheme guard against its stated claim (which it does meet, literally), the metadata fetch's own transport, brainsEndpoint()'s provenance, and that no origin comparison exists anywhere in the file. Not read: the keychain record's corrupt/hollow handling and the port-rebind fallback — both matter less than where the token goes.

Merge: ⛔ not yet into main — bind the endpoint to the issuer's origin first. Everything else here I'd take as-is.

.and_then(|v| v.as_str())
.ok_or_else(|| format!("register: no client_id ({})", reg))?
.to_string();
let client_id = match reusable_client_id {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Stale stored client_id has no recovery path: if the server no longer recognizes it (registration pruned server-side, or mcp_url changed — OauthClient is not bound to an endpoint), /oauth/authorize fails in the browser, brains_oauth_login dies at the 5-minute timeout, and nothing ever clears the keychain record — no delete function exists, so every retry reuses the same dead id and sign-in stays broken until the port happens to be occupied or the keychain item is removed by hand. On login failure after reuse (timeout or token-exchange invalid_client), clear the stored record (or retry once with a fresh registration), and store the mcp origin alongside client_id so a record is only reused against the server that issued it.

) -> Result<String, String> {
if let Some(old) = replacing {
if let Ok(Some(old_token)) = keychain_token() {
let revoked = revoke_token(mcp, &old_token, Some(&old.client_id)).await;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 register_client revokes the current bearer via revoke_token before the replacement login has succeeded; if /register fails, the user cancels the browser consent, or the token exchange fails, the keychain still holds a token that was just revoked server-side and the app keeps presenting it as a signed-in session that now 401s. Revoke the old token only after the new token is secured (e.g. at the end of brains_oauth_login on the success path).

.ok_or_else(|| format!("register: no client_id ({})", reg))?
.to_string();
let client_id = match reusable_client_id {
Some(id) => id,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 BRNS-DESK-029: revoke-on-relogin is missing on the common reuse path — when the stored port binds, register_client is never called, so a re-login while already signed in overwrites the old bearer (brainsOauthLoginsetBrainsToken) without revoking it, leaving it live server-side; the ticket's "re-login replacing old registration revokes old token" only fires on the rare port-collision branch, so live tokens still accumulate on every re-login-without-logout. In brains_oauth_login, after a successful exchange, revoke the previous keychain token before returning the new one.

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.

3 participants