diff --git a/src/commands/auth/login.ts b/src/commands/auth/login.ts index db6323012..50d698f86 100644 --- a/src/commands/auth/login.ts +++ b/src/commands/auth/login.ts @@ -20,6 +20,7 @@ import { getDbPath } from "../../lib/db/index.js"; import { getUserInfo, setUserInfo } from "../../lib/db/user.js"; import { getEnv } from "../../lib/env.js"; import { + ApiError, AuthError, HostScopeError, ValidationError, @@ -356,6 +357,32 @@ async function handleExistingAuth(force: boolean): Promise { return true; } +/** + * Handle a failure while validating a user-supplied `--token` via + * {@link getUserRegions}. A 401/403 means the token itself is rejected (invalid + * or insufficiently scoped) — clear it and raise a user-facing {@link AuthError}. + * Any other failure (network, 5xx, parse) is NOT a token problem, so the token + * is kept and the original error is re-thrown: surfacing "Invalid API token" + * would mislead the user and discard the real cause, and clearing a + * possibly-valid token on a transient blip is hostile (CLI-19). + * + * @throws {AuthError} for a 401/403 (after clearing the stored token) + * @throws the original error for any other failure + */ +async function handleTokenValidationError(error: unknown): Promise { + if ( + error instanceof ApiError && + (error.status === 401 || error.status === 403) + ) { + await clearAuth(); + throw new AuthError( + "invalid", + "Invalid API token. Please check your token and try again." + ); + } + throw error; +} + export const loginCommand = buildCommand({ auth: false, skipRcUrlCheck: true, @@ -461,12 +488,8 @@ export const loginCommand = buildCommand({ try { await getUserRegions(); - } catch { - await clearAuth(); - throw new AuthError( - "invalid", - "Invalid API token. Please check your token and try again." - ); + } catch (error) { + await handleTokenValidationError(error); } persistLoginUrlAsDefault(flags.url, effectiveHost); diff --git a/src/lib/error-reporting.ts b/src/lib/error-reporting.ts index fc6fdafda..ac145c6e5 100644 --- a/src/lib/error-reporting.ts +++ b/src/lib/error-reporting.ts @@ -4,10 +4,10 @@ * Provides two things: * * 1. **Silencing rules** — `OutputError`, network failures (offline/DNS/proxy), - * `ContextError` (a required value the user omitted), expected-state - * `AuthError`, 401–499 `ApiError`, and 400 `ApiError`s that report an - * unparseable user search query are not sent to Sentry as issues. A - * `cli.error.silenced` metric preserves volume + user/org context. + * `ContextError` (a required value the user omitted), `AuthError` (expected + * auth states the user must act on), 401–499 `ApiError`, and 400 `ApiError`s + * that report an unparseable user search query are not sent to Sentry as + * issues. A `cli.error.silenced` metric preserves volume + user/org context. * * 2. **Grouping tags** — enriches every error event with `cli_error.*` tags * that Sentry's server-side fingerprint rules use for stable grouping. @@ -82,10 +82,13 @@ export function classifySilenced(error: unknown): SilenceReason | null { if (error instanceof ContextError) { return "context_missing"; } - if ( - error instanceof AuthError && - (error.reason === "not_authenticated" || error.reason === "expired") - ) { + // All AuthError reasons are expected auth states the user must act on, not + // CLI bugs: `not_authenticated` (no token), `expired` (token aged out), and + // `invalid` (a bad/insufficiently-scoped token the user supplied). `invalid` + // is now only thrown for a genuine 401/403 (see auth/login.ts) — transient + // network/server failures no longer masquerade as it — so it is safe to + // silence alongside the others (CLI-19). + if (error instanceof AuthError) { return "auth_expected"; } if (error instanceof ApiError && error.status > 400 && error.status < 500) { diff --git a/test/commands/auth/login.test.ts b/test/commands/auth/login.test.ts index 7e9c3acad..4eabc9df4 100644 --- a/test/commands/auth/login.test.ts +++ b/test/commands/auth/login.test.ts @@ -128,7 +128,11 @@ vi.mock("../../../src/lib/db/user.js", async (importOriginal) => { // biome-ignore lint/performance/noNamespaceImport: needed for spyOn mocking import * as dbUser from "../../../src/lib/db/user.js"; -import { AuthError, ValidationError } from "../../../src/lib/errors.js"; +import { + ApiError, + AuthError, + ValidationError, +} from "../../../src/lib/errors.js"; vi.mock("../../../src/lib/interactive-login.js", async (importOriginal) => { const actual = @@ -361,10 +365,10 @@ describe("loginCommand.func --token path", () => { expect(out).toContain("x@y.com"); }); - test("--token: invalid token clears auth and throws AuthError", async () => { + test("--token: 401 from validation clears auth and throws AuthError", async () => { isAuthenticatedSpy.mockReturnValue(false); setAuthTokenSpy.mockReturnValue(undefined); - getUserRegionsSpy.mockRejectedValue(new Error("401 Unauthorized")); + getUserRegionsSpy.mockRejectedValue(new ApiError("Unauthorized", 401)); clearAuthSpy.mockResolvedValue(undefined); const { context } = createContext(); @@ -376,6 +380,24 @@ describe("loginCommand.func --token path", () => { expect(getCurrentUserSpy).not.toHaveBeenCalled(); }); + test("--token: non-401 validation failure keeps token and re-throws cause", async () => { + // A network/server failure during validation is not a token problem: + // surface the real error and do NOT clear a possibly-valid token (CLI-19). + isAuthenticatedSpy.mockReturnValue(false); + setAuthTokenSpy.mockReturnValue(undefined); + const cause = new ApiError("Server error", 503); + getUserRegionsSpy.mockRejectedValue(cause); + clearAuthSpy.mockResolvedValue(undefined); + + const { context } = createContext(); + await expect( + func.call(context, { token: "maybe-good", force: false, timeout: 900 }) + ).rejects.toBe(cause); + + expect(clearAuthSpy).not.toHaveBeenCalled(); + expect(getCurrentUserSpy).not.toHaveBeenCalled(); + }); + test("--token: shows 'Logged in as' when user info fetch succeeds", async () => { isAuthenticatedSpy.mockReturnValue(false); setAuthTokenSpy.mockReturnValue(undefined); diff --git a/test/lib/error-reporting.test.ts b/test/lib/error-reporting.test.ts index c3f253216..75582ded8 100644 --- a/test/lib/error-reporting.test.ts +++ b/test/lib/error-reporting.test.ts @@ -225,8 +225,10 @@ describe("classifySilenced", () => { expect(classifySilenced(new AuthError("expired"))).toBe("auth_expected"); }); - test("does NOT silence AuthError(invalid)", () => { - expect(classifySilenced(new AuthError("invalid"))).toBeNull(); + test("silences AuthError(invalid)", () => { + // `invalid` is only thrown for a genuine 401/403 (a bad/insufficient token + // the user supplied), so it is an expected auth state like the others. + expect(classifySilenced(new AuthError("invalid"))).toBe("auth_expected"); }); test.each([ @@ -534,9 +536,19 @@ describe("reportCliError integration", () => { expect(metricSpy).not.toHaveBeenCalled(); }); - test("captures AuthError(invalid)", () => { + test("silences AuthError(invalid) and emits metric", () => { reportCliError(new AuthError("invalid")); - expect(captureSpy).toHaveBeenCalled(); + expect(captureSpy).not.toHaveBeenCalled(); + expect(metricSpy).toHaveBeenCalledWith( + "cli.error.silenced", + 1, + expect.objectContaining({ + attributes: expect.objectContaining({ + reason: "auth_expected", + auth_reason: "invalid", + }), + }) + ); }); test("captures ApiError(400) with normalized endpoint tag", () => {