diff --git a/backend/CLAUDE.md b/backend/CLAUDE.md index 9c681a4d..f1a48a97 100644 --- a/backend/CLAUDE.md +++ b/backend/CLAUDE.md @@ -71,7 +71,9 @@ thin: it proxies OpenAI requests with the server-held API key and writes study l - **Auth** (`src/auth.ts`): Better Auth (Google sign-in + device-code flow for the add-in), on the shared `app.db`, enabled by `BETTER_AUTH_ENABLED=true`. Carries the user's `loggingConsent` level as a user field; `beforeDelete` purges study logs and - anonymizes usage rows. + anonymizes usage rows. The fixed public Mindmap OAuth client is provisioned through + Better Auth's adapter at startup (`oauth-clients.ts`); app migration v7 performs the + one-time cleanup of clients created while dynamic registration was enabled. ## Commands @@ -90,7 +92,9 @@ pays for sessionless requests — without it they're refused wherever auth is on "delete my data" to purge a user's PostHog person; unset => that step no-ops), `LOG_DIR` (overrides just the logs subdir). Auth: `BETTER_AUTH_ENABLED`, `BETTER_AUTH_SECRET`, `BETTER_AUTH_URL`, `BETTER_AUTH_TRUSTED_ORIGINS`, `GOOGLE_CLIENT_ID`, -`GOOGLE_CLIENT_SECRET`. For local dev, run `python scripts/get_env.py` to generate +`GOOGLE_CLIENT_SECRET`, `MINDMAP_OAUTH_CLIENT_ID`, and +`MINDMAP_OAUTH_REDIRECT_URIS` (comma-separated exact callback URLs). For local +dev, run `python scripts/get_env.py` to generate `backend/.env`; it prompts for `OPENAI_DEMO_API_KEY` too and defaults it to the main key, since an unset demo key makes every demo/anonymous request 401 once auth is enabled. diff --git a/backend/README.md b/backend/README.md index 3b0e2d19..c714b204 100644 --- a/backend/README.md +++ b/backend/README.md @@ -46,4 +46,5 @@ key for it locally), for `app.db` + `logs/`), `PORT` (default 8000), `DEBUG`, `POSTHOG_PROJECT_TOKEN`, `POSTHOG_HOST`, `LOG_DIR` (overrides just the logs subdir). Auth (Better Auth) adds `BETTER_AUTH_ENABLED`, `BETTER_AUTH_SECRET`, `GOOGLE_CLIENT_ID`, `GOOGLE_CLIENT_SECRET`, -and related vars — see [CLAUDE.md](CLAUDE.md) for the full list. +`MINDMAP_OAUTH_CLIENT_ID`, `MINDMAP_OAUTH_REDIRECT_URIS`, and related vars — see +[CLAUDE.md](CLAUDE.md) for the full list. diff --git a/backend/package-lock.json b/backend/package-lock.json index 84135932..1bf60817 100644 --- a/backend/package-lock.json +++ b/backend/package-lock.json @@ -8,6 +8,7 @@ "name": "writing-tools-backend", "version": "1.0.0", "dependencies": { + "@better-auth/oauth-provider": "^1.6.22", "@hono/node-server": "^1.13.7", "better-auth": "1.6.22", "better-sqlite3": "^12.0.0", @@ -110,6 +111,23 @@ } } }, + "node_modules/@better-auth/oauth-provider": { + "version": "1.6.22", + "resolved": "https://registry.npmjs.org/@better-auth/oauth-provider/-/oauth-provider-1.6.22.tgz", + "integrity": "sha512-xXFoRqP6pUFbYsgC3eTJ1d+DijcKJVJnq1KAMlaocRM2UNwRlXwrV4zI+GMGCAgBYHrcEhoxxn17IYUGXy6dOw==", + "license": "MIT", + "dependencies": { + "jose": "^6.1.3", + "zod": "^4.3.6" + }, + "peerDependencies": { + "@better-auth/core": "^1.6.22", + "@better-auth/utils": "0.4.2", + "@better-fetch/fetch": "1.3.1", + "better-auth": "^1.6.22", + "better-call": "1.3.7" + } + }, "node_modules/@better-auth/prisma-adapter": { "version": "1.6.22", "resolved": "https://registry.npmjs.org/@better-auth/prisma-adapter/-/prisma-adapter-1.6.22.tgz", diff --git a/backend/package.json b/backend/package.json index 780ae0fa..eb4e2b41 100644 --- a/backend/package.json +++ b/backend/package.json @@ -13,6 +13,7 @@ "test:watch": "vitest" }, "dependencies": { + "@better-auth/oauth-provider": "^1.6.22", "@hono/node-server": "^1.13.7", "better-auth": "1.6.22", "better-sqlite3": "^12.0.0", diff --git a/backend/src/__tests__/app.test.ts b/backend/src/__tests__/app.test.ts index 5dd31b6a..6a279617 100644 --- a/backend/src/__tests__/app.test.ts +++ b/backend/src/__tests__/app.test.ts @@ -246,6 +246,9 @@ describe('DELETE /api/me/activity', () => { const res = await authApp.request('/api/me/activity', { method: 'DELETE' }); expect(res.status).toBe(200); + expect(await res.json()).toEqual({ + message: 'Your logged activity has been erased.', + }); await expect(readUserLog('usr-del')).rejects.toThrow(); }); diff --git a/backend/src/__tests__/db.test.ts b/backend/src/__tests__/db.test.ts index 5493ec99..b6012f0e 100644 --- a/backend/src/__tests__/db.test.ts +++ b/backend/src/__tests__/db.test.ts @@ -22,7 +22,7 @@ afterEach(() => { describe('migrations', () => { it('creates our schema and records the version', () => { const conn = db(); - expect(conn.pragma('user_version', { simple: true })).toBe(3); + expect(conn.pragma('user_version', { simple: true })).toBe(7); // The table is usable, not merely declared. const table = conn @@ -45,6 +45,11 @@ describe('migrations', () => { ) .get(); expect(grantTable).toBeDefined(); + + // v6 removes the now-unnecessary transient OAuth room selection. + expect( + conn.prepare(`SELECT name FROM sqlite_master WHERE name='oauth_room_selection'`).get(), + ).toBeUndefined(); }); it('is idempotent across reopens — a second open re-runs nothing', () => { @@ -58,12 +63,46 @@ describe('migrations', () => { // Reopening must not drop or recreate the table (CREATE TABLE would throw). const conn = db(); - expect(conn.pragma('user_version', { simple: true })).toBe(3); + expect(conn.pragma('user_version', { simple: true })).toBe(7); const rows = conn.prepare(`SELECT COUNT(*) AS n FROM llm_usage`).get() as { n: number; }; expect(rows.n).toBe(1); }); + + it('removes stale dynamic OAuth clients once when upgrading from v6', () => { + const file = path.join(dataDir, 'app.db'); + const legacy = new Database(file); + legacy.exec(` + CREATE TABLE oauthClient ( + id TEXT PRIMARY KEY, + clientId TEXT NOT NULL UNIQUE + ); + INSERT INTO oauthClient (id, clientId) VALUES + ('trusted', 'configured-mindmap'), + ('stale', 'old-dynamic-client'); + PRAGMA user_version = 6; + `); + legacy.close(); + process.env.MINDMAP_OAUTH_CLIENT_ID = 'configured-mindmap'; + + const conn = db(); + expect(conn.pragma('user_version', { simple: true })).toBe(7); + expect(conn.prepare(`SELECT clientId FROM oauthClient`).all()).toEqual([ + { clientId: 'configured-mindmap' }, + ]); + + // Reopening at v7 must not repeat cleanup or remove a later client. + conn.prepare( + `INSERT INTO oauthClient (id, clientId) VALUES ('later', 'later-fixed-client')`, + ).run(); + closeDb(); + expect(db().prepare(`SELECT clientId FROM oauthClient ORDER BY clientId`).all()) + .toEqual([ + { clientId: 'configured-mindmap' }, + { clientId: 'later-fixed-client' }, + ]); + }); }); describe('legacy auth.db rename', () => { @@ -87,7 +126,7 @@ describe('legacy auth.db rename', () => { expect(user).toEqual({ email: 'a@b.c' }); // ...and our migrations then run on top of the adopted database. - expect(conn.pragma('user_version', { simple: true })).toBe(3); + expect(conn.pragma('user_version', { simple: true })).toBe(7); }); it('leaves an existing app.db alone when a stray auth.db is also present', async () => { diff --git a/backend/src/__tests__/erasure.test.ts b/backend/src/__tests__/erasure.test.ts index 8e1825fa..93fba838 100644 --- a/backend/src/__tests__/erasure.test.ts +++ b/backend/src/__tests__/erasure.test.ts @@ -4,9 +4,10 @@ import { tmpdir } from 'node:os'; import path from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { closeDb } from '../db.js'; -import { eraseLoggedData } from '../erasure.js'; +import { eraseAccountData, eraseLoggedData } from '../erasure.js'; import { appendLog } from '../logging.js'; import { deletePosthogPerson } from '../posthog.js'; +import { createRoom, listRooms } from '../rooms.js'; import { DELETED_USER_ID, recordUsage, @@ -73,19 +74,37 @@ function logExists(userId: string): boolean { } describe('eraseLoggedData', () => { - it('removes the study log and the analytics profile together', async () => { + it('removes logged activity while preserving active rooms', async () => { await logSomething('usr-1'); + createRoom('usr-1', 'Private draft', { + beforeCursor: 'private', selectedText: '', afterCursor: '', + }); expect(logExists('usr-1')).toBe(true); + expect(listRooms('usr-1')).toHaveLength(1); await eraseLoggedData('usr-1'); expect(logExists('usr-1')).toBe(false); + expect(listRooms('usr-1')).toHaveLength(1); expect(deletePosthogPerson).toHaveBeenCalledWith('usr-1'); }); }); describe('account deletion (Better Auth beforeDelete)', () => { - it('does everything withdrawal does, and anonymizes the usage rows on top', async () => { + it('still deletes rooms when a shared activity-erasure operation fails', async () => { + const invalidLogKey = '../usr-partial'; + createRoom(invalidLogKey, 'Private draft', { + beforeCursor: 'private', selectedText: '', afterCursor: '', + }); + + await expect(eraseAccountData(invalidLogKey)).rejects.toBeInstanceOf( + AggregateError, + ); + expect(deletePosthogPerson).toHaveBeenCalledWith(invalidLogKey); + expect(listRooms(invalidLogKey)).toEqual([]); + }); + + it('includes every activity erasure, then deletes rooms and anonymizes usage', async () => { // Imported here, not at module load: auth.ts opens the DB as a side effect of // evaluation, so it has to see this test's temp DATA_DIR. process.env.BETTER_AUTH_SECRET = 'x'.repeat(32); @@ -96,15 +115,19 @@ describe('account deletion (Better Auth beforeDelete)', () => { await logSomething('usr-gone'); meterSomething('usr-gone'); + createRoom('usr-gone', 'Private draft', { + beforeCursor: 'private', selectedText: '', afterCursor: '', + }); const beforeDelete = auth.options.user?.deleteUser?.beforeDelete; expect(beforeDelete).toBeDefined(); // Better Auth hands the hook the full user record; only the id is read. await beforeDelete?.({ id: 'usr-gone' } as never); - // The withdrawal erasure — this is the half account deletion used to skip. + // Full account erasure includes both activity records and room snapshots. expect(logExists('usr-gone')).toBe(false); expect(deletePosthogPerson).toHaveBeenCalledWith('usr-gone'); + expect(listRooms('usr-gone')).toEqual([]); // ...plus the tombstone: the spend survives, detached from the person. const rows = summarizeUsage(0, Date.now() + 1000); diff --git a/backend/src/__tests__/oauth-room-authorization.test.ts b/backend/src/__tests__/oauth-room-authorization.test.ts new file mode 100644 index 00000000..7248716c --- /dev/null +++ b/backend/src/__tests__/oauth-room-authorization.test.ts @@ -0,0 +1,29 @@ +import { describe, expect, it } from 'vitest'; +import { parseOAuthAuthorizationQuery } from '../oauth-room-authorization.js'; + +describe('OAuth room authorization context', () => { + it('derives the exact launched room and client from the verified query', () => { + expect( + parseOAuthAuthorizationQuery( + new URLSearchParams({ + state: 'room_exact.random-csrf', + client_id: 'dynamic-client', + redirect_uri: 'https://mindmap.example/callback', + }).toString(), + ), + ).toEqual({ + state: 'room_exact.random-csrf', + roomId: 'room_exact', + clientId: 'dynamic-client', + redirectUri: 'https://mindmap.example/callback', + }); + }); + + it('fails closed when the authorization lacks a room-bound state', () => { + expect(() => + parseOAuthAuthorizationQuery( + 'state=random&client_id=client&redirect_uri=https%3A%2F%2Fapp.example', + ), + ).toThrow(); + }); +}); diff --git a/backend/src/__tests__/oauth-room-middleware.integration.test.ts b/backend/src/__tests__/oauth-room-middleware.integration.test.ts new file mode 100644 index 00000000..bc6ffea8 --- /dev/null +++ b/backend/src/__tests__/oauth-room-middleware.integration.test.ts @@ -0,0 +1,264 @@ +import { createHash } from 'node:crypto'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'; +import { getMigrations } from 'better-auth/db/migration'; +import { oauthProviderResourceClient } from '@better-auth/oauth-provider/resource-client'; +import type { Auth } from '../auth.js'; +import { createApp } from '../app.js'; +import { closeDb, db } from '../db.js'; +import { provisionTrustedMindmapClient } from '../oauth-clients.js'; +import { createRoom } from '../rooms.js'; + +const AUTH_BASE = 'http://localhost:8000/api/auth'; +const CLIENT_ID = 'integration-mindmap'; +const REDIRECT_URI = 'https://mindmap.example/'; +let auth: Auth; +let roomConsentReferenceId: typeof import('../auth.js').roomConsentReferenceId; +let dataDir: string; + +function authRequest(pathname: string, init?: RequestInit): Promise { + return auth.handler(new Request(`${AUTH_BASE}${pathname}`, init)); +} + +function cookieHeader(response: Response): string { + const headers = response.headers as Headers & { getSetCookie?: () => string[] }; + const values = headers.getSetCookie?.() ?? [response.headers.get('set-cookie') ?? '']; + return values.filter(Boolean).map((value) => value.split(';', 1)[0]).join('; '); +} + +async function anonymousSession(): Promise<{ cookie: string; userId: string }> { + const response = await authRequest('/sign-in/anonymous', { + method: 'POST', + headers: { 'Content-Type': 'application/json', Origin: 'http://localhost:8000' }, + body: '{}', + }); + expect(response.status).toBe(200); + const body = (await response.json()) as { user: { id: string } }; + return { cookie: cookieHeader(response), userId: body.user.id }; +} + +function authorizeUrl(roomId: string, challenge: string, clientId = CLIENT_ID): URL { + const url = new URL(`${AUTH_BASE}/oauth2/authorize`); + url.search = new URLSearchParams({ + client_id: clientId, + redirect_uri: REDIRECT_URI, + response_type: 'code', + scope: 'openai:chat doc:read', + resource: 'http://localhost:8000', + state: `${roomId}.random-csrf`, + code_challenge: challenge, + code_challenge_method: 'S256', + }).toString(); + return url; +} + +beforeAll(async () => { + dataDir = mkdtempSync(path.join(tmpdir(), 'writing-tools-oauth-integration-')); + process.env.DATA_DIR = dataDir; + process.env.BETTER_AUTH_SECRET = 'integration-secret-that-is-at-least-32-characters'; + process.env.BETTER_AUTH_URL = 'http://localhost:8000'; + process.env.GOOGLE_CLIENT_ID = 'test-client'; + process.env.GOOGLE_CLIENT_SECRET = 'test-secret'; + process.env.MINDMAP_OAUTH_CLIENT_ID = CLIENT_ID; + process.env.MINDMAP_OAUTH_REDIRECT_URIS = REDIRECT_URI; + process.env.OPENAI_API_KEY = 'test-openai-key'; + process.env.OPENAI_DEMO_API_KEY = 'test-demo-openai-key'; + ({ auth, roomConsentReferenceId } = await import('../auth.js')); + const { runMigrations } = await getMigrations(auth.options); + await runMigrations(); + await provisionTrustedMindmapClient(auth); +}); + +afterAll(() => { + vi.unstubAllGlobals(); + closeDb(); + rmSync(dataDir, { recursive: true, force: true }); +}); + +describe('trusted Mindmap OAuth launch', () => { + it('idempotently provisions the configured trusted client', async () => { + await provisionTrustedMindmapClient(auth); + const clients = db().prepare( + `SELECT clientId, skipConsent, requirePKCE FROM oauthClient`, + ).all(); + expect(clients).toEqual([{ + clientId: CLIENT_ID, + skipConsent: 1, + requirePKCE: 1, + }]); + }); + + it('issues a real room-bound token with no consent or room screen', async () => { + const { cookie, userId } = await anonymousSession(); + const room = createRoom(userId, 'Signed query draft', { + beforeCursor: 'private', selectedText: '', afterCursor: '', + }); + const other = createRoom(userId, 'Other draft', { + beforeCursor: 'other', selectedText: '', afterCursor: '', + }); + const verifier = 'mindmap-pkce-verifier-that-is-long-enough-for-the-test-123456'; + const challenge = createHash('sha256').update(verifier).digest('base64url'); + + const authorization = await auth.handler( + new Request(authorizeUrl(room.id, challenge), { headers: { Cookie: cookie } }), + ); + expect(authorization.status).toBe(302); + const callback = new URL(authorization.headers.get('location') ?? ''); + expect(callback.origin + callback.pathname).toBe(REDIRECT_URI); + const code = callback.searchParams.get('code'); + expect(code).toBeTruthy(); + + const tokenResponse = await authRequest('/oauth2/token', { + method: 'POST', + headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, + body: new URLSearchParams({ + grant_type: 'authorization_code', client_id: CLIENT_ID, + redirect_uri: REDIRECT_URI, code: code!, code_verifier: verifier, + resource: 'http://localhost:8000', + }), + }); + expect(tokenResponse.status).toBe(200); + const token = (await tokenResponse.json()) as { + access_token: string; room_id: string; + }; + expect(token.room_id).toBe(room.id); + const payload = JSON.parse( + Buffer.from(token.access_token.split('.')[1]!, 'base64url').toString(), + ) as Record; + expect(payload.room_id).toBe(room.id); + const jwks = await authRequest('/jwks').then((response) => response.json()) as { + keys: Array>; + }; + expect(jwks).toMatchObject({ keys: expect.any(Array) }); + const providerVerifier = oauthProviderResourceClient(auth).getActions().verifyAccessToken; + const verifyOAuthAccessToken = ( + accessToken: string, + options: { + verifyOptions: { audience: string; issuer: string }; + scopes: string[]; + }, + ) => providerVerifier(accessToken, { + ...options, + // The production verifier fetches this endpoint over HTTP. Supplying the + // same real JWKS as a function keeps this integration test in-process. + jwksUrl: (async () => jwks) as unknown as string, + }); + + vi.stubGlobal('fetch', vi.fn(async () => + new Response(JSON.stringify({ + id: 'chatcmpl_test', model: 'gpt-4o', choices: [], + usage: { prompt_tokens: 1, completion_tokens: 1, total_tokens: 2 }, + }), { status: 200, headers: { 'Content-Type': 'application/json' } }), + )); + await expect(verifyOAuthAccessToken( + token.access_token, + { + verifyOptions: { + audience: 'http://localhost:8000', + issuer: 'http://localhost:8000/api/auth', + }, + scopes: ['doc:read'], + }, + )).resolves.toMatchObject({ room_id: room.id }); + const app = createApp({ auth, verifyOAuthAccessToken }); + const granted = await app.request(`/api/rooms/${room.id}`, { + headers: { Authorization: `Bearer ${token.access_token}` }, + }); + expect(granted.status).toBe(200); + expect(await granted.json()).toMatchObject({ id: room.id }); + + const denied = await app.request(`/api/rooms/${other.id}`, { + headers: { Authorization: `Bearer ${token.access_token}` }, + }); + expect(denied.status).toBe(403); + + const proxy = await app.request('/api/openai/chat/completions', { + method: 'POST', + headers: { + Authorization: `Bearer ${token.access_token}`, + 'Content-Type': 'application/json', + }, + body: JSON.stringify({ model: 'gpt-4o', messages: [] }), + }); + expect(proxy.status).toBe(200); + }); + + it('refuses registration, unknown clients, and rooms owned by another user', async () => { + const registration = await authRequest('/oauth2/register', { + method: 'POST', headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ redirect_uris: [REDIRECT_URI] }), + }); + expect(registration.status).toBe(403); + + const owner = await anonymousSession(); + const visitor = await anonymousSession(); + const room = createRoom(owner.userId, 'Owner only', { + beforeCursor: 'private', selectedText: '', afterCursor: '', + }); + const challenge = createHash('sha256').update('v'.repeat(64)).digest('base64url'); + const unknown = await auth.handler(new Request( + authorizeUrl(room.id, challenge, 'unregistered-client'), + { headers: { Cookie: visitor.cookie } }, + )); + const unknownLocation = new URL(unknown.headers.get('location') ?? REDIRECT_URI); + expect(unknownLocation.searchParams.get('code')).toBeNull(); + expect(unknownLocation.searchParams.get('error')).toBeTruthy(); + + const foreignRoom = await auth.handler(new Request( + authorizeUrl(room.id, challenge), + { headers: { Cookie: visitor.cookie } }, + )); + expect(foreignRoom.status).toBe(302); + const foreignLocation = new URL(foreignRoom.headers.get('location') ?? ''); + expect(foreignLocation.origin + foreignLocation.pathname).toBe(REDIRECT_URI); + expect(foreignLocation.searchParams.get('error')).toBe('access_denied'); + expect(foreignLocation.searchParams.get('error_description')) + .toBe('The launched room does not belong to this account.'); + expect(foreignLocation.searchParams.get('state')).toBe(`${room.id}.random-csrf`); + expect(foreignLocation.searchParams.get('code')).toBeNull(); + + const missingRoomId = 'room_missing-but-well-formed'; + const missingRoom = await auth.handler(new Request( + authorizeUrl(missingRoomId, challenge), + { headers: { Cookie: visitor.cookie } }, + )); + expect(missingRoom.status).toBe(302); + const missingLocation = new URL(missingRoom.headers.get('location') ?? ''); + expect(missingLocation.origin + missingLocation.pathname).toBe(REDIRECT_URI); + expect(missingLocation.searchParams.get('error')).toBe('access_denied'); + expect(missingLocation.searchParams.get('state')) + .toBe(`${missingRoomId}.random-csrf`); + expect(missingLocation.searchParams.get('code')).toBeNull(); + }); + + it('fails cleanly if the consent hook is reached without a session user', async () => { + await expect(roomConsentReferenceId({ + user: undefined, + scopes: ['doc:read'], + })).rejects.toMatchObject({ statusCode: 401 }); + }); + + it('observes an operational client disable without a process restart', async () => { + const { cookie, userId } = await anonymousSession(); + const room = createRoom(userId, 'Disabled client', { + beforeCursor: '', selectedText: '', afterCursor: '', + }); + const challenge = createHash('sha256').update('d'.repeat(64)).digest('base64url'); + db().prepare(`UPDATE oauthClient SET disabled = 1 WHERE clientId = ?`).run(CLIENT_ID); + + try { + const authorization = await auth.handler(new Request( + authorizeUrl(room.id, challenge), + { headers: { Cookie: cookie } }, + )); + expect(authorization.status).toBe(302); + const location = new URL(authorization.headers.get('location') ?? ''); + expect(location.searchParams.get('code')).toBeNull(); + expect(location.searchParams.get('error')).toBe('client_disabled'); + } finally { + db().prepare(`UPDATE oauthClient SET disabled = 0 WHERE clientId = ?`).run(CLIENT_ID); + } + }); +}); diff --git a/backend/src/__tests__/oauth-room-resource.test.ts b/backend/src/__tests__/oauth-room-resource.test.ts new file mode 100644 index 00000000..9cdab3a2 --- /dev/null +++ b/backend/src/__tests__/oauth-room-resource.test.ts @@ -0,0 +1,70 @@ +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { createApp } from '../app.js'; +import { closeDb } from '../db.js'; +import { createRoom } from '../rooms.js'; + +let dir: string; + +beforeEach(() => { + dir = mkdtempSync(path.join(tmpdir(), 'writing-tools-oauth-resource-')); + process.env.DATA_DIR = dir; +}); + +afterEach(() => { + closeDb(); + rmSync(dir, { recursive: true, force: true }); + delete process.env.DATA_DIR; +}); + +describe('GET /api/rooms/:roomId', () => { + it('serves only the room named by the verified claim and owned by its subject', async () => { + const room = createRoom('owner', 'Draft', { + beforeCursor: 'private', selectedText: '', afterCursor: '', + }); + const verifyOAuthAccessToken = vi.fn().mockResolvedValue({ + sub: 'owner', + room_id: room.id, + }); + const app = createApp({ verifyOAuthAccessToken }); + const response = await app.request(`/api/rooms/${room.id}`, { + headers: { Authorization: 'Bearer header.payload.signature' }, + }); + expect(response.status).toBe(200); + expect(await response.json()).toMatchObject({ + id: room.id, + doc: { beforeCursor: 'private' }, + }); + expect(verifyOAuthAccessToken).toHaveBeenCalledWith( + 'header.payload.signature', + { + verifyOptions: { + audience: expect.any(String), + issuer: expect.any(String), + }, + scopes: ['doc:read'], + }, + ); + }); + + it('rejects a valid token whose room claim does not match the path', async () => { + const requested = createRoom('owner', 'Requested', { + beforeCursor: '', selectedText: 'requested', afterCursor: '', + }); + const other = createRoom('owner', 'Other', { + beforeCursor: '', selectedText: 'other', afterCursor: '', + }); + const app = createApp({ + verifyOAuthAccessToken: vi.fn().mockResolvedValue({ + sub: 'owner', + room_id: other.id, + }), + }); + const response = await app.request(`/api/rooms/${requested.id}`, { + headers: { Authorization: 'Bearer header.payload.signature' }, + }); + expect(response.status).toBe(403); + }); +}); diff --git a/backend/src/__tests__/rooms.test.ts b/backend/src/__tests__/rooms.test.ts new file mode 100644 index 00000000..637e1667 --- /dev/null +++ b/backend/src/__tests__/rooms.test.ts @@ -0,0 +1,46 @@ +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { closeDb } from '../db.js'; +import { + createRoom, + deleteRoomsForUser, + getRoomForUser, + listRooms, +} from '../rooms.js'; + +let dir: string; + +beforeEach(() => { + dir = mkdtempSync(path.join(tmpdir(), 'writing-tools-rooms-')); + process.env.DATA_DIR = dir; +}); + +afterEach(() => { + closeDb(); + rmSync(dir, { recursive: true, force: true }); + delete process.env.DATA_DIR; +}); + +describe('rooms', () => { + it('keeps document snapshots private to their owner', () => { + const room = createRoom('owner', 'Draft', { + beforeCursor: 'hello', + selectedText: ' ', + afterCursor: 'world', + }); + expect(listRooms('owner')).toHaveLength(1); + expect(getRoomForUser(room.id, 'owner')?.doc.afterCursor).toBe('world'); + expect(getRoomForUser(room.id, 'other')).toBeNull(); + }); + + it('deletes durable rooms for erasure', async () => { + const room = createRoom('owner', 'Private draft', { + beforeCursor: 'private', selectedText: '', afterCursor: '', + }); + await deleteRoomsForUser('owner'); + expect(listRooms('owner')).toEqual([]); + expect(getRoomForUser(room.id, 'owner')).toBeNull(); + }); +}); diff --git a/backend/src/app.ts b/backend/src/app.ts index ba89c4d6..cacd7c77 100644 --- a/backend/src/app.ts +++ b/backend/src/app.ts @@ -1,6 +1,7 @@ import { Hono } from 'hono'; import type { Context } from 'hono'; import { cors } from 'hono/cors'; +import { oauthProviderResourceClient } from '@better-auth/oauth-provider/resource-client'; import type { Auth, SessionUser } from './auth.js'; // type-only import, no runtime cost import { CONSENT_LEVELS, @@ -8,8 +9,15 @@ import { filterExtraDataForConsent, isConsentLevel, } from './consent.js'; -import { deviceClientIds, gitCommit, logSecret } from './config.js'; +import { + betterAuthUrl, + BETTER_AUTH_BASE_PATH, + deviceClientIds, + gitCommit, + logSecret, +} from './config.js'; import { eraseLoggedData } from './erasure.js'; +import { db } from './db.js'; import { appendLog, pollLogs, zipLogs } from './logging.js'; import { attributeRequest, @@ -31,6 +39,7 @@ import { } from './toolGrants.js'; import { summarizeUsage } from './usage.js'; import { isUserAllowed } from './userAllowlist.js'; +import { createRoom, getRoomForUser, isRoomDoc } from './rooms.js'; // Mints short-lived ephemeral credentials so a browser can open a WebRTC // Realtime session without ever seeing the server API key. This is the only @@ -75,8 +84,31 @@ function logSecretGate(c: Context, provided: string): Response | null { return null; } -export function createApp({ auth }: { auth?: Auth } = {}): Hono { +type OAuthAccessTokenVerifier = ( + token: string, + options: { + verifyOptions: { audience: string; issuer: string }; + scopes: string[]; + }, +) => Promise>; + +export function createApp({ + auth, + verifyOAuthAccessToken: suppliedOAuthVerifier, +}: { + auth?: Auth; + /** Test seam for the resource-server boundary; production derives it from auth. */ + verifyOAuthAccessToken?: OAuthAccessTokenVerifier; +} = {}): Hono { const app = new Hono(); + const verifyOAuthAccessToken = + suppliedOAuthVerifier ?? + (auth?.options + ? oauthProviderResourceClient(auth).getActions().verifyAccessToken + : null); + // Better Auth signs OAuth JWTs with baseURL + basePath. Its resource helper can + // otherwise default to the bare baseURL, yielding opaque 401s for valid tokens. + const oauthIssuer = `${betterAuthUrl().replace(/\/$/, '')}${BETTER_AUTH_BASE_PATH}`; // CORS stays fully permissive for now to preserve existing behaviour. app.use('*', cors({ exposeHeaders: [PLATFORM_AUTH_ERROR_HEADER] })); @@ -196,6 +228,50 @@ export function createApp({ auth }: { auth?: Auth } = {}): Hono { if (bearer?.startsWith('wtk_')) { return resolveToolToken(bearer)?.user ?? null; } + if (bearer && verifyOAuthAccessToken) { + try { + const claims = await verifyOAuthAccessToken(bearer, { + verifyOptions: { audience: betterAuthUrl(), issuer: oauthIssuer }, + scopes: ['openai:chat'], + }); + if (typeof claims.sub !== 'string') return null; + const row = db() + .prepare( + `SELECT email, loggingConsent, isAnonymous, alwaysAllow + FROM user WHERE id = ?`, + ) + .get(claims.sub) as + | { + email?: string | null; + loggingConsent?: unknown; + isAnonymous?: number | boolean | null; + alwaysAllow?: number | boolean | null; + } + | undefined; + if (!row) return null; + const isAnonymous = row.isAnonymous === true || row.isAnonymous === 1; + return { + id: claims.sub, + loggingConsent: isConsentLevel(row.loggingConsent) + ? row.loggingConsent + : DEFAULT_CONSENT_LEVEL, + isAnonymous, + isAllowed: isUserAllowed({ + email: row.email, + isAnonymous, + alwaysAllow: row.alwaysAllow === true || row.alwaysAllow === 1, + }), + clientId: + typeof claims.azp === 'string' + ? claims.azp + : typeof claims.client_id === 'string' + ? claims.client_id + : null, + }; + } catch { + return null; + } + } if (!auth) return null; const session = await auth.api.getSession({ headers: c.req.raw.headers }); @@ -226,6 +302,59 @@ export function createApp({ auth }: { auth?: Auth } = {}): Hono { }; } + // A room is the durable resource the OAuth authorization points at. The add-in + // creates it with its normal user session; the browser app later reads it with + // an OAuth token whose signed room_id claim must match the path. + app.post('/api/rooms', async (c) => { + if (!auth) return c.json({ detail: 'Authentication is disabled.' }, 503); + const session = await auth.api.getSession({ headers: c.req.raw.headers }); + if (!session) return c.json({ detail: 'Unauthorized' }, 401); + const body = (await c.req.json().catch(() => ({}))) as { + name?: unknown; + doc?: unknown; + }; + if (!isRoomDoc(body.doc)) { + return c.json({ detail: 'A valid document snapshot is required.' }, 400); + } + const fallbackName = body.doc.documentLabel?.trim() || 'Untitled document'; + const name = + typeof body.name === 'string' && body.name.trim() + ? body.name.trim().slice(0, 160) + : fallbackName.slice(0, 160); + const room = createRoom(session.user.id, name, body.doc); + return c.json({ id: room.id, name: room.name, created_at: room.createdAt }, 201); + }); + + app.get('/api/rooms/:roomId', async (c) => { + const token = bearerToken(c); + if (!token || !verifyOAuthAccessToken) { + return c.json({ detail: 'Unauthorized' }, 401); + } + try { + const claims = await verifyOAuthAccessToken(token, { + verifyOptions: { audience: betterAuthUrl(), issuer: oauthIssuer }, + scopes: ['doc:read'], + }); + const roomId = c.req.param('roomId'); + if ( + typeof claims.sub !== 'string' || + claims.room_id !== roomId + ) { + return c.json({ detail: 'This token does not grant access to that room.' }, 403); + } + const room = getRoomForUser(roomId, claims.sub); + if (!room) return c.json({ detail: 'Room not found.' }, 404); + return c.json({ + id: room.id, + name: room.name, + doc: room.doc, + updated_at: room.updatedAt, + }); + } catch { + return c.json({ detail: 'Unauthorized' }, 401); + } + }); + async function resolveProxyIdentity(c: Context): Promise { const bearer = bearerToken(c); if (bearer?.startsWith('wtk_')) { @@ -339,9 +468,9 @@ export function createApp({ auth }: { auth?: Auth } = {}): Hono { return c.json({ loggingConsent: level }); }); - // Erase the authenticated user's logged activity — study logs and analytics - // profile — while keeping their account. This is *withdrawal*: they carry on - // using the add-in, they just want what we've recorded about them gone. + // Erase the authenticated user's logged activity — study logs and analytics — + // while keeping their account and active room snapshots. This is *withdrawal*: + // they carry on using the add-in and any open room-backed tools. // // Not "delete my data", which is what this used to be called: it doesn't touch // the account, and it deliberately leaves the LLM usage rows, because the diff --git a/backend/src/auth.ts b/backend/src/auth.ts index b52b0d91..60682d75 100644 --- a/backend/src/auth.ts +++ b/backend/src/auth.ts @@ -1,14 +1,18 @@ import { betterAuth } from 'better-auth'; +import { oauthProvider } from '@better-auth/oauth-provider'; import { anonymous, bearer, customSession, deviceAuthorization, + jwt, } from 'better-auth/plugins'; +import { APIError } from 'better-auth/api'; import { betterAuthSecret, betterAuthTrustedOrigins, betterAuthUrl, + BETTER_AUTH_BASE_PATH, deviceClientIds, googleClientId, googleClientSecret, @@ -20,9 +24,59 @@ import { FULL_CONSENT_LEVEL, } from './consent.js'; import { db } from './db.js'; -import { eraseLoggedData } from './erasure.js'; +import { eraseAccountData } from './erasure.js'; import { anonymizeUserUsage } from './usage.js'; import { isUserAllowed } from './userAllowlist.js'; +import { getRoomForUser } from './rooms.js'; +import { + currentOAuthAuthorization, + roomOAuthAuthorization, +} from './oauth-room-authorization.js'; + +interface RoomConsentUser { + id: string; +} + +function roomAuthorizationErrorRedirect( + redirectUri: string, + state: string, + description: string, +): never { + // The provider has already validated client_id and the exact redirect URI before + // invoking this hook, so returning the OAuth error to that URI is safe. + const redirect = new URL(redirectUri); + redirect.searchParams.set('error', 'access_denied'); + redirect.searchParams.set('error_description', description); + redirect.searchParams.set('state', state); + throw new APIError('FOUND', undefined, { + Location: redirect.toString(), + 'Cache-Control': 'no-store', + }); +} + +export const roomConsentReferenceId = async ({ + user, + scopes, +}: { + user?: RoomConsentUser; + scopes: readonly string[]; +}): Promise => { + if (!scopes.includes('doc:read')) return undefined; + if (!user) { + throw new APIError('UNAUTHORIZED', { + message: 'Sign in before authorizing a room.', + }); + } + const authorization = await currentOAuthAuthorization(); + if (!getRoomForUser(authorization.roomId, user.id)) { + roomAuthorizationErrorRedirect( + authorization.redirectUri, + authorization.state, + 'The launched room does not belong to this account.', + ); + } + return authorization.roomId; +}; // A module-level `auth` singleton (not a factory) so the Better Auth CLI can // auto-discover it: `npx @better-auth/cli migrate` looks for an exported `auth` @@ -57,6 +111,7 @@ export const auth = betterAuth({ }, }, baseURL: betterAuthUrl(), + basePath: BETTER_AUTH_BASE_PATH, secret: betterAuthSecret(), trustedOrigins: betterAuthTrustedOrigins(), // Logging-consent level lives on the user record so it's available on every @@ -95,21 +150,48 @@ export const auth = betterAuth({ // Google-only accounts have no password, so deletion proceeds from the // session alone (no verification flow configured). // - // beforeDelete runs the same erasure as the withdrawal endpoint (study logs - // + analytics profile), so account deletion is by construction a superset of - // it — see erasure.ts. On top of that it anonymizes the LLM usage rows rather + // beforeDelete removes study logs, the analytics profile, and durable rooms; + // activity withdrawal intentionally preserves rooms for active tools. See + // erasure.ts. It then anonymizes the LLM usage rows rather // than deleting them: they're content-free billing records, and dropping them // would make our per-user spend stop reconciling with the provider's invoice // (see usage.ts). Better Auth then drops the account, sessions and OAuth links. deleteUser: { enabled: true, beforeDelete: async (user) => { - await eraseLoggedData(user.id); + await eraseAccountData(user.id); anonymizeUserUsage(user.id); }, }, }, plugins: [ + jwt(), + oauthProvider({ + loginPage: '/api/oauth/login', + consentPage: '/api/oauth/consent', + scopes: ['openai:chat', 'doc:read'], + validAudiences: [betterAuthUrl()], + grantTypes: ['authorization_code'], + allowDynamicClientRegistration: false, + allowUnauthenticatedClientRegistration: false, + accessTokenExpiresIn: 60 * 60, + // Better Auth 1.6.22 calls shouldRedirect unconditionally whenever postLogin + // is present. This constant-false compatibility hook does not implement a + // selection step; `page` is consequently unreachable. + postLogin: { + page: '/api/oauth/login', + shouldRedirect: () => false, + consentReferenceId: roomConsentReferenceId, + }, + customAccessTokenClaims: ({ referenceId }) => ({ + ...(referenceId ? { room_id: referenceId } : {}), + }), + customTokenResponseFields: ({ verificationValue }) => { + if (!verificationValue?.referenceId) return {}; + return { room_id: verificationValue.referenceId }; + }, + }), + roomOAuthAuthorization(), bearer(), // Demo mode. signIn.anonymous() mints a real user + session, so the whole // identity-keyed stack (resolveUser, /api/log, consent gating, usage diff --git a/backend/src/config.ts b/backend/src/config.ts index 4d72bce2..bbdaf096 100644 --- a/backend/src/config.ts +++ b/backend/src/config.ts @@ -50,6 +50,10 @@ export const betterAuthSecret = () => // .env line both yield '', which `??` alone would let through. export const betterAuthUrl = () => (process.env.BETTER_AUTH_URL ?? '').trim() || 'http://localhost:8000'; +// Explicitly shared by auth.ts and the resource verifier. Better Auth includes +// this path in JWT `iss`; validating against the bare BETTER_AUTH_URL rejects +// otherwise valid OAuth access tokens. +export const BETTER_AUTH_BASE_PATH = '/api/auth'; export const betterAuthTrustedOrigins = (): string[] => (process.env.BETTER_AUTH_TRUSTED_ORIGINS ?? '') .split(',') @@ -59,6 +63,25 @@ export const googleClientId = () => (process.env.GOOGLE_CLIENT_ID ?? '').trim(); export const googleClientSecret = () => (process.env.GOOGLE_CLIENT_SECRET ?? '').trim(); +// Fixed public OAuth client used by the separately hosted Mindmap. The client id +// is an identifier, not a secret. Production must provide the exact deployed +// callback URL; local development defaults to Vite's Mindmap origin. +export const mindmapOAuthClientId = () => { + const configured = (process.env.MINDMAP_OAUTH_CLIENT_ID ?? '').trim(); + if (configured) return configured; + if ((process.env.NODE_ENV ?? '').toLowerCase() === 'production') return ''; + return 'writing-tools-mindmap'; +}; +export const mindmapOAuthRedirectUris = (): string[] => { + const configured = (process.env.MINDMAP_OAUTH_REDIRECT_URIS ?? '') + .split(',') + .map((value) => value.trim()) + .filter(Boolean); + if (configured.length > 0) return configured; + if ((process.env.NODE_ENV ?? '').toLowerCase() === 'production') return []; + return ['http://localhost:5181/']; +}; + // Comma-separated allowed device client IDs. An empty list rejects all requests. export const deviceClientIds = (): string[] => (process.env.BETTER_AUTH_DEVICE_CLIENT_IDS ?? '') diff --git a/backend/src/db.ts b/backend/src/db.ts index 3dc4a73c..30937017 100644 --- a/backend/src/db.ts +++ b/backend/src/db.ts @@ -15,7 +15,7 @@ import { mkdirSync, renameSync, existsSync } from 'node:fs'; import path from 'node:path'; import Database from 'better-sqlite3'; -import { dataDir } from './config.js'; +import { dataDir, mindmapOAuthClientId } from './config.js'; function dbPath(): string { return path.join(dataDir(), 'app.db'); @@ -118,6 +118,78 @@ const MIGRATIONS: Array<(conn: Database.Database) => void> = [ CREATE INDEX tool_grant_user ON tool_grant (json_extract(user_snapshot, '$.id')); `); }, + // v4 — durable document rooms plus the short-lived room choice made during an + // OAuth authorization. OAuth Provider stores the chosen room id as referenceId + // on the consent, authorization code and access token; these tables hold the + // application resource and the server-side selection that feeds that hook. + (conn) => { + conn.exec(` + CREATE TABLE room ( + id TEXT PRIMARY KEY, + owner_user_id TEXT NOT NULL, + name TEXT NOT NULL, + doc_snapshot TEXT NOT NULL, + created_at INTEGER NOT NULL, + updated_at INTEGER NOT NULL + ); + CREATE INDEX room_owner_updated ON room (owner_user_id, updated_at DESC); + + CREATE TABLE oauth_room_selection ( + session_id TEXT PRIMARY KEY, + user_id TEXT NOT NULL, + room_id TEXT NOT NULL, + expires_at INTEGER NOT NULL, + FOREIGN KEY (room_id) REFERENCES room(id) ON DELETE CASCADE + ); + CREATE INDEX oauth_room_selection_expiry ON oauth_room_selection (expires_at); + `); + }, + // v5 — key the transient room choice to one OAuth authorization request, not + // merely to the browser session. A user can authorize in multiple tabs; one + // tab must never supply or overwrite the room for another tab's grant. + (conn) => { + conn.exec(` + DROP TABLE oauth_room_selection; + CREATE TABLE oauth_room_selection ( + session_id TEXT NOT NULL, + authorization_state TEXT NOT NULL, + user_id TEXT NOT NULL, + room_id TEXT NOT NULL, + expires_at INTEGER NOT NULL, + PRIMARY KEY (session_id, authorization_state), + FOREIGN KEY (room_id) REFERENCES room(id) ON DELETE CASCADE + ); + CREATE INDEX oauth_room_selection_expiry ON oauth_room_selection (expires_at); + `); + }, + // v6 — the trusted Mindmap client binds a room directly from the verified OAuth + // request and checks ownership server-side, so no transient human selection is + // stored anymore. Appending a drop migration preserves deployed v5 databases. + (conn) => { + conn.exec(`DROP TABLE oauth_room_selection;`); + }, + // v7 — one-time cleanup of clients created while unauthenticated dynamic + // registration was enabled. On a fresh database Better Auth's oauthClient table + // does not exist yet, so there is nothing to clean; provisioning runs after its + // migrations. Existing databases retain only the configured fixed Mindmap row. + (conn) => { + const table = conn + .prepare( + `SELECT 1 FROM sqlite_master WHERE type = 'table' AND name = 'oauthClient'`, + ) + .get(); + if (!table) return; + const trustedClientId = mindmapOAuthClientId(); + if (!trustedClientId) { + throw new Error('MINDMAP_OAUTH_CLIENT_ID is required before OAuth client cleanup.'); + } + const result = conn + .prepare(`DELETE FROM oauthClient WHERE clientId <> ?`) + .run(trustedClientId); + if (result.changes > 0) { + console.log(`Removed ${result.changes} stale dynamically registered OAuth client(s).`); + } + }, ]; function migrate(conn: Database.Database): void { diff --git a/backend/src/erasure.ts b/backend/src/erasure.ts index cc642732..63d2b52d 100644 --- a/backend/src/erasure.ts +++ b/backend/src/erasure.ts @@ -1,41 +1,51 @@ /** - * Erasing a user's logged activity — the one definition of what "delete my data" - * means, shared by both paths that promise it. + * User-data erasure has two deliberately different scopes: * - * Two different requests reach this: - * - "delete my logged activity" (DELETE /api/me/activity) — withdrawal. The user - * keeps their account and keeps using the add-in; they just want what we've - * recorded about them gone. Their LLM usage rows stay: the account is still - * open and still running up a bill, so it still has to be metered. - * - "delete my account" (Better Auth's deleteUser) — departure. The `beforeDelete` - * hook calls this too, and *then* anonymizes the usage rows and drops the - * account, so account deletion is by construction a superset of the above. + * - Activity withdrawal (`DELETE /api/me/activity`) removes study logs and the + * analytics profile. It preserves active rooms because the user keeps their + * account and may have Mindmap open against one of those resources. + * - Account deletion removes those same records plus durable room document + * snapshots. Better Auth then deletes the account and sessions, while the + * caller anonymizes content-free usage rows for invoice reconciliation. * - * Keeping this in one function is the point: when the two paths each maintained - * their own list, account deletion quietly forgot to purge the PostHog person — - * the thorough option was doing less than the lesser one. + * Keep the two exported operations here so their difference remains explicit. */ import { deleteUserLogs } from './logging.js'; import { deletePosthogPerson } from './posthog.js'; +import { deleteRoomsForUser } from './rooms.js'; -/** - * Delete everything we've logged about a user: their study-log file and their - * analytics profile. PostHog deletion is best-effort (it needs a management API - * key; see deletePosthogPerson) and never throws. The two touch unrelated systems, - * so we run them concurrently and independently — a failure deleting the log file - * must not skip the PostHog deletion, or vice versa. If either genuinely fails we - * still surface it, so the caller (e.g. Better Auth's beforeDelete) can abort rather - * than drop an account whose data we couldn't erase. - */ -export async function eraseLoggedData(userId: string): Promise { - const results = await Promise.allSettled([ - deleteUserLogs(userId), - deletePosthogPerson(userId), - ]); +function loggedDataOperations(userId: string): Array> { + return [deleteUserLogs(userId), deletePosthogPerson(userId)]; +} + +async function settleErasure( + label: string, + operations: Array>, +): Promise { + const results = await Promise.allSettled(operations); const failures = results - .filter((r): r is PromiseRejectedResult => r.status === 'rejected') - .map((r) => r.reason); + .filter((result): result is PromiseRejectedResult => + result.status === 'rejected', + ) + .map((result) => result.reason); if (failures.length > 0) { - throw new AggregateError(failures, 'eraseLoggedData: partial failure'); + throw new AggregateError(failures, `${label}: partial failure`); } } + +/** Remove logged activity without disrupting active room-backed tools. */ +export async function eraseLoggedData(userId: string): Promise { + await settleErasure('eraseLoggedData', loggedDataOperations(userId)); +} + +/** + * Remove all person-linked stored content before deleting the account. + * Keep this as a structural superset of loggedDataOperations: these paths once + * had separate lists and account deletion accidentally omitted PostHog data. + */ +export async function eraseAccountData(userId: string): Promise { + await settleErasure('eraseAccountData', [ + ...loggedDataOperations(userId), + deleteRoomsForUser(userId), + ]); +} diff --git a/backend/src/index.ts b/backend/src/index.ts index 02b19cb6..9fd38585 100644 --- a/backend/src/index.ts +++ b/backend/src/index.ts @@ -7,6 +7,8 @@ import { deviceClientIds, googleClientId, googleClientSecret, + mindmapOAuthClientId, + mindmapOAuthRedirectUris, openaiApiKey, PORT, } from './config.js'; @@ -36,11 +38,21 @@ if (authEnabled()) { 'BETTER_AUTH_DEVICE_CLIENT_IDS is empty — all device code requests will be rejected.', ); } + if (!mindmapOAuthClientId() || mindmapOAuthRedirectUris().length === 0) { + console.error( + 'MINDMAP_OAUTH_CLIENT_ID and MINDMAP_OAUTH_REDIRECT_URIS are required when auth is enabled in production', + ); + process.exit(1); + } } // Import the auth singleton only when enabled. The dynamic import means auth.ts // (and its SQLite connection) is never executed when auth is disabled or in tests. const auth = authEnabled() ? (await import('./auth.js')).auth : undefined; +if (auth) { + const { provisionTrustedMindmapClient } = await import('./oauth-clients.js'); + await provisionTrustedMindmapClient(auth); +} const app = createApp({ auth }); // if auth is enabled, register the device approval page route. This is separate from the main @@ -50,6 +62,8 @@ const app = createApp({ auth }); if (auth) { const { devicePageHandler } = await import('./routes/device-approval.js'); app.get('/api/device', devicePageHandler); + const { registerOAuthPages } = await import('./routes/oauth-pages.js'); + registerOAuthPages(app); } // Debug UI — only when auth is enabled AND DEBUG=true. Registered here (not in diff --git a/backend/src/migrate.ts b/backend/src/migrate.ts index 3c82e2bb..7e2a0801 100644 --- a/backend/src/migrate.ts +++ b/backend/src/migrate.ts @@ -15,6 +15,7 @@ import { getMigrations } from 'better-auth/db/migration'; import { auth } from './auth.js'; import { db } from './db.js'; +import { provisionTrustedMindmapClient } from './oauth-clients.js'; // Let the process exit naturally rather than calling process.exit(): in a // container stdout is a pipe (async on Unix), so exiting immediately after a @@ -28,6 +29,8 @@ try { const { runMigrations } = await getMigrations(auth.options); await runMigrations(); console.log('Better Auth migrations applied.'); + await provisionTrustedMindmapClient(auth); + console.log('Trusted Mindmap OAuth client provisioned.'); } catch (err) { console.error('Migration failed:', err); process.exitCode = 1; diff --git a/backend/src/oauth-clients.ts b/backend/src/oauth-clients.ts new file mode 100644 index 00000000..afc8aabc --- /dev/null +++ b/backend/src/oauth-clients.ts @@ -0,0 +1,63 @@ +import type { Auth } from './auth.js'; +import { + mindmapOAuthClientId, + mindmapOAuthRedirectUris, +} from './config.js'; + +/** + * Idempotently provision the configured first-party Mindmap client through + * Better Auth's adapter so booleans, dates, and arrays use the provider's own + * storage encoding. Stale dynamic clients are removed once by app migration v7. + */ +export async function provisionTrustedMindmapClient(auth: Auth): Promise { + const clientId = mindmapOAuthClientId(); + const redirectUris = mindmapOAuthRedirectUris(); + if (!clientId || redirectUris.length === 0) { + throw new Error( + 'MINDMAP_OAUTH_CLIENT_ID and MINDMAP_OAUTH_REDIRECT_URIS are required in production.', + ); + } + + for (const redirectUri of redirectUris) { + const parsed = new URL(redirectUri); + if (!['http:', 'https:'].includes(parsed.protocol)) { + throw new Error(`Mindmap OAuth redirect URI must use HTTP(S): ${redirectUri}`); + } + } + + const context = await auth.$context; + const existing = await context.adapter.findOne({ + model: 'oauthClient', + where: [{ field: 'clientId', value: clientId }], + }); + const now = new Date(); + const data = { + clientId, + skipConsent: true, + scopes: ['openai:chat', 'doc:read'], + updatedAt: now, + name: 'Writing Tools Mindmap', + uri: new URL(redirectUris[0]!).origin, + redirectUris, + tokenEndpointAuthMethod: 'none', + grantTypes: ['authorization_code'], + responseTypes: ['code'], + public: true, + type: 'user-agent-based', + requirePKCE: true, + }; + + if (existing) { + await context.adapter.update({ + model: 'oauthClient', + where: [{ field: 'clientId', value: clientId }], + update: data, + }); + return; + } + + await context.adapter.create({ + model: 'oauthClient', + data: { ...data, disabled: false, createdAt: now }, + }); +} diff --git a/backend/src/oauth-room-authorization.ts b/backend/src/oauth-room-authorization.ts new file mode 100644 index 00000000..49b99491 --- /dev/null +++ b/backend/src/oauth-room-authorization.ts @@ -0,0 +1,94 @@ +import { getOAuthProviderState } from '@better-auth/oauth-provider'; +import { + APIError, + createAuthEndpoint, + getSessionFromCtx, + sessionMiddleware, +} from 'better-auth/api'; +import { z } from 'zod'; +import { getRoomForUser } from './rooms.js'; + +interface OAuthAuthorizationContext { + state: string; + roomId: string; + clientId: string; + redirectUri: string; +} + +/** Read the OAuth Provider plugin's already signature-verified request context. */ +export async function currentOAuthAuthorization(): Promise { + const providerState = await getOAuthProviderState(); + if (!providerState?.query) { + throw new APIError('BAD_REQUEST', { + message: 'Missing verified OAuth authorization context.', + }); + } + return parseOAuthAuthorizationQuery(providerState.query); +} + +export function parseOAuthAuthorizationQuery( + value: string, +): OAuthAuthorizationContext { + const query = new URLSearchParams(value); + const state = query.get('state') ?? ''; + const roomId = state.split('.', 1)[0] ?? ''; + const clientId = query.get('client_id') ?? ''; + const redirectUri = query.get('redirect_uri') ?? ''; + if (!state || !roomId.startsWith('room_') || !clientId || !redirectUri) { + throw new APIError('BAD_REQUEST', { + message: 'Invalid room authorization request.', + }); + } + return { state, roomId, clientId, redirectUri }; +} + +export function roomOAuthAuthorization() { + // Load-bearing coupling with @better-auth/oauth-provider: its global before + // hook matches endpoints whose parsed body contains `oauth_query`, verifies the + // signature and expiry, then populates the async-local provider state read by + // currentOAuthAuthorization(). The handlers do not read this field directly, + // but removing it would bypass that hook and leave no verified request context. + const body = z.object({ oauth_query: z.string().min(1) }); + return { + id: 'room-oauth-authorization', + endpoints: { + oauthRoomContext: createAuthEndpoint( + '/oauth2/room-context', + { method: 'POST', body, use: [sessionMiddleware] }, + async (ctx) => { + const session = await getSessionFromCtx(ctx); + if (!session) throw new APIError('UNAUTHORIZED'); + const authorization = await currentOAuthAuthorization(); + const room = getRoomForUser( + authorization.roomId, + session.user.id, + ); + if (!room) { + throw new APIError('NOT_FOUND', { + message: 'The launched room was not found for this account.', + }); + } + const client = (await ctx.context.adapter.findOne({ + model: 'oauthClient', + where: [{ field: 'clientId', value: authorization.clientId }], + })) as + | { clientId: string; name?: string | null; redirectUris?: string[] } + | null; + if (!client?.redirectUris?.includes(authorization.redirectUri)) { + throw new APIError('BAD_REQUEST', { + message: 'The OAuth client or redirect URI is no longer registered.', + }); + } + return ctx.json({ + room: { id: room.id, name: room.name }, + client: { + id: client.clientId, + name: client.name || client.clientId, + redirect_origin: new URL(authorization.redirectUri).origin, + }, + }); + }, + ), + }, + } as const; +} diff --git a/backend/src/rooms.ts b/backend/src/rooms.ts new file mode 100644 index 00000000..e7fda7be --- /dev/null +++ b/backend/src/rooms.ts @@ -0,0 +1,84 @@ +import { randomBytes } from 'node:crypto'; +import { db } from './db.js'; + +export interface RoomDoc { + documentLabel?: string; + beforeCursor: string; + selectedText: string; + afterCursor: string; + contextData?: unknown; +} + +export interface Room { + id: string; + name: string; + doc: RoomDoc; + createdAt: number; + updatedAt: number; +} + +interface RoomRow { + id: string; + name: string; + doc_snapshot: string; + created_at: number; + updated_at: number; +} + +function parseRoom(row: RoomRow): Room { + return { + id: row.id, + name: row.name, + doc: JSON.parse(row.doc_snapshot) as RoomDoc, + createdAt: row.created_at, + updatedAt: row.updated_at, + }; +} + +export function isRoomDoc(value: unknown): value is RoomDoc { + if (!value || typeof value !== 'object' || Array.isArray(value)) return false; + const doc = value as Record; + return ( + typeof doc.beforeCursor === 'string' && + typeof doc.selectedText === 'string' && + typeof doc.afterCursor === 'string' && + (doc.documentLabel === undefined || typeof doc.documentLabel === 'string') + ); +} + +export function createRoom(userId: string, name: string, doc: RoomDoc): Room { + const id = `room_${randomBytes(18).toString('base64url')}`; + const now = Date.now(); + db() + .prepare( + `INSERT INTO room + (id, owner_user_id, name, doc_snapshot, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?)`, + ) + .run(id, userId, name, JSON.stringify(doc), now, now); + return { id, name, doc, createdAt: now, updatedAt: now }; +} + +export function listRooms(userId: string): Room[] { + const rows = db() + .prepare( + `SELECT id, name, doc_snapshot, created_at, updated_at + FROM room WHERE owner_user_id = ? ORDER BY updated_at DESC`, + ) + .all(userId) as RoomRow[]; + return rows.map(parseRoom); +} + +export function getRoomForUser(roomId: string, userId: string): Room | null { + const row = db() + .prepare( + `SELECT id, name, doc_snapshot, created_at, updated_at + FROM room WHERE id = ? AND owner_user_id = ?`, + ) + .get(roomId, userId) as RoomRow | undefined; + return row ? parseRoom(row) : null; +} + +export async function deleteRoomsForUser(userId: string): Promise { + db().prepare(`DELETE FROM room WHERE owner_user_id = ?`).run(userId); +} diff --git a/backend/src/routes/oauth-pages.ts b/backend/src/routes/oauth-pages.ts new file mode 100644 index 00000000..4999193b --- /dev/null +++ b/backend/src/routes/oauth-pages.ts @@ -0,0 +1,64 @@ +const PAGE_STYLE = ` +body{font:16px/1.5 system-ui,sans-serif;max-width:620px;margin:3rem auto;padding:0 1.5rem;color:#172033} +h1{font-size:1.5rem}button{font:inherit;padding:.65rem 1rem;border:1px solid #aab2c0;border-radius:7px;cursor:pointer} +.primary{background:#3157d5;color:white;border-color:#3157d5}.muted{color:#657087}.error{color:#b42318} +.actions{display:flex;gap:.7rem;margin-top:1.5rem}`; + +function shell(title: string, script: string): string { + return `${title}

${title}

Loading…

`; +} + +const helpers = ` +const app=document.getElementById('app'); +function el(tag,text,cls){const node=document.createElement(tag);if(text!==undefined)node.textContent=text;if(cls)node.className=cls;return node} +function show(...nodes){app.replaceChildren(...nodes)} +async function json(url,init){const response=await fetch(url,{credentials:'include',...init});const body=await response.json().catch(()=>({}));if(!response.ok)throw new Error(body.message||body.detail||body.error_description||body.error||('Request failed ('+response.status+')'));return body} +function oauthBody(extra){return JSON.stringify({...extra,oauth_query:location.search.slice(1)})} +function go(result){const url=result.url||result.redirect_uri;if(url)location.href=url;else throw new Error('Authorization server returned no redirect URL')} +`; + +const LOGIN_HTML = shell( + 'Connect an app', + `${helpers} +async function start(){ + const session=await json('/api/auth/get-session').catch(()=>null); + if(session?.user){location.replace('/api/auth/oauth2/authorize'+location.search);return} + const p=el('p','Sign in to authorize the Writing Tools room opened by this app.'); + const b=el('button','Sign in with Google','primary'); + b.onclick=async()=>{try{ + const callbackURL=location.href; + const result=await json('/api/auth/sign-in/social',{method:'POST',headers:{'Content-Type':'application/json'},body:JSON.stringify({provider:'google',callbackURL,errorCallbackURL:callbackURL})}); + location.href=result.url; + }catch(error){show(el('p',String(error),'error'))}}; + show(p,b); +} +start();`, +); + +const CONSENT_HTML = shell( + 'Allow access?', + `${helpers} +async function start(){ + try{ + const context=await json('/api/auth/oauth2/room-context',{method:'POST',headers:{'Content-Type':'application/json'},body:oauthBody({})}); + const body=el('p',context.client.name+' ('+context.client.redirect_origin+') wants to read “'+context.room.name+'” and use Writing Tools AI on your behalf.'); + const note=el('p','The access token is limited to this room and expires in one hour.','muted'); + const actions=el('div',undefined,'actions'); + const allow=el('button','Allow','primary');allow.onclick=()=>decide(true); + const deny=el('button','Deny');deny.onclick=()=>decide(false); + actions.append(allow,deny);show(body,note,actions); + }catch(error){show(el('p',String(error),'error'))} +} +async function decide(accept){ + try{ + const result=await json('/api/auth/oauth2/consent',{method:'POST',headers:{'Content-Type':'application/json','Accept':'application/json'},body:oauthBody({accept})}); + go(result); + }catch(error){show(el('p',String(error),'error'))} +} +start();`, +); + +export function registerOAuthPages(app: import('hono').Hono): void { + app.get('/api/oauth/login', (c) => c.html(LOGIN_HTML)); + app.get('/api/oauth/consent', (c) => c.html(CONSENT_HTML)); +} diff --git a/docker-compose.yml b/docker-compose.yml index 0c7a248e..59269232 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -31,6 +31,8 @@ services: - BETTER_AUTH_TRUSTED_ORIGINS=${BETTER_AUTH_TRUSTED_ORIGINS:-} - GOOGLE_CLIENT_ID=${GOOGLE_CLIENT_ID:-} - GOOGLE_CLIENT_SECRET=${GOOGLE_CLIENT_SECRET:-} + - MINDMAP_OAUTH_CLIENT_ID=${MINDMAP_OAUTH_CLIENT_ID:-} + - MINDMAP_OAUTH_REDIRECT_URIS=${MINDMAP_OAUTH_REDIRECT_URIS:-} - BETTER_AUTH_DEVICE_CLIENT_IDS=${BETTER_AUTH_DEVICE_CLIENT_IDS:-} restart: unless-stopped diff --git a/docs/oauth-rooms-pkce-poc.md b/docs/oauth-rooms-pkce-poc.md new file mode 100644 index 00000000..b65f83ed --- /dev/null +++ b/docs/oauth-rooms-pkce-poc.md @@ -0,0 +1,109 @@ +# Rooms + OAuth Authorization Code/PKCE proof of concept + +This branch replaces the launcher grant for Mindmap with a provider-native OAuth +flow. It deliberately leaves the existing `/api/handoff` implementation in place +for comparison and for other tools. + +## Mental model + +There are three different objects: + +1. **Room** — durable server-side resource containing a document snapshot. The + add-in creates it while authenticated as the writer. +2. **Authorization code** — short-lived, single-use result of the writer approving + Mindmap. It is useless without the PKCE verifier that Mindmap generated and kept + in its own session storage. +3. **Access token** — one-hour credential issued after the code + verifier exchange. + Its signed `room_id` claim limits it to one room; scopes independently limit the + operations (`doc:read`, `openai:chat`). + +The room id is not a secret. It may appear in the launch URL and OAuth request. +Possession of it grants nothing. + +## End-to-end sequence + +1. The taskpane reads the current `DocContext` and sends it to `POST /api/rooms` + with its existing Better Auth session bearer. +2. The taskpane opens `https://mindmap…/?room=room_…`. +3. Mindmap uses its pre-registered trusted public client id, creates a random PKCE + verifier, stores the verifier in session storage, and sends + only its SHA-256 challenge to `/api/auth/oauth2/authorize`. The non-secret room + id is included in the standard `state` value to bind this authorization request + to the exact launched room; the complete state also contains random bytes and + is matched exactly at callback. +4. The authorization server authenticates the writer in the system browser. This + may require Google sign-in because the Office taskpane and system browser do not + necessarily share cookies. +5. `consentReferenceId` derives the room from Mindmap's OAuth state and verifies + server-side that the signed-in user owns it. The room id is not signed + provenance; the boundary is the exact-redirect client, authenticated user, + ownership check, and room-bound signed token together. A missing room or a + different signed-in account returns `error=access_denied` to the already + validated Mindmap callback, where the app can explain the mismatch. +6. The trusted client has `skipConsent`, so an existing browser session returns + directly to Mindmap without a room-confirmation or consent screen. A user with + no browser session signs in first and then returns directly. +7. Mindmap receives the code at its registered redirect URI and exchanges it with + the locally retained verifier. The verifier never travels in the launcher URL. +8. Mindmap calls `GET /api/rooms/:roomId`. The resource server verifies the token's + signature, issuer, audience, `doc:read` scope, subject, and exact `room_id`. + +## Relationship to #579 and #593 + +- This does not need `wt_api`, and the backend URL comes from Mindmap's deploy-time + `VITE_BACKEND_URL`. That removes the attacker-selected proxy problem identified + on #579 for this flow. +- Origin checks may remain defense-in-depth for the old handoff flow; they are not + treated as client authentication here. +- #593's Pages deployment is still useful and should supply the production backend + URL. Its skipped `wt_api` smoke assertion should be replaced for this branch with + an OAuth configuration/launch smoke test. +- The clean implementation branch is based on current `origin/main`, not the dirty + or ahead Mindmap worktrees. The useful #579/#593 changes should be rebased or + cherry-picked after review rather than making this proof of concept depend on + their current branch topology. + +## Deliberate proof-of-concept limits + +- A room currently has one owner and one document snapshot. There is no membership + table, live synchronization, document update endpoint, or multi-document room. +- Dynamic and unauthenticated client registration are disabled. Startup + idempotently provisions the configured Mindmap client through Better Auth's + adapter. Migration v7 removes stale dynamic clients once; it preserves the + configured Mindmap row and does not delete clients added after that migration. +- Tokens expire after one hour; refresh tokens are not enabled. +- Removing the confirmation checkpoint is deliberate: an attacker would need both + an unguessable room id and the ability to drive its owner's browser. Fresh random + OAuth state still prevents callback mixups and CSRF. +- Deleting logged activity preserves active rooms. Account deletion removes room + snapshots. +- This is bearer-token security. PKCE prevents interception of the authorization + code; it does not make a stolen access token unusable. Sender-constrained tokens + (for example DPoP) would be a separate layer. + +## Trusted-client implementation findings + +In installed `@better-auth/oauth-provider` 1.6.22, `consentReferenceId` runs before +the `client.skipConsent` branch and its result is stored on the authorization-code +verification value. The resulting `referenceId` reaches both the custom access-token +claim and token response. The no-screen integration test exercises that behavior +with a real signed token, room-resource request, and OpenAI-proxy request. + +Better Auth 1.6.22 calls `postLogin.shouldRedirect` whenever a `postLogin` object +exists. The configuration therefore retains a constant-false compatibility hook +and unreachable page value; there is no room-selection or confirmation machinery. + +An appended v6 application migration drops `oauth_room_selection`, preserving +upgrades from existing v5 databases. Migration v7 performs the one-time stale-client +cleanup. The Mindmap client is deliberately not placed in Better Auth's process-wide +trusted-client cache, so changes such as `disabled = 1` are observed on the next +authorization request. Provisioning preserves that operational disable on later +starts. Environment-driven redirect changes still require a restart because startup +provisioning is what applies them. + +The resource verifier explicitly validates the issuer as `BETTER_AUTH_URL` plus +Better Auth's `/api/auth` base path. Tokens are issued with that full value; using +the bare origin produces an otherwise opaque 401. + +The final production hostname—and therefore the exact production redirect URI and +matching build-time values—remains the sole deployment configuration decision. diff --git a/docs/oauth-trusted-client-spec.md b/docs/oauth-trusted-client-spec.md new file mode 100644 index 00000000..ed246872 --- /dev/null +++ b/docs/oauth-trusted-client-spec.md @@ -0,0 +1,282 @@ +# Spec — trusted Mindmap client, no-screen launch, end-to-end test + +Target branch: `agent/oauth-rooms-pkce-poc` (PR #594, draft). +Base: commit `d3053b8c`, all 11 CI checks green. +Companion doc: `docs/oauth-rooms-pkce-poc.md` (update it as part of this work). + +Implementation status (2026-07-31): implemented and validated locally. Task 0 +confirmed that Better Auth 1.6.22 calls `consentReferenceId` before its +`skipConsent` branch. The only remaining deployment input is the final production +Mindmap hostname/exact redirect URI. Review hardening added an OAuth error redirect +for wrong-account launches, one-time v7 stale-client cleanup, adapter-backed +provisioning, live database reads instead of the permanent trusted-client cache, +and explicit issuer validation. Nothing from this pass has been pushed. + +This spec covers three changes that are **one logical change**: making Mindmap a +pre-registered trusted OAuth client, deleting the room-confirmation machinery +that a trusted client makes unnecessary, and pinning the resulting no-screen +flow with a real end-to-end test. + +--- + +## Why + +Two review rounds established: + +1. **Open dynamic client registration is the one genuine merge blocker.** + `allowDynamicClientRegistration` and `allowUnauthenticatedClientRegistration` + are both `true` in `backend/src/auth.ts`. Anyone can register a client + against a production authorization server. + +2. **Both per-launch screens are our design, not PKCE's**, and they have + *different* causes — fixing one does not fix the other: + + | Screen | Gated by | Cause | + |---|---|---| + | `/api/oauth/room` | `postLogin.shouldRedirect` → selection keyed by `(session, state)` | fresh `state` every launch ⇒ never a match | + | `/api/oauth/consent` | Better Auth remembered consent | keyed on `clientId + userId + referenceId`; `referenceId` is the room, new every launch ⇒ never a match | + + Verified in `@better-auth/oauth-provider@1.6.22`: the consent lookup at + `dist/index.mjs` (~line 58) is + `where: [clientId, userId, ...(referenceId ? [referenceId] : [])]`, and + `skipConsent` exists as a client field (`dist/oauth-D74mBkw6.d.mts:1237`, + schema at `:25`). + +A trusted client with `skipConsent` fixes the consent screen. Deleting the +confirm page fixes the other. Together they give: existing browser session → +brief redirect, no screens; no session → sign in, then straight back. + +--- + +## The security boundary (state it this way) + +Do **not** describe the room as coming from "the signed authorization request." +The room id originates in Mindmap-generated OAuth `state` and is not signed by +the taskpane — any party can put any value there. Better Auth integrity-protects +the *continuation* (`oauth_query`, the `sig=` parameter), not the provenance of +`state`. + +What actually holds the flow together is four things in conjunction: + +1. a fixed trusted client with an **exact** registered redirect URI, +2. an authenticated user, +3. **server-side verification that the room belongs to that user**, +4. a room-bound signed access token, re-checked at the resource. + +Removing the confirmation page removes the last human checkpoint, so (3) plus +room-id unguessability (`room_` + 18 random bytes) carries the weight. That is +an acceptable trade — an attacker must already know the victim's room id *and* +drive the victim's browser — but record it as a decision in +`docs/oauth-rooms-pkce-poc.md`, not as a silent side effect of a UX cleanup. + +--- + +## Task 0 — verify the load-bearing assumption FIRST + +**Do not build on this until it is confirmed.** The entire design assumes the +room id still reaches the access token when consent is skipped. + +Today the chain is: + +``` +postLogin.consentReferenceId → referenceId + → customAccessTokenClaims({ referenceId }) → room_id claim + → customTokenResponseFields → room_id in the token response +``` + +`consentReferenceId` is a **consent-time** hook. If `skipConsent: true` bypasses +it, `referenceId` is never set, `room_id` never reaches the token, and +`finishRoomAuthorization` fails its own `roomId !== request.roomId` check. + +**Determine, from the installed 1.6.22 source, whether `consentReferenceId` +still fires for a client with `skipConsent: true`.** Write a throwaway probe if +reading the source is ambiguous. + +- **If it fires** — proceed as specified below. +- **If it does not** — stop and report. Do not invent a workaround; the + alternative (deriving the room inside `customAccessTokenClaims` from the + authorization query, with the ownership check moved there) changes where the + security boundary sits and needs review before implementation. + +Record the finding in the PR description either way. + +--- + +## Task 1 — trusted fixed client + +### Backend (`backend/src/auth.ts`) + +- Set `allowDynamicClientRegistration: false` and + `allowUnauthenticatedClientRegistration: false`. +- Seed one Mindmap client at startup (idempotent — safe to run on every boot): + - client id from config, not hardcoded; expose as an env var alongside the + existing Better Auth config in `backend/src/config.ts` + - `token_endpoint_auth_method: "none"` (public client) + - `grant_types: ["authorization_code"]`, `response_types: ["code"]` + - scopes `openai:chat doc:read` + - **exact** `redirect_uris` — no wildcards, no prefix matching + - `skipConsent: true` +- **`skipConsent` must apply to this client only.** Never make it a global + option, and never grant it to a dynamically registered client. + +### Purge stale clients + +Disabling registration prevents *new* registrations; it does **not** revoke +clients already stored. Codex's own smoke runs have created some. Add a +deliberate cleanup (a migration or a documented one-off) that removes +dynamically registered `oauthClient` rows, and say in the doc which rows are +expected to survive. + +### Mindmap (`prototype-mindmap/src/platform-session.ts`) + +- Delete the `clientId()` self-registration path entirely: the + `POST /oauth2/register` call and the `OAUTH_CLIENT_STORAGE_KEY` localStorage + cache (~lines 220–255). +- Read a build-time `VITE_OAUTH_CLIENT_ID` instead. Fail closed in production if + it is missing, matching how `VITE_BACKEND_URL` is handled at ~line 21. +- A public OAuth client id in the compiled bundle is **fine** — it is an + identifier, not a secret. Do not add a secret-handling path for it. + +### Redirect URI + +`callbackUri()` returns `window.location.origin + window.location.pathname`. +The registered value must match exactly. **The production hostname is not yet +decided** (`mindmap.thoughtful-ai.com` vs a `prototypes.*` umbrella), so: + +- implement the code now, +- leave the concrete registered URI and `.env.production` value as the only + outstanding item, clearly marked, +- or register both candidate URLs deliberately if that is preferred — but say so + explicitly rather than leaving it ambiguous. + +--- + +## Task 2 — remove the confirm/selection machinery + +A trusted client that derives the room server-side has nothing to "select." All +of the following goes: + +| Delete | File | +|---|---| +| `ROOM_HTML` and the `/api/oauth/room` route | `backend/src/routes/oauth-pages.ts` | +| `authorizeOAuthRoom` endpoint | `backend/src/oauth-room-authorization.ts` | +| `selectRoomForOAuth`, `selectedRoomForOAuth`, `consumeRoomSelection` | `backend/src/rooms.ts` | +| `oauth_room_selection` table + the v5 migration | `backend/src/db.ts` | +| `postLogin.shouldRedirect` | `backend/src/auth.ts` | +| the tests covering the above | `backend/src/__tests__/rooms.test.ts` | + +**Keep:** + +- `currentOAuthAuthorization()` / `parseOAuthAuthorizationQuery()` — still how + the room id is derived. +- `oauthRoomContext` — still useful for the consent page shown to *untrusted* + clients, which keep normal consent. +- The `oauth_query` body parameter on the remaining endpoints, **and its + comment**. It looks unused; it is what makes the provider's `before` + middleware fire and verify the signature. Do not "clean it up." +- Fresh OAuth `state` per launch. It prevents callback mixups and CSRF. It is + not what was causing consent to re-prompt. + +`consentReferenceId` keeps the ownership check — parse the room from the +authorization query, `getRoomForUser(roomId, user.id)`, return the id or throw. +That check is now load-bearing on its own; it must fail closed. + +Removing the v5 migration means the schema version drops. Decide deliberately +whether to renumber (clean, but breaks any existing dev DB) or add a v6 that +drops the table (safer). Document the choice — `db.test.ts` asserts the version +number and will need updating either way. + +Note: this deletes the concurrent-tab state-keying from an earlier review round. +That fix was correct for the design as it stood; it simply stops being needed +once the page it protected is gone. Do not preserve it out of sunk cost. + +--- + +## Task 3 — full no-screen integration test + +Current coverage has a real gap. `oauth-room-middleware.integration.test.ts` +pins the signed-query middleware well, and `oauth-room-resource.test.ts` covers +the resource boundary — but it **injects a mocked `verifyOAuthAccessToken`**. +Nothing exercises a genuine signed token end to end. + +Add one integration test, in the style of the existing middleware test (real +`auth.handler`, real adapter, temp `DATA_DIR`), covering: + +``` +trusted pre-registered client + → /oauth2/authorize with an existing session + → skipConsent (assert NO redirect to a consent or room page) + → server-side room ownership check + → authorization code at the exact registered redirect URI + → /oauth2/token with code + code_verifier + → REAL signed access token (not mocked) + → GET /api/rooms/:roomId with that token + → an authenticated OpenAI-proxy request with that token +``` + +Assert along the way: + +- the authorize response goes **straight to the redirect URI**, not to + `/api/oauth/room` or `/api/oauth/consent` +- the issued token carries the expected `room_id` claim +- `GET /api/rooms/` with that token returns **403** +- a room owned by a *different* user fails the ownership check rather than + issuing a token + +Also add the negative case: an untrusted (dynamically registered, if any can +still exist) or unregistered client is refused. + +--- + +## Also in scope + +Stale comments in `frontend/src/pages/tools/index.tsx` still describe the device +allowlist as a requirement for first-party tools: + +- `:10` — "such a tool signs in for itself via the device flow" +- `:30` — "must also be listed in the backend device allowlist" +- `:43` — `BETTER_AUTH_DEVICE_CLIENT_IDS` + +Under this flow Mindmap's launch never reaches `deviceClientIds()` — that gate +is at `backend/src/app.ts:500`, on grant creation, and `POST /api/rooms` +(`app.ts:296`) authenticates with the taskpane session alone. Correct the +comments to say the device allowlist applies to **grant-based tools**, and note +that the tool/device-client split is still owed for those. + +--- + +## Out of scope + +Do not start these; they belong to a later polish PR: + +- room lifecycle (every launch creates a new room; no TTL, no dedup) +- a user-facing room list or delete endpoint +- refresh tokens / the 1-hour expiry UX +- the raw-SQL user lookup in `resolveUser`'s OAuth branch +- CSS extraction / inline-`