From 6c07be0987183d949892cd97f43ba692cbfd99d0 Mon Sep 17 00:00:00 2001 From: Gauransh Mathur Date: Fri, 16 Oct 2026 10:19:51 +0000 Subject: [PATCH 1/7] feat(beyondleetcode): C32 sign-in action, throttle give-back, unknown pages private, HTTP sign-in off Co-Authored-By: Claude Sonnet 5.5 --- src/lib/server/identity/auth-handler.test.ts | 35 +- src/lib/server/identity/auth.test.ts | 55 +- src/lib/server/identity/auth.ts | 14 +- src/lib/server/identity/gate.test.ts | 93 ++- src/lib/server/identity/gate.ts | 29 +- src/lib/server/identity/throttle.test.ts | 59 +- src/lib/server/identity/throttle.ts | 24 +- src/routes/signin/sign-in.test.ts | 645 +++++++++++++++++++ src/routes/signin/sign-in.ts | 140 ++++ 9 files changed, 1051 insertions(+), 43 deletions(-) create mode 100644 src/routes/signin/sign-in.test.ts create mode 100644 src/routes/signin/sign-in.ts diff --git a/src/lib/server/identity/auth-handler.test.ts b/src/lib/server/identity/auth-handler.test.ts index 6fd9e158..a33696ea 100644 --- a/src/lib/server/identity/auth-handler.test.ts +++ b/src/lib/server/identity/auth-handler.test.ts @@ -11,22 +11,25 @@ async function setup() { return { db, auth: createAuth({ db, origin, secret }) }; } -const signIn = (headers: Record = {}) => - new Request(`${origin}/api/auth/sign-in/email`, { +// Password sign-in is not served over HTTP (the Sign in form is the one way, see auth.ts), so these tests +// use change-password: Better Auth gives it the same 3-per-10-seconds rule it gives the sign-in paths, +// it answers 401 to a visitor without a session, and a 429 is the limiter alone. +const limited = (headers: Record = {}) => + new Request(`${origin}/api/auth/change-password`, { method: 'POST', headers: { 'content-type': 'application/json', origin, ...headers }, - body: JSON.stringify({ email: 'nobody@example.com', password: 'not the password' }) + body: JSON.stringify({ currentPassword: 'not the password', newPassword: 'not a new password' }) }); describe('handleAuthRequest', () => { - it('limits sign-in attempts per client address, whatever X-Forwarded-For claims', async () => { + it('limits attempts per client address, whatever X-Forwarded-For claims', async () => { const { auth } = await setup(); const statuses = []; for (let i = 0; i < 5; i++) { // A client rotating a forged header must still share one bucket. const res = await handleAuthRequest( auth, - signIn({ 'x-forwarded-for': `203.0.113.${i + 1}` }), + limited({ 'x-forwarded-for': `203.0.113.${i + 1}` }), '192.0.2.10' ); statuses.push(res.status); @@ -38,10 +41,10 @@ describe('handleAuthRequest', () => { it('gives each client address its own bucket', async () => { const { auth } = await setup(); - for (let i = 0; i < 4; i++) await handleAuthRequest(auth, signIn(), '192.0.2.20'); + for (let i = 0; i < 4; i++) await handleAuthRequest(auth, limited(), '192.0.2.20'); - const other = await handleAuthRequest(auth, signIn(), '192.0.2.21'); - const same = await handleAuthRequest(auth, signIn(), '192.0.2.20'); + const other = await handleAuthRequest(auth, limited(), '192.0.2.21'); + const same = await handleAuthRequest(auth, limited(), '192.0.2.20'); expect(other.status).not.toBe(429); expect(same.status).toBe(429); @@ -50,11 +53,11 @@ describe('handleAuthRequest', () => { it('groups IPv6 clients by /64: rotating addresses inside one /64 does not escape the limit', async () => { const { auth } = await setup(); for (let i = 1; i <= 4; i++) { - await handleAuthRequest(auth, signIn(), `fd00:aa:bb:cc::${i}`); + await handleAuthRequest(auth, limited(), `fd00:aa:bb:cc::${i}`); } - const sameSubnet = await handleAuthRequest(auth, signIn(), 'fd00:aa:bb:cc:1:2:3:4'); - const otherSubnet = await handleAuthRequest(auth, signIn(), 'fd00:aa:bb:dd::1'); + const sameSubnet = await handleAuthRequest(auth, limited(), 'fd00:aa:bb:cc:1:2:3:4'); + const otherSubnet = await handleAuthRequest(auth, limited(), 'fd00:aa:bb:dd::1'); expect(sameSubnet.status).toBe(429); expect(otherSubnet.status).not.toBe(429); @@ -62,9 +65,9 @@ describe('handleAuthRequest', () => { it('treats an IPv4-mapped IPv6 address as its IPv4 client', async () => { const { auth } = await setup(); - for (let i = 0; i < 4; i++) await handleAuthRequest(auth, signIn(), '::ffff:192.0.2.77'); + for (let i = 0; i < 4; i++) await handleAuthRequest(auth, limited(), '::ffff:192.0.2.77'); - const mapped = await handleAuthRequest(auth, signIn(), '192.0.2.77'); + const mapped = await handleAuthRequest(auth, limited(), '192.0.2.77'); expect(mapped.status).toBe(429); }); @@ -75,14 +78,14 @@ describe('handleAuthRequest', () => { // Each attempt claims a fresh address in the header the limiter reads. await handleAuthRequest( auth, - signIn({ 'x-beyondleetcode-client-ip': `198.51.100.${i + 1}` }), + limited({ 'x-beyondleetcode-client-ip': `198.51.100.${i + 1}` }), '192.0.2.30' ); } const res = await handleAuthRequest( auth, - signIn({ 'x-beyondleetcode-client-ip': '198.51.100.99' }), + limited({ 'x-beyondleetcode-client-ip': '198.51.100.99' }), '192.0.2.30' ); expect(res.status).toBe(429); @@ -116,7 +119,7 @@ describe('handleAuthRequest', () => { it('answers with headers the caller can still add to', async () => { const { auth } = await setup(); - const res = await handleAuthRequest(auth, signIn(), '192.0.2.40'); + const res = await handleAuthRequest(auth, limited(), '192.0.2.40'); res.headers.set('X-Frame-Options', 'DENY'); expect(res.headers.get('x-frame-options')).toBe('DENY'); diff --git a/src/lib/server/identity/auth.test.ts b/src/lib/server/identity/auth.test.ts index 1c8b5ceb..752fd4a9 100644 --- a/src/lib/server/identity/auth.test.ts +++ b/src/lib/server/identity/auth.test.ts @@ -200,7 +200,6 @@ describe('createAuth', () => { expect(res.status, path).toBe(404); } for (const [method, path] of [ - ['POST', 'sign-in/email'], ['POST', 'sign-out'], ['GET', 'get-session'], ['POST', 'change-password'] @@ -216,6 +215,47 @@ describe('createAuth', () => { expect(kept.status).not.toBe(404); }); + it('serves no password sign-in over HTTP: the Sign in form, with its per-Account limits, is the one way', async () => { + const { auth } = await setup(); + await auth.api.signUpEmail({ body }); + const signIn = (headers: Record = {}) => + auth.handler( + new Request('http://localhost:3000/api/auth/sign-in/email', { + method: 'POST', + headers: { + 'content-type': 'application/json', + origin: 'http://localhost:3000', + ...headers + }, + body: JSON.stringify({ email: body.email, password: body.password }) + }) + ); + + // The right password is a 404 too, and no session comes out of it. + const res = await signIn(); + expect(res.status).toBe(404); + expect(res.headers.getSetCookie()).toEqual([]); + // The router refuses it before anything else, so even a form-style post cannot reach it. + expect((await signIn({ 'content-type': 'application/x-www-form-urlencoded' })).status).toBe( + 404 + ); + expect( + ( + await auth.handler( + new Request('http://localhost:3000/api/auth/sign-in/email', { method: 'GET' }) + ) + ).status + ).toBe(404); + + // The server-side call the form action makes is not an HTTP path, so it is not switched off. + const { headers, response } = await auth.api.signInEmail({ + body: { email: body.email, password: body.password }, + returnHeaders: true + }); + expect(response.user.email).toBe(body.email); + expect(headers.getSetCookie()[0]).toMatch(/^better-auth\.session_token=/); + }); + it('still refuses a cross-origin POST when the environment looks like a test', async () => { // Better Auth 1.7.7 turns its origin and CSRF checks off when NODE_ENV is "test" or TEST is set, // production included, unless the config says otherwise. This run has NODE_ENV=test as well. @@ -229,12 +269,14 @@ describe('createAuth', () => { headers: { cookie: 'better-auth.session_token=x', origin } }) ); - const signIn = (headers: Record) => + // Sign-up still takes a form post without a cookie, so it carries the cookie-less checks. This + // setup admits everyone, so a 403 here is the origin check and not a closed sign-up. + const signUp = (headers: Record) => auth.handler( - new Request('http://localhost:3000/api/auth/sign-in/email', { + new Request('http://localhost:3000/api/auth/sign-up/email', { method: 'POST', headers: { 'content-type': 'application/json', ...headers }, - body: JSON.stringify({ email: body.email, password: body.password }) + body: JSON.stringify(body) }) ); @@ -242,9 +284,10 @@ describe('createAuth', () => { expect((await signOut('http://localhost:3000')).status).not.toBe(403); // No cookie: the cross-site navigation check and a foreign Origin are refused too. expect( - (await signIn({ 'sec-fetch-site': 'cross-site', 'sec-fetch-mode': 'navigate' })).status + (await signUp({ 'sec-fetch-site': 'cross-site', 'sec-fetch-mode': 'navigate' })).status ).toBe(403); - expect((await signIn({ origin: 'https://evil.example' })).status).toBe(403); + expect((await signUp({ origin: 'https://evil.example' })).status).toBe(403); + expect((await signUp({ origin: 'http://localhost:3000' })).status).toBe(200); }); it('refuses the BETTER_AUTH_SECRET that .env.example ships, whatever its wording', async () => { diff --git a/src/lib/server/identity/auth.ts b/src/lib/server/identity/auth.ts index 2c246d39..4d10d401 100644 --- a/src/lib/server/identity/auth.ts +++ b/src/lib/server/identity/auth.ts @@ -94,6 +94,12 @@ const UNUSED_CORE_PATHS = [ '/list-accounts' ]; +// Password sign-in is not served over HTTP at all. Better Auth's limiter keys on the address alone +// and only covers requests through `auth.handler`; the Sign in form action (`routes/signin`) calls +// `auth.api` and carries the per-Account limits (`throttle.ts`), so it is the one way in with a +// password. Server-side `auth.api.signInEmail` calls are not HTTP paths and are unaffected. +const HTTP_SIGN_IN_PATHS = ['/sign-in/email']; + const signUpClosed = () => new APIError('FORBIDDEN', { message: 'Sign-up is closed' }); /** Better Auth over our database: email + password and the admin plugin, nothing else. */ @@ -123,9 +129,9 @@ export function createAuth({ ...(origin ? { useSecureCookies: origin.startsWith('https:') } : {}), // Only the header our own route sets: X-Forwarded-For is client-controlled and never read. // IPv6 clients are grouped by /64 (Better Auth's default, stated here so it cannot change under us). - // Not /128: sign-in over HTTP has no per-Account backstop, and whoever holds a /64 controls all - // of its addresses, so a finer key would let them rotate addresses and guess without limit. The - // cost is that clients in one LAN /64 share a budget, the same trade-off throttle.ts makes. + // Not /128: whoever holds a /64 controls all of its addresses, so a finer key would let them + // rotate addresses without limit (sign-up and the other endpoints have no per-Account backstop). + // The cost is that clients in one LAN /64 share a budget, the same trade-off throttle.ts makes. ipAddress: { ipAddressHeaders: [CLIENT_IP_HEADER], ipv6Subnet: 64 } }, // On explicitly (Better Auth defaults it to production only), in memory (one process owns the @@ -201,7 +207,7 @@ export function createAuth({ if (!(await admitSignUp({ email: ctx.body?.email }))) throw signUpClosed(); }) }, - disabledPaths: [...UNUSED_ADMIN_PATHS, ...UNUSED_CORE_PATHS], + disabledPaths: [...UNUSED_ADMIN_PATHS, ...UNUSED_CORE_PATHS, ...HTTP_SIGN_IN_PATHS], plugins: [admin({ defaultRole: 'learner' }), ...extraPlugins] }); } diff --git a/src/lib/server/identity/gate.test.ts b/src/lib/server/identity/gate.test.ts index 9215d98a..8235e8da 100644 --- a/src/lib/server/identity/gate.test.ts +++ b/src/lib/server/identity/gate.test.ts @@ -75,6 +75,11 @@ describe('gate', () => { status(404) ], ['local, home -> pass', request('/', { mode: 'local', who: admin }), pass], + [ + 'local, unknown page -> pass (the 404 page)', + request(null, { mode: 'local', who: admin }), + pass + ], [ 'local, set up lookalike /api/authx -> pass', request('/api/authx', { mode: 'local', who: admin }), @@ -105,7 +110,17 @@ describe('gate', () => { request('/account/password', { pathname: '/account/password' }), redirect('/signin?next=%2Faccount%2Fpassword') ], - ['multi nobody, 404 page -> pass', request(null), pass], + // A path with no route is a private page: nobody learns from the 404 page which paths exist. + [ + 'multi nobody, unknown page -> signin with next', + request(null, { pathname: '/no/such/page', search: '?a=1' }), + redirect('/signin?next=%2Fno%2Fsuch%2Fpage%3Fa%3D1') + ], + [ + 'multi nobody, unknown page that merely looks like a public one -> signin', + request(null, { pathname: '/signin/extra' }), + redirect('/signin?next=%2Fsignin%2Fextra') + ], // 5. must change password [ 'must change, home -> password', @@ -143,10 +158,64 @@ describe('gate', () => { redirect('/account/password') ], [ - 'must change, 404 page -> pass', + 'must change, unknown page -> password', request(null, { who: learner, mustChangePassword: true }), + redirect('/account/password') + ], + [ + 'must change, POST Settings sign-out action -> pass', + request('/settings', { + who: learner, + mustChangePassword: true, + method: 'POST', + pathname: '/settings', + search: '?/signout' + }), pass ], + [ + 'must change, GET Settings sign-out -> password', + request('/settings', { + who: learner, + mustChangePassword: true, + pathname: '/settings', + search: '?/signout' + }), + redirect('/account/password') + ], + [ + 'must change, POST Settings reset -> password', + request('/settings', { + who: learner, + mustChangePassword: true, + method: 'POST', + pathname: '/settings', + search: '?/reset' + }), + redirect('/account/password') + ], + [ + 'must change, POST Settings with the reset first and sign-out second -> password (SvelteKit runs the first)', + request('/settings', { + who: learner, + mustChangePassword: true, + method: 'POST', + pathname: '/settings', + search: '?/reset&/signout' + }), + redirect('/account/password') + ], + [ + 'must change, POST another page with a sign-out action -> password', + request('/', { + who: learner, + mustChangePassword: true, + method: 'POST', + pathname: '/', + search: '?/signout' + }), + redirect('/account/password') + ], ['signed in without the flag, home -> pass', request('/', { who: learner }), pass], [ 'local ignores the flag', @@ -159,6 +228,8 @@ describe('gate', () => { ['admin, /users -> pass', request('/users', { who: admin }), pass], ['learner, /usersX lookalike -> pass', request('/usersX', { who: learner }), pass], // 7. otherwise + ['multi learner, unknown page -> pass (the 404 page)', request(null, { who: learner }), pass], + ['multi admin, unknown page -> pass (the 404 page)', request(null, { who: admin }), pass], ['multi learner, home -> pass', request('/', { who: learner }), pass], ['multi learner, /signin -> pass', request('/signin', { who: learner }), pass], ['multi admin, /account/password -> pass', request('/account/password', { who: admin }), pass] @@ -196,7 +267,23 @@ describe('safeNext', () => { ['/\t/evil.com', '/'], ['/\u007f', '/'], [' /ok', '/'], - ['/ok ', '/ok '] + ['/ok ', '/ok '], + ['/\\/evil.com', '/'], + ['/\\\\evil.com', '/'], + ['\\\\/evil.com', '/'], + ['\t//evil.com', '/'], + ['/\r/evil.com', '/'], + ['/\n/evil.com', '/'], + ['/\u0001', '/'], + ['/%2F/evil.com', '/%2F/evil.com'], + ['/%5Cevil.com', '/%5Cevil.com'], + ['/https://evil.com', '/https://evil.com'], + ['/@evil.com', '/@evil.com'], + ['HTTPS://evil.com', '/'], + ['mailto:a@b.c', '/'], + ['file:///etc/passwd', '/'], + ['//', '/'], + ['/?next=//evil.com', '/?next=//evil.com'] ]; it.each(cases)('%j -> %j', (input, expected) => { diff --git a/src/lib/server/identity/gate.ts b/src/lib/server/identity/gate.ts index 5441f897..c6a9d40c 100644 --- a/src/lib/server/identity/gate.ts +++ b/src/lib/server/identity/gate.ts @@ -36,35 +36,50 @@ const isPublic = (routeId: string) => const isMultiOnly = (routeId: string) => isPublic(routeId) || isUnder(routeId, '/users') || routeId === '/account/password'; +/** The form action a POST runs, read the way SvelteKit reads it: the first query key that starts with '/'. */ +const formAction = (search: string): string | null => { + for (const key of new URLSearchParams(search).keys()) if (key.startsWith('/')) return key; + return null; +}; + +/** The two ways to sign out: Better Auth's own endpoint, and the Settings page's `signout` form action. */ +const isSigningOut = ({ routeId, method, pathname, search }: GateInput) => + method === 'POST' && + (pathname === '/api/auth/sign-out' || + (routeId === '/settings' && formAction(search) === '/signout')); + /** * Decides what a request may do, first matching rule wins. Pure: the caller skips static assets * and SvelteKit internals (`/_app/`, favicon) and acts on the result. + * + * A path with no route (`routeId` null) is a private page in Multi-user Mode, so the 404 page does + * not tell a signed-out visitor which paths exist; in Local Mode it is the 404 page for everyone. */ export function gate(input: GateInput): GateResult { - const { mode, who, routeId, method, pathname, search } = input; + const { mode, who, routeId, pathname, search } = input; if (mode === null) return routeId === '/setup' ? pass : redirect('/setup'); - if (routeId === null) return pass; if (routeId === '/setup') { const converting = mode === 'local' && new URLSearchParams(search).has('convert'); return converting ? pass : redirect('/'); } - if (mode === 'local') return isMultiOnly(routeId) ? status(404) : pass; + if (mode === 'local') return routeId !== null && isMultiOnly(routeId) ? status(404) : pass; if (who === null) { - return isPublic(routeId) + return routeId !== null && isPublic(routeId) ? pass : redirect(`/signin?next=${encodeURIComponent(pathname + search)}`); } if (input.mustChangePassword) { - const signingOut = method === 'POST' && pathname === '/api/auth/sign-out'; - if (routeId !== '/account/password' && !signingOut) return redirect('/account/password'); + if (routeId !== '/account/password' && !isSigningOut(input)) { + return redirect('/account/password'); + } } - if (isUnder(routeId, '/users') && who.role !== 'admin') return status(403); + if (routeId !== null && isUnder(routeId, '/users') && who.role !== 'admin') return status(403); return pass; } diff --git a/src/lib/server/identity/throttle.test.ts b/src/lib/server/identity/throttle.test.ts index cd777325..003ea365 100644 --- a/src/lib/server/identity/throttle.test.ts +++ b/src/lib/server/identity/throttle.test.ts @@ -117,20 +117,69 @@ describe('throttle', () => { expect(throttle.attempt(from('1.1.1.1')).allowed).toBe(false); }); - it('succeed does not reset the address budget', () => { + it('succeed gives back exactly one count to the address, so right passwords never use up its budget', () => { const { throttle } = setup({ addressLimit: 6 }); for (let i = 0; i < 5; i++) throttle.attempt(from('1.1.1.1', 'mine')); throttle.succeed(from('1.1.1.1', 'mine')); + // 5 counted, one taken back: two more fit under 6, the third does not. + expect(throttle.attempt(from('1.1.1.1', 'mine')).allowed).toBe(true); expect(throttle.attempt(from('1.1.1.1', 'mine')).allowed).toBe(true); expect(throttle.attempt(from('1.1.1.1', 'mine')).allowed).toBe(false); }); - it('succeed does not reset the account budget', () => { + it('succeed gives back exactly one count to the Account, however many addresses it signs in from', () => { const { throttle } = setup({ accountLimit: 6 }); - for (let i = 0; i < 5; i++) throttle.attempt(from('1.1.1.1', 'mine')); + for (let i = 0; i < 5; i++) throttle.attempt(from(`1.1.1.${i}`, 'mine')); + throttle.succeed(from('1.1.1.0', 'mine')); + expect(throttle.attempt(from('2.2.2.1', 'mine')).allowed).toBe(true); + expect(throttle.attempt(from('2.2.2.2', 'mine')).allowed).toBe(true); + expect(throttle.attempt(from('2.2.2.3', 'mine')).allowed).toBe(false); + }); + + it('a class behind one address can all sign in: a right password is attempt + succeed, net zero', () => { + const { throttle } = setup(); + for (let student = 0; student < 100; student++) { + expect(throttle.attempt(from('203.0.113.7', `student-${student}`)).allowed).toBe(true); + throttle.succeed(from('203.0.113.7', `student-${student}`)); + } + expect(throttle.attempt(from('203.0.113.7', 'student-0')).allowed).toBe(true); + }); + + it('the same Account signing in over and over never runs out either', () => { + const { throttle } = setup(); + for (let i = 0; i < 200; i++) { + expect(throttle.attempt(from('1.1.1.1', 'mine')).allowed).toBe(true); + throttle.succeed(from('1.1.1.1', 'mine')); + } + }); + + it("a right password never clears or lowers another Account's failures at the same address", () => { + const { throttle } = setup({ addressLimit: 8 }); + // An attacker's guesses at the victim's Account... + for (let i = 0; i < 5; i++) throttle.attempt(from('1.1.1.1', 'victim')); + // ...then a sign-in with the attacker's own valid Account, many times over. + for (let i = 0; i < 20; i++) { + const turn = throttle.attempt(from('1.1.1.1', 'attacker')); + if (turn.allowed) throttle.succeed(from('1.1.1.1', 'attacker')); + } + // The victim's pair budget is untouched, and so is the address's count of their failures. + expect(throttle.attempt(from('1.1.1.1', 'victim')).allowed).toBe(false); + expect(throttle.attempt(from('1.1.1.1', 'other-1')).allowed).toBe(true); + expect(throttle.attempt(from('1.1.1.1', 'other-2')).allowed).toBe(true); + expect(throttle.attempt(from('1.1.1.1', 'other-3')).allowed).toBe(true); + expect(throttle.attempt(from('1.1.1.1', 'other-4')).allowed).toBe(false); + }); + + it('succeed leaves no empty entry behind, even when its window has ended', () => { + const { throttle, advance } = setup(); + throttle.attempt(from('1.1.1.1', 'mine')); throttle.succeed(from('1.1.1.1', 'mine')); - expect(throttle.attempt(from('1.1.1.1', 'mine')).allowed).toBe(true); - expect(throttle.attempt(from('1.1.1.1', 'mine')).allowed).toBe(false); + expect(throttle.size()).toBe(0); + throttle.attempt(from('1.1.1.1', 'mine')); + advance(10 * MIN); + throttle.succeed(from('1.1.1.1', 'mine')); + throttle.attempt(from('9.9.9.9', null)); + expect(throttle.size()).toBe(1); }); it('succeed with no account does nothing', () => { diff --git a/src/lib/server/identity/throttle.ts b/src/lib/server/identity/throttle.ts index 9ee02357..f372d6d0 100644 --- a/src/lib/server/identity/throttle.ts +++ b/src/lib/server/identity/throttle.ts @@ -28,7 +28,12 @@ export interface Throttle { * sign-ins cannot all pass before any is counted. Call before checking the password. */ attempt(input: ThrottleInput): ThrottleCheck; - /** Call after a right password; clears the account + address counter only. */ + /** + * Call after a right password. Clears the Account + address counter, and takes back the one count + * `attempt` added to the address and to the Account, so right passwords never use up either budget + * (a class behind one shared address would otherwise be locked out after 20 sign-ins). Never clears + * the address or Account counters: a valid Account cannot reset what someone's wrong guesses used. + */ succeed(input: ThrottleInput): void; size(): number; } @@ -88,6 +93,10 @@ interface Entry { * address counter applies. If that lookup ever disagrees with the library, the Account limits are * skipped but the address limit still holds. * + * A right password takes its own attempt back from the address and the Account counters (`succeed`), + * so only wrong passwords spend budget. Callers that count successes too (Sign up) never call it. + * Residual: a success whose window ended before it finished takes a count from the next window. + * * The caller must show one message for "wrong password", "no such account" and "too many attempts", * or the Account limit tells an attacker whether an email has an Account. The Account limit also lets * someone lock a known Account out for a window; that is accepted. @@ -122,6 +131,14 @@ export const createThrottle = ({ return slots; }; + /** One count less; an entry left at zero is dropped, as if the attempt had never been made. */ + const giveBack = ({ entries }: Counter, key: string) => { + const entry = entries.get(key); + if (!entry) return; + if (entry.count <= 1) entries.delete(key); + else entry.count -= 1; + }; + const sweep = ({ entries }: Counter, at: number) => { for (const [id, entry] of entries) { if (entry.resetAt > at) return; @@ -159,7 +176,10 @@ export const createThrottle = ({ }, succeed(input) { if (input.accountId === null) return; - pairs.entries.delete(`${normaliseAddress(input.address)}|${input.accountId}`); + const where = normaliseAddress(input.address); + pairs.entries.delete(`${where}|${input.accountId}`); + giveBack(addresses, where); + giveBack(accounts, input.accountId); }, size: () => addresses.entries.size + pairs.entries.size + accounts.entries.size }; diff --git a/src/routes/signin/sign-in.test.ts b/src/routes/signin/sign-in.test.ts new file mode 100644 index 00000000..73e24bb8 --- /dev/null +++ b/src/routes/signin/sign-in.test.ts @@ -0,0 +1,645 @@ +import { isRedirect } from '@sveltejs/kit'; +import { APIError } from 'better-auth/api'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { createAuth } from '$lib/server/identity/auth'; +import { createMultiUserIdentity } from '$lib/server/identity/multi-user'; +import { createThrottle } from '$lib/server/identity/throttle'; +import { createFileTestDb } from '$lib/server/test-db'; +import { createSignIn, landing } from './sign-in'; + +afterEach(() => vi.restoreAllMocks()); + +const SESSION_COOKIE = + 'better-auth.session_token=abc.def%3D; Max-Age=604800; Path=/; HttpOnly; SameSite=Lax'; +const ada = { email: 'ada@example.com', password: 'correct horse battery' }; +const WRONG = 'Email or password is wrong.'; +const FAILED = 'Signing in failed. Try again.'; + +type Form = Record; + +function formOf(values: Form) { + const body = new FormData(); + for (const [key, value] of Object.entries(values)) body.set(key, value); + return body; +} + +const unauthorized = () => + new APIError('UNAUTHORIZED', { + message: 'Invalid email or password', + code: 'INVALID_EMAIL_OR_PASSWORD' + }); + +/** The request event the Sign in page reads; `locals.auth` is whatever the test passes. */ +function harness( + options: { + auth?: unknown; + /** The Account the email names, by the id Better Auth would find; none by default. */ + accounts?: Record; + /** Which passwords are right, by email. */ + passwords?: Record; + registration?: 'open' | 'invite'; + identity?: unknown; + throttle?: ReturnType; + address?: string | (() => string); + next?: string | null; + } = {} +) { + const accounts = options.accounts ?? { 'ada@example.com': 'user-ada' }; + const passwords = options.passwords ?? { 'ada@example.com': ada.password }; + const findUserByEmail = vi.fn(async (email: string) => + accounts[email] ? { user: { id: accounts[email] }, accounts: [] } : null + ); + const signInEmail = vi.fn(async ({ body }: { body: { email: string; password: string } }) => { + if (passwords[body.email.toLowerCase()] !== body.password) throw unauthorized(); + const headers = new Headers(); + headers.append('set-cookie', SESSION_COOKIE); + return { headers, response: {} }; + }); + const auth = + 'auth' in options + ? options.auth + : { + api: { signInEmail }, + $context: Promise.resolve({ internalAdapter: { findUserByEmail } }) + }; + const throttle = options.throttle ?? createThrottle(); + const signIn = createSignIn(throttle); + const cookies = { set: vi.fn() }; + const setHeaders = vi.fn(); + const address = options.address ?? '203.0.113.7'; + const locals = { + instance: { registrationMode: () => options.registration ?? 'open' }, + auth, + identity: options.identity + }; + const url = new URL( + `http://localhost/signin${options.next === undefined || options.next === null ? '' : `?next=${encodeURIComponent(options.next)}`}` + ); + const event = (values: Form) => ({ + request: { formData: async () => formOf(values) }, + locals, + cookies, + setHeaders, + url, + getClientAddress: typeof address === 'function' ? address : () => address + }); + async function post(values: Form) { + try { + return await signIn.action(event(values) as never); + } catch (thrown) { + if (isRedirect(thrown)) return thrown; + throw thrown; + } + } + function load() { + try { + return signIn.load({ locals, url } as never); + } catch (thrown) { + if (isRedirect(thrown)) return thrown; + throw thrown; + } + } + return { signIn, signInEmail, findUserByEmail, cookies, setHeaders, post, load, throttle }; +} + +describe('landing: where a sign-in goes', () => { + it.each([ + ['/topics/stacks', '/topics/stacks'], + ['/problems/p?tab=hints#top', '/problems/p?tab=hints#top'], + [null, '/'], + ['', '/'], + ['/', '/'] + ])('%j -> %j', (next, expected) => { + expect(landing(next)).toBe(expected); + }); + + it.each([ + ['//evil.example'], + ['//evil.example/path'], + ['/\\evil.example'], + ['\\\\evil.example'], + ['\\/evil.example'], + ['https://evil.example'], + ['http://evil.example/x'], + ['javascript:alert(1)'], + ['data:text/html,x'], + ['evil.example'], + ['topics'], + ['/ok\nhttps://evil.example'], + ['/ok\r\nSet-Cookie: a=b'], + ['/\t/evil.example'], + ['\t//evil.example'], + ['/\u0000'], + [' /ok'] + ])('never leaves this site: %j -> the Map', (next) => { + expect(landing(next)).toBe('/'); + }); + + it.each([ + ['/signin'], + ['/signin?next=/topics/stacks'], + ['/signin/'], + ['/SignIn?x'], + ['/%73ignin'], + ['/signup'], + ['/signup?x=1'] + ])('never lands on a sign-in or sign-up page: %j -> the Map', (next) => { + expect(landing(next)).toBe('/'); + }); + + it('keeps a page whose name merely starts with signin', () => { + expect(landing('/signing-off')).toBe('/signing-off'); + }); +}); + +describe('the page', () => { + it('offers Create an account while the Registration Mode is open', () => { + expect(harness({ registration: 'open' }).load()).toEqual({ open: true }); + }); + + it('offers no sign-up link while the Registration Mode is invite-only', () => { + expect(harness({ registration: 'invite' }).load()).toEqual({ open: false }); + }); + + it('sends someone who is already signed in to the page they asked for, or the Map', () => { + expect(harness({ identity: { learnerId: 'l1' }, next: '/topics/stacks' }).load()).toMatchObject( + { status: 303, location: '/topics/stacks' } + ); + expect(harness({ identity: { learnerId: 'l1' } }).load()).toMatchObject({ + status: 303, + location: '/' + }); + }); + + it('does not send a signed-in visitor off the site, or round and round Sign in', () => { + for (const next of ['//evil.example', 'https://evil.example', '/signin?next=/signin']) { + expect(harness({ identity: { learnerId: 'l1' }, next }).load()).toMatchObject({ + status: 303, + location: '/' + }); + } + }); +}); + +describe('signing in', () => { + it('signs in with the typed email and password, forwards the session cookie and lands on the Map', async () => { + const { post, signInEmail, cookies } = harness(); + + const result = await post(ada); + + expect(signInEmail).toHaveBeenCalledOnce(); + expect(signInEmail).toHaveBeenCalledWith({ body: ada, returnHeaders: true }); + expect(cookies.set).toHaveBeenCalledWith( + 'better-auth.session_token', + 'abc.def=', + expect.objectContaining({ httpOnly: true, path: '/', secure: false }) + ); + expect(result).toMatchObject({ status: 303, location: '/' }); + }); + + it('returns to the page the visitor asked for', async () => { + const { post } = harness({ next: '/topics/stacks?x=1' }); + + expect(await post(ada)).toMatchObject({ status: 303, location: '/topics/stacks?x=1' }); + }); + + it('goes to the Map instead of a next that is not a path on this site', async () => { + for (const next of ['//evil.example', 'https://evil.example', '/\\evil.example', '/signin']) { + const { post } = harness({ next }); + expect(await post(ada), next).toMatchObject({ status: 303, location: '/' }); + } + }); + + it('passes nothing else on: not a remember-me flag, a callback URL, a role or any other field', async () => { + const { post, signInEmail } = harness(); + + await post({ ...ada, rememberMe: 'false', callbackURL: 'https://evil.example', role: 'admin' }); + + expect(signInEmail).toHaveBeenCalledOnce(); + expect(signInEmail.mock.calls[0][0]).toEqual({ body: ada, returnHeaders: true }); + }); + + it('looks the Account up the way Better Auth does: lowercased, not trimmed, as typed otherwise', async () => { + const { post, findUserByEmail, signInEmail } = harness(); + + await post({ email: 'Ada@Example.COM', password: ada.password }); + await post({ email: ' ada@example.com', password: ada.password }); + + expect(findUserByEmail.mock.calls.map(([email]) => email)).toEqual([ + 'ada@example.com', + ' ada@example.com' + ]); + // What Better Auth is asked to sign in is what was typed, so both agree on the Account. + expect(signInEmail.mock.calls[0][0].body.email).toBe('Ada@Example.COM'); + expect(signInEmail.mock.calls[1][0].body.email).toBe(' ada@example.com'); + }); + + it('does nothing for someone who is already signed in', async () => { + const { post, signInEmail, findUserByEmail } = harness({ + identity: { learnerId: 'l1' }, + next: '/topics/stacks' + }); + + expect(await post(ada)).toMatchObject({ status: 303, location: '/topics/stacks' }); + expect(signInEmail).not.toHaveBeenCalled(); + expect(findUserByEmail).not.toHaveBeenCalled(); + }); + + it('does not log the password, the email or the cookies', async () => { + const spies = (['log', 'info', 'warn', 'error', 'debug'] as const).map((m) => + vi.spyOn(console, m).mockImplementation(() => {}) + ); + const { post } = harness(); + await post(ada); + await post({ ...ada, password: 'wrong wrong wrong' }); + const failing = harness({ + auth: { + api: { + signInEmail: async () => { + throw new Error(`insert failed for ${ada.email} / ${ada.password} abc.def`); + } + }, + $context: Promise.resolve({ internalAdapter: { findUserByEmail: async () => null } }) + } + }); + await failing.post(ada); + + expect(JSON.stringify(spies.flatMap((s) => s.mock.calls))).not.toMatch( + /correct horse|wrong wrong|abc\.def|ada@example/ + ); + }); +}); + +describe('one message for everything that is wrong with a sign-in', () => { + const refused = (result: unknown) => JSON.stringify(result); + + it('says the same thing, with the same status, for a wrong password, no such account and a blank form', async () => { + const wrong = await harness().post({ ...ada, password: 'not the password' }); + const unknown = await harness().post({ email: 'nobody@example.com', password: ada.password }); + const blank = await harness().post({ email: '', password: '' }); + const noEmail = await harness().post({ password: ada.password }); + const notAnEmail = await harness().post({ email: 'ada', password: ada.password }); + + for (const result of [wrong, unknown, blank, noEmail, notAnEmail]) { + expect(result).toMatchObject({ status: 400, data: { message: WRONG } }); + } + // Byte for byte the same apart from the typed email. + expect(refused(wrong)).toBe(refused(unknown).replace('nobody@example.com', 'ada@example.com')); + }); + + it('keeps the typed email but never the password', async () => { + const result = await harness().post({ email: 'ada@example.com', password: 'hunter2-hunter2' }); + + expect(result).toMatchObject({ + status: 400, + data: { email: 'ada@example.com', message: WRONG } + }); + expect(refused(result)).not.toContain('hunter2'); + expect(Object.keys((result as { data: object }).data).sort()).toEqual(['email', 'message']); + }); + + it('does not ask Better Auth anything when the form is blank, but still counts the attempt', async () => { + const { post, signInEmail, throttle } = harness(); + const attempt = vi.spyOn(throttle, 'attempt'); + + await post({ email: '', password: '' }); + await post({ email: ada.email, password: '' }); + + expect(signInEmail).not.toHaveBeenCalled(); + expect(attempt).toHaveBeenCalledTimes(2); + }); + + it.each([ + ['INVALID_EMAIL', 'BAD_REQUEST'], + ['PASSWORD_TOO_LONG', 'BAD_REQUEST'], + ['INVALID_EMAIL_OR_PASSWORD', 'UNAUTHORIZED'] + ] as const)('treats Better Auth’s %s as the same wrong sign-in', async (code, status) => { + const { post } = harness({ + auth: { + api: { + signInEmail: async () => { + throw new APIError(status, { message: `Better Auth says ${code}`, code }); + } + }, + $context: Promise.resolve({ internalAdapter: { findUserByEmail: async () => null } }) + } + }); + + const result = await post(ada); + + expect(result).toMatchObject({ status: 400, data: { message: WRONG } }); + expect(refused(result)).not.toContain(code); + }); + + it('hides any other failure behind a fixed message and logs only the kind of failure', async () => { + const spy = vi.spyOn(console, 'error').mockImplementation(() => {}); + const failure = Object.assign(new Error('SQLITE_BUSY at /data/app.db: SELECT FROM "User"'), { + code: 'SQLITE_BUSY' + }); + const { post, cookies } = harness({ + auth: { + api: { + signInEmail: async () => { + throw failure; + } + }, + $context: Promise.resolve({ internalAdapter: { findUserByEmail: async () => null } }) + } + }); + + const result = await post(ada); + + expect(result).toMatchObject({ status: 500, data: { message: FAILED } }); + expect(refused(result)).not.toMatch(/SQLITE|app\.db|SELECT/); + expect(JSON.stringify(spy.mock.calls)).not.toMatch(/app\.db|SELECT|ada@example/); + expect(spy).toHaveBeenCalledOnce(); + expect(cookies.set).not.toHaveBeenCalled(); + }); + + it('answers a refusal it does not know (a ban, an unverified email) as a failure, not as a reason', async () => { + vi.spyOn(console, 'error').mockImplementation(() => {}); + const { post } = harness({ + auth: { + api: { + signInEmail: async () => { + throw new APIError('FORBIDDEN', { + message: 'You have been banned from this application', + code: 'BANNED_USER' + }); + } + }, + $context: Promise.resolve({ internalAdapter: { findUserByEmail: async () => null } }) + } + }); + + const result = await post(ada); + + expect(result).toMatchObject({ status: 500, data: { message: FAILED } }); + expect(refused(result)).not.toMatch(/banned/i); + }); + + it('fails without asking Better Auth when the Account lookup fails', async () => { + const spy = vi.spyOn(console, 'error').mockImplementation(() => {}); + const signInEmail = vi.fn(); + const { post } = harness({ + auth: { + api: { signInEmail }, + $context: Promise.resolve({ + internalAdapter: { + findUserByEmail: async () => { + throw Object.assign(new Error('SELECT * FROM "User" WHERE email = ada@example.com'), { + code: 'SQLITE_BUSY' + }); + } + } + }) + } + }); + + const result = await post(ada); + + expect(result).toMatchObject({ status: 500, data: { message: FAILED } }); + expect(signInEmail).not.toHaveBeenCalled(); + expect(JSON.stringify(spy.mock.calls)).not.toMatch(/SELECT|ada@example/); + }); + + it('is unavailable, not a crash, when the request has no Better Auth', async () => { + const { post } = harness({ auth: undefined }); + + expect(await post(ada)).toMatchObject({ status: 503, data: { message: FAILED } }); + }); +}); + +describe('the sign-in limits', () => { + const wrongPassword = { ...ada, password: 'not the password' }; + + it('refuses the sixth wrong password for an Account from one address, even with the right one, with the one message', async () => { + const { post, signInEmail } = harness(); + for (let i = 0; i < 5; i++) await post(wrongPassword); + signInEmail.mockClear(); + + const withRightPassword = await post(ada); + const withWrongPassword = await post(wrongPassword); + + expect(withRightPassword).toMatchObject({ status: 400, data: { message: WRONG } }); + expect(withWrongPassword).toMatchObject({ status: 400, data: { message: WRONG } }); + expect(signInEmail).not.toHaveBeenCalled(); + }); + + it('answers a throttled attempt exactly like a wrong password: same status, same text, no Retry-After', async () => { + const throttled = harness(); + for (let i = 0; i < 5; i++) await throttled.post(wrongPassword); + + const refusal = await throttled.post(wrongPassword); + const ordinary = await harness().post(wrongPassword); + + expect(JSON.stringify(refusal)).toBe(JSON.stringify(ordinary)); + expect(throttled.setHeaders).not.toHaveBeenCalled(); + }); + + it('counts the attempt against the Account and address Better Auth would sign in, and the address alone for an unknown email', async () => { + const throttle = createThrottle(); + const attempt = vi.spyOn(throttle, 'attempt'); + const { post } = harness({ throttle, address: '203.0.113.9' }); + + await post(wrongPassword); + await post({ email: 'nobody@example.com', password: 'x' }); + await post({ email: '', password: '' }); + + expect(attempt.mock.calls.map(([input]) => input)).toEqual([ + { address: '203.0.113.9', accountId: 'user-ada' }, + { address: '203.0.113.9', accountId: null }, + { address: '203.0.113.9', accountId: null } + ]); + }); + + it('counts before anything else: a flood of guesses at unknown emails is limited per address at 20', async () => { + const { post, signInEmail } = harness(); + for (let i = 0; i < 20; i++) await post({ email: `nobody${i}@example.com`, password: 'x' }); + signInEmail.mockClear(); + + const result = await post(ada); + + expect(result).toMatchObject({ status: 400, data: { message: WRONG } }); + expect(signInEmail).not.toHaveBeenCalled(); + }); + + it('keeps one address from using up another one', async () => { + const throttle = createThrottle(); + const first = harness({ throttle, address: '203.0.113.1' }); + for (let i = 0; i < 5; i++) await first.post(wrongPassword); + + const second = harness({ throttle, address: '203.0.113.2' }); + + expect(await second.post(ada)).toMatchObject({ status: 303 }); + }); + + it('limits one Account across many addresses at 50', async () => { + const throttle = createThrottle(); + for (let i = 0; i < 50; i++) { + await harness({ throttle, address: `198.51.100.${i}` }).post(wrongPassword); + } + + const result = await harness({ throttle, address: '203.0.113.200' }).post(ada); + + expect(result).toMatchObject({ status: 400, data: { message: WRONG } }); + }); + + it('does not let right passwords use up the budget: a class behind one address signs in 30 times', async () => { + const accounts: Record = {}; + const passwords: Record = {}; + for (let i = 0; i < 30; i++) { + accounts[`student${i}@example.com`] = `user-${i}`; + passwords[`student${i}@example.com`] = 'correct horse battery'; + } + const throttle = createThrottle(); + const { post } = harness({ throttle, accounts, passwords, address: '203.0.113.50' }); + + for (let i = 0; i < 30; i++) { + const result = await post({ + email: `student${i}@example.com`, + password: 'correct horse battery' + }); + expect(result, `student ${i}`).toMatchObject({ status: 303, location: '/' }); + } + }); + + it('does not let a valid Account reset an attacker’s guesses at another', async () => { + const accounts = { 'ada@example.com': 'user-ada', 'eve@example.com': 'user-eve' }; + const passwords = { 'ada@example.com': ada.password, 'eve@example.com': 'eve-eve-eve-eve' }; + const throttle = createThrottle(); + const { post } = harness({ throttle, accounts, passwords }); + for (let i = 0; i < 5; i++) await post(wrongPassword); + + // The attacker signs in as themselves, over and over. + for (let i = 0; i < 10; i++) + await post({ email: 'eve@example.com', password: 'eve-eve-eve-eve' }); + + expect(await post(ada)).toMatchObject({ status: 400, data: { message: WRONG } }); + }); + + it('forgives an Account’s own typos at an address once the right password is entered', async () => { + const { post } = harness(); + for (let i = 0; i < 4; i++) await post(wrongPassword); + expect(await post(ada)).toMatchObject({ status: 303 }); + + for (let i = 0; i < 5; i++) await post(wrongPassword); + + expect(await post(ada)).toMatchObject({ status: 400 }); + }); + + it('keeps all unreadable addresses in one shared bucket, so the limit still holds', async () => { + const throwing = () => { + throw new Error('no address'); + }; + const throttle = createThrottle(); + for (let i = 0; i < 20; i++) { + await harness({ throttle, address: throwing }).post({ + email: `nobody${i}@example.com`, + password: 'x' + }); + } + + const result = await harness({ throttle, address: throwing }).post(ada); + + expect(result).toMatchObject({ status: 400, data: { message: WRONG } }); + }); + + it('uses a throttle of its own: the sign-up limit and this one never share counters', async () => { + const signUpLike = createThrottle(); + for (let i = 0; i < 20; i++) signUpLike.attempt({ address: '203.0.113.7', accountId: null }); + + expect(await harness().post(ada)).toMatchObject({ status: 303 }); + }); +}); + +describe('with the real Better Auth', () => { + const password = 'correct horse battery'; + + async function real() { + const db = await createFileTestDb(); + const auth = createAuth({ + db, + origin: 'http://localhost:3000', + secret: 'test-secret-test-secret-test-secret-0123', + admitSignUp: () => ({}) + }); + await auth.api.signUpEmail({ body: { name: 'Ada', email: 'ada@example.com', password } }); + const signIn = createSignIn(createThrottle()); + const cookies = new Map(); + const jar = { + set: (name: string, value: string) => void cookies.set(name, value) + }; + const post = async (values: Form, next = '/') => { + const event = { + request: { formData: async () => formOf(values) }, + locals: { instance: { registrationMode: () => 'open' }, auth }, + cookies: jar, + setHeaders: () => {}, + url: new URL(`http://localhost/signin?next=${encodeURIComponent(next)}`), + getClientAddress: () => '203.0.113.7' + }; + try { + return await signIn.action(event as never); + } catch (thrown) { + if (isRedirect(thrown)) return thrown; + throw thrown; + } + }; + const whoIs = async () => { + const headers = new Headers({ + cookie: [...cookies] + .map(([name, value]) => `${name}=${encodeURIComponent(value)}`) + .join('; ') + }); + return createMultiUserIdentity(auth).resolve({ + cookies: { get: () => undefined }, + request: { headers } + }); + }; + return { db, auth, post, cookies, whoIs }; + } + + it('signs in the Account, whatever case the email is typed in, and the session names its Learner', async () => { + const { post, whoIs, cookies } = await real(); + + const result = await post({ email: 'Ada@Example.com', password }, '/topics/stacks'); + + expect(result).toMatchObject({ status: 303, location: '/topics/stacks' }); + expect([...cookies.keys()]).toContain('better-auth.session_token'); + expect(await whoIs()).toMatchObject({ displayName: 'Ada', role: 'learner' }); + }); + + it('refuses a wrong password and an unknown account with the same answer, and sets no cookie', async () => { + const { post, cookies } = await real(); + + const wrong = await post({ email: 'ada@example.com', password: 'wrong wrong wrong' }); + const unknown = await post({ email: 'nobody@example.com', password: 'wrong wrong wrong' }); + + expect(wrong).toMatchObject({ status: 400, data: { message: WRONG } }); + expect(JSON.stringify(wrong)).toBe( + JSON.stringify(unknown).replace('nobody@example.com', 'ada@example.com') + ); + expect(cookies.size).toBe(0); + }); + + it('refuses the sixth wrong password even when the sixth is the right one', async () => { + const { post, cookies } = await real(); + for (let i = 0; i < 5; i++) + await post({ email: 'ada@example.com', password: 'wrong wrong wrong' }); + + const result = await post({ email: 'ada@example.com', password }); + + expect(result).toMatchObject({ status: 400, data: { message: WRONG } }); + expect(cookies.size).toBe(0); + }); + + it('maps the whole of Better Auth’s email check: a space-padded email is the one wrong message', async () => { + const { post, cookies } = await real(); + + const result = await post({ email: ' ada@example.com ', password }); + + expect(result).toMatchObject({ status: 400, data: { message: WRONG } }); + expect(cookies.size).toBe(0); + }); +}); diff --git a/src/routes/signin/sign-in.ts b/src/routes/signin/sign-in.ts new file mode 100644 index 00000000..304b184c --- /dev/null +++ b/src/routes/signin/sign-in.ts @@ -0,0 +1,140 @@ +import { fail, type RequestEvent, redirect } from '@sveltejs/kit'; +import { isAPIError } from 'better-auth/api'; +import { forwardSetCookies } from '$lib/server/identity/auth-handler'; +import { safeNext } from '$lib/server/identity/gate'; +import type { Throttle } from '$lib/server/identity/throttle'; + +/** + * Fixed texts only: an error's own message (Prisma's carry paths and query text) never reaches the form. + * + * `wrong` is the one text for a wrong password, no such Account and too many attempts. The limits count + * per Account, so a different text for "too many attempts" would tell an attacker which emails have one. + */ +const MESSAGES = { + wrong: 'Email or password is wrong.', + failed: 'Signing in failed. Try again.' +} as const; + +type SignInEvent = Pick< + RequestEvent, + 'request' | 'locals' | 'cookies' | 'getClientAddress' | 'url' +>; + +/** What the form page gets back from a refused attempt: always the same keys, never the password. */ +const refused = (status: 400 | 500 | 503, email: string, message: string) => + fail(status, { email, message }); + +const text = (data: FormData, key: string) => { + const value = data.get(key); + return typeof value === 'string' ? value : ''; +}; + +/** Pages a sign-in never returns to: it would land on a form that sends it straight back. */ +const AUTH_PAGES = new Set(['/signin', '/signup']); + +/** + * Where a visitor goes once signed in: the page they asked for when `next` is a path on this site (see + * `safeNext`), otherwise the Map. Never the Sign in or Sign up page. + */ +export function landing(next: string | null): string { + const target = safeNext(next); + let pathname: string; + try { + // Decoded, because SvelteKit matches `/%73ignin` to the same route as `/signin`. + pathname = decodeURIComponent(new URL(target, 'http://localhost').pathname); + } catch { + return '/'; + } + const page = pathname.length > 1 ? pathname.replace(/\/+$/, '').toLowerCase() : pathname; + return AUTH_PAGES.has(page) ? '/' : target; +} + +/** The caller's address, or '' when the server cannot tell: the throttle keeps those in one shared bucket. */ +function clientAddress(event: Pick): string { + try { + return event.getClientAddress(); + } catch { + return ''; + } +} + +/** The kind of failure only: the error text may hold paths, query text and the input. */ +function logFailure(what: string, error: unknown) { + const code = isAPIError(error) ? error.body?.code : (error as { code?: unknown })?.code; + console.error( + `Sign in: ${what}:`, + error instanceof Error ? error.name : typeof error, + typeof code === 'string' ? code : undefined + ); +} + +/** Better Auth answers 400 to an email or password it will not look at and 401 to a wrong one. */ +const isWrongSignIn = (error: unknown) => + isAPIError(error) && (error.statusCode === 400 || error.statusCode === 401); + +/** + * The Sign in page: whether it links to Sign up, and the action that signs an Account in. + * + * Password sign-in is not served over HTTP (see `disabledPaths` in auth.ts), so this action is the one + * way in with a password, and it carries the limits Better Auth's own limiter cannot: `throttle` counts + * every attempt against the caller's address and, when the email names an Account, against that Account + * (`throttle.ts`). Its counters live in memory in this process. + */ +export function createSignIn(throttle: Throttle) { + return { + /** `open` is whether the Registration Mode lets anyone sign up: only then the page links to Sign up. */ + load({ locals, url }: Pick) { + // Someone who is already signed in goes where they were going. + if (locals.identity) redirect(303, landing(url.searchParams.get('next'))); + return { open: locals.instance.registrationMode() === 'open' }; + }, + + async action(event: SignInEvent) { + const { request, locals, cookies, url } = event; + const next = landing(url.searchParams.get('next')); + if (locals.identity) redirect(303, next); + + const data = await request.formData(); + const email = text(data, 'email'); + const password = text(data, 'password'); + + const { auth } = locals; + if (!auth) return refused(503, email, MESSAGES.failed); + + // The Account Better Auth would sign in: looked up the way it does (lowercased, not trimmed), so the + // Account limits cannot be dodged by how the email is spelled. Null when there is none. + let accountId: string | null = null; + if (email !== '') { + try { + const { internalAdapter } = await auth.$context; + accountId = (await internalAdapter.findUserByEmail(email.toLowerCase()))?.user.id ?? null; + } catch (error) { + logFailure('looking up the account failed', error); + return refused(500, email, MESSAGES.failed); + } + } + + // Before the password is looked at, so a flood of guesses of any kind is counted and none is free. + const input = { address: clientAddress(event), accountId }; + if (!throttle.attempt(input).allowed) return refused(400, email, MESSAGES.wrong); + if (email === '' || password === '') return refused(400, email, MESSAGES.wrong); + + let headers: Headers; + try { + // Only these two fields go on: a client cannot ask for a longer session or a redirect. + ({ headers } = await auth.api.signInEmail({ + body: { email, password }, + returnHeaders: true + })); + } catch (error) { + if (isWrongSignIn(error)) return refused(400, email, MESSAGES.wrong); + logFailure('signing in failed', error); + return refused(500, email, MESSAGES.failed); + } + // A right password takes its own attempt back, so signing in never uses up a shared address's budget. + throttle.succeed(input); + forwardSetCookies(cookies, headers.getSetCookie()); + redirect(303, next); + } + }; +} From c4784dbbb0de5b53819aa6413fa45072d03e167d Mon Sep 17 00:00:00 2001 From: Gauransh Mathur Date: Fri, 16 Oct 2026 10:26:12 +0000 Subject: [PATCH 2/7] feat(beyondleetcode): C32 Sign in page, sign-out action and nav, window-aware throttle give-back, unknown emails limited like known ones Co-Authored-By: Claude Sonnet 5.5 --- src/lib/server/identity/throttle.test.ts | 140 ++++++++-- src/lib/server/identity/throttle.ts | 128 ++++++--- src/routes/+layout.server.ts | 4 +- src/routes/+layout.svelte | 34 ++- src/routes/settings/+page.server.ts | 32 ++- src/routes/settings/+page.svelte | 15 +- src/routes/settings/signout.test.ts | 146 ++++++++++ src/routes/signin/+page.server.ts | 11 + src/routes/signin/+page.svelte | 340 +++++++++++++++++++++++ src/routes/signin/sign-in.test.ts | 97 +++++-- src/routes/signin/sign-in.ts | 37 ++- 11 files changed, 888 insertions(+), 96 deletions(-) create mode 100644 src/routes/settings/signout.test.ts create mode 100644 src/routes/signin/+page.server.ts create mode 100644 src/routes/signin/+page.svelte diff --git a/src/lib/server/identity/throttle.test.ts b/src/lib/server/identity/throttle.test.ts index 003ea365..394d3ecd 100644 --- a/src/lib/server/identity/throttle.test.ts +++ b/src/lib/server/identity/throttle.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest'; -import { createThrottle } from './throttle.ts'; +import { createThrottle, type ThrottleCheck, type ThrottleToken } from './throttle.ts'; const MIN = 60_000; @@ -11,6 +11,12 @@ const setup = (options: Parameters[0] = {}) => { const from = (address: string, accountId: string | null = 'acct') => ({ address, accountId }); +/** The token of an attempt that was admitted; a refused one has none. */ +const tokenOf = (check: ThrottleCheck): ThrottleToken => { + if (!check.allowed) throw new Error('the attempt was refused'); + return check.token; +}; + describe('address spellings', () => { const share = (spellings: string[]) => { const { throttle } = setup({ addressLimit: 3 }); @@ -106,21 +112,22 @@ describe('throttle', () => { for (let i = 0; i < 4; i++) throttle.attempt(from('1.1.1.1')); expect(throttle.attempt(from('1.1.1.1'))).toEqual({ allowed: false, retryAfterMs: MIN }); advance(MIN); - expect(throttle.attempt(from('1.1.1.1'))).toEqual({ allowed: true }); + expect(throttle.attempt(from('1.1.1.1'))).toMatchObject({ allowed: true }); }); it('succeed clears only the account + address counter', () => { const { throttle } = setup(); - for (let i = 0; i < 4; i++) throttle.attempt(from('1.1.1.1')); - throttle.succeed(from('1.1.1.1')); + for (let i = 0; i < 3; i++) throttle.attempt(from('1.1.1.1')); + const last = tokenOf(throttle.attempt(from('1.1.1.1'))); + throttle.succeed(last); for (let i = 0; i < 5; i++) expect(throttle.attempt(from('1.1.1.1')).allowed).toBe(true); expect(throttle.attempt(from('1.1.1.1')).allowed).toBe(false); }); it('succeed gives back exactly one count to the address, so right passwords never use up its budget', () => { const { throttle } = setup({ addressLimit: 6 }); - for (let i = 0; i < 5; i++) throttle.attempt(from('1.1.1.1', 'mine')); - throttle.succeed(from('1.1.1.1', 'mine')); + for (let i = 0; i < 4; i++) throttle.attempt(from('1.1.1.1', 'mine')); + throttle.succeed(tokenOf(throttle.attempt(from('1.1.1.1', 'mine')))); // 5 counted, one taken back: two more fit under 6, the third does not. expect(throttle.attempt(from('1.1.1.1', 'mine')).allowed).toBe(true); expect(throttle.attempt(from('1.1.1.1', 'mine')).allowed).toBe(true); @@ -129,8 +136,8 @@ describe('throttle', () => { it('succeed gives back exactly one count to the Account, however many addresses it signs in from', () => { const { throttle } = setup({ accountLimit: 6 }); - for (let i = 0; i < 5; i++) throttle.attempt(from(`1.1.1.${i}`, 'mine')); - throttle.succeed(from('1.1.1.0', 'mine')); + for (let i = 1; i < 5; i++) throttle.attempt(from(`1.1.1.${i}`, 'mine')); + throttle.succeed(tokenOf(throttle.attempt(from('1.1.1.0', 'mine')))); expect(throttle.attempt(from('2.2.2.1', 'mine')).allowed).toBe(true); expect(throttle.attempt(from('2.2.2.2', 'mine')).allowed).toBe(true); expect(throttle.attempt(from('2.2.2.3', 'mine')).allowed).toBe(false); @@ -139,8 +146,9 @@ describe('throttle', () => { it('a class behind one address can all sign in: a right password is attempt + succeed, net zero', () => { const { throttle } = setup(); for (let student = 0; student < 100; student++) { - expect(throttle.attempt(from('203.0.113.7', `student-${student}`)).allowed).toBe(true); - throttle.succeed(from('203.0.113.7', `student-${student}`)); + const turn = throttle.attempt(from('203.0.113.7', `student-${student}`)); + expect(turn.allowed).toBe(true); + throttle.succeed(tokenOf(turn)); } expect(throttle.attempt(from('203.0.113.7', 'student-0')).allowed).toBe(true); }); @@ -148,8 +156,9 @@ describe('throttle', () => { it('the same Account signing in over and over never runs out either', () => { const { throttle } = setup(); for (let i = 0; i < 200; i++) { - expect(throttle.attempt(from('1.1.1.1', 'mine')).allowed).toBe(true); - throttle.succeed(from('1.1.1.1', 'mine')); + const turn = throttle.attempt(from('1.1.1.1', 'mine')); + expect(turn.allowed).toBe(true); + throttle.succeed(tokenOf(turn)); } }); @@ -160,7 +169,7 @@ describe('throttle', () => { // ...then a sign-in with the attacker's own valid Account, many times over. for (let i = 0; i < 20; i++) { const turn = throttle.attempt(from('1.1.1.1', 'attacker')); - if (turn.allowed) throttle.succeed(from('1.1.1.1', 'attacker')); + if (turn.allowed) throttle.succeed(turn.token); } // The victim's pair budget is untouched, and so is the address's count of their failures. expect(throttle.attempt(from('1.1.1.1', 'victim')).allowed).toBe(false); @@ -170,22 +179,105 @@ describe('throttle', () => { expect(throttle.attempt(from('1.1.1.1', 'other-4')).allowed).toBe(false); }); - it('succeed leaves no empty entry behind, even when its window has ended', () => { - const { throttle, advance } = setup(); - throttle.attempt(from('1.1.1.1', 'mine')); - throttle.succeed(from('1.1.1.1', 'mine')); + it('succeed leaves no empty entry behind', () => { + const { throttle } = setup(); + throttle.succeed(tokenOf(throttle.attempt(from('1.1.1.1', 'mine')))); expect(throttle.size()).toBe(0); - throttle.attempt(from('1.1.1.1', 'mine')); - advance(10 * MIN); - throttle.succeed(from('1.1.1.1', 'mine')); - throttle.attempt(from('9.9.9.9', null)); - expect(throttle.size()).toBe(1); + }); + + it('succeed takes a count back only once per attempt, however often it is called', () => { + const { throttle } = setup({ addressLimit: 3 }); + throttle.attempt(from('1.1.1.1', 'a')); + throttle.attempt(from('1.1.1.1', 'b')); + const mine = tokenOf(throttle.attempt(from('1.1.1.1', 'c'))); + for (let i = 0; i < 5; i++) throttle.succeed(mine); + // Three attempts, one given back: one more fits, the next does not. + expect(throttle.attempt(from('1.1.1.1', 'd')).allowed).toBe(true); + expect(throttle.attempt(from('1.1.1.1', 'e')).allowed).toBe(false); + }); + + describe('when the window ended between the attempt and its success', () => { + // The counted attempt belonged to a window that is gone; what the counters hold now is other + // attempts' failures, and a right password must not give them a free guess back. + it('leaves the new window of the address as it is', () => { + const { throttle, advance } = setup({ addressLimit: 3 }); + const slow = tokenOf(throttle.attempt(from('1.1.1.1', 'mine'))); + advance(10 * MIN); + throttle.attempt(from('1.1.1.1', 'guess-1')); + throttle.attempt(from('1.1.1.1', 'guess-2')); + + throttle.succeed(slow); + + expect(throttle.attempt(from('1.1.1.1', 'guess-3')).allowed).toBe(true); + expect(throttle.attempt(from('1.1.1.1', 'guess-4')).allowed).toBe(false); + }); + + it('leaves the new window of the Account as it is', () => { + const { throttle, advance } = setup({ accountLimit: 3 }); + const slow = tokenOf(throttle.attempt(from('1.1.1.1', 'mine'))); + advance(10 * MIN); + throttle.attempt(from('2.2.2.2', 'mine')); + throttle.attempt(from('3.3.3.3', 'mine')); + + throttle.succeed(slow); + + expect(throttle.attempt(from('4.4.4.4', 'mine')).allowed).toBe(true); + expect(throttle.attempt(from('5.5.5.5', 'mine')).allowed).toBe(false); + }); + + it('does not clear the Account + address counter of the new window', () => { + const { throttle, advance } = setup({ limit: 3 }); + const slow = tokenOf(throttle.attempt(from('1.1.1.1', 'mine'))); + advance(10 * MIN); + throttle.attempt(from('1.1.1.1', 'mine')); + throttle.attempt(from('1.1.1.1', 'mine')); + + throttle.succeed(slow); + + expect(throttle.attempt(from('1.1.1.1', 'mine')).allowed).toBe(true); + expect(throttle.attempt(from('1.1.1.1', 'mine')).allowed).toBe(false); + }); + + it('still gives the count back while the window is the same, even if it ended just now', () => { + const { throttle, advance } = setup({ addressLimit: 3 }); + throttle.attempt(from('1.1.1.1', 'a')); + const mine = tokenOf(throttle.attempt(from('1.1.1.1', 'b'))); + advance(10 * MIN); + + throttle.succeed(mine); + // Nothing in the new window is touched: it starts empty. + for (let i = 0; i < 3; i++) + expect(throttle.attempt(from('1.1.1.1', `n${i}`)).allowed).toBe(true); + expect(throttle.attempt(from('1.1.1.1', 'n3')).allowed).toBe(false); + }); + + it('cannot take a count from a later attempt that landed in the same millisecond as one it already gave back', () => { + const { throttle } = setup({ addressLimit: 2 }); + const first = tokenOf(throttle.attempt(from('1.1.1.1', 'a'))); + throttle.succeed(first); + throttle.attempt(from('1.1.1.1', 'b')); + throttle.attempt(from('1.1.1.1', 'c')); + + throttle.succeed(first); + + expect(throttle.attempt(from('1.1.1.1', 'd')).allowed).toBe(false); + }); + }); + + it('succeed still gives the address its count back when a full Account map had no room for the Account', () => { + const { throttle } = setup({ maxKeys: 2, addressLimit: 3 }); + throttle.attempt(from('1.1.1.1', 'a')); + throttle.attempt(from('1.1.1.2', 'b')); + // The Account maps are full of live entries: this Account goes uncounted there. + const mine = tokenOf(throttle.attempt(from('1.1.1.3', 'c'))); + throttle.succeed(mine); + expect(throttle.attempt(from('1.1.1.4', 'd')).allowed).toBe(true); }); it('succeed with no account does nothing', () => { const { throttle } = setup(); - throttle.attempt(from('1.1.1.1', null)); - throttle.succeed(from('1.1.1.1', null)); + const turn = throttle.attempt(from('1.1.1.1', null)); + throttle.succeed(tokenOf(turn)); expect(throttle.size()).toBe(1); }); diff --git a/src/lib/server/identity/throttle.ts b/src/lib/server/identity/throttle.ts index f372d6d0..1cc0c24c 100644 --- a/src/lib/server/identity/throttle.ts +++ b/src/lib/server/identity/throttle.ts @@ -3,11 +3,37 @@ import { isIP } from 'node:net'; export interface ThrottleInput { /** The client address as the server saw it; any spelling of one address shares a counter. */ address: string; - /** The Account's id, or null when the sign-in names none. */ + /** + * Whom the sign-in names, as a key: an Account's id, or for an email that has no Account a digest of + * that email (the caller makes it, see `signin/sign-in.ts`). Null when it names nobody (a blank form). + */ accountId: string | null; } -export type ThrottleCheck = { allowed: true } | { allowed: false; retryAfterMs: number }; +/** + * Opaque: what `attempt` counted and in which window, handed back to `succeed`. Not to be built or read + * by callers. + */ +export interface ThrottleToken { + /** Whether the attempt named an Account; one that named none gives nothing back. */ + readonly named: boolean; + readonly counted: { + readonly address: CountedIn; + readonly pair?: CountedIn; + readonly account?: CountedIn; + }; + spent: boolean; +} + +/** The entry an attempt was counted in: its key and the end of its window, which names the window. */ +interface CountedIn { + readonly key: string; + readonly resetAt: number; +} + +export type ThrottleCheck = + | { allowed: true; token: ThrottleToken } + | { allowed: false; retryAfterMs: number }; export interface ThrottleOptions { /** Attempts per window for one Account from one address. */ @@ -29,12 +55,17 @@ export interface Throttle { */ attempt(input: ThrottleInput): ThrottleCheck; /** - * Call after a right password. Clears the Account + address counter, and takes back the one count - * `attempt` added to the address and to the Account, so right passwords never use up either budget - * (a class behind one shared address would otherwise be locked out after 20 sign-ins). Never clears - * the address or Account counters: a valid Account cannot reset what someone's wrong guesses used. + * Call with the token of an admitted attempt after a right password. Clears the Account + address + * counter, and takes back the one count `attempt` added to the address and to the Account, so right + * passwords never use up either budget (a class behind one shared address would otherwise be locked out + * after 20 sign-ins). Never clears the address or Account counters: a valid Account cannot reset what + * someone's wrong guesses used. + * + * A counter gives back only if it is still in the window the attempt was counted in. If the window + * ended and a new one began meanwhile, that count is gone and the new window's failures stay. A token + * is good once. A token with no Account (a blank form, as Sign up uses) gives nothing back. */ - succeed(input: ThrottleInput): void; + succeed(token: ThrottleToken): void; size(): number; } @@ -87,15 +118,19 @@ interface Entry { /** * In-memory sign-in throttle, per process: counters reset on restart and are not shared between instances. * - * Three counters, none keyed on a string the client types: the address (20), the Account at an address - * (5, which the address counter caps at 20 per address) and the Account (50). The caller looks the - * Account up the way Better Auth does and passes its id, or null when there is none; then only the - * address counter applies. If that lookup ever disagrees with the library, the Account limits are - * skipped but the address limit still holds. + * Three counters: the address (20), the Account at an address (5, which the address counter caps at 20 per + * address) and the Account (50). The caller names the Account by a key (`accountId`): the id of the Account + * the email names, looked up the way Better Auth does, or, when the email has no Account, a digest of the + * email (`email:` + its SHA-256), so the real address is never held in memory. Known and unknown emails + * therefore reach the throttled state on the same schedule. Without that, only a known email would be + * throttled, and since a throttled refusal skips the password hash and answers faster, six tries and a + * stopwatch would tell an attacker which emails have an Account. The only key built from what the client + * typed is that digest, which is bounded in size and counted in the capped Account maps (below). A blank + * form names nobody (null) and only the address counter applies. * - * A right password takes its own attempt back from the address and the Account counters (`succeed`), - * so only wrong passwords spend budget. Callers that count successes too (Sign up) never call it. - * Residual: a success whose window ended before it finished takes a count from the next window. + * A right password takes its own attempt back from the address and the Account counters (`succeed`), if + * their window is still the one it was counted in, so only wrong passwords spend budget. Callers that + * count successes too (Sign up) never call it. * * The caller must show one message for "wrong password", "no such account" and "too many attempts", * or the Account limit tells an attacker whether an email has an Account. The Account limit also lets @@ -105,8 +140,11 @@ interface Entry { * live entry (the one whose window ends earliest), and only when it holds more distinct unexpired * addresses than maxKeys. Residual: an attacker with more addresses than that inside one window gets * fresh per-address budgets and is still held to the per-Account limit. The Account maps never drop a - * live entry (a full one skips storing the new key), and a full map never refuses an attempt. - * Worst case: 3 * maxKeys entries. + * live entry (a full one skips storing the new key), and a full map never refuses an attempt. Because + * unknown emails are counted too, an attacker typing many different emails can fill the Account maps: new + * keys then go uncounted until entries expire, and the address limit still holds. Doing that takes more + * than maxKeys attempts inside one window, so more than maxKeys / 20 addresses. Worst case: 3 * maxKeys + * entries. */ export const createThrottle = ({ limit = 5, @@ -123,19 +161,28 @@ export const createThrottle = ({ const accounts = counter(accountLimit); type Counter = ReturnType; - const slotsFor = ({ address, accountId }: ThrottleInput): [Counter, string][] => { + type Slot = 'address' | 'pair' | 'account'; + const slotsFor = ({ address, accountId }: ThrottleInput): [Slot, Counter, string][] => { const where = normaliseAddress(address); - const slots: [Counter, string][] = [[addresses, where]]; + const slots: [Slot, Counter, string][] = [['address', addresses, where]]; // The address has no '|', so the key is unambiguous whatever the id holds. - if (accountId !== null) slots.push([pairs, `${where}|${accountId}`], [accounts, accountId]); + if (accountId !== null) { + slots.push(['pair', pairs, `${where}|${accountId}`], ['account', accounts, accountId]); + } return slots; }; + /** The entry `counted` names, only if it is still in the window the attempt was counted in. */ + const sameWindow = ({ entries }: Counter, counted: CountedIn | undefined) => { + const entry = counted && entries.get(counted.key); + return entry && counted && entry.resetAt === counted.resetAt ? entry : undefined; + }; + /** One count less; an entry left at zero is dropped, as if the attempt had never been made. */ - const giveBack = ({ entries }: Counter, key: string) => { - const entry = entries.get(key); - if (!entry) return; - if (entry.count <= 1) entries.delete(key); + const giveBack = (c: Counter, counted: CountedIn | undefined) => { + const entry = sameWindow(c, counted); + if (!entry || !counted) return; + if (entry.count <= 1) c.entries.delete(counted.key); else entry.count -= 1; }; @@ -152,16 +199,18 @@ export const createThrottle = ({ for (const c of [addresses, pairs, accounts]) sweep(c, at); const slots = slotsFor(input); let retryAfterMs = 0; - for (const [c, id] of slots) { + for (const [, c, id] of slots) { const entry = c.entries.get(id); if (entry && entry.count >= c.max) retryAfterMs = Math.max(retryAfterMs, entry.resetAt - at); } if (retryAfterMs > 0) return { allowed: false, retryAfterMs }; - for (const [c, id] of slots) { + const counted: Partial> = {}; + for (const [slot, c, id] of slots) { const entry = c.entries.get(id); if (entry) { entry.count += 1; + counted[slot] = { key: id, resetAt: entry.resetAt }; continue; } if (c.entries.size >= maxKeys) { @@ -171,15 +220,28 @@ export const createThrottle = ({ if (earliest !== undefined) c.entries.delete(earliest); } c.entries.set(id, { count: 1, resetAt: at + windowMs }); + counted[slot] = { key: id, resetAt: at + windowMs }; } - return { allowed: true }; + // The address slot is always counted: the address map evicts instead of skipping. + const { address, pair, account } = counted; + return { + allowed: true, + token: { + named: input.accountId !== null, + counted: { address: address as CountedIn, pair, account }, + spent: false + } + }; }, - succeed(input) { - if (input.accountId === null) return; - const where = normaliseAddress(input.address); - pairs.entries.delete(`${where}|${input.accountId}`); - giveBack(addresses, where); - giveBack(accounts, input.accountId); + succeed(token) { + const { address, pair, account } = token.counted; + // No Account was named, or this token already gave its count back. + if (token.spent || !token.named) return; + token.spent = true; + // The pair counter is cleared, but only the window the right password belongs to. + if (pair && sameWindow(pairs, pair)) pairs.entries.delete(pair.key); + giveBack(addresses, address); + giveBack(accounts, account); }, size: () => addresses.entries.size + pairs.entries.size + accounts.entries.size }; diff --git a/src/routes/+layout.server.ts b/src/routes/+layout.server.ts index 32c5abb5..c76e4677 100644 --- a/src/routes/+layout.server.ts +++ b/src/routes/+layout.server.ts @@ -5,5 +5,7 @@ export const load: LayoutServerLoad = ({ locals }) => ({ displayName: locals.identity?.displayName, // Only the admin of a Multi-user Instance has Accounts to manage. The Local Learner is an admin // too, but Local Mode has no Accounts and no Users page. - showUsers: locals.instance.mode() === 'multi' && locals.identity?.role === 'admin' + showUsers: locals.instance.mode() === 'multi' && locals.identity?.role === 'admin', + // Only a signed-in Account has a session to end. Local Mode has no accounts, so no Sign out. + showSignOut: locals.instance.mode() === 'multi' && locals.identity !== undefined }); diff --git a/src/routes/+layout.svelte b/src/routes/+layout.svelte index 1eae79cd..e6fd5db9 100644 --- a/src/routes/+layout.svelte +++ b/src/routes/+layout.svelte @@ -4,6 +4,10 @@ import favicon from '$lib/assets/favicon.svg'; import '$lib/styles/tokens.css'; let { children, data } = $props(); + +// These pages show the wordmark alone: before the Instance is set up, or before anyone is signed in, +// there is no Map and no name to link to. +const BARE = new Set(['/setup', '/signin', '/signup']); @@ -13,8 +17,7 @@ let { children, data } = $props(); {#if !(page.route.id === '/problems/[problemId]' && !page.error)}
Beyond Leetcode - - {#if page.route.id !== '/setup'} + {#if !BARE.has(page.route.id ?? '')} {/if}
@@ -63,6 +72,27 @@ let { children, data } = $props(); line-height: 20px; text-decoration: none; } + /* A real button set exactly like a nav link (F.11): mono 14, ink, no fill, no border, 44px tall. */ + .signout { + display: contents; + } + .signout button { + padding: 12px 0; + line-height: 20px; + background: transparent; + border: 0; + border-radius: 2px; + color: var(--ink); + font-family: inherit; + font-size: inherit; + cursor: pointer; + } + /* Below 768px the header keeps at most Users and the name; Sign out is on Settings (F.10). */ + @media (max-width: 767px) { + .signout { + display: none; + } + } nav a[aria-current='page'] { text-decoration: underline; text-decoration-color: var(--accent); diff --git a/src/routes/settings/+page.server.ts b/src/routes/settings/+page.server.ts index 01b6a4e9..fb783aae 100644 --- a/src/routes/settings/+page.server.ts +++ b/src/routes/settings/+page.server.ts @@ -1,11 +1,15 @@ -import { fail, redirect } from '@sveltejs/kit'; +import { error, fail, redirect } from '@sveltejs/kit'; +import { isAPIError } from 'better-auth/api'; +import { forwardSetCookies } from '$lib/server/identity/auth-handler'; import { parseTheme, THEME_COOKIE } from '$lib/theme'; import type { Actions, PageServerLoad } from './$types'; const ONE_YEAR = 60 * 60 * 24 * 365; -export const load: PageServerLoad = ({ cookies, url }) => ({ +export const load: PageServerLoad = ({ cookies, url, locals }) => ({ theme: parseTheme(cookies.get(THEME_COOKIE)), + // Only a Multi-user Instance has sessions to end, so only it shows Sign out. + multiUser: locals.instance.mode() === 'multi', // "Reset progress" is a plain GET that opens the confirm, so it works without JavaScript. confirmingReset: url.searchParams.get('reset') === 'confirm' }); @@ -23,6 +27,30 @@ export const actions: Actions = { cookies.set(THEME_COOKIE, theme, { path: '/', maxAge: ONE_YEAR, sameSite: 'lax' }); return { theme }; }, + /** + * Sign out: ends this session in the database, clears its cookies and goes to Sign in. A POST form + * action, never a GET, so a link or an image on another page cannot sign anyone out. The nav posts + * here too (`/settings?/signout`), and the gate lets it through for a user who must change their password. + */ + signout: async ({ request, locals, cookies }) => { + // Local Mode has no accounts, so nothing to sign out of (the same 404 as its other Multi-user pages). + const { auth } = locals; + if (!auth) error(404, 'Not found'); + try { + const { headers } = await auth.api.signOut({ headers: request.headers, returnHeaders: true }); + forwardSetCookies(cookies, headers.getSetCookie()); + } catch (failure) { + // Only the kind of failure is logged: the error text may hold paths and query text. + const code = isAPIError(failure) ? failure.body?.code : (failure as { code?: unknown })?.code; + console.error( + 'Sign out failed:', + failure instanceof Error ? failure.name : typeof failure, + typeof code === 'string' ? code : undefined + ); + return fail(500, { signOut: 'Signing out failed. Try again.' }); + } + redirect(303, '/signin'); + }, /** The confirmed reset: deletes the Learner's progress (the theme cookie stays) and shows the Map. */ reset: async ({ locals }) => { await locals.learning.resetProgress(); diff --git a/src/routes/settings/+page.svelte b/src/routes/settings/+page.svelte index 715b45a4..4978f507 100644 --- a/src/routes/settings/+page.svelte +++ b/src/routes/settings/+page.svelte @@ -59,7 +59,7 @@ const applyTheme: SubmitFunction = ({ formData }) => { -
+

Reset progress

Start over from the beginning.

@@ -87,6 +87,19 @@ const applyTheme: SubmitFunction = ({ formData }) => { {/if}
+ + + {#if data.multiUser} +
+
+

Session

+
+
+ + {#if form?.signOut}{/if} +
+
+ {/if} diff --git a/src/routes/signin/sign-in.test.ts b/src/routes/signin/sign-in.test.ts index 73e24bb8..49023346 100644 --- a/src/routes/signin/sign-in.test.ts +++ b/src/routes/signin/sign-in.test.ts @@ -1,3 +1,4 @@ +import { createHash } from 'node:crypto'; import { isRedirect } from '@sveltejs/kit'; import { APIError } from 'better-auth/api'; import { afterEach, describe, expect, it, vi } from 'vitest'; @@ -281,7 +282,7 @@ describe('one message for everything that is wrong with a sign-in', () => { const notAnEmail = await harness().post({ email: 'ada', password: ada.password }); for (const result of [wrong, unknown, blank, noEmail, notAnEmail]) { - expect(result).toMatchObject({ status: 400, data: { message: WRONG } }); + expect(result).toMatchObject({ status: 400, data: { message: WRONG, wrong: true } }); } // Byte for byte the same apart from the typed email. expect(refused(wrong)).toBe(refused(unknown).replace('nobody@example.com', 'ada@example.com')); @@ -295,7 +296,11 @@ describe('one message for everything that is wrong with a sign-in', () => { data: { email: 'ada@example.com', message: WRONG } }); expect(refused(result)).not.toContain('hunter2'); - expect(Object.keys((result as { data: object }).data).sort()).toEqual(['email', 'message']); + expect(Object.keys((result as { data: object }).data).sort()).toEqual([ + 'email', + 'message', + 'wrong' + ]); }); it('does not ask Better Auth anything when the form is blank, but still counts the attempt', async () => { @@ -327,7 +332,7 @@ describe('one message for everything that is wrong with a sign-in', () => { const result = await post(ada); - expect(result).toMatchObject({ status: 400, data: { message: WRONG } }); + expect(result).toMatchObject({ status: 400, data: { message: WRONG, wrong: true } }); expect(refused(result)).not.toContain(code); }); @@ -349,7 +354,7 @@ describe('one message for everything that is wrong with a sign-in', () => { const result = await post(ada); - expect(result).toMatchObject({ status: 500, data: { message: FAILED } }); + expect(result).toMatchObject({ status: 500, data: { message: FAILED, wrong: false } }); expect(refused(result)).not.toMatch(/SQLITE|app\.db|SELECT/); expect(JSON.stringify(spy.mock.calls)).not.toMatch(/app\.db|SELECT|ada@example/); expect(spy).toHaveBeenCalledOnce(); @@ -374,7 +379,7 @@ describe('one message for everything that is wrong with a sign-in', () => { const result = await post(ada); - expect(result).toMatchObject({ status: 500, data: { message: FAILED } }); + expect(result).toMatchObject({ status: 500, data: { message: FAILED, wrong: false } }); expect(refused(result)).not.toMatch(/banned/i); }); @@ -398,7 +403,7 @@ describe('one message for everything that is wrong with a sign-in', () => { const result = await post(ada); - expect(result).toMatchObject({ status: 500, data: { message: FAILED } }); + expect(result).toMatchObject({ status: 500, data: { message: FAILED, wrong: false } }); expect(signInEmail).not.toHaveBeenCalled(); expect(JSON.stringify(spy.mock.calls)).not.toMatch(/SELECT|ada@example/); }); @@ -406,7 +411,7 @@ describe('one message for everything that is wrong with a sign-in', () => { it('is unavailable, not a crash, when the request has no Better Auth', async () => { const { post } = harness({ auth: undefined }); - expect(await post(ada)).toMatchObject({ status: 503, data: { message: FAILED } }); + expect(await post(ada)).toMatchObject({ status: 503, data: { message: FAILED, wrong: false } }); }); }); @@ -421,8 +426,8 @@ describe('the sign-in limits', () => { const withRightPassword = await post(ada); const withWrongPassword = await post(wrongPassword); - expect(withRightPassword).toMatchObject({ status: 400, data: { message: WRONG } }); - expect(withWrongPassword).toMatchObject({ status: 400, data: { message: WRONG } }); + expect(withRightPassword).toMatchObject({ status: 400, data: { message: WRONG, wrong: true } }); + expect(withWrongPassword).toMatchObject({ status: 400, data: { message: WRONG, wrong: true } }); expect(signInEmail).not.toHaveBeenCalled(); }); @@ -437,20 +442,66 @@ describe('the sign-in limits', () => { expect(throttled.setHeaders).not.toHaveBeenCalled(); }); - it('counts the attempt against the Account and address Better Auth would sign in, and the address alone for an unknown email', async () => { + it('counts the attempt against the Account Better Auth would sign in and the address, a digest of the email when there is no Account, and the address alone for a blank form', async () => { const throttle = createThrottle(); const attempt = vi.spyOn(throttle, 'attempt'); const { post } = harness({ throttle, address: '203.0.113.9' }); await post(wrongPassword); - await post({ email: 'nobody@example.com', password: 'x' }); + await post({ email: 'Nobody@Example.com', password: 'x' }); await post({ email: '', password: '' }); - expect(attempt.mock.calls.map(([input]) => input)).toEqual([ - { address: '203.0.113.9', accountId: 'user-ada' }, - { address: '203.0.113.9', accountId: null }, - { address: '203.0.113.9', accountId: null } - ]); + const inputs = attempt.mock.calls.map(([input]) => input); + expect(inputs[0]).toEqual({ address: '203.0.113.9', accountId: 'user-ada' }); + expect(inputs[1]).toEqual({ + address: '203.0.113.9', + accountId: `email:${createHash('sha256').update('nobody@example.com').digest('hex')}` + }); + expect(inputs[2]).toEqual({ address: '203.0.113.9', accountId: null }); + }); + + it('never hands the throttle an email, and keys unknown emails the way the lookup spells them', async () => { + const throttle = createThrottle(); + const attempt = vi.spyOn(throttle, 'attempt'); + const { post } = harness({ throttle }); + + await post({ email: 'nobody@example.com', password: 'x' }); + await post({ email: 'NOBODY@example.com', password: 'x' }); + await post({ email: ' nobody@example.com', password: 'x' }); + + const keys = attempt.mock.calls.map(([input]) => input.accountId); + expect(JSON.stringify(keys)).not.toContain('nobody@'); + // Lowercased like Better Auth's lookup, so one spelling cannot escape the other's count; not trimmed. + expect(keys[0]).toBe(keys[1]); + expect(keys[2]).not.toBe(keys[0]); + }); + + it('refuses the 6th wrong try for an unknown email exactly as for a known one, from one address', async () => { + const unknown = { email: 'nobody@example.com', password: 'not the password' }; + for (const typed of [wrongPassword, unknown]) { + const { post, signInEmail } = harness(); + const answers = []; + for (let i = 0; i < 6; i++) answers.push(await post(typed)); + + // Five were asked (and wrong); the sixth was refused before the password was looked at. + expect(signInEmail, typed.email).toHaveBeenCalledTimes(5); + expect(answers.map((a) => (a as { status: number }).status)).toEqual([ + 400, 400, 400, 400, 400, 400 + ]); + } + }); + + it('limits an unknown email across many addresses at 50, like a known one', async () => { + const throttle = createThrottle(); + const unknown = { email: 'nobody@example.com', password: 'x' }; + let asked = 0; + for (let i = 0; i < 52; i++) { + const { post, signInEmail } = harness({ throttle, address: `198.51.100.${i}` }); + await post(unknown); + asked += signInEmail.mock.calls.length; + } + + expect(asked).toBe(50); }); it('counts before anything else: a flood of guesses at unknown emails is limited per address at 20', async () => { @@ -460,7 +511,7 @@ describe('the sign-in limits', () => { const result = await post(ada); - expect(result).toMatchObject({ status: 400, data: { message: WRONG } }); + expect(result).toMatchObject({ status: 400, data: { message: WRONG, wrong: true } }); expect(signInEmail).not.toHaveBeenCalled(); }); @@ -482,7 +533,7 @@ describe('the sign-in limits', () => { const result = await harness({ throttle, address: '203.0.113.200' }).post(ada); - expect(result).toMatchObject({ status: 400, data: { message: WRONG } }); + expect(result).toMatchObject({ status: 400, data: { message: WRONG, wrong: true } }); }); it('does not let right passwords use up the budget: a class behind one address signs in 30 times', async () => { @@ -515,7 +566,7 @@ describe('the sign-in limits', () => { for (let i = 0; i < 10; i++) await post({ email: 'eve@example.com', password: 'eve-eve-eve-eve' }); - expect(await post(ada)).toMatchObject({ status: 400, data: { message: WRONG } }); + expect(await post(ada)).toMatchObject({ status: 400, data: { message: WRONG, wrong: true } }); }); it('forgives an Account’s own typos at an address once the right password is entered', async () => { @@ -542,7 +593,7 @@ describe('the sign-in limits', () => { const result = await harness({ throttle, address: throwing }).post(ada); - expect(result).toMatchObject({ status: 400, data: { message: WRONG } }); + expect(result).toMatchObject({ status: 400, data: { message: WRONG, wrong: true } }); }); it('uses a throttle of its own: the sign-up limit and this one never share counters', async () => { @@ -616,7 +667,7 @@ describe('with the real Better Auth', () => { const wrong = await post({ email: 'ada@example.com', password: 'wrong wrong wrong' }); const unknown = await post({ email: 'nobody@example.com', password: 'wrong wrong wrong' }); - expect(wrong).toMatchObject({ status: 400, data: { message: WRONG } }); + expect(wrong).toMatchObject({ status: 400, data: { message: WRONG, wrong: true } }); expect(JSON.stringify(wrong)).toBe( JSON.stringify(unknown).replace('nobody@example.com', 'ada@example.com') ); @@ -630,7 +681,7 @@ describe('with the real Better Auth', () => { const result = await post({ email: 'ada@example.com', password }); - expect(result).toMatchObject({ status: 400, data: { message: WRONG } }); + expect(result).toMatchObject({ status: 400, data: { message: WRONG, wrong: true } }); expect(cookies.size).toBe(0); }); @@ -639,7 +690,7 @@ describe('with the real Better Auth', () => { const result = await post({ email: ' ada@example.com ', password }); - expect(result).toMatchObject({ status: 400, data: { message: WRONG } }); + expect(result).toMatchObject({ status: 400, data: { message: WRONG, wrong: true } }); expect(cookies.size).toBe(0); }); }); diff --git a/src/routes/signin/sign-in.ts b/src/routes/signin/sign-in.ts index 304b184c..ce4391be 100644 --- a/src/routes/signin/sign-in.ts +++ b/src/routes/signin/sign-in.ts @@ -1,3 +1,4 @@ +import { createHash } from 'node:crypto'; import { fail, type RequestEvent, redirect } from '@sveltejs/kit'; import { isAPIError } from 'better-auth/api'; import { forwardSetCookies } from '$lib/server/identity/auth-handler'; @@ -20,9 +21,13 @@ type SignInEvent = Pick< 'request' | 'locals' | 'cookies' | 'getClientAddress' | 'url' >; -/** What the form page gets back from a refused attempt: always the same keys, never the password. */ -const refused = (status: 400 | 500 | 503, email: string, message: string) => - fail(status, { email, message }); +/** + * What the form page gets back from a refused attempt: always the same keys, never the password. + * `wrong` marks the one message of a wrong sign-in (the page puts it on the fields); a failure is not that. + */ +const refused = (status: 400 | 500 | 503, email: string, message: string, wrong = false) => + fail(status, { email, message, wrong }); +const wrongSignIn = (email: string) => refused(400, email, MESSAGES.wrong, true); const text = (data: FormData, key: string) => { const value = data.get(key); @@ -58,6 +63,16 @@ function clientAddress(event: Pick): string { } } +/** + * The limits' key for an email that has no Account: a digest of the normalised email (lowercased like + * Better Auth's lookup, not trimmed), so the email itself is never held in memory. Known and unknown + * emails then reach the throttled state on the same schedule; if only known ones did, a throttled + * refusal (which skips the password hash, so it answers faster) would tell an attacker which emails + * have an Account. User ids hold no ':', so the two kinds of key never meet. + */ +const unknownAccountKey = (email: string) => + `email:${createHash('sha256').update(email.toLowerCase()).digest('hex')}`; + /** The kind of failure only: the error text may hold paths, query text and the input. */ function logFailure(what: string, error: unknown) { const code = isAPIError(error) ? error.body?.code : (error as { code?: unknown })?.code; @@ -102,12 +117,14 @@ export function createSignIn(throttle: Throttle) { if (!auth) return refused(503, email, MESSAGES.failed); // The Account Better Auth would sign in: looked up the way it does (lowercased, not trimmed), so the - // Account limits cannot be dodged by how the email is spelled. Null when there is none. + // Account limits cannot be dodged by how the email is spelled. With none, the email's digest stands + // in for its id, so an unknown email is limited exactly like a known one. A blank form names nobody. let accountId: string | null = null; if (email !== '') { try { const { internalAdapter } = await auth.$context; - accountId = (await internalAdapter.findUserByEmail(email.toLowerCase()))?.user.id ?? null; + const found = await internalAdapter.findUserByEmail(email.toLowerCase()); + accountId = found?.user.id ?? unknownAccountKey(email); } catch (error) { logFailure('looking up the account failed', error); return refused(500, email, MESSAGES.failed); @@ -115,9 +132,9 @@ export function createSignIn(throttle: Throttle) { } // Before the password is looked at, so a flood of guesses of any kind is counted and none is free. - const input = { address: clientAddress(event), accountId }; - if (!throttle.attempt(input).allowed) return refused(400, email, MESSAGES.wrong); - if (email === '' || password === '') return refused(400, email, MESSAGES.wrong); + const turn = throttle.attempt({ address: clientAddress(event), accountId }); + if (!turn.allowed) return wrongSignIn(email); + if (email === '' || password === '') return wrongSignIn(email); let headers: Headers; try { @@ -127,12 +144,12 @@ export function createSignIn(throttle: Throttle) { returnHeaders: true })); } catch (error) { - if (isWrongSignIn(error)) return refused(400, email, MESSAGES.wrong); + if (isWrongSignIn(error)) return wrongSignIn(email); logFailure('signing in failed', error); return refused(500, email, MESSAGES.failed); } // A right password takes its own attempt back, so signing in never uses up a shared address's budget. - throttle.succeed(input); + throttle.succeed(turn.token); forwardSetCookies(cookies, headers.getSetCookie()); redirect(303, next); } From 5f999d70f8b5be66751c0e63b27c37ae0c361595 Mon Sep 17 00:00:00 2001 From: Gauransh Mathur Date: Fri, 16 Oct 2026 10:36:19 +0000 Subject: [PATCH 3/7] feat(beyondleetcode): C32 Multi-user on for everyone, separate counters for unknown emails, browser tests Co-Authored-By: Claude Sonnet 5.5 --- src/lib/server/app.test.ts | 38 ++- src/lib/server/app.ts | 15 +- src/lib/server/handle.test.ts | 84 ++++++- src/lib/server/identity/throttle.test.ts | 130 +++++++++++ src/lib/server/identity/throttle.ts | 110 ++++++--- src/routes/setup/+page.server.ts | 10 +- src/routes/setup/+page.svelte | 4 +- src/routes/setup/setup-action.test.ts | 36 ++- src/routes/setup/setup.multi.e2e.ts | 52 +++-- src/routes/signin/fresh-server.ts | 89 ++++++++ src/routes/signin/multi-user-flow.test.ts | 118 ++++++++++ src/routes/signin/sign-in.test.ts | 30 ++- src/routes/signin/sign-in.ts | 27 ++- src/routes/signin/signin.multi.e2e.ts | 267 ++++++++++++++++++++++ 14 files changed, 895 insertions(+), 115 deletions(-) create mode 100644 src/routes/signin/fresh-server.ts create mode 100644 src/routes/signin/multi-user-flow.test.ts create mode 100644 src/routes/signin/signin.multi.e2e.ts diff --git a/src/lib/server/app.test.ts b/src/lib/server/app.test.ts index e05a92a7..781225a8 100644 --- a/src/lib/server/app.test.ts +++ b/src/lib/server/app.test.ts @@ -94,18 +94,40 @@ const bootMulti = (url: string, extra: Record = {}) }); describe('Multi-user', () => { - it('is a later release on a server without the test switch, with or without a secret', async () => { + it('can be chosen on a server without the test switch once the secret and origin are set, and signs the admin in', async () => { + const { url, db } = await migratedDatabase(); + const app = await boot(url, { BETTER_AUTH_SECRET: secret, ORIGIN: 'http://localhost:4174' }); + expect(app.setup.multiUser).toEqual({ kind: 'open' }); + + const { headers } = await app.setup.createFirstAdmin(admin); + + expect(app.instance.mode()).toBe('multi'); + const cookie = headers + .getSetCookie() + .map((c) => c.split(';')[0]) + .join('; '); + const who = await app.multiUser?.identity.resolve({ + cookies: { get: () => undefined }, + request: { headers: new Headers({ cookie }) } + }); + expect(who).toMatchObject({ displayName: 'Ada', role: 'admin', learnerId: 'local' }); + expect(await db.learner.findMany()).toEqual([expect.objectContaining({ id: 'local' })]); + }, 30_000); + + it('says what to add to .env on a server without the test switch and without a secret', async () => { const { url } = await migratedDatabase(); - for (const extra of [{}, { BETTER_AUTH_SECRET: secret, ORIGIN: 'http://localhost:4174' }]) { - const app = await boot(url, extra); - expect(app.setup.multiUser).toEqual({ kind: 'later' }); - await expect(app.setup.createFirstAdmin(admin)).rejects.toThrow(/not available/); - } - expect((await boot(url)).instance.mode()).toBeNull(); + const app = await boot(url); + + expect(app.setup.multiUser).toEqual({ + kind: 'needs-env', + message: expect.stringMatching(/BETTER_AUTH_SECRET.*\.env/s) + }); + await expect(app.setup.createFirstAdmin(admin)).rejects.toThrow(/not available/); + expect(app.instance.mode()).toBeNull(); }, 30_000); - it('can be chosen on the test server once the secret and origin are set, and signs the admin in', async () => { + it('can be chosen on the test server too, once the secret and origin are set', async () => { const { url, db } = await migratedDatabase(); const app = await bootMulti(url); expect(app.setup.multiUser).toEqual({ kind: 'open' }); diff --git a/src/lib/server/app.ts b/src/lib/server/app.ts index 5fba3031..e5107df0 100644 --- a/src/lib/server/app.ts +++ b/src/lib/server/app.ts @@ -18,7 +18,11 @@ import { createCatalogueScriptedRunner } from './runner/scripted-catalogue.ts'; /** Whether Setup can offer Multi-user on this server, and if not, why. */ export type MultiUserChoice = | { kind: 'open' } - /** Released builds, until Sign in exists (C32): only the browser tests' server can choose it. */ + /** + * Never produced since C32: every Instance with what Better Auth needs is `open`. Only still named + * because test stubs outside Setup's files (`signup-gate.test.ts`, `users.test.ts`) build one; delete + * the variant, and the handling of it in Setup, once they stop. + */ | { kind: 'later' } /** `message` says what to add to `.env`. */ | { kind: 'needs-env'; message: string }; @@ -104,9 +108,9 @@ export async function createApp(env: { identity, multiUser, setup: { - multiUser: choice(env, auth), + multiUser: choice(auth), async createFirstAdmin(input) { - if (choice(env, auth).kind !== 'open' || auth.kind !== 'built') { + if (auth.kind !== 'built') { throw new MultiUserNotAvailableError(); } return createFirstAdmin({ instance, auth: auth.auth, db }, input); @@ -201,9 +205,8 @@ function originOf(value: string | undefined): string | null { const chooseMultiUser = (auth: Auth) => ({ auth, identity: createMultiUserIdentity(auth) }); -/** Released builds keep Multi-user off until Sign in exists (C32 removes the test-switch condition). */ -function choice(env: { IDENTITY_TEST_LEARNER?: string }, auth: BuiltAuth): MultiUserChoice { - if (env.IDENTITY_TEST_LEARNER !== '1') return { kind: 'later' }; +/** Multi-user is open to every Instance that has what Better Auth needs; otherwise Setup says what to add. */ +function choice(auth: BuiltAuth): MultiUserChoice { return auth.kind === 'built' ? { kind: 'open' } : { kind: 'needs-env', message: auth.message }; } diff --git a/src/lib/server/handle.test.ts b/src/lib/server/handle.test.ts index 78fd90bd..fee60930 100644 --- a/src/lib/server/handle.test.ts +++ b/src/lib/server/handle.test.ts @@ -19,7 +19,7 @@ async function setup(options: { local?: boolean } = {}) { await identity.ensureLocalLearner(); } const core = { forLearner: (learnerId: string) => ({ learnerId }) } as unknown as App['core']; - const setupPort = { multiUser: { kind: 'later' }, createFirstAdmin: vi.fn() } as App['setup']; + const setupPort = { multiUser: { kind: 'open' }, createFirstAdmin: vi.fn() } as App['setup']; return { db, handle: createHandle({ instance, identity, core, multiUser: null, setup: setupPort }) @@ -241,7 +241,7 @@ describe('Multi-user Mode', () => { it('lets the auth route through to a signed-out visitor, with the Better Auth instance on locals', async () => { const { handle, asking, auth } = await multiSetup(); - const { event, locals, resolve } = asking('/api/auth/sign-in/email', '/api/auth/[...all]', ''); + const { event, locals, resolve } = asking('/api/auth/get-session', '/api/auth/[...all]', ''); const response = await handle({ event, resolve }); @@ -253,7 +253,7 @@ describe('Multi-user Mode', () => { it('still refuses a remote call that claims the auth path', async () => { const { handle, asking } = await multiSetup(); - const { event, resolve } = asking('/api/auth/sign-in/email', '/api/auth/[...all]', ''); + const { event, resolve } = asking('/api/auth/get-session', '/api/auth/[...all]', ''); (event as { isRemoteRequest: boolean }).isRemoteRequest = true; const response = await handle({ event, resolve }); @@ -378,9 +378,23 @@ describe('Multi-user Mode', () => { expect(response.headers.get('location')).toBe('/'); }); - it('answers an unmatched path as the gate says (the 404 page for everyone), with the headers', async () => { + it('treats a path with no route as a private page: a signed-out visitor is sent to Sign in, not shown the 404', async () => { const { handle, asking } = await multiSetup(); - const { event, resolve } = asking('/no/such/page', null, ''); + const { event, resolve } = asking('/no/such/page?x=1', null, ''); + + const response = await handle({ event, resolve }); + + expect(response.status).toBe(303); + expect(response.headers.get('location')).toBe( + `/signin?next=${encodeURIComponent('/no/such/page?x=1')}` + ); + expect(Object.fromEntries(response.headers)).toMatchObject(framing); + expect(resolve).not.toHaveBeenCalled(); + }); + + it('shows a signed-in user the 404 page for a path with no route, with the headers', async () => { + const { handle, asking } = await multiSetup(); + const { event, resolve } = asking('/no/such/page', null); const response = await handle({ event, resolve }); @@ -389,6 +403,66 @@ describe('Multi-user Mode', () => { expect(Object.fromEntries(response.headers)).toMatchObject(framing); }); + it('sends a user who must change their password from a path with no route to that page', async () => { + const { handle, asking, db } = await multiSetup(); + await db.user.updateMany({ data: { mustChangePassword: true } }); + const { event, resolve } = asking('/no/such/page', null); + + const response = await handle({ event, resolve }); + + expect(response.status).toBe(303); + expect(response.headers.get('location')).toBe('/account/password'); + expect(resolve).not.toHaveBeenCalled(); + }); + + it('still lets a user who must change their password sign out, from the nav and from Settings', async () => { + const { handle, db, cookie } = await multiSetup(); + await db.user.updateMany({ data: { mustChangePassword: true } }); + for (const [path, routeId] of [ + ['/settings?/signout', '/settings'], + ['/api/auth/sign-out', '/api/auth/[...all]'] + ] as const) { + const { event, resolve } = request(path, routeId, 'POST', new Headers({ cookie })); + + const response = await handle({ event, resolve }); + + expect(response.status, path).toBe(200); + expect(resolve, path).toHaveBeenCalledOnce(); + } + // Any other action of that page is still held back. + const reset = request('/settings?/reset', '/settings', 'POST', new Headers({ cookie })); + const held = await handle({ event: reset.event, resolve: reset.resolve }); + expect(held.status).toBe(303); + expect(held.headers.get('location')).toBe('/account/password'); + }); + + it('does not set the refreshed session cookie over the one a sign-out clears', async () => { + const { handle, db, cookie } = await multiSetup(); + await db.session.updateMany({ + data: { expiresAt: new Date(Date.now() + 5 * 24 * 60 * 60 * 1000) } + }); + const { event, resolve } = request( + '/settings?/signout', + '/settings', + 'POST', + new Headers({ cookie }) + ); + resolve.mockImplementation(async () => { + const response = new Response(null, { status: 303, headers: { location: '/signin' } }); + response.headers.append( + 'set-cookie', + 'better-auth.session_token=; Max-Age=0; Path=/; HttpOnly' + ); + return response; + }); + + const response = await handle({ event, resolve }); + + expect(response.headers.getSetCookie()).toEqual([ + 'better-auth.session_token=; Max-Age=0; Path=/; HttpOnly' + ]); + }); + it('fails closed when a multi Instance has no Better Auth', async () => { const db = await createTestDb(); const instance = await createInstance(db); diff --git a/src/lib/server/identity/throttle.test.ts b/src/lib/server/identity/throttle.test.ts index 394d3ecd..67fc8338 100644 --- a/src/lib/server/identity/throttle.test.ts +++ b/src/lib/server/identity/throttle.test.ts @@ -334,3 +334,133 @@ describe('throttle', () => { expect(throttle.size()).toBe(1); }); }); + +describe('emails that have no Account', () => { + // The caller names them by a digest (`unknownEmail`), kept apart from real Account ids, so that a + // stranger typing many emails can use up only the room reserved for emails. + const unknown = (address: string, digest: string) => ({ + address, + accountId: null, + unknownEmail: digest + }); + + it('are refused on the 6th try from one address, on the same schedule as a known Account', () => { + for (const input of [from('1.1.1.1', 'user-id'), unknown('1.1.1.1', 'digest')]) { + const { throttle } = setup(); + const answers = Array.from({ length: 7 }, () => throttle.attempt(input).allowed); + expect(answers).toEqual([true, true, true, true, true, false, false]); + } + }); + + it('are refused on the 51st try across many addresses, on the same schedule as a known Account', () => { + for (const make of [ + (address: string) => from(address, 'user-id'), + (address: string) => unknown(address, 'digest') + ]) { + const { throttle } = setup(); + const answers = Array.from( + { length: 52 }, + (_, i) => throttle.attempt(make(`10.0.${i}.1`)).allowed + ); + expect(answers.filter(Boolean)).toHaveLength(50); + } + }); + + it('count against the address like any attempt', () => { + const { throttle } = setup({ addressLimit: 3 }); + for (let i = 0; i < 3; i++) throttle.attempt(unknown('1.1.1.1', `d${i}`)); + expect(throttle.attempt(unknown('1.1.1.1', 'd3')).allowed).toBe(false); + expect(throttle.attempt(from('1.1.1.1', 'user-id')).allowed).toBe(false); + }); + + it('never share a counter with a real Account id, whatever the two are called', () => { + const { throttle } = setup(); + for (let i = 0; i < 5; i++) throttle.attempt(unknown('1.1.1.1', 'same')); + expect(throttle.attempt(unknown('1.1.1.1', 'same')).allowed).toBe(false); + // A real Account whose id happens to read like the digest has a count of its own. + expect(throttle.attempt(from('1.1.1.1', 'same')).allowed).toBe(true); + }); + + it('give a count back on succeed, in the window they were counted in, like a known Account', () => { + const { throttle, advance } = setup({ addressLimit: 3 }); + throttle.attempt(unknown('1.1.1.1', 'a')); + throttle.succeed(tokenOf(throttle.attempt(unknown('1.1.1.1', 'a')))); + expect(throttle.attempt(unknown('1.1.1.1', 'b')).allowed).toBe(true); + expect(throttle.attempt(unknown('1.1.1.1', 'c')).allowed).toBe(true); + expect(throttle.attempt(unknown('1.1.1.1', 'd')).allowed).toBe(false); + + advance(10 * MIN); + const slow = tokenOf(throttle.attempt(unknown('2.2.2.2', 'x'))); + advance(10 * MIN); + throttle.attempt(unknown('2.2.2.2', 'x')); + throttle.attempt(unknown('2.2.2.2', 'x')); + throttle.succeed(slow); + // Two failures in the new window, nothing given back: one more fits under 3. + expect(throttle.attempt(unknown('2.2.2.2', 'x')).allowed).toBe(true); + expect(throttle.attempt(unknown('2.2.2.2', 'x')).allowed).toBe(false); + }); + + describe('typed in a flood', () => { + const maxKeys = 20; + // One address with no limit of its own, so only the email maps are under test. + const flood = (throttle: ReturnType, count: number, start = 0) => { + for (let i = start; i < start + count; i++) throttle.attempt(unknown('9.9.9.9', `fake-${i}`)); + }; + + it('evict the oldest emails and never grow past maxKeys, never refusing', () => { + const { throttle } = setup({ maxKeys, addressLimit: 1_000_000 }); + for (let i = 0; i < 5 * maxKeys; i++) { + expect(throttle.attempt(unknown('9.9.9.9', `fake-${i}`)).allowed).toBe(true); + } + + // The address, plus the two email maps at their cap; no real-Account map holds anything. + expect(throttle.size()).toBe(1 + 2 * maxKeys); + }); + + it('leave a known Account counted: refused on its 6th wrong try from one address', () => { + const { throttle } = setup({ maxKeys, addressLimit: 1_000_000 }); + // Two tries before the flood, so its counters already exist; the rest come after. + for (let i = 0; i < 2; i++) throttle.attempt(from('1.1.1.1', 'victim')); + flood(throttle, 10 * maxKeys); + + const rest = Array.from({ length: 4 }, () => throttle.attempt(from('1.1.1.1', 'victim'))); + + expect(rest.map((r) => r.allowed)).toEqual([true, true, true, false]); + }); + + it('leave a known Account the flood never saw counted too: refused on its 6th try', () => { + const { throttle } = setup({ maxKeys, addressLimit: 1_000_000 }); + flood(throttle, 10 * maxKeys); + + const tries = Array.from({ length: 7 }, () => throttle.attempt(from('1.1.1.1', 'victim'))); + + expect(tries.map((r) => r.allowed)).toEqual([true, true, true, true, true, false, false]); + }); + + it('leave a known Account held to 50 across addresses: refused on its 51st try', () => { + const { throttle } = setup({ maxKeys, addressLimit: 1_000_000 }); + for (let i = 0; i < 25; i++) throttle.attempt(from(`10.0.0.${i}`, 'victim')); + flood(throttle, 10 * maxKeys); + let admitted = 25; + for (let i = 25; i < 60; i++) { + if (throttle.attempt(from(`10.0.0.${i}`, 'victim')).allowed) admitted++; + flood(throttle, 3, 1000 + i * 3); + } + + expect(admitted).toBe(50); + }); + + it('can evict an unknown email, which then starts a fresh count: the cost is more than maxKeys attempts per check', () => { + const { throttle } = setup({ maxKeys, addressLimit: 1_000_000 }); + for (let i = 0; i < 5; i++) throttle.attempt(unknown('1.1.1.1', 'target')); + expect(throttle.attempt(unknown('1.1.1.1', 'target')).allowed).toBe(false); + + // Fewer than maxKeys others does not move it ... + flood(throttle, maxKeys - 2); + expect(throttle.attempt(unknown('1.1.1.1', 'target')).allowed).toBe(false); + // ... more than maxKeys does. + flood(throttle, maxKeys + 1, 100); + expect(throttle.attempt(unknown('1.1.1.1', 'target')).allowed).toBe(true); + }); + }); +}); diff --git a/src/lib/server/identity/throttle.ts b/src/lib/server/identity/throttle.ts index 1cc0c24c..ce035b76 100644 --- a/src/lib/server/identity/throttle.ts +++ b/src/lib/server/identity/throttle.ts @@ -3,11 +3,14 @@ import { isIP } from 'node:net'; export interface ThrottleInput { /** The client address as the server saw it; any spelling of one address shares a counter. */ address: string; + /** The id of the Account the sign-in names, or null when it names none. */ + accountId: string | null; /** - * Whom the sign-in names, as a key: an Account's id, or for an email that has no Account a digest of - * that email (the caller makes it, see `signin/sign-in.ts`). Null when it names nobody (a blank form). + * Set when `accountId` is null and the sign-in typed an email: a digest that stands for that email (the + * caller makes it, see `signin/sign-in.ts`). Such keys are the client's to choose, so they have counters + * of their own, apart from real Account ids. Ignored when `accountId` is set. */ - accountId: string | null; + unknownEmail?: string; } /** @@ -15,8 +18,8 @@ export interface ThrottleInput { * by callers. */ export interface ThrottleToken { - /** Whether the attempt named an Account; one that named none gives nothing back. */ - readonly named: boolean; + /** Which kind of key the attempt named, so `succeed` finds the same counters; null gives nothing back. */ + readonly named: 'account' | 'email' | null; readonly counted: { readonly address: CountedIn; readonly pair?: CountedIn; @@ -43,7 +46,7 @@ export interface ThrottleOptions { /** Attempts per window for one Account across all addresses. */ accountLimit?: number; windowMs?: number; - /** Cap on stored counters per map (address, account + address, account). */ + /** Cap on stored counters per map (five maps: see `createThrottle`). */ maxKeys?: number; now?: () => number; } @@ -119,14 +122,26 @@ interface Entry { * In-memory sign-in throttle, per process: counters reset on restart and are not shared between instances. * * Three counters: the address (20), the Account at an address (5, which the address counter caps at 20 per - * address) and the Account (50). The caller names the Account by a key (`accountId`): the id of the Account - * the email names, looked up the way Better Auth does, or, when the email has no Account, a digest of the - * email (`email:` + its SHA-256), so the real address is never held in memory. Known and unknown emails - * therefore reach the throttled state on the same schedule. Without that, only a known email would be - * throttled, and since a throttled refusal skips the password hash and answers faster, six tries and a - * stopwatch would tell an attacker which emails have an Account. The only key built from what the client - * typed is that digest, which is bounded in size and counted in the capped Account maps (below). A blank - * form names nobody (null) and only the address counter applies. + * address) and the Account (50). The caller looks the Account up the way Better Auth does and passes its + * id (`accountId`). A blank form names nobody (both null) and only the address counter applies. + * + * Emails that have no Account are limited too (`unknownEmail`, a digest of the normalised email, so the + * address itself is never held in memory), on the same schedule as a known one. Without that, only a + * known email would ever be throttled, and since a throttled refusal skips the password hash and answers + * faster, six tries and a stopwatch would tell an attacker which emails have an Account. + * + * The two kinds of key have separate maps, five in all (address; pair and Account for real ids; pair and + * Account for unknown emails), because they differ in who chooses the key. A real Account id exists only + * for a real Account, so its maps hold as many keys as there are Accounts and never fill in practice; they + * never drop a live entry (a full one skips the new key), which is what holds an Account to 5 tries per + * address and 50 overall however many addresses an attacker has. An unknown email is whatever the client + * types, so its maps are the ones a flood of made-up emails fills. When one is full it evicts the entry + * whose window ends earliest and never skips or refuses, so a flood can only evict other made-up emails: + * a known Account's counters stay. Residual: to make one unknown email forget its count, an attacker must + * push it out, which takes more than maxKeys other emails inside one window, so more than maxKeys / 20 + * addresses, for each fresh set of guesses. The address map evicts the same way, with the same residual: + * an attacker with more addresses than maxKeys inside one window gets fresh per-address budgets and is + * still held to the per-Account limit. * * A right password takes its own attempt back from the address and the Account counters (`succeed`), if * their window is still the one it was counted in, so only wrong passwords spend budget. Callers that @@ -136,15 +151,8 @@ interface Entry { * or the Account limit tells an attacker whether an email has an Account. The Account limit also lets * someone lock a known Account out for a window; that is accepted. * - * Each map is capped at maxKeys; expired entries are dropped first. Only the address map ever drops a - * live entry (the one whose window ends earliest), and only when it holds more distinct unexpired - * addresses than maxKeys. Residual: an attacker with more addresses than that inside one window gets - * fresh per-address budgets and is still held to the per-Account limit. The Account maps never drop a - * live entry (a full one skips storing the new key), and a full map never refuses an attempt. Because - * unknown emails are counted too, an attacker typing many different emails can fill the Account maps: new - * keys then go uncounted until entries expire, and the address limit still holds. Doing that takes more - * than maxKeys attempts inside one window, so more than maxKeys / 20 addresses. Worst case: 3 * maxKeys - * entries. + * Each map is capped at maxKeys, expired entries dropped first, and a full map never refuses an attempt. + * Worst case: 5 * maxKeys entries. */ export const createThrottle = ({ limit = 5, @@ -155,23 +163,43 @@ export const createThrottle = ({ now = Date.now }: ThrottleOptions = {}): Throttle => { // Insertion order is window-start order, since windows are fixed, so the first entry ends first. - const counter = (max: number) => ({ max, entries: new Map() }); - const addresses = counter(addressLimit); - const pairs = counter(limit); - const accounts = counter(accountLimit); + // `evicts`: a full map drops its earliest-ending entry for the new key; otherwise it skips the new key. + const counter = (max: number, evicts: boolean) => ({ + max, + evicts, + entries: new Map() + }); + const addresses = counter(addressLimit, true); + const pairs = counter(limit, false); + const accounts = counter(accountLimit, false); + const emailPairs = counter(limit, true); + const emailAccounts = counter(accountLimit, true); + const everyCounter = [addresses, pairs, accounts, emailPairs, emailAccounts]; type Counter = ReturnType; type Slot = 'address' | 'pair' | 'account'; - const slotsFor = ({ address, accountId }: ThrottleInput): [Slot, Counter, string][] => { + const slotsFor = ({ + address, + accountId, + unknownEmail + }: ThrottleInput): [Slot, Counter, string][] => { const where = normaliseAddress(address); const slots: [Slot, Counter, string][] = [['address', addresses, where]]; - // The address has no '|', so the key is unambiguous whatever the id holds. + // The address has no '|', so the key is unambiguous whatever the id or digest holds. if (accountId !== null) { slots.push(['pair', pairs, `${where}|${accountId}`], ['account', accounts, accountId]); + } else if (unknownEmail !== undefined) { + slots.push( + ['pair', emailPairs, `${where}|${unknownEmail}`], + ['account', emailAccounts, unknownEmail] + ); } return slots; }; + const countersOf = (named: 'account' | 'email') => + named === 'account' ? { pairs, accounts } : { pairs: emailPairs, accounts: emailAccounts }; + /** The entry `counted` names, only if it is still in the window the attempt was counted in. */ const sameWindow = ({ entries }: Counter, counted: CountedIn | undefined) => { const entry = counted && entries.get(counted.key); @@ -196,7 +224,7 @@ export const createThrottle = ({ return { attempt(input) { const at = now(); - for (const c of [addresses, pairs, accounts]) sweep(c, at); + for (const c of everyCounter) sweep(c, at); const slots = slotsFor(input); let retryAfterMs = 0; for (const [, c, id] of slots) { @@ -214,8 +242,8 @@ export const createThrottle = ({ continue; } if (c.entries.size >= maxKeys) { - // Only the address map may drop a live entry; Account maps keep theirs and skip the new key. - if (c !== addresses) continue; + // Only maps for keys the client chooses may drop a live entry; the rest keep theirs and skip the new key. + if (!c.evicts) continue; const earliest = c.entries.keys().next().value; if (earliest !== undefined) c.entries.delete(earliest); } @@ -227,7 +255,12 @@ export const createThrottle = ({ return { allowed: true, token: { - named: input.accountId !== null, + named: + input.accountId !== null + ? 'account' + : input.unknownEmail !== undefined + ? 'email' + : null, counted: { address: address as CountedIn, pair, account }, spent: false } @@ -235,14 +268,15 @@ export const createThrottle = ({ }, succeed(token) { const { address, pair, account } = token.counted; - // No Account was named, or this token already gave its count back. - if (token.spent || !token.named) return; + // No Account or email was named, or this token already gave its count back. + if (token.spent || token.named === null) return; token.spent = true; + const named = countersOf(token.named); // The pair counter is cleared, but only the window the right password belongs to. - if (pair && sameWindow(pairs, pair)) pairs.entries.delete(pair.key); + if (pair && sameWindow(named.pairs, pair)) named.pairs.entries.delete(pair.key); giveBack(addresses, address); - giveBack(accounts, account); + giveBack(named.accounts, account); }, - size: () => addresses.entries.size + pairs.entries.size + accounts.entries.size + size: () => everyCounter.reduce((total, c) => total + c.entries.size, 0) }; }; diff --git a/src/routes/setup/+page.server.ts b/src/routes/setup/+page.server.ts index ae199afe..df67d6b2 100644 --- a/src/routes/setup/+page.server.ts +++ b/src/routes/setup/+page.server.ts @@ -1,9 +1,14 @@ import { fail, redirect } from '@sveltejs/kit'; +import type { MultiUserChoice } from '$lib/server/app'; import { forwardSetCookies } from '$lib/server/identity/auth-handler'; import { InvalidFirstAdminError, MultiUserNotAvailableError } from '$lib/server/identity/setup'; import { InstanceAlreadySetUpError } from '$lib/server/instance'; import type { Actions, PageServerLoad } from './$types'; +/** Why Multi-user cannot be chosen here: what to add to `.env`. (`later` is never produced, see `MultiUserChoice`.) */ +const unavailable = (choice: Exclude) => + choice.kind === 'needs-env' ? choice.message : 'Multi-user is not available on this server.'; + /** The Instance module's refusal when someone set it up first (see instance.ts). */ const isAlreadySetUp = (error: unknown) => error instanceof Error && error.message.includes('already set up'); @@ -15,7 +20,6 @@ const FIELD_MESSAGES = { password: 'Use 12 to 128 characters.' } as const; const ALREADY_SET_UP = 'This instance has already been set up.'; -const LATER = 'Multi-user arrives in the next update.'; const text = (data: FormData, key: string) => { const value = data.get(key); return typeof value === 'string' ? value : ''; @@ -36,7 +40,7 @@ export const actions: Actions = { if (mode === 'multi') { const { multiUser } = locals.setup; if (multiUser.kind === 'open') return adminStep({ name: '', email: '' }); - return fail(400, { message: multiUser.kind === 'later' ? LATER : multiUser.message }); + return fail(400, { message: unavailable(multiUser) }); } if (mode !== 'local') return fail(400, { message: 'Choose a mode to continue.' }); try { @@ -61,7 +65,7 @@ export const actions: Actions = { const { multiUser } = locals.setup; if (multiUser.kind !== 'open') { - return fail(400, { message: multiUser.kind === 'later' ? LATER : multiUser.message }); + return fail(400, { message: unavailable(multiUser) }); } // The gate also lets /setup?convert through on a Local Instance, which anyone can reach: only an // Instance that is not set up may make its first admin here (converting Local has its own card). diff --git a/src/routes/setup/+page.svelte b/src/routes/setup/+page.svelte index 437be14a..48c771b5 100644 --- a/src/routes/setup/+page.svelte +++ b/src/routes/setup/+page.svelte @@ -107,9 +107,7 @@ const formError = $derived( Multi-user Several people, each with their own account. You create the admin account next. - {#if multi.kind === 'later'} - Multi-user arrives in the next update. - {:else if multi.kind === 'needs-env'} + {#if multi.kind === 'needs-env'} {multi.message} {/if} diff --git a/src/routes/setup/setup-action.test.ts b/src/routes/setup/setup-action.test.ts index bfe3f45a..b03c3763 100644 --- a/src/routes/setup/setup-action.test.ts +++ b/src/routes/setup/setup-action.test.ts @@ -45,7 +45,10 @@ const setupWith = ( const choose = ( mode: string, chooseLocal: () => Promise, - multiUser: MultiUserChoice = { kind: 'later' } + multiUser: MultiUserChoice = { + kind: 'needs-env', + message: 'Add BETTER_AUTH_SECRET to your .env.' + } ) => run(actions.mode, { mode }, { instance: { chooseLocal }, setup: setupWith(multiUser) }); describe('step 1: choose a mode', () => { @@ -73,18 +76,6 @@ describe('step 1: choose a mode', () => { ).rejects.toThrow('disk full'); }); - it('Multi-user is refused while it is a later release: nothing is set up', async () => { - const chooseLocal = vi.fn(async () => {}); - - const result = await choose('multi', chooseLocal, { kind: 'later' }); - - expect(result).toMatchObject({ - status: 400, - data: { message: 'Multi-user arrives in the next update.' } - }); - expect(chooseLocal).not.toHaveBeenCalled(); - }); - it('Multi-user without its secret or origin says what to add to .env', async () => { const message = 'Add BETTER_AUTH_SECRET to your .env.'; @@ -196,20 +187,25 @@ describe('step 2: create the admin', () => { } ); - it('refuses where Multi-user is not open, without calling createFirstAdmin', async () => { + it('refuses where Multi-user is not open, saying what to add to .env, without calling createFirstAdmin', async () => { const createFirstAdmin = vi.fn(); + const message = 'Add BETTER_AUTH_SECRET to your .env.'; const result = await run(actions.admin, admin, { - setup: setupWith({ kind: 'later' }, createFirstAdmin) + setup: setupWith({ kind: 'needs-env', message }, createFirstAdmin) }); - expect(result).toMatchObject({ - status: 400, - data: { message: 'Multi-user arrives in the next update.' } - }); + expect(result).toMatchObject({ status: 400, data: { message } }); expect(createFirstAdmin).not.toHaveBeenCalled(); }); + it('needs no test switch to offer Multi-user: choosing it goes on to the admin form wherever it is open', async () => { + // The Setup port says open or not; nothing in the action looks at an environment switch. + const result = await choose('multi', vi.fn(), { kind: 'open' }); + + expect(result).toMatchObject({ step: 'admin' }); + }); + it.each([ ['name', 'Enter a name.'], ['email', 'Enter a valid email address.'], @@ -305,7 +301,7 @@ describe('step 2: create the admin', () => { describe('load', () => { it('hands the page the Multi-user choice', () => { - const multiUser: MultiUserChoice = { kind: 'later' }; + const multiUser: MultiUserChoice = { kind: 'open' }; expect(load({ locals: { setup: setupWith(multiUser) } } as never)).toEqual({ multiUser }); }); diff --git a/src/routes/setup/setup.multi.e2e.ts b/src/routes/setup/setup.multi.e2e.ts index ba45a7d5..0e0985b1 100644 --- a/src/routes/setup/setup.multi.e2e.ts +++ b/src/routes/setup/setup.multi.e2e.ts @@ -62,6 +62,20 @@ test('a fresh Instance shows Setup everywhere; Multi-user needs the admin form, await expect(page).toHaveURL('/'); await page.reload(); await expect(page.getByRole('navigation', { name: 'Primary' })).toContainText(E2E_ADMIN.name); + + // Sign out from the nav: back to Sign in, and the Map is out of reach until signing in again. + await page + .getByRole('navigation', { name: 'Primary' }) + .getByRole('button', { name: 'Sign out' }) + .click(); + await expect(page).toHaveURL('/signin'); + await page.goto('/'); + await expect(page).toHaveURL('/signin?next=%2F'); + await page.getByLabel('Email').fill(E2E_ADMIN.email); + await page.getByLabel('Password').fill(E2E_ADMIN.password); + await page.getByRole('button', { name: 'Sign in →' }).click(); + await expect(page).toHaveURL('/'); + await expect(page.getByRole('navigation', { name: 'Primary' })).toContainText(E2E_ADMIN.name); }); test('once set up, a visitor without the admin session is sent to Sign in, test cookie or not', async ({ @@ -80,26 +94,28 @@ test('once set up, a visitor without the admin session is sent to Sign in, test await context.close(); }); -test('Better Auth answers over HTTP on this server: the right password signs in, a wrong one does not, and nothing can be framed', async ({ +test('Better Auth answers over HTTP on this server, but not to a password sign-in, and nothing can be framed', async ({ request }) => { const headers = { origin: 'http://localhost:4174' }; - const signIn = (password: string) => - request.post('/api/auth/sign-in/email', { - headers, - data: { email: E2E_ADMIN.email, password } - }); - - const wrong = await signIn('not the password at all'); - expect(wrong.status()).toBe(401); - - const right = await signIn(E2E_ADMIN.password); - expect(right.status()).toBe(200); - expect(right.headers()['set-cookie']).toContain('better-auth.session_token='); - expect(right.headers()['x-frame-options']).toBe('DENY'); - expect(right.headers()['content-security-policy']).toBe("frame-ancestors 'none'"); - - // That session is the admin: the Map answers for it. + + // Signing in with a password is the Sign in form's alone (it carries the per-Account limits). + const signIn = await request.post('/api/auth/sign-in/email', { + headers, + data: { email: E2E_ADMIN.email, password: E2E_ADMIN.password } + }); + expect(signIn.status()).toBe(404); + expect(signIn.headers()['set-cookie']).toBeUndefined(); + + // The rest of its API is served, and every answer refuses framing. No session, so no one is signed in. + const session = await request.get('/api/auth/get-session', { headers }); + expect(session.status()).toBe(200); + expect(await session.json()).toBeNull(); + expect(session.headers()['x-frame-options']).toBe('DENY'); + expect(session.headers()['content-security-policy']).toBe("frame-ancestors 'none'"); + + // And without a session the Map answers with a redirect to Sign in, never a page. const map = await request.get('/', { maxRedirects: 0 }); - expect(map.status()).toBe(200); + expect(map.status()).toBe(303); + expect(map.headers().location).toBe('/signin?next=%2F'); }); diff --git a/src/routes/signin/fresh-server.ts b/src/routes/signin/fresh-server.ts new file mode 100644 index 00000000..1af72270 --- /dev/null +++ b/src/routes/signin/fresh-server.ts @@ -0,0 +1,89 @@ +import { type ChildProcess, execFileSync, spawn } from 'node:child_process'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { createServer } from 'node:net'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +// Test-only (imported by the browser tests, never by the app): a second server from the same build, on a +// free port and a fresh database, started the way a released Instance is, with no test switch and no +// scripted Runner. Its Instance is not set up, so a test can walk Setup for real. It has BETTER_AUTH_SECRET +// and ORIGIN, the two things Multi-user needs from the operator. + +export type FreshServer = { url: string; stop(): Promise }; + +export async function startFreshServer(): Promise { + const dir = mkdtempSync(join(tmpdir(), 'beyondleetcode-fresh-')); + const port = await freePort(); + const url = `http://localhost:${port}`; + const env: NodeJS.ProcessEnv = { + ...process.env, + DATABASE_URL: `file:${join(dir, 'fresh.db')}`, + CONTENT_DIR: 'src/lib/server/content/fixture', + ORIGIN: url, + PORT: String(port), + BETTER_AUTH_SECRET: 'e2e-only-secret-e2e-only-secret-e2e-only-0123', + // The Runner is never called by these pages. + RUNNER_URL: 'http://127.0.0.1:9', + RUNNER_TOKEN: 'e2e-unused' + }; + delete env.IDENTITY_TEST_LEARNER; + delete env.RUNNER_ADAPTER; + + execFileSync('node_modules/.bin/prisma', ['migrate', 'deploy'], { env, stdio: 'pipe' }); + const server = spawn('bun', ['./build/index.js'], { env, stdio: ['ignore', 'pipe', 'pipe'] }); + let output = ''; + server.stdout?.on('data', (chunk) => { + output += chunk; + }); + server.stderr?.on('data', (chunk) => { + output += chunk; + }); + const stopAll = async () => { + await stop(server); + rmSync(dir, { recursive: true, force: true }); + }; + try { + await ready(server, `${url}/setup`, () => output); + } catch (error) { + await stopAll(); + throw error; + } + return { url, stop: stopAll }; +} + +async function freePort(): Promise { + return new Promise((resolve, reject) => { + const probe = createServer(); + probe.once('error', reject); + probe.listen(0, '127.0.0.1', () => { + const { port } = probe.address() as { port: number }; + probe.close(() => resolve(port)); + }); + }); +} + +async function ready(server: ChildProcess, url: string, output: () => string) { + const stopped = () => server.exitCode !== null || server.signalCode !== null; + for (let i = 0; i < 150; i++) { + if (stopped()) throw new Error(`the fresh server exited:\n${output()}`); + try { + if ((await fetch(url)).ok) return; + } catch { + // not listening yet + } + await new Promise((resolve) => setTimeout(resolve, 200)); + } + throw new Error(`the fresh server did not answer in time:\n${output()}`); +} + +async function stop(server: ChildProcess) { + if (server.exitCode !== null || server.signalCode !== null) return; + const exited = new Promise((resolve) => server.once('exit', resolve)); + server.kill('SIGTERM'); + const timedOut = Symbol('timeout'); + const result = await Promise.race([ + exited, + new Promise((resolve) => setTimeout(() => resolve(timedOut), 5_000)) + ]); + if (result === timedOut) server.kill('SIGKILL'); +} diff --git a/src/routes/signin/multi-user-flow.test.ts b/src/routes/signin/multi-user-flow.test.ts new file mode 100644 index 00000000..38c4476a --- /dev/null +++ b/src/routes/signin/multi-user-flow.test.ts @@ -0,0 +1,118 @@ +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { isRedirect } from '@sveltejs/kit'; +import { expect, it, onTestFinished } from 'vitest'; +import { createApp } from '$lib/server/app'; +import { createHandle } from '$lib/server/handle'; +import { createThrottle } from '$lib/server/identity/throttle'; +import { actions as settingsActions } from '../settings/+page.server'; +import { createSignIn } from './sign-in'; + +// The whole path a released Instance takes, with no test switch anywhere: Setup offers Multi-user, the first +// admin is created, signs out and signs back in through the Sign in action, and the hook sees who they are. + +const secret = 'a-test-secret-of-at-least-thirty-two-characters'; +const admin = { name: 'Ada', email: 'ada@example.com', password: 'correct horse battery' }; + +const cookieHeader = (setCookies: string[]) => setCookies.map((c) => c.split(';')[0]).join('; '); + +it('a fresh Instance without the test switch: choose Multi-user, create the admin, sign out, sign back in', async () => { + const dir = mkdtempSync(join(tmpdir(), 'beyondleetcode-flow-')); + onTestFinished(() => rmSync(dir, { recursive: true, force: true })); + const url = `file:${join(dir, 'app.db')}`; + execFileSync('node_modules/.bin/prisma', ['migrate', 'deploy'], { + env: { ...process.env, DATABASE_URL: url }, + stdio: 'pipe' + }); + // No IDENTITY_TEST_LEARNER and no scripted Runner: the environment of a released Instance. + const app = await createApp({ + CONTENT_DIR: 'src/lib/server/content/fixture', + DATABASE_URL: url, + RUNNER_URL: 'http://127.0.0.1:1', + RUNNER_TOKEN: 'test-token', + BETTER_AUTH_SECRET: secret, + ORIGIN: 'http://localhost:3000' + }); + expect(app.setup.multiUser).toEqual({ kind: 'open' }); + + // 1. Setup creates the admin and signs them in. + const created = await app.setup.createFirstAdmin(admin); + expect(app.instance.mode()).toBe('multi'); + const first = cookieHeader(created.headers.getSetCookie()); + + // 2. The hook knows them; with no cookie it sends the visitor to Sign in. + const handle = createHandle(app); + const visit = async (path: string, routeId: string, cookie: string, method = 'GET') => { + const resolved: unknown[] = []; + const event = { + url: new URL(path, 'http://localhost:3000'), + route: { id: routeId }, + request: { method, headers: new Headers(cookie ? { cookie } : {}) }, + cookies: { get: () => undefined }, + locals: {} as Record + }; + const response = await handle({ + event: event as never, + resolve: async (e) => { + resolved.push(e); + return new Response('page'); + } + }); + return { response, locals: event.locals, rendered: resolved.length > 0 }; + }; + const signedIn = await visit('/', '/', first); + expect(signedIn.rendered).toBe(true); + expect(signedIn.locals.identity).toMatchObject({ displayName: 'Ada', role: 'admin' }); + const stranger = await visit('/topics/stacks', '/topics/[topicId]', ''); + expect(stranger.response.headers.get('location')).toBe('/signin?next=%2Ftopics%2Fstacks'); + + // 3. Sign out: the Settings action, with their cookie. + const jar = new Map(); + const cookies = { set: (name: string, value: string) => void jar.set(name, value) }; + const outcome = async (run: () => unknown) => { + try { + return await run(); + } catch (thrown) { + if (isRedirect(thrown)) return thrown; + throw thrown; + } + }; + const out = await outcome(() => + settingsActions.signout({ + request: { headers: new Headers({ cookie: first }) }, + locals: { auth: app.multiUser?.auth }, + cookies + } as never) + ); + expect(out).toMatchObject({ status: 303, location: '/signin' }); + expect(jar.get('better-auth.session_token')).toBe(''); + // The cookie they had is nobody now. + expect((await visit('/', '/', first)).response.headers.get('location')).toBe('/signin?next=%2F'); + + // 4. Sign back in through the form, to the page they asked for. + const signIn = createSignIn(createThrottle()); + jar.clear(); + const form = new FormData(); + form.set('email', admin.email); + form.set('password', admin.password); + const back = await outcome(() => + signIn.action({ + request: { formData: async () => form }, + locals: { auth: app.multiUser?.auth, instance: app.instance }, + cookies, + url: new URL('http://localhost:3000/signin?next=%2Ftopics%2Fstacks'), + getClientAddress: () => '203.0.113.7' + } as never) + ); + expect(back).toMatchObject({ status: 303, location: '/topics/stacks' }); + const second = [...jar].map(([name, value]) => `${name}=${encodeURIComponent(value)}`).join('; '); + const again = await visit('/topics/stacks', '/topics/[topicId]', second); + expect(again.rendered).toBe(true); + expect(again.locals.identity).toMatchObject({ + displayName: 'Ada', + role: 'admin', + learnerId: 'local' + }); +}, 60_000); diff --git a/src/routes/signin/sign-in.test.ts b/src/routes/signin/sign-in.test.ts index 49023346..1ed6bdb2 100644 --- a/src/routes/signin/sign-in.test.ts +++ b/src/routes/signin/sign-in.test.ts @@ -162,6 +162,19 @@ describe('the page', () => { expect(harness({ registration: 'invite' }).load()).toEqual({ open: false }); }); + it('reads the Registration Mode on every request: closing sign-up takes the link away at once', () => { + let mode: 'open' | 'invite' = 'open'; + const { signIn } = harness(); + const locals = { instance: { registrationMode: () => mode } }; + const url = new URL('http://localhost/signin'); + + expect(signIn.load({ locals, url } as never)).toEqual({ open: true }); + mode = 'invite'; + expect(signIn.load({ locals, url } as never)).toEqual({ open: false }); + mode = 'open'; + expect(signIn.load({ locals, url } as never)).toEqual({ open: true }); + }); + it('sends someone who is already signed in to the page they asked for, or the Map', () => { expect(harness({ identity: { learnerId: 'l1' }, next: '/topics/stacks' }).load()).toMatchObject( { status: 303, location: '/topics/stacks' } @@ -455,7 +468,8 @@ describe('the sign-in limits', () => { expect(inputs[0]).toEqual({ address: '203.0.113.9', accountId: 'user-ada' }); expect(inputs[1]).toEqual({ address: '203.0.113.9', - accountId: `email:${createHash('sha256').update('nobody@example.com').digest('hex')}` + accountId: null, + unknownEmail: createHash('sha256').update('nobody@example.com').digest('hex') }); expect(inputs[2]).toEqual({ address: '203.0.113.9', accountId: null }); }); @@ -469,7 +483,7 @@ describe('the sign-in limits', () => { await post({ email: 'NOBODY@example.com', password: 'x' }); await post({ email: ' nobody@example.com', password: 'x' }); - const keys = attempt.mock.calls.map(([input]) => input.accountId); + const keys = attempt.mock.calls.map(([input]) => input.unknownEmail); expect(JSON.stringify(keys)).not.toContain('nobody@'); // Lowercased like Better Auth's lookup, so one spelling cannot escape the other's count; not trimmed. expect(keys[0]).toBe(keys[1]); @@ -491,6 +505,18 @@ describe('the sign-in limits', () => { } }); + it('keeps a known Account limited while a flood of made-up emails fills the unknown-email counters', async () => { + const throttle = createThrottle({ maxKeys: 20, addressLimit: 1_000_000 }); + const { post, signInEmail } = harness({ throttle }); + for (let i = 0; i < 200; i++) await post({ email: `made-up-${i}@example.com`, password: 'x' }); + signInEmail.mockClear(); + + for (let i = 0; i < 6; i++) await post(wrongPassword); + + // Five were asked and wrong; the sixth was refused before the password was looked at. + expect(signInEmail).toHaveBeenCalledTimes(5); + }); + it('limits an unknown email across many addresses at 50, like a known one', async () => { const throttle = createThrottle(); const unknown = { email: 'nobody@example.com', password: 'x' }; diff --git a/src/routes/signin/sign-in.ts b/src/routes/signin/sign-in.ts index ce4391be..00476085 100644 --- a/src/routes/signin/sign-in.ts +++ b/src/routes/signin/sign-in.ts @@ -3,7 +3,7 @@ import { fail, type RequestEvent, redirect } from '@sveltejs/kit'; import { isAPIError } from 'better-auth/api'; import { forwardSetCookies } from '$lib/server/identity/auth-handler'; import { safeNext } from '$lib/server/identity/gate'; -import type { Throttle } from '$lib/server/identity/throttle'; +import type { Throttle, ThrottleInput } from '$lib/server/identity/throttle'; /** * Fixed texts only: an error's own message (Prisma's carry paths and query text) never reaches the form. @@ -64,14 +64,15 @@ function clientAddress(event: Pick): string { } /** - * The limits' key for an email that has no Account: a digest of the normalised email (lowercased like - * Better Auth's lookup, not trimmed), so the email itself is never held in memory. Known and unknown - * emails then reach the throttled state on the same schedule; if only known ones did, a throttled - * refusal (which skips the password hash, so it answers faster) would tell an attacker which emails - * have an Account. User ids hold no ':', so the two kinds of key never meet. + * The limits' stand-in for an email that has no Account: a digest of the normalised email (lowercased like + * Better Auth's lookup, not trimmed), so the email itself is never held in memory. Known and unknown emails + * then reach the throttled state on the same schedule; if only known ones did, a throttled refusal (which + * skips the password hash, so it answers faster) would tell an attacker which emails have an Account. It + * goes in its own field, not in `accountId`: the throttle keeps these keys, which the client chooses, apart + * from real Account ids (`throttle.ts`). */ -const unknownAccountKey = (email: string) => - `email:${createHash('sha256').update(email.toLowerCase()).digest('hex')}`; +const emailDigest = (email: string) => + createHash('sha256').update(email.toLowerCase()).digest('hex'); /** The kind of failure only: the error text may hold paths, query text and the input. */ function logFailure(what: string, error: unknown) { @@ -118,13 +119,15 @@ export function createSignIn(throttle: Throttle) { // The Account Better Auth would sign in: looked up the way it does (lowercased, not trimmed), so the // Account limits cannot be dodged by how the email is spelled. With none, the email's digest stands - // in for its id, so an unknown email is limited exactly like a known one. A blank form names nobody. - let accountId: string | null = null; + // in, so an unknown email is limited exactly like a known one. A blank form names nobody. + let named: Pick = { accountId: null }; if (email !== '') { try { const { internalAdapter } = await auth.$context; const found = await internalAdapter.findUserByEmail(email.toLowerCase()); - accountId = found?.user.id ?? unknownAccountKey(email); + named = found + ? { accountId: found.user.id } + : { accountId: null, unknownEmail: emailDigest(email) }; } catch (error) { logFailure('looking up the account failed', error); return refused(500, email, MESSAGES.failed); @@ -132,7 +135,7 @@ export function createSignIn(throttle: Throttle) { } // Before the password is looked at, so a flood of guesses of any kind is counted and none is free. - const turn = throttle.attempt({ address: clientAddress(event), accountId }); + const turn = throttle.attempt({ address: clientAddress(event), ...named }); if (!turn.allowed) return wrongSignIn(email); if (email === '' || password === '') return wrongSignIn(email); diff --git a/src/routes/signin/signin.multi.e2e.ts b/src/routes/signin/signin.multi.e2e.ts new file mode 100644 index 00000000..6438a395 --- /dev/null +++ b/src/routes/signin/signin.multi.e2e.ts @@ -0,0 +1,267 @@ +import { expect, type Page, test } from '@playwright/test'; +import { E2E_ADMIN } from '../../lib/e2e-admin.ts'; +import { LEARNER_PASSWORD, openMultiUserDatabase } from '../users/multi-fixture.ts'; +import { startFreshServer } from './fresh-server.ts'; + +// Sign in and Sign out on the Multi-user server (the "e2e-multi" project, after "setup-multi" made the admin). +// Learners are made through multi-fixture.ts; each test uses its own, so the limits one test spends +// (five wrong passwords lock an Account out of one address for ten minutes) never reach another. +// +// Sign in carries a shared budget per address: twenty wrong passwords from this machine in ten minutes, across +// every Account. This file spends six in all; keep the total across files well under twenty. + +const NAV = 'Primary'; +const WRONG = 'Email or password is wrong.'; + +const people = { + asker: { name: 'Ana Asker', email: 'ana.signin@example.com' }, + settings: { name: 'Sol Settings', email: 'sol.signin@example.com' }, + locked: { name: 'Lou Locked', email: 'lou.signin@example.com' }, + hostile: { name: 'Hal Hostile', email: 'hal.signin@example.com' }, + typo: { name: 'Tia Typo', email: 'tia.signin@example.com' }, + phone: { name: 'Pat Phone', email: 'pat.signin@example.com' } +}; + +let fixture: Awaited>; + +test.beforeAll(async () => { + fixture = await openMultiUserDatabase(); + for (const person of Object.values(people)) await fixture.ensureLearner(person); +}); + +test.afterAll(async () => { + await fixture.close(); +}); + +async function signInWith(page: Page, email: string, password: string) { + await page.getByLabel('Email').fill(email); + await page.getByLabel('Password').fill(password); + await page.getByRole('button', { name: 'Sign in →' }).click(); +} + +test.describe('a signed-out visitor', () => { + test('lands on Sign in from any page and, once signed in, on the page they asked for', async ({ + page + }) => { + for (const path of ['/', '/topics/stacks', '/settings', '/users', '/no/such/page']) { + await page.goto(path); + await expect(page, path).toHaveURL(`/signin?next=${encodeURIComponent(path)}`); + } + + await page.goto('/topics/stacks'); + await expect(page.getByRole('heading', { level: 1 })).toHaveText('Welcome back.'); + await signInWith(page, people.asker.email, LEARNER_PASSWORD); + + await expect(page).toHaveURL('/topics/stacks'); + await expect(page.getByRole('navigation', { name: NAV })).toContainText(people.asker.name); + }); + + test('sees the board: wordmark alone in the header, no Map or name link, and no sign-up link on an invite-only Instance', async ({ + page + }) => { + await page.goto('/signin'); + + await expect(page.getByRole('navigation', { name: NAV })).toHaveCount(0); + await expect(page.getByRole('link', { name: 'Beyond Leetcode' })).toBeVisible(); + await expect(page.getByLabel('Email')).toBeVisible(); + await expect(page.getByLabel('Password')).toBeVisible(); + await expect(page.getByText('Forgot password? Ask your admin.')).toBeVisible(); + // This Instance is invite-only (every new one is), so the page does not point at a closed form. + await expect(page.getByRole('link', { name: 'Create an account' })).toHaveCount(0); + await page.goto('/signup'); + await expect(page.getByRole('navigation', { name: NAV })).toHaveCount(0); + }); + + test('is never sent off the site by a hostile next', async ({ page }) => { + await page.goto(`/signin?next=${encodeURIComponent('//evil.example/phish')}`); + + await signInWith(page, people.hostile.email, LEARNER_PASSWORD); + + await expect(page).toHaveURL('/'); + }); + + test('is told one thing for a wrong password and for no such account, keeping the email but not the password', async ({ + page, + browser + }) => { + await page.goto('/signin'); + await signInWith(page, people.typo.email, 'not the password at all'); + + await expect(page).toHaveURL(/\/signin$/); + await expect(page.getByText(WRONG)).toBeVisible(); + await expect(page.getByLabel('Email')).toHaveValue(people.typo.email); + await expect(page.getByLabel('Password')).toHaveValue(''); + await expect(page.getByLabel('Email')).toHaveAttribute('aria-invalid', 'true'); + await expect(page.getByLabel('Password')).toHaveAttribute('aria-invalid', 'true'); + // The right password then works from that very form. + await page.getByLabel('Password').fill(LEARNER_PASSWORD); + await page.getByRole('button', { name: 'Sign in →' }).click(); + await expect(page).toHaveURL('/'); + + // No Account at all: the same words, the same fields marked. + const stranger = await browser.newContext({ baseURL: 'http://localhost:4174' }); + const other = await stranger.newPage(); + await other.goto('/signin'); + await signInWith(other, 'nobody.signin@example.com', 'whatever whatever'); + await expect(other.getByText(WRONG)).toBeVisible(); + await expect(other.getByLabel('Email')).toHaveAttribute('aria-invalid', 'true'); + await stranger.close(); + }); + + test('is refused on the sixth wrong password, with the same message, even with the right one', async ({ + page + }) => { + await page.goto('/signin'); + for (let attempt = 1; attempt <= 5; attempt++) { + await signInWith(page, people.locked.email, `wrong password number ${attempt}`); + await expect(page.getByText(WRONG), `wrong password ${attempt}`).toBeVisible(); + } + + // The sixth is the right password, and it is still refused, in the same words. + await signInWith(page, people.locked.email, LEARNER_PASSWORD); + + await expect(page.getByText(WRONG)).toBeVisible(); + await expect(page).toHaveURL(/\/signin$/); + await expect(page.getByRole('navigation', { name: NAV })).toHaveCount(0); + }); + + test('cannot sign in over HTTP, only through the form', async ({ request }) => { + const response = await request.post('/api/auth/sign-in/email', { + headers: { origin: 'http://localhost:4174' }, + data: { email: people.asker.email, password: LEARNER_PASSWORD } + }); + + expect(response.status()).toBe(404); + expect(response.headers()['set-cookie']).toBeUndefined(); + }); +}); + +test.describe('a signed-in user', () => { + test('signs out from the nav and is back at Sign in, with the old page no longer theirs', async ({ + page + }) => { + await page.goto('/signin'); + await signInWith(page, people.asker.email, LEARNER_PASSWORD); + const nav = page.getByRole('navigation', { name: NAV }); + await expect(nav.getByRole('link')).toHaveText(['Map', people.asker.name]); + await expect(nav.getByRole('button')).toHaveText(['Sign out']); + + await nav.getByRole('button', { name: 'Sign out' }).click(); + + await expect(page).toHaveURL('/signin'); + await expect(page.getByRole('heading', { level: 1 })).toHaveText('Welcome back.'); + await page.goto('/topics/stacks'); + await expect(page).toHaveURL('/signin?next=%2Ftopics%2Fstacks'); + expect((await page.context().cookies()).map((c) => c.name)).not.toContain( + 'better-auth.session_token' + ); + }); + + test('signs out from Settings too, and Sign out is not a link: a plain GET changes nothing', async ({ + page + }) => { + await page.goto('/signin'); + await signInWith(page, people.settings.email, LEARNER_PASSWORD); + await page.goto('/settings'); + await expect(page.getByRole('heading', { name: 'Session' })).toBeVisible(); + + // A GET to the action's URL is just the Settings page, and still signed in. + await page.goto('/settings?/signout'); + await expect(page.getByRole('heading', { level: 1 })).toHaveText('Settings'); + await expect(page.getByRole('navigation', { name: NAV })).toContainText(people.settings.name); + + await page.getByRole('main').getByRole('button', { name: 'Sign out' }).click(); + + await expect(page).toHaveURL('/signin'); + await page.goto('/settings'); + await expect(page).toHaveURL('/signin?next=%2Fsettings'); + }); + + test('sees the 404 page for a path with no route, signed in', async ({ page }) => { + await page.goto('/signin'); + await signInWith(page, people.asker.email, LEARNER_PASSWORD); + + const response = await page.goto('/no/such/page'); + + expect(response?.status()).toBe(404); + await expect(page).toHaveURL('/no/such/page'); + }); + + test('has Sign out only on Settings on a phone', async ({ page }) => { + await page.goto('/signin'); + await signInWith(page, people.phone.email, LEARNER_PASSWORD); + await page.setViewportSize({ width: 390, height: 844 }); + + await page.goto('/'); + await expect(page.getByRole('navigation', { name: NAV })).toContainText(people.phone.name); + await expect( + page.getByRole('navigation', { name: NAV }).getByRole('button', { name: 'Sign out' }) + ).toBeHidden(); + + await page.goto('/settings'); + await page.getByRole('main').getByRole('button', { name: 'Sign out' }).click(); + await expect(page).toHaveURL('/signin'); + }); + + test('is sent from Sign in to where they were going, not shown the form', async ({ page }) => { + await page.goto('/signin'); + await signInWith(page, people.asker.email, LEARNER_PASSWORD); + + await page.goto(`/signin?next=${encodeURIComponent('/topics/stacks')}`); + + await expect(page).toHaveURL('/topics/stacks'); + }); +}); + +test('the admin has Users, their name and Sign out in the nav, in that order', async ({ page }) => { + await page.goto('/signin'); + await signInWith(page, E2E_ADMIN.email, E2E_ADMIN.password); + + const nav = page.getByRole('navigation', { name: NAV }); + await expect(nav.getByRole('link')).toHaveText(['Map', 'Users', E2E_ADMIN.name]); + await expect(nav.getByRole('button')).toHaveText(['Sign out']); + await nav.getByRole('button', { name: 'Sign out' }).click(); + await expect(page).toHaveURL('/signin'); +}); + +test.describe('a fresh Instance started like a released one, with no test switch', () => { + // Its own server, from the same build, on a fresh database: Setup offers Multi-user because the operator + // gave it a secret and an origin, and nothing else is needed. + test('lets the first visitor choose Multi-user, create the admin, sign out and sign back in', async ({ + browser + }) => { + test.setTimeout(120_000); + const server = await startFreshServer(); + try { + const context = await browser.newContext({ baseURL: server.url }); + const page = await context.newPage(); + + await page.goto('/'); + await expect(page).toHaveURL('/setup'); + await expect(page.getByLabel('Multi-user')).toBeEnabled(); + await expect(page.getByText(/next update/)).toHaveCount(0); + await page.getByLabel('Multi-user').check(); + await page.getByRole('button', { name: 'Continue →' }).click(); + await page.getByLabel('Name').fill('Fresh Admin'); + await page.getByLabel('Email').fill('fresh.admin@example.com'); + await page.getByLabel('Password').fill('a long enough password'); + await page.getByRole('button', { name: 'Create admin account →' }).click(); + await expect(page).toHaveURL('/'); + const nav = page.getByRole('navigation', { name: NAV }); + await expect(nav).toContainText('Fresh Admin'); + + await nav.getByRole('button', { name: 'Sign out' }).click(); + await expect(page).toHaveURL('/signin'); + await page.goto('/topics/stacks'); + await expect(page).toHaveURL('/signin?next=%2Ftopics%2Fstacks'); + + await signInWith(page, 'fresh.admin@example.com', 'a long enough password'); + + await expect(page).toHaveURL('/topics/stacks'); + await expect(page.getByRole('navigation', { name: NAV })).toContainText('Fresh Admin'); + await context.close(); + } finally { + await server.stop(); + } + }); +}); From 7da492e8614d9ed91b47521848018e4fee50790a Mon Sep 17 00:00:00 2001 From: Gauransh Mathur Date: Fri, 16 Oct 2026 10:41:47 +0000 Subject: [PATCH 4/7] feat(beyondleetcode): C32 own servers for the Sign in browser tests, next safe for a Location header Co-Authored-By: Claude Sonnet 5.5 --- src/routes/signin/fresh-server.ts | 25 +- src/routes/signin/sign-in.test.ts | 7 + src/routes/signin/sign-in.ts | 6 +- src/routes/signin/signin.multi.e2e.ts | 426 +++++++++++++++----------- src/routes/signup/signup.multi.e2e.ts | 12 +- 5 files changed, 289 insertions(+), 187 deletions(-) diff --git a/src/routes/signin/fresh-server.ts b/src/routes/signin/fresh-server.ts index 1af72270..c549c705 100644 --- a/src/routes/signin/fresh-server.ts +++ b/src/routes/signin/fresh-server.ts @@ -3,15 +3,23 @@ import { mkdtempSync, rmSync } from 'node:fs'; import { createServer } from 'node:net'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; +import { createDb } from '../../lib/server/db.ts'; // Test-only (imported by the browser tests, never by the app): a second server from the same build, on a // free port and a fresh database, started the way a released Instance is, with no test switch and no -// scripted Runner. Its Instance is not set up, so a test can walk Setup for real. It has BETTER_AUTH_SECRET -// and ORIGIN, the two things Multi-user needs from the operator. +// scripted Runner. By default its Instance is not set up, so a test can walk Setup for real. It has +// BETTER_AUTH_SECRET and ORIGIN, the two things Multi-user needs from the operator. +// +// A test that needs Accounts without touching the shared Multi-user server's database (two processes writing +// one SQLite file made the other files' setup time out) asks for `openRegistration`: the Instance is then +// already Multi-user with Registration Open, as a finished Setup plus the Users toggle (C34b) leave it, and +// Accounts are made through the Sign up form. The app gains no way to open registration from this. export type FreshServer = { url: string; stop(): Promise }; -export async function startFreshServer(): Promise { +export async function startFreshServer( + options: { openRegistration?: boolean } = {} +): Promise { const dir = mkdtempSync(join(tmpdir(), 'beyondleetcode-fresh-')); const port = await freePort(); const url = `http://localhost:${port}`; @@ -30,6 +38,15 @@ export async function startFreshServer(): Promise { delete env.RUNNER_ADAPTER; execFileSync('node_modules/.bin/prisma', ['migrate', 'deploy'], { env, stdio: 'pipe' }); + if (options.openRegistration) { + // Before the server starts, so nothing else holds the file. + const db = createDb(env.DATABASE_URL as string); + try { + await db.instance.create({ data: { mode: 'multi', registrationMode: 'open' } }); + } finally { + await db.$disconnect(); + } + } const server = spawn('bun', ['./build/index.js'], { env, stdio: ['ignore', 'pipe', 'pipe'] }); let output = ''; server.stdout?.on('data', (chunk) => { @@ -43,7 +60,7 @@ export async function startFreshServer(): Promise { rmSync(dir, { recursive: true, force: true }); }; try { - await ready(server, `${url}/setup`, () => output); + await ready(server, `${url}/${options.openRegistration ? 'signup' : 'setup'}`, () => output); } catch (error) { await stopAll(); throw error; diff --git a/src/routes/signin/sign-in.test.ts b/src/routes/signin/sign-in.test.ts index 1ed6bdb2..81a8ae03 100644 --- a/src/routes/signin/sign-in.test.ts +++ b/src/routes/signin/sign-in.test.ts @@ -107,6 +107,9 @@ describe('landing: where a sign-in goes', () => { it.each([ ['/topics/stacks', '/topics/stacks'], ['/problems/p?tab=hints#top', '/problems/p?tab=hints#top'], + ['/topics/%E6%97%A5', '/topics/%E6%97%A5'], + ['/topics/日本?q=é', '/topics/%E6%97%A5%E6%9C%AC?q=%C3%A9'], + ['/ok ', '/ok%20'], [null, '/'], ['', '/'], ['/', '/'] @@ -148,6 +151,10 @@ describe('landing: where a sign-in goes', () => { expect(landing(next)).toBe('/'); }); + it('answers with text a Location header can hold, even for a lone surrogate', () => { + expect(landing('/\ud800')).toBe('/'); + }); + it('keeps a page whose name merely starts with signin', () => { expect(landing('/signing-off')).toBe('/signing-off'); }); diff --git a/src/routes/signin/sign-in.ts b/src/routes/signin/sign-in.ts index 00476085..43f1fe04 100644 --- a/src/routes/signin/sign-in.ts +++ b/src/routes/signin/sign-in.ts @@ -44,14 +44,18 @@ const AUTH_PAGES = new Set(['/signin', '/signup']); export function landing(next: string | null): string { const target = safeNext(next); let pathname: string; + let encoded: string; try { // Decoded, because SvelteKit matches `/%73ignin` to the same route as `/signin`. pathname = decodeURIComponent(new URL(target, 'http://localhost').pathname); + // A Location header holds ASCII: whatever else was typed (non-Latin letters, spaces) goes as %XX, so a + // hand-made `?next=/日本` is a redirect and not a crash. Already-encoded text is left as it is. + encoded = target.replace(/[^\x21-\x7e]/gu, encodeURIComponent); } catch { return '/'; } const page = pathname.length > 1 ? pathname.replace(/\/+$/, '').toLowerCase() : pathname; - return AUTH_PAGES.has(page) ? '/' : target; + return AUTH_PAGES.has(page) ? '/' : encoded; } /** The caller's address, or '' when the server cannot tell: the throttle keeps those in one shared bucket. */ diff --git a/src/routes/signin/signin.multi.e2e.ts b/src/routes/signin/signin.multi.e2e.ts index 6438a395..a1394cee 100644 --- a/src/routes/signin/signin.multi.e2e.ts +++ b/src/routes/signin/signin.multi.e2e.ts @@ -1,17 +1,24 @@ -import { expect, type Page, test } from '@playwright/test'; +import { test as base, expect, type Page } from '@playwright/test'; import { E2E_ADMIN } from '../../lib/e2e-admin.ts'; -import { LEARNER_PASSWORD, openMultiUserDatabase } from '../users/multi-fixture.ts'; -import { startFreshServer } from './fresh-server.ts'; - -// Sign in and Sign out on the Multi-user server (the "e2e-multi" project, after "setup-multi" made the admin). -// Learners are made through multi-fixture.ts; each test uses its own, so the limits one test spends -// (five wrong passwords lock an Account out of one address for ten minutes) never reach another. +import { type FreshServer, startFreshServer } from './fresh-server.ts'; + +// Sign in and Sign out in the browser, on three kinds of server: +// - the Multi-user server on 4174 (the "e2e-multi" project, after "setup-multi" made the admin): signed-out +// redirects, the board of an invite-only Instance, the admin's nav. It has only the admin, and the +// admin only signs in and out here. +// - a server of this file's own, from the same build, with a fresh database whose Instance is Multi-user +// with Registration Open (see fresh-server.ts). The Accounts the Sign in tests need are made through its +// Sign up form, so this file never writes to the shared database: another file's setup there timed out +// when two test processes wrote to it at once. +// - a second own server whose Instance is not set up, to walk Setup, Sign out and Sign in as a released +// Instance would, with no test switch. // -// Sign in carries a shared budget per address: twenty wrong passwords from this machine in ten minutes, across -// every Account. This file spends six in all; keep the total across files well under twenty. +// Sign in keeps a budget per address: twenty wrong passwords from this machine in ten minutes, across every +// Account, and five per Account. The own servers have budgets of their own; 4174's is spent only by the admin. const NAV = 'Primary'; const WRONG = 'Email or password is wrong.'; +const PASSWORD = 'correct horse battery staple'; const people = { asker: { name: 'Ana Asker', email: 'ana.signin@example.com' }, @@ -22,43 +29,61 @@ const people = { phone: { name: 'Pat Phone', email: 'pat.signin@example.com' } }; -let fixture: Awaited>; - -test.beforeAll(async () => { - fixture = await openMultiUserDatabase(); - for (const person of Object.values(people)) await fixture.ensureLearner(person); -}); - -test.afterAll(async () => { - await fixture.close(); -}); - async function signInWith(page: Page, email: string, password: string) { await page.getByLabel('Email').fill(email); await page.getByLabel('Password').fill(password); await page.getByRole('button', { name: 'Sign in →' }).click(); } -test.describe('a signed-out visitor', () => { - test('lands on Sign in from any page and, once signed in, on the page they asked for', async ({ - page - }) => { - for (const path of ['/', '/topics/stacks', '/settings', '/users', '/no/such/page']) { - await page.goto(path); - await expect(page, path).toHaveURL(`/signin?next=${encodeURIComponent(path)}`); - } - - await page.goto('/topics/stacks'); - await expect(page.getByRole('heading', { level: 1 })).toHaveText('Welcome back.'); - await signInWith(page, people.asker.email, LEARNER_PASSWORD); +/** The own server with Registration Open and one Account for each person above, made through Sign up. */ +const onOpen = base.extend({ + open: [ + // biome-ignore lint/correctness/noEmptyPattern: Playwright reads the fixtures this one needs from this pattern + async ({}, use) => { + const server = await startFreshServer({ openRegistration: true }); + try { + for (const person of Object.values(people)) { + const response = await fetch(`${server.url}/signup`, { + method: 'POST', + redirect: 'manual', + headers: { origin: server.url, accept: 'text/html' }, + body: new URLSearchParams({ ...person, password: PASSWORD }) + }); + if (response.status !== 303) { + throw new Error(`signing up ${person.email} answered ${response.status}`); + } + } + await use(server); + } finally { + await server.stop(); + } + }, + { scope: 'worker', timeout: 120_000 } + ], + baseURL: async ({ open }, use) => { + await use(open.url); + } +}); - await expect(page).toHaveURL('/topics/stacks'); - await expect(page.getByRole('navigation', { name: NAV })).toContainText(people.asker.name); - }); +onOpen.describe('a signed-out visitor, on an Instance where anyone may sign up', () => { + onOpen( + 'lands on Sign in from any page and, once signed in, on the page they asked for', + async ({ page }) => { + for (const path of ['/', '/topics/stacks', '/settings', '/users', '/no/such/page']) { + await page.goto(path); + await expect(page, path).toHaveURL(`/signin?next=${encodeURIComponent(path)}`); + } + + await page.goto('/topics/stacks?x=1'); + await expect(page.getByRole('heading', { level: 1 })).toHaveText('Welcome back.'); + await signInWith(page, people.asker.email, PASSWORD); + + await expect(page).toHaveURL('/topics/stacks?x=1'); + await expect(page.getByRole('navigation', { name: NAV })).toContainText(people.asker.name); + } + ); - test('sees the board: wordmark alone in the header, no Map or name link, and no sign-up link on an invite-only Instance', async ({ - page - }) => { + onOpen('sees the board, with a way to create an account', async ({ page }) => { await page.goto('/signin'); await expect(page.getByRole('navigation', { name: NAV })).toHaveCount(0); @@ -66,120 +91,127 @@ test.describe('a signed-out visitor', () => { await expect(page.getByLabel('Email')).toBeVisible(); await expect(page.getByLabel('Password')).toBeVisible(); await expect(page.getByText('Forgot password? Ask your admin.')).toBeVisible(); - // This Instance is invite-only (every new one is), so the page does not point at a closed form. - await expect(page.getByRole('link', { name: 'Create an account' })).toHaveCount(0); + await expect(page.getByRole('link', { name: 'Create an account' })).toHaveAttribute( + 'href', + '/signup' + ); await page.goto('/signup'); await expect(page.getByRole('navigation', { name: NAV })).toHaveCount(0); }); - test('is never sent off the site by a hostile next', async ({ page }) => { + onOpen('is never sent off the site by a hostile next', async ({ page }) => { await page.goto(`/signin?next=${encodeURIComponent('//evil.example/phish')}`); - await signInWith(page, people.hostile.email, LEARNER_PASSWORD); + await signInWith(page, people.hostile.email, PASSWORD); await expect(page).toHaveURL('/'); }); - test('is told one thing for a wrong password and for no such account, keeping the email but not the password', async ({ - page, - browser - }) => { - await page.goto('/signin'); - await signInWith(page, people.typo.email, 'not the password at all'); - - await expect(page).toHaveURL(/\/signin$/); - await expect(page.getByText(WRONG)).toBeVisible(); - await expect(page.getByLabel('Email')).toHaveValue(people.typo.email); - await expect(page.getByLabel('Password')).toHaveValue(''); - await expect(page.getByLabel('Email')).toHaveAttribute('aria-invalid', 'true'); - await expect(page.getByLabel('Password')).toHaveAttribute('aria-invalid', 'true'); - // The right password then works from that very form. - await page.getByLabel('Password').fill(LEARNER_PASSWORD); - await page.getByRole('button', { name: 'Sign in →' }).click(); - await expect(page).toHaveURL('/'); - - // No Account at all: the same words, the same fields marked. - const stranger = await browser.newContext({ baseURL: 'http://localhost:4174' }); - const other = await stranger.newPage(); - await other.goto('/signin'); - await signInWith(other, 'nobody.signin@example.com', 'whatever whatever'); - await expect(other.getByText(WRONG)).toBeVisible(); - await expect(other.getByLabel('Email')).toHaveAttribute('aria-invalid', 'true'); - await stranger.close(); - }); + onOpen( + 'is told one thing for a wrong password and for no such account, keeping the email but not the password', + async ({ page, browser, open }) => { + await page.goto('/signin'); + await signInWith(page, people.typo.email, 'not the password at all'); + + await expect(page).toHaveURL(/\/signin$/); + await expect(page.getByText(WRONG)).toBeVisible(); + await expect(page.getByLabel('Email')).toHaveValue(people.typo.email); + await expect(page.getByLabel('Password')).toHaveValue(''); + await expect(page.getByLabel('Email')).toHaveAttribute('aria-invalid', 'true'); + await expect(page.getByLabel('Password')).toHaveAttribute('aria-invalid', 'true'); + // The right password then works from that very form. + await page.getByLabel('Password').fill(PASSWORD); + await page.getByRole('button', { name: 'Sign in →' }).click(); + await expect(page).toHaveURL('/'); - test('is refused on the sixth wrong password, with the same message, even with the right one', async ({ - page - }) => { - await page.goto('/signin'); - for (let attempt = 1; attempt <= 5; attempt++) { - await signInWith(page, people.locked.email, `wrong password number ${attempt}`); - await expect(page.getByText(WRONG), `wrong password ${attempt}`).toBeVisible(); + // No Account at all: the same words, the same fields marked. + const stranger = await browser.newContext({ baseURL: open.url }); + const other = await stranger.newPage(); + await other.goto('/signin'); + await signInWith(other, 'nobody.signin@example.com', 'whatever whatever'); + await expect(other.getByText(WRONG)).toBeVisible(); + await expect(other.getByLabel('Email')).toHaveAttribute('aria-invalid', 'true'); + await stranger.close(); } - - // The sixth is the right password, and it is still refused, in the same words. - await signInWith(page, people.locked.email, LEARNER_PASSWORD); - - await expect(page.getByText(WRONG)).toBeVisible(); - await expect(page).toHaveURL(/\/signin$/); - await expect(page.getByRole('navigation', { name: NAV })).toHaveCount(0); - }); - - test('cannot sign in over HTTP, only through the form', async ({ request }) => { - const response = await request.post('/api/auth/sign-in/email', { - headers: { origin: 'http://localhost:4174' }, - data: { email: people.asker.email, password: LEARNER_PASSWORD } - }); - - expect(response.status()).toBe(404); - expect(response.headers()['set-cookie']).toBeUndefined(); - }); + ); + + onOpen( + 'is refused on the sixth wrong password, with the same message, even with the right one', + async ({ page }) => { + await page.goto('/signin'); + for (let attempt = 1; attempt <= 5; attempt++) { + await signInWith(page, people.locked.email, `wrong password number ${attempt}`); + await expect(page.getByText(WRONG), `wrong password ${attempt}`).toBeVisible(); + } + + // The sixth is the right password, and it is still refused, in the same words. + await signInWith(page, people.locked.email, PASSWORD); + + await expect(page.getByText(WRONG)).toBeVisible(); + await expect(page).toHaveURL(/\/signin$/); + await expect(page.getByRole('navigation', { name: NAV })).toHaveCount(0); + } + ); + + onOpen( + 'is refused on the sixth wrong try for an email with no Account too, in the same words', + async ({ page }) => { + await page.goto('/signin'); + for (let attempt = 1; attempt <= 6; attempt++) { + await signInWith(page, 'nobody.six@example.com', `wrong password number ${attempt}`); + await expect(page.getByText(WRONG), `try ${attempt}`).toBeVisible(); + } + await expect(page).toHaveURL(/\/signin$/); + } + ); }); -test.describe('a signed-in user', () => { - test('signs out from the nav and is back at Sign in, with the old page no longer theirs', async ({ - page - }) => { - await page.goto('/signin'); - await signInWith(page, people.asker.email, LEARNER_PASSWORD); - const nav = page.getByRole('navigation', { name: NAV }); - await expect(nav.getByRole('link')).toHaveText(['Map', people.asker.name]); - await expect(nav.getByRole('button')).toHaveText(['Sign out']); +onOpen.describe('a signed-in user', () => { + onOpen( + 'signs out from the nav and is back at Sign in, with the old page no longer theirs', + async ({ page }) => { + await page.goto('/signin'); + await signInWith(page, people.asker.email, PASSWORD); + const nav = page.getByRole('navigation', { name: NAV }); + await expect(nav.getByRole('link')).toHaveText(['Map', people.asker.name]); + await expect(nav.getByRole('button')).toHaveText(['Sign out']); - await nav.getByRole('button', { name: 'Sign out' }).click(); + await nav.getByRole('button', { name: 'Sign out' }).click(); - await expect(page).toHaveURL('/signin'); - await expect(page.getByRole('heading', { level: 1 })).toHaveText('Welcome back.'); - await page.goto('/topics/stacks'); - await expect(page).toHaveURL('/signin?next=%2Ftopics%2Fstacks'); - expect((await page.context().cookies()).map((c) => c.name)).not.toContain( - 'better-auth.session_token' - ); - }); + await expect(page).toHaveURL('/signin'); + await expect(page.getByRole('heading', { level: 1 })).toHaveText('Welcome back.'); + await page.goto('/topics/stacks'); + await expect(page).toHaveURL('/signin?next=%2Ftopics%2Fstacks'); + expect((await page.context().cookies()).map((c) => c.name)).not.toContain( + 'better-auth.session_token' + ); + } + ); - test('signs out from Settings too, and Sign out is not a link: a plain GET changes nothing', async ({ - page - }) => { - await page.goto('/signin'); - await signInWith(page, people.settings.email, LEARNER_PASSWORD); - await page.goto('/settings'); - await expect(page.getByRole('heading', { name: 'Session' })).toBeVisible(); + onOpen( + 'signs out from Settings too, and Sign out is not a link: a plain GET changes nothing', + async ({ page }) => { + await page.goto('/signin'); + await signInWith(page, people.settings.email, PASSWORD); + await page.goto('/settings'); + await expect(page.getByRole('heading', { name: 'Session' })).toBeVisible(); - // A GET to the action's URL is just the Settings page, and still signed in. - await page.goto('/settings?/signout'); - await expect(page.getByRole('heading', { level: 1 })).toHaveText('Settings'); - await expect(page.getByRole('navigation', { name: NAV })).toContainText(people.settings.name); + // A GET to the action's URL is just the Settings page, and still signed in. + await page.goto('/settings?/signout'); + await expect(page.getByRole('heading', { level: 1 })).toHaveText('Settings'); + await expect(page.getByRole('navigation', { name: NAV })).toContainText(people.settings.name); - await page.getByRole('main').getByRole('button', { name: 'Sign out' }).click(); + await page.getByRole('main').getByRole('button', { name: 'Sign out' }).click(); - await expect(page).toHaveURL('/signin'); - await page.goto('/settings'); - await expect(page).toHaveURL('/signin?next=%2Fsettings'); - }); + await expect(page).toHaveURL('/signin'); + await page.goto('/settings'); + await expect(page).toHaveURL('/signin?next=%2Fsettings'); + } + ); - test('sees the 404 page for a path with no route, signed in', async ({ page }) => { + onOpen('sees the 404 page for a path with no route, signed in', async ({ page }) => { await page.goto('/signin'); - await signInWith(page, people.asker.email, LEARNER_PASSWORD); + await signInWith(page, people.asker.email, PASSWORD); const response = await page.goto('/no/such/page'); @@ -187,9 +219,9 @@ test.describe('a signed-in user', () => { await expect(page).toHaveURL('/no/such/page'); }); - test('has Sign out only on Settings on a phone', async ({ page }) => { + onOpen('has Sign out only on Settings on a phone', async ({ page }) => { await page.goto('/signin'); - await signInWith(page, people.phone.email, LEARNER_PASSWORD); + await signInWith(page, people.phone.email, PASSWORD); await page.setViewportSize({ width: 390, height: 844 }); await page.goto('/'); @@ -203,9 +235,9 @@ test.describe('a signed-in user', () => { await expect(page).toHaveURL('/signin'); }); - test('is sent from Sign in to where they were going, not shown the form', async ({ page }) => { + onOpen('is sent from Sign in to where they were going, not shown the form', async ({ page }) => { await page.goto('/signin'); - await signInWith(page, people.asker.email, LEARNER_PASSWORD); + await signInWith(page, people.asker.email, PASSWORD); await page.goto(`/signin?next=${encodeURIComponent('/topics/stacks')}`); @@ -213,55 +245,93 @@ test.describe('a signed-in user', () => { }); }); -test('the admin has Users, their name and Sign out in the nav, in that order', async ({ page }) => { - await page.goto('/signin'); - await signInWith(page, E2E_ADMIN.email, E2E_ADMIN.password); +// The Multi-user server on 4174: invite-only, with the admin the "setup-multi" project made. +base.describe('the shared Multi-user server', () => { + base( + 'sends a signed-out visitor from any page, even an unknown one, to Sign in', + async ({ page }) => { + for (const path of ['/', '/topics/stacks', '/settings', '/users', '/no/such/page']) { + await page.goto(path); + await expect(page, path).toHaveURL(`/signin?next=${encodeURIComponent(path)}`); + } + } + ); - const nav = page.getByRole('navigation', { name: NAV }); - await expect(nav.getByRole('link')).toHaveText(['Map', 'Users', E2E_ADMIN.name]); - await expect(nav.getByRole('button')).toHaveText(['Sign out']); - await nav.getByRole('button', { name: 'Sign out' }).click(); - await expect(page).toHaveURL('/signin'); -}); + base( + 'shows the board with no sign-up link while registration is invite-only', + async ({ page }) => { + await page.goto('/signin'); -test.describe('a fresh Instance started like a released one, with no test switch', () => { - // Its own server, from the same build, on a fresh database: Setup offers Multi-user because the operator - // gave it a secret and an origin, and nothing else is needed. - test('lets the first visitor choose Multi-user, create the admin, sign out and sign back in', async ({ - browser - }) => { - test.setTimeout(120_000); - const server = await startFreshServer(); - try { - const context = await browser.newContext({ baseURL: server.url }); - const page = await context.newPage(); - - await page.goto('/'); - await expect(page).toHaveURL('/setup'); - await expect(page.getByLabel('Multi-user')).toBeEnabled(); - await expect(page.getByText(/next update/)).toHaveCount(0); - await page.getByLabel('Multi-user').check(); - await page.getByRole('button', { name: 'Continue →' }).click(); - await page.getByLabel('Name').fill('Fresh Admin'); - await page.getByLabel('Email').fill('fresh.admin@example.com'); - await page.getByLabel('Password').fill('a long enough password'); - await page.getByRole('button', { name: 'Create admin account →' }).click(); - await expect(page).toHaveURL('/'); - const nav = page.getByRole('navigation', { name: NAV }); - await expect(nav).toContainText('Fresh Admin'); + await expect(page.getByRole('heading', { level: 1 })).toHaveText('Welcome back.'); + await expect(page.getByRole('navigation', { name: NAV })).toHaveCount(0); + await expect(page.getByRole('link', { name: 'Create an account' })).toHaveCount(0); + } + ); + + base('does not serve password sign-in over HTTP, only through the form', async ({ request }) => { + const response = await request.post('/api/auth/sign-in/email', { + headers: { origin: 'http://localhost:4174' }, + data: { email: E2E_ADMIN.email, password: E2E_ADMIN.password } + }); + + expect(response.status()).toBe(404); + expect(response.headers()['set-cookie']).toBeUndefined(); + }); + + base( + 'gives the admin Users, their name and Sign out in the nav, in that order', + async ({ page }) => { + await page.goto('/signin'); + await signInWith(page, E2E_ADMIN.email, E2E_ADMIN.password); + const nav = page.getByRole('navigation', { name: NAV }); + await expect(nav.getByRole('link')).toHaveText(['Map', 'Users', E2E_ADMIN.name]); + await expect(nav.getByRole('button')).toHaveText(['Sign out']); await nav.getByRole('button', { name: 'Sign out' }).click(); await expect(page).toHaveURL('/signin'); - await page.goto('/topics/stacks'); - await expect(page).toHaveURL('/signin?next=%2Ftopics%2Fstacks'); - - await signInWith(page, 'fresh.admin@example.com', 'a long enough password'); + } + ); +}); - await expect(page).toHaveURL('/topics/stacks'); - await expect(page.getByRole('navigation', { name: NAV })).toContainText('Fresh Admin'); - await context.close(); - } finally { - await server.stop(); +base.describe('a fresh Instance started like a released one, with no test switch', () => { + // Its own server, from the same build, on a fresh database: Setup offers Multi-user because the operator + // gave it a secret and an origin, and nothing else is needed. + base( + 'lets the first visitor choose Multi-user, create the admin, sign out and sign back in', + async ({ browser }) => { + base.setTimeout(120_000); + const server = await startFreshServer(); + try { + const context = await browser.newContext({ baseURL: server.url }); + const page = await context.newPage(); + + await page.goto('/'); + await expect(page).toHaveURL('/setup'); + await expect(page.getByLabel('Multi-user')).toBeEnabled(); + await expect(page.getByText(/next update/)).toHaveCount(0); + await page.getByLabel('Multi-user').check(); + await page.getByRole('button', { name: 'Continue →' }).click(); + await page.getByLabel('Name').fill('Fresh Admin'); + await page.getByLabel('Email').fill('fresh.admin@example.com'); + await page.getByLabel('Password').fill('a long enough password'); + await page.getByRole('button', { name: 'Create admin account →' }).click(); + await expect(page).toHaveURL('/'); + const nav = page.getByRole('navigation', { name: NAV }); + await expect(nav).toContainText('Fresh Admin'); + + await nav.getByRole('button', { name: 'Sign out' }).click(); + await expect(page).toHaveURL('/signin'); + await page.goto('/topics/stacks'); + await expect(page).toHaveURL('/signin?next=%2Ftopics%2Fstacks'); + + await signInWith(page, 'fresh.admin@example.com', 'a long enough password'); + + await expect(page).toHaveURL('/topics/stacks'); + await expect(page.getByRole('navigation', { name: NAV })).toContainText('Fresh Admin'); + await context.close(); + } finally { + await server.stop(); + } } - }); + ); }); diff --git a/src/routes/signup/signup.multi.e2e.ts b/src/routes/signup/signup.multi.e2e.ts index 00f3bcd7..e8ac56e3 100644 --- a/src/routes/signup/signup.multi.e2e.ts +++ b/src/routes/signup/signup.multi.e2e.ts @@ -269,10 +269,14 @@ test.describe('registration open', () => { form: { name: 'Late', email: 'late@example.com', password: 'a long enough secret' } }); expect(valid.status()).toBe(429); - const signIn = await request.post(`${open.url}/api/auth/sign-in/email`, { - headers: { origin: open.url }, - data: { email: 'late@example.com', password: 'a long enough secret' } + // Nothing was created: there is no Account to sign in to. (Password sign-in is not served over HTTP; + // the Sign in form answers a missing Account and a wrong password alike.) + const signIn = await request.post(`${open.url}/signin`, { + headers: { origin: open.url, accept: 'text/html' }, + form: { email: 'late@example.com', password: 'a long enough secret' } }); - expect(signIn.status()).toBe(401); + expect(signIn.status()).toBe(400); + expect(await signIn.text()).toContain('Email or password is wrong.'); + expect(signIn.headers()['set-cookie']).toBeUndefined(); }); }); From 03fb17e3741a172d78fa21da506cf74558fd059c Mon Sep 17 00:00:00 2001 From: Gauransh Mathur Date: Fri, 16 Oct 2026 10:43:06 +0000 Subject: [PATCH 5/7] feat(beyondleetcode): C32 keep the Sign in button on one line Co-Authored-By: Claude Sonnet 5.5 --- src/routes/signin/+page.svelte | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/routes/signin/+page.svelte b/src/routes/signin/+page.svelte index 5b319b0e..0fe12295 100644 --- a/src/routes/signin/+page.svelte +++ b/src/routes/signin/+page.svelte @@ -171,6 +171,7 @@ const failure = $derived(form && !form.wrong ? form.message : undefined); color: var(--accent); } button { + white-space: nowrap; box-sizing: border-box; align-self: flex-start; height: 52px; @@ -201,9 +202,10 @@ const failure = $derived(form && !form.wrong ? form.message : undefined); } .actions { display: flex; + flex-wrap: wrap; justify-content: space-between; align-items: center; - gap: 24px; + gap: 12px 24px; } .forgot { font-size: 18px; From a0931b08e58622961667bf39ec7f59537cb40de4 Mon Sep 17 00:00:00 2001 From: Gauransh Mathur Date: Fri, 16 Oct 2026 11:03:42 +0000 Subject: [PATCH 6/7] feat(beyondleetcode): C32 security review fixes: no redirect loop, no Settings data through sign-out, timing residual, README Co-Authored-By: Claude Sonnet 5.5 --- README.md | 4 +- src/lib/server/handle.test.ts | 29 ++++++++++++++ src/lib/server/identity/gate.test.ts | 60 ++++++++++++++++++++++++++++ src/lib/server/identity/gate.ts | 19 +++++++-- src/lib/server/identity/throttle.ts | 10 +++-- 5 files changed, 113 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 630797d7..9f30e981 100644 --- a/README.md +++ b/README.md @@ -18,13 +18,13 @@ Learn DSA the way senior engineers actually use it — not isolated leetcode puz ## Before you turn on Multi-user -Multi-user Mode (accounts, each keeping its own progress) arrives in the next update. Setup says so until then. Set these up first, so your Instance is ready when it does. They go in the `.env` next to `compose.yaml`. +Multi-user Mode (accounts, each keeping its own progress) can be chosen at Setup. Set these up first, so Setup can offer it. They go in the `.env` next to `compose.yaml`. - **`BETTER_AUTH_SECRET` is required.** It signs sessions. Use at least 32 characters, and not the example value from `.env.example`. `init` writes a random one for you. `openssl rand -hex 24` makes another. - **`ORIGIN` is required.** It is the public URL of your Instance, for example `https://learn.example.com`. Setup does not offer Multi-user without it, and a Multi-user Instance will not start without it. `PROTOCOL_HEADER` and `HOST_HEADER` do not replace it. - **Behind a reverse proxy, set `ORIGIN` plus `ADDRESS_HEADER` and `XFF_DEPTH`.** `ADDRESS_HEADER` is the header your proxy fills in with the client's address, such as `X-Forwarded-For`. `XFF_DEPTH` is how many trusted proxies sit in front of the app, as the [SvelteKit `adapter-node` docs](https://svelte.dev/docs/kit/adapter-node#Environment-variables-ADDRESS_HEADER-and-XFF_DEPTH) describe. Without them every client looks like the proxy, and they all share one sign-in rate limit. - **If you set those headers, only the proxy may reach the app's port.** A client that goes round the proxy can forge the header, so it gets a fresh sign-in limit on every request, or locks out someone else by naming their address. Docker [publishes a port on every interface](https://docs.docker.com/engine/network/port-publishing/) and [gets past ufw](https://docs.docker.com/engine/network/packet-filtering-firewalls/). So change the `ports` line in `compose.yaml` to `"127.0.0.1:${PORT}:3000"` if the proxy runs on the same machine, or keep the app on the proxy's Docker network only. Use Docker 28.0.0 or later: older releases let hosts on the same network segment reach a port published to `127.0.0.1`. If clients connect straight to the app, set neither header and leave the port binding alone. -- **Open Setup straight after the first start.** On a fresh Instance the first visitor to Setup chooses the mode. When Multi-user is available, whoever chooses it first creates the admin Account and becomes the admin. +- **Open Setup straight after the first start.** On a fresh Instance the first visitor to Setup chooses the mode. Whoever chooses Multi-user first creates the admin Account and becomes the admin. - **Use https if the Instance is reachable beyond your own machine.** Over plain http, passwords and session cookies cross the network unencrypted. - **If an older `init` wrote your `compose.yaml`,** edit `web`'s `environment` and then run `docker compose up -d`. Replace `ORIGIN: ${ORIGIN:-}` with a bare `ORIGIN:` line: the old line passes an empty `ORIGIN` when `.env` has none, and [an empty `ORIGIN` stops the server](https://github.com/sveltejs/kit/blob/fef182780ecfc5eecd1c12e03afb99af9d1f7049/packages/adapter-node/utils.js#L26-L35). Behind a proxy, add `ADDRESS_HEADER: ${ADDRESS_HEADER:-}` and `XFF_DEPTH: ${XFF_DEPTH:-1}` too. Without them the headers you set in `.env` never reach the app. - **Never set** `BETTER_AUTH_SECRETS`, `BETTER_AUTH_URL`, `PUBLIC_BETTER_AUTH_URL`, `BASE_URL`, `BETTER_AUTH_TRUSTED_ORIGINS`, `BETTER_AUTH_TELEMETRY` or `BETTER_AUTH_TELEMETRY_ENDPOINT`. Better Auth, the sign-in library, reads each from the environment on its own. [`BETTER_AUTH_SECRETS`](https://github.com/better-auth/better-auth/blob/db02f233918ad1233bf0753e437e1c0da353273d/packages/better-auth/src/context/create-context.ts#L174) replaces the session secret. [`BETTER_AUTH_TRUSTED_ORIGINS`](https://github.com/better-auth/better-auth/blob/db02f233918ad1233bf0753e437e1c0da353273d/packages/better-auth/src/context/helpers.ts#L155) adds origins it accepts. The app refuses to start while either is set. [The URL variables](https://github.com/better-auth/better-auth/blob/db02f233918ad1233bf0753e437e1c0da353273d/packages/better-auth/src/utils/url.ts#L153-L160) name another base URL, and [the telemetry pair](https://github.com/better-auth/better-auth/blob/db02f233918ad1233bf0753e437e1c0da353273d/packages/telemetry/src/index.ts#L26) sends Better Auth's telemetry to the endpoint you give. Keep those five unset too. diff --git a/src/lib/server/handle.test.ts b/src/lib/server/handle.test.ts index fee60930..c1d35862 100644 --- a/src/lib/server/handle.test.ts +++ b/src/lib/server/handle.test.ts @@ -415,6 +415,35 @@ describe('Multi-user Mode', () => { expect(resolve).not.toHaveBeenCalled(); }); + it('does not send a user who must change their password round in a loop while /account/password has no route', async () => { + const { handle, asking, db } = await multiSetup(); + await db.user.updateMany({ data: { mustChangePassword: true } }); + const { event, resolve } = asking('/account/password', null); + + const response = await handle({ event, resolve }); + + // Where the redirect points, they arrive: the 404 page renders, nothing redirects again. + expect(response.status).toBe(200); + expect(resolve).toHaveBeenCalledOnce(); + }); + + it('keeps the Settings load data from a user who must change their password, sign-out action or not', async () => { + const { handle, db, cookie } = await multiSetup(); + await db.user.updateMany({ data: { mustChangePassword: true } }); + const { event, resolve } = request( + '/settings/__data.json?/signout', + '/settings', + 'POST', + new Headers({ cookie }) + ); + + const response = await handle({ event, resolve }); + + expect(response.status).toBe(303); + expect(response.headers.get('location')).toBe('/account/password'); + expect(resolve).not.toHaveBeenCalled(); + }); + it('still lets a user who must change their password sign out, from the nav and from Settings', async () => { const { handle, db, cookie } = await multiSetup(); await db.user.updateMany({ data: { mustChangePassword: true } }); diff --git a/src/lib/server/identity/gate.test.ts b/src/lib/server/identity/gate.test.ts index 8235e8da..adc8e2dc 100644 --- a/src/lib/server/identity/gate.test.ts +++ b/src/lib/server/identity/gate.test.ts @@ -157,6 +157,34 @@ describe('gate', () => { }), redirect('/account/password') ], + // /account/password has no route until its page lands; the user must not be sent round in a loop. + [ + 'must change, /account/password with no route yet -> pass (the 404 page, not a redirect to itself)', + request(null, { + who: learner, + mustChangePassword: true, + pathname: '/account/password' + }), + pass + ], + [ + 'must change, other path with no route -> password, even one that starts like it', + request(null, { + who: learner, + mustChangePassword: true, + pathname: '/account/password/extra' + }), + redirect('/account/password') + ], + [ + 'must change, /account/passwords with no route -> password', + request(null, { + who: learner, + mustChangePassword: true, + pathname: '/account/passwords' + }), + redirect('/account/password') + ], [ 'must change, unknown page -> password', request(null, { who: learner, mustChangePassword: true }), @@ -173,6 +201,38 @@ describe('gate', () => { }), pass ], + [ + 'must change, POST Settings data request with the sign-out action -> password', + request('/settings', { + who: learner, + mustChangePassword: true, + method: 'POST', + pathname: '/settings/__data.json', + search: '?/signout' + }), + redirect('/account/password') + ], + [ + 'must change, POST data request of a page that is not a folder -> password', + request('/settings', { + who: learner, + mustChangePassword: true, + method: 'POST', + pathname: '/settings.html__data.json', + search: '?/signout' + }), + redirect('/account/password') + ], + [ + 'must change, POST /api/auth/sign-out with a data suffix is no sign-out -> password', + request('/api/auth/[...all]', { + who: learner, + mustChangePassword: true, + method: 'POST', + pathname: '/api/auth/sign-out/__data.json' + }), + redirect('/account/password') + ], [ 'must change, GET Settings sign-out -> password', request('/settings', { diff --git a/src/lib/server/identity/gate.ts b/src/lib/server/identity/gate.ts index c6a9d40c..0d48568f 100644 --- a/src/lib/server/identity/gate.ts +++ b/src/lib/server/identity/gate.ts @@ -42,9 +42,18 @@ const formAction = (search: string): string | null => { return null; }; -/** The two ways to sign out: Better Auth's own endpoint, and the Settings page's `signout` form action. */ +/** SvelteKit's data requests (client navigation and invalidation) end like this; they return load data. */ +const isDataRequest = (pathname: string) => + pathname.endsWith('/__data.json') || pathname.endsWith('.html__data.json'); + +/** + * The two ways to sign out: Better Auth's own endpoint, and the Settings page's `signout` form action. + * Never as a data request: that answers with the layout's and the page's load data, which a user who + * must change their password has no business reading. + */ const isSigningOut = ({ routeId, method, pathname, search }: GateInput) => method === 'POST' && + !isDataRequest(pathname) && (pathname === '/api/auth/sign-out' || (routeId === '/settings' && formAction(search) === '/signout')); @@ -74,9 +83,11 @@ export function gate(input: GateInput): GateResult { } if (input.mustChangePassword) { - if (routeId !== '/account/password' && !isSigningOut(input)) { - return redirect('/account/password'); - } + // The page they are sent to. Until it has a route (routeId null) its exact path counts as arriving, + // so the 404 page shows instead of a redirect to itself. + const atPasswordPage = + routeId === '/account/password' || (routeId === null && pathname === '/account/password'); + if (!atPasswordPage && !isSigningOut(input)) return redirect('/account/password'); } if (routeId !== null && isUnder(routeId, '/users') && who.role !== 'admin') return status(403); diff --git a/src/lib/server/identity/throttle.ts b/src/lib/server/identity/throttle.ts index ce035b76..180f3f0a 100644 --- a/src/lib/server/identity/throttle.ts +++ b/src/lib/server/identity/throttle.ts @@ -139,9 +139,13 @@ interface Entry { * whose window ends earliest and never skips or refuses, so a flood can only evict other made-up emails: * a known Account's counters stay. Residual: to make one unknown email forget its count, an attacker must * push it out, which takes more than maxKeys other emails inside one window, so more than maxKeys / 20 - * addresses, for each fresh set of guesses. The address map evicts the same way, with the same residual: - * an attacker with more addresses than maxKeys inside one window gets fresh per-address budgets and is - * still held to the per-Account limit. + * addresses, for each fresh set of guesses. Timing residual of the same flood: once an unknown email has been + * pushed out, its next try is counted afresh and runs the slow password hash, while a known Account's try + * (its counters stay) is refused fast, so an attacker who spends more than maxKeys unknown emails from about + * maxKeys / 20 addresses or /64s in one window can tell the two apart again by their response time, for as + * long as the flood lasts. The cost scales with maxKeys; it is accepted. The address map evicts the same + * way, with the same residual: an attacker with more addresses than maxKeys inside one window gets fresh + * per-address budgets and is still held to the per-Account limit. * * A right password takes its own attempt back from the address and the Account counters (`succeed`), if * their window is still the one it was counted in, so only wrong passwords spend budget. Callers that From 9fede2be3e41faef251c5cb44591c12c6c12ad84 Mon Sep 17 00:00:00 2001 From: Gauransh Mathur Date: Fri, 16 Oct 2026 11:15:42 +0000 Subject: [PATCH 7/7] feat(beyondleetcode): C32 data requests are told by event.isDataRequest, not the pathname suffix Co-Authored-By: Claude Sonnet 5.5 --- src/lib/server/handle.test.ts | 16 +++++- src/lib/server/handle.ts | 1 + src/lib/server/identity/gate.test.ts | 23 +++------ src/lib/server/identity/gate.ts | 16 +++--- src/routes/signin/fresh-server.ts | 5 +- src/routes/signin/signin.multi.e2e.ts | 74 ++++++++++++++++++++++++++- 6 files changed, 110 insertions(+), 25 deletions(-) diff --git a/src/lib/server/handle.test.ts b/src/lib/server/handle.test.ts index c1d35862..7fc14822 100644 --- a/src/lib/server/handle.test.ts +++ b/src/lib/server/handle.test.ts @@ -430,12 +430,15 @@ describe('Multi-user Mode', () => { it('keeps the Settings load data from a user who must change their password, sign-out action or not', async () => { const { handle, db, cookie } = await multiSetup(); await db.user.updateMany({ data: { mustChangePassword: true } }); + // As SvelteKit builds it for POST /settings/__data.json?/signout: the suffix is already off event.url + // (respond.js strips it) and event.isDataRequest says what it was. const { event, resolve } = request( - '/settings/__data.json?/signout', + '/settings?/signout', '/settings', 'POST', new Headers({ cookie }) ); + (event as { isDataRequest: boolean }).isDataRequest = true; const response = await handle({ event, resolve }); @@ -444,6 +447,17 @@ describe('Multi-user Mode', () => { expect(resolve).not.toHaveBeenCalled(); }); + it('still serves a signed-in user a page\u2019s load data', async () => { + const { handle, asking } = await multiSetup(); + const data = asking('/settings', '/settings'); + (data.event as { isDataRequest: boolean }).isDataRequest = true; + + const response = await handle({ event: data.event, resolve: data.resolve }); + + expect(response.status).toBe(200); + expect(data.resolve).toHaveBeenCalledOnce(); + }); + it('still lets a user who must change their password sign out, from the nav and from Settings', async () => { const { handle, db, cookie } = await multiSetup(); await db.user.updateMany({ data: { mustChangePassword: true } }); diff --git a/src/lib/server/handle.ts b/src/lib/server/handle.ts index d5345957..dfe5cfbb 100644 --- a/src/lib/server/handle.ts +++ b/src/lib/server/handle.ts @@ -50,6 +50,7 @@ export function createHandle( who, mustChangePassword: who?.mustChangePassword, routeId: event.route.id, + isDataRequest: event.isDataRequest, method: request.method, pathname: url.pathname, search: url.search diff --git a/src/lib/server/identity/gate.test.ts b/src/lib/server/identity/gate.test.ts index adc8e2dc..1e27d2ae 100644 --- a/src/lib/server/identity/gate.test.ts +++ b/src/lib/server/identity/gate.test.ts @@ -8,6 +8,7 @@ const request = (routeId: string | null, extra: Partial = {}): GateIn mode: 'multi', who: null, routeId, + isDataRequest: false, method: 'GET', pathname: routeId ?? '/nowhere', search: '', @@ -201,35 +202,27 @@ describe('gate', () => { }), pass ], + // SvelteKit hands `handle` a data request with the suffix already cut off the pathname. [ 'must change, POST Settings data request with the sign-out action -> password', request('/settings', { who: learner, mustChangePassword: true, method: 'POST', - pathname: '/settings/__data.json', - search: '?/signout' - }), - redirect('/account/password') - ], - [ - 'must change, POST data request of a page that is not a folder -> password', - request('/settings', { - who: learner, - mustChangePassword: true, - method: 'POST', - pathname: '/settings.html__data.json', - search: '?/signout' + pathname: '/settings', + search: '?/signout', + isDataRequest: true }), redirect('/account/password') ], [ - 'must change, POST /api/auth/sign-out with a data suffix is no sign-out -> password', + 'must change, POST /api/auth/sign-out as a data request is no sign-out -> password', request('/api/auth/[...all]', { who: learner, mustChangePassword: true, method: 'POST', - pathname: '/api/auth/sign-out/__data.json' + pathname: '/api/auth/sign-out', + isDataRequest: true }), redirect('/account/password') ], diff --git a/src/lib/server/identity/gate.ts b/src/lib/server/identity/gate.ts index 0d48568f..88fbad65 100644 --- a/src/lib/server/identity/gate.ts +++ b/src/lib/server/identity/gate.ts @@ -10,6 +10,14 @@ export interface GateInput { mustChangePassword?: boolean; /** SvelteKit's `event.route.id`; null when no route matched. */ routeId: string | null; + /** + * SvelteKit's `event.isDataRequest`: the request is for a page's load data (`.../__data.json`). By the time + * `handle` runs SvelteKit has already cut that suffix off `event.url.pathname` and set this flag instead + * (kit 2.70.3, `src/runtime/server/respond.js`: `has_data_suffix` sets `is_data_request`, `strip_data_suffix` + * rewrites `url.pathname`, the event gets `isDataRequest`), so the pathname cannot tell. `render_data` runs + * whatever the method, and never runs an action. + */ + isDataRequest: boolean; method: string; pathname: string; search: string; @@ -42,18 +50,14 @@ const formAction = (search: string): string | null => { return null; }; -/** SvelteKit's data requests (client navigation and invalidation) end like this; they return load data. */ -const isDataRequest = (pathname: string) => - pathname.endsWith('/__data.json') || pathname.endsWith('.html__data.json'); - /** * The two ways to sign out: Better Auth's own endpoint, and the Settings page's `signout` form action. * Never as a data request: that answers with the layout's and the page's load data, which a user who * must change their password has no business reading. */ -const isSigningOut = ({ routeId, method, pathname, search }: GateInput) => +const isSigningOut = ({ routeId, method, pathname, search, isDataRequest }: GateInput) => method === 'POST' && - !isDataRequest(pathname) && + !isDataRequest && (pathname === '/api/auth/sign-out' || (routeId === '/settings' && formAction(search) === '/signout')); diff --git a/src/routes/signin/fresh-server.ts b/src/routes/signin/fresh-server.ts index c549c705..41e5db7e 100644 --- a/src/routes/signin/fresh-server.ts +++ b/src/routes/signin/fresh-server.ts @@ -15,7 +15,8 @@ import { createDb } from '../../lib/server/db.ts'; // already Multi-user with Registration Open, as a finished Setup plus the Users toggle (C34b) leave it, and // Accounts are made through the Sign up form. The app gains no way to open registration from this. -export type FreshServer = { url: string; stop(): Promise }; +/** `databaseUrl` is the server's own file, for a test that must change a row (the server and the test are the only users). */ +export type FreshServer = { url: string; databaseUrl: string; stop(): Promise }; export async function startFreshServer( options: { openRegistration?: boolean } = {} @@ -65,7 +66,7 @@ export async function startFreshServer( await stopAll(); throw error; } - return { url, stop: stopAll }; + return { url, databaseUrl: env.DATABASE_URL as string, stop: stopAll }; } async function freePort(): Promise { diff --git a/src/routes/signin/signin.multi.e2e.ts b/src/routes/signin/signin.multi.e2e.ts index a1394cee..71a0ff9e 100644 --- a/src/routes/signin/signin.multi.e2e.ts +++ b/src/routes/signin/signin.multi.e2e.ts @@ -1,5 +1,6 @@ import { test as base, expect, type Page } from '@playwright/test'; import { E2E_ADMIN } from '../../lib/e2e-admin.ts'; +import { createDb } from '../../lib/server/db.ts'; import { type FreshServer, startFreshServer } from './fresh-server.ts'; // Sign in and Sign out in the browser, on three kinds of server: @@ -26,7 +27,9 @@ const people = { locked: { name: 'Lou Locked', email: 'lou.signin@example.com' }, hostile: { name: 'Hal Hostile', email: 'hal.signin@example.com' }, typo: { name: 'Tia Typo', email: 'tia.signin@example.com' }, - phone: { name: 'Pat Phone', email: 'pat.signin@example.com' } + phone: { name: 'Pat Phone', email: 'pat.signin@example.com' }, + forced: { name: 'Fay Forced', email: 'fay.signin@example.com' }, + forcedData: { name: 'Dee Data', email: 'dee.signin@example.com' } }; async function signInWith(page: Page, email: string, password: string) { @@ -166,6 +169,75 @@ onOpen.describe('a signed-out visitor, on an Instance where anyone may sign up', ); }); +onOpen.describe('a user who must change their password', () => { + // C36a sets the flag and C36b adds /account/password; until then the 404 page is where they are sent. + // The flag is set on the own server's database, between two requests, as an admin's reset would. + onOpen( + 'is held to that page: no page, no load data, and no Settings data through the sign-out exception', + async ({ page, open }) => { + await page.goto('/signin'); + await signInWith(page, people.forced.email, PASSWORD); + await expect(page).toHaveURL('/'); + const db = createDb(open.databaseUrl); + try { + await db.user.update({ + where: { email: people.forced.email }, + data: { mustChangePassword: true } + }); + } finally { + await db.$disconnect(); + } + const headers = { origin: open.url, accept: 'text/html' }; + + // A page and its load data are both sent to /account/password. + await page.goto('/settings'); + await expect(page).toHaveURL('/account/password'); + const data = await page.request.get('/settings/__data.json'); + expect(await data.json()).toMatchObject({ type: 'redirect', location: '/account/password' }); + + // The sign-out action is the way out ... + const out = await page.request.post('/settings?/signout', { + headers, + form: {}, + maxRedirects: 0 + }); + expect(out.status()).toBe(303); + expect(out.headers().location).toBe('/signin'); + } + ); + + onOpen( + 'cannot read the Settings load data by posting to the data URL with the sign-out action', + async ({ page, open }) => { + await page.goto('/signin'); + await signInWith(page, people.forcedData.email, PASSWORD); + await expect(page).toHaveURL('/'); + const db = createDb(open.databaseUrl); + try { + await db.user.update({ + where: { email: people.forcedData.email }, + data: { mustChangePassword: true } + }); + } finally { + await db.$disconnect(); + } + + const response = await page.request.post('/settings/__data.json?/signout', { + headers: { origin: open.url }, + form: {}, + maxRedirects: 0 + }); + + // SvelteKit answers a data request's redirect as JSON; the load data is never run. + const body = await response.text(); + expect(JSON.parse(body)).toMatchObject({ type: 'redirect', location: '/account/password' }); + expect(body).not.toContain('"type":"data"'); + expect(body).not.toContain(people.forcedData.name); + expect(body).not.toContain('multiUser'); + } + ); +}); + onOpen.describe('a signed-in user', () => { onOpen( 'signs out from the nav and is back at Sign in, with the old page no longer theirs',