diff --git a/packages/ai/CHANGELOG.md b/packages/ai/CHANGELOG.md index aa9bf44af..e1fea979e 100644 --- a/packages/ai/CHANGELOG.md +++ b/packages/ai/CHANGELOG.md @@ -68,6 +68,7 @@ ### Fixed - Fixed GitLab Duo Workflow `direct_access` errors dropping the HTTP status when GitLab returned a JSON error body (e.g. a 401 `{"message":"Unauthorized"}` from an expired OAuth token, or a 429 quota body). The thrown error now embeds `HTTP ` alongside the body message so the streaming auth-retry path (`extractStatusFromAssistantError` → `extractHttpStatusFromError`) can recover the status and refresh/rotate the parked broker credential instead of surfacing a hard failure. +- Fixed `AuthStorage.login` always synthesizing a default manual-code paste prompt, which made the loopback `OAuthCallbackFlow` race a readline prompt against the HTTP callback for normal (non-paste-code) OAuth providers and could leave that prompt dangling — a dirty/blocked terminal — when the browser callback won. The default prompt is now synthesized only for `pasteCodeFlow` providers (`PASTE_CODE_LOGIN_PROVIDERS`); loopback providers get no manual-code race unless a caller explicitly supplies `onManualCodeInput`. This is the authoritative gate covering every caller (not just the auth-broker CLI). ## [16.1.16] - 2026-06-23 diff --git a/packages/ai/src/auth-storage.ts b/packages/ai/src/auth-storage.ts index f3ee4926c..aad0acdf4 100644 --- a/packages/ai/src/auth-storage.ts +++ b/packages/ai/src/auth-storage.ts @@ -13,7 +13,7 @@ import * as path from "node:path"; import { extractHttpStatusFromError, getAgentDbPath, logger } from "@oh-my-pi/pi-utils"; import type { ApiKeyResolver } from "./auth-retry"; import { isUsageLimitOutcome } from "./rate-limit-utils"; -import { getProviderDefinition } from "./registry"; +import { getProviderDefinition, PASTE_CODE_LOGIN_PROVIDERS } from "./registry"; import { getOAuthApiKey, getOAuthProvider, refreshOAuthToken } from "./registry/oauth"; import type { OAuthController, OAuthCredentials, OAuthProvider, OAuthProviderId } from "./registry/oauth/types"; import { getEnvApiKey, getEnvApiKeyName } from "./stream"; @@ -1884,7 +1884,17 @@ export class AuthStorage { onPrompt: (prompt: { message: string; placeholder?: string }) => Promise; }, ): Promise { - const manualCodeInput = () => ctrl.onPrompt({ message: "Paste the authorization code (or full redirect URL):" }); + // Only paste-code providers (fixed non-loopback redirect, e.g. GitLab Duo + // Agent's vscode:// URI) get a default manual-code prompt. For loopback OAuth + // providers the `OAuthCallbackFlow` would otherwise race this readline prompt + // against the HTTP callback and, when the callback wins, leave the prompt + // outstanding — a dirty/blocked terminal. Synthesizing the default only for + // paste-code providers is the authoritative gate (it covers every caller, not + // just the CLI); an explicit caller-supplied `onManualCodeInput` is still + // honored for any provider as an escape hatch. + const manualCodeInput = PASTE_CODE_LOGIN_PROVIDERS.has(provider) + ? () => ctrl.onPrompt({ message: "Paste the authorization code (or full redirect URL):" }) + : undefined; // Built-in registry first, then runtime-registered extension providers. const def = getProviderDefinition(provider) ?? getOAuthProvider(provider); if (!def?.login) { diff --git a/packages/ai/test/auth-storage-manual-code-gate.test.ts b/packages/ai/test/auth-storage-manual-code-gate.test.ts new file mode 100644 index 000000000..eef8ccd6a --- /dev/null +++ b/packages/ai/test/auth-storage-manual-code-gate.test.ts @@ -0,0 +1,106 @@ +import { Database } from "bun:sqlite"; +import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test"; +import { AuthStorage, SqliteAuthCredentialStore } from "@oh-my-pi/pi-ai/auth-storage"; +import { registerOAuthProvider, unregisterOAuthProviders } from "@oh-my-pi/pi-ai/registry/oauth"; +import * as gitlabDuoWorkflowOAuth from "@oh-my-pi/pi-ai/registry/oauth/gitlab-duo-workflow"; +import type { OAuthLoginCallbacks, OAuthProviderInterface } from "@oh-my-pi/pi-ai/registry/oauth/types"; + +const TEST_SOURCE = "manual-code-gate-test"; + +// A custom (extension) OAuth provider is, by construction, NOT in +// PASTE_CODE_LOGIN_PROVIDERS (that set is built from the static built-in +// registry's `pasteCodeFlow` flags). It therefore exercises the loopback path: +// AuthStorage.login must NOT synthesize a default manual-code prompt for it. +function registerCapturingLoopbackProvider(id: string): { received: () => OAuthLoginCallbacks | undefined } { + let captured: OAuthLoginCallbacks | undefined; + const provider: OAuthProviderInterface = { + id, + name: `Capturing ${id}`, + sourceId: TEST_SOURCE, + async login(callbacks: OAuthLoginCallbacks) { + captured = callbacks; + // Return an empty string so AuthStorage treats it as "no key entered" + // and skips credential persistence — we only assert the forwarded callbacks. + return ""; + }, + }; + registerOAuthProvider(provider); + return { received: () => captured }; +} + +describe("AuthStorage.login default manual-code prompt gating", () => { + let store: SqliteAuthCredentialStore; + let storage: AuthStorage; + + beforeEach(async () => { + store = new SqliteAuthCredentialStore(new Database(":memory:")); + storage = new AuthStorage(store); + await storage.reload(); + }); + + afterEach(() => { + unregisterOAuthProviders(TEST_SOURCE); + vi.restoreAllMocks(); + store.close(); + }); + + it("does NOT synthesize a default manual-code prompt for a loopback provider", async () => { + const capture = registerCapturingLoopbackProvider("loopback-capture-provider"); + + await storage.login("loopback-capture-provider", { + onAuth: () => {}, + onPrompt: async () => "should-not-be-called", + }); + + const forwarded = capture.received(); + expect(forwarded).toBeDefined(); + // The loopback OAuthCallbackFlow keys its readline-vs-callback race solely on + // a truthy `onManualCodeInput`; leaving it undefined is what prevents the + // dangling-prompt regression for normal loopback logins. + expect(forwarded?.onManualCodeInput).toBeUndefined(); + }); + + it("honors an explicit caller-supplied manual-code prompt for a loopback provider (escape hatch)", async () => { + const capture = registerCapturingLoopbackProvider("loopback-explicit-provider"); + const explicit = async () => "explicit-code"; + + await storage.login("loopback-explicit-provider", { + onAuth: () => {}, + onPrompt: async () => "unused", + onManualCodeInput: explicit, + }); + + const forwarded = capture.received(); + expect(forwarded?.onManualCodeInput).toBe(explicit); + }); + + it("synthesizes a default manual-code prompt for a paste-code provider when the caller omits one", async () => { + // gitlab-duo-agent is a built-in pasteCodeFlow provider (fixed vscode:// + // redirect): the default manual-code prompt is required so the user can paste + // the callback URL. Spy on the lazily-imported login to capture the callbacks + // AuthStorage forwards, and have it short-circuit before any network call. + let forwarded: OAuthLoginCallbacks | undefined; + const promptText = "PASTE-CODE-DEFAULT-PROMPT-PROBE"; + vi.spyOn(gitlabDuoWorkflowOAuth, "loginGitLabDuoWorkflow").mockImplementation( + async (callbacks: OAuthLoginCallbacks) => { + forwarded = callbacks; + return { access: "access-token", refresh: "refresh-token", expires: Date.now() + 60_000 }; + }, + ); + + await storage.login("gitlab-duo-agent", { + onAuth: () => {}, + onPrompt: async prompt => { + // The synthesized default routes its prompt through onPrompt; return a + // sentinel so we can prove the default (not the caller) produced it. + return `${promptText}:${prompt.message}`; + }, + }); + + expect(forwarded).toBeDefined(); + expect(forwarded?.onManualCodeInput).toBeDefined(); + // Invoking the synthesized default must route through the caller's onPrompt. + const result = await forwarded?.onManualCodeInput?.(); + expect(result).toContain(promptText); + }); +}); diff --git a/packages/coding-agent/src/cli/auth-broker-cli.ts b/packages/coding-agent/src/cli/auth-broker-cli.ts index 881e44723..2d04ee61f 100644 --- a/packages/coding-agent/src/cli/auth-broker-cli.ts +++ b/packages/coding-agent/src/cli/auth-broker-cli.ts @@ -213,10 +213,13 @@ async function runLocalLogin(provider: OAuthProvider): Promise { await storage.reload(); try { // Only paste-code providers (fixed non-loopback redirect, e.g. GitLab Duo - // Agent's vscode:// URI) get the manual paste fallback. For normal loopback - // providers `onManualCodeInput` would make OAuthCallbackFlow race a readline - // prompt against the HTTP callback; if the callback wins, the outstanding - // prompt is never cancelled and leaves the terminal in a dirty/blocked state. + // Agent's vscode:// URI) get the manual paste fallback. An explicit + // `onManualCodeInput` is honored for ANY provider (the storage escape hatch), + // so for loopback providers we must not pass it: it would make + // `OAuthCallbackFlow` race a readline prompt against the HTTP callback and, if + // the callback wins, leave that prompt outstanding (dirty/blocked terminal). + // `AuthStorage.login` independently refuses to synthesize the default prompt + // for non-paste-code providers, so this is defense-in-depth on the same gate. const usesManualInput = PASTE_CODE_LOGIN_PROVIDERS.has(provider); await storage.login(provider, { onAuth({ url, instructions }) {