Develop by predator-bites · Pull Request #244 · mate-academy/node_auth-app

predator-bites · 2026-03-01T21:49:51Z

No description provided.

mateacademy-ai-mentor

Review Summary

This is a great effort, and the overall structure of your application is solid. However, I'm requesting a few changes to address some critical security and functional requirements before I can approve it.

Here are the key issues to address:

Security Vulnerability in Profile Updates: In src/controllers/users.controller.ts, the changeSensetive function identifies the user to update by the email in the request body. This is a critical security flaw that could allow an authenticated user to change another user's account details. Please modify this to use the ID from the authenticated user's session (req.normalizedUser).
Missing Password Confirmation: The requirements state that changing a password (both from the profile and during a password reset) must include a confirmation field that matches the new password. This validation is currently missing from the changePassword and changeSensetive functions in src/controllers/users.controller.ts.
Unprotected Logout Route: The /logout route in src/routes/users.routes.ts should only be accessible to authenticated users. Please add the authMiddleware to this route to protect it.

Once you've addressed these points, your project will be in excellent shape. Keep up the good work!

✨ 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 · 2026-03-01T21:53:06Z

src/controllers/tokens.controller.ts

+  }
+
+  const accessToken = jws.generateAccessToken(
+    UsersRepository.normalize(normalizedUser)


The UsersRepository.normalize function is designed to work with User objects retrieved from the database, but here it's being called with a decoded JWT payload. While this may work at runtime if the properties match, it's not type-safe and misuses the function's intended purpose. A better practice would be to create a new NormalizedUser object by explicitly picking the id, name, and email properties from the decoded token payload.

mateacademy-ai-mentor · 2026-03-01T21:53:06Z

src/controllers/users.controller.ts

+    }]);
+  }
+
+  if (name && !validateEmail(email) && !validatePassword(password) && !user) {


The validation checks within this if statement are redundant. The email, password, and user existence have already been validated in the preceding lines. If any of those checks had failed, an error would have been thrown, and this line would not be reached. You can simplify this condition.

mateacademy-ai-mentor · 2026-03-01T21:53:06Z

src/controllers/users.controller.ts

+
+const changeSensetive = async (req: Request, res: Response) => {
+  const loginData: LoginData | null = req.body.loginData || null;
+  const toChange: UserPropsToUpdate | null = req.body.toChange || null;


According to the task requirements, changing a password from the profile page requires an old password, a new password, and a confirmation. This implementation is missing the logic to check for a confirmation field that matches the new password within the toChange object.

mateacademy-ai-mentor · 2026-03-01T21:53:06Z

src/controllers/users.controller.ts

+    }]);
+  }
+
+  const user = await userRepository.getByEmail(loginData.email) || null;


This line introduces a security risk. The user to be updated is fetched based on the email from the request body (loginData.email), not from the currently authenticated user's session (req.normalizedUser). This means an authenticated user could change another user's account details if they know their password. You should always use the ID from req.normalizedUser to identify which user's data to modify.

mateacademy-ai-mentor · 2026-03-01T21:53:06Z

src/controllers/users.controller.ts

+}
+
+const changePassword = async (req: Request, res: Response) => {
+  const { email, passwordToken, password } = req.body;


The task requirements state that the password reset confirmation page must have password and confirmation fields that are equal. This function only accepts a password from the request body and is missing the required confirmation field and the logic to validate it.

mateacademy-ai-mentor · 2026-03-01T21:53:06Z

src/routes/users.routes.ts

+
+router.get('/profile', authMiddleware, catchAsync(userController.profile));
+router.get('/activate/:email/:activationToken', catchAsync(userController.activate));
+router.post('/logout', catchAsync(userController.logout))


According to the task requirements, the logout feature should only be available to authenticated users. This route is missing the authMiddleware to ensure it is protected.

mateacademy-ai-mentor

Review Summary

Great job on addressing the previous security feedback and protecting the logout route! Those are excellent improvements. However, this submission requires changes because there are a couple of critical issues that need to be resolved before it can be approved.

First, there's a significant security vulnerability:

Unhashed Passwords: In src/controllers/users.controller.ts, within the changeSensetive function, the new password is being saved directly to the database without being hashed. All passwords must be hashed using bcrypt before storage to protect user data.

Second, a key functional requirement is still missing:

Password Confirmation: The task requires that changing a password (both in the user profile via changeSensetive and during a password reset via changePassword) must include a confirmation field that matches the new password. This validation is currently missing from both flows.

Please address these two critical points. I've also left a few minor comments in the review regarding improving HTTP status codes and cleaning up some redundant code.

✨ 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 · 2026-03-01T22:03:23Z

src/controllers/tokens.controller.ts

+  }
+
+  const accessToken = jws.generateAccessToken(
+    UsersRepository.normalize(normalizedUser)


The UsersRepository.normalize function is designed to work with a Prisma User database object, but here it's being passed a decoded JWT payload. While this might work at runtime due to JavaScript's nature, it's not type-safe and is a misuse of the function.

A better approach is to manually construct a new, clean payload object for the access token using the properties from the decoded refresh token. For example:

const payload = { id: normalizedUser.id, name: normalizedUser.name, email: normalizedUser.email, }; const accessToken = jws.generateAccessToken(payload);

mateacademy-ai-mentor · 2026-03-01T22:03:23Z

src/controllers/users.controller.ts

+
+  res.clearCookie('refreshToken');
+
+  const deleted = await tokenRepository.deleteToken(refreshToken);


This deleteToken call will cause the request to fail with a 500 error if the refreshToken in the cookie is invalid or not found in the database. A logout function should be robust and succeed even in such cases. The primary goal is to log the user out from the client's perspective, which is achieved by clearing the cookie.

Consider handling potential errors here, for example, with a try...catch block, to prevent the server from crashing on a routine logout.

mateacademy-ai-mentor · 2026-03-01T22:03:23Z

src/controllers/users.controller.ts

+  if (!deleted) {
+    throw ApiError.notFound([{
+      message: 'Account not found'
+    }]);


This check is currently unreachable. The deleteToken function uses prisma.delete, which throws an exception if the token is not found, rather than returning a falsy value. The function execution will stop at the line above, and this condition will never be evaluated.

mateacademy-ai-mentor · 2026-03-01T22:03:23Z

src/controllers/users.controller.ts

+    }])
+  }
+
+  const updatedUser = await userRepository.change(user.id, toChange);


There are two issues here related to changing the password:

Missing Confirmation: The task requires that changing a password includes a confirmation field that must match the new password. This validation is still missing from this function.

Security Vulnerability: If the toChange object contains a password, it is being passed directly to the database update, which means the new password would be stored as plain text. All passwords must be hashed before being saved. You can reuse the hashing logic from the create function.

mateacademy-ai-mentor · 2026-03-01T22:03:23Z

src/controllers/users.controller.ts

+  const { email, passwordToken, password } = req.body;
+
+  if (!email || !passwordToken || !password ) {
+    throw ApiError.badRequest([{ message: 'Bad request'}]);


The task requirements for the password reset flow specify that the confirmation page must have password and confirmation fields that are equal. This function needs to be updated to read a confirmation field from the request body and throw an error if it doesn't match the password field.

mateacademy-ai-mentor · 2026-03-01T22:03:23Z

src/middlewares/authMiddleware.ts

+    throw ApiError.badRequest([
+      {
+        message: 'Bad access token',
+      },
+    ]);


When the access token is invalid or expired, it represents an authentication failure. The standard HTTP status code for this is 401 Unauthorized. Using ApiError.authError here instead of ApiError.badRequest would be more appropriate and consistent with the check for a missing token on line 14.

mateacademy-ai-mentor · 2026-03-01T22:03:23Z

tsconfig.json

+
+    // Recommended Options
+    "strict": true,
+    "jsx": "react-jsx",


This option is for configuring JSX syntax, which is used by frontend libraries like React. Since this is a backend-only Express application, this setting is not necessary and can be safely removed to make the configuration cleaner and more focused.

mateacademy-ai-mentor

Review Summary

Excellent work on hashing the passwords upon update—that's a critical security fix and a fantastic improvement! I am approving this submission as an educational override to help you progress, but there is one mandatory requirement from the task that still needs to be addressed.

The application is missing the password confirmation validation. In src/controllers/users.controller.ts, both the changeSensetive and changePassword functions must check that the new password from the request body matches a confirmation field. This is crucial to prevent users from accidentally mistyping their new password.

I've also left a few minor suggestions in the comments to improve your API's adherence to web standards, such as using more conventional HTTP status codes (401 for auth errors, 404 for not found, etc.). You've built a very solid and well-structured application here. Keep up the great work!

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

Rate AI review example ↩

mateacademy-ai-mentor · 2026-03-01T22:12:51Z

src/controllers/tokens.controller.ts

+    UsersRepository.normalize(normalizedUser)
+  );
+
+  res.statusCode = 201;


While a 201 Created status isn't strictly incorrect as a new token is created, the more conventional HTTP status code for a successful token refresh operation is 200 OK. Consider using res.status(200) for better adherence to web standards.

mateacademy-ai-mentor · 2026-03-01T22:12:51Z

src/controllers/users.controller.ts

+
+  await mailer.sendMail(email, html);
+
+  res.statusCode = 200;


When creating a new resource like a user, the more semantically correct HTTP status code is 201 Created. Consider changing this to res.status(201).

mateacademy-ai-mentor · 2026-03-01T22:12:51Z

src/controllers/users.controller.ts

+  if (!user) {
+    throw ApiError.badRequest(


If the user is not found by their email, a 404 Not Found status code would be more appropriate than a 400 Bad Request. Consider using ApiError.notFound here.

mateacademy-ai-mentor · 2026-03-01T22:12:51Z

src/controllers/users.controller.ts

+
+  const updatedToChange = { ...toChange }
+
+  if (Object.keys(updatedToChange).includes('password')) {


According to the task requirements, changing a password requires a new password and a confirmation field. Before hashing and saving the new password, you need to add a check here to ensure that toChange.password is equal to a toChange.confirmation field from the request body. This was also a critical point from the last review.

mateacademy-ai-mentor · 2026-03-01T22:12:51Z

src/controllers/users.controller.ts

+  const { email, passwordToken, password } = req.body;
+
+  if (!email || !passwordToken || !password ) {


The task requirements specify that the password reset confirmation page must include password and confirmation fields that are equal. You need to read a confirmation field from req.body and validate that it matches the password field before proceeding to update the user's password.

mateacademy-ai-mentor · 2026-03-01T22:12:51Z

src/middlewares/authMiddleware.ts

@@ -0,0 +1,35 @@
+import type { NextFunction, Request, Response } from 'express';
+// import { ApiError } from '../types/index.js';


This commented-out line is no longer needed and can be safely removed to keep the code clean.

mateacademy-ai-mentor · 2026-03-01T22:12:51Z

src/middlewares/authMiddleware.ts

+    throw ApiError.badRequest([
+      {
+        message: 'Bad access token',
+      },
+    ]);


While 400 Bad Request is not entirely wrong, a 401 Unauthorized status code is more appropriate here. A 401 error indicates that the request lacks valid authentication credentials for the target resource. Consider using ApiError.authError for consistency and to provide a more accurate response to the client.

predator-bites added 2 commits March 1, 2026 23:42

created

66f1449

task is completed

9f07788

mateacademy-ai-mentor suggested changes Mar 1, 2026

View reviewed changes

made changes, confirmation password is validated on frontend

eebe0cc

predator-bites requested a review from mateacademy-ai-mentor March 1, 2026 21:59

mateacademy-ai-mentor suggested changes Mar 1, 2026

View reviewed changes

added hashing for changeSensetive

7f1e8bc

predator-bites requested a review from mateacademy-ai-mentor March 1, 2026 22:10

mateacademy-ai-mentor approved these changes Mar 1, 2026

View reviewed changes

predator-bites added 4 commits March 2, 2026 13:26

changed build command

2d20750

update

e1bbfd9

deleted .env

a14d815

solved

a6f26c3


		res.clearCookie('refreshToken');

		const deleted = await tokenRepository.deleteToken(refreshToken);


		const updatedToChange = { ...toChange }

		if (Object.keys(updatedToChange).includes('password')) {

		const { email, passwordToken, password } = req.body;

		if (!email \|\| !passwordToken \|\| !password ) {

		@@ -0,0 +1,35 @@
		import type { NextFunction, Request, Response } from 'express';
		// import { ApiError } from '../types/index.js';

Conversation

predator-bites commented Mar 1, 2026

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants