add task implementation#277
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The implementation has several critical blockers that prevent task completion. Password validation is insufficient - it only checks length > 6, but requirements explicitly state users must be informed of password rules and validation must check them (uppercase, lowercase, numbers, special characters). Auth routes lack non-authenticated user guards - registration, login, forgot-password, and reset-password must be accessible ONLY to non-authenticated users as explicitly required. Error handlers use incorrect status codes - res.send() returns implicit 200 for errors instead of 400/401/403/500. Registration endpoint returns the hashed password in the response, creating a security vulnerability. Logout route is unprotected - it has no authMiddleware despite being an authenticated-only operation. Activation doesn't clear the activationToken after successful use. Email change immediately updates without requiring confirmation from the new email. The changePassword function also doesn't validate the new password against the same rules enforced at registration. Additionally, ESLint configuration is missing TypeScript parser support, preventing code validation. These issues must be addressed before approval.
✨ 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
| 'no-return-await': 'off', | ||
| }, | ||
| plugins: ['jest'] | ||
| plugins: ['jest', '@typescript-eslint'] |
There was a problem hiding this comment.
The registration route doesn't check if user is already authenticated. The task requires this route be accessible ONLY to non-authenticated users.
| '@typescript-eslint/no-unused-vars': ['warn'], | ||
| 'no-shadow': 'off', | ||
| '@typescript-eslint/no-shadow': ['error'], | ||
| 'no-return-await': 'off', |
There was a problem hiding this comment.
Error handlers use res.send() with status 200 instead of res.status(). For errors, appropriate status codes like 400 (Bad Request), 401 (Unauthorized), 403 (Forbidden) should be used.
| res.send({ error: (error as Error).message }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The password validation only checks length > 6. The requirements state: 'Inform users about the rules for a password and check them'. Consider implementing more comprehensive password rules that can be shown to users during registration.
|
|
||
| res.send('Акаунт активовано'); |
There was a problem hiding this comment.
The activation endpoint returns plain text 'Акаунт активовано'. The requirements state: 'redirect to Profile after the activation'. Since this is an API endpoint, consider returning a response that indicates successful activation so the frontend can redirect (e.g., status code 200 with a success indicator, or redirect URL).
|
|
||
| res.send({ message: 'Успішний вихід' }); |
There was a problem hiding this comment.
The logout endpoint returns a success message but the requirements state: 'Redirect to login after logging out'. Consider returning an appropriate status code so the frontend can redirect to login.
| res.send({ message: "Ім'я змінено" }); | ||
| } catch (error) { | ||
| res.send({ error: (error as Error).message }); | ||
| } |
There was a problem hiding this comment.
The activation catch block uses console.log instead of sending an error response to the client, silently failing on errors.
| export const updateName = async (req: Request, res: Response) => { | ||
| try { | ||
| const email = (req as any).user.email; | ||
| const { newName } = req.body; |
There was a problem hiding this comment.
The task requires confirming new email before the change takes effect. Currently, email is changed immediately. Consider implementing a two-step confirmation where the new email must be verified first.
|
|
||
| return; |
There was a problem hiding this comment.
The Token model limits refreshToken to 500 characters. JWT tokens can vary in size depending on the claims and algorithm. Consider using TEXT type or increasing the limit to prevent potential truncation issues.
| @Column({ | ||
| type: DataType.STRING(500), | ||
| allowNull: false, | ||
| unique: true, |
There was a problem hiding this comment.
The authMiddleware doesn't validate the token format before accessing it. If header is 'Bearer' without a token, split()[1] would be undefined. Consider adding validation for the token existence after the split.
|
|
||
| authRouter.post('/registration', registration); | ||
| authRouter.get('/activation/:activationToken', activation); | ||
| authRouter.post('/login', login); | ||
| authRouter.post('/refresh', refreshing); | ||
| authRouter.post('/logout', logout); | ||
|
|
||
| authRouter.post('/forgot-password', forgotPassword); |
There was a problem hiding this comment.
The authRouter doesn't have middleware to restrict registration, login, forgot-password, and reset-password routes to non-authenticated users only. The task requires these routes be accessible ONLY to non-authenticated users.
| authRouter.post('/logout', logout); | ||
|
|
There was a problem hiding this comment.
The password validation only checks length > 6 characters. The task requires informing users about password rules and validating against them. Consider implementing comprehensive password rules (uppercase, lowercase, numbers, special characters) that can be displayed to users during registration.
| const existingUser = await User.findOne({ where: { email: email } }); | ||
|
|
||
| if (existingUser) { | ||
| throw new Error('Користувач з таким емейлом уже зареєстрований'); | ||
| } | ||
|
|
||
| if (password.length < 6) { | ||
| throw new Error('Довжина паролю має бути більше 6 символів'); | ||
| } |
There was a problem hiding this comment.
The authRouter lacks middleware to restrict registration, login, forgot-password, and reset-password routes to non-authenticated users only. The requirements explicitly state these routes should be accessible ONLY to non-authenticated users.
|
|
||
| return { | ||
| user, | ||
| accessToken, | ||
| refreshToken, |
There was a problem hiding this comment.
The login function returns the full user object which includes the hashed password. Consider returning only necessary user data (id, name, email) without sensitive fields.
|
|
||
| user.isActive = true; |
There was a problem hiding this comment.
The activate function sets isActive to true but doesn't clear the activationToken. After activation, the token remains in the database which is unnecessary and could be a security concern.
|
|
||
| if (password.length < 6) { | ||
| throw new Error('Довжина паролю має бути більше 6 символів'); |
There was a problem hiding this comment.
The password validation only checks length > 6 characters. The requirements state to 'Inform users about the rules for a password and check them'. Consider implementing comprehensive password rules (uppercase, lowercase, numbers, special characters) that can be displayed to users.
|
|
||
| export const sendActivationEmail = async ( | ||
| email: string, | ||
| activationToken: string, | ||
| ) => { | ||
| await transporter.sendMail({ | ||
| from: process.env.SMTP_USER, | ||
| to: email, | ||
| subject: 'Активація акаунта', |
There was a problem hiding this comment.
The authRouter has no middleware to restrict routes to non-authenticated users. According to requirements, registration, login, activation, and password reset must be accessible ONLY to non-authenticated users. Consider adding middleware to check if user is already authenticated on these routes.
| email: string, | ||
| activationToken: string, | ||
| ) => { | ||
| await transporter.sendMail({ |
There was a problem hiding this comment.
The logout route has no authentication middleware. According to requirements, logout must be accessible only to authenticated users. Consider adding authMiddleware to this route.
| await transporter.sendMail({ | ||
| from: process.env.SMTP_USER, | ||
| to: email, |
There was a problem hiding this comment.
The password validation only checks for minimum length (6 characters). The task requires informing users about password rules and validating them. Consider implementing more comprehensive validation (uppercase, lowercase, numbers, special characters) and returning clear error messages about all failed rules.
| secure: false, | ||
| auth: { | ||
| user: process.env.SMTP_USER as string, | ||
| pass: process.env.SMTP_PASSWORD as string, | ||
| }, |
There was a problem hiding this comment.
The register function doesn't validate email format before saving to the database. Invalid email formats could be stored.
| import { authMiddleware } from '../middlewares/authMiddleware'; | ||
|
|
||
| export const userRouter = Router(); | ||
|
|
There was a problem hiding this comment.
The routes are well-structured and the userRouter correctly applies authMiddleware to all protected routes.
|
|
||
| const hashedNewPass = await bcrypt.hash(newPassword, 10); | ||
|
|
||
| user.password = hashedNewPass; |
There was a problem hiding this comment.
The changePassword function doesn't validate the new password against the same rules used during registration. Consider applying consistent password validation to ensure the new password meets requirements (such as minimum length and complexity rules).
|
|
||
| const newActivationToken = uuidv4(); | ||
|
|
||
| user.email = newEmail; | ||
| user.activationToken = newActivationToken; | ||
| user.isActive = false; | ||
|
|
There was a problem hiding this comment.
The email change flow correctly requires password validation, sends activation to the new email, and notifies the old email address. However, the new email is immediately updated without waiting for confirmation. Consider whether the new email should be verified first before finalizing the change.
| { | ||
| "compilerOptions": { | ||
| "target": "ES2020", | ||
| "module": "CommonJS", | ||
| "moduleResolution": "node", | ||
| "ignoreDeprecations": "6.0", | ||
| "outDir": "./dist", | ||
| "rootDir": "./src", | ||
| "strict": true, | ||
| "types": ["node"], | ||
| "experimentalDecorators": true, | ||
| "emitDecoratorMetadata": true, | ||
| "esModuleInterop": true, | ||
| "skipLibCheck": true, | ||
| "forceConsistentCasingInFileNames": true | ||
| }, | ||
| "include": ["src/**/*"] |
There was a problem hiding this comment.
The tsconfig.json is properly configured with necessary options for TypeScript compilation, including decorators support for sequelize-typescript, strict mode, and CommonJS module output.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation has 7 critical blockers that must be fixed before approval:
-
Password validation is incomplete (REQ-4) - Currently only checks
length > 6. You must add regex validation requiring uppercase, lowercase, numbers, and special characters. Update bothregisterandchangePasswordfunctions. -
Registration exposes hashed password -
res.send(newUser)returns the entire user object including the password field. Return only safe fields like{ id, name, email, isActive }instead. -
Missing route guards for non-authenticated users (REQ-2, REQ-6, REQ-10, REQ-15) - Registration, login, forgot-password, reset-password, and activation routes are accessible to all users. Add
notAuthMiddlewareto these routes inauthRouter.ts. -
Logout route is unprotected (REQ-13) - Add
authMiddlewareto the logout route. -
ActivationToken not cleared after use - After successful activation, set
user.activationToken = nullbefore saving. -
Error handlers return HTTP 200 status - Use
res.status(400/401/500).send()instead ofres.send()for all error responses. -
changePassword doesn't validate new password - Apply the same password rules (uppercase, lowercase, numbers, special characters) in
userService.ts.
These issues represent security vulnerabilities and broken functionality that violate explicit task requirements. Please address all 7 issues and resubmit.
✨ 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
| rules: { | ||
| 'no-proto': 0 | ||
| 'no-proto': 0, | ||
| 'no-console': 0, |
There was a problem hiding this comment.
Security issue: res.send(newUser) returns the entire user object including the hashed password field. This exposes sensitive data to the client. Return only safe fields like { message: '...', user: { id, name, email } } or { message: '...' }.
| 'no-proto': 0, | ||
| 'no-console': 0, | ||
| 'indent': 'off', | ||
| '@typescript-eslint/no-shadow': ['error'], |
There was a problem hiding this comment.
Error handler uses res.send() which returns HTTP 200 status. Should use res.status(400/401/500).send() for error responses to comply with REST conventions.
| '@typescript-eslint/no-unused-vars': ['warn'], | ||
| 'no-shadow': 'off', | ||
| '@typescript-eslint/no-shadow': ['error'], | ||
| 'no-return-await': 'off', |
There was a problem hiding this comment.
Password validation only checks length > 6, but requirements state users must be informed of rules AND validation must check them (uppercase, lowercase, numbers, special characters). Add regex validation for these rules.
| }, | ||
| rules: { | ||
| 'no-proto': 0 | ||
| 'no-proto': 0, |
There was a problem hiding this comment.
Error handler uses implicit 200 status. Should use appropriate error status codes.
| }, | ||
| plugins: ['jest'] | ||
| plugins: ['jest', '@typescript-eslint'] | ||
| }; |
There was a problem hiding this comment.
Error handler uses implicit 200 status. Should use appropriate error status codes.
| jest: true, | ||
| node: true, | ||
| }, | ||
| rules: { |
There was a problem hiding this comment.
Requirement not met: Registration route is accessible to all users. According to REQ-2, registration must be accessible ONLY to non-authenticated users. Add a guard middleware like notAuthMiddleware.
| rules: { | ||
| 'no-proto': 0 | ||
| 'no-proto': 0, | ||
| 'no-console': 0, |
There was a problem hiding this comment.
Requirement not met: Login route is accessible to all users. According to REQ-10, login must be accessible ONLY to non-authenticated users. Add a guard middleware.
| '@typescript-eslint/no-unused-vars': ['warn'], | ||
| 'no-shadow': 'off', | ||
| '@typescript-eslint/no-shadow': ['error'], | ||
| 'no-return-await': 'off', |
There was a problem hiding this comment.
Requirement not met: Forgot password route is accessible to all users. According to REQ-15, it must be accessible ONLY to non-authenticated users. Add a guard middleware.
| 'no-shadow': 'off', | ||
| '@typescript-eslint/no-shadow': ['error'], | ||
| 'no-return-await': 'off', | ||
| }, |
There was a problem hiding this comment.
Requirement not met: Reset password route is accessible to all users. According to REQ-15, it must be accessible ONLY to non-authenticated users. Add a guard middleware.
| }, | ||
| env: { | ||
| jest: true | ||
| jest: true, |
There was a problem hiding this comment.
Requirement not met: Activation route is accessible to all users. According to REQ-6, activation page must be accessible ONLY to non-authenticated users. Add a guard middleware.
|
|
||
| await activate(activationToken); |
There was a problem hiding this comment.
Password validation only checks length > 6. Per requirements, users must be informed of password rules and validation must check them (uppercase, lowercase, numbers, special characters).
|
|
||
| try { | ||
| const newUser = await register(name, email, password); | ||
|
|
There was a problem hiding this comment.
Registration returns the entire user object including hashed password - this is a security vulnerability. Should return user-safe data only (id, name, email).
| const refreshToken = req.cookies.refreshToken; | ||
|
|
There was a problem hiding this comment.
Activation succeeds but doesn't clear the activationToken. The token remains valid after use.
| }; | ||
|
|
||
| export const login = async (req: Request, res: Response) => { | ||
| try { |
There was a problem hiding this comment.
changePassword doesn't validate the new password against the password rules enforced at registration. Should check for uppercase, lowercase, numbers, and special characters.
|
|
||
| res.send(result.newAccessToken); | ||
| } catch (error) { | ||
| res.send({ error: (error as Error).message }); |
There was a problem hiding this comment.
changeEmail immediately updates the email and deactivates the account without requiring confirmation from the new email. Per requirements, email should only change after new email confirmation.
| const newUser = await register(name, email, password); | ||
|
|
||
| res.send(newUser); | ||
| } catch (error) { |
There was a problem hiding this comment.
Error responses use res.send() which returns implicit 200 status. Should use res.status(400).send() or similar for validation errors.
| res.send('Акаунт активовано'); | ||
| } catch (error) { |
There was a problem hiding this comment.
Error responses use res.send() with implicit 200 status. Should return appropriate status codes (401 for unauthorized, 400 for bad request, 500 for server errors).
| }); | ||
|
|
||
| res.send({ user: resObj.user, accessToken: resObj.accessToken }); | ||
| } catch (error) { |
There was a problem hiding this comment.
Error responses use res.send() with implicit 200 status instead of proper error status codes.
| res.clearCookie('refreshToken'); | ||
|
|
||
| res.send({ message: 'Успішний вихід' }); | ||
| } catch (error) { |
There was a problem hiding this comment.
Error responses use res.send() with implicit 200 status instead of proper error status codes.
|
|
||
| await forgotPass(email); | ||
| res.send({ message: 'Лист для відновлення пароля відправлено' }); | ||
| } catch (error) { |
There was a problem hiding this comment.
Error responses use res.send() with implicit 200 status instead of proper error status codes.
|
|
||
| await resetPass(resetToken, password); | ||
| res.send({ message: 'Пароль успішно змінено' }); | ||
| } catch (error) { |
There was a problem hiding this comment.
Error responses use res.send() with implicit 200 status instead of proper error status codes.
| pass: process.env.SMTP_PASSWORD as string, | ||
| }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Registration route lacks guard for non-authenticated users. Per REQ-2, registration must be accessible ONLY to non-authenticated users. Add a notAuthMiddleware.
| }, | ||
| }); | ||
|
|
||
| export const sendActivationEmail = async ( |
There was a problem hiding this comment.
Activation route lacks guard for non-authenticated users. Per REQ-6, activation must be accessible ONLY to non-authenticated users. Add a notAuthMiddleware.
| }); | ||
|
|
||
| export const sendActivationEmail = async ( | ||
| email: string, |
There was a problem hiding this comment.
Login route lacks guard for non-authenticated users. Per REQ-10, login must be accessible ONLY to non-authenticated users. Add a notAuthMiddleware.
| export const sendActivationEmail = async ( | ||
| email: string, | ||
| activationToken: string, | ||
| ) => { |
There was a problem hiding this comment.
Logout route is missing authMiddleware. Per REQ-13, logout must be accessible ONLY to authenticated users. Add authMiddleware to this route.
| ) => { | ||
| await transporter.sendMail({ | ||
| from: process.env.SMTP_USER, | ||
| to: email, |
There was a problem hiding this comment.
Forgot-password route lacks guard for non-authenticated users. Per REQ-15, it must be accessible ONLY to non-authenticated users. Add a notAuthMiddleware.
| await transporter.sendMail({ | ||
| from: process.env.SMTP_USER, | ||
| to: email, | ||
| subject: 'Активація акаунта', |
There was a problem hiding this comment.
Reset-password route lacks guard for non-authenticated users. Per REQ-15, it must be accessible ONLY to non-authenticated users. Add a notAuthMiddleware.
| await transporter.sendMail({ | ||
| from: process.env.SMTP_USER, | ||
| to: email, |
There was a problem hiding this comment.
Password validation only checks length < 6. Per REQ-3 and REQ-4, users must be informed of password rules and validation must check them (uppercase, lowercase, numbers, special characters).
| from: process.env.SMTP_USER, | ||
| to: email, | ||
| subject: 'Відновлення пароля', | ||
| html: `<a href="${process.env.CLIENT_URL}/reset-password/${resetToken}">Змінити пароль</a>`, |
There was a problem hiding this comment.
Registration returns the entire user object including the hashed password. This is a security vulnerability - the response should only return a success message.
|
|
||
| if (existingUser) { | ||
| throw new Error('Користувач з таким емейлом уже зареєстрований'); | ||
| } |
There was a problem hiding this comment.
REQ-13 not met: Logout route has no authMiddleware. Per requirements, logout must be accessible ONLY to authenticated users. Add authMiddleware here.
| const existingUser = await User.findOne({ where: { email: email } }); | ||
|
|
||
| if (existingUser) { | ||
| throw new Error('Користувач з таким емейлом уже зареєстрований'); | ||
| } | ||
|
|
||
| if (password.length < 6) { | ||
| throw new Error('Довжина паролю має бути більше 6 символів'); |
There was a problem hiding this comment.
REQ-2, REQ-6, REQ-10, REQ-15 not met: These routes lack non-authenticated user guards. Per requirements, registration, activation, login, forgot-password, and reset-password must be accessible ONLY to non-authenticated users. Need to create and apply a notAuthMiddleware to these routes.
|
|
||
| if (password.length < 6) { | ||
| throw new Error('Довжина паролю має бути більше 6 символів'); |
There was a problem hiding this comment.
REQ-4 not met: Password validation only checks length > 6. Per requirements, validation must check for uppercase, lowercase, numbers, and special characters. Add regex validation for these rules.
| }); | ||
|
|
||
| await sendActivationEmail(email, activationToken); | ||
|
|
There was a problem hiding this comment.
Security issue: return newUser exposes the user object including hashed password to the controller. Return only safe data like { message: '...' } or extract only safe fields.
|
|
||
| user.isActive = true; |
There was a problem hiding this comment.
Security issue: After activation, the activationToken is not cleared from the user record. It should be set to null to prevent token reuse.
| if (!user) { | ||
| throw new Error('Недійсне або застаріле посилання'); | ||
| } | ||
|
|
There was a problem hiding this comment.
REQ-4 not met: The new password is not validated against the stated password rules. The resetPassword function should check for uppercase, lowercase, numbers, and special characters.
| } from '../controllers/authController'; | ||
|
|
||
| export const authRouter = Router(); | ||
|
|
There was a problem hiding this comment.
REQ-2 not met: Registration route is accessible to all users. Per requirements, registration must be accessible ONLY to non-authenticated users. Add notAuthMiddleware guard.
|
|
||
| export const authRouter = Router(); | ||
|
|
||
| authRouter.post('/registration', registration); |
There was a problem hiding this comment.
REQ-6 not met: Activation route is accessible to all users. Per requirements, activation must be accessible ONLY to non-authenticated users. Add notAuthMiddleware guard.
| export const authRouter = Router(); | ||
|
|
||
| authRouter.post('/registration', registration); | ||
| authRouter.get('/activation/:activationToken', activation); |
There was a problem hiding this comment.
REQ-10 not met: Login route is accessible to all users. Per requirements, login must be accessible ONLY to non-authenticated users. Add notAuthMiddleware guard.
| authRouter.post('/registration', registration); | ||
| authRouter.get('/activation/:activationToken', activation); | ||
| authRouter.post('/login', login); | ||
| authRouter.post('/refresh', refreshing); |
There was a problem hiding this comment.
REQ-13 not met: Logout route is not protected. Per requirements, logout must be accessible ONLY to authenticated users. Add authMiddleware guard.
| authRouter.post('/login', login); | ||
| authRouter.post('/refresh', refreshing); | ||
| authRouter.post('/logout', logout); | ||
|
|
There was a problem hiding this comment.
REQ-15 not met: Forgot password route is accessible to all users. Per requirements, password reset must be accessible ONLY to non-authenticated users. Add notAuthMiddleware guard.
| authRouter.post('/refresh', refreshing); | ||
| authRouter.post('/logout', logout); | ||
|
|
||
| authRouter.post('/forgot-password', forgotPassword); |
There was a problem hiding this comment.
REQ-15 not met: Reset password route is accessible to all users. Per requirements, password reset must be accessible ONLY to non-authenticated users. Add notAuthMiddleware guard.
| authRouter.post('/logout', logout); | ||
|
|
||
| authRouter.post('/forgot-password', forgotPassword); |
There was a problem hiding this comment.
REQ-4 not met: Password validation only checks length < 6. Per requirements, users must be informed of password rules AND validation must check them (uppercase, lowercase, numbers, special characters). Add regex validation for these rules.
|
|
||
| userRouter.use(authMiddleware); | ||
|
|
||
| userRouter.patch('/name', updateName); |
There was a problem hiding this comment.
Requirement not met: Registration route is accessible to all users. Per REQ-2, registration must be accessible ONLY to non-authenticated users. Add a guard middleware (e.g., notAuthMiddleware).
| userRouter.use(authMiddleware); | ||
|
|
||
| userRouter.patch('/name', updateName); | ||
| userRouter.patch('/email', updateEmail); |
There was a problem hiding this comment.
Requirement not met: Activation route is accessible to all users. Per REQ-6, activation page must be accessible ONLY to non-authenticated users. Add a guard middleware.
|
|
||
| userRouter.patch('/name', updateName); | ||
| userRouter.patch('/email', updateEmail); | ||
| userRouter.patch('/password', updatePassword); |
There was a problem hiding this comment.
Requirement not met: Login route is accessible to all users. Per REQ-10, login must be accessible ONLY to non-authenticated users. Add a guard middleware.
|
|
||
| export const changePassword = async ( | ||
| email: string, | ||
| oldPassword: string, | ||
| newPassword: string, | ||
| ) => { | ||
| const user = await User.findOne({ where: { email } }); | ||
|
|
||
| if (!user) { | ||
| throw new Error('Користувача не існує'); | ||
| } | ||
|
|
||
| const valid = await bcrypt.compare(oldPassword, user.password); | ||
|
|
||
| if (!valid) { | ||
| throw new Error('Невірний старий пароль'); | ||
| } | ||
|
|
||
| const hashedNewPass = await bcrypt.hash(newPassword, 10); | ||
|
|
||
| user.password = hashedNewPass; | ||
| await user.save(); |
There was a problem hiding this comment.
The changePassword function doesn't validate the new password against the stated password rules. Per requirements, the new password must follow the same rules enforced during registration (uppercase, lowercase, numbers, special characters). Add validation similar to what's in the register function in authService.ts.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The implementation shows solid progress with correct password validation, proper route guards, token cleanup, and safe field patterns. However, two key issues need fixing: (1) Express response chaining must be corrected from 'res.send(201).send(newUser)' to 'res.status(201).send(newUser)', and (2) Data exposure in login/refresh functions must be addressed by returning only safe fields like { id, name, email, isActive } instead of the full Sequelize User object. A minor fix is also needed in authMiddleware.ts where the error message typo 'He авторизовано' should use Cyrillic 'Не'. These fixes will make the auth system production-ready.
✨ 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
| rules: { | ||
| 'no-proto': 0 | ||
| 'no-proto': 0, | ||
| 'no-console': 0, |
There was a problem hiding this comment.
Invalid syntax: res.send(201).send(newUser). Express res.send() returns Response but chaining two send() calls causes TypeError. Use res.status(201).send(newUser) instead.
| 'no-proto': 0, | ||
| 'no-console': 0, | ||
| 'indent': 'off', | ||
| '@typescript-eslint/no-shadow': ['error'], |
There was a problem hiding this comment.
Invalid syntax: res.send(400).send(...). Use res.status(400).send(...).
| }, | ||
| plugins: ['jest'] | ||
| plugins: ['jest', '@typescript-eslint'] | ||
| }; |
There was a problem hiding this comment.
Invalid syntax: res.send(200).send(...). Use res.status(200).send(...).
| jest: true | ||
| jest: true, | ||
| node: true, | ||
| }, |
There was a problem hiding this comment.
Invalid syntax: res.send(200).send(...). Use res.status(200).send(...).
| }, | ||
| rules: { | ||
| 'no-proto': 0 | ||
| 'no-proto': 0, |
There was a problem hiding this comment.
Invalid syntax: res.send(400).send(...). Use res.status(400).send(...).
| 'no-shadow': 'off', | ||
| '@typescript-eslint/no-shadow': ['error'], | ||
| 'no-return-await': 'off', | ||
| }, |
There was a problem hiding this comment.
Invalid syntax: res.send(200).send(...). Use res.status(200).send(...).
| }, | ||
| plugins: ['jest'] | ||
| plugins: ['jest', '@typescript-eslint'] | ||
| }; |
There was a problem hiding this comment.
Invalid syntax: res.send(400).send(...). Use res.status(400).send(...).
|
|
||
| res.send(200).send({ message: 'Лист для підтвердження відправлено на нову пошту' }); | ||
| } catch (error) { | ||
| res.send(400).send({ error: (error as Error).message }); |
There was a problem hiding this comment.
BUG: res.send() returns undefined, so .send(newUser) never executes. Use res.status(201).send(newUser) instead.
| await changeName(email, newName); | ||
|
|
||
| res.send(200).send({ message: "Ім'я змінено" }); | ||
| } catch (error) { |
There was a problem hiding this comment.
BUG: Should be res.status(200).send(...)
| res.send(200).send({ message: "Ім'я змінено" }); | ||
| } catch (error) { | ||
| res.send(400).send({ error: (error as Error).message }); | ||
| } |
There was a problem hiding this comment.
BUG: Should be res.status(400).send(...)
|
|
||
| await changePassword(email, oldPassword, newPassword); | ||
|
|
||
| res.send(200).send({ message: 'Пароль змінено' }); |
There was a problem hiding this comment.
BUG: Should be res.status(200).send({ user: resObj.user, accessToken: resObj.accessToken })
|
|
||
| res.send(200).send({ message: 'Пароль змінено' }); | ||
| } catch (error) { | ||
| res.send(400).send({ error: (error as Error).message }); |
There was a problem hiding this comment.
BUG: Should be res.status(401).send(...)
| const { newEmail, password } = req.body; | ||
|
|
||
| await changeEmail(oldEmail, newEmail, password); | ||
|
|
There was a problem hiding this comment.
BUG: Should be res.status(200).send(...)
| await changeEmail(oldEmail, newEmail, password); | ||
|
|
||
| res.send(200).send({ message: 'Лист для підтвердження відправлено на нову пошту' }); | ||
| } catch (error) { |
There was a problem hiding this comment.
BUG: Should be res.status(400).send(...)
| const { newName } = req.body; | ||
|
|
||
| await changeName(email, newName); | ||
|
|
There was a problem hiding this comment.
BUG: Should be res.status(200).send(...)
| await changeName(email, newName); | ||
|
|
||
| res.send(200).send({ message: "Ім'я змінено" }); | ||
| } catch (error) { |
There was a problem hiding this comment.
BUG: Should be res.status(400).send(...)
| } | ||
|
|
||
| await changePassword(email, oldPassword, newPassword); | ||
|
|
There was a problem hiding this comment.
BUG: Should be res.status(200).send(...)
| await changePassword(email, oldPassword, newPassword); | ||
|
|
||
| res.send(200).send({ message: 'Пароль змінено' }); | ||
| } catch (error) { |
There was a problem hiding this comment.
BUG: Should be res.status(400).send(...)
|
|
||
| try { | ||
| const newUser = await register(name, email, password); | ||
|
|
There was a problem hiding this comment.
BUG: res.send(201).send(newUser) is incorrect. res.send() sends the response and cannot be chained with another send(). Use res.status(201).send(newUser) instead. Same issue on all error handlers in this file - should be res.status(400).send(...).
| const newUser = await register(name, email, password); | ||
|
|
||
| res.send(201).send(newUser); | ||
| } catch (error) { |
There was a problem hiding this comment.
BUG: res.send(400).send(...) should be res.status(400).send(...). The send() method ends the response cycle.
| const activationToken = req.params.activationToken as string; | ||
|
|
||
| await activate(activationToken); | ||
|
|
There was a problem hiding this comment.
BUG: res.send(200).send(...) should be res.status(200).send(...).
| await activate(activationToken); | ||
|
|
||
| res.send(200).send('Акаунт активовано'); | ||
| } catch (error) { |
There was a problem hiding this comment.
BUG: res.send(400).send(...) should be res.status(400).send(...).
| maxAge: 30 * 24 * 60 * 60 * 1000, | ||
| httpOnly: true, | ||
| }); | ||
|
|
There was a problem hiding this comment.
SECURITY ISSUE: resObj.user returns the entire Sequelize User object including the hashed password field. Return only safe fields like { id, name, email, isActive } instead.
| }); | ||
|
|
||
| res.send(200).send({ user: resObj.user, accessToken: resObj.accessToken }); | ||
| } catch (error) { |
There was a problem hiding this comment.
BUG: res.send(401).send(...) should be res.status(401).send(...).
| maxAge: 30 * 24 * 60 * 60 * 1000, | ||
| httpOnly: true, | ||
| }); | ||
|
|
There was a problem hiding this comment.
BUG: res.send(200).send(...) should be res.status(200).send(...).
| }); | ||
|
|
||
| res.send(200).send(result.newAccessToken); | ||
| } catch (error) { |
There was a problem hiding this comment.
BUG: res.send(401).send(...) should be res.status(401).send(...).
| await lout(refreshToken); | ||
|
|
||
| res.clearCookie('refreshToken'); | ||
|
|
There was a problem hiding this comment.
BUG: res.send(204).send(...) should be res.status(204).send(...) or simply res.sendStatus(204).
| res.clearCookie('refreshToken'); | ||
|
|
||
| res.send(204).send({ message: 'Успішний вихід' }); | ||
| } catch (error) { |
There was a problem hiding this comment.
BUG: res.send(400).send(...) should be res.status(400).send(...).
| try { | ||
| const email = req.body.email; | ||
|
|
||
| await forgotPass(email); |
There was a problem hiding this comment.
BUG: res.send(200).send(...) should be res.status(200).send(...).
|
|
||
| await forgotPass(email); | ||
| res.send(200).send({ message: 'Лист для відновлення пароля відправлено' }); | ||
| } catch (error) { |
There was a problem hiding this comment.
BUG: res.send(400).send(...) should be res.status(400).send(...).
| throw new Error('Паролі не збігаються'); | ||
| } | ||
|
|
||
| await resetPass(resetToken, password); |
There was a problem hiding this comment.
BUG: res.send(200).send(...) should be res.status(200).send(...).
|
|
||
| await resetPass(resetToken, password); | ||
| res.send(200).send({ message: 'Пароль успішно змінено' }); | ||
| } catch (error) { |
There was a problem hiding this comment.
BUG: res.send(400).send(...) should be res.status(400).send(...).
| const { name, email, password } = req.body; | ||
|
|
||
| try { | ||
| const newUser = await register(name, email, password); |
There was a problem hiding this comment.
BUG: res.send(400).send(...) should be res.status(400).send(...).
| try { | ||
| const activationToken = req.params.activationToken as string; | ||
|
|
||
| await activate(activationToken); |
There was a problem hiding this comment.
BUG: res.send(400).send(...) should be res.status(400).send(...).
| const activationToken = req.params.activationToken as string; | ||
|
|
||
| await activate(activationToken); | ||
|
|
There was a problem hiding this comment.
BUG: res.send(400).send(...) should be res.status(400).send(...).
| maxAge: 30 * 24 * 60 * 60 * 1000, | ||
| httpOnly: true, | ||
| }); | ||
|
|
There was a problem hiding this comment.
BUG: res.send(400).send(...) should be res.status(400).send(...).
| httpOnly: true, | ||
| }); | ||
|
|
||
| res.send(200).send({ user: resObj.user, accessToken: resObj.accessToken }); |
There was a problem hiding this comment.
BUG: res.send(400).send(...) should be res.status(400).send(...).
| ) => { | ||
| const header = req.headers.authorization; | ||
|
|
||
| if (!header) { |
There was a problem hiding this comment.
Minor: 'He авторизовано' appears to be a typo (Latin 'He' instead of Cyrillic 'Не'), but matches the pattern on line 25 where 'Не авторизовано' is used correctly.
| refreshToken: refreshToken, | ||
| }); | ||
|
|
||
| return { |
There was a problem hiding this comment.
SECURITY ISSUE: Returns entire User object including hashed password. Change to { id: user.id, name: user.name, email: user.email, isActive: user.isActive } to match the safe pattern used in register function (lines 37-42).
| const user = await User.findOne({ where: { email } }); | ||
|
|
||
| if (!user) { | ||
| throw new Error('Невірний емейл або пароль'); |
There was a problem hiding this comment.
consider avoiding non-English comments in your code
No description provided.