Peter/split cookie #229
Peter/split cookie #229
Conversation
WalkthroughThe pull request introduces significant changes across multiple files, primarily focusing on the refactoring of the Changes
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
src/session/sessionManager.jsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-jest". (The package "eslint-plugin-jest" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-jest" was referenced from the config file in ".eslintrc.json". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 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: 9
🧹 Outside diff range and nitpick comments (9)
src/utils/generateOrganizationObject.ts (1)
10-29: Consider size optimization for session storage.
Given that this PR aims to address cookie size limitations, consider implementing property filtering to reduce the size of the organization object when stored in session:
- Allow specifying which properties to include
- Omit undefined properties
Example implementation:
export const generateOrganizationObject = (
idToken: KindeIdToken,
accessToken: KindeAccessToken,
// Optional parameter to specify which properties to include
includeProperties?: OrgPropertyKey[]
) => {
// ... existing validation ...
const properties = Object.fromEntries(
propertyKeys
.filter(({ key }) =>
!includeProperties || includeProperties.includes(key))
.map(({ key }) => [
key,
getOrgProperty(key, idToken, accessToken),
])
.filter(([_, value]) => value !== undefined)
);
return {
orgCode: accessToken.org_code,
orgName: accessToken.org_name,
...(Object.keys(properties).length > 0 && { properties }),
};
};src/session/getOrganization.ts (1)
6-16: Enhance JSDoc documentation for better clarity.
While the basic documentation is present, consider adding:
- Description of the function's purpose
- Error scenarios and return values
- Parameter descriptions
/**
+ * Factory function that creates an organization data retriever.
+ * Handles session token retrieval and organization object generation.
* @callback getOrganization
+ * @throws {Error} When ID token or access token is missing
* @returns {Promise<import('../../types').KindeOrganization | null>}
*/
/**
- *
+ * Creates a function to retrieve organization data from session tokens.
* @param {import('next').NextApiRequest} [req] - Next.js request object
* @param {import('next').NextApiResponse} [res] - Next.js response object
* @returns {getOrganization}
*/src/handlers/setup.ts (1)
Line range hint 71-89: Enhance error handling for cookie-related failures.
Since we're dealing with cookie size limitations, we should add specific error handling for cookie storage failures.
Add cookie-specific error handling:
} catch (error) {
if (config.isDebugMode) {
console.debug(error);
}
+ // Handle cookie storage errors
+ if (error.message?.includes('cookie') || error.message?.includes('storage')) {
+ return routerClient.json(
+ {
+ error: 'Session storage failed - data size might exceed limits',
+ needsRefresh: true
+ },
+ {status: 413}
+ );
+ }
+
if (error.code == 'ERR_JWT_EXPIRED') {tests/frontend/generate-organization-object.test.ts (1)
1-2: Remove unnecessary comment about function location.
The comment "Assuming the function is exported from utils file" suggests uncertainty about the import path. If the import works, the comment is unnecessary; if there's uncertainty, it should be verified.
-import {generateOrganizationObject} from '../../src/utils/generateOrganizationObject'; // Assuming the function is exported from utils file
+import {generateOrganizationObject} from '../../src/utils/generateOrganizationObject';tests/frontend/generate-user-object.test.ts (1)
1-2: Remove unnecessary comment in import statement.
The comment "Assuming the function is exported from utils file" is redundant as the import path clearly indicates the source.
-import {generateUserObject} from '../../src/utils/generateUserObject'; // Assuming the function is exported from utils file
+import {generateUserObject} from '../../src/utils/generateUserObject';src/session/sessionManager.js (4)
Line range hint 201-223: Implement splitting logic in pageRouterSessionManager's setSessionItem
The setSessionItem function in pageRouterSessionManager does not include the logic to split large itemValue strings into multiple cookies. This inconsistency can lead to issues when storing large session items in the Page Router context.
Consider implementing the same splitting logic used in appRouterSessionManager to handle large session items consistently across both router types. This ensures that large session data is properly stored and retrieved.
Line range hint 185-197: Update getSessionItem in pageRouterSessionManager to handle split cookies
The getSessionItem function in pageRouterSessionManager retrieves the session item directly from req.cookies[itemKey] without accounting for items that may be split across multiple cookies (e.g., itemKey0, itemKey1, etc.).
To accurately reconstruct session items that have been split, update the function to concatenate values from all relevant cookies:
getSessionItem: (itemKey) => {
- const itemValue = req.cookies[itemKey];
+ let itemValue = '';
+ let index = 0;
+ let key = `${String(itemKey)}${index}`;
+ while (req.cookies[key]) {
+ itemValue += req.cookies[key];
+ index++;
+ key = `${String(itemKey)}${index}`;
+ }
if (itemValue) {
try {
const jsonValue = JSON.parse(itemValue);Line range hint 225-237: Modify removeSessionItem to delete all cookie segments
In the removeSessionItem function of pageRouterSessionManager, only the base cookie with itemKey is deleted. If the session item was stored in multiple cookies due to splitting, the additional segments remain.
Adjust the function to remove all cookies that start with itemKey:
removeSessionItem: (itemKey) => {
+ const cookies = req.cookies;
+ Object.keys(cookies).forEach((key) => {
+ if (key.startsWith(`${String(itemKey)}`)) {
+ res?.setHeader('Set-Cookie', [
+ cookie.serialize(key, '', {
+ domain: config.cookieDomain ? config.cookieDomain : undefined,
+ maxAge: -1,
+ ...GLOBAL_COOKIE_OPTIONS
+ })
+ ]);
+ }
+ });
},27-38: Ensure splitString handles non-string inputs gracefully
The splitString function assumes that str is a string. If str is null, undefined, or not a string, it could cause runtime errors.
Consider adding a type check or converting str to a string to enhance robustness:
const splitString = (str, length) => {
+ if (typeof str !== 'string') {
+ str = String(str || '');
+ }
if (length <= 0) {
return [];
}📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- src/handlers/setup.ts (1 hunks)
- src/session/getOrganization.js (0 hunks)
- src/session/getOrganization.ts (1 hunks)
- src/session/isAuthenticated.js (1 hunks)
- src/session/sessionManager.js (6 hunks)
- src/utils/generateOrganizationObject.ts (1 hunks)
- tests/frontend/generate-organization-object.test.ts (1 hunks)
- tests/frontend/generate-user-object.test.ts (1 hunks)
- tests/frontend/utils.test.ts (0 hunks)
💤 Files with no reviewable changes (2)
- src/session/getOrganization.js
- tests/frontend/utils.test.ts
🔇 Additional comments (9)
src/session/isAuthenticated.js (1)
3-3: LGTM! Clean integration with sessionManager.
The changes effectively integrate the new sessionManager for token retrieval while maintaining the existing authentication logic. This aligns well with the PR's objective of handling larger cookie sizes.
Let's verify the sessionManager integration:
Also applies to: 12-14
✅ Verification successful
Let me gather more information about the sessionManager implementation since the previous results don't show its complete implementation.
Let me check the implementation details of the sessionManager to verify it properly handles cookie splitting.
Verified: sessionManager implementation properly handles cookie splitting
The implementation in sessionManager.js correctly implements cookie splitting functionality:
- Uses
splitStringfunction to split large values into chunks of 3000 bytes - Properly handles both storage and retrieval of split cookies
- Maintains consistent cookie management across App Router and Pages Router
- Correctly implements cleanup of split cookies during removal
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of sessionManager across the codebase
# and ensure it's properly implemented for token retrieval
# Check if sessionManager is consistently used for access_token retrieval
rg -A 3 "getSessionItem.*access_token"
# Verify the sessionManager implementation exists and handles cookie splitting
ast-grep --pattern 'export const sessionManager = ($req, $res) => {
$$$
getSessionItem($$$) {
$$$
}
$$$
}'
Length of output: 807
Script:
#!/bin/bash
# Search for sessionManager implementation
rg -l "sessionManager"
# Look for the actual implementation with more context
rg -A 10 "export.*sessionManager"
# Check if there are any other files importing sessionManager
rg "import.*sessionManager"
Length of output: 3188
Script:
#!/bin/bash
# Get the complete implementation of sessionManager
cat src/session/sessionManager.js
# Check for cookie splitting related code
rg -A 5 "splitCookie|joinCookie|cookie.*split"
Length of output: 6438
src/utils/generateOrganizationObject.ts (1)
7-9: LGTM!
The basic organization fields are correctly mapped from the access token.
src/session/getOrganization.ts (3)
1-5: LGTM! Imports are well-organized and complete.
The imports include all necessary dependencies and follow a logical organization pattern.
1-44: Verify alignment with cookie size management.
The implementation aligns with the PR objective of handling larger cookie sizes by separating token retrieval and using a factory pattern. However, we should verify that the sessionManager properly implements cookie splitting for larger tokens.
#!/bin/bash
# Check sessionManager implementation for cookie handling
rg -A 10 "sessionManager.*getSessionItem" --type typescript17-44: 🛠️ Refactor suggestion
Consider enhancing error handling and performance.
The implementation is generally solid, but there are a few areas for improvement:
- Consider caching session items to reduce async calls:
export const getOrganizationFactory = (req, res) => async () => {
+ const session = sessionManager(req, res);
try {
- const idTokenString = await sessionManager(req, res).getSessionItem(
+ const idTokenString = await session.getSessionItem(
'id_token'
);
// ... rest of the code
- const accessTokenString = await sessionManager(req, res).getSessionItem(
+ const accessTokenString = await session.getSessionItem(- Consider more specific error handling:
try {
// ... existing code
} catch (error) {
+ const errorMessage = error instanceof Error ? error.message : 'Unknown error';
if (config.isDebugMode) {
- console.error(error);
+ console.error(`Organization retrieval failed: ${errorMessage}`);
}
return null;
}Let's verify the token validation approach:
src/handlers/setup.ts (1)
Line range hint 32-89: Consider response size optimization for cookie storage.
Given that this PR aims to address cookie size limitations, we should optimize the response structure to minimize the data stored in cookies. Consider:
- Removing redundant data (e.g., both raw and decoded tokens)
- Implementing selective claim storage
- Flattening nested structures
Let's verify the potential cookie size impact:
tests/frontend/generate-organization-object.test.ts (2)
1-198: Verify the connection to cookie splitting functionality.
While these tests are well-structured, it's not immediately clear how they relate to the PR's objective of implementing cookie splitting for larger cookie sizes. Could you clarify if this organization object generation is part of the cookie size optimization strategy?
#!/bin/bash
# Look for related cookie handling code
rg -l "cookie|setCookie|getCookie" --type ts4-136: Consider sanitizing sensitive information in test data.
The mock data contains what appears to be real email addresses, URLs, and potentially real organization codes. Consider using clearly fictional data (e.g., test@example.com) for test cases.
tests/frontend/generate-user-object.test.ts (1)
1-363: Verify the connection between these tests and cookie splitting functionality.
While these tests thoroughly cover the generateUserObject function, it's unclear how they relate to the PR's objective of implementing cookie splitting for larger sizes. We should verify if this is part of a larger change set.
Let's check for related changes:
# Conflicts: # src/session/getOrganization.js
…lt to no number prefix
…nextjs into peter/split-cookie
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (4)
src/session/getOrganization.ts (1)
6-16: Enhance JSDoc documentation for better clarity.
Consider adding more detailed documentation:
- Document error cases and when null is returned
- Add descriptions for req/res parameters
- Include example usage
/**
+ * Factory function that creates an organization data retriever
+ * @param {import('next').NextApiRequest} [req] - The Next.js request object
+ * @param {import('next').NextApiResponse} [res] - The Next.js response object
+ * @returns {getOrganization} A function that retrieves organization data
+ * @throws {Error} When tokens are missing
+ * @example
+ * const getOrganization = getOrganizationFactory(req, res);
+ * const org = await getOrganization();
*/src/routerClients/PagesRouterClient.js (1)
Line range hint 1-85: Consider documenting the session management strategy.
Given that this change is part of a larger session management improvement, it would be valuable to:
- Add JSDoc comments explaining the session management strategy and cookie handling approach
- Document any breaking changes or migration steps if moving from
sessionManagertopageRouterSessionManager
src/session/sessionManager.js (2)
47-58: Simplify control flow in sessionManager function
The nested if conditions in the sessionManager function can be simplified for better readability.
Consider refactoring as follows:
export const sessionManager = async (req, res) => {
try {
if (!req || isAppRouter(req)) {
const cookieStore = await cookies(req, res);
return appRouterSessionManager(cookieStore);
} else {
return pageRouterSessionManager(req, res);
}
} catch (error) {
console.error('Failed to initialize session manager:', error);
throw error;
}
};80-98: Handle JSON parsing errors more effectively in getSessionItem
In the getSessionItem function, the error handling for JSON parsing can be improved to provide more insightful debugging information.
Consider logging the problematic itemValue when parsing fails:
try {
const jsonValue = JSON.parse(itemValue);
if (typeof jsonValue === 'object') {
return jsonValue;
}
} catch (err) {
+ if (config.isDebugMode)
+ console.error('Failed to parse session item JSON:', err, 'Item Value:', itemValue);
}
return itemValue;📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- src/handlers/setup.ts (1 hunks)
- src/routerClients/PagesRouterClient.js (2 hunks)
- src/session/getOrganization.ts (1 hunks)
- src/session/getUser.ts (1 hunks)
- src/session/sessionManager.js (7 hunks)
- src/utils/generateOrganizationObject.ts (1 hunks)
🔇 Additional comments (13)
src/session/getUser.ts (2)
14-15: LGTM! Improved code clarity with explicit token variable.
The introduction of the rawToken variable improves code readability by making the token handling flow more explicit.
14-15: Verify cookie size handling implementation.
Given that this PR's objective is to handle cookie splitting for larger sizes, we should verify if this token retrieval implementation properly handles split cookies.
✅ Verification successful
Let me check the cookie handling implementation more specifically by looking at the session manager implementation.
Based on the session manager implementation, I can now provide a final response regarding the cookie splitting concern.
Cookie splitting is properly implemented for handling large tokens
The session manager has built-in support for handling large cookies through cookie splitting:
- A
MAX_LENGTHconstant of 3000 bytes is defined to stay within cookie size limits - The
splitStringutility function splits large values into chunks - Both
appRouterSessionManagerandpageRouterSessionManagerimplementations:- Split large values during storage using
splitString(itemValueString, MAX_LENGTH) - Concatenate split values during retrieval by iterating through indexed cookies
- Handle cleanup of all split cookies during removal
- Split large values during storage using
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for cookie splitting implementation in session management
# Expected: Find evidence of cookie size handling in session manager
# Check sessionManager implementation for cookie handling
echo "Checking session manager implementation..."
rg -A 10 "sessionManager" src/
# Look for cookie-related configurations or utilities
echo "Checking for cookie-related configurations..."
rg -l "cookie" src/
Length of output: 27938
Script:
#!/bin/bash
# Check session manager implementation for cookie handling
echo "Checking session manager implementation..."
cat src/session/sessionManager.js
# Check cookie-related configurations
echo "Checking cookie configurations..."
cat src/config/index.js
Length of output: 11785
src/utils/generateOrganizationObject.ts (2)
1-9: LGTM! Well-structured type definitions.
The type definitions provide good type safety and the property keys are comprehensive for organization data.
21-37: Consider cookie size implications and add validation.
-
The previous review comment about validating required fields (org_code, org_name) is still applicable.
-
Given that this PR aims to address cookie size limitations:
- Consider implementing property filtering to include only non-null values
- Add size monitoring for the generated object
Here's a suggested implementation that addresses both concerns:
export const generateOrganizationObject = (
idToken: KindeIdToken,
accessToken: KindeAccessToken
) => {
+ if (!accessToken.org_code || !accessToken.org_name) {
+ throw new Error('Missing required organization fields in access token');
+ }
+
+ const properties = {
+ city: getOrgProperty('city', idToken, accessToken),
+ industry: getOrgProperty('industry', idToken, accessToken),
+ postcode: getOrgProperty('postcode', idToken, accessToken),
+ state_region: getOrgProperty('state_region', idToken, accessToken),
+ street_address: getOrgProperty('street_address', idToken, accessToken),
+ street_address_2: getOrgProperty('street_address_2', idToken, accessToken)
+ };
+
+ // Filter out undefined properties to reduce object size
+ const filteredProperties = Object.fromEntries(
+ Object.entries(properties).filter(([_, value]) => value !== undefined)
+ );
+
return {
orgCode: accessToken.org_code,
orgName: accessToken.org_name,
- properties: {
- city: getOrgProperty('city', idToken, accessToken),
- industry: getOrgProperty('industry', idToken, accessToken),
- postcode: getOrgProperty('postcode', idToken, accessToken),
- state_region: getOrgProperty('state_region', idToken, accessToken),
- street_address: getOrgProperty('street_address', idToken, accessToken),
- street_address_2: getOrgProperty('street_address_2', idToken, accessToken)
- }
+ properties: filteredProperties
};
};src/session/getOrganization.ts (2)
1-5: LGTM! Clean and well-organized imports.
The imports are properly structured with clear separation between runtime dependencies and type imports.
17-44: Verify session manager's cookie handling capacity.
Given the PR's objective to handle larger cookie sizes, we should verify the session manager's implementation.
src/routerClients/PagesRouterClient.js (2)
46-46: LGTM! Verify integration with cookie splitting functionality.
The switch to pageRouterSessionManager aligns with the PR's objective to improve cookie handling. The implementation maintains the same interface while potentially enabling better cookie size management.
Let's verify the implementation of the new session manager:
#!/bin/bash
# Description: Check pageRouterSessionManager implementation for cookie splitting logic
rg -A 10 "pageRouterSessionManager\s*=\s*" "src/session/sessionManager"3-6: Verify if both session manager imports are necessary.
Both pageRouterSessionManager and sessionManager are imported, but only pageRouterSessionManager is used. Consider removing the unused sessionManager import to keep the code clean.
Let's check if sessionManager is used elsewhere in the file:
src/handlers/setup.ts (2)
23-34: Verify alignment with PR objectives.
The PR's objective is to implement cookie splitting for larger cookie sizes, but the changes focus on direct token claim access. While this might be related, it's not immediately clear how these changes address the cookie size issue.
Could you please clarify:
- How do these token claim access changes relate to cookie splitting?
- Are there additional changes needed to implement the cookie splitting functionality?
Let's check for cookie-related code:
#!/bin/bash
# Search for cookie-related implementations
rg -A 5 "cookie|split.*cookie|sessionManager.*set"23-30: Consider adding token validation before accessing claims.
Direct access to token claims without validation could expose the application to security risks if the tokens are malformed or tampered with. Consider adding token validation using the JWT signature and expiry checks before accessing the claims.
Let's verify if token validation is implemented elsewhere:
Consider implementing a token validation middleware that:
- Verifies the JWT signature
- Checks token expiration
- Validates required claim presence and types
src/session/sessionManager.js (3)
108-120: Delete all existing cookies with prefix itemKey before setting new values
In the setSessionItem function, existing cookies with keys starting with itemKey are not deleted before setting new values. This can leave stale cookie segments if the new value has fewer segments than the old value.
27-38: Optimize the splitString function using regular expressions
149-160: Verify cookie deletion logic in destroySession
Ensure that all cookies matching the prefixes in COOKIE_LIST are correctly identified and deleted.
Run the following script to confirm that cookies are properly deleted:
✅ Verification successful
Cookie deletion logic in destroySession is correctly implemented
The implementation correctly:
- Uses
cookieStore.getAll()to fetch all cookies - Checks each cookie name against the
COOKIE_LISTprefixes usingstartsWith - Deletes matching cookies by setting empty value and
maxAge: 0 - Applies consistent cookie options including domain and global settings
- Handles both App Router and Page Router scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any cookies that should be deleted but remain.
# Test: List all cookies matching COOKIE_LIST prefixes.
# Expect: No cookies should be listed after destroySession is called.
COOKIE_PREFIXES=("ac-state-key" "id_token_payload" "id_token" "access_token_payload" "access_token" "user" "refresh_token" "post_login_redirect_url")
for prefix in "${COOKIE_PREFIXES[@]}"; do
grep -r "^Set-Cookie: $prefix" .
done
Length of output: 803
Script:
#!/bin/bash
# Let's verify the cookie deletion logic by checking:
# 1. The COOKIE_LIST constant definition
# 2. The cookie deletion implementation
# 3. Any tests related to cookie deletion
# Check for COOKIE_LIST definition
rg "COOKIE_LIST.*=.*\[" -A 5
# Check for cookie deletion implementation
ast-grep --pattern 'cookieStore.set($name, $value, $options)'
# Look for test files related to session/cookie management
fd "test|spec" --type f | rg -i "session|cookie"
Length of output: 445
Script:
#!/bin/bash
# Let's check for more cookie-related code and tests with broader patterns
# Check for cookie-related code in sessionManager.js
rg "cookie" -i src/session/sessionManager.js -B 2 -A 2
# Look for cookie-related test files with a more permissive pattern
fd -e test.js -e spec.js | xargs rg "cookie" -i -l
# Check for any cookie store usage
ast-grep --pattern 'cookieStore.$_($$$)'
Length of output: 8052
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/utils/generateOrganizationObject.ts (1)
21-41: Implementation aligns with PR objectives for cookie size optimization.
The function efficiently retrieves organization properties by:
- Only including necessary fields in the returned object
- Using a consistent property access pattern
- Properly handling missing or undefined values
This implementation supports the PR's goal of managing cookie sizes by ensuring efficient data structure.
Consider these additional optimizations if cookie size remains a concern:
- Implement property filtering to exclude null/undefined values
- Add compression for longer string values
- Consider implementing property versioning for backward compatibility
src/session/sessionManager.js (1)
125-139: Remove redundant deletion in removeSessionItem
In the removeSessionItem function, deleting itemKey before iterating over all cookies starting with itemKey is unnecessary since the loop will handle it. Removing the redundant deletion simplifies the code.
Apply this diff to streamline the function:
-if (cookieStore.has(itemKey)) {
- cookieStore.delete(itemKey);
-}
cookieStore
.getAll()
.map((c) => c.name)
.forEach((key) => {
if (key.startsWith(`${String(itemKey)}`)) {
cookieStore.set(key, '', {
domain: config.cookieDomain ? config.cookieDomain : undefined,
maxAge: 0,
...GLOBAL_COOKIE_OPTIONS
});
}
});📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/session/sessionManager.js (7 hunks)
- src/utils/generateOrganizationObject.ts (1 hunks)
🔇 Additional comments (7)
src/utils/generateOrganizationObject.ts (2)
1-9: LGTM! Well-structured type definitions.
The type definitions are clear and provide good type safety for the organization properties.
11-19: Consider handling edge case when both values are falsy.
The current implementation returns the first truthy value or undefined. However, if both idValue and accessValue are falsy but exist (e.g., empty string), the function might not behave as expected.
Let's verify if there are any cases where empty strings are valid values:
src/session/sessionManager.js (5)
29-34: Efficient implementation of splitString function
The splitString function efficiently splits a string into chunks using a regular expression, which improves performance over iterative methods.
231-245:
Correct usage of res.setHeader in setSessionItem
Passing a third argument (options object) to res.setHeader is incorrect. The method accepts only the header name and value. You should apply the options within the cookie serialization.
Apply this diff to fix the header setting:
res?.setHeader(
'Set-Cookie',
[
...(cookies.filter((cookie) => !cookie.startsWith(`${itemKey}`)) || []),
...splitString(itemValueString, MAX_LENGTH).map((value, index) => {
return cookie.serialize(
itemKey + (index === 0 ? '' : index),
value,
{
domain: config.cookieDomain ? config.cookieDomain : undefined,
...GLOBAL_COOKIE_OPTIONS,
maxAge: TWENTY_NINE_DAYS
}
);
})
- ],
- {
- domain: config.cookieDomain ? config.cookieDomain : undefined,
- ...GLOBAL_COOKIE_OPTIONS,
- maxAge: TWENTY_NINE_DAYS
- }
+ ]
);Likely invalid or redundant comment.
291-304: Verify the comprehensive deletion of session cookies in destroySession
Ensure that all cookies listed in COOKIE_LIST are being properly deleted during session destruction.
Run the following script to confirm that all cookies are removed:
✅ Verification successful
Let me check the actual implementation of cookie deletion by examining the COOKIE_LIST constant and the destroySession function.
Let me check the complete COOKIE_LIST definition to ensure all session-related cookies are included.
Session cookies are properly handled during destruction
The implementation correctly handles the deletion of all session-related cookies:
- The
COOKIE_LISTarray includes all essential session cookies:ac-state-key,id_token_payload,id_token,access_token_payload,access_token,user,refresh_token, andpost_login_redirect_url - The
destroySessionfunction properly iterates through all cookies and removes those that match any prefix inCOOKIE_LIST - Cookie deletion is implemented with appropriate options:
- Sets empty value
- Uses correct domain from config
- Sets negative/zero maxAge to ensure immediate expiration
- Includes global cookie options
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all cookies in COOKIE_LIST are deleted.
# Expected result: No cookies matching COOKIE_LIST remain.
for cookie_name in 'ac-state-key' 'id_token_payload' 'id_token' 'access_token_payload' 'access_token' 'user' 'refresh_token' 'post_login_redirect_url'; do
grep -rl "$cookie_name" src/
done
Length of output: 2804
Script:
#!/bin/bash
# Check the COOKIE_LIST definition and destroySession implementation
rg "COOKIE_LIST\s*=" src/ -A 2
rg "destroySession" src/session/sessionManager.js -A 20
Length of output: 1403
Script:
#!/bin/bash
# Get the complete COOKIE_LIST definition
rg "COOKIE_LIST\s*=" src/session/sessionManager.js -A 10
Length of output: 239
291-304:
Filter out undefined values in destroySession
In the destroySession function, when mapping over Object.keys(req.cookies), some entries may return undefined if they don't match the COOKIE_LIST criteria. To prevent potential errors, filter out any undefined values before setting the header.
Apply this diff to ensure only valid cookies are processed:
res?.setHeader('Set-Cookie', [
...Object.keys(req.cookies).map((c) => {
if (COOKIE_LIST.some((substr) => c.startsWith(substr))) {
return cookie.serialize(c.split('=')[0], '', {
domain: config.cookieDomain ? config.cookieDomain : undefined,
maxAge: -1,
...GLOBAL_COOKIE_OPTIONS
});
}
+ return null;
}).filter(Boolean)
]);Likely invalid or redundant comment.
276-288:
Ensure all cookies starting with itemKey are removed in removeSessionItem
When modifying the Set-Cookie header, ensure that you're removing all cookies that start with itemKey. Currently, you're iterating over cookies, which may not include all existing cookies. To comprehensively delete all matching cookies, include cookies from req.cookies.
Apply this diff to enhance cookie removal:
res?.setHeader('Set-Cookie', [
+ ...Object.keys(req.cookies).map((c) => {
+ if (c.startsWith(`${itemKey}`)) {
+ return cookie.serialize(c, '', {
+ domain: config.cookieDomain ? config.cookieDomain : undefined,
+ maxAge: -1,
+ ...GLOBAL_COOKIE_OPTIONS
+ });
+ }
+ }).filter(Boolean),
...cookies.map((c) => {
if (c.startsWith(`${itemKey}`)) {
return cookie.serialize(c.split('=')[0], '', {
domain: config.cookieDomain ? config.cookieDomain : undefined,
maxAge: -1,
...GLOBAL_COOKIE_OPTIONS
});
} else {
return c;
}
})
]);Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/session/sessionManager.js (2)
29-34: Add input validation tosplitStringfunctionThe function should validate that the input string is not null/undefined before processing.
Consider this enhanced implementation:
const splitString = (str, length) => { + if (!str) { + return []; + } if (length <= 0) { return []; } return str.match(new RegExp(`.{1,${length}}`, 'g')) || []; };
84-93: Enhance error handling for JSON parsingThe current implementation silently ignores JSON parsing errors. Consider logging the actual parsing error in debug mode.
try { const jsonValue = JSON.parse(itemValue); if (typeof jsonValue === 'object') { return jsonValue; } - } catch (err) {} + } catch (err) { + if (config.isDebugMode) { + console.error('JSON parsing failed:', err); + } + } return itemValue; } catch (error) { if (config.isDebugMode)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/session/sessionManager.js(8 hunks)
🧰 Additional context used
📓 Learnings (1)
src/session/sessionManager.js (1)
Learnt from: DanielRivers
PR: kinde-oss/kinde-auth-nextjs#229
File: src/session/sessionManager.js:74-91
Timestamp: 2024-11-12T02:26:20.896Z
Learning: In `src/session/sessionManager.js`, within the `getSessionItem` method, not including the `0` in the index for the first item key is intentional.
🔇 Additional comments (3)
src/session/sessionManager.js (3)
44-54: LGTM! Clean router-specific session manager initialization
The conditional logic properly handles both Next.js App Router and Pages Router scenarios.
174-197: LGTM! Consistent implementation with app router version
The getSessionItem implementation correctly mirrors the app router version, including the intentional behavior of not including '0' in the first item key.
214-251: Remove redundant options object in setHeader call
The setHeader call includes a redundant options object that should be removed as noted in past reviews.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
src/session/sessionManager.js (3)
Line range hint
17-24: Consider moving sensitive cookie names to a secure configurationThe
COOKIE_LISTcontains sensitive token names. Consider moving these to a secure configuration file to centralize security-sensitive constants.
44-54: Enhance error handling with specific error typesThe current error handling simply re-throws the error. Consider:
- Creating specific error types for different failure scenarios
- Adding context to the error
} catch (error) { - throw error; + throw new SessionManagerError( + `Failed to initialize session manager: ${error.message}`, + { cause: error } + ); }
122-129: Optimize cookie cleanup with filter and batch operationsThe current implementation iterates through all cookies individually. Consider using filter and batch operations:
- cookieStore - .getAll() - .map((c) => c.name) - .forEach((key) => { - if (key.startsWith(`${String(itemKey)}`)) { - cookieStore.delete(key); - } - }); + const keysToDelete = cookieStore + .getAll() + .map((c) => c.name) + .filter(key => key.startsWith(`${String(itemKey)}`)); + + keysToDelete.forEach(key => cookieStore.delete(key));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/session/sessionManager.js(8 hunks)
🧰 Additional context used
📓 Learnings (1)
src/session/sessionManager.js (1)
Learnt from: DanielRivers
PR: kinde-oss/kinde-auth-nextjs#229
File: src/session/sessionManager.js:74-91
Timestamp: 2024-11-12T02:26:20.896Z
Learning: In `src/session/sessionManager.js`, within the `getSessionItem` method, not including the `0` in the index for the first item key is intentional.
🔇 Additional comments (1)
src/session/sessionManager.js (1)
27-34: Validate MAX_LENGTH against browser and server limits
The MAX_LENGTH constant of 3000 needs validation:
- Verify it's below browser cookie size limits (typically 4096 bytes)
- Consider server-side limitations
- Account for cookie header overhead
Additionally, the splitString function should validate input types:
Explain your changes
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.