diff --git a/src/app.ts b/src/app.ts index 378f015..8ef7201 100644 --- a/src/app.ts +++ b/src/app.ts @@ -247,14 +247,22 @@ function prefersHtml(acceptHeader: string | undefined): boolean { return acceptHeader.includes("text/html") || acceptHeader.includes("application/xhtml+xml"); } +/** + * `browser_completion.attemptId` is never accepted from a request body. It + * exists only to correlate a completion with the attempt the private command + * plane's `beginAuth` created server-side (see `startOAuthFlow`'s + * `trustedBrowserAttemptId` parameter) — a handle a public caller could name + * here would let it complete someone else's pending attempt (R21.1). A public + * `browser_completion` caller has no attempt to correlate: it gets the + * completion code directly in the callback response or handoff page. + */ function normalizeOAuthHandoff(value: OAuthStartRequest["handoff"]): OAuthHandoff { if (!value) { return { kind: "browser_completion" }; } if (value.kind === "browser_completion") { - const attemptId = value.attemptId?.trim(); - return attemptId ? { kind: "browser_completion", attemptId } : { kind: "browser_completion" }; + return { kind: "browser_completion" }; } if ( @@ -507,7 +515,20 @@ export async function startOAuthFlow( ctx: Pick, provider: Provider, caller: AppCaller | null, - input: OAuthStartRequest + input: OAuthStartRequest, + options?: { + /** + * The attempt handle to bind into the state token's `browser_completion` + * handoff. Set only by `beginAuth` (private-command-plane.ts), which + * mints the handle itself via `store.createAuthAttempt` before calling + * here — it is a TypeScript function argument, never a value parsed out + * of `input`/the request body, so a public HTTP caller has no way to + * populate it (R21.1). `normalizeOAuthHandoff` already strips any + * body-supplied `attemptId` for this reason; this is the only path by + * which one reaches the signed state token. + */ + trustedBrowserAttemptId?: string; + } ): Promise { const { config, keys, store } = ctx; const profile = await resolveAppProfile(store, input.appSlug); @@ -562,6 +583,9 @@ export async function startOAuthFlow( }, }; } + if (options?.trustedBrowserAttemptId && handoff.kind === "browser_completion") { + handoff = { kind: "browser_completion", attemptId: options.trustedBrowserAttemptId }; + } if (entitlementPolicy && (!caller || caller.appSlug !== profile.slug)) { return { statusCode: 401, body: { error: "app_auth_required" } }; } diff --git a/src/private-command-plane.ts b/src/private-command-plane.ts index adbfece..54d5746 100644 --- a/src/private-command-plane.ts +++ b/src/private-command-plane.ts @@ -277,13 +277,19 @@ async function beginAuth( }); // Same authority, same non-HTTP entry as refreshAuth above. const context = getHeimdallRuntimeContext(app); - const start = await startOAuthFlow({ config: context.config, keys: context.keys, store: context.store }, provider, callerFromOpenedEnvelope(appSlug), { - appSlug, - mode, - returnTo, - handoff: { kind: "browser_completion", attemptId: attempt.handle }, - ...(entitlementPolicy ? { entitlementPolicy } : {}), - }); + const start = await startOAuthFlow( + { config: context.config, keys: context.keys, store: context.store }, + provider, + callerFromOpenedEnvelope(appSlug), + { + appSlug, + mode, + returnTo, + handoff: { kind: "browser_completion" }, + ...(entitlementPolicy ? { entitlementPolicy } : {}), + }, + { trustedBrowserAttemptId: attempt.handle } + ); const startPayload = start.body as Record; if (start.statusCode !== 201) { await context.store.updateAuthAttempt(appSlug, attempt.handle, { diff --git a/src/store/in-memory.ts b/src/store/in-memory.ts index 12e04e4..d405720 100644 --- a/src/store/in-memory.ts +++ b/src/store/in-memory.ts @@ -353,6 +353,17 @@ export class InMemoryStore implements HeimdallStore { async createAuthCompletion(input: CreateAuthCompletionInput): Promise { // The code is always minted here; nothing upstream may choose it (see // src/app.ts createAuthCompletion call site for why that mattered). + // Mirrors the partial unique index on (app_slug, attempt_id) WHERE + // consumed_at IS NULL (schema.ts): one attempt handle binds at most one + // unconsumed completion, so consumeAuthCompletionByAttempt never has more + // than one live row to choose between. + if (input.attemptId) { + const existingCode = this.authCompletionsByAttempt.get(`${input.appSlug}:${input.attemptId}`); + const existing = existingCode ? this.authCompletions.get(existingCode) : undefined; + if (existing && !existing.consumedAt) { + throw new Error("Attempt handle already has an unconsumed completion."); + } + } const completion: StoredAuthCompletion = { code: randomUUID(), ...(input.attemptId ? { attemptId: input.attemptId } : {}), diff --git a/src/store/schema.ts b/src/store/schema.ts index 7656100..bd04856 100644 --- a/src/store/schema.ts +++ b/src/store/schema.ts @@ -116,8 +116,18 @@ CREATE INDEX IF NOT EXISTS auth_completions_lookup_idx -- redeem the completion it created without ever needing the code. ALTER TABLE auth_completions ADD COLUMN IF NOT EXISTS attempt_id TEXT; -CREATE INDEX IF NOT EXISTS auth_completions_attempt_idx - ON auth_completions(app_slug, attempt_id); +-- One attempt handle may bind at most one unconsumed completion (R21.1): a +-- second completion for the same handle would let the by-attempt UPDATE in +-- consumeAuthCompletionByAttempt match more than one row, silently consuming +-- and discarding all but the one it returns. The predicate is partial rather +-- than a plain UNIQUE(app_slug, attempt_id) because a NULL attempt_id (no +-- correlation handle) and a *consumed*, already-settled completion must not +-- collide with a later one. +DROP INDEX IF EXISTS auth_completions_attempt_idx; + +CREATE UNIQUE INDEX IF NOT EXISTS auth_completions_attempt_unconsumed_unique_idx + ON auth_completions(app_slug, attempt_id) + WHERE attempt_id IS NOT NULL AND consumed_at IS NULL; CREATE TABLE IF NOT EXISTS capability_grants ( id TEXT PRIMARY KEY, diff --git a/tests/caller-identity-attack-soul.test.ts b/tests/caller-identity-attack-soul.test.ts index ecc371f..88da15d 100644 --- a/tests/caller-identity-attack-soul.test.ts +++ b/tests/caller-identity-attack-soul.test.ts @@ -236,19 +236,17 @@ describe("consumeAuthCompletion: single-use, scoped, expiring", () => { }); describe("attempt-handle binding: the public start route may attach a completion to any attempt", () => { - // Hole. The private plane creates attempt H for its app and later redeems - // by handle (consumeAuthCompletionByAttempt). The public start route - // accepts handoff.attemptId = H from anyone, and the callback stores a - // completion under attempt_id = H and flips attempt H to "completed". - // Nothing checks that the flow carrying attemptId H was the flow the plane - // started for H. Whoever learns H (Bifrost puts it in the wait-page URL) - // can bind their own provider identity to the app's pending attempt, and - // the app's completeAuth(H) then authenticates the victim's app session as - // the attacker. Exploitability rests on H leaking; the binding gap itself - // is real. Close it by refusing an attemptId at the public start route - // unless the caller is the app (AppCaller), or by having the store refuse - // to create a completion for an attempt whose state token it did not mint. - it.fails("a public flow carrying the app's attempt handle must not complete that attempt", async () => { + // Closed (R21.1). The private plane creates attempt H for its app and later + // redeems by handle (consumeAuthCompletionByAttempt). handoff.attemptId is + // no longer read from the request body at all (normalizeOAuthHandoff always + // returns a bare `{ kind: "browser_completion" }` for a public caller); the + // only path by which an attempt handle reaches the signed state token is + // `startOAuthFlow`'s `trustedBrowserAttemptId` option, which only + // `beginAuth` (private-command-plane.ts) can set, from the handle it just + // minted server-side. A value that never crosses the public boundary cannot + // be chosen by a stranger, so a public flow carrying H as body JSON now has + // no effect on H at all. + it("a public flow carrying the app's attempt handle must not complete that attempt", async () => { const { app, store } = await harness(); const attempt = await store.createAuthAttempt({ appSlug: "ghostlight",