fix: codeExchange, storage updates, generateAuthUrl fixes #15
Conversation
…ng store with nonce and state.
WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on enhancing session management and updating the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
lib/utils/generateAuthUrl.ts (1)
33-40: Consider increasing nonce length for enhanced securityWhile 16 characters provide reasonable security, consider increasing the nonce length to 32 characters (like the state parameter) for additional protection against replay attacks.
- options.nonce = generateRandomString(16); + options.nonce = generateRandomString(32);lib/utils/mapLoginMethodParamsForUrl.test.ts (3)
79-79: Consider adding more URL sanitization test casesWhile the test correctly verifies audience defaulting alongside URL sanitization, consider adding separate test cases for URL sanitization edge cases (e.g., multiple trailing slashes, query parameters, fragments).
120-120: Consider testing additional falsy values for audienceWhile the test correctly verifies handling of undefined audience, consider adding test cases for other falsy values (null, empty string, etc.) to ensure consistent behavior.
Example test case:
it("should handle falsy audience values consistently", () => { expect(mapLoginMethodParamsForUrl({ audience: null })).toHaveProperty("audience", ""); expect(mapLoginMethodParamsForUrl({ audience: "" })).toHaveProperty("audience", ""); });
Line range hint
1-124: Consider restructuring tests for better organizationWhile the current tests effectively verify the new audience handling, consider organizing them into nested describe blocks for better structure:
- Basic parameter mapping
- Default value handling
- Parameter sanitization
- Edge cases
This would make it easier to maintain and extend the test suite as new features are added.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- lib/utils/generateAuthUrl.ts (3 hunks)
- lib/utils/mapLoginMethodParamsForUrl.test.ts (5 hunks)
- lib/utils/mapLoginMethodParamsForUrl.ts (1 hunks)
- lib/utils/token/index.ts (1 hunks)
🔇 Additional comments (9)
lib/utils/mapLoginMethodParamsForUrl.ts (2)
14-14: LGTM: Improved audience parameter handlingThe change ensures that the audience parameter always returns a string value, preventing potential undefined values in the resulting object.
Line range hint
1-26: Verify implementation of missing PR objectivesThe PR objectives mention automatic setting of
state,nonce, andcode_challengeparameters, but these changes are not visible in this file. Please ensure these parameters are implemented as described in the PR objectives.Let's verify if these parameters are implemented elsewhere:
lib/utils/generateAuthUrl.ts (4)
Line range hint
1-17: LGTM: Function signature improvementsThe addition of a default value for the
typeparameter and the well-structured return type including security parameters (stateandnonce) enhance the function's usability while maintaining type safety.
Line range hint
24-31: LGTM: Secure state parameter handlingThe implementation follows security best practices by:
- Generating a cryptographically secure state parameter
- Using sufficient length (32 characters) for entropy
- Properly persisting the state for validation
Line range hint
1-55: Verify consistent usage of generateAuthUrl across the codebaseLet's ensure this authentication flow is used consistently and that there are no alternate implementations that might bypass these security improvements.
✅ Verification successful
Let me gather more information to verify if there are any other implementations or usages of auth URL generation.
Based on the results from the shell scripts, I can now generate the final response:
Authentication URL generation is implemented consistently and securely
The codebase shows that:
generateAuthUrlis the single source of truth for OAuth URL generation- All auth URL construction is properly handled through this utility
- The implementation includes all necessary security parameters (state, nonce, PKCE)
- Test coverage confirms the security parameters are properly set
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Look for other auth URL generation implementations rg -A 5 "oauth2/auth|/oauth2/authorize" # Check for direct URL construction without proper state/nonce ast-grep --pattern 'new URL($_, "/oauth2")'Length of output: 3070
Script:
#!/bin/bash # Look for all imports and usages of generateAuthUrl rg -A 2 "generateAuthUrl" # Look for any other URL construction patterns related to auth rg -A 3 "new URL.*auth" # Check for any other files that might be handling authentication URLs fd -e ts -e js -x rg -l "oauth|auth.*url"Length of output: 2763
Line range hint
18-23: Verify required parameters validationWhile the code sets up the basic parameters, it's important to ensure that required parameters (especially
clientId) are validated before use.✅ Verification successful
The previous search might have been too specific. Let's broaden our search to understand how the parameters are handled and validated. We'll look at both the function definition and any type definitions that might enforce parameter validation through TypeScript's type system.
From the output, we can see that
clientIdis already marked as a required parameter in theLoginOptionstype definition (no?optional modifier), which means TypeScript's type system will enforce this validation at compile time. However, let's verify if there's any runtime validation as well.
Parameter validation is properly handled through TypeScript types
The
clientIdparameter is already enforced as required through TypeScript's type system in theLoginOptionsinterface. The implementation correctly uses this typed parameter without need for additional runtime validation since:
- TypeScript will prevent compilation if
clientIdis not provided- The parameter is used directly in URL construction without modification
- The OAuth2 server will reject the request if
clientIdis invalid🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there's any validation for required parameters rg -A 5 "clientId.*undefined|required.*clientId" .Length of output: 52
Script:
#!/bin/bash # Let's try a broader search for parameter validation patterns # First, let's look at the function definition and its usage ast-grep --pattern 'function generateAuthUrl($_) { $$$ }' # Also search for any type definitions or interfaces related to the options rg -A 10 "interface.*Options|type.*Options" lib/Length of output: 526
Script:
#!/bin/bash # Let's look at the full implementation of generateAuthUrl rg -l "generateAuthUrl" lib/ # And then examine its content along with any validation cat lib/utils/generateAuthUrl.tsLength of output: 1846
lib/utils/mapLoginMethodParamsForUrl.test.ts (3)
49-49: LGTM: Test correctly verifies empty audience handlingThe test case properly validates that audience defaults to an empty string when not provided in the options.
64-64: LGTM: Test validates audience defaulting with other defaultsThe test case appropriately verifies that audience defaulting works correctly alongside other default values like scope.
96-96: LGTM: Test verifies audience defaulting with boolean handlingThe test case properly validates that audience defaulting works correctly alongside boolean value conversions.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
lib/utils/generateAuthUrl.test.ts (4)
21-21: LGTM! Consider adding a constant for the expected length.The validation for code_challenge length is correct, but consider extracting the magic number 32 into a named constant for better maintainability.
+ const CODE_CHALLENGE_LENGTH = 32; // ... - expect(codeChallenge!.length).toBe(32); + expect(codeChallenge!.length).toBe(CODE_CHALLENGE_LENGTH);Also applies to: 28-30
54-54: Remove unnecessary empty line.This empty line affects readability and should be removed.
Line range hint
58-81: Rename test case to reflect its unique scenario.This test case has the same name as the next one: "should handle default responseType if not provided". Consider renaming it to better describe its specific scenario, such as "should handle default responseType with state parameter".
Line range hint
1-107: Consider adding negative test cases.The test suite thoroughly covers the positive scenarios, but it would be beneficial to add test cases for:
- Invalid code_challenge values
- Missing required parameters
- Invalid code_challenge_method values
Would you like me to help generate these additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- lib/utils/generateAuthUrl.test.ts (5 hunks)
- lib/utils/mapLoginMethodParamsForUrl.ts (1 hunks)
- lib/utils/token/getDecodedToken.test.ts (1 hunks)
- lib/utils/token/getDecodedToken.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/utils/mapLoginMethodParamsForUrl.ts
🔇 Additional comments (2)
lib/utils/token/getDecodedToken.ts (1)
19-22: LGTM! Robust null-safety check added.The addition of this early return improves the function's robustness by preventing potential null pointer exceptions when accessing storage methods.
Let's verify the usage of
getActiveStorageto ensure consistent null handling:✅ Verification successful
Let me check the implementation of
generateAuthUrlto verify if it needs similar null handling.
Null check in
getDecodedTokenis appropriate, butgenerateAuthUrlhas proper guardsThe null check in
getDecodedTokenis well-placed as it needs to abort early when storage isn't available. In contrast,generateAuthUrlalready has appropriate null guards (if (activeStorage)) around storage operations and can function without storage, only using it optionally to persist state and nonce values. The implementations are consistent with their respective requirements.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if getActiveStorage's null case is handled consistently across the codebase # Expected: All usages of getActiveStorage should check for null before accessing its methods # Search for all usages of getActiveStorage rg -A 5 "getActiveStorage\(\)"Length of output: 755
Script:
#!/bin/bash # Get the full implementation of generateAuthUrl cat lib/utils/generateAuthUrl.tsLength of output: 1718
lib/utils/token/getDecodedToken.test.ts (1)
8-9: Verify API documentation for the behavioral change.The change from throwing an error to returning
nullwhen no active storage is defined is a breaking change in error handling. While this simplification is good, we should ensure this is properly documented.Let's check if this behavioral change is documented:
✅ Verification successful
Let me check the actual implementation and documentation to verify this behavior change.
API documentation is up-to-date with the behavioral change
The JSDoc comment in
getDecodedToken.tscorrectly documents that the function returnsPromise<JWTDecoded | null>, which aligns with the new behavior. The implementation matches the test case, and the return type is properly documented. No further documentation updates are needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Look for API documentation or README updates that might need to be updated # regarding the error handling change in getDecodedToken # Check for potential documentation files fd -e md -e ts --exclude '*.test.ts' -x rg -l "getDecodedToken" # Check for JSDoc comments in the implementation rg -A 5 "export (function |const )?getDecodedToken"Length of output: 745
Script:
#!/bin/bash # Check the implementation of getDecodedToken cat lib/utils/token/getDecodedToken.ts # Check if there are any recent changes in the changelog rg -A 5 "getDecodedToken" CHANGELOG.md # Check readme documentation rg -A 5 "getDecodedToken" readme.mdLength of output: 1484
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
lib/utils/token/index.ts (3)
18-24: Enhance JSDoc documentation for better clarity.The JSDoc could be more descriptive about the function's purpose and impact on the system state.
/** * Sets the active storage * @param store Session manager instance + * @description Updates the global storage instance used across the authentication flow. + * This storage is used for managing session-related data like tokens and state. + * @example + * const sessionManager = new SessionManager(); + * setActiveStorage(sessionManager); */
26-31: LGTM! Consider enhancing JSDoc for null case.The implementation correctly handles the null case and has proper type annotations.
/** * Gets the current active storage * @returns Session manager instance or null + * @returns {SessionManager | null} The active session manager if set, null otherwise + * @see hasActiveStorage for type-safe checking of storage availability */
34-40: LGTM! Consider enhancing JSDoc with type guard usage.The implementation provides a clean type guard for null checking.
/** * Checks if there is an active storage * @returns boolean + * @description Type guard function to check if active storage is available. + * When this returns true, TypeScript knows getActiveStorage() returns SessionManager. + * @example + * if (hasActiveStorage()) { + * const storage = getActiveStorage(); // TypeScript knows this is SessionManager + * } */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- lib/main.test.ts (1 hunks)
- lib/utils/token/index.ts (1 hunks)
🔇 Additional comments (3)
lib/utils/token/index.ts (2)
45-45: LGTM! Export addition is correct.The new
hasActiveStoragefunction is properly exported.
18-45: Verify integration with generateAuthUrl.The storage management changes look good, but let's verify how they're used in generateAuthUrl.
✅ Verification successful
Storage functions are correctly integrated with generateAuthUrl
The verification shows that
getActiveStorageis properly imported and used ingenerateAuthUrl.ts. The function is used to retrieve the active storage instance which is then used to manage session parameters. The integration aligns with the PR's objective of enhancing thegenerateAuthUrlfunctionality.Key findings:
getActiveStorageis imported from "../main" ingenerateAuthUrl.ts- The function is used to obtain the active storage instance:
const activeStorage = getActiveStorage();- The storage instance is used in conjunction with other parameters like
clientIdandresponseTypeto generate the auth URL🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check how the new storage functions are used in generateAuthUrl rg -A 5 "getActiveStorage|hasActiveStorage|setActiveStorage" "lib/utils/generateAuthUrl"Length of output: 154
Script:
#!/bin/bash # First, let's find the correct path for generateAuthUrl fd -t f "generateAuthUrl" # Then, let's search for usage of storage functions in the entire codebase rg -A 5 "getActiveStorage|hasActiveStorage|setActiveStorage"Length of output: 11410
lib/main.test.ts (1)
52-52: LGTM! Verify the hasActiveStorage implementation.The addition of "hasActiveStorage" to the expected exports list is correct and maintains the test coverage for module exports.
Let's verify the implementation exists:
✅ Verification successful
✓ The
hasActiveStoragefunction is properly implemented and exportedThe function is correctly implemented in
lib/utils/token/index.ts:
- Implementation:
const hasActiveStorage = (): boolean => { return storage.value !== null; }- Properly exported in the same file
- Used to check if there's an active storage value
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the hasActiveStorage function implementation exists # Expected: Find the function definition and its usage # Test 1: Look for the function definition echo "Searching for hasActiveStorage function definition..." ast-grep --pattern 'export const hasActiveStorage = $_' # Test 2: Look for any imports of this function echo "Searching for hasActiveStorage imports..." rg "import.*hasActiveStorage.*from"Length of output: 305
Script:
#!/bin/bash # Let's try different approaches to find the implementation # Search for any occurrence of hasActiveStorage in the codebase echo "Searching for hasActiveStorage in all files..." rg "hasActiveStorage" -A 2 # Look for files that might contain the implementation echo "Finding potential implementation files..." fd -t f "storage|session" -e ts -e js --exclude "*.test.*" --exclude "*.d.ts"Length of output: 743
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
lib/utils/token/index.ts (3)
18-24: Enhance function documentation and type safety.The JSDoc could be more descriptive about the function's purpose and impact. Consider adding return type annotation for clarity.
/** * Sets the active storage * @param store Session manager instance + * @description Updates the global storage instance used across the authentication flow + * @returns void */ -const setActiveStorage = (store: SessionManager) => { +const setActiveStorage = (store: SessionManager): void => { storage.value = store; };
26-32: Document breaking change in return type.The function now returns
nullinstead of throwing an error, which is a breaking change. This should be documented clearly./** * Gets the current active storage * @returns Session manager instance or null + * @description Returns the current storage instance if set, or null if no storage is initialized. + * Note: Previously this function threw an error when storage was not initialized. */
34-40: LGTM! Consider enhancing type guard documentation.The implementation correctly addresses type safety concerns raised in previous reviews.
/** * Checks if there is an active storage * @returns boolean + * @description Type guard function to check if storage is initialized. + * Use this before getActiveStorage() for better type safety. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- lib/utils/generateAuthUrl.test.ts (5 hunks)
- lib/utils/generateAuthUrl.ts (4 hunks)
- lib/utils/token/getDecodedToken.ts (1 hunks)
- lib/utils/token/index.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/utils/generateAuthUrl.test.ts
- lib/utils/token/getDecodedToken.ts
🧰 Additional context used
🪛 GitHub Check: main (20.x)
lib/utils/generateAuthUrl.ts
[failure] 47-47:
Property 'codeVerifier' does not exist on type 'typeof StorageKeys'.
🔇 Additional comments (6)
lib/utils/token/index.ts (2)
45-45: LGTM!The export is correctly added and follows the established pattern.
30-32: Verify null handling in dependent code.Let's ensure all consumers properly handle the null case from
getActiveStorage()using the newhasActiveStorage()guard.Also applies to: 38-40
✅ Verification successful
The previous results show that
getActiveStorage()is used in two places, but we need to verify:
- The complete implementation of
generateAuthUrlto understand how it handles null- The context around activeStorage usage to ensure proper null handling
Proper null handling confirmed in all usages
The verification shows that both consumers of
getActiveStorage()handle the null case appropriately:
- In
getDecodedToken.ts, there's an explicit null check:if (!activeStorage) { return null; }
- In
generateAuthUrl.ts, all operations onactiveStorageare guarded with null checks:if (activeStorage) { activeStorage.setSessionItem(...); }The code correctly functions whether storage is available or not, making the
hasActiveStorage()guard optional in these cases.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for direct usage of getActiveStorage() without null checks # or hasActiveStorage() guard # Search for getActiveStorage usage patterns echo "=== Checking getActiveStorage usage patterns ===" rg -A 3 "getActiveStorage\(\)" --type ts # Search for hasActiveStorage usage echo -e "\n=== Checking hasActiveStorage adoption ===" rg "hasActiveStorage\(\)" --type tsLength of output: 810
Script:
#!/bin/bash # Let's check for any null checks on activeStorage variable usage echo "=== Checking null handling patterns for activeStorage ===" rg -B 2 -A 4 "const activeStorage = getActiveStorage\(\)" --type ts # Let's also check the implementation of generateAuthUrl to understand the impact echo -e "\n=== Checking generateAuthUrl implementation ===" cat lib/utils/generateAuthUrl.tsLength of output: 3599
lib/utils/generateAuthUrl.ts (4)
12-16: LGTM: Function signature improvementsThe changes to make the function async and add a default type parameter are well-implemented.
18-18: LGTM: Proper storage initializationGood practice to initialize storage early and handle it as an optional dependency.
38-40: LGTM: Secure nonce implementationThe nonce generation and storage implementation follows security best practices.
44-51: Security concern: PKCE method overrideWhile the PKCE implementation is solid, allowing
codeChallengeMethodoverride could potentially weaken security if a less secure method is chosen.Consider enforcing S256 as the only allowed method:
- searchParams["code_challenge_method"] = "S256"; - - if (options.codeChallengeMethod) { - searchParams["code_challenge_method"] = options.codeChallengeMethod; - } + // S256 is the only secure option for PKCE + searchParams["code_challenge_method"] = "S256";🧰 Tools
🪛 GitHub Check: main (20.x)
[failure] 47-47:
Property 'codeVerifier' does not exist on type 'typeof StorageKeys'.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
lib/utils/token/index.test.ts (1)
1-24: Consider adding error case tests.The current test suite doesn't cover error scenarios. Consider adding tests for:
- Setting invalid storage objects
- Edge cases in storage operations
Would you like me to provide additional test cases for error scenarios?
🧰 Tools
🪛 GitHub Check: main (20.x)
[failure] 1-1:
'beforeEach' is defined but never used
[failure] 1-1:
'vi' is defined but never usedlib/utils/token/index.ts (3)
18-24: Enhance function documentation and type safety.The JSDoc comments could be more descriptive, and the function could benefit from explicit return type annotation.
/** * Sets the active storage * @param store Session manager instance + * @throws {Error} If store is null or undefined + * @returns void */ -const setActiveStorage = (store: SessionManager) => { +const setActiveStorage = (store: SessionManager): void => { + if (!store) { + throw new Error('Session manager instance is required'); + } storage.value = store; };
26-32: Enhance null case documentation.Consider adding more detail about when and why null might be returned.
/** * Gets the current active storage * @returns Session manager instance or null + * @returns {SessionManager} The active session manager instance + * @returns {null} If no active storage has been set */
42-43: Add JSDoc documentation for clearActiveStorage.The function should have documentation explaining its purpose and effects.
+/** + * Clears the active storage instance + * @returns void + */ const clearActiveStorage = (): void => { storage.value = null; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- lib/main.test.ts (1 hunks)
- lib/utils/generateAuthUrl.test.ts (5 hunks)
- lib/utils/generateAuthUrl.ts (4 hunks)
- lib/utils/token/index.test.ts (1 hunks)
- lib/utils/token/index.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/main.test.ts
- lib/utils/generateAuthUrl.test.ts
🧰 Additional context used
📓 Learnings (1)
lib/utils/generateAuthUrl.ts (1)
Learnt from: DanielRivers PR: kinde-oss/js-utils#15 File: lib/utils/generateAuthUrl.ts:27-30 Timestamp: 2024-10-25T23:50:56.599Z Learning: In `lib/utils/generateAuthUrl.ts`, `StorageKeys` is imported from `../main` and includes the `state` key.
🪛 GitHub Check: main (20.x)
lib/utils/token/index.test.ts
[failure] 1-1:
'beforeEach' is defined but never used
[failure] 1-1:
'vi' is defined but never used
🔇 Additional comments (7)
lib/utils/token/index.ts (3)
34-40: LGTM! Well-implemented type guard function.The implementation is clean, type-safe, and well-documented.
49-50: LGTM! Export additions are properly organized.The new functions are correctly exported while maintaining the existing structure.
Line range hint
1-63: Verify integration with generateAuthUrl changes.Let's ensure the new storage management functions are properly integrated with the async changes in generateAuthUrl.
✅ Verification successful
Storage integration with generateAuthUrl is properly implemented
The verification shows that:
generateAuthUrlcorrectly usesgetActiveStorage()for optional storage operations- Storage operations are properly guarded with null checks (
if (activeStorage))- Test coverage includes verification of storage integration
- The function is properly marked as async and handles PKCE generation correctly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify proper usage of storage functions in generateAuthUrl # Check for proper null handling and async/await usage rg -A 5 "getActiveStorage|hasActiveStorage" lib/utils/generateAuthUrl.ts # Verify test coverage rg -A 5 "describe.*storage" lib/utils/**/*.test.tsLength of output: 969
Script:
#!/bin/bash # Let's verify the integration more thoroughly # Check if generateAuthUrl is async ast-grep --pattern 'export async function generateAuthUrl' # Check for any test files related to generateAuthUrl fd -e test.ts generateAuthUrl # Look for any error handling related to storage in generateAuthUrl rg -B 3 -A 3 "throw.*storage" lib/utils/generateAuthUrl.ts # Check for any storage-related tests rg -l "describe.*('|\")(Active)?Storage" lib/utils/token/Length of output: 249
Script:
#!/bin/bash # Let's examine the actual implementation and tests # Check the implementation of generateAuthUrl cat lib/utils/generateAuthUrl.ts # Check the test file cat lib/utils/generateAuthUrl.test.ts # Look for any storage-related error handling in the token utils rg "if.*!.*storage|throw.*storage" lib/utils/token/Length of output: 8580
lib/utils/generateAuthUrl.ts (4)
1-1: LGTM: Function signature improvementsThe changes to make the function async and add a default type parameter are good improvements. The new imports support the storage functionality.
Also applies to: 12-16
27-30: LGTM: Secure state parameter handlingThe implementation correctly generates and stores the state parameter with sufficient entropy (32 bytes) and proper storage handling.
38-40: LGTM: Secure nonce parameter handlingThe implementation correctly generates and stores the nonce with sufficient entropy (16 bytes).
44-51: LGTM: Secure PKCE configurationThe implementation correctly:
- Stores the code verifier securely
- Uses the recommended S256 challenge method by default
- Allows method override if needed
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (3)
lib/utils/generateAuthUrl.ts (1)
51-54: Consider validating code_challenge_method valuesWhile the default "S256" is correct, consider validating custom codeChallengeMethod values to ensure they match PKCE specifications (only "plain" or "S256" are valid).
searchParams["code_challenge_method"] = "S256"; if (options.codeChallengeMethod) { + if (!["plain", "S256"].includes(options.codeChallengeMethod)) { + throw new Error('code_challenge_method must be "plain" or "S256"'); + } searchParams["code_challenge_method"] = options.codeChallengeMethod; }lib/sessionManager/stores/expressSession.ts (1)
1-13: Consider renaming the file to match the class name.The file is named
expressSession.tsbut contains aMemoryStorageclass implementation. This could be misleading as it suggests Express.js session integration rather than in-memory storage.Consider renaming the file to
memoryStorage.tsto better reflect its contents.lib/utils/exchangeAuthCode.ts (1)
33-33: Remove unnecessary call togetSessionItemThe call to
activeStorage.getSessionItem(StorageKeys.state);on line 33 does not assign its result to any variable and appears to have no effect. Since you retrievestoredStatein line 42, you can remove this line to clean up the code.Apply this diff to remove the unnecessary call:
- activeStorage.getSessionItem(StorageKeys.state);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- lib/main.test.ts (2 hunks)
- lib/sessionManager/stores/expressSession.ts (1 hunks)
- lib/sessionManager/types.ts (3 hunks)
- lib/utils/exchangeAuthCode.test.ts (1 hunks)
- lib/utils/exchangeAuthCode.ts (1 hunks)
- lib/utils/generateAuthUrl.ts (4 hunks)
- lib/utils/index.ts (2 hunks)
- lib/utils/token/index.test.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/main.test.ts
- lib/sessionManager/types.ts
- lib/utils/token/index.test.ts
🧰 Additional context used
📓 Learnings (2)
lib/sessionManager/stores/expressSession.ts (1)
Learnt from: DanielRivers PR: kinde-oss/js-utils#16 File: lib/sessionManager/types.ts:75-80 Timestamp: 2024-10-25T14:24:05.260Z Learning: All storage classes (`MemoryStorage`, `LocalStorage`, `ChromeStore`, `ExpoSecureStore`) extend `SessionBase`, inheriting the `setItems` method, so they do not need to implement `setItems` explicitly.lib/utils/generateAuthUrl.ts (1)
Learnt from: DanielRivers PR: kinde-oss/js-utils#15 File: lib/utils/generateAuthUrl.ts:27-30 Timestamp: 2024-10-25T23:50:56.599Z Learning: In `lib/utils/generateAuthUrl.ts`, `StorageKeys` is imported from `../main` and includes the `state` key.
🪛 Biome
lib/utils/exchangeAuthCode.test.ts
[error] 9-9: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
[error] 18-18: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
[error] 35-35: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
[error] 52-52: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
[error] 65-65: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
🔇 Additional comments (9)
lib/utils/index.ts (1)
8-8: LGTM! Clean integration of the new auth utility.The addition of
exchangeAuthCodefollows the established patterns for imports and exports, maintaining consistency with the existing codebase structure.Also applies to: 17-17
lib/utils/generateAuthUrl.ts (4)
1-1: LGTM: Function signature and import changes are well-structuredThe changes to make the function async and add storage-related imports align well with the new PKCE and storage functionality. The default type parameter is a nice usability improvement.
Also applies to: 12-16
27-30: LGTM: Secure state parameter handlingThe state parameter implementation follows security best practices:
- Uses cryptographically secure random string
- Safely stores in session storage when available
- Maintains existing state if provided
38-40: LGTM: Secure nonce parameter handlingThe nonce implementation follows the same secure pattern as state handling, with appropriate length and storage management.
44-49: LGTM: PKCE implementation follows RFC 7636 specificationsThe PKCE implementation is now correct and secure:
- Code verifier length meets RFC 7636 requirements (43-128 chars)
- SHA-256 hashing is implemented properly
- Base64URL encoding is handled correctly
- Code verifier is securely stored
Also applies to: 65-76
lib/sessionManager/stores/expressSession.ts (1)
1-88: Verify integration with authentication flow.The
MemoryStorageimplementation looks solid and supports the PR objectives. However, we should verify its integration with the authentication flow.✅ Verification successful
Let me gather more information about the integration by checking the actual usage patterns.
Integration with authentication flow verified successfully
The implementation is properly integrated within the authentication flow:
- State and nonce are correctly stored in
generateAuthUrl.ts- Values are retrieved and validated in
exchangeAuthCode.ts- Test coverage confirms the storage functionality for authentication data
- The implementation follows the security best practices by storing and validating state/nonce
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of MemoryStorage in the authentication flow # Test 1: Check if MemoryStorage is used in the authentication flow echo "Checking MemoryStorage usage in authentication..." rg -l "MemoryStorage" --type ts # Test 2: Check for state and nonce storage patterns echo "Checking state and nonce storage patterns..." rg "setSessionItem.*(?:state|nonce)" --type tsLength of output: 1062
Script:
#!/bin/bash # Check the implementation of generateAuthUrl.ts to verify proper integration echo "Checking generateAuthUrl.ts implementation..." rg -A 10 "setSessionItem.*(?:state|nonce)" lib/utils/generateAuthUrl.ts # Check test coverage for state/nonce storage echo "Checking test coverage..." rg "describe.*MemoryStorage" -A 10 "lib/sessionManager/stores/memory.test.ts" # Verify storage retrieval in auth flow echo "Checking state/nonce retrieval..." rg "getSessionItem.*(?:state|nonce)" --type tsLength of output: 2540
lib/utils/exchangeAuthCode.test.ts (1)
1-8: LGTM! Test setup and imports are well structured.The necessary testing utilities are properly imported and fetch mock is correctly configured.
lib/utils/exchangeAuthCode.ts (2)
86-90: Uncomment and implement URL parameter clearing if intendedThe code for clearing URL parameters after successful authentication is currently commented out (lines 86-90). If you intend to remove the authentication parameters from the URL for security and cleanliness, consider uncommenting and testing this code.
Before making this change, ensure that removing the URL parameters does not interfere with other functionalities.
91-96: Ensure sensitive tokens are handled securelyWhen returning the tokens in the response (lines 91-96), be cautious about exposing sensitive information. If this object is returned to a calling function that may expose it to the client or logs, consider omitting the tokens or providing them in a secure manner.
[security]
Consider returning a success status without including the tokens, if they are already stored securely.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
lib/utils/index.ts (1)
8-21: Consider documenting the OAuth flow architecture.
The addition of exchangeAuthCode alongside the existing generateAuthUrl suggests implementation of a complete OAuth flow. Consider adding documentation (e.g., in README.md) that explains:
- The full OAuth flow implementation
- The relationship between these utilities
- Usage examples for common scenarios
This would help users understand how to properly chain these utilities together.
lib/utils/exchangeAuthCode.ts (1)
22-28: Enhance error message specificity.
The current error message doesn't specify which parameter is missing. This could make debugging harder.
- console.error("Invalid state or code");
+ console.error(`Missing required parameters: ${!state ? 'state' : ''} ${!code ? 'code' : ''}`.trim());
return {
success: false,
- error: "Invalid state or code",
+ error: `Missing required parameters: ${!state ? 'state' : ''} ${!code ? 'code' : ''}`.trim(),
};📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
package.jsonis excluded by!**/*.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/*.yaml
📒 Files selected for processing (4)
- lib/main.test.ts (3 hunks)
- lib/utils/exchangeAuthCode.test.ts (1 hunks)
- lib/utils/exchangeAuthCode.ts (1 hunks)
- lib/utils/index.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/main.test.ts
- lib/utils/exchangeAuthCode.test.ts
🔇 Additional comments (6)
lib/utils/index.ts (2)
11-21: LGTM! Clean organization of exports.
The exports are well-organized with clear categorization between config and utils sections. The new additions maintain consistent formatting with the existing code.
8-8: Verify the exchangeAuthCode module exists.
The import looks correct, but let's verify the module exists and contains these exports.
✅ Verification successful
Import statement is correctly referencing existing exports
The exchangeAuthCode.ts module exists and properly exports both exchangeAuthCode function and frameworkSettings object, matching exactly what's being imported in the barrel file. The import statement is valid and well-structured.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and exports of the exchangeAuthCode module
# Expected: Find the module and confirm it exports both entities
# Check if the file exists and contains the exports
rg -l "export.*(?:exchangeAuthCode|frameworkSettings)" --type ts
Length of output: 96
Script:
#!/bin/bash
# Let's verify the actual exports in the exchangeAuthCode.ts file
rg "export.*(?:exchangeAuthCode|frameworkSettings)" --type ts -A 2
Length of output: 440
lib/utils/exchangeAuthCode.ts (4)
3-6: Framework settings issue already identified.
A previous review comment has already identified the issue with empty framework settings. The concern remains valid.
43-50: LGTM! Good state validation.
The state validation is thorough and provides helpful error messages for debugging.
64-76: Review commented code and error handling.
- The commented code about cookie usage (
isUseCookie) suggests incomplete feature implementation. - Error handling for the fetch request is missing, as noted in a previous review.
Let's check for other instances of this commented feature flag:
#!/bin/bash
# Search for other instances of isUseCookie
rg "isUseCookie"84-90: Verify secure storage implementation.
The code handles sensitive data (tokens, state). Ensure that the storage implementation (activeStorage) uses secure storage mechanisms appropriate for the runtime environment.
Let's examine the storage implementation:
#!/bin/bash
# Look for storage implementation details
rg -t typescript "class.*implements.*Storage" -A 10
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
package.jsonis excluded by!**/*.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/*.yaml
📒 Files selected for processing (4)
- lib/utils/exchangeAuthCode.test.ts (1 hunks)
- lib/utils/exchangeAuthCode.ts (1 hunks)
- lib/utils/index.ts (1 hunks)
- vite.config.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/utils/exchangeAuthCode.test.ts
- lib/utils/index.ts
🔇 Additional comments (3)
vite.config.ts (2)
1-1: LGTM! Good choice using vitest/config.
The change to import defineConfig from "vitest/config" is appropriate as it provides better integration with Vitest testing framework while maintaining all Vite configuration capabilities.
7-9: LGTM! Appropriate test environment configuration.
Setting up jsdom as the test environment is crucial for testing browser-based functionality, which is necessary for URL generation and authentication flows. This configuration will ensure that the tests run in a simulated browser environment with access to DOM APIs.
lib/utils/exchangeAuthCode.ts (1)
11-24: Well-structured type definitions!
The interfaces provide clear type safety and good documentation of the expected parameters and return values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
lib/sessionManager/utils.ts (1)
Line range hint
1-6: LGTM! Excellent refactor to use regex.The new implementation is more elegant and potentially more performant. It:
- Maintains the same functionality while being more concise
- Properly handles edge cases
- Uses a safe regex pattern construction
Consider adding JSDoc comments to document the function's purpose and parameters, especially since it's exported:
/** * Splits a string into an array of substrings of specified length * @param str - The input string to split * @param length - The maximum length of each substring * @returns An array of substrings, each with maximum length specified */ export function splitString(str: string, length: number): string[] {
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
lib/sessionManager/utils.test.ts(0 hunks)lib/utils/exchangeAuthCode.ts(1 hunks)
💤 Files with no reviewable changes (1)
- lib/sessionManager/utils.test.ts
🔇 Additional comments (2)
lib/utils/exchangeAuthCode.ts (2)
11-24: LGTM! Well-structured TypeScript interfaces.
The interfaces are well-defined with proper typing for parameters and results, making the code type-safe and self-documenting.
105-109: LGTM! URL cleanup implementation.
The URL cleanup code effectively removes sensitive parameters from the browser's URL bar and history.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
lib/utils/exchangeAuthCode.ts (1)
18-24: Enhance type safety for token propertiesThe interface uses computed property names which could lead to runtime issues if StorageKeys enum values change. Consider using explicit string literals for better type safety.
interface ExchangeAuthCodeResult { success: boolean; error?: string; - [StorageKeys.accessToken]?: string; - [StorageKeys.idToken]?: string; - [StorageKeys.refreshToken]?: string; + accessToken?: string; + idToken?: string; + refreshToken?: string; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
lib/utils/exchangeAuthCode.test.ts(1 hunks)lib/utils/exchangeAuthCode.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/utils/exchangeAuthCode.test.ts
DanielRivers
changed the title
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (1)
lib/utils/exchangeAuthCode.test.ts (1)
93-146: Enhance assertions in successful token exchange test.While the test covers the basic functionality, consider adding these assertions:
- Verify the request body contains expected parameters
- Validate the code verifier is included in the request
Add these assertions after line 145:
// Verify request body parameters const body = new URLSearchParams(options.body as string); expect(body.get('grant_type')).toBe('authorization_code'); expect(body.get('code')).toBe(input); expect(body.get('client_id')).toBe('test'); expect(body.get('redirect_uri')).toBe('http://test.kinde.com');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
lib/sessionManager/utils.ts(1 hunks)lib/utils/exchangeAuthCode.test.ts(1 hunks)lib/utils/exchangeAuthCode.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/sessionManager/utils.ts
🧰 Additional context used
🪛 GitHub Check: main (20.x)
lib/utils/exchangeAuthCode.test.ts
[failure] 65-65: Unhandled error
AssertionError: promise resolved "{ success: false, …(1) }" instead of rejecting
- Expected
- Received
- [Error: rejected promise]
- Object {
- "error": "Authentication storage is not initialized",
- "success": false,
- }
❯ Assertion.VITEST_REJECTS ../node_modules/.pnpm/@vitest[email protected]/node_modules/@vitest/expect/dist/index.js:1802:21
❯ Assertion.propertyGetter ../node_modules/.pnpm/[email protected]/node_modules/chai/chai.js:1466:29
❯ Object.proxyGetter [as get] ../node_modules/.pnpm/[email protected]/node_modules/chai/chai.js:1550:22
❯ utils/exchangeAuthCode.test.ts:65:5
❯ ../node_modules/.pnpm/@vitest[email protected]/node_modules/@vitest/runner/dist/index.js:146:14
❯ ../node_modules/.pnpm/@vitest[email protected]/node_modules/@vitest/runner/dist/index.js:61:7
❯ runTest ../node_modules/.pnpm/@vitest[email protected]/node_modules/@vitest/runner/dist/index.js:960:17
❯ processTicksAndRejections ../node:internal/process/task_queues:95:5
❯ runSuite ../node_modules/.pnpm/@vitest[email protected]/node_modules/@vitest/runner/dist/index.js:1116:15
This error originated in "utils/exchangeAuthCode.test.ts" test file. It doesn't mean the error was thrown inside the file itself, but while it was running.
The latest test that might've caused the error is "utils/exchangeAuthCode.test.ts". It might mean one of the following:
- The error was thrown, while Vitest was running this test.
- If the error occurred after the test had been completed, this was the last documented test before it was thrown.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15 +/- ##
==========================================
+ Coverage 79.58% 83.13% +3.55%
==========================================
Files 28 29 +1
Lines 622 753 +131
Branches 97 122 +25
==========================================
+ Hits 495 626 +131
Misses 127 127
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Explain your changes
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.