From 22b6fdeee78e6919b89fb3a6f39a66a774a1e825 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Tue, 22 Sep 2026 17:08:02 -0700 Subject: [PATCH 1/5] Hash Better Auth rate-limit keys and stop trusting a client IP header (audit Q1) The audit's Email codes FAIL: Better Auth's database limiter stored "|" in plaintext, and on Node the IP came from x-pgstencil-client-ip, a header the client supplies, so rotating it escaped the limit and pinning a victim's address exhausted theirs. - rateLimit.customStorage keeps Better Auth's limits and windows but stores keyed(secret, 'rate-limit', key) through one atomic upsert; migration 005 deletes the plaintext rows. - protectAuth overwrites x-pgstencil-client-ip on every request from the @hono/node-server socket (env.incoming), or from a header only when the application opts in with ipAddressHeaders. Better Auth and the per-IP email budget both read that one trusted value. - SECURITY.md states the hashed-key and trusted-IP rules and pins them to five new integration tests; PACKAGES.md documents the opt-in. Co-Authored-By: Claude Opus 5.5 (1M context) --- PACKAGES.md | 10 +- SECURITY.md | 7 +- examples/better-auth/src/dev.ts | 4 +- examples/better-auth/src/node.ts | 23 ++- .../005_hashed_rate_limit_keys.sql | 4 + packages/auth/src/better-auth-security.ts | 81 +++++++- packages/auth/src/better-auth.ts | 18 +- scripts/verify-packages.ts | 2 +- tests/integration/better-auth-oauth.test.ts | 30 +-- tests/integration/better-auth.test.ts | 187 +++++++++++++++++- .../snapshots/better-auth-cookies.json | 2 +- .../snapshots/better-auth-email.md | 6 +- .../snapshots/better-auth-session.json | 6 +- tests/support/better-auth-entry.ts | 5 +- 14 files changed, 333 insertions(+), 52 deletions(-) create mode 100644 packages/auth/better-auth-migrations/005_hashed_rate_limit_keys.sql diff --git a/PACKAGES.md b/PACKAGES.md index 0f85d0d..49a97b8 100644 --- a/PACKAGES.md +++ b/PACKAGES.md @@ -78,7 +78,15 @@ retains its old migration source and adds this one; it must not drop checksum history. Old and new auth tables coexist, but sessions/accounts are independent. Node hosts use `createAuthApp({databaseUrl, origin, secret, email, ...})` and call -`close()` before returning their database lease. Tests bundle their application +`close()` before returning their database lease. Serve `app.fetch` through +`@hono/node-server`, passing its `env` along: the socket in `env.incoming` is the +client IP that Better Auth's per-IP limiter and pgstencil's per-IP email budget +count, and any client-sent `x-pgstencil-client-ip` is overwritten. Behind a +reverse proxy, opt in with `ipAddressHeaders: ['x-real-ip']`, naming a +single-address header the proxy overwrites on every request; never name one a +client can set. A request with neither shares one bucket. The Workers adapter +always counts `cf-connecting-ip`. Stored limiter keys are HMACs under +`AUTH_SECRET`, never raw addresses. Tests bundle their application with esbuild's `inject` set to the **actual module file** resolved from `@pgstencil/auth/better-auth-testing`. Injecting a re-export shim does not work. Use that module's `deterministicScope.run({time, random, outboundFetch}, action)` diff --git a/SECURITY.md b/SECURITY.md index 93518b0..5c75a82 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -2,7 +2,7 @@ pgstencil ships `pgstencil`, `@pgstencil/auth` and `@pgstencil/stripe` as packed tarballs ([PACKAGES.md](PACKAGES.md)). This file states what the Better Auth integration in `@pgstencil/auth` and `pgstencil` itself guarantee to a consuming application, as conditions an auditor can check at one commit; each section ends with the tests that pin them. Bare names are under `packages/auth/src/`, `packages/auth/better-auth-migrations/` or `tests/`. -The application owns everything outside the packages: TLS and its origin gate, the CSP of its own pages, secret and credential storage, database provisioning, and provider registration. `@pgstencil/stripe` and the original code/link `Auth` exports carry no rules here yet. +The application owns everything outside the packages: TLS and its origin gate, the CSP of its own pages, secret and credential storage, database provisioning, provider registration, and — only when it opts in with `ipAddressHeaders` — a proxy that overwrites those headers on every request. `@pgstencil/stripe` and the original code/link `Auth` exports carry no rules here yet. ## Sessions and cookies @@ -26,10 +26,11 @@ Pinned by `integration/better-auth.test.ts`: `auth surface: explicit CSRF, exact ## Email codes - **FAIL IF** a stored email code is recoverable without the application secret, is not separated by purpose from other keyed values, lives longer than ten minutes, survives more than three failed guesses, or is redeemed twice; inspect `emailOTP` in `better-auth.ts` and `keyed` in `better-auth-security.ts`. -- **FAIL IF** a rate-limit key contains a raw email address or IP, or a changing client IP resets an address's one-minute resend cooldown, five sends or fifteen verification attempts per fifteen minutes; inspect `consume` in `better-auth-security.ts`. +- **FAIL IF** a stored rate-limit key — Better Auth's `rateLimit` rows or pgstencil's `pgstencil_auth_limits` — contains a raw email address or IP instead of a secret-keyed HMAC, or a changing client IP resets an address's one-minute resend cooldown, five sends or fifteen verification attempts per fifteen minutes; inspect `rateLimitStorage` and `consume` in `better-auth-security.ts`, `rateLimit` in `better-auth.ts` and `005_hashed_rate_limit_keys.sql`. +- **FAIL IF** Better Auth's per-IP limiter or pgstencil's per-IP email budget counts any address but one trusted client IP — on Node the socket address `@hono/node-server` passes as `env.incoming`, or a forwarded header only when the application names it in `ipAddressHeaders` — or a client-supplied `x-pgstencil-client-ip` reaches either limiter; inspect `protectAuth` in `better-auth-security.ts` and `advanced.ipAddress` in `better-auth.ts`. - **FAIL IF** the Workers adapter counts a client-IP header other than `cf-connecting-ip`, or an address in the reserved `identity.pgstencil.invalid` namespace reaches an email route or the email sender; inspect `better-auth-workers.ts` and `isIdentityEmail`. -Pinned by `integration/better-auth.test.ts`: `email policy: secret-keyed codes, cross-browser redemption, concurrent single use and no token exposure`, `email policy: distributed IPs cannot bypass cooldown, send quota or attempt budget`; `integration/better-auth-workers.test.ts`: `Better Auth in workerd: deterministic replay, separate clocks, shared database rate limits`; `integration/better-auth-oauth.test.ts`: `Optional email: signs in by stable identity without a mailbox`. +Pinned by `integration/better-auth.test.ts`: `email policy: secret-keyed codes, cross-browser redemption, concurrent single use and no token exposure`, `email policy: distributed IPs cannot bypass cooldown, send quota or attempt budget`, `IP rate limits: stored limiter keys never contain a raw client IP`, `IP rate limits: the default Node path counts the socket, so rotating x-pgstencil-client-ip cannot escape`, `IP rate limits: concurrent requests from one address are counted atomically`, `IP rate limits: naming a victim's IP in x-pgstencil-client-ip spends only the caller's budget`, `IP rate limits: an explicit ipAddressHeaders opt-in counts the configured header`; `integration/better-auth-workers.test.ts`: `Better Auth in workerd: deterministic replay, separate clocks, shared database rate limits`; `integration/better-auth-oauth.test.ts`: `Optional email: signs in by stable identity without a mailbox`. ## OAuth diff --git a/examples/better-auth/src/dev.ts b/examples/better-auth/src/dev.ts index 90928f9..716e801 100644 --- a/examples/better-auth/src/dev.ts +++ b/examples/better-auth/src/dev.ts @@ -11,7 +11,7 @@ const database = await allocateDatabase(betterAuthMigrations); const email = new EmailDev(new SystemTime()); let app: ReturnType | undefined; const server = await listen( - async (request) => { + async (request, env) => { if (new URL(request.url).pathname === '/dev/emails') return new Response( await html` @@ -39,7 +39,7 @@ const server = await listen( }, ); return app - ? app.app.fetch(request) + ? app.app.fetch(request, env) : new Response('Starting', { status: 503 }); }, Number(process.env.PORT ?? 8082), diff --git a/examples/better-auth/src/node.ts b/examples/better-auth/src/node.ts index e316d1a..365d777 100644 --- a/examples/better-auth/src/node.ts +++ b/examples/better-auth/src/node.ts @@ -1,21 +1,20 @@ import { createServer } from 'node:http'; import { once } from 'node:events'; -import { getRequestListener } from '@hono/node-server'; +import { + getRequestListener, + type Http2Bindings, + type HttpBindings, +} from '@hono/node-server'; export async function listen( - fetch: (request: Request) => Response | Promise, + fetch: ( + request: Request, + env: HttpBindings | Http2Bindings, + ) => Response | Promise, port = 0, ) { - const server = createServer( - getRequestListener((request, env) => { - // Derive from the actual socket, overwriting any caller-supplied value. - request.headers.set( - 'x-pgstencil-client-ip', - env.incoming.socket.remoteAddress ?? '127.0.0.1', - ); - return fetch(request); - }), - ); + // createAuthApp reads the client IP from env.incoming's socket. + const server = createServer(getRequestListener(fetch)); server.listen(port, '127.0.0.1'); await once(server, 'listening'); const address = server.address(); diff --git a/packages/auth/better-auth-migrations/005_hashed_rate_limit_keys.sql b/packages/auth/better-auth-migrations/005_hashed_rate_limit_keys.sql new file mode 100644 index 0000000..4f69f89 --- /dev/null +++ b/packages/auth/better-auth-migrations/005_hashed_rate_limit_keys.sql @@ -0,0 +1,4 @@ +-- Up Migration +-- Rate-limit keys are now an HMAC of Better Auth's "|" key. Drop the +-- plaintext rows written before; every window they held is under a minute. +DELETE FROM "rateLimit"; diff --git a/packages/auth/src/better-auth-security.ts b/packages/auth/src/better-auth-security.ts index 00e0887..c083cd2 100644 --- a/packages/auth/src/better-auth-security.ts +++ b/packages/auth/src/better-auth-security.ts @@ -26,6 +26,60 @@ function cookies(request: Request) { ); } +/** + * The one header Better Auth and the email budgets read a client IP from. + * `protectAuth` always overwrites or deletes it before any handler runs. + */ +export const clientIpHeader = 'x-pgstencil-client-ip'; + +/** + * Better Auth's per-IP limiter, keyed by an HMAC of its `|` key so the + * table never holds a raw address. One upsert decides and increments atomically, + * across requests and Worker isolates, with Better Auth's rolling-window rule. + */ +export function rateLimitStorage( + db: ReturnType, + secret: string, +) { + return { + async consume(key: string, rule: { window: number; max: number }) { + const hashed = keyed(secret, 'rate-limit', key); + const now = Date.now(); + const cutoff = now - rule.window * 1000; + const result = await sql<{ count: number }>` + INSERT INTO "rateLimit" (id, key, count, "lastRequest") + VALUES (gen_random_uuid()::text, ${hashed}, 1, ${now}) + ON CONFLICT (key) DO UPDATE SET + count = CASE WHEN "rateLimit"."lastRequest" <= ${cutoff} THEN 1 ELSE "rateLimit".count + 1 END, + "lastRequest" = ${now} + WHERE "rateLimit"."lastRequest" <= ${cutoff} OR "rateLimit".count < ${rule.max} + RETURNING count + `.execute(db); + if (result.rows.length === 1) { + // Better Auth's windows are at most a minute; an hour-old row is dead. + if (result.rows[0]!.count === 1) + await sql`DELETE FROM "rateLimit" WHERE "lastRequest" < ${now - 3_600_000}`.execute( + db, + ); + return { allowed: true, retryAfter: null }; + } + const last = await sql<{ + lastRequest: string; + }>`SELECT "lastRequest" FROM "rateLimit" WHERE key = ${hashed}`.execute( + db, + ); + const since = Number(last.rows[0]?.lastRequest ?? now); + return { + allowed: false, + retryAfter: Math.max( + 1, + Math.ceil((since + rule.window * 1000 - now) / 1000), + ), + }; + }, + }; +} + /** Shared, atomic limits: changing client IP cannot reset an email's budget. */ async function consume( db: ReturnType, @@ -53,6 +107,7 @@ export function protectAuth( origin: string; secret: string; database: ReturnType; + /** Trusted forwarded headers; without them only the Node socket counts. */ ipAddressHeaders?: string[]; accountLinking?: 'explicit' | 'same-email'; }, @@ -64,6 +119,27 @@ export function protectAuth( const [token, signature] = value.split('.') as [string, string]; return equal(signature, keyed(options.secret, 'csrf', token)); }; + app.use('*', async (c, next) => { + // Never let a client name its own IP: derive it from the socket that + // @hono/node-server passes as env.incoming, or from a header the + // application explicitly trusts, then overwrite the internal header. + const candidates = options.ipAddressHeaders + ? options.ipAddressHeaders.map((name) => c.req.header(name)) + : [ + ( + c.env as + { incoming?: { socket?: { remoteAddress?: string } } } | undefined + )?.incoming?.socket?.remoteAddress, + ]; + const ip = candidates + .map((value) => value?.trim()) + .find((value) => !!value && /^[0-9A-Fa-f:.]{2,45}$/.test(value)); + const headers = new Headers(c.req.raw.headers); + if (ip) headers.set(clientIpHeader, ip); + else headers.delete(clientIpHeader); + c.req.raw = new Request(c.req.raw, { headers }); + await next(); + }); app.use('*', async (c, next) => { await next(); // Apply to the final response, including upstream immutable redirects. @@ -154,10 +230,7 @@ export function protectAuth( const send = path === '/email-otp/send-verification-otp'; if (send && body.type !== 'sign-in') return c.json({ message: 'Unsupported email operation' }, 400); - const ip = - (options.ipAddressHeaders ?? ['x-pgstencil-client-ip']) - .map((name) => c.req.header(name)) - .find(Boolean) ?? 'unknown'; + const ip = c.req.header(clientIpHeader) ?? 'unknown'; // Bound per-email counter creation even after upstream IP limits reject. if ( !(await consume( diff --git a/packages/auth/src/better-auth.ts b/packages/auth/src/better-auth.ts index 5995cc2..ae32443 100644 --- a/packages/auth/src/better-auth.ts +++ b/packages/auth/src/better-auth.ts @@ -14,9 +14,11 @@ import { sql } from 'kysely'; import { connectDatabase } from 'pgstencil/postgres'; import type { EmailSender } from 'pgstencil'; import { + clientIpHeader, keyed, protectAuth, publicAuthResponse, + rateLimitStorage, } from './better-auth-security.ts'; import { socialProviders, @@ -38,6 +40,11 @@ export interface AuthOptions { origin: string; secret: string; email: EmailSender; + /** + * Opt in to trusting these forwarded headers, in order, for the client IP. + * Only set this behind a proxy that overwrites them. Without it, Node counts + * the socket address and ignores every client-supplied IP header. + */ ipAddressHeaders?: string[]; sessionPolicy?: 'single' | 'multiple'; accountLinking?: 'explicit' | 'same-email'; @@ -184,12 +191,15 @@ export function authOptions(options: AuthOptions): BetterAuthOptions { }, disableOriginCheck: false, disableCSRFCheck: false, - ipAddress: { - ipAddressHeaders: options.ipAddressHeaders ?? ['x-pgstencil-client-ip'], - }, + // protectAuth overwrites this header from a trusted source on every request. + ipAddress: { ipAddressHeaders: [clientIpHeader] }, }, // Workers are request-scoped; an in-memory limiter would reset every request. - rateLimit: { enabled: true, storage: 'database' }, + rateLimit: { + enabled: true, + storage: 'database', + customStorage: rateLimitStorage(options.database, options.secret), + }, session: { additionalFields: { emailAuthenticated: { diff --git a/scripts/verify-packages.ts b/scripts/verify-packages.ts index 70f54a1..b4c7e3c 100644 --- a/scripts/verify-packages.ts +++ b/scripts/verify-packages.ts @@ -106,7 +106,7 @@ try { const csrfResponse = await modern.app.fetch(new Request('https://consumer.test/api/auth/csrf')); const csrf = (await csrfResponse.json()).csrf; const cookie = csrfResponse.headers.getSetCookie().map((v) => v.split(';')[0]).join('; '); - const post = (path: string, body: object) => modern.app.fetch(new Request('https://consumer.test/api/auth/' + path, {method:'POST',headers:{origin:'https://consumer.test',cookie,'x-csrf-token':csrf,'content-type':'application/json','x-pgstencil-client-ip':'127.0.0.1'},body:JSON.stringify(body)})); + const post = (path: string, body: object) => modern.app.fetch(new Request('https://consumer.test/api/auth/' + path, {method:'POST',headers:{origin:'https://consumer.test',cookie,'x-csrf-token':csrf,'content-type':'application/json'},body:JSON.stringify(body)}),{incoming:{socket:{remoteAddress:'127.0.0.1'}}}); assert.equal((await post('email-otp/send-verification-otp',{email:'modern@example.test',type:'sign-in'})).status,200); const otp = (await email.next()).text.match(/\\b\\d{8}\\b/)![0]; const signedIn = await post('sign-in/email-otp',{email:'modern@example.test',otp}); diff --git a/tests/integration/better-auth-oauth.test.ts b/tests/integration/better-auth-oauth.test.ts index e719b3b..e72f071 100644 --- a/tests/integration/better-auth-oauth.test.ts +++ b/tests/integration/better-auth-oauth.test.ts @@ -82,8 +82,11 @@ async function fixture( oauth: allOAuthCredentials, outboundFetch, }); - const request = (path: string, init?: RequestInit) => - app.fetch(new Request(origin + path, init)); + const request = (path: string, init?: RequestInit, ip?: string) => + app.fetch( + new Request(origin + path, init), + ip ? { incoming: { socket: { remoteAddress: ip } } } : undefined, + ); let ip = 0; const browser = async () => { const csrfResponse = await request('/api/auth/csrf'); @@ -106,17 +109,20 @@ async function fixture( cookie, post: async (path: string, body: object) => accept( - await request('/api/auth/' + path, { - method: 'POST', - headers: { - origin, - cookie: cookie(), - 'x-csrf-token': csrf, - 'content-type': 'application/json', - 'x-pgstencil-client-ip': address, + await request( + '/api/auth/' + path, + { + method: 'POST', + headers: { + origin, + cookie: cookie(), + 'x-csrf-token': csrf, + 'content-type': 'application/json', + }, + body: JSON.stringify(body), }, - body: JSON.stringify(body), - }), + address, + ), ), get: async (path: string) => accept(await request(path, { headers: { cookie: cookie() } })), diff --git a/tests/integration/better-auth.test.ts b/tests/integration/better-auth.test.ts index 1de2d35..eda12d6 100644 --- a/tests/integration/better-auth.test.ts +++ b/tests/integration/better-auth.test.ts @@ -46,6 +46,7 @@ const built = (async () => { async function fixture( now = '2020-01-01T00:00:00.000Z', sessionPolicy: 'single' | 'multiple' = 'multiple', + extra: { ipAddressHeaders?: string[] } = {}, ) { const context = await createTestContext({ migrations, @@ -56,6 +57,7 @@ async function fixture( const app = createDeterministicApp({ ...context, sessionPolicy, + ...extra, databaseUrl: context.database.url, origin, secret: 'better-auth-local-test-secret-32-characters', @@ -221,12 +223,16 @@ test('Better Auth email rejects expired codes and cross-origin sign-in', async ( expect(f.email.all()).toHaveLength(1); }); +/** The binding @hono/node-server passes: the connection a client cannot forge. */ +const socket = (remoteAddress: string) => ({ + incoming: { socket: { remoteAddress } }, +}); async function directPost( f: Fixture, path: string, body: object, ip = '192.0.2.1', - cookie = '', + headers: Record = {}, ) { return f.app.fetch( new Request(origin + '/api/auth/' + path, { @@ -235,11 +241,12 @@ async function directPost( origin, 'content-type': 'application/json', 'x-csrf-token': f.csrf, - 'x-pgstencil-client-ip': ip, - cookie: [f.csrfCookie, cookie].filter(Boolean).join('; '), + cookie: f.csrfCookie, + ...headers, }, body: JSON.stringify(body), }), + socket(ip), ); } @@ -283,10 +290,10 @@ test('email policy: secret-keyed codes, cross-browser redemption, concurrent sin 'content-type': 'application/json', 'x-csrf-token': other.body.csrf, cookie: cookieFrom(other), - 'x-pgstencil-client-ip': `192.0.2.${i + 1}`, }, body: JSON.stringify({ email: 'alice@example.test', otp }), }), + socket(`192.0.2.${i + 1}`), ), ), ); @@ -389,6 +396,178 @@ test('email policy: distributed IPs cannot bypass cooldown, send quota or attemp ).toBe(200); }); +const secret = 'better-auth-local-test-secret-32-characters'; +const hmac = async (purpose: string, value: string) => + (await import('node:crypto')) + .createHmac('sha256', secret) + .update(purpose + '\0') + .update(value) + .digest('hex'); +const sendStatuses = async ( + count: number, + send: (email: string, i: number) => Promise<{ status: number }>, +) => { + const statuses = []; + for (let i = 0; i < count; i++) + statuses.push( + (await send(`ip-${i}-${statuses.length}@example.test`, i)).status, + ); + return statuses; +}; +const sendOtp = { type: 'sign-in' }; + +test('IP rate limits: stored limiter keys never contain a raw client IP', async ({ + onTestFinished, +}) => { + const f = await fixture(); + onTestFinished(() => f.close()); + await f + .post('email-otp/send-verification-otp', { + email: 'loopback@example.test', + type: 'sign-in', + }) + .expect(200); + expect( + await sendStatuses(4, (email) => + directPost( + f, + 'email-otp/send-verification-otp', + { ...sendOtp, email }, + '203.0.113.50', + ), + ), + ).toEqual([200, 200, 200, 429]); + await directPost( + f, + 'sign-in/email-otp', + { email: 'loopback@example.test', otp: '00000000' }, + '2001:db8::77', + ); + const upstream = await queryDatabase<{ key: string }>( + f.database.url, + 'SELECT * FROM "rateLimit"', + ); + const own = await queryDatabase<{ key: string }>( + f.database.url, + 'SELECT * FROM pgstencil_auth_limits', + ); + const stored = JSON.stringify([upstream, own]); + for (const ip of ['203.0.113.50', '2001:db8', '127.0.0.1', '::1']) + expect(stored).not.toContain(ip); + expect(upstream.length).toBeGreaterThanOrEqual(3); + for (const row of upstream) expect(row.key).toMatch(/^[0-9a-f]{64}$/); + // Both limiters count the same trusted address, each under its own secret-keyed purpose. + expect(upstream.map((row) => row.key)).toContain( + await hmac('rate-limit', '203.0.113.50|/email-otp/send-verification-otp'), + ); + expect(own.map((row) => row.key)).toContain( + `send:ip:${await hmac('ip-rate', '203.0.113.50')}`, + ); +}); + +test('IP rate limits: the default Node path counts the socket, so rotating x-pgstencil-client-ip cannot escape', async ({ + onTestFinished, +}) => { + const f = await fixture(); + onTestFinished(() => f.close()); + // A real @hono/node-server socket: every request arrives from loopback. + expect( + await sendStatuses(4, (email, i) => + f.client + .post('/api/auth/email-otp/send-verification-otp') + .set('Origin', origin) + .set('Cookie', f.csrfCookie) + .set('X-CSRF-Token', f.csrf) + .set('x-pgstencil-client-ip', `198.51.100.${i + 1}`) + .set('x-forwarded-for', `198.51.100.${i + 1}`) + .send({ ...sendOtp, email }), + ), + ).toEqual([200, 200, 200, 429]); +}); + +test('IP rate limits: concurrent requests from one address are counted atomically', async ({ + onTestFinished, +}) => { + const f = await fixture(); + onTestFinished(() => f.close()); + const burst = await Promise.all( + Array.from({ length: 8 }, (_, i) => + directPost( + f, + 'email-otp/send-verification-otp', + { ...sendOtp, email: `burst-${i}@example.test` }, + '198.51.100.30', + ), + ), + ); + expect(burst.filter((r) => r.status === 200)).toHaveLength(3); + expect(burst.filter((r) => r.status === 429)).toHaveLength(5); +}); + +test("IP rate limits: naming a victim's IP in x-pgstencil-client-ip spends only the caller's budget", async ({ + onTestFinished, +}) => { + const f = await fixture(); + onTestFinished(() => f.close()); + const victim = '203.0.113.9'; + expect( + await sendStatuses(4, (email) => + directPost( + f, + 'email-otp/send-verification-otp', + { ...sendOtp, email }, + '198.51.100.7', + { 'x-pgstencil-client-ip': victim }, + ), + ), + ).toEqual([200, 200, 200, 429]); + expect( + ( + await directPost( + f, + 'email-otp/send-verification-otp', + { ...sendOtp, email: 'victim@example.test' }, + victim, + ) + ).status, + ).toBe(200); +}); + +test('IP rate limits: an explicit ipAddressHeaders opt-in counts the configured header', async ({ + onTestFinished, +}) => { + const f = await fixture(undefined, undefined, { + ipAddressHeaders: ['x-real-ip'], + }); + onTestFinished(() => f.close()); + // Behind the trusted proxy every socket is the proxy's; the header decides. + expect( + await sendStatuses(4, (email, i) => + directPost( + f, + 'email-otp/send-verification-otp', + { ...sendOtp, email }, + '10.0.0.1', + { 'x-real-ip': `198.51.100.${i + 1}` }, + ), + ), + ).toEqual([200, 200, 200, 200]); + expect( + await sendStatuses(4, (email, i) => + directPost( + f, + 'email-otp/send-verification-otp', + { ...sendOtp, email: 'again-' + email }, + '10.0.0.1', + { + 'x-real-ip': '203.0.113.20', + 'x-pgstencil-client-ip': `192.0.2.${i + 1}`, + }, + ), + ), + ).toEqual([200, 200, 200, 429]); +}); + test('auth surface: explicit CSRF, exact origin, security headers and disabled unused endpoints', async ({ onTestFinished, }) => { diff --git a/tests/integration/snapshots/better-auth-cookies.json b/tests/integration/snapshots/better-auth-cookies.json index 1f3b281..d101636 100644 --- a/tests/integration/snapshots/better-auth-cookies.json +++ b/tests/integration/snapshots/better-auth-cookies.json @@ -1,3 +1,3 @@ [ - "__Host-pgstencil.session_token=w2yuuiOO8YfDZfLCdfUGFslJ8qZHIS6V.mqayyrrKNWCIB0180YVZOcSKWCcgtgQmnfgOCgZcgp4%3D; Max-Age=86400; Path=/; HttpOnly; Secure; SameSite=Lax" + "__Host-pgstencil.session_token=znQJVUNBKgzMDfXmgy4tNwbzdAxy8Zpo.zBzOyU8x%2BWdTnRtM0x1jxEZDwz5qsEsXwqiYn3HRJMk%3D; Max-Age=86400; Path=/; HttpOnly; Secure; SameSite=Lax" ] diff --git a/tests/integration/snapshots/better-auth-email.md b/tests/integration/snapshots/better-auth-email.md index 26f955b..361cb7b 100644 --- a/tests/integration/snapshots/better-auth-email.md +++ b/tests/integration/snapshots/better-auth-email.md @@ -10,14 +10,14 @@ ## Plaintext -Your sign-in code is 19667655. It expires in 10 minutes. +Your sign-in code is 47752232. It expires in 10 minutes. ## Markdown -Your sign-in code is **19667655**. +Your sign-in code is **47752232**. It expires in 10 minutes. ## HTML -

Your sign-in code is 19667655.

It expires in 10 minutes.

+

Your sign-in code is 47752232.

It expires in 10 minutes.

diff --git a/tests/integration/snapshots/better-auth-session.json b/tests/integration/snapshots/better-auth-session.json index bca2d30..1c90d94 100644 --- a/tests/integration/snapshots/better-auth-session.json +++ b/tests/integration/snapshots/better-auth-session.json @@ -3,12 +3,12 @@ "createdAt": "2020-01-01T00:00:00.000Z", "emailAuthenticated": true, "expiresAt": "2020-01-02T00:00:00.000Z", - "id": "4jTWBPymxPMErp1QsQeJ4O6cDuEjb0zY", + "id": "Z4QlCqqJmzirhpnjxhm1hzgQNDhFSWGL", "ipAddress": "127.0.0.1", "singleSession": false, - "token": "w2yuuiOO8YfDZfLCdfUGFslJ8qZHIS6V", + "token": "znQJVUNBKgzMDfXmgy4tNwbzdAxy8Zpo", "updatedAt": "2020-01-01T00:00:00.000Z", "userAgent": "", - "userId": "Z4QlCqqJmzirhpnjxhm1hzgQNDhFSWGL" + "userId": "WUE21FXrpzK25M6JkIkC2CnTS34plZ2u" } ] diff --git a/tests/support/better-auth-entry.ts b/tests/support/better-auth-entry.ts index d728882..f9e1d22 100644 --- a/tests/support/better-auth-entry.ts +++ b/tests/support/better-auth-entry.ts @@ -12,8 +12,9 @@ export function createDeterministicApp( const app = deterministicScope.run(options, () => createEmailApp(options)); return { close: app.close, - fetch: (request: Request) => - deterministicScope.run(options, () => app.app.fetch(request)), + // env carries the Node socket (`incoming`) that the client IP is read from. + fetch: (request: Request, env?: object) => + deterministicScope.run(options, () => app.app.fetch(request, env)), // Deliberately crosses async boundaries to test independent concurrent app contexts. probe: () => deterministicScope.run(options, async () => { From c922897b4ae4f3627150631441681ebfeb4365e6 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Tue, 22 Sep 2026 17:08:45 -0700 Subject: [PATCH 2/5] Narrow the OAuth error-detail rule to what diagnostics records (audit Q2) The audit's OAuth FAIL: SECURITY.md said no provider error code reaches a diagnostic record, but diagnosticError deliberately records allowlisted OAuth codes and Microsoft's bounded numeric code, and a unit test pins that. The rule now says exactly that: a provider's error description, or a code outside the errorCodes allowlist, never reaches a redirect, a page or a diagnostic record; an allowlisted code may reach only a diagnostic record. The Diagnostics rule names the allowlist and provider descriptions too. Co-Authored-By: Claude Opus 5.5 (1M context) --- SECURITY.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index 5c75a82..6f5dcc8 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -37,9 +37,10 @@ Pinned by `integration/better-auth.test.ts`: `email policy: secret-keyed codes, - **FAIL IF** a callback is processed without a signed browser-bound state cookie, an unexpired state row for the provider it names, and an atomic single-use claim taken in Postgres before the code exchange; inspect `oauthRequest` in `better-auth-oauth.ts` and `003_oauth_claims.sql`. - **FAIL IF** a caller can choose the callback destination, the scopes or any OAuth parameter beyond the provider name, or can sign in by presenting a provider ID or access token directly; inspect `oauthRequest` and `protectAuth`. - **FAIL IF** a Google, Apple or Microsoft ID token is accepted without signature, issuer, audience, expiry and nonce verification or under an algorithm other than RS256, a Microsoft identity is keyed on anything but a verified tenant and object ID, or GitHub signs in without a verified primary email; inspect `verifiedOidc`, `socialProviders` and `providerSubject` in `better-auth-email.ts`. -- **FAIL IF** a provider access, refresh or ID token survives identity verification in the database or a response, or a provider's error code or description reaches a redirect URL, a page or a diagnostic record; inspect `databaseHooks.account` and the callback redirect handling in `oauthRequest`. +- **FAIL IF** a provider access, refresh or ID token survives identity verification in the database or a response; inspect `databaseHooks.account`. +- **FAIL IF** a provider's error description, or any provider error code outside the `errorCodes` allowlist in `packages/pgstencil/src/diagnostics.ts`, reaches a redirect URL, a page or a diagnostic record, or an allowlisted code reaches a redirect URL or a page; a diagnostic record may carry an allowlisted code as `errorCode` and Microsoft's bounded numeric `error_codes[0]` as `providerCode`, nothing else from the provider's error; inspect the callback redirect handling in `oauthRequest` and `diagnosticError`. -Pinned by `integration/better-auth-oauth.test.ts`: `Better Auth OAuth: login, safe token storage and callback replay`, `Better Auth OAuth: rejects invalid signed claims`, `Better Auth OAuth: concurrent callbacks exchange once and errors contain no provider details`, `Better Auth OAuth: caller cannot override callback origin or use direct provider tokens`, `Microsoft tenant identity and unverified email cannot capture another account`; `integration/better-auth-workers.test.ts`: `Better Auth OAuth in workerd: `. +Pinned by `integration/better-auth-oauth.test.ts`: `Better Auth OAuth: login, safe token storage and callback replay`, `Better Auth OAuth: rejects invalid signed claims`, `Better Auth OAuth: concurrent callbacks exchange once and errors contain no provider details`, `Better Auth OAuth: caller cannot override callback origin or use direct provider tokens`, `Microsoft tenant identity and unverified email cannot capture another account`; `integration/better-auth-workers.test.ts`: `Better Auth OAuth in workerd: `; `unit/diagnostics.test.ts`: `diagnostics allowlist drops secrets even in unexpected fields and malformed values`, `provider error extraction keeps numeric codes and tolerates hostile getters`. ## Account linking @@ -52,7 +53,7 @@ Pinned by `integration/better-auth-oauth.test.ts`: `Better Auth OAuth: email col ## Diagnostics -- **FAIL IF** a diagnostic record carries anything outside the enumerated events, categories, bounded numbers, booleans and validated correlation identifiers — never an email, code, token, cookie, header, path, query string, SQL statement, error message or stack; inspect `diagnostic` and `diagnosticError` in `packages/pgstencil/src/diagnostics.ts`. +- **FAIL IF** a diagnostic record carries anything outside the enumerated events, categories (error codes only from the `errorCodes` allowlist), bounded numbers, booleans and validated correlation identifiers — never an email, sign-in code, token, cookie, header, path, query string, SQL statement, error message, provider error description or stack; inspect `diagnostic` and `diagnosticError` in `packages/pgstencil/src/diagnostics.ts`. - **FAIL IF** a failing or hostile sink changes a response, leaks exception text, or lets one request's context reach another's record; inspect `diagnostic` and `observeRequest`. - **FAIL IF** a caller-supplied request ID is trusted, or an expected 4xx auth rejection is recorded as a server failure; inspect `observeRequest` and `onAPIError` in `better-auth.ts`. From a6c077d579761793c5cc4c47af184c72fcc4dd4b Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Tue, 22 Sep 2026 17:11:28 -0700 Subject: [PATCH 3/5] Pin the live auth controls no test exercised (audit Q7) The audit found controls that exist in code but that no Pinned-by test drives, so a regression would pass CI. New integration tests: - RS256 pinning: ID tokens forged under HS256 (keyed with the public key), none, PS256 and RS512 are rejected for Google, Apple and Microsoft. With the pin removed, Microsoft's PS256 token signs in, so this test fails. - 415 for a non-JSON body on an allowlisted write. - 405 for a non-GET on a non-Apple callback and for Apple POST without form encoding. - authOptions rejects a non-canonical origin, a secret under 32 characters and off-origin success/error paths (//, /\, tab variants). - The fifteen-verifications-per-fifteen-minutes budget, across IPs and against the right code. - X-Content-Type-Options and X-Frame-Options on a page and an API response. SECURITY.md names each test in its Pinned-by line. Co-Authored-By: Claude Opus 5.5 (1M context) --- SECURITY.md | 6 +- tests/integration/better-auth-oauth.test.ts | 48 +++++++++ tests/integration/better-auth.test.ts | 107 ++++++++++++++++++++ tests/support/oauth-server.ts | 40 +++++++- 4 files changed, 195 insertions(+), 6 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index 6f5dcc8..4a77e45 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -21,7 +21,7 @@ Pinned by `integration/better-auth.test.ts`: `email policy: secret-keyed codes, - **FAIL IF** a response leaves without `Cache-Control: no-store`, `Referrer-Policy: no-referrer`, `X-Content-Type-Options: nosniff`, `X-Frame-Options: DENY` and a CSP denying framing, inline script and third-party sources; inspect `protectAuth`. - **FAIL IF** the app accepts a non-canonical origin, a secret under 32 characters, or a success/error path off the application origin; inspect `authOptions` in `better-auth.ts`. -Pinned by `integration/better-auth.test.ts`: `auth surface: explicit CSRF, exact origin, security headers and disabled unused endpoints`, `Better Auth email rejects expired codes and cross-origin sign-in`; `integration/better-auth-oauth.test.ts`: `Better Auth OAuth: Apple form_post relay, wrong browser, mismatched provider and expired state`. +Pinned by `integration/better-auth.test.ts`: `auth surface: explicit CSRF, exact origin, security headers and disabled unused endpoints`, `auth surface: an allowlisted write with a non-JSON body answers 415`, `auth options reject a non-canonical origin, a short secret and off-origin redirect paths`, `Better Auth email rejects expired codes and cross-origin sign-in`; `integration/better-auth-oauth.test.ts`: `Better Auth OAuth: Apple form_post relay, wrong browser, mismatched provider and expired state`, `Better Auth OAuth: callbacks refuse every method but GET and Apple's form-encoded POST`. ## Email codes @@ -30,7 +30,7 @@ Pinned by `integration/better-auth.test.ts`: `auth surface: explicit CSRF, exact - **FAIL IF** Better Auth's per-IP limiter or pgstencil's per-IP email budget counts any address but one trusted client IP — on Node the socket address `@hono/node-server` passes as `env.incoming`, or a forwarded header only when the application names it in `ipAddressHeaders` — or a client-supplied `x-pgstencil-client-ip` reaches either limiter; inspect `protectAuth` in `better-auth-security.ts` and `advanced.ipAddress` in `better-auth.ts`. - **FAIL IF** the Workers adapter counts a client-IP header other than `cf-connecting-ip`, or an address in the reserved `identity.pgstencil.invalid` namespace reaches an email route or the email sender; inspect `better-auth-workers.ts` and `isIdentityEmail`. -Pinned by `integration/better-auth.test.ts`: `email policy: secret-keyed codes, cross-browser redemption, concurrent single use and no token exposure`, `email policy: distributed IPs cannot bypass cooldown, send quota or attempt budget`, `IP rate limits: stored limiter keys never contain a raw client IP`, `IP rate limits: the default Node path counts the socket, so rotating x-pgstencil-client-ip cannot escape`, `IP rate limits: concurrent requests from one address are counted atomically`, `IP rate limits: naming a victim's IP in x-pgstencil-client-ip spends only the caller's budget`, `IP rate limits: an explicit ipAddressHeaders opt-in counts the configured header`; `integration/better-auth-workers.test.ts`: `Better Auth in workerd: deterministic replay, separate clocks, shared database rate limits`; `integration/better-auth-oauth.test.ts`: `Optional email: signs in by stable identity without a mailbox`. +Pinned by `integration/better-auth.test.ts`: `email policy: secret-keyed codes, cross-browser redemption, concurrent single use and no token exposure`, `email policy: distributed IPs cannot bypass cooldown, send quota or attempt budget`, `email policy: fifteen verification attempts per fifteen minutes stop even the right code from any IP`, `IP rate limits: stored limiter keys never contain a raw client IP`, `IP rate limits: the default Node path counts the socket, so rotating x-pgstencil-client-ip cannot escape`, `IP rate limits: concurrent requests from one address are counted atomically`, `IP rate limits: naming a victim's IP in x-pgstencil-client-ip spends only the caller's budget`, `IP rate limits: an explicit ipAddressHeaders opt-in counts the configured header`; `integration/better-auth-workers.test.ts`: `Better Auth in workerd: deterministic replay, separate clocks, shared database rate limits`; `integration/better-auth-oauth.test.ts`: `Optional email: signs in by stable identity without a mailbox`. ## OAuth @@ -40,7 +40,7 @@ Pinned by `integration/better-auth.test.ts`: `email policy: secret-keyed codes, - **FAIL IF** a provider access, refresh or ID token survives identity verification in the database or a response; inspect `databaseHooks.account`. - **FAIL IF** a provider's error description, or any provider error code outside the `errorCodes` allowlist in `packages/pgstencil/src/diagnostics.ts`, reaches a redirect URL, a page or a diagnostic record, or an allowlisted code reaches a redirect URL or a page; a diagnostic record may carry an allowlisted code as `errorCode` and Microsoft's bounded numeric `error_codes[0]` as `providerCode`, nothing else from the provider's error; inspect the callback redirect handling in `oauthRequest` and `diagnosticError`. -Pinned by `integration/better-auth-oauth.test.ts`: `Better Auth OAuth: login, safe token storage and callback replay`, `Better Auth OAuth: rejects invalid signed claims`, `Better Auth OAuth: concurrent callbacks exchange once and errors contain no provider details`, `Better Auth OAuth: caller cannot override callback origin or use direct provider tokens`, `Microsoft tenant identity and unverified email cannot capture another account`; `integration/better-auth-workers.test.ts`: `Better Auth OAuth in workerd: `; `unit/diagnostics.test.ts`: `diagnostics allowlist drops secrets even in unexpected fields and malformed values`, `provider error extraction keeps numeric codes and tolerates hostile getters`. +Pinned by `integration/better-auth-oauth.test.ts`: `Better Auth OAuth: login, safe token storage and callback replay`, `Better Auth OAuth: rejects invalid signed claims`, `Better Auth OAuth: rejects ID tokens whose header names an algorithm other than RS256`, `Better Auth OAuth: concurrent callbacks exchange once and errors contain no provider details`, `Better Auth OAuth: caller cannot override callback origin or use direct provider tokens`, `Microsoft tenant identity and unverified email cannot capture another account`; `integration/better-auth-workers.test.ts`: `Better Auth OAuth in workerd: `; `unit/diagnostics.test.ts`: `diagnostics allowlist drops secrets even in unexpected fields and malformed values`, `provider error extraction keeps numeric codes and tolerates hostile getters`. ## Account linking diff --git a/tests/integration/better-auth-oauth.test.ts b/tests/integration/better-auth-oauth.test.ts index e72f071..64b8cc8 100644 --- a/tests/integration/better-auth-oauth.test.ts +++ b/tests/integration/better-auth-oauth.test.ts @@ -244,6 +244,26 @@ for (const provider of ['google', 'apple', 'microsoft'] as const) } }); +for (const provider of ['google', 'apple', 'microsoft'] as const) + test(`Better Auth OAuth: ${provider} rejects ID tokens whose header names an algorithm other than RS256`, async ({ + onTestFinished, + }) => { + const f = await fixture(); + onTestFinished(() => f.close()); + for (const algorithm of ['HS256', 'none', 'PS256', 'RS512'] as const) { + const browser = await f.browser(); + const { response } = await login(f, browser, provider, { algorithm }); + expect(response.headers.get('location'), algorithm).toContain( + 'error=oauth_failed', + ); + expect(await session(browser), algorithm).toBeNull(); + } + // The same token under RS256 still signs in: only the algorithm changed. + const browser = await f.browser(); + await login(f, browser, provider); + expect((await session(browser))?.user.email).toBe('oauth@example.test'); + }); + test('Better Auth OAuth: Apple form_post relay, wrong browser, mismatched provider and expired state', async ({ onTestFinished, }) => { @@ -292,6 +312,34 @@ test('Better Auth OAuth: Apple form_post relay, wrong browser, mismatched provid ); }); +test("Better Auth OAuth: callbacks refuse every method but GET and Apple's form-encoded POST", async ({ + onTestFinished, +}) => { + const f = await fixture(); + onTestFinished(() => f.close()); + const form = { 'content-type': 'application/x-www-form-urlencoded' }; + const json = { 'content-type': 'application/json' }; + for (const [provider, method, headers] of [ + ['google', 'POST', form], + ['github', 'POST', json], + ['facebook', 'POST', form], + ['microsoft', 'POST', form], + ['google', 'PUT', form], + ['apple', 'POST', json], + ['apple', 'POST', { 'content-type': 'text/plain' }], + ['apple', 'PUT', form], + ['apple', 'DELETE', form], + ] as const) { + const response = await f.request(`/api/auth/callback/${provider}`, { + method, + headers, + body: 'code=attacker&state=attacker', + }); + expect(response.status, `${method} ${provider}`).toBe(405); + } + expect(f.destinations).toEqual([]); +}); + test('Better Auth OAuth: email collision requires explicit linking; linking binds to live session', async ({ onTestFinished, }) => { diff --git a/tests/integration/better-auth.test.ts b/tests/integration/better-auth.test.ts index eda12d6..cfc8180 100644 --- a/tests/integration/better-auth.test.ts +++ b/tests/integration/better-auth.test.ts @@ -13,6 +13,8 @@ import { stableJson, } from '../../packages/pgstencil/src/snapshots.ts'; import { schemaChanges } from '../../examples/better-auth/src/schema.ts'; +import { authOptions } from '../../examples/better-auth/src/auth.ts'; +import { connectDatabase } from '../../packages/pgstencil/src/postgres.ts'; import { listen } from '../../examples/better-auth/src/node.ts'; import type { createDeterministicApp } from '../support/better-auth-entry.ts'; @@ -568,6 +570,32 @@ test('IP rate limits: an explicit ipAddressHeaders opt-in counts the configured ).toEqual([200, 200, 200, 429]); }); +test('email policy: fifteen verification attempts per fifteen minutes stop even the right code from any IP', async ({ + onTestFinished, +}) => { + const f = await fixture(); + onTestFinished(() => f.close()); + const email = 'guessed@example.test'; + const send = (ip: string) => + directPost(f, 'email-otp/send-verification-otp', { ...sendOtp, email }, ip); + const verify = (otp: string, ip: string) => + directPost(f, 'sign-in/email-otp', { email, otp }, ip); + expect((await send('203.0.113.1')).status).toBe(200); + await f.email.next(); + for (let i = 0; i < 15; i++) + expect((await verify('00000000', `198.51.100.${i + 1}`)).status).not.toBe( + 429, + ); + f.time.advanceMilliseconds(60_000); + expect((await send('203.0.113.2')).status).toBe(200); + const otp = (await f.email.next()).text.match(/\b\d{8}\b/)![0]; + expect((await verify(otp, '192.0.2.200')).status).toBe(429); + f.time.advanceMilliseconds(15 * 60_000); + expect((await send('203.0.113.3')).status).toBe(200); + const fresh = (await f.email.next()).text.match(/\b\d{8}\b/)![0]; + expect((await verify(fresh, '192.0.2.201')).status).toBe(200); +}); + test('auth surface: explicit CSRF, exact origin, security headers and disabled unused endpoints', async ({ onTestFinished, }) => { @@ -609,9 +637,88 @@ test('auth surface: explicit CSRF, exact origin, security headers and disabled u ); expect(page.headers['cache-control']).toBe('no-store'); expect(page.headers['referrer-policy']).toBe('no-referrer'); + const api = await f.client.get('/api/auth/get-session'); + for (const response of [page, api]) { + expect(response.headers['x-content-type-options']).toBe('nosniff'); + expect(response.headers['x-frame-options']).toBe('DENY'); + } + expect(f.email.all()).toHaveLength(0); +}); + +test('auth surface: an allowlisted write with a non-JSON body answers 415', async ({ + onTestFinished, +}) => { + const f = await fixture(); + onTestFinished(() => f.close()); + for (const [path, type, body] of [ + [ + 'email-otp/send-verification-otp', + 'application/x-www-form-urlencoded', + 'email=alice%40example.test&type=sign-in', + ], + [ + 'sign-in/email-otp', + 'text/plain', + '{"email":"alice@example.test","otp":"00000000"}', + ], + ['sign-in/social', 'multipart/form-data; boundary=x', '--x--'], + ] as const) + await f.client + .post('/api/auth/' + path) + .set('Origin', origin) + .set('Cookie', f.csrfCookie) + .set('X-CSRF-Token', f.csrf) + .set('Content-Type', type) + .send(body) + .expect(415); expect(f.email.all()).toHaveLength(0); }); +test('auth options reject a non-canonical origin, a short secret and off-origin redirect paths', ({ + onTestFinished, +}) => { + // Validation runs before any query; the pool never connects. + const database = connectDatabase('postgres://unused.invalid/none'); + onTestFinished(() => database.destroy()); + const base = { + database, + origin: 'https://app.example.test', + secret: 'a'.repeat(32), + email: { send: async () => {} }, + }; + expect(() => authOptions(base)).not.toThrow(); + for (const origin of [ + 'https://APP.example.test', + 'https://app.example.test/', + 'https://app.example.test:443', + 'https://app.example.test/path', + ]) + expect(() => authOptions({ ...base, origin }), origin).toThrow( + 'canonical origin', + ); + expect(() => authOptions({ ...base, secret: 'a'.repeat(31) })).toThrow( + 'at least 32 characters', + ); + for (const path of [ + '//attacker.test/', + '/\\attacker.test', + '/\t/attacker.test', + 'https://attacker.test/', + 'relative', + ]) { + expect(() => authOptions({ ...base, successPath: path }), path).toThrow( + 'application origin', + ); + expect(() => authOptions({ ...base, errorPath: path }), path).toThrow( + 'application origin', + ); + } + for (const path of ['/', '/profile?tab=1', '/%2F%2Fattacker.test']) + expect(() => + authOptions({ ...base, successPath: path, errorPath: path }), + ).not.toThrow(); +}); + for (const policy of ['single', 'multiple'] as const) test(`session policy: ${policy}`, async ({ onTestFinished }) => { const f = await fixture('2020-01-01T00:00:00Z', policy); diff --git a/tests/support/oauth-server.ts b/tests/support/oauth-server.ts index bc23b92..a4082bf 100644 --- a/tests/support/oauth-server.ts +++ b/tests/support/oauth-server.ts @@ -1,6 +1,12 @@ import { createServer } from 'node:http'; import { once } from 'node:events'; -import { createHash, createHmac, generateKeyPairSync, sign } from 'node:crypto'; +import { + constants, + createHash, + createHmac, + generateKeyPairSync, + sign, +} from 'node:crypto'; import type { OAuthFetch } from '../../examples/login/src/oauth-providers.ts'; import type { Provider, @@ -71,6 +77,8 @@ export interface GrantOptions { verified?: boolean; claims?: Record; badSignature?: boolean; + /** Forge the ID token under another algorithm; HS256 is keyed with the public key. */ + algorithm?: 'HS256' | 'none' | 'PS256' | 'RS512'; missingIdToken?: boolean; tokenFailure?: boolean; profileFailure?: boolean; @@ -213,13 +221,39 @@ export async function mockOAuthServer( : {}), ...grant.options.claims, }; + const algorithm = grant.options.algorithm ?? 'RS256'; const unsigned = [ Buffer.from( - JSON.stringify({ alg: 'RS256', kid: 'test-key' }), + JSON.stringify({ alg: algorithm, kid: 'test-key' }), ).toString('base64url'), Buffer.from(JSON.stringify(claims)).toString('base64url'), ].join('.'); - response.id_token = `${unsigned}.${sign('RSA-SHA256', Buffer.from(unsigned), grant.options.badSignature ? otherKey().privateKey : key.privateKey).toString('base64url')}`; + const data = Buffer.from(unsigned); + const privateKey = grant.options.badSignature + ? otherKey().privateKey + : key.privateKey; + const signature = + algorithm === 'none' + ? Buffer.alloc(0) + : algorithm === 'HS256' + ? createHmac( + 'sha256', + key.publicKey.export({ type: 'spki', format: 'pem' }), + ) + .update(data) + .digest() + : algorithm === 'PS256' + ? sign('sha256', data, { + key: privateKey, + padding: constants.RSA_PKCS1_PSS_PADDING, + saltLength: 32, + }) + : sign( + algorithm === 'RS512' ? 'RSA-SHA512' : 'RSA-SHA256', + data, + privateKey, + ); + response.id_token = `${unsigned}.${signature.toString('base64url')}`; } return json(response); } From b4e71d38b7b621330078172e7b1098f34764c9a2 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Tue, 22 Sep 2026 17:12:50 -0700 Subject: [PATCH 4/5] Contain the migrations path and state three unstated limits (audit Q3, Q4, Q5, Q8) - Q5: paths.ts refuses a pgstencil.json migrations path that resolves outside the project root; a unit test pins it and PACKAGES.md says so. - Q3: the Build and test isolation rule now says better-auth-testing is a published export, imported by packages:verify, and inert unless esbuild injects it; the rule fails on an injection in a normal build or a global changed by import, pinned by the production-bundle and unchanged-host-clock tests. - Q4, Q8: "What is not defended" names Facebook's unverified email and two applications sharing both a database and AUTH_SECRET. Co-Authored-By: Claude Opus 5.5 (1M context) --- PACKAGES.md | 2 +- SECURITY.md | 7 ++++--- packages/pgstencil/src/paths.ts | 14 ++++++++++++-- tests/unit/paths.test.ts | 27 +++++++++++++++++++++++++++ 4 files changed, 44 insertions(+), 6 deletions(-) create mode 100644 tests/unit/paths.test.ts diff --git a/PACKAGES.md b/PACKAGES.md index 49a97b8..20709fb 100644 --- a/PACKAGES.md +++ b/PACKAGES.md @@ -20,7 +20,7 @@ Every other dependency is private: pgstencil owns its version and applications d Production imports use `pgstencil`, `pgstencil/postgres`, `@pgstencil/auth` and `@pgstencil/stripe`. Docker startup and test fixtures are opt-in imports through `pgstencil/database`, `pgstencil/testing`, and `@pgstencil/stripe/testing`. -The database tooling uses the current working directory as the project root, or `PGSTENCIL_PROJECT_ROOT` when explicitly set. State goes in that project's `.pgstencil`; its Docker Compose project name derives from the project path. A project can supply `compose.yaml`, otherwise the package's bundled default is used. The default SQL directory is `migrations`, overridable by a `pgstencil.json` file containing a `migrations` directory path. Applications composing packages should pass their complete source list explicitly: +The database tooling uses the current working directory as the project root, or `PGSTENCIL_PROJECT_ROOT` when explicitly set. State goes in that project's `.pgstencil`; its Docker Compose project name derives from the project path. A project can supply `compose.yaml`, otherwise the package's bundled default is used. The default SQL directory is `migrations`, overridable by a `pgstencil.json` file containing a `migrations` directory path inside the project root; a path resolving outside it is refused. Applications composing packages should pass their complete source list explicitly: ```ts import { authMigrations } from '@pgstencil/auth/migrations'; diff --git a/SECURITY.md b/SECURITY.md index 4a77e45..4f5af3b 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -61,10 +61,10 @@ Pinned by `unit/diagnostics.test.ts`: `diagnostics allowlist drops secrets even ## Build and test isolation -- **FAIL IF** a normal build reaches `@pgstencil/auth/better-auth-testing`, or a deployed Worker answers a test clock route; inspect `examples/better-auth/src/worker.ts` beside `support/better-auth-worker.ts`, and the esbuild `inject` that only test bundles carry. +- **FAIL IF** a normal build injects `@pgstencil/auth/better-auth-testing`, importing that module changes a process global, or a deployed Worker answers a test clock route. The module is a published export that `packages:verify` imports; it is inert unless esbuild's `inject` names it, which only test bundles do. Inspect `better-auth-testing.ts`, `examples/better-auth/src/worker.ts` beside `support/better-auth-worker.ts`, and each bundle's `inject`. - **FAIL IF** Better Auth introspects or migrates the schema during a request, or the committed migrations stop matching Better Auth's generated plan; inspect `advanced.database` in `better-auth.ts` and `schemaChanges` in `examples/better-auth/src/schema.ts`. -Pinned by `integration/better-auth-workers.test.ts`: `normal Workers build uses real time and randomness and contains no test clock controls`; `integration/better-auth.test.ts`: `Better Auth email: repeatable cookies, database and email snapshots across parallel apps`. +Pinned by `integration/better-auth-workers.test.ts`: `normal Workers build uses real time and randomness and contains no test clock controls`; `integration/better-auth.test.ts`: `Better Auth email: repeatable cookies, database and email snapshots across parallel apps`, `Better Auth time: 23 hours, expiration boundary, isolated async contexts and unchanged host clock`. ## Packed provenance @@ -95,6 +95,7 @@ Report privately through GitHub's [advisory form for diffplug/pgstencil](https:/ - The consumer-owned controls listed above. - An attacker holding both the database contents and `AUTH_SECRET`. Better Auth stores native session tokens, so that pair mints a session cookie; either alone does not. - Better Auth behavior beyond what these tests pin; its private range admits patches only, and an upgrade must rerun the security and snapshot suites. -- A provider that asserts an email it did not verify, and account recovery after a lost provider account or mailbox. +- A provider that asserts an email it did not verify, and account recovery after a lost provider account or mailbox. Facebook sends no verification claim, so pgstencil treats any address Facebook returns as verified. +- Two applications sharing both a database and `AUTH_SECRET`: they share OAuth state, sessions and limits, and count as one application here. - Multi-factor authentication and passkeys, and abuse beyond the budgets above. - `@pgstencil/stripe`, the original code/link `Auth` exports, and the example applications; see [BILLING.md](BILLING.md), [OAUTH.md](OAUTH.md) and [LOGIN_FLOW.md](LOGIN_FLOW.md). diff --git a/packages/pgstencil/src/paths.ts b/packages/pgstencil/src/paths.ts index 3896f8d..80b08ea 100644 --- a/packages/pgstencil/src/paths.ts +++ b/packages/pgstencil/src/paths.ts @@ -1,5 +1,5 @@ import { createHash } from 'node:crypto'; -import { resolve, join } from 'node:path'; +import { resolve, join, relative, isAbsolute, sep } from 'node:path'; import { existsSync, readFileSync } from 'node:fs'; import { fileURLToPath } from 'node:url'; /** Workspace paths, kept free of heavy imports so light scripts can use them. */ @@ -10,7 +10,17 @@ const configFile = join(projectRoot, 'pgstencil.json'); const config = existsSync(configFile) ? (JSON.parse(readFileSync(configFile, 'utf8')) as { migrations?: string }) : {}; -export const defaultMigrations = resolve( +/** Resolve a configured migrations directory, refusing one outside the project root. */ +export function migrationsDirectory(root: string, configured: string) { + const directory = resolve(root, configured); + const path = relative(root, directory); + if (path === '..' || path.startsWith('..' + sep) || isAbsolute(path)) + throw new Error( + `pgstencil.json migrations must stay inside the project root: ${configured}`, + ); + return directory; +} +export const defaultMigrations = migrationsDirectory( projectRoot, config.migrations ?? 'migrations', ); diff --git a/tests/unit/paths.test.ts b/tests/unit/paths.test.ts new file mode 100644 index 0000000..46e8a20 --- /dev/null +++ b/tests/unit/paths.test.ts @@ -0,0 +1,27 @@ +import { test, expect } from 'vitest'; +import { join, resolve } from 'node:path'; +import { migrationsDirectory } from '../../packages/pgstencil/src/paths.ts'; + +test('pgstencil.json migrations path must stay inside the project root', () => { + const root = resolve('/work/app'); + expect(migrationsDirectory(root, 'migrations')).toBe( + join(root, 'migrations'), + ); + expect(migrationsDirectory(root, 'packages/auth/migrations')).toBe( + join(root, 'packages/auth/migrations'), + ); + expect(migrationsDirectory(root, './db/../sql')).toBe(join(root, 'sql')); + expect(migrationsDirectory(root, '..migrations')).toBe( + join(root, '..migrations'), + ); + for (const escape of [ + '..', + '../elsewhere', + '../../etc', + 'migrations/../../app-sibling', + resolve('/tmp/migrations'), + ]) + expect(() => migrationsDirectory(root, escape), escape).toThrow( + 'inside the project root', + ); +}); From 72f75e829c3c95995fb23e8efd2bffae4d8f5a77 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Tue, 22 Sep 2026 17:15:31 -0700 Subject: [PATCH 5/5] Group IPv6 by /64 in pgstencil's send:ip budget, as Better Auth does The per-IP email budget hashed the full address, so a client rotating within its IPv6 /64 got fresh budgets while Better Auth's limiter held. It now keys on ipBucket, which reproduces Better Auth's normalizeIP(ip, { ipv6Subnet: 64 }). Better Auth does not re-export that helper, and depending on @better-auth/core directly drags in its exactly pinned peers, which failed packages:verify's fresh install; a unit test instead compares ipBucket with the normalizeIP Better Auth's limiter runs. An integration test shows two addresses in one /64 share one budget, and the trusted-IP rule names the /64 grouping. Co-Authored-By: Claude Opus 5.5 (1M context) --- SECURITY.md | 4 +-- packages/auth/src/better-auth-security.ts | 32 ++++++++++++++++++++- tests/integration/better-auth.test.ts | 35 +++++++++++++++++++++++ tests/unit/ip-bucket.test.ts | 35 +++++++++++++++++++++++ 4 files changed, 103 insertions(+), 3 deletions(-) create mode 100644 tests/unit/ip-bucket.test.ts diff --git a/SECURITY.md b/SECURITY.md index 4f5af3b..fcc104c 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -27,10 +27,10 @@ Pinned by `integration/better-auth.test.ts`: `auth surface: explicit CSRF, exact - **FAIL IF** a stored email code is recoverable without the application secret, is not separated by purpose from other keyed values, lives longer than ten minutes, survives more than three failed guesses, or is redeemed twice; inspect `emailOTP` in `better-auth.ts` and `keyed` in `better-auth-security.ts`. - **FAIL IF** a stored rate-limit key — Better Auth's `rateLimit` rows or pgstencil's `pgstencil_auth_limits` — contains a raw email address or IP instead of a secret-keyed HMAC, or a changing client IP resets an address's one-minute resend cooldown, five sends or fifteen verification attempts per fifteen minutes; inspect `rateLimitStorage` and `consume` in `better-auth-security.ts`, `rateLimit` in `better-auth.ts` and `005_hashed_rate_limit_keys.sql`. -- **FAIL IF** Better Auth's per-IP limiter or pgstencil's per-IP email budget counts any address but one trusted client IP — on Node the socket address `@hono/node-server` passes as `env.incoming`, or a forwarded header only when the application names it in `ipAddressHeaders` — or a client-supplied `x-pgstencil-client-ip` reaches either limiter; inspect `protectAuth` in `better-auth-security.ts` and `advanced.ipAddress` in `better-auth.ts`. +- **FAIL IF** Better Auth's per-IP limiter or pgstencil's per-IP email budget counts any address but one trusted client IP — on Node the socket address `@hono/node-server` passes as `env.incoming`, or a forwarded header only when the application names it in `ipAddressHeaders` — or either limiter keys an IPv6 address on anything finer than its /64, or a client-supplied `x-pgstencil-client-ip` reaches either limiter; inspect `protectAuth` and `ipBucket` in `better-auth-security.ts` and `advanced.ipAddress` in `better-auth.ts`. - **FAIL IF** the Workers adapter counts a client-IP header other than `cf-connecting-ip`, or an address in the reserved `identity.pgstencil.invalid` namespace reaches an email route or the email sender; inspect `better-auth-workers.ts` and `isIdentityEmail`. -Pinned by `integration/better-auth.test.ts`: `email policy: secret-keyed codes, cross-browser redemption, concurrent single use and no token exposure`, `email policy: distributed IPs cannot bypass cooldown, send quota or attempt budget`, `email policy: fifteen verification attempts per fifteen minutes stop even the right code from any IP`, `IP rate limits: stored limiter keys never contain a raw client IP`, `IP rate limits: the default Node path counts the socket, so rotating x-pgstencil-client-ip cannot escape`, `IP rate limits: concurrent requests from one address are counted atomically`, `IP rate limits: naming a victim's IP in x-pgstencil-client-ip spends only the caller's budget`, `IP rate limits: an explicit ipAddressHeaders opt-in counts the configured header`; `integration/better-auth-workers.test.ts`: `Better Auth in workerd: deterministic replay, separate clocks, shared database rate limits`; `integration/better-auth-oauth.test.ts`: `Optional email: signs in by stable identity without a mailbox`. +Pinned by `integration/better-auth.test.ts`: `email policy: secret-keyed codes, cross-browser redemption, concurrent single use and no token exposure`, `email policy: distributed IPs cannot bypass cooldown, send quota or attempt budget`, `email policy: fifteen verification attempts per fifteen minutes stop even the right code from any IP`, `IP rate limits: stored limiter keys never contain a raw client IP`, `IP rate limits: the default Node path counts the socket, so rotating x-pgstencil-client-ip cannot escape`, `IP rate limits: concurrent requests from one address are counted atomically`, `IP rate limits: two IPv6 addresses in one /64 share one send:ip budget`, `IP rate limits: naming a victim's IP in x-pgstencil-client-ip spends only the caller's budget`, `IP rate limits: an explicit ipAddressHeaders opt-in counts the configured header`; `unit/ip-bucket.test.ts`: `ipBucket groups addresses by /64 as Better Auth's own normalizeIP does`; `integration/better-auth-workers.test.ts`: `Better Auth in workerd: deterministic replay, separate clocks, shared database rate limits`; `integration/better-auth-oauth.test.ts`: `Optional email: signs in by stable identity without a mailbox`. ## OAuth diff --git a/packages/auth/src/better-auth-security.ts b/packages/auth/src/better-auth-security.ts index c083cd2..e379502 100644 --- a/packages/auth/src/better-auth-security.ts +++ b/packages/auth/src/better-auth-security.ts @@ -80,6 +80,36 @@ export function rateLimitStorage( }; } +/** + * Better Auth's `normalizeIP(ip, { ipv6Subnet: 64 })`, which it does not + * re-export: IPv6 collapses to its /64 and IPv4-mapped IPv6 to IPv4, so one + * host cannot rotate addresses within its /64 for fresh budgets. A unit test + * compares the two. + */ +export function ipBucket(ip: string) { + if (!ip.includes(':')) return ip.toLowerCase(); + let host: string; + try { + host = new URL(`http://[${ip}]`).hostname.slice(1, -1); + } catch { + return ip.toLowerCase(); + } + const [left = '', right = ''] = host.split('::'); + const head = left ? left.split(':') : []; + const tail = host.includes('::') && right ? right.split(':') : []; + const groups = [ + ...head, + ...Array(8 - head.length - tail.length).fill('0'), + ...tail, + ].map((group) => group.padStart(4, '0')); + if (groups.slice(0, 5).every((g) => g === '0000') && groups[5] === 'ffff') + return groups + .slice(6) + .flatMap((g) => [parseInt(g.slice(0, 2), 16), parseInt(g.slice(2), 16)]) + .join('.'); + return [...groups.slice(0, 4), '0000', '0000', '0000', '0000'].join(':'); +} + /** Shared, atomic limits: changing client IP cannot reset an email's budget. */ async function consume( db: ReturnType, @@ -230,7 +260,7 @@ export function protectAuth( const send = path === '/email-otp/send-verification-otp'; if (send && body.type !== 'sign-in') return c.json({ message: 'Unsupported email operation' }, 400); - const ip = c.req.header(clientIpHeader) ?? 'unknown'; + const ip = ipBucket(c.req.header(clientIpHeader) ?? 'unknown'); // Bound per-email counter creation even after upstream IP limits reject. if ( !(await consume( diff --git a/tests/integration/better-auth.test.ts b/tests/integration/better-auth.test.ts index cfc8180..7a260a6 100644 --- a/tests/integration/better-auth.test.ts +++ b/tests/integration/better-auth.test.ts @@ -506,6 +506,41 @@ test('IP rate limits: concurrent requests from one address are counted atomicall expect(burst.filter((r) => r.status === 429)).toHaveLength(5); }); +test('IP rate limits: two IPv6 addresses in one /64 share one send:ip budget', async ({ + onTestFinished, +}) => { + const f = await fixture(); + onTestFinished(() => f.close()); + const neighbours = ['2001:db8:1:2::a', '2001:db8:1:2:ffff:ffff:ffff:b']; + const send = (ip: string, i: number) => + directPost( + f, + 'email-otp/send-verification-otp', + { ...sendOtp, email: `v6-${i}@example.test` }, + ip, + ); + // Better Auth's own /64 limit allows three a minute; stay under it. + for (let i = 0; i < 30; i++) { + if (i && i % 3 === 0) f.time.advanceMilliseconds(60_000); + expect((await send(neighbours[i % 2]!, i)).status).toBe(200); + } + f.time.advanceMilliseconds(60_000); + expect((await send(neighbours[1]!, 30)).status).toBe(429); + expect((await send('2001:db8:1:3::a', 31)).status).toBe(200); + const own = await queryDatabase<{ key: string; count: number }>( + f.database.url, + "SELECT key, count FROM pgstencil_auth_limits WHERE key LIKE 'send:ip:%'", + ); + expect(own).toEqual( + expect.arrayContaining([ + { + key: `send:ip:${await hmac('ip-rate', '2001:0db8:0001:0002:0000:0000:0000:0000')}`, + count: 30, + }, + ]), + ); +}); + test("IP rate limits: naming a victim's IP in x-pgstencil-client-ip spends only the caller's budget", async ({ onTestFinished, }) => { diff --git a/tests/unit/ip-bucket.test.ts b/tests/unit/ip-bucket.test.ts new file mode 100644 index 0000000..e9fe9f1 --- /dev/null +++ b/tests/unit/ip-bucket.test.ts @@ -0,0 +1,35 @@ +import { test, expect } from 'vitest'; +import { realpathSync } from 'node:fs'; +import { resolve } from 'node:path'; +import { pathToFileURL } from 'node:url'; +import { ipBucket } from '../../packages/auth/src/better-auth-security.ts'; + +test("ipBucket groups addresses by /64 as Better Auth's own normalizeIP does", async () => { + // Better Auth's copy of @better-auth/core, the one its limiter runs. + const betterAuth = realpathSync( + resolve('packages/auth/node_modules/better-auth'), + ); + const { normalizeIP } = (await import( + pathToFileURL(resolve(betterAuth, '../@better-auth/core/dist/utils/ip.mjs')) + .href + )) as { + normalizeIP: (ip: string, options: { ipv6Subnet: number }) => string; + }; + for (const ip of [ + '192.0.2.1', + '203.0.113.255', + '2001:db8::1', + '2001:DB8:1:2:ffff:ffff:ffff:b', + '2001:db8:1:2::a', + 'fe80::1', + '::1', + '::', + '::ffff:192.0.2.1', + '::ffff:c000:201', + '0:0:0:0:0:ffff:198.51.100.7', + '2001:0db8:0000:0000:0000:0000:0000:0001', + '1:2:3:4:5:6:7:8', + 'unknown', + ]) + expect(ipBucket(ip), ip).toBe(normalizeIP(ip, { ipv6Subnet: 64 })); +});