Skip to content

fix(auth): give each login failure its own error code and stage #442

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
djgould merged 2 commits into from
Aug 24, 2026
Merged

fix(auth): give each login failure its own error code and stage #442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8192d58
fix(auth): give each login failure its own error code and stage
djgould Aug 22, 2026
8a0416a
fix(auth): address review feedback
djgould Aug 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/auth-login-error-codes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"clerk": patch
---

Report why `clerk auth login` failed instead of collapsing every failure into one error.
The browser sign-in wait, OAuth provider errors, state mismatches, missing authorization codes, and callback bind failures now carry distinct error codes, and login records which step of the flow it reached.
81 changes: 81 additions & 0 deletions packages/cli-core/src/commands/auth/login.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ mock.module("../../lib/autoclaim.ts", () => ({
}));

const { setLogLevel } = await import("../../lib/log.ts");
const telemetryMod = await import("../../lib/telemetry.ts");
const { login } = await import("./login.ts");

describe("login", () => {
Expand Down Expand Up @@ -609,4 +610,84 @@ describe("login", () => {

expect(captured.err).not.toContain("Next steps:");
});

describe("telemetry stages", () => {
let stageSpy: ReturnType<typeof spyOn> | undefined;
let callerStageSpy: ReturnType<typeof spyOn> | undefined;

afterEach(() => {
stageSpy?.mockRestore();
callerStageSpy?.mockRestore();
stageSpy = undefined;
callerStageSpy = undefined;
});

function trackStages() {
const spy = spyOn(telemetryMod, "setTelemetryStage");
stageSpy = spy;
return { calls: () => spy.mock.calls.map((call) => call[0]) };
}

test("a completed login reports the terminal stage", async () => {
mockGetValidToken.mockResolvedValue(null);
mockOAuthSuccess();
const stages = trackStages();

await runLogin();

expect(stages.calls().at(-1)).toBe("done");
});

// The browser wait is where the flow is known to lose people; a run that
// stops there must be attributable to that step and no other.
test("a login abandoned during the browser wait stops at awaiting_callback", async () => {
mockGetValidToken.mockResolvedValue(null);
mockBunSpawn();
mockStartAuthServer.mockReturnValue({
port: 54321,
waitForCallback: mock().mockRejectedValue(new Error("Authentication timed out.")),
stop: mock(),
});
const stages = trackStages();

await expect(runLogin()).rejects.toThrow("Authentication timed out");

expect(stages.calls().at(-1)).toBe("awaiting_callback");
});

// `init` and `link` invoke login mid-flow; a clean return must hand the
// caller's stage back instead of leaving the outer command at "done".
test("a nested login hands the caller's stage back on success", async () => {
mockGetValidToken.mockResolvedValue(null);
mockOAuthSuccess();
callerStageSpy = spyOn(telemetryMod, "currentTelemetryStage").mockReturnValue("link");
const stages = trackStages();

await runLogin({ showNextSteps: false });

expect(stages.calls().at(-1)).toBe("link");
});

test("a failure persisting the token stops at store", async () => {
mockGetValidToken.mockResolvedValue(null);
mockOAuthSuccess();
mockStoreToken.mockRejectedValue(new Error("keychain unavailable"));
const stages = trackStages();

await expect(runLogin()).rejects.toThrow("keychain unavailable");

expect(stages.calls().at(-1)).toBe("store");
});

test("a failure exchanging the code stops at token_exchange", async () => {
mockGetValidToken.mockResolvedValue(null);
mockOAuthSuccess();
mockExchangeCodeForToken.mockRejectedValue(new Error("exchange failed"));
const stages = trackStages();

await expect(runLogin()).rejects.toThrow("exchange failed");

expect(stages.calls().at(-1)).toBe("token_exchange");
});
});
});
20 changes: 20 additions & 0 deletions packages/cli-core/src/commands/auth/login.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { attemptAutoclaim, type AutoclaimResult } from "../../lib/autoclaim.ts";
import { openBrowser } from "../../lib/open.ts";
import { cyan, dim } from "../../lib/color.ts";
import { log } from "../../lib/log.ts";
import { currentTelemetryStage, setTelemetryStage } from "../../lib/telemetry.ts";
import { ensureFirstApplication } from "../../lib/first-application.ts";

interface LoginOptions {
Expand Down Expand Up @@ -92,6 +93,7 @@ async function performOAuthFlow(): Promise<OAuthFlowResult> {
const timeoutMinutes = Math.round(AUTH_TIMEOUT_MS / 60_000);
log.info(`Waiting for authentication (timeout in ${timeoutMinutes}m)...`);

setTelemetryStage("awaiting_callback");
const { code } = await withSpinner("Waiting for authentication...", async () =>
authServer.waitForCallback().catch((error: unknown) => {
authServer.stop();
Expand All @@ -113,6 +115,7 @@ async function performOAuthFlow(): Promise<OAuthFlowResult> {
log.debug(`credentials: could not read outgoing session — ${errorMessage(error)}`);
}

setTelemetryStage("token_exchange");
const tokenResponse = await withSpinner("Completing authentication...", async () =>
exchangeCodeForToken({
code,
Expand All @@ -121,6 +124,7 @@ async function performOAuthFlow(): Promise<OAuthFlowResult> {
}),
);

setTelemetryStage("store");

@wyattjoh wyattjoh Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"store" is the last stage set before performOAuthFlow returns, so it covers storeToken, fetchUserInfo, setAuth, and then back in login(): revokeToken for the superseded session and ensureFirstApplication.

ensureFirstApplication is the one that matters — it's a PLAPI round trip that creates an application, and it's the slowest and most failure-prone step in the tail of this flow. A user whose login dies there produces stage: "store", which reads as a credential-store problem and points debugging at the keychain instead of the API.

Suggested fix — give the app-creation step its own marker

In lib/telemetry.ts:

   // `clerk auth login`
   | "session_check"
   | "awaiting_callback"
   | "token_exchange"
   | "store"
+  | "first_application"

In commands/auth/login.ts:

   // Best-effort: ensure the user has at least one application so downstream
   // commands (clerk link, clerk init) have something to operate on.
+  setTelemetryStage("first_application");
   await withSpinner("Setting up your default application...", async () => ensureFirstApplication());
Sent from Claude

@djgould djgould Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in bf122db — first_application stage added before ensureFirstApplication.

await storeToken(createOAuthSession(tokenResponse));

const userInfo = await fetchUserInfo(tokenResponse.access_token);
Expand All @@ -130,13 +134,27 @@ async function performOAuthFlow(): Promise<OAuthFlowResult> {
}

export async function login(options: LoginOptions = {}): Promise<UserInfo> {
// `init` and `link` call this mid-flow and share the one process-global
// telemetry stage. Login's own markers are worth having while it runs, but
// on a clean return the caller's stage comes back so the rest of *their*
// work isn't reported as `done`. On a throw the login stage stands: that is
// genuinely where the run stopped.
const callerStage = currentTelemetryStage();
const userInfo = await runLogin(options);
if (callerStage) setTelemetryStage(callerStage);
return userInfo;
}

async function runLogin(options: LoginOptions = {}): Promise<UserInfo> {
const { showNextSteps = true, yes } = options;
intro("Signing in");
setTelemetryStage("session_check");
const existingSession = await withSpinner("Checking session...", async () =>
getExistingSession(),
);

if (existingSession && !isHuman()) {
setTelemetryStage("done");
log.success(`Logged in as ${existingSession.email}`);
const claimResult = await handleAutoclaim(process.cwd());
if (showNextSteps) {
Expand Down Expand Up @@ -180,9 +198,11 @@ export async function login(options: LoginOptions = {}): Promise<UserInfo> {

// Best-effort: ensure the user has at least one application so downstream
// commands (clerk link, clerk init) have something to operate on.
setTelemetryStage("first_application");
await withSpinner("Setting up your default application...", async () => ensureFirstApplication());

bar();
setTelemetryStage("done");

@wyattjoh wyattjoh Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

login() isn't only the clerk auth login command body — init calls it from resolveAuthLabel (init/index.ts:460), link from ensureAuth (link/index.ts:119), and doctor from its context fixer. The telemetry context is process-global (telemetry.ts:87), created once per run by the preAction hook against the top-level command, so a stage set in here lands on that command's event.

The concrete failure: clerk link in a logged-out directory calls login(), which reaches this line and sets stage: "done". Control returns to ensureAuth, and link then does the work it actually exists to do — resolveProfile, app selection, writing the profile. If any of that throws, the event is command: "link", outcome: "error", stage: "done". link sets no stages of its own, so this is the only stage it will ever report, and it reports it exclusively on runs that failed after completing auth.

init has the same shape: setTelemetryStage("link"), then authenticateAndLink → login() → "done", then the rest of authenticateAndLink plus detectAndInstall all run before init's next marker ("keys" at index.ts:179). Every failure in that window is attributed to "done".

That inverts what the stage field is for. The same hazard applies to the whole flat TelemetryStage union — a future command adding a "link" or "keys" stage will silently collide with init's.

Suggested fix — restore the caller's stage on a clean return

Restoring only on a clean return keeps the useful half: a run that dies inside the browser wait still reports awaiting_callback, correctly attributed to whichever command was running. It's the successful return that has to hand the stage back.

In lib/telemetry.ts:

/** Read the stage a caller had set, so a nested flow can hand it back. */
export function currentTelemetryStage(): TelemetryStage | null {
  return context?.stage ?? null;
}

In commands/auth/login.ts — rename the existing body to runLogin and wrap it:

export async function login(options: LoginOptions = {}): Promise<UserInfo> {
  // `init`, `link`, and `doctor` call this mid-flow and share the one
  // process-global stage. Its own markers are worth having while it runs, but
  // on a clean return the caller's stage comes back so the rest of *their*
  // work isn't reported as `done`. On a throw the login stage stands: that is
  // genuinely where the run stopped.
  const callerStage = currentTelemetryStage();
  const userInfo = await runLogin(options);
  if (callerStage) setTelemetryStage(callerStage);
  return userInfo;
}

Gating the stage calls on login being the invoked command works too, and is more explicit — but it needs a signal login doesn't currently have (showNextSteps: false correlates with nested calls today, which is a coincidence worth not depending on).

Sent from Claude

@djgould djgould Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in bf122db — took the wrapper: currentTelemetryStage() reader, and login() hands the caller's stage back on a clean return (login's own stage stands on a throw). Mutation-checked: the nested test fails with the restore removed.

log.success(`Logged in as ${userInfo.email}`);

const claimResult = await handleAutoclaim(process.cwd());
Expand Down
53 changes: 53 additions & 0 deletions packages/cli-core/src/lib/auth-server.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,25 @@
import { afterEach, describe, expect, spyOn, test } from "bun:test";
import { startAuthServer } from "./auth-server.ts";
import { AUTH_TIMEOUT_MS } from "./constants.ts";
import { ERROR_CODE } from "./errors.ts";
import { useCaptureLog } from "../test/lib/stubs.ts";

describe("auth-server", () => {
let serveSpy: ReturnType<typeof spyOn> | undefined;
let clearTimeoutSpy: ReturnType<typeof spyOn> | undefined;
let timeoutSpy: ReturnType<typeof spyOn> | undefined;
let openServer: { stop: () => void } | undefined;
useCaptureLog();

afterEach(() => {
serveSpy?.mockRestore();
clearTimeoutSpy?.mockRestore();
timeoutSpy?.mockRestore();
openServer?.stop();
serveSpy = undefined;
clearTimeoutSpy = undefined;
timeoutSpy = undefined;
openServer = undefined;
});

test("starts on a random port", () => {
Expand All @@ -31,6 +39,48 @@ describe("auth-server", () => {
expect(clearTimeoutSpy).toHaveBeenCalled();
});

// A host that forbids binding loopback fails every login on the machine —
// worth telling apart from anything the user did.
test("a bind failure carries the callback_bind_failed code", () => {
serveSpy = spyOn(Bun, "serve").mockImplementation(() => {
throw new Error("listen failed");
});

expect(() => startAuthServer("test-state")).toThrowError(
expect.objectContaining({ code: ERROR_CODE.CALLBACK_BIND_FAILED }),
);
});

// The wait expiring is the single most common way login ends without a
// session; it must not look like a crash.
test("the callback wait timing out carries the auth_timeout code", async () => {
let fire: (() => void) | undefined;
const realSetTimeout = globalThis.setTimeout;
timeoutSpy = spyOn(globalThis, "setTimeout").mockImplementation(((
cb: () => void,
ms?: number,
...rest: unknown[]
) => {
// Capture only the login deadline; everything else keeps real timing.
if (ms === AUTH_TIMEOUT_MS) {
fire = cb;
return 0 as unknown as ReturnType<typeof setTimeout>;
}
return realSetTimeout(cb, ms, ...rest);
}) as typeof setTimeout);

const server = startAuthServer("test-state");
openServer = server;
const errorPromise = server.waitForCallback().catch((e: unknown) => e);

expect(fire).toBeDefined();
fire?.();

const error = await errorPromise;
expect(error).toMatchObject({ code: ERROR_CODE.AUTH_TIMEOUT });
expect((error as Error).message).toContain("timed out");
});

test("callback resolves with code on valid request", async () => {
const state = "my-test-state";
const server = startAuthServer(state);
Expand Down Expand Up @@ -60,6 +110,7 @@ describe("auth-server", () => {
const error = await errorPromise;
expect(error).toBeInstanceOf(Error);
expect((error as Error).message).toContain("Invalid state");
expect(error).toMatchObject({ code: ERROR_CODE.OAUTH_STATE_MISMATCH });
});

test("callback rejects on missing code", async () => {
Expand All @@ -73,6 +124,7 @@ describe("auth-server", () => {
const error = await errorPromise;
expect(error).toBeInstanceOf(Error);
expect((error as Error).message).toContain("No authorization code");
expect(error).toMatchObject({ code: ERROR_CODE.OAUTH_NO_CODE });
});

test("callback rejects on OAuth error", async () => {
Expand All @@ -88,6 +140,7 @@ describe("auth-server", () => {
const error = await errorPromise;
expect(error).toBeInstanceOf(Error);
expect((error as Error).message).toContain("User denied access");
expect(error).toMatchObject({ code: ERROR_CODE.OAUTH_PROVIDER_ERROR });
});

test("root path returns waiting message", async () => {
Expand Down
34 changes: 29 additions & 5 deletions packages/cli-core/src/lib/auth-server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
*/

import { AUTH_TIMEOUT_MS, CALLBACK_PATH } from "./constants.ts";
import { CliError, ERROR_CODE, errorMessage } from "./errors.ts";
import { observeHostCapabilityFailure } from "./host-execution.ts";
import { log } from "./log.ts";
import { whileAwaitingUser } from "./signals.ts";
Expand Down Expand Up @@ -188,8 +189,9 @@ export function startAuthServer(expectedState: string): AuthServerResult {
const timeout = setTimeout(() => {
log.debug(`auth-server: timed out after ${AUTH_TIMEOUT_MS}ms`);
rejectCallback(
new Error(
new CliError(
"Authentication timed out. Run `clerk auth login` to try again — if your browser did not open, copy the printed URL into any browser on this machine.",
{ code: ERROR_CODE.AUTH_TIMEOUT },
),
);
// `stop()` is a promise nothing can wait on: the login flow has already
Expand All @@ -213,7 +215,11 @@ export function startAuthServer(expectedState: string): AuthServerResult {
if (error) {
const description = url.searchParams.get("error_description") || error;
log.debug(`auth-server: OAuth error in callback — ${error}: ${description}`);
rejectCallback(new Error(`OAuth error: ${description}`));
rejectCallback(
new CliError(`OAuth error: ${description}`, {
code: ERROR_CODE.OAUTH_PROVIDER_ERROR,
}),
);
clearTimeout(timeout);
setTimeout(() => void server?.stop(), 100);
return new Response(ERROR_HTML(description), {
Expand All @@ -223,7 +229,11 @@ export function startAuthServer(expectedState: string): AuthServerResult {

if (state !== expectedState) {
log.debug(`auth-server: state mismatch (expected=${expectedState}, got=${state})`);
rejectCallback(new Error("Invalid state parameter. Possible CSRF attack."));
rejectCallback(
new CliError("Invalid state parameter. Possible CSRF attack.", {
code: ERROR_CODE.OAUTH_STATE_MISMATCH,
}),
);
clearTimeout(timeout);
setTimeout(() => void server?.stop(), 100);
return new Response(ERROR_HTML("Invalid state parameter."), {
Expand All @@ -234,7 +244,11 @@ export function startAuthServer(expectedState: string): AuthServerResult {

if (!code) {
log.debug("auth-server: callback received with no authorization code");
rejectCallback(new Error("No authorization code received."));
rejectCallback(
new CliError("No authorization code received.", {
code: ERROR_CODE.OAUTH_NO_CODE,
}),
);
clearTimeout(timeout);
setTimeout(() => void server?.stop(), 100);
return new Response(ERROR_HTML("No authorization code received."), {
Expand Down Expand Up @@ -266,7 +280,17 @@ export function startAuthServer(expectedState: string): AuthServerResult {
target: "127.0.0.1:0",
label: CALLBACK_PATH,
});
throw error;
// A sandbox or firewall that forbids binding loopback fails every login on
// the machine; it is a distinct condition from anything the user did.
log.debug(
`auth-server: bind failed — ${error instanceof Error ? (error.stack ?? error.message) : String(error)}`,
);
throw new CliError(

@wyattjoh wyattjoh Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

errorMessage(error) preserves the message text, but the thrown CliError carries no reference to the original — CliErrorOptions has no cause, and observeHostCapabilityFailure only logs when isAgent() and the failure matches its sandbox heuristic. In a normal human-mode run the errno and stack are gone before anyone can look at them, and EACCES (sandbox/firewall) versus EADDRINUSE versus something else is exactly the distinction the new callback_bind_failed code exists to help diagnose.

Suggested fix — keep the original recoverable under --verbose

Every other branch in this file already debug-logs before rejecting; this one is the outlier.

Suggested change
throw new CliError(
log.debug(`auth-server: bind failed — ${error instanceof Error ? (error.stack ?? error.message) : String(error)}`);
throw new CliError(
Sent from Claude

@djgould djgould Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in bf122db — original error (stack included) debug-logged before the CliError wraps it.

`Could not start the local sign-in callback server: ${errorMessage(error)}`,
{
code: ERROR_CODE.CALLBACK_BIND_FAILED,
},
);
}

const activeServer = server;
Expand Down
11 changes: 11 additions & 0 deletions packages/cli-core/src/lib/errors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,17 @@ export const ERROR_CODE = {
NO_INTERACTIVE_TERMINAL: "no_interactive_terminal",
/** Named environment isn't configured. */
INVALID_ENVIRONMENT: "invalid_environment",

/** The browser sign-in wait expired before a callback arrived. */
AUTH_TIMEOUT: "auth_timeout",
/** The authorization server redirected back with an `error` parameter. */
OAUTH_PROVIDER_ERROR: "oauth_provider_error",
/** Callback `state` did not match what this process generated. */
OAUTH_STATE_MISMATCH: "oauth_state_mismatch",
/** Callback arrived without an authorization code. */
OAUTH_NO_CODE: "oauth_no_code",
/** The loopback callback server could not bind a local port. */
CALLBACK_BIND_FAILED: "callback_bind_failed",
} as const;

export type ErrorCode = (typeof ERROR_CODE)[keyof typeof ERROR_CODE];
Expand Down
Loading