Skip to content

feat(beyondleetcode): C32 Sign in and sign out - #185

Merged
GauranshMathur merged 8 commits into
mainfrom
C32-signin-signout
Oct 16, 2026
Merged

GauranshMathur merged 8 commits into
mainfrom
C32-signin-signout

Conversation

@GauranshMathur

@GauranshMathur GauranshMathur commented Oct 16, 2026 •

Copy link
Copy Markdown
Owner

Card C32 (access control: security review before merge). Worker: builder-signin.

A signed-out visitor to any page lands on /signin and, 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 through createApp, 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 unchanged
  • throttle.ts: emails with no Account are limited like known ones, in separate evicting maps (see "From the automated review")
  • auth.ts: /sign-in/email added to disabledPaths; the limiter and anti-framing tests moved to live endpoints (change-password, sign-up/email, get-session); server-side auth.api.signInEmail still works
  • gate.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 action
  • signin/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, next validated by safeNext plus never /signin or /signup, non-ASCII percent-encoded for the Location header
  • signin/+page.svelte to 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 request
  • Sign out: POST form action signout on Settings (/settings?/signout), revokes the session through auth.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
  • Nav hidden on /signin, /signup and /setup
  • Setup: Multi-user on for everyone (no test-switch condition, no "next update" state, with its tests)
  • Tests: Vitest 881 pass; Playwright 91 pass locally under the shared lock; typecheck and Biome clean
  • Self-review done against the card, the orchestrator notes, CONTEXT.md and docs/module-map.md
  • Codex second opinion: skipped, codex is not installed on this machine
  • CI green; ready for security review (not merged by the worker)

From the automated review

  1. Window-aware give-back. An attempt counted in one window and succeeding after a reset used to take a count from the new window. 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.
  2. Throttling must not reveal which emails have an Account. A throttled refusal skips the password hash and answers faster, so only known emails reaching the throttle would be a timing oracle. Unknown emails now reach the throttled state on the same schedule: 5 per address, 50 overall, 20 per address across Accounts. Better Auth 1.7.7 already hashes the password when the email is unknown (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.
  3. Unknown emails never share space with real Accounts. My first version keyed unknown emails into the Account and pair maps, where a flood of made-up emails could fill a map and make real Accounts go uncounted (about 10,000 attempts, 500 addresses, cheap for a /48). Now the throttle takes unknownEmail (a SHA-256 of the lowercased email, so the address is never held) as a separate field from accountId, 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 at maxKeys and 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 of throttle.ts): to make one unknown email forget its count an attacker must push it out with more than maxKeys other emails per fresh set of guesses. Tests: past maxKeys the unknown maps stay at maxKeys; 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.
  4. Registration Mode per request. The Sign in load calls 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; its users.test.ts still builds the later stub, so the unused variant stays.

  1. No redirect loop on /account/password. That page has no route yet (routeId null), so a user who must change their password was sent to a path that redirected to itself. In gate.ts, routeId null with the exact pathname /account/password now counts as arriving, so the 404 page shows. Any other unrouted path (including /account/password/extra and /account/passwords) still redirects there. Tests in gate.test.ts and through the real hook in handle.test.ts.
  2. No Settings data through the sign-out exception. The must-change-password exception for /settings?/signout no longer applies to a data request. My first version tested the pathname for /__data.json, which cannot work: SvelteKit strips that suffix from event.url.pathname before handle runs and sets event.isDataRequest 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; render_data runs for any method and never runs an action). GateInput now carries isDataRequest, handle.ts passes event.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 real POST /settings/__data.json?/signout as 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 that GET /settings/__data.json is redirected too.
  3. Flood timing residual documented in throttle.ts's header, next to the existing residual: after more than maxKeys unknown emails from about maxKeys / 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.
  4. README.md lines 20-21 and 27 now say Multi-user can be chosen at Setup (and that Setup needs the secret and origin), with no "next update" wording. Nothing else in the README changed.

Decisions and notes for the security review

  • Status and message are identical for wrong password, no such account, a blank form and too many attempts (status 400, no 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.
  • Copy. The board's figure caption is a bracketed placeholder about a hash map ([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.
  • A failed sign-out (Better Auth throws) shows "Signing out failed. Try again." and sets no cookie; Better Auth itself swallows a failed row delete and still clears the cookie (library behaviour, noted).
  • Banned user (not reachable in MVP1, ban-user is 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 in choice() here, not under src/routes/setup/. The later variant of MultiUserChoice is now never produced but still in the type, with a comment: src/routes/signup/signup-gate.test.ts and src/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.
  • New test-only files under src/routes/signin/: fresh-server.ts (a second server from the same build for the browser tests), multi-user-flow.test.ts; and src/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

GauranshMathur and others added 5 commits October 16, 2026 10:19
… 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>
@GauranshMathur
GauranshMathur marked this pull request as ready for review October 16, 2026 10:47
@GauranshMathur

Copy link
Copy Markdown
Owner Author

Security review (orchestrator-dispatched, 2026-10-16): verdict merge, after four small changes.

All nine decisions held:

  • next is same-origin only.
  • Sign out is a POST that revokes the session and forwards Better Auth's clearing cookie.
  • A fresh Instance can choose Multi-user without the test switch.
  • The throttle is keyed on address plus Account, the sixth try is refused, and one message covers every failure.
  • A successful sign-in gives its count back only inside its own window.
  • Unknown emails have their own maps, which evict, apart from real Accounts, which never do.
  • Unknown pages are private in multi.
  • /sign-in/email is off over HTTP, and no other password sign-in route is enabled.
  • The Sign up link reads the mode on each request.

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: /change-password, which can be used to guess a password with a stolen session, goes to C36b. Accepted: Sign up shows that an email is taken when registration is Open. Codex second opinion did not run (not installed).

🤖 Generated with Claude Code

GauranshMathur and others added 3 commits October 16, 2026 11:00
… 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>
@GauranshMathur

Copy link
Copy Markdown
Owner Author

Pre-merge fixes verified (orchestrator, 2026-10-16) at 9fede2b, which includes main. The must-change-password sign-out exception now uses SvelteKit's event.isDataRequest, not the path, which SvelteKit has already stripped. A browser test sends a real POST /settings/__data.json?/signout and gets no load data. The /account/password redirect loop is gone. The flood residual is documented, and the README says Multi-user can be chosen at Setup. CI green. Merging.

🤖 Generated with Claude Code

@GauranshMathur
GauranshMathur merged commit 85edabd into main Oct 16, 2026
10 checks passed
@GauranshMathur
GauranshMathur deleted the C32-signin-signout branch October 16, 2026 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant