fix(cli): stop losing the OAuth session on refresh - #385
Open
andrelandgraf wants to merge 2 commits into
Open
Conversation
Neon's authorization server rotates refresh tokens and retires the presented one, so the moment an exchange returns, the rotated set is the only working credential in existence. The refresh path did not treat it that way. - Persist before anything else can fail. The refresh used to call `GET /users/me` and write the file afterwards, so a failed lookup discarded a token set whose predecessor was already dead. `user_id` is carried over from the previous set instead, since a refresh cannot change who is signed in. - Recover from a lost race. Two commands refreshing at once left the loser with `invalid_grant`; it now adopts the winner's already-persisted result. - Refresh on a 401 rather than deleting the refresh token, and delete only once the authorization server confirms the session is gone. Nothing is mutated on the final attempt, where no retry remains. - Write atomically through resolved symlinks, report a corrupt credentials file instead of silently re-authenticating, and name the authorization server when it is the host that is unreachable. Covered by cross-process tests that run the built CLI against a local authorization server reproducing the rotation and reuse semantics verified against oauth2.neon.tech.
…andling Engineering review follow-ups. - Terminal is now an allowlist (invalid_grant, token_inactive, invalid_token) rather than "any 4xx". Deleting the session is the only consequence of terminal, and a 429 or an invalid_client is not evidence that the user's refresh token is dead. - Classify WWWAuthenticateChallengeError too. A 401 carrying a challenge header arrives as that class rather than ResponseBodyError, and would otherwise read as a network failure. - Use the rejected token recorded in AuthContext: when the file already holds a different usable token, retry with it instead of spending a refresh. - Follow symlinks when deleting credentials, so removal doesn't detach a configured path the write side is careful to preserve. - Clean up the temp file when the write or the flush fails, not only the rename. - Report a corrupt credentials file at warning level on the commands that continue without credentials; `dev` dropping env injection silently reads as a bug in the user's app. - Build the stored shape from validated fields instead of casting the parsed object, and treat an explicit null as absent. - Drop the `force` parameter that no caller reached. Tests: null and non-finite field handling, and symlink-following deletion.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
Neon's authorization server rotates refresh tokens. Verified against
https://oauth2.neon.techwhile writing this:So the instant an exchange returns, the rotated token set is the only working credential in existence — the one we presented is already dead server-side. The refresh path did not treat it that way, and three ordinary situations therefore ended in a browser login.
1. A failure after the exchange threw the new token away.
handleExistingTokenrefreshed and then calledpreserveCredentials, which didawait apiClient.getCurrentUserInfo()beforewriteFileSync. Any failure on that request — a 500, a timeout, a dropped connection — hit the catch, becameAUTH_REFRESH_FAILED, and nothing was written. The rotated set existed only in memory and went with the process.2. Two commands at once invalidated each other. No serialization and a direct
writeFileSyncto the live path. Both read the same expired set, both exchanged it; the loser gotinvalid_grantand demanded a new sign-in.3. A 401 deleted the refresh token instead of using it.
index.tscalleddeleteCredentials(configDir)on any 401 from stored credentials. An access token that expires mid-command is indistinguishable from a revoked one, and only the authorization server can tell them apart — so this signed the user out every time the answer would have been "just expired". It also clearedDEFAULT's credentials when the failure belonged to a named profile, since it resolved by config directory rather than by the file actually in use.The solution
packages/cli/src/credentials.tsis new and owns the file: validation, durable writes, and race recovery. Pure logic (isAccessTokenUsable,toStoredCredentials) is separated from the I/O.user_idis carried over from the previous token set.authFlowlikewise writes the token set the moment the browser flow returns, then looks up the account, then mergesuser_idin.expires_at, retries the command on success, and deletes only when the server confirms the grant is dead. A transient failure keeps the session. Nothing is mutated on the final attempt.wxat0600,fsync,renameover the resolved target,fsyncof the directory.~/.config/neonctlis a symlink on machines holding several accounts andprofiles.jsonmay point anywhere, so a naive rename would replace the link and strand the real file.Interface
No new commands, and no change to the shape of
credentials.json. What changes is what a user sees.Refresh no longer touches the API, and the rotated set lands on disk before anything else runs:
A 401 recovers instead of signing you out:
An unreachable authorization server keeps the session and names the right host. It previously read
Could not reach the Neon API. … check https://neonstatus.com, which is a different service:A dead grant says so, and only then are credentials cleared:
A damaged credentials file is reported rather than silently replaced with a browser login:
One new hidden flag,
--allow-unsafe-tls.allowUnsafeTlswas already threaded through every OAuth call site but unreachable from the command line, so the auth flow could not be exercised as a real process against a local authorization server. It sits alongside the equally hidden and equally sharp--oauth-host, and only affects requests to that host.Verification
pnpm --filter neon test— 3308 passed, 6 skipped (the skips are pre-existingskipIfguards for Windows and opt-in psql integration runs).packages/env,packages/configandpackages/initalso pass, since they readcredentials.jsontoo.pnpm lintclean.Against production. The behaviour the fix is built on was measured directly against
https://oauth2.neon.tech— rotation, one-time use, and the exact error shapes (400 invalid_grant, and401 token_inactivefor an immediate replay).src/test_utils/rotating_oauth_server.tsreproduces that contract, and the tests inauth_refresh.test.tsfork the builtdist/index.jsagainst it with its own config directory, so the top-level 401 retry loop inindex.tsis exercised as a real process rather than called in-process.The concurrency test forces the collision rather than hoping for it: neither exchange is answered until both have arrived, so both must present the same refresh token and only one can win. It fails when the recovery re-read is reduced to a single attempt, which is how I know it isn't vacuous.
Not verified live: the built CLI refreshing against production
oauth2.neon.tech. That needs a live OAuth session, and re-establishing one requires a browser login. Everything below that line — the server's contract, and the CLI against a server implementing it — is covered.CI=trueis set on every child process in the tests, so an unexpected fall-through to the browser flow fails loudly instead of opening a browser on whoever runs the suite.Incidental changes
AuthContextbecame a discriminated union carrying the credentials path, the token that was sent, whether this run already refreshed, and the OAuth settings. ThecredentialsPathis what fixes the named-profile deletion bug above.AUTH_REFRESH_FAILED, a sentinel string matched by message, is replaced by anAuthRefreshErrorclass with aterminalflag. That flag is the whole basis for "delete the session" versus "keep it and report".Datecomputation inauth()and arefresh_token as stringcast inrefreshToken.commands/auth.test.tswas rewritten. It previously spied on our ownauthandrefreshTokenexports, whichAGENTS.mdforbids, and two of its cases asserted behaviour this PR deliberately changes. The refresh and 401 paths moved to the cross-process suite; what remains in-process is the browser flow and the per-command auth skips.Flagging
--allow-unsafe-tlsis a real footgun, even hidden. It disables HTTPS enforcement toward--oauth-host. I judged it acceptable because it is inert without--oauth-hostand because the alternative was keeping the auth flow untestable as a process, but say the word and I will gate it behind an env var only.analytics.tsreadsuser_idfrom theDEFAULTcredentials path even under--profile.packages/init/src/lib/auth.tsreadsaccess_tokenstraight off disk with no expiry check and posts it to/api_keys, soneon initon an hour-old session surfaces a bare 401. Neither is touched here.