Fix/refresh token #247
Fix/refresh token #247
Conversation
DanielRivers
requested review from
DaveOrDead and
peterphanouvong
as code owners
WalkthroughThis pull request introduces enhancements to token management within the application. Key changes include the addition of a validation step for access tokens in the Changes
Assessment against linked issues
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/utils/validateToken.tsOops! 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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
DanielRivers
force-pushed
the
fix/refresh-token
branch
from
7f82398 to
d4ddd10
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (21)
src/server/index.js (1)
Line range hint
1-9: Documentation update may be neededSince this change removes a previously exported middleware, please ensure that:
- The documentation is updated to reflect this change
- A migration guide is provided for users who need to switch from
authMiddlewaretowithAuth- The changelog includes this breaking change
Would you like me to help draft the documentation updates or migration guide?
clientConfig/rollup.config.js (1)
Line range hint
3-5: Consider configuring TypeScript plugin optionsThe TypeScript plugin is included but lacks specific configuration. Consider adding TypeScript compiler options to the plugin configuration for better control over the compilation process.
import babel from '@rollup/plugin-babel'; import {terser} from 'rollup-plugin-terser'; import tsPlugin from '@rollup/plugin-typescript'; export default { - plugins: [babel({babelHelpers: 'bundled'}), terser(), tsPlugin()], + plugins: [ + babel({babelHelpers: 'bundled'}), + terser(), + tsPlugin({ + tsconfig: './tsconfig.json', + declaration: true, + declarationDir: './dist/types', + rootDir: './src' + }) + ],src/session/getAccessToken.js (2)
19-20: Consider separating error handlingThe current implementation catches errors from both
getAccessTokenandjwtDecoderin the same try-catch block. Consider handling them separately to provide more specific error handling and logging.export const getAccessTokenFactory = (req, res) => async () => { try { const accessToken = await getAccessToken(req, res); - return jwtDecoder(accessToken) + try { + return jwtDecoder(accessToken); + } catch (jwtError) { + if (config.isDebugMode) { + console.error('JWT decoding failed:', jwtError); + } + return null; + } } catch (err) { if (config.isDebugMode) { - console.error(err); + console.error('Access token retrieval failed:', err); } return null; } };
Based on the evidence gathered, I can now provide the final response:
Token refresh mechanism is missing and needs to be implemented
The current implementation in
getAccessToken.tshas a placeholder comment "look for refresh token" but lacks actual refresh token handling:
- No refresh token implementation exists despite token validation
- The function simply returns null when tokens are invalid
- No integration with the OAuth token endpoint for refresh flows
This could lead to unnecessary logouts when tokens expire. Consider:
- Implementing token refresh logic when validation fails
- Using the OAuth token endpoint at
${config.issuerURL}/oauth2/token- Storing and managing refresh tokens in the session
🔗 Analysis chain
Line range hint
1-24: Verify integration with refresh token mechanismSince this PR focuses on fixing the refresh token mechanism, we should verify how this implementation integrates with the token refresh flow.
Let me gather more information about the token handling implementation.
Let me check one more aspect to see if there's any token refresh implementation in other files or if there are any token refresh endpoints being used.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check integration with refresh token mechanism # Expected: Should see proper token refresh handling # Look for refresh token handling rg -A 10 "refresh.*token" "src/utils/getAccessToken.ts" # Check for token refresh related tests fd -e test.ts -e test.js -e spec.ts -e spec.js | xargs rg "refresh.*token" # Look for other components using getAccessToken ast-grep --pattern 'import { getAccessToken } from "$_"'Length of output: 405
Script:
#!/bin/bash # Check the complete implementation of getAccessToken utility cat src/utils/getAccessToken.ts # Look for token-related error handling and refresh logic rg -A 5 "token.*expired|invalid.*token" # Check for any token refresh implementations ast-grep --pattern 'async function refreshToken($$$) { $$$ }' ast-grep --pattern 'const refreshToken = async ($$$) => { $$$ }'Length of output: 2649
Script:
#!/bin/bash # Look for token refresh related endpoints or implementations rg -A 5 "refresh.*token.*endpoint|token.*refresh.*url" # Check for any OAuth token refresh implementations rg -A 10 "oauth.*token|token.*refresh" src/ # Look for refresh token storage or handling in session manager cat src/session/sessionManager.tsLength of output: 1931
src/session/getIdToken.js (1)
4-4: Consider migrating this file to TypeScriptSince this file imports from TypeScript modules and handles sensitive authentication logic, consider migrating it to TypeScript to maintain consistency and leverage type safety across the authentication flow.
src/session/getOrganization.ts (2)
29-29: Consider adding explicit token validationWhile the code correctly decodes the token, consider adding explicit validation of the decoded token's contents before using it.
const accessToken = await getAccessToken(req, res); const decodedToken = jwtDecoder<KindeAccessToken>(accessToken) +if (!decodedToken || !decodedToken.org_code) { + throw new Error('Invalid access token structure'); +} return generateOrganizationObject(idToken, decodedToken);Also applies to: 31-31
Line range hint
32-37: Consider more specific error handlingThe current catch-all error handling could be more specific to help with debugging and user feedback.
} catch (error) { if (config.isDebugMode) { - console.error(error); + console.error('Failed to get organization:', error instanceof Error ? error.message : 'Unknown error'); } return null; }src/utils/getIdToken.ts (2)
6-10: Add JSDoc documentation for better code maintainability.The implementation looks good, but adding JSDoc documentation would improve code maintainability and IDE support.
+/** + * Retrieves and validates the ID token from the session. + * @param req - Next.js API request + * @param res - Next.js API response + * @returns Promise<string | null> - The validated ID token or null if invalid/missing + */ export const getIdToken = async(req: NextApiRequest, res: NextApiResponse) => {
30-37: Consider handling specific error types differently.The current implementation catches all errors and returns null. Consider handling specific error types (e.g., validation errors vs network errors) differently for better error reporting.
- } catch (error) { + } catch (error: unknown) { + if (error instanceof Error) { + if (config.isDebugMode) { + console.error('getIdToken:', error.message); + } + // Handle specific error types + if (error.name === 'ValidationError') { + return null; + } + if (error.name === 'NetworkError') { + throw error; // Let the caller handle network errors + } + } if (config.isDebugMode) { - console.error('getIdToken', error); + console.error('getIdToken: Unknown error occurred'); } return null; }src/utils/getAccessToken.ts (1)
35-40: Consider adding more specific error handling.While the current error handling is functional, it could be more informative by:
- Distinguishing between different types of errors (validation errors vs network errors)
- Providing more context in the error logs
} catch (error) { if (config.isDebugMode) { - console.error('getAccessToken', error); + console.error('getAccessToken error:', { + type: error instanceof Error ? error.name : 'Unknown', + message: error instanceof Error ? error.message : String(error), + stack: error instanceof Error ? error.stack : undefined + }); } return null; }src/handlers/callback.ts (3)
Line range hint
44-53: Strengthen URL validation to prevent open redirect vulnerabilitiesThe current URL validation only checks if the URL starts with 'http', which could allow malicious redirects. Consider implementing stronger URL validation:
if (postLoginRedirectURL) { - if (postLoginRedirectURL.startsWith('http')) { - return routerClient.redirect(postLoginRedirectURL); + // Validate against allowed domains + const allowedDomains = [new URL(routerClient.clientConfig.siteUrl).hostname]; + try { + const redirectUrl = new URL(postLoginRedirectURL); + if (allowedDomains.includes(redirectUrl.hostname)) { + return routerClient.redirect(postLoginRedirectURL); + } + throw new Error('Invalid redirect domain'); + } catch { + // If URL parsing fails or domain not allowed, redirect to relative path + return routerClient.redirect( + `${routerClient.clientConfig.siteUrl}${postLoginRedirectURL}` + ); } - return routerClient.redirect( - `${routerClient.clientConfig.siteUrl}${postLoginRedirectURL}` - ); }
Line range hint
21-40: Consider enhancing error handlingThe error handling could be more robust by:
- Using specific error types instead of string matching
- Logging structured error information in debug mode
} catch (error) { if (config.isDebugMode) { - console.error('callback', error); + console.error('Callback handler error', { + error: error.message, + stack: error.stack, + url: routerClient.getUrl() + }); } - if (error.message.includes('Expected: State not found')) { + if (error instanceof StateNotFoundError) { return routerClient.json( { error: `Error: State not found.\nTo resolve this error please visit our docs https://docs.kinde.com/developer-tools/sdks/backend/nextjs-sdk/#state-not-found-error` + error.message }, {status: 500} ); } - return routerClient.json({error: error.message}, {status: 500}); + return routerClient.json({ + error: 'Authentication failed. Please try again.', + details: config.isDebugMode ? error.message : undefined + }, {status: 500});
Incomplete refresh token implementation found
The code in
getAccessToken.tscontains a comment "look for refresh token" but no actual implementation of token refresh logic. The related files (RouterClient.ts,sessionManager.ts,KindeClient.ts) appear to be missing or not accessible, suggesting the refresh token mechanism mentioned in PR objectives is not fully implemented.
src/utils/getAccessToken.ts: Contains a placeholder comment for refresh token logic but no implementation- Missing critical files:
src/routerClients/RouterClient.tssrc/session/sessionManager.tsKindeClient.ts🔗 Analysis chain
Line range hint
1-53: Verify refresh token implementationWhile this file handles the callback flow, it doesn't directly address the refresh token mechanism mentioned in the PR objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for refresh token related implementations echo "Searching for refresh token related files..." rg -l "refresh.*token" --type ts echo "\nSearching for token validation implementations..." ast-grep --pattern 'function $_(token: $_) { $$$ }'Length of output: 330
Script:
#!/bin/bash # Let's examine the refresh token implementation in getAccessToken.ts echo "Content of getAccessToken.ts:" cat src/utils/getAccessToken.ts # Search for token refresh related methods in RouterClient echo -e "\nSearching for token refresh methods in RouterClient:" rg -A 10 "refresh.*token" src/routerClients/RouterClient.ts # Look for session management implementations echo -e "\nSearching for session token management:" ast-grep --pattern 'class SessionManager { $$$ }'Length of output: 1678
Script:
#!/bin/bash # Let's check the session manager implementation echo "Searching for session manager file:" fd "sessionManager.ts" echo -e "\nChecking session manager content:" cat src/session/sessionManager.ts # Look for KindeClient implementation which might handle token refresh echo -e "\nSearching for KindeClient implementation:" fd "KindeClient.ts"Length of output: 403
src/api-client.js (2)
32-32: Consider caching the API tokenThe current implementation always removes and regenerates the API token, which could be inefficient. Consider implementing token caching with expiration checks to reduce unnecessary token requests.
Here's a suggested implementation:
const store = await sessionManager(req, res); -store.removeSessionItem('kinde_api_access_token'); +const cachedToken = store.getSessionItem('kinde_api_access_token'); +if (cachedToken) { + try { + const decoded = JSON.parse(atob(cachedToken.split('.')[1])); + if (decoded.exp * 1000 > Date.now()) { + return cachedToken; + } + } catch (e) { + // Invalid token format, proceed to fetch new token + } +} +store.removeSessionItem('kinde_api_access_token');
Line range hint
34-45: Add error handling for token fetchThe token fetch operation lacks error handling and timeout management, which could lead to unhandled exceptions or hanging requests.
Consider implementing proper error handling:
- const response = await fetch(`${config.issuerURL}/oauth2/token`, { - method: 'POST', - headers: { - 'content-type': 'application/x-www-form-urlencoded' - }, - body: new URLSearchParams({ - grant_type: 'client_credentials', - client_id: config.clientID || '', - client_secret: config.clientSecret || '', - audience: config.issuerURL + '/api' - }) - }); - apiToken = (await response.json()).access_token; + try { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 5000); + + const response = await fetch(`${config.issuerURL}/oauth2/token`, { + method: 'POST', + headers: { + 'content-type': 'application/x-www-form-urlencoded' + }, + body: new URLSearchParams({ + grant_type: 'client_credentials', + client_id: config.clientID || '', + client_secret: config.clientSecret || '', + audience: config.issuerURL + '/api' + }), + signal: controller.signal + }); + + clearTimeout(timeoutId); + + if (!response.ok) { + throw new Error(`HTTP error! status: ${response.status}`); + } + + const data = await response.json(); + if (!data.access_token) { + throw new Error('No access token received'); + } + + apiToken = data.access_token; + store.setSessionItem('kinde_api_access_token', apiToken); + } catch (error) { + if (error.name === 'AbortError') { + throw new Error('Request timeout while fetching access token'); + } + throw new Error(`Failed to fetch access token: ${error.message}`); + }src/handlers/setup.ts (1)
17-24: Enhance error handling and type safety.While the validation logic is correct, consider these improvements:
- Add more descriptive error messages including validation details
- Add type checking for the token parameter
- const accessTokenValidation = await validateToken({ + if (!accessTokenEncoded || typeof accessTokenEncoded !== 'string') { + throw new Error('Access token is missing or invalid format'); + } + const accessTokenValidation = await validateToken({ token: accessTokenEncoded, domain: config.issuerURL }) if (!accessTokenValidation.valid) { - throw new Error('Invalid access token'); + throw new Error(`Invalid access token: ${accessTokenValidation.error?.message || 'unknown error'}`); }src/session/sessionManager.js (3)
103-110: Consider optimizing cookie cleanup performanceThe cookie cleanup logic could be more efficient by using filter instead of map+forEach.
- cookieStore - .getAll() - .map((c) => c.name) - .forEach((key) => { - if (key.startsWith(`${String(itemKey)}`)) { - cookieStore.delete(key); - } - }); + const cookies = cookieStore.getAll(); + cookies + .filter(c => c.name.startsWith(`${String(itemKey)}`)) + .forEach(c => cookieStore.delete(c.name));
Line range hint
8-13: Enhance security configuration for token cookiesCurrent security configuration has potential vulnerabilities:
- Secure flag is only set in production
- No validation of cookie domain configuration
- No explicit HTTPS enforcement
Consider:
- Always enabling secure flag for token cookies
- Validating cookie domain configuration
- Adding HTTPS enforcement middleware
export const GLOBAL_COOKIE_OPTIONS = { sameSite: 'lax', httpOnly: true, - secure: process.env.NODE_ENV === 'production', + secure: true, // Always require HTTPS for token cookies path: '/' };
Incomplete refresh token implementation detected
The codebase shows an incomplete implementation of refresh token handling:
getAccessToken.tshas a commented placeholder for refresh token logic ("// look for refresh token")sessionManager.jsonly handles storage/retrieval of refresh tokens but lacks actual refresh mechanism- No implementation found for token refresh flow when access token becomes invalid
🔗 Analysis chain
Line range hint
1-1: Verify complete refresh token fix implementationWhile this file improves cookie cleanup, it's unclear how it addresses the refresh token issues mentioned in the PR objectives.
Let's check for related changes:
Let me check the content of these files to understand the refresh token implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Look for other files handling refresh tokens rg -l "refresh.*token" --type ts --type js # Check for recent changes to refresh token handling git log -p --grep="refresh.*token"Length of output: 102
Script:
#!/bin/bash # Check the content of files handling refresh tokens echo "=== sessionManager.js ===" cat src/session/sessionManager.js echo -e "\n=== getAccessToken.ts ===" cat src/utils/getAccessToken.tsLength of output: 9670
src/authMiddleware/authMiddleware.ts (1)
Line range hint
55-68: Simplify conditional logic to reduce redundancyThe conditions
validateTokenResponse.valid && customValidationValidare checked multiple times. Consider refactoring to streamline the control flow and improve readability.Here's a suggested refactor to consolidate the condition checks:
if (!validateTokenResponse.valid || !customValidationValid) { return NextResponse.redirect( new URL(loginRedirectUrl, options?.redirectURLBase || config.redirectURL) ); } - if (validateTokenResponse.valid && customValidationValid && onSuccess) { + if (onSuccess) { return await onSuccess({ token: accessTokenValue, user: { family_name: idTokenValue.family_name, given_name: idTokenValue.given_name, email: idTokenValue.email, id: idTokenValue.sub, picture: idTokenValue.picture } }); } - if (validateTokenResponse.valid && customValidationValid) { - return; - } + return;This refactoring handles the redirect early if validation fails and proceeds with
onSuccessor returns accordingly if validation passes.src/config/index.ts (1)
68-68: Ensure consistent naming conventions for property namesConsider standardizing the casing of property names to improve code consistency and readability. For example, use
clientIdinstead ofclientIDto maintain consistent camelCase formatting across the codebase.Also applies to: 88-88
🛑 Comments failed to post (7)
src/session/isAuthenticated.js (1)
11-14: 🛠️ Refactor suggestion
Add error handling for token and user retrieval
The current implementation lacks explicit error handling for failed token retrieval or user fetch operations. This could lead to silent failures and make debugging more difficult.
Consider adding error handling:
export const isAuthenticatedFactory = (req, res) => async () => { - const token = getAccessToken(req, res); + try { + const token = getAccessToken(req, res); + const user = await getUserFactory(req, res)(); + return token && Boolean(user); + } catch (error) { + console.error('Authentication check failed:', error); + return false; + } - const user = await getUserFactory(req, res)(); - return token && Boolean(user); };Additionally, consider adding logging to track authentication failures for better observability.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export const isAuthenticatedFactory = (req, res) => async () => { try { const token = getAccessToken(req, res); const user = await getUserFactory(req, res)(); return token && Boolean(user); } catch (error) { console.error('Authentication check failed:', error); return false; } };src/session/getUser.ts (1)
14-19: 💡 Codebase verification
Error handling is properly implemented but token refresh is incomplete
The error propagation chain is properly implemented with all functions returning
nullon errors and logging in debug mode. However, there's an incomplete token refresh implementation ingetAccessToken:
getIdTokenproperly validates and returns null for invalid/missing tokensgetAccessTokenhas a commented "look for refresh token" section that's not implementedgetUserproperly handles errors from both token utilities🔗 Analysis chain
Verify error propagation from token utilities.
The code correctly retrieves and decodes both ID and access tokens. However, we should verify that token refresh errors are properly propagated from the new utility functions.
Consider adding specific error types for token-related failures to improve error handling:
interface TokenError extends Error { code: 'TOKEN_EXPIRED' | 'TOKEN_INVALID' | 'TOKEN_MISSING'; }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check error handling in token utility functions # Check error handling in getIdToken ast-grep --pattern 'export function getIdToken($_) { try { $$$ } catch($_) { $$$ } }' # Check error handling in getAccessToken ast-grep --pattern 'export function getAccessToken($_) { try { $$$ } catch($_) { $$$ } }'Length of output: 207
Script:
#!/bin/bash # Let's try a different approach to find the token utility functions and their implementations # Search for getIdToken function definition rg "function getIdToken" -A 10 # Search for getAccessToken function definition rg "function getAccessToken" -A 10 # Also search for any error handling related to these functions rg "throw|catch" --type tsLength of output: 892
Script:
#!/bin/bash # Let's examine the implementation of token utility functions # Check getIdToken implementation cat src/utils/getIdToken.ts # Check getAccessToken implementation cat src/utils/getAccessToken.ts # Check how errors are handled in getUser.ts cat src/session/getUser.tsLength of output: 3402
src/utils/getAccessToken.ts (1)
23-32:
⚠️ Potential issueImplement refresh token logic in the invalid token case.
There's a comment indicating refresh token logic should be implemented here, but the implementation is missing. Given this PR's focus on fixing refresh token functionality, this section should handle token refresh when the current token is invalid.
Would you like me to help implement the refresh token logic? A typical implementation would:
- Check for a refresh token in the session
- If present, use it to obtain a new access token
- Update the session with the new token
- Return the new token or null if refresh fails
src/handlers/callback.ts (1)
6-8: 🛠️ Refactor suggestion
Add type validation before type assertion
The type assertion
as stringcould mask potential type-related issues if the session item is undefined or null. Consider adding validation:- (await routerClient.sessionManager.getSessionItem( - 'post_login_redirect_url' - )) as string; + const redirectUrl = await routerClient.sessionManager.getSessionItem( + 'post_login_redirect_url' + ); + if (redirectUrl !== undefined && typeof redirectUrl !== 'string') { + throw new Error('post_login_redirect_url must be a string'); + } + redirectUrl as string;Committable suggestion skipped: line range outside the PR's diff.
src/handlers/setup.ts (1)
30-37: 🛠️ Refactor suggestion
Refactor duplicate token validation logic.
The ID token validation duplicates the access token validation logic. Consider extracting this into a reusable function.
+ const validateTokenWithType = async (token: string | null, tokenType: string) => { + if (!token || typeof token !== 'string') { + throw new Error(`${tokenType} is missing or invalid format`); + } + const validation = await validateToken({ + token, + domain: config.issuerURL + }); + if (!validation.valid) { + throw new Error(`Invalid ${tokenType}: ${validation.error?.message || 'unknown error'}`); + } + }; + - const idTokenValidation = await validateToken({ - token: idTokenEncoded, - domain: config.issuerURL - }) - - if (!idTokenValidation.valid) { - throw new Error('Invalid id token'); - } + await validateTokenWithType(accessTokenEncoded, 'access token'); + await validateTokenWithType(idTokenEncoded, 'ID token');Committable suggestion skipped: line range outside the PR's diff.
src/config/index.ts (2)
63-63: 🛠️ Refactor suggestion
Define a specific type for
initialStateinstead ofanyUsing
anydefeats the purpose of TypeScript's type safety. Defining a specific type forinitialStatewill improve code maintainability and prevent potential runtime errors.Would you like assistance in defining the appropriate type for
initialState?
71-71: 🛠️ Refactor suggestion
Type
audienceasstring[]and default to an empty arrayTo maintain type consistency and avoid potential runtime errors, consider typing
audienceasstring[]and defaulting to an empty array whenKINDE_AUDIENCEis not provided.Apply this diff to adjust the type and default value:
- audience: string[] | ''; + audience: string[];And update the default assignment in the config object:
- audience: KINDE_AUDIENCE ? KINDE_AUDIENCE.split(' ') : '', + audience: KINDE_AUDIENCE ? KINDE_AUDIENCE.split(' ') : [],Also applies to: 86-86
DanielRivers
force-pushed
the
fix/refresh-token
branch
from
d4ddd10 to
42407fa
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
src/utils/refreshTokens.ts (1)
12-27: Enhance error handling and loggingThe current error handling approach might mask different types of failures and make debugging more difficult.
Consider these improvements:
if (token && typeof token === 'string') { - const validationResult = await validateToken({ - token, - domain: config.issuerURL - }); + try { + const validationResult = await validateToken({ + token, + domain: config.issuerURL + }); - return validationResult.valid + return validationResult.valid; + } catch (validationError) { + if (config.isDebugMode) { + console.error('Token validation failed:', validationError); + } + return false; + } } return false; } catch (error) { if (config.isDebugMode) { - console.error('refreshTokens', error); + console.error('Token refresh failed:', { + error: error instanceof Error ? error.message : 'Unknown error', + stack: error instanceof Error ? error.stack : undefined + }); } return false; }src/handlers/setup.ts (4)
24-26: Fix typo in error message and enhance error handlingThe error message contains a typo ("refrehs" should be "refresh").
- throw new Error('Invalid access token and refrehs failed'); + throw new Error('Invalid access token and refresh failed');
24-26: Consider enhancing the refresh token implementationThe current implementation has several potential improvements to consider:
- Add a mechanism to prevent infinite refresh loops
- Include more detailed error information (e.g., original validation error)
- Consider adding specific error types for better error handling
Consider implementing something like this:
if (!accessTokenValidation.valid) { try { const refreshResult = await refreshTokens(routerClient.sessionManager); if (!refreshResult) { throw new Error(`Token refresh failed: ${accessTokenValidation.error}`); } } catch (refreshError) { throw new Error(`Failed to refresh invalid access token: ${refreshError.message}. Original validation error: ${accessTokenValidation.error}`); } }
Line range hint
93-109: Consider enhancing error handling for different JWT error typesThe current error handling only specifically handles
ERR_JWT_EXPIRED. Consider handling other JWT-specific error cases and providing more detailed error information in debug mode.} catch (error) { if (config.isDebugMode) { console.debug('Token setup error:', { error, code: error.code, message: error.message, stack: error.stack }); } switch (error.code) { case 'ERR_JWT_EXPIRED': return routerClient.json( { needsRefresh: true }, { status: 200 } ); case 'ERR_JWT_INVALID': case 'ERR_JWT_MALFORMED': return routerClient.json( { error: 'Invalid token format' }, { status: 401 } ); default: return routerClient.json( { error: 'Internal server error' }, { status: 500 } ); } }
Line range hint
13-109: Consider adding atomic token refresh and retry mechanismThe current implementation could benefit from additional architectural improvements:
- Consider implementing an atomic token refresh mechanism to prevent race conditions when multiple requests attempt to refresh tokens simultaneously.
- Add retry logic with exponential backoff for token refresh attempts.
- Consider implementing a token cache to reduce the number of validation and refresh operations.
Would you like assistance in implementing these architectural improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/handlers/setup.ts(2 hunks)src/utils/getAccessToken.ts(2 hunks)src/utils/refreshTokens.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/getAccessToken.ts
🔇 Additional comments (2)
src/utils/refreshTokens.ts (1)
1-4: LGTM! Imports are well-organized
The imports are properly structured with a clear separation between external packages and internal modules.
src/handlers/setup.ts (1)
6-6: Verify the refreshTokens utility implementation
The import suggests a new utility file. Let's ensure it exists and follows best practices.
✅ Verification successful
Implementation of refreshTokens utility looks correct and follows best practices
The utility implementation:
- Properly handles token refresh using Kinde SDK's client
- Validates the refreshed token using @kinde/jwt-validator
- Includes appropriate error handling and debug logging
- Returns boolean indicating success/failure of refresh operation
- Uses correct type annotations with SessionManager
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the refreshTokens utility implementation
# Check if the file exists and examine its implementation
fd "refreshTokens.ts" --type f --exec cat {}
Length of output: 896
DanielRivers
force-pushed
the
fix/refresh-token
branch
from
a245057 to
fcdca63
Compare
DanielRivers
force-pushed
the
fix/refresh-token
branch
from
73a7f23 to
e753ebf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/session/getUser.ts (1)
Line range hint
14-24: Consider implementing comprehensive token validationWhile the type assertions handle the immediate type safety concerns, consider implementing a more robust token validation strategy:
- Add token expiration checks before attempting refresh
- Consider implementing retry logic for failed refresh attempts
- Add logging for token refresh events in debug mode
Would you like assistance in implementing these improvements?
src/session/getOrganization.ts (1)
Line range hint
29-33: Consider adding refresh token error handlingThe catch block currently returns null for all errors, which might mask specific token refresh failures. Consider adding specific error handling for refresh token scenarios.
Apply this diff to improve error handling:
} catch (error) { if (config.isDebugMode) { - console.error(error); + if (error instanceof Error) { + console.error('Organization retrieval failed:', error.message); + // Log specific error types for token refresh failures + if (error.message.includes('refresh')) { + console.error('Token refresh failed - user may need to re-authenticate'); + } + } } return null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/handlers/setup.ts(3 hunks)src/session/getOrganization.ts(1 hunks)src/session/getUser.ts(1 hunks)src/utils/getAccessToken.ts(2 hunks)src/utils/getIdToken.ts(2 hunks)src/utils/refreshTokens.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/handlers/setup.ts
- src/utils/getAccessToken.ts
- src/utils/refreshTokens.ts
🔇 Additional comments (7)
src/session/getUser.ts (2)
20-20: 🛠️ Refactor suggestion
Enhance error handling for token refresh scenarios
With the new refresh token mechanism in getAccessToken, the type assertion might mask potential errors during the refresh process. Consider:
- Adding specific error handling for refresh failures
- Documenting the possible return types and error scenarios
Here's a suggested improvement:
- const accessToken = await getAccessToken(req, res) as string;
+ const accessToken = await getAccessToken(req, res);
+ if (typeof accessToken !== 'string') {
+ if (config.isDebugMode) {
+ console.debug('Access token refresh failed:', accessToken);
+ }
+ return null;
+ }Let's verify the token refresh implementation:
14-14: Verify type assertion necessity and token refresh handling
The type assertion as string might be redundant since there's an explicit null check immediately after. Consider letting TypeScript infer the type or using type guards instead.
Let's verify the return type of getIdToken and its usage:
✅ Verification successful
Let me gather more information about the getIdToken implementation since we only see its usage but not its definition.
Let's check the content of these files to understand the implementation and return types.
Type assertion is necessary and correctly implemented
The type assertion as string is actually necessary and correct here because:
- The
getIdTokenfunction insrc/utils/getIdToken.tsreturns either a string token or null - The subsequent code specifically handles the null case with an explicit check
- When the token exists, it's guaranteed to be a string due to the runtime check
typeof token !== 'string'in the implementation - The type assertion helps TypeScript understand that non-null values are definitely strings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check getIdToken implementation and usage
# Expected: Find the return type definition and other usages to ensure type consistency
# Check getIdToken implementation
ast-grep --pattern 'export $_ getIdToken = $_'
# Check other usages of getIdToken
rg -A 3 'getIdToken\(' --type ts
Length of output: 288
Script:
#!/bin/bash
# Search for getIdToken function definition
ast-grep --pattern 'function getIdToken($_) {
$$$
}'
# Also check for const/let/var declaration
ast-grep --pattern '$_ getIdToken = $_'
# Search for files containing getIdToken to ensure we don't miss any implementation
fd getIdToken
Length of output: 193
Script:
#!/bin/bash
# Check content of all files related to getIdToken
cat src/session/getIdToken.js src/session/getIdTokenRaw.js src/utils/getIdToken.ts
# Also search for type definitions related to these functions
rg -g '*.d.ts' 'getIdToken'
Length of output: 2597
src/session/getOrganization.ts (1)
28-29: Verify token refresh integration
Since getAccessToken has been updated to include refresh token functionality, we should verify that the token refresh mechanism works correctly with this implementation.
Let's verify the token refresh integration:
✅ Verification successful
Let me gather more information about the token refresh implementation.
Let me check the complete implementation of getAccessToken to understand the full context of the refresh token logic.
Token refresh mechanism is properly integrated and handled
The implementation in getAccessToken.ts shows a robust token refresh flow:
- Validates the existing token first
- Attempts to refresh the token when validation fails
- Includes proper error handling and debugging
- Returns null for invalid tokens, triggering re-authentication
The usage in getOrganization.ts is correct as it properly handles the token type and potential failures.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of getAccessToken and its usage
# Expected: Find the refresh token logic and verify error handling
# Check getAccessToken implementation
echo "Checking getAccessToken implementation:"
ast-grep --pattern 'export const getAccessToken = async $_' -A 20
# Check for other files using getAccessToken to ensure consistent error handling
echo "Checking getAccessToken usage:"
rg "getAccessToken\(" --type ts -A 2
Length of output: 681
Script:
#!/bin/bash
# Let's check the implementation of getAccessToken and related token refresh logic
# Find the getAccessToken implementation file
echo "Finding getAccessToken implementation file:"
fd "getAccessToken.ts"
# Search for token refresh related code
echo -e "\nSearching for token refresh implementation:"
rg "refresh.*token" --type ts -A 3
# Look for error handling in token-related files
echo -e "\nChecking error handling in token-related code:"
rg "throw.*Error.*token" --type ts -A 2
Length of output: 1752
Script:
#!/bin/bash
# Get the complete implementation of getAccessToken
echo "Content of getAccessToken.ts:"
cat src/utils/getAccessToken.ts
# Check for any session-related types and interfaces
echo -e "\nChecking for session-related types:"
rg "interface.*Session|type.*Session" --type ts -A 3
Length of output: 1555
src/utils/getIdToken.ts (4)
5-8: Good improvement with string constant extraction!
Moving the token key to a constant improves maintainability and reduces the risk of typos. The new import is properly aligned with the refresh token functionality.
Line range hint 11-23: Good addition of type checking!
The explicit type check typeof token !== 'string' improves type safety and prevents potential runtime issues. The validation flow is well-structured.
Line range hint 29-42: Error handling looks good!
The error handling is consistent and properly utilizes the debug mode configuration for logging.
5-5: Verify kindeClient integration and usage
Let's ensure the refresh token implementation is consistent across the codebase.
✅ Verification successful
Based on the results, I can now provide a final response as the verification is complete. The implementation shows a consistent pattern across the codebase:
Token refresh implementation is properly integrated
The refresh token implementation is consistent across the codebase with proper integration:
- Centralized
refreshTokensutility insrc/utils/refreshTokens.ts - Consistent usage pattern in token validation flows:
getAccessToken.tsgetIdToken.tssetup.ts
- Proper error handling and debug logging
- Consistent integration with
kindeClientand token validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other uses of refreshTokens and token validation patterns
echo "Checking for refreshTokens usage patterns..."
rg "refreshTokens" -A 5 -B 5
echo "Checking for token validation patterns..."
rg "validateToken" -A 5 -B 5
echo "Checking for other potential token refresh implementations..."
ast-grep --pattern 'if (!$token) { $$$ }'
Length of output: 16036
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
src/utils/validateToken.ts (4)
17-17: Remove debug console.log statementThis debug statement should not be present in production code.
- console.log('attempting to validate token');
10-15: Enhance error logging specificityThe error message could be more specific about whether the token is missing or has an invalid type.
if (config.isDebugMode) { - console.error('validateToken: invalid token or token is missing'); + console.error( + `validateToken: ${!token ? 'token is missing' : 'token must be a string'}` + ); }
18-28: Add type assertion and enhance error detailsConsider adding type assertion for validation result and including more error details in debug mode.
const validationResult = await jwtValidator({ token, domain: config.issuerURL - }); + }) as { valid: boolean; error?: string }; if (!validationResult.valid) { if (config.isDebugMode) { - console.error('validateToken: invalid token'); + console.error('validateToken: invalid token', + validationResult.error || 'unknown validation error' + ); } return false; }
30-36: Add type safety for decoded tokenConsider adding type interface for decoded token and extracting the timestamp conversion to a constant for clarity.
+ interface DecodedToken { + exp?: number; + iss?: string; + } + const currentTimestamp = Date.now() / 1000; - const decodedToken = jwtDecoder(token); + const decodedToken = jwtDecoder(token) as DecodedToken; if (decodedToken.exp && decodedToken.exp < currentTimestamp) {src/routerClients/AppRouterClient.js (1)
Line range hint
1-93: Consider enhancing token refresh capabilitiesGiven that this PR aims to implement automatic token refresh functionality (per issue #237), consider enhancing this class to support token refresh operations. This could include:
- Adding configuration options for token refresh (e.g., refreshThreshold)
- Implementing methods to handle token refresh logic
- Adding event handlers for token expiry
This would provide a more complete solution for the token refresh requirements.
Would you like me to provide a detailed implementation proposal for these enhancements?
src/session/sessionManager.js (1)
Line range hint
1-400: Consider architectural improvements for token refresh.While the session management implementation is solid, here are some architectural suggestions to enhance the token refresh mechanism:
Consider adding a method to the session manager interface specifically for token refresh operations. This would centralize the refresh logic and make it easier to maintain.
The current implementation splits cookies when they exceed MAX_LENGTH. For refresh tokens, consider implementing compression to reduce the cookie size and minimize the number of cookie chunks.
Here's a suggested interface enhancement:
interface SessionManager { // ... existing methods ... /** * Refreshes the access token using the stored refresh token * @returns {Promise<boolean>} True if refresh was successful */ refreshAccessToken(): Promise<boolean>; /** * Validates if the current access token needs refresh * @returns {Promise<boolean>} True if token needs refresh */ needsTokenRefresh(): Promise<boolean>; }This would allow for:
- Centralized token refresh logic
- Proactive token refresh before expiration
- Better error handling and retry mechanisms
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
src/authMiddleware/authMiddleware.ts(2 hunks)src/handlers/setup.ts(2 hunks)src/routerClients/AppRouterClient.js(1 hunks)src/session/getAccessToken.js(2 hunks)src/session/getIdToken.js(1 hunks)src/session/getOrganization.ts(2 hunks)src/session/getUser.ts(2 hunks)src/session/isAuthenticated.js(1 hunks)src/session/sessionManager.js(1 hunks)src/utils/generateOrganizationObject.ts(1 hunks)src/utils/getAccessToken.ts(1 hunks)src/utils/getIdToken.ts(1 hunks)src/utils/refreshTokens.ts(1 hunks)src/utils/validateToken.ts(1 hunks)src/utils/version.js(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- src/authMiddleware/authMiddleware.ts
- src/session/getAccessToken.js
- src/session/getIdToken.js
- src/session/isAuthenticated.js
- src/utils/generateOrganizationObject.ts
- src/utils/version.js
🚧 Files skipped from review as they are similar to previous changes (6)
- src/handlers/setup.ts
- src/session/getOrganization.ts
- src/session/getUser.ts
- src/utils/getAccessToken.ts
- src/utils/getIdToken.ts
- src/utils/refreshTokens.ts
🔇 Additional comments (7)
src/utils/validateToken.ts (2)
1-9: LGTM! Well-structured imports and type definitions.
The function signature is properly typed and the necessary dependencies are correctly imported.
1-52: Verify integration with token refresh mechanism
While the token validation logic is solid, we should verify how this integrates with the token refresh mechanism mentioned in the PR objectives.
✅ Verification successful
Token validation is properly integrated with refresh mechanism
The verification shows that the token validation is correctly integrated with the refresh token flow:
- In
getAccessToken.tsandgetIdToken.ts, invalid tokens trigger the refresh flow viakindeClient.refreshTokens - The
refreshTokens.tsutility validates the newly obtained tokens using the same validation logic - The
setup.tshandler implements proper validation and refresh sequence for both access and ID tokens - The middleware correctly uses token validation before allowing access
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to validateToken in the refresh token implementation
rg -l "validateToken" | grep -v "validateToken.ts" | xargs rg -A 10 "validateToken"
# Look for refresh token related functions
ast-grep --pattern 'function $FUNC_NAME($$$) {
$$$
refreshToken
$$$
}'
Length of output: 7738
src/routerClients/AppRouterClient.js (3)
39-39: LGTM!
Cosmetic change that improves readability.
45-45: LGTM!
Minor formatting improvement.
Line range hint 15-38: Verify token-related configuration
The constructor handles various authentication configurations, but there might be missing properties needed for token refresh functionality.
✅ Verification successful
Let me gather more information about the token refresh functionality by checking the implementation details.
Let me check one final aspect to verify if all necessary token-related configurations are properly handled in the client configuration.
Token refresh functionality is properly configured
Based on the verification results, the token refresh functionality is well-implemented and properly configured:
- Token refresh is handled through the
refreshTokensutility insrc/utils/refreshTokens.ts - The client configuration includes all necessary properties for authentication and token management
- Token refresh is integrated with session management and error handling
- The
createKindeServerClientis properly initialized with the required configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any token refresh related configurations in other files
rg -A 5 "token.*refresh|refresh.*token" --type js --type ts
Length of output: 1842
Script:
#!/bin/bash
# Check the implementation of refreshTokens method and token-related configurations
ast-grep --pattern 'refreshTokens($_) {
$$$
}'
# Check for token-related configuration properties
rg -A 2 "tokenRefresh|refreshToken" --type js --type ts
Length of output: 2031
Script:
#!/bin/bash
# Check the client configuration structure and token-related settings
ast-grep --pattern 'const config = {
$$$
}'
# Also check for any token-related configuration in the createKindeServerClient
ast-grep --pattern 'createKindeServerClient($$$)'
Length of output: 1030
src/session/sessionManager.js (2)
33-33: LGTM! Minor formatting change.
The formatting change in the splitString function maintains the same logic while improving code consistency.
Line range hint 8-15: Verify cookie security settings for token storage.
The current cookie security settings look good with httpOnly, secure (in production), and sameSite: 'lax'. However, since this file handles sensitive token storage, let's verify there are no security advisories for the cookie package being used.
✅ Verification successful
Let me check the package version being used to determine if we're affected by this vulnerability.
Cookie security settings are verified and safe
The cookie security settings in GLOBAL_COOKIE_OPTIONS are well-configured, and the project uses cookie v1.0.1 which is not affected by the known vulnerability (CVE affects versions < 0.7.0). The implementation includes all recommended security headers:
httpOnly: trueprevents XSS attackssecure: truein production ensures HTTPS-onlysameSite: 'lax'provides CSRF protection
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for security advisories in the cookie package
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "cookie") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 574
Script:
#!/bin/bash
# Check package.json for cookie dependency version
rg '"cookie":\s*"[^"]*"' -g 'package.json'
Length of output: 79
Explain your changes
Automatically refresh tokens on retrieval
Resolves: #237
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.