From a39cf49f3a0d2ed9a3b7c4b125e8d99275d3c643 Mon Sep 17 00:00:00 2001 From: Federico Jaramillo Martinez Date: Fri, 17 Jul 2026 23:18:58 +0200 Subject: [PATCH] fix(auth): prevent API key reuse across login prompts --- .../sessions/authProviderOptions.test.ts | 15 +- src/server/sessions/authProviderOptions.ts | 4 +- src/server/sessions/authService.test.ts | 148 +++++++++++++++++- src/server/sessions/authService.ts | 29 +++- 4 files changed, 180 insertions(+), 16 deletions(-) diff --git a/src/server/sessions/authProviderOptions.test.ts b/src/server/sessions/authProviderOptions.test.ts index 7ed820f..b7d5112 100644 --- a/src/server/sessions/authProviderOptions.test.ts +++ b/src/server/sessions/authProviderOptions.test.ts @@ -4,13 +4,15 @@ import { getLoginProviderOptions, getLogoutProviderOptions, type AuthProviderRun function runtime(): AuthProviderRuntime { const credentials = [{ providerId: "openai", type: "api_key" as const }]; // Auth shapes mirror what the Pi SDK actually reports for these providers: - // github-copilot supports both methods, openai-codex is oauth-only. + // github-copilot supports both methods, openai-codex is OAuth-only, and + // ambient providers resolve credentials without offering interactive login. const providers = [ - { id: "anthropic", name: "Anthropic", auth: { oauth: {}, apiKey: {} } }, - { id: "github-copilot", name: "GitHub Copilot", auth: { oauth: {}, apiKey: {} } }, + { id: "anthropic", name: "Anthropic", auth: { oauth: {}, apiKey: { login: () => undefined } } }, + { id: "github-copilot", name: "GitHub Copilot", auth: { oauth: {}, apiKey: { login: () => undefined } } }, { id: "openai-codex", name: "ChatGPT Plus/Pro (Codex Subscription)", auth: { oauth: {} } }, - { id: "openai", name: "OpenAI", auth: { apiKey: {} } }, - { id: "custom", name: "Custom", auth: { apiKey: {} } }, + { id: "openai", name: "OpenAI", auth: { apiKey: { login: () => undefined } } }, + { id: "custom", name: "Custom", auth: { apiKey: { login: () => undefined } } }, + { id: "ambient", name: "Ambient credentials", auth: { apiKey: {} } }, ]; return { getProviders: () => providers, @@ -20,7 +22,7 @@ function runtime(): AuthProviderRuntime { } describe("auth provider options", () => { - it("offers both api-key and oauth login options for every provider the backend supports each method for", () => { + it("offers each interactive login method reported by the backend", () => { const options = getLoginProviderOptions(runtime()); expect(options).toEqual(expect.arrayContaining([ // Dual-capable providers surface both login methods, driven purely by SDK data. @@ -36,6 +38,7 @@ describe("auth provider options", () => { ])); expect(options).not.toEqual(expect.arrayContaining([expect.objectContaining({ id: "openai-codex", authType: "api_key" })])); expect(options).not.toEqual(expect.arrayContaining([expect.objectContaining({ id: "openai", authType: "oauth" })])); + expect(options).not.toEqual(expect.arrayContaining([expect.objectContaining({ id: "ambient", authType: "api_key" })])); }); it("returns only currently stored credentials for logout", async () => { diff --git a/src/server/sessions/authProviderOptions.ts b/src/server/sessions/authProviderOptions.ts index 58409c6..529ae90 100644 --- a/src/server/sessions/authProviderOptions.ts +++ b/src/server/sessions/authProviderOptions.ts @@ -4,7 +4,7 @@ import type { AuthProviderOption, AuthProviderStatus, AuthType } from "../../sha interface AuthProviderInfo { id: string; name: string; - auth: { apiKey?: unknown; oauth?: unknown }; + auth: { apiKey?: { login?: unknown }; oauth?: unknown }; } /** Non-secret stored-credential metadata, keyed by provider id. */ @@ -40,7 +40,7 @@ export function getLoginProviderOptions(runtime: AuthProviderRuntime, authType?: } for (const provider of providers) { - if (provider.auth.apiKey === undefined) continue; + if (provider.auth.apiKey?.login === undefined) continue; options.push({ id: provider.id, name: provider.name, diff --git a/src/server/sessions/authService.test.ts b/src/server/sessions/authService.test.ts index 64fce77..0e49ece 100644 --- a/src/server/sessions/authService.test.ts +++ b/src/server/sessions/authService.test.ts @@ -2,7 +2,7 @@ import { mkdtemp, readFile, rm } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { ModelRuntime } from "@earendil-works/pi-coding-agent"; -import { InMemoryCredentialStore, type Credential } from "@earendil-works/pi-ai"; +import { InMemoryCredentialStore, type AuthPrompt, type Credential } from "@earendil-works/pi-ai"; import { afterEach, describe, expect, it, vi } from "vitest"; import type { OAuthFlowState } from "../../shared/apiTypes.js"; import { AuthService, type AuthChange } from "./authService.js"; @@ -43,6 +43,127 @@ describe("AuthService", () => { auth.dispose(); }); + it("rejects Cloudflare multi-field setup without storing the secret as provider metadata", async () => { + const { auth, credentials, changes } = await createAuthService(); + + await expect(auth.saveApiKey("cloudflare-ai-gateway", "cf-secret")).rejects.toThrow( + "Cloudflare AI Gateway requires interactive setup; use Pi's generic /login flow", + ); + + await expect(credentials.read("cloudflare-ai-gateway")).resolves.toBeUndefined(); + expect(changes).toEqual([]); + auth.dispose(); + }); + + it.each([ + { providerId: "amazon-bedrock", providerName: "Amazon Bedrock" }, + { providerId: "google-vertex", providerName: "Google Vertex AI" }, + ])("rejects $providerName select-first setup without storing the secret", async ({ providerId, providerName }) => { + const { auth, credentials, changes } = await createAuthService(); + + await expect(auth.saveApiKey(providerId, "submitted-secret")).rejects.toThrow( + `${providerName} requires interactive setup; use Pi's generic /login flow`, + ); + + await expect(credentials.read(providerId)).resolves.toBeUndefined(); + expect(changes).toEqual([]); + auth.dispose(); + }); + + it.each([ + { label: "text", prompt: { type: "text", message: "Account" } satisfies AuthPrompt }, + { + label: "select", + prompt: { type: "select", message: "Region", options: [{ id: "us", label: "US" }] } satisfies AuthPrompt, + }, + { label: "manual-code", prompt: { type: "manual_code", message: "Code" } satisfies AuthPrompt }, + ])("rejects a first $label prompt before credential persistence", async ({ prompt }) => { + const { auth, runtime, credentials, changes } = await createAuthService(); + const login = mockLoginPromptsBeforePersistence(runtime, credentials, [prompt]); + + await expect(auth.saveApiKey("anthropic", "sk-test")).rejects.toThrow( + "Anthropic requires interactive setup; use Pi's generic /login flow", + ); + + expect(login).toHaveBeenCalledOnce(); + await expect(credentials.read("anthropic")).resolves.toBeUndefined(); + expect(changes).toEqual([]); + auth.dispose(); + }); + + it("rejects a repeated secret prompt before credential persistence", async () => { + const { auth, runtime, credentials, changes } = await createAuthService(); + const login = mockLoginPromptsBeforePersistence(runtime, credentials, [ + { type: "secret", message: "API key" }, + { type: "secret", message: "API key again" }, + ]); + + await expect(auth.saveApiKey("anthropic", "sk-test")).rejects.toThrow( + "Anthropic requires interactive setup; use Pi's generic /login flow", + ); + + expect(login).toHaveBeenCalledOnce(); + await expect(credentials.read("anthropic")).resolves.toBeUndefined(); + expect(changes).toEqual([]); + auth.dispose(); + }); + + it("rejects an aborted secret prompt before credential persistence", async () => { + const { auth, runtime, credentials, changes } = await createAuthService(); + const abort = new AbortController(); + abort.abort(); + const login = mockLoginPromptsBeforePersistence(runtime, credentials, [ + { type: "secret", message: "API key", signal: abort.signal }, + ]); + + await expect(auth.saveApiKey("anthropic", "sk-test")).rejects.toThrow("Login cancelled"); + + expect(login).toHaveBeenCalledOnce(); + await expect(credentials.read("anthropic")).resolves.toBeUndefined(); + expect(changes).toEqual([]); + auth.dispose(); + }); + + it("rejects unknown providers before starting API-key login", async () => { + const { auth, runtime, credentials, changes } = await createAuthService(); + const login = vi.spyOn(runtime, "login"); + + await expect(auth.saveApiKey("unknown-provider", "sk-test")).rejects.toThrow( + "API key provider not found: unknown-provider", + ); + + expect(login).not.toHaveBeenCalled(); + await expect(credentials.read("unknown-provider")).resolves.toBeUndefined(); + expect(changes).toEqual([]); + auth.dispose(); + }); + + it("rejects ambient-only providers before starting API-key login", async () => { + const { auth, runtime, credentials, changes } = await createAuthService(); + const providers = [...runtime.getProviders()]; + const interactiveProvider = providers.find((provider) => provider.auth.apiKey?.login !== undefined); + if (interactiveProvider?.auth.apiKey === undefined) throw new Error("Expected an interactive API-key provider"); + const ambientApiKey = { ...interactiveProvider.auth.apiKey }; + delete ambientApiKey.login; + const ambientProvider = { + ...interactiveProvider, + id: "ambient-only", + name: "Ambient Only", + auth: { apiKey: ambientApiKey }, + }; + vi.spyOn(runtime, "getProviders").mockReturnValue([...providers, ambientProvider]); + const login = vi.spyOn(runtime, "login"); + + await expect(auth.saveApiKey("ambient-only", "sk-test")).rejects.toThrow( + "Ambient Only does not support interactive API-key setup", + ); + + expect(login).not.toHaveBeenCalled(); + await expect(credentials.read("ambient-only")).resolves.toBeUndefined(); + expect(changes).toEqual([]); + auth.dispose(); + }); + it("stores credentials in the configured agent directory", async () => { const agentDir = await tempAgentDir(); const auth = await AuthService.create({ agentDir }); @@ -54,7 +175,11 @@ describe("AuthService", () => { }); it("refreshes auth state after OAuth login completes", async () => { - const runtime = await ModelRuntime.create({ credentials: new InMemoryCredentialStore() }); + const runtime = await ModelRuntime.create({ + credentials: new InMemoryCredentialStore(), + modelsPath: null, + allowModelNetwork: false, + }); const authFlows = new CapturingOAuthLoginFlowService(); const auth = await AuthService.create({ runtime, authFlows }); const changes: AuthChange[] = []; @@ -88,11 +213,26 @@ async function createAuthService(seed: Record = {}) { for (const [providerId, credential] of Object.entries(seed)) { await credentials.modify(providerId, () => Promise.resolve(credential)); } - const runtime = await ModelRuntime.create({ credentials }); + const runtime = await ModelRuntime.create({ credentials, modelsPath: null, allowModelNetwork: false }); const auth = await AuthService.create({ runtime }); const changes: AuthChange[] = []; auth.subscribe((change) => { changes.push(change); }); - return { auth, credentials, changes }; + return { auth, runtime, credentials, changes }; +} + +function mockLoginPromptsBeforePersistence( + runtime: ModelRuntime, + credentials: InMemoryCredentialStore, + prompts: readonly AuthPrompt[], +) { + return vi.spyOn(runtime, "login").mockImplementation(async (providerId, _authType, interaction) => { + let key: string | undefined; + for (const prompt of prompts) key = await interaction.prompt(prompt); + if (key === undefined) throw new Error("Expected at least one login prompt"); + const credential: Credential = { type: "api_key", key }; + await credentials.modify(providerId, () => Promise.resolve(credential)); + return credential; + }); } async function tempAgentDir(): Promise { diff --git a/src/server/sessions/authService.ts b/src/server/sessions/authService.ts index 5d0679c..386e320 100644 --- a/src/server/sessions/authService.ts +++ b/src/server/sessions/authService.ts @@ -57,11 +57,20 @@ export class AuthService { async saveApiKey(providerId: string, key: string): Promise<{ accepted: true }> { if (key.trim() === "") throw new Error("API key is required"); - // The provider's api-key login prompts for the key and persists the returned - // credential through the runtime's credential store; feed the key back via a - // non-interactive AuthInteraction. + const provider = await this.requireApiKeyLoginProvider(providerId); + let promptAttempted = false; const interaction: AuthInteraction = { - prompt: () => Promise.resolve(key), + prompt: (prompt) => { + if (promptAttempted) { + throw new Error(`${provider.name} requires interactive setup; use Pi's generic /login flow`); + } + promptAttempted = true; + if (prompt.signal?.aborted === true) throw new Error("Login cancelled"); + if (prompt.type !== "secret") { + throw new Error(`${provider.name} requires interactive setup; use Pi's generic /login flow`); + } + return Promise.resolve(key); + }, notify: () => undefined, }; await this.runtime.login(providerId, "api_key", interaction); @@ -108,6 +117,18 @@ export class AuthService { for (const listener of this.listeners) listener(change); } + private async requireApiKeyLoginProvider(providerId: string) { + await this.runtime.refresh(); + const provider = getLoginProviderOptions(this.runtime, "api_key").find((option) => option.id === providerId); + if (provider !== undefined) return provider; + + const knownProvider = this.runtime.getProviders().find((option) => option.id === providerId); + if (knownProvider !== undefined) { + throw new Error(`${knownProvider.name} does not support interactive API-key setup`); + } + throw new Error(`API key provider not found: ${providerId}`); + } + private async requireOAuthLoginProvider(providerId: string) { await this.runtime.refresh(); const provider = getLoginProviderOptions(this.runtime, "oauth").find((option) => option.id === providerId);