feat(service-bff): Integrate encryption for session cookies #17277
Conversation
Implements the BffDoubleSessionModal to handle cases where a user has multiple active sessions. This modal provides options to switch users or keep the current session. Enhances error handling in the AuthService by replacing generic error messages with more specific ones and adding a new method to manage missing login attempts, improving user experience during login failures.
Update the BffError class to use a number type for the statusCode instead of a string. Modify the constructor and related methods to ensure consistent handling of status codes as numbers. This change improves type safety and prevents potential issues with string-to-number conversions.
Refactor error handling introducing a new enum for BFF error codes. Update error messages and status codes for clarity. Enhance user session management by ensuring proper handling of multiple active sessions. Implement a method to remove BFF error parameters from the URL to improve user experience. Adjust button click handlers to use arrow functions for consistency.
Enhance the cache retrieval logic in the AuthController test to improve readability. Update the documentation in AuthService to provide clearer explanations of scenarios where a login attempt cache entry may not be found. This includes specifying the conditions for cache expiration and the recovery process for login attempts, ensuring better understanding for future developers.
Remove unnecessary console logs during login redirection and directly set the window location for a cleaner flow. Enhance the button wrapper styles for better responsiveness and alignment in the BffDoubleSession component.
Adds encryption for session IDs in the authentication flow to enhance security. Updates tests to reflect changes in ID handling, during that encrypted session are used consistently across controllers and services. Introduces `CryptoService` and `SessionCookieService` to manage encryption and session cookie operations.
WalkthroughThis pull request introduces a comprehensive refactoring of session management across multiple services and modules in the BFF (Backend for Frontend) application. The primary focus is on enhancing session security by implementing a new Changes
Suggested labels
Suggested reviewers
Possibly related PRs
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
Documentation and Community
|
Removes unnecessary cache checks for existing sessions in the authentication process. The code now directly returns a conflict response when a session ID is present, streamlining the logic. Additionally, it eliminates the `loginAttemptData` from the `updateTokenCache` method, as it is no longer needed, reducing complexity and improving maintainability.
Refactor the `onRetry` function to use `useCallback` for better performance and prevent unnecessary re-renders. Introduce a new `onKeepCurrentUser` function that removes BFF error parameters from the URL and calls `onRetry`.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## bff-double-session #17277 +/- ##
======================================================
+ Coverage 35.61% 35.73% +0.12%
======================================================
Files 6931 6941 +10
Lines 149294 148337 -957
Branches 42744 42337 -407
======================================================
- Hits 53164 53007 -157
+ Misses 96130 95330 -800 Flags with carried forward coverage won't be shown. Click here to find out more. see 180 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 10 Total Test Services: 0 Failed, 10 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (14)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts (1)
Line range hint
42-255: Consider adding timeout configuration test.The test suite is well-structured and provides good coverage. Consider adding a test case that verifies the timeout configuration for token refresh retries to ensure the system behaves correctly under different timeout settings.
Example test case:
it('should respect configured timeout value for refresh polling', async () => { // Arrange const customTimeout = 1000; await cacheService.save({ key: refreshInProgressKey, value: true, ttl: 3000, }); // Act const result = await service.refreshToken({ sid: testSid, encryptedRefreshToken: testRefreshToken, pollingTimeout: customTimeout, }); // Assert expect(mockLogger.warn).toHaveBeenCalled(); expect(idsService.refreshToken).toHaveBeenCalledWith(testRefreshToken); expect(result).toEqual(mockTokenResponse); }, customTimeout + 500); // Add buffer to prevent test timeout
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
apps/services/bff/src/app/modules/auth/auth.controller.spec.ts(11 hunks)apps/services/bff/src/app/modules/auth/auth.module.ts(1 hunks)apps/services/bff/src/app/modules/auth/auth.service.ts(7 hunks)apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts(1 hunks)apps/services/bff/src/app/modules/auth/token-refresh.service.ts(2 hunks)apps/services/bff/src/app/modules/cache/cache.service.ts(4 hunks)apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts(10 hunks)apps/services/bff/src/app/modules/proxy/proxy.module.ts(2 hunks)apps/services/bff/src/app/modules/proxy/proxy.service.ts(3 hunks)apps/services/bff/src/app/modules/user/user.controller.spec.ts(12 hunks)apps/services/bff/src/app/modules/user/user.module.ts(2 hunks)apps/services/bff/src/app/modules/user/user.service.ts(3 hunks)apps/services/bff/src/app/services/crypto.service.spec.ts(2 hunks)apps/services/bff/src/app/services/crypto.service.ts(3 hunks)apps/services/bff/src/app/services/error.service.ts(3 hunks)apps/services/bff/src/app/services/sessionCookie.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (16)
apps/services/bff/src/app/modules/proxy/proxy.module.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/user/user.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/auth/token-refresh.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/services/error.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/auth/auth.controller.spec.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/auth/auth.module.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/user/user.module.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/auth/auth.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/services/sessionCookie.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/user/user.controller.spec.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/proxy/proxy.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/services/crypto.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/services/crypto.service.spec.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/cache/cache.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (90)
apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts (1)
181-181: Security improvement: Removed session ID from log message.The simplification of the warning message aligns with the PR's security objectives by preventing sensitive session IDs from appearing in logs.
Let's verify that this logging pattern is consistently applied across other test files:
✅ Verification successful
Let me check the context of those remaining occurrences to ensure they're not related to logging.
Session ID logging has been consistently removed from test assertions
The found occurrences of "sid" in
auth.controller.spec.tsare not related to logging messages - they are used in test setup for query parameters and test case descriptions. The original change to remove session IDs from log messages appears to be consistently applied across the test files.
- The occurrence in
auth.controller.spec.tsline withquery({ sid: SID_VALUE })is for API request setup- The other occurrence is just a test description for an invalid token scenario
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining session ID logging in test files # Look for patterns that might log session IDs in warning/error messages rg -g "*.spec.ts" "warn.*sid|error.*sid|log.*sid" apps/services/bff/src/Length of output: 366
Script:
#!/bin/bash # Get context around the matches to verify if they're related to logging rg -g "*.spec.ts" -B 2 -A 2 "warn.*sid|error.*sid|log.*sid" apps/services/bff/src/Length of output: 1045
apps/services/bff/src/app/modules/auth/auth.module.ts (2)
2-4: Good practice: Import required services for modular design.Including
CryptoService,PKCEService, andSessionCookieServicedemonstrates adherence to NestJS's modular structure by injecting only what's needed. This separation of services helps ensure each one is testable, maintainable, and easily replaced if necessary.
11-17: Properly structured providers array.Each service is clearly listed, which aligns with NestJS best practices. This fosters clear visibility and a straightforward dependency injection setup, making the AuthModule self-contained and easy to integrate into the wider application.
apps/services/bff/src/app/modules/user/user.module.ts (2)
4-4: Centralizing session handling with SessionCookieService.Injecting
SessionCookieServicein theUserModuleto handle session cookies follows NestJS's principle of modularization. Ensure you have relevant tests for session management in your user flows to validate session retrieval and clearing logic.
20-20: Confirm usage coverage.While adding
SessionCookieServicehere is beneficial, verify that all necessary endpoints within this module properly use it for session management. This promotes consistency across the user-related features.apps/services/bff/src/app/modules/proxy/proxy.module.ts (2)
4-4: Consistent cookie encryption approach.Importing
SessionCookieServicein theProxyModuleensures consistent encryption and decryption logic throughout all proxy-related flows. This centralization helps avoid duplicate code and potential security oversights.
20-20: Ensure thorough testing of proxy path flows.Verify that the proxy routes using
SessionCookieServicefor cookie handling have adequate test coverage, particularly for encryption edge cases and invalid cookie scenarios.apps/services/bff/src/app/services/sessionCookie.service.ts (11)
1-2: Consistent logging provider import.Using
@island.is/loggingaligns with the shared logging standards. Verify that the appropriate log levels are employed for different error and info scenarios, reducing noise and ensuring meaningful log entries.
3-3: Clean dependency injection with NestJS.Annotating with
@InjectforLOGGER_PROVIDERis consistent with NestJS patterns, ensuring the logger instance is properly resolved at runtime.
4-4: Request and Response typing.Importing
RequestandResponsefromexpressis valid. Ensure that any usage remains consistent with NestJS's@Reqand@Resdecorators, if applicable, and that the application still benefits from NestJS's abstraction layers.
5-5: Use of central constants for cookie name.Referencing a shared
SESSION_COOKIE_NAMEhelps maintain clarity and global consistency across the codebase. Keep an eye out for potential duplication of cookie constants in other modules.
6-6: Reusability of shared cookie options.
getCookieOptions()demonstrates DRY usage for cookie settings. Ensure it’s flexible enough (i.e., sameSite config, secure flags) to handle production and development differences.
7-7: Encryption service injection.Dependency injecting
CryptoServiceensures the session cookie encryption steps remain consistent with the rest of the application’s cryptographic operations. Verify that the same encryption approach is used uniformly for any sensitive data.
9-10: Follows NestJS service architecture.
@Injectable()and a constructor-based injection pattern reflect standard NestJS practices. This approach simplifies testing by allowing theSessionCookieServiceto be easily mocked or replaced for test environments.
11-15: Logger-based error handling.Injecting
loggerensures errors get captured consistently. It's good practice to monitor error rates in production to detect any potential decryption or encryption anomalies.
17-32: Comprehensive error handling in set().By catching the encryption process error, logging, and rethrowing, you preserve the call stack for debugging while ensuring the application can handle failures gracefully.
34-55: Fail-safe design in get().Returning
undefinedupon decryption failures gracefully handles corrupted or tampered cookies. Logging the error while not crashing the request flow can help reduce disruption in user sessions.
56-62: Appropriate cookie clearing in clear().Clearing the session cookie with the same options ensures consistent domain and path scoping. Confirm that the relevant tests cover user logouts, timeouts, and manual session clearing logic.
apps/services/bff/src/app/modules/user/user.service.ts (3)
7-7: Good import usage
Properly importing theSessionCookieServicefosters cleaner code organization and better maintainability.
19-19: Constructor DI looks good
InjectingSessionCookieServicevia constructor aligns with NestJS best practices for dependency injection.
44-44: Appropriate session retrieval
Retrieving the session ID viathis.sessionCookieService.get(req)centralizes session logic, enhancing security and readability.apps/services/bff/src/app/modules/cache/cache.service.ts (5)
10-10: Centralizing separator usage
Storing the separator in a private property makes the code more maintainable and prevents hardcoded string repetition.
31-37: Clear logic for removing session ID
ThekeyWithoutSidmethod effectively sanitizes cache keys to avoid leaking session IDs in logs. This is a valuable security measure.
40-40: Good design for error messages
Usingthis.keyWithoutSid(key)increateKeyErrorhelps ensure session IDs are not exposed in error logs.
54-54: Consistent use of separator
Referencingthis.separatorhere ensures consistency across the codebase when generating session keys.
92-94: Secure deletion error handling
Wrapping the cache deletion in a try-catch and usingthis.keyWithoutSidin the error message further mitigates session ID leaks.apps/services/bff/src/app/services/error.service.ts (3)
6-6: Single import for session cookie operations
ImportingSessionCookieServicereduces duplicated cookie-handling code across multiple services.
36-36: DI for central cookie management
InjectingSessionCookieServiceensures consistent and secure operations on session cookies, aligning with NestJS patterns.
82-82: Secure session cleanup
Clearing the session cookie viathis.sessionCookieService.clear(res)maintains a single source of truth for cookie management.apps/services/bff/src/app/services/crypto.service.spec.ts (5)
6-6: Relevant import
ImportingCryptoServiceis aligned with testing best practices, ensuring the tested class is clearly referenced.
92-93: New test suite
Defining a dedicatedskipAlgorithmPrefixsuite improves clarity and separation of concerns in the test file.
96-109: Default behavior test
Verifies that the encryption includes the algorithm prefix under normal circumstances. This is essential for backward compatibility.
111-124: Prefix skipping test
Ensures the encryption can omit the algorithm prefix, increasing flexibility for environments that need a more compact format.
126-141: Error throwing with mismatched prefix
Validating that mismatched prefix usage properly fails guards against incorrect usage, emphasizing robust error handling.apps/services/bff/src/app/services/crypto.service.ts (9)
35-35: Documentation parameter addition looks good.
TheskipAlgorithmPrefixparameter is clearly documented, and its purpose is straightforward.
41-44: Informative usage examples.
Including multiple examples for theencryptmethod clarifies howskipAlgorithmPrefixbehaves in different use cases.
46-46: Method signature updates are consistent.
The addition ofskipAlgorithmPrefixaligns well with the examples and helps retain a flexible encryption format.
61-66: Conditional logic checks out.
The logic to conditionally prepend the algorithm prefix is implemented correctly and keeps the encryption output flexible.
69-69: Maintaining backward compatibility.
Returning the output in the old format ifskipAlgorithmPrefixis false ensures existing consumers are unaffected.
81-81: Decryption parameter documentation is consistent.
The docstring forskipAlgorithmPrefixmirrors the encryption usage, ensuring clarity.
87-90: Examples for decryption method are well explained.
Providing both prefix and non-prefix scenarios is very helpful for developers.
92-92: Method signature alignment.
IncludingskipAlgorithmPrefixindecryptensures symmetry with the encryption method.
94-109: Split logic is carefully implemented.
Usingslice(1)to exclude the algorithm prefix is a neat approach. The additional checks help avoid malformed data issues.apps/services/bff/src/app/modules/auth/token-refresh.service.ts (2)
129-129: Redacted session details in logs.
Removing direct session ID references from logs mitigates potential leakage. This is a beneficial security practice.
218-218: Warning message updated for privacy.
Simplifying the log statement while signaling a token refresh failure helps reduce the risk of session ID exposure.apps/services/bff/src/app/modules/proxy/proxy.service.ts (3)
22-22: Centralizing session management.
ImportingSessionCookieServicehelps maintain a single source of truth for session handling.
72-72: Dependency Injection practice is correct.
InjectingSessionCookieServicealigns with NestJS best practices for service encapsulation.
87-87: Secure session ID retrieval.
Obtaining the session ID viasessionCookieService.getpromotes consistency and removes direct cookie references.apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts (4)
14-14: Appropriate import for encryption.
IncludingCryptoServicefor test usage ensures security measures are consistently evaluated here as well.
64-65: New variables for encrypted session handling.
DeclaringcryptoServiceandencryptedSidclarifies the test approach for secure session checks.
84-88: Initializing and usingcryptoService.
EncryptingSID_VALUEadds an extra layer of security in test scenarios, reflecting real usage.
134-134: Consistent usage of the encrypted session ID.
All test calls include the encrypted SID in the cookie, ensuring uniform coverage and preventing sensitive data leaks.Also applies to: 146-146, 152-152, 180-180, 185-185, 198-198, 203-203, 228-228, 233-233, 262-262, 267-267, 308-308, 313-313
apps/services/bff/src/app/modules/user/user.controller.spec.ts (14)
14-14: Good import for CryptoService.
This reference allows secure session-encryption logic to be integrated into the tests.
74-75: Neat approach adding cryptoService and encryptedSid.
This test-level injection neatly aligns with the new encryption flow.
89-89: Initialization logic looks correct.
Usingapp.getto retrieve CryptoService is a proper NestJS pattern.
91-93: Ensure that the encryption key is properly secured.
While encrypting the SID is beneficial, confirm that the cryptographic key and algorithm configurations are securely stored.
122-122: Cookie set with encryptedSid is correct.
This ensures sessions remain hidden in transit.
139-139: Encrypted SID usage is consistent.
Using the encrypted SID for callback testing aligns with the new security approach.
147-147: Consistent encryption approach in tests.
The cookie is set to the new encrypted SID, ensuring thorough coverage.
172-172: Cookie usage is secured.
Passing the encrypted SID supports safe session transmission.
189-189: Cookie setting is well integrated.
Ensuring continuity with the encrypted SID helps validate new session logic.
214-214: Encrypted SID in the cookie is maintained.
Consistency helps mitigate session hijacking risks.
231-231: Cookie set to encrypted SID remains consistent.
This strengthens test coverage for session management.
253-253: Cookie assignment is secure.
Verifying that the encrypted SID is applied across various test paths is good.
270-270: Sustained usage of encryption in cookie.
Ensures uniform testing of all code paths with the updated approach.
337-337: Repeating pattern of securely setting cookies is correct.
This test covers additional refresh scenarios with encrypted SID.apps/services/bff/src/app/modules/auth/auth.controller.spec.ts (15)
16-16: Good inclusion of CryptoService.
This import paves the way for secure session encryption.
74-75: Nicely integrates cryptoService and encryptedSid.
This will ensure each test scenario uses the encrypted session identifier.
87-87: cryptoService retrieval is correct.
Aligns well with the NestJS injection pattern.
89-89: Configuration retrieval is standard.
No issues found in how the config is fetched.
92-93: Secure cookie encryption.
SID encryption ensures that sensitive data is less exposed in transit.
322-322: Ensuring the correct cache key.
This verifies that the login attempt key uses the plain SID (while the cookie is encrypted).
333-333: Cookie set with encryptedSid is correct.
Matches the new security requirements.
340-340: current login key usage confirmed.
This assertion verifies appropriate transitions from login attempt to current login.
367-367: Cookie usage for subsequent requests is consistent.
Ensures the new security approach remains stable across endpoints.
397-402: Negative test scenario with invalid UUID.
Passing an encrypted but incorrect SID effectively tests error handling behavior.
425-425: Consistent approach to secure cookie usage.
This method call is properly using the encrypted SID in testing.
431-431: Logout flow test referencing encrypted SID.
Ensures alignment with the new cookie encryption method.
460-460: Callback logout test coverage.
This verifies graceful handling of POST-based logout flow.
529-529: Secure cookie in logout callback.
Ensures alignment with the rest of the encryption approach.
578-578: Verifies double session detection with encrypted cookie.
Appropriate consistency across all session logic scenarios.apps/services/bff/src/app/modules/auth/auth.service.ts (9)
24-24: SessionCookieService import is appropriate.
Centralizing cookie management is a strong design choice.
54-54: Injecting SessionCookieService.
This fosters better abstraction for cookie operations, simplifying future maintenance.
266-266: Fetching SID from the session cookie.
This is a secure approach to unify how sid is retrieved throughout the app.
359-359: Clearing session cookie.
Good use of sessionCookieService to handle cookie cleanup consistently.
362-365: Setting the new SID in session cookie.
Using sessionCookieService ensures robust encryption and consistency in all flows.
368-368: Retrieving the old session cookie.
This step is crucial for revoking or cleaning up stale tokens.
424-424: Cookie retrieval for logout is correct.
The sessionCookieService usage is consistent with the rest of the codebase.
434-436: Clear and concise error logging.
This provides a direct reference to the mismatched cookie for easier debugging.
474-474: Consistent cookie clearing on logout.
Centralized session management is good practice.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
apps/services/bff/src/app/modules/user/user.controller.spec.ts (4)
78-87: Enhance type safety of mock objects.While the mock objects are well-structured, consider using more specific types for the mock functions to ensure type safety.
const mockResponse: Partial<Response> = { - cookie: jest.fn(), - clearCookie: jest.fn(), + cookie: jest.fn<Response['cookie']>(), + clearCookie: jest.fn<Response['clearCookie']>(), }
101-117: Add error handling test cases for SessionCookieService.The setup properly integrates SessionCookieService, but consider adding test cases for error scenarios:
- Invalid session values
- Cookie service failures
- Edge cases in session encryption
This aligns with the PR's security focus and ensures robust error handling.
124-125: Enhance test cleanup for better isolation.Consider resetting all mock function calls and restoring the original SessionCookieService state.
// Reset mocks mockRequest.cookies = {} +mockResponse.cookie.mockClear() +mockResponse.clearCookie.mockClear() +jest.spyOn(sessionCookieService, 'set').mockClear()
206-206: Add security-focused assertions for session handling.While the cache key updates correctly use the hashed session ID, consider adding assertions to verify:
- The hashed session ID format
- Prevention of session fixation
- Proper encryption of sensitive data in cache
This aligns with the PR's security enhancement objectives.
// Add these assertions in relevant test cases expect(hashedSid).not.toEqual(SID_VALUE) expect(hashedSid).toMatch(/^[a-f0-9]{64}$/) // Assuming SHA-256 hashingAlso applies to: 218-218, 248-248, 287-287, 353-353
apps/services/bff/src/app/services/cryptoKey.service.ts (2)
16-21: Validate presence of required config at startup.Currently, the constructor calls
initializeCryptoKey()but doesn't handle missing configuration intokenSecretBase64. You might consider adding a user-friendly error or fallback mechanism if the environment variable is empty or undefined, preventing the code from throwing a less descriptive error.
30-40: Robust error handling for invalid key length.The explicit check for 32-byte length is good. Consider logging the incorrect length for operational insights (without revealing the actual key) in case of misconfiguration. For example, log something like: “Expected 32 bytes, got X bytes.”
apps/services/bff/src/app/services/sessionCookie.service.ts (1)
31-43: Potential fallback logic or retry.If hashing fails, the error is rethrown. That’s valid for immediate troubleshooting. However, consider customizing the error or adding more context.
apps/services/bff/src/app/modules/auth/auth.controller.spec.ts (4)
104-116: Enhance setup documentation for better clarity.Consider improving the comments to better explain:
- Why we're capturing the hashed value from the first mock call
- The relationship between the mock response and request cookies
- // Set the hashed SID + // Hash the session ID using SessionCookieService for subsequent test cases sessionCookieService.set({ res: mockResponse as Response, value: SID_VALUE, }) - // Capture the hashed value from the mock cookie call + // Extract the hashed session ID from the first (index 0) cookie setting call hashedSid = (mockResponse.cookie as jest.Mock).mock.calls[0][1] - // Set up the mock request cookies for subsequent get() calls + // Configure mock request with the hashed session ID for cookie validation mockRequest.cookies = { [SESSION_COOKIE_NAME]: hashedSid, }
Line range hint
345-364: Enhance cache key validation in login tests.Consider adding more robust validations:
- Extract the cache key format into a constant
- Add validation for the hash format
+ const CACHE_KEY_FORMAT = `attempt::${mockConfig.name}::%s` + expect(loginAttempt[0]).toContain( - `attempt::${mockConfig.name}::${SID_VALUE}`, + CACHE_KEY_FORMAT.replace('%s', SID_VALUE), ) + // Verify the hashed session ID format + expect(hashedSid).toMatch(/^[a-f0-9]{64}$/) // Assuming SHA-256 hash
356-357: Reduce duplication in cookie header setting.Consider extracting the cookie header setup into a helper function to maintain DRY principles and improve maintainability.
+ const withSessionCookie = (request: request.Test) => + request.set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) + - .set('Cookie', [`${SESSION_COOKIE_NAME}=${hashedSid}`]) + withSessionCookie(request)Also applies to: 390-391, 443-444, 547-548
Line range hint
578-596: Enhance logout test coverage and reduce duplication.Consider the following improvements:
- Extract cache key construction to a helper
- Add test cases for edge cases (e.g., expired sessions, invalid hash formats)
+ const buildCacheKey = (type: 'current' | 'attempt', sid: string) => + `${type}::${mockConfig.name}::${sid}` + - const currentKey = `current::${mockConfig.name}::${hashedSid}` + const currentKey = buildCacheKey('current', hashedSid)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/services/bff/src/app/modules/auth/auth.controller.spec.ts(12 hunks)apps/services/bff/src/app/modules/auth/auth.module.ts(1 hunks)apps/services/bff/src/app/modules/auth/auth.service.ts(9 hunks)apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts(8 hunks)apps/services/bff/src/app/modules/auth/token-refresh.service.ts(6 hunks)apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts(11 hunks)apps/services/bff/src/app/modules/proxy/proxy.module.ts(2 hunks)apps/services/bff/src/app/modules/proxy/proxy.service.ts(4 hunks)apps/services/bff/src/app/modules/user/user.controller.spec.ts(14 hunks)apps/services/bff/src/app/modules/user/user.module.ts(2 hunks)apps/services/bff/src/app/modules/user/user.service.ts(4 hunks)apps/services/bff/src/app/services/crypto.service.spec.ts(2 hunks)apps/services/bff/src/app/services/crypto.service.ts(3 hunks)apps/services/bff/src/app/services/cryptoKey.service.ts(1 hunks)apps/services/bff/src/app/services/sessionCookie.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/services/bff/src/app/services/crypto.service.spec.ts
- apps/services/bff/src/app/modules/auth/token-refresh.service.spec.ts
- apps/services/bff/src/app/modules/user/user.module.ts
- apps/services/bff/src/app/modules/proxy/proxy.module.ts
- apps/services/bff/src/app/modules/proxy/proxy.service.ts
- apps/services/bff/src/app/modules/auth/token-refresh.service.ts
🧰 Additional context used
📓 Path-based instructions (9)
apps/services/bff/src/app/modules/user/user.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/auth/auth.module.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/user/user.controller.spec.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/services/crypto.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/services/cryptoKey.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/auth/auth.controller.spec.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/services/sessionCookie.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/auth/auth.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (26)
apps/services/bff/src/app/modules/user/user.controller.spec.ts (2)
4-4: LGTM! Appropriate imports for session handling.The added imports properly support the new session cookie functionality, following NestJS best practices for type safety.
Also applies to: 15-15
Line range hint
1-378: Well-structured test suite with robust security coverage.The implementation successfully integrates session encryption while maintaining good test practices:
- Proper dependency injection
- Comprehensive test scenarios
- Clear separation of concerns
- Consistent use of hashed session IDs
The test suite aligns well with the PR's security objectives and NestJS architecture guidelines.
apps/services/bff/src/app/services/cryptoKey.service.ts (2)
1-11: Excellent use of NestJS and clear documentation.The file introduces a well-documented and self-contained
CryptoKeyService. Adhering to NestJS conventions for dependency injection and providing detailed docstrings helps maintain clarity.
46-48: Public getter aligns with best practices.Exposing the
_cryptoKeyvia a getter fosters encapsulation. This approach allows future modifications or validations in a single location.apps/services/bff/src/app/modules/auth/auth.module.ts (2)
2-5: Well-structured imports for NestJS module.You are cleanly organizing providers (
CryptoService,PKCEService,CryptoKeyService, etc.) which ensures clear dependencies.
12-19: Providers array is properly expanded.The addition of
SessionCookieServiceandCryptoKeyServicefollows NestJS best practices for modular dependency injection. Ensure each service is thoroughly tested.apps/services/bff/src/app/services/sessionCookie.service.ts (6)
1-9: Import statements and constants are well-structured.You are correctly importing
Logger,Injectable,CryptoKeyService, and defining the session cookie constant. This sets a clean foundation for the rest of the service.
10-17: Constructor injection is consistent with NestJS.The use of
LOGGER_PROVIDERandCryptoKeyServicepromotes clear separation of concerns.
24-26: Graceful handling of missing session cookies.Returning
undefinedwhen no session cookie is found simplifies checks in consuming code.
48-58: Comparison logic is properly hashed.This ensures the original session value is never exposed. Good to see consistent hashing.
63-65: Cookie clearing is straightforward.Using
clearCookiewith shared options consolidates the session removal process.
70-75: Secure HMAC approach.Utilizing HMAC with SHA-256 ensures robust hashing. This setup prevents reverse engineering of session IDs.
apps/services/bff/src/app/modules/user/user.service.ts (4)
7-7: Centralized session cookie management.Importing
SessionCookieServicereplaces direct cookie handling, aligning with the new architecture.
19-19: Constructor injection for the session cookie.This ensures the
UserServicecan handle sessions without duplicating logic.
44-44: Secure retrieval of hashed session ID.Using
sessionCookieService.get(req)is a safer approach, ensuring no raw session ID is exposed.
69-69: Consistent naming with “cacheKey” parameter.Renaming from
sidtocacheKeyprevents confusion and clarifies that the stored session data is hashed.apps/services/bff/src/app/services/crypto.service.ts (2)
13-14: LGTM! Good separation of concerns.The constructor now properly delegates key management to CryptoKeyService, improving modularity and maintainability.
34-38: Verify the cryptographic key handling.The encryption and decryption methods now consistently use the key from CryptoKeyService. However, let's verify the key's lifecycle and potential race conditions.
Also applies to: 81-85
✅ Verification successful
Let me gather more information about the CryptoKeyService implementation.
Let me check one more aspect about the service initialization.
Cryptographic key handling is properly implemented and thread-safe
The CryptoKeyService implementation shows a robust and thread-safe design:
- The key is initialized once during service construction
- Uses a private readonly field
_cryptoKeywith a public getter-only access- Key initialization is synchronous and validates the key length
- The service is managed by NestJS DI container (Injectable decorator) which ensures singleton scope by default
The implementation prevents race conditions and ensures consistent key access across the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential race conditions in key initialization # Test: Search for CryptoKeyService initialization and usage patterns ast-grep --pattern 'class CryptoKeyService { $$$ cryptoKey $$$ }' # Test: Check for any direct key assignments or modifications rg -A 5 'cryptoKey\s*='Length of output: 639
Script:
#!/bin/bash # Check the complete implementation of CryptoKeyService cat apps/services/bff/src/app/services/cryptoKey.service.ts # Check where CryptoKeyService is instantiated rg -l "new CryptoKeyService" # Check if there are any async operations in key initialization rg "async.*initializeCryptoKey"Length of output: 1575
Script:
#!/bin/bash # Check how the service is registered in the module rg -A 5 "CryptoKeyService.*providers" # Check for any direct instantiation in tests rg "CryptoKeyService.*beforeEach" # Check if the service is used in singleton scope ast-grep --pattern '@Injectable({ $$$ scope $$$ })'Length of output: 135
apps/services/bff/src/app/modules/proxy/proxy.controller.spec.ts (3)
65-77: LGTM! Well-structured test setup.Good test isolation with proper mocking of Response and Request objects.
96-111: LGTM! Proper session cookie service initialization.The setup correctly initializes the session cookie service and captures the hashed session ID for subsequent tests.
156-156: LGTM! Consistent session cookie usage.All test cases have been properly updated to use the hashed session ID, maintaining consistency across the test suite.
Also applies to: 168-168, 174-174, 202-202, 207-207, 220-220, 225-225, 250-250, 255-255, 284-284, 289-289, 330-330, 335-335
apps/services/bff/src/app/modules/auth/auth.service.ts (4)
54-54: LGTM! Good dependency injection.Properly injected SessionCookieService for centralized session management.
128-131: LGTM! Secure cache key handling.Good security practice using hashed session IDs for cache keys.
Line range hint
431-483: LGTM! Comprehensive logout handling.The logout process properly handles session verification, cleanup, and error cases.
362-379: Verify session transition handling.The session transition logic looks secure, but let's verify the handling of concurrent sessions.
✅ Verification successful
Session transition handling is properly implemented with secure cleanup
The verification shows that the session handling implementation in
auth.service.tsfollows secure practices for managing concurrent sessions:
- Existing sessions are properly cleared before setting new ones (
sessionCookieService.clear(res))- The code includes explicit checks for old session cookies and their verification
- The
SessionCookieServiceimplements proper hashing and encryption for cookie values- Session cleanup is consistently handled across login and logout flows
Key security measures found:
- Session cookies are cleared before setting new ones to prevent multiple active sessions
- Old session verification is performed using secure hash comparison
- Proper error handling and logging is implemented for session-related operations
- Logout flow includes complete session cleanup (cookies and cache)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential session handling edge cases # Test: Look for other places handling multiple sessions rg -A 5 'session.*cookie' # Test: Check for proper session cleanup patterns ast-grep --pattern 'clear($$$)'Length of output: 10986
apps/services/bff/src/app/modules/auth/auth.controller.spec.ts (1)
5-5: LGTM! Well-structured test setup with proper type definitions.The mock objects and type imports are correctly implemented, following NestJS testing best practices.
Also applies to: 17-17, 78-87
|
Affected services are: services-bff, Deployed services: services-bff-portals-admin,services-bff-portals-my-pages. |
Integrate encryption for session cookies
What
Adds encryption for session IDs in the authentication flow to enhance security.
CryptoServiceand introducesSessionCookieServiceto manage encryption and session cookie operations.Why
Currently if sid is leaked, then anyone could call the user endpoint and get information about the associated user.
TODO
Checklist:
Summary by CodeRabbit
Release Notes
Security Enhancements
New Features
SessionCookieServiceto centralize cookie management.CryptoKeyServicefor cryptographic key management.Improvements
Testing