From 3c3741b565bb60c9a4450fb8b13201aa94cb3728 Mon Sep 17 00:00:00 2001 From: Federico Jaramillo Martinez Date: Sat, 18 Jul 2026 07:48:27 +0200 Subject: [PATCH] fix(auth): reconcile committed OAuth cancellation --- .changeset/fix-pi-0-80-8-modelruntime-auth.md | 2 +- src/server/sessions/authService.test.ts | 58 +++++++++++++++++-- src/server/sessions/authService.ts | 15 ++++- .../sessions/oauthLoginFlowService.test.ts | 24 ++++++++ src/server/sessions/oauthLoginFlowService.ts | 58 +++++++++++++++---- 5 files changed, 137 insertions(+), 20 deletions(-) diff --git a/.changeset/fix-pi-0-80-8-modelruntime-auth.md b/.changeset/fix-pi-0-80-8-modelruntime-auth.md index 0e25b42..822b379 100644 --- a/.changeset/fix-pi-0-80-8-modelruntime-auth.md +++ b/.changeset/fix-pi-0-80-8-modelruntime-auth.md @@ -2,4 +2,4 @@ "@jmfederico/pi-web": patch --- -Restore session-daemon startup and authentication on supported Pi `>=0.80.8 <0.81` releases by migrating model and credential handling to `ModelRuntime`. Login options now follow each provider's interactive API-key and OAuth capabilities, OAuth prompts retain their input, selection, and device-code semantics, and unsupported multi-step API-key setup fails safely instead of storing malformed credentials. PI WEB now requires Node.js `>=22.19.0`. +Restore session-daemon startup and authentication on supported Pi `>=0.80.8 <0.81` releases by migrating model and credential handling to `ModelRuntime`. Login options now follow each provider's interactive API-key and OAuth capabilities, OAuth prompts retain their input, selection, and device-code semantics, committed OAuth login remains truthful when cancellation races the final refresh, and unsupported multi-step API-key setup fails safely instead of storing malformed credentials. PI WEB now requires Node.js `>=22.19.0`. diff --git a/src/server/sessions/authService.test.ts b/src/server/sessions/authService.test.ts index bbccade..d3fb5ae 100644 --- a/src/server/sessions/authService.test.ts +++ b/src/server/sessions/authService.test.ts @@ -41,8 +41,9 @@ describe("AuthService", () => { auth.dispose(); }); - it("persists an API key and attempts every listener when propagation fails", async () => { - const error = vi.fn(); + it("persists an API key and attempts every listener when failure logging throws", async () => { + const loggingFailure = new Error("auth logger failed"); + const error = vi.fn(() => { throw loggingFailure; }); const logger: AuthServiceLogger = { error }; const { auth, credentials, changes } = await createAuthService({}, logger); const failure = new Error("session auth refresh failed"); @@ -260,6 +261,44 @@ describe("AuthService", () => { auth.dispose(); }); + it("reconciles cancellation after ModelRuntime persists OAuth but before its refresh completes", async () => { + const { auth, runtime, credentials, changes } = await createAuthService(); + const provider = runtime.getProviders().find((option) => option.id === "anthropic" && option.auth.oauth !== undefined); + if (provider?.auth.oauth === undefined) throw new Error("Expected built-in OAuth provider"); + const credential: Credential = { + type: "oauth", + refresh: "refresh-token", + access: "access-token", + expires: Date.now() + 60_000, + }; + vi.spyOn(provider.auth.oauth, "login").mockResolvedValue(credential); + vi.spyOn(runtime, "reloadConfig").mockResolvedValue(undefined); + const refreshStarted = deferred(); + const finishRefresh = deferred(); + const refresh = vi.spyOn(runtime, "refresh").mockImplementation(async () => { + refreshStarted.resolve(undefined); + await finishRefresh.promise; + return { aborted: false, errors: new Map() }; + }); + + const state = await auth.startOAuthLogin(provider.id); + await refreshStarted.promise; + + await expect(credentials.read(provider.id)).resolves.toEqual(credential); + expect(auth.cancelOAuthFlow(state.flowId)).toMatchObject({ status: "cancelled", error: "Login cancelled" }); + expect(changes).toEqual([]); + + finishRefresh.resolve(undefined); + await vi.waitFor(() => { expect(auth.oauthFlow(state.flowId).status).toBe("complete"); }); + + expect(auth.oauthFlow(state.flowId)).toMatchObject({ status: "complete", progress: ["Login complete"] }); + expect(auth.oauthFlow(state.flowId)).not.toHaveProperty("error"); + await expect(credentials.read(provider.id)).resolves.toEqual(credential); + expect(changes).toEqual([{}]); + expect(refresh).toHaveBeenCalledOnce(); + auth.dispose(); + }); + it("emits an auth change after OAuth login completes without refreshing twice", async () => { const runtime = await ModelRuntime.create({ credentials: new InMemoryCredentialStore(), @@ -293,8 +332,9 @@ describe("AuthService", () => { expect(authFlows.disposed).toBe(true); }); - it("completes OAuth when an auth-change listener rejects", async () => { - const error = vi.fn(); + it("completes OAuth when an auth-change listener and failure logging throw", async () => { + const loggingFailure = new Error("auth logger failed"); + const error = vi.fn(() => { throw loggingFailure; }); const logger: AuthServiceLogger = { error }; const { auth, runtime, changes } = await createAuthService({}, logger); const provider = runtime.getProviders().find((option) => option.id === "anthropic" && option.auth.oauth !== undefined); @@ -353,6 +393,16 @@ async function tempAgentDir(): Promise { return dir; } +function deferred() { + let resolveValue: (value: T) => void = () => undefined; + let rejectValue: (reason?: unknown) => void = () => undefined; + const promise = new Promise((resolve, reject) => { + resolveValue = resolve; + rejectValue = reject; + }); + return { promise, resolve: resolveValue, reject: rejectValue }; +} + function radiusModelsConfig(name: string): string { return JSON.stringify({ providers: { diff --git a/src/server/sessions/authService.ts b/src/server/sessions/authService.ts index cd939e0..4e26c04 100644 --- a/src/server/sessions/authService.ts +++ b/src/server/sessions/authService.ts @@ -53,8 +53,9 @@ export class AuthService { static async create(deps: AuthServiceDependencies = {}): Promise { const runtime = deps.runtime ?? (deps.agentDir === undefined ? await ModelRuntime.create({}) : await createModelRuntimeForAgentDir(deps.agentDir)); - const authFlows = deps.authFlows ?? new OAuthLoginFlowService(); - return new AuthService(runtime, authFlows, deps.logger ?? noopLogger); + const logger = deps.logger ?? noopLogger; + const authFlows = deps.authFlows ?? new OAuthLoginFlowService({ logger }); + return new AuthService(runtime, authFlows, logger); } subscribe(listener: AuthChangeListener): () => void { @@ -130,11 +131,19 @@ export class AuthService { const results = await Promise.allSettled([...this.listeners].map(async (listener) => listener(change))); for (const result of results) { if (result.status === "rejected") { - this.logger.error({ err: result.reason, ...context }, "auth-change listener failed"); + this.logErrorNoThrow({ err: result.reason, ...context }, "auth-change listener failed"); } } } + private logErrorNoThrow(details: Record, message: string): void { + try { + this.logger.error(details, message); + } catch { + // A diagnostic failure cannot turn an already-committed auth mutation into an API failure. + } + } + private async requireApiKeyLoginProvider(providerId: string) { await this.runtime.reloadConfig(); const provider = getLoginProviderOptions(this.runtime, "api_key").find((option) => option.id === providerId); diff --git a/src/server/sessions/oauthLoginFlowService.test.ts b/src/server/sessions/oauthLoginFlowService.test.ts index d42fbe8..90c0500 100644 --- a/src/server/sessions/oauthLoginFlowService.test.ts +++ b/src/server/sessions/oauthLoginFlowService.test.ts @@ -60,6 +60,30 @@ describe("OAuthLoginFlowService", () => { service.dispose(); }); + it("keeps a committed login complete when its completion callback and logger throw", async () => { + const completionFailure = new Error("completion propagation failed"); + const loggingFailure = new Error("OAuth logger failed"); + const error = vi.fn(() => { throw loggingFailure; }); + const onComplete = vi.fn(() => { throw completionFailure; }); + const service = new OAuthLoginFlowService({ logger: { error } }); + const state = service.start({ + providerId: "test-provider", + providerName: "Test Provider", + runtime: fakeRuntime(() => Promise.resolve()), + onComplete, + }); + + await vi.waitFor(() => { expect(service.get(state.flowId).status).toBe("complete"); }); + + expect(service.get(state.flowId)).toMatchObject({ status: "complete", progress: ["Login complete"] }); + expect(onComplete).toHaveBeenCalledOnce(); + expect(error).toHaveBeenCalledWith( + { err: completionFailure, flowId: state.flowId, providerId: "test-provider" }, + "OAuth login completion callback failed", + ); + service.dispose(); + }); + it("allows blank text responses for providers that use blank as a default", async () => { let domain: string | undefined; const service = new OAuthLoginFlowService(); diff --git a/src/server/sessions/oauthLoginFlowService.ts b/src/server/sessions/oauthLoginFlowService.ts index 214f694..d38cf18 100644 --- a/src/server/sessions/oauthLoginFlowService.ts +++ b/src/server/sessions/oauthLoginFlowService.ts @@ -27,25 +27,33 @@ interface OAuthFlowRecord { cleanupTimer?: TimerHandle; } +export interface OAuthLoginFlowLogger { + error(details: Record, message: string): void; +} + export interface OAuthLoginFlowServiceOptions { terminalTtlMs?: number; runningTtlMs?: number; now?: () => number; + logger?: OAuthLoginFlowLogger; } const DEFAULT_TERMINAL_TTL_MS = 5 * 60 * 1000; const DEFAULT_RUNNING_TTL_MS = 30 * 60 * 1000; +const noopLogger: OAuthLoginFlowLogger = { error() { /* no-op */ } }; export class OAuthLoginFlowService { private readonly flows = new Map(); private readonly terminalTtlMs: number; private readonly runningTtlMs: number; private readonly now: () => number; + private readonly logger: OAuthLoginFlowLogger; constructor(options: OAuthLoginFlowServiceOptions = {}) { this.terminalTtlMs = options.terminalTtlMs ?? DEFAULT_TERMINAL_TTL_MS; this.runningTtlMs = options.runningTtlMs ?? DEFAULT_RUNNING_TTL_MS; this.now = options.now ?? (() => Date.now()); + this.logger = options.logger ?? noopLogger; } start(options: { @@ -80,20 +88,15 @@ export class OAuthLoginFlowService { notify: (event) => { this.handleEvent(record, event); }, }; - void options.runtime.login(options.providerId, "oauth", interaction) - .then(async () => { - if (!this.isCurrentRunning(record)) return; - this.clearPending(record); - await options.onComplete?.(); - if (!this.isCurrentRunning(record)) return; - this.markTerminal(record, { ...withoutInteraction(record.state), status: "complete", progress: [...record.state.progress, "Login complete"] }); - }) - .catch((error: unknown) => { - if (this.flows.get(record.flowId) !== record) return; + void options.runtime.login(options.providerId, "oauth", interaction).then( + () => this.reconcileCommittedLogin(record, options.onComplete), + (error: unknown) => { + if (!this.isCurrent(record)) return; this.clearPending(record); if (record.state.status !== "running") return; this.markTerminal(record, { ...withoutInteraction(record.state), status: "error", error: error instanceof Error ? error.message : String(error) }); - }); + }, + ); return this.get(flowId); } @@ -274,8 +277,39 @@ export class OAuthLoginFlowService { return pending; } + // ModelRuntime persists the credential before its post-login refresh. If a + // cancellation lands during that refresh, the resolved login is committed + // truth and must supersede the transient cancelled state. + private async reconcileCommittedLogin(record: OAuthFlowRecord, onComplete?: () => void | Promise): Promise { + if (this.isCurrent(record)) this.clearPending(record); + try { + await onComplete?.(); + } catch (error) { + this.logErrorNoThrow( + { err: error, flowId: record.flowId, providerId: record.state.providerId }, + "OAuth login completion callback failed", + ); + } + if (!this.isCurrent(record)) return; + const completed = withoutInteraction(record.state); + delete completed.error; + this.markTerminal(record, { ...completed, status: "complete", progress: [...record.state.progress, "Login complete"] }); + } + + private isCurrent(record: OAuthFlowRecord): boolean { + return this.flows.get(record.flowId) === record; + } + private isCurrentRunning(record: OAuthFlowRecord): boolean { - return this.flows.get(record.flowId) === record && record.state.status === "running"; + return this.isCurrent(record) && record.state.status === "running"; + } + + private logErrorNoThrow(details: Record, message: string): void { + try { + this.logger.error(details, message); + } catch { + // Logging is post-commit diagnostics and must never change auth truth. + } } private updateState(record: OAuthFlowRecord, state: OAuthFlowState): void {