From e8ce2b0818686ad966181b962f9b9a04483eae39 Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Mon, 19 Oct 2026 12:43:07 +0800 Subject: [PATCH 1/6] feat(step): store and refresh an account session beside the API key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `step login` redeemed only the account's API key, which is enough for model requests but not for anything that needs to know who the user is. The web and desktop clients need that, so the login flow now also redeems an account session, keeps it on disk, and renews it on demand. The API key stays the only credential for model requests and lives where it always did. The session is a separate file with a separate lifetime, because the two fail differently: a dead session must never cost the user their key. Providers gains the protocol half — parsing the optional session out of the login poll, and the refresh call that exchanges it for a new one. The refresh rebuilds the Oasis cookie from the two stored tokens, because a CLI has no cookie jar, and reads the new pair from the response body. Coding-agent gains the product half — storage, the refresh policy, and the entry point hosts will call. Refreshing moves the network call outside the cross-process lock and re-reads under it before committing, so a sign-out or a sign-in with another account that lands mid-refresh discards the stale result instead of resurrecting a session the user already replaced. Only an answer that positively reports an anonymous session counts as dead. Every HTTP failure and every unexpected body is treated as transient, since deleting a long-lived credential on a guess costs a full re-login while keeping it costs one retried request. Backends that predate the session field deliver the API key alone, which is the behaviour the CLI has always had; nothing changes for a user who never signs in again. --- apps/cli/src/main.ts | 12 + packages/coding-agent/src/index.ts | 11 + packages/coding-agent/src/step/auth.ts | 10 +- packages/coding-agent/src/step/login-flow.ts | 56 ++- packages/coding-agent/src/step/login-state.ts | 389 ++++++++++++++++++ .../coding-agent/src/step/login-status.ts | 43 +- packages/coding-agent/src/step/onboarding.ts | 4 + .../coding-agent/test/step-cli-login.test.ts | 89 ++++ .../test/step-login-state.test.ts | 358 ++++++++++++++++ .../test/step-login-status.test.ts | 76 +++- .../coding-agent/test/step-plan-login.test.ts | 86 +++- packages/providers/src/step-provider/index.ts | 8 + .../src/step-provider/login-client.ts | 97 ++++- .../src/step-provider/login-refresh.ts | 165 ++++++++ .../providers/test/step-login-refresh.test.ts | 137 ++++++ 15 files changed, 1522 insertions(+), 19 deletions(-) create mode 100644 packages/coding-agent/src/step/login-state.ts create mode 100644 packages/coding-agent/test/step-login-state.test.ts create mode 100644 packages/providers/src/step-provider/login-refresh.ts create mode 100644 packages/providers/test/step-login-refresh.test.ts diff --git a/apps/cli/src/main.ts b/apps/cli/src/main.ts index 123ae8ac..a1c7efc3 100644 --- a/apps/cli/src/main.ts +++ b/apps/cli/src/main.ts @@ -603,6 +603,18 @@ try { ? "credential is invalid or expired" : "could not check credential validity (network error)"; process.stdout.write(`Validity: ${validity}.\n`); + // The account session is reported next to the key, never instead + // of it: a user who only ever had a key still has a working key. + const accountSession = status.session; + if (accountSession && accountSession.state !== "missing") { + const detail = + accountSession.state === "expired" + ? "expired; account features will ask you to sign in again" + : accountSession.state === "expiring" + ? "expiring; it refreshes on next use" + : "active"; + process.stdout.write(`Account session: ${detail}.\n`); + } if (status.validity === "invalid") process.exitCode = 1; } } diff --git a/packages/coding-agent/src/index.ts b/packages/coding-agent/src/index.ts index 9451c774..1205780e 100644 --- a/packages/coding-agent/src/index.ts +++ b/packages/coding-agent/src/index.ts @@ -611,6 +611,17 @@ export { type StepLoginOutcome, syncStepLoginProfileEndpoint, } from "./step/login-flow.ts"; +export { + buildStepLoginState, + clearStepLoginState, + getStepLoginStatePath, + getStepLoginToken, + readStepLoginState, + type StepLoginState, + type StepLoginTokenOptions, + type StepLoginTokenOutcome, + saveStepLoginState, +} from "./step/login-state.ts"; export { getStepLoginStatus, type StepCredentialValidity, diff --git a/packages/coding-agent/src/step/auth.ts b/packages/coding-agent/src/step/auth.ts index 86745770..a53b6a02 100644 --- a/packages/coding-agent/src/step/auth.ts +++ b/packages/coding-agent/src/step/auth.ts @@ -13,6 +13,7 @@ import type { Credential } from "@step-harness/providers"; import { AuthStorage, readStoredCredential } from "../core/auth-storage.ts"; import { STEP_PROVIDER_ID, STEP_STATIC_REFRESH_TOKEN } from "../features/step-provider/index.ts"; import { LEGACY_RENAMED_CONFIG_DIR, resolveStepConfigRoot, resolveStepHomeDir } from "./environment.ts"; +import { clearStepLoginState, getStepLoginStatePath } from "./login-state.ts"; /** Current Pi-shaped credential path under the StepCode agent directory. */ export function getStepAuthPath(env: Record = process.env): string { @@ -125,6 +126,8 @@ export async function migrateLegacyStepCredential( export interface StepLogoutResult { removedNative: boolean; removedLegacy: boolean; + /** True when a stored account session was removed. */ + removedLoginState: boolean; nativePath: string; legacyPath: string; remainingSource: "environment" | null; @@ -136,8 +139,13 @@ export async function logoutStepCredentials( env?: Record; } = {}, ): Promise { + const env = options.env ?? process.env; const nativePath = normalize(options.nativePath ?? getStepAuthPath()); const legacyPath = normalize(options.legacyPath ?? getLegacyStepAuthPath()); + // The account session is a separate file with a separate lifetime, so it is + // cleared here too — but only the session, never anything else on disk. + const removedLoginState = existsSync(getStepLoginStatePath(env)); + await clearStepLoginState(env); let removedNative = false; let removedLegacy = false; @@ -172,10 +180,10 @@ export async function logoutStepCredentials( } } - const env = options.env ?? process.env; return { removedNative, removedLegacy, + removedLoginState, nativePath, legacyPath, remainingSource: env.STEP_API_KEY?.trim() ? "environment" : null, diff --git a/packages/coding-agent/src/step/login-flow.ts b/packages/coding-agent/src/step/login-flow.ts index bedb09b2..c30ee238 100644 --- a/packages/coding-agent/src/step/login-flow.ts +++ b/packages/coding-agent/src/step/login-flow.ts @@ -3,12 +3,15 @@ import { AuthStorage, readStoredCredential } from "../core/auth-storage.ts"; import { defaultStepCliClientInfo, loginStepOAuth, + parseStepCliLoginState, STEP_PROVIDER_ID, STEP_STATIC_REFRESH_TOKEN, + type StepCliLoginState, } from "../features/step-provider/index.ts"; import { detectTerminalBackgroundFromEnv, initTheme, resolveThemeSetting, theme } from "../theme/theme.ts"; import { openBrowser } from "../utils/open-browser.ts"; import { resolveStepAgentDir } from "./environment.ts"; +import { buildStepLoginState, saveStepLoginState } from "./login-state.ts"; import { INITIAL_STEP_LOGIN_STEP, isStepLoginSettled, @@ -58,8 +61,20 @@ export interface RunStepLoginOptions { readonly requiredPlan?: "step_plan" | "step_plan_oversea"; } -/** Model discovery can return fallback models even when the account has no plan. */ -export function getMissingStepPlanProfile(state: AccountUsageState): "step_plan" | "step_plan_oversea" | undefined { +/** + * Narrow the account session that rode along on the OAuth credentials. + * + * It arrives through an `unknown` bag, so it is validated here rather than + * trusted. This is the *parsed* shape; the wire shape is validated where the + * protocol defines it, in providers, so the two cannot drift apart. + */ +function readLoginState(value: unknown): StepCliLoginState | undefined { + return parseStepCliLoginState(value); +} + +/** Model discovery can return fallback models even when the account has no plan. */ export function getMissingStepPlanProfile( + state: AccountUsageState, +): "step_plan" | "step_plan_oversea" | undefined { const { snapshot, account } = state; if (state.status !== "ready" || !snapshot || !account.eligible) return undefined; if (account.profile !== "step_plan" && account.profile !== "step_plan_oversea") return undefined; @@ -229,6 +244,8 @@ export async function runStepLogin(options: RunStepLoginOptions = {}): Promise void) | undefined; const finished = new Promise((resolve) => { settle = resolve; @@ -272,14 +289,22 @@ export async function runStepLogin(options: RunStepLoginOptions = {}): Promise { + async function persist( + choice: StepLoginProfileId, + apiKey: string, + uid?: string, + login?: StepCliLoginState, + ): Promise { const saving = step; try { const profile = profiles.find((candidate) => candidate.id === choice); @@ -348,10 +381,21 @@ export async function runStepLogin(options: RunStepLoginOptions = {}): Promise new Date()))().toISOString(), }); + // The account session is a separate file on purpose: it expires and + // refreshes on its own, and losing it must never cost the API key. + if (login) { + try { + await saveStepLoginState(buildStepLoginState(login, choice), options.env ?? process.env); + } catch (error) { + loginWarning = `Signed in, but the account session could not be saved: ${ + error instanceof Error ? error.message : String(error) + }. Account features will ask you to sign in again.`; + } + } if (step !== saving) return; savedProfile = profile; savedPath = authPath; - warning = undefined; + warning = loginWarning; syncStepLoginProfileEndpoint(authPath, options.env ?? process.env); dispatch({ type: profile.credentialSource === "browser" ? "checkPlan" : "saved" }); } catch (error) { diff --git a/packages/coding-agent/src/step/login-state.ts b/packages/coding-agent/src/step/login-state.ts new file mode 100644 index 00000000..ca73882b --- /dev/null +++ b/packages/coding-agent/src/step/login-state.ts @@ -0,0 +1,389 @@ +/** + * Account session stored beside the API key. + * + * `step login` redeems two independent things: an API key that authorizes model + * requests, and an account session that lets a host act as the signed-in user + * (account lookups) and renew itself. The API key lives in `auth.json` with the + * other provider credentials; this session lives here, because the two have + * different lifetimes, different owners, and different failure modes — a dead + * session must never take the API key down with it. + * + * Nothing in the CLI needs the session today. It is stored and refreshed so + * that hosts which do need an identity (the web and desktop clients) can read + * it, and so a user who never signs in again keeps working exactly as before. + */ + +import { randomUUID } from "node:crypto"; +import { existsSync, readFileSync } from "node:fs"; +import { chmod, mkdir, rename, rm, writeFile } from "node:fs/promises"; +import { dirname, join } from "node:path"; +import type { StepCliLoginState } from "@step-harness/providers/step-provider"; +import { refreshStepLoginState, StepLoginRefreshError } from "@step-harness/providers/step-provider"; +import { acquireSettingsLockSync } from "../core/settings-manager.ts"; +import { resolveStepConfigRoot } from "./environment.ts"; +import { resolveStepLoginProfiles, type StepLoginProfileId } from "./onboarding.ts"; + +/** Bump when the on-disk shape changes incompatibly. */ +const LOGIN_STATE_VERSION = 1; + +/** Refresh this long before the access token actually expires. */ +export const STEP_LOGIN_REFRESH_MARGIN_SEC = 5 * 60; + +/** Fallback when neither the server nor the token states a lifetime. */ +const DEFAULT_ACCESS_TOKEN_TTL_SEC = 30 * 60; + +const LOGIN_STATE_FILE_NAME = "step-login.json"; + +export interface StepLoginState { + readonly version: number; + /** Login region; decides which developer center refreshes this session. */ + readonly profile: StepLoginProfileId; + readonly appId: string; + readonly deviceId: string; + readonly accessToken: string; + readonly refreshToken: string; + /** Unix seconds. */ + readonly accessExpiresAt: number; + /** Unix seconds, when the refresh token carries one. Past this, re-login. */ + readonly refreshExpiresAt?: number; +} + +/** Where the session lives. Overridable so hosts can isolate their resources. */ +export function getStepLoginStatePath(env: Record = process.env): string { + return env.STEPCODE_LOGIN_STATE_PATH?.trim() || join(resolveStepConfigRoot(env), LOGIN_STATE_FILE_NAME); +} + +/** + * Read the stored session. + * + * A missing, unreadable or wrongly shaped file means "no session" rather than an + * error: the CLI must keep working for users who only ever had an API key. + */ +export function readStepLoginState(env: Record = process.env): StepLoginState | undefined { + const path = getStepLoginStatePath(env); + if (!existsSync(path)) return undefined; + let parsed: unknown; + try { + parsed = JSON.parse(readFileSync(path, "utf8")) as unknown; + } catch { + return undefined; + } + return parseStepLoginState(parsed); +} + +function parseStepLoginState(value: unknown): StepLoginState | undefined { + if (!value || typeof value !== "object" || Array.isArray(value)) return undefined; + const record = value as Record; + // A newer shape must not be read as an older one: the required fields below + // would otherwise either pass by luck or silently degrade to "no session". + if (typeof record.version === "number" && record.version > LOGIN_STATE_VERSION) return undefined; + const accessToken = readToken(record.accessToken); + const refreshToken = readToken(record.refreshToken); + const deviceId = readToken(record.deviceId); + const appId = readToken(record.appId); + const profile = readToken(record.profile); + const accessExpiresAt = readTimestamp(record.accessExpiresAt); + if (!accessToken || !refreshToken || !deviceId || !appId || !profile || accessExpiresAt === undefined) { + return undefined; + } + const refreshExpiresAt = readTimestamp(record.refreshExpiresAt); + return { + version: typeof record.version === "number" ? record.version : LOGIN_STATE_VERSION, + profile: profile as StepLoginProfileId, + appId, + deviceId, + accessToken, + refreshToken, + accessExpiresAt, + ...(refreshExpiresAt === undefined ? {} : { refreshExpiresAt }), + }; +} + +/** Build the stored shape from what the login flow redeemed. */ +export function buildStepLoginState( + login: StepCliLoginState, + profile: StepLoginProfileId, + now: number = Date.now(), +): StepLoginState { + const nowSec = Math.floor(now / 1000); + const accessTtl = login.expiresIn && login.expiresIn > 0 ? login.expiresIn : DEFAULT_ACCESS_TOKEN_TTL_SEC; + const refreshExpiresAt = readJwtExpiresAt(login.refreshToken); + return { + version: LOGIN_STATE_VERSION, + profile, + appId: login.appId, + deviceId: login.deviceId, + accessToken: login.accessToken, + refreshToken: login.refreshToken, + accessExpiresAt: nowSec + accessTtl, + ...(refreshExpiresAt === undefined ? {} : { refreshExpiresAt }), + }; +} + +/** + * Persist the session. + * + * Written atomically and owner-only: it holds a long-lived refresh token, so a + * partially written or world-readable file is a credential leak. + */ +export async function saveStepLoginState( + state: StepLoginState, + env: Record = process.env, +): Promise { + const path = getStepLoginStatePath(env); + await mkdir(dirname(path), { recursive: true }); + // A fixed name would let two concurrent writers in one process share a + // temporary file, and a crash mid-write would leave a credential behind. + const temporary = `${path}.${process.pid}.${randomUUID()}.tmp`; + try { + await writeFile(temporary, `${JSON.stringify(state, null, 2)}\n`, { encoding: "utf8", mode: 0o600 }); + await rename(temporary, path); + } catch (error) { + await rm(temporary, { force: true }).catch(() => {}); + throw error; + } + // `rename` keeps the temporary file's mode, but a pre-existing file may have + // been created with wider permissions by an older release. + await chmod(path, 0o600); +} + +/** Drop the session. The API key in `auth.json` is deliberately left alone. */ +export async function clearStepLoginState(env: Record = process.env): Promise { + await rm(getStepLoginStatePath(env), { force: true }).catch(() => {}); +} + +/** + * Delete the session, but only while it is still the one that was examined. + * + * A sign-in with another account, or a sign-out, can land while a refresh is in + * flight. Deleting unconditionally would then remove a session the user just + * created, so the stored refresh token has to match before anything is removed. + * + * The lock matters for the same reason: without it a delete can land between a + * committer's re-read and its write, resurrecting a session that was signed out. + */ +async function discardLoginState( + env: Record, + expectedRefreshToken: string, +): Promise { + const path = getStepLoginStatePath(env); + if (!existsSync(path)) return false; + let release: (() => void) | undefined; + try { + release = acquireSettingsLockSync(path); + } catch { + // Not lockable: it either vanished or another process owns it. Leaving it + // is safer than deleting something that may already have been replaced. + return false; + } + try { + const latest = readStepLoginState(env); + if (!latest || latest.refreshToken !== expectedRefreshToken) return false; + await rm(path, { force: true }); + return true; + } finally { + release(); + } +} + +export type StepLoginTokenOutcome = + | { + readonly status: "ready"; + readonly accessToken: string; + readonly deviceId: string; + readonly appId: string; + readonly profile: StepLoginProfileId; + } + | { readonly status: "missing" } + /** The refresh token is gone; only a new sign-in can recover. */ + | { readonly status: "expired"; readonly reason: string } + /** Transient failure; the session is still on disk and may refresh later. */ + | { readonly status: "unavailable"; readonly reason: string }; + +export interface StepLoginTokenOptions { + readonly env?: Record; + readonly signal?: AbortSignal; + readonly fetch?: typeof fetch; +} + +/** + * Return a usable access token, refreshing it first when it is close to expiry. + * + * This is the entry point for anything that needs an identity: the web and + * desktop hosts, and account lookups. Callers get an explicit outcome instead + * of a token that may be stale, so "not signed in" is never mistaken for + * "signed in as nobody". + */ +export async function getStepLoginToken(options: StepLoginTokenOptions = {}): Promise { + const env = options.env ?? process.env; + const current = readStepLoginState(env); + if (!current) return { status: "missing" }; + // A refresh token past its own expiry cannot be renewed at all, so there is + // nothing to wait for: drop it now rather than let every caller rediscover it. + if (isRefreshTokenExpired(current, Date.now())) { + await discardLoginState(env, current.refreshToken); + return { status: "expired", reason: "the account session has expired" }; + } + if (!needsRefresh(current, Date.now())) return ready(current); + + try { + const result = await refreshAndCommit(env, options); + if (result.kind === "refreshed") return ready(result.state); + if (result.kind === "discarded") { + return { status: "expired", reason: "the account session is no longer signed in" }; + } + const latest = result.state ?? readStepLoginState(env); + if (!latest) return { status: "missing" }; + if (isRefreshTokenExpired(latest, Date.now())) + return { status: "expired", reason: "the account session has expired" }; + if (!needsRefresh(latest, Date.now())) return ready(latest); + // Another process is mid-refresh. The stored token may still be valid + // inside the refresh margin, and handing it back beats reporting no + // identity while a concurrent refresh is still in flight. + if (latest.accessExpiresAt > Math.floor(Date.now() / 1000)) return ready(latest); + return { status: "unavailable", reason: "another process is refreshing the session" }; + } catch (error) { + // A transient failure keeps the session. The access token may still be + // valid for a while inside the refresh margin, and handing that back is + // strictly better than reporting no identity at all. + const latest = readStepLoginState(env); + if (latest && latest.accessExpiresAt > Math.floor(Date.now() / 1000)) return ready(latest); + return { status: "unavailable", reason: describe(error) }; + } +} + +function describe(error: unknown): string { + return error instanceof Error ? error.message : String(error); +} + +function isRefreshTokenExpired(state: StepLoginState, nowMs: number): boolean { + return state.refreshExpiresAt !== undefined && state.refreshExpiresAt <= Math.floor(nowMs / 1000); +} + +function ready(state: StepLoginState): StepLoginTokenOutcome { + return { + status: "ready", + accessToken: state.accessToken, + deviceId: state.deviceId, + appId: state.appId, + profile: state.profile, + }; +} + +function needsRefresh(state: StepLoginState, nowMs: number): boolean { + return state.accessExpiresAt - STEP_LOGIN_REFRESH_MARGIN_SEC <= Math.floor(nowMs / 1000); +} + +/** + * Outcome of a refresh attempt. + * + * `discarded` means the session was positively dead and has been removed; + * `unchanged` means nothing was written, and `state` carries whatever is on disk + * now when there is something. + */ +type RefreshCommitResult = + | { readonly kind: "refreshed"; readonly state: StepLoginState } + | { readonly kind: "discarded" } + | { readonly kind: "unchanged"; readonly state?: StepLoginState }; + +/** + * Refresh the session, then commit the result under the cross-process lock. + * + * The network call deliberately runs *outside* the lock: holding it for up to + * `REFRESH_TIMEOUT_MS` would let a slow refresh block sign-out for that long, + * and would let a stolen lock (proper-lockfile's 10s stale window) corrupt a + * session once refresh tokens start rotating. The lock only guards the + * read-modify-write, which is where a lost update would actually matter. + * + * Committing re-reads the file, so a sign-out or a sign-in with another account + * that landed while the request was in flight discards this result instead of + * writing a session the user has already replaced. A session that turns out to + * be unredeemable is dropped the same way — only while it is still the session + * that failed. + */ +async function refreshAndCommit( + env: Record, + options: StepLoginTokenOptions, +): Promise { + const path = getStepLoginStatePath(env); + const before = readStepLoginState(env); + if (!before) return { kind: "unchanged" }; + + let refreshed: StepCliLoginState; + try { + refreshed = await refreshStepLoginState({ + authBaseUrl: resolveAuthBaseUrl(before.profile, env), + state: { + accessToken: before.accessToken, + refreshToken: before.refreshToken, + deviceId: before.deviceId, + appId: before.appId, + }, + ...(options.signal ? { signal: options.signal } : {}), + ...(options.fetch ? { fetch: options.fetch } : {}), + }); + } catch (error) { + if (error instanceof StepLoginRefreshError && error.failure === "unredeemable") { + return (await discardLoginState(env, before.refreshToken)) ? { kind: "discarded" } : { kind: "unchanged" }; + } + throw error; + } + + if (!existsSync(path)) return { kind: "unchanged" }; + let release: (() => void) | undefined; + try { + release = acquireSettingsLockSync(path); + } catch { + // Another process is committing; it re-reads the same way, so letting it + // win is safe and avoids a second write. + return { kind: "unchanged" }; + } + try { + const latest = readStepLoginState(env); + // Signed out, or signed in as someone else, while the request was running. + if (!latest || latest.refreshToken !== before.refreshToken) { + return { kind: "unchanged", ...(latest ? { state: latest } : {}) }; + } + const next: StepLoginState = { + ...latest, + accessToken: refreshed.accessToken, + refreshToken: refreshed.refreshToken, + accessExpiresAt: Math.floor(Date.now() / 1000) + (refreshed.expiresIn ?? DEFAULT_ACCESS_TOKEN_TTL_SEC), + }; + await saveStepLoginState(next, env); + return { kind: "refreshed", state: next }; + } finally { + release(); + } +} + +/** The developer center that issued the session is the one that refreshes it. */ +function resolveAuthBaseUrl(profile: StepLoginProfileId, env: Record): string { + const resolved = resolveStepLoginProfiles(env).find((candidate) => candidate.id === profile); + return resolved?.authBaseUrl ?? "https://platform.stepfun.com"; +} + +/** + * Read `exp` out of a JWT without verifying it. + * + * Only the expiry is used, and only to schedule a refresh; a wrong value costs + * one extra refresh or one failed request, never a wrong credential. + */ +function readJwtExpiresAt(token: string): number | undefined { + const parts = token.split("."); + if (parts.length < 2) return undefined; + try { + const payload = JSON.parse(Buffer.from(parts[1], "base64url").toString("utf8")) as Record; + return readTimestamp(payload.exp); + } catch { + return undefined; + } +} + +function readTimestamp(value: unknown): number | undefined { + return typeof value === "number" && Number.isFinite(value) && value > 0 ? Math.floor(value) : undefined; +} + +function readToken(value: unknown): string | undefined { + return typeof value === "string" && value.trim() ? value.trim() : undefined; +} diff --git a/packages/coding-agent/src/step/login-status.ts b/packages/coding-agent/src/step/login-status.ts index e4bb61a7..b1829244 100644 --- a/packages/coding-agent/src/step/login-status.ts +++ b/packages/coding-agent/src/step/login-status.ts @@ -1,5 +1,6 @@ import { readStoredCredential } from "../core/auth-storage.ts"; import { createStepProviderConfig } from "../features/step-provider/index.ts"; +import { readStepLoginState, STEP_LOGIN_REFRESH_MARGIN_SEC } from "./login-state.ts"; import { resolveStepLoginProfiles, type StepLoginProfileId } from "./onboarding.ts"; export type StepLoginMethod = "step_plan" | "step_plan_oversea" | "api_key" | null; @@ -13,6 +14,21 @@ export interface StepLoginStatus { readonly account?: string; readonly validity: StepCredentialValidity; readonly error?: string; + /** + * Account session state, when one was ever stored. + * + * Reported separately from `validity`, which only describes the API key: a + * user who signed in before the session existed has a working key and no + * session, and that is not a fault. + */ + readonly session?: StepLoginSessionStatus; +} + +/** Whether a usable account session is on disk right now. */ +export interface StepLoginSessionStatus { + readonly state: "ready" | "expiring" | "expired" | "missing"; + /** Unix seconds when the session can no longer be renewed. */ + readonly refreshExpiresAt?: number; } export interface StepLoginStatusOptions { @@ -34,7 +50,9 @@ export async function getStepLoginStatus(options: StepLoginStatusOptions): Promi const access = envKey ?? storedAccess; const loginMethod: StepLoginMethod = envKey || storedApiKey ? "api_key" : access ? planLoginMethod(storedProfileId) : null; - if (!access || !loginMethod) return { loggedIn: false, loginMethod: null, validity: "missing" }; + if (!access || !loginMethod) { + return { loggedIn: false, loginMethod: null, validity: "missing", session: describeStepLoginSession(env) }; + } const accountValue = envKey ? undefined : (stored?.uid ?? stored?.profile); const account = typeof accountValue === "string" && accountValue.trim() ? accountValue.trim() : undefined; @@ -48,6 +66,7 @@ export async function getStepLoginStatus(options: StepLoginStatusOptions): Promi loginMethod, ...(profile ? { profile: profile.id } : {}), ...(account ? { account } : {}), + session: describeStepLoginSession(env), } as const; const baseUrl = options.apiBaseUrl ?? @@ -81,3 +100,25 @@ export async function getStepLoginStatus(options: StepLoginStatusOptions): Promi function planLoginMethod(profileId: string): StepLoginMethod { return profileId === "step_plan_oversea" ? "step_plan_oversea" : "step_plan"; } + +/** + * Describe the account session without touching the network. + * + * Reported next to — never instead of — the API key's validity, so a user who + * only ever had a key still sees a clean status. + */ +function describeStepLoginSession(env: Record): StepLoginSessionStatus { + const state = readStepLoginState(env); + if (!state) return { state: "missing" }; + const nowSec = Math.floor(Date.now() / 1000); + if (state.refreshExpiresAt !== undefined && state.refreshExpiresAt <= nowSec) { + return { state: "expired", refreshExpiresAt: state.refreshExpiresAt }; + } + // Mirrors the refresh margin so the reported state matches what the next use + // will actually do. + if (state.accessExpiresAt - STEP_LOGIN_REFRESH_MARGIN_SEC <= nowSec) return { state: "expiring" }; + return { + state: "ready", + ...(state.refreshExpiresAt === undefined ? {} : { refreshExpiresAt: state.refreshExpiresAt }), + }; +} diff --git a/packages/coding-agent/src/step/onboarding.ts b/packages/coding-agent/src/step/onboarding.ts index f3c4b507..082b79fa 100644 --- a/packages/coding-agent/src/step/onboarding.ts +++ b/packages/coding-agent/src/step/onboarding.ts @@ -1,5 +1,7 @@ /** Shared Step login profiles and the side-effect-free onboarding reducer. */ +import type { StepCliLoginState } from "@step-harness/providers/step-provider"; + export const STEP_LOGIN_PROFILE_IDS = ["step_plan", "step_plan_oversea", "platform_cn", "platform_oversea"] as const; export type StepLoginProfileId = (typeof STEP_LOGIN_PROFILE_IDS)[number]; @@ -131,6 +133,8 @@ export type StepLoginEvent = readonly type: "credential"; readonly apiKey: string; readonly uid?: string; + /** Account session, when the backend delivered one. */ + readonly login?: StepCliLoginState; } | { readonly type: "type"; readonly text: string } | { readonly type: "backspace" } diff --git a/packages/coding-agent/test/step-cli-login.test.ts b/packages/coding-agent/test/step-cli-login.test.ts index 58b6421b..32d1b293 100644 --- a/packages/coding-agent/test/step-cli-login.test.ts +++ b/packages/coding-agent/test/step-cli-login.test.ts @@ -265,6 +265,95 @@ describe("Step CLI login poll", () => { ).rejects.toMatchObject({ kind: "protocol" }); }); + it("carries the account session beside the api_key when the backend sends one", async () => { + const login = { + access_token: "access-jwt", + refresh_token: "refresh-jwt", + device_id: "device-1", + app_id: "10300", + expires_in: 1800, + }; + await expect( + pollStepCliLogin({ + authBaseUrl: AUTH_BASE_URL, + pollToken: "token", + flowId: "flow-1", + fetch: async () => json({ status: 0, state: "ready", credential: { api_key: "sk-step", login } }), + }), + ).resolves.toEqual({ + state: "ready", + apiKey: "sk-step", + login: { + accessToken: "access-jwt", + refreshToken: "refresh-jwt", + deviceId: "device-1", + appId: "10300", + expiresIn: 1800, + }, + }); + }); + + it("treats a ready response without a login field as api-key-only", async () => { + await expect( + pollStepCliLogin({ + authBaseUrl: AUTH_BASE_URL, + pollToken: "token", + flowId: "flow-1", + fetch: async () => json({ status: 0, state: "ready", credential: { api_key: "sk-step" } }), + }), + ).resolves.toEqual({ state: "ready", apiKey: "sk-step" }); + }); + + it("rejects a login field it cannot refresh rather than storing it half-formed", async () => { + for (const login of [ + { access_token: "a", refresh_token: "b", device_id: "d" }, + { access_token: "a", refresh_token: "b", app_id: "10300" }, + { access_token: "a", device_id: "d", app_id: "10300" }, + { refresh_token: "b", device_id: "d", app_id: "10300" }, + { access_token: "a", refresh_token: "b", device_id: "", app_id: "10300" }, + "not-an-object", + ]) { + await expect( + pollStepCliLogin({ + authBaseUrl: AUTH_BASE_URL, + pollToken: "token", + flowId: "flow-1", + fetch: async () => json({ status: 0, state: "ready", credential: { api_key: "sk-step", login } }), + }), + ).rejects.toMatchObject({ kind: "protocol" }); + } + }); + + it("treats an unusable expires_in as absent rather than as a broken session", async () => { + await expect( + pollStepCliLogin({ + authBaseUrl: AUTH_BASE_URL, + pollToken: "token", + flowId: "flow-1", + fetch: async () => + json({ + status: 0, + state: "ready", + credential: { + api_key: "sk-step", + login: { + access_token: "a", + refresh_token: "b", + device_id: "d", + app_id: "10300", + expires_in: "soon", + }, + }, + }), + }), + ).resolves.toEqual({ + state: "ready", + apiKey: "sk-step", + // The CLI falls back to the token's own expiry, so this stays advisory. + login: { accessToken: "a", refreshToken: "b", deviceId: "d", appId: "10300" }, + }); + }); + it("classifies retryable and fatal failures", () => { const make = (kind: "http" | "protocol" | "transport", httpStatus?: number) => new StepCliLoginRequestError(kind, "boom", httpStatus === undefined ? {} : { httpStatus }); diff --git a/packages/coding-agent/test/step-login-state.test.ts b/packages/coding-agent/test/step-login-state.test.ts new file mode 100644 index 00000000..77727444 --- /dev/null +++ b/packages/coding-agent/test/step-login-state.test.ts @@ -0,0 +1,358 @@ +import { mkdtempSync, rmSync, statSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { acquireSettingsLockSync } from "../src/core/settings-manager.ts"; +import type { StepCliLoginState } from "../src/features/step-provider/index.ts"; +import { + buildStepLoginState, + clearStepLoginState, + getStepLoginStatePath, + getStepLoginToken, + readStepLoginState, + type StepLoginState, + saveStepLoginState, +} from "../src/step/login-state.ts"; + +/** Minimal unsigned JWTs; only `exp` is read, and only to schedule a refresh. */ +let tokenCounter = 0; +function jwt(payload: Record): string { + const encode = (value: Record) => Buffer.from(JSON.stringify(value)).toString("base64url"); + // A per-call nonce keeps two tokens minted in the same second distinct, which + // is what "has this session been replaced?" relies on. + return `header.${encode({ ...payload, jti: `t${tokenCounter++}` })}.signature`; +} + +function accessToken(expiresInSec: number): string { + return jwt({ exp: Math.floor(Date.now() / 1000) + expiresInSec }); +} + +function refreshToken(expiresInSec = 30 * 24 * 60 * 60): string { + return jwt({ exp: Math.floor(Date.now() / 1000) + expiresInSec }); +} + +function loginState(overrides: Partial = {}): StepCliLoginState { + return { + accessToken: accessToken(1800), + refreshToken: refreshToken(), + deviceId: "device-1", + appId: "10300", + expiresIn: 1800, + ...overrides, + }; +} + +function stored(overrides: Partial = {}): StepLoginState { + return { + version: 1, + profile: "step_plan", + appId: "10300", + deviceId: "device-1", + accessToken: accessToken(1800), + refreshToken: refreshToken(), + accessExpiresAt: Math.floor(Date.now() / 1000) + 1800, + refreshExpiresAt: Math.floor(Date.now() / 1000) + 30 * 24 * 60 * 60, + ...overrides, + }; +} + +let dir: string; +let env: Record; + +beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), "step-login-state-")); + env = { STEPCODE_LOGIN_STATE_PATH: join(dir, "step-login.json") }; +}); + +afterEach(() => { + rmSync(dir, { recursive: true, force: true }); + vi.unstubAllGlobals(); +}); + +describe("step login state storage", () => { + it("reports missing when nothing was ever signed in", () => { + expect(readStepLoginState(env)).toBeUndefined(); + }); + + it("round-trips a session and keeps the file owner-only", async () => { + await saveStepLoginState(stored(), env); + expect(readStepLoginState(env)).toMatchObject({ profile: "step_plan", appId: "10300", deviceId: "device-1" }); + // A refresh token lives here, so the mode is part of the contract. + expect(statSync(getStepLoginStatePath(env)).mode & 0o777).toBe(0o600); + }); + + it("treats an unreadable or wrongly shaped file as no session", async () => { + await saveStepLoginState(stored(), env); + writeFileSync(getStepLoginStatePath(env), "{ not json", "utf8"); + expect(readStepLoginState(env)).toBeUndefined(); + + await saveStepLoginState(stored(), env); + writeFileSync(getStepLoginStatePath(env), JSON.stringify({ version: 1, appId: "10300" }), "utf8"); + expect(readStepLoginState(env)).toBeUndefined(); + }); + + it("refuses a file written by a newer version", async () => { + await saveStepLoginState(stored(), env); + writeFileSync( + getStepLoginStatePath(env), + JSON.stringify({ ...stored(), version: 99, somethingNew: true }), + "utf8", + ); + // Reading a future shape as the current one would either pass by luck or + // silently drop the session. + expect(readStepLoginState(env)).toBeUndefined(); + }); + + it("keeps the api key out of this file's business: clearing only removes the session", async () => { + await saveStepLoginState(stored(), env); + await clearStepLoginState(env); + expect(readStepLoginState(env)).toBeUndefined(); + }); + + it("builds a stored session from what the login flow redeemed", () => { + const now = Date.now(); + const state = buildStepLoginState(loginState(), "step_plan_oversea", now); + expect(state.profile).toBe("step_plan_oversea"); + expect(state.accessExpiresAt).toBe(Math.floor(now / 1000) + 1800); + expect(state.refreshExpiresAt).toBeGreaterThan(Math.floor(now / 1000)); + }); + + it("falls back to a default lifetime when the server reports none", () => { + const state = buildStepLoginState(loginState({ expiresIn: undefined }), "step_plan"); + expect(state.accessExpiresAt).toBeGreaterThan(Math.floor(Date.now() / 1000)); + }); +}); + +describe("getStepLoginToken", () => { + it("returns missing without touching the network", async () => { + const fetchSpy = vi.fn(); + vi.stubGlobal("fetch", fetchSpy); + expect(await getStepLoginToken({ env })).toEqual({ status: "missing" }); + expect(fetchSpy).not.toHaveBeenCalled(); + }); + + it("returns a fresh token without refreshing", async () => { + await saveStepLoginState(stored(), env); + const fetchSpy = vi.fn(); + vi.stubGlobal("fetch", fetchSpy); + const outcome = await getStepLoginToken({ env }); + expect(outcome.status).toBe("ready"); + expect(fetchSpy).not.toHaveBeenCalled(); + }); + + it("refreshes a token that is close to expiry and persists the result", async () => { + const state = stored({ + accessExpiresAt: Math.floor(Date.now() / 1000) + 60, + accessToken: accessToken(60), + }); + await saveStepLoginState(state, env); + const fetchSpy = vi.fn(async (url: string | URL, init?: RequestInit) => { + expect(String(url)).toContain("/passport/proto.api.passport.v1.PassportService/RefreshToken"); + const cookie = new Headers(init?.headers).get("cookie") ?? ""; + expect(cookie).toContain("Oasis-Token="); + // The cookie carries both halves, joined the way passport joins them. + expect(cookie.split("...")).toHaveLength(2); + expect(new Headers(init?.headers).get("Oasis-appID")).toBe("10300"); + expect(new Headers(init?.headers).get("Oasis-Webid")).toBe("device-1"); + return new Response( + JSON.stringify({ + accessToken: { raw: accessToken(1800), duration: 1800, mode: 2 }, + refreshToken: { raw: refreshToken() }, + }), + { status: 200, headers: { "content-type": "application/json" } }, + ); + }); + vi.stubGlobal("fetch", fetchSpy); + + const outcome = await getStepLoginToken({ env }); + expect(outcome.status).toBe("ready"); + if (outcome.status !== "ready") return; + expect(outcome.accessToken).toBe(readStepLoginState(env)?.accessToken); + expect(readStepLoginState(env)?.accessExpiresAt).toBeGreaterThan(state.accessExpiresAt); + expect(fetchSpy).toHaveBeenCalledTimes(1); + }); + + it("drops the session when the refresh comes back anonymous", async () => { + await saveStepLoginState( + stored({ accessExpiresAt: Math.floor(Date.now() / 1000) + 60, accessToken: accessToken(60) }), + env, + ); + vi.stubGlobal( + "fetch", + vi.fn( + async () => + new Response( + JSON.stringify({ + accessToken: { raw: accessToken(1800), duration: 1800, mode: 1 }, + refreshToken: { raw: refreshToken() }, + }), + { status: 200, headers: { "content-type": "application/json" } }, + ), + ), + ); + + // Passport answers an unredeemable refresh with an anonymous session + // instead of an error, so the mode is the only signal there is. + expect(await getStepLoginToken({ env })).toEqual({ + status: "expired", + reason: expect.stringContaining("no longer signed in"), + }); + expect(readStepLoginState(env)).toBeUndefined(); + }); + + it("keeps the session when the refresh fails transiently", async () => { + // Already past expiry, so there is nothing valid to hand back. + await saveStepLoginState( + stored({ accessExpiresAt: Math.floor(Date.now() / 1000) - 1, accessToken: accessToken(-1) }), + env, + ); + vi.stubGlobal( + "fetch", + vi.fn(async () => new Response("", { status: 503 })), + ); + + expect(await getStepLoginToken({ env })).toEqual({ + status: "unavailable", + reason: expect.stringContaining("503"), + }); + expect(readStepLoginState(env)).toBeDefined(); + }); + + it("hands back the still-valid token when a transient refresh fails", async () => { + // Inside the refresh margin the access token is still good for minutes, + // so reporting no identity at all would be a needless outage. + await saveStepLoginState( + stored({ accessExpiresAt: Math.floor(Date.now() / 1000) + 120, accessToken: accessToken(120) }), + env, + ); + vi.stubGlobal( + "fetch", + vi.fn(async () => new Response("", { status: 503 })), + ); + + const outcome = await getStepLoginToken({ env }); + expect(outcome.status).toBe("ready"); + expect(readStepLoginState(env)).toBeDefined(); + }); + + it("does not resurrect a session that was signed out while the refresh was running", async () => { + const state = stored({ + accessExpiresAt: Math.floor(Date.now() / 1000) + 60, + accessToken: accessToken(60), + }); + await saveStepLoginState(state, env); + vi.stubGlobal( + "fetch", + vi.fn(async () => { + // The user signs out from another process mid-refresh. + await clearStepLoginState(env); + return new Response( + JSON.stringify({ + accessToken: { raw: accessToken(1800), duration: 1800, mode: 2 }, + refreshToken: { raw: refreshToken() }, + }), + { status: 200, headers: { "content-type": "application/json" } }, + ); + }), + ); + + expect(await getStepLoginToken({ env })).toEqual({ status: "missing" }); + expect(readStepLoginState(env)).toBeUndefined(); + }); + + it("keeps a session that was replaced while an unredeemable refresh was running", async () => { + const dead = stored({ + accessExpiresAt: Math.floor(Date.now() / 1000) + 60, + accessToken: accessToken(60), + }); + await saveStepLoginState(dead, env); + vi.stubGlobal( + "fetch", + vi.fn(async () => { + // Another account signs in from another process mid-refresh. + await saveStepLoginState( + stored({ accessToken: accessToken(1800), refreshToken: refreshToken(), profile: "step_plan_oversea" }), + env, + ); + return new Response( + JSON.stringify({ + accessToken: { raw: "anon", duration: 1800, mode: 1 }, + refreshToken: { raw: refreshToken() }, + }), + { status: 200, headers: { "content-type": "application/json" } }, + ); + }), + ); + + // The dead session is dropped, but the newer one must survive: deleting + // unconditionally would silently sign the new account back out. + expect(await getStepLoginToken({ env })).toMatchObject({ status: "ready", profile: "step_plan_oversea" }); + expect(readStepLoginState(env)?.profile).toBe("step_plan_oversea"); + }); + + it("hands back the still-valid token when another process holds the commit lock", async () => { + const state = stored({ + accessExpiresAt: Math.floor(Date.now() / 1000) + 120, + accessToken: accessToken(120), + }); + await saveStepLoginState(state, env); + // Simulate losing the lock: the file stays, so the local token is still + // usable and reporting "unavailable" would be a needless outage. + const path = getStepLoginStatePath(env); + const release = acquireSettingsLockSync(path); + try { + const outcome = await getStepLoginToken({ + env, + fetch: (async () => new Response("", { status: 503 })) as typeof fetch, + }); + expect(outcome.status).toBe("ready"); + } finally { + release(); + } + }); + + it("does not overwrite a session that changed while the refresh was running", async () => { + await saveStepLoginState( + stored({ accessExpiresAt: Math.floor(Date.now() / 1000) + 60, accessToken: accessToken(60) }), + env, + ); + vi.stubGlobal( + "fetch", + vi.fn(async () => { + // Another account signs in from another process mid-refresh. + await saveStepLoginState( + stored({ accessToken: accessToken(1800), refreshToken: refreshToken(), profile: "step_plan_oversea" }), + env, + ); + return new Response( + JSON.stringify({ + accessToken: { raw: accessToken(1800), duration: 1800, mode: 2 }, + refreshToken: { raw: refreshToken() }, + }), + { status: 200, headers: { "content-type": "application/json" } }, + ); + }), + ); + + expect((await getStepLoginToken({ env })).status).toBe("ready"); + // The newer session survives; the stale refresh result is discarded. + expect(readStepLoginState(env)?.profile).toBe("step_plan_oversea"); + }); + + it("asks for a new sign-in once the refresh token itself has expired", async () => { + await saveStepLoginState( + stored({ + accessExpiresAt: Math.floor(Date.now() / 1000) + 60, + accessToken: accessToken(60), + refreshExpiresAt: Math.floor(Date.now() / 1000) - 1, + }), + env, + ); + const fetchSpy = vi.fn(); + vi.stubGlobal("fetch", fetchSpy); + + expect((await getStepLoginToken({ env })).status).toBe("expired"); + expect(fetchSpy).not.toHaveBeenCalled(); + expect(readStepLoginState(env)).toBeUndefined(); + }); +}); diff --git a/packages/coding-agent/test/step-login-status.test.ts b/packages/coding-agent/test/step-login-status.test.ts index 2a44e8bb..c6e4d006 100644 --- a/packages/coding-agent/test/step-login-status.test.ts +++ b/packages/coding-agent/test/step-login-status.test.ts @@ -3,6 +3,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, test } from "vitest"; import { AuthStorage } from "../src/core/auth-storage.ts"; +import { type StepLoginState, saveStepLoginState } from "../src/step/login-state.ts"; import { getStepLoginStatus } from "../src/step/login-status.ts"; const dirs: string[] = []; @@ -10,18 +11,46 @@ afterEach(async () => { await Promise.all(dirs.splice(0).map((dir) => rm(dir, { recursive: true, force: true }))); }); +/** Isolate the account session as well, or a real `~/.stepcode` would leak in. */ +async function isolatedEnv(): Promise> { + const dir = await mkdtemp(join(tmpdir(), "step-status-")); + dirs.push(dir); + return { STEPCODE_LOGIN_STATE_PATH: join(dir, "step-login.json") }; +} + function response(status: number): Response { return new Response(null, { status }); } +function session(overrides: Partial = {}): StepLoginState { + const now = Math.floor(Date.now() / 1000); + return { + version: 1, + profile: "step_plan", + appId: "10300", + deviceId: "device-1", + accessToken: "access", + refreshToken: "refresh", + accessExpiresAt: now + 1800, + refreshExpiresAt: now + 30 * 24 * 60 * 60, + ...overrides, + }; +} + describe("Step login status", () => { test("reports missing credentials", async () => { const dir = await mkdtemp(join(tmpdir(), "step-status-")); dirs.push(dir); - expect(await getStepLoginStatus({ authPath: join(dir, "auth.json"), env: {} })).toEqual({ + expect( + await getStepLoginStatus({ + authPath: join(dir, "auth.json"), + env: { STEPCODE_LOGIN_STATE_PATH: join(dir, "step-login.json") }, + }), + ).toEqual({ loggedIn: false, loginMethod: null, validity: "missing", + session: { state: "missing" }, }); }); @@ -80,7 +109,7 @@ describe("Step login status", () => { let requestedUrl = ""; const status = await getStepLoginStatus({ authPath, - env: {}, + env: await isolatedEnv(), fetch: async (input) => { requestedUrl = String(input); return response(200); @@ -92,6 +121,7 @@ describe("Step login status", () => { profile: "step_plan_oversea", account: "user-456", validity: "valid", + session: { state: "missing" }, }); expect(requestedUrl).toBe("https://api.stepfun.ai/step_plan/v1/models"); }); @@ -110,6 +140,7 @@ describe("Step login status", () => { let requestedUrl = ""; const status = await getStepLoginStatus({ authPath, + env: await isolatedEnv(), fetch: async (input) => { requestedUrl = String(input); return response(200); @@ -118,4 +149,45 @@ describe("Step login status", () => { expect(status).toMatchObject({ loggedIn: true, loginMethod: "api_key", validity: "valid" }); expect(requestedUrl).toBe("https://api.stepfun.ai/v1/models"); }); + + test("reports the account session beside the key, never instead of it", async () => { + const dir = await mkdtemp(join(tmpdir(), "step-status-")); + dirs.push(dir); + const authPath = join(dir, "auth.json"); + const env = { STEPCODE_LOGIN_STATE_PATH: join(dir, "step-login.json") }; + await AuthStorage.create(authPath).modify("step", async () => ({ + type: "oauth", + access: "secret-token", + refresh: "step-static-credential", + expires: Number.MAX_SAFE_INTEGER, + profile: "step_plan", + uid: "user-123", + })); + + // A user who signed in before account sessions existed: the key works and + // there is simply no session. That is not a fault worth warning about. + expect(await getStepLoginStatus({ authPath, env, fetch: async () => response(200) })).toMatchObject({ + validity: "valid", + session: { state: "missing" }, + }); + + const now = Math.floor(Date.now() / 1000); + for (const [state, expected] of [ + ["ready", { state: "ready", refreshExpiresAt: now + 30 * 24 * 60 * 60 }], + ["expiring", { state: "expiring" }], + ["expired", { state: "expired", refreshExpiresAt: now - 1 }], + ] as const) { + await saveStepLoginState( + session({ + accessExpiresAt: state === "expiring" ? now + 60 : now + 1800, + refreshExpiresAt: state === "expired" ? now - 1 : now + 30 * 24 * 60 * 60, + }), + env, + ); + expect(await getStepLoginStatus({ authPath, env, fetch: async () => response(200) })).toMatchObject({ + validity: "valid", + session: expected, + }); + } + }); }); diff --git a/packages/coding-agent/test/step-plan-login.test.ts b/packages/coding-agent/test/step-plan-login.test.ts index 243b56be..28ceca83 100644 --- a/packages/coding-agent/test/step-plan-login.test.ts +++ b/packages/coding-agent/test/step-plan-login.test.ts @@ -18,14 +18,12 @@ import * as browser from "../src/utils/open-browser.ts"; vi.mock("../src/features/step-provider/index.ts", async (original) => ({ ...(await original()), - loginStepOAuth: vi.fn(async () => ({ - access: "plan-key", - refresh: "static", - expires: Number.MAX_SAFE_INTEGER, - uid: "123", - })), + loginStepOAuth: vi.fn(async () => oauthResult.current), })); +/** What the mocked sign-in returns; swapped per test. */ +const oauthResult = vi.hoisted(() => ({ current: {} as Record })); + function snapshot(family: "none" | "coding" | "token" = "none"): PlanUsageSnapshot { return { schema_version: 1, @@ -74,6 +72,12 @@ describe("Step Plan login availability", () => { authPath = join(root, "auth.json"); vi.mocked(loginStepOAuth).mockClear(); vi.spyOn(browser, "openBrowser").mockImplementation(() => {}); + oauthResult.current = { + access: "plan-key", + refresh: "static", + expires: Number.MAX_SAFE_INTEGER, + uid: "123", + }; }); afterEach(async () => { vi.restoreAllMocks(); @@ -82,12 +86,16 @@ describe("Step Plan login availability", () => { function controller(fetcher: typeof fetch) { return new PlanUsageController(() => resolvePlanUsageAccount(authPath, {}), fetcher, Date.now, 100); } - function start(planUsage: PlanUsageController, requiredPlan?: "step_plan" | "step_plan_oversea") { + function start( + planUsage: PlanUsageController, + requiredPlan?: "step_plan" | "step_plan_oversea", + env: Record = {}, + ) { let view!: StepOnboardingView; const stop = vi.fn(); const login = runStepLogin({ authPath, - env: {}, + env, noBrowser: true, planUsage, requiredPlan, @@ -308,4 +316,66 @@ describe("Step Plan login availability", () => { await login; } }); + + it("stores the account session beside the api key after a browser sign-in", async () => { + // Regression: the session rode along on the credential event but was + // never handed to `persist`, so nothing reached the disk and the whole + // account-session feature was silently absent. + const loginStatePath = join(root, "step-login.json"); + oauthResult.current = { + access: "plan-key", + refresh: "static", + expires: Number.MAX_SAFE_INTEGER, + uid: "123", + login: { + accessToken: "access-jwt", + refreshToken: "refresh-jwt", + deviceId: "device-1", + appId: "10300", + expiresIn: 1800, + }, + }; + const { login, view } = start( + controller(vi.fn(async () => Response.json(snapshot("coding")))), + undefined, + // Isolate the account session; an empty env resolves to the real home. + { STEPCODE_LOGIN_STATE_PATH: loginStatePath }, + ); + try { + view.handleInput("1"); + await expect(login).resolves.toMatchObject({ kind: "completed" }); + const stored = JSON.parse(await readFile(loginStatePath, "utf8")) as Record; + expect(stored).toMatchObject({ + profile: "step_plan", + appId: "10300", + deviceId: "device-1", + accessToken: "access-jwt", + refreshToken: "refresh-jwt", + }); + expect(stored.accessExpiresAt).toBeGreaterThan(Math.floor(Date.now() / 1000)); + // The api key is unaffected by the session being stored. + expect(readStepLoginCredential(authPath)).toMatchObject({ type: "oauth", access: "plan-key" }); + } finally { + view.handleInput("\x03"); + await login; + } + }); + + it("completes the sign-in when the backend sends no account session", async () => { + const loginStatePath = join(root, "step-login.json"); + const { login, view } = start( + controller(vi.fn(async () => Response.json(snapshot("coding")))), + undefined, + { STEPCODE_LOGIN_STATE_PATH: loginStatePath }, + ); + try { + view.handleInput("1"); + await expect(login).resolves.toMatchObject({ kind: "completed" }); + await expect(readFile(loginStatePath, "utf8")).rejects.toThrow(); + expect(readStepLoginCredential(authPath)).toMatchObject({ type: "oauth", access: "plan-key" }); + } finally { + view.handleInput("\x03"); + await login; + } + }); }); diff --git a/packages/providers/src/step-provider/index.ts b/packages/providers/src/step-provider/index.ts index eeb3b0e4..520f3314 100644 --- a/packages/providers/src/step-provider/index.ts +++ b/packages/providers/src/step-provider/index.ts @@ -42,17 +42,22 @@ export type { StepCliLoginFailureReason, StepCliLoginInit, StepCliLoginPoll, + StepCliLoginState, } from "./login-client.ts"; export { defaultStepCliClientInfo, initStepCliLogin, isRetryableStepCliLoginError, + parseStepCliLoginState, + parseStepLoginState, pollStepCliLogin, STEP_CLI_LOGIN_PATH_PREFIX, StepCliLoginRequestError, } from "./login-client.ts"; export type { StepCliLoginReady, WaitForStepCliLoginInput } from "./login-polling.ts"; export { MIN_STEP_POLL_INTERVAL_MS, waitForStepCliLogin } from "./login-polling.ts"; +export type { RefreshStepLoginStateInput } from "./login-refresh.ts"; +export { refreshStepLoginState, StepLoginRefreshError } from "./login-refresh.ts"; /** Provider id used by the default Step extension. */ export const STEP_PROVIDER_ID = "step"; @@ -310,6 +315,9 @@ export async function loginStepOAuth( refresh: STEP_STATIC_REFRESH_TOKEN, expires: Number.MAX_SAFE_INTEGER, ...(ready.uid ? { uid: ready.uid } : {}), + // Rides along as an extra field: the API key stays the credential, + // and hosts that need an identity read this beside it. + ...(ready.login ? { login: ready.login } : {}), }; }, ); diff --git a/packages/providers/src/step-provider/login-client.ts b/packages/providers/src/step-provider/login-client.ts index a31f94b3..943a2a37 100644 --- a/packages/providers/src/step-provider/login-client.ts +++ b/packages/providers/src/step-provider/login-client.ts @@ -64,9 +64,26 @@ export type StepCliLoginPoll = readonly state: "ready"; readonly apiKey: string; readonly uid?: string; + readonly login?: StepCliLoginState; } | { readonly state: "failed"; readonly reason: StepCliLoginFailureReason }; +/** + * Account session delivered beside the API key. + * + * The API key remains the only credential for model requests. This is what + * lets the CLI act as the signed-in user (account lookups) and refresh that + * state on its own, so hosts that need an identity can use it later. + */ +export interface StepCliLoginState { + readonly accessToken: string; + readonly refreshToken: string; + readonly deviceId: string; + readonly appId: string; + /** Remaining access-token lifetime in seconds, when the server reports it. */ + readonly expiresIn?: number; +} + export type StepCliLoginErrorKind = /** Non-2xx HTTP response, or a non-zero `status` field in a 2xx envelope. */ | "http" @@ -201,10 +218,87 @@ export async function pollStepCliLogin(input: PollStepCliLoginInput): Promise, "api_key"); if (!apiKey) throw protocolError("poll", "ready response did not contain an api_key"); const uid = readBoundedUid(readString(payload, "uid")); + const login = readLoginState(credential as Record); return { state: "ready", apiKey, ...(uid ? { uid } : {}), + ...(login ? { login } : {}), + }; +} + +/** + * Read the optional account session. + * + * Absent on backends that predate it, which is the api-key-only behaviour the + * CLI has always had, and the only case that is tolerated: the backend ships + * first and CLI users upgrade selectively, so a missing field must not fail a + * sign-in. + * + * Present but unusable is a contract violation and fails the poll, exactly like + * a missing api_key. Tolerating it would mean a renamed or dropped field + * silently costs every CLI its account session with nothing to show for it. + */ +function readLoginState(credential: Record): StepCliLoginState | undefined { + const raw = credential.login; + if (raw === undefined || raw === null) return undefined; + const parsed = parseStepLoginState(raw); + if (!parsed) throw protocolError("poll", "login field is not a usable account session"); + return parsed; +} + +/** + * Validate a login state that arrived through an untyped channel. + * + * Returns `undefined` instead of throwing so callers that only need "is this + * usable" — the product layer, which receives the session as an `unknown` extra + * field on the OAuth credentials — do not have to re-implement the contract. + * + * This validates the **wire** shape (`access_token`). The product layer needs + * the **parsed** shape (`accessToken`); see `parseStepCliLoginState`. + */ +export function parseStepLoginState(value: unknown): StepCliLoginState | undefined { + if (!value || typeof value !== "object" || Array.isArray(value)) return undefined; + const record = value as Record; + const accessToken = readString(record, "access_token"); + const refreshToken = readString(record, "refresh_token"); + const deviceId = readString(record, "device_id"); + const appId = readString(record, "app_id"); + if (!accessToken || !refreshToken || !deviceId || !appId) return undefined; + const expiresIn = readNumber(record, "expires_in"); + return { + accessToken, + refreshToken, + deviceId, + appId, + ...(expiresIn !== undefined && expiresIn > 0 ? { expiresIn } : {}), + }; +} + +/** + * Validate a login state that has already been through the wire parser. + * + * `loginStepOAuth` returns the session as an extra field on `OAuthCredentials`, + * whose index signature types it as `unknown`, so the product layer has to + * narrow it again. This checks the **parsed** (camelCase) shape, not the wire + * one: a session that lost a field somewhere in between would otherwise be + * stored and then silently fail to refresh. + */ +export function parseStepCliLoginState(value: unknown): StepCliLoginState | undefined { + if (!value || typeof value !== "object" || Array.isArray(value)) return undefined; + const record = value as Record; + const accessToken = readString(record, "accessToken"); + const refreshToken = readString(record, "refreshToken"); + const deviceId = readString(record, "deviceId"); + const appId = readString(record, "appId"); + if (!accessToken || !refreshToken || !deviceId || !appId) return undefined; + const expiresIn = readNumber(record, "expiresIn"); + return { + accessToken, + refreshToken, + deviceId, + appId, + ...(expiresIn !== undefined && expiresIn > 0 ? { expiresIn } : {}), }; } @@ -317,7 +411,8 @@ async function readBoundedText(response: Response, phase: "init" | "poll"): Prom return Buffer.concat(chunks).toString("utf8"); } -function parseAuthBaseUrl(value: string): URL { +/** Exported so the login-state refresh can reuse the same endpoint validation. */ +export function parseAuthBaseUrl(value: string): URL { let url: URL; try { url = new URL(value); diff --git a/packages/providers/src/step-provider/login-refresh.ts b/packages/providers/src/step-provider/login-refresh.ts new file mode 100644 index 00000000..546406f8 --- /dev/null +++ b/packages/providers/src/step-provider/login-refresh.ts @@ -0,0 +1,165 @@ +/** + * Refresh the account session that `step login` delivered beside the API key. + * + * Passport's `RefreshToken` takes no request body: the refresh credential + * travels in the `Oasis-Token` cookie, which is the access and refresh JWTs + * joined by `...`. A CLI has no cookie jar, so it rebuilds that cookie value + * from the two tokens it stored and reads the new pair from the response body. + * + * Two behaviours are load-bearing and easy to get wrong: + * - A refresh that cannot be redeemed does not fail. Passport answers 200 + * with `mode: ANONYMOUS` and a different account id, so callers must check + * the mode instead of trusting the status code. + * - The refresh token is not rotated. Persisting the returned one is still + * correct and keeps working if that ever changes. + */ + +import { parseAuthBaseUrl, type StepCliLoginState } from "./login-client.ts"; + +/** Route prefix the gateway serves passport under, for both regions. */ +const PASSPORT_PATH_PREFIX = "/passport"; + +/** Connect-RPC procedure the developer center's login page calls. */ +const PASSPORT_REFRESH_PATH = `${PASSPORT_PATH_PREFIX}/proto.api.passport.v1.PassportService/RefreshToken`; + +/** Access-token lifetime the server reports when it omits `expires_in`. */ +const DEFAULT_ACCESS_TOKEN_TTL_SEC = 30 * 60; + +/** A stuck refresh must not hold the credential lock or block a request. */ +const REFRESH_TIMEOUT_MS = 15_000; + +/** Passport's `Mode` enum; only a signed-in session is usable. */ +const MODE_SIGN_IN = 2; + +export type StepLoginRefreshFailure = + /** The session can no longer be redeemed; only a new sign-in recovers. */ + | "unredeemable" + /** Transport or server-side problem; the session may still refresh later. */ + | "transient"; + +/** + * Thrown when a refresh cannot be completed. + * + * `unredeemable` is reserved for the one answer that positively proves the + * session is dead: an anonymous mode. Everything else — any HTTP failure, an + * unexpected body, a missing refresh token — is `transient`, because deleting a + * long-lived credential on a guess costs the user a full re-login while keeping + * it costs at most one retried request. + */ +export class StepLoginRefreshError extends Error { + readonly failure: StepLoginRefreshFailure; + readonly httpStatus?: number; + + constructor( + failure: StepLoginRefreshFailure, + message: string, + options: { httpStatus?: number; cause?: unknown } = {}, + ) { + super(message, options.cause === undefined ? undefined : { cause: options.cause }); + this.name = "StepLoginRefreshError"; + this.failure = failure; + if (options.httpStatus !== undefined) this.httpStatus = options.httpStatus; + } +} + +export interface RefreshStepLoginStateInput { + readonly authBaseUrl: string; + readonly state: StepCliLoginState; + readonly signal?: AbortSignal; + readonly fetch?: typeof fetch; +} + +/** + * Exchange the stored session for a fresh one. + * + * Throws when the session can no longer be redeemed, which is the caller's + * signal to drop the local login state and ask the user to sign in again. + */ +export async function refreshStepLoginState(input: RefreshStepLoginStateInput): Promise { + const base = parseAuthBaseUrl(input.authBaseUrl); + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), REFRESH_TIMEOUT_MS); + const onAbort = () => controller.abort(); + input.signal?.addEventListener("abort", onAbort, { once: true }); + try { + const response = await (input.fetch ?? globalThis.fetch.bind(globalThis))( + `${base.origin}${PASSPORT_REFRESH_PATH}`, + { + method: "POST", + headers: { + "content-type": "application/json", + "connect-protocol-version": "1", + // The cookie carries both halves; header casing follows the + // gateway's own expectations. + cookie: `Oasis-Token=${input.state.accessToken}...${input.state.refreshToken}`, + "Oasis-appID": input.state.appId, + "Oasis-Platform": "web", + "Oasis-Webid": input.state.deviceId, + }, + body: "{}", + signal: controller.signal, + }, + ); + if (!response.ok) { + // Every HTTP failure is treated as transient. The only signal that a + // session is truly dead is a 200 carrying an anonymous mode, which was + // verified against the live service; a 4xx here is far more likely to + // be a gateway, region or WAF rejection, and dropping a long-lived + // refresh token on that guess would sign the user out for no reason. + throw new StepLoginRefreshError("transient", `Step login state refresh failed (HTTP ${response.status})`, { + httpStatus: response.status, + }); + } + const payload: unknown = await response.json().catch((cause: unknown) => { + throw new StepLoginRefreshError("transient", "Step login state refresh returned an unreadable body", { + cause, + }); + }); + return parseRefreshResponse(payload, input.state); + } finally { + clearTimeout(timer); + input.signal?.removeEventListener("abort", onAbort); + } +} + +function parseRefreshResponse(payload: unknown, previous: StepCliLoginState): StepCliLoginState { + const record = + payload && typeof payload === "object" && !Array.isArray(payload) + ? (payload as Record) + : undefined; + const access = record ? readToken(record.accessToken) : undefined; + // Only an answer that positively says "not signed in" proves the session is + // dead — that is the one signal verified against the live service. A body of + // any other shape is contract drift or an infrastructure answer, and deleting + // a long-lived credential on that guess would sign the user out for nothing. + if (access?.mode !== undefined && access.mode !== MODE_SIGN_IN) { + throw new StepLoginRefreshError("unredeemable", "Step login state is no longer signed in"); + } + if (!access || access.mode === undefined) { + throw new StepLoginRefreshError("transient", "Step login state refresh returned an unexpected response"); + } + // The refresh token is not rotated today, so a response without one is not a + // failure: keep the stored one rather than dropping a still-valid session. + const refresh = record ? readToken(record.refreshToken) : undefined; + return { + accessToken: access.raw, + refreshToken: refresh?.raw ?? previous.refreshToken, + deviceId: previous.deviceId, + appId: previous.appId, + expiresIn: access.duration && access.duration > 0 ? access.duration : DEFAULT_ACCESS_TOKEN_TTL_SEC, + }; +} + +function readToken(value: unknown): { raw: string; mode?: number; duration?: number } | undefined { + if (!value || typeof value !== "object" || Array.isArray(value)) return undefined; + const record = value as Record; + const raw = record.raw; + if (typeof raw !== "string" || !raw.trim()) return undefined; + const mode = record.mode; + const duration = record.duration; + return { + raw: raw.trim(), + ...(typeof mode === "number" && Number.isFinite(mode) ? { mode } : {}), + ...(typeof duration === "number" && Number.isFinite(duration) && duration > 0 ? { duration } : {}), + }; +} diff --git a/packages/providers/test/step-login-refresh.test.ts b/packages/providers/test/step-login-refresh.test.ts new file mode 100644 index 00000000..9dd9deb7 --- /dev/null +++ b/packages/providers/test/step-login-refresh.test.ts @@ -0,0 +1,137 @@ +import { describe, expect, it, vi } from "vitest"; +import { refreshStepLoginState, type StepCliLoginState } from "../src/step-provider/index.ts"; + +const AUTH_BASE_URL = "https://auth.example.test"; + +function state(overrides: Partial = {}): StepCliLoginState { + return { + accessToken: "access-jwt", + refreshToken: "refresh-jwt", + deviceId: "device-1", + appId: "10300", + ...overrides, + }; +} + +function ok(body: unknown): Response { + return new Response(JSON.stringify(body), { status: 200, headers: { "content-type": "application/json" } }); +} + +function signedIn(overrides: Record = {}): Record { + return { + accessToken: { raw: "new-access-jwt", duration: 1800, mode: 2 }, + refreshToken: { raw: "new-refresh-jwt" }, + ...overrides, + }; +} + +describe("refreshStepLoginState", () => { + it("rebuilds the Oasis cookie from the two stored tokens", async () => { + const fetchImpl = vi.fn(async (_input: string | URL, init?: RequestInit) => { + expect(String(_input)).toBe(`${AUTH_BASE_URL}/passport/proto.api.passport.v1.PassportService/RefreshToken`); + const headers = new Headers(init?.headers); + expect(headers.get("cookie")).toBe("Oasis-Token=access-jwt...refresh-jwt"); + expect(headers.get("Oasis-appID")).toBe("10300"); + expect(headers.get("Oasis-Platform")).toBe("web"); + expect(headers.get("Oasis-Webid")).toBe("device-1"); + expect(init?.body).toBe("{}"); + return ok(signedIn()); + }); + vi.stubGlobal("fetch", fetchImpl); + + await expect(refreshStepLoginState({ authBaseUrl: AUTH_BASE_URL, state: state() })).resolves.toEqual({ + accessToken: "new-access-jwt", + refreshToken: "new-refresh-jwt", + deviceId: "device-1", + appId: "10300", + expiresIn: 1800, + }); + expect(fetchImpl).toHaveBeenCalledTimes(1); + }); + + it("keeps the device and app identity across refreshes", async () => { + vi.stubGlobal("fetch", async () => ok(signedIn())); + const refreshed = await refreshStepLoginState({ + authBaseUrl: AUTH_BASE_URL, + state: state({ deviceId: "device-9", appId: "20700" }), + }); + expect(refreshed.deviceId).toBe("device-9"); + expect(refreshed.appId).toBe("20700"); + }); + + it("falls back to a default lifetime when the server reports none", async () => { + vi.stubGlobal("fetch", async () => ok(signedIn({ accessToken: { raw: "new-access-jwt", mode: 2 } }))); + const refreshed = await refreshStepLoginState({ authBaseUrl: AUTH_BASE_URL, state: state() }); + expect(refreshed.expiresIn).toBe(1800); + }); + + it("treats an anonymous refresh as unredeemable, not as a new session", async () => { + vi.stubGlobal("fetch", async () => ok(signedIn({ accessToken: { raw: "anon", duration: 1800, mode: 1 } }))); + await expect(refreshStepLoginState({ authBaseUrl: AUTH_BASE_URL, state: state() })).rejects.toMatchObject({ + name: "StepLoginRefreshError", + failure: "unredeemable", + }); + }); + + it("treats a missing refresh token as a still-valid session, not a dead one", async () => { + // The refresh token is not rotated today, so a response without one says + // nothing about the session being dead. + vi.stubGlobal("fetch", async () => ok({ accessToken: { raw: "new-access-jwt", duration: 1800, mode: 2 } })); + await expect(refreshStepLoginState({ authBaseUrl: AUTH_BASE_URL, state: state() })).resolves.toMatchObject({ + accessToken: "new-access-jwt", + refreshToken: "refresh-jwt", + }); + }); + + it("treats an unexpected 200 body as transient rather than a dead session", async () => { + // A gateway or proxy answering 200 with its own envelope must not cost the + // user their long-lived credential. + for (const body of ['{"code":1,"msg":"busy"}', "[]", "null", '"text"', "{}"]) { + vi.stubGlobal( + "fetch", + async () => new Response(body, { status: 200, headers: { "content-type": "application/json" } }), + ); + await expect(refreshStepLoginState({ authBaseUrl: AUTH_BASE_URL, state: state() })).rejects.toMatchObject({ + failure: "transient", + }); + } + }); + + it("treats every HTTP failure as transient, because only the mode proves a dead session", async () => { + // A 4xx here is far more likely to be a gateway, region or WAF rejection + // than a dead session, and dropping a long-lived refresh token on that + // guess would sign the user out for no reason. + for (const status of [400, 401, 403, 404, 405, 408, 409, 429, 500, 503]) { + vi.stubGlobal("fetch", async () => new Response("", { status })); + await expect(refreshStepLoginState({ authBaseUrl: AUTH_BASE_URL, state: state() })).rejects.toMatchObject({ + failure: "transient", + httpStatus: status, + }); + } + }); + + it("treats an unreadable body as transient", async () => { + vi.stubGlobal("fetch", async () => new Response("not json", { status: 200 })); + await expect(refreshStepLoginState({ authBaseUrl: AUTH_BASE_URL, state: state() })).rejects.toMatchObject({ + failure: "transient", + }); + }); + + it("never puts a token in the URL", async () => { + const calls: Array = []; + vi.stubGlobal("fetch", async (input: string | URL) => { + calls.push(input); + return ok(signedIn()); + }); + await refreshStepLoginState({ authBaseUrl: AUTH_BASE_URL, state: state() }); + const url = String(calls[0]); + expect(url).not.toContain("access-jwt"); + expect(url).not.toContain("refresh-jwt"); + }); + + it("refuses a plain-HTTP developer center", async () => { + await expect(refreshStepLoginState({ authBaseUrl: "http://auth.example.test", state: state() })).rejects.toThrow( + /must use https/, + ); + }); +}); From 1b39a4d6139f0d46bd7c4df4001c6352e47f38a2 Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Mon, 19 Oct 2026 17:09:05 +0800 Subject: [PATCH 2/6] fix(step): close the account-session races and keep it out of auth.json MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the previous commit found the session could still be written or removed concurrently with a refresh, and that it could reach auth.json after all. Every writer and remover of the session file now takes the same cross-process lock, so a sign-in, a sign-out and an in-flight refresh cannot interleave. The lock only guards the read-modify-write; the network call stays outside it, so a slow refresh cannot block a sign-out. Deleting the session no longer swallows errors. Reporting a successful sign-out while a long-lived refresh token is still on disk is worse than failing, so only "already gone" is ignored and anything else surfaces. The login flow rides the account session along on the credential it returns. Credentials are persisted verbatim, so that field is stripped in the provider adapter before it reaches auth.json — a copy there would never be refreshed and would outlive the session it belongs to. A previous session is now dropped when a new sign-in is persisted. Signing in again as another account, or with a profile that carries no session, would otherwise leave the old account's session behind for a host to read as the current identity. The session path follows the credential path when only that is configured, so a host that isolates auth.json isolates the session too instead of sharing one file between instances. A session that arrives unusable is reported and dropped instead of failing the poll. The API key is already in hand at that point, and a session problem must never cost the user their key; the reason is surfaced so contract drift stays visible. The refresh deadline is recomputed from the token the server returned rather than inherited from the one it replaced, which matters as soon as refresh tokens start rotating. Both warnings are shown when a plan check and a failed session save happen together, instead of one hiding the other. An already-aborted signal now cancels the refresh instead of letting it run to its timeout. --- .../src/core/provider-composer.ts | 7 +- packages/coding-agent/src/step/login-flow.ts | 23 ++++- packages/coding-agent/src/step/login-state.ts | 83 +++++++++++++++++-- packages/coding-agent/src/step/onboarding.ts | 4 +- .../coding-agent/test/step-cli-login.test.ts | 8 +- .../test/step-login-state.test.ts | 48 +++++++++++ .../coding-agent/test/step-plan-login.test.ts | 31 +++++++ packages/providers/src/step-provider/index.ts | 5 +- .../src/step-provider/login-client.ts | 29 ++++--- .../src/step-provider/login-refresh.ts | 3 + 10 files changed, 210 insertions(+), 31 deletions(-) diff --git a/packages/coding-agent/src/core/provider-composer.ts b/packages/coding-agent/src/core/provider-composer.ts index 13411964..1d4d0ac5 100644 --- a/packages/coding-agent/src/core/provider-composer.ts +++ b/packages/coding-agent/src/core/provider-composer.ts @@ -265,7 +265,12 @@ function adaptOAuth(config: ExtensionOAuthConfig): OAuthAuth { onSelect: (prompt) => callbacks.prompt({ type: "select", ...prompt }), signal: callbacks.signal, }); - return { ...credential, type: "oauth" }; + // A login flow may ride an account session along on the credential so + // the product layer can store it separately. It must not reach + // `auth.json`: credentials are persisted verbatim, and a copy there + // would never be refreshed. + const { login: _accountSession, ...credentialFields } = credential; + return { ...credentialFields, type: "oauth" }; }, refresh: async (credential, signal) => ({ ...(await config.refreshToken(credential, signal)), type: "oauth" }), toAuth: async (credential) => ({ apiKey: config.getApiKey(credential) }), diff --git a/packages/coding-agent/src/step/login-flow.ts b/packages/coding-agent/src/step/login-flow.ts index c30ee238..c0312f1c 100644 --- a/packages/coding-agent/src/step/login-flow.ts +++ b/packages/coding-agent/src/step/login-flow.ts @@ -11,7 +11,7 @@ import { import { detectTerminalBackgroundFromEnv, initTheme, resolveThemeSetting, theme } from "../theme/theme.ts"; import { openBrowser } from "../utils/open-browser.ts"; import { resolveStepAgentDir } from "./environment.ts"; -import { buildStepLoginState, saveStepLoginState } from "./login-state.ts"; +import { buildStepLoginState, clearStepLoginState, saveStepLoginState } from "./login-state.ts"; import { INITIAL_STEP_LOGIN_STEP, isStepLoginSettled, @@ -72,6 +72,12 @@ function readLoginState(value: unknown): StepCliLoginState | undefined { return parseStepCliLoginState(value); } +/** Both warnings matter; showing only one hides why account features are gone. */ +function combineWarnings(...warnings: Array): string | undefined { + const present = warnings.filter((warning): warning is string => Boolean(warning)); + return present.length > 0 ? present.join("\n") : undefined; +} + /** Model discovery can return fallback models even when the account has no plan. */ export function getMissingStepPlanProfile( state: AccountUsageState, ): "step_plan" | "step_plan_oversea" | undefined { @@ -294,6 +300,7 @@ export async function runStepLogin(options: RunStepLoginOptions = {}): Promise { const saving = step; try { @@ -383,6 +392,12 @@ export async function runStepLogin(options: RunStepLoginOptions = {}): Promise = process.env): string { - return env.STEPCODE_LOGIN_STATE_PATH?.trim() || join(resolveStepConfigRoot(env), LOGIN_STATE_FILE_NAME); + const explicit = env.STEPCODE_LOGIN_STATE_PATH?.trim(); + if (explicit) return explicit; + // Follow the credential path: a host that isolates `auth.json` isolates the + // session too. Falling back to the shared default would let two isolated + // instances overwrite each other's identity. + const authPath = env.STEPCODE_AUTH_PATH?.trim(); + if (authPath) return join(dirname(resolve(authPath)), LOGIN_STATE_FILE_NAME); + return join(resolveStepConfigRoot(env), LOGIN_STATE_FILE_NAME); } /** @@ -123,6 +130,9 @@ export function buildStepLoginState( /** * Persist the session. * + * Takes the cross-process lock, so a sign-in that lands while another process + * is committing a refresh cannot be overwritten by that refresh's result. + * * Written atomically and owner-only: it holds a long-lived refresh token, so a * partially written or world-readable file is a credential leak. */ @@ -130,6 +140,24 @@ export async function saveStepLoginState( state: StepLoginState, env: Record = process.env, ): Promise { + const path = await ensureLoginStateFile(env); + let release: (() => void) | undefined; + try { + release = acquireSettingsLockSync(path); + } catch { + // Another process holds it. It re-reads before committing, so letting it + // win is safe and avoids interleaved writes. + return; + } + try { + await writeLoginStateFile(state, env); + } finally { + release(); + } +} + +/** Write without locking; callers must already hold the lock. */ +async function writeLoginStateFile(state: StepLoginState, env: Record): Promise { const path = getStepLoginStatePath(env); await mkdir(dirname(path), { recursive: true }); // A fixed name would let two concurrent writers in one process share a @@ -147,9 +175,41 @@ export async function saveStepLoginState( await chmod(path, 0o600); } -/** Drop the session. The API key in `auth.json` is deliberately left alone. */ +/** The lock needs a file to lock, so create an inert one when there is none. */ +async function ensureLoginStateFile(env: Record): Promise { + const path = getStepLoginStatePath(env); + if (!existsSync(path)) { + await mkdir(dirname(path), { recursive: true }); + await writeFile(path, "", { encoding: "utf8", mode: 0o600 }); + } + return path; +} + +/** + * Drop the session. The API key in `auth.json` is deliberately left alone. + * + * Takes the lock so a sign-out cannot land between a committer's re-read and + * its write, which would resurrect a session the user just signed out of. + * + * Errors other than "already gone" are surfaced: reporting a successful sign-out + * while a long-lived refresh token is still on disk is worse than failing. + */ export async function clearStepLoginState(env: Record = process.env): Promise { - await rm(getStepLoginStatePath(env), { force: true }).catch(() => {}); + const path = getStepLoginStatePath(env); + if (!existsSync(path)) return; + let release: (() => void) | undefined; + try { + release = acquireSettingsLockSync(path); + } catch { + // Not lockable: another process owns it, so removing it could delete a + // session that was just written. Leave it for that process to settle. + return; + } + try { + await rm(path, { force: true }); + } finally { + release(); + } } /** @@ -344,14 +404,21 @@ async function refreshAndCommit( if (!latest || latest.refreshToken !== before.refreshToken) { return { kind: "unchanged", ...(latest ? { state: latest } : {}) }; } - const next: StepLoginState = { - ...latest, + const { refreshExpiresAt: _inherited, ...rest } = latest; + const committed: StepLoginState = { + ...rest, accessToken: refreshed.accessToken, refreshToken: refreshed.refreshToken, accessExpiresAt: Math.floor(Date.now() / 1000) + (refreshed.expiresIn ?? DEFAULT_ACCESS_TOKEN_TTL_SEC), + // Recomputed rather than inherited from the token being replaced: the + // response carries a refresh token, and once rotation starts its + // deadline is the previous token's, which would discard a live session. + ...(readJwtExpiresAt(refreshed.refreshToken) === undefined + ? {} + : { refreshExpiresAt: readJwtExpiresAt(refreshed.refreshToken) }), }; - await saveStepLoginState(next, env); - return { kind: "refreshed", state: next }; + await writeLoginStateFile(committed, env); + return { kind: "refreshed", state: committed }; } finally { release(); } diff --git a/packages/coding-agent/src/step/onboarding.ts b/packages/coding-agent/src/step/onboarding.ts index 082b79fa..98c016e2 100644 --- a/packages/coding-agent/src/step/onboarding.ts +++ b/packages/coding-agent/src/step/onboarding.ts @@ -133,8 +133,10 @@ export type StepLoginEvent = readonly type: "credential"; readonly apiKey: string; readonly uid?: string; - /** Account session, when the backend delivered one. */ + /** Account session, when the backend delivered a usable one. */ readonly login?: StepCliLoginState; + /** Why a delivered session was unusable; the API key is still valid. */ + readonly loginError?: string; } | { readonly type: "type"; readonly text: string } | { readonly type: "backspace" } diff --git a/packages/coding-agent/test/step-cli-login.test.ts b/packages/coding-agent/test/step-cli-login.test.ts index 32d1b293..8981b75f 100644 --- a/packages/coding-agent/test/step-cli-login.test.ts +++ b/packages/coding-agent/test/step-cli-login.test.ts @@ -304,13 +304,13 @@ describe("Step CLI login poll", () => { ).resolves.toEqual({ state: "ready", apiKey: "sk-step" }); }); - it("rejects a login field it cannot refresh rather than storing it half-formed", async () => { + it("degrades to api-key-only when a login field is unusable", async () => { + // A session problem must never cost the user their key: the key is already + // in hand, so the unusable session is reported and dropped instead. for (const login of [ { access_token: "a", refresh_token: "b", device_id: "d" }, { access_token: "a", refresh_token: "b", app_id: "10300" }, { access_token: "a", device_id: "d", app_id: "10300" }, - { refresh_token: "b", device_id: "d", app_id: "10300" }, - { access_token: "a", refresh_token: "b", device_id: "", app_id: "10300" }, "not-an-object", ]) { await expect( @@ -320,7 +320,7 @@ describe("Step CLI login poll", () => { flowId: "flow-1", fetch: async () => json({ status: 0, state: "ready", credential: { api_key: "sk-step", login } }), }), - ).rejects.toMatchObject({ kind: "protocol" }); + ).resolves.toMatchObject({ state: "ready", apiKey: "sk-step", loginError: expect.any(String) }); } }); diff --git a/packages/coding-agent/test/step-login-state.test.ts b/packages/coding-agent/test/step-login-state.test.ts index 77727444..0bca8a25 100644 --- a/packages/coding-agent/test/step-login-state.test.ts +++ b/packages/coding-agent/test/step-login-state.test.ts @@ -42,6 +42,11 @@ function loginState(overrides: Partial = {}): StepCliLoginSta }; } +/** A successful Connect-RPC envelope from passport. */ +function ok(body: unknown): Response { + return new Response(JSON.stringify(body), { status: 200, headers: { "content-type": "application/json" } }); +} + function stored(overrides: Partial = {}): StepLoginState { return { version: 1, @@ -103,6 +108,25 @@ describe("step login state storage", () => { expect(readStepLoginState(env)).toBeUndefined(); }); + it("derives the session path from the credential path when only that is set", async () => { + // A host that isolates auth.json must isolate the session too, or two + // instances would overwrite each other's identity. + const authPath = join(dir, "nested", "auth.json"); + expect(getStepLoginStatePath({ STEPCODE_AUTH_PATH: authPath })).toBe(join(dir, "nested", "step-login.json")); + expect(getStepLoginStatePath({ STEPCODE_LOGIN_STATE_PATH: "/explicit.json" })).toBe("/explicit.json"); + }); + + it("drops the previous session when a new sign-in is persisted", async () => { + const envA = { STEPCODE_LOGIN_STATE_PATH: join(dir, "a.json") }; + const envB = { STEPCODE_LOGIN_STATE_PATH: join(dir, "b.json") }; + await saveStepLoginState(stored({ refreshToken: refreshToken() }), envA); + await saveStepLoginState(stored({ refreshToken: refreshToken(), profile: "step_plan_oversea" }), envB); + // A stale session from another account must never outlive the new sign-in. + await clearStepLoginState(envA); + expect(readStepLoginState(envA)).toBeUndefined(); + expect(readStepLoginState(envB)).toMatchObject({ profile: "step_plan_oversea" }); + }); + it("keeps the api key out of this file's business: clearing only removes the session", async () => { await saveStepLoginState(stored(), env); await clearStepLoginState(env); @@ -260,6 +284,30 @@ describe("getStepLoginToken", () => { expect(readStepLoginState(env)).toBeUndefined(); }); + it("recomputes the refresh deadline from the token the server returned", async () => { + // The refresh token is not rotated today. Once it is, inheriting the old + // deadline would discard a session the server still considers live. + const shortLived = stored({ + accessExpiresAt: Math.floor(Date.now() / 1000) + 60, + accessToken: accessToken(60), + refreshExpiresAt: Math.floor(Date.now() / 1000) + 3600, + }); + await saveStepLoginState(shortLived, env); + vi.stubGlobal( + "fetch", + vi.fn(async () => + ok({ + accessToken: { raw: accessToken(1800), duration: 1800, mode: 2 }, + refreshToken: { raw: refreshToken(30 * 24 * 60 * 60) }, + }), + ), + ); + + expect((await getStepLoginToken({ env })).status).toBe("ready"); + const committed = readStepLoginState(env); + expect(committed?.refreshExpiresAt).toBeGreaterThan(shortLived.refreshExpiresAt!); + }); + it("keeps a session that was replaced while an unredeemable refresh was running", async () => { const dead = stored({ accessExpiresAt: Math.floor(Date.now() / 1000) + 60, diff --git a/packages/coding-agent/test/step-plan-login.test.ts b/packages/coding-agent/test/step-plan-login.test.ts index 28ceca83..c40b665d 100644 --- a/packages/coding-agent/test/step-plan-login.test.ts +++ b/packages/coding-agent/test/step-plan-login.test.ts @@ -378,4 +378,35 @@ describe("Step Plan login availability", () => { await login; } }); + + it("still signs in when the backend sends an unusable account session", async () => { + // The API key is already in hand, so a broken session field must not cost + // the user their sign-in — only account features become unavailable. + const loginStatePath = join(root, "step-login.json"); + oauthResult.current = { + access: "plan-key", + refresh: "static", + expires: Number.MAX_SAFE_INTEGER, + uid: "123", + login: { access_token: "a", refresh_token: "b" }, + loginError: "the login field was missing a required token, device id or app id", + }; + const { login, view } = start( + controller(vi.fn(async () => Response.json(snapshot("coding")))), + undefined, + { STEPCODE_LOGIN_STATE_PATH: loginStatePath }, + ); + try { + view.handleInput("1"); + await expect(login).resolves.toMatchObject({ + kind: "completed", + warning: expect.stringContaining("account session was rejected"), + }); + await expect(readFile(loginStatePath, "utf8")).rejects.toThrow(); + expect(readStepLoginCredential(authPath)).toMatchObject({ type: "oauth", access: "plan-key" }); + } finally { + view.handleInput("\x03"); + await login; + } + }); }); diff --git a/packages/providers/src/step-provider/index.ts b/packages/providers/src/step-provider/index.ts index 520f3314..8f76a894 100644 --- a/packages/providers/src/step-provider/index.ts +++ b/packages/providers/src/step-provider/index.ts @@ -315,9 +315,10 @@ export async function loginStepOAuth( refresh: STEP_STATIC_REFRESH_TOKEN, expires: Number.MAX_SAFE_INTEGER, ...(ready.uid ? { uid: ready.uid } : {}), - // Rides along as an extra field: the API key stays the credential, - // and hosts that need an identity read this beside it. + // Rides along as extra fields: the API key stays the credential, and + // hosts that need an identity read these beside it. ...(ready.login ? { login: ready.login } : {}), + ...(ready.loginError ? { loginError: ready.loginError } : {}), }; }, ); diff --git a/packages/providers/src/step-provider/login-client.ts b/packages/providers/src/step-provider/login-client.ts index 943a2a37..cb16e4d9 100644 --- a/packages/providers/src/step-provider/login-client.ts +++ b/packages/providers/src/step-provider/login-client.ts @@ -65,6 +65,8 @@ export type StepCliLoginPoll = readonly apiKey: string; readonly uid?: string; readonly login?: StepCliLoginState; + /** Why a delivered session could not be used; the API key is still valid. */ + readonly loginError?: string; } | { readonly state: "failed"; readonly reason: StepCliLoginFailureReason }; @@ -218,12 +220,13 @@ export async function pollStepCliLogin(input: PollStepCliLoginInput): Promise, "api_key"); if (!apiKey) throw protocolError("poll", "ready response did not contain an api_key"); const uid = readBoundedUid(readString(payload, "uid")); - const login = readLoginState(credential as Record); + const session = readLoginState(credential as Record); return { state: "ready", apiKey, ...(uid ? { uid } : {}), - ...(login ? { login } : {}), + ...(session.state ? { login: session.state } : {}), + ...(session.error ? { loginError: session.error } : {}), }; } @@ -231,20 +234,20 @@ export async function pollStepCliLogin(input: PollStepCliLoginInput): Promise): StepCliLoginState | undefined { +function readLoginState(credential: Record): { state?: StepCliLoginState; error?: string } { const raw = credential.login; - if (raw === undefined || raw === null) return undefined; + if (raw === undefined || raw === null) return {}; + if (!raw || typeof raw !== "object" || Array.isArray(raw)) { + return { error: "the login field was not an object" }; + } const parsed = parseStepLoginState(raw); - if (!parsed) throw protocolError("poll", "login field is not a usable account session"); - return parsed; + if (!parsed) return { error: "the login field was missing a required token, device id or app id" }; + return { state: parsed }; } /** diff --git a/packages/providers/src/step-provider/login-refresh.ts b/packages/providers/src/step-provider/login-refresh.ts index 546406f8..9b3956ad 100644 --- a/packages/providers/src/step-provider/login-refresh.ts +++ b/packages/providers/src/step-provider/login-refresh.ts @@ -78,6 +78,9 @@ export interface RefreshStepLoginStateInput { export async function refreshStepLoginState(input: RefreshStepLoginStateInput): Promise { const base = parseAuthBaseUrl(input.authBaseUrl); const controller = new AbortController(); + // A caller that already gave up must not leave the request running: the + // abort event has fired before the listener exists, so it is checked here. + if (input.signal?.aborted) controller.abort(input.signal.reason); const timer = setTimeout(() => controller.abort(), REFRESH_TIMEOUT_MS); const onAbort = () => controller.abort(); input.signal?.addEventListener("abort", onAbort, { once: true }); From 50feac83b20eac81bbfdcab377f27b4564d4c2ad Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Mon, 19 Oct 2026 17:13:31 +0800 Subject: [PATCH 3/6] fix(step): report a dead session instead of falling back to its token MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the server answered a refresh with an anonymous session but the discard could not proceed — because another process held the lock — the caller fell back to the stored token and reported it as ready. That hands out a credential the server has just rejected, and the caller's request fails instead of the caller learning the session is gone. The two outcomes are now separate: a session that is positively dead reports expired whether or not it could be removed, while a session that was *replaced* during the request still reports the replacement. The refresh deadline also gets an hour of clock slack before it retires a session. That deadline is read from a server-issued token, so a machine whose clock runs fast would otherwise discard a session the server still honours; waiting costs one refresh the server answers honestly. --- packages/coding-agent/src/step/login-state.ts | 32 +++++++++-- .../test/step-login-state.test.ts | 54 ++++++++++++++++++- 2 files changed, 82 insertions(+), 4 deletions(-) diff --git a/packages/coding-agent/src/step/login-state.ts b/packages/coding-agent/src/step/login-state.ts index 8e4393e0..0246d722 100644 --- a/packages/coding-agent/src/step/login-state.ts +++ b/packages/coding-agent/src/step/login-state.ts @@ -32,6 +32,15 @@ export const STEP_LOGIN_REFRESH_MARGIN_SEC = 5 * 60; /** Fallback when neither the server nor the token states a lifetime. */ const DEFAULT_ACCESS_TOKEN_TTL_SEC = 30 * 60; +/** + * Clock slack applied before a refresh token is considered expired. + * + * Its deadline is read from a server-issued token, so a machine whose clock runs + * fast would otherwise discard a session the server still accepts. The cost of + * waiting is one refresh attempt the server answers honestly. + */ +const REFRESH_EXPIRY_SKEW_SEC = 60 * 60; + const LOGIN_STATE_FILE_NAME = "step-login.json"; export interface StepLoginState { @@ -289,7 +298,7 @@ export async function getStepLoginToken(options: StepLoginTokenOptions = {}): Pr try { const result = await refreshAndCommit(env, options); if (result.kind === "refreshed") return ready(result.state); - if (result.kind === "discarded") { + if (result.kind === "discarded" || result.kind === "dead") { return { status: "expired", reason: "the account session is no longer signed in" }; } const latest = result.state ?? readStepLoginState(env); @@ -317,7 +326,12 @@ function describe(error: unknown): string { } function isRefreshTokenExpired(state: StepLoginState, nowMs: number): boolean { - return state.refreshExpiresAt !== undefined && state.refreshExpiresAt <= Math.floor(nowMs / 1000); + // The deadline comes from a server-issued token, so a local clock that runs + // fast would otherwise retire a session the server still honours. Waiting out + // the skew costs at most one failed refresh, which the server answers + // honestly anyway. + if (state.refreshExpiresAt === undefined) return false; + return state.refreshExpiresAt + REFRESH_EXPIRY_SKEW_SEC <= Math.floor(nowMs / 1000); } function ready(state: StepLoginState): StepLoginTokenOutcome { @@ -344,6 +358,8 @@ function needsRefresh(state: StepLoginState, nowMs: number): boolean { type RefreshCommitResult = | { readonly kind: "refreshed"; readonly state: StepLoginState } | { readonly kind: "discarded" } + /** Positively dead but could not be removed; the caller must not use it. */ + | { readonly kind: "dead"; readonly state?: StepLoginState } | { readonly kind: "unchanged"; readonly state?: StepLoginState }; /** @@ -384,7 +400,17 @@ async function refreshAndCommit( }); } catch (error) { if (error instanceof StepLoginRefreshError && error.failure === "unredeemable") { - return (await discardLoginState(env, before.refreshToken)) ? { kind: "discarded" } : { kind: "unchanged" }; + // The session is positively dead. Whether it could be removed decides + // what is left on disk — but a session that was *replaced* while the + // request ran must survive, so that case is reported as a no-op. + const removed = await discardLoginState(env, before.refreshToken); + if (!removed) { + const replacement = readStepLoginState(env); + if (replacement && replacement.refreshToken !== before.refreshToken) { + return { kind: "unchanged", state: replacement }; + } + } + return removed ? { kind: "discarded" } : { kind: "dead" }; } throw error; } diff --git a/packages/coding-agent/test/step-login-state.test.ts b/packages/coding-agent/test/step-login-state.test.ts index 0bca8a25..2caced16 100644 --- a/packages/coding-agent/test/step-login-state.test.ts +++ b/packages/coding-agent/test/step-login-state.test.ts @@ -42,6 +42,15 @@ function loginState(overrides: Partial = {}): StepCliLoginSta }; } +/** A signed-in refresh response. */ +function signedIn(overrides: Record = {}): Record { + return { + accessToken: { raw: accessToken(1800), duration: 1800, mode: 2 }, + refreshToken: { raw: refreshToken() }, + ...overrides, + }; +} + /** A successful Connect-RPC envelope from passport. */ function ok(body: unknown): Response { return new Response(JSON.stringify(body), { status: 200, headers: { "content-type": "application/json" } }); @@ -392,7 +401,8 @@ describe("getStepLoginToken", () => { stored({ accessExpiresAt: Math.floor(Date.now() / 1000) + 60, accessToken: accessToken(60), - refreshExpiresAt: Math.floor(Date.now() / 1000) - 1, + // Past the clock skew, so a fast machine cannot keep it alive. + refreshExpiresAt: Math.floor(Date.now() / 1000) - 2 * 60 * 60, }), env, ); @@ -403,4 +413,46 @@ describe("getStepLoginToken", () => { expect(fetchSpy).not.toHaveBeenCalled(); expect(readStepLoginState(env)).toBeUndefined(); }); + + it("keeps a session whose refresh token only looks expired to a fast clock", async () => { + // The deadline is server-issued; a machine running fast must not retire a + // session the server still honours. + await saveStepLoginState( + stored({ + accessExpiresAt: Math.floor(Date.now() / 1000) + 60, + accessToken: accessToken(60), + refreshExpiresAt: Math.floor(Date.now() / 1000) - 60, + }), + env, + ); + const fetchSpy = vi.fn(async () => ok(signedIn())); + vi.stubGlobal("fetch", fetchSpy); + + expect((await getStepLoginToken({ env })).status).toBe("ready"); + expect(fetchSpy).toHaveBeenCalledTimes(1); + expect(readStepLoginState(env)).toBeDefined(); + }); + + it("reports expired rather than ready when a dead session cannot be removed", async () => { + await saveStepLoginState( + stored({ accessExpiresAt: Math.floor(Date.now() / 1000) + 60, accessToken: accessToken(60) }), + env, + ); + const path = getStepLoginStatePath(env); + // Hold the lock so the discard cannot proceed, then have the server say the + // session is anonymous. Falling back to the stored token would hand out a + // credential the server just rejected. + const release = acquireSettingsLockSync(path); + try { + vi.stubGlobal( + "fetch", + vi.fn(async () => + ok({ accessToken: { raw: "anon", duration: 1800, mode: 1 }, refreshToken: { raw: refreshToken() } }), + ), + ); + expect(await getStepLoginToken({ env })).toMatchObject({ status: "expired" }); + } finally { + release(); + } + }); }); From 426510b510f2501e7e92d8b20a5938a0f7df612d Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Mon, 19 Oct 2026 17:22:29 +0800 Subject: [PATCH 4/6] fix(step): stop swallowing session lock failures Adding the lock reintroduced a bare catch that treated every acquisition failure as "another process won". That is only true for lock contention. A read-only directory or a permission problem failed the same way and silently did nothing, so a sign-in could report success while the previous account's session was still on disk, and a sign-out could report a removal that never happened. Only contention is tolerated now; anything else surfaces. Both operations return whether they took effect so callers can report it honestly, and `logoutStepCredentials` derives its result from that instead of from whether the file existed beforehand. A failed session update during sign-in is a warning rather than a failed login. The API key is already written by then, and reporting "Could not save credentials" for a session problem misrepresents what happened and skips the endpoint sync. The refresh deadline is decoded once instead of twice. --- packages/coding-agent/src/step/auth.ts | 7 +- packages/coding-agent/src/step/login-flow.ts | 32 ++++--- packages/coding-agent/src/step/login-state.ts | 86 +++++++++++-------- .../test/step-login-state.test.ts | 32 +++++++ 4 files changed, 103 insertions(+), 54 deletions(-) diff --git a/packages/coding-agent/src/step/auth.ts b/packages/coding-agent/src/step/auth.ts index a53b6a02..c815127f 100644 --- a/packages/coding-agent/src/step/auth.ts +++ b/packages/coding-agent/src/step/auth.ts @@ -13,7 +13,7 @@ import type { Credential } from "@step-harness/providers"; import { AuthStorage, readStoredCredential } from "../core/auth-storage.ts"; import { STEP_PROVIDER_ID, STEP_STATIC_REFRESH_TOKEN } from "../features/step-provider/index.ts"; import { LEGACY_RENAMED_CONFIG_DIR, resolveStepConfigRoot, resolveStepHomeDir } from "./environment.ts"; -import { clearStepLoginState, getStepLoginStatePath } from "./login-state.ts"; +import { clearStepLoginState } from "./login-state.ts"; /** Current Pi-shaped credential path under the StepCode agent directory. */ export function getStepAuthPath(env: Record = process.env): string { @@ -144,8 +144,9 @@ export async function logoutStepCredentials( const legacyPath = normalize(options.legacyPath ?? getLegacyStepAuthPath()); // The account session is a separate file with a separate lifetime, so it is // cleared here too — but only the session, never anything else on disk. - const removedLoginState = existsSync(getStepLoginStatePath(env)); - await clearStepLoginState(env); + // The result is the actual outcome: reporting a removal that did not happen + // would leave a long-lived refresh token behind while claiming success. + const removedLoginState = await clearStepLoginState(env); let removedNative = false; let removedLegacy = false; diff --git a/packages/coding-agent/src/step/login-flow.ts b/packages/coding-agent/src/step/login-flow.ts index c0312f1c..eec578bf 100644 --- a/packages/coding-agent/src/step/login-flow.ts +++ b/packages/coding-agent/src/step/login-flow.ts @@ -391,25 +391,31 @@ export async function runStepLogin(options: RunStepLoginOptions = {}): Promise new Date()))().toISOString(), }); // The account session is a separate file on purpose: it expires and - // refreshes on its own, and losing it must never cost the API key. + // refreshes on its own, and losing it must never cost the API key. Every + // failure here is a warning, never a failed sign-in — the key is already + // written by this point. // // Any previous session is dropped first. Signing in again — as another // account, or with a profile that carries no session at all — must not // leave the old account's session behind, or a host would read an // identity that does not match the API key just written. - await clearStepLoginState(options.env ?? process.env); - if (login) { - try { - await saveStepLoginState(buildStepLoginState(login, choice), options.env ?? process.env); - } catch (error) { - loginWarning = `Signed in, but the account session could not be saved: ${ - error instanceof Error ? error.message : String(error) - }. Account features will ask you to sign in again.`; + try { + const cleared = await clearStepLoginState(options.env ?? process.env); + let written = true; + if (login) + written = await saveStepLoginState(buildStepLoginState(login, choice), options.env ?? process.env); + if (!cleared || !written) { + loginWarning = + "Signed in, but the account session could not be written. Account features may use a previous session."; + } else if (loginError) { + // The backend sent a session that could not be used. The API key is + // unaffected; only account features are unavailable. + loginWarning = `Signed in, but the account session was rejected by the server (${loginError}).`; } - } else if (loginError) { - // The backend sent a session that could not be used. The API key is - // unaffected; only account features are unavailable. - loginWarning = `Signed in, but the account session was rejected by the server (${loginError}).`; + } catch (error) { + loginWarning = `Signed in, but the account session could not be saved: ${ + error instanceof Error ? error.message : String(error) + }. Account features will ask you to sign in again.`; } if (step !== saving) return; savedProfile = profile; diff --git a/packages/coding-agent/src/step/login-state.ts b/packages/coding-agent/src/step/login-state.ts index 0246d722..817b7123 100644 --- a/packages/coding-agent/src/step/login-state.ts +++ b/packages/coding-agent/src/step/login-state.ts @@ -137,7 +137,31 @@ export function buildStepLoginState( } /** - * Persist the session. + * Acquire the cross-process session lock. + * + * Returns undefined when another process holds it: letting that process win is + * safe because every writer re-reads before committing. Anything else — a + * read-only directory, a permission problem — is a real failure and must + * surface, because silently skipping would report a successful sign-in or + * sign-out while the file on disk says otherwise. + */ +function acquireSessionLock(path: string): (() => void) | undefined { + try { + return acquireSettingsLockSync(path); + } catch (error) { + if (isLockContention(error)) return undefined; + throw error; + } +} + +function isLockContention(error: unknown): boolean { + return ( + typeof error === "object" && error !== null && "code" in error && (error as { code?: unknown }).code === "ELOCKED" + ); +} + +/** + * Persist the session, returning whether it was written. * * Takes the cross-process lock, so a sign-in that lands while another process * is committing a refresh cannot be overwritten by that refresh's result. @@ -148,18 +172,13 @@ export function buildStepLoginState( export async function saveStepLoginState( state: StepLoginState, env: Record = process.env, -): Promise { +): Promise { const path = await ensureLoginStateFile(env); - let release: (() => void) | undefined; - try { - release = acquireSettingsLockSync(path); - } catch { - // Another process holds it. It re-reads before committing, so letting it - // win is safe and avoids interleaved writes. - return; - } + const release = acquireSessionLock(path); + if (!release) return false; try { await writeLoginStateFile(state, env); + return true; } finally { release(); } @@ -195,27 +214,23 @@ async function ensureLoginStateFile(env: Record): Pr } /** - * Drop the session. The API key in `auth.json` is deliberately left alone. + * Drop the session, returning whether it is gone. * - * Takes the lock so a sign-out cannot land between a committer's re-read and - * its write, which would resurrect a session the user just signed out of. + * Takes the lock so a sign-out cannot land between a committer's re-read and its + * write, which would resurrect a session the user just signed out of. * - * Errors other than "already gone" are surfaced: reporting a successful sign-out - * while a long-lived refresh token is still on disk is worse than failing. + * Only lock contention is tolerated: any other failure is surfaced, because + * reporting a successful sign-out while a long-lived refresh token is still on + * disk is worse than failing. */ -export async function clearStepLoginState(env: Record = process.env): Promise { +export async function clearStepLoginState(env: Record = process.env): Promise { const path = getStepLoginStatePath(env); - if (!existsSync(path)) return; - let release: (() => void) | undefined; - try { - release = acquireSettingsLockSync(path); - } catch { - // Not lockable: another process owns it, so removing it could delete a - // session that was just written. Leave it for that process to settle. - return; - } + if (!existsSync(path)) return true; + const release = acquireSessionLock(path); + if (!release) return false; try { await rm(path, { force: true }); + return true; } finally { release(); } @@ -237,12 +252,10 @@ async function discardLoginState( ): Promise { const path = getStepLoginStatePath(env); if (!existsSync(path)) return false; - let release: (() => void) | undefined; - try { - release = acquireSettingsLockSync(path); - } catch { - // Not lockable: it either vanished or another process owns it. Leaving it - // is safer than deleting something that may already have been replaced. + const release = acquireSessionLock(path); + if (!release) { + // Another process owns it. Leaving the session is safer than deleting + // something that may already have been replaced. return false; } try { @@ -416,10 +429,8 @@ async function refreshAndCommit( } if (!existsSync(path)) return { kind: "unchanged" }; - let release: (() => void) | undefined; - try { - release = acquireSettingsLockSync(path); - } catch { + const release = acquireSessionLock(path); + if (!release) { // Another process is committing; it re-reads the same way, so letting it // win is safe and avoids a second write. return { kind: "unchanged" }; @@ -431,6 +442,7 @@ async function refreshAndCommit( return { kind: "unchanged", ...(latest ? { state: latest } : {}) }; } const { refreshExpiresAt: _inherited, ...rest } = latest; + const refreshExpiresAt = readJwtExpiresAt(refreshed.refreshToken); const committed: StepLoginState = { ...rest, accessToken: refreshed.accessToken, @@ -439,9 +451,7 @@ async function refreshAndCommit( // Recomputed rather than inherited from the token being replaced: the // response carries a refresh token, and once rotation starts its // deadline is the previous token's, which would discard a live session. - ...(readJwtExpiresAt(refreshed.refreshToken) === undefined - ? {} - : { refreshExpiresAt: readJwtExpiresAt(refreshed.refreshToken) }), + ...(refreshExpiresAt === undefined ? {} : { refreshExpiresAt }), }; await writeLoginStateFile(committed, env); return { kind: "refreshed", state: committed }; diff --git a/packages/coding-agent/test/step-login-state.test.ts b/packages/coding-agent/test/step-login-state.test.ts index 2caced16..fe41573a 100644 --- a/packages/coding-agent/test/step-login-state.test.ts +++ b/packages/coding-agent/test/step-login-state.test.ts @@ -125,6 +125,38 @@ describe("step login state storage", () => { expect(getStepLoginStatePath({ STEPCODE_LOGIN_STATE_PATH: "/explicit.json" })).toBe("/explicit.json"); }); + it("reports whether the session was actually removed", async () => { + await saveStepLoginState(stored(), env); + expect(await clearStepLoginState(env)).toBe(true); + expect(readStepLoginState(env)).toBeUndefined(); + // Nothing left to remove is still a successful removal. + expect(await clearStepLoginState(env)).toBe(true); + + await saveStepLoginState(stored(), env); + const release = acquireSettingsLockSync(getStepLoginStatePath(env)); + try { + expect(await clearStepLoginState(env)).toBe(false); + } finally { + release(); + } + expect(readStepLoginState(env)).toBeDefined(); + }); + + it("reports whether the session was actually written", async () => { + expect(await saveStepLoginState(stored(), env)).toBe(true); + expect(readStepLoginState(env)).toBeDefined(); + + // Another process holds the lock: the caller must be able to tell that + // nothing was written, or a sign-in would report success with a stale + // session still on disk. + const release = acquireSettingsLockSync(getStepLoginStatePath(env)); + try { + expect(await saveStepLoginState(stored({ refreshToken: refreshToken() }), env)).toBe(false); + } finally { + release(); + } + }); + it("drops the previous session when a new sign-in is persisted", async () => { const envA = { STEPCODE_LOGIN_STATE_PATH: join(dir, "a.json") }; const envB = { STEPCODE_LOGIN_STATE_PATH: join(dir, "b.json") }; From 01d08260879ef8e328376810937799e4b6c4453b Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Mon, 19 Oct 2026 17:49:18 +0800 Subject: [PATCH 5/6] fix(step): keep session failures from changing the outcome they report MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of the previous fixes traded one silent path for another. Discarding a session could itself fail — a read-only directory, a permission problem — and that error escaped the code paths that were supposed to report an explicit outcome. In one it reached the caller's catch, which fell back to handing out a token the server had just rejected; in the other it turned a definitive "expired" into a rejection. Discarding is now quiet: a session that cannot be removed is still a session the server refused. Sign-out cleared the session before removing the API key, so a failure there aborted the whole logout and left the key in place. It is cleared last, its failure cannot stop the key, and whether anything survived is reported — which the two call sites now act on instead of ignoring, since reporting a session still on disk is the difference between a user knowing and not. A written session has already replaced whatever preceded it, so an unsuccessful clear alongside a successful write is no longer reported as a stale session. The reason the server rejected a session is also no longer dropped when another warning is present. --- apps/cli/src/main.ts | 16 ++++++++++++- packages/coding-agent/src/step/auth.ts | 13 +++++----- packages/coding-agent/src/step/login-flow.ts | 24 ++++++++++--------- packages/coding-agent/src/step/login-state.ts | 21 ++++++++++++++-- .../test/step-login-state.test.ts | 24 +++++++++++++++++++ .../coding-agent/test/step-plan-login.test.ts | 2 +- 6 files changed, 79 insertions(+), 21 deletions(-) diff --git a/apps/cli/src/main.ts b/apps/cli/src/main.ts index a1c7efc3..80e3cef7 100644 --- a/apps/cli/src/main.ts +++ b/apps/cli/src/main.ts @@ -282,6 +282,13 @@ const stepMainOptions: MainOptions = { legacyPath: getLegacyStepAuthPath(), }); syncStepLoginProfileEndpoint(getStepAuthPath()); + if (!report.removedLoginState) { + // The API key is gone either way; only the account session survives, + // and it holds a long-lived refresh token. + process.stderr.write( + "Note: the account session could not be removed and is still on disk. Run `step logout` again.\n", + ); + } return { removed: report.removedNative || report.removedLegacy, remainingSource: report.remainingSource ? "the environment (STEP_API_KEY)" : null, @@ -395,9 +402,16 @@ try { nativePath: getStepAuthPath(), legacyPath: getLegacyStepAuthPath(), }); - if (report.removedNative || report.removedLegacy) { + if (report.removedLoginState) { process.stdout.write("Signed out of Step.\n"); process.stdout.write(`Removed: ${report.nativePath}\n`); + } else { + process.stderr.write( + "Note: the account session could not be removed and is still on disk. Run `step logout` again.\n", + ); + } + if (report.removedNative || report.removedLegacy) { + process.stdout.write(`Removed: ${report.nativePath}\n`); } else { process.stdout.write(`No stored Step credential at ${report.nativePath}\n`); } diff --git a/packages/coding-agent/src/step/auth.ts b/packages/coding-agent/src/step/auth.ts index c815127f..45728e99 100644 --- a/packages/coding-agent/src/step/auth.ts +++ b/packages/coding-agent/src/step/auth.ts @@ -126,7 +126,7 @@ export async function migrateLegacyStepCredential( export interface StepLogoutResult { removedNative: boolean; removedLegacy: boolean; - /** True when a stored account session was removed. */ + /** True when no account session remains on disk. */ removedLoginState: boolean; nativePath: string; legacyPath: string; @@ -142,11 +142,6 @@ export async function logoutStepCredentials( const env = options.env ?? process.env; const nativePath = normalize(options.nativePath ?? getStepAuthPath()); const legacyPath = normalize(options.legacyPath ?? getLegacyStepAuthPath()); - // The account session is a separate file with a separate lifetime, so it is - // cleared here too — but only the session, never anything else on disk. - // The result is the actual outcome: reporting a removal that did not happen - // would leave a long-lived refresh token behind while claiming success. - const removedLoginState = await clearStepLoginState(env); let removedNative = false; let removedLegacy = false; @@ -181,6 +176,12 @@ export async function logoutStepCredentials( } } + // The account session is cleared last, and its failure never stops the API key + // from being removed. Whether anything is left on disk is reported, because a + // sign-out that claims success while a long-lived refresh token survives is + // worse than one that says otherwise. + const removedLoginState = await clearStepLoginState(env).catch(() => false); + return { removedNative, removedLegacy, diff --git a/packages/coding-agent/src/step/login-flow.ts b/packages/coding-agent/src/step/login-flow.ts index eec578bf..b83caaf3 100644 --- a/packages/coding-agent/src/step/login-flow.ts +++ b/packages/coding-agent/src/step/login-flow.ts @@ -401,17 +401,19 @@ export async function runStepLogin(options: RunStepLoginOptions = {}): Promise, expectedRefreshToken: string): Promise { + try { + return await discardLoginState(env, expectedRefreshToken); + } catch { + return false; + } +} + /** * Delete the session, but only while it is still the one that was examined. * @@ -303,7 +318,9 @@ export async function getStepLoginToken(options: StepLoginTokenOptions = {}): Pr // A refresh token past its own expiry cannot be renewed at all, so there is // nothing to wait for: drop it now rather than let every caller rediscover it. if (isRefreshTokenExpired(current, Date.now())) { - await discardLoginState(env, current.refreshToken); + // Whether the file could actually be removed, the session is finished: the + // caller gets an explicit outcome instead of an exception. + await discardQuietly(env, current.refreshToken); return { status: "expired", reason: "the account session has expired" }; } if (!needsRefresh(current, Date.now())) return ready(current); @@ -416,7 +433,7 @@ async function refreshAndCommit( // The session is positively dead. Whether it could be removed decides // what is left on disk — but a session that was *replaced* while the // request ran must survive, so that case is reported as a no-op. - const removed = await discardLoginState(env, before.refreshToken); + const removed = await discardQuietly(env, before.refreshToken); if (!removed) { const replacement = readStepLoginState(env); if (replacement && replacement.refreshToken !== before.refreshToken) { diff --git a/packages/coding-agent/test/step-login-state.test.ts b/packages/coding-agent/test/step-login-state.test.ts index fe41573a..cafa90d6 100644 --- a/packages/coding-agent/test/step-login-state.test.ts +++ b/packages/coding-agent/test/step-login-state.test.ts @@ -2,6 +2,7 @@ import { mkdtempSync, rmSync, statSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import * as settingsManager from "../src/core/settings-manager.ts"; import { acquireSettingsLockSync } from "../src/core/settings-manager.ts"; import type { StepCliLoginState } from "../src/features/step-provider/index.ts"; import { @@ -465,6 +466,29 @@ describe("getStepLoginToken", () => { expect(readStepLoginState(env)).toBeDefined(); }); + it("reports expired even when removing the dead session fails outright", async () => { + // A filesystem failure while discarding must not surface as a transient + // error: the fallback would hand out a token the server just rejected. + await saveStepLoginState( + stored({ accessExpiresAt: Math.floor(Date.now() / 1000) + 60, accessToken: accessToken(60) }), + env, + ); + vi.spyOn(settingsManager, "acquireSettingsLockSync").mockImplementation(() => { + throw Object.assign(new Error("permission denied"), { code: "EACCES" }); + }); + try { + vi.stubGlobal( + "fetch", + vi.fn(async () => + ok({ accessToken: { raw: "anon", duration: 1800, mode: 1 }, refreshToken: { raw: refreshToken() } }), + ), + ); + expect(await getStepLoginToken({ env })).toMatchObject({ status: "expired" }); + } finally { + vi.restoreAllMocks(); + } + }); + it("reports expired rather than ready when a dead session cannot be removed", async () => { await saveStepLoginState( stored({ accessExpiresAt: Math.floor(Date.now() / 1000) + 60, accessToken: accessToken(60) }), diff --git a/packages/coding-agent/test/step-plan-login.test.ts b/packages/coding-agent/test/step-plan-login.test.ts index c40b665d..a0cdb7fa 100644 --- a/packages/coding-agent/test/step-plan-login.test.ts +++ b/packages/coding-agent/test/step-plan-login.test.ts @@ -400,7 +400,7 @@ describe("Step Plan login availability", () => { view.handleInput("1"); await expect(login).resolves.toMatchObject({ kind: "completed", - warning: expect.stringContaining("account session was rejected"), + warning: expect.stringContaining("server rejected the account session"), }); await expect(readFile(loginStatePath, "utf8")).rejects.toThrow(); expect(readStepLoginCredential(authPath)).toMatchObject({ type: "oauth", access: "plan-key" }); From 4c7ed8250bde602a4d45eb7672f05e04f9bd4529 Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Mon, 19 Oct 2026 18:08:06 +0800 Subject: [PATCH 6/6] fix(step): report the account session without disturbing the surface it runs on Two of the previous round's fixes reported the session in the wrong place. `step logout` decided whether to print "Signed out" from the session result instead of the credential result, so a user who had never signed in saw a sign-out confirmation and the credential path printed twice. The credential result decides that message again; a session that survived is a separate note. The TUI path wrote that note to stderr, which lands underneath the running terminal and is then cleared by shutdown. The logout result now carries whether the session was removed, and the host shows it as a warning like every other message it presents. The sign-in warning conflated two different failures. A session that could not be written and a previous session that could not be cleared are separate problems with different consequences, and reporting one as the other sent the user looking for the wrong thing. They are reported independently now, and the reason the server rejected a session survives alongside either of them. --- apps/cli/src/main.ts | 20 ++--- apps/cli/src/ui/interactive-mode.ts | 3 + .../src/modes/interactive-contract.ts | 7 +- packages/coding-agent/src/step/login-flow.ts | 24 ++++-- packages/coding-agent/test/step-auth.test.ts | 73 ++++++++++++++++- .../coding-agent/test/step-plan-login.test.ts | 80 +++++++++++++++++++ 6 files changed, 184 insertions(+), 23 deletions(-) diff --git a/apps/cli/src/main.ts b/apps/cli/src/main.ts index 80e3cef7..afef34e7 100644 --- a/apps/cli/src/main.ts +++ b/apps/cli/src/main.ts @@ -282,15 +282,11 @@ const stepMainOptions: MainOptions = { legacyPath: getLegacyStepAuthPath(), }); syncStepLoginProfileEndpoint(getStepAuthPath()); - if (!report.removedLoginState) { - // The API key is gone either way; only the account session survives, - // and it holds a long-lived refresh token. - process.stderr.write( - "Note: the account session could not be removed and is still on disk. Run `step logout` again.\n", - ); - } return { removed: report.removedNative || report.removedLegacy, + // The terminal belongs to the TUI here, so the host reports this + // instead of writing to stderr underneath it. + accountSessionRemoved: report.removedLoginState, remainingSource: report.remainingSource ? "the environment (STEP_API_KEY)" : null, }; }, @@ -402,19 +398,17 @@ try { nativePath: getStepAuthPath(), legacyPath: getLegacyStepAuthPath(), }); - if (report.removedLoginState) { + if (report.removedNative || report.removedLegacy) { process.stdout.write("Signed out of Step.\n"); process.stdout.write(`Removed: ${report.nativePath}\n`); } else { + process.stdout.write(`No stored Step credential at ${report.nativePath}\n`); + } + if (!report.removedLoginState) { process.stderr.write( "Note: the account session could not be removed and is still on disk. Run `step logout` again.\n", ); } - if (report.removedNative || report.removedLegacy) { - process.stdout.write(`Removed: ${report.nativePath}\n`); - } else { - process.stdout.write(`No stored Step credential at ${report.nativePath}\n`); - } if (report.remainingSource) { process.stderr.write("Note: STEP_API_KEY is still set, so requests will keep working.\n"); } else { diff --git a/apps/cli/src/ui/interactive-mode.ts b/apps/cli/src/ui/interactive-mode.ts index 9b8fd289..b04f9f3b 100644 --- a/apps/cli/src/ui/interactive-mode.ts +++ b/apps/cli/src/ui/interactive-mode.ts @@ -5737,6 +5737,9 @@ export class InteractiveMode { const report = await this.options.stepLogout?.(); if (!report) return; this.showStatus(report.removed ? "Signed out of Step." : "No stored Step credential."); + if (report.accountSessionRemoved === false) { + this.showWarning("The account session could not be removed and is still on disk. Run /logout again."); + } if (report.remainingSource) { this.showWarning(`An API key is still active from ${report.remainingSource}.`); } diff --git a/packages/coding-agent/src/modes/interactive-contract.ts b/packages/coding-agent/src/modes/interactive-contract.ts index b4b0a460..a106e450 100644 --- a/packages/coding-agent/src/modes/interactive-contract.ts +++ b/packages/coding-agent/src/modes/interactive-contract.ts @@ -75,7 +75,12 @@ export interface InteractiveModeOptions { /** Shared Step onboarding flow used by the Step-only login surfaces. */ stepLogin?: (host: StepLoginHost) => Promise; /** Shared Step logout flow used by the Step-only TUI command. */ - stepLogout?: () => Promise<{ removed: boolean; remainingSource?: string | null }>; + stepLogout?: () => Promise<{ + removed: boolean; + remainingSource?: string | null; + /** False when an account session could not be removed and is still on disk. */ + accountSessionRemoved?: boolean; + }>; /** * One-time offer to migrate another agent's MCP servers. * diff --git a/packages/coding-agent/src/step/login-flow.ts b/packages/coding-agent/src/step/login-flow.ts index b83caaf3..c84db8ae 100644 --- a/packages/coding-agent/src/step/login-flow.ts +++ b/packages/coding-agent/src/step/login-flow.ts @@ -399,25 +399,33 @@ export async function runStepLogin(options: RunStepLoginOptions = {}): Promise { openai: { type: "api_key", key: "unrelated" }, }); }); + + describe("account session on logout", () => { + async function signedInFixture() { + const directory = await mkdtemp(join(tmpdir(), "step-auth-session-")); + temporaryDirectories.push(directory); + const nativePath = join(directory, "auth.json"); + const legacyPath = join(directory, "legacy-auth.json"); + const env = { STEPCODE_LOGIN_STATE_PATH: join(directory, "step-login.json") }; + await AuthStorage.create(nativePath).modify(STEP_PROVIDER_ID, async () => ({ + type: "oauth", + access: "access", + refresh: STEP_STATIC_REFRESH_TOKEN, + expires: Number.MAX_SAFE_INTEGER, + })); + await saveStepLoginState( + { + version: 1, + profile: "step_plan", + appId: "10300", + deviceId: "device-1", + accessToken: "access-jwt", + refreshToken: "refresh-jwt", + accessExpiresAt: Math.floor(Date.now() / 1000) + 1800, + }, + env, + ); + return { nativePath, legacyPath, env }; + } + + test("removes the session together with the api key", async () => { + const { nativePath, legacyPath, env } = await signedInFixture(); + + const result = await logoutStepCredentials({ nativePath, legacyPath, env }); + + expect(result).toMatchObject({ removedNative: true, removedLoginState: true }); + expect(readStepLoginState(env)).toBeUndefined(); + }); + + test("still removes the api key and reports a session held by another process", async () => { + const { nativePath, legacyPath, env } = await signedInFixture(); + const release = acquireSettingsLockSync(getStepLoginStatePath(env)); + try { + const result = await logoutStepCredentials({ nativePath, legacyPath, env }); + + expect(result).toMatchObject({ removedNative: true, removedLoginState: false }); + expect(readStepLoginState(env)).toBeDefined(); + } finally { + release(); + } + expect(await AuthStorage.create(nativePath).read(STEP_PROVIDER_ID)).toBeUndefined(); + }); + + test("still removes the api key when the session cannot be locked at all", async () => { + const { nativePath, legacyPath, env } = await signedInFixture(); + vi.spyOn(settingsManager, "acquireSettingsLockSync").mockImplementation(() => { + throw Object.assign(new Error("permission denied"), { code: "EACCES" }); + }); + try { + const result = await logoutStepCredentials({ nativePath, legacyPath, env }); + + expect(result).toMatchObject({ removedNative: true, removedLoginState: false }); + expect(readStepLoginState(env)).toBeDefined(); + } finally { + vi.restoreAllMocks(); + } + expect(await AuthStorage.create(nativePath).read(STEP_PROVIDER_ID)).toBeUndefined(); + }); + }); }); diff --git a/packages/coding-agent/test/step-plan-login.test.ts b/packages/coding-agent/test/step-plan-login.test.ts index a0cdb7fa..e3a5635b 100644 --- a/packages/coding-agent/test/step-plan-login.test.ts +++ b/packages/coding-agent/test/step-plan-login.test.ts @@ -2,6 +2,8 @@ import { mkdtemp, readFile, rm } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import * as settingsManager from "../src/core/settings-manager.ts"; +import { acquireSettingsLockSync } from "../src/core/settings-manager.ts"; import { loginStepOAuth } from "../src/features/step-provider/index.ts"; import { checkStepPlanBeforeInteractive, @@ -9,6 +11,7 @@ import { runStepLogin, writeStepLoginCredential, } from "../src/step/login-flow.ts"; +import { readStepLoginState, saveStepLoginState } from "../src/step/login-state.ts"; import type { StepLoginProfileId } from "../src/step/onboarding.ts"; import type { StepOnboardingView } from "../src/step/onboarding-view.ts"; import { PlanUsageController, resolvePlanUsageAccount } from "../src/step/usage/account.ts"; @@ -409,4 +412,81 @@ describe("Step Plan login availability", () => { await login; } }); + + it("completes the sign-in with a warning when the account session cannot be locked at all", async () => { + // Regression: a non-contention lock failure (read-only directory, + // permissions) used to escape `persist` and report the whole sign-in as + // "Could not save credentials" although the API key was already written. + const loginStatePath = join(root, "step-login.json"); + oauthResult.current = { + access: "plan-key", + refresh: "static", + expires: Number.MAX_SAFE_INTEGER, + uid: "123", + login: { accessToken: "access-jwt", refreshToken: "refresh-jwt", deviceId: "device-1", appId: "10300" }, + loginError: "ignored when a session is present but not stored", + }; + vi.spyOn(settingsManager, "acquireSettingsLockSync").mockImplementation(() => { + throw Object.assign(new Error("permission denied"), { code: "EACCES" }); + }); + const { login, view } = start( + controller(vi.fn(async () => Response.json(snapshot("coding")))), + undefined, + { STEPCODE_LOGIN_STATE_PATH: loginStatePath }, + ); + try { + view.handleInput("1"); + const outcome = await login; + expect(outcome.kind).toBe("completed"); + expect(outcome.warning).toContain("could not be saved: permission denied"); + // The server's reason is kept next to the local failure, not replaced by it. + expect(outcome.warning).toContain("The server rejected the account session"); + expect(readStepLoginCredential(authPath)).toMatchObject({ type: "oauth", access: "plan-key" }); + } finally { + view.handleInput("\x03"); + await login; + } + }); + + it("warns that the previous session survives when another process holds it", async () => { + const loginStatePath = join(root, "step-login.json"); + const env = { STEPCODE_LOGIN_STATE_PATH: loginStatePath }; + await saveStepLoginState( + { + version: 1, + profile: "step_plan", + appId: "10300", + deviceId: "old-device", + accessToken: "old-access", + refreshToken: "old-refresh", + accessExpiresAt: Math.floor(Date.now() / 1000) + 1800, + }, + env, + ); + oauthResult.current = { + access: "plan-key", + refresh: "static", + expires: Number.MAX_SAFE_INTEGER, + uid: "123", + login: { accessToken: "access-jwt", refreshToken: "refresh-jwt", deviceId: "device-1", appId: "10300" }, + }; + const release = acquireSettingsLockSync(loginStatePath); + const { login, view } = start( + controller(vi.fn(async () => Response.json(snapshot("coding")))), + undefined, + env, + ); + try { + view.handleInput("1"); + const outcome = await login; + expect(outcome.kind).toBe("completed"); + expect(outcome.warning).toContain("could not be written"); + expect(outcome.warning).toContain("previous account session may still be on disk"); + expect(readStepLoginState(env)?.refreshToken).toBe("old-refresh"); + } finally { + release(); + view.handleInput("\x03"); + await login; + } + }); });