feat: accounts, server sessions, and gated writes (auth PR A) - #18
Merged
Conversation
Add the auth foundation (PR A of the auth + superadmin milestone): accounts, opaque server sessions, and login/register/logout/me, then gate every existing write route on a valid session and attribute uploads to the user. Core (Users/Auth per CLAUDE.md): - `sessions` table + nullable `users.email` (0007); username uniqueness enforced on the canonical `lower(username)` form via a functional unique index (0008), so `Alice`/`alice` can never become two accounts. User + session repositories are the sole SQL layer. - `createAuthService`: register (first account → admin via an ATOMIC, advisory-locked bootstrap so concurrent first-registrations can't both win admin; Argon2id hashing; auto-login), timing-safe login, currentUser, logout, and bounded expired-session GC. Sessions are opaque 256-bit tokens; the DB stores only their sha256 hash. Typed auth errors + permission predicates (canWrite now; ownership/role reserved for PR B). API (dual transport for API-mode clients): - Session token accepted via `Authorization: Bearer` OR the httpOnly `bunbooru_session` cookie; login/register set the cookie AND return the token. A present-but-malformed Authorization header does NOT fall back to the cookie. - `/auth/register|login|logout|me`; `requireUser` gates POST/PATCH /assets, PATCH /assets/:id/tags, and all /uploads routes; uploads carry `uploaderId`. - `SESSION_EXPIRY_MS` (30d, >= 1s) + `SESSION_GC_INTERVAL_MS` (1h) env + a session GC sweep. `PublicUser` serialization never leaks the password hash. Web: - `useCurrentUser`/`useLogin`/`useRegister`/`useLogout` (auth rides the cookie; `/auth/me` is the source of truth), login + signup pages (errors in a live region), header account state, and write UIs (upload, post edit, tag edit) gated on login — the editors also close + reset if auth drops mid-edit. Unique-violation detection walks the error cause chain and accepts the SQLSTATE on either `.code` (node-postgres) or `.errno` (Bun native SQL), so a duplicate registration returns 409 rather than 500. Covered by db-integration (incl. the atomic bootstrap + case-insensitive uniqueness), core-unit, and API-route tests (Bearer + cookie transports). Deferred to PR B: binding upload-session ops (offset/append/cancel) and post edits to the owner via isOwnerOrAdmin, and per-IP/user rate-limiting on login/registration. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds session-based authentication end to end: DB tables and repositories, core auth/session logic, API auth routes and write gating, and web login/signup plus auth-aware UI behavior. ChangesSession Authentication Feature
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
packages/db/test/auth-repositories.test.ts (1)
72-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise
createBootstrapping()concurrently.This only proves sequential role assignment. A regression that drops
pg_advisory_xact_lock()would still pass here, so the first-user admin guarantee is still effectively untested.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/test/auth-repositories.test.ts` around lines 72 - 86, The current createBootstrapping test only checks sequential calls, so it does not verify the first-user admin guarantee under concurrency. Update the test in auth-repositories.test.ts to exercise users.createBootstrapping concurrently with multiple requests and assert that only one account gets the admin role while the others become members; keep the focus on the createBootstrapping behavior so a missing pg_advisory_xact_lock() would fail the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/server.ts`:
- Around line 206-245: The browser-facing auth handlers in server.ts should stop
returning the raw session token in the JSON response. Update the /auth/register
and /auth/login routes to continue setting the HttpOnly cookie via
buildSessionCookie, but only return the serialized user payload from
authService.register and authService.login. Keep the token internal to the
cookie/session flow and use serializeUser plus /auth/me for auth state.
In `@apps/web/src/lib/auth.ts`:
- Around line 37-50: The auth query in useCurrentUser() can remain stale for too
long after logout or session expiry, so update the auth refresh strategy in
auth.ts. Either reduce the staleTime on the CURRENT_USER_KEY query or add an
explicit invalidation/refetch path so useIsLoggedIn() and related edit-gating
state update promptly across tabs and after auth changes.
In `@apps/web/src/routes/upload.tsx`:
- Around line 31-33: The upload page currently treats any non-pending auth
result as anonymous, so a failed /auth/me request incorrectly shows the
login/signup CTA and blocks uploads. Update the auth gating in upload.tsx around
useCurrentUser and the upload prompt logic to handle the auth query error state
separately from a true unauthenticated user. In the branch that decides between
the dropzone and the anonymous-upload CTA, only show the CTA when the query
resolved successfully with no user; if auth errored, keep the upload flow from
falling back to the anonymous prompt and surface or preserve the signed-in path
instead.
In `@packages/core/src/services/auth-service.ts`:
- Around line 121-133: Make signup atomic in auth-service by ensuring
`users.createBootstrapping()` and `openSession()` succeed or fail together.
Update `register()` (and the `openSession`/`sessions.create` flow it calls) to
run both writes in a single transaction, or add rollback/cleanup if session
creation fails after the user is created. Keep the unique-violation handling in
`isUniqueViolation()` and `RegistrationConflictError`, but make sure a failed
session open does not leave a committed user behind.
- Around line 52-53: The username normalization path currently allows
whitespace-only input to become an empty string and be persisted. Update
normalizeUsername and the register() flow in auth-service to validate the
normalized result before hashing/inserting, and reject empty normalized
usernames for all callers with a clear validation error instead of continuing to
persistence.
In `@packages/core/test/auth-service.test.ts`:
- Around line 120-122: The `expect(...).rejects` checks in
`auth-service.test.ts` are not being awaited, so the test can complete before
the rejection assertion runs. Update the affected assertions in the `register`
test and the other nearby `rejects` checks to await them directly, using the
existing `service.register` and `RegistrationConflictError` assertions so the
async expectation is guaranteed to execute.
In `@packages/db/drizzle/0008_canonical_username_unique.sql`:
- Around line 1-2: The migration in the users canonical-username change needs a
preflight check for existing case-colliding values before removing the old
uniqueness constraint. Update the SQL in the canonical username migration to add
a guard step ahead of ALTER TABLE "users" DROP CONSTRAINT
"users_username_unique", using a DO block or equivalent to detect duplicate
lower(username) groups and raise a clear exception if any exist, so the rollout
fails early instead of during CREATE UNIQUE INDEX "users_username_lower_idx".
In `@packages/db/src/repositories/session-repository.ts`:
- Around line 40-45: The session cleanup logic in deleteExpired() should use the
same expiry cutoff as findValidByTokenHash(), which means treating expiresAt ===
now as expired too. Update the session repository’s deleteExpired flow (and any
shared predicate it uses) to delete rows with expiresAt less than or equal to
now, matching the gt(sessions.expiresAt, now) auth lookup behavior in
findValidByTokenHash(). Make the change in the session-repository methods so
both GC and validation stay consistent.
In `@packages/db/src/repositories/user-repository.ts`:
- Around line 47-57: Canonicalize usernames inside the repository so writes and
reads use the same normalized form. Update createBootstrapping and any username
lookup in findByUsername to normalize via lowercasing before insert/select,
rather than relying on callers to do it. Make the repository’s
createBootstrapping and findByUsername paths consistent with the lower(username)
unique index so mixed-case usernames like Alice can still be found with alice.
---
Nitpick comments:
In `@packages/db/test/auth-repositories.test.ts`:
- Around line 72-86: The current createBootstrapping test only checks sequential
calls, so it does not verify the first-user admin guarantee under concurrency.
Update the test in auth-repositories.test.ts to exercise
users.createBootstrapping concurrently with multiple requests and assert that
only one account gets the admin role while the others become members; keep the
focus on the createBootstrapping behavior so a missing pg_advisory_xact_lock()
would fail the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b97bb53d-5039-46b8-af23-c34332ea3f21
📒 Files selected for processing (33)
.env.exampleapps/api/src/env-config.tsapps/api/src/index.tsapps/api/src/lib/auth.tsapps/api/src/lib/http.tsapps/api/src/server.tsapps/api/test/server.test.tsapps/web/src/components/tags/post-tag-panel.tsxapps/web/src/lib/api.tsapps/web/src/lib/auth.tsapps/web/src/router.tsxapps/web/src/routes/__root.tsxapps/web/src/routes/login.tsxapps/web/src/routes/post-detail.tsxapps/web/src/routes/signup.tsxapps/web/src/routes/upload.tsxpackages/core/src/core.tspackages/core/src/errors.tspackages/core/src/index.tspackages/core/src/services/auth-service.tspackages/core/src/services/permissions.tspackages/core/test/auth-service.test.tspackages/db/drizzle/0007_auth_sessions.sqlpackages/db/drizzle/0008_canonical_username_unique.sqlpackages/db/drizzle/meta/0007_snapshot.jsonpackages/db/drizzle/meta/0008_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/index.tspackages/db/src/repositories/session-repository.tspackages/db/src/repositories/user-repository.tspackages/db/src/schema.tspackages/db/test/auth-repositories.test.tsscripts/seed.ts
Comment on lines
+206
to
+245
| .post( | ||
| "/auth/register", | ||
| async ({ body, set }) => { | ||
| const email = body.email?.trim(); | ||
| const { token, user } = await core.authService.register({ | ||
| username: body.username, | ||
| password: body.password, | ||
| email: email ? email : null, | ||
| }); | ||
| set.headers["set-cookie"] = buildSessionCookie(token, envConfig.SESSION_EXPIRY_MS, { | ||
| secure: envConfig.COOKIE_SECURE, | ||
| }); | ||
| set.status = 201; | ||
| return { user: serializeUser(user), token }; | ||
| }, | ||
| { | ||
| body: t.Object({ | ||
| username: t.String({ minLength: 1, maxLength: 100 }), | ||
| password: t.String({ minLength: 8, maxLength: 200 }), | ||
| email: t.Optional(t.String({ maxLength: 320 })), | ||
| }), | ||
| }, | ||
| ) | ||
| // Verify credentials and open a session (sets cookie + returns token). | ||
| .post( | ||
| "/auth/login", | ||
| async ({ body, set }) => { | ||
| const { token, user } = await core.authService.login(body.username, body.password); | ||
| set.headers["set-cookie"] = buildSessionCookie(token, envConfig.SESSION_EXPIRY_MS, { | ||
| secure: envConfig.COOKIE_SECURE, | ||
| }); | ||
| return { user: serializeUser(user), token }; | ||
| }, | ||
| { | ||
| body: t.Object({ | ||
| username: t.String({ minLength: 1, maxLength: 100 }), | ||
| password: t.String({ minLength: 1, maxLength: 200 }), | ||
| }), | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Don't return the raw session token from the browser-facing auth endpoints.
These handlers already set the HttpOnly session cookie, and the web flow in this PR learns auth state via /auth/me. Returning the same bearer token in JSON makes it readable to any injected script, which defeats the cookie-only protection and leaves the session reusable even after the cookie is cleared locally.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/server.ts` around lines 206 - 245, The browser-facing auth
handlers in server.ts should stop returning the raw session token in the JSON
response. Update the /auth/register and /auth/login routes to continue setting
the HttpOnly cookie via buildSessionCookie, but only return the serialized user
payload from authService.register and authService.login. Keep the token internal
to the cookie/session flow and use serializeUser plus /auth/me for auth state.
Comment on lines
+52
to
+53
| function normalizeUsername(username: string): string { | ||
| return username.trim().toLowerCase(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject usernames that normalize to empty strings.
register() lowercases and trims before insert, so " " becomes "" and can be persisted as a real username. Please validate the normalized value before hashing/inserting so whitespace-only usernames fail for every caller, not just the HTTP boundary.
Also applies to: 113-124
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/services/auth-service.ts` around lines 52 - 53, The
username normalization path currently allows whitespace-only input to become an
empty string and be persisted. Update normalizeUsername and the register() flow
in auth-service to validate the normalized result before hashing/inserting, and
reject empty normalized usernames for all callers with a clear validation error
instead of continuing to persistence.
Comment on lines
+121
to
+133
| try { | ||
| user = await users.createBootstrapping({ | ||
| username: normalized, | ||
| email: email ?? null, | ||
| passwordHash, | ||
| }); | ||
| } catch (error) { | ||
| if (isUniqueViolation(error)) throw new RegistrationConflictError(); | ||
| throw error; | ||
| } | ||
|
|
||
| const token = await openSession(user.id); | ||
| return { token, user }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make signup atomic with session creation.
users.createBootstrapping() commits the account before openSession() runs. If sessions.create() fails, register() returns an error even though the user row already exists, so a retry turns into a conflict while the caller thinks signup never completed. Please wrap both writes in one transaction or add compensating cleanup on session-open failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/services/auth-service.ts` around lines 121 - 133, Make
signup atomic in auth-service by ensuring `users.createBootstrapping()` and
`openSession()` succeed or fail together. Update `register()` (and the
`openSession`/`sessions.create` flow it calls) to run both writes in a single
transaction, or add rollback/cleanup if session creation fails after the user is
created. Keep the unique-violation handling in `isUniqueViolation()` and
`RegistrationConflictError`, but make sure a failed session open does not leave
a committed user behind.
- Reject whitespace-only usernames at the API boundary (username pattern requires a non-whitespace char), so they can't persist as an empty username. - Canonicalize usernames inside the user repository (store lowercase on createBootstrapping; findByUsername matches lower(username)) so identity is case-insensitive regardless of caller casing and aligned with the unique index. - Align session GC with the auth lookup: deleteExpired now removes `expiresAt <= now` (a session expiring exactly at `now` reads as expired, so GC reclaims it). - Guard migration 0008 with a preflight DO block that raises a clear error if pre-existing case-colliding usernames would break CREATE UNIQUE INDEX. - Upload page distinguishes an /auth/me error from a logged-out user (no more anonymous CTA blocking a possibly-signed-in user on a transient auth failure). - Auth query: shorter staleTime + refetch-on-focus so logout/expiry elsewhere is picked up promptly and edit gates re-evaluate. - Tests: await the `.rejects` assertions (they could pass without running); add a concurrency test proving the advisory-locked bootstrap yields exactly one admin, a case-insensitive username lookup test, and a 422 whitespace- username test. Intentionally kept: login/register still return the session token in the body — that's the approved dual-transport design so non-browser (Bearer) clients can obtain a token; the web ignores it and uses the httpOnly cookie. Deferred to PR B: wrapping user+session creation in one transaction (rare, recoverable — the account exists and the user can log in; a clean fix needs transaction-aware repositories). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
Addressed the CodeRabbit review in Two findings I deliberately did not change:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/db/src/repositories/user-repository.ts (1)
13-17: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCanonicalization is still split across repository entrypoints.
The new repository-level contract says usernames are canonicalized here, but only
createBootstrapping()lowercases before insert.create()still persists caller casing, so seeded/admin-created rows can bypass the invariant thatfindByUsername()now assumes. Either canonicalizecreate()too or narrow this contract to the bootstrapping path.Suggested alignment
async create(input) { - const [row] = await db.insert(users).values(input).returning(); + const [row] = await db + .insert(users) + .values({ ...input, username: input.username.toLowerCase() }) + .returning(); if (!row) { throw new Error("user insert returned no row"); } return row; },Also applies to: 57-59, 73-79
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/src/repositories/user-repository.ts` around lines 13 - 17, The username canonicalization contract in UserRepository is inconsistent across entrypoints: createBootstrapping() lowercases input, but create() still stores caller casing while findByUsername() assumes lowercased identities. Update UserRepository.create() to canonicalize usernames the same way as the bootstrapping path, or else tighten the repository comment/contract to apply only to createBootstrapping() and keep findByUsername() aligned with that behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/db/src/repositories/user-repository.ts`:
- Around line 13-17: The username canonicalization contract in UserRepository is
inconsistent across entrypoints: createBootstrapping() lowercases input, but
create() still stores caller casing while findByUsername() assumes lowercased
identities. Update UserRepository.create() to canonicalize usernames the same
way as the bootstrapping path, or else tighten the repository comment/contract
to apply only to createBootstrapping() and keep findByUsername() aligned with
that behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f61d9e6-5a45-4039-bf96-add799912a30
📒 Files selected for processing (9)
apps/api/src/server.tsapps/api/test/server.test.tsapps/web/src/lib/auth.tsapps/web/src/routes/upload.tsxpackages/core/test/auth-service.test.tspackages/db/drizzle/0008_canonical_username_unique.sqlpackages/db/src/repositories/session-repository.tspackages/db/src/repositories/user-repository.tspackages/db/test/auth-repositories.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/web/src/routes/upload.tsx
- packages/core/test/auth-service.test.ts
- apps/web/src/lib/auth.ts
- packages/db/src/repositories/session-repository.ts
- apps/api/test/server.test.ts
- apps/api/src/server.ts
Follow-up to the PR #18 review: `create()` stored the caller's casing while `createBootstrapping()` lowercased and `findByUsername()` matched `lower(username)` — an inconsistent contract. `create()` now lowercases the username too, so all write paths store the canonical form the doc/index promise. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR A of the auth + superadmin milestone: accounts, opaque server sessions, and login/register/logout/me, then gate every existing write route on a valid session and attribute uploads to the user. (PR B — superadmin powers + runtime settings — follows.)
Auth lives in Core (CLAUDE.md: Core owns Users/Auth/Permissions);
createCorealready encapsulates thedb → repo → servicewiring, so no boundary edges change.Design
expires_at); a bounded periodic sweep reclaims them.Authorization: Bearer <token>OR the httpOnlybunbooru_sessioncookie. Login/register set the cookie and return the token in the JSON body (so scripts/mobile can capture it). A present-but-malformedAuthorizationheader does not fall back to the cookie.admin(assigned atomically under a Postgres advisory lock so two concurrent first-registrations can't both win admin), the restmember. Register auto-logs-in.isOwnerOrAdminis built but not yet enforced.Bun.password);PublicUserserialization never leaks the hash. Login is timing-safe against username enumeration.HttpOnly,SameSite=Lax,Securein production,Path=/,Max-Age= session lifetime.Schema (migrations 0007 + 0008)
sessionstable (token_hashunique,user_idFK ON DELETE CASCADE,expires_at, indexes on user + expiry).users.emailmade nullable (registration needs only username + password).lower(username)form via a functional unique index —Alice/alicecan never become two accounts.API
/auth/register(201, sets cookie, returns{ user, token }, no hash),/auth/login,/auth/logout(204, clears cookie),/auth/me({ user | null }).requireUsergatesPOST /assets,PATCH /assets/:id,PATCH /assets/:id/tags, and all/uploadsroutes; uploads threaduploaderId.SESSION_EXPIRY_MS(30d, ≥ 1s) andSESSION_GC_INTERVAL_MS(1h) + a session-GC sweep.Web
useCurrentUser/useLogin/useRegister/useLogout(auth rides the cookie;/auth/meis the source of truth). Login + signup pages (errors in a live region), header account state, and write UIs (upload, post-edit, tag-edit) gated on login — editors close + reset if auth drops mid-edit.Testing
typecheck✅,lint:boundaries✅, full suite 222 pass / 0 fail including DB-integration tests against real Postgres (session validity/expiry, revocation, bounded GC, FK cascade, case-insensitive uniqueness, atomic admin bootstrap), core-unit, and API-route tests (Bearer + cookie transports, 401 gating,uploaderIdattribution, 409 on duplicate).app.handleagainst Postgres, and the advisory-locked bootstrap proven under 8-way concurrency (exactly one admin).Notes / caught during review
cause.errno(not.code, which isERR_POSTGRES_SERVER_ERROR);isUniqueViolationwalks the cause chain and checks both, so a duplicate registration returns 409 not 500.Deferred to PR B
isOwnerOrAdmin.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Securein production.Bug Fixes