Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This is a very solid and well-structured project, and you've successfully implemented most of the features. However, I'm requesting changes because there are a few critical issues that prevent the application from running and meeting key security requirements.
Here are the main points to address:
- The server will not start. Your
nodemon.jsonandsrc/server/app.jsare configured to run TypeScript files with the standardnoderuntime, which will cause a crash. You need to use a TypeScript runner liketsxorts-nodein yourexeccommand. - Insecure Profile Updates: The backend endpoint in
src/server/controllers/users.controller.tsdoes not validate the user's current password when changing their password or email. This is a critical security vulnerability and a direct requirement of the task. - Client Imports Server Code: The client-side component
src/client/components/FormTemplate.tsxdirectly imports code from thesrc/serverdirectory. Client and server code must be completely separate; this needs to be refactored. - Missing Email Confirmation: The form for changing an email in
src/client/components/ChangeEmailForm.tsxis missing the required confirmation field to prevent typos.
Please address these issues to make your application functional and secure.
✨ 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
| "watch": ["src"], | ||
| "ext": "ts,js,json", | ||
| "ignore": ["node_modules"], | ||
| "exec": "node src/server/app.js" |
There was a problem hiding this comment.
The exec command node src/server/app.js will cause an error because app.js imports .ts files directly. The standard node runtime cannot handle TypeScript modules. Consider using a tool like tsx or ts-node to run your server in development, which can execute TypeScript files on the fly. For example: "exec": "tsx src/server/app.js".
| <Routes> | ||
| <Route path="/" element={<HomePage />} /> | ||
| <Route path="sign-up" element={<SignUpPage />} /> | ||
| <Route | ||
| path="auth/activation/:email/:activationToken" | ||
| element={<AccountActivationPage />} | ||
| /> | ||
| <Route | ||
| path="reset-password/:resetToken" | ||
| element={<ResetPassPage />} | ||
| /> | ||
| <Route path="reset-password" element={<ResetPassPage />} /> | ||
| <Route path="login" element={<LoginPage />} /> | ||
|
|
||
| <Route path="/" element={<RequireAuth />}> | ||
| <Route path="profile" element={<ProfilePage />} /> | ||
| <Route | ||
| path="profile/change/:paramToChange" | ||
| element={<ChangePage />} | ||
| /> | ||
| </Route> |
There was a problem hiding this comment.
While the individual page components handle the redirection for authenticated users, a more consistent approach would be to use a layout route with the RequireNonAuth component to protect all non-authenticated routes. This would centralize your authorization logic and align with how you're using RequireAuth for protected routes.
Consider wrapping SignUpPage, AccountActivationPage, ResetPassPage, and LoginPage in a parent route that uses RequireNonAuth as its element.
| const fields = { | ||
| newEmail: { type: 'email', label: 'New email' }, | ||
| } as Fields; |
There was a problem hiding this comment.
According to the task requirements, changing an email should include a confirmation step for the new email address. This form currently only has a single newEmail field. You should add a second field (e.g., confirmNewEmail) and add validation to ensure both email fields match before allowing the form to be submitted.
| import React from 'react'; | ||
| import cn from 'classnames'; | ||
| import { Link, useNavigate } from 'react-router-dom'; | ||
| import { userService } from '../../server/services/user.service.ts'; |
There was a problem hiding this comment.
Client-side components should not import code directly from the server directory. This is a significant architectural issue that can lead to security vulnerabilities and incorrect application bundling. Validation logic, if needed on both client and server, should be placed in a shared directory that both can access, or the validation rules should be duplicated on the client side.
|
|
||
| // add `Authorization` header to all requests | ||
| httpClient.interceptors.request.use((request) => { | ||
| const accessToken = localStorage.getItem('accessToken'); |
There was a problem hiding this comment.
You have an accessTokenService that abstracts away the details of token storage. For better consistency and maintainability, it would be ideal to use accessTokenService.get() here instead of accessing localStorage directly.
| <Link to={`/profile/change/${item}`} key={item}> | ||
| <li className="button menu-item-box"> | ||
| <p style={{ margin: 0 }}>Change {item}</p> | ||
| <i className="fa-sharp-duotone fa-solid fa-chevron-right"></i> | ||
| </li> | ||
| </Link> |
There was a problem hiding this comment.
For semantically correct HTML, the <Link> component should be nested inside the <li> element, not the other way around. A <ul> element's direct children should only be <li> elements. The correct structure would be <li><Link to={...}>...</Link></li>.
| throw ApiError.badRequest('Email is already taken'); | ||
| } | ||
|
|
||
| const activationToken = bcrypt.genSaltSync(1); |
There was a problem hiding this comment.
Using bcrypt.genSaltSync(1) is not a secure or appropriate method for generating an activation token. This function is designed to create salts for password hashing, not for generating unique, unpredictable tokens. You should use a cryptographically secure random string generator, like crypto.randomBytes(32).toString('hex'), which is already used correctly elsewhere in the application for registration.
| switch (type) { | ||
| case 'name': | ||
| dataToUpdate = await checkNewName(req, user); | ||
| break; | ||
|
|
||
| case 'password': | ||
| dataToUpdate = await checkNewPassword(req); | ||
| break; | ||
|
|
||
| case 'email': | ||
| await checkNewEmail(req, user); | ||
| res.sendStatus(204); | ||
|
|
||
| return; | ||
|
|
||
| default: | ||
| throw ApiError.badRequest('Invalid update type'); | ||
| } |
There was a problem hiding this comment.
The task requires users to provide their current password when changing their password or email. While the client-side handles this in a separate step, this API endpoint does not enforce it. For security, the logic within the 'password' and 'email' cases should also require the user's current password in the request body and validate it before making any changes.
| const activate: RequestHandler = async (req, res) => { | ||
| const { token } = req.params; | ||
| const tokenString = Array.isArray(token) ? token[0] : token; | ||
|
|
||
| const user = await usersRepository.getByActivationToken(tokenString); | ||
|
|
||
| if (!user) { | ||
| throw ApiError.notFound(); | ||
| } | ||
|
|
||
| const updatedUser = await usersRepository.update(user.id, { | ||
| email: user.pendingEmail ?? user.email, | ||
| pendingEmail: null, | ||
| activationToken: null, | ||
| }); | ||
|
|
||
| await sendAuthentication(res, updatedUser); | ||
|
|
||
| if (user.pendingEmail) { | ||
| await mailer.sendEmailChangeNotification(user.email, user.pendingEmail); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The activate function should verify that the activation token belongs to the user with the email address provided in the URL. Currently, it only uses the token to find the user, ignoring the email parameter from the request. You should retrieve both email and token from req.params and ensure the user found by the token also has a matching email or pendingEmail.
| import { authRouter } from './routes/auth.router.ts'; | ||
| import { errorMiddleware } from './middlewares/errorMiddleware.ts'; | ||
| import { userRouter } from './routes/user.router.ts'; | ||
| import { authMiddleware } from './middlewares/authMiddleware.ts'; |
There was a problem hiding this comment.
This server file is a JavaScript file (.js), but it's importing TypeScript files (.ts) directly. The standard node runtime cannot execute TypeScript code. This will cause a runtime error.
To fix this, you should either:
- Rename this file to
app.tsand use a TypeScript runner liketsxorts-nodeto start your server. - Compile your TypeScript files to JavaScript and import the compiled
.jsfiles instead.
2 participants
No description provided.