Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: reset password email #339

Merged
merged 1 commit into from
Nov 14, 2024
Merged

feat: reset password email #339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions sample.env
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,14 @@ CORS_METHODS=GET,POST,PUT,PATCH,DELETE
CORS_HEADERS=Content-Type,Authorization
CORS_CREDENTIALS=true

# 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
Comment on lines +44 to +46
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
# 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


# The prefix of the password reset link in the email
# This prefix will be appended to FRONTEND_BASE_URL
PASSWORD_RESET_PREFIX=/account/recover/password/verify?token=

# additionally setup the following if you want to use docker-compose
# to setup environment
POSTGRES_DB=${DB_NAME}
Expand Down
4 changes: 4 additions & 0 deletions src/common/config/configuration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ export default () => {
// expiresIn: process.env.JWT_EXPIRES_IN,
},
cookieBasePath: process.env.COOKIE_BASE_PATH || '/',
frontendBaseUrl: process.env.FRONTEND_BASE_URL || '',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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 || '';
})(),

passwordResetPath:
process.env.PASSWORD_RESET_PREFIX ||
'/account/recover/password/verify?token=',
Comment on lines +29 to +32
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve URL construction safety

The current implementation might lead to URL construction issues. Consider:

  1. Adding URL path joining logic to handle trailing slashes
  2. Making the token parameter name configurable
  3. 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',
  },
});

};
};

Expand Down
2 changes: 2 additions & 0 deletions src/email/email.module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { Module } from '@nestjs/common';
import { join } from 'path';
import { EmailRuleService } from './email-rule.service';
import { EmailService } from './email.service';
import { ConfigModule } from '@nestjs/config';

@Module({
imports: [
Expand Down Expand Up @@ -40,6 +41,7 @@ import { EmailService } from './email.service';
},
},
}),
ConfigModule,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

],
providers: [EmailService, EmailRuleService],
exports: [EmailService, EmailRuleService],
Expand Down
7 changes: 6 additions & 1 deletion src/email/email.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,14 @@
import { MailerService } from '@nestjs-modules/mailer';
import { Injectable } from '@nestjs/common';
import { EmailRuleService } from './email-rule.service';
import { ConfigService } from '@nestjs/config';

@Injectable()
export class EmailService {
constructor(
private readonly mailerService: MailerService,
private readonly emailRuleService: EmailRuleService,
private readonly configService: ConfigService,
) {}

async sendPasswordResetEmail(
Expand All @@ -30,7 +32,10 @@ export class EmailService {
template: './password-reset.english.hbs',
context: {
username,
token,
resetUrl:
this.configService.get('frontendBaseUrl') +
this.configService.get('passwordResetPath') +
token,
Comment on lines +35 to +38
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

URL construction needs improvement but with fallback values

The review's concerns are valid but can be refined based on the codebase evidence:

  1. Config values have fallback defaults:
    • frontendBaseUrl defaults to empty string
    • passwordResetPath defaults 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:

  1. No validation of config values
  2. No handling of trailing/leading slashes
  3. No error handling for undefined config values
  4. 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 getOrThrow to 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

},
});
}
Expand Down
13 changes: 2 additions & 11 deletions src/resources/email-templates/password-reset.english.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,3 @@
<p>Hello, {{ username }}. You are trying to reset your password.</p>
<p>
Here should be a link, so that if you click it,
you will be directed to a page to reset your password.
</p>
<p>
Unfortunatelly, it is not implemented, but here is your password reset token: {{ token }}.
</p>
<p>
Use postman to post a request to (backend)/users/recover/password/verify with body {"token": "{{token}}", "new_password": "&lt;new password&gt;"}.
</p>
<p>I'm sure you are a super great developer, so you know how to do it.</p>
<p>Please use the following link to reset your password:</p>
<p><a href="{{ resetUrl }}">{{ resetUrl }}</a></p>