feat: reset password email #339
Conversation
WalkthroughThis pull request introduces new environment variables to the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #339 +/- ##
=======================================
Coverage 91.48% 91.49%
=======================================
Files 71 71
Lines 2736 2739 +3
Branches 379 384 +5
=======================================
+ Hits 2503 2506 +3
Misses 233 233 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
src/resources/email-templates/password-reset.english.hbs (2)
1-3: Add security notices and token expiration information.While the template is clear and user-friendly, consider adding important security-related information:
<p>Hello, {{ username }}. You are trying to reset your password.</p> <p>Please use the following link to reset your password:</p> <p><a href="{{ resetUrl }}">{{ resetUrl }}</a></p> +<p>This link will expire in {{ expirationTime }} minutes.</p> +<p>For your security:</p> +<ul> + <li>Never share this link with anyone</li> + <li>Make sure you're on our official website before entering your new password</li> + <li>We will never ask for your password via email</li> +</ul>
1-3: Add support contact information.Consider adding contact information in case users encounter issues:
<p>Hello, {{ username }}. You are trying to reset your password.</p> <p>Please use the following link to reset your password:</p> <p><a href="{{ resetUrl }}">{{ resetUrl }}</a></p> +<p>If you didn't request this password reset or need assistance, please contact our support team at {{ supportEmail }}.</p>src/common/config/configuration.ts (1)
29-32: Document the configuration requirementsConsider adding JSDoc comments to document:
- The expected format of the frontend URL
- The purpose of these configuration values
- The complete reset URL structure
+ /** + * Base URL of the frontend application where users will be redirected + * @example 'https://example.com' + */ frontendBaseUrl: process.env.FRONTEND_BASE_URL || '', + /** + * Path and query parameter structure for password reset URLs + * The final URL will be: ${frontendBaseUrl}${passwordResetPath}${token} + * @example '/account/recover/password/verify?token=' + */ passwordResetPath: process.env.PASSWORD_RESET_PREFIX || '/account/recover/password/verify?token=',src/email/email.service.ts (1)
Line range hint
23-41: Consider additional security measures for password resetWhile the basic functionality is implemented, consider these security enhancements:
- Add rate limiting for password reset emails
- Log password reset attempts (without tokens) for security monitoring
- Consider adding email template versioning for audit purposes
Would you like me to provide implementation examples for these security enhancements?
sample.env (1)
48-50: Review password reset token security considerationsExposing reset tokens in URLs can pose security risks:
- Tokens may be logged in browser history
- Tokens can appear in referrer headers when clicking links
- URLs might be inadvertently shared or cached
Consider these security improvements:
- Use POST requests with tokens in the request body instead of URL parameters
- Add comments about implementing token expiration
- Document one-time-use token requirements
Would you like assistance in implementing these security enhancements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
sample.env(1 hunks)src/common/config/configuration.ts(1 hunks)src/email/email.module.ts(2 hunks)src/email/email.service.ts(2 hunks)src/resources/email-templates/password-reset.english.hbs(1 hunks)
🔇 Additional comments (3)
src/common/config/configuration.ts (1)
29-32: Verify the configuration usage across the codebase
Let's verify how these new configuration values are being used to construct the password reset URLs.
✅ Verification successful
Let me check the implementation details of the email service to verify how these configurations are being used.
Configuration values are properly used in password reset URL construction
The configuration values are correctly utilized in the EmailService.sendPasswordResetEmail method to construct the password reset URL. The implementation:
- Properly injects the
ConfigService - Uses
configService.get()to retrieve bothfrontendBaseUrlandpasswordResetPath - Concatenates them with the token to create the complete reset URL
- No direct usage of environment variables was found elsewhere in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usage of these configuration values in email services or URL construction
# Look for URL construction using these config values
rg -l "frontendBaseUrl|passwordResetPath" --type ts
# Look for potential direct env var usage that should use config instead
rg -l "FRONTEND_BASE_URL|PASSWORD_RESET_PREFIX" --type ts
# Look for email service implementations
ast-grep --pattern 'class $_ implements EmailService {
$$$
sendPasswordResetEmail($$$) {
$$$
}
$$$
}'
Length of output: 326
Script:
#!/bin/bash
# Check the implementation of email.service.ts to see how these configs are used
cat src/email/email.service.ts
Length of output: 1496
src/email/email.service.ts (1)
13-13: LGTM: Clean ConfigService integration
The ConfigService is properly imported and injected following NestJS best practices.
Also applies to: 20-20
src/email/email.module.ts (1)
15-15: LGTM!
The import of ConfigModule is correctly added to support configuration injection in the EmailService.
| frontendBaseUrl: process.env.FRONTEND_BASE_URL || '', | ||
| passwordResetPath: | ||
| process.env.PASSWORD_RESET_PREFIX || | ||
| '/account/recover/password/verify?token=', |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve URL construction safety
The current implementation might lead to URL construction issues. Consider:
- Adding URL path joining logic to handle trailing slashes
- Making the token parameter name configurable
- Adding TypeScript interfaces for type safety
// Add at the top of the file
interface AppConfig {
port: number;
elasticsearch: ElasticsearchModuleOptions;
jwt: {
secret: string;
};
cookieBasePath: string;
frontendBaseUrl: string;
passwordReset: {
path: string;
tokenParam: string;
};
}
// Modified configuration
export default (): AppConfig => ({
// ... other config
frontendBaseUrl: process.env.FRONTEND_BASE_URL?.replace(/\/$/, '') || '',
passwordReset: {
path: process.env.PASSWORD_RESET_PATH || '/account/recover/password/verify',
tokenParam: process.env.PASSWORD_RESET_TOKEN_PARAM || 'token',
},
});| @@ -26,6 +26,10 @@ export default () => { | |||
| // expiresIn: process.env.JWT_EXPIRES_IN, | |||
| }, | |||
| cookieBasePath: process.env.COOKIE_BASE_PATH || '/', | |||
| frontendBaseUrl: process.env.FRONTEND_BASE_URL || '', | |||
There was a problem hiding this comment.
Add validation for frontendBaseUrl
An empty string default for frontendBaseUrl could lead to invalid URLs being constructed. Consider adding validation and throwing a configuration error if the URL is invalid or empty in production.
- frontendBaseUrl: process.env.FRONTEND_BASE_URL || '',
+ frontendBaseUrl: (() => {
+ const url = process.env.FRONTEND_BASE_URL;
+ if (process.env.NODE_ENV === 'production' && (!url || !url.startsWith('http'))) {
+ throw new Error('FRONTEND_BASE_URL must be a valid URL in production');
+ }
+ return url || '';
+ })(),📝 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.
| frontendBaseUrl: process.env.FRONTEND_BASE_URL || '', | |
| frontendBaseUrl: (() => { | |
| const url = process.env.FRONTEND_BASE_URL; | |
| if (process.env.NODE_ENV === 'production' && (!url || !url.startsWith('http'))) { | |
| throw new Error('FRONTEND_BASE_URL must be a valid URL in production'); | |
| } | |
| return url || ''; | |
| })(), |
| resetUrl: | ||
| this.configService.get('frontendBaseUrl') + | ||
| this.configService.get('passwordResetPath') + | ||
| token, |
There was a problem hiding this comment.
💡 Codebase verification
URL construction needs improvement but with fallback values
The review's concerns are valid but can be refined based on the codebase evidence:
- Config values have fallback defaults:
frontendBaseUrldefaults to empty stringpasswordResetPathdefaults to '/account/recover/password/verify?token='
The suggested improvements are still valuable but can be simplified:
private buildResetUrl(token: string): string {
const baseUrl = this.configService.get<string>('frontendBaseUrl');
const resetPath = this.configService.get<string>('passwordResetPath');
// Ensure clean URL parts
const cleanBase = baseUrl.replace(/\/+$/, '') || '';
const cleanPath = resetPath.replace(/^\/+|\/+$/g, '') || 'account/recover/password/verify';
return `${cleanBase}/${cleanPath}?token=${encodeURIComponent(token)}`;
}This implementation:
- Maintains existing fallback behavior
- Handles URL slashes
- Properly encodes the token
- Keeps the URL query parameter structure from the default config
🔗 Analysis chain
URL construction needs validation and error handling
The current URL construction has several potential issues:
- No validation of config values
- No handling of trailing/leading slashes
- No error handling for undefined config values
- Direct concatenation could create malformed URLs
Consider this safer implementation:
- resetUrl:
- this.configService.get('frontendBaseUrl') +
- this.configService.get('passwordResetPath') +
- token,
+ resetUrl: this.buildResetUrl(token),
+ private buildResetUrl(token: string): string {
+ const baseUrl = this.configService.getOrThrow<string>('frontendBaseUrl');
+ const resetPath = this.configService.getOrThrow<string>('passwordResetPath');
+
+ // Ensure clean URL parts
+ const cleanBase = baseUrl.replace(/\/+$/, '');
+ const cleanPath = resetPath.replace(/^\/+|\/+$/g, '');
+
+ try {
+ const url = new URL(`${cleanBase}/${cleanPath}/${token}`);
+ return url.toString();
+ } catch (error) {
+ throw new Error(`Failed to construct reset URL: ${error.message}`);
+ }
+ }This implementation:
- Uses
getOrThrowto fail fast if config is missing - Properly handles URL slashes
- Validates URL construction
- Provides clear error messages
Let's verify the config setup:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the required config values are properly defined
rg -l 'frontendBaseUrl|passwordResetPath' --type ts
Length of output: 115
Script:
#!/bin/bash
# Check how these config values are defined and used
rg 'frontendBaseUrl|passwordResetPath' -A 3 -B 3 --type ts
Length of output: 1046
| @@ -40,6 +41,7 @@ import { EmailService } from './email.service'; | |||
| }, | |||
| }, | |||
| }), | |||
| ConfigModule, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use ConfigModule.forFeature() for better configuration management
The current implementation imports ConfigModule directly, which might lead to configuration initialization issues. In NestJS, feature modules should use ConfigModule.forFeature() to properly scope their configuration.
Apply this diff:
- ConfigModule,
+ ConfigModule.forFeature(() => ({
+ // Validate and expose only email-related config
+ PASSWORD_RESET_PREFIX: process.env.PASSWORD_RESET_PREFIX,
+ FRONTEND_BASE_URL: process.env.FRONTEND_BASE_URL,
+ // Add other email-specific config keys here
+ })),This change:
- Explicitly declares which configuration keys are used by the EmailModule
- Provides better encapsulation and maintainability
- Enables configuration validation at the module level
Committable suggestion skipped: line range outside the PR's diff.
| # This url means the frontend url, usually it is the same as the CORS_ORIGINS | ||
| # It is used to send the password reset email | ||
| FRONTEND_BASE_URL=http://localhost:3000 |
There was a problem hiding this comment.
Consider security implications of using http in the default URL
While http://localhost:3000 is acceptable for local development, it's crucial to use HTTPS in production environments to prevent password reset tokens from being intercepted.
Consider adding a comment to warn about production security requirements:
# This url means the frontend url, usually it is the same as the CORS_ORIGINS
# It is used to send the password reset email
+# IMPORTANT: Use https:// URLs in production environments
FRONTEND_BASE_URL=http://localhost:3000📝 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 url means the frontend url, usually it is the same as the CORS_ORIGINS | |
| # It is used to send the password reset email | |
| FRONTEND_BASE_URL=http://localhost:3000 | |
| # This url means the frontend url, usually it is the same as the CORS_ORIGINS | |
| # It is used to send the password reset email | |
| # IMPORTANT: Use https:// URLs in production environments | |
| FRONTEND_BASE_URL=http://localhost:3000 |
No description provided.