Repository navigation
fix(auth): prevent web users from being logged out after ~5 minutes - #831
Conversation
Web users were being aggressively logged out due to a cascading failure: socket tokens expire after 5 minutes, triggering a device token refresh that fails because login endpoints never created device tokens for web. Root causes fixed: - Socket auth error now tries getting a fresh socket token first before falling back to full device refresh (stops the 5-min cascade) - All web login endpoints (password, Google, Apple, passkey, signup) now create device tokens for session recovery - Per-device session revocation replaces revokeAllUserSessions, enabling multi-device login (logging in on phone no longer logs out laptop) - Lazy device registration endpoint covers magic link and other flows that can't return device tokens inline Security hardening: - CSRF validation + rate limiting on /api/auth/device/register - deviceId max length constraint (128) on all Zod schemas - Capacitor guard prevents web device tokens on mobile apps Includes shared helpers (revokeSessionsForLogin, createWebDeviceToken), DB migration for sessions.device_id column, and 24 new tests.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed84adcc02
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const response = await fetch('/api/auth/device/register', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, |
There was a problem hiding this comment.
The new lazy registration call uses raw fetch with only Content-Type, but /api/auth/device/register authenticates with requireCSRF: true in authenticateRequestWithOptions. For session-cookie web requests, this means the endpoint will return CSRF_TOKEN_MISSING and never issue a device token, so fallback flows (e.g. magic-link/passkey/OAuth cases that depend on lazy registration) still miss localStorage.deviceToken and remain vulnerable to forced logout during refresh.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 26055f6 — switched to post() helper which includes X-CSRF-Token automatically.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 13 minutes and 51 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (22)
📝 WalkthroughWalkthroughAdds device-aware session support: persist device_id on sessions and index it; introduce helpers for device-scoped session revocation and web device-token creation; update auth routes to handle deviceId/deviceToken; add device registration API and client lazy registration; adjust socket reconnect token flow and supporting tests/migrations. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/web/src/stores/useSocketStore.ts (1)
122-170:⚠️ Potential issue | 🟠 MajorGuard the delayed auth-recovery path against stale socket instances.
Line 122's timer survives
connect(true)anddisconnect(). If the store socket changes before the async work completes, Lines 134-137 or 168-170 can reconnect the orphanednewSocket, leaving two live connections and stale status updates.💡 Minimal guard
setTimeout(async () => { try { + const isCurrentSocket = () => get().socket === newSocket; + if (!isCurrentSocket()) { + return; + } + // Re-check desktop status const isDesktopNow = typeof window !== 'undefined' && window.electron && typeof window.electron.auth?.getSessionToken === 'function'; if (!isDesktopNow) { // Web: Try getting a fresh socket token first (session cookie may still be valid) const freshToken = await getSocketToken(); if (freshToken) { + if (!isCurrentSocket()) { + return; + } console.log('🔄 Got fresh socket token, reconnecting socket...'); newSocket.auth = { token: freshToken }; newSocket.io.opts.reconnection = true; newSocket.io.opts.reconnectionAttempts = 15; newSocket.connect(); return; } // Socket token fetch failed — session cookie likely expired, fall through to full refresh console.log('🔄 Socket token fetch failed, attempting full session refresh...'); } // Full session refresh (desktop always uses this; web falls back to it) const { refreshAuthSession, clearSessionCache } = await import('@/lib/auth/auth-fetch'); const result = await refreshAuthSession(); if (result.success) { + if (!isCurrentSocket()) { + return; + } console.log('🔄 Token refreshed via unified auth, reconnecting socket...');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/stores/useSocketStore.ts` around lines 122 - 170, The delayed recovery timer can act on a stale socket instance (newSocket) after the store's active socket has changed; capture the active socket reference at the start of the setTimeout callback (e.g., const originalSocket = newSocket) and before any mutating/reconnect steps (before setting newSocket.auth, changing newSocket.io.opts, or calling newSocket.connect()) verify the socket is still the current store socket (e.g., if (originalSocket !== socket || originalSocket.disconnected === false) return; or compare by an explicit id property), and abort the recovery if it no longer matches; apply this guard in the branches that perform reconnects (the freshToken branch that sets newSocket.auth and calls connect, and the unified auth refresh branch that updates auth/io.opts and calls connect).apps/web/src/app/api/auth/signup/route.ts (1)
209-214:⚠️ Potential issue | 🟠 MajorMask
namebeforetrackAuthEvent.This metadata is persisted, so passing raw signup PII here keeps full
namein the activity log. Use the standard masked forms before callingtrackAuthEvent.Based on learnings,
PII (email and name) must be masked before passing to trackAuthEvent to comply with data retention policies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/auth/signup/route.ts` around lines 209 - 214, Mask the PII before calling trackAuthEvent: replace the raw email and name with their masked equivalents (e.g., use the existing maskEmail(email) and maskName(name) utilities or implement them if missing) and then call trackAuthEvent(user.id, 'signup', { email: maskedEmail, name: maskedName, ip: clientIP, userAgent: req.headers.get('user-agent') }); ensure you reference the same symbols (trackAuthEvent, email, name, clientIP, req.headers.get) so the log persists only masked values.apps/web/src/app/api/auth/login/route.ts (1)
164-168:⚠️ Potential issue | 🟠 MajorMask
trackAuthEventcalls.The failed-login and success paths still write raw email into activity metadata.
maskEmail(email)is already available in this route, so use it before both calls.Based on learnings,
any auth route that calls trackAuthEvent masks the user email before logging.Also applies to: 215-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/auth/login/route.ts` around lines 164 - 168, The trackAuthEvent calls are logging the raw email; use the existing maskEmail(email) helper and pass the masked value instead of the raw email to both trackAuthEvent invocations (the failed-login path where trackAuthEvent(user?.id, 'failed_login', { reason, email: clientIP }) is called and the success path around the successful-login trackAuthEvent), and also update any related auth logging calls in the same blocks (e.g., logAuthEvent / securityAudit.logAuthFailure usage) to use the masked email when they include email in metadata so that all auth events recorded via trackAuthEvent and related log calls store maskEmail(email) rather than the raw email.
🧹 Nitpick comments (4)
packages/lib/src/auth/session-service.ts (1)
140-147: JSDoc comment is slightly misleading.The comment states "Falls back to revokeAllUserSessions when deviceId is absent" but this method requires
deviceIdas a parameter. The fallback logic actually lives in the caller (revokeSessionsForLoginin device-auth-helpers.ts). Consider updating the comment to clarify:/** * Revoke sessions for a specific device only, enabling multi-device login. - * Used by login endpoints when deviceId is available. Falls back to - * revokeAllUserSessions when deviceId is absent (backward compat). + * Used by login endpoints when deviceId is available. Callers should fall + * back to revokeAllUserSessions when deviceId is absent for backward compat. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/src/auth/session-service.ts` around lines 140 - 147, Update the JSDoc for revokeDeviceSessions to remove the misleading "Falls back to revokeAllUserSessions when deviceId is absent" line and instead state that this method requires a deviceId and only revokes sessions for that specific device via sessionRepository.revokeForUserDevice; mention that any fallback to revokeAllUserSessions is implemented by the caller (revokeSessionsForLogin) rather than inside revokeDeviceSessions.apps/web/src/app/api/auth/device/register/__tests__/route.test.ts (1)
43-136: Add a regression for the endpoint's CSRF gate.The suite covers auth, validation, rate limiting, and success, but it never proves that
POST /api/auth/device/registerrejects requests without the required CSRF protection. A 403 case, or an assertion on the auth helper options, would keep the endpoint's main hardening from regressing silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/auth/device/register/__tests__/route.test.ts` around lines 43 - 136, The test suite is missing a regression that the endpoint enforces CSRF protection; add a test that ensures POST /api/auth/device/register either returns 403 when CSRF is not provided or that authenticateRequestWithOptions is invoked with the CSRF requirement. Specifically, add a case in the describe block that (1) mocks authenticateRequestWithOptions to return a 403 Response (or error result) and asserts the POST returns status 403, or (2) calls POST and asserts vi.mocked(authenticateRequestWithOptions) was called with an options object containing the expected CSRF flag (e.g., requireCsrf: true) to lock the CSRF gate; use the existing POST helper and authenticateRequestWithOptions identifier to locate where to change tests.apps/web/src/app/api/auth/device/register/route.ts (2)
40-49: Consider returning 400 for malformed JSON bodies.If
req.json()throws (e.g., empty body, invalid JSON syntax), the error is caught by the generic catch block and returns 500. A malformed request body is a client error and should return 400.🔧 Proposed fix to handle JSON parse errors separately
try { - const body = await req.json(); + let body; + try { + body = await req.json(); + } catch { + return Response.json({ error: 'Invalid JSON body' }, { status: 400 }); + } const validation = registerDeviceSchema.safeParse(body);Also applies to: 74-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/auth/device/register/route.ts` around lines 40 - 49, The request JSON parsing can throw and currently falls through to the generic catch returning 500; wrap the await req.json() call in its own try/catch (or detect SyntaxError/JSON parsing errors) before calling registerDeviceSchema.safeParse and return Response.json({ error: 'Malformed JSON' }, { status: 400 }) on parse failure; update the same pattern around the later parsing at the other block (the second req.json() usage referenced) so both JSON-parse errors return 400 instead of hitting the generic error handler.
58-64: PassclientIPtocreateWebDeviceTokenfor audit logging.
clientIPis already obtained at line 27 for rate limiting but is not passed tocreateWebDeviceToken. The helper function acceptsipAddresswhich would be valuable for device token audit trails and security logging.🔧 Proposed fix
const deviceToken = await createWebDeviceToken({ userId: auth.userId, deviceId, tokenVersion: user.tokenVersion, deviceName: deviceName || req.headers.get('user-agent') || 'Web Browser', userAgent: req.headers.get('user-agent') || undefined, + ipAddress: clientIP || undefined, });The AI summary mentions "passing through
user-agentand client IP when available" but the code only passesuserAgent, notipAddress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/auth/device/register/route.ts` around lines 58 - 64, The call to createWebDeviceToken is missing the client IP for audit logging: pass the already-collected clientIP variable into the call using the helper's expected parameter name (ipAddress) so the object includes ipAddress: clientIP alongside userId, deviceId, tokenVersion, deviceName, and userAgent; update the createWebDeviceToken invocation in route.ts to include ipAddress: clientIP.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/app/api/auth/apple/callback/route.ts`:
- Around line 223-232: Compute a single resolved device id before calling
revokeSessionsForLogin and sessionService.createSession and use that same
identifier in the iOS token branch; specifically, resolve deviceId into a
canonical value (e.g., resolvedDeviceId or iosDeviceId) when Apple may omit
deviceId, pass resolvedDeviceId to revokeSessionsForLogin(user.id,
resolvedDeviceId, ...) and to sessionService.createSession({ ..., deviceId:
resolvedDeviceId, ... }), and reuse that same resolvedDeviceId when creating the
iOS device token later (the code that currently generates a separate iosDeviceId
in the iOS branch). Ensure the resolution logic runs once and is referenced by
revokeSessionsForLogin, createSession, and the iOS token creation path.
In `@apps/web/src/app/api/auth/google/callback/route.ts`:
- Around line 214-223: Compute a single resolvedDeviceId up front and reuse it
for revocation, session creation, and iOS token minting: before calling
revokeSessionsForLogin(user.id, deviceId, ...) and
sessionService.createSession(... deviceId ...), set const resolvedDeviceId =
deviceId ?? iosDeviceId ?? generateIosDeviceIdLikeExistingLogic() (i.e. apply
the same iOS-normalization/generation logic used later), pass resolvedDeviceId
into revokeSessionsForLogin and createSession, and when minting the iOS token
(the code that currently creates a fresh iosDeviceId in the minting block) use
the already-computed resolvedDeviceId instead of generating a new one so session
and device token share the same ID.
In `@apps/web/src/app/api/auth/passkey/authenticate/route.ts`:
- Line 113: The current call to revokeSessionsForLogin(userId, deviceId,
'passkey_login', 'passkey') makes passkey auth device-scoped; replace it with a
full cross-device session reset by calling
sessionService.revokeAllUserSessions(userId, 'passkey_login') instead (remove or
stop using revokeSessionsForLogin in this route) so passkey login performs a
hard reset of all user sessions; ensure you reference the same userId variable
and preserve any surrounding error handling or logging around the session
revocation.
In `@apps/web/src/app/api/auth/signup/route.ts`:
- Around line 289-292: The code currently appends deviceTokenValue to
redirectUrl.searchParams which exposes a recovery credential; instead remove any
use of redirectUrl.searchParams.set('deviceToken', ...) and set the token as a
SameSite cookie on the server response (e.g., via Set-Cookie) or implement a
one‑time server-side exchange (store token server-side and return a short-lived
reference). Update the signup route handler that builds redirectUrl to attach
the cookie with Secure and SameSite (and HttpOnly if JS access is not needed)
and ensure the token is invalidated/cleared after consumption so the redirect
URL contains no deviceToken query param.
In `@apps/web/src/hooks/useAuth.ts`:
- Around line 355-360: The POST to '/api/auth/device/register' uses a plain
fetch with JSON and cookies but omits the CSRF proof, causing 403s; fix by
replacing this direct fetch with the existing post() helper or by attaching the
expected CSRF header (the same header/name the app uses elsewhere) and including
the CSRF proof/token value before sending. Locate the fetch call that builds the
request body ({ deviceId, deviceName }) and either call
post('/api/auth/device/register', { deviceId, deviceName }, ...) or add the CSRF
header (matching your app's CSRF header key) populated from the same source used
elsewhere in the codebase.
---
Outside diff comments:
In `@apps/web/src/app/api/auth/login/route.ts`:
- Around line 164-168: The trackAuthEvent calls are logging the raw email; use
the existing maskEmail(email) helper and pass the masked value instead of the
raw email to both trackAuthEvent invocations (the failed-login path where
trackAuthEvent(user?.id, 'failed_login', { reason, email: clientIP }) is called
and the success path around the successful-login trackAuthEvent), and also
update any related auth logging calls in the same blocks (e.g., logAuthEvent /
securityAudit.logAuthFailure usage) to use the masked email when they include
email in metadata so that all auth events recorded via trackAuthEvent and
related log calls store maskEmail(email) rather than the raw email.
In `@apps/web/src/app/api/auth/signup/route.ts`:
- Around line 209-214: Mask the PII before calling trackAuthEvent: replace the
raw email and name with their masked equivalents (e.g., use the existing
maskEmail(email) and maskName(name) utilities or implement them if missing) and
then call trackAuthEvent(user.id, 'signup', { email: maskedEmail, name:
maskedName, ip: clientIP, userAgent: req.headers.get('user-agent') }); ensure
you reference the same symbols (trackAuthEvent, email, name, clientIP,
req.headers.get) so the log persists only masked values.
In `@apps/web/src/stores/useSocketStore.ts`:
- Around line 122-170: The delayed recovery timer can act on a stale socket
instance (newSocket) after the store's active socket has changed; capture the
active socket reference at the start of the setTimeout callback (e.g., const
originalSocket = newSocket) and before any mutating/reconnect steps (before
setting newSocket.auth, changing newSocket.io.opts, or calling
newSocket.connect()) verify the socket is still the current store socket (e.g.,
if (originalSocket !== socket || originalSocket.disconnected === false) return;
or compare by an explicit id property), and abort the recovery if it no longer
matches; apply this guard in the branches that perform reconnects (the
freshToken branch that sets newSocket.auth and calls connect, and the unified
auth refresh branch that updates auth/io.opts and calls connect).
---
Nitpick comments:
In `@apps/web/src/app/api/auth/device/register/__tests__/route.test.ts`:
- Around line 43-136: The test suite is missing a regression that the endpoint
enforces CSRF protection; add a test that ensures POST /api/auth/device/register
either returns 403 when CSRF is not provided or that
authenticateRequestWithOptions is invoked with the CSRF requirement.
Specifically, add a case in the describe block that (1) mocks
authenticateRequestWithOptions to return a 403 Response (or error result) and
asserts the POST returns status 403, or (2) calls POST and asserts
vi.mocked(authenticateRequestWithOptions) was called with an options object
containing the expected CSRF flag (e.g., requireCsrf: true) to lock the CSRF
gate; use the existing POST helper and authenticateRequestWithOptions identifier
to locate where to change tests.
In `@apps/web/src/app/api/auth/device/register/route.ts`:
- Around line 40-49: The request JSON parsing can throw and currently falls
through to the generic catch returning 500; wrap the await req.json() call in
its own try/catch (or detect SyntaxError/JSON parsing errors) before calling
registerDeviceSchema.safeParse and return Response.json({ error: 'Malformed
JSON' }, { status: 400 }) on parse failure; update the same pattern around the
later parsing at the other block (the second req.json() usage referenced) so
both JSON-parse errors return 400 instead of hitting the generic error handler.
- Around line 58-64: The call to createWebDeviceToken is missing the client IP
for audit logging: pass the already-collected clientIP variable into the call
using the helper's expected parameter name (ipAddress) so the object includes
ipAddress: clientIP alongside userId, deviceId, tokenVersion, deviceName, and
userAgent; update the createWebDeviceToken invocation in route.ts to include
ipAddress: clientIP.
In `@packages/lib/src/auth/session-service.ts`:
- Around line 140-147: Update the JSDoc for revokeDeviceSessions to remove the
misleading "Falls back to revokeAllUserSessions when deviceId is absent" line
and instead state that this method requires a deviceId and only revokes sessions
for that specific device via sessionRepository.revokeForUserDevice; mention that
any fallback to revokeAllUserSessions is implemented by the caller
(revokeSessionsForLogin) rather than inside revokeDeviceSessions.
🪄 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
Run ID: fcae36f7-6d52-47c0-bda9-30ed2ee5c77b
📒 Files selected for processing (20)
apps/web/src/app/api/auth/apple/callback/route.tsapps/web/src/app/api/auth/device/register/__tests__/route.test.tsapps/web/src/app/api/auth/device/register/route.tsapps/web/src/app/api/auth/google/callback/route.tsapps/web/src/app/api/auth/google/one-tap/route.tsapps/web/src/app/api/auth/login/route.tsapps/web/src/app/api/auth/passkey/authenticate/route.tsapps/web/src/app/api/auth/signup/route.tsapps/web/src/hooks/useAuth.tsapps/web/src/lib/auth/__tests__/device-auth-helpers.test.tsapps/web/src/lib/auth/device-auth-helpers.tsapps/web/src/lib/auth/index.tsapps/web/src/stores/useSocketStore.tspackages/db/drizzle/0093_flawless_the_captain.sqlpackages/db/drizzle/meta/0093_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/sessions.tspackages/lib/src/auth/__tests__/session-service-unit.test.tspackages/lib/src/auth/session-repository.tspackages/lib/src/auth/session-service.ts
| await revokeSessionsForLogin(user.id, deviceId, 'new_login', 'Apple OAuth'); | ||
|
|
||
| // Create new session | ||
| const sessionToken = await sessionService.createSession({ | ||
| userId: user.id, | ||
| type: 'user', | ||
| scopes: ['*'], | ||
| expiresInMs: SESSION_DURATION_MS, | ||
| deviceId, | ||
| createdByIp: clientIP !== 'unknown' ? clientIP : undefined, |
There was a problem hiding this comment.
Normalize the iOS deviceId before revocation and session creation.
When Apple returns without a deviceId, Lines 226-232 store the session with undefined, but Lines 310-321 then create the iOS device token against a generated iosDeviceId. That mismatch breaks later device-scoped revocation/recovery for the current iOS session. Compute one resolved device ID before revokeSessionsForLogin() / createSession() and reuse it in the iOS branch.
Also applies to: 309-321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/api/auth/apple/callback/route.ts` around lines 223 - 232,
Compute a single resolved device id before calling revokeSessionsForLogin and
sessionService.createSession and use that same identifier in the iOS token
branch; specifically, resolve deviceId into a canonical value (e.g.,
resolvedDeviceId or iosDeviceId) when Apple may omit deviceId, pass
resolvedDeviceId to revokeSessionsForLogin(user.id, resolvedDeviceId, ...) and
to sessionService.createSession({ ..., deviceId: resolvedDeviceId, ... }), and
reuse that same resolvedDeviceId when creating the iOS device token later (the
code that currently generates a separate iosDeviceId in the iOS branch). Ensure
the resolution logic runs once and is referenced by revokeSessionsForLogin,
createSession, and the iOS token creation path.
There was a problem hiding this comment.
Valid observation, but this is a pre-existing pattern — the iOS/desktop branches already used a separate iosDeviceId before this PR. This PR only added deviceId to createSession. The deviceId normalization for iOS is an improvement worth making but out of scope for this auth fix. Filed mentally for follow-up.
There was a problem hiding this comment.
@2witstudios, understood — thanks for the context! It makes sense to keep this PR focused on the auth fix. Would you like me to open a GitHub issue to track the iOS deviceId normalization improvement so it doesn't get lost?
(´｡• ᵕ •｡`) ♡
🧠 Learnings used
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 681
File: apps/web/src/app/api/auth/passkey/authenticate/route.ts:109-116
Timestamp: 2026-02-13T19:10:04.274Z
Learning: In `apps/web/src/app/api/auth/passkey/authenticate/route.ts`, the code intentionally calls `sessionService.revokeAllUserSessions(userId, 'passkey_login')` to revoke all existing user sessions across all devices when authenticating with a passkey. This aggressive session fixation prevention approach is a deliberate security design choice to provide strong security guarantees for passkey authentication, treating passkey login as a "hard reset" of all sessions. This behavior is logged and considered intentional, not a bug.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 790
File: apps/web/src/app/api/auth/apple/callback/route.ts:178-182
Timestamp: 2026-03-15T22:34:56.850Z
Learning: Context: The four OAuth routes (apple/callback, apple/native, google/callback, google/native) currently compute the provider as provider = user.password ? 'both' : '<provider>', which can overwrite an existing linked OAuth provider and degrade a user’s linked accounts.Action: Refactor these routes to derive the new provider from the current user.provider instead of user.password, ensuring that if a user is already linked to an OAuth provider, that linked state is preserved and merged correctly.Implementation notes:
- Compute newProvider from user.provider (e.g., if user.provider is 'google', keep it as 'google' unless a merge with another provider is intended; if merging, implement a dedicated merge policy and apply it to all four routes).
- Update any database update/upsert logic to use newProvider instead of the old conditional.
- Add/adjust unit tests to cover cases: no existing providers, single-provider linked accounts, and multi-provider merge scenarios; verify that provider state is preserved or merged as intended.
- Ensure CI checks validate provider state across all four routes and that there is a regression test for not downgrading provider on update.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 790
File: apps/web/src/app/api/auth/__tests__/login.test.ts:303-307
Timestamp: 2026-03-15T22:55:14.633Z
Learning: Ensure that any auth route that calls trackAuthEvent masks the user email before logging. Specifically, replace the raw email with a masked pattern of ${local.slice(0,3)}***@${domain} (e.g., take the local part before @, keep the first 3 chars, append *** and the same domain). Apply this consistently across auth routes, including login, signup, and similar endpoints. For the provided route (apps/web/src/app/api/auth/login/route.ts), implement the masking before trackAuthEvent. The corresponding tests (apps/web/src/app/api/auth/__tests__/login.test.ts) should be updated only after the route implementation is changed; currently the test reflects the existing route behavior and should be adjusted to align with the new masking once the route is updated.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 258
File: apps/realtime/src/validation.ts:0-0
Timestamp: 2026-01-27T03:45:52.322Z
Learning: Enforce using paralleldrive/cuid2 for ID generation across the TypeScript codebase (not UUIDs). IDs should follow the CUID2 format: lowercase alphanumeric starting with a letter, matching ^[a-z][a-z0-9]{1,31}$ with a maximum length of 32 characters. Audit code paths that generate IDs (e.g., new UUID usages) and replace with cuid2 equivalents; ensure generated IDs are consistently lowercase and validated against the regex, and document any exceptions where IDs may differ in semantic meaning.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 406
File: apps/web/src/app/api/integrations/providers/route.ts:68-77
Timestamp: 2026-02-06T16:00:02.410Z
Learning: Maintain the existing pattern of using validation.error.flatten().fieldErrors across API routes. A future, repo-wide migration to Zod v4's z.flattenError() is planned; when migrating, update all affected routes to use z.flattenError() and adjust error handling accordingly, ensuring the fieldErrors structure remains consistent.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 699
File: apps/marketing/src/app/docs/self-hosting/environment/page.tsx:34-37
Timestamp: 2026-02-18T05:15:03.695Z
Learning: Ensure that cross-subdomain cookie handling uses two environment variables: COOKIE_DOMAIN (server-side, for Set-Cookie headers in server code like apps/web/src/lib/auth/cookie-config.ts) and NEXT_PUBLIC_COOKIE_DOMAIN (client-side, for document.cookie interactions in theme-cookie.ts in both apps/web and apps/marketing). This pattern should be verified across all related files that set or rely on cookie domains to maintain consistent domain scoping and enable cross-subdomain functionality.
| await revokeSessionsForLogin(user.id, deviceId, 'new_login', 'Google OAuth'); | ||
|
|
||
| // Create new session | ||
| const sessionToken = await sessionService.createSession({ | ||
| userId: user.id, | ||
| type: 'user', | ||
| scopes: ['*'], | ||
| expiresInMs: SESSION_DURATION_MS, | ||
| deviceId, | ||
| createdByIp: clientIP !== 'unknown' ? clientIP : undefined, |
There was a problem hiding this comment.
Normalize the iOS deviceId before revocation and session creation.
When the callback arrives without a deviceId, Lines 217-223 create the session with undefined, but Lines 325-337 later mint the iOS device token against a fresh iosDeviceId. That leaves the active session and the device token keyed to different IDs, so later device-scoped revoke/recovery logic won't hit the current iOS session. Compute one resolved device ID up front and reuse it everywhere.
Also applies to: 323-337
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/api/auth/google/callback/route.ts` around lines 214 - 223,
Compute a single resolvedDeviceId up front and reuse it for revocation, session
creation, and iOS token minting: before calling revokeSessionsForLogin(user.id,
deviceId, ...) and sessionService.createSession(... deviceId ...), set const
resolvedDeviceId = deviceId ?? iosDeviceId ??
generateIosDeviceIdLikeExistingLogic() (i.e. apply the same
iOS-normalization/generation logic used later), pass resolvedDeviceId into
revokeSessionsForLogin and createSession, and when minting the iOS token (the
code that currently creates a fresh iosDeviceId in the minting block) use the
already-computed resolvedDeviceId instead of generating a new one so session and
device token share the same ID.
There was a problem hiding this comment.
Same as the Apple callback — this is a pre-existing pattern where iOS branches generate their own deviceId. The web session flow in this PR correctly passes deviceId through. iOS normalization is a follow-up improvement, not a regression from this PR.
There was a problem hiding this comment.
@2witstudios, understood — the iOS branch generating its own iosDeviceId is pre-existing behavior and the web session flow correctly propagates deviceId in this PR. I'll defer to your judgment on the iOS normalization as a follow-up.
Would you like me to open a GitHub issue to track the iOS deviceId normalization (aligning the session and device token to the same ID in the iOS branch) as a future improvement?
🧠 Learnings used
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 681
File: apps/web/src/app/api/auth/passkey/authenticate/route.ts:109-116
Timestamp: 2026-02-13T19:10:04.274Z
Learning: In `apps/web/src/app/api/auth/passkey/authenticate/route.ts`, the code intentionally calls `sessionService.revokeAllUserSessions(userId, 'passkey_login')` to revoke all existing user sessions across all devices when authenticating with a passkey. This aggressive session fixation prevention approach is a deliberate security design choice to provide strong security guarantees for passkey authentication, treating passkey login as a "hard reset" of all sessions. This behavior is logged and considered intentional, not a bug.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 790
File: apps/web/src/app/api/auth/apple/callback/route.ts:178-182
Timestamp: 2026-03-15T22:34:56.850Z
Learning: Context: The four OAuth routes (apple/callback, apple/native, google/callback, google/native) currently compute the provider as provider = user.password ? 'both' : '<provider>', which can overwrite an existing linked OAuth provider and degrade a user’s linked accounts.Action: Refactor these routes to derive the new provider from the current user.provider instead of user.password, ensuring that if a user is already linked to an OAuth provider, that linked state is preserved and merged correctly.Implementation notes:
- Compute newProvider from user.provider (e.g., if user.provider is 'google', keep it as 'google' unless a merge with another provider is intended; if merging, implement a dedicated merge policy and apply it to all four routes).
- Update any database update/upsert logic to use newProvider instead of the old conditional.
- Add/adjust unit tests to cover cases: no existing providers, single-provider linked accounts, and multi-provider merge scenarios; verify that provider state is preserved or merged as intended.
- Ensure CI checks validate provider state across all four routes and that there is a regression test for not downgrading provider on update.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 790
File: apps/web/src/app/api/auth/__tests__/login.test.ts:303-307
Timestamp: 2026-03-15T22:55:14.633Z
Learning: Ensure that any auth route that calls trackAuthEvent masks the user email before logging. Specifically, replace the raw email with a masked pattern of ${local.slice(0,3)}***@${domain} (e.g., take the local part before @, keep the first 3 chars, append *** and the same domain). Apply this consistently across auth routes, including login, signup, and similar endpoints. For the provided route (apps/web/src/app/api/auth/login/route.ts), implement the masking before trackAuthEvent. The corresponding tests (apps/web/src/app/api/auth/__tests__/login.test.ts) should be updated only after the route implementation is changed; currently the test reflects the existing route behavior and should be adjusted to align with the new masking once the route is updated.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 310
File: apps/web/src/components/layout/middle-content/page-views/channel/ChannelView.tsx:153-279
Timestamp: 2026-02-01T23:31:07.888Z
Learning: In apps/web/src/components/layout/middle-content/page-views/channel/ChannelView.tsx, the `temp-${Date.now()}` prefix pattern for optimistic reaction IDs is the preferred approach. This pattern is intentionally chosen for its clarity (makes optimistic state obvious), simplicity (reliable prefix-based detection with startsWith('temp-')), and zero runtime overhead compared to CUID2. The precise matching by emoji AND userId ensures correct replacement of temporary reactions with server-confirmed ones.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 258
File: apps/realtime/src/validation.ts:0-0
Timestamp: 2026-01-27T03:45:52.322Z
Learning: Enforce using paralleldrive/cuid2 for ID generation across the TypeScript codebase (not UUIDs). IDs should follow the CUID2 format: lowercase alphanumeric starting with a letter, matching ^[a-z][a-z0-9]{1,31}$ with a maximum length of 32 characters. Audit code paths that generate IDs (e.g., new UUID usages) and replace with cuid2 equivalents; ensure generated IDs are consistently lowercase and validated against the regex, and document any exceptions where IDs may differ in semantic meaning.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 406
File: apps/web/src/app/api/integrations/providers/route.ts:68-77
Timestamp: 2026-02-06T16:00:02.410Z
Learning: Maintain the existing pattern of using validation.error.flatten().fieldErrors across API routes. A future, repo-wide migration to Zod v4's z.flattenError() is planned; when migrating, update all affected routes to use z.flattenError() and adjust error handling accordingly, ensuring the fieldErrors structure remains consistent.
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 699
File: apps/marketing/src/app/docs/self-hosting/environment/page.tsx:34-37
Timestamp: 2026-02-18T05:15:03.695Z
Learning: Ensure that cross-subdomain cookie handling uses two environment variables: COOKIE_DOMAIN (server-side, for Set-Cookie headers in server code like apps/web/src/lib/auth/cookie-config.ts) and NEXT_PUBLIC_COOKIE_DOMAIN (client-side, for document.cookie interactions in theme-cookie.ts in both apps/web and apps/marketing). This pattern should be verified across all related files that set or rely on cookie domains to maintain consistent domain scoping and enable cross-subdomain functionality.
…evocation - Fix useAuth test: seed deviceToken in beforeEach to prevent lazy registration effect from interfering with unrelated test assertions - Fix login test: update mocks for revokeSessionsForLogin and createWebDeviceToken helpers (route no longer calls session service directly) - Add 5 new login tests: deviceToken in response, createWebDeviceToken params, no deviceToken without deviceId, graceful failure on token creation error, deviceId passed to createSession - Update revocation test to verify revokeSessionsForLogin helper call with provider and deviceId
- revokeSessionsForLogin no longer falls back to revokeAllUserSessions when deviceId is absent — returns 0 instead - Remove redundant dynamic import of getOrCreateDeviceId/getDeviceName in lazy registration (already statically imported at module level)
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/web/src/hooks/__tests__/useAuth.test.ts (1)
311-321: Consider asserting forwardeddeviceTokenin the login payload.Given this suite seeds
localStorage.deviceToken, this test can also verify the request body includes it (matchesuseAuthbehavior inapps/web/src/hooks/useAuth.ts:165-178).Diff suggestion
- const body = JSON.parse(String((init as RequestInit | undefined)?.body ?? '{}')) as { - deviceId?: string; - deviceName?: string; - }; + const body = JSON.parse(String((init as RequestInit | undefined)?.body ?? '{}')) as { + deviceId?: string; + deviceName?: string; + deviceToken?: string; + }; expect(body.deviceId).toBe('device-123'); expect(body.deviceName).toBe('Test Device'); + expect(body.deviceToken).toBe('ps_dev_existing_token');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/__tests__/useAuth.test.ts` around lines 311 - 321, The test seeds localStorage.deviceToken but doesn't assert that useAuth forwards it in the login request; update the test in useAuth.test.ts to check the fetch call for the login (the second mocked fetch) includes the seeded deviceToken in the request body (JSON payload) to mirror useAuth's behavior in useAuth.ts (see the login/registration payload logic around lines 165-178). Locate the mocked fetch calls in the test, inspect the fetch invocation arguments (body) for the login request, parse the JSON body, and add an assertion that body.deviceToken === the seeded localStorage.deviceToken.apps/web/src/app/api/auth/__tests__/login.test.ts (1)
985-1052: Add a negative-path test for the newdeviceIdcontract.These cases only exercise valid
deviceIds. Since login now accepts a persisted device identifier and the PR depends on the 128-character cap, add a 400-path assertion for an oversizeddeviceIdso schema regressions don't silently reach session creation or device-token issuance.Example assertion to add
+ it('returns 400 when deviceId exceeds 128 characters', async () => { + const request = createLoginRequest({ + ...validLoginPayload, + deviceId: 'a'.repeat(129), + }); + + const response = await POST(request); + const body = await response.json(); + + expect(response.status).toBe(400); + expect(body.errors.deviceId).toBeDefined(); + expect(sessionService.createSession).not.toHaveBeenCalled(); + expect(createWebDeviceToken).not.toHaveBeenCalled(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/auth/__tests__/login.test.ts` around lines 985 - 1052, Add a negative-path test that submits a login payload with a deviceId longer than 128 characters and asserts the handler returns a 400 and does not call sessionService.createSession or createWebDeviceToken; use the same helpers (createLoginRequest, POST, validLoginPayload) to build the request, call POST, expect response.status toBe(400), and expect sessionService.createSession.not.toHaveBeenCalled() and createWebDeviceToken.not.toHaveBeenCalled() to ensure oversized deviceIds are rejected before session/device-token logic runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/app/api/auth/__tests__/login.test.ts`:
- Around line 985-1052: Add a negative-path test that submits a login payload
with a deviceId longer than 128 characters and asserts the handler returns a 400
and does not call sessionService.createSession or createWebDeviceToken; use the
same helpers (createLoginRequest, POST, validLoginPayload) to build the request,
call POST, expect response.status toBe(400), and expect
sessionService.createSession.not.toHaveBeenCalled() and
createWebDeviceToken.not.toHaveBeenCalled() to ensure oversized deviceIds are
rejected before session/device-token logic runs.
In `@apps/web/src/hooks/__tests__/useAuth.test.ts`:
- Around line 311-321: The test seeds localStorage.deviceToken but doesn't
assert that useAuth forwards it in the login request; update the test in
useAuth.test.ts to check the fetch call for the login (the second mocked fetch)
includes the seeded deviceToken in the request body (JSON payload) to mirror
useAuth's behavior in useAuth.ts (see the login/registration payload logic
around lines 165-178). Locate the mocked fetch calls in the test, inspect the
fetch invocation arguments (body) for the login request, parse the JSON body,
and add an assertion that body.deviceToken === the seeded
localStorage.deviceToken.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9ed04ef-adb3-4519-a73e-02c9f93742f9
📒 Files selected for processing (2)
apps/web/src/app/api/auth/__tests__/login.test.tsapps/web/src/hooks/__tests__/useAuth.test.ts
|
I found four regressions in this auth/device-token change that should be addressed before merge:
These look blocking from a security/session-integrity standpoint. |
… reset - Fix CSRF on lazy device registration: use post() helper which handles CSRF automatically instead of raw fetch() (would have 403'd) - Move device tokens from redirect URL query params to short-lived SameSite cookie (ps_device_token, 60s) to avoid exposure in browser history, server logs, and referer headers - Restore passkey login to full session reset (revokeAllUserSessions) since passkey is the strongest auth flow and should hard-reset - Update client to read device token from cookie instead of URL params
|
Addressed all review comments in 26055f6: CSRF on lazy registration (P1) — Fixed. Uses Device token in redirect URLs (Major) — Fixed. Replaced URL query params with a short-lived Passkey should hard-reset (Major) — Fixed. Passkey login restored to iOS deviceId mismatch (CodeRabbit) — This is pre-existing behavior in the iOS/desktop branches, not introduced by this PR. The All 110 tests pass, TypeScript clean. |
…f, guard sockets 1. Restore revokeSessionsForLogin fallback to revokeAllUserSessions when no deviceId is present (regression from 2b26a12) 2. Fix cookie-based device token handoff: replace invalid manual `HttpOnly=false` strings with centralized createDeviceTokenHandoffCookie() using the cookie `serialize()` function 3. Device-scoped revocation now also revokes legacy sessions with NULL device_id (post-migration safety) 4. Add missing ipAddress to /api/auth/device/register endpoint 5. Guard socket store reconnect timeout against stale socket instances
Addressed all 4 blocking items + bot review feedbackFixed in Owner feedback1. deviceToken in URL query strings — Replaced URL params with a short-lived (60s) 2. revokeSessionsForLogin returns 0 without fallback — Restored the 3. Lazy device registration missing CSRF — Already fixed in 4. Legacy NULL device_id sessions survive device-scoped revocation — Updated Bot review feedback5. Passkey full-session reset (CodeRabbit) — Already fixed in 6. Missing ipAddress in /api/auth/device/register — Added 7. Stale socket guard (CodeRabbit) — Added All tests pass (3943 lib + 66 web auth + 25 useAuth hook). |
…etry - Add mocks for revokeSessionsForLogin, createWebDeviceToken, createDeviceTokenHandoffCookie across all OAuth/login test files - Update socket store + integration tests for two-phase retry: web users now try getSocketToken() first, fall back to refreshAuthSession - Update passkey test assertions for direct revokeAllUserSessions call
1 participant
Summary
/api/auth/device/registerendpoint covers magic link and other redirect-based flows that can't return device tokens inlinerevokeSessionsForLoginandcreateWebDeviceTokento eliminate duplication across auth routesRoot cause
Web login endpoints never created device tokens for browser users. When Socket.IO's 5-minute token expired, the socket reconnect handler called
refreshAuthSession()which required a device token from localStorage — but it was never stored. This returnedshouldLogout: true, dispatchingauth:expiredand forcing the user to the login page.Security
/api/auth/device/registerendpointdeviceIdmax length constraint (128 chars) on all Zod schemasSameSitecookie (60s expiry) for OAuth/signup redirects — no URL query param exposurepost()helper for automatic CSRF token injectionrevokeAllUserSessions)DB Migration
Adds nullable
device_idcolumn + index tosessionstable (0093_flawless_the_captain.sql). Runpnpm db:migratebefore deploying.Test plan
localStorage.getItem('deviceToken')is not null