diff --git a/.changeset/cli-refresh-durability.md b/.changeset/cli-refresh-durability.md new file mode 100644 index 00000000..0fc7f37e --- /dev/null +++ b/.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. diff --git a/packages/cli/src/auth.ts b/packages/cli/src/auth.ts index d5f2469a..6403c50a 100644 --- a/packages/cli/src/auth.ts +++ b/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, ) => { + 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(); }; diff --git a/packages/cli/src/auth_context.ts b/packages/cli/src/auth_context.ts index ddce3725..7af08646 100644 --- a/packages/cli/src/auth_context.ts +++ b/packages/cli/src/auth_context.ts @@ -2,16 +2,33 @@ * 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 => { @@ -19,18 +36,3 @@ export const setAuthContext = (context: AuthContext): void => { }; 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; diff --git a/packages/cli/src/commands/auth.test.ts b/packages/cli/src/commands/auth.test.ts index a1303c1d..6f73ccfd 100644 --- a/packages/cli/src/commands/auth.test.ts +++ b/packages/cli/src/commands/auth.test.ts @@ -1,312 +1,209 @@ -import { - existsSync, - mkdtempSync, - readFileSync, - rmSync, - writeFileSync, -} from "node:fs"; -import type { AddressInfo } from "node:net"; +/** + * The sign-in flow and the auth middleware, in process. + * + * The refresh path, the 401 retry loop and credential-file durability are covered end to end + * in `auth_refresh.test.ts`, which runs the real CLI binary. What is left here is what only + * an in-process test can reach: the browser flow itself, and the per-command auth skips. + * + * `open` is stubbed to fetch the authorization URL, standing in for the human who would + * otherwise click through the browser. Nothing of ours is stubbed. + */ + +import { existsSync, mkdtempSync, readFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import type { OAuth2Server } from "oauth2-mock-server"; -import { join } from "path"; -import { afterAll, beforeAll, beforeEach, describe, expect, vi } from "vitest"; -import type { NeonApiClient } from "../api.js"; -import * as authModule from "../auth"; -import { test } from "../test_utils/fixtures"; -import { startOauthServer } from "../test_utils/oauth_server"; -import { authFlow, deleteCredentials, ensureAuth } from "./auth"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { getApiClient } from "../api.js"; +import { writeCredentials } from "../credentials.js"; +import { + type NeonApiServer, + startNeonApiServer, +} from "../test_utils/neon_api_server.js"; +import { startOauthServer } from "../test_utils/oauth_server.js"; +import { authFlow, deleteCredentials, ensureAuth } from "./auth.js"; + +// `open` launches the user's browser. Standing in for the human who would click through it +// is the only way to drive the authorization-code flow without one. Nothing of ours is +// replaced: the authorization server, the API and the callback handler are all real. vi.mock("open", () => ({ default: vi.fn((url: string) => fetch(url)) })); -vi.mock("../pkg.ts", () => ({ default: { version: "0.0.0" } })); -describe("auth", () => { - let configDir = ""; - let oauthServer: OAuth2Server; - - beforeAll(async () => { - configDir = mkdtempSync("test-config"); - oauthServer = await startOauthServer(); - }); +let configDir = ""; +let oauthServer: OAuth2Server; +let api: NeonApiServer; - afterAll(async () => { - rmSync(configDir, { recursive: true }); - await oauthServer.stop(); - }); +beforeEach(async () => { + configDir = mkdtempSync(join(tmpdir(), "neon-auth-")); + oauthServer = await startOauthServer(); + api = await startNeonApiServer(); +}); - test("should auth", async ({ runMockServer }) => { - const server = await runMockServer("main"); - await authFlow({ - _: ["auth"], - apiHost: `http://localhost:${(server.address() as AddressInfo).port}`, - clientId: "test-client-id", - configDir, - forceAuth: true, - oauthHost: `http://localhost:${oauthServer.address().port}`, - allowUnsafeTls: true, - }); +afterEach(async () => { + rmSync(configDir, { recursive: true, force: true }); + await Promise.all([oauthServer.stop(), api.stop()]); + vi.clearAllMocks(); +}); - const credentials = JSON.parse( - readFileSync(`${configDir}/credentials.json`, "utf-8"), - ); - expect(credentials.access_token).toEqual(expect.any(String)); - expect(credentials.refresh_token).toEqual(expect.any(String)); - expect(credentials.user_id).toEqual(expect.any(String)); - }); +const props = (overrides: Record = {}) => ({ + _: ["some-command"], + configDir, + oauthHost: `http://localhost:${oauthServer.address().port}`, + clientId: "test-client-id", + forceAuth: true, + apiKey: "", + apiHost: api.url, + help: false, + // `ensureAuth` replaces this with a client built from whatever credential it settles on. + // Starting from a real unauthenticated client keeps the fixture free of casts. + apiClient: getApiClient({ apiKey: "", apiHost: api.url }), + allowUnsafeTls: true, + ...overrides, }); -describe("ensureAuth", () => { - let configDir = ""; - let oauthServer: OAuth2Server; - let mockApiClient: NeonApiClient; - let authSpy: any; - let refreshTokenSpy: any; - - beforeAll(async () => { - configDir = mkdtempSync("test-config"); - oauthServer = await startOauthServer(); - mockApiClient = {} as NeonApiClient; - authSpy = vi.spyOn(authModule, "auth"); - refreshTokenSpy = vi.spyOn(authModule, "refreshToken"); - }); +const readCredentialsFile = (name = "credentials.json") => + JSON.parse(readFileSync(join(configDir, name), "utf8")); - afterAll(async () => { - rmSync(configDir, { recursive: true }); - await oauthServer.stop(); - vi.restoreAllMocks(); - }); +describe("authFlow", () => { + it("persists the token set and the account it belongs to", async () => { + const token = await authFlow(props({ _: ["auth"] })); - beforeEach(() => { - authSpy.mockClear(); - refreshTokenSpy.mockClear(); + const credentials = readCredentialsFile(); + expect(credentials.access_token).toEqual(expect.any(String)); + expect(credentials.refresh_token).toEqual(expect.any(String)); + expect(credentials.user_id).toBe("user-1"); + expect(credentials.expires_at).toBeGreaterThan(Date.now()); + expect(token).toBe(credentials.access_token); }); - const setupTestProps = (server: any) => ({ - _: ["some-command"], - configDir, - oauthHost: `http://localhost:${oauthServer.address().port}`, - clientId: "test-client-id", - forceAuth: true, - apiKey: "", - apiHost: `http://localhost:${(server.address() as AddressInfo).port}`, - help: false, - apiClient: mockApiClient, - allowUnsafeTls: true, - }); + // The account lookup is a convenience — it names a profile. Losing a completed browser + // login because it failed would make the user sign in all over again for nothing. + it("keeps the credentials when the account lookup fails", async () => { + api.failUserLookup(true); - test("should start new auth flow when refresh token fails", async ({ - runMockServer, - }) => { - refreshTokenSpy.mockImplementationOnce(() => - Promise.reject(new Error("AUTH_REFRESH_FAILED")), - ); + const token = await authFlow(props({ _: ["auth"] })); - authSpy.mockImplementationOnce(() => - Promise.resolve({ - access_token: "new-auth-token", - refresh_token: "new-refresh-token", - expires_at: Math.floor(Date.now() / 1000) + 3600, - }), - ); + const credentials = readCredentialsFile(); + expect(credentials.access_token).toEqual(expect.any(String)); + expect(credentials.refresh_token).toEqual(expect.any(String)); + expect(credentials.user_id).toBeUndefined(); + expect(token).toBe(credentials.access_token); + }); - const server = await runMockServer("main"); - const expiredTokenSet = { - access_token: "expired-token", - refresh_token: "refresh-token", - expires_at: Date.now() - 3600 * 1000, - }; + // A 401 here is different: the token we just wrote is not accepted, so reporting a + // successful sign-in would be a lie the next command has to discover. + it("fails when the API rejects the token it just issued", async () => { + api.rejectAll(true); - writeFileSync( - join(configDir, "credentials.json"), - JSON.stringify(expiredTokenSet), - { mode: 0o700 }, + await expect(authFlow(props({ _: ["auth"] }))).rejects.toThrow( + /rejected the new access token/, ); - - const props = setupTestProps(server); - await ensureAuth(props); - - expect(refreshTokenSpy).toHaveBeenCalledTimes(1); - expect(authSpy).toHaveBeenCalledTimes(1); - expect(props.apiKey).toBe("new-auth-token"); }); - test("should trigger auth flow when credentials.json does not exist", async ({ - runMockServer, - }) => { - const server = await runMockServer("main"); - - // Ensure the credentials file does not exist - const credentialsPath = join(configDir, "credentials.json"); - if (existsSync(credentialsPath)) { - rmSync(credentialsPath); - } - - const props = setupTestProps(server); - await ensureAuth(props); + it("labels a named profile with the account email", async () => { + await authFlow(props({ _: ["auth"], profile: "work" })); - expect(authSpy).toHaveBeenCalledTimes(1); - expect(refreshTokenSpy).not.toHaveBeenCalled(); - expect(props.apiKey).toEqual(expect.any(String)); + const profiles = JSON.parse( + readFileSync(join(configDir, "profiles.json"), "utf8"), + ); + expect(profiles.profiles.work).toMatchObject({ + credentials: "credentials.work.json", + label: "user@example.com", + userId: "user-1", + }); + expect( + readCredentialsFile("credentials.work.json").refresh_token, + ).toEqual(expect.any(String)); }); - test("should trigger auth flow when credentials.json is invalid", async ({ - runMockServer, - }) => { - const server = await runMockServer("main"); - - // Write an empty credentials file - writeFileSync(join(configDir, "credentials.json"), "", { mode: 0o700 }); + it("still records a named profile when the account lookup fails", async () => { + api.failUserLookup(true); - const props = setupTestProps(server); - await ensureAuth(props); + await authFlow(props({ _: ["auth"], profile: "work" })); - expect(authSpy).toHaveBeenCalledTimes(1); - expect(refreshTokenSpy).not.toHaveBeenCalled(); - expect(props.apiKey).toEqual(expect.any(String)); - }); - - test("should try refresh when token is missing access_token but has refresh_token", async ({ - runMockServer, - }) => { - const server = await runMockServer("main"); - const tokenWithoutAccess = { - refresh_token: "refresh-token", - }; - - writeFileSync( - join(configDir, "credentials.json"), - JSON.stringify(tokenWithoutAccess), - { mode: 0o700 }, + const profiles = JSON.parse( + readFileSync(join(configDir, "profiles.json"), "utf8"), ); - - refreshTokenSpy.mockImplementationOnce(() => - Promise.resolve({ - access_token: "refreshed-token", - refresh_token: "new-refresh-token", - expires_at: Math.floor(Date.now() / 1000) + 3600, - }), + expect(profiles.profiles.work.credentials).toBe( + "credentials.work.json", ); - - const props = setupTestProps(server); - await ensureAuth(props); - - expect(refreshTokenSpy).toHaveBeenCalledTimes(1); - expect(authSpy).not.toHaveBeenCalled(); - expect(props.apiKey).toBe("refreshed-token"); + expect(profiles.profiles.work.label).toBeUndefined(); }); +}); - test("should use existing valid token", async ({ runMockServer }) => { - const server = await runMockServer("main"); - const validTokenSet = { +describe("ensureAuth", () => { + it("uses a valid stored token without contacting the authorization server", async () => { + writeCredentials(join(configDir, "credentials.json"), { access_token: "valid-token", refresh_token: "refresh-token", - expires_at: Date.now() + 3600 * 1000, // 1 hour from now - }; - - writeFileSync( - join(configDir, "credentials.json"), - JSON.stringify(validTokenSet), - { mode: 0o700 }, - ); + token_type: "bearer", + expires_at: Date.now() + 3600_000, + }); - const props = setupTestProps(server); - await ensureAuth(props); + const args = props(); + await ensureAuth(args); - expect(authSpy).not.toHaveBeenCalled(); - expect(refreshTokenSpy).not.toHaveBeenCalled(); - expect(props.apiKey).toBe("valid-token"); + expect(args.apiKey).toBe("valid-token"); }); - test("should skip global auth for init command", async ({ - runMockServer, - }) => { - const server = await runMockServer("main"); - - const credentialsPath = join(configDir, "credentials.json"); - if (existsSync(credentialsPath)) { - rmSync(credentialsPath); - } - - const props = { - ...setupTestProps(server), - _: ["init"], - }; - - await ensureAuth(props); + it("signs in when there are no credentials at all", async () => { + const args = props(); + await ensureAuth(args); - expect(authSpy).not.toHaveBeenCalled(); - expect(refreshTokenSpy).not.toHaveBeenCalled(); + expect(args.apiKey).toEqual(expect.any(String)); + expect(readCredentialsFile().refresh_token).toEqual(expect.any(String)); }); - test("should successfully refresh expired token", async ({ - runMockServer, - }) => { - refreshTokenSpy.mockImplementationOnce(() => - Promise.resolve({ - access_token: "new-token", - refresh_token: "new-refresh-token", - expires_at: Math.floor(Date.now() / 1000) + 3600, - }), - ); - - const server = await runMockServer("main"); - const expiredTokenSet = { - access_token: "expired-token", - refresh_token: "refresh-token", - expires_at: Date.now() - 3600 * 1000, // expired 1 hour ago - }; + it.each([ + "init", + "dev", + "bootstrap", + ])("leaves %s to run without credentials rather than signing in", async (command) => { + const args = props({ _: [command] }); + await ensureAuth(args); - writeFileSync( - join(configDir, "credentials.json"), - JSON.stringify(expiredTokenSet), - { mode: 0o700 }, - ); + expect(args.apiKey).toBe(""); + expect(existsSync(join(configDir, "credentials.json"))).toBe(false); + }); - const props = setupTestProps(server); - await ensureAuth(props); + it("skips auth for the profile command so a lapsed profile stays removable", async () => { + const args = props({ _: ["profile", "list"] }); + await ensureAuth(args); - expect(refreshTokenSpy).toHaveBeenCalledTimes(1); - expect(authSpy).not.toHaveBeenCalled(); - expect(props.apiKey).toBe("new-token"); + expect(args.apiKey).toBe(""); + expect(existsSync(join(configDir, "credentials.json"))).toBe(false); }); }); describe("deleteCredentials", () => { - let configDir = ""; - - beforeAll(() => { - configDir = mkdtempSync("test-config-delete"); - }); - - afterAll(() => { - rmSync(configDir, { recursive: true }); - }); - - test("should successfully delete credentials file", () => { - const credentialsPath = join(configDir, "credentials.json"); - writeFileSync(credentialsPath, "test-content", { mode: 0o700 }); - - expect(existsSync(credentialsPath)).toBe(true); + it("removes the file", () => { + const path = join(configDir, "credentials.json"); + writeCredentials(path, { + access_token: "a", + refresh_token: "r", + token_type: "bearer", + expires_at: Date.now(), + }); deleteCredentials(configDir); - expect(existsSync(credentialsPath)).toBe(false); + expect(existsSync(path)).toBe(false); }); - test("should handle non-existent file gracefully", () => { - const nonExistentDir = mkdtempSync("test-config-nonexistent"); - - // Ensure the file doesn't exist - const credentialsPath = join(nonExistentDir, "credentials.json"); - if (existsSync(credentialsPath)) { - rmSync(credentialsPath); - } + it("does nothing when there is no file", () => { + expect(() => deleteCredentials(configDir)).not.toThrow(); + }); - expect(existsSync(credentialsPath)).toBe(false); + // A named profile's failure must not sign the user out of the default account. + it("only removes the selected profile's credentials", async () => { + await authFlow(props({ _: ["auth"] })); + await authFlow(props({ _: ["auth"], profile: "work" })); - // Should not throw an error - expect(() => { - deleteCredentials(nonExistentDir); - }).not.toThrow(); + deleteCredentials(configDir, "work"); - rmSync(nonExistentDir, { recursive: true }); + expect(existsSync(join(configDir, "credentials.work.json"))).toBe( + false, + ); + expect(existsSync(join(configDir, "credentials.json"))).toBe(true); }); }); diff --git a/packages/cli/src/commands/auth.ts b/packages/cli/src/commands/auth.ts index 9d7687e2..3b0ee48b 100644 --- a/packages/cli/src/commands/auth.ts +++ b/packages/cli/src/commands/auth.ts @@ -1,18 +1,31 @@ import { createHash } from "node:crypto"; -import { existsSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import type yargs from "yargs"; import type { NeonApiClient } from "../api.js"; -import { getApiClient } from "../api.js"; -import { auth, refreshToken } from "../auth.js"; -import { setAuthContext } from "../auth_context.js"; +import { getApiClient, isNeonApiError } from "../api.js"; +import { + AuthRefreshError, + auth, + classifyRefreshFailure, + refreshToken, +} from "../auth.js"; +import { type OAuthSettings, setAuthContext } from "../auth_context.js"; import { credentialsPath as defaultCredentialsPath } from "../config.js"; import { isConfigInit, isCurrentBranchProbe, isProfileCommand, } from "../context.js"; +import { + isAccessTokenUsable, + readCredentials, + recoverConcurrentRefresh, + removeCredentials, + type StoredCredentials, + toStoredCredentials, + writeCredentials, +} from "../credentials.js"; import { isCi } from "../env.js"; import { log } from "../log.js"; import { @@ -24,8 +37,6 @@ import { selectProfileName, upsertProfile, } from "../profiles.js"; -import type { ExtendedTokenSet } from "../types.js"; -import { extendTokenSet } from "../utils/auth.js"; type AuthProps = { _: (string | number)[]; @@ -100,19 +111,29 @@ export const authFlow = async ({ ? newProfileCredentialsPath(configDir, profileName) : credentialsPathFor({ configDir, profile }); - let identity: { id?: string; email?: string } = {}; - try { - identity = await preserveCredentials( - credentialsPath, - tokenSet, - getApiClient({ - apiKey: tokenSet.access_token || "", - apiHost, - }), - ); - } catch { - log.error("Failed to save credentials"); - return ""; + // Persist before anything else can fail. The browser flow is the expensive part and it + // has already happened; a token set that only exists in memory is one thrown exception + // away from making the user do it again. + const credentials = toStoredCredentials(tokenSet, null, Date.now()); + writeCredentials(credentialsPath, credentials); + logCredentialsHash(credentials); + + const apiClient = getApiClient({ + apiKey: credentials.access_token ?? "", + apiHost, + }); + const identity = await fetchIdentity(apiClient); + + // `user_id` is what analytics reads and what labels a profile. It is not worth a second + // browser login, so a failure to look it up leaves the (already durable) credentials + // alone. Re-read first: a concurrent command may have rotated the token meanwhile, and + // only the identity belongs to this write. + if (identity.id) { + const current = readCredentials(credentialsPath) ?? credentials; + writeCredentials(credentialsPath, { + ...current, + user_id: identity.id, + }); } if (isNamed) { @@ -124,93 +145,158 @@ export const authFlow = async ({ log.info('Saved profile "%s" (%s)', profileName, credentialsPath); } log.info("Auth complete"); - return tokenSet.access_token || ""; + return credentials.access_token || ""; }; /** - * Persist the token set and return the account it belongs to, so a named profile can be - * labelled with an email. The credentials file records only `user_id` — a UUID with no - * email — which is why identifying a stored profile offline is otherwise impossible. + * Who the freshly issued token belongs to. + * + * A 401 here means the token we just persisted is not accepted, so reporting success would + * be a lie — that throws. Any other failure is a lookup problem, not an authentication + * problem, and only costs the profile its display label. */ -const preserveCredentials = async ( - path: string, - credentials: ExtendedTokenSet, +const fetchIdentity = async ( apiClient: NeonApiClient, ): Promise<{ id?: string; email?: string }> => { - const { - data: { id, email }, - } = await apiClient.getCurrentUserInfo(); - const contents = JSON.stringify({ - // Cast to a plain record: we intentionally spread the credentials object. - ...(credentials as Record), - user_id: id, - }); - // Owner-only. A credentials file needs read/write, never execute. - writeFileSync(path, contents, { - mode: 0o600, - }); - log.debug("Saved credentials to %s", path); - log.debug("Credentials MD5 hash: %s", md5hash(contents)); - return { ...(id ? { id } : {}), ...(email ? { email } : {}) }; + try { + const { + data: { id, email }, + } = await apiClient.getCurrentUserInfo(); + return { ...(id ? { id } : {}), ...(email ? { email } : {}) }; + } catch (err) { + if (isNeonApiError(err) && err.status === 401) { + throw new Error( + "Signed in, but the Neon API rejected the new access token. Try `neon auth` again.", + ); + } + log.warning( + "Signed in, but could not look up the account: %s", + err instanceof Error ? err.message : String(err), + ); + return {}; + } +}; + +type ResolvedAuth = { + apiKey: string; + apiClient: NeonApiClient; + refreshed: boolean; }; -const handleExistingToken = async ( - tokenSet: ExtendedTokenSet, +/** + * Turn a stored token set into an authorized API client, refreshing when needed. + * + * Returns `null` only when there is nothing to refresh *with*, which is the one case that + * legitimately leads to a new sign-in. Every other failure throws, classified. + */ +const resolveStoredCredentials = async ( + credentials: StoredCredentials, props: AuthProps, credentialsPath: string, -): Promise<{ apiKey: string; apiClient: NeonApiClient } | null> => { - // Use existing access_token, if present and valid - if (tokenSet.access_token && tokenSet.expires_at > Date.now()) { +): Promise => { + if (isAccessTokenUsable(credentials, Date.now())) { log.debug("Using existing valid access_token"); - const apiClient = getApiClient({ - apiKey: tokenSet.access_token, - apiHost: props.apiHost, - }); + return { + apiKey: credentials.access_token ?? "", + apiClient: getApiClient({ + apiKey: credentials.access_token ?? "", + apiHost: props.apiHost, + }), + refreshed: false, + }; + } - return { apiKey: tokenSet.access_token, apiClient }; + if (!credentials.refresh_token) { + log.debug("Stored credentials hold no refresh_token"); + return null; } - // Either access_token is missing or its expired. Refresh the token - log.debug( - tokenSet.expires_at < Date.now() - ? "Token is expired, attempting refresh" - : "Token is missing access_token, attempting refresh", + log.debug("Access token is missing or expired, attempting refresh"); + + const { credentials: next, rotated } = await performRefresh( + credentials, + { + oauthHost: props.oauthHost, + clientId: props.clientId, + ...(props.allowUnsafeTls === undefined + ? {} + : { allowUnsafeTls: props.allowUnsafeTls }), + }, + credentialsPath, ); - if (!tokenSet.refresh_token) { - log.debug("TokenSet is missing refresh_token, starting authentication"); - return null; - } + return { + apiKey: next.access_token ?? "", + apiClient: getApiClient({ + apiKey: next.access_token ?? "", + apiHost: props.apiHost, + }), + refreshed: rotated, + }; +}; +/** + * Exchange the refresh token and persist the result. + * + * Nothing is allowed between the exchange and the write: rotation kills the presented token + * the moment the server answers, so from that point the file is the only copy of a working + * session. `user_id` comes from the previous set rather than a lookup — a refresh cannot + * change who is authenticated, and an API call here is exactly the failure that used to + * strand a rotated token in memory. + * + * `rotated` is false when the token in hand came from another invocation instead of from us. + */ +const performRefresh = async ( + credentials: StoredCredentials, + oauth: OAuthSettings, + credentialsPath: string, +): Promise<{ credentials: StoredCredentials; rotated: boolean }> => { + let next: StoredCredentials; try { - const refreshedTokenSet = await refreshToken( - { - oauthHost: props.oauthHost, - clientId: props.clientId, - allowUnsafeTls: props.allowUnsafeTls, - }, - tokenSet, + const refreshed = await refreshToken(oauth, credentials); + next = toStoredCredentials(refreshed, credentials, Date.now()); + } catch (err) { + // Refresh tokens are one-time use, so a concurrent invocation that beat us to it is + // the likely reason this failed — and its result is already on disk. + const recovered = await recoverConcurrentRefresh( + credentialsPath, + credentials, ); + if (recovered) { + log.debug( + "Refresh lost a race; adopting the credentials another command wrote", + ); + return { credentials: recovered, rotated: false }; + } + throw err instanceof AuthRefreshError + ? err + : classifyRefreshFailure(err); + } - // Extend the token set with expires_at - const extendedTokenSet = extendTokenSet(refreshedTokenSet); - - const apiKey = extendedTokenSet.access_token; - const apiClient = getApiClient({ - apiKey, - apiHost: props.apiHost, - }); - - await preserveCredentials(credentialsPath, extendedTokenSet, apiClient); - log.debug("Token refresh successful"); + writeCredentials(credentialsPath, next); + logCredentialsHash(next); + log.debug("Token refresh successful"); + return { credentials: next, rotated: true }; +}; - return { apiKey, apiClient }; - } catch (err: unknown) { - const typedErr = - err instanceof Error ? err : new Error("Unknown error"); - log.debug("Failed to refresh token: %s", typedErr.message); - throw new Error("AUTH_REFRESH_FAILED"); - } +/** + * Refresh the credentials at `credentialsPath`, ignoring `expires_at`. + * + * Used by the top-level 401 handler, which knows the token was rejected but not why. Returns + * false when there is nothing to refresh with; throws {@link AuthRefreshError} otherwise, so + * the caller can tell a dead session from an unreachable server. + */ +export const refreshStoredCredentials = async ({ + credentialsPath, + oauth, +}: { + credentialsPath: string; + oauth: OAuthSettings; +}): Promise => { + const credentials = readCredentials(credentialsPath); + if (!credentials?.refresh_token) return false; + await performRefresh(credentials, oauth, credentialsPath); + return true; }; export const ensureAuth = async ( @@ -260,14 +346,14 @@ export const ensureAuth = async ( // then triggers OAuth at the right time). Skip the global auth middleware. const isInit = props._[0] === "init"; + /** Commands that must degrade to "no credentials" instead of failing or opening a browser. */ + const runsWithoutCredentials = isLocalDev || isBootstrap || isInit; + // Use existing API key or handle auth command if (props.apiKey || props._[0] === "auth") { if (props.apiKey) { log.debug("Using an API key to authorize requests"); - setAuthContext({ - source: "api-key", - configDir: props.configDir, - }); + setAuthContext({ source: "api-key" }); } props.apiClient = getApiClient({ apiKey: props.apiKey, @@ -277,45 +363,73 @@ export const ensureAuth = async ( } const credentialsPath = credentialsPathFor(props); + const oauth: OAuthSettings = { + oauthHost: props.oauthHost, + clientId: props.clientId, + ...(props.allowUnsafeTls === undefined + ? {} + : { allowUnsafeTls: props.allowUnsafeTls }), + }; + + log.debug("Trying to read credentials from %s", credentialsPath); + let stored: StoredCredentials | null; + try { + stored = readCredentials(credentialsPath); + } catch (err) { + // A corrupt or unreadable credentials file is reported rather than papered over with + // a browser login: the user may have a perfectly good session in a file we simply + // failed to read, and re-authenticating would hide that. + if (runsWithoutCredentials) { + // These commands still run, but the user has a broken credentials file and needs + // to know — `dev` silently dropping env injection looks like a bug in the app. + log.warning( + "%s Continuing without credentials.", + err instanceof Error ? err.message : String(err), + ); + return; + } + throw err; + } - // Handle case when credentials file exists - if (existsSync(credentialsPath)) { - log.debug("Trying to read credentials from %s", credentialsPath); + if (stored) { + let resolved: ResolvedAuth | null; try { - const contents = readFileSync(credentialsPath, "utf8"); - log.debug("Credentials MD5 hash: %s", md5hash(contents)); - const tokenSet: ExtendedTokenSet = JSON.parse(contents); - - // Try to use existing token or refresh it - const result = await handleExistingToken( - tokenSet, + resolved = await resolveStoredCredentials( + stored, props, credentialsPath, ); - if (result) { - props.apiKey = result.apiKey; - props.apiClient = result.apiClient; - setAuthContext({ - source: "stored-credentials", - configDir: props.configDir, - }); - return; - } } catch (err) { - if ( - !( - err instanceof Error && - err.message === "AUTH_REFRESH_FAILED" - ) && - (err as { code: string }).code !== "ENOENT" && - !(err instanceof SyntaxError) - ) { - // Throw for any errors except auth refresh failure, missing file, or invalid credentials file + if (err instanceof AuthRefreshError && !err.terminal) { + // The session may well be fine — we couldn't reach the server to find out. + // Signing the user out here would turn an outage into a re-login. + if (runsWithoutCredentials) { + log.warning( + "%s Continuing without credentials.", + err.message, + ); + return; + } throw err; } + log.debug( + "Stored session is no longer valid, starting authentication: %s", + err instanceof Error ? err.message : String(err), + ); + resolved = null; + } - // Fall through to new auth flow for auth failures - log.debug("Ensure auth failed, starting authentication", err); + if (resolved) { + props.apiKey = resolved.apiKey; + props.apiClient = resolved.apiClient; + setAuthContext({ + source: "stored-credentials", + credentialsPath, + accessToken: resolved.apiKey, + refreshed: resolved.refreshed, + oauth, + }); + return; } } else { log.debug( @@ -351,7 +465,10 @@ export const ensureAuth = async ( }); setAuthContext({ source: "stored-credentials", - configDir: props.configDir, + credentialsPath, + accessToken: apiKey, + refreshed: true, + oauth, }); }; @@ -366,13 +483,18 @@ export const deleteCredentials = ( configDir: string, profile?: string, ): void => { - const credentialsPath = credentialsPathFor({ - configDir, - ...(profile ? { profile } : {}), - }); + deleteCredentialsAt( + credentialsPathFor({ + configDir, + ...(profile ? { profile } : {}), + }), + ); +}; + +/** Delete one credentials file by path, so a named profile's 401 can't clear DEFAULT's. */ +export const deleteCredentialsAt = (credentialsPath: string): void => { try { - if (existsSync(credentialsPath)) { - rmSync(credentialsPath); + if (removeCredentials(credentialsPath)) { log.info("Deleted credentials from %s", credentialsPath); } else { log.debug("Credentials file %s does not exist", credentialsPath); @@ -385,4 +507,12 @@ export const deleteCredentials = ( } }; +/** + * A fingerprint of what was written, so a support log can tell "the file changed" from "the + * file is the one I saw before" without ever containing a token. + */ +const logCredentialsHash = (credentials: StoredCredentials): void => { + log.debug("Credentials MD5 hash: %s", md5hash(JSON.stringify(credentials))); +}; + const md5hash = (s: string) => createHash("md5").update(s).digest("hex"); diff --git a/packages/cli/src/commands/auth_refresh.test.ts b/packages/cli/src/commands/auth_refresh.test.ts new file mode 100644 index 00000000..ee15683b --- /dev/null +++ b/packages/cli/src/commands/auth_refresh.test.ts @@ -0,0 +1,330 @@ +/** + * The refresh path, exercised as a real `neon` process against a real authorization server. + * + * Everything here runs the built CLI in a child process with its own config directory, so + * what is under test is the behaviour a user gets — including the top-level 401 retry loop, + * which only exists in `index.ts` and cannot be reached from an in-process call. + * + * `CI=true` is set on every child. It makes an unexpected fall-through to the browser flow + * fail loudly and deterministically instead of opening a browser on whoever is running the + * suite. + */ + +import { type ChildProcess, fork } from "node:child_process"; +import { + existsSync, + mkdtempSync, + readFileSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { type StoredCredentials, writeCredentials } from "../credentials.js"; +import { + type NeonApiServer, + startNeonApiServer, +} from "../test_utils/neon_api_server.js"; +import { + type RotatingOauthServer, + startRotatingOauthServer, +} from "../test_utils/rotating_oauth_server.js"; + +let oauth: RotatingOauthServer; +let api: NeonApiServer; +let configDir = ""; +const running = new Set(); + +const credentialsFile = () => join(configDir, "credentials.json"); + +beforeEach(async () => { + oauth = await startRotatingOauthServer(); + api = await startNeonApiServer(); + configDir = mkdtempSync(join(tmpdir(), "neon-auth-refresh-")); +}); + +afterEach(async () => { + // A child still waiting on a held request would otherwise keep the socket — and the + // server's `close()` — open forever, turning one failure into a hung suite. + for (const child of running) child.kill("SIGKILL"); + running.clear(); + oauth.releaseHeld(); + rmSync(configDir, { recursive: true, force: true }); + await Promise.all([oauth.stop(), api.stop()]); +}); + +/** Put a token set on disk the way a previous `neon auth` would have left it. */ +const seedCredentials = ( + overrides: Partial = {}, +): StoredCredentials => { + const issued = oauth.issue(); + const stored: StoredCredentials = { + ...issued, + expires_at: Date.now() + issued.expires_in * 1000, + user_id: "user-1", + ...overrides, + }; + writeCredentials(credentialsFile(), stored); + return stored; +}; + +const readStored = (): StoredCredentials => + JSON.parse(readFileSync(credentialsFile(), "utf8")); + +type RunResult = { code: number; stdout: string; stderr: string }; + +const runCli = (args: string[] = ["me"]): Promise => { + const child = fork( + join(process.cwd(), "./dist/index.js"), + [ + "--config-dir", + configDir, + "--api-host", + api.url, + "--oauth-host", + oauth.url, + "--client-id", + "test-client", + "--allow-unsafe-tls", + "--no-analytics", + "--output", + "json", + ...args, + ], + { + stdio: "pipe", + env: { + PATH: process.env.PATH ?? "", + HOME: configDir, + CI: "true", + }, + }, + ); + + running.add(child); + + let stdout = ""; + let stderr = ""; + child.stdout?.on("data", (d: Buffer) => { + stdout += d.toString(); + }); + child.stderr?.on("data", (d: Buffer) => { + stderr += d.toString(); + }); + + return new Promise((resolve, reject) => { + // Shorter than the CLI's own 60s browser-auth timeout, so a run that reaches the + // sign-in flow fails as a test rather than stalling the suite. + const timer = setTimeout(() => { + child.kill("SIGKILL"); + reject( + new Error( + `neon ${args.join(" ")} did not exit within 30s.\nstdout: ${stdout}\nstderr: ${stderr}`, + ), + ); + }, 30_000); + + child.on("error", (err) => { + clearTimeout(timer); + running.delete(child); + reject(err); + }); + child.on("close", (code) => { + clearTimeout(timer); + running.delete(child); + resolve({ code: code ?? -1, stdout, stderr }); + }); + }); +}; + +describe("a valid session", () => { + it("is used as-is, without contacting the authorization server", async () => { + seedCredentials(); + + const result = await runCli(); + + expect(result.code).toBe(0); + expect(result.stdout).toContain("user@example.com"); + expect(oauth.refreshAttempts()).toBe(0); + }); +}); + +describe("an expired session", () => { + it("is refreshed, and the rotated token set replaces the old one", async () => { + const before = seedCredentials({ expires_at: Date.now() - 1000 }); + + const result = await runCli(); + + expect(result.code).toBe(0); + expect(oauth.rotations()).toBe(1); + + const after = readStored(); + expect(after.access_token).not.toBe(before.access_token); + expect(after.refresh_token).not.toBe(before.refresh_token); + expect(after.expires_at).toBeGreaterThan(Date.now()); + // A refresh cannot change who is signed in, so the account survives without a lookup. + expect(after.user_id).toBe("user-1"); + }); + + // The defect this suite exists for: the rotated token is the only working credential in + // the world the instant the exchange returns, so nothing may sit between it and the disk. + it("keeps the rotated token even when the command afterwards fails", async () => { + const before = seedCredentials({ expires_at: Date.now() - 1000 }); + api.failUserLookup(true); + + const failed = await runCli(); + expect(failed.code).toBe(1); + expect(oauth.rotations()).toBe(1); + + const persisted = readStored(); + expect(persisted.access_token).not.toBe(before.access_token); + + // And the session still works: the next command needs no browser and no second + // rotation, because the token that was minted was not thrown away. + api.failUserLookup(false); + const recovered = await runCli(); + expect(recovered.code).toBe(0); + expect(oauth.rotations()).toBe(1); + }); + + it("is left alone when the authorization server cannot be reached", async () => { + const before = seedCredentials({ expires_at: Date.now() - 1000 }); + oauth.setUnreachable(true); + + const result = await runCli(); + + expect(result.code).toBe(1); + expect(result.stderr).toContain( + "Could not reach the Neon authorization server", + ); + // An outage must not cost the user their session. + expect(readStored().refresh_token).toBe(before.refresh_token); + }); + + it("falls back to signing in again once the refresh token is dead", async () => { + const before = seedCredentials({ expires_at: Date.now() - 1000 }); + oauth.revoke(before.refresh_token as string); + + const result = await runCli(); + + expect(result.code).toBe(1); + // `CI=true` turns the browser flow into this error, which is the proof we got there. + expect(result.stderr).toContain("Cannot run interactive auth in CI"); + }); +}); + +describe("concurrent invocations", () => { + // Refresh tokens are one-time use, so two commands that start together cannot both + // exchange: one wins and the other must notice rather than demanding a new login. + it("share a single rotation instead of invalidating each other", async () => { + seedCredentials({ expires_at: Date.now() - 1000 }); + // Force the collision rather than hoping for it: neither exchange is answered until + // both have arrived, so both must have presented the same refresh token and exactly + // one of them can possibly succeed. + oauth.holdUntil(2); + + const [first, second] = await Promise.all([runCli(), runCli()]); + + expect(oauth.refreshAttempts()).toBe(2); + expect(oauth.rotations()).toBe(1); + expect(first.code).toBe(0); + expect(second.code).toBe(0); + expect(readStored().expires_at).toBeGreaterThan(Date.now()); + }); + + it("leaves a working session behind for the next command", async () => { + seedCredentials({ expires_at: Date.now() - 1000 }); + await Promise.all([runCli(), runCli(), runCli()]); + + const result = await runCli(); + + expect(result.code).toBe(0); + expect(oauth.rotations()).toBe(1); + }); +}); + +describe("a 401 from the Neon API", () => { + // An access token that expires mid-command is indistinguishable from a revoked one, and + // only the authorization server can tell them apart. Deleting the refresh token to find + // out costs a browser login every time the answer would have been "expired". + it("is recovered by refreshing, not by deleting the credentials", async () => { + const before = seedCredentials(); + api.reject(before.access_token as string); + + const result = await runCli(); + + expect(result.code).toBe(0); + expect(result.stdout).toContain("user@example.com"); + expect(oauth.rotations()).toBe(1); + expect(existsSync(credentialsFile())).toBe(true); + }); + + it("deletes the credentials only once the refresh confirms the session is gone", async () => { + const before = seedCredentials(); + api.reject(before.access_token as string); + oauth.revoke(before.refresh_token as string); + + const result = await runCli(); + + expect(result.code).toBe(1); + expect(existsSync(credentialsFile())).toBe(false); + }); + + it("keeps the credentials when the refresh cannot reach the server", async () => { + const before = seedCredentials(); + api.reject(before.access_token as string); + oauth.setUnreachable(true); + + const result = await runCli(); + + expect(result.code).toBe(1); + expect(existsSync(credentialsFile())).toBe(true); + expect(readStored().refresh_token).toBe(before.refresh_token); + }); + + // The retry budget is two attempts. On the last one there is no next command to prepare + // for, so rotating or deleting anything only makes the *following* invocation worse. + it("does not touch the credentials when no retry is left", async () => { + const before = seedCredentials(); + api.rejectAll(true); + + const result = await runCli(); + + expect(result.code).toBe(1); + expect(oauth.rotations()).toBe(1); + expect(existsSync(credentialsFile())).toBe(true); + expect(readStored().refresh_token).not.toBe(before.refresh_token); + }); + + it("reports an API key rejection without touching stored credentials", async () => { + seedCredentials(); + api.rejectAll(true); + + const result = await runCli(["me", "--api-key", "some-key"]); + + expect(result.code).toBe(1); + expect(result.stderr).toContain("rejected the API key"); + expect(oauth.refreshAttempts()).toBe(0); + expect(existsSync(credentialsFile())).toBe(true); + }); +}); + +describe("a damaged credentials file", () => { + // The shape a torn write used to leave behind. Reading it as "not signed in" hid the + // damage behind a browser login and lost whatever session was really in there. + it("is reported rather than silently replaced with a new sign-in", async () => { + writeFileSync(credentialsFile(), '{"access_token": "abc', { + mode: 0o600, + }); + + const result = await runCli(); + + expect(result.code).toBe(1); + expect(result.stderr).toContain("is not valid JSON"); + expect(result.stderr).not.toContain( + "Cannot run interactive auth in CI", + ); + }); +}); diff --git a/packages/cli/src/credentials.test.ts b/packages/cli/src/credentials.test.ts new file mode 100644 index 00000000..52b41445 --- /dev/null +++ b/packages/cli/src/credentials.test.ts @@ -0,0 +1,375 @@ +import { + chmodSync, + existsSync, + lstatSync, + mkdirSync, + mkdtempSync, + readdirSync, + readFileSync, + rmSync, + statSync, + symlinkSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { + isAccessTokenUsable, + readCredentials, + recoverConcurrentRefresh, + removeCredentials, + resolveCredentialsTarget, + type StoredCredentials, + toStoredCredentials, + writeCredentials, +} from "./credentials.js"; + +let dir = ""; + +beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), "neon-credentials-")); +}); + +afterEach(() => { + rmSync(dir, { recursive: true, force: true }); +}); + +const credentials = ( + overrides: Partial = {}, +): StoredCredentials => ({ + access_token: "access-1", + refresh_token: "refresh-1", + token_type: "bearer", + expires_at: Date.now() + 3600_000, + ...overrides, +}); + +describe("isAccessTokenUsable", () => { + it("accepts a token whose deadline is in the future", () => { + expect( + isAccessTokenUsable(credentials({ expires_at: 2000 }), 1000), + ).toBe(true); + }); + + it("rejects a token at or past its deadline", () => { + expect( + isAccessTokenUsable(credentials({ expires_at: 1000 }), 1000), + ).toBe(false); + }); + + it("rejects a token set that has no access token", () => { + const withoutAccess = credentials(); + delete (withoutAccess as { access_token?: string }).access_token; + expect(isAccessTokenUsable(withoutAccess, 0)).toBe(false); + }); +}); + +describe("toStoredCredentials", () => { + it("turns the relative expires_in into an absolute deadline", () => { + const stored = toStoredCredentials( + { + access_token: "a", + token_type: "bearer", + expires_in: 3600, + }, + null, + 1_000_000, + ); + expect(stored.expires_at).toBe(1_000_000 + 3600_000); + }); + + it("carries the previous refresh token when the response omits one", () => { + const stored = toStoredCredentials( + { access_token: "new", token_type: "bearer", expires_in: 60 }, + credentials({ refresh_token: "keep-me" }), + 0, + ); + expect(stored.refresh_token).toBe("keep-me"); + }); + + it("prefers a rotated refresh token over the previous one", () => { + const stored = toStoredCredentials( + { + access_token: "new", + token_type: "bearer", + expires_in: 60, + refresh_token: "rotated", + }, + credentials({ refresh_token: "old" }), + 0, + ); + expect(stored.refresh_token).toBe("rotated"); + }); + + it("carries user_id forward, because a refresh cannot change who is signed in", () => { + const stored = toStoredCredentials( + { access_token: "new", token_type: "bearer", expires_in: 60 }, + credentials({ user_id: "user-123" }), + 0, + ); + expect(stored.user_id).toBe("user-123"); + }); +}); + +describe("readCredentials", () => { + it("returns null when there is no file", () => { + expect(readCredentials(join(dir, "credentials.json"))).toBeNull(); + }); + + it("reads a token set back", () => { + const path = join(dir, "credentials.json"); + writeCredentials(path, credentials({ user_id: "user-1" })); + expect(readCredentials(path)).toMatchObject({ + access_token: "access-1", + refresh_token: "refresh-1", + user_id: "user-1", + }); + }); + + it("fails loudly on malformed JSON instead of reading as signed out", () => { + const path = join(dir, "credentials.json"); + writeFileSync(path, "{ not json"); + expect(() => readCredentials(path)).toThrow(/is not valid JSON/); + }); + + it("fails loudly when the file holds no token at all", () => { + const path = join(dir, "credentials.json"); + writeFileSync(path, JSON.stringify({ user_id: "user-1" })); + expect(() => readCredentials(path)).toThrow( + /neither an access token nor a refresh token/, + ); + }); + + it("fails loudly when a token field is the wrong type", () => { + const path = join(dir, "credentials.json"); + writeFileSync(path, JSON.stringify({ access_token: 42 })); + expect(() => readCredentials(path)).toThrow( + /`access_token` must be a string/, + ); + }); + + it("treats an explicit null the same as an absent field", () => { + const path = join(dir, "credentials.json"); + writeFileSync( + path, + JSON.stringify({ + access_token: "a", + refresh_token: "r", + user_id: null, + expires_at: null, + }), + ); + const read = readCredentials(path); + expect(read?.user_id).toBeUndefined(); + expect(read?.expires_at).toBe(0); + }); + + it("rejects a non-finite expires_at rather than treating it as a deadline", () => { + const path = join(dir, "credentials.json"); + writeFileSync(path, '{"access_token":"a","expires_at":1e999}'); + expect(() => readCredentials(path)).toThrow( + /`expires_at` must be a number/, + ); + }); + + it("treats a file written before expires_at existed as expired", () => { + const path = join(dir, "credentials.json"); + writeFileSync( + path, + JSON.stringify({ access_token: "a", refresh_token: "r" }), + ); + const read = readCredentials(path); + expect(read?.expires_at).toBe(0); + expect(isAccessTokenUsable(read as StoredCredentials, 0)).toBe(false); + }); +}); + +describe("writeCredentials", () => { + it("writes owner-only and leaves no temporary file behind", () => { + const path = join(dir, "credentials.json"); + writeCredentials(path, credentials()); + + expect(statSync(path).mode & 0o777).toBe(0o600); + expect(readdirSync(dir)).toEqual(["credentials.json"]); + }); + + it("tightens the permissions of a file an older version left at 0700", () => { + const path = join(dir, "credentials.json"); + writeFileSync(path, JSON.stringify(credentials()), { mode: 0o700 }); + chmodSync(path, 0o700); + + writeCredentials(path, credentials({ access_token: "access-2" })); + + expect(statSync(path).mode & 0o777).toBe(0o600); + }); + + it("replaces the contents atomically rather than truncating in place", () => { + const path = join(dir, "credentials.json"); + writeCredentials(path, credentials({ access_token: "first" })); + writeCredentials(path, credentials({ access_token: "second" })); + + expect(JSON.parse(readFileSync(path, "utf8")).access_token).toBe( + "second", + ); + expect(readdirSync(dir)).toEqual(["credentials.json"]); + }); + + // `~/.config/neonctl` is a symlink on machines that keep several accounts side by side, + // and a `profiles.json` entry may point through one too. + it("writes through a symlinked directory to the real file", () => { + const real = join(dir, "real"); + mkdirSync(real); + const link = join(dir, "link"); + symlinkSync(real, link); + + const path = join(link, "credentials.json"); + writeCredentials(path, credentials({ access_token: "through-link" })); + + expect( + JSON.parse(readFileSync(join(real, "credentials.json"), "utf8")) + .access_token, + ).toBe("through-link"); + }); + + // The dangerous one: rename replaces the *name* it is given, so a naive implementation + // swaps the symlink for a regular file and strands the credentials it pointed at. + it("follows a symlinked credentials file instead of replacing the link", () => { + const target = join(dir, "actual-credentials.json"); + writeCredentials(target, credentials({ access_token: "original" })); + const path = join(dir, "credentials.json"); + symlinkSync(target, path); + + writeCredentials(path, credentials({ access_token: "updated" })); + + // The link must survive: replacing it with a regular file would leave the real + // credentials file frozen at its old contents and split the session in two. + expect(lstatSync(path).isSymbolicLink()).toBe(true); + expect(JSON.parse(readFileSync(target, "utf8")).access_token).toBe( + "updated", + ); + expect(resolveCredentialsTarget(path)).toBe( + resolveCredentialsTarget(target), + ); + }); + + it("creates the target of a dangling symlink rather than replacing the link", () => { + const target = join(dir, "actual-credentials.json"); + const path = join(dir, "credentials.json"); + symlinkSync(target, path); + + writeCredentials(path, credentials({ access_token: "created" })); + + expect(JSON.parse(readFileSync(target, "utf8")).access_token).toBe( + "created", + ); + }); + + it("fails loudly when the directory cannot be written to", () => { + const readOnly = join(dir, "read-only"); + mkdirSync(readOnly); + chmodSync(readOnly, 0o500); + try { + expect(() => + writeCredentials( + join(readOnly, "credentials.json"), + credentials(), + ), + ).toThrow(); + } finally { + chmodSync(readOnly, 0o700); + } + }); +}); + +describe("removeCredentials", () => { + it("reports when there was nothing to remove", () => { + expect(removeCredentials(join(dir, "credentials.json"))).toBe(false); + }); + + it("removes the file", () => { + const path = join(dir, "credentials.json"); + writeCredentials(path, credentials()); + + expect(removeCredentials(path)).toBe(true); + expect(existsSync(path)).toBe(false); + }); + + // Removing the link would leave the real credentials behind and detach the configured + // path, so the next sign-in would write a regular file where the link used to be. + it("removes what a symlink points at, leaving the link in place", () => { + const target = join(dir, "actual-credentials.json"); + writeCredentials(target, credentials()); + const path = join(dir, "credentials.json"); + symlinkSync(target, path); + + expect(removeCredentials(path)).toBe(true); + expect(existsSync(target)).toBe(false); + expect(lstatSync(path).isSymbolicLink()).toBe(true); + }); +}); + +describe("recoverConcurrentRefresh", () => { + it("adopts the token set another invocation wrote", async () => { + const path = join(dir, "credentials.json"); + const attempted = credentials({ access_token: "ours" }); + writeCredentials( + path, + credentials({ access_token: "theirs", refresh_token: "rotated" }), + ); + + expect( + (await recoverConcurrentRefresh(path, attempted))?.access_token, + ).toBe("theirs"); + }); + + it("waits for a winner that is still mid-write", async () => { + const path = join(dir, "credentials.json"); + const attempted = credentials({ access_token: "ours" }); + writeCredentials(path, attempted); + + // The winner's exchange has returned but its write hasn't landed yet — a single + // read here would wrongly report "no winner" and send the user to a browser. + setTimeout(() => { + writeCredentials(path, credentials({ access_token: "theirs" })); + }, 120); + + expect( + (await recoverConcurrentRefresh(path, attempted))?.access_token, + ).toBe("theirs"); + }); + + it("gives up when the file never changes", async () => { + const path = join(dir, "credentials.json"); + const attempted = credentials({ access_token: "ours" }); + writeCredentials(path, attempted); + + expect(await recoverConcurrentRefresh(path, attempted)).toBeNull(); + }); + + it("does not adopt an expired token", async () => { + const path = join(dir, "credentials.json"); + writeCredentials( + path, + credentials({ access_token: "theirs", expires_at: 500 }), + ); + + expect( + await recoverConcurrentRefresh( + path, + credentials({ access_token: "ours" }), + () => 1000, + ), + ).toBeNull(); + }); + + it("returns null rather than throwing when the file went missing", async () => { + expect( + await recoverConcurrentRefresh( + join(dir, "credentials.json"), + credentials(), + ), + ).toBeNull(); + }); +}); diff --git a/packages/cli/src/credentials.ts b/packages/cli/src/credentials.ts new file mode 100644 index 00000000..7e6b929b --- /dev/null +++ b/packages/cli/src/credentials.ts @@ -0,0 +1,357 @@ +/** + * # The credentials file + * + * Reading, validating and durably writing `credentials.json` — the OAuth token set the CLI + * keeps per profile. + * + * Two properties of the Neon authorization server shape everything here: + * + * 1. **Refresh tokens are one-time use and rotate.** Every `grant_type=refresh_token` call + * returns a new refresh token and kills the one that was presented (replaying it answers + * `401 token_inactive`). So the instant a refresh succeeds, the only copy of a usable + * refresh token is in memory, and losing it before it reaches disk costs the user a + * browser login. + * 2. **Rotation already elects a winner.** When two invocations race, the server lets exactly + * one through. That is why this module has no lock: the race has a decided outcome, and + * the loser only needs to notice that someone else won — see + * {@link recoverConcurrentRefresh}. + * + * Consequently a write must be durable and atomic, and nothing may happen between "the + * server rotated our token" and "the new token is on disk". + */ + +import { randomBytes } from "node:crypto"; +import { + closeSync, + fsyncSync, + lstatSync, + openSync, + readFileSync, + readlinkSync, + realpathSync, + renameSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { basename, dirname, isAbsolute, join, resolve } from "node:path"; +import type { TokenEndpointResponse } from "openid-client"; + +import { log } from "./log.js"; + +/** + * What actually lives in `credentials.json`: the authorization server's token response plus + * two fields the CLI adds — `expires_at`, an absolute deadline derived from the relative + * `expires_in`, and `user_id`, the account the token belongs to (read by analytics and used + * to label profiles). + */ +export type StoredCredentials = { + access_token?: string; + refresh_token?: string; + id_token?: string; + scope?: string; + token_type?: string; + expires_in?: number; + /** Absolute deadline in epoch milliseconds, derived from `expires_in` when written. */ + expires_at: number; + /** The Neon account the token belongs to. Read by analytics and used to label profiles. */ + user_id?: string; +}; + +/** An access token is usable when it exists and its deadline has not passed. */ +export const isAccessTokenUsable = ( + credentials: StoredCredentials, + now: number, +): boolean => + typeof credentials.access_token === "string" && + credentials.access_token !== "" && + credentials.expires_at > now; + +/** + * Fold a token response into the credentials to persist. + * + * `refresh_token` and `user_id` are carried over from the previous set when the response + * omits them. Neon always rotates, so in practice `refresh_token` is always present — but + * `NEON_OAUTH_HOST` points the CLI at other authorization servers (the test server among + * them), and one that reuses refresh tokens instead of rotating them would otherwise have + * its only refresh token silently dropped on the first refresh. + * + * A refresh never changes who is authenticated, so `user_id` is carried rather than + * re-fetched. That is what keeps the refresh path free of network calls. + */ +export const toStoredCredentials = ( + response: TokenEndpointResponse, + previous: StoredCredentials | null, + now: number, +): StoredCredentials => { + const refreshToken = response.refresh_token ?? previous?.refresh_token; + return { + ...response, + ...(refreshToken ? { refresh_token: refreshToken } : {}), + ...(previous?.user_id ? { user_id: previous.user_id } : {}), + expires_at: now + (response.expires_in ?? 0) * 1000, + }; +}; + +/** + * Validate a parsed credentials file. + * + * A file that exists but doesn't hold a token set is a corruption to report, not a reason to + * quietly reopen a browser: the user may have a perfectly good session in a file we simply + * failed to read, and silently re-authenticating hides that. + */ +export const parseStoredCredentials = ( + raw: unknown, + path: string, +): StoredCredentials => { + const invalid = (why: string): never => { + throw new Error( + `Credentials file ${path} is not valid: ${why}. Delete it and run \`neon auth\` to sign in again.`, + ); + }; + + if (raw === null || typeof raw !== "object" || Array.isArray(raw)) + return invalid("expected a JSON object"); + + const record: Record = { ...raw }; + + /** + * `null` counts as absent; a wrong type is a corruption to report. The key is taken off + * the record either way, so a normalized value is not shadowed by the raw one when the + * unrecognised remainder is spread back below. + */ + const optionalString = (key: string): string | undefined => { + const value = record[key]; + delete record[key]; + if (value === undefined || value === null) return undefined; + if (typeof value !== "string") invalid(`\`${key}\` must be a string`); + return typeof value === "string" ? value : undefined; + }; + + const accessToken = optionalString("access_token"); + const refreshToken = optionalString("refresh_token"); + const userId = optionalString("user_id"); + const tokenType = optionalString("token_type"); + + const rawExpiresAt = record.expires_at; + delete record.expires_at; + if ( + rawExpiresAt !== undefined && + rawExpiresAt !== null && + (typeof rawExpiresAt !== "number" || !Number.isFinite(rawExpiresAt)) + ) + invalid("`expires_at` must be a number"); + + if (accessToken === undefined && refreshToken === undefined) + return invalid("it holds neither an access token nor a refresh token"); + + return { + // Unrecognised fields are preserved rather than validated: the token response is the + // authorization server's to shape, and dropping what we don't know about would lose + // data on the next write. + ...record, + ...(accessToken === undefined ? {} : { access_token: accessToken }), + ...(refreshToken === undefined ? {} : { refresh_token: refreshToken }), + ...(userId === undefined ? {} : { user_id: userId }), + ...(tokenType === undefined ? {} : { token_type: tokenType }), + // A file written before `expires_at` existed is treated as already expired, which + // sends it down the refresh path rather than presenting a token of unknown age. + expires_at: typeof rawExpiresAt === "number" ? rawExpiresAt : 0, + }; +}; + +/** + * The token set at `path`, or `null` when there is no file there. + * + * Only a missing file reads as "not signed in". Every other failure — unreadable, malformed, + * wrong shape — throws, because those are conditions the user needs told about. + */ +export const readCredentials = (path: string): StoredCredentials | null => { + let contents: string; + try { + contents = readFileSync(path, "utf8"); + } catch (err) { + if (isErrnoCode(err, "ENOENT")) return null; + throw err; + } + + let parsed: unknown; + try { + parsed = JSON.parse(contents); + } catch (err) { + throw new Error( + `Credentials file ${path} is not valid JSON: ${ + err instanceof Error ? err.message : String(err) + }. Delete it and run \`neon auth\` to sign in again.`, + ); + } + + return parseStoredCredentials(parsed, path); +}; + +/** + * Write the token set so that a reader either sees the previous contents or the new ones, + * never a half-written file, and so that a rotated refresh token survives a crash. + * + * Written to a fresh sibling file, flushed, then renamed over the target. `rename` is atomic + * within a filesystem, which is why the temp file is a sibling of the *resolved* target + * rather than of `path`. + */ +export const writeCredentials = ( + path: string, + credentials: StoredCredentials, +): void => { + const target = resolveCredentialsTarget(path); + const directory = dirname(target); + const temp = join( + directory, + `.${basename(target)}.${process.pid}.${randomBytes(6).toString("hex")}.tmp`, + ); + const contents = JSON.stringify(credentials); + + // `wx` fails rather than following a symlink or reusing an existing file, and the mode + // is set at creation so the token is never briefly world-readable. + const fd = openSync(temp, "wx", 0o600); + let renamed = false; + try { + try { + writeFileSync(fd, contents); + fsyncSync(fd); + } finally { + closeSync(fd); + } + renameSync(temp, target); + renamed = true; + } finally { + // Any failure before the rename leaves a file holding a live token. Remove it — + // including when the write or the flush is what failed, not just the rename. + if (!renamed) rmSync(temp, { force: true }); + } + + syncDirectory(directory); + log.debug("Saved credentials to %s", target); +}; + +/** + * Remove the credentials at `path`, following a symlink to whatever it points at. + * + * `rm` on the link itself would delete the pointer and leave the real file behind, so the + * next sign-in would write a regular file at the link's place and permanently detach the + * configured target — the same trap the write path avoids. + */ +export const removeCredentials = (path: string): boolean => { + let target: string; + try { + target = realpathSync(path); + } catch (err) { + if (isErrnoCode(err, "ENOENT")) return false; + throw err; + } + rmSync(target); + return true; +}; + +/** + * Where a write to `path` must actually land. + * + * `rename` replaces the name it is given, so renaming onto a symlink would replace the + * symlink with a regular file and orphan the credentials it pointed at. Both the legacy + * `~/.config/neonctl` directory and `profiles.json` entries are routinely symlinks or paths + * through them, so the final target is resolved first and the temp file is created beside + * *it*. + */ +export const resolveCredentialsTarget = (path: string): string => { + try { + return realpathSync(path); + } catch (err) { + if (!isErrnoCode(err, "ENOENT")) throw err; + } + + // No file there yet — or a symlink pointing at one that doesn't exist. Resolve the + // directory, and follow a dangling link by hand so its target still gets created. + const directory = realpathSync(dirname(path)); + const name = basename(path); + const linked = readLinkTarget(join(directory, name)); + if (linked === null) return join(directory, name); + return isAbsolute(linked) ? linked : resolve(directory, linked); +}; + +/** + * How long to keep looking for a concurrent winner's write, and how often. + * + * The winner persists immediately after its exchange returns, so the gap between "our + * request was rejected" and "their result is on disk" is a few milliseconds of scheduling. + * The budget is generous because the alternative to waiting it out is a browser login. + */ +const RECOVERY_ATTEMPTS = 10; +const RECOVERY_INTERVAL_MS = 50; + +/** + * After a failed refresh, whether another invocation already won the race. + * + * Rotation means a concurrent refresh doesn't just duplicate work, it invalidates ours — the + * winner's success is exactly why we failed. So the recovery is to re-read: a *different* + * usable access token on disk is the winner's, and adopting it turns the loser's failure + * into a no-op instead of a browser login. + * + * Re-read more than once. Losing the race means the winner was mid-exchange when we sent + * ours, so its write may still be a moment away; a single read would report "no winner" + * purely on timing and send the user to a browser. An unchanged file after the whole budget + * means we really did fail. + */ +export const recoverConcurrentRefresh = async ( + path: string, + attempted: StoredCredentials, + now: () => number = Date.now, +): Promise => { + for (let attempt = 0; attempt < RECOVERY_ATTEMPTS; attempt++) { + if (attempt > 0) await delay(RECOVERY_INTERVAL_MS); + + let current: StoredCredentials | null; + try { + current = readCredentials(path); + } catch (err) { + log.debug( + "Could not re-read credentials after a failed refresh: %s", + err instanceof Error ? err.message : String(err), + ); + return null; + } + + if (!current) return null; + if (current.access_token === attempted.access_token) continue; + if (!isAccessTokenUsable(current, now())) continue; + return current; + } + return null; +}; + +const delay = (ms: number): Promise => + new Promise((resolve) => setTimeout(resolve, ms)); + +/** + * Flush the directory entry so the rename itself survives a crash, not just the bytes. + * Directories can't be opened for this on Windows, where the rename is already durable + * enough for our purposes. + */ +const syncDirectory = (directory: string): void => { + if (process.platform === "win32") return; + const fd = openSync(directory, "r"); + try { + fsyncSync(fd); + } finally { + closeSync(fd); + } +}; + +const readLinkTarget = (path: string): string | null => { + try { + if (!lstatSync(path).isSymbolicLink()) return null; + return readlinkSync(path); + } catch (err) { + if (isErrnoCode(err, "ENOENT")) return null; + throw err; + } +}; + +const isErrnoCode = (err: unknown, code: string): boolean => + err instanceof Error && (err as NodeJS.ErrnoException).code === code; diff --git a/packages/cli/src/index.ts b/packages/cli/src/index.ts index 4f78dae0..cf386ea3 100644 --- a/packages/cli/src/index.ts +++ b/packages/cli/src/index.ts @@ -10,12 +10,17 @@ import { trackEvent, } from "./analytics.js"; import { isNeonApiError, messageFromBody, type NeonApiClient } from "./api.js"; -import { defaultClientID } from "./auth.js"; -import { credentialsToClearOn401, getAuthContext } from "./auth_context.js"; -import { deleteCredentials, ensureAuth } from "./commands/auth.js"; +import { AuthRefreshError, defaultClientID } from "./auth.js"; +import { getAuthContext } from "./auth_context.js"; +import { + deleteCredentialsAt, + ensureAuth, + refreshStoredCredentials, +} from "./commands/auth.js"; import commands from "./commands/index.js"; import { defaultDir, ensureConfigDir } from "./config.js"; import { currentContextFile, enrichFromContext } from "./context.js"; +import { isAccessTokenUsable, readCredentials } from "./credentials.js"; import { isNetworkError, matchErrorCode, @@ -119,6 +124,17 @@ builder = builder type: "string", default: defaultClientID, }, + // Only meaningful together with `--oauth-host`, and only for a host that isn't + // HTTPS. The OAuth client refuses plain HTTP outright, so without this there is no + // way to point the CLI at a local authorization server — which is what the auth + // tests need in order to exercise the real flow in a real process. + "allow-unsafe-tls": { + description: + "Allow plain HTTP and unverified TLS when talking to --oauth-host", + hidden: true, + type: "boolean", + default: false, + }, "api-key": { describe: "API key", group: "Global options:", @@ -192,7 +208,112 @@ builder = builder .wrap(null) .fail(false); -async function handleError(msg: string, err: unknown): Promise { +/** + * Recover from a 401 on stored credentials, returning true when the command is worth + * re-running. + * + * The access token is refreshed rather than thrown away. A 401 says the token was rejected, + * not that the session is over — an access token that expired mid-command looks exactly like + * one that was revoked, and only the authorization server can tell them apart. Asking it is + * cheap; guessing wrong costs the user a browser login. + */ +/** Whether the credentials file now holds a usable token other than the one that was rejected. */ +function supersededOnDisk(credentialsPath: string, rejected: string): boolean { + try { + const current = readCredentials(credentialsPath); + return ( + current !== null && + current.access_token !== rejected && + isAccessTokenUsable(current, Date.now()) + ); + } catch { + // A corrupt file is the read path's problem to report, not this one's. + return false; + } +} + +async function recoverFrom401(canRetry: boolean): Promise { + const context = getAuthContext(); + + // The request was authorized with a key the user supplied, so there is nothing of ours + // to clear and nothing to retry — the same key would just be rejected again. + if (context === null || context.source === "api-key") { + log.error( + "Authentication failed: the Neon API rejected the API key. Check --api-key or NEON_API_KEY.", + ); + return false; + } + + // Nothing may be mutated on the last attempt: the command is about to exit either way, + // and rotating or deleting credentials here only makes the *next* invocation worse. + if (!canRetry) { + log.error( + "Authentication failed: the Neon API rejected the stored credentials.", + ); + return false; + } + + // A token minted seconds ago and rejected immediately is not an expiry problem, and + // refreshing again would just mint another one to be rejected. + if (context.refreshed) { + log.error( + "Authentication failed: the Neon API rejected freshly issued credentials. Run `neon auth` to sign in again.", + ); + return false; + } + + // Another command may have rotated the session while this one was mid-request, in which + // case the rejection is of a token that is already superseded. Retry with what is on + // disk rather than spending a refresh — and, since refresh tokens are one-time use, + // rather than presenting one that has already been consumed. + if (supersededOnDisk(context.credentialsPath, context.accessToken)) { + log.debug( + "The rejected token has already been replaced on disk; retrying with the current one", + ); + return true; + } + + try { + if ( + await refreshStoredCredentials({ + credentialsPath: context.credentialsPath, + oauth: context.oauth, + }) + ) { + log.debug("Refreshed the stored session after a 401; retrying"); + return true; + } + } catch (err) { + if (err instanceof AuthRefreshError && !err.terminal) { + // Couldn't reach the authorization server, so we still don't know whether the + // session is dead. Keep the credentials and say what happened. + log.error(err.message); + return false; + } + log.debug( + "Refresh after 401 failed: %s", + err instanceof Error ? err.message : String(err), + ); + } + + log.info("Authentication failed, deleting credentials..."); + try { + deleteCredentialsAt(context.credentialsPath); + return true; + } catch (deleteErr) { + log.debug( + "Failed to delete credentials: %s", + deleteErr instanceof Error ? deleteErr.message : "unknown error", + ); + return false; + } +} + +async function handleError( + msg: string, + err: unknown, + canRetry: boolean, +): Promise { if (process.argv.some((arg) => arg === "--help" || arg === "-h")) { await showHelp(builder); process.exit(0); @@ -203,6 +324,19 @@ async function handleError(msg: string, err: unknown): Promise { log.debug("Stack: %s", err.stack); } + // A failed refresh already carries a message naming the authorization server, which is a + // different host from the API. It has to be reported before the generic network branch + // below, or an unreachable `oauth2.neon.tech` gets blamed on the Neon API and sends the + // user to look at the wrong status page. + if (err instanceof AuthRefreshError) { + log.error(err.message); + if (err.terminal) { + log.error("Run `neon auth` to sign in again."); + } + sendError(err, "AUTH_FAILED"); + return false; + } + // A connection-level failure (no response ever reached us) reads as a cryptic // `fetch failed` from the @neon/sdk / global `fetch` path. Detect it first and // swap in one clear "check your connection" hint. We deliberately do not retry @@ -223,29 +357,7 @@ async function handleError(msg: string, err: unknown): Promise { return false; } else if (err.status === 401) { sendError(err, "AUTH_FAILED"); - const configDir = credentialsToClearOn401(getAuthContext()); - // The request was authorized with a key the user supplied, so there - // is nothing of ours to clear and nothing to retry — the same key - // would just be rejected again. - if (configDir === null) { - log.error( - "Authentication failed: the Neon API rejected the API key. Check --api-key or NEON_API_KEY.", - ); - return false; - } - log.info("Authentication failed, deleting credentials..."); - try { - deleteCredentials(configDir); - return true; // Allow retry for auth failures - } catch (deleteErr) { - log.debug( - "Failed to delete credentials: %s", - deleteErr instanceof Error - ? deleteErr.message - : "unknown error", - ); - return false; - } + return await recoverFrom401(canRetry); } else { const serverMessage = messageFromBody(err.data); if (serverMessage) { @@ -296,7 +408,13 @@ void (async () => { break; } catch (err) { attempts++; - const shouldRetry = await handleError("", err); + // The handler mutates credentials to make a retry viable, so it must know + // whether one is coming. On the last attempt there is nothing to prepare for. + const shouldRetry = await handleError( + "", + err, + attempts < MAX_ATTEMPTS, + ); if (!shouldRetry || attempts >= MAX_ATTEMPTS) { await closeAnalytics(); process.exit(1); diff --git a/packages/cli/src/test_utils/neon_api_server.ts b/packages/cli/src/test_utils/neon_api_server.ts new file mode 100644 index 00000000..71772a34 --- /dev/null +++ b/packages/cli/src/test_utils/neon_api_server.ts @@ -0,0 +1,101 @@ +/** + * A local stand-in for the Neon API, just rich enough to answer `neon me` and to reject a + * bearer token on demand. + * + * The auth tests need to control *which* access token is accepted — that is the difference + * between "expired and refreshed" and "rejected and refreshed" — and to count the calls that + * a refresh must not make. + */ + +import { createServer, type ServerResponse } from "node:http"; +import type { AddressInfo } from "node:net"; + +export type NeonApiServer = { + url: string; + /** Reject this bearer token with 401, as the control plane would for a revoked one. */ + reject: (accessToken: string) => void; + /** Reject every bearer token, whatever it is. */ + rejectAll: (value: boolean) => void; + /** Answer `/users/me` with a 500, to exercise a failure after a successful refresh. */ + failUserLookup: (value: boolean) => void; + /** Bearer tokens seen on `/users/me`, oldest first. */ + seenTokens: () => string[]; + stop: () => Promise; +}; + +export const startNeonApiServer = async ( + user: { id: string; email: string } = { + id: "user-1", + email: "user@example.com", + }, +): Promise => { + const rejected = new Set(); + const seen: string[] = []; + let rejectAll = false; + let failUserLookup = false; + + const server = createServer((req, res) => { + const url = new URL(req.url ?? "/", `http://${req.headers.host}`); + const token = (req.headers.authorization ?? "").replace( + /^Bearer\s+/i, + "", + ); + + if (url.pathname !== "/users/me") { + return json(res, 404, { message: "Not Found" }); + } + + seen.push(token); + + if (rejectAll || token === "" || rejected.has(token)) { + return json(res, 401, { message: "Unauthorized" }); + } + + if (failUserLookup) { + return json(res, 500, { message: "Internal Server Error" }); + } + + return json(res, 200, { + id: user.id, + email: user.email, + login: "tester", + name: "Tester", + projects_limit: 10, + branches_limit: 10, + max_autoscaling_limit: 1, + plan: "free", + auth_accounts: [], + }); + }); + + await new Promise((resolve) => { + server.listen(0, "127.0.0.1", resolve); + }); + + return { + url: `http://127.0.0.1:${(server.address() as AddressInfo).port}`, + reject: (accessToken: string) => { + rejected.add(accessToken); + }, + rejectAll: (value: boolean) => { + rejectAll = value; + }, + failUserLookup: (value: boolean) => { + failUserLookup = value; + }, + seenTokens: () => [...seen], + stop: () => + new Promise((resolve, reject) => { + server.close((err) => (err ? reject(err) : resolve())); + }), + }; +}; + +const json = (res: ServerResponse, status: number, body: unknown): void => { + const payload = JSON.stringify(body); + res.writeHead(status, { + "content-type": "application/json", + "content-length": Buffer.byteLength(payload), + }); + res.end(payload); +}; diff --git a/packages/cli/src/test_utils/rotating_oauth_server.ts b/packages/cli/src/test_utils/rotating_oauth_server.ts new file mode 100644 index 00000000..778a6d68 --- /dev/null +++ b/packages/cli/src/test_utils/rotating_oauth_server.ts @@ -0,0 +1,186 @@ +/** + * A local authorization server that rotates refresh tokens exactly the way Neon's does. + * + * Verified against `https://oauth2.neon.tech`: every `grant_type=refresh_token` exchange + * returns a *new* refresh token and retires the presented one, and replaying a retired token + * answers `400 invalid_grant`. That behaviour is the whole reason the CLI's refresh path + * needs care, so the tests run against a server that reproduces it rather than one that + * hands out the same token forever. + * + * This is a real HTTP server speaking real OAuth, not a stand-in for any of our own code. + */ + +import { randomUUID } from "node:crypto"; +import { + createServer, + type IncomingMessage, + type Server, + type ServerResponse, +} from "node:http"; +import type { AddressInfo } from "node:net"; + +export type RotatingOauthServer = { + url: string; + /** How many refresh exchanges have been attempted, successful or not. */ + refreshAttempts: () => number; + /** How many refresh exchanges have succeeded. */ + rotations: () => number; + /** Mint a token set and register its refresh token, for seeding a credentials file. */ + issue: (options?: { expiresIn?: number }) => { + access_token: string; + refresh_token: string; + expires_in: number; + token_type: "bearer"; + }; + /** Retire a refresh token without exchanging it, to simulate a session revoked elsewhere. */ + revoke: (refreshToken: string) => void; + /** Refuse every exchange with a network-level failure instead of an OAuth error. */ + setUnreachable: (unreachable: boolean) => void; + /** + * Hold every exchange until `count` of them have arrived, then answer in arrival order. + * + * Makes the concurrent case deterministic: without this, one process can finish before + * the other even reads the file, and the test would pass without a race ever happening. + */ + holdUntil: (count: number) => void; + /** Answer anything still held, so a test that ends early doesn't leave a socket open. */ + releaseHeld: () => void; + stop: () => Promise; +}; + +export const startRotatingOauthServer = async ( + options: { accessTokenLifetimeSeconds?: number } = {}, +): Promise => { + const lifetime = options.accessTokenLifetimeSeconds ?? 3600; + const liveRefreshTokens = new Set(); + let refreshAttempts = 0; + let rotations = 0; + let unreachable = false; + let heldUntil: number | null = null; + const held: Array<() => void> = []; + + const issue = ({ expiresIn = lifetime }: { expiresIn?: number } = {}) => { + const refresh = `refresh-${randomUUID()}`; + liveRefreshTokens.add(refresh); + return { + access_token: `access-${randomUUID()}`, + refresh_token: refresh, + expires_in: expiresIn, + token_type: "bearer" as const, + }; + }; + + const server = createServer((req, res) => { + const url = new URL(req.url ?? "/", `http://${req.headers.host}`); + + if (url.pathname === "/.well-known/openid-configuration") { + return json(res, 200, { + issuer: baseUrl(), + token_endpoint: `${baseUrl()}/oauth2/token`, + authorization_endpoint: `${baseUrl()}/oauth2/authorize`, + revocation_endpoint: `${baseUrl()}/oauth2/revoke`, + response_types_supported: ["code"], + grant_types_supported: ["authorization_code", "refresh_token"], + token_endpoint_auth_methods_supported: ["none"], + scopes_supported: ["openid", "offline", "offline_access"], + }); + } + + if (url.pathname === "/oauth2/token" && req.method === "POST") { + return readBody(req, (body) => { + const form = new URLSearchParams(body); + if (form.get("grant_type") !== "refresh_token") { + return json(res, 400, { + error: "unsupported_grant_type", + }); + } + + refreshAttempts += 1; + + if (unreachable) { + // Kill the socket so the client sees a transport failure, which is what + // separates "the server said no" from "we never reached the server". + res.destroy(); + return; + } + + const answer = () => { + const presented = form.get("refresh_token") ?? ""; + if (!liveRefreshTokens.delete(presented)) { + return json(res, 400, { + error: "invalid_grant", + error_description: + "The provided authorization grant or refresh token is invalid, expired, or revoked.", + }); + } + + rotations += 1; + return json(res, 200, { + ...issue(), + scope: "openid offline offline_access", + }); + }; + + if (heldUntil === null) return answer(); + + held.push(answer); + if (held.length < heldUntil) return; + const release = held.splice(0); + heldUntil = null; + for (const respond of release) respond(); + }); + } + + return json(res, 404, { error: "not_found" }); + }); + + await new Promise((resolve) => { + server.listen(0, "127.0.0.1", resolve); + }); + + const baseUrl = () => + `http://127.0.0.1:${(server.address() as AddressInfo).port}`; + + return { + url: baseUrl(), + refreshAttempts: () => refreshAttempts, + rotations: () => rotations, + issue, + revoke: (refreshToken: string) => { + liveRefreshTokens.delete(refreshToken); + }, + setUnreachable: (value: boolean) => { + unreachable = value; + }, + holdUntil: (count: number) => { + heldUntil = count; + }, + releaseHeld: () => { + heldUntil = null; + for (const respond of held.splice(0)) respond(); + }, + stop: () => closeServer(server), + }; +}; + +const json = (res: ServerResponse, status: number, body: unknown): void => { + const payload = JSON.stringify(body); + res.writeHead(status, { + "content-type": "application/json;charset=UTF-8", + "content-length": Buffer.byteLength(payload), + }); + res.end(payload); +}; + +const readBody = (req: IncomingMessage, next: (body: string) => void): void => { + let body = ""; + req.on("data", (chunk: Buffer) => { + body += chunk.toString(); + }); + req.on("end", () => next(body)); +}; + +const closeServer = (server: Server): Promise => + new Promise((resolve, reject) => { + server.close((err) => (err ? reject(err) : resolve())); + });