From ea804ddbbeab6d485e0db042536bbe707c81980f Mon Sep 17 00:00:00 2001 From: Meta Vens Date: Tue, 8 Sep 2026 14:43:33 +0200 Subject: [PATCH] R21.1: bind the browser_completion attempt handle to the private plane The public /v1/oauth/:provider/start route read handoff.attemptId straight out of the request body and folded it into the signed state token unconditionally. Since the private command plane's beginAuth mints an attempt handle server-side and later redeems by that handle (consumeAuthCompletionByAttempt), anyone who learned a pending handle could carry it through their own OAuth flow and bind their own identity to it -- the app's completeAuth(H) would then authenticate the victim's session as the attacker. Same family as the takeover chain closed in the prior pass: a caller-chosen identifier trusted as authority. normalizeOAuthHandoff now always strips attemptId from a browser_completion handoff; a public caller has no attempt to correlate and gets its completion code directly. The only path an attempt handle can reach the state token is startOAuthFlow's new trustedBrowserAttemptId option, which is a TypeScript function argument set by beginAuth from the handle it just minted -- never a value parsed out of a request body. Also adds the uniqueness constraint the by-attempt redemption needed: a partial unique index on (app_slug, attempt_id) WHERE attempt_id IS NOT NULL AND consumed_at IS NULL, mirrored in InMemoryStore. Without it, consumeAuthCompletionByAttempt's UPDATE could match more than one row and silently discard all but the one it returned. Flips Soul's attempt-handle-binding it.fails to a passing it. Co-Authored-By: Claude Opus 5 --- src/app.ts | 30 ++++++++++++++++++++--- src/private-command-plane.ts | 20 +++++++++------ src/store/in-memory.ts | 11 +++++++++ src/store/schema.ts | 14 +++++++++-- tests/caller-identity-attack-soul.test.ts | 24 +++++++++--------- 5 files changed, 74 insertions(+), 25 deletions(-) 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",