feat(auth)!: OAuth login as the default for mux login - #74
Conversation
|
@daniel-hayes your PR description says "The production client_id is still a placeholder. It's the one value discovery can't supply, so it has to be right before this ships." is this still strue? |
davekiss
left a comment
There was a problem hiding this comment.
Reviewed the whole PR at ddac170 — the loopback/PKCE/discovery security surface and the command UX held up under everything I threw at them, and I saw the Bugbot findings are all already fixed with regression tests. Nice work.
The comments below are one theme: the refresh/concurrency layer can double-spend a rotating refresh token or terminally flag a valid credential on a hostile network. Three fixes, easiest order:
- Narrow terminal classification in
oauth.ts(two committable suggestions) - Typed rethrow in
token-refresh.ts(suggestion) + clean exit inwebhooks listen— depends on 1 - Mutex-guarded stale-lock break in
refresh-lock.ts
Happy to approve once these land.
|
Current version of PR was reviewed by /review-bugbot on Sep 16, 15:48 EDT. It flagged 0 findings. Bugbot on commit |
Great question, sorry I missed this the first round, but this is no longer true. All the caveats should have been addressed now |
Token refresh, revocation, and endpoint discovery derived their host from MUX_BASE_URL or the default only, while API calls honored the host stored with the environment. For an environment with a stored baseUrl, a refresh token was presented to a different host than the one that issued it, and the resulting invalid_grant flagged a healthy login as failed. OAuth endpoints now follow the same precedence as API calls: MUX_BASE_URL, then the stored host, then the default. A browser login against a non-default host also stores that host with the environment, as token logins already do, so a later shell without MUX_BASE_URL does not send the bearer token to the default host. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
When consent was denied, the state did not match, or the callback carried neither a code nor an error, the wait rejected synchronously and the caller force-closed the listener before the response was written. The browser showed a connection error instead of the "Login failed" page. The callback is now refused for any later request immediately, but the wait rejects only after the same grace period the success path already allows, so the page reaches the browser. A close that lands inside that window reports the pending failure rather than a generic cancellation. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…er login Entries saved before environment ids were recorded (before v1.2.0) could not be matched to the environment a browser login granted, so the login landed in a second, active entry. The signing keys, forward URL, and access token pair stayed behind on the old entry, and `mux sign` reported that no signing keys were configured. When no stored entry matches by id, entries that hold a token pair but no environment id are resolved through /whoami. One that belongs to the granted environment is treated as the existing entry: it is updated in place, keeps its settings, and is stamped with the id. Entries that already record an id are never queried, and a lookup that fails leaves the login to proceed as before. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A waiter that found the lock held, and then found it gone because the holder had released, went down the break path and unlinked the path. The break mutex serializes breakers, not acquirers, so a contender that linked a fresh lock in between had it deleted, and two processes held the lock at once. With rotating refresh tokens that means a double spend: the authorization server answers the second with invalid_grant, which flags a healthy login as failed. A released lock is now the ordinary hand-off it is: the waiter retries its link and stays out of the break path, and breakStaleLock reports a missing lock as nothing to break. An abandoned lock is removed only if the file is still the one that was judged. Measured over 14,400 contended acquisitions: 16 double holders before, none after. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 312dc9b. Configure here.
The previous commit treated a lock that could not be read as one that had been released, and retried the link immediately. But readLockFile reported every read failure as missing, so a lock that exists and cannot be read (another user's file, a directory at the path) failed the link, read as missing, and retried with no sleep and no deadline: a busy loop that never ended. Missing and unreadable are now different answers. Only a missing file means released. An unreadable one is reported as unparseable contents, which takes the abandoned path as it did before: removed when it can be, and otherwise a bounded wait that ends in the timeout error naming the file to delete. The released-lock retry now checks the deadline too, so no pass through the loop is unbounded. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Description
mux loginnow opens the browser by default to kick off the OAuth flow. You pick an organization and environment in the Mux Dashboard, the CLI catches the redirect on a loopback port, exchanges the code for tokens, and handles refresh from then on. Mux API access tokens keep working exactly as before.The four ways to authenticate are now explicit and mutually exclusive:
--oauth(default),--interactive,--env-file,--from-env.Breaking changes
Read these before approving. The first two are deliberate; the third is a consequence of the config format.
mux loginerrors whenMUX_TOKEN_ID/MUX_TOKEN_SECRETare set. It used to save them silently. Since env vars always outrank the config, that produced an entry that did nothing until you unset them. It now prints the four explicit options and exits 1 without writing. CI that runsmux loginwith injected credentials needs--from-env.mux loginwith no flags opens a browser instead of prompting for a Token ID and Secret. Anything scripted against those prompts needs--interactive.oauth/tokenblocks. Reading old flat entries works fine and needs no migration, but not the reverse — and note that any write converts every entry, including ones the command didn't touch. A downgrade means re-runningmux login. Nothing is lost: the credentials are still inconfig.json, just nested one level deeper. Worth saying explicitly in the release notes, since Mux only shows an access token secret once at creation and people may otherwise mint a replacement.--interactivealways requires a TTY, and fails immediately under--json, in agent mode, or with piped stdin rather than hanging. Browser sign-in (--oauth, the default) requires a TTY only in the default pretty mode: under--jsonor in agent mode it runs without one, emitting the authorization URL as a JSON event on stderr and the result on stdout, with--timeout(default 300s) bounding the wait.mux env listandmux auth statusoutput changed shape, so text parsing of them breaks. Both have--json.mux logoutnow makes a network call to revoke refresh tokens. Failure prints a warning and still removes the local credentials.For the access token methods (
--interactive,--env-file,--from-env),mux login --jsonoutput and itssourcevalues are unchanged. Browser sign-in prints a different document —{ name, identity, activated, replacedExisting, dropped }— since there is no token source to report. Note that a baremux login --jsonwithMUX_TOKEN_ID/MUX_TOKEN_SECRETset used to returnsource: "env"and now errors (breaking change 1);--from-envrestores it.What's new
mux auth status— every credential source, which is active, and why. No network calls, never prints token material.mux logout --all, and revocation on logout.mux env switchwith no argument gives an interactive picker;--jsononenv list,env switch, andlogout.muxprocesses coordinate through a lock file so a rotating refresh token is never spent twice.webhooks listenrefreshes and reconnects instead of dying mid-stream.mux auth statusexplains it, and a token pair on the same environment takes over from the next command on (the command that discovers the dead login still reports it and exits 1)./whoamiwhich environment its access token pair belongs to, and updates it in place. Without that, the login landed in a second, active entry and the signing keys stayed behind on the old one, somux signstopped working. Entries that already record an id are never queried.Read these six, in this order
src/lib/credentials.ts(236) — start here. The credential model: what an environment holds, how the two historical config layouts are normalized on read, andgetPreferredCredential, which decides OAuth-vs-token including the fallback when a credential is flagged as failing. Everything else assumes this.src/lib/mux.ts(+264) — the choke point.resolveCredentials()returns a discriminatedResolvedCredentials, which is whyBearervsBasicneeded no changes in ~98 command files. Also where the SDK client is built, and where the pre-existingMUX_AUTHORIZATION_TOKENfootgun is defused.src/lib/token-refresh.ts+src/lib/refresh-lock.ts(130 + 170) — the riskiest code in the PR. Proactive refresh, the re-read under lock that stops two processes spending one rotating refresh token, and flag-don't-delete on terminal failure. The lock deliberately fails rather than breaking a live holder's lock; that tradeoff is the thing to argue with.src/lib/oauth-loopback.ts(299) — the security surface. Binds 127.0.0.1 only, one path, one accepted callback, constant-timestatecompare, forced close. If you're only going to scrutinize one file for safety, this is it.src/lib/oauth.ts(503) — endpoint resolution (env override → discovery → built-in) the three grant calls, scope policy, and error normalization. The top ~90 lines are configuration and comments explaining why each default is what it is.src/commands/login-mode.ts(88) — pure function, no I/O, and it encodes the breaking behavior: four mutually exclusive methods, and the error when shell credentials are set. Quickest way to review the UX contract.Worth a reviewer's attention
The SDK already supported bearer auth.
@mux/mux-nodehas anauthorizationTokenoption, so no custom client was needed. It also revealed a pre-existing bug: that option defaults toprocess.env.MUX_AUTHORIZATION_TOKEN, and the SDK builds the bearer header after the Basic one, so a stray variable in someone's shell would silently override Basic auth. We now passnullexplicitly for whichever credential kind isn't in use.Endpoints derive from one base, and are discoverable. All three live on the API host (
/ui/v1/oauth/authorizefor the browser leg,/auth/v1/oauth/{token,revoke}for the back channel), soMUX_BASE_URLmoves the API calls, discovery, and the sign-in flow together — the token endpoint can't end up on a different host than the authorize endpoint. The same holds per environment: OAuth endpoints resolve with the precedence API calls use (MUX_BASE_URL, then the host stored with the environment, then the default), and a browser login against a non-default host stores that host as token logins do. A refresh token is therefore never presented to a host other than the one that issued it — which matters because the wrong host answersinvalid_grant, and that would flag a healthy login as dead. On top of that, RFC 8414 / OIDC discovery is consulted on login and refresh (cached a day) so Mux can move endpoints without stranding installed binaries. Discovery is never load-bearing: any failure falls back to the built-ins. Discovered endpoints are validated to behttps:on a.mux.comhost (dot-boundary matched, somux.com.evil.testfails) or the document's own origin, because a document that can repointtoken_endpointcould otherwise collect authorization codes.Scopes are hardcoded deliberately.
video/data/robots/systemread+write — the union of what the commands need. Not taken from the server'sscopes_supported, which would mean silently requesting any scope Mux adds later. Noopenid/profile/email: noid_tokenis requested or consumed, and identity comes from/system/v1/whoami, which reports what the access token can actually do. That also keeps JWKS out of the client.The 401 retry is one function, not 98 changes. It's the SDK's
fetchimplementation, so every command inherits refresh-and-retry.The browser always gets an answer. On a denied consent, a state mismatch, or a callback with neither code nor error, the listener refuses further callbacks immediately but rejects the wait only after the same 250ms grace the success path allows. Rejecting synchronously made the caller force-close the listener before the response was written, so the browser showed a connection error instead of the "Login failed" page.
Refresh is lock-guarded. Acquisition uses
link()of a fully-written temp file — anopen('wx')lock is briefly empty, and a competitor reading that mistakes a live holder for a crashed one. A waiter never breaks a live holder's lock, and release is ownership-checked.Testing
1263 tests pass, up from 1014 on
main. Beyond unit coverage, the flows were driven against real staging and a local fake authorization server, and verified on the wire:Bearerfor OAuth vsBasicfor token pairs on the same commandBasicon the same environmentMUX_BASE_URLunset, and nothing going toapi.mux.commux signworking afterwardsNo secrets in the diff: scanned the commits, working tree, and untracked files for
sk-ant-,eyJ, private key blocks,client_secret, and the staging client ID. The only high-entropy strings are the RFC 7636 Appendix B PKCE test vectors.Note
High Risk
Changes authentication defaults, credential storage format, and OAuth loopback/revocation behavior—breaking CI/scripts and affecting security-sensitive token handling across all API commands.
Overview
Browser sign-in is now the default for
mux login, with loopback OAuth, token refresh, and agent/--jsonflows that emit the authorization URL on stderr. Access tokens remain via--interactive,--env-file, or the new--from-env; withMUX_TOKEN_ID/MUX_TOKEN_SECRETset, baremux loginrefuses to guess and exits without writing config (CI must pass--from-env).Stored credentials move into nested
oauth/tokenblocks per environment (legacy flat entries still read); one environment can hold both kinds, OAuth preferred, with flagged failures and token-pair fallback. Config writes are atomic, and re-login updates entries in place by environment id so signing keys and the active selection are not lost.New
mux auth status(plusauth login/logoutaliases) inspects all sources locally without printing secrets.mux env list/switchgain richer output,--json, and optional interactive switch;mux logoutrevokes OAuth refresh tokens (best effort) and supports--all.API traffic uses Bearer vs Basic from resolved credentials, proactive refresh, 401 retry via SDK
fetch, andwebhooks listenreconnects after refresh.mux sign/ asset manage can sign with only signing keys when the active login is OAuth-only. README documents the full auth model and upgrade notes.Reviewed by Cursor Bugbot for commit db56b22. Bugbot is set up for automated code reviews on this repo. Configure here.