Conversation
hugperez
requested review from
DecampsRenan,
ivan-dalmet,
ntatoud and
yoannfleurydev
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces a refactoring of session management within the application. Key changes include the addition of a new environment variable for session expiration, the removal of the Lucia authentication library, and the relocation and reimplementation of session-related functions into a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant Server
participant SessionManager
participant Database
Client->>Server: Request with session token
Server->>SessionManager: Validate session
SessionManager->>Database: Check session existence
Database-->>SessionManager: Session details
alt Session Valid
SessionManager->>SessionManager: Refresh session expiration
SessionManager-->>Server: Valid session
Server-->>Client: Process request
else Session Invalid
SessionManager->>Database: Clear session
SessionManager-->>Server: Invalid session
Server-->>Client: Authentication failed
end
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/server/routers/auth.tsx (1)
210-214: Update log message to reflect session invalidationThe log message
Delete auth cookiedoes not accurately describe the current action, which now includes invalidating the session in the database and clearing the cookie. Updating the log message will improve clarity and maintain accurate logging.Apply this diff to update the log message:
ctx.logger.info('Delete auth cookie'); + ctx.logger.info('Invalidating session and clearing auth cookie');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.env.example(1 hunks)package.json(1 hunks)src/env.mjs(2 hunks)src/server/config/auth.ts(0 hunks)src/server/config/lucia.ts(0 hunks)src/server/config/session.ts(1 hunks)src/server/config/trpc.ts(2 hunks)src/server/routers/auth.tsx(2 hunks)src/server/routers/oauth.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- src/server/config/auth.ts
- src/server/config/lucia.ts
✅ Files skipped from review due to trivial changes (1)
- src/server/routers/oauth.tsx
🔇 Additional comments (5)
src/env.mjs (2)
14-14: Addition ofSESSION_EXPIRATION_SECONDSlooks goodThe environment variable
SESSION_EXPIRATION_SECONDSis correctly added to the server-side schema usingz.coerce.number().int(), ensuring it is an integer.
82-82:SESSION_EXPIRATION_SECONDSincluded in runtime environment configurationThe environment variable is appropriately included in the
runtimeEnv, allowing it to be accessed at runtime.src/server/config/trpc.ts (1)
22-22: Verify the implementation of getCurrentSession.The migration from Lucia auth to a custom session management solution requires careful verification of the new implementation.
Run the following script to analyze the session management implementation:
Also applies to: 43-43
✅ Verification successful
Session management implementation verified and secure
The new session management implementation follows security best practices with:
- Secure token generation and validation
- Database-backed session storage
- Proper session lifecycle management
- Automatic session refresh mechanism
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the session management implementation # Check for proper token validation, session expiration handling, and secure storage # Check the session implementation echo "Checking session implementation..." ast-grep --pattern 'function getCurrentSession() { $$$ }' # Check for proper session validation echo "Checking session validation..." rg -A 10 "validateSession|verifySession|checkSession" # Check for secure token handling echo "Checking token handling..." rg -A 5 "token.*=|generateToken|createToken"Length of output: 8277
package.json (1)
56-57: 🛠️ Refactor suggestionPin dependency versions for security.
Using caret (^) in version constraints could lead to automatic updates with breaking changes. For security-critical dependencies, it's recommended to pin exact versions.
Apply this diff:
- "@oslojs/crypto": "^1.0.1", - "@oslojs/encoding": "^1.1.0", + "@oslojs/crypto": "1.0.1", + "@oslojs/encoding": "1.1.0",Also, verify these dependencies for known security vulnerabilities:
.env.example (1)
19-21: Consider reducing the default session expiration time.A 30-day session expiration time might be too long from a security perspective. Consider:
- Reducing the default to 24 hours (86400 seconds)
- Adding separate expiration times for different environments
- Implementing session refresh tokens for longer-term persistence
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/server/config/session.ts (3)
1-16:⚠️ Potential issueCritical: Remove caching of session validation.
Based on previous discussions, using React's
cacheutility with session validation can lead to security issues by serving stale session data.Additionally, the imports and constants look good:
- The 20-byte entropy size provides sufficient security
- The cookie name constant improves maintainability
74-74:⚠️ Potential issueMissing
awaitwhen calling asynchronousrefreshSessionfunction.The
refreshSessionfunction is asynchronous, but it's called withoutawait.
120-124:⚠️ Potential issueCritical: Incorrect usage of
getRandomValuesin Node.js environment.The
getRandomValuesmethod fromnode:cryptois not the recommended way to generate random values in Node.js.
🧹 Nitpick comments (2)
src/env.mjs (1)
14-14: LGTM! Consider adding a comment for the default value.The implementation of
SESSION_EXPIRATION_SECONDSis correct. Consider adding a comment explaining that the default value (2592000) represents 30 days, making it more maintainable.- SESSION_EXPIRATION_SECONDS: z.coerce.number().int().default(2592000), + // Default session expiry: 30 days (30 * 24 * 60 * 60 seconds) + SESSION_EXPIRATION_SECONDS: z.coerce.number().int().default(2592000),src/server/config/session.ts (1)
18-33: Improve Bearer token extraction robustness.While the current implementation works, the Bearer token extraction could be more robust against malformed inputs.
- headers().get('Authorization')?.split('Bearer ')[1] ?? + headers() + .get('Authorization') + ?.match(/^Bearer\s+(.*)$/)?.[1] ??
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
src/env.mjs(2 hunks)src/server/config/session.ts(1 hunks)src/server/config/trpc.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/config/trpc.ts
🧰 Additional context used
📓 Learnings (1)
src/server/config/session.ts (1)
Learnt from: hugperez
PR: BearStudio/start-ui-web#555
File: src/server/config/session.ts:17-32
Timestamp: 2025-01-16T13:25:36.984Z
Learning: React's `cache` utility should not be used with authentication-related functions like session validation, as it can lead to stale session data being served, creating potential security issues. Session state should always be fresh and validated on each request.
🔇 Additional comments (3)
src/server/config/session.ts (3)
83-101: LGTM! Well-implemented session refresh mechanism.The half-life based refresh mechanism is a good approach that:
- Reduces database load by not refreshing on every request
- Maintains security by regularly updating session expiration
126-129: LGTM! Secure implementation of session token hashing.The implementation correctly hashes the session token using SHA-256 before storage, which prevents token leakage in case of database compromise.
131-149: LGTM! Secure cookie management implementation.The cookie management implementation follows security best practices:
- HttpOnly flag prevents XSS attacks
- Secure flag in production prevents MITM attacks
- SameSite=lax prevents CSRF attacks
- Proper cookie clearing with maxAge=0
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/server/config/session.ts (2)
1-1:⚠️ Potential issueRemove React cache to prevent stale session data
Based on the provided learnings, using React's
cacheutility with session validation can lead to security issues by serving stale session data.Apply this diff to fix the issue:
-import { cache } from 'react'; -export const getCurrentSession = cache( +export async function getCurrentSession( async (): Promise<SessionValidationResult> => { // ... function body ... - } -); + }Also applies to: 18-33
82-82:⚠️ Potential issueMissing
awaitwhen calling asynchronousrefreshSessionfunctionThe
refreshSessionfunction is asynchronous, but it's called withoutawait. This could lead to race conditions and incorrect session handling.Apply this diff to fix the issue:
- await refreshSession(session); + await refreshSession(session); return { session, user };
🧹 Nitpick comments (2)
src/server/config/session.ts (2)
139-147: Enhance cookie security with stricter SameSite policyConsider using 'Strict' instead of 'Lax' for the SameSite attribute to provide stronger protection against CSRF attacks.
Apply this diff to enhance security:
cookies().set(AUTH_COOKIE_NAME, token, { httpOnly: true, - sameSite: 'lax', + sameSite: 'strict', secure: env.NODE_ENV === 'production', expires: expiresAt, path: '/', });
91-109: Simplify session refresh logic for better readabilityThe current implementation of half-life based refresh is complex and could be made more maintainable.
Consider this clearer implementation:
export async function refreshSession(session: Session): Promise<void> { + const halfLife = env.SESSION_EXPIRATION_SECONDS * 500; // half of expiration in ms + const timeUntilExpiry = session.expiresAt.getTime() - Date.now(); + - if ( - Date.now() >= - session.expiresAt.getTime() - (1000 * env.SESSION_EXPIRATION_SECONDS) / 2 - ) { + if (timeUntilExpiry <= halfLife) { + const newExpiryTime = Date.now() + env.SESSION_EXPIRATION_SECONDS * 1000; - session.expiresAt = new Date( - Date.now() + 1000 * env.SESSION_EXPIRATION_SECONDS - ); + session.expiresAt = new Date(newExpiryTime); await db.session.update({ where: { id: session.id }, data: { expiresAt: session.expiresAt }, }); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/server/config/session.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/server/config/session.ts (1)
Learnt from: hugperez
PR: BearStudio/start-ui-web#555
File: src/server/config/session.ts:17-32
Timestamp: 2025-01-16T13:25:36.984Z
Learning: React's `cache` utility should not be used with authentication-related functions like session validation, as it can lead to stale session data being served, creating potential security issues. Session state should always be fresh and validated on each request.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Playwright E2E Tests
🔇 Additional comments (1)
src/server/config/session.ts (1)
1-161: Add comprehensive test coverage for session managementThis module handles critical security functionality and requires thorough testing. Consider adding tests for:
- Session creation and validation
- Token generation and comparison
- Cookie handling
- Refresh mechanism
- Error cases
Would you like me to help generate a test suite for this module?
3 participants
Describe your changes
As Lucia v3 will be deprecated in March 2025, I updated to implement our own session management based on their guidelines https://lucia-next.pages.dev/. In addition I added some features:
Before (master branch)
master.mp4
After (this PR)
branch.mp4
Checklist
pnpm storybookcommand and everything is working(If you cannot update the french language, just let us know in the PR description)
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
Dependencies
@lucia-auth/adapter-prisma.@oslojs/cryptoand@oslojs/encoding.Changes
Environment
SESSION_EXPIRATION_SECONDSenvironment variable for session management.