feat: add isAuthenticated and refreshToken functions #11
Conversation
WalkthroughThis pull request introduces several modifications related to authentication and utility functions in the codebase. It adds two new exports, Changes
Possibly related PRs
Warning Rate limit exceeded@DanielRivers has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 13 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. 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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #11 +/- ##
==========================================
+ Coverage 83.13% 86.16% +3.03%
==========================================
Files 29 32 +3
Lines 753 918 +165
Branches 122 166 +44
==========================================
+ Hits 626 791 +165
Misses 127 127
|
DanielRivers
force-pushed
the
feat/refreshToken
branch
from
b6fa93b to
721f088
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (12)
lib/utils/token/index.ts (1)
44-45: LGTM: New exports added correctly. Consider sorting exports alphabetically.The new exports for
isAuthenticatedandrefreshTokenare correctly added, making these functions available to consumers of this module. This aligns with the PR objectives.For improved readability and consistency, consider sorting all exports alphabetically. This would make it easier to locate specific exports in the future as the list grows.
lib/main.test.ts (2)
42-43: LGTM! Consider grouping related exports.The addition of
isAuthenticatedandrefreshTokento theexpectedExportsarray is correct and aligns with the PR objectives. These new functions will now be properly tested as part of the index exports.Consider grouping related exports together within the
expectedExportsarray for better readability and maintenance. For example, you could move these new authentication-related functions next to other auth-related exports if they exist, or create a new group for them.
Missing Exports for
isAuthenticatedandrefreshTokenin main.tsThe
isAuthenticatedandrefreshTokenfunctions are implemented and have corresponding test files, but they are not exported fromlib/main.ts. Please ensure these functions are exported to make them available as part of the library's public API.
- Update
lib/main.tsto include exports forisAuthenticatedandrefreshToken.🔗 Analysis chain
Line range hint
1-69: Summary: New exports successfully added to test suiteThe changes in this file successfully integrate the new
isAuthenticatedandrefreshTokenfunctions into the test suite for index exports. This ensures that these new functions will be properly tested as part of the library's public API.To ensure consistency across the codebase, let's verify the implementation of these new functions:
This script will help us confirm that the new functions are properly implemented, exported, and have corresponding test files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of isAuthenticated and refreshToken functions # Test 1: Check for the implementation files echo "Checking for implementation files:" fd -e ts -e js "isAuthenticated|refreshToken" # Test 2: Verify exports in main file echo "\nVerifying exports in main file:" rg -n "export.*isAuthenticated|export.*refreshToken" lib/main.ts # Test 3: Check for corresponding test files echo "\nChecking for test files:" fd -e test.ts -e test.js "isAuthenticated|refreshToken"Length of output: 612
lib/utils/token/isAuthenticated.test.ts (2)
38-79: LGTM with suggestion: Comprehensive tests for expired tokensThese test cases effectively cover various scenarios involving expired tokens, including refresh attempts. The mocking and assertions are appropriate for each case.
Consider enhancing the test case for successful token refresh (lines 48-64) by verifying the state after the refresh. You could add an assertion to check if
getDecodedTokenis called again after the refresh:expect(tokenUtils.getDecodedToken).toHaveBeenCalledTimes(2);This would ensure that the function not only attempts to refresh the token but also retrieves the new token after a successful refresh.
81-93: LGTM with suggestion: Proper error handling testThis test case effectively covers the scenario where an exception occurs during token decoding. The mocking of
console.errorand the assertions are appropriate.Consider refactoring the error handling in the
isAuthenticatedfunction to use a custom error logger instead ofconsole.error. This would make the function more flexible and easier to test. You could then update this test to verify that the custom logger is called correctly.Example refactor:
import { logger } from './logger'; // Create a custom logger // In isAuthenticated function } catch (error) { logger.error("Error checking authentication:", error); return false; } // In the test file import { logger } from './logger'; // ... in the test case const loggerSpy = vi.spyOn(logger, 'error').mockImplementation(() => {}); // ... after calling isAuthenticated expect(loggerSpy).toHaveBeenCalledWith( "Error checking authentication:", mockError );This approach would make it easier to customize logging behavior in different environments and improve testability.
lib/utils/token/refreshToken.test.ts (4)
17-28: Consider removing or implementing the commented out sanitizeURL mock.The beforeEach and afterEach hooks are well-implemented, ensuring a clean state for each test. However, there's a commented out line for mocking
sanitizeURL:// vi.spyOn(Utils, 'sanatizeURL').mockImplementation((url) => url);This might indicate an incomplete implementation or forgotten cleanup. Consider either removing this line if it's no longer needed or implementing it if it's required for the tests.
46-51: Consider removing the commented out mock setup.This test case correctly verifies the behavior when no refresh token is found. However, there's a commented out line:
// mockStorage.getSessionItem.mockResolvedValue(null);This might be an alternative implementation or leftover code. Consider removing this line if it's no longer needed, or implement it if it's a better way to set up the test condition.
76-86: Consider adding an error logging assertion.This test case effectively verifies the behavior when the response doesn't contain an access token. However, to make it more comprehensive, consider adding an assertion to check for error logging. This would ensure that the function not only returns
falsebut also logs the error appropriately.Example:
expect(console.error).toHaveBeenCalledWith("No access token in response");Otherwise, the test case is well-structured and covers an important edge case.
119-134: LGTM: Good test for URL sanitization. Consider a more specific assertion.This test case effectively verifies that the domain URL is properly sanitized before making the fetch request. The use of
expect.stringContainingallows for flexible matching of the URL.To make the test even more robust, consider using a more specific assertion for the URL. Instead of:
expect.stringContaining(`https://example.com/oauth2/token`)You could use an exact match:
`https://example.com/oauth2/token`This would ensure that the URL is exactly as expected, including any query parameters if they're part of the implementation.
Otherwise, the test is well-structured and covers an important aspect of the function's behavior.
lib/utils/token/isAuthenticated.ts (2)
20-23: Enhance JSDoc comments to document parametersThe JSDoc comment for
isAuthenticatedcurrently lacks details about its parameters. Providing descriptions for thepropsparameter and its properties would improve code readability and developer experience.Update the JSDoc comment as follows:
/** * Check if the user is authenticated with an option to refresh the token. + * @param {IsAuthenticatedProps} [props] - Optional properties. + * @param {boolean} [props.useRefreshToken] - Whether to use the refresh token if the access token is expired. + * @param {string} [props.domain] - The authentication domain (required if using refresh token). + * @param {string} [props.clientId] - The client ID (required if using refresh token). * @returns { Promise<boolean> } */
25-26: Consider making 'props' a required parameter or provide default valuesWhile
propsis optional, certain properties may be required when using the refresh token functionality. To improve clarity, consider makingpropsa required parameter or provide default values where appropriate.lib/utils/token/refreshToken.ts (1)
71-72: Improve error logging for better debuggingUsing
console.errorprovides minimal information. Consider enhancing the error logging to include more context or integrate with a logging framework consistent with the project's standards.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- lib/main.test.ts (1 hunks)
- lib/utils/token/index.ts (2 hunks)
- lib/utils/token/isAuthenticated.test.ts (1 hunks)
- lib/utils/token/isAuthenticated.ts (1 hunks)
- lib/utils/token/refreshToken.test.ts (1 hunks)
- lib/utils/token/refreshToken.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (12)
lib/utils/token/index.ts (2)
13-14: LGTM: New imports added correctly.The new imports for
isAuthenticatedandrefreshTokenare added correctly, following the existing import pattern in the file. This maintains consistency and modularity in the codebase.
Line range hint
1-48: Overall changes look good. Consider updating documentation.The additions of
isAuthenticatedandrefreshTokenfunctions enhance the module's capabilities without disrupting the existing structure. These non-breaking changes align well with the file's purpose of providing token-related utilities.To ensure comprehensive documentation:
- Update any relevant README files or API documentation to include these new functions.
- Consider adding JSDoc comments for the new functions in their respective implementation files.
Let's verify if there's any existing documentation that needs updating:
lib/utils/token/isAuthenticated.test.ts (2)
1-18: LGTM: Proper test setup and mockingThe test file is well-structured with appropriate imports, mocking, and setup. The use of
vi.mock("..")to mock the entire token utils module and the fixed timestamp setup ensure isolated and consistent test execution.
20-36: LGTM: Comprehensive tests for token presence and validityThese test cases effectively cover two crucial scenarios: absence of a token and presence of a valid token. The mocking of
getDecodedTokenand the assertions are appropriate for each case.lib/utils/token/refreshToken.test.ts (5)
1-15: LGTM: Imports and initial setup are well-structured.The import statements and initial setup for the test suite are comprehensive and well-organized. The mock data and storage object are appropriately defined for the subsequent tests.
30-44: LGTM: Comprehensive tests for missing parameters.These test cases effectively verify the behavior of
refreshTokenwhen essential parameters (domain and clientId) are missing. The assertions check both the return value and the error logging, ensuring proper error handling.
53-74: LGTM: Thorough error handling tests.These test cases effectively verify the
refreshTokenfunction's behavior under different error conditions:
- Fetch request failure
- Unsuccessful response
The mocking is well-implemented, and the assertions comprehensively check both the return value and error logging. This ensures robust error handling in the token refresh process.
88-117: LGTM: Comprehensive test for successful token refresh.This test case effectively verifies the successful path of the token refresh process. It checks:
- The function returns
true.- The session storage is updated with new token values for access token, ID token, and refresh token.
The mocking is well-implemented, and the assertions are thorough, covering both the return value and the side effects of updating the session storage.
1-135: Overall, excellent test coverage with minor suggestions for improvement.This test suite for the
refreshTokenfunction is comprehensive and well-structured. It covers various scenarios including error cases, edge cases, and the successful path. The use of mocks and spies effectively isolates the function under test.Some minor improvements to consider:
- Clean up commented-out code or implement if necessary.
- Add error logging assertions where missing.
- Use more specific URL matching in the sanitization test.
These small enhancements would further strengthen an already robust test suite. Great job on the thorough testing approach!
lib/utils/token/isAuthenticated.ts (2)
33-35: 🛠️ Refactor suggestionSimplify condition by checking 'props?.useRefreshToken' directly
Since
useRefreshTokendefaults toundefined, the condition can be simplified for better readability.Simplify the conditional statement:
-if (isExpired && props?.useRefreshToken) { +if (isExpired && props && props.useRefreshToken) {Likely invalid or redundant comment.
33-35: Ensure 'refreshToken' returns 'Promise' to match the expected return typeThe
isAuthenticatedfunction returns aPromise<boolean>. Verify that therefreshTokenfunction also returns aPromise<boolean>to maintain consistency and fulfill the expected return type ofisAuthenticated.Run the following script to check the return type of
refreshToken:✅ Verification successful
Verified:
refreshTokencorrectly returns aPromise<boolean>as expected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the export and return type of 'refreshToken' function. # Search for the 'refreshToken' function export and display its signature. rg --type typescript -A 2 -B 2 '^export .*function refreshToken' # Alternatively, if 'refreshToken' is an exported constant function: rg --type typescript -A 2 -B 2 '^export const refreshToken'Length of output: 198
Script:
#!/bin/bash # Description: Check the export and return type of 'refreshToken' function. # Search for the 'refreshToken' function export and display its signature within .ts files. rg --glob '*.ts' -A 2 -B 2 '^export\s+function\s+refreshToken' # Alternatively, if 'refreshToken' is an exported constant function within .ts files: rg --glob '*.ts' -A 2 -B 2 '^export\s+const\s+refreshToken'Length of output: 399
Script:
#!/bin/bash # Description: Retrieve the full implementation of the 'refreshToken' function to verify its return type. # Search for the 'refreshToken' function implementation and display its complete body. rg --glob '*.ts' -A 20 '^export\s+const\s+refreshToken\s*=' lib/utils/token/refreshToken.tsLength of output: 568
Script:
#!/bin/bash # Description: Retrieve the complete implementation of the 'refreshToken' function to verify its return type. # Search for the 'refreshToken' function implementation and display its complete body within .ts files. rg --glob '*.ts' -A 40 '^export\s+const\s+refreshToken\s*=' lib/utils/token/refreshToken.tsLength of output: 1102
Script:
#!/bin/bash # Description: Retrieve the complete implementation of the 'refreshToken' function to verify its return type. # Search for the 'refreshToken' function implementation and display its complete body within .ts files. rg --glob '*.ts' -A 100 '^export\s+const\s+refreshToken\s*=' lib/utils/token/refreshToken.tsLength of output: 1686
lib/utils/token/refreshToken.ts (1)
38-41:⚠️ Potential issueCorrect 'Content-Type' header and request body format
The
Content-Typeheader is set to'multipart/form-data', but the request body is being sent as a JSON string. When sending JSON data, theContent-Typeshould be'application/json'. Alternatively, if the server expects form data, you should construct and send form data accordingly.Apply this diff to set the correct header and adjust the request body:
Option 1: Send JSON data
headers: { - "Content-Type": "multipart/form-data", + "Content-Type": "application/json", "Kinde-SDK": "js-utils", }, body: JSON.stringify({ refresh_token: refreshTokenValue, grant_type: "refresh_token", client_id: clientId, }),Option 2: Send URL-encoded form data
headers: { - "Content-Type": "multipart/form-data", + "Content-Type": "application/x-www-form-urlencoded", "Kinde-SDK": "js-utils", }, -body: JSON.stringify({ +body: new URLSearchParams({ refresh_token: refreshTokenValue, grant_type: "refresh_token", client_id: clientId, }),Please confirm the expected
Content-Typeand adjust the request accordingly.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
lib/utils/base64UrlEncode.ts (1)
Line range hint
1-4: Update documentation to reflect both input types.The JSDoc comment needs to be updated to accurately describe both accepted input types and their handling.
/** - * Encodes the provided ArrayBuffer string to base-64 format. - * @param str String to encode + * Encodes the provided string or ArrayBuffer to base-64 URL format. + * @param str - Input string or ArrayBuffer to encode * @returns encoded string */lib/utils/mapLoginMethodParamsForUrl.ts (1)
Line range hint
4-28: Consider improving type safety of the return valueThe function's return type could be made more explicit to avoid the type assertion.
Consider this improvement:
export const mapLoginMethodParamsForUrl = ( options: Partial<LoginMethodParams>, -): Record<string, string> => { +): Record<string, string> => { const translate: Record<string, string> = Object.entries({ login_hint: options.loginHint, is_create_org: options.isCreateOrg?.toString(), connection_id: options.connectionId, redirect_uri: options.redirectURL ? sanitizeUrl(options.redirectURL) : undefined, audience: options.audience || "", scope: options.scope?.join(" ") || "email profile openid offline", prompt: options.prompt, lang: options.lang, org_code: options.orgCode, org_name: options.orgName, has_success_page: options.hasSuccessPage?.toString(), workflow_id: options.workflowId, release_id: options.releaseId, }) .filter(([_, value]) => value !== undefined) - .reduce((acc, [key, value]) => ({ ...acc, [key]: value }), {}); + .reduce<Record<string, string>>((acc, [key, value]) => ({ ...acc, [key]: value as string }), {}); - return translate as Record<string, string>; + return translate; };lib/utils/generateAuthUrl.ts (1)
66-77: Remove commented-out code.The implementation is correct, but please remove the commented-out code as it's no longer needed.
const data = new TextEncoder().encode(codeVerifier); const hashed = await crypto.subtle.digest("SHA-256", data); - // const hashArray = Array.from(new Uint8Array(hashed)); - // const hashString = hashArray.map((b) => String.fromCharCode(b)).join(""); const codeChallenge = base64UrlEncode(hashed);lib/utils/exchangeAuthCode.ts (1)
Line range hint
1-144: Consider enhancing token type safety and refresh flow integration.Since this PR introduces new authentication-related functions (
isAuthenticatedandrefreshToken), consider the following architectural improvements:
- Create shared TypeScript interfaces for token responses and validation
- Add explicit token expiration handling
- Consider implementing token refresh pre-emptively before expiration
Example shared interface:
interface TokenResponse { access_token: string; id_token: string; refresh_token: string; expires_in: number; token_type: string; } interface TokenValidation { isValid: boolean; expiresAt: number; needsRefresh: boolean; }lib/utils/token/refreshToken.ts (2)
5-8: Enhance the function documentationConsider providing more detailed JSDoc comments to describe the purpose of the
refreshTokenfunction, its parameters (domainandclientId), and the conditions under which it returnstrueorfalse. This will help other developers understand how to use this function correctly.
77-77: Avoid logging sensitive error informationLogging the
errorobject directly might expose sensitive information in the console. Consider logging a more general error message or ensuring that the error details do not contain sensitive data before logging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
lib/main.test.ts(1 hunks)lib/utils/base64UrlEncode.ts(1 hunks)lib/utils/exchangeAuthCode.ts(2 hunks)lib/utils/generateAuthUrl.ts(2 hunks)lib/utils/index.ts(2 hunks)lib/utils/mapLoginMethodParamsForUrl.ts(2 hunks)lib/utils/sanitizeUrl.ts(1 hunks)lib/utils/token/index.ts(2 hunks)lib/utils/token/isAuthenticated.test.ts(1 hunks)lib/utils/token/isAuthenticated.ts(1 hunks)lib/utils/token/refreshToken.test.ts(1 hunks)lib/utils/token/refreshToken.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- lib/utils/index.ts
- lib/utils/sanitizeUrl.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/main.test.ts
- lib/utils/token/index.ts
- lib/utils/token/isAuthenticated.test.ts
- lib/utils/token/refreshToken.test.ts
🔇 Additional comments (10)
lib/utils/base64UrlEncode.ts (1)
6-22: Verify usage in authentication context.
Let's verify how this function is used in the new authentication functions to ensure proper integration.
✅ Verification successful
Let me gather more information about the token-related functionality to understand the complete authentication flow.
Changes to base64UrlEncode are safely integrated in the authentication flow
The verification shows that base64UrlEncode is properly integrated and used in the authentication flow:
- It's used in the PKCE (Proof Key for Code Exchange) implementation within generateAuthUrl.ts for generating code challenges
- The function handles both string and ArrayBuffer inputs correctly, which is essential for the PKCE flow where it processes SHA-256 hashed data
- The changes maintain compatibility with the existing authentication flow, as evidenced by passing tests in generateAuthUrl.test.ts
- The function is not directly used in token handling or refresh flows, which use separate JWT-based mechanisms
The modifications to base64UrlEncode.ts are safe and properly support the authentication functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of base64UrlEncode in authentication-related code
# Expected: Find usage in isAuthenticated and refreshToken functions
# Search for imports and usage
rg -l "base64UrlEncode" | xargs rg -A 5 "base64UrlEncode"
# Search for any token-related files that might use this function
fd -e ts -e js token
Length of output: 4786
Script:
#!/bin/bash
# Check the implementation of token-related files and their dependencies
cat lib/utils/token/getDecodedToken.ts lib/utils/token/refreshToken.ts
# Check if there are any authentication-related files that might use base64UrlEncode
fd -e ts auth | xargs cat
# Check the implementation of generateAuthUrl which uses base64UrlEncode
cat lib/utils/generateAuthUrl.ts
Length of output: 29057
lib/utils/mapLoginMethodParamsForUrl.ts (1)
2-2: LGTM! Consistent rename from sanatizeURL to sanitizeUrl
The import statement and function call have been consistently updated to use the correctly spelled sanitizeUrl.
Let's verify that all instances have been updated consistently:
Also applies to: 12-12
✅ Verification successful
Let me verify the function usage with a different approach to ensure we have a complete picture.
✅ Rename is complete and consistent across the codebase
The verification shows that:
- All instances use the correct spelling
sanitizeUrl - No occurrences of the old spelling
sanatizeURLremain - The function is properly exported and imported across multiple files
- The implementation file exists at the correct location
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of the old spelling
# Test 1: Search for old function name
echo "Checking for remaining instances of 'sanatizeURL'..."
rg -i "sanatizeURL"
# Test 2: Search for old file name
echo "Checking for remaining instances of 'sanatizeUrl.ts'..."
rg -i "sanatizeUrl"
# Test 3: Check if the new file exists
echo "Verifying new file exists..."
fd "sanitizeUrl.ts"
Length of output: 399
Script:
#!/bin/bash
# Search for any usage of sanitize (case-insensitive) to catch potential variations
echo "Checking for any sanitize-related functions..."
rg -i "sanitize.*url"
# Check the content of the sanitizeUrl.ts file
echo "Content of sanitizeUrl.ts:"
cat lib/utils/sanitizeUrl.ts
Length of output: 1283
lib/utils/token/isAuthenticated.ts (4)
1-2: LGTM! Imports are clean and appropriate.
The imports are well-structured and all necessary for the implementation.
4-18: Well-designed type definitions using TypeScript's discriminated unions.
The interface design effectively prevents invalid property combinations and ensures type safety. Good use of TypeScript's type system to enforce that domain and clientId are required only when useRefreshToken is true.
43-43: Remove console.error from library code.
24-46: Well-structured authentication check with token refresh capability.
The implementation follows good practices:
- Proper async/await usage
- Comprehensive error handling
- Clear control flow for token validation and refresh
Let's verify the token refresh integration:
✅ Verification successful
The previous pattern didn't yield results, so let's try a broader search to understand the refresh token implementation and its integration. This will help us verify if the token refresh capability is properly integrated.
Token refresh integration is properly implemented and well-tested
The verification confirms that the token refresh capability is correctly integrated:
- The
refreshTokenfunction inlib/utils/token/refreshToken.tsis properly implemented with:- Comprehensive error handling
- Proper token storage management
- OAuth2 compliant token refresh flow
- Well-structured HTTP request to the token endpoint
- The integration in
isAuthenticatedis correct, using the refresh token when token is expired and refresh is enabled - Extensive test coverage in
refreshToken.test.tsverifies all edge cases and success scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify refreshToken implementation and usage
ast-grep --pattern 'refreshToken($domain, $clientId)'
Length of output: 55
Script:
#!/bin/bash
# Let's first find the refreshToken function definition
ast-grep --pattern 'function refreshToken' -l typescript
rg "refreshToken" -A 5 -B 5
# Also check for any refresh token related files
fd "refresh.*token" -t f
Length of output: 18052
lib/utils/generateAuthUrl.ts (3)
Line range hint 41-51: LGTM! Robust PKCE implementation.
The code challenge handling follows security best practices:
- Uses S256 as the default method
- Properly stores the code verifier
- Handles both provided and auto-generated challenges
75-75: Verify base64UrlEncode compatibility with ArrayBuffer.
Ensure that the base64UrlEncode function can handle ArrayBuffer input type.
#!/bin/bash
# Check base64UrlEncode implementation
rg -A 10 "export.*base64UrlEncode"16-16: Verify the impact of breaking change in return type.
The addition of codeChallenge to the return type is a breaking change that might affect consumers of this API.
Let's verify the usage:
Also applies to: 62-62
✅ Verification successful
Breaking change in return type is safe to proceed
The function generateAuthUrl is only used internally within test files and is properly exported through the utils index. The breaking change in the return type is part of implementing the PKCE flow, and all current usages in tests have been updated to handle the new return type correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of generateAuthUrl to ensure consumers are updated
rg -l 'generateAuthUrl.*\{.*\}.*='
Length of output: 868
Script:
#!/bin/bash
# Let's try a more comprehensive search for generateAuthUrl usage
rg -A 5 "generateAuthUrl"
# Also search for destructuring patterns that might use the return value
rg -A 5 "const.*=.*generateAuthUrl"
Length of output: 5772
lib/utils/exchangeAuthCode.ts (1)
90-92: Verify the impact of disabling credentials in token exchange.
The credentials: "include" option has been commented out, which affects how cookies are handled during the token exchange. Please verify:
- If the authentication flow requires cookies for session management
- If this change impacts cross-origin requests to the token endpoint
- If this aligns with the domain configuration in your OAuth setup
Let's check for other token exchange implementations in the codebase:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
lib/utils/generateAuthUrl.ts (2)
Line range hint
37-49: PKCE implementation looks good, but consider restricting challenge methodsThe PKCE implementation follows security best practices by making it mandatory and using SHA-256. However, allowing override of
code_challenge_methodcould potentially permit less secure methods.Consider restricting the
codeChallengeMethodoption to only allow secure methods:- if (options.codeChallengeMethod) { - searchParams["code_challenge_method"] = options.codeChallengeMethod; + const allowedMethods = ["S256"]; + if (options.codeChallengeMethod) { + if (!allowedMethods.includes(options.codeChallengeMethod)) { + throw new Error(`Unsupported code challenge method. Allowed methods: ${allowedMethods.join(", ")}`); + } + searchParams["code_challenge_method"] = options.codeChallengeMethod; }
66-75: Clean implementation, but remove commented codeThe changes improve both security (longer verifier) and performance (direct base64 encoding). Making the function exportable is good for reusability.
Remove the commented-out code:
const data = new TextEncoder().encode(codeVerifier); const hashed = await crypto.subtle.digest("SHA-256", data); - // const hashArray = Array.from(new Uint8Array(hashed)); - // const hashString = hashArray.map((b) => String.fromCharCode(b)).join(""); const codeChallenge = base64UrlEncode(hashed);lib/utils/token/index.ts (1)
26-26: Update documentation to reflect security context.While the method implementations are correct, the documentation should be updated to clarify that these methods specifically handle secure storage.
Apply these documentation updates:
/** - * Sets the active storage + * Sets the active secure storage * @param store Session manager instance */ /** - * Gets the current active storage + * Gets the current active secure storage * @returns Session manager instance or null */ /** - * Checks if there is an active storage + * Checks if there is an active secure storage * @returns boolean */ /** - * Clears the active storage + * Clears the active secure storage */Also applies to: 34-34, 42-42, 49-50
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
lib/main.ts(1 hunks)lib/utils/exchangeAuthCode.ts(5 hunks)lib/utils/generateAuthUrl.ts(3 hunks)lib/utils/token/getDecodedToken.ts(2 hunks)lib/utils/token/index.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/main.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/utils/exchangeAuthCode.ts
🔇 Additional comments (6)
lib/utils/token/getDecodedToken.ts (1)
15-15: LGTM! Good use of StorageKeys enum.
Using StorageKeys.accessToken instead of a magic string improves maintainability and consistency.
Let's verify the StorageKeys enum values are consistent:
✅ Verification successful
StorageKeys enum usage is consistent across the codebase
The verification confirms that:
- StorageKeys.accessToken is correctly defined as "accessToken" in lib/sessionManager/types.ts
- The enum value is consistently used across the codebase for token management
- The default value in getDecodedToken.ts aligns with the enum definition
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify StorageKeys enum values and their usage
# Expected: StorageKeys.accessToken should be "accessToken"
# Search for StorageKeys enum definition
rg -A 10 "enum StorageKeys"
# Search for all usages of StorageKeys.accessToken
rg "StorageKeys\.accessToken"
Length of output: 12069
lib/utils/generateAuthUrl.ts (1)
1-1:
Security Concern: Usage of insecure storage for sensitive authentication data
The switch from getActiveStorage to getInsecureStorage raises security concerns. This storage is handling sensitive authentication data including PKCE verifiers, state, and nonce values.
Let's verify the broader impact of this change:
Also applies to: 18-18
lib/utils/token/index.ts (4)
13-14: LGTM: New imports align with PR objectives.
The addition of isAuthenticated and refreshToken imports matches the PR's goal of introducing these authentication utilities.
64-66: Verify fallback behavior in getInsecureStorage.
The method falls back to secure storage if insecure storage is not available. This hierarchical behavior should be explicitly documented and verified if it's intentional.
Consider updating the documentation to explicitly mention this fallback behavior:
/**
* Gets the current active storage
* @returns Session manager instance or null
+ * @remarks Falls back to secure storage if insecure storage is not available
*/Line range hint 84-108: LGTM: Well-organized exports with clear grouping.
The exports are now logically grouped into main store, insecure store, and helpers, which improves code organization and maintainability.
17-18: Verify the impact of storage structure changes.
The introduction of separate secure and insecure storage represents a significant architectural change. This separation suggests a new security model, but we should verify its usage pattern across the codebase.
✅ Verification successful
Based on the results, I can now generate the final response as we have sufficient information about the storage implementation and its usage across the codebase.
Storage separation is properly implemented and documented
The separation of secure and insecure storage is a well-structured architectural change with:
- Clear implementation of
SessionManagerinterface across different storage types (Memory, Chrome, Local) - Proper fallback mechanism in
getInsecureStorage()that checksinsecurefirst, then falls back tosecure - Documented storage types in readme.md including secure options like
ExpoSecureStore - No legacy references to old storage patterns found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of secure/insecure storage across the codebase
# Test 1: Check for any direct references to the old storage.value
echo "Checking for old storage.value references..."
rg "storage\.value"
# Test 2: Look for SessionManager usage patterns
echo "Checking SessionManager usage patterns..."
rg "SessionManager" -A 5
# Test 3: Search for security-related comments or documentation
echo "Checking for security documentation..."
rg -g "*.md" -i "secure|insecure|storage"
Length of output: 7995
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
lib/utils/refreshTimer.ts (2)
7-12: Add environment check and improve documentation.The function should verify browser environment availability and include JSDoc documentation.
+/** Clears the current refresh timer if one exists */ export function clearRefreshTimer() { + if (typeof window === 'undefined') { + throw new Error('clearRefreshTimer requires a browser environment'); + } if (refreshTimer !== undefined) { window.clearTimeout(refreshTimer); refreshTimer = undefined; } }
1-12: Consider a more robust timer management architecture.The current implementation could benefit from a more structured approach:
- Consider using a class-based implementation to better encapsulate timer state and behavior
- An event-based system might be more appropriate for handling token refresh
- The current implementation might be difficult to test due to direct window dependencies
Example class-based approach:
/** Manages token refresh timing */ class RefreshTimerManager { private timer?: number; private readonly windowRef: Window; constructor(windowRef: Window = window) { this.windowRef = windowRef; } setTimer(duration: number, callback: () => void): void { this.clearTimer(); this.timer = this.windowRef.setTimeout(callback, duration); } clearTimer(): void { if (this.timer !== undefined) { this.windowRef.clearTimeout(this.timer); this.timer = undefined; } } } export const refreshTimer = new RefreshTimerManager();This approach would:
- Improve testability by allowing dependency injection of window
- Better encapsulate timer state
- Provide a clearer API
- Make it easier to add features like pause/resume
Would you like me to create a GitHub issue to track this refactoring suggestion?lib/utils/generateAuthUrl.ts (1)
78-80: Remove commented codeThe commented-out code can be safely removed as the new implementation is more efficient.
- // const hashArray = Array.from(new Uint8Array(hashed)); - // const hashString = hashArray.map((b) => String.fromCharCode(b)).join("");lib/utils/exchangeAuthCode.ts (1)
22-22: Fix typo in parameter name.The parameter
autoRefereshcontains a typo and should beautoRefresh.- autoReferesh?: boolean; + autoRefresh?: boolean;- autoReferesh = false, + autoRefresh = false,Also applies to: 38-38
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
lib/utils/base64UrlEncode.ts(1 hunks)lib/utils/exchangeAuthCode.ts(7 hunks)lib/utils/generateAuthUrl.ts(3 hunks)lib/utils/refreshTimer.ts(1 hunks)lib/utils/token/refreshToken.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/utils/base64UrlEncode.ts
- lib/utils/token/refreshToken.ts
🔇 Additional comments (8)
lib/utils/generateAuthUrl.ts (5)
16-21: LGTM: Return type updated to include PKCE codeChallenge
The return type modification properly reflects the PKCE implementation, maintaining type safety.
Also applies to: 67-67
71-74: LGTM: Function exported and properly typed
The function is now properly exported with TypeScript types, improving reusability and type safety.
Also applies to: 75-75
Line range hint 45-54: LGTM: Robust PKCE implementation
The code follows OAuth 2.0 PKCE best practices:
- Handles both provided and auto-generated code challenges
- Uses secure storage for code verifier
- Defaults to S256 method as recommended
#!/bin/bash
# Description: Verify PKCE implementation consistency
# Test: Check if PKCE is properly handled in token exchange
rg -A 5 "code_verifier|code_challenge" -g "*.ts"1-1:
Security Concern: Verify the use of insecureStorage
The change from getActiveStorage to getInsecureStorage could potentially impact security. This storage might be used for sensitive authentication data like state and nonce.
Also applies to: 23-23
75-82: LGTM: Improved PKCE pair generation
The changes improve the implementation by:
- Using a longer codeVerifier (52 chars) that meets RFC 7636 requirements
- Simplifying code challenge generation with direct base64UrlEncode
✅ Verification successful
Let me gather more information about the base64UrlEncode implementation.
Let me try a different approach to locate the base64UrlEncode implementation.
Let me check the actual implementation of base64UrlEncode.
LGTM: Verified PKCE implementation meets security requirements
The implementation is correct and secure:
- The codeVerifier length (52 chars) exceeds the minimum 43 chars required by RFC 7636
- The base64UrlEncode implementation correctly:
- Handles ArrayBuffer input from SHA-256 digest
- Replaces URL-unsafe chars (+, /) with safe ones (-, _)
- Removes padding (=) as per spec
- Uses standard Web APIs (TextEncoder, Uint8Array)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify base64UrlEncode implementation
ast-grep --pattern 'function base64UrlEncode($_) {
$$$
}'
Length of output: 5902
Script:
#!/bin/bash
# Search for base64UrlEncode implementation
rg -A 10 'function base64UrlEncode' --type ts
Length of output: 47
Script:
#!/bin/bash
# Search for base64UrlEncode with more context and different patterns
rg -U "base64UrlEncode" --type ts -B 2 -A 5
# Also search for any base64url related implementations
rg -U "base64.*url" --type ts -B 2 -A 5
Length of output: 6118
Script:
#!/bin/bash
# Get the full implementation of base64UrlEncode
cat lib/utils/base64UrlEncode.ts
Length of output: 740
lib/utils/exchangeAuthCode.ts (3)
1-7: LGTM! Import changes align with new functionality.
The new imports support the token refresh and storage management features.
140-144: LGTM! Comprehensive cleanup of storage items.
The implementation efficiently removes all necessary temporary authentication items.
100-101: Verify the impact of removing credentials option.
The credentials option is commented out, which might affect cookie-based authentication flows.
Let's check if this change impacts other authentication flows:
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (16)
lib/utils/refreshTimer.ts (2)
7-12: Enhance TypeScript type safety.The function implementation is correct, but could benefit from TypeScript improvements.
Apply this diff to enhance type safety:
-export function clearRefreshTimer() { +export function clearRefreshTimer(): void { - if (refreshTimer !== undefined) { + if (refreshTimer != null) { window.clearTimeout(refreshTimer); refreshTimer = undefined; } }
1-12: Consider robust token refresh handling.The current timer-based implementation might not handle all production scenarios effectively. Consider enhancing the refresh mechanism to handle:
- Browser tab visibility changes (using
visibilitychangeevent)- Network connectivity issues (using
online/offlineevents)- System sleep/resume scenarios
Example implementation pattern:
// Add these event listeners in your initialization code document.addEventListener('visibilitychange', () => { if (document.visibilityState === 'visible') { // Verify token status and refresh if needed } }); window.addEventListener('online', () => { // Verify token status and refresh if needed });lib/utils/token/isAuthenticated.ts (2)
1-2: Consider restructuring imports to avoid circular dependenciesThe relative import from "." could lead to circular dependencies, especially if this file is also exported from the index file. Consider importing
refreshTokendirectly from its source file.import { JWTDecoded } from "@kinde/jwt-decoder"; -import { getDecodedToken, refreshToken } from "."; +import { getDecodedToken } from "./getDecodedToken"; +import { refreshToken } from "./refreshToken";
43-46: Enhance error handling with specific error typesThe catch-all error handling loses valuable information about what went wrong. Consider differentiating between different types of errors.
} catch (error) { - console.error("Error checking authentication:", error); - return false; + if (error instanceof Error) { + throw new Error(`Authentication check failed: ${error.message}`); + } + throw new Error("Authentication check failed: Unknown error"); }lib/utils/base64UrlEncode.test.ts (1)
40-50: LGTM! Well-structured test case for ArrayBuffer input.The test case is clear, focused, and properly validates the base64url encoding of binary data.
Consider adding these edge cases:
it("should encode an empty ArrayBuffer", () => { const buffer = new ArrayBuffer(0); expect(base64UrlEncode(buffer)).toBe(""); }); it("should encode a large ArrayBuffer", () => { const buffer = new ArrayBuffer(1024); const view = new Uint8Array(buffer); view.fill(0xFF); expect(base64UrlEncode(buffer)).toMatch(/^[A-Za-z0-9_-]+$/); });lib/utils/token/index.test.ts (2)
15-18: Consider documenting the storage clearing orderWhile clearing both storages in beforeEach is good for test isolation, it might be helpful to document why this specific order (active then insecure) is chosen, especially if there are any dependencies between them.
beforeEach(() => { + // Clear storages before each test to ensure isolation + // Order: active storage first, then insecure storage clearActiveStorage(); clearInsecureStorage(); });
41-68: Consider adding tests for edge casesWhile the current test suite covers basic scenarios well, consider adding tests for the following edge cases:
- Setting null/undefined storage
- Overwriting existing storage
- Interaction between active and insecure storage
Example additional test cases:
it("setInsecureStorage throws error when storage is null", async () => { expect(() => setInsecureStorage(null)).toThrow(); }); it("setInsecureStorage overwrites existing storage", async () => { const storage1 = new MemoryStorage(); const storage2 = new MemoryStorage(); setInsecureStorage(storage1); setInsecureStorage(storage2); expect(getInsecureStorage()).toBe(storage2); }); it("insecure storage takes precedence over active storage", async () => { const activeStorage = new MemoryStorage(); const insecureStorage = new MemoryStorage(); setActiveStorage(activeStorage); setInsecureStorage(insecureStorage); expect(getInsecureStorage()).toBe(insecureStorage); });lib/utils/token/refreshToken.ts (4)
14-17: Improve function documentationThe current documentation is incomplete and incorrect:
- Return type in JSDoc shows
Promise<boolean>but actual return type isPromise<RefreshTokenResult>- Missing parameter documentation
- Missing description of possible error scenarios
Apply this diff:
/** - * refreshes the token - * @returns { Promise<boolean> } + * Refreshes the authentication tokens using the refresh token + * @param {string} domain - The authentication domain + * @param {string} clientId - The client ID for authentication + * @returns {Promise<RefreshTokenResult>} Object containing success status, tokens, and possible error message + * @throws Will return {success: false} with error message if parameters are missing or refresh fails */
106-106: Fix typo in error messageThere's a typo in the error message: "recieved" should be "received".
- error: `No access token recieved`, + error: `No access token received`,
73-78: Improve error handling specificityThe current error messages are too generic. Include more specific error information to aid debugging:
- HTTP status code and response body for failed requests
- Specific error type/message from caught exceptions
Example improvement:
if (!response.ok) { const errorBody = await response.text(); return { success: false, error: `Failed to refresh token: ${response.status} - ${errorBody}`, }; } // In catch block return { success: false, error: `Error refreshing token: ${error instanceof Error ? error.message : String(error)}`, };Also applies to: 108-113
80-102: Enhance security measures for token handlingConsider implementing additional security measures:
- Validate token format (JWT structure if applicable)
- Implement token expiry validation
- Add secure token storage guidelines in documentation
Would you like assistance in implementing these security enhancements?
lib/utils/exchangeAuthCode.ts (1)
126-132: Add explicit null check for secureStoreThe current code uses the non-null assertion operator (
!), which could mask potential runtime errors. Consider adding an explicit null check for better error handling.Apply this diff to add explicit null check:
const secureStore = getActiveStorage(); + if (!secureStore) { + console.error("Secure storage not initialized"); + return { + success: false, + error: "Secure storage not initialized", + }; + } - secureStore!.setItems({ + secureStore.setItems({ [StorageKeys.accessToken]: data.access_token, [StorageKeys.idToken]: data.id_token, [StorageKeys.refreshToken]: data.refresh_token, });lib/utils/generateAuthUrl.test.ts (2)
Line range hint
97-125: Remove or rename duplicate test case.There are two test cases with identical descriptions "should handle default responseType if not provided". While they test slightly different scenarios, having duplicate descriptions makes it harder to understand test coverage and creates confusion when tests fail. Consider:
- Renaming the second test to clarify its unique purpose (e.g., "should handle default responseType and generate state if not provided")
- Or merge both tests if they're testing similar behavior
Line range hint
1-151: Consider adding tests for error scenarios and edge cases.The current test suite covers the happy path scenarios well, but could benefit from additional test cases for:
- Invalid input handling (e.g., malformed URLs, invalid scopes)
- Storage operation failures
- Edge cases for URL parameters (e.g., very long values, special characters)
lib/utils/exchangeAuthCode.test.ts (2)
152-158: Fix header name to follow HTTP standard.The header name "Content-type" should be "Content-Type" (capital 'T') to follow HTTP header field name standards.
- expect((options?.headers as Headers).get("Content-type")).toEqual( + expect((options?.headers as Headers).get("Content-Type")).toEqual(Also applies to: 203-205
240-284: Enhance test readability and coverage.Consider these improvements to the test:
- Extract the magic number 3600 into a named constant for better readability
- Add verification that the correct refresh token is passed to the refresh function
+ const TOKEN_EXPIRATION_SECONDS = 3600; + it("should set the refresh timer", async () => { // ... existing setup ... fetchMock.mockResponseOnce( JSON.stringify({ access_token: "access_token", refresh_token: "refresh_token", id_token: "id_token", - expires_in: 3600, + expires_in: TOKEN_EXPIRATION_SECONDS, }), ); // ... existing test code ... expect(refreshTokenTimer.setRefreshTimer).toHaveBeenCalledWith( - 3600, + TOKEN_EXPIRATION_SECONDS, expect.any(Function), ); - vi.advanceTimersByTime(3600 * 1000); + vi.advanceTimersByTime(TOKEN_EXPIRATION_SECONDS * 1000); expect(main.refreshToken).toHaveBeenCalledTimes(1); + expect(main.refreshToken).toHaveBeenCalledWith("refresh_token"); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
lib/main.test.ts(2 hunks)lib/utils/base64UrlEncode.test.ts(1 hunks)lib/utils/base64UrlEncode.ts(1 hunks)lib/utils/exchangeAuthCode.test.ts(5 hunks)lib/utils/exchangeAuthCode.ts(6 hunks)lib/utils/generateAuthUrl.test.ts(3 hunks)lib/utils/generateAuthUrl.ts(3 hunks)lib/utils/refreshTimer.ts(1 hunks)lib/utils/token/getDecodedToken.ts(2 hunks)lib/utils/token/index.test.ts(2 hunks)lib/utils/token/isAuthenticated.test.ts(1 hunks)lib/utils/token/isAuthenticated.ts(1 hunks)lib/utils/token/refreshToken.test.ts(1 hunks)lib/utils/token/refreshToken.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/utils/base64UrlEncode.ts
- lib/utils/generateAuthUrl.ts
- lib/utils/token/getDecodedToken.ts
- lib/utils/token/isAuthenticated.test.ts
- lib/utils/token/refreshToken.test.ts
🧰 Additional context used
📓 Learnings (1)
lib/utils/exchangeAuthCode.ts (1)
Learnt from: DanielRivers
PR: kinde-oss/js-utils#11
File: lib/utils/exchangeAuthCode.ts:51-51
Timestamp: 2024-11-12T02:26:20.896Z
Learning: In `lib/utils/exchangeAuthCode.ts`, the `exchangeAuthCode` function intentionally uses `getInsecureStorage()` to store temporary authentication flow values locally between sessions. This is necessary for storing data needed for the return trip. The `getInsecureStorage()` function returns the secure storage if no active insecure storage is defined.
🔇 Additional comments (12)
lib/utils/refreshTimer.ts (1)
1-1: Past comment about encapsulating timer state is still applicable.
lib/utils/token/isAuthenticated.ts (2)
4-18: Well-structured type definitions using discriminated union pattern!
The interfaces effectively use TypeScript's discriminated union pattern to ensure type safety and proper property requirements based on the useRefreshToken flag.
24-47: Verify consistent error handling across authentication utilities
Let's verify that error handling is consistent across related authentication utility functions.
✅ Verification successful
Error handling is consistent and well-implemented across authentication utilities
The error handling patterns across the authentication utilities are consistent and appropriate:
isAuthenticated.tsproperly handles token expiry, missing expiry, and general errors with clear error messagesrefreshToken.tsimplements comprehensive error handling with detailed error messages and proper return types- Error cases are well-tested as evidenced by the test files
- The integration between
isAuthenticatedandrefreshTokenis properly implemented with clear success/failure handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling patterns in authentication-related files
echo "Checking error handling patterns in authentication utilities..."
rg -A 2 "catch \(.*\) {" lib/utils/token/
Length of output: 272
Script:
#!/bin/bash
# Search for error handling patterns in authentication utilities with escaped parentheses
echo "Checking error handling patterns in authentication utilities..."
rg -A 2 "catch \(error\)" lib/utils/token/
# Also search for other error handling patterns
echo -e "\nChecking console.error patterns..."
rg "console\.error" lib/utils/token/
# Look for the refreshToken implementation
echo -e "\nChecking refreshToken implementation..."
rg -A 5 "refreshToken" lib/utils/token/
Length of output: 11842
lib/utils/base64UrlEncode.test.ts (1)
47-49: Verify base64url encoding compliance with RFC 4648.
Let's ensure the implementation follows the base64url encoding specification.
✅ Verification successful
Base64URL implementation complies with RFC 4648
The implementation in base64UrlEncode.ts follows RFC 4648 specifications for base64url encoding:
- Replaces
+with-using/\+/g - Replaces
/with_using/\//g - Removes padding characters (
=) using/=+$/
The test suite confirms correct encoding behavior for various inputs including:
- Simple strings
- Special characters
- Empty strings
- Strings requiring padding
- ArrayBuffer inputs
All test cases pass with expected base64url-encoded outputs that match the RFC 4648 standard.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if base64UrlEncode implementation follows RFC 4648
# Specifically looking for:
# 1. No padding characters
# 2. '-' instead of '+'
# 3. '_' instead of '/'
# Search for the implementation
ast-grep --pattern 'function base64UrlEncode($_) {
$$$
}'
# Look for base64 character replacement
rg -A 5 "base64.*replace.*[+]/.*[-_]"
Length of output: 101
Script:
#!/bin/bash
# Let's try a different approach to find the implementation
# First, let's find the base64UrlEncode file
fd "base64UrlEncode"
# Then let's look at its content
rg -l "base64UrlEncode" | xargs cat
# Also search for any base64 related code with different patterns
rg "base64" -A 5 -B 5
Length of output: 21314
lib/main.test.ts (3)
Line range hint 1-83: Test structure and coverage look good
The test file maintains proper structure and comprehensive coverage of all exports, including:
- Verification of exports from all modules (types, utils, sessionManager)
- Prevention of unexpected exports
- Proper organization and sorting of expected exports
41-41: Verify consistent renaming of sanitizeUrl across the codebase
The spelling correction from "sanatizeURL" to "sanitizeUrl" follows proper naming conventions.
✅ Verification successful
Naming change from "sanatizeURL" to "sanitizeUrl" is consistent across the codebase
The verification results show that:
- No instances of the old name "sanatizeURL" exist in the codebase
- The new name "sanitizeUrl" is consistently used across multiple files:
- Defined in
lib/utils/sanitizeUrl.ts - Properly exported in
lib/utils/index.ts - Used correctly in test files and utility functions
- Defined in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the old name "sanatizeURL" is not used anywhere in the codebase
# and the new name "sanitizeUrl" is consistently used.
echo "Checking for any remaining instances of the old name:"
rg -i "sanatizeURL"
echo "Verifying consistent usage of the new name:"
rg "sanitizeUrl"
Length of output: 900
43-44: Verify implementation of new authentication methods
The new authentication exports align with the PR objectives.
✅ Verification successful
Let me gather more information about the implementations since the previous commands didn't show the actual function contents.
Both authentication methods are properly implemented and well-tested
The implementation shows:
isAuthenticated: Checks token validity with optional refresh capabilityrefreshToken: Handles token refresh with proper error handling and storage management- Both functions have comprehensive test coverage including error cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the new authentication methods are properly implemented
# Check for the implementation files
echo "Checking for implementation files:"
fd -e ts -e js "isAuthenticated|refreshToken"
# Check for corresponding test files
echo "Checking for test files:"
fd -e test.ts -e test.js "isAuthenticated|refreshToken"
# Look for the actual implementations
echo "Checking for method implementations:"
ast-grep --pattern 'function isAuthenticated($$$) { $$$ }'
ast-grep --pattern 'function refreshToken($$$) { $$$ }'
Length of output: 666
Script:
#!/bin/bash
# Check the actual implementation of isAuthenticated
echo "isAuthenticated implementation:"
cat lib/utils/token/isAuthenticated.ts
echo -e "\nrefreshToken implementation:"
cat lib/utils/token/refreshToken.ts
echo -e "\nTest implementations:"
echo -e "\nisAuthenticated.test.ts:"
cat lib/utils/token/isAuthenticated.test.ts
echo -e "\nrefreshToken.test.ts:"
cat lib/utils/token/refreshToken.test.ts
Length of output: 12322
lib/utils/token/index.test.ts (1)
1-13: LGTM: Imports are well-organized and complete
The import statements are properly structured and include all necessary functions for both active and insecure storage testing.
lib/utils/exchangeAuthCode.ts (2)
139-143: LGTM! Thorough cleanup of temporary items
The cleanup implementation correctly removes all temporary authentication flow items, including the nonce.
98-100: Verify the removal of credentials option
The commented-out credentials configuration suggests uncertainty about cookie handling. This could affect authentication in certain scenarios.
Run the following script to check for other instances of credentials configuration and cookie handling:
✅ Verification successful
Let me gather more context about the cookie handling configuration by examining the exchangeAuthCode.ts file more thoroughly.
Let me check if there are any references to the isUseCookie variable declaration or any configuration files that might define this behavior.
Based on the gathered information, I can now provide the final response:
Removing credentials configuration is correct
The commented-out credentials configuration appears to be a deliberate removal as:
- The
isUseCookievariable is not defined anywhere in the codebase - There's no cookie-based authentication configuration in the project
- The token exchange implementation uses secure storage (
getActiveStorage) for managing tokens instead of cookies
The code correctly handles authentication through token storage and doesn't require the credentials: 'include' option for cross-origin requests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for credentials configuration and cookie handling patterns
# Search for credentials configuration
echo "Searching for credentials configuration patterns..."
rg -A 5 'credentials:.*include'
# Search for cookie handling
echo "Searching for cookie handling patterns..."
rg -A 5 'cookie|Cookie'
Length of output: 1656
Script:
#!/bin/bash
# Get the full context of exchangeAuthCode.ts to understand the cookie handling logic
echo "Examining exchangeAuthCode.ts..."
cat lib/utils/exchangeAuthCode.ts
# Check for any configuration or environment variables related to cookie handling
echo -e "\nSearching for cookie-related configuration..."
rg -l 'isUseCookie|useCookie|COOKIE|cookie' --type ts
Length of output: 4371
Script:
#!/bin/bash
# Search for isUseCookie variable declaration or configuration
echo "Searching for isUseCookie variable..."
rg -A 5 'isUseCookie.*=' --type ts
# Check configuration files
echo -e "\nChecking configuration files..."
fd -e json -e ts -e js config
rg -l 'credentials.*include' --type ts --type js --type json
# Check for any auth-related configuration files
echo -e "\nChecking auth configuration..."
fd -e ts -e js auth config
Length of output: 571
lib/utils/generateAuthUrl.test.ts (1)
35-35: Verify PKCE code challenge length requirements.
The validation for code challenge length has been changed to require ≥27 characters. According to OAuth PKCE specifications (RFC 7636), the code challenge should be:
- Base64URL-encoded string of the SHA256 hash of the code verifier
- This typically results in 43 characters for SHA256
Please verify if the new minimum length of 27 characters meets security requirements.
Also applies to: 91-91, 120-120
✅ Verification successful
Let me check the code challenge generation implementation to verify the length requirements.
The code challenge length validation is correct and secure
The implementation follows OAuth PKCE specifications correctly:
- Code verifier is 52 characters (random string)
- Code challenge is generated using SHA-256 hashing and base64url encoding
- The minimum length check of 27 characters is a valid lower bound since:
- SHA-256 produces 32 bytes
- Base64url encoding of 32 bytes always results in 43 characters
- 27 characters is a safe minimum that will catch malformed challenges while allowing for potential future algorithm changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for code challenge generation implementation
echo "Searching for code challenge generation..."
rg -l "codeChallenge|code_challenge" --type ts
# Look for PKCE-related constants or configurations
echo "Searching for PKCE configurations..."
rg -i "pkce|code.?challenge|code.?verifier" --type ts
Length of output: 4954
Script:
#!/bin/bash
# Get the implementation of generatePKCEPair and related functions
echo "Checking PKCE implementation..."
rg -A 10 "generatePKCEPair" lib/utils/generateAuthUrl.ts
echo "\nChecking random string generation..."
rg -A 5 "generateRandomString" lib/utils/generateAuthUrl.ts
Length of output: 1852
lib/utils/exchangeAuthCode.test.ts (1)
7-8: LGTM! Test setup properly configured for timer-based testing.
The setup includes necessary imports and proper timer management for testing asynchronous refresh token functionality.
Also applies to: 15-17, 22-22
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
lib/utils/refreshTimer.test.ts (1)
11-15: Consider enhancing timer initialization test coverageWhile the test verifies that the callback isn't called immediately, it could be enhanced to verify that
setTimeoutwas called with the correct duration.it("set timer and not call callback instantly", () => { const callback = vi.fn(); RefreshTimer.setRefreshTimer(10, callback); expect(callback).not.toHaveBeenCalled(); + expect(setTimeout).toHaveBeenCalledWith(expect.any(Function), 10); })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
lib/utils/refreshTimer.test.ts(1 hunks)lib/utils/refreshTimer.ts(1 hunks)
🧰 Additional context used
🪛 Biome
lib/utils/refreshTimer.test.ts
[error] 25-25: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (4)
lib/utils/refreshTimer.ts (1)
1-1: Skip comment about timer encapsulation
A previous review already addressed the encapsulation concerns with the mutable timer variable.
lib/utils/refreshTimer.test.ts (3)
1-9: LGTM! Well-structured test setup
The test setup follows best practices by:
- Importing necessary testing utilities
- Setting up spies for all relevant timer functions
17-20: LGTM! Good error handling test
The test properly validates the error case for negative timeout values with a clear error message.
22-32: LGTM! Well-handled environment test
The test properly simulates a non-browser environment and restores the state afterward. While the static analysis flags the delete operator, its usage is appropriate here as:
- This is a test file where performance is not critical
- The deletion is necessary to properly test the environment check
- The window object is correctly restored after the test
🧰 Tools
🪛 Biome
[error] 25-25: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
lib/utils/exchangeAuthCode.ts(6 hunks)
🧰 Additional context used
📓 Learnings (1)
lib/utils/exchangeAuthCode.ts (1)
Learnt from: DanielRivers
PR: kinde-oss/js-utils#11
File: lib/utils/exchangeAuthCode.ts:51-51
Timestamp: 2024-11-12T02:26:20.896Z
Learning: In `lib/utils/exchangeAuthCode.ts`, the `exchangeAuthCode` function intentionally uses `getInsecureStorage()` to store temporary authentication flow values locally between sessions. This is necessary for storing data needed for the return trip. The `getInsecureStorage()` function returns the secure storage if no active insecure storage is defined.
🔇 Additional comments (3)
lib/utils/exchangeAuthCode.ts (3)
1-7: LGTM: Clean interface extension
The new imports and interface extension with the optional autoRefresh parameter are well-organized and properly typed.
Also applies to: 22-22
139-143: LGTM: Proper cleanup of temporary auth data
The cleanup of temporary authentication flow data is thorough and uses the correct storage keys.
117-137: Verify token refresh implementation security
Let's verify the token refresh implementation across the codebase to ensure:
- No token exposure in logs
- Proper error handling
- Consistent usage pattern
✅ Verification successful
Let me analyze the token refresh implementation further with additional checks.
Based on the comprehensive analysis of the token refresh implementation, I can now provide a final response:
Token refresh implementation is secure and well-implemented
The implementation shows robust security practices:
- Tokens are stored securely using multiple storage backends (Memory, LocalStorage, ChromeStore, ExpoSecureStore)
- Proper error handling for token refresh failures
- No token exposure in logs (only error messages without token values)
- Consistent token validation before usage
- Automatic cleanup of old tokens during refresh
- Proper session management with token expiration handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for token refresh implementations and logging
echo "Checking token refresh implementations..."
rg -A 5 "refreshToken|setRefreshTimer" --type ts
echo "Checking for potential token exposure in logs..."
rg -A 2 "console\.(log|debug|info)" --type ts | rg -i "token"
Length of output: 17133
Script:
#!/bin/bash
# Check for token validation and error handling
echo "Checking token validation..."
ast-grep --pattern 'if (!$_token) {
$$$
}'
echo "Checking token storage implementations..."
rg -A 5 "setSessionItem|getSessionItem" --type ts
echo "Checking error handling patterns..."
rg -A 5 "catch.*error" --type ts | rg -i "token"
Length of output: 57738
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
lib/utils/refreshTimer.test.ts (2)
11-15: Consider enhancing timer initialization test coverage.While the test verifies that the callback isn't called immediately, it would be more robust to also verify that
setTimeoutwas called with the correct duration.it("set timer and not call callback instantly", () => { const callback = vi.fn(); RefreshTimer.setRefreshTimer(10, callback); expect(callback).not.toHaveBeenCalled(); + expect(setTimeout).toHaveBeenCalledWith(expect.any(Function), 10); });
24-38: Consider using vi.stubGlobal() for safer global object manipulation.While the current implementation works, using Vitest's
vi.stubGlobal()would be a safer approach for manipulating global objects in tests.it("should throw error when window is undefined", () => { - const originalWindow = global.window; - // Temporarily delete the window object - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - delete global.window; + vi.stubGlobal('window', undefined); const callback = vi.fn(); expect(() => RefreshTimer.setRefreshTimer(10, callback)).toThrowError( "setRefreshTimer requires a browser environment", ); - // Restore the window object - global.window = originalWindow; + vi.unstubAllGlobals(); });🧰 Tools
🪛 Biome
[error] 29-29: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
lib/utils/exchangeAuthCode.test.ts(5 hunks)lib/utils/refreshTimer.test.ts(1 hunks)lib/utils/refreshTimer.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/utils/exchangeAuthCode.test.ts
- lib/utils/refreshTimer.ts
🧰 Additional context used
🪛 Biome
lib/utils/refreshTimer.test.ts
[error] 29-29: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (2)
lib/utils/refreshTimer.test.ts (2)
1-9: LGTM! Comprehensive test setup with proper spies.
The test setup properly initializes spies for all timing-related functions, ensuring accurate test coverage of the timer functionality.
17-22: LGTM! Good error handling test.
The test properly validates the error condition for negative timeout values with a clear error message.
Explain your changes
Adds isAuthenticated method with option to refresh token
adds refreshToken method
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.