Archived
fix(auth): prevent API key reuse across login prompts
This commit is contained in:
@@ -4,13 +4,15 @@ import { getLoginProviderOptions, getLogoutProviderOptions, type AuthProviderRun
|
|||||||
function runtime(): AuthProviderRuntime {
|
function runtime(): AuthProviderRuntime {
|
||||||
const credentials = [{ providerId: "openai", type: "api_key" as const }];
|
const credentials = [{ providerId: "openai", type: "api_key" as const }];
|
||||||
// Auth shapes mirror what the Pi SDK actually reports for these providers:
|
// 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 = [
|
const providers = [
|
||||||
{ id: "anthropic", name: "Anthropic", auth: { oauth: {}, apiKey: {} } },
|
{ id: "anthropic", name: "Anthropic", auth: { oauth: {}, apiKey: { login: () => undefined } } },
|
||||||
{ id: "github-copilot", name: "GitHub Copilot", auth: { oauth: {}, apiKey: {} } },
|
{ id: "github-copilot", name: "GitHub Copilot", auth: { oauth: {}, apiKey: { login: () => undefined } } },
|
||||||
{ id: "openai-codex", name: "ChatGPT Plus/Pro (Codex Subscription)", auth: { oauth: {} } },
|
{ id: "openai-codex", name: "ChatGPT Plus/Pro (Codex Subscription)", auth: { oauth: {} } },
|
||||||
{ id: "openai", name: "OpenAI", auth: { apiKey: {} } },
|
{ id: "openai", name: "OpenAI", auth: { apiKey: { login: () => undefined } } },
|
||||||
{ id: "custom", name: "Custom", auth: { apiKey: {} } },
|
{ id: "custom", name: "Custom", auth: { apiKey: { login: () => undefined } } },
|
||||||
|
{ id: "ambient", name: "Ambient credentials", auth: { apiKey: {} } },
|
||||||
];
|
];
|
||||||
return {
|
return {
|
||||||
getProviders: () => providers,
|
getProviders: () => providers,
|
||||||
@@ -20,7 +22,7 @@ function runtime(): AuthProviderRuntime {
|
|||||||
}
|
}
|
||||||
|
|
||||||
describe("auth provider options", () => {
|
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());
|
const options = getLoginProviderOptions(runtime());
|
||||||
expect(options).toEqual(expect.arrayContaining([
|
expect(options).toEqual(expect.arrayContaining([
|
||||||
// Dual-capable providers surface both login methods, driven purely by SDK data.
|
// 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-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: "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 () => {
|
it("returns only currently stored credentials for logout", async () => {
|
||||||
|
|||||||
@@ -4,7 +4,7 @@ import type { AuthProviderOption, AuthProviderStatus, AuthType } from "../../sha
|
|||||||
interface AuthProviderInfo {
|
interface AuthProviderInfo {
|
||||||
id: string;
|
id: string;
|
||||||
name: string;
|
name: string;
|
||||||
auth: { apiKey?: unknown; oauth?: unknown };
|
auth: { apiKey?: { login?: unknown }; oauth?: unknown };
|
||||||
}
|
}
|
||||||
|
|
||||||
/** Non-secret stored-credential metadata, keyed by provider id. */
|
/** Non-secret stored-credential metadata, keyed by provider id. */
|
||||||
@@ -40,7 +40,7 @@ export function getLoginProviderOptions(runtime: AuthProviderRuntime, authType?:
|
|||||||
}
|
}
|
||||||
|
|
||||||
for (const provider of providers) {
|
for (const provider of providers) {
|
||||||
if (provider.auth.apiKey === undefined) continue;
|
if (provider.auth.apiKey?.login === undefined) continue;
|
||||||
options.push({
|
options.push({
|
||||||
id: provider.id,
|
id: provider.id,
|
||||||
name: provider.name,
|
name: provider.name,
|
||||||
|
|||||||
@@ -2,7 +2,7 @@ import { mkdtemp, readFile, rm } from "node:fs/promises";
|
|||||||
import { tmpdir } from "node:os";
|
import { tmpdir } from "node:os";
|
||||||
import { join } from "node:path";
|
import { join } from "node:path";
|
||||||
import { ModelRuntime } from "@earendil-works/pi-coding-agent";
|
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 { afterEach, describe, expect, it, vi } from "vitest";
|
||||||
import type { OAuthFlowState } from "../../shared/apiTypes.js";
|
import type { OAuthFlowState } from "../../shared/apiTypes.js";
|
||||||
import { AuthService, type AuthChange } from "./authService.js";
|
import { AuthService, type AuthChange } from "./authService.js";
|
||||||
@@ -43,6 +43,127 @@ describe("AuthService", () => {
|
|||||||
auth.dispose();
|
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 () => {
|
it("stores credentials in the configured agent directory", async () => {
|
||||||
const agentDir = await tempAgentDir();
|
const agentDir = await tempAgentDir();
|
||||||
const auth = await AuthService.create({ agentDir });
|
const auth = await AuthService.create({ agentDir });
|
||||||
@@ -54,7 +175,11 @@ describe("AuthService", () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
it("refreshes auth state after OAuth login completes", async () => {
|
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 authFlows = new CapturingOAuthLoginFlowService();
|
||||||
const auth = await AuthService.create({ runtime, authFlows });
|
const auth = await AuthService.create({ runtime, authFlows });
|
||||||
const changes: AuthChange[] = [];
|
const changes: AuthChange[] = [];
|
||||||
@@ -88,11 +213,26 @@ async function createAuthService(seed: Record<string, Credential> = {}) {
|
|||||||
for (const [providerId, credential] of Object.entries(seed)) {
|
for (const [providerId, credential] of Object.entries(seed)) {
|
||||||
await credentials.modify(providerId, () => Promise.resolve(credential));
|
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 auth = await AuthService.create({ runtime });
|
||||||
const changes: AuthChange[] = [];
|
const changes: AuthChange[] = [];
|
||||||
auth.subscribe((change) => { changes.push(change); });
|
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<string> {
|
async function tempAgentDir(): Promise<string> {
|
||||||
|
|||||||
@@ -57,11 +57,20 @@ export class AuthService {
|
|||||||
|
|
||||||
async saveApiKey(providerId: string, key: string): Promise<{ accepted: true }> {
|
async saveApiKey(providerId: string, key: string): Promise<{ accepted: true }> {
|
||||||
if (key.trim() === "") throw new Error("API key is required");
|
if (key.trim() === "") throw new Error("API key is required");
|
||||||
// The provider's api-key login prompts for the key and persists the returned
|
const provider = await this.requireApiKeyLoginProvider(providerId);
|
||||||
// credential through the runtime's credential store; feed the key back via a
|
let promptAttempted = false;
|
||||||
// non-interactive AuthInteraction.
|
|
||||||
const interaction: AuthInteraction = {
|
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,
|
notify: () => undefined,
|
||||||
};
|
};
|
||||||
await this.runtime.login(providerId, "api_key", interaction);
|
await this.runtime.login(providerId, "api_key", interaction);
|
||||||
@@ -108,6 +117,18 @@ export class AuthService {
|
|||||||
for (const listener of this.listeners) listener(change);
|
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) {
|
private async requireOAuthLoginProvider(providerId: string) {
|
||||||
await this.runtime.refresh();
|
await this.runtime.refresh();
|
||||||
const provider = getLoginProviderOptions(this.runtime, "oauth").find((option) => option.id === providerId);
|
const provider = getLoginProviderOptions(this.runtime, "oauth").find((option) => option.id === providerId);
|
||||||
|
|||||||
Reference in New Issue
Block a user