Repository navigation
feat(beyondleetcode): C32 Sign in and sign out - #185
Conversation
… pages private, HTTP sign-in off Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ow-aware throttle give-back, unknown emails limited like known ones Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…rs for unknown emails, browser tests Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…next safe for a Location header Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Security review (orchestrator-dispatched, 2026-10-16): verdict merge, after four small changes. All nine decisions held:
Better Auth 1.7.7 hashes the password for an unknown email. Sign-in mints a new token, so there is no fixation. Asked before merge: merge in main (C34b); stop the /account/password redirect loop while that page has no route; keep data requests out of the sign-out exception; document the flood timing residual; correct the README lines that say Multi-user arrives in the next update. Routed: 🤖 Generated with Claude Code |
… Settings data through sign-out, timing residual, README Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…st, not the pathname suffix Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Pre-merge fixes verified (orchestrator, 2026-10-16) at 9fede2b, which includes main. The must-change-password sign-out exception now uses SvelteKit's 🤖 Generated with Claude Code |
Card C32 (access control: security review before merge). Worker: builder-signin.
A signed-out visitor to any page lands on
/signinand, after signing in, on the page they asked for. Sign out returns them to Sign in. On a fresh Instance without the test switch, choosing Multi-user, creating the admin, signing out and signing back in works (unit-tested throughcreateApp, and in a browser against a second server of the same build started with no test switch).Checklist
throttle.ts:attempt()returns a token;succeed(token)gives one count back to the address and Account counters (never clears them), still clears the Account + address pair, once per token, and only to a counter still in the window the attempt was counted in. Sign up never calls it and its tests are unchangedthrottle.ts: emails with no Account are limited like known ones, in separate evicting maps (see "From the automated review")auth.ts:/sign-in/emailadded todisabledPaths; the limiter and anti-framing tests moved to live endpoints (change-password,sign-up/email,get-session); server-sideauth.api.signInEmailstill worksgate.ts: in Multi-user Mode a path with no route is a private page (signed out -> 303 to Sign in with next; must-change-password -> /account/password; signed in -> the 404 page). Local Mode unchanged. A must-change-password user can still POST Settings?/signout(matched the way SvelteKit picks the action: the first/-key), never another actionsignin/sign-in.ts: the action. Looks the Account up like Better Auth (email.toLowerCase(), no trim), counts the attempt before the password is looked at (key: client address + Account), one message and one status for a wrong password, no such account, a blank form and too many attempts, fixed strings, forwarded Set-Cookie,nextvalidated bysafeNextplus never/signinor/signup, non-ASCII percent-encoded for the Location headersignin/+page.svelteto the Sign in board (desktop, mobile, light, dark, error state). "Create an account" shows only while Registration is Open, read in the load on every requestsignouton Settings (/settings?/signout), revokes the session throughauth.api.signOut, forwards its Set-Cookie, goes to Sign in. Nav button as F.11/F.15 (Map, Users, name, Sign out), hidden under 768px where Settings has it (F.10); Session section on Settings, Multi-user only/signin,/signupand/setupCONTEXT.mdanddocs/module-map.mdcodexis not installed on this machineFrom the automated review
attempt()now returns a token holding each counter's window end;succeed(token)gives back only to a counter whose current window still matches (address, Account and pair alike), once per token, and drops an emptied entry rather than going below zero. Tests: attempt, window reset, succeed leaves the new window's count unchanged (address, Account, pair), and a spent token cannot take a later attempt's count.sign-in.mjs:await ctx.context.password.hash(password)before the 401), so no extra hashing was added; a test pins that, so a library upgrade that drops it fails.unknownEmail(a SHA-256 of the lowercased email, so the address is never held) as a separate field fromaccountId, with its own pair and Account maps. Real-Account maps keep their rule (never evict, a full one skips the new key; they hold as many keys as there are Accounts). The unknown-email maps cap atmaxKeysand evict the entry whose window ends earliest, never skipping or refusing, so a flood evicts only other made-up emails. Residual (stated in the header comment ofthrottle.ts): to make one unknown email forget its count an attacker must push it out with more thanmaxKeysother emails per fresh set of guesses. Tests: pastmaxKeysthe unknown maps stay atmaxKeys; a known Account is still refused on its 6th wrong try from one address and its 51st overall after a flood, whether its counters existed before the flood or not; a known and an unknown email are both refused on the 6th try.locals.instance.registrationMode()each time, never at module scope. Test: the mode changes between two loads and the link follows.From the security review
0. Merged
origin/main(C34b, #184) into the branch with no conflicts; itsusers.test.tsstill builds thelaterstub, so the unused variant stays.gate.ts, routeId null with the exact pathname/account/passwordnow counts as arriving, so the 404 page shows. Any other unrouted path (including/account/password/extraand/account/passwords) still redirects there. Tests ingate.test.tsand through the real hook inhandle.test.ts./settings?/signoutno longer applies to a data request. My first version tested the pathname for/__data.json, which cannot work: SvelteKit strips that suffix fromevent.url.pathnamebeforehandleruns and setsevent.isDataRequestinstead (kit 2.70.3,src/runtime/server/respond.js:has_data_suffixsetsis_data_request,strip_data_suffixrewritesurl.pathname, the event getsisDataRequest;render_dataruns for any method and never runs an action).GateInputnow carriesisDataRequest,handle.tspassesevent.isDataRequest, the exception requires!isDataRequest, and the suffix helper is gone. Gate tests build inputs the way SvelteKit does (stripped pathname, flag set); a handle test builds the event the same way; and a browser test on a server of its own sends a realPOST /settings/__data.json?/signoutas a must-change-password user and gets SvelteKit's redirect JSON to/account/password, no load data. I confirmed that browser test fails without the fix. The same browser file checks the real sign-out action still works for that user and thatGET /settings/__data.jsonis redirected too.throttle.ts's header, next to the existing residual: after more thanmaxKeysunknown emails from aboutmaxKeys/ 20 addresses or /64s in one window, one unknown email's next try runs the slow hash while a known Account's is refused fast. No code change.Decisions and notes for the security review
Retry-After), so neither the text nor the status says the Account limit fired. A throttled refusal is still faster than a wrong password; with unknown emails throttled alike, it no longer says which emails exist.[Signing in is a hash map lookup ...]), and a session lookup is a unique-index lookup in SQLite, not a hash map. The figure now draws the real path: token, session table row, account, progress, with a caption that says so. Right-hand "Fig. 0" and the other texts are mine, as C33b did for Sign up.ban-useris disabled) would get the generic failure text, not a reason.Files outside the C32 Files line (each needed for the card's "Done when"; please confirm)
src/lib/server/app.ts,src/lib/server/app.test.ts: the "test-switch condition" the Release gap decision names lives inchoice()here, not undersrc/routes/setup/. Thelatervariant ofMultiUserChoiceis now never produced but still in the type, with a comment:src/routes/signup/signup-gate.test.tsandsrc/routes/users/users.test.ts(the other lane's) still build{ kind: 'later' }stubs that would stop type-checking if it went. Remove it once they stop.src/routes/+layout.server.ts: one flag,showSignOut, so the layout can show Sign out to learners as well as the admin.src/routes/signup/signup.multi.e2e.ts(in the lane): its "creates nothing" check signed in over HTTP; it now posts to the Sign in form.src/routes/signin/:fresh-server.ts(a second server from the same build for the browser tests),multi-user-flow.test.ts; andsrc/routes/settings/signout.test.ts.README.md: the two stale lines, reworded at the security review (see above).Why the Sign in browser tests use servers of their own
Creating Accounts from the test process in the shared Multi-user server's database made
users.multi.e2e.ts's setup time out when both files wrote at once (reproduced twice). This file makes Accounts through the Sign up form on its own server (fresh database, Instance Multi-user and Open as C34b's toggle will leave it), so it writes nothing to the shared database. The shared server is used only for read-only checks and the admin's sign-in and sign-out.🤖 Generated with Claude Code