From ee28af3487cfdf84161ea1d40437448e3273afc9 Mon Sep 17 00:00:00 2001 From: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com> Date: Mon, 9 Nov 2026 17:29:39 -0800 Subject: [PATCH] fix: hosted agent approval no longer fails with "Your session cannot approve" app.t3.codes only caches a T3 Connect token for environments saved in its catalog, so on the approval page each call minted a new credential, and the environment saw a new session. The approval's CSRF token was bound to the session that loaded the page, so the decision was refused. Bind the CSRF token to the DPoP key for DPoP sessions; cookie and plain bearer sessions stay bound to their session. The hosted page also reuses one authorization per call instead of minting two. Co-Authored-By: Claude Opus 5.5 --- apps/server/src/auth/EnvironmentAuth.test.ts | 39 +----------- apps/server/src/auth/McpOAuth.test.ts | 60 +++++++++++++++++-- apps/server/src/auth/McpOAuth.ts | 14 +++-- apps/server/src/auth/dpop.testkit.ts | 39 ++++++++++++ .../src/state/connectMcpApproval.ts | 13 +++- 5 files changed, 114 insertions(+), 51 deletions(-) create mode 100644 apps/server/src/auth/dpop.testkit.ts diff --git a/apps/server/src/auth/EnvironmentAuth.test.ts b/apps/server/src/auth/EnvironmentAuth.test.ts index 3c05047e4a4c3..14f12059ce5ce 100644 --- a/apps/server/src/auth/EnvironmentAuth.test.ts +++ b/apps/server/src/auth/EnvironmentAuth.test.ts @@ -1,14 +1,6 @@ -// @effect-diagnostics-next-line nodeBuiltinImport:off -- Effect's Crypto has no generateKeyPairSync or sign. -import * as NodeCrypto from "node:crypto"; - import * as NodePlatformCrypto from "@effect/platform-node/NodeCrypto"; import * as NodeServices from "@effect/platform-node/NodeServices"; import { authScopeResponse, AuthStandardClientScopes } from "@t3tools/contracts"; -import { - computeDpopAccessTokenHash, - computeDpopJwkThumbprint, - type DpopPublicJwk, -} from "@t3tools/shared/dpop"; import { expect, it } from "@effect/vitest"; import * as DateTime from "effect/DateTime"; import * as Effect from "effect/Effect"; @@ -16,6 +8,7 @@ import * as Layer from "effect/Layer"; import * as Redacted from "effect/Redacted"; import * as ServerConfig from "../config.ts"; +import { makeDpopKey } from "./dpop.testkit.ts"; import * as ServerEnvironment from "../environment/ServerEnvironment.ts"; import * as SqlitePersistence from "../persistence/Sqlite.ts"; import * as EnvironmentAuth from "./EnvironmentAuth.ts"; @@ -80,36 +73,6 @@ const requestMetadata = { ipAddress: "192.168.1.23", }; -const makeDpopKey = () => { - const { privateKey, publicKey } = NodeCrypto.generateKeyPairSync("ec", { namedCurve: "P-256" }); - const publicJwk = publicKey.export({ format: "jwk" }) as DpopPublicJwk; - let proofCount = 0; - return { - thumbprint: computeDpopJwkThumbprint(publicJwk), - sign: (input: { - readonly method: string; - readonly url: string; - readonly token: string; - readonly iat: number; - }) => { - const encode = (value: unknown) => Buffer.from(JSON.stringify(value)).toString("base64url"); - const header = encode({ typ: "dpop+jwt", alg: "ES256", jwk: publicJwk }); - const payload = encode({ - htm: input.method, - htu: input.url, - jti: `proof-${++proofCount}`, - iat: input.iat, - ath: computeDpopAccessTokenHash(input.token), - }); - const signature = NodeCrypto.sign("sha256", Buffer.from(`${header}.${payload}`), { - key: privateKey, - dsaEncoding: "ieee-p1363", - }).toString("base64url"); - return `${header}.${payload}.${signature}`; - }, - }; -}; - it.layer(Layer.mergeAll(NodeServices.layer, NodePlatformCrypto.layer))( "EnvironmentAuth.layer", (it) => { diff --git a/apps/server/src/auth/McpOAuth.test.ts b/apps/server/src/auth/McpOAuth.test.ts index a397576340a0f..c7cf59db93863 100644 --- a/apps/server/src/auth/McpOAuth.test.ts +++ b/apps/server/src/auth/McpOAuth.test.ts @@ -10,6 +10,7 @@ import { mcpClientScopes, } from "@t3tools/contracts"; import { expect, it } from "@effect/vitest"; +import * as Clock from "effect/Clock"; import * as Context from "effect/Context"; import * as SecureRandom from "effect/crypto/SecureRandom"; import * as Effect from "effect/Effect"; @@ -24,6 +25,7 @@ import * as Schema from "effect/Schema"; import * as CloudLink from "../cloud/CloudLink.ts"; import * as ServerConfig from "../config.ts"; +import { makeDpopKey } from "./dpop.testkit.ts"; import * as ServerEnvironment from "../environment/ServerEnvironment.ts"; import * as ServerSettings from "../serverSettings.ts"; import * as Sqlite from "../persistence/Sqlite.ts"; @@ -130,17 +132,19 @@ const withRoutes = ( const json = (response: Response) => Effect.promise(() => response.json() as Promise); -/** `credential` is a browser cookie, or `Bearer ` for a session such as T3 Connect's. */ -const postJson = (path: string, body: unknown, credential?: string) => +/** `credential` is a browser cookie, `Bearer `, or the headers of a DPoP request. */ +const postJson = (path: string, body: unknown, credential?: string | Record) => at(path, { method: "POST", headers: { "content-type": "application/json", ...(credential === undefined ? {} - : credential.startsWith("Bearer ") - ? { authorization: credential } - : { cookie: credential }), + : typeof credential !== "string" + ? credential + : credential.startsWith("Bearer ") + ? { authorization: credential } + : { cookie: credential }), }, body: encodeJson(body), }); @@ -149,7 +153,7 @@ const decide = ( handler: Handler, authorization: Record, decision: Record, - cookie?: string, + cookie?: string | Record, ) => handler(postJson("/oauth/mcp/decision", { authorization, decision }, cookie)).pipe( Effect.flatMap((response) => @@ -742,6 +746,50 @@ it.live("one-click approves only access the browser session holds the scopes for connect, ); expect(new URL(viaConnect.redirectTo!).searchParams.get("code")).toEqual(expect.any(String)); + + // The hosted app may load and decide with two credentials minted for the same browser key. + const key = makeDpopKey(); + const dpopSession = (thumbprint: string) => + auth.issuePairingCredential({ scopes: [...AuthStandardClientScopes] }).pipe( + Effect.flatMap((pairing) => + auth.exchangeBootstrapCredentialForAccessToken( + pairing.credential, + undefined, + { deviceType: "desktop" }, + { proofKeyThumbprint: thumbprint }, + ), + ), + Effect.map((exchanged) => exchanged.access_token), + ); + const iat = Math.floor((yield* Clock.currentTimeMillis) / 1_000); + const dpopHeaders = (signer: typeof key, token: string, path: string) => ({ + authorization: `DPoP ${token}`, + dpop: signer.sign({ method: "POST", url: `${ORIGIN}${path}`, token, iat }), + }); + const loadedWith = yield* dpopSession(key.thumbprint); + const loaded = yield* handler( + postJson( + "/oauth/mcp/approval", + params, + dpopHeaders(key, loadedWith, "/oauth/mcp/approval"), + ), + ).pipe(Effect.flatMap(json<{ csrfToken?: string }>)); + const renewed = yield* dpopSession(key.thumbprint); + const sameKey = yield* decide( + handler, + params, + { _tag: "browser-session", access: "auto", csrfToken: loaded.csrfToken! }, + dpopHeaders(key, renewed, "/oauth/mcp/decision"), + ); + expect(new URL(sameKey.redirectTo!).searchParams.get("code")).toEqual(expect.any(String)); + const otherKey = makeDpopKey(); + const elsewhere = yield* decide( + handler, + params, + { _tag: "browser-session", access: "auto", csrfToken: loaded.csrfToken! }, + dpopHeaders(otherKey, yield* dpopSession(otherKey.thumbprint), "/oauth/mcp/decision"), + ); + expect(elsewhere.status).toBe(400); }), ), ); diff --git a/apps/server/src/auth/McpOAuth.ts b/apps/server/src/auth/McpOAuth.ts index 7625fe436a8ea..426cb2213f3ef 100644 --- a/apps/server/src/auth/McpOAuth.ts +++ b/apps/server/src/auth/McpOAuth.ts @@ -307,11 +307,11 @@ const make = Effect.gen(function* () { ) : Effect.succeed(false); - const csrfToken = (sessionId: string, authorization: AuthorizationRequest) => + const csrfToken = (holder: string, authorization: AuthorizationRequest) => sign( "mcp-csrf", JSON.stringify([ - sessionId, + holder, authorization.client.clientId, authorization.redirectUri, authorization.codeChallenge, @@ -413,9 +413,13 @@ const make = Effect.gen(function* () { // Approving manages access, and a session may only hand out scopes it holds. session.scopes.includes(AuthAccessWriteScope) && session.scopes.includes(AuthOrchestrationReadScope) - ? csrfToken(session.sessionId, authorization).pipe( - Effect.map((token) => ({ csrfToken: token, scopes: session.scopes })), - ) + ? // The hosted app may get a fresh credential per call, all bound to one browser key. + csrfToken( + session.proofKeyThumbprint === undefined + ? session.sessionId + : `dpop:${session.proofKeyThumbprint}`, + authorization, + ).pipe(Effect.map((token) => ({ csrfToken: token, scopes: session.scopes }))) : Effect.undefined, ), Effect.orElseSucceed(() => undefined), diff --git a/apps/server/src/auth/dpop.testkit.ts b/apps/server/src/auth/dpop.testkit.ts new file mode 100644 index 0000000000000..f0f00c6ccc439 --- /dev/null +++ b/apps/server/src/auth/dpop.testkit.ts @@ -0,0 +1,39 @@ +// @effect-diagnostics-next-line nodeBuiltinImport:off -- Effect's Crypto has no generateKeyPairSync or sign. +import * as NodeCrypto from "node:crypto"; + +import { + computeDpopAccessTokenHash, + computeDpopJwkThumbprint, + type DpopPublicJwk, +} from "@t3tools/shared/dpop"; + +/** A client's DPoP key, signing proofs the way a browser or the mobile app does. */ +export const makeDpopKey = () => { + const { privateKey, publicKey } = NodeCrypto.generateKeyPairSync("ec", { namedCurve: "P-256" }); + const publicJwk = publicKey.export({ format: "jwk" }) as DpopPublicJwk; + let proofCount = 0; + return { + thumbprint: computeDpopJwkThumbprint(publicJwk), + sign: (input: { + readonly method: string; + readonly url: string; + readonly token: string; + readonly iat: number; + }) => { + const encode = (value: unknown) => Buffer.from(JSON.stringify(value)).toString("base64url"); + const header = encode({ typ: "dpop+jwt", alg: "ES256", jwk: publicJwk }); + const payload = encode({ + htm: input.method, + htu: input.url, + jti: `proof-${++proofCount}`, + iat: input.iat, + ath: computeDpopAccessTokenHash(input.token), + }); + const signature = NodeCrypto.sign("sha256", Buffer.from(`${header}.${payload}`), { + key: privateKey, + dsaEncoding: "ieee-p1363", + }).toString("base64url"); + return `${header}.${payload}.${signature}`; + }, + }; +}; diff --git a/packages/client-runtime/src/state/connectMcpApproval.ts b/packages/client-runtime/src/state/connectMcpApproval.ts index 979754aa14158..eaa2f2f16131b 100644 --- a/packages/client-runtime/src/state/connectMcpApproval.ts +++ b/packages/client-runtime/src/state/connectMcpApproval.ts @@ -9,6 +9,7 @@ import type { EnvironmentId, } from "@t3tools/contracts"; import * as Effect from "effect/Effect"; +import * as Option from "effect/Option"; import type * as HttpClient from "effect/http/HttpClient"; import type { Atom } from "effect/reactivity"; import * as Schema from "effect/Schema"; @@ -75,8 +76,16 @@ const callConnectMcpApproval = Effect.fn("clientRuntime.state.connectMcpApproval }), }, signer: yield* Effect.serviceOption(ManagedRelay.ManagedRelayDpopSigner), - remoteAuthorization: yield* Effect.serviceOption( - RemoteEnvironmentAuthorization.RemoteEnvironmentAuthorization, + // A token for an unsaved environment is not cached, so reuse this one until it is rejected. + remoteAuthorization: Option.some( + RemoteEnvironmentAuthorization.RemoteEnvironmentAuthorization.of({ + ...remote, + // Untraced: the relay authorization it may call owns the span. + authorizeDpopHttp: Effect.fnUntraced(function* (request) { + if (request.rejectedAccessToken === undefined) return authorized; + return yield* remote.authorizeDpopHttp(request); + }), + }), ), group: "mcpOAuth", method: "POST",