fix(cli): stop losing the OAuth session on refresh

.changeset/cli-refresh-durability.md

@@ -0,0 +1,38 @@
+---
+"neon": patch
+---
+
+Stop losing the OAuth session to a network blip, a concurrent command, or a 401.
+
+Neon's authorization server rotates refresh tokens: every `grant_type=refresh_token`
+exchange returns a new refresh token and retires the presented one, and replaying a
+retired one answers `400 invalid_grant`. The CLI's refresh path did not account for that,
+so three ordinary situations ended in a browser login.
+
+**A failure after the exchange threw the new token away.** The refresh persisted by calling
+`GET /users/me` first and writing the file second. If that request failed, the catch turned
+it into a generic refresh failure and nothing was written — but the presented refresh token
+was already dead server-side, so the rotated set existed only in memory and went with the
+process. The account lookup is gone from the refresh path entirely; a refresh cannot change
+who is signed in, so `user_id` is carried over from the previous token set instead.
+
+**Two commands at once invalidated each other.** Both read the same expired token set and
+both exchanged it; the loser got `invalid_grant` and demanded a new sign-in. The loser now
+re-reads the credentials file and adopts the winner's result, which is already there.
+
+**A 401 deleted the refresh token instead of using it.** An access token that expires
+mid-command is indistinguishable from a revoked one, and only the authorization server can
+tell them apart — so the 401 handler now refreshes and retries, and deletes credentials only
+once the refresh confirms the session is really gone. A 401 on the final attempt no longer
+mutates anything, since there is no retry left to prepare for.
+
+Alongside those:
+
+- Credentials are written atomically (temp file, `fsync`, rename) and resolve symlinks
+  first, so a crash or a concurrent write can't leave a truncated file and a symlinked
+  `credentials.json` is followed rather than replaced. Files an older version left at `0700`
+  are tightened to `0600` on the next write.
+- A corrupt or unreadable `credentials.json` is now reported instead of being silently
+  treated as "not signed in", which used to hide the damage behind a browser login.
+- An unreachable authorization server is reported as such, and leaves the session alone.
+  It previously surfaced as "Could not reach the Neon API", naming the wrong host.

packages/cli/src/auth.ts

@@ -10,6 +10,7 @@ import { fileURLToPath } from "node:url";
 import open from "open";
 import * as client from "openid-client";
 import { sendError } from "./analytics.js";
+import type { StoredCredentials } from "./credentials.js";
 import { matchErrorCode } from "./errors.js";
 import { log } from "./log.js";
 import type { ExtendedTokenSet } from "./types.js";
@@ -45,10 +46,116 @@ export type AuthProps = {
 	allowUnsafeTls?: boolean;
 };
 
+/**
+ * A refresh that didn't produce a new token set.
+ *
+ * `terminal` is the part callers act on: it separates "this session is over, sign in again"
+ * from "the network was in the way". Deleting credentials is only ever correct for the
+ * former — doing it for a DNS blip signs the user out of a session that was fine.
+ */
+export class AuthRefreshError extends Error {
+	readonly terminal: boolean;
+	/** The `error` code from the authorization server, when it gave one. */
+	readonly oauthError: string | undefined;
+	/** The failure this was classified from, kept for debug logging. */
+	readonly cause: unknown;
+
+	constructor(
+		message: string,
+		options: { terminal: boolean; oauthError?: string; cause?: unknown },
+	) {
+		super(message);
+		this.name = "AuthRefreshError";
+		this.terminal = options.terminal;
+		this.oauthError = options.oauthError;
+		this.cause = options.cause;
+	}
+}
+
+/**
+ * The only OAuth error codes that prove the *grant* is dead.
+ *
+ * Terminal is an allowlist rather than "any 4xx", because the sole consequence of terminal
+ * is deleting the user's session. A 429, an `invalid_client`, or a malformed request says
+ * something is wrong with us or with the moment — signing the user out fixes none of them
+ * and costs a browser login. A retired refresh token answers `invalid_grant`, or
+ * `token_inactive` when it was consumed moments ago.
+ */
+const DEAD_GRANT_ERRORS = new Set([
+	"invalid_grant",
+	"token_inactive",
+	"invalid_token",
+]);
+
+export const classifyRefreshFailure = (err: unknown): AuthRefreshError => {
+	const rejection = oauthRejection(err);
+	if (rejection) {
+		return new AuthRefreshError(
+			`The Neon authorization server rejected the stored session: ${rejection.error}${
+				rejection.description ? ` — ${rejection.description}` : ""
+			}`,
+			{
+				terminal: DEAD_GRANT_ERRORS.has(rejection.error),
+				oauthError: rejection.error,
+				cause: err,
+			},
+		);
+	}
+
+	return new AuthRefreshError(
+		`Could not reach the Neon authorization server to refresh the stored session: ${
+			err instanceof Error ? err.message : String(err)
+		}`,
+		{ terminal: false, cause: err },
+	);
+};
+
+/**
+ * The error code the authorization server gave, whichever shape it arrived in.
+ *
+ * A JSON error body surfaces as `ResponseBodyError`, but a 401 carrying a `WWW-Authenticate`
+ * header surfaces as `WWWAuthenticateChallengeError` instead — and that is exactly the shape
+ * an immediately-replayed refresh token can produce, so missing it would classify a dead
+ * grant as a network problem.
+ */
+const oauthRejection = (
+	err: unknown,
+): { error: string; description?: string } | null => {
+	if (err instanceof client.ResponseBodyError) {
+		return {
+			error: err.error,
+			...(err.error_description
+				? { description: err.error_description }
+				: {}),
+		};
+	}
+
+	if (err instanceof client.WWWAuthenticateChallengeError) {
+		const challenge = err.cause.find(({ parameters }) => parameters.error);
+		const error = challenge?.parameters.error;
+		if (typeof error !== "string") return null;
+		const description = challenge?.parameters.error_description;
+		return {
+			error,
+			...(typeof description === "string" ? { description } : {}),
+		};
+	}
+
+	return null;
+};
+
 export const refreshToken = async (
 	{ oauthHost, clientId, allowUnsafeTls }: AuthProps,
-	tokenSet: ExtendedTokenSet,
+	credentials: Pick<StoredCredentials, "refresh_token">,
 ) => {
+	const refresh = credentials.refresh_token;
+	if (typeof refresh !== "string" || refresh === "") {
+		throw new AuthRefreshError(
+			"The stored credentials hold no refresh token.",
+			{ terminal: true },
+		);
+	}
+
 	log.debug("Discovering oauth server");
 	const configuration = await client.discovery(
 		new URL(oauthHost),
@@ -63,10 +170,7 @@ export const refreshToken = async (
 		},
 	);
 
-	return await client.refreshTokenGrant(
-		configuration,
-		tokenSet.refresh_token as string,
-	);
+	return await client.refreshTokenGrant(configuration, refresh);
 };
 
 /**
@@ -200,8 +304,6 @@ export const auth = async ({
 			).pipe(response);
 
 			clearTimeout(timer);
-			const exp = new Date();
-			exp.setSeconds(exp.getSeconds() + (tokenSet.expires_in ?? 0));
 			resolve(extendTokenSet(tokenSet));
 			server.close();
 		};

packages/cli/src/auth_context.ts

@@ -2,35 +2,37 @@
  * How the current invocation authenticated, recorded by `ensureAuth` so the
  * top-level 401 handler can react to the credential that actually failed.
  *
- * - `api-key`: an explicit `--api-key` flag or `NEON_API_KEY`.
- * - `stored-credentials`: the OAuth token set the CLI keeps in the config dir.
+ * - `api-key`: an explicit `--api-key` flag or `NEON_API_KEY`. Nothing of ours is at stake.
+ * - `stored-credentials`: the OAuth token set the CLI keeps on disk.
+ *
+ * The stored-credentials case carries everything a 401 needs to act without re-parsing
+ * argv: which file the token came from (so a named profile's failure can't clear DEFAULT's
+ * credentials), which token was sent (so a rejection can be told apart from a token another
+ * command has since rotated), whether this run already refreshed (so a rejected *fresh*
+ * token is reported instead of triggering an endless refresh), and how to reach the
+ * authorization server.
  */
-export type AuthSource = "api-key" | "stored-credentials";
 
-export type AuthContext = {
-	source: AuthSource;
-	configDir: string;
+export type OAuthSettings = {
+	oauthHost: string;
+	clientId: string;
+	allowUnsafeTls?: boolean;
 };
 
+export type AuthContext =
+	| { source: "api-key" }
+	| {
+			source: "stored-credentials";
+			credentialsPath: string;
+			accessToken: string;
+			refreshed: boolean;
+			oauth: OAuthSettings;
+	  };
+
 let current: AuthContext | null = null;
 
 export const setAuthContext = (context: AuthContext): void => {
 	current = context;
 };
 
 export const getAuthContext = (): AuthContext | null => current;
-
-/**
- * The config directory whose credentials a 401 should clear, or `null` to leave
- * stored credentials alone.
- *
- * An API key passed on the command line never touches the stored OAuth token,
- * so a 401 on that key says nothing about whether the stored credentials are
- * still good — clearing them would sign the user out of an account the failed
- * request never used. The directory comes from the context rather than the
- * default so that `--config-dir` isolates the deletion too.
- */
-export const credentialsToClearOn401 = (
-	context: AuthContext | null,
-): string | null =>
-	context?.source === "stored-credentials" ? context.configDir : null;