diff --git a/CHANGELOG.md b/CHANGELOG.md index a18f23a..9fb9a83 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,17 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ## [Unreleased] +### Fixed — approving in the sign-in popup can't be broken by the email tab (mcp) + +The hosted MCP's sign-in popup and the email-link tab share one cookie, and every load of the Approve page used to mint a new CSRF token into it. If the link tab's fallback link (or a reload) loaded the page again, the Allow button already showing in the popup failed with "csrf validation failed". The only Allow that still worked was the one in the email tab, which returns to ChatGPT without its pending state, so the connection never finished. + +- **One token per sign-in.** The Approve page reuses the CSRF token the request's cookie already carries, so every window of the browser agrees on it. +- **Deny is final.** Denying consumes the request, so an Allow button still open in another window can't connect afterwards. +- **The fallback says what it costs.** The email tab's link now reads "Approve in this tab instead" and warns that the app may not finish connecting from there. +- **Token responses are never cached.** `/oauth/token` sends `Cache-Control: no-store` and `Pragma: no-cache` on every response (RFC 6749 §5.1). +- **Failed refreshes are logged, and a wrong client doesn't burn the token.** A refused refresh logs its status and a client-id prefix, so a connection that drops after its 1-hour token shows in `wrangler tail`. A refresh sent with another client's id is refused before the token is used up, so it no longer turns the owner's next refresh into a false theft alarm that revokes the connection. +- **`ping` answers.** The server returns an empty result for MCP's `ping`, instead of "method not found". + ### Fixed — the served API spec offers only the networks a buyer can pay on (api) Circle's marketplace reviewers and buying agents read `/openapi.json`. Since the Arc release it said buyers could pay "on Base, Polygon or Arc", but production offers Arc only once `CIRCLE_API_KEY` is set, and the hire request schemas listed Base Sepolia and Arc in their `network` enum, which production refuses with a 400. diff --git a/MCP_CONNECTOR_SPEC.md b/MCP_CONNECTOR_SPEC.md index 23c3e82..6943a2b 100644 --- a/MCP_CONNECTOR_SPEC.md +++ b/MCP_CONNECTOR_SPEC.md @@ -72,21 +72,22 @@ The request is persisted for 10 minutes. A signed, httpOnly, `SameSite=Lax` cook 1. **Email form**, `POST /oauth/email`. The CSRF token must match the cookie. The send path is throttled to 10 requests per IP per hour, before any account lookup. The page always says "if an account exists…", so it can't be used to enumerate accounts. A one-time link (15-minute login challenge) is mailed only when an owner account exists and that owner is under 5 sends per hour. The mail goes out after the response (`waitUntil`), so its round-trip is not a timing signal. 2. **No account yet.** The sign-in only works for existing BasedAgents owner accounts. The authorize and "check your email" pages link to `https://app.basedagents.ai/start` (one email field) to create one first. -3. **Landing**, `GET /oauth/continue?lt=…&req=…`. The login challenge is consumed atomically and is single-use. It must belong to `req`. **The same-browser binding is mandatory:** the click must carry the `mcp_authreq` cookie for the same `req`. A link opened on another device or browser is rejected with "Finish in the same browser". This blocks cross-account authorization, where an attacker starts a request and seeds a challenge to a victim's email. The owner is bound to the request exactly once. This tab shows no Allow form. It says "go back to the window where you started", with an "Approve here instead" link for flows that ran in this same tab. +3. **Landing**, `GET /oauth/continue?lt=…&req=…`. The login challenge is consumed atomically and is single-use. It must belong to `req`. **The same-browser binding is mandatory:** the click must carry the `mcp_authreq` cookie for the same `req`. A link opened on another device or browser is rejected with "Finish in the same browser". This blocks cross-account authorization, where an attacker starts a request and seeds a challenge to a victim's email. The owner is bound to the request exactly once. This tab shows no Allow form. It says "go back to the window where you started", with an "Approve in this tab instead" link for flows that ran in this same tab or whose window was closed (the app may not finish connecting from there). 4. **Back to the window that started it.** OAuth clients (ChatGPT, claude.ai) open `/oauth/authorize` in a popup and expect the redirect back in that window, which holds their pending state. The email link opens a new tab, and a redirect from there fails ("Missing OAuth callback data"). So the "check your email" page polls `GET /oauth/status` every 2.5 seconds for up to 15 minutes. - `/oauth/status` answers only for the request named in the browser's signed cookie: `pending`, `ready`, `done` or `expired`. It sends `Cache-Control: no-store`. - - On `ready`, the page moves to `GET /oauth/approve`. That page requires the same cookie and an owner already bound, which only step 3 does. It renders consent with a fresh CSRF token. + - On `ready`, the page moves to `GET /oauth/approve`. That page requires the same cookie and an owner already bound, which only step 3 does. It renders consent with the request's CSRF token from the cookie, minting one only if the cookie has none. Every window of the browser shares that cookie, so a second load (the link tab's fallback, a reload) must not replace the token: that would fail the Allow form already open in the popup. - Both endpoints reveal nothing without the cookie. Neither adds a path around the same-browser binding. - A no-JavaScript fallback link, "I've clicked the link, continue", goes to the same page. -5. **Consent**, `POST /oauth/decision`. The page names the client and its redirect host. Allow consumes the request atomically and mints a 60-second single-use code bound to the full tuple (client, owner, redirect URI, challenge, resource, scope). Deny redirects with `error=access_denied`. +5. **Consent**, `POST /oauth/decision`. The page names the client and its redirect host. Allow consumes the request atomically and mints a 60-second single-use code bound to the full tuple (client, owner, redirect URI, challenge, resource, scope). Deny also consumes the request, so an Allow form still open in another window can't mint a code afterwards, then redirects with `error=access_denied`. ## §4 Tokens `POST /oauth/token` (form-encoded): - **`authorization_code`.** The code is consumed atomically, so a replay is dead. The server re-verifies `client_id`, the exact `redirect_uri`, PKCE (`base64url(sha256(code_verifier))`) and `resource` if it is resent. It returns an access token (1 hour), a refresh token (30 days), `token_type: Bearer` and the scope. -- **`refresh_token`.** The token rotates on every use. **Reuse detection:** presenting an already-consumed refresh token revokes the whole chain. If `client_id` is sent, it must be the token's own client. +- **`refresh_token`.** The token rotates on every use. **Reuse detection:** presenting an already-consumed refresh token revokes the whole chain. If `client_id` is sent, it must be the token's own client. That is checked before the token is consumed, so a mismatch (`invalid_client`) leaves the token usable by its own client. - Errors use RFC 6749 §5.2 JSON (`invalid_grant`, `invalid_client`, `invalid_target`, `unsupported_grant_type`). +- Every response, success or error, carries `Cache-Control: no-store` and `Pragma: no-cache` (RFC 6749 §5.1). A failed refresh logs `[oauth] refresh failed` with its status (`reused` means the chain was just revoked) and the first 8 characters of `client_id`, so a dropped connection shows in `wrangler tail`. ## §5 The `/mcp` resource server diff --git a/packages/api/src/mcp/handler.test.ts b/packages/api/src/mcp/handler.test.ts index 99532f9..c8b3fa1 100644 --- a/packages/api/src/mcp/handler.test.ts +++ b/packages/api/src/mcp/handler.test.ts @@ -220,6 +220,13 @@ describe('/mcp handler', () => { } }); + it('ping answers an empty result, anonymously too', async () => { + const pong = (await (await rpc(null, { jsonrpc: '2.0', id: 'p1', method: 'ping' })).json()) as Rpc; + expect(pong.error).toBeUndefined(); + expect(pong.result).toEqual({}); + expect(pong.id).toBe('p1'); + }); + it('unknown method → -32601; malformed JSON body → -32700', async () => { makeOwner('ow_e', 'e@example.com'); const token = await mintToken({ ownerId: 'ow_e', scope: 'registry:read' }); diff --git a/packages/api/src/mcp/handler.ts b/packages/api/src/mcp/handler.ts index 4783a5f..32b6bac 100644 --- a/packages/api/src/mcp/handler.ts +++ b/packages/api/src/mcp/handler.ts @@ -1256,6 +1256,9 @@ app.post('/mcp', bearerMiddleware, async (c) => { instructions: chatgptMeta.instructions, }); } + case 'ping': + // MCP basic utilities: a receiver MUST answer ping promptly with an empty result. + return rpcResult(c, id, {}); case 'tools/list': return rpcResult(c, id, { tools: TOOLS.map((t) => ({ diff --git a/packages/api/src/mcp/oauth-store.test.ts b/packages/api/src/mcp/oauth-store.test.ts index 1f82117..1f7641b 100644 --- a/packages/api/src/mcp/oauth-store.test.ts +++ b/packages/api/src/mcp/oauth-store.test.ts @@ -200,6 +200,13 @@ describe('oauth_refresh_tokens — rotation + reuse detection', () => { expect((await store.rotateRefreshToken(token, iso())).status).toBe('invalid'); }); + it('another client_id → client_mismatch, and the token stays usable by its own client', async () => { + seedOwner('ow_a'); + const { token } = await store.mintRefreshToken({ clientId: 'oc_1', ownerId: 'ow_a', resource: RESOURCE, scope: 'board:post' }); + expect((await store.rotateRefreshToken(token, iso(), 'oc_2')).status).toBe('client_mismatch'); + expect((await store.rotateRefreshToken(token, iso(), 'oc_1')).status).toBe('ok'); + }); + it('REUSE of a consumed token revokes the ENTIRE successor chain', async () => { seedOwner('ow_a'); const { token: r0 } = await store.mintRefreshToken({ clientId: 'oc_1', ownerId: 'ow_a', resource: RESOURCE, scope: 'board:post' }); diff --git a/packages/api/src/mcp/oauth-store.ts b/packages/api/src/mcp/oauth-store.ts index 4a2f0f3..fdfc130 100644 --- a/packages/api/src/mcp/oauth-store.ts +++ b/packages/api/src/mcp/oauth-store.ts @@ -118,7 +118,8 @@ export interface AccessTokenRow { export type RotateResult = | { status: 'ok'; refresh_token: string; owner_id: string; client_id: string; resource: string; scope: string } | { status: 'reused' } // presented an already-consumed token → chain revoked - | { status: 'invalid' }; // unknown / revoked / expired + | { status: 'invalid' } // unknown / revoked / expired + | { status: 'client_mismatch' }; // live token presented by another client_id; left unconsumed // Narrow raw-row typing without pulling a helper: every column we read is TEXT. type Raw = Record; @@ -506,6 +507,9 @@ export class OAuthStore { * - already CONSUMED (replay = theft) → walk `rotated_to` from the presented * row forward and revoke the entire successor chain, forcing full re-auth → * {status:'reused'} + * - live, but `expectedClientId` given and not the token's own client + * → {status:'client_mismatch'}, token left + * untouched (a mismatch must not burn the legitimate holder's token) * - live + unconsumed → atomically consume it (SET consumed_at, * rotated_to=; changes===1 is the gate), then mint the successor → * {status:'ok', refresh_token, …binding} @@ -514,7 +518,7 @@ export class OAuthStore { * consume, changes!==1 and we mint no successor (no orphan valid token). The * caller mints the paired access token from the returned binding. */ - async rotateRefreshToken(token: string, nowIso: string): Promise { + async rotateRefreshToken(token: string, nowIso: string, expectedClientId?: string): Promise { const presentedHash = sha256hex(token); const row = await this.db.get( `SELECT client_id, owner_id, resource, scope, expires_at, rotated_to, @@ -533,6 +537,9 @@ export class OAuthStore { if (row.revoked_at != null || s(row.expires_at) <= nowIso) { return { status: 'invalid' }; } + if (expectedClientId !== undefined && expectedClientId !== s(row.client_id)) { + return { status: 'client_mismatch' }; + } const binding = { client_id: s(row.client_id), diff --git a/packages/api/src/mcp/oauth.test.ts b/packages/api/src/mcp/oauth.test.ts index d0452a0..658d37e 100644 --- a/packages/api/src/mcp/oauth.test.ts +++ b/packages/api/src/mcp/oauth.test.ts @@ -564,6 +564,41 @@ describe('sign-in completes in the window that started it', () => { expect(await status(appr.cookie)).toBe('done'); }); + it('a second Approve load (the link tab or a reload) keeps the popup\'s Allow form valid', async () => { + const { popupCookie, lt, req } = await startAndSend('twice@example.com', 'st-twice'); + const linkTab = await app.request(`/oauth/continue?lt=${encodeURIComponent(lt)}&req=${encodeURIComponent(req)}`, { headers: { Cookie: popupCookie } }, ENV); + const jar = cookieOf(linkTab); + // The popup polls to ready and renders its Allow form first... + const popup = await approve(jar); + // ...then the link tab's "Approve in this tab instead" loads the page again + // in the same browser, re-arming the shared cookie. + const linkTabApprove = await approve(popup.cookie); + expect(linkTabApprove.csrf).toBe(popup.csrf); + // The browser now holds the cookie from the latest load; the popup's form + // (rendered earlier) must still verify against it. + const dec = await app.request('/oauth/decision', { method: 'POST', headers: { ...FORM, Cookie: linkTabApprove.cookie }, body: new URLSearchParams({ req, csrf: popup.csrf, decision: 'allow' }).toString() }, ENV); + expect(dec.status).toBe(302); + const loc = new URL(dec.headers.get('location')!); + expect(loc.searchParams.get('code')).toBeTruthy(); + expect(loc.searchParams.get('state')).toBe('st-twice'); + }); + + it('Deny is final: an Allow form still open in another window cannot mint a code afterwards', async () => { + const { popupCookie, lt, req } = await startAndSend('denyfinal@example.com', 'st-deny'); + const jar = cookieOf(await app.request(`/oauth/continue?lt=${encodeURIComponent(lt)}&req=${encodeURIComponent(req)}`, { headers: { Cookie: popupCookie } }, ENV)); + const popup = await approve(jar); + const linkTab = await approve(popup.cookie); // same browser, same token + const deny = await app.request('/oauth/decision', { method: 'POST', headers: { ...FORM, Cookie: linkTab.cookie }, body: new URLSearchParams({ req, csrf: linkTab.csrf, decision: 'deny' }).toString() }, ENV); + expect(deny.status).toBe(302); + expect(new URL(deny.headers.get('location')!).searchParams.get('error')).toBe('access_denied'); + expect(await status(linkTab.cookie)).toBe('done'); + + const lateAllow = await app.request('/oauth/decision', { method: 'POST', headers: { ...FORM, Cookie: linkTab.cookie }, body: new URLSearchParams({ req, csrf: popup.csrf, decision: 'allow' }).toString() }, ENV); + expect(lateAllow.status).toBe(400); + const codes = rawDb.prepare('SELECT COUNT(*) AS n FROM oauth_auth_codes').get() as { n: number }; + expect(codes.n).toBe(0); + }); + it('status and approve answer only for the cookie: no cookie or a forged one gets nothing', async () => { const { lt, req, popupCookie } = await startAndSend('nocookie@example.com'); await app.request(`/oauth/continue?lt=${encodeURIComponent(lt)}&req=${encodeURIComponent(req)}`, { headers: { Cookie: popupCookie } }, ENV); @@ -622,6 +657,19 @@ describe('/oauth/token', () => { expect(body.scope).toBe('registry:read board:post'); }); + it('marks every token response, success or error, as not cacheable', async () => { + const verifier = 'nostore-verifier-0000000'; + const { cid, code } = await freshCode('nostore@example.com', verifier); + const ok = await tokenExchange({ grant_type: 'authorization_code', code, client_id: cid, redirect_uri: REDIRECT, code_verifier: verifier }); + expect(ok.status).toBe(200); + const bad = await tokenExchange({ grant_type: 'refresh_token', refresh_token: 'not-a-real-refresh-token', client_id: cid }); + expect(bad.status).toBe(400); + for (const res of [ok, bad]) { + expect(res.headers.get('cache-control')).toBe('no-store'); + expect(res.headers.get('pragma')).toBe('no-cache'); + } + }); + it('rejects a wrong PKCE verifier with invalid_grant', async () => { const { cid, code } = await freshCode('t2@example.com', 'right-verifier-111111'); const res = await tokenExchange({ grant_type: 'authorization_code', code, client_id: cid, redirect_uri: REDIRECT, code_verifier: 'WRONG-verifier' }); @@ -680,6 +728,20 @@ describe('/oauth/token', () => { expect(live.n).toBe(0); }); + it('a refresh from the wrong client_id is refused without burning the token', async () => { + const verifier = 'mismatch-verifier-666666'; + const { cid, code } = await freshCode('t7@example.com', verifier); + const first = (await (await tokenExchange({ grant_type: 'authorization_code', code, client_id: cid, redirect_uri: REDIRECT, code_verifier: verifier })).json()) as { refresh_token: string }; + + const wrong = await tokenExchange({ grant_type: 'refresh_token', refresh_token: first.refresh_token, client_id: 'oc_someone_else' }); + expect(wrong.status).toBe(400); + expect((await wrong.json()) as { error: string }).toMatchObject({ error: 'invalid_client' }); + + // The token's own client can still use it: nothing was consumed or revoked. + const right = await tokenExchange({ grant_type: 'refresh_token', refresh_token: first.refresh_token, client_id: cid }); + expect(right.status).toBe(200); + }); + it('rejects an unsupported grant_type', async () => { const res = await tokenExchange({ grant_type: 'password', username: 'x', password: 'y' }); expect(res.status).toBe(400); diff --git a/packages/api/src/mcp/oauth.ts b/packages/api/src/mcp/oauth.ts index 199f0d6..58597ad 100644 --- a/packages/api/src/mcp/oauth.ts +++ b/packages/api/src/mcp/oauth.ts @@ -535,17 +535,28 @@ function authreqStatus(row: AuthRequestRow | null, now: string): 'pending' | 're } /** - * The Approve page, with a fresh CSRF token re-armed into the cookie. Shows + * The Approve page, re-arming the cookie with the request's existing CSRF + * token (a fresh one only when the cookie has none). Reusing it keeps every + * window of the same browser on one token: the popup and the email-link tab + * share the cookie jar, so minting a new token per load would let a second + * /oauth/approve load (the link tab's "Approve here instead", a reload) + * silently invalidate the Allow form already showing in the popup. Shows * WHAT is being authorized (defense in depth + informed consent): the * registered client name and the redirect host the code will be sent to, so * the human can recognize the app instead of blindly approving. */ -async function approvePage(c: Context, config: McpConfig, store: OAuthStore, row: AuthRequestRow): Promise { +async function approvePage( + c: Context, + config: McpConfig, + store: OAuthStore, + row: AuthRequestRow, + existingCsrf: string | undefined, +): Promise { const client = await store.getClient(row.client_id); const clientLabel = client?.client_name || 'An application'; let redirectHost = ''; try { redirectHost = new URL(row.redirect_uri).host; } catch { redirectHost = ''; } - const csrf = newCsrfToken(); + const csrf = existingCsrf || newCsrfToken(); setCookie(c, AUTHREQ_COOKIE, await signCookie(config.signingSecret, { authreq_id: row.id, csrf }), { httpOnly: true, secure: true, @@ -615,7 +626,8 @@ app.get('/oauth/approve', async (c) => { 400, ); } - return approvePage(c, config, store, row); + // The row was looked up by this cookie's authreq_id, so its csrf belongs to it. + return approvePage(c, config, store, row, cookie?.csrf); }); // ── GET /oauth/continue (magic-link landing) ── @@ -690,7 +702,8 @@ app.get('/oauth/continue', async (c) => { 'Signed in', `

You're signed in

Go back to the window where you started connecting. It continues by itself and asks you to approve there. You can close this tab.

-

Don't see that window anymore? Approve here instead.

`, +

Closed that window? Approve in this tab instead. The app may not + finish connecting from here; if it doesn't, restart the connection from the app.

`, ), ); }); @@ -725,6 +738,12 @@ app.post('/oauth/decision', async (c) => { // Deny → redirect with access_denied (RFC 6749 §4.1.2.1), no code minted. if (decision !== 'allow') { + // Deny is final: consume the request so an Allow form still open in + // another window of this browser (they share one CSRF token) cannot mint + // a code after the user said no, and polling windows see 'done'. + if (!(await store.consumeAuthRequest(req, now))) { + return oauthError(c, 400, 'invalid_request', 'authorization request already used or expired'); + } const url = new URL(row.redirect_uri); url.searchParams.set('error', 'access_denied'); if (row.state) url.searchParams.set('state', row.state); @@ -757,6 +776,10 @@ app.post('/oauth/token', async (c) => { const config = cfg(c); const db = getDb(c); const store = new OAuthStore(db); + // RFC 6749 §5.1: responses carrying tokens MUST NOT be cached. Set once, so + // every success and error body from this endpoint carries it. + c.header('Cache-Control', 'no-store'); + c.header('Pragma', 'no-cache'); const form = await c.req.parseBody(); const grantType = typeof form.grant_type === 'string' ? form.grant_type : ''; const now = nowIso(); @@ -820,14 +843,17 @@ app.post('/oauth/token', async (c) => { return oauthError(c, 400, 'invalid_grant', 'refresh_token required'); } // Rotate with reuse-detection: a replayed (already-consumed) token revokes - // the whole chain and fails; unknown/expired/revoked also fail. - const rot = await store.rotateRefreshToken(presented, now); + // the whole chain and fails; unknown/expired/revoked also fail. If the + // client identifies itself it must be the token's own client, checked + // before the token is consumed so a mismatch doesn't burn it. + const rot = await store.rotateRefreshToken(presented, now, clientId); if (rot.status !== 'ok') { - return oauthError(c, 400, 'invalid_grant', 'refresh token invalid, expired, or reused'); - } - // If the client identifies itself it must be the token's own client. - if (clientId !== undefined && clientId !== rot.client_id) { - return oauthError(c, 400, 'invalid_client', 'client_id does not match the refresh token'); + // A failed refresh ends that connection until the user signs in again, + // so leave a trace: 'reused' means the whole chain was just revoked. + console.warn('[oauth] refresh failed', { status: rot.status, client: clientId?.slice(0, 8) ?? 'none' }); + return rot.status === 'client_mismatch' + ? oauthError(c, 400, 'invalid_client', 'client_id does not match the refresh token') + : oauthError(c, 400, 'invalid_grant', 'refresh token invalid, expired, or reused'); } const access = await store.mintAccessToken({ clientId: rot.client_id,