node_auth-app #204

MRKucherenko · 2025-12-01T18:10:34Z

No description provided.

mateacademy-ai-mentor

Review Summary

Great work on the overall structure of the application! The separation of concerns is well-implemented. However, there are a couple of critical security vulnerabilities that need to be addressed before this can be approved.

Here are the required changes:

Password Reset Logic: In src/controllers/auth.controller.js, the query to find a user for a password reset is incorrect. It finds the first user with an unexpired token, not the specific user associated with the provided resetPasswordToken. This could allow one user to reset another's password. You must update the query to find the user by the token itself.
Email Change Vulnerability: In src/controllers/user.controller.js, the new email address is taken directly from the URL parameters (req.params.newEmail). This is insecure, as an attacker could modify the link to hijack an account. The new email should be stored securely on the server when the confirmation token is generated and retrieved using only that token.

These changes are crucial for ensuring the application is secure and functions correctly. Once they are implemented, I'll be happy to review your work again.

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button² in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag³.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

mateacademy-ai-mentor · 2025-12-01T18:13:08Z

src/controllers/user.controller.js

+};
+
+const updateUserEmail = async (req, res) => {
+  const { newEmail, activationEmailToken } = req.params;


This approach of taking newEmail from the URL parameters is insecure. A malicious user could intercept the activation link, replace the email address with their own, and hijack the account. The new email address should be verified against the token itself. A common pattern is to store the pending new email in the database when the token is generated and then retrieve it here using only the token, ignoring any email from the URL.

mateacademy-ai-mentor · 2025-12-01T18:13:08Z

src/controllers/auth.controller.js

+  const user = await User.findOne({
+    where: {
+      resetPasswordExpires: { [Op.gt]: Date.now() },
+    },
+  });


This query to find a user is incorrect. It fetches the first user with an unexpired resetPasswordExpires timestamp, which might not be the user associated with the provided resetPasswordToken. If multiple users request a password reset simultaneously, this could lead to incorrect behavior or security vulnerabilities. You need to find the specific user who owns the token.

mateacademy-ai-mentor · 2025-12-01T18:13:08Z

src/controllers/auth.controller.js

+const logout = async (req, res) => {
+  const { refreshToken } = req.cookies;
+
+  const userData = await jwtService.verifyRefresh(refreshToken);


The function jwtService.verifyRefresh is synchronous and does not return a Promise. The await keyword here has no effect and should be removed to avoid confusion.

mateacademy-ai-mentor · 2025-12-01T18:13:09Z

src/index.js

@@ -1 +1,36 @@
 'use strict';
+'use strict';


This 'use strict'; directive is a duplicate. You only need to declare it once at the top of the file.

mateacademy-ai-mentor · 2025-12-01T18:13:09Z

src/index.js

+app.use(authRouter);
+app.use('/user', userRouter);
+
+app.use((req, res, nest) => {


There's a small typo here. The third parameter for Express middleware is conventionally named next, not nest.

mateacademy-ai-mentor · 2025-12-01T18:13:09Z

src/routers/auth.router.js

+authRouter.post('/auth/registration', catchError(authController.register));
+
+authRouter.get(
+  '/auth/activation/:email/:activationToken',


The :email parameter in this route is not used by the corresponding activation controller, which correctly relies only on the activationToken. Including unused parameters in the URL can be misleading. For better security and clarity, it's best practice to design activation links that depend solely on the unique, unguessable token. Consider changing the route to /auth/activation/:activationToken.

mateacademy-ai-mentor · 2025-12-01T18:13:09Z

src/services/email.service.js

+}
+
+function sendActivationEmail(email, token) {
+  const href = `${process.env.CLIENT_HOST}/activate/${email}/${token}`;


Including the user's email directly in the activation URL can be a security risk. It exposes personally identifiable information (PII) which could be logged by servers, proxies, or stored in browser history. It's better practice to design activation flows that rely only on the unique, unguessable token.

mateacademy-ai-mentor · 2025-12-01T18:13:09Z

src/services/email.service.js

+}
+
+function sendChangeEmailNotification(email, token) {
+  const href = `${process.env.CLIENT_HOST}/activate-email/${email}/${token}`;


Similar to the account activation link, this URL includes the new email address. This poses a security and privacy risk. The email change confirmation process should ideally rely only on the token to identify the user and the pending action.

mateacademy-ai-mentor

Review Summary

Great job on the overall structure and implementation of many features. However, this requires changes because a few critical security vulnerabilities, which were also highlighted in the previous review, remain unresolved.

Here are the issues that need to be fixed:

Critical Password Reset Vulnerability in src/controllers/auth.controller.js:

The query still finds the first user with any unexpired token, not the specific user matching the resetPasswordToken. Since the token is hashed, you need to fetch all users with an unexpired token and then loop through them to find the match using bcrypt.compare.
Critical Email Change Vulnerability in src/controllers/user.controller.js and src/routers/user.router.js:

The new email address is still being passed as a URL parameter (:newEmail). This is insecure as it allows an attacker to modify the link. The new email should be stored temporarily in the user's database record when the change is initiated and retrieved via the token during confirmation.
Incorrect Function Call in src/services/user.service.js:

The sendEmailChange function is calling sendChangeEmailNotification instead of sendChangeEmailConfirmation. This prevents the confirmation email from being sent to the new address, breaking the feature.

Please address these critical issues. Once they are resolved, your project will be in great shape!

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button² in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag³.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

mateacademy-ai-mentor · 2025-12-01T18:38:23Z

src/controllers/auth.controller.js

+  const user = await User.findOne({
+    where: {
+      resetPasswordExpires: { [Op.gt]: Date.now() },
+    },
+  });


This query is still incorrect and was the subject of a critical comment in the last review. It fetches the first user with an unexpired token, not necessarily the user who owns the resetPasswordToken provided in the request. This creates a security vulnerability.

Since you are using bcrypt to hash the token (which is good practice), you cannot query the database directly for a matching token. You should fetch all users with an unexpired token using User.findAll and then loop through the results to find the user whose resetPasswordToken matches the one provided using bcrypt.compare.

mateacademy-ai-mentor · 2025-12-01T18:38:24Z

src/controllers/user.controller.js

+};
+
+const updateUserEmail = async (req, res) => {
+  const { newEmail, activationEmailToken } = req.params;


This implementation still contains the critical security vulnerability mentioned in the previous review. The newEmail should not be taken from the URL parameters (req.params), as this can be tampered with by an attacker.

To fix this, you should:

When the user initiates an email change, store the pending new email address in a temporary field in the user's database record.

The confirmation link should only contain the activationEmailToken.

In this function, find the user by the token, retrieve the new email from the database record, and then perform the update.

mateacademy-ai-mentor · 2025-12-01T18:38:24Z

src/routers/user.router.js

+);
+
+userRouter.post(
+  '/activation_email/:newEmail/:activationEmailToken',


This route definition is insecure and directly relates to the critical feedback from the previous review. Including :newEmail in the URL allows an attacker to modify the link and hijack the user's account by changing it to their own email address.

The route should only contain the token, for example: /activation_email/:activationEmailToken. The new email address should be stored temporarily in the database when the change is initiated and retrieved using the token during confirmation.

mateacademy-ai-mentor · 2025-12-01T18:38:24Z

src/services/user.service.js

+  await emailService.sendChangeEmailNotification(
+    newEmail,
+    activationEmailToken,
+  );


This is calling the wrong function from the email service. sendChangeEmailNotification is intended to notify the old email address after a successful change. You should be calling sendChangeEmailConfirmation here to send the confirmation link to the newEmail.

mateacademy-ai-mentor

Review Summary

I am approving this submission as an educational override to help you progress, but please note the email change feature is not yet functional and has a security flaw.

Excellent work securing the password reset flow and building a solid application structure! However, to complete the email change feature, you'll need to make a few corrections:

Security: Remove the :newEmail parameter from the route in src/routers/user.router.js. The new email should be retrieved from the database via the token, not passed in the URL.
Functionality: The feature is broken because the User model (src/models/user.js) is missing the pendingNewEmail and activationEmailExpires fields, and src/controllers/user.controller.js is missing the Op import from Sequelize.
Logic: The sendEmailChange function in src/services/user.service.js is still calling the incorrect email service method.

Addressing these points will make your feature fully secure and operational. You're doing a fantastic job, keep it up!

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Rate AI review example ↩

mateacademy-ai-mentor · 2025-12-01T19:00:09Z

src/controllers/user.controller.js

+  user.pendingNewEmail = newEmail;
+  user.activationEmailToken = token;
+  user.activationEmailExpires = Date.now() + 60 * 60 * 1000;


The fields pendingNewEmail and activationEmailExpires (on line 177) are being assigned to the user object, but they are not defined in the User model (src/models/user.js). Sequelize will not save these properties to the database, which will cause the email update logic in updateUserEmail to fail. Please add these fields to your User model.

mateacademy-ai-mentor · 2025-12-01T19:00:09Z

src/controllers/user.controller.js

+  const user = await User.findOne({
+    where: {
+      activationEmailToken,
+      activationEmailExpires: { [Op.gt]: Date.now() },


Op is used here to query by date, but it has not been imported from sequelize. This will cause a ReferenceError at runtime. You should add import { Op } from 'sequelize'; at the top of the file.

mateacademy-ai-mentor · 2025-12-01T19:00:09Z

src/models/user.js

+  },
+  resetPasswordExpires: {
+    type: DataTypes.DATE,
+  },


This model is missing the pendingNewEmail (type: DataTypes.STRING) and activationEmailExpires (type: DataTypes.DATE) fields. These are assigned in user.controller.js (lines 175 and 177) and are essential for the email change confirmation process to work. Without defining them here, Sequelize will not save them to the database.

mateacademy-ai-mentor · 2025-12-01T19:00:09Z

src/routers/user.router.js

+);
+
+userRouter.post(
+  '/activation_email/:newEmail/:activationEmailToken',


This route still includes :newEmail as a URL parameter, which is a security vulnerability that was highlighted in the previous review. An attacker could potentially manipulate this URL. The new email address should be retrieved from the user's database record using the activationEmailToken, not passed in the URL. Please remove the :newEmail parameter from this route.

mateacademy-ai-mentor · 2025-12-01T19:00:09Z

src/services/user.service.js

+  await emailService.sendChangeEmailNotification(
+    newEmail,
+    activationEmailToken,
+  );


This function is still calling sendChangeEmailNotification. As mentioned in the previous review, it should be calling emailService.sendChangeEmailConfirmation to send the verification link to the user's new email address. The current implementation sends the wrong email and breaks the email change confirmation flow.

node_auth-app

da093d4

mateacademy-ai-mentor suggested changes Dec 1, 2025

View reviewed changes

node_auth-app

c42475d

MRKucherenko requested a review from mateacademy-ai-mentor December 1, 2025 18:35

mateacademy-ai-mentor suggested changes Dec 1, 2025

View reviewed changes

node_auth-app

417e4d3

MRKucherenko requested a review from mateacademy-ai-mentor December 1, 2025 18:57

mateacademy-ai-mentor approved these changes Dec 1, 2025

View reviewed changes

node_auth-app #204

Are you sure you want to change the base?

node_auth-app #204

Uh oh!

Conversation

MRKucherenko commented Dec 1, 2025

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants