From d2a5bdcca6548271005ea7e1928724218aa4b32b Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Sat, 14 Mar 2026 15:06:04 +0000 Subject: [PATCH 1/2] fix: use upstream OAuth consent UI instead of broken auth-service reimplementation The auth-service consent page had hard-coded permissions ('Read and write posts', 'Access your profile', 'Manage your follows') that ignored the actual OAuth scopes requested by the client. This replaces it with the stock @atproto/oauth-provider consent UI. Changes: - epds-callback now redirects through /oauth/authorize after account creation and device session binding, letting the stock oauthMiddleware handle consent with actual scopes via consent-view.tsx - Remove auth-service consent page (consent.ts) and its tests - Remove client_login table, hasClientLogin/recordClientLogin from db.ts - Add v8 migration to drop client_logins table - Simplify complete.ts: remove consent branching (step 5b) - Add design doc (docs/design/consent-flow-fix.md) with research findings --- docs/design/consent-flow-fix.md | 170 ++++++++++++ .../src/__tests__/consent.test.ts | 256 ------------------ packages/auth-service/src/index.ts | 2 - packages/auth-service/src/routes/complete.ts | 40 +-- packages/auth-service/src/routes/consent.ts | 255 ----------------- packages/pds-core/src/index.ts | 91 ++----- packages/shared/src/db.ts | 23 +- 7 files changed, 213 insertions(+), 624 deletions(-) create mode 100644 docs/design/consent-flow-fix.md delete mode 100644 packages/auth-service/src/__tests__/consent.test.ts delete mode 100644 packages/auth-service/src/routes/consent.ts diff --git a/docs/design/consent-flow-fix.md b/docs/design/consent-flow-fix.md new file mode 100644 index 0000000..adf9fa3 --- /dev/null +++ b/docs/design/consent-flow-fix.md @@ -0,0 +1,170 @@ +# Design: Fix Consent Flow to Use Upstream OAuth UI + +## Problem + +The ePDS consent page (`packages/auth-service/src/routes/consent.ts`) is a +broken reimplementation of the consent UI that already exists in the upstream +`@atproto/oauth-provider-ui` package. Specifically: + +1. **Hard-coded permissions** — the consent page shows "Read and write posts", + "Access your profile", "Manage your follows" regardless of what OAuth scopes + the client actually requested. + +2. **Ignores OAuth scopes entirely** — never reads `parameters.scope` from the + PAR request. A client requesting `transition:chat.bsky` would still see + generic permissions. + +3. **Separate consent tracking** — uses its own `client_login` table + (`hasClientLogin` / `recordClientLogin`) instead of the atproto provider's + built-in `authorizedClients` tracking, which already handles scope-level + consent (re-prompting when new scopes are requested). + +4. **Missing consent on signup** — the new-user flow (handle picker → + `epds-callback`) bypasses `complete.ts` step 5c, so `recordClientLogin` is + never called. The user sees the consent page on their second login instead + of their first. + +## Root Cause + +The `epds-callback` endpoint bypasses the stock OAuth middleware entirely. It +calls `requestManager.get()` and `requestManager.setAuthorized()` directly +instead of going through `provider.authorize()`, which is what the stock PDS +uses (via `oauthMiddleware` in `auth-routes.ts`). + +The stock PDS delegates the full authorization UI to `oauthMiddleware` from +`@atproto/oauth-provider`, which serves a React-based UI from the +`@atproto/oauth-provider-ui` package. This UI includes a proper consent view +(`consent-view.tsx`) that displays actual requested scopes/permissionSets. + +## Fix: Hand Back to the Stock OAuth Flow After Authentication + +After the auth-service authenticates the user (OTP) and pds-core creates the +account (if new), redirect back through the stock `/oauth/authorize` endpoint +instead of calling `setAuthorized()` directly. + +### Flow Change + +**Current flow (broken consent):** + +``` +OTP verify → /auth/complete → check consent (auth-service) → + → show broken consent page (auth-service) → + → /oauth/epds-callback → setAuthorized() → redirect to client +``` + +**Fixed flow:** + +``` +OTP verify → /auth/complete → + → /oauth/epds-callback → create account if needed, upsertDeviceAccount → + → redirect to /oauth/authorize?request_uri=... (stock middleware) → + → stock middleware calls provider.authorize() → + → if consent needed: renders upstream consent-view.tsx with real scopes → + → if no consent needed: auto-approves (SSO match) → + → redirect to client +``` + +### Implementation Steps + +#### Step 1: Modify `epds-callback` to redirect through stock OAuth flow + +In `packages/pds-core/src/index.ts`, after account creation and +`upsertDeviceAccount`, instead of calling `requestManager.setAuthorized()` +and building the redirect URL ourselves: + +- Redirect to `/oauth/authorize?request_uri=&client_id=` +- The stock `oauthMiddleware` will handle this request, find the existing + device session (just created via `upsertDeviceAccount`), check consent via + `provider.authorize()`, and either auto-approve or show the upstream + consent UI. + +The `epds-callback` handler should **stop calling**: + +- `requestManager.setAuthorized()` +- `provider.checkConsentRequired()` +- `provider.accountManager.setAuthorizedClient()` + +These are all handled internally by the stock middleware when it processes the +`/oauth/authorize` redirect. + +#### Step 2: Remove auth-service consent page + +Delete or disable: + +- `packages/auth-service/src/routes/consent.ts` +- The consent route registration in `packages/auth-service/src/index.ts` +- The `needsConsent` check in `packages/auth-service/src/routes/complete.ts` + (step 5b) — the auth-service no longer decides whether consent is needed +- `ctx.db.hasClientLogin()` and `ctx.db.recordClientLogin()` methods +- The `client_login` table (add a migration to drop it) + +#### Step 3: Simplify `complete.ts` + +`/auth/complete` no longer needs to check consent. Its only job is: + +1. Verify the auth flow cookie and better-auth session +2. For new users → redirect to `/auth/choose-handle` +3. For existing users → redirect to `/oauth/epds-callback` (which then + redirects through the stock OAuth flow) + +#### Step 4: Verify the stock middleware handles the redirect correctly + +The key assumption to verify: when `oauthMiddleware` receives +`/oauth/authorize?request_uri=...` and finds an existing device session +(created by `upsertDeviceAccount` moments earlier), it should: + +- Call `provider.authorize()` which returns `sessions` with + `consentRequired` and `loginRequired` flags +- Since `loginRequired` should be false (device session just created) and + `consentRequired` depends on whether this client was previously authorized, + it should either auto-approve or show consent +- The `permissionSets` from the requested scopes will be displayed correctly + +**Risk**: the stock middleware might require a full browser login flow (cookies, +CSRF) rather than just a device session. This needs to be tested. If the +middleware requires its own session state, we may need to ensure that the +device session created by `upsertDeviceAccount` is sufficient for the +middleware to recognize the user as authenticated. + +### What This Fixes + +- Consent page shows actual requested scopes (from upstream `consent-view.tsx`) +- Consent tracking uses the atproto provider's `authorizedClients` system + (scope-aware, re-prompts for new scopes) +- No more hard-coded "Read and write posts" etc. +- No more separate `client_login` table +- New users see consent at the right time (determined by the provider) +- Removes ~250 lines of auth-service code (`consent.ts` + DB methods) + +### Research Findings (Resolved Questions) + +1. **Device identification** — the stock middleware uses `dev-id` and `ses-id` + HTTP cookies (set by `deviceManager.load()` with ~10-year expiry). Since + the browser carries these cookies on both the original `/oauth/authorize` + visit and the redirect back from `epds-callback`, the deviceId will match. + `upsertDeviceAccount(deviceId, sub)` creates the device-account association + that `authorize()` finds via `listDeviceAccounts(deviceId)`. + +2. **PAR request state** — `requestManager.get()` called WITHOUT a deviceId + (as we now do in `epds-callback`) does NOT bind a deviceId to the request. + When the stock middleware subsequently calls `get(requestUri, deviceId, +clientId)`, it binds the browser's deviceId for the first time. This is + the expected PAR → redirect → authorize flow. + +3. **Auto-approve conditions** — `provider.authorize()` auto-approves when: + - `prompt=none` with a single matching session (no login/consent required) + - No explicit prompt + `login_hint` matching one session (no login/consent) + - For unauthenticated clients (`token_endpoint_auth_method: 'none'`), + `requestManager.validate()` forces `prompt: 'consent'`, so consent is + always shown — this is correct behavior. + +4. **Consent UI rendering** — the stock middleware serves a React SPA from + `@atproto/oauth-provider-ui`. It injects hydration data (requestUri, + clientMetadata, scope, permissionSets, sessions with consentRequired flags) + as `window.__authorizeData`. The SPA calls back to + `/oauth/authorize/api/consent` which internally calls `setAuthorized()`. + +5. **AS metadata override** — the redirect from `epds-callback` to + `/oauth/authorize` is a 303 redirect on the same pds-core host. The AS + metadata's `authorization_endpoint` (pointing to the auth service) is + irrelevant here since the browser follows the redirect directly. diff --git a/packages/auth-service/src/__tests__/consent.test.ts b/packages/auth-service/src/__tests__/consent.test.ts deleted file mode 100644 index 1f2f6b0..0000000 --- a/packages/auth-service/src/__tests__/consent.test.ts +++ /dev/null @@ -1,256 +0,0 @@ -/** - * Tests for the consent route — covers both flow_id mode (new) and legacy - * request_uri mode (for backward compatibility with old OTP path). - * - * Uses minimal mock req/res objects to avoid a supertest dependency. - */ -import { describe, it, expect, beforeEach, afterEach } from 'vitest' -import * as fs from 'node:fs' -import * as path from 'node:path' -import * as os from 'node:os' -import { EpdsDb } from '@certified-app/shared' -import type { AuthServiceContext } from '../context.js' -import type { AuthServiceConfig } from '../context.js' - -// Build a minimal mock context for consent tests -function makeMockContext(db: EpdsDb): AuthServiceContext { - const config: AuthServiceConfig = { - hostname: 'auth.localhost', - port: 3001, - sessionSecret: 'test-session-secret', - csrfSecret: 'test-csrf-secret', - epdsCallbackSecret: 'test-callback-secret-32-chars-long!!', - pdsHostname: 'pds.localhost', - pdsPublicUrl: 'http://pds.localhost:3000', - email: { - provider: 'smtp', - smtpHost: 'localhost', - smtpPort: 1025, - from: 'noreply@localhost', - fromName: 'Test PDS', - }, - dbLocation: ':memory:', - } - - return { - db, - config, - emailSender: null as unknown as AuthServiceContext['emailSender'], - destroy: () => { - db.close() - }, - } -} - -// Mock helpers for future route-level tests (currently only DB logic is tested) - -function _makeMockRes() { - const res = { - _status: 200, - body: '', - redirectTo: '', - cleared: [] as string[], - locals: { csrfToken: 'test-csrf-token' }, - status(code: number) { - this._status = code - return this - }, - type(_t: string) { - return this - }, - send(body: string) { - this.body = body - return this - }, - redirect(code: number, url: string) { - this._status = code - this.redirectTo = url - return this - }, - clearCookie(name: string) { - this.cleared.push(name) - return this - }, - } - return res -} - -function _makeGetReq(queryStr: string) { - const url = new URL('http://localhost/auth/consent?' + queryStr) - const query: Record = {} - url.searchParams.forEach((v, k) => { - query[k] = v - }) - return { query, cookies: {}, body: {} } -} - -function _makePostReq(body: Record) { - return { query: {}, cookies: {}, body } -} - -describe('Consent route logic', () => { - let db: EpdsDb - let dbPath: string - // eslint-disable-next-line @typescript-eslint/no-unused-vars - let ctx: AuthServiceContext - - beforeEach(() => { - dbPath = path.join(os.tmpdir(), `test-consent-${Date.now()}.db`) - db = new EpdsDb(dbPath) - ctx = makeMockContext(db) - }) - - afterEach(() => { - db.close() - try { - fs.unlinkSync(dbPath) - // eslint-disable-next-line no-empty - } catch {} - }) - - describe('auth_flow table operations (used by consent)', () => { - it('creates and retrieves an auth_flow record', () => { - db.createAuthFlow({ - flowId: 'flow-abc', - requestUri: 'urn:ietf:params:oauth:request_uri:test', - clientId: 'https://client.example.com', - expiresAt: Date.now() + 5 * 60 * 1000, - }) - - const flow = db.getAuthFlow('flow-abc') - expect(flow).toBeDefined() - expect(flow!.requestUri).toBe('urn:ietf:params:oauth:request_uri:test') - expect(flow!.clientId).toBe('https://client.example.com') - }) - - it('returns undefined for non-existent flow', () => { - expect(db.getAuthFlow('non-existent')).toBeUndefined() - }) - - it('returns undefined for expired flow', () => { - db.createAuthFlow({ - flowId: 'expired-flow', - requestUri: 'urn:ietf:params:oauth:request_uri:expired', - clientId: null, - expiresAt: Date.now() - 1000, // already expired - }) - - expect(db.getAuthFlow('expired-flow')).toBeUndefined() - }) - - it('deletes a flow after approval', () => { - db.createAuthFlow({ - flowId: 'delete-me', - requestUri: 'urn:ietf:params:oauth:request_uri:delete', - clientId: null, - expiresAt: Date.now() + 5 * 60 * 1000, - }) - - expect(db.getAuthFlow('delete-me')).toBeDefined() - db.deleteAuthFlow('delete-me') - expect(db.getAuthFlow('delete-me')).toBeUndefined() - }) - }) - - describe('client login tracking (used by consent)', () => { - it('records and detects client login', () => { - expect( - db.hasClientLogin('user@example.com', 'https://app.example.com'), - ).toBe(false) - db.recordClientLogin('user@example.com', 'https://app.example.com') - expect( - db.hasClientLogin('user@example.com', 'https://app.example.com'), - ).toBe(true) - }) - - it('is case-insensitive for email', () => { - db.recordClientLogin('User@Example.COM', 'https://app.example.com') - expect( - db.hasClientLogin('user@example.com', 'https://app.example.com'), - ).toBe(true) - }) - - it('does not share logins across different clients', () => { - db.recordClientLogin('user@example.com', 'https://app1.example.com') - expect( - db.hasClientLogin('user@example.com', 'https://app2.example.com'), - ).toBe(false) - }) - }) - - describe('signCallback (used by consent POST)', () => { - it('produces a stable HMAC signature for the same inputs', async () => { - const { signCallback } = await import('@certified-app/shared') - const params = { - request_uri: 'urn:req:test', - email: 'user@example.com', - approved: '1', - new_account: '0', - } - const r1 = signCallback(params, 'secret-key') - // Same params but allow ts to change — just verify structure - expect(r1.sig).toMatch(/^[0-9a-f]{64}$/) - expect(r1.ts).toMatch(/^\d+$/) - }) - - it('produces different signatures for different emails', async () => { - const { signCallback } = await import('@certified-app/shared') - const base = { request_uri: 'urn:req:x', approved: '1', new_account: '0' } - const r1 = signCallback({ ...base, email: 'alice@example.com' }, 'secret') - const r2 = signCallback({ ...base, email: 'bob@example.com' }, 'secret') - expect(r1.sig).not.toBe(r2.sig) - }) - - it('verifies correctly with verifyCallback', async () => { - const { signCallback, verifyCallback } = - await import('@certified-app/shared') - const params = { - request_uri: 'urn:req:verify-test', - email: 'user@example.com', - approved: '1', - new_account: '0', - } - const { sig, ts } = signCallback(params, 'shared-secret') - const valid = verifyCallback(params, ts, sig, 'shared-secret') - expect(valid).toBe(true) - }) - }) - - describe('flow_id mode: consent uses auth_flow table', () => { - it('auth_flow row survives consent GET (not deleted until POST approve)', () => { - // Simulate complete.ts creating an auth_flow before redirecting to consent - db.createAuthFlow({ - flowId: 'get-flow', - requestUri: 'urn:ietf:params:oauth:request_uri:get-test', - clientId: 'https://app.example.com', - expiresAt: Date.now() + 5 * 60 * 1000, - }) - - // GET consent should not delete the flow - const flow = db.getAuthFlow('get-flow') - expect(flow).toBeDefined() - - // Simulate: consent GET reads the flow but doesn't delete it - const retrieved = db.getAuthFlow('get-flow') - expect(retrieved).toBeDefined() - expect(retrieved!.requestUri).toBe( - 'urn:ietf:params:oauth:request_uri:get-test', - ) - }) - - it('auth_flow row is deleted after approve (POST)', () => { - db.createAuthFlow({ - flowId: 'post-flow', - requestUri: 'urn:ietf:params:oauth:request_uri:post-test', - clientId: null, - expiresAt: Date.now() + 5 * 60 * 1000, - }) - - // Simulate what consent POST does on approve - const flow = db.getAuthFlow('post-flow') - expect(flow).toBeDefined() - db.deleteAuthFlow('post-flow') - expect(db.getAuthFlow('post-flow')).toBeUndefined() - }) - }) -}) diff --git a/packages/auth-service/src/index.ts b/packages/auth-service/src/index.ts index 0123992..47f8046 100644 --- a/packages/auth-service/src/index.ts +++ b/packages/auth-service/src/index.ts @@ -10,7 +10,6 @@ import { createBetterAuth, runBetterAuthMigrations } from './better-auth.js' import { csrfProtection } from './middleware/csrf.js' import { requestRateLimit } from './middleware/rate-limit.js' import { createLoginPageRouter } from './routes/login-page.js' -import { createConsentRouter } from './routes/consent.js' import { createRecoveryRouter } from './routes/recovery.js' import { createAccountLoginRouter } from './routes/account-login.js' import { createAccountSettingsRouter } from './routes/account-settings.js' @@ -73,7 +72,6 @@ export function createAuthService(config: AuthServiceConfig): { // Routes app.use(createLoginPageRouter(ctx)) - app.use(createConsentRouter(ctx)) app.use(createRecoveryRouter(ctx, betterAuthInstance)) app.use(createAccountLoginRouter(betterAuthInstance)) app.use(createAccountSettingsRouter(ctx, betterAuthInstance)) diff --git a/packages/auth-service/src/routes/complete.ts b/packages/auth-service/src/routes/complete.ts index 13227b4..3ae3a30 100644 --- a/packages/auth-service/src/routes/complete.ts +++ b/packages/auth-service/src/routes/complete.ts @@ -14,9 +14,11 @@ * 3. Get better-auth session → extract verified email * 4. Check if this is a new user (no PDS account for email) * 5a. New user → redirect to /auth/choose-handle (auth_flow + cookie kept intact) - * 5b. Existing user, needs consent → redirect to /auth/consent?flow_id=... - * 5c. Existing user, no consent needed → build HMAC-signed redirect to pds-core /oauth/epds-callback - * 6. Delete auth_flow row + clear cookie (only for 5c path) + * 5b. Existing user → build HMAC-signed redirect to pds-core /oauth/epds-callback + * 6. Delete auth_flow row + clear cookie (only for 5b path) + * + * Note: consent is handled by the stock @atproto/oauth-provider middleware + * after pds-core's epds-callback redirects through /oauth/authorize. */ import { Router, type Request, type Response } from 'express' import type { AuthServiceContext } from '../context.js' @@ -91,16 +93,10 @@ export function createCompleteRouter( const email = session.user.email.toLowerCase() - // Step 4: Check whether this is a new account. - // New accounts (no PDS account yet) are redirected to the handle picker. - // Existing accounts may need consent for first-time client logins. + // Step 4: Check whether this is a new user (no PDS account for email). const did = await getDidByEmail(email, pdsUrl, internalSecret) const isNewAccount = !did - const clientId = flow.clientId ?? '' - const needsConsent = - !isNewAccount && clientId && !ctx.db.hasClientLogin(email, clientId) - if (isNewAccount) { // Step 5a (new user): Redirect to handle picker. // Do NOT delete auth_flow or clear cookie here — TTL cleanup handles expiry. @@ -110,31 +106,15 @@ export function createCompleteRouter( return } - if (needsConsent) { - // Step 5b: Redirect to consent screen, passing flow_id so consent can - // look up request_uri and perform cleanup itself. - // Do NOT delete auth_flow or clear cookie here — consent does it. - const consentUrl = new URL( - '/auth/consent', - `https://${ctx.config.hostname}`, - ) - consentUrl.searchParams.set('flow_id', flowId) - consentUrl.searchParams.set('email', email) - consentUrl.searchParams.set('new', '0') - res.redirect(303, consentUrl.pathname + consentUrl.search) - return - } - - // Step 5c: Record client login before redirecting (no consent needed, existing user) - if (clientId) { - ctx.db.recordClientLogin(email, clientId) - } + // Step 5b (existing user): Build HMAC-signed redirect to pds-core /oauth/epds-callback. + // Consent is handled by the stock @atproto/oauth-provider middleware — + // pds-core's epds-callback redirects through /oauth/authorize which shows + // the upstream consent UI with actual OAuth scopes if needed. // Cleanup: remove auth_flow row and cookie ctx.db.deleteAuthFlow(flowId) res.clearCookie(AUTH_FLOW_COOKIE) - // Step 5c (cont): Build HMAC-signed redirect to pds-core /oauth/epds-callback const callbackParams = { request_uri: flow.requestUri, email, diff --git a/packages/auth-service/src/routes/consent.ts b/packages/auth-service/src/routes/consent.ts deleted file mode 100644 index 8223ca5..0000000 --- a/packages/auth-service/src/routes/consent.ts +++ /dev/null @@ -1,255 +0,0 @@ -import { Router, type Request, type Response } from 'express' -import type { AuthServiceContext } from '../context.js' -import { resolveClientName } from '../lib/client-metadata.js' -import { escapeHtml, signCallback } from '@certified-app/shared' -import { createLogger } from '@certified-app/shared' - -const logger = createLogger('auth:consent') - -const AUTH_FLOW_COOKIE = 'epds_auth_flow' - -/** - * GET /auth/consent - * POST /auth/consent - * - * Shows the consent screen and handles the approve/deny decision. - * - * Supports two modes: - * 1. flow_id mode (new): reads request_uri/client_id from auth_flow table. - * The complete.ts bridge passes flow_id when consent is needed so that - * we can maintain a single source of truth and avoid long URLs. - * 2. Legacy mode: reads request_uri/email/client_id from query params directly - * (used by the old OTP path which is still active). - */ -export function createConsentRouter(ctx: AuthServiceContext): Router { - const router = Router() - - router.get('/auth/consent', async (req: Request, res: Response) => { - const flowId = req.query.flow_id as string | undefined - - if (flowId) { - // New mode: look up auth_flow by flow_id - const flow = ctx.db.getAuthFlow(flowId) - if (!flow) { - logger.warn({ flowId }, 'auth_flow not found or expired on consent GET') - res - .status(400) - .send('

Authentication session expired. Please try again.

') - return - } - - const email = req.query.email as string | undefined - const isNew = req.query.new === '1' - const clientId = flow.clientId ?? '' - const clientName = clientId - ? await resolveClientName(clientId) - : 'the application' - - res.type('html').send( - renderConsent({ - flowId, - requestUri: flow.requestUri, - email: email ?? '', - isNew, - clientId, - clientName, - csrfToken: res.locals.csrfToken, - }), - ) - return - } - - // Legacy mode: request_uri passed directly in query params - const requestUri = req.query.request_uri as string - const email = req.query.email as string - const isNew = req.query.new === '1' - const clientId = req.query.client_id as string | undefined - - if (!requestUri || !email) { - res.status(400).send('

Missing parameters

') - return - } - - const clientName = clientId - ? await resolveClientName(clientId) - : 'the application' - - res.type('html').send( - renderConsent({ - flowId: undefined, - requestUri, - email, - isNew, - clientId: clientId || '', - clientName, - csrfToken: res.locals.csrfToken, - }), - ) - }) - - router.post('/auth/consent', (req: Request, res: Response) => { - const flowId = req.body.flow_id as string | undefined - const action = req.body.action as string - - let requestUri: string - let email: string - let isNew: boolean - let clientId: string - - if (flowId) { - // New mode: look up auth_flow by flow_id - const flow = ctx.db.getAuthFlow(flowId) - if (!flow) { - logger.warn( - { flowId }, - 'auth_flow not found or expired on consent POST', - ) - res - .status(400) - .send('

Authentication session expired. Please try again.

') - return - } - - requestUri = flow.requestUri - email = ((req.body.email as string) || '').trim().toLowerCase() - isNew = req.body.is_new === '1' - clientId = flow.clientId ?? '' - - if (!email) { - res.status(400).send('

Missing email parameter

') - return - } - - // Cleanup the auth_flow row and cookie after reading it - ctx.db.deleteAuthFlow(flowId) - res.clearCookie(AUTH_FLOW_COOKIE) - } else { - // Legacy mode: direct params - requestUri = req.body.request_uri as string - email = req.body.email as string - isNew = req.body.is_new === '1' - clientId = (req.body.client_id as string) || '' - - if (!requestUri || !email) { - res.status(400).send('

Missing parameters

') - return - } - } - - if (action === 'deny') { - // Redirect back to client with access_denied error via the PDS oauth provider - res.redirect( - 303, - `${ctx.config.pdsPublicUrl}/oauth/authorize?request_uri=${encodeURIComponent(requestUri)}&error=access_denied`, - ) - return - } - - // action === 'approve' - // Record first-time consent for this email+client combination - if (clientId) { - ctx.db.recordClientLogin(email, clientId) - } - - // Build HMAC-signed redirect URL to pds-core /oauth/epds-callback - const callbackParams = { - request_uri: requestUri, - email, - approved: '1', - new_account: isNew ? '1' : '0', - } - const { sig, ts } = signCallback( - callbackParams, - ctx.config.epdsCallbackSecret, - ) - const params = new URLSearchParams({ ...callbackParams, ts, sig }) - - logger.info( - { email, isNew, clientId }, - 'Consent approved, redirecting to epds-callback', - ) - res.redirect( - 303, - `${ctx.config.pdsPublicUrl}/oauth/epds-callback?${params.toString()}`, - ) - }) - - return router -} - -function renderConsent(opts: { - flowId: string | undefined - requestUri: string - email: string - isNew: boolean - clientId: string - clientName: string - csrfToken: string -}): string { - const title = opts.isNew - ? 'Create Account & Authorize' - : 'Authorize Application' - - // Hidden fields: use flow_id if available, otherwise fall back to direct params - const hiddenFields = opts.flowId - ? ` - - ` - : ` - - - ` - - return ` - - - - - ${title} - - - -
-

${title}

-

${escapeHtml(opts.clientName)} wants to access your account

- - ${opts.isNew ? '' : ''} - -
-

Requested permissions:

-
    -
  • Read and write posts
    • -
  • Access your profile
    • -
  • Manage your follows
    • -
-
- -
- - ${hiddenFields} -
- - -
-
-
- -` -} diff --git a/packages/pds-core/src/index.ts b/packages/pds-core/src/index.ts index f64f8bd..31b2146 100644 --- a/packages/pds-core/src/index.ts +++ b/packages/pds-core/src/index.ts @@ -147,18 +147,15 @@ async function main() { ) const { deviceId, deviceMetadata } = deviceInfo - // Step 2: Get the pending authorization request + // Step 2: Refresh the PAR request expiry timer. + // Call get() WITHOUT deviceId so it doesn't bind one — the stock + // oauthMiddleware will bind the browser's deviceId when we redirect + // through /oauth/authorize below. // eslint-disable-next-line @typescript-eslint/no-explicit-any -- @atproto/oauth-provider requestManager not exported - const requestData = await (provider.requestManager as any).get( - requestUri, - deviceId, - ) - const { clientId, parameters } = requestData - - // Step 3: Get the client - const client = await provider.clientManager.getClient(clientId) + const requestData = await (provider.requestManager as any).get(requestUri) + const { clientId } = requestData - // Step 4: Resolve or create the account. + // Step 3: Resolve or create the account. // Use the PDS accountManager directly — account.sqlite is the single source of truth. // Backup email lookup (recovery flow) is handled by the auth-service before issuing // the HMAC-signed callback; by the time we reach here, email is the verified primary. @@ -276,65 +273,31 @@ async function main() { } } - // Step 5: Bind account to device session (for future SSO) + // Step 4: Bind account to device session (for future SSO). + // This creates a device-account association so that when the stock + // oauthMiddleware processes the /oauth/authorize redirect below, + // provider.authorize() will find this session and can auto-approve + // or show the upstream consent UI with actual scopes. await provider.accountManager.upsertDeviceAccount(deviceId, account.sub) - // Step 6: Issue authorization code - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- @atproto/oauth-provider requestManager not exported - const code = await (provider.requestManager as any).setAuthorized( - requestUri, - client, - account, - deviceId, - deviceMetadata, - ) - - // Step 7: Update authorized clients (consent tracking) - const { authorizedClients } = await provider.accountManager.getAccount( - account.sub, - ) - const clientData = authorizedClients.get(clientId) - if (provider.checkConsentRequired(parameters, clientData)) { - const scopes = new Set(clientData?.authorizedScopes) - for (const s of parameters.scope?.split(' ') ?? []) scopes.add(s) - await provider.accountManager.setAuthorizedClient(account, client, { - ...clientData, - authorizedScopes: [...scopes], - }) - } - - // Step 8: Build redirect URL and send user back to client - const redirectUri = parameters.redirect_uri - if (!redirectUri) { - res - .status(400) - .json({ error: 'No redirect_uri in authorization request' }) - return - } - - const redirectUrl = new URL(redirectUri) - const responseMode = parameters.response_mode || 'query' - - const redirectParams: [string, string][] = [ - ['iss', pdsUrl], - ['code', code], - ] - if (parameters.state) { - redirectParams.push(['state', parameters.state]) - } - - if (responseMode === 'fragment') { - const fragmentParams = new URLSearchParams() - for (const [k, v] of redirectParams) fragmentParams.set(k, v) - redirectUrl.hash = fragmentParams.toString() - } else { - for (const [k, v] of redirectParams) redirectUrl.searchParams.set(k, v) - } + // Step 5: Redirect through the stock /oauth/authorize endpoint. + // The oauthMiddleware will call provider.authorize() which: + // - Finds the device session we just created via upsertDeviceAccount + // - Checks checkConsentRequired() against actual OAuth scopes + // - Auto-approves if no consent needed (SSO match, previously authorized scopes) + // - Renders the upstream consent UI (consent-view.tsx) if consent is required + // This replaces the broken auth-service consent page that had hard-coded permissions. + const authorizeUrl = new URL('/oauth/authorize', pdsUrl) + authorizeUrl.searchParams.set('request_uri', requestUri) + authorizeUrl.searchParams.set('client_id', clientId) res.setHeader('Cache-Control', 'no-store') - res.redirect(303, redirectUrl.toString()) + res.redirect(303, authorizeUrl.toString()) - logger.info({ did, redirectUri }, 'Auth code issued') + logger.info( + { did, clientId }, + 'ePDS callback: redirecting to stock /oauth/authorize for consent/approval', + ) } catch (err) { logger.error({ err }, 'ePDS callback error') diff --git a/packages/shared/src/db.ts b/packages/shared/src/db.ts index 8bffef8..9c1bc0d 100644 --- a/packages/shared/src/db.ts +++ b/packages/shared/src/db.ts @@ -175,6 +175,12 @@ export class EpdsDb { () => { this.db.exec(`DROP TABLE IF EXISTS account_session;`) }, + + // v8: Drop client_logins table — consent tracking is now handled by the + // stock @atproto/oauth-provider via its authorizedClients mechanism. + () => { + this.db.exec(`DROP TABLE IF EXISTS client_logins;`) + }, ] for (let i = currentVersion; i < migrations.length; i++) { @@ -479,23 +485,6 @@ export class EpdsDb { return { pendingTokens, backupEmails, rateLimitEntries } } - // ── Per-Client Login Tracking ── - - hasClientLogin(email: string, clientId: string): boolean { - const row = this.db - .prepare(`SELECT 1 FROM client_logins WHERE email = ? AND client_id = ?`) - .get(email.toLowerCase(), clientId) - return !!row - } - - recordClientLogin(email: string, clientId: string): void { - this.db - .prepare( - `INSERT OR IGNORE INTO client_logins (email, client_id, first_login_at) VALUES (?, ?, ?)`, - ) - .run(email.toLowerCase(), clientId, Date.now()) - } - close(): void { this.db.close() } From 9a4c5c33a9474ac2c77dbbe8a14e8fbe85ed4464 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Sat, 14 Mar 2026 15:26:27 +0000 Subject: [PATCH 2/2] test: add handle validation tests and ratchet coverage thresholds - Add handle.test.ts with 13 tests for validateLocalPart() - Add migration v8 tests verifying client_logins table is dropped - Ratchet coverage thresholds up (statements: 21, branches: 15, functions: 35, lines: 20) to reflect new coverage floor - Document coverage ratcheting policy in AGENTS.md --- AGENTS.md | 25 ++++++++ packages/shared/src/__tests__/db.test.ts | 20 ++++++ packages/shared/src/__tests__/handle.test.ts | 66 ++++++++++++++++++++ vitest.config.ts | 9 +-- 4 files changed, 116 insertions(+), 4 deletions(-) create mode 100644 packages/shared/src/__tests__/handle.test.ts diff --git a/AGENTS.md b/AGENTS.md index 74eec92..a024f26 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -47,6 +47,31 @@ pnpm vitest run packages/shared Tests live in `packages//src/__tests__/`. There is no per-package test script — all tests are run from the root via vitest. +### Coverage Ratcheting Policy + +Coverage thresholds in `vitest.config.ts` must **only ever increase**. +When a PR increases coverage above the current thresholds, the thresholds +must be ratcheted up to the new floor (rounded down to the nearest integer) +as part of the same PR. This ensures coverage can never regress. + +```bash +pnpm test:coverage # check current coverage vs thresholds +``` + +After confirming coverage exceeds thresholds, update `vitest.config.ts`: + +```ts +thresholds: { + statements: , + branches: , + functions: , + lines: , +}, +``` + +**Never lower thresholds.** If a change removes tested code (e.g. deleting +a feature), add tests for other code to compensate. + ## Docker ```bash diff --git a/packages/shared/src/__tests__/db.test.ts b/packages/shared/src/__tests__/db.test.ts index 9f2cae4..bc75f95 100644 --- a/packages/shared/src/__tests__/db.test.ts +++ b/packages/shared/src/__tests__/db.test.ts @@ -213,3 +213,23 @@ describe('Auth Flow Operations', () => { expect(db.getAuthFlow('flow-cleanup')).toBeUndefined() }) }) + +describe('Migration: v8 drops client_logins table', () => { + it('client_logins table does not exist after migration', () => { + // EpdsDb constructor runs all migrations including v8. + // Verify the table was dropped by querying sqlite_master. + const tables = db['db'] + .prepare( + `SELECT name FROM sqlite_master WHERE type='table' AND name='client_logins'`, + ) + .all() + expect(tables).toHaveLength(0) + }) + + it('schema version is at least 8 after migration', () => { + const row = db['db'] + .prepare('SELECT version FROM schema_version') + .get() as { version: number } + expect(row.version).toBeGreaterThanOrEqual(8) + }) +}) diff --git a/packages/shared/src/__tests__/handle.test.ts b/packages/shared/src/__tests__/handle.test.ts new file mode 100644 index 0000000..a2cddda --- /dev/null +++ b/packages/shared/src/__tests__/handle.test.ts @@ -0,0 +1,66 @@ +import { describe, it, expect } from 'vitest' +import { validateLocalPart, LOCAL_PART_MIN, LOCAL_PART_MAX } from '../handle.js' + +const DOMAIN = 'pds.example.com' + +describe('validateLocalPart', () => { + it('accepts a valid local part', () => { + expect(validateLocalPart('alice', DOMAIN)).toBe('alice') + }) + + it('normalizes to lowercase', () => { + expect(validateLocalPart('Alice', DOMAIN)).toBe('alice') + expect(validateLocalPart('BOB99', DOMAIN)).toBe('bob99') + }) + + it('accepts hyphens in the middle', () => { + expect(validateLocalPart('my-handle', DOMAIN)).toBe('my-handle') + }) + + it('accepts exactly LOCAL_PART_MIN characters', () => { + const handle = 'a'.repeat(LOCAL_PART_MIN) + expect(validateLocalPart(handle, DOMAIN)).toBe(handle) + }) + + it('accepts exactly LOCAL_PART_MAX characters', () => { + const handle = 'a'.repeat(LOCAL_PART_MAX) + expect(validateLocalPart(handle, DOMAIN)).toBe(handle) + }) + + it('rejects local part shorter than LOCAL_PART_MIN', () => { + const handle = 'a'.repeat(LOCAL_PART_MIN - 1) + expect(validateLocalPart(handle, DOMAIN)).toBeNull() + }) + + it('rejects local part longer than LOCAL_PART_MAX', () => { + const handle = 'a'.repeat(LOCAL_PART_MAX + 1) + expect(validateLocalPart(handle, DOMAIN)).toBeNull() + }) + + it('rejects dots in the local part', () => { + expect(validateLocalPart('has.dot', DOMAIN)).toBeNull() + }) + + it('rejects empty string', () => { + expect(validateLocalPart('', DOMAIN)).toBeNull() + }) + + it('rejects invalid atproto handle characters', () => { + expect(validateLocalPart('user@name', DOMAIN)).toBeNull() + expect(validateLocalPart('user name', DOMAIN)).toBeNull() + expect(validateLocalPart('user_name', DOMAIN)).toBeNull() + }) + + it('rejects handle starting with hyphen', () => { + expect(validateLocalPart('-alice', DOMAIN)).toBeNull() + }) + + it('rejects handle ending with hyphen', () => { + expect(validateLocalPart('alice-', DOMAIN)).toBeNull() + }) + + it('exports correct min/max constants', () => { + expect(LOCAL_PART_MIN).toBe(5) + expect(LOCAL_PART_MAX).toBe(20) + }) +}) diff --git a/vitest.config.ts b/vitest.config.ts index c4c509e..0758d25 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -15,11 +15,12 @@ export default defineConfig({ reportsDirectory: './coverage', include: ['packages/*/src/**/*.ts'], exclude: ['packages/*/src/__tests__/**', '**/*.test.ts', '**/*.d.ts'], - // Baseline thresholds — ratchet these up as coverage improves + // Ratchet thresholds — update these whenever coverage increases. + // See AGENTS.md for the ratcheting policy. thresholds: { - statements: 20, - branches: 13, - functions: 34, + statements: 21, + branches: 15, + functions: 35, lines: 20, }, },