develop #212
develop #212
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
You've set up a good structure for the application, but several critical requirements are missing, and there are some significant security issues that need to be addressed. I am requesting changes to align the project with the task description.
Here are the key areas to focus on:
- Missing Validations: The registration process is missing the
namefield. The password reset and profile update features (for both password and email) are missing the requiredconfirmationfield checks. - Security: User-specific routes in
user.route.jsincorrectly use an email from the URL to identify the user. This is a major security risk; you must use the user's identity from the validated authentication token instead. Similarly, thechangeDatafunction inuser.service.jsallows updating any field, which should be restricted to only safe fields likename. - Core Functionality: The application is missing the required 404 error handler for undefined routes. Also, the email service has an incorrect transporter configuration that will prevent emails from being sent.
Please address these points to ensure the application is secure and fully functional according to the requirements.
✨ Please rate this review in the Platform's notification center1. 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 button2 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 tag3.
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.
Footnotes
src/config/token.config.js
Outdated
|
|
||
| res.cookie('refreshToken', refreshToken, { | ||
| maxAge: 30 * 24 * 60 * 60 * 1000, | ||
| HttpOnly: true, |
There was a problem hiding this comment.
The property name for setting an HTTP-only cookie is case-sensitive and should be camel-cased. Please change HttpOnly to httpOnly to ensure the cookie is properly secured against client-side script access.
src/controllers/auth.controller.js
Outdated
| import { tokenConfig } from '../config/token.config.js'; | ||
|
|
||
| const register = async (req, res) => { | ||
| const { email, password } = req.body; |
There was a problem hiding this comment.
The task requirements state that registration should use name, email, and password. The name field is missing from the request body handling and the user creation logic.
src/controllers/auth.controller.js
Outdated
|
|
||
| const resetPassword = async (req, res) => { | ||
| const { email, resetToken } = req.params; | ||
| const { password } = req.body; |
There was a problem hiding this comment.
The requirements specify that the reset password confirmation should have password and confirmation fields that must be equal. The backend should validate this, but currently, it only accepts the password and doesn't perform the confirmation check.
src/controllers/user.controller.js
Outdated
|
|
||
| const changePassword = async (req, res) => { | ||
| const { email } = req.params; | ||
| const { oldPassword, newPassword } = req.body; |
There was a problem hiding this comment.
The task requires that changing a password should include an old one, new password, and confirmation. This implementation is missing the confirmation field from the request body and the logic to verify that newPassword matches it.
|
|
||
| const changeEmail = async (req, res) => { | ||
| const { email } = req.params; | ||
| const { password, newEmail } = req.body; |
There was a problem hiding this comment.
According to the requirements, the user must "confirm the new email". This suggests that the request body should also include a confirmation field for the new email, and you should validate that it matches newEmail. This check is currently missing.
| auth: { | ||
| type: 'OAuth2', | ||
| user: 'v.hulaievych@gmail.com', | ||
| emailConfig, |
There was a problem hiding this comment.
The nodemailer transporter's auth object for OAuth2 does not have a property named emailConfig. You should spread the properties from your emailConfig object directly into the auth object like this: ...emailConfig.
| } catch (error) { | ||
| console.error('Error sending email:', error.message); | ||
| } |
There was a problem hiding this comment.
This catch block only logs the error but doesn't re-throw it. This means if sending the email fails, the API route will still return a success status, and the user will never receive their password reset link. The error should be propagated up, similar to how you handle it in sendActivationEmail.
src/services/email.service.js
Outdated
|
|
||
| try { | ||
| await send({ email, subject, html }); | ||
| console.log('✅ Reset password email sent to', email); |
There was a problem hiding this comment.
This log message seems to be a copy-paste from the sendResetPasswordEmail function. It should be updated to accurately reflect that an email change notification was sent.
| } catch (error) { | ||
| console.error('Error sending email:', error.message); | ||
| } |
There was a problem hiding this comment.
Similar to the password reset function, this catch block swallows the error. If the notification email fails to send, the error should be re-thrown so the application can handle it appropriately instead of silently failing.
src/services/user.service.js
Outdated
| id: data.id, | ||
| }, | ||
| data: { | ||
| ...data, |
There was a problem hiding this comment.
This is a major security risk. Spreading the data object directly from the request body into the update query allows a user to change any field in their user record, including potentially id, password, or activationToken. The task requirement is to allow changing the user's name. This function should be updated to only allow specific fields (like name) to be changed.
3 participants
No description provided.