Fix: Resolve login error handling issues #2771
Conversation
- Fix translation service to load locales from correct path (src/locales) - Improve login security to return generic error message for both invalid email and wrong password - Make frontend error handling generic to display backend error messages properly - Remove hardcoded string matching in favor of HTTP status code checks Fixes login flow returning raw localization keys instead of user-facing messages Fixes #2768
gorkem-bwl
requested review from
ajhollid,
mohicody,
Br0wnHammer,
Owaiseimdad and
karenvicent
as code owners
WalkthroughThe loginUser flow is enclosed in try/catch, failing with a single generic authentication error on any auth issue. On success, it sanitizes the user, fetches app settings, issues a token, and returns { user, token }. TranslationService now reads locale files from src/locales instead of locales. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UserService
participant UserRepo
participant PasswordHasher
participant SettingsService
participant TokenService
Client->>UserService: login(email, password)
UserService->>UserRepo: findByEmail(email)
UserRepo-->>UserService: user | null
alt user found
UserService->>PasswordHasher: verify(password, user.hash)
PasswordHasher-->>UserService: match | no match
alt match
UserService->>SettingsService: fetchAppSettings()
SettingsService-->>UserService: settings
UserService->>TokenService: issueToken(sanitizedUser)
TokenService-->>UserService: token
UserService-->>Client: { user(with avatar), token }
else no match
UserService-->>Client: throw GenericAuthError
end
else user not found
UserService-->>Client: throw GenericAuthError
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
P.S. Canadians apologize for errors; Americans call them “beta features.” 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used🪛 GitHub Actions: Format Check (Client & Server)server/src/service/business/userService.js[warning] 1-1: Prettier formatting issues detected in this file. Run 'prettier --write' to fix. 🔇 Additional comments (6)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
gorkem-bwl
removed request for
ajhollid,
Owaiseimdad,
Br0wnHammer and
mohicody
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
Auto Pull Request Review from LlamaPReview
1. Overview
1.1 Core Changes
- Primary purpose and scope: Fixes critical login flow issues including raw localization key exposure, brittle frontend error handling, and security vulnerabilities that revealed user existence
- Key components modified:
- Backend:
userService.js(authentication logic),translationService.js(localization path) - Frontend:
Login/index.jsx(error handling)
- Backend:
- Cross-component impacts:
- Translation service changes affect all localized messages
- Security improvements impact authentication flow
- Business value alignment:
- Improves user experience with proper error messages
- Enhances security by preventing user enumeration
- Makes error handling more robust and extensible
1.2 Technical Architecture
- System design modifications:
- Changed translation file loading from
locales/tosrc/locales/ - Modified authentication flow to use generic error messages
- Changed translation file loading from
- Component interaction changes:
- Backend now returns consistent error messages for all auth failures
- Frontend relies on HTTP status codes instead of string matching
- Integration points impact:
- Translation service path change affects all localized error messages
- Security improvements prevent user enumeration attacks
- Dependency changes and implications:
- No new dependencies added
- Existing dependencies (logger, filesystem) usage remains consistent
2. Critical Findings
2.1 Must Fix (P0🔴)
Issue: Null User Handling Vulnerability in userService.js
- Analysis Confidence: High
- Impact: If
getUserByEmail()returns null, callinguser.comparePassword()will throw a TypeError, crashing the authentication process and potentially the entire service. This creates both a security vulnerability and availability risk. - Resolution: Add explicit null check before password comparison:
const user = await this.db.userModule.getUserByEmail(email);
if (!user) {
throw this.errorService.createAuthenticationError(this.stringService.authIncorrectPassword);
}Issue: Overly Broad Error Handling in userService.js
- Analysis Confidence: High
- Impact: Current catch-block masks all error types (including database failures), losing critical diagnostic information while maintaining security posture. This prevents proper monitoring and troubleshooting.
- Resolution: Preserve original error context while maintaining security:
} catch (error) {
this.logger.error('Login failed', {
email,
error: error.message,
stack: error.stack
});
throw this.errorService.createAuthenticationError(this.stringService.authIncorrectPassword);
}2.2 Should Fix (P1🟡)
Issue: Hardcoded Translation Path in translationService.js
- Analysis Confidence: High
- Impact: Hardcoded path reduces deployment flexibility and may fail in containerized environments or different deployment setups.
- Suggested Solution: Make path configurable via environment variable:
// In config/service.config.js
const LOCALES_PATH = process.env.LOCALES_PATH || 'src/locales';
// In translationService.js
this.localesDir = path.join(process.cwd(), LOCALES_PATH);Issue: Inconsistent Error Handling Pattern
- Analysis Confidence: High
- Impact:
registerUserlogs errors whileloginUserdoes not, creating inconsistent behavior that could complicate troubleshooting. - Suggested Solution: Standardize error handling pattern across both methods to ensure consistent logging and monitoring.
2.3 Consider (P2🟢)
Area: Structured Error Codes for Frontend
- Analysis Confidence: Medium
- Improvement Opportunity: Current implementation returns raw error messages which could lead to potential XSS vulnerabilities and lacks machine-readable error codes. Implementing structured error responses would enable better frontend handling and internationalization.
Area: Unit Test Coverage
- Analysis Confidence: High
- Improvement Opportunity: While manual testing was performed, adding unit tests for critical scenarios (null user, database failures, missing translations) would improve long-term maintainability.
Area: Password Removal Logic
- Analysis Confidence: Medium
- Improvement Opportunity: The comment "Should this be abstracted to DB layer?" suggests this logic might be better placed in the database layer for consistency and reusability.
2.4 Summary of Action Items
-
P0 Issues (Blockers):
- Add null check before password comparison in
userService.js - Implement proper error logging in catch-block while maintaining security
- Add null check before password comparison in
-
P1 Improvements (High Priority):
- Make translation path configurable via environment variable
- Standardize error handling pattern across authentication methods
-
P2 Considerations (Future Improvements):
- Implement structured error codes for frontend
- Add comprehensive unit tests for edge cases
- Consider moving password removal logic to DB layer
3. Technical Analysis
3.1 Code Logic Analysis
📁 server/src/service/business/userService.js - loginUser method
- Submitted PR Code:
loginUser = async (email, password) => {
try {
const user = await this.db.userModule.getUserByEmail(email);
const match = await user.comparePassword(password);
if (match !== true) {
throw this.errorService.createAuthenticationError(this.stringService.authIncorrectPassword);
}
// ... success path
} catch (error) {
throw this.errorService.createAuthenticationError(this.stringService.authIncorrectPassword);
}
};- Analysis:
- Current logic properly handles password mismatch cases and returns generic error messages for security
- Critical Issues:
- No null check for
userbefore callingcomparePassword()- will crash if user not found - Overly broad catch-block masks all error types including database failures
- No error logging preserves security but loses diagnostic information
- No null check for
- Security Improvements:
- Generic error message prevents user enumeration
- Consistent error handling for all failure scenarios
- LlamaPReview Suggested Improvements:
loginUser = async (email, password) => {
try {
const user = await this.db.userModule.getUserByEmail(email);
if (!user) {
throw this.errorService.createAuthenticationError(this.stringService.authIncorrectPassword);
}
const match = await user.comparePassword(password);
if (match !== true) {
throw this.errorService.createAuthenticationError(this.stringService.authIncorrectPassword);
}
// ... success path
} catch (error) {
// Log original error for diagnostics while maintaining security
this.logger.error('Login failed', {
email,
error: error.message,
stack: error.stack
});
throw this.errorService.createAuthenticationError(this.stringService.authIncorrectPassword);
}
};- Improvement rationale:
- Technical benefits: Prevents crashes, enables monitoring, maintains security
- Business value: Improves reliability and troubleshooting capabilities
- Risk assessment: Minimal risk - maintains all existing security protections
📁 server/src/service/system/translationService.js - Constructor
- Submitted PR Code:
constructor(logger) {
this.logger = logger;
this.translations = {};
this._language = "en";
this.localesDir = path.join(process.cwd(), "src", "locales");
}- Analysis:
- Current logic fixes the immediate issue of incorrect translation path
- Potential Issues:
- Hardcoded path reduces deployment flexibility
- May fail in containerized environments or different setups
- Configuration:
- Simple and direct solution for the immediate problem
- LlamaPReview Suggested Improvements:
constructor(logger) {
this.logger = logger;
this.translations = {};
this._language = "en";
const LOCALES_PATH = process.env.LOCALES_PATH || 'src/locales';
this.localesDir = path.join(process.cwd(), LOCALES_PATH);
}- Improvement rationale:
- Technical benefits: Increases deployment flexibility
- Business value: Supports diverse deployment scenarios
- Risk assessment: Low risk - maintains backward compatibility
3.2 Key Quality Aspects
-
System scalability considerations:
- Changes don't affect horizontal scaling capabilities
- Translation files loaded once at service initialization (no performance impact)
-
Performance bottlenecks and optimizations:
- No performance bottlenecks introduced
- Error handling changes add minimal overhead (<1ms)
- Database operations remain unchanged
-
Testing strategy and coverage:
- Manual testing performed for happy paths and error scenarios
- Missing tests that should be added:
- Null user scenarios
- Database connection failures during login
- Missing translation files
- Invalid password for existing user
- HTTP 500 scenarios
-
Documentation needs:
- Excellent PR description explaining changes and impact
- Should add comments about security tradeoffs in the code
- Consider updating translation service documentation to reflect path changes
4. Overall Evaluation
-
Technical assessment:
- ✅ Excellent security hardening with generic error messages
- ✅ Clean frontend error handling improvements
- ✅ Precise resolution of the reported issues
⚠️ Critical null-check missing before password comparison⚠️ Error logging needed for production diagnostics
-
Business impact:
- ✅ Fixes user-facing error messages improving UX
- ✅ Enhances security by preventing user enumeration
- ✅ Makes error handling more robust and extensible
- ✅ Resolves translation system issues affecting all localized messages
-
Risk evaluation:
- High Risk: Null user crash vulnerability (must fix before merge)
- Medium Risk: Lost error context affects production troubleshooting
- Medium Risk: Hardcoded path may cause deployment issues
- Low Risk: All other changes are well-contained and tested
-
Notable positive aspects and good practices:
- Excellent PR description with clear problem statement and testing results
- Proper security considerations with generic error messages
- Clean separation of concerns in the changes
- Maintains backward compatibility with existing API contracts
-
Implementation quality:
- High quality implementation for the core fixes
- Missing some defensive programming practices (null checks)
- Could benefit from more comprehensive testing
-
Final recommendation: Request Changes
- The P0 issues (null check and error logging) must be addressed before merging
- The P1 improvements (configurable path and consistent error handling) are highly recommended
- Once these are addressed, the PR will be ready for merging as it provides significant security and UX improvements
💡 LlamaPReview Community
Have feedback on this AI Code review tool? Join our GitHub Discussions to share your thoughts and help shape the future of LlamaPReview.
Summary
Fixes critical login flow issue where backend was returning raw localization keys (
authIncorrectPassword) instead of proper user-facing error messages. This issue made error handling brittle and caused poor user experience during authentication failures.Problem Description
Changes Made
Backend Fixes
Fixed translation service path (
translationService.js):locales/tosrc/locales/directoryImproved login security (
userService.js):Frontend Fixes
Login/index.jsx):=== "Incorrect password")Testing Results
✅ Before Fix: Backend returned
"authIncorrectPassword"(raw key)✅ After Fix: Backend returns
"Incorrect password"(translated message)Impact
Fixes #2768
Summary by CodeRabbit