1097 wasa changes #30
Conversation
WalkthroughThis pull request introduces a comprehensive refactoring of session storage management across the application. A new Changes
Sequence DiagramsequenceDiagram
participant Component
participant SessionStorageService
participant SessionStorage
Component->>SessionStorageService: setItem("key", value)
SessionStorageService->>SessionStorageService: Encrypt value
SessionStorageService->>SessionStorage: Store encrypted value
Component->>SessionStorageService: getItem("key")
SessionStorageService->>SessionStorage: Retrieve encrypted value
SessionStorageService->>SessionStorageService: Decrypt value
SessionStorageService-->>Component: Return decrypted value
Poem
✨ Finishing Touches
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: 24
🧹 Outside diff range and nitpick comments (16)
src/environments/environment.ts (1)
Security implementation requires immediate attention
The verification reveals several critical security concerns that need to be addressed:
- The encryption keys for both development and production environments are identical and exposed in the source code
- The session storage service implements AES encryption but lacks proper key management
- No security documentation exists for the encryption implementation
Action items:
- Remove hardcoded encryption keys from source code
- Implement proper key management through secure configuration
- Document the security implementation including encryption methods and key rotation procedures
- Add security considerations in the technical documentation
🔗 Analysis chain
Line range hint
23-34: Provide comprehensive documentation for security changesBased on the AI summary mentioning "sessionStorageService" and these encryption key changes, this appears to be part of a larger security implementation. Given the security-sensitive nature of these changes, please:
- Update the PR description with:
- Detailed explanation of the security changes
- Threat model being addressed
- Testing methodology
- Add technical documentation covering:
- Encryption algorithm and key usage
- Session storage security measures
- Security considerations for developers
Let me gather more information about the encryption and security implementation.
Let me check the session storage service implementation to understand how the encryption keys are being used.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Look for related security changes echo "Searching for session storage related changes:" rg -l "sessionStorage|localStorage" echo "\nSearching for security-related services:" ast-grep --pattern 'class $_ { $$$ session$_($$$) { $$$ } $$$ }'Length of output: 1306
Script:
#!/bin/bash # Check encryption key usage and security implementations echo "Searching for encryption related code:" rg -l "encKey|encryption|decrypt" echo "\nChecking encryption key implementations:" ast-grep --pattern 'export const $_ = { $$$ encKey: $$$ $$$ }' echo "\nLooking for security documentation:" fd "README|SECURITY|CONTRIBUTING" --extension md --exec cat {}Length of output: 7917
Script:
#!/bin/bash # Check session storage service implementation echo "Checking session storage service implementation:" cat src/app/services/sessionStorageService/session-storage.service.ts echo "\nChecking encryption key definitions:" cat src/environments/enckey.ts 2>/dev/null || echo "File not accessible" echo "\nChecking for security-related imports:" rg -l "CryptoJS|crypto|encrypt" --type tsLength of output: 3623
src/app/services/authGuardService/auth-guard2.services.ts (1)
Line range hint
59-67: Improve route guard implementationCurrent implementation has several issues:
- Using alert() for user feedback is not recommended
- Double semicolon in return statement
- Variable names are not descriptive
Consider this improved implementation:
canActivate(route: ActivatedRouteSnapshot, state: RouterStateSnapshot): boolean { const isOnCall = this.sessionstorage.getItem(this.IS_ON_CALL_KEY); if (isOnCall === 'yes') { return true; } // Use a service for showing messages this.dataSettingService.alert(this.currentLanguageSet.pleseWaitForCallToComeOrLogout); return false; }🧰 Tools
🪛 Biome (1.9.4)
[error] 60-61: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
src/app/services/authGuardService/auth-guard.services.ts (2)
Line range hint
67-93: Remove or restore commented code blockLarge blocks of commented code can lead to confusion and maintenance issues.
Either:
- Remove the commented authorization code if it's no longer needed
- Restore and update it if it's still required
- If kept for reference, move it to documentation
Line range hint
97-98: Replace alert with proper error handlingUsing
alert()for user feedback is not recommended in modern web applications.Consider using a modal or toast notification service instead:
// Example using a notification service if (key === 'yes') { this.notificationService.warn(this.currentLanguageSet.NotAllowedToGoBackPleaseComplete); return false; }src/app/outbond-worklist/outbond-worklist.component.ts (1)
87-90: Clean up commented code.There are several commented lines around the session storage call that should be removed if they're no longer needed.
- // this._dataServivce.outboundBenID = data.beneficiary.beneficiaryRegID; - // this._dataServivce.outboundCallReqID = data.outboundCallReqID; this.sessionstorage.setItem("isOnCall", "yes"); - // this._dataServivce.isSelf = data.isSelf;src/app/service-role-selection/service-role-selection.component.ts (3)
Line range hint
77-77: Use sessionStorageService for apiman_key.Replace direct sessionStorage usage with the injected service for consistency.
- sessionStorage.setItem('apiman_key',service.apimanClientKey); + this.sessionstorage.setItem('apiman_key',service.apimanClientKey);
Line range hint
81-82: Simplify null checks.The pattern
!=undefined && !=nullis redundant. Using just!=nullwill check for both null and undefined.- if (screen !=undefined && screen !=null && screen.trim().toLowerCase() === 'Registration_Counselling'.trim().toLowerCase()) { + if (screen != null && screen.trim().toLowerCase() === 'Registration_Counselling'.trim().toLowerCase()) {Also applies to: 108-109
Migration to sessionStorageService is incomplete and needs attention
The codebase shows inconsistent usage of session storage:
- Several components have properly imported and injected the new
sessionStorageService- However, there are still direct
sessionStoragecalls in multiple files including:
http.interceptor.tsauth.service.tsauth-guard.services.tslogin.component.tsmulti-role-screen.component.tsThese direct calls bypass the encryption functionality provided by the service and should be migrated to use the new service for consistent security implementation.
🔗 Analysis chain
Line range hint
1-1: PR needs better documentation and testing strategy.While the implementation of sessionStorageService is consistent across components, this PR represents a significant architectural change that should be:
- Properly documented with a clear PR description
- Tested thoroughly to ensure all session storage operations work as expected
- Accompanied by a migration guide if other components need to adopt this pattern
Let me analyze the usage of sessionStorage across the codebase to better understand the architectural changes. I'll check for the new SessionStorageService implementation and its adoption.
Let me try a different approach to find the service implementation and its usage.
Let me check for the actual usage of this service in components to verify the migration status.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining direct sessionStorage usage that should be migrated rg "sessionStorage\." --type tsLength of output: 2328
Script:
#!/bin/bash # Check for SessionStorageService usage rg "SessionStorageService" --type ts -A 2Length of output: 41
Script:
#!/bin/bash # Look for session storage service file and its content fd "session-storage.service" --type f --exec cat {}Length of output: 2853
Script:
#!/bin/bash # Look for constructor injections of sessionStorageService rg "constructor.*sessionStorageService" --type ts -B 2 -A 2 # Also check for imports of the service rg "import.*sessionStorageService" --type tsLength of output: 6911
src/app/http.interceptor.ts (1)
253-254: Use TypeScript features for better type safety.The Observable declaration can be improved using TypeScript features.
-logoutUserFromPreviousSession = new BehaviorSubject(this.dologout); -logoutUserFromPreviousSessions$ = - this.logoutUserFromPreviousSession.asObservable(); +logoutUserFromPreviousSession = new BehaviorSubject<boolean>(false); +logoutUserFromPreviousSessions$ = this.logoutUserFromPreviousSession.asObservable();Also applies to: 258-258
src/app/services/czentrix/czentrix.service.ts (1)
91-94: Consider consolidating multiple removeItem calls.Multiple consecutive removeItem calls could be combined into a single clear operation for better efficiency.
- this.sessionstorage.removeItem("privilege_flag"); - this.sessionstorage.removeItem("session_id"); - this.sessionstorage.removeItem("callTransferred"); + this.sessionstorage.clear(["privilege_flag", "session_id", "callTransferred"]);src/app/multi-role-screen/multi-role-screen.component.ts (1)
376-376: Consider adding error handling for setItem operation.The language preference storage should handle potential storage errors.
- this.sessionstorage.setItem("setLanguage", language); + try { + this.sessionstorage.setItem("setLanguage", language); + } catch (error) { + console.warn('Failed to persist language preference:', error); + // Fallback to default language + this.app_language = 'English'; + }src/app/closure/closure.component.ts (2)
Line range hint
136-141: Consider adding type safety for session storage values.While the logic is correct, consider adding type safety for session storage values to prevent potential runtime errors.
-this.isEverwell = this.sessionstorage.getItem("isEverwellCall"); +this.isEverwell = this.sessionstorage.getItem("isEverwellCall") || "no";
Line range hint
661-672: Add error handling for session storage operations.Consider adding try-catch block for session storage operations to handle potential errors gracefully.
showAlert() { + try { this.sessionstorage.removeItem("isOnCall"); if (this.transferValid == true) { this.message.alert( this.currentLanguageSet.callTransferredSuccessfully, "success" ); } else { this.message.alert( this.currentLanguageSet.callClosedSuccessfully, "success" ); } + } catch (error) { + console.error('Error clearing session storage:', error); + // Still show the success message as the call operation succeeded + this.message.alert(this.currentLanguageSet.callClosedSuccessfully, "success"); + } }src/app/innerpage/innerpage.component.ts (3)
988-989: Improve language persistence robustness.Add type safety and error handling for language-related session storage operations.
+interface LanguageSettings { + language: string; + timestamp: number; +} -if (this.sessionstorage.getItem("setLanguage") != null) { - this.changeLanguage(this.sessionstorage.getItem("setLanguage")); +const savedLanguage = this.sessionstorage.getItem("setLanguage"); +if (savedLanguage) { + try { + this.changeLanguage(savedLanguage); + } catch (error) { + console.error('Error loading language settings:', error); + this.changeLanguage(this.app_language); + } } else { this.changeLanguage(this.app_language); } -this.sessionstorage.setItem("setLanguage", language); +try { + this.sessionstorage.setItem("setLanguage", language); +} catch (error) { + console.error('Error saving language settings:', error); +}Also applies to: 1019-1019
808-809: Maintain consistency in session storage cleanup.Use the suggested clearSessionStorage method here as well for consistency.
-this.sessionstorage.removeItem("isOnCall"); -this.sessionstorage.removeItem("isEverwellCall"); +this.clearSessionStorage();
Line range hint
1-1037: Centralize session storage keys.Consider creating a centralized configuration for session storage keys to improve maintainability and prevent typos.
// Create a new file: session-storage-keys.ts export const SESSION_STORAGE_KEYS = { IS_ON_CALL: 'isOnCall', IS_EVERWELL_CALL: 'isEverwellCall', APIMAN_KEY: 'apiman_key', SET_LANGUAGE: 'setLanguage', CLI: 'CLI', CALL_CATEGORY: 'callCategory', SESSION_ID: 'session_id' } as const;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
package.json(1 hunks)src/app/1097-co/1097-co.component.ts(5 hunks)src/app/activity-this-week/activity-this-week.component.ts(3 hunks)src/app/app.module.ts(2 hunks)src/app/closure/closure.component.ts(4 hunks)src/app/dashboard-user-id/dashboardUserId.component.ts(6 hunks)src/app/dashboard/dashboard.component.ts(13 hunks)src/app/everwell-outbound-worklist/everwell-outbound-worklist.component.ts(3 hunks)src/app/http.factory.ts(1 hunks)src/app/http.interceptor.ts(9 hunks)src/app/innerpage/innerpage.component.ts(12 hunks)src/app/login/login.component.ts(8 hunks)src/app/multi-role-screen/multi-role-screen.component.ts(7 hunks)src/app/outbond-worklist/outbond-worklist.component.ts(3 hunks)src/app/service-role-selection/service-role-selection.component.ts(2 hunks)src/app/services/authGuardService/auth-guard.services.ts(2 hunks)src/app/services/authGuardService/auth-guard2.services.ts(2 hunks)src/app/services/config/config.service.ts(2 hunks)src/app/services/czentrix/czentrix.service.ts(3 hunks)src/app/services/loginService/login.service.ts(5 hunks)src/app/services/sessionStorageService/session-storage.service.ts(1 hunks)src/environments/enckey.ts(1 hunks)src/environments/environment.prod.ts(1 hunks)src/environments/environment.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- package.json
- src/environments/enckey.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/services/sessionStorageService/session-storage.service.ts
[error] 34-37: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
src/app/services/config/config.service.ts
[error] 41-41: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 44-44: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 45-45: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 46-46: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 49-49: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
🔇 Additional comments (21)
src/app/services/config/config.service.ts (1)
41-49: Verify API version compatibility
The significant version jump from v1.x to v3.0.0 suggests breaking changes. Please ensure:
- All API endpoints are compatible with v3.0.0
- All request/response structures are updated accordingly
✅ Verification successful
Let me check the actual usage context of v1.x in the identified file to determine if it's an API endpoint version reference or something else, and whether it poses compatibility concerns.
No API version compatibility issues found
The v1.x reference appears only in a commented-out URL, while the actual implementation uses the dynamic base URL from config service (this._config.getCommonBaseURL()), which we verified is already set to v3.0.0. The commented code can be safely ignored as it's not in use.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining v1.0 or v1.2 API calls
rg -l "v1\.[0-2]" --type ts
Length of output: 99
Script:
#!/bin/bash
# Check the specific usage of v1.x in the outbound service file
rg -A 5 -B 5 "v1\.[0-2]" src/app/services/outboundServices/outbound-call-allocation.service.ts
Length of output: 627
🧰 Tools
🪛 Biome (1.9.4)
[error] 41-41: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 44-44: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 45-45: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 46-46: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 49-49: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
src/app/services/loginService/login.service.ts (1)
62-64: Review authentication security settings
The withCredentials: true setting allows sending cookies in cross-origin requests.
Ensure:
- CORS is properly configured on the server
- Cookie security attributes (Secure, HttpOnly, SameSite) are set
- The authentication endpoint is protected against CSRF attacks
src/app/outbond-worklist/outbond-worklist.component.ts (1)
32-32: LGTM! Service integration looks good.
The sessionStorageService is correctly imported and injected into the constructor.
Also applies to: 45-45
src/app/activity-this-week/activity-this-week.component.ts (2)
34-34: LGTM! Service integration looks good.
The sessionStorageService is correctly imported and injected into the constructor.
Also applies to: 50-50
Line range hint 140-152: LGTM! Good error handling improvement.
The error handling using includes() is more robust than the commented contains() approach. The use of an errorText variable also improves readability.
src/app/service-role-selection/service-role-selection.component.ts (1)
32-32: LGTM! Service integration looks good.
The sessionStorageService is correctly imported and injected into the constructor.
Also applies to: 46-46
src/app/everwell-outbound-worklist/everwell-outbound-worklist.component.ts (2)
33-33: LGTM! Good practice using dependency injection for session storage.
The introduction of sessionStorageService and its injection through the constructor follows good Angular practices for dependency management and service encapsulation.
Also applies to: 47-47
108-109: LGTM! Proper usage of the session storage service.
The migration from direct sessionStorage calls to the sessionStorageService improves maintainability and testability.
src/app/dashboard-user-id/dashboardUserId.component.ts (1)
34-34: LGTM! Good practice using dependency injection for session storage.
The introduction of sessionStorageService and its injection through the constructor follows good Angular practices.
Also applies to: 49-49
src/app/services/czentrix/czentrix.service.ts (1)
33-33: LGTM: Service dependency properly injected.
The sessionStorageService is correctly imported and injected into the constructor.
Also applies to: 56-56
src/app/multi-role-screen/multi-role-screen.component.ts (1)
345-346: LGTM: Proper null check for language preference.
The code correctly checks for stored language preference before applying defaults.
src/app/1097-co/1097-co.component.ts (3)
89-89: LGTM: Proper dependency injection of sessionStorageService
The service is correctly injected as a private member in the constructor.
126-131: LGTM: Consistent usage of sessionStorage service
The direct sessionStorage API calls are properly replaced with the injected service for CLI and session_id retrieval.
290-291: LGTM: Proper cleanup of session storage items
The service is correctly used to remove session items during call closure.
src/app/dashboard/dashboard.component.ts (4)
79-79: LGTM: Proper dependency injection of sessionStorageService
The service is correctly injected as a private member in the constructor.
Line range hint 171-234: LGTM: Consistent campaign management with session storage service
The service is properly used throughout the campaign management logic, maintaining the existing business logic while improving encapsulation.
Line range hint 307-337: LGTM: Proper event handling with session storage service
The service is correctly used for managing session state during event handling.
Line range hint 379-402: LGTM: Proper campaign switching with session storage service
The service is correctly used during campaign switching operations.
src/app/app.module.ts (2)
239-239: LGTM: Proper import of sessionStorageService
The service is correctly imported.
415-415: LGTM: Proper registration of sessionStorageService
The service is correctly registered in the providers array, making it available for dependency injection throughout the application.
src/app/closure/closure.component.ts (1)
118-118: LGTM! Proper dependency injection.
The sessionStorageService is correctly injected following Angular's dependency injection pattern.
src/environments/environment.prod.ts
Outdated
| @@ -20,8 +20,11 @@ | |||
| * along with this program. If not, see https://www.gnu.org/licenses/. | |||
| */ | |||
|
|
|||
| import { keys } from "./enckey"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the results, I can generate the final response since I found clear evidence that encryption keys are hardcoded in the source code:
Critical: Hardcoded encryption keys detected in source control
The file src/environments/enckey.ts contains hardcoded encryption keys that are identical for both development and production environments:
dev: '896Y7lbv0qFr1WgppH0NjSNRJZ9nDciBNeH6MA0F1MZDJb0SXhceLPw2kSpdObG1',
prod: '896Y7lbv0qFr1WgppH0NjSNRJZ9nDciBNeH6MA0F1MZDJb0SXhceLPw2kSpdObG1'
Security issues:
- Encryption keys are committed to source control, exposing them to unauthorized access
- Same key is used for both development and production environments
- Keys appear to be hardcoded rather than managed through secure configuration
Recommendations:
- Remove encryption keys from source code immediately
- Use environment variables or secure key management service
- Use different keys for development and production environments
- Rotate the exposed keys as they should be considered compromised
🔗 Analysis chain
Verify the security of encryption key management
The encryption keys should not be stored in source control. Consider using environment variables or a secure configuration management system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if enckey.ts exists and verify if it contains hardcoded keys
if fd -t f "enckey.ts$"; then
echo "Found enckey.ts file(s):"
rg -l "const keys"
echo "\nChecking key contents:"
rg "(?:dev|prod).*:.*['|\"].*['|\"]"
fi
Length of output: 95223
src/environments/environment.prod.ts
Outdated
| invalidCallType: 'Invalid', | ||
| encKey: keys.prod, |
There was a problem hiding this comment.
💡 Codebase verification
Security Issue: Development and Production Using Same Encryption Key
The verification reveals critical security concerns:
- The same encryption key is being used for both development and production environments (
devandprodkeys are identical) - No key rotation mechanism is implemented
- Keys are hardcoded in the source code instead of using a secure key management solution
This poses significant security risks and violates security best practices. The application needs:
- Different encryption keys for development and production environments
- Implementation of a secure key management system
- Regular key rotation mechanism
- Removal of hardcoded keys from source code
🔗 Analysis chain
Review the encryption key implementation
Several concerns need to be addressed:
- Ensure different keys are used for development and production environments
- Document the key rotation policy
- Consider implementing a secure key management solution
Let me gather more information about the encryption key implementation.
Let me check the encryption key implementation in more detail.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any documentation about encryption implementation
fd -t f "README|SECURITY|CONTRIBUTING" -e md -X cat
Length of output: 6814
Script:
#!/bin/bash
# Check for environment files and their key implementations
fd -t f "environment" -X cat
# Search for any key-related configuration or implementation
rg -i "key|encrypt" --type ts
Length of output: 33841
Script:
#!/bin/bash
# Check the enckey file content and implementation
fd -t f "enckey" -X cat
# Look for any key rotation or management related code
rg -i "rotation|management.*key" --type ts
Length of output: 246
src/environments/environment.ts
Outdated
| @@ -20,6 +20,8 @@ | |||
| * along with this program. If not, see https://www.gnu.org/licenses/. | |||
| */ | |||
|
|
|||
| import { keys } from "./enckey"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Documentation needed for session storage encryption implementation
The codebase implements AES encryption for session storage using CryptoJS, with the following findings:
- A hardcoded encryption key is shared between dev and prod environments
- Basic encryption/decryption is implemented in SessionStorageService
- No key rotation mechanism is present
- Missing documentation for security practices
Action items needed:
- Remove hardcoded encryption keys from source control
- Implement secure key management and rotation procedures
- Add developer documentation for the encryption system
- Include security testing guidelines for encrypted data handling
🔗 Analysis chain
Document the encryption implementation details
The addition of encryption keys suggests significant security-related changes. Please provide:
- Documentation about the encryption implementation
- Usage guidelines for developers
- Testing requirements for encrypted data
- Key rotation procedures
Also applies to: 33-34
Let me gather more specific information about the encryption implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for encryption-related code to understand the implementation
echo "Files potentially using encryption:"
rg -l "encrypt|decrypt|crypto"
echo "\nSearching for encryption-related services:"
ast-grep --pattern 'class $_ {
$$$
encrypt($$$) {
$$$
}
$$$
}'
Length of output: 533
Script:
#!/bin/bash
# Check the content of encryption-related files
echo "Content of src/environments/enckey.ts:"
cat src/environments/enckey.ts
echo -e "\nChecking session storage service implementation:"
cat src/app/services/sessionStorageService/session-storage.service.ts
echo -e "\nChecking for encryption key usage:"
rg "keys" -A 3 -B 3
Length of output: 13307
| requestOptions: RequestOptions, loaderService: LoaderService,sessionstorage:sessionStorageService, | ||
| router: Router, authService: AuthService, message: ConfirmationDialogsService): Http { | ||
| return new InterceptedHttp(xhrBackend, requestOptions, loaderService, router, authService, message); | ||
| return new InterceptedHttp(xhrBackend,sessionstorage, requestOptions, loaderService, router, authService, message); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve parameter ordering and type safety
The current implementation has two issues:
- Parameter ordering is inconsistent - session storage service is inserted between existing parameters
- Type safety - parameter name should follow TypeScript naming conventions
Apply this diff to improve the code:
export function httpFactory(xhrBackend: XHRBackend,
- requestOptions: RequestOptions, loaderService: LoaderService,sessionstorage:sessionStorageService,
- router: Router, authService: AuthService, message: ConfirmationDialogsService): Http {
- return new InterceptedHttp(xhrBackend,sessionstorage, requestOptions, loaderService, router, authService, message);
+ requestOptions: RequestOptions,
+ loaderService: LoaderService,
+ router: Router,
+ authService: AuthService,
+ sessionStorage: sessionStorageService,
+ message: ConfirmationDialogsService): Http {
+ return new InterceptedHttp(
+ xhrBackend,
+ sessionStorage,
+ requestOptions,
+ loaderService,
+ router,
+ authService,
+ message
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| requestOptions: RequestOptions, loaderService: LoaderService,sessionstorage:sessionStorageService, | |
| router: Router, authService: AuthService, message: ConfirmationDialogsService): Http { | |
| return new InterceptedHttp(xhrBackend, requestOptions, loaderService, router, authService, message); | |
| return new InterceptedHttp(xhrBackend,sessionstorage, requestOptions, loaderService, router, authService, message); | |
| export function httpFactory(xhrBackend: XHRBackend, | |
| requestOptions: RequestOptions, | |
| loaderService: LoaderService, | |
| router: Router, | |
| authService: AuthService, | |
| sessionStorage: sessionStorageService, | |
| message: ConfirmationDialogsService): Http { | |
| return new InterceptedHttp( | |
| xhrBackend, | |
| sessionStorage, | |
| requestOptions, | |
| loaderService, | |
| router, | |
| authService, | |
| message | |
| ); |
| var key = this.sessionstorage.getItem("isOnCall"); | ||
| var key2 = this.sessionstorage.getItem("authen"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract session storage keys as constants
Magic strings for session storage keys should be extracted as constants to prevent typos and improve maintainability.
Add these constants at the class level:
private readonly IS_ON_CALL_KEY = 'isOnCall';
private readonly AUTH_KEY = 'authen';Then use them in the code:
- var key = this.sessionstorage.getItem("isOnCall");
- var key2 = this.sessionstorage.getItem("authen");
+ const isOnCall = this.sessionstorage.getItem(this.IS_ON_CALL_KEY);
+ const isAuthenticated = this.sessionstorage.getItem(this.AUTH_KEY);
src/app/login/login.component.ts
Outdated
| console.error("response"); | ||
| this.loginservice | ||
| .authenticateUser(this.userID, this.encryptPassword, doLogOut) | ||
| .subscribe( | ||
| (response: any) => { | ||
| console.error("response",response); | ||
| let tkn = response.Jwttoken; | ||
| this.sessionstorage.setCookie('Jwttoken', tkn,1 ); | ||
|
|
There was a problem hiding this comment.
Remove console.error statements from production code.
Production code should not contain console.error statements. Consider using a proper logging service.
- console.error("response");
- console.error("response",response);
+ // Use a logging service instead
+ this.loggingService.error("Authentication response", response);Committable suggestion skipped: line range outside the PR's diff.
src/app/login/login.component.ts
Outdated
| let tkn = response.Jwttoken; | ||
| this.sessionstorage.setCookie('Jwttoken', tkn,1 ); |
There was a problem hiding this comment.
💡 Codebase verification
Security flags needed for JWT cookie storage
I found the setCookie implementation in src/app/services/sessionStorageService/session-storage.service.ts and it doesn't include any security configurations. The current implementation is vulnerable as it only sets expiration and path:
document.cookie = name + "=" + (value || "") + expires + "; path=/";The JWT token cookie needs the following security enhancements:
Secureflag to ensure cookie is only sent over HTTPSHttpOnlyflag to prevent XSS attacks from accessing the cookieSameSiteattribute to prevent CSRF attacks
🔗 Analysis chain
Review JWT token storage security.
Storing JWT tokens in cookies requires proper security configurations.
Consider adding security flags to the cookie:
- this.sessionstorage.setCookie('Jwttoken', tkn,1 );
+ this.sessionstorage.setCookie('Jwttoken', tkn, 1, {
+ secure: true,
+ httpOnly: true,
+ sameSite: 'strict'
+ });Let me check the implementation of the setCookie method to understand if security configurations are handled internally.
Let me try a different approach to find the cookie-related service implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for secure cookie configurations in the codebase
rg -A 5 "setCookie.*token"
Length of output: 876
Script:
#!/bin/bash
# Check for the implementation of setCookie method
ast-grep --pattern 'setCookie($$$) { $$$ }'
# Also check for any cookie-related configurations in the codebase
rg -i "cookie.*secure|httpOnly|sameSite"
Length of output: 88
Script:
#!/bin/bash
# Find the sessionstorage service file
fd "sessionstorage" --type f
# Look for any cookie-related configurations in the entire codebase
rg -i "cookie" -A 5 -B 5
Length of output: 19548
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| this.sessionstorage.removeItem('isOnCall'); | ||
| this.sessionstorage.removeItem('isEverwellCall'); | ||
| sessionStorage.removeItem('apiman_key'); | ||
| sessionStorage.removeItem("setLanguage"); | ||
| this.sessionstorage.removeItem("setLanguage"); | ||
| this.dataSettingService.appLanguage="English"; | ||
| this.authService.removeToken(); | ||
| this.router.navigate(['']); | ||
|
|
||
| // this.socketService.logOut(); | ||
| }, | ||
| (err) => { | ||
| sessionStorage.removeItem('isOnCall'); | ||
| sessionStorage.removeItem('isEverwellCall'); | ||
| this.sessionstorage.removeItem('isOnCall'); | ||
| this.sessionstorage.removeItem('isEverwellCall'); | ||
| sessionStorage.removeItem('apiman_key'); | ||
| sessionStorage.removeItem("setLanguage"); | ||
| this.sessionstorage.removeItem("setLanguage"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate duplicate session cleanup logic.
The same session cleanup code is repeated in both success and error handlers.
Extract the cleanup logic into a separate method:
+ private cleanupSession() {
+ this.sessionstorage.removeItem('isOnCall');
+ this.sessionstorage.removeItem('isEverwellCall');
+ sessionStorage.removeItem('apiman_key');
+ this.sessionstorage.removeItem("setLanguage");
+ this.dataSettingService.appLanguage="English";
+ this.authService.removeToken();
+ }
ipSuccessLogoutHandler(response) {
this.Czentrix.userLogout().subscribe(
(response) => {
- this.sessionstorage.removeItem('isOnCall');
- this.sessionstorage.removeItem('isEverwellCall');
- sessionStorage.removeItem('apiman_key');
- this.sessionstorage.removeItem("setLanguage");
- this.dataSettingService.appLanguage="English";
- this.authService.removeToken();
+ this.cleanupSession();
this.router.navigate(['']);
},
(err) => {
- this.sessionstorage.removeItem('isOnCall');
- this.sessionstorage.removeItem('isEverwellCall');
- sessionStorage.removeItem('apiman_key');
- this.sessionstorage.removeItem("setLanguage");
- this.dataSettingService.appLanguage="English";
- this.authService.removeToken();
+ this.cleanupSession();
this.router.navigate(['']);
}
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.sessionstorage.removeItem('isOnCall'); | |
| this.sessionstorage.removeItem('isEverwellCall'); | |
| sessionStorage.removeItem('apiman_key'); | |
| sessionStorage.removeItem("setLanguage"); | |
| this.sessionstorage.removeItem("setLanguage"); | |
| this.dataSettingService.appLanguage="English"; | |
| this.authService.removeToken(); | |
| this.router.navigate(['']); | |
| // this.socketService.logOut(); | |
| }, | |
| (err) => { | |
| sessionStorage.removeItem('isOnCall'); | |
| sessionStorage.removeItem('isEverwellCall'); | |
| this.sessionstorage.removeItem('isOnCall'); | |
| this.sessionstorage.removeItem('isEverwellCall'); | |
| sessionStorage.removeItem('apiman_key'); | |
| sessionStorage.removeItem("setLanguage"); | |
| this.sessionstorage.removeItem("setLanguage"); | |
| private cleanupSession() { | |
| this.sessionstorage.removeItem('isOnCall'); | |
| this.sessionstorage.removeItem('isEverwellCall'); | |
| sessionStorage.removeItem('apiman_key'); | |
| this.sessionstorage.removeItem("setLanguage"); | |
| this.dataSettingService.appLanguage="English"; | |
| this.authService.removeToken(); | |
| } | |
| this.cleanupSession(); | |
| this.router.navigate(['']); | |
| // this.socketService.logOut(); | |
| }, | |
| (err) => { | |
| this.cleanupSession(); | |
| this.router.navigate(['']); |
| this.sessionstorage.removeItem("isOnCall"); | ||
| this.sessionstorage.removeItem("isEverwellCall"); | ||
| this.sessionstorage.removeItem("apiman_key"); | ||
| this.sessionstorage.removeItem("setLanguage"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract repeated session storage cleanup into a separate method.
The session storage cleanup logic is duplicated across multiple code blocks. Consider extracting it into a reusable method.
+private clearSessionStorage(): void {
+ try {
+ this.sessionstorage.removeItem("isOnCall");
+ this.sessionstorage.removeItem("isEverwellCall");
+ this.sessionstorage.removeItem("apiman_key");
+ this.sessionstorage.removeItem("setLanguage");
+ } catch (error) {
+ console.error('Error clearing session storage:', error);
+ }
+}
// Replace all instances of repeated cleanup with:
this.clearSessionStorage();Also applies to: 512-515, 531-534, 541-544
| if (this.sessionstorage.getItem("CLI") !== undefined) { | ||
| this.callerNumber = this.sessionstorage.getItem("CLI"); | ||
| } | ||
| if (sessionStorage.getItem("callCategory") !== undefined) { | ||
| if (sessionStorage.getItem("callCategory") === "OUTBOUND") { | ||
| if (this.sessionstorage.getItem("callCategory") !== undefined) { | ||
| if (this.sessionstorage.getItem("callCategory") === "OUTBOUND") { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor session storage checks for better maintainability.
Consider consolidating the session storage checks and adding proper null handling.
-if (this.sessionstorage.getItem("CLI") !== undefined) {
- this.callerNumber = this.sessionstorage.getItem("CLI");
-}
-if (this.sessionstorage.getItem("callCategory") !== undefined) {
- if (this.sessionstorage.getItem("callCategory") === "OUTBOUND") {
+const cli = this.sessionstorage.getItem("CLI");
+const callCategory = this.sessionstorage.getItem("callCategory");
+
+if (cli) {
+ this.callerNumber = cli;
+}
+
+if (callCategory) {
+ if (callCategory === "OUTBOUND") {
this.getCommonData.isOutbound = true;
} else {
this.getCommonData.isOutbound = false;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (this.sessionstorage.getItem("CLI") !== undefined) { | |
| this.callerNumber = this.sessionstorage.getItem("CLI"); | |
| } | |
| if (sessionStorage.getItem("callCategory") !== undefined) { | |
| if (sessionStorage.getItem("callCategory") === "OUTBOUND") { | |
| if (this.sessionstorage.getItem("callCategory") !== undefined) { | |
| if (this.sessionstorage.getItem("callCategory") === "OUTBOUND") { | |
| const cli = this.sessionstorage.getItem("CLI"); | |
| const callCategory = this.sessionstorage.getItem("callCategory"); | |
| if (cli) { | |
| this.callerNumber = cli; | |
| } | |
| if (callCategory) { | |
| if (callCategory === "OUTBOUND") { | |
| this.getCommonData.isOutbound = true; | |
| } else { | |
| this.getCommonData.isOutbound = false; | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/app/login/login.component.ts (1)
243-245:⚠️ Potential issueImplement secure JWT token storage
The commented code suggests JWT tokens are stored in cookies without proper security configurations. If implementing cookie-based token storage, ensure proper security measures:
- Use HttpOnly flag
- Set Secure flag
- Configure SameSite attribute
🧹 Nitpick comments (2)
src/app/login/login.component.ts (2)
35-35: Consider using path alias for importsInstead of using a relative path, consider using the
@apppath alias for better maintainability.-import { sessionStorageService } from 'app/services/sessionStorageService/session-storage.service'; +import { sessionStorageService } from '@app/services/sessionStorageService/session-storage.service';
297-299: Implement proper error logging mechanismReplace commented console.log statements with a proper logging service that can handle different environments (development/production) appropriately.
+private logError(message: string, error?: any) { + // Implement proper error logging + this.loggingService.error(message, error); +} -// console.log("response.Jwttoken",response) +this.logError('Processing JWT token', response);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/http.interceptor.ts(9 hunks)src/app/login/login.component.ts(8 hunks)src/environments/environment.prod.ts(1 hunks)src/environments/environment.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/environments/environment.ts
- src/environments/environment.prod.ts
- src/app/http.interceptor.ts
🔇 Additional comments (1)
src/app/login/login.component.ts (1)
Line range hint 238-264: Remove console.error and clean up formatting
- Production code should not contain console.error statements
- Remove unnecessary empty lines
-console.error("response");
- this.loginservice
+this.loginservice
.authenticateUser(this.userID, this.encryptPassword, doLogOut)
.subscribe(
(response: any) => {
- // console.error("response",response);
if (
response !== undefined &&
response !== null &&
response.previlegeObj !== undefined &&
response.previlegeObj !== null
) {
this.successCallback(response, this.userID, this.password);
}
},
(error: any) => this.errorCallback(error)
-
- );
-
-
-
-
+ );| this.sessionstorage.removeItem('isOnCall'); | ||
| this.sessionstorage.removeItem('isEverwellCall'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract repeated session cleanup logic into a method
The same session cleanup code is duplicated in multiple places. Consider extracting it into a dedicated method.
+private clearSessionState() {
+ this.sessionstorage.removeItem('isOnCall');
+ this.sessionstorage.removeItem('isEverwellCall');
+}
-this.sessionstorage.removeItem('isOnCall');
-this.sessionstorage.removeItem('isEverwellCall');
+this.clearSessionState();Also applies to: 138-139, 348-349, 357-358
|
All changes I have reviewed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/services/config/config.service.ts (1)
59-61: Document configuration alternativesInstead of keeping commented URLs, consider documenting these alternative APIMAN gateway configurations in a separate configuration file or documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/services/config/config.service.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/services/config/config.service.ts
[error] 41-41: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 44-44: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 45-45: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 46-46: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 49-49: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
src/app/services/config/config.service.ts (1)
32-35: Consider security implications of mixed HTTP/HTTPS endpointsWhile most endpoints have been moved to HTTPS,
telephonyServerIPstill uses HTTP. This mixed protocol usage could pose security risks in production.Previous suggestion to move configurations to environment files still applies for better environment management.
|
|
||
| @Injectable() | ||
| export class ConfigService { | ||
|
|
||
| // private _commonBaseURL: String = `${commonIP}commonapi-v1.0/`; | ||
| private _commonBaseURL: String = `http://10.208.122.38:8080/commonapi-v1.2/`; | ||
| private _commonBaseURL: String = `https://amritwprdev.piramalswasthya.org/commonapi-v1.0/`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix type declarations and reduce duplication
- Use lowercase
stringtype instead ofStringfor TypeScript consistency. - Use the defined constants instead of duplicating URLs.
Apply this diff:
- private _commonBaseURL: String = `https://amritwprdev.piramalswasthya.org/commonapi-v1.0/`;
+ private _commonBaseURL: string = `${commonIP}commonapi-v1.0/`;
- private openCommonURL: String = `https://amritwprdev.piramalswasthya.org/commonapi-v1.0/`;
+ private openCommonURL: string = `${commonIP}commonapi-v1.0/`;
- private _helpline1097BaseURL: String = `${IP1097}1097api-v1.0/`;
+ private _helpline1097BaseURL: string = `${IP1097}1097api-v1.0/`;
- private _telephonyServerURL: String = `${telephonyServerIP}`;
+ private _telephonyServerURL: string = `${telephonyServerIP}`;
- private _opencommonBaseURL: String = `${commonIP}commonapi-v1.0/`;
+ private _opencommonBaseURL: string = `${commonIP}commonapi-v1.0/`;Also applies to: 44-44, 45-45, 46-46, 49-49
🧰 Tools
🪛 Biome (1.9.4)
[error] 41-41: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
📋 Description
JIRA ID:
Please provide a summary of the change and the motivation behind it. Include relevant context and details.
✅ Type of Change
ℹ️ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.
Summary by CodeRabbit
Release Notes
New Features
sessionStorageServicefor managing session storage operations across multiple components.sessionStoragecalls with the new service methods.Bug Fixes
Documentation
Chores