🐛 Fix infinite loop when trying to migrate legacy cookie #1561
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning Rate limit exceeded@lukevella has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 33 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (6)
WalkthroughThe changes update the cookie migration and authentication middleware logic. Function signatures in both legacy and standard authentication modules have been modified to enforce a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthMigration
participant Middleware
Client->>AuthMigration: Send request
AuthMigration->>AuthMigration: Check if new cookie exists
alt New cookie exists
AuthMigration->>Client: Return NextResponse immediately
else No new cookie
AuthMigration->>AuthMigration: Validate old cookie
alt Old cookie invalid
AuthMigration->>Client: Delete old cookie and return NextResponse
else Old cookie valid
AuthMigration->>Middleware: Execute middleware via helper
Middleware-->>AuthMigration: Return NextResponse
AuthMigration->>Client: Set new cookie, delete old cookie, return response
end
end
sequenceDiagram
participant Client
participant AuthMiddleware
Client->>AuthMiddleware: Send request
AuthMiddleware->>AuthMiddleware: Await middleware execution
alt Valid response received
AuthMiddleware->>Client: Construct NextResponse from response details
else No response
AuthMiddleware->>Client: Return NextResponse.next()
end
Possibly related PRs
Poem
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: 0
🧹 Nitpick comments (4)
apps/web/src/auth/legacy/next-auth-cookie-migration.ts (2)
7-7: Ensure robust handling forabsoluteUrl().
IfabsoluteUrl()somehow fails or returns an unexpected result, consider providing a fallback or error-handling path. This could prevent edge cases when HTTPS is not configured or in local development.
21-30: Functional encapsulation is good; consider cookie domain/expiration.
runMiddlewareAndDeleteOldCookienicely centralizes old cookie removal. You might still want to specify a domain or expiration to ensure consistent removal in multi-domain scenarios.apps/web/src/utils/session/session-config.ts (1)
7-7: Simplify the boolean expression.
SincestartsWith("https://")always returns a boolean, the?? falsefallback is likely redundant. Also confirm thatabsoluteUrl()never returns an empty or undefined value in dev/test environments.apps/web/src/auth/middleware.ts (1)
13-24: Consider returning the original response if no modifications are needed.
RebuildingNextResponsefrom the original response is sometimes necessary for transformations, but if you don’t modify it, returning the original could reduce complexity. Also ensure large response bodies are handled efficiently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/src/auth/legacy/next-auth-cookie-migration.ts(2 hunks)apps/web/src/auth/middleware.ts(2 hunks)apps/web/src/utils/session/session-config.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Unit tests
- GitHub Check: Type check
- GitHub Check: Linting
- GitHub Check: Integration tests
🔇 Additional comments (8)
apps/web/src/auth/legacy/next-auth-cookie-migration.ts (6)
1-2: Consistent use of typed imports.
ImportingNextResponseas a type helps keep the codebase clean and consistent when dealing with response objects.
19-19: Improved middleware return type.
Requiring aPromise<NextResponse>for themiddlewarefunction aligns well with Next.js middleware expectations and prevents inconsistent returns.
34-36: Short-circuit avoids unnecessary work.
By exiting early when the new cookie already exists or the old cookie is invalid, you help prevent repeated migration attempts and avert infinite loops.
39-40: Check repeated decode attempts.
If a decode fails, the catch block will remove the old cookie. Ensure follow-up requests don’t continually retry decoding a stale or corrupted cookie if the environment somehow reintroduces it.
43-68: VerifySECRET_PASSWORDand reduce potential sensitive logging.
Ifprocess.env.SECRET_PASSWORDis missing or empty,encode()could fail. Also be cautious about logging the entire error object if it contains sensitive information.
69-73: Repeated error path check.
ReturningrunMiddlewareAndDeleteOldCookieupon error ensures the old cookie is cleared. Confirm that once cleared, a second attempt with the same stale cookie isn’t possible, preventing repeated error loops.apps/web/src/auth/middleware.ts (2)
2-2: Streamlined NextResponse import.
ImportingNextResponsedirectly from"next/server"is straightforward and clean for building new responses.
11-12: Strong alignment with Next.js middleware.
Enforcing a promise-based return type for themiddlewarefunction ensures consistent handling of asynchronous operations and response generation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/web/tests/helpers/next-auth-v4.ts (2)
20-25: Add JSDoc comments to interface properties.Consider adding JSDoc comments to document the purpose and constraints of each parameter, especially the
saltparameter which has special meaning when empty.interface JWTEncodeParams { + /** The JWT payload to encode */ token?: JWT; + /** Empty salt indicates a session token */ salt?: string; + /** The secret key material for encryption */ secret: string | Buffer; + /** Token expiration time in seconds. Defaults to 30 days */ maxAge?: number; }
27-37: Add error handling for encryption failures.The encryption operation could fail due to invalid inputs or other issues. Consider wrapping it in a try-catch block to handle errors gracefully.
export async function encode(params: JWTEncodeParams) { /** @note empty `salt` means a session token. See {@link JWTEncodeParams.salt}. */ const { token = {}, secret, maxAge = 30 * 24 * 60 * 60, salt = "" } = params; - const encryptionSecret = await getDerivedEncryptionKey(secret, salt); - return await new EncryptJWT(token) - .setProtectedHeader({ alg: "dir", enc: "A256GCM" }) - .setIssuedAt() - .setExpirationTime(now() + maxAge) - .setJti(nanoid()) - .encrypt(encryptionSecret); + try { + const encryptionSecret = await getDerivedEncryptionKey(secret, salt); + return await new EncryptJWT(token) + .setProtectedHeader({ alg: "dir", enc: "A256GCM" }) + .setIssuedAt() + .setExpirationTime(now() + maxAge) + .setJti(nanoid()) + .encrypt(encryptionSecret); + } catch (error) { + throw new Error(`Failed to encode JWT: ${error.message}`); + } }apps/web/tests/next-auth-migration.spec.ts (3)
7-27: Consider using test data factories.Instead of hard-coding test data, consider using a test data factory pattern to generate consistent test data. This would make the tests more maintainable and reduce duplication if more test cases are added.
Example factory pattern:
const createTestPoll = (overrides = {}) => ({ id: nanoid(), title: "Test Poll", adminUrlId: nanoid(), participantUrlId: nanoid(), guestId: nanoid(), ...overrides, });
39-49: Document cookie security settings.The cookie is set with
secure: falsewhich is fine for localhost testing, but it would be good to document this explicitly and possibly add a test for secure cookie settings in production environment.await context.addCookies([ { name: "next-auth.session-token", value: token, httpOnly: true, + // Note: secure is false for localhost testing only secure: false, sameSite: "Lax", path: "/", domain: "localhost", }, ]);
29-52: Enhance test coverage.The test only verifies that the poll is visible. Consider adding more assertions to verify:
- The correct user is logged in
- Other polls are not visible
- Error cases (e.g., invalid token)
Would you like me to help generate additional test cases to improve coverage?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/tests/helpers/next-auth-v4.ts(1 hunks)apps/web/tests/next-auth-migration.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Unit tests
- GitHub Check: Linting
- GitHub Check: Type check
- GitHub Check: Integration tests
🔇 Additional comments (1)
apps/web/tests/helpers/next-auth-v4.ts (1)
7-18: LGTM! Secure key derivation implementation.The implementation uses HKDF with SHA-256 and generates a 32-byte key, which follows cryptographic best practices.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/web/src/auth/legacy/next-auth-cookie-migration.ts (3)
21-30: Add error handling to the helper function.The helper function should include error handling to ensure robustness when middleware execution fails.
async function runMiddlewareAndDeleteOldCookie(req: NextRequest) { + try { const res = await middleware(req); res.cookies.set(oldCookieName, "", { httpOnly: true, secure: isSecureCookie, sameSite: "lax", path: "/", }); return res; + } catch (error) { + console.error('Error in runMiddlewareAndDeleteOldCookie:', error); + throw error; + } }
39-72: Improve error handling specificity.The current catch-all error handling could be more specific to help with debugging and maintenance.
try { const decodedCookie = await decodeLegacyJWT(oldCookie.value); // ... rest of the code ... - } catch (e) { - console.error(e); + } catch (error) { + if (error instanceof JsonWebTokenError) { + console.error('JWT validation failed:', error.message); + } else { + console.error('Unexpected error during cookie migration:', error); + } }
60-66: Extract cookie configuration to constants.Consider extracting the cookie configuration to constants for better maintainability and reusability.
+const COOKIE_CONFIG = { + httpOnly: true, + secure: isSecureCookie, + sameSite: "lax" as const, + path: "/", +}; + +const COOKIE_EXPIRY_DAYS = 7; + res.cookies.set(newCookieName, encodedCookie, { - httpOnly: true, - secure: isSecureCookie, - expires: new Date(Date.now() + 1000 * 60 * 60 * 24 * 7), - sameSite: "lax", - path: "/", + ...COOKIE_CONFIG, + expires: new Date(Date.now() + 1000 * 60 * 60 * 24 * COOKIE_EXPIRY_DAYS), });apps/web/.env.test (1)
2-3: Document the relationship between URL environment variables.Both
NEXT_PUBLIC_BASE_URLandAUTH_URLare set to the same value. Consider adding a comment to explain why both are needed and their relationship to prevent future inconsistencies.NEXT_PUBLIC_BASE_URL=http://localhost:3002 +# AUTH_URL must match NEXT_PUBLIC_BASE_URL for local development AUTH_URL=http://localhost:3002
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/.env.test(1 hunks)apps/web/src/auth/legacy/next-auth-cookie-migration.ts(2 hunks)apps/web/tests/helpers/next-auth-v4.ts(1 hunks)apps/web/tests/next-auth-migration.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/tests/next-auth-migration.spec.ts
- apps/web/tests/helpers/next-auth-v4.ts
🔇 Additional comments (3)
apps/web/src/auth/legacy/next-auth-cookie-migration.ts (3)
1-2: LGTM! Improved secure cookie determination.The changes improve security by using
absoluteUrl()to determine secure cookies instead of environment variables, making the logic more robust.Also applies to: 7-7
18-20: LGTM! Improved type safety.The updated function signature enforces a more explicit and type-safe return type, which helps prevent potential type-related bugs.
34-37: LGTM! Fixed infinite loop with proper exit conditions.The early exit conditions effectively prevent the infinite loop by checking for the new cookie's existence and old cookie's validity.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/web/src/auth/legacy/helpers/jwt.ts(1 hunks)apps/web/src/auth/legacy/helpers/types.ts(0 hunks)apps/web/src/auth/legacy/next-auth-cookie-migration.ts(1 hunks)apps/web/src/auth/middleware.ts(1 hunks)apps/web/src/middleware.ts(1 hunks)apps/web/src/next-auth.ts(3 hunks)apps/web/tests/next-auth-migration.spec.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/web/src/auth/legacy/helpers/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/tests/next-auth-migration.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Unit tests
- GitHub Check: Linting
- GitHub Check: Type check
- GitHub Check: Integration tests
🔇 Additional comments (9)
apps/web/src/auth/legacy/helpers/jwt.ts (1)
3-3: LGTM! Import aligned with next-auth package.The change to import
JWTtype directly fromnext-auth/jwtinstead of a local type definition improves maintainability by using the official type definitions.apps/web/src/auth/middleware.ts (2)
15-17: LGTM! Improved type safety with Promise.The strict return type ensures consistent response handling and prevents potential type-related issues.
36-38: Verify atomic execution of JWT migration.The JWT migration happens after the middleware response is generated. Ensure there are no race conditions if the response is modified by both the middleware and the migration.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for any middleware that modifies response headers or cookies rg -A 5 'headers\.set|cookies\.(set|delete)' --type tsLength of output: 1520
I'll take a closer look at the
migrateLegacyJWTfunction to verify that there isn’t an interleaving modifying the same response concurrently.I'll start by searching for its definition, so we can inspect its operations relative to the middleware modifications.
apps/web/src/middleware.ts (1)
10-40: LGTM! Simplified middleware chain.The direct use of
withAuthwithout the additionalwithAuthMigrationwrapper reduces complexity while maintaining the same functionality. This change helps prevent infinite loops by having a single point of authentication handling.apps/web/src/auth/legacy/next-auth-cookie-migration.ts (2)
9-9: LGTM! Improved cookie security determination.Using
absoluteUrl()instead of environment variables provides more reliable security determination based on the actual request URL.
49-68: LGTM! Robust JWT migration implementation.The migration function effectively prevents infinite loops by:
- Only migrating if a legacy JWT exists
- Creating a new JWT with proper encoding
- Setting the new cookie with secure attributes
- Deleting the old cookie to prevent repeated migrations
apps/web/src/next-auth.ts (3)
10-10: LGTM!The import of
getLegacySessionaligns with the PR objective of fixing legacy cookie migration.
26-31: LGTM!Clean destructuring pattern with appropriate renaming of
authtooriginalAuthto avoid naming conflicts.
188-188: LGTM!Clean export syntax using named exports.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/web/src/auth/middleware.ts (1)
17-44: 🛠️ Refactor suggestionAdd error handling for middleware execution.
The middleware execution could fail, but there's no try-catch block to handle errors gracefully.
Apply this diff to add error handling:
return async (request: NextAuthRequest) => { + try { const legacySession = await getLegacySession(); const session = legacySession || (await auth()); const res = await nextAuthConfig.callbacks.authorized({ request, auth: session, }); request.auth = session; if (res !== true) { return res; } const middlewareRes = await middleware(request); if (legacySession) { try { await migrateLegacyJWT(middlewareRes); } catch (e) { console.error(e); } } return middlewareRes; + } catch (error) { + console.error('Auth middleware error:', error); + return new NextResponse(null, { status: 500 }); + } };apps/web/src/auth/legacy/next-auth-cookie-migration.ts (1)
16-32: 🛠️ Refactor suggestionAdd null check for expiration date.
The session expiration date could be undefined if the JWT doesn't contain an
expclaim.Apply this diff to add a fallback expiration date:
return { user: { id, ...rest }, - expires: new Date(decodedCookie.exp ?? 0).toISOString(), + expires: decodedCookie.exp + ? new Date(decodedCookie.exp * 1000).toISOString() + : new Date(Date.now() + 24 * 60 * 60 * 1000).toISOString(), // 24 hours from now };
🧹 Nitpick comments (1)
apps/web/src/auth/legacy/next-auth-cookie-migration.ts (1)
49-68: Consider making cookie attributes configurable.The cookie expiration time is hardcoded to 7 days and the path is set to "/". Consider making these attributes configurable to allow for different security requirements.
Apply this diff to make the attributes configurable:
+interface CookieOptions { + expiresIn?: number; // in seconds + path?: string; +} + /** * Replace the old legacy cookie with the new one */ -export async function migrateLegacyJWT(res: NextResponse) { +export async function migrateLegacyJWT( + res: NextResponse, + options: CookieOptions = {}, +) { const legacyJWT = await getLegacyJWT(); if (legacyJWT) { const newJWT = await encode({ token: legacyJWT, secret: process.env.SECRET_PASSWORD, salt: newCookieName, }); res.cookies.set(newCookieName, newJWT, { httpOnly: true, secure: isSecureCookie, - expires: new Date(Date.now() + 1000 * 60 * 60 * 24 * 7), + expires: new Date( + Date.now() + (options.expiresIn ?? 7 * 24 * 60 * 60) * 1000, + ), sameSite: "lax", - path: "/", + path: options.path ?? "/", }); res.cookies.delete(oldCookieName); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/src/auth/legacy/next-auth-cookie-migration.ts(1 hunks)apps/web/src/auth/middleware.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Unit tests
- GitHub Check: Integration tests
- GitHub Check: Linting
- GitHub Check: Type check
🔇 Additional comments (1)
apps/web/src/auth/legacy/next-auth-cookie-migration.ts (1)
34-44: LGTM!The function has proper error handling and returns null for missing or invalid cookies.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/src/auth/legacy/next-auth-cookie-migration.ts (2)
36-46: Consider optimizing the JWT retrieval.The function could be optimized by reusing the decoded JWT to avoid multiple decoding operations.
-async function getLegacyJWT() { +export async function getLegacyJWT() { const cookieStore = cookies(); const legacySessionCookie = cookieStore.get(oldCookieName); if (legacySessionCookie) { const decodedCookie = await decodeLegacyJWT(legacySessionCookie.value); - if (decodedCookie) { - return decodedCookie; - } + return decodedCookie ?? null; } return null; }
51-70: Add error handling for JWT encoding.The JWT encoding operation could fail, and it would be beneficial to handle such errors gracefully.
export async function migrateLegacyJWT(res: NextResponse) { const legacyJWT = await getLegacyJWT(); if (legacyJWT) { - const newJWT = await encode({ - token: legacyJWT, - secret: process.env.SECRET_PASSWORD, - salt: newCookieName, - }); + try { + const newJWT = await encode({ + token: legacyJWT, + secret: process.env.SECRET_PASSWORD, + salt: newCookieName, + }); + res.cookies.set(newCookieName, newJWT, { + httpOnly: true, + secure: isSecureCookie, + expires: new Date(Date.now() + 1000 * 60 * 60 * 24 * 7), + sameSite: "lax", + path: "/", + }); + res.cookies.delete(oldCookieName); + } catch (error) { + console.error('Failed to migrate legacy JWT:', error); + // Keep the legacy cookie intact if migration fails + } + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/auth/legacy/next-auth-cookie-migration.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Unit tests
- GitHub Check: Type check
- GitHub Check: Linting
- GitHub Check: Integration tests
🔇 Additional comments (2)
apps/web/src/auth/legacy/next-auth-cookie-migration.ts (2)
1-14: LGTM! Improved cookie security configuration.The code correctly determines cookie security based on the URL scheme using
absoluteUrl()and properly sets the cookie prefix.
16-34: LGTM! Robust session handling with proper expiration.The function correctly handles legacy session retrieval and expiration date calculation, with a fallback to 30 hours from now if no expiration is present in the JWT.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes