add main solution#290
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This implementation has several critical functional issues that prevent core user flows from working correctly. First, password confirmation is not validated server-side — the DataForUpdatePasswordDto is missing the confirmNewPassword field, despite the requirements explicitly stating that changing password must require 'old one, new password and confirmation'. This creates a security gap where backend validation is bypassed. Second, email update field names are mismatched — frontend sends { email, confirmEmail } but the backend DTO expects { newEmail, confirmNewEmail }, causing email confirmation to silently fail. Third, activation endpoint returns the wrong response format — the backend returns { message: 'Account activated' } on success, but the frontend API route checks for normalizedUser in the response and incorrectly returns 401 Unauthorized, making activation broken for users. Fourth, the update-user-data route will make a fetch request to an empty URL if none of the update conditions (newName, newEmail, newPassword) are met in the request body. These issues directly break functionality specified in the task requirements and must be fixed 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
| } catch (error) { | ||
| throw new BadRequestException({ | ||
| statusCode: 400, |
There was a problem hiding this comment.
The entire Error object is being passed as message. Should extract the message: message: error instanceof Error ? error.message : 'Unknown error'
|
|
||
| if (!user.isActivated) { | ||
| const normalizedUser = { id: user.id, activationToken: true} | ||
| res.status(200).json({ normalizedUser }) | ||
| return; |
There was a problem hiding this comment.
Non-activated users should NOT receive a successful login response. This returns a 200 with activationToken flag, which doesn't align with the requirement 'If user is not active, prompt them to activate their email'. Consider throwing an error or returning a distinct response that forces the frontend to redirect to activation.
|
|
||
| async login(data: LoginDto, res) { | ||
| const user = await this.prisma.user.findUnique({ | ||
| where: { email: data.email} | ||
| }) | ||
|
|
||
| if (!user ) { | ||
| throw new UnauthorizedException('Invalid credentials') | ||
| } | ||
|
|
||
| const compare = await bcrypt.compare(data.password, user.password || '') | ||
|
|
||
| if(!compare) { | ||
| throw new UnauthorizedException('Invalid credentials') | ||
| }; | ||
|
|
||
| if (!user.isActivated) { | ||
| const normalizedUser = { id: user.id, activationToken: true} | ||
| res.status(200).json({ normalizedUser }) | ||
| return; | ||
| } | ||
|
|
||
| const normalizedUser = normalizeUser(user); | ||
|
|
||
| const accessToken = jwt.generateAccessToken(normalizedUser); | ||
| const refreshToken = jwt.generateRefreshToken(normalizedUser); | ||
|
|
||
| const hashedRefreshAccessToken = createHash('sha256').update(refreshToken).digest('hex'); | ||
|
|
||
| await this.prisma.user.update({ | ||
| where: { id: user.id}, | ||
| data: {refreshAccessTokenHash: hashedRefreshAccessToken}, | ||
| }) | ||
|
|
||
| res.cookie('accessToken', accessToken, { | ||
| httpOnly: true, | ||
| secure: false, // dev only | ||
| sameSite: "lax", | ||
| path: "/", | ||
| maxAge: 1 * 1 * 15 * 60 * 1000, | ||
| }); | ||
| res.cookie("refreshToken", refreshToken, { | ||
| httpOnly: true, | ||
| secure: false, // dev only | ||
| sameSite: "lax", | ||
| path: "/", | ||
| maxAge: 7 * 24 * 60 * 60 * 1000, | ||
| }); | ||
|
|
||
|
|
||
| res.status(200).json( | ||
| { | ||
| normalizedUser, | ||
| accessToken, | ||
| }) | ||
| return { | ||
| normalizedUser, | ||
| accessToken, | ||
| } |
There was a problem hiding this comment.
The login method duplicates the session issuance logic (lines 176-210) instead of reusing the issueSession() method defined earlier. This creates inconsistent cookie handling and code duplication.
| @@ -0,0 +1,7 @@ | |||
| import { IsNotEmpty, IsString } from 'class-validator'; | |||
There was a problem hiding this comment.
The MinLength import is unused in this file. Consider removing it to keep the imports clean.
| @@ -0,0 +1,7 @@ | |||
| import { IsEmail, IsNotEmpty, IsString, MinLength } from "class-validator"; | |||
There was a problem hiding this comment.
The MinLength import is unused in this file since only email validation is required.
| handleRequest(err, user) { | ||
| if (err || !user) { |
There was a problem hiding this comment.
The strategy returns { userId: payload.sub, username: payload.username } but the AuthUserPayload interface defines id, email, name fields. This mismatch means if this strategy is used, user data won't be correctly mapped.
|
|
||
| @Controller('mail') | ||
| export class MailController { | ||
| constructor(private readonly mailService: MailService) {} |
There was a problem hiding this comment.
The MailController has no endpoint handlers but exists in the dependency tree. The MailService is injected here but never exposed via HTTP routes - it's used directly by AuthService and UserService. This controller appears to be unnecessary.
|
|
||
| async validate(payload: any) { |
There was a problem hiding this comment.
The JWT strategy returns { userId: payload.sub, username: payload.username } but the auth service generates tokens with { id, email, name }. The sub and username fields won't exist in the payload, resulting in incorrect user data extraction. This should return { userId: payload.id, username: payload.name } or similar to match the token structure.
|
|
||
| @Controller('prisma') | ||
| export class PrismaController { | ||
| constructor(private readonly prismaService: PrismaService) {} |
There was a problem hiding this comment.
The PrismaController has no endpoint handlers but is registered in the module. This appears to be dead code.
| @Put('update-user-email') | ||
| updateUserEmail( | ||
| @Request() req: AuthenticatedRequest, | ||
| @Body() dataForUpdate: DataForUpdateEmailDto, | ||
| ) { | ||
| return this.userService.updateUserEmail(req.user.id, dataForUpdate); | ||
| } | ||
|
|
||
| @Put('update-user-password') | ||
| updateUserPassword( |
There was a problem hiding this comment.
The DataForUpdatePasswordDto is missing the confirmNewPassword field. According to the requirements, changing password must require 'old one, new password and confirmation'. Add @IsStrongPassword() decorated confirmNewPassword field with @MatchField('newPassword') validation.
| @Request() req: AuthenticatedRequest, | ||
| @Body() dataForUpdate: DataForUpdateNameDto, | ||
| ) { | ||
| return this.userService.updateUserName(req.user.id, dataForUpdate); | ||
| } |
There was a problem hiding this comment.
The DataForActivateNewEmailDto class is defined but never imported or used anywhere. Consider removing it to keep the code clean.
| import { PrismaPg } from '@prisma/adapter-pg'; | ||
| import 'dotenv/config'; | ||
| import { PrismaClient } from 'generated/prisma/client'; |
There was a problem hiding this comment.
The DataForActivateNewEmailDto is imported but never used in this file. This appears to be dead code that can be removed.
| @@ -0,0 +1,28 @@ | |||
| import { Injectable, OnModuleDestroy, OnModuleInit } from '@nestjs/common'; | |||
There was a problem hiding this comment.
The Query parameter decorator is imported but not used in any of the controller methods. Consider removing it from the imports.
| @@ -0,0 +1,39 @@ | |||
| import { IsEmail, IsNotEmpty, IsString } from 'class-validator'; | |||
There was a problem hiding this comment.
The Query import is unused in this file. Consider removing it to clean up imports.
|
|
||
| export class DataForActivateNewEmailDto { | ||
| @IsString() | ||
| @IsNotEmpty() | ||
| newName!: string; |
There was a problem hiding this comment.
The DataForActivateNewEmailDto class is defined but never used in the controller. It appears to be dead code.
| if (!user) { | ||
| throw new UnauthorizedException(); | ||
| } | ||
|
|
||
| return normalizeUser(user); | ||
| } | ||
|
|
||
| async updateUserName(userId: number, dataForUpdate: DataForUpdateNameDto) { | ||
| const newName = dataForUpdate.newName.trim(); | ||
| if (!newName) { |
There was a problem hiding this comment.
The DataForUpdatePasswordDto requires a confirmNewPassword field for password confirmation, but the frontend profile page sends only password (current) and newPassword without a confirmation field. This mismatch will cause validation to fail. Either add a confirmation field to the frontend form or remove the @MatchField validation from the backend DTO.
| try { | ||
| return jsonwebtoken.verify(token, getAccessSecret()); | ||
| } catch (error) { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| function validateRefreshToken(token: string) { | ||
| try { | ||
| return jsonwebtoken.verify(token, getRefreshSecret()); |
There was a problem hiding this comment.
The DataForUpdatePasswordDto is missing the confirmNewPassword field. According to the requirements, changing password must require 'old one, new password and confirmation'. Add a @IsStrongPassword() decorated confirmNewPassword field with @MatchField('newPassword') validation to ensure passwords match before the service receives the request.
|
|
||
| function getAccessSecret(): string { | ||
| const secret = process.env.JWT_ACCESS_SECRET; | ||
| if (!secret) { |
There was a problem hiding this comment.
The UnauthorizedException import is not used in this file. Consider removing it to clean up imports.
| "testEnvironment": "node", | ||
| "testRegex": ".e2e-spec.ts$", | ||
| "transform": { |
There was a problem hiding this comment.
The prisma.ts creates a Prisma client without checking if DATABASE_URL is defined. Unlike PrismaService in prisma.service.ts which validates the connection string, this utility will throw an unclear error if DATABASE_URL is missing. Consider adding validation: if (!process.env.DATABASE_URL) throw new Error('DATABASE_URL is not set')
| .then(() => { | ||
| if (!cancelled) { |
There was a problem hiding this comment.
The check if (!payload.normalizedUser) returns 401 'Unauthorized', but successful activation returns { message: 'Account activated', statusCode: 200 } without normalizedUser. The actual normalizedUser is sent via httpOnly cookies, not JSON body. This check will incorrectly treat successful activation as unauthorized.
| const url = `${base}/user`; | ||
|
|
||
| let res: Response; | ||
| try { | ||
| res = await fetch(url, { | ||
| method: 'GET', | ||
| headers: backendHeaders(request), | ||
| cache: 'no-store', | ||
| }); | ||
| } catch (err) { | ||
| const message = | ||
| err instanceof Error ? err.message : 'Failed to reach backend'; | ||
| return NextResponse.json( |
There was a problem hiding this comment.
If none of the conditions match (no newName, newEmail, or newPassword in body), the url remains empty string and a fetch request is made to an invalid URL. Consider returning a 400 error if url is empty before making the fetch request.
| let res: Response; | ||
| try { | ||
| res = await fetch(url, { | ||
| method: 'GET', | ||
| headers: backendHeaders(request), | ||
| }); | ||
| } catch (err) { |
There was a problem hiding this comment.
If the request body doesn't contain newName, newEmail, or newPassword, the url variable remains an empty string and the fetch request goes to a malformed URL. Consider adding a validation check or returning an error early if no update type is specified.
|
|
||
| const base = process.env.SERVER_PATH; | ||
| const url = `${base}/auth/sign-in`; | ||
|
|
||
| let res: Response; | ||
| try { | ||
| res = await fetch(url, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify(body), | ||
| }); | ||
| } catch (err) { | ||
| const message = | ||
| err instanceof Error ? err.message : 'Failed to reach backend'; | ||
| return NextResponse.json( | ||
| { message: `Backend fetch failed (${url}): ${message}` }, | ||
| { status: 502 }, | ||
| ); |
There was a problem hiding this comment.
If the request body doesn't contain newName, newEmail, or newPassword, the url variable remains an empty string and the fetch is called with an invalid URL. Consider validating that at least one update field is present, or returning a 400 error with a descriptive message.
| router.refresh(); | ||
| } catch(err){ | ||
| console.error(err) |
There was a problem hiding this comment.
The logout error is caught and only logged to console. If the sign-out API fails (e.g., network issue), the user sees no feedback and might assume they're logged out when they're not. Consider showing a toast notification on error.
|
|
||
| import { useUser } from "@/app/hooks/useUser"; | ||
| import { usePathname, useRouter } from "next/navigation"; | ||
|
|
There was a problem hiding this comment.
The component is named SingInButton but should be SignInButton (missing 'g'). This naming inconsistency could cause issues if importing with the correct name.
| type="button" | ||
| onClick={() => router.push('/sign-in')} | ||
| className="text-white w-fit cursor-pointer rounded-2xl border border-rose-700/35 bg-surface-panel p-6 text-center shadow-[inset_0_0_20px_rgba(244,63,94,0.14)] sm:p-8" | ||
| > |
There was a problem hiding this comment.
The button text says 'Got to sign in' but should be 'Go to sign in' (grammatical error).
| --color-foreground: var(--foreground); | ||
| --color-surface-panel: var(--bg-surface-panel); | ||
| --color-surface-input: var(--bg-surface-input); | ||
| --color-surface-card: var(--bg-surface-card); |
There was a problem hiding this comment.
The button text says 'Got to sign in' but should be 'Go to sign in' (grammar error: 'Got' vs 'Go').
| import Providers from "./providers"; | ||
| import LogoutButton from "./components/logoutButton"; | ||
| import SingInButton from "./components/singInButton"; | ||
|
|
||
| const geistSans = Geist({ | ||
| variable: "--font-geist-sans", | ||
| subsets: ["latin"], | ||
| }); |
There was a problem hiding this comment.
The AUTH_SKIP_PATHS array doesn't include /api/auth/reset-password, /api/auth/reset-password/confirmed, or /api/auth/activate-new-email. If a user has an expired session and attempts a password reset or email activation, the interceptor will try to refresh the token (failing), then redirect to sign-in. These are public pages that shouldn't trigger session recovery.
|
|
||
| const AUTH_SKIP_PATHS = [ | ||
| '/api/auth/refresh', | ||
| '/api/auth/sign-in', | ||
| '/api/auth/sign-out', | ||
| '/api/auth/register', | ||
| '/api/auth/activate', | ||
| '/api/auth/google', |
There was a problem hiding this comment.
The AUTH_SKIP_PATHS array doesn't include /api/auth/activate-new-email and /api/auth/reset-password/confirmed. These are authentication endpoints that shouldn't trigger token refresh. Consider adding them to prevent unnecessary refresh attempts.
|
|
||
| export const ProfilePasswordSchema = z | ||
| .object({ | ||
| currentPassword: z.string().min(1, 'Current password is required'), | ||
| newPassword: passwordSchema, | ||
| confirmPassword: z.string().min(1, 'Please confirm your new password'), | ||
| }) | ||
| .refine((data) => data.newPassword === data.confirmPassword, { | ||
| message: 'Passwords do not match', | ||
| path: ['confirmPassword'], | ||
| }); | ||
|
|
||
| export const ResetSchema = z.object({ | ||
| email: z.email(), | ||
| }); | ||
|
|
||
| export const ChangePasswordSchema = z | ||
| .object({ | ||
| newPassword: passwordSchema, | ||
| confirmPassword: z.string().min(1, 'Please confirm your new password'), | ||
| }) | ||
| .refine((data) => data.newPassword === data.confirmPassword, { | ||
| message: 'Passwords do not match', | ||
| path: ['confirmPassword'], |
There was a problem hiding this comment.
The frontend ProfilePasswordSchema sends currentPassword, newPassword, confirmPassword, but the backend DataForUpdatePasswordDto expects password (not currentPassword) and has no confirmPassword field. This mismatch means the backend won't validate password confirmation server-side.
| import Link from 'next/link' | ||
|
|
There was a problem hiding this comment.
The imports of LogoutButton and SingInButton are unused - the home page doesn't render them directly. Consider removing these imports to clean up the code.
| type ProfileNameInput = z.infer<typeof ProfileNameSchema>; | ||
| type ProfileEmailInput = z.infer<typeof ProfileEmailSchema>; | ||
| type ProfilePasswordInput = z.infer<typeof ProfilePasswordSchema>; | ||
|
|
||
| function UpdateNameForm({ defaultName }: { defaultName?: string }) { | ||
| const { | ||
| register, | ||
| handleSubmit, |
There was a problem hiding this comment.
The GoogleOAuthProvider opening tag is at a different indentation level than the closing tag. Looking at the structure, </GoogleOAuthProvider> on line 34 closes after the Toaster and QueryClientProvider, but the opening tag on line 27 appears to be at the same level as QueryClientProvider. This creates invalid JSX nesting. The closing tag should come immediately after </QueryClientProvider> without any siblings after it.
| </div> | ||
|
|
||
| </div> | ||
| ); |
There was a problem hiding this comment.
The form validates confirmPassword on the frontend with Zod but doesn't send it to the backend. According to the requirements, the Reset Password confirmation page should have 'password and confirmation fields that must be equal'. The backend CompleteResetPasswordDto should also validate that newPassword equals confirmPassword for security (currently only has resetToken and newPassword).
| return ( | ||
| <div className="mx-auto w-full max-w-md"> | ||
| <h2 className="mb-1 text-2xl font-semibold text-rose-50">Reset password</h2> | ||
|
|
There was a problem hiding this comment.
The frontend validates confirmPassword using ChangePasswordSchema but the API call only sends { resetToken, newPassword }. The confirmPassword value is not being sent to the backend. According to the task requirements, password and confirmation fields must be equal - this validation should be enforced server-side as well.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Several critical blockers prevent this solution from working correctly. The JWT payload mismatch in jwt.strategy.ts returns { userId, username } but tokens are signed with { id, email, name }, breaking all authenticated routes. The CompleteResetPasswordDto is missing a confirmPassword field required by the task - the reset password confirmation page must have password and confirmation fields that must be equal, but this is only validated on frontend. The update-user-data/route.ts makes a fetch call with an empty URL string when none of newName, newEmail, or newPassword are provided, causing a runtime error. The activation route at src/frontend/app/activate/page.tsx checks if (!payload.normalizedUser) to return 401, but the backend returns { message: 'Account activated', statusCode: 200 } without normalizedUser, incorrectly treating successful activation as unauthorized. Additionally, most BFF routes don't validate SERVER_PATH before constructing URLs, which will cause failures if the environment variable is undefined. These issues must be fixed before the authentication flow can work correctly.
✨ 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
| @@ -0,0 +1,39 @@ | |||
| import { IsEmail, IsNotEmpty, IsString } from 'class-validator'; | |||
There was a problem hiding this comment.
The Query import is unused in this file. Consider removing it to clean up imports.
|
|
||
| export class DataForActivateNewEmailDto { | ||
| @IsString() | ||
| @IsNotEmpty() | ||
| newName!: string; |
There was a problem hiding this comment.
The DataForActivateNewEmailDto class is defined but never used in the controller. It appears to be dead code.
| try { | ||
| return jsonwebtoken.verify(token, getAccessSecret()); | ||
| } catch (error) { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| function validateRefreshToken(token: string) { | ||
| try { | ||
| return jsonwebtoken.verify(token, getRefreshSecret()); |
There was a problem hiding this comment.
The DataForUpdatePasswordDto is missing the confirmNewPassword field. According to the requirements, changing password must require 'old one, new password and confirmation'. Add a @IsStrongPassword() decorated confirmNewPassword field with @MatchField('newPassword') validation to ensure passwords match before the service receives the request.
|
|
||
| function getAccessSecret(): string { | ||
| const secret = process.env.JWT_ACCESS_SECRET; | ||
| if (!secret) { |
There was a problem hiding this comment.
The UnauthorizedException import is not used in this file. Consider removing it to clean up imports.
| .then(() => { | ||
| if (!cancelled) { |
There was a problem hiding this comment.
The check if (!payload.normalizedUser) returns 401 'Unauthorized', but successful activation returns { message: 'Account activated', statusCode: 200 } without normalizedUser. The actual normalizedUser is sent via httpOnly cookies, not JSON body. This check will incorrectly treat successful activation as unauthorized.
|
|
||
| const base = process.env.SERVER_PATH; | ||
| const url = `${base}/auth/sign-in`; | ||
|
|
||
| let res: Response; | ||
| try { | ||
| res = await fetch(url, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify(body), | ||
| }); | ||
| } catch (err) { | ||
| const message = | ||
| err instanceof Error ? err.message : 'Failed to reach backend'; | ||
| return NextResponse.json( | ||
| { message: `Backend fetch failed (${url}): ${message}` }, | ||
| { status: 502 }, | ||
| ); |
There was a problem hiding this comment.
If the request body doesn't contain newName, newEmail, or newPassword, the url variable remains an empty string and the fetch is called with an invalid URL. Consider validating that at least one update field is present, or returning a 400 error with a descriptive message.
| --color-foreground: var(--foreground); | ||
| --color-surface-panel: var(--bg-surface-panel); | ||
| --color-surface-input: var(--bg-surface-input); | ||
| --color-surface-card: var(--bg-surface-card); |
There was a problem hiding this comment.
The button text says 'Got to sign in' but should be 'Go to sign in' (grammar error: 'Got' vs 'Go').
| type ProfileNameInput = z.infer<typeof ProfileNameSchema>; | ||
| type ProfileEmailInput = z.infer<typeof ProfileEmailSchema>; | ||
| type ProfilePasswordInput = z.infer<typeof ProfilePasswordSchema>; | ||
|
|
||
| function UpdateNameForm({ defaultName }: { defaultName?: string }) { | ||
| const { | ||
| register, | ||
| handleSubmit, |
There was a problem hiding this comment.
The GoogleOAuthProvider opening tag is at a different indentation level than the closing tag. Looking at the structure, </GoogleOAuthProvider> on line 34 closes after the Toaster and QueryClientProvider, but the opening tag on line 27 appears to be at the same level as QueryClientProvider. This creates invalid JSX nesting. The closing tag should come immediately after </QueryClientProvider> without any siblings after it.
| return ( | ||
| <div className="mx-auto w-full max-w-md"> | ||
| <h2 className="mb-1 text-2xl font-semibold text-rose-50">Reset password</h2> | ||
|
|
There was a problem hiding this comment.
The frontend validates confirmPassword using ChangePasswordSchema but the API call only sends { resetToken, newPassword }. The confirmPassword value is not being sent to the backend. According to the task requirements, password and confirmation fields must be equal - this validation should be enforced server-side as well.
|
|
||
| export class CreateAuthDto { | ||
| @IsString() | ||
| @IsNotEmpty() | ||
| name!: string; | ||
|
|
||
| @IsEmail({}, { message: 'Invalid email address' }) | ||
| @IsNotEmpty() | ||
| email!: string; | ||
|
|
There was a problem hiding this comment.
The CompleteResetPasswordDto is missing a confirmPassword field with @MatchField('newPassword') decorator. The task requires 'Reset Password confirmation page (with password and confirmation fields that must be equal)' but only newPassword is validated. Consider adding a confirmation field.
| HttpException, | ||
| Injectable, | ||
| UnauthorizedException, | ||
| } from '@nestjs/common'; | ||
| import type { Response } from 'express'; | ||
| import { CreateAuthDto } from './dto/create-auth-dto'; | ||
| import { PrismaService } from 'src/prisma/prisma.service'; | ||
| import bcrypt from 'bcrypt'; | ||
| import { createHash, randomBytes } from 'crypto'; | ||
| import { MailService } from 'src/mail/mail.service'; |
There was a problem hiding this comment.
The CompleteResetPasswordDto is missing a confirmPassword field. According to the task requirements, the reset password confirmation page must have 'password' and 'confirmation' fields that must be equal. Currently, only newPassword exists, and confirmation is handled only on the frontend which can be bypassed.
|
|
||
| res.cookie('accessToken', accessToken, { | ||
| httpOnly: true, | ||
| secure: false, // dev only | ||
| sameSite: "lax", | ||
| path: "/", | ||
| maxAge: 1 * 1 * 15 * 60 * 1000, | ||
| }); | ||
| res.cookie("refreshToken", refreshToken, { | ||
| httpOnly: true, | ||
| secure: false, // dev only | ||
| sameSite: "lax", | ||
| path: "/", | ||
| maxAge: 7 * 24 * 60 * 60 * 1000, | ||
| }); | ||
|
|
||
|
|
||
| res.status(200).json( | ||
| { | ||
| normalizedUser, | ||
| accessToken, | ||
| }) | ||
| return { | ||
| normalizedUser, | ||
| accessToken, | ||
| } |
There was a problem hiding this comment.
The login() method (lines 186-211) duplicates cookie-setting logic that already exists in issueSession(). Consider using issueSession() consistently for both login and activation flows to avoid code duplication and ensure consistent cookie configuration.
| } | ||
| } catch (error) { | ||
| throw new BadRequestException({ | ||
| statusCode: 400, | ||
| message: "Can't create a new user", | ||
| error: 'Bad Request', | ||
| }); |
There was a problem hiding this comment.
The create() catch block (line 94-100) catches all errors and returns the same generic message, losing important error details. Consider distinguishing between unique constraint violations (email already exists) and other errors.
| import { IsNotEmpty, IsString } from 'class-validator'; | ||
| import { IsStrongPassword } from 'src/common/validators/is-strong-password.decorator'; | ||
|
|
||
| export class CompleteResetPasswordDto { | ||
| @IsString() | ||
| @IsNotEmpty() | ||
| resetToken!: string; | ||
|
|
||
| @IsString() | ||
| @IsNotEmpty() | ||
| @IsStrongPassword() | ||
| newPassword!: string; |
There was a problem hiding this comment.
The CompleteResetPasswordDto only has newPassword but the task requires a confirmation page with both password and confirmation fields that must be equal. The backend should validate that the new password and confirmation match before accepting the reset.
| validator: { | ||
| validate(value: unknown, args: ValidationArguments) { | ||
| const [relatedPropertyName] = args.constraints as [string]; |
There was a problem hiding this comment.
The JwtStrategy.validate() returns { userId: payload.sub, username: payload.username } but AuthenticatedRequest expects a user property of type AuthUserPayload with id, email, name. The return values don't match. Consider returning { id: payload.sub, email: payload.email, name: payload.name } to align with the expected type.
| @Injectable() | ||
| export class JwtStrategy extends PassportStrategy(Strategy) { | ||
| constructor() { | ||
| super({ |
There was a problem hiding this comment.
The JwtStrategy extracts JWT from Authorization Bearer header using ExtractJwt.fromAuthHeaderAsBearerToken(), but the auth.service.ts stores tokens in httpOnly cookies (req.cookies.refreshToken, etc.). If the JWT auth guard uses this strategy for cookie-based auth, it won't work. Consider using ExtractJwt.fromExtractors to also extract from cookies.
|
|
||
| async validate(payload: any) { | ||
| return { userId: payload.sub, username: payload.username }; |
There was a problem hiding this comment.
The validate method returns { userId: payload.sub, username: payload.username } but this doesn't match the expected AuthUserPayload structure (which likely expects id, email, name). The authenticated-request.ts expects req.user to have the proper payload shape.
| constructor() { | ||
| super({ | ||
| jwtFromRequest: ExtractJwt.fromAuthHeaderAsBearerToken(), | ||
| ignoreExpiration: false, |
There was a problem hiding this comment.
Falling back to an empty string || '' for JWT secretOrKey is a security risk. If the environment variable is not set, tokens will be validated with an empty secret, making them easily forgeable. Consider throwing an error instead.
| handleRequest(err, user) { | ||
| if (err || !user) { | ||
| throw err || new UnauthorizedException(); |
There was a problem hiding this comment.
The JwtStrategy's validate() method returns { userId: payload.sub, username: payload.username }, but the JWT tokens are generated with { id, email, name } payload fields (see jwt.generateAccessToken). This mismatch means the validated user object won't have the correct properties. The validate method should return { id: payload.id, email: payload.email, name: payload.name } or whatever fields are actually in the JWT payload.
| } from './dto/data-for-update-dto'; | ||
| import { JwtAuthGuard } from 'src/common/guards/jwt-auth.guard'; |
There was a problem hiding this comment.
The user.controller.ts imports JwtAuthGuard from 'src/common/guards/jwt-auth.guard' but I don't see this file in the provided context. The JwtAuthGuard was shown in 'src/jwt/jwt.auth.guard.ts'. Verify that the guard exists at the imported path or update the import path.
| } from './dto/data-for-update-dto'; | ||
| import { PrismaService } from 'src/prisma/prisma.service'; | ||
| import { normalizeUser } from 'src/utils/normalizeUser'; | ||
| import { MailService } from 'src/mail/mail.service'; | ||
| import { createHash, randomBytes } from 'crypto'; |
There was a problem hiding this comment.
The UserModule's JwtModule configuration only sets secret: process.env.JWT_ACCESS_SECRET but the auth.service.ts refresh method uses jwtService.verifyAsync(refreshToken, { secret: process.env.JWT_REFRESH_SECRET }). The JWT module should be configured with both access and refresh secrets, or use a different approach for refresh token verification.
|
|
||
| async updateUserPassword( | ||
| userId: number, | ||
| dataForUpdate: DataForUpdatePasswordDto, | ||
| ) { | ||
| const user = await this.prisma.user.findUnique({ | ||
| where: { id: userId }, | ||
| }); | ||
|
|
||
| if (!user) { | ||
| throw new NotFoundException('User not found'); | ||
| } | ||
|
|
||
| const passwordValid = await bcrypt.compare( | ||
| dataForUpdate.password, | ||
| user.password || '', | ||
| ); | ||
|
|
||
| if (!passwordValid) { | ||
| throw new BadRequestException({ | ||
| message: 'Invalid password', | ||
| code: 'INVALID_PASSWORD', | ||
| }); | ||
| } | ||
|
|
||
| const hashedPassword = await bcrypt.hash(dataForUpdate.newPassword, 10); | ||
| await this.prisma.user.update({ | ||
| where: { id: userId }, | ||
| data: { | ||
| password: hashedPassword, | ||
| }, | ||
| }); | ||
|
|
||
| return { | ||
| statusCode: 200, | ||
| message: 'ok', | ||
| }; |
There was a problem hiding this comment.
After updating the password in updateUserPassword, the user's existing refresh token hash becomes invalid since the password change should invalidate all sessions. Consider clearing the refreshAccessTokenHash in the database to force re-login, which is a security best practice.
| import { Test, TestingModule } from '@nestjs/testing'; | ||
| import { INestApplication } from '@nestjs/common'; | ||
| import request from 'supertest'; | ||
| import { App } from 'supertest/types'; | ||
| import { AppModule } from './../src/app.module'; | ||
|
|
||
| describe('AppController (e2e)', () => { | ||
| let app: INestApplication<App>; | ||
|
|
There was a problem hiding this comment.
The prisma.ts file creates a standalone Prisma client instance that doesn't appear to be used anywhere in the application (PrismaService is used instead). This could cause connection issues or resource waste. Either use this instance or remove it.
| import "dotenv/config"; | ||
| import { PrismaPg } from "@prisma/adapter-pg"; | ||
| import { PrismaClient } from "generated/prisma/client"; | ||
|
|
There was a problem hiding this comment.
The prisma.ts utility doesn't validate that DATABASE_URL exists before creating the PrismaClient. If DATABASE_URL is undefined, the adapter will be created with an invalid connection string. Consider adding a check similar to prisma.service.ts.
| "moduleFileExtensions": ["js", "json", "ts"], | ||
| "rootDir": ".", | ||
| "testEnvironment": "node", |
There was a problem hiding this comment.
The googleClient.ts creates an OAuth2Client with process.env.GOOGLE_CLIENT_ID without validation. If this environment variable is undefined, the client will be created with undefined credentials and will fail silently. Consider adding a check or using the same pattern as the MailService constructor that throws an error if required environment variables are missing.
| { | ||
| "moduleFileExtensions": ["js", "json", "ts"], | ||
| "rootDir": ".", | ||
| "testEnvironment": "node", | ||
| "testRegex": ".e2e-spec.ts$", | ||
| "transform": { | ||
| "^.+\\.(t|j)s$": "ts-jest" | ||
| } | ||
| } |
There was a problem hiding this comment.
The prisma.ts utility file creates a standalone PrismaClient separate from the PrismaService. This could cause issues: 1) multiple database connections if both are used, 2) the DATABASE_URL is not validated before use, 3) there's no dotenv validation. If this file is used as an alternative to PrismaService, consider removing it or ensuring it's not creating duplicate connections.
| "testEnvironment": "node", | ||
| "testRegex": ".e2e-spec.ts$", |
There was a problem hiding this comment.
In prisma.ts, the connectionString variable is created with a template literal ${process.env.DATABASE_URL} which is redundant - it could just be process.env.DATABASE_URL. This doesn't cause a bug but is unnecessary.
|
|
||
| export default client; No newline at end of file |
There was a problem hiding this comment.
The connectionString on line 4 uses an unnecessary template literal. Change const connectionString = \${process.env.DATABASE_URL}`;toconst connectionString = process.env.DATABASE_URL;` for cleaner code.
| import { OAuth2Client } from "google-auth-library"; | ||
|
|
||
| const client = new OAuth2Client(process.env.GOOGLE_CLIENT_ID); | ||
|
|
There was a problem hiding this comment.
The prisma.ts file doesn't validate that DATABASE_URL exists before creating the connection. If the environment variable is missing, the error message won't be clear. Consider adding validation similar to PrismaService.
| <div | ||
| className="mb-4 flex h-12 w-12 items-center justify-center rounded-2xl border border-rose-500/30 bg-gradient-to-br from-rose-600/25 to-fuchsia-700/20 shadow-[0_0_20px_rgba(244,63,94,0.2)]" | ||
| aria-hidden |
There was a problem hiding this comment.
On line 6, process.env.SERVER_PATH is used without validation. If SERVER_PATH is undefined, the URL becomes 'undefined/auth/activate...' which will fail. Consider adding a check or providing a default value like const base = process.env.SERVER_PATH ?? 'http://localhost:4000';
|
|
||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
The routes don't validate that SERVER_PATH is defined before using it. If process.env.SERVER_PATH is undefined, the URL will be undefined/auth/activate... which will fail. Consider adding a check similar to the refresh route.
| const activateToken = new URL(request.url).searchParams.get('activateToken'); | ||
|
|
||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
If activateToken is null (line 5), the URL becomes /auth/activate?activateToken=null which will be sent to the backend. Consider returning a 400 error if the token is missing instead of sending 'null' as a string.
| try { | ||
| res = await fetch(url, { | ||
| method: 'GET', | ||
| headers: { 'Content-Type': 'application/json' }, |
There was a problem hiding this comment.
The activation routes don't pass existing cookies to the backend fetch. While this is fine for email link activation (stateless), it means existing sessions aren't used. The routes should use backendHeaders(request) to forward cookies, ensuring the user stays authenticated after activation.
|
|
||
| async function proxyRefresh(request: Request) { |
There was a problem hiding this comment.
The google/route.ts doesn't validate that SERVER_PATH is defined before using it. If undefined, the URL becomes 'undefined/auth/google'. Compare with refresh/route.ts lines 16-21 which properly validates this.
| import { NextResponse } from 'next/server'; | ||
|
|
There was a problem hiding this comment.
The reset-password/confirmed/route.ts doesn't validate SERVER_PATH before constructing the URL. Consider adding validation similar to the refresh route pattern.
| return NextResponse.json( | ||
| { message: 'Refresh token missing' }, | ||
| { status: 401 }, | ||
| ); |
There was a problem hiding this comment.
The google/route.ts doesn't pass cookies from the incoming request to the backend. If the user already has an existing session, the Google sign-in should not interfere with it. Consider using backendHeaders(request) or similar to forward cookies.
| return NextResponse.json( | ||
| { message: 'Refresh token missing' }, | ||
| { status: 401 }, |
There was a problem hiding this comment.
The reset-password/confirmed/route.ts doesn't pass cookies to the backend. While password reset doesn't require existing authentication, consistent cookie handling is good practice.
| const base = process.env.SERVER_PATH; | ||
| const url = `${base}/auth/reset-password/confirmed`; |
There was a problem hiding this comment.
The google/route.ts doesn't validate that SERVER_PATH is defined. If undefined, the URL becomes 'undefined/auth/google' which will fail. Consider adding validation similar to the refresh route.
| import { NextResponse } from 'next/server'; | ||
|
|
||
| export async function POST(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
The reset-password routes don't validate SERVER_PATH before constructing URLs. If undefined, the URLs will contain 'undefined' as a string, causing fetch failures.
| if (raw) { | ||
| try { | ||
| payload = JSON.parse(raw) as Record<string, unknown>; | ||
| } catch { |
There was a problem hiding this comment.
The refreshHeaders() function in refresh/route.ts filters out accessToken from cookies to only send refreshToken to the backend. This is correct, but the function should also validate that the request parameter is properly passed to refreshHeaders.
| export async function POST(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
The google/route.ts, reset-password/route.ts, and reset-password/confirmed/route.ts don't validate SERVER_PATH before using it. If undefined, URLs become 'undefined/auth/google' which will fail. The refresh route has this validation - consider adding it to the other routes for consistency.
| export async function POST(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
All routes except refresh/route.ts don't validate that process.env.SERVER_PATH is defined before using it. If SERVER_PATH is undefined, URLs become 'undefined/auth/...' which will fail. Consider adding validation similar to the refresh route (lines 15-21 in refresh/route.ts).
| const body = await request.json(); | ||
| let url = ''; | ||
|
|
||
| if (body.newName) { | ||
| url = `${base}/user/update-user-name`; | ||
| } else if (body.newEmail) { | ||
| url = `${base}/user/update-user-email`; | ||
| } else if (body.newPassword) { | ||
| url = `${base}/user/update-user-password`; |
There was a problem hiding this comment.
The update-user-data/route.ts initializes url = '' and only sets it if one of the conditions is met (lines 6-14). If the request body has none of the expected fields, the URL will be an empty string, causing the fetch to fail. Consider returning a 400 error if no valid update type is provided.
| export async function PUT(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
All three routes (sign-out, user, update-user-data) don't validate that SERVER_PATH is defined before constructing URLs. If undefined, URLs become 'undefined/auth/...' which will fail silently. Consider adding validation similar to the refresh route.
| @@ -0,0 +1,47 @@ | |||
| import { applySetCookies, backendHeaders } from '@/app/lib/bff'; | |||
| import { NextResponse } from 'next/server'; | |||
|
|
|||
There was a problem hiding this comment.
The sign-out route uses GET method which is less secure than POST for logout operations. However, since the backend also uses GET, this is consistent with the backend implementation.
| primaryHref?: string; | ||
| primaryLabel?: string; | ||
| } | ||
|
|
||
| export default function AuthLinkError({ | ||
| title = 'Invalid or expired link', | ||
| description = 'This link is missing or has expired. Request a new one and try again.', | ||
| primaryHref = '/sign-in', | ||
| primaryLabel = 'Go to sign in', |
There was a problem hiding this comment.
The update-user-data route initializes url as an empty string and only sets it if body.newName, body.newEmail, or body.newPassword exists. If none of these conditions match (e.g., body is empty or has unexpected properties), the URL remains '' and line 18 makes a fetch to an empty URL. Consider adding validation to return a 400 error if no valid update type is specified.
| title?: string; | ||
| description?: string; |
There was a problem hiding this comment.
The sign-out, user, and update-user-data routes don't validate SERVER_PATH before using it. If undefined, the URLs become 'undefined/auth/sign-out' or 'undefined/user'. Consider adding validation similar to the refresh route.
| export async function GET(request: Request) { | ||
| const base = process.env.SERVER_PATH; | ||
| const url = `${base}/user`; | ||
|
|
||
| let res: Response; | ||
| try { | ||
| res = await fetch(url, { | ||
| method: 'GET', | ||
| headers: backendHeaders(request), | ||
| cache: 'no-store', | ||
| }); |
There was a problem hiding this comment.
The update-user-data route doesn't validate SERVER_PATH and doesn't handle the case where url remains empty (when none of body.newName, body.newEmail, or body.newPassword are provided). The fetch will fail with 'Failed to fetch' or similar. Consider validating SERVER_PATH and returning a 400 error if no update field is provided.
| export async function GET(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
All three routes (sign-out, user, update-user-data) don't validate SERVER_PATH before using it. If undefined, URLs become 'undefined/auth/...' or 'undefined/user/...' which will fail.
| let res: Response; | ||
| try { | ||
| res = await fetch(url, { | ||
| method: 'GET', | ||
| headers: backendHeaders(request), | ||
| }); | ||
| } catch (err) { |
There was a problem hiding this comment.
In update-user-data/route.ts, if none of body.newName, body.newEmail, or body.newPassword are truthy, url remains an empty string (line 6). The fetch call on line 18 would then send a request to an empty URL, which could cause unexpected behavior. Consider adding validation to reject invalid requests early.
| @@ -0,0 +1,31 @@ | |||
| import { applySetCookies, backendHeaders } from '@/app/lib/bff'; | |||
| import { NextResponse } from 'next/server'; | |||
|
|
|||
There was a problem hiding this comment.
The sign-out route uses GET method, which could be exploited via CSRF attacks. Consider changing to POST method to align with security best practices for logout actions.
| export async function GET(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
These routes don't validate SERVER_PATH before constructing URLs. If undefined, URLs will contain 'undefined' as a string. Compare with refresh/route.ts which properly validates this.
| }: PasswordRulesPopoverProps) { | ||
| const [focused, setFocused] = useState(false); |
There was a problem hiding this comment.
The singInButton.tsx has a typo: 'Got to sign in' on line 20 should be 'Go to sign in'. This is a minor UX issue that should be corrected.
|
|
||
| const rules = useMemo( | ||
| () => PASSWORD_CHECKS.map((rule) => ({ ...rule, met: rule.test(password) })), |
There was a problem hiding this comment.
The logoutButton.tsx catches errors but only logs them to console without any user feedback. Consider showing a toast notification to inform the user if logout failed.
| > | ||
| Got to sign in |
There was a problem hiding this comment.
There's a typo on line 20: 'Got to sign in' should be 'Go to sign in'. This is a minor UI text error.
| router.refresh(); | ||
| } catch(err){ | ||
| console.error(err) |
There was a problem hiding this comment.
The logoutButton catches errors in handleLogout but only logs them to console.error without notifying the user. Consider showing a toast message so users know if logout failed.
| queryClient.removeQueries({ queryKey: ["user"] }); | ||
| router.push("/sign-in"); |
There was a problem hiding this comment.
The singInButton has a typo on line 20: 'Got to sign in' should be 'Go to sign in'. This is a minor cosmetic issue.
| import Providers from "./providers"; | ||
| import LogoutButton from "./components/logoutButton"; | ||
| import SingInButton from "./components/singInButton"; | ||
|
|
||
| const geistSans = Geist({ | ||
| variable: "--font-geist-sans", | ||
| subsets: ["latin"], | ||
| }); |
There was a problem hiding this comment.
The AUTH_SKIP_PATHS array (lines 4-11) doesn't include '/api/auth/reset-password' and '/api/auth/reset-password/confirmed'. If a user makes these requests while their access token is expired, the interceptor will try to refresh, which may cause unnecessary token refresh attempts.
| import { api } from '@/app/lib/api'; | ||
| import axios from 'axios'; | ||
|
|
||
| const fetchUser = async () => { | ||
| try { | ||
| const { data } = await api.get('/api/user'); | ||
|
|
||
| return data; |
There was a problem hiding this comment.
The AUTH_SKIP_PATHS doesn't include '/api/auth/reset-password' and '/api/auth/reset-password/confirmed'. These endpoints are for non-authenticated users and should skip token refresh attempts, otherwise a 401 on these routes would trigger an infinite refresh loop.
| </h2> | ||
| <p className="mb-6 text-sm leading-relaxed text-rose-100/75 text-center"> | ||
| If an account with that email exists, we've sent a password reset link. |
There was a problem hiding this comment.
The GoogleOAuthProvider on line 27 uses process.env.NEXT_PUBLIC_GOOGLE_CLIENT_ID! with non-null assertion. If this environment variable is not set, the Google OAuth flow will fail silently or throw an error. Consider adding validation or a fallback.
| "use client"; | ||
|
|
There was a problem hiding this comment.
The page.tsx imports LogoutButton and SingInButton on lines 1-2 but never uses them. These unused imports should be removed to keep the code clean.
| }, []); | ||
|
|
||
| return ( | ||
| <GoogleOAuthProvider |
There was a problem hiding this comment.
In providers.tsx line 28, process.env.NEXT_PUBLIC_GOOGLE_CLIENT_ID! uses a non-null assertion. If this environment variable is not set, the application will crash at runtime when Google OAuth is used. Consider adding a check or providing a fallback.
| "use client"; | ||
|
|
There was a problem hiding this comment.
The page.tsx imports LogoutButton and SingInButton but doesn't use them. These imports are unnecessary and add to the bundle size. Either remove them or use them in the component.
| const resetToken = searchParams.get("resetToken"); | ||
| const [fatalError, setFatalError] = useState<string | null>(null); | ||
|
|
||
| const { |
There was a problem hiding this comment.
The providers.tsx uses process.env.NEXT_PUBLIC_GOOGLE_CLIENT_ID! with a non-null assertion without validating it exists. If this env variable is missing, the Google OAuth provider will fail silently. Consider adding a check or validation.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation has four critical issues that must be fixed before approval:
1. JWT payload mismatch in jwt.strategy.ts (lines 11-13): The validate method returns { userId: payload.sub, username: payload.username } but JWT tokens are signed with { id, email, name } from normalizeUser. This breaks all authenticated routes that depend on the token payload structure. Update the validate method to return { id: payload.id, email: payload.email, name: payload.name }.
2. Missing confirmPassword field in CompleteResetPasswordDto (backend/src/auth/dto/complete-reset-password-dto.ts): The task explicitly requires the reset password confirmation page to have both password and confirmation fields that must be equal, with server-side validation. Currently only newPassword exists. Add confirmPassword field with @IsStrongPassword() and @MatchField('newPassword') decorators.
3. Activation routes return 401 on successful activation (src/frontend/app/api/auth/activate/route.ts, activate-new-email/route.ts, google/route.ts): Lines check if (!payload?.normalizedUser) to return 401, but the backend auth.service.ts activate method returns { message: 'Account activated', statusCode: 200 } without normalizedUser. This incorrectly treats successful activations as unauthorized. Either adjust the BFF to check response.ok directly, or ensure the backend includes normalizedUser in the JSON response body.
4. Missing SERVER_PATH validation in BFF routes: Most routes (register/route.ts, sign-in/route.ts, sign-out/route.ts, user/route.ts, reset-password/route.ts, reset-password/confirmed/route.ts, update-user-data/route.ts) don't validate process.env.SERVER_PATH before constructing URLs. If undefined, URLs become undefined/auth/... causing runtime failures. Add validation like the refresh/route.ts pattern: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
Minor: The typo in singInButton.tsx line 20: 'Got to sign in' should be 'Go to sign in'.
Note: The ESLint parsing errors are a configuration issue (missing TypeScript parser in ESLint config), not a code implementation problem—focus on fixing the critical issues above.
✨ 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
| import { IsEmail, IsNotEmpty, IsString } from 'class-validator'; | ||
| import { IsStrongPassword } from 'src/common/validators/is-strong-password.decorator'; | ||
|
|
||
| export class CreateAuthDto { | ||
| @IsString() | ||
| @IsNotEmpty() | ||
| name!: string; | ||
|
|
||
| @IsEmail({}, { message: 'Invalid email address' }) | ||
| @IsNotEmpty() | ||
| email!: string; | ||
|
|
There was a problem hiding this comment.
The CompleteResetPasswordDto only has resetToken and newPassword fields. According to the task requirements, the reset password confirmation page must have 'password' and 'confirmation' fields that must be equal. The confirmPassword field is missing from this DTO - it's only validated on the frontend but not on the backend.
| import { | ||
| BadRequestException, | ||
| HttpException, | ||
| Injectable, | ||
| UnauthorizedException, | ||
| } from '@nestjs/common'; | ||
| import type { Response } from 'express'; | ||
| import { CreateAuthDto } from './dto/create-auth-dto'; | ||
| import { PrismaService } from 'src/prisma/prisma.service'; | ||
| import bcrypt from 'bcrypt'; | ||
| import { createHash, randomBytes } from 'crypto'; | ||
| import { MailService } from 'src/mail/mail.service'; |
There was a problem hiding this comment.
The CompleteResetPasswordDto is missing the confirmPassword field. The task requirements explicitly state the reset password confirmation page must have password and confirmation fields that must be equal. This cross-field validation should be enforced on the backend DTO using the MatchField decorator, not just on the frontend.
| import { IsNotEmpty, IsString } from 'class-validator'; | ||
| import { IsStrongPassword } from 'src/common/validators/is-strong-password.decorator'; | ||
|
|
||
| export class CompleteResetPasswordDto { | ||
| @IsString() | ||
| @IsNotEmpty() | ||
| resetToken!: string; | ||
|
|
||
| @IsString() | ||
| @IsNotEmpty() | ||
| @IsStrongPassword() | ||
| newPassword!: string; |
There was a problem hiding this comment.
The CompleteResetPasswordDto is missing confirmPassword field. According to the task requirements, the reset password confirmation page must have password and confirmation fields that must be equal. Currently only newPassword exists, but the frontend sends confirmPassword as well - this validation should be done on the backend DTO using the MatchField decorator.
| import { Module } from '@nestjs/common'; | ||
| import { JwtModule } from '@nestjs/jwt'; | ||
| import { AuthService } from './auth.service'; | ||
| import { AuthController } from './auth.controller'; | ||
| import { PrismaModule } from 'src/prisma/prisma.module'; | ||
| import { MailModule } from 'src/mail/mail.module'; | ||
|
|
||
| @Module({ | ||
| controllers: [AuthController], | ||
| providers: [AuthService], | ||
| imports: [ | ||
| PrismaModule, |
There was a problem hiding this comment.
The CompleteResetPasswordDto only validates newPassword but is missing confirmPassword field. According to the task requirements, the reset password confirmation page must have password and confirmation fields that must be equal. Currently, the password match validation only happens on the frontend - the backend should also validate that both fields match.
|
|
||
| async validate(payload: any) { | ||
| return { userId: payload.sub, username: payload.username }; |
There was a problem hiding this comment.
The validate method returns { userId: payload.sub, username: payload.username } but JWT tokens are signed with { id, email, name }. This payload mismatch will break authenticated routes. The validate method should return { id: payload.id, email: payload.email, name: payload.name } to match the token structure.
| handleRequest(err, user) { | ||
| if (err || !user) { | ||
| throw err || new UnauthorizedException(); |
There was a problem hiding this comment.
The validate method returns { userId, username } but JWT tokens are signed with { id, email, name } from normalizeUser. This payload mismatch will cause authenticated routes to fail. The validate method should return the correct structure that matches the token payload: { id: payload.id, email: payload.email, name: payload.name }.
|
|
||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
The BFF routes don't validate SERVER_PATH before constructing URLs. If process.env.SERVER_PATH is undefined, url becomes undefined/auth/activate... which will fail. Add validation like other routes (e.g., /api/auth/refresh/route.ts has this check).
|
|
||
| if (!payload?.normalizedUser) { | ||
| return NextResponse.json({ message: 'Unauthorized' }, { status: 401 }); |
There was a problem hiding this comment.
The check if (!payload?.normalizedUser) returns 401, but this incorrectly treats successful activation as unauthorized. The backend returns normalizedUser on success, so this check should pass. However, if the backend response format changes or is malformed, this could cause issues.
|
|
||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
These BFF routes don't validate SERVER_PATH before constructing the URL. If process.env.SERVER_PATH is undefined, url becomes undefined/auth/activate?activateToken=... which will cause fetch to fail. Should add validation like: if (!base) { return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 }); } before using the base URL.
|
|
||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
Same issue: SERVER_PATH is not validated before being used to construct the URL.
| export async function GET(request: Request) { | ||
| const activateToken = new URL(request.url).searchParams.get('activateToken'); |
There was a problem hiding this comment.
Same issue: SERVER_PATH is not validated before being used to construct the URL.
| const url = `${base}/auth/google`; | ||
| const body = await request.json(); |
There was a problem hiding this comment.
Missing SERVER_PATH validation. If process.env.SERVER_PATH is undefined, url becomes undefined/auth/activate?activateToken=... which will cause the fetch to fail. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This check if (!payload?.normalizedUser) returns 401, but the backend returns { message: 'Account activated', statusCode: 200 } without normalizedUser on successful activation. The condition should be removed or modified to allow successful activation responses through.
| import { useEffect, useState } from "react"; | ||
| import AuthLinkError from "@/app/components/auth-link-error"; |
There was a problem hiding this comment.
The BFF routes don't validate that SERVER_PATH is defined before constructing URLs. If process.env.SERVER_PATH is undefined, the URL becomes undefined/auth/... which will cause a runtime error during the fetch call. Consider adding validation similar to the refresh route which checks: if (!base) { return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 }); }
| import { useEffect, useState } from "react"; | ||
| import AuthLinkError from "@/app/components/auth-link-error"; |
There was a problem hiding this comment.
Same issue: SERVER_PATH is not validated before constructing the URL. If undefined, the URL will be malformed.
| import axios from "axios"; | ||
| import { useRouter, useSearchParams } from "next/navigation"; |
There was a problem hiding this comment.
Same issue: SERVER_PATH is not validated before constructing the URL.
| const base = process.env.SERVER_PATH; | ||
| if (!base) { | ||
| return NextResponse.json( | ||
| { message: 'SERVER_PATH is not configured' }, | ||
| { status: 500 }, | ||
| ); |
There was a problem hiding this comment.
This route is correctly implementing SERVER_PATH validation (lines 16-21) - good pattern to follow.
|
|
||
| async function proxyRefresh(request: Request) { |
There was a problem hiding this comment.
Missing SERVER_PATH validation. If undefined, URL becomes undefined/auth/register which will fail. Add validation similar to refresh/route.ts: check if base is defined before using it.
| import { NextResponse } from 'next/server'; | ||
|
|
There was a problem hiding this comment.
Missing SERVER_PATH validation. If undefined, URL becomes undefined/auth/reset-password/confirmed which will fail. Add validation like refresh/route.ts.
|
|
||
| async function proxyRefresh(request: Request) { |
There was a problem hiding this comment.
Missing SERVER_PATH validation. If undefined, URL becomes undefined/auth/reset-password which will fail. Add validation like refresh/route.ts.
| } catch (err) { | ||
| const message = | ||
| err instanceof Error ? err.message : 'Failed to reach backend'; | ||
| return NextResponse.json( | ||
| { message: `Backend fetch failed (${url}): ${message}` }, | ||
| { status: 502 }, | ||
| ); |
There was a problem hiding this comment.
This route is correctly implemented with proper SERVER_PATH validation (lines 15-21). It should serve as the pattern for other routes.
| const base = process.env.SERVER_PATH; | ||
| const url = `${base}/auth/reset-password/confirmed`; |
There was a problem hiding this comment.
Missing SERVER_PATH validation. If process.env.SERVER_PATH is undefined, url becomes undefined/auth/register which will fail. Add validation like refresh/route.ts: if (!base) { return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 }); }
| export async function POST(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
Missing SERVER_PATH validation. If process.env.SERVER_PATH is undefined, url becomes undefined/auth/reset-password/confirmed which will fail.
| const base = process.env.SERVER_PATH; | ||
| const url = `${base}/auth/reset-password/confirmed`; |
There was a problem hiding this comment.
Missing SERVER_PATH validation. If process.env.SERVER_PATH is undefined, url becomes undefined/auth/reset-password which will fail.
| export async function POST(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
Missing SERVER_PATH validation - if undefined, URL becomes undefined/auth/register.... The refresh route (lines 16-21) shows the correct pattern: check if (!base) and return 500 error before using the URL.
|
|
||
| export async function POST(request: Request) { |
There was a problem hiding this comment.
Missing SERVER_PATH validation - if undefined, URL becomes undefined/auth/reset-password/confirmed.... Should validate before constructing the URL.
| export async function POST(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
Missing SERVER_PATH validation - if undefined, URL becomes undefined/auth/reset-password.... Should validate before constructing the URL.
| }); | ||
| } catch (err) { | ||
| const message = | ||
| err instanceof Error ? err.message : 'Failed to reach backend'; | ||
| return NextResponse.json( | ||
| { message: `Backend fetch failed (${url}): ${message}` }, | ||
| { status: 502 }, |
There was a problem hiding this comment.
This route correctly validates SERVER_PATH before constructing the URL (lines 16-21), returning a 500 error if not configured. Other routes should follow this pattern.
| export async function POST(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
Missing SERVER_PATH validation. If process.env.SERVER_PATH is undefined, url becomes undefined/auth/register which will cause the fetch to fail. Add validation similar to the refresh route.
|
|
||
| export async function POST(request: Request) { |
There was a problem hiding this comment.
Missing SERVER_PATH validation. If undefined, url becomes undefined/auth/reset-password/confirmed.
| export async function POST(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
Missing SERVER_PATH validation. If undefined, url becomes undefined/auth/reset-password.
|
|
||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
Missing SERVER_PATH validation. If process.env.SERVER_PATH is undefined, url becomes undefined/auth/sign-in which will fail. Add validation like the refresh route: check if base is defined before using it.
| export async function POST(request: Request) { | ||
| const body = await request.json(); |
There was a problem hiding this comment.
Missing SERVER_PATH validation. If undefined, URL becomes undefined/auth/sign-out.
| export async function POST(request: Request) { | ||
| const body = await request.json(); |
There was a problem hiding this comment.
Missing SERVER_PATH validation. If undefined, URL becomes undefined/user.
| const url = `${base}/auth/sign-in`; | ||
|
|
||
| let res: Response; | ||
| try { | ||
| res = await fetch(url, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, |
There was a problem hiding this comment.
Missing SERVER_PATH validation. If base is undefined, the URL construction on lines 9, 11, 13 will produce malformed URLs like undefined/user/update-user-name. Should validate base before constructing URLs.
| const url = `${base}/user`; | ||
|
|
There was a problem hiding this comment.
Missing SERVER_PATH validation. If process.env.SERVER_PATH is undefined, url becomes undefined/auth/sign-in which will fail. Add validation like refresh/route.ts.
| export async function GET(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
Missing SERVER_PATH validation. If undefined, url becomes undefined/auth/sign-out.
| export async function GET(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
Missing SERVER_PATH validation. If undefined, url becomes undefined/user.
| try { | ||
| res = await fetch(url, { | ||
| method: 'GET', | ||
| headers: backendHeaders(request), | ||
| cache: 'no-store', |
There was a problem hiding this comment.
Missing SERVER_PATH validation. Lines 9-13 use base without checking if it's defined first.
| const url = `${base}/auth/sign-out`; | ||
|
|
There was a problem hiding this comment.
Missing SERVER_PATH validation. If undefined, URL becomes undefined/auth/sign-in. Add validation like refresh route.
| export async function GET(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
Missing SERVER_PATH validation. If undefined, URL becomes undefined/auth/sign-out. Add validation like refresh route.
| export async function GET(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
Missing SERVER_PATH validation. If undefined, URL becomes undefined/user. Add validation like refresh route.
| export async function GET(request: Request) { | ||
| const base = process.env.SERVER_PATH; | ||
| const url = `${base}/auth/sign-out`; | ||
|
|
||
| let res: Response; | ||
| try { | ||
| res = await fetch(url, { | ||
| method: 'GET', | ||
| headers: backendHeaders(request), | ||
| }); | ||
| } catch (err) { |
There was a problem hiding this comment.
Missing SERVER_PATH validation. If undefined, URL becomes undefined/user/update-user-name etc. Add validation like refresh route.
| const body = await request.json(); | ||
| let url = ''; |
There was a problem hiding this comment.
Missing SERVER_PATH validation. If undefined, url becomes undefined/auth/sign-in. Add validation: if (!base) { return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 }); }
| export async function PUT(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
Missing SERVER_PATH validation. If undefined, url becomes undefined/auth/sign-out. Add validation like other routes do.
| export async function PUT(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
Missing SERVER_PATH validation. If undefined, url becomes undefined/user. Add validation like other routes do.
| export async function PUT(request: Request) { | ||
| const base = process.env.SERVER_PATH; | ||
| const body = await request.json(); | ||
| let url = ''; | ||
|
|
||
| if (body.newName) { |
There was a problem hiding this comment.
Missing SERVER_PATH validation. If undefined, url becomes undefined/user/update-user-name etc. Add validation before line 8.
| > | ||
| Got to sign in |
There was a problem hiding this comment.
Minor typo: 'Got to sign in' should be 'Go to sign in'.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your authentication implementation demonstrates solid understanding of the core requirements. All major features (registration, activation, login, logout, password reset, profile management) are working, and the code structure is clean with proper separation of concerns. However, there are three educational priority items to address: First, fix the JWT payload mismatch in jwt.strategy.ts lines 17-18 where validate() returns { userId, username } instead of { id, email, name } to match the actual token structure from normalizeUser. Second, add the missing confirmPassword field with @MatchField('newPassword') decorator to CompleteResetPasswordDto - this is a checklist requirement #12 for server-side password confirmation validation. Third, add SERVER_PATH validation to the 10 BFF routes following the pattern in refresh/route.ts lines 16-21 to prevent runtime fetch failures when the environment variable is undefined. The ESLint errors shown are a configuration issue (missing TypeScript parser) rather than code problems - ensure @typescript-eslint/parser is properly configured. You did great work on the frontend validation, email notifications, and the cookie-forwarding BFF pattern. Keep up the momentum!
✨ 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
| throw new Error('DATABASE_URL is not set'); | ||
| } |
There was a problem hiding this comment.
The init migration uses resetPasswordToken (line 12) but the auth service (auth.service.ts) references resetPasswordTokenHash. Ensure the rename migration (20260602120600_rename_reset_password_token_to_hash) is applied, or the schema and migrations are in sync.
| import { Module } from '@nestjs/common'; | ||
| import { JwtModule } from '@nestjs/jwt'; | ||
| import { AuthService } from './auth.service'; | ||
| import { AuthController } from './auth.controller'; | ||
| import { PrismaModule } from 'src/prisma/prisma.module'; | ||
| import { MailModule } from 'src/mail/mail.module'; | ||
|
|
||
| @Module({ | ||
| controllers: [AuthController], | ||
| providers: [AuthService], | ||
| imports: [ | ||
| PrismaModule, |
There was a problem hiding this comment.
The task description explicitly requires 'password and confirmation fields that must be equal' in the reset password confirmation page, with server-side validation. Add confirmPassword field with @IsStrongPassword() and @MatchField('newPassword', { message: 'Passwords do not match' }) decorators.
| import { IsEmail, IsNotEmpty, IsString } from 'class-validator'; | ||
| import { IsStrongPassword } from 'src/common/validators/is-strong-password.decorator'; | ||
|
|
||
| export class CreateAuthDto { | ||
| @IsString() | ||
| @IsNotEmpty() | ||
| name!: string; | ||
|
|
||
| @IsEmail({}, { message: 'Invalid email address' }) | ||
| @IsNotEmpty() | ||
| email!: string; | ||
|
|
There was a problem hiding this comment.
The DTO is missing the confirmPassword field that must match newPassword. Per task requirements: "add Reset Password confirmation page (with password and confirmation fields that must be equal". Add confirmPassword field with @IsStrongPassword() and @MatchField('newPassword') decorators.
| import { | ||
| BadRequestException, | ||
| HttpException, | ||
| Injectable, | ||
| UnauthorizedException, | ||
| } from '@nestjs/common'; | ||
| import type { Response } from 'express'; | ||
| import { CreateAuthDto } from './dto/create-auth-dto'; | ||
| import { PrismaService } from 'src/prisma/prisma.service'; | ||
| import bcrypt from 'bcrypt'; | ||
| import { createHash, randomBytes } from 'crypto'; | ||
| import { MailService } from 'src/mail/mail.service'; |
There was a problem hiding this comment.
Critical: Missing confirmPassword field per checklist item #12. The task requires reset password confirmation page with both password and confirmation fields that must be equal with server-side validation. Add confirmPassword field with @IsStrongPassword() and @MatchField('newPassword') decorators.
| import { IsNotEmpty, IsString } from 'class-validator'; | ||
| import { IsStrongPassword } from 'src/common/validators/is-strong-password.decorator'; | ||
|
|
||
| export class CompleteResetPasswordDto { | ||
| @IsString() | ||
| @IsNotEmpty() | ||
| resetToken!: string; | ||
|
|
||
| @IsString() | ||
| @IsNotEmpty() | ||
| @IsStrongPassword() | ||
| newPassword!: string; |
There was a problem hiding this comment.
The task requirement (#12) specifies 'Reset Password confirmation page with password and confirmation fields that must be equal'. The DTO only has newPassword but is missing the required confirmPassword field with validation. Add @MatchField('newPassword') decorator similar to how confirmNewPassword is validated in data-for-update-dto.ts.
|
|
||
| async validate(payload: any) { |
There was a problem hiding this comment.
CRITICAL: JWT payload mismatch (CHECKLIST ITEM #21). The validate method returns { userId: payload.sub, username: payload.username } but JWT tokens are signed with { id, email, name } from normalizeUser in auth.service.ts and jwt.ts. The payload fields don't match. Fix to: return { id: payload.id, email: payload.email, name: payload.name };
| handleRequest(err, user) { | ||
| if (err || !user) { | ||
| throw err || new UnauthorizedException(); |
There was a problem hiding this comment.
Critical: JWT payload mismatch (CHECKLIST ITEM #21 from previous review). The validate method returns { userId: payload.sub, username: payload.username } but tokens are signed with { id, email, name } from normalizeUser. Update to return { id: payload.id, email: payload.email, name: payload.name } to match the actual token structure.
| const url = `${base}/auth/google`; | ||
| const body = await request.json(); |
There was a problem hiding this comment.
Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). If process.env.SERVER_PATH is undefined, url becomes undefined/auth/activate-new-email... causing runtime fetch failures. Add validation like refresh/route.ts: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| const url = `${base}/auth/google`; | ||
| const body = await request.json(); |
There was a problem hiding this comment.
Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). If process.env.SERVER_PATH is undefined, url becomes undefined/auth/activate... causing runtime fetch failures. Add validation like refresh/route.ts: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function POST(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). If process.env.SERVER_PATH is undefined, url becomes undefined/auth/google causing runtime fetch failures. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
|
|
||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
MEDIUM: Missing SERVER_PATH validation (CHECKLIST ITEM #24). Add validation like refresh/route.ts: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 }); before constructing the URL on line 7.
| try { | ||
| res = await fetch(url, { | ||
| method: 'GET', | ||
| headers: { 'Content-Type': 'application/json' }, |
There was a problem hiding this comment.
Missing cookies forwarding. The route doesn't forward cookies from the incoming request. The backend needs cookies for potential authentication checks. Add credentials: 'include' to the fetch or manually forward the cookie header.
|
|
||
| if (!payload?.normalizedUser) { | ||
| return NextResponse.json({ message: 'Unauthorized' }, { status: 401 }); |
There was a problem hiding this comment.
CRITICAL: Activation routes check payload.normalizedUser for 401 (CHECKLIST ITEM #23 from previous review). The BFF routes return 401 if !payload?.normalizedUser, but the backend always returns normalizedUser on successful activation. Consider checking res.ok directly or ensure the check handles the case where the backend returns normalizedUser. Since backend now includes normalizedUser in the 200 response, this should work, but validation should be explicit.
|
|
||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
MEDIUM: Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). Line 6 gets process.env.SERVER_PATH without checking if it exists. If undefined, line 7 creates URL undefined/auth/activate-new-email?activateToken=.... Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| import { useEffect, useState } from "react"; | ||
| import AuthLinkError from "@/app/components/auth-link-error"; |
There was a problem hiding this comment.
CRITICAL (CHECKLIST ITEM #24): Missing SERVER_PATH validation. Line 6-7 construct URL with base but don't check if process.env.SERVER_PATH is set. If undefined, URL becomes undefined/auth/activate?.... Add validation before URL construction: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function POST(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). Lines 4-5 use process.env.SERVER_PATH without checking if it's defined. If undefined, url becomes undefined/auth/register. Add validation like refresh/route.ts lines 15-21: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function POST(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). Lines 4-5 use process.env.SERVER_PATH without checking if it's defined. If undefined, url becomes undefined/auth/reset-password. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
|
|
||
| export async function POST(request: Request) { |
There was a problem hiding this comment.
Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). Lines 3-4 use process.env.SERVER_PATH without checking if it's defined. If undefined, url becomes undefined/auth/reset-password/confirmed. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
|
|
||
| const base = process.env.SERVER_PATH; | ||
| if (!base) { | ||
| return NextResponse.json( | ||
| { message: 'SERVER_PATH is not configured' }, | ||
| { status: 500 }, | ||
| ); |
There was a problem hiding this comment.
This is the REFERENCE implementation for SERVER_PATH validation (CHECKLIST ITEM #24). Lines 15-21 show the correct pattern that all other BFF routes should follow. Other routes are missing this validation.
|
|
||
| async function proxyRefresh(request: Request) { |
There was a problem hiding this comment.
Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). Line 4-5 construct URL with process.env.SERVER_PATH without checking if it's defined. If undefined, URL becomes undefined/auth/register. Add validation like refresh/route.ts: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| import { NextResponse } from 'next/server'; | ||
|
|
There was a problem hiding this comment.
Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). Lines 3-4 construct URL with process.env.SERVER_PATH without checking if it's defined. If undefined, URL becomes undefined/auth/reset-password/confirmed. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
|
|
||
| async function proxyRefresh(request: Request) { |
There was a problem hiding this comment.
Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). Lines 4-5 construct URL with process.env.SERVER_PATH without checking if it's defined. If undefined, URL becomes undefined/auth/reset-password. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| const base = process.env.SERVER_PATH; | ||
| const url = `${base}/auth/reset-password/confirmed`; |
There was a problem hiding this comment.
MEDIUM: Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). Line 4-5 get process.env.SERVER_PATH without checking if it exists. If undefined, line 5 creates URL undefined/auth/register. Add validation like refresh/route.ts: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 }); after line 4.
| export async function POST(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
MEDIUM: Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). Line 3-4 construct URL with undefined base if SERVER_PATH is not set. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| const base = process.env.SERVER_PATH; | ||
| const url = `${base}/auth/reset-password/confirmed`; |
There was a problem hiding this comment.
MEDIUM: Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). Line 4-5 construct URL with undefined base if SERVER_PATH is not set. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function POST(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
MEDIUM (CHECKLIST ITEM #24 from previous review): Missing SERVER_PATH validation. Add the pattern from refresh/route.ts lines 16-21: if (!base) { return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 }); } before constructing the URL on line 5.
|
|
||
| export async function POST(request: Request) { |
There was a problem hiding this comment.
MEDIUM (CHECKLIST ITEM #24 from previous review): Missing SERVER_PATH validation. Add: if (!base) { return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 }); } before line 5 constructs the URL with undefined base.
| export async function POST(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
MEDIUM (CHECKLIST ITEM #24 from previous review): Missing SERVER_PATH validation. Add: if (!base) { return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 }); } before line 5 constructs the URL with undefined base.
|
|
||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). Lines 6-7 construct URL with process.env.SERVER_PATH without checking if defined. If undefined, URL becomes undefined/auth/sign-in. Add validation like refresh/route.ts: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function POST(request: Request) { | ||
| const body = await request.json(); |
There was a problem hiding this comment.
Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). Lines 4-5 construct URL with process.env.SERVER_PATH without checking if defined. If undefined, URL becomes undefined/auth/sign-out. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function POST(request: Request) { | ||
| const body = await request.json(); |
There was a problem hiding this comment.
Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). Lines 4-5 construct URL with process.env.SERVER_PATH without checking if defined. If undefined, URL becomes undefined/user. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function POST(request: Request) { | ||
| const body = await request.json(); | ||
|
|
||
| const base = process.env.SERVER_PATH; | ||
| const url = `${base}/auth/sign-in`; | ||
|
|
||
| let res: Response; | ||
| try { | ||
| res = await fetch(url, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, |
There was a problem hiding this comment.
Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). Line 4 uses process.env.SERVER_PATH without checking if defined. If undefined, lines 9-14 create URLs like undefined/user/update-user-name. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| const url = `${base}/user`; | ||
|
|
There was a problem hiding this comment.
MEDIUM (CHECKLIST ITEM #24 from previous review): Missing SERVER_PATH validation. Line 6-7 use process.env.SERVER_PATH without checking if it's defined. If undefined, URL becomes undefined/auth/sign-in. Add validation like refresh/route.ts: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function GET(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
MEDIUM (CHECKLIST ITEM #24 from previous review): Missing SERVER_PATH validation. Lines 4-5 use process.env.SERVER_PATH without checking if it's defined. If undefined, URL becomes undefined/auth/sign-out. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function GET(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
MEDIUM (CHECKLIST ITEM #24 from previous review): Missing SERVER_PATH validation. Lines 4-5 use process.env.SERVER_PATH without checking if it's defined. If undefined, URL becomes undefined/user. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function GET(request: Request) { | ||
| const base = process.env.SERVER_PATH; | ||
| const url = `${base}/user`; | ||
|
|
||
| let res: Response; | ||
| try { | ||
| res = await fetch(url, { | ||
| method: 'GET', | ||
| headers: backendHeaders(request), | ||
| cache: 'no-store', | ||
| }); |
There was a problem hiding this comment.
MEDIUM (CHECKLIST ITEM #24 from previous review): Missing SERVER_PATH validation. Line 4 uses process.env.SERVER_PATH without checking if it's defined, and lines 9-14 construct URLs with base. If undefined, URLs become undefined/user/.... Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 }); before line 9.
| const body = await request.json(); | ||
| let url = ''; |
There was a problem hiding this comment.
Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). Line 6-7 use process.env.SERVER_PATH without checking if defined. If undefined, line 7 creates URL undefined/auth/sign-in. Add validation like refresh/route.ts: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function PUT(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). Line 4-5 construct URL with process.env.SERVER_PATH without validation. If undefined, URL becomes undefined/auth/sign-out. Add: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function PUT(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). Line 4-5 construct URL with process.env.SERVER_PATH without validation. If undefined, URL becomes undefined/user. Add: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| const base = process.env.SERVER_PATH; | ||
| const body = await request.json(); | ||
| let url = ''; | ||
|
|
||
| if (body.newName) { | ||
| url = `${base}/user/update-user-name`; | ||
| } else if (body.newEmail) { | ||
| url = `${base}/user/update-user-email`; | ||
| } else if (body.newPassword) { | ||
| url = `${base}/user/update-user-password`; |
There was a problem hiding this comment.
Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). Lines 5-14 use process.env.SERVER_PATH without validation. If undefined, URLs become undefined/user/.... Add: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 }); after line 5.
| const url = `${base}/auth/sign-out`; | ||
|
|
There was a problem hiding this comment.
MEDIUM: Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). Lines 6-7 construct URL with process.env.SERVER_PATH without checking if it's defined. If undefined, URL becomes undefined/auth/sign-in. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function GET(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
MEDIUM: Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). Lines 4-5 construct URL with process.env.SERVER_PATH without checking if it's defined. If undefined, URL becomes undefined/auth/sign-out. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function GET(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
MEDIUM: Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). Lines 4-5 construct URL with process.env.SERVER_PATH without checking if it's defined. If undefined, URL becomes undefined/user. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function GET(request: Request) { | ||
| const base = process.env.SERVER_PATH; | ||
| const url = `${base}/auth/sign-out`; | ||
|
|
||
| let res: Response; | ||
| try { | ||
| res = await fetch(url, { | ||
| method: 'GET', | ||
| headers: backendHeaders(request), | ||
| }); | ||
| } catch (err) { |
There was a problem hiding this comment.
MEDIUM: Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). Lines 4-14 construct URLs with process.env.SERVER_PATH without checking if it's defined. If undefined, URLs become undefined/user/update-.... Add validation after line 4: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
|
Я змінила, все що потрібно було! |
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This is an educational approval to help you progress while learning. You've implemented a solid authentication system overall — registration, activation, login, password reset, Google OAuth, and profile management are all working. However, three critical issues need your attention: (1) jwt.strategy.ts line 16 returns { userId: payload.sub, username: payload.username } but JWT tokens are signed with { id, email, name } — this breaks authenticated routes using Passport strategies; fix by returning { id: payload.id, email: payload.email, name: payload.name }. (2) CompleteResetPasswordDto is missing the required confirmPassword field with @MatchField('newPassword') decorator per checklist items #17-18. (3) 8 BFF routes (sign-in, sign-out, user, reset-password, reset-password/confirmed, activate, activate-new-email, google) lack SERVER_PATH validation — if undefined, URLs become undefined/auth/... causing runtime failures; add if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 }); before constructing URLs. Also note: the ESLint check failures are due to a configuration issue (missing TypeScript parser), not your code. Keep up the great work — you're very close to a complete implementation!
✨ 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
| import { IsNotEmpty, IsString } from 'class-validator'; | ||
| import { IsStrongPassword } from 'src/common/validators/is-strong-password.decorator'; | ||
|
|
||
| export class CompleteResetPasswordDto { | ||
| @IsString() | ||
| @IsNotEmpty() | ||
| resetToken!: string; | ||
|
|
||
| @IsString() | ||
| @IsNotEmpty() | ||
| @IsStrongPassword() | ||
| newPassword!: string; |
There was a problem hiding this comment.
The task requirement (#12) specifies 'Reset Password confirmation page with password and confirmation fields that must be equal'. The DTO only has newPassword but is missing the required confirmPassword field with validation. Add @MatchField('newPassword') decorator similar to how confirmNewPassword is validated in data-for-update-dto.ts.
| import { useEffect, useState } from "react"; | ||
| import AuthLinkError from "@/app/components/auth-link-error"; |
There was a problem hiding this comment.
CRITICAL (CHECKLIST ITEM #24): Missing SERVER_PATH validation. Line 6-7 construct URL with base but don't check if process.env.SERVER_PATH is set. If undefined, URL becomes undefined/auth/activate?.... Add validation before URL construction: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function POST(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
MEDIUM (CHECKLIST ITEM #24 from previous review): Missing SERVER_PATH validation. Add the pattern from refresh/route.ts lines 16-21: if (!base) { return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 }); } before constructing the URL on line 5.
|
|
||
| export async function POST(request: Request) { |
There was a problem hiding this comment.
MEDIUM (CHECKLIST ITEM #24 from previous review): Missing SERVER_PATH validation. Add: if (!base) { return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 }); } before line 5 constructs the URL with undefined base.
| export async function POST(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
MEDIUM (CHECKLIST ITEM #24 from previous review): Missing SERVER_PATH validation. Add: if (!base) { return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 }); } before line 5 constructs the URL with undefined base.
| const body = await request.json(); | ||
| let url = ''; |
There was a problem hiding this comment.
Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). Line 6-7 use process.env.SERVER_PATH without checking if defined. If undefined, line 7 creates URL undefined/auth/sign-in. Add validation like refresh/route.ts: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function PUT(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). Line 4-5 construct URL with process.env.SERVER_PATH without validation. If undefined, URL becomes undefined/auth/sign-out. Add: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function PUT(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). Line 4-5 construct URL with process.env.SERVER_PATH without validation. If undefined, URL becomes undefined/user. Add: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| const base = process.env.SERVER_PATH; | ||
| const body = await request.json(); | ||
| let url = ''; | ||
|
|
||
| if (body.newName) { | ||
| url = `${base}/user/update-user-name`; | ||
| } else if (body.newEmail) { | ||
| url = `${base}/user/update-user-email`; | ||
| } else if (body.newPassword) { | ||
| url = `${base}/user/update-user-password`; |
There was a problem hiding this comment.
Missing SERVER_PATH validation (CHECKLIST ITEM #24 from previous review). Lines 5-14 use process.env.SERVER_PATH without validation. If undefined, URLs become undefined/user/.... Add: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 }); after line 5.
| }); | ||
| } | ||
|
|
||
| const oldEmail = user.email; |
There was a problem hiding this comment.
The resetPassword and completeResetPassword methods reference a pendingNewEmail field that doesn't exist in the database schema (migrations only show resetPasswordTokenHash and resetPasswordTokenExpiresAt). This will cause runtime errors when attempting to change email.
|
|
||
| res.cookie('accessToken', accessToken, { | ||
| httpOnly: true, | ||
| secure: false, // dev only | ||
| sameSite: "lax", | ||
| path: "/", | ||
| maxAge: 1 * 1 * 15 * 60 * 1000, | ||
| }); | ||
| res.cookie("refreshToken", refreshToken, { | ||
| httpOnly: true, | ||
| secure: false, // dev only | ||
| sameSite: "lax", | ||
| path: "/", | ||
| maxAge: 7 * 24 * 60 * 60 * 1000, |
There was a problem hiding this comment.
The login method duplicates cookie-setting logic instead of using the existing issueSession helper. This creates maintenance issues and potential inconsistencies in cookie configuration.
| handleRequest(err, user) { | ||
| if (err || !user) { |
There was a problem hiding this comment.
CRITICAL - JWT payload mismatch not fixed: The validate method returns { userId: payload.sub, username: payload.username } but JWT tokens are signed with { id, email, name } from normalizeUser. This creates a mismatch where payload.sub will be undefined and payload.username won't exist. Update to: return { userId: payload.id, username: payload.name }; or preferably use the AuthUserPayload structure: { id: payload.id, email: payload.email, name: payload.name }
|
|
||
| async validate(payload: any) { | ||
| return { userId: payload.sub, username: payload.username }; |
There was a problem hiding this comment.
CRITICAL - NOT FIXED FROM PREVIOUS REVIEW: Line 16-17 returns { userId: payload.sub, username: payload.username } but JWT tokens are signed with { id, email, name } (from normalizeUser). Since payload.sub and payload.username don't exist, this always returns { userId: undefined, username: undefined }, breaking authenticated routes. Fix: return { id: payload.id, email: payload.email, name: payload.name };
| message: PASSWORD_VALIDATION_MESSAGES.uppercase, | ||
| }), | ||
| Matches(PASSWORD_RULE_PATTERNS.lowercase, { | ||
| message: PASSWORD_VALIDATION_MESSAGES.lowercase, |
There was a problem hiding this comment.
CRITICAL - JWT payload mismatch: Line 16 returns { userId: payload.sub, username: payload.username } but JWT tokens are signed with { id, email, name } from normalizeUser. Properties sub and username don't exist in the payload—they should be id and email. Update to: return { id: payload.id, email: payload.email, name: payload.name };
|
|
||
| const connectionString = `${process.env.DATABASE_URL}`; | ||
| const adapter = new PrismaPg({ connectionString }); |
There was a problem hiding this comment.
The prisma.ts utility file (lines 4-6) will throw at import time if DATABASE_URL is undefined because it constructs the adapter immediately. Consider lazy initialization or validation similar to PrismaService.
| import { Test, TestingModule } from '@nestjs/testing'; | ||
| import { INestApplication } from '@nestjs/common'; | ||
| import request from 'supertest'; | ||
| import { App } from 'supertest/types'; | ||
| import { AppModule } from './../src/app.module'; | ||
|
|
||
| describe('AppController (e2e)', () => { | ||
| let app: INestApplication<App>; | ||
|
|
There was a problem hiding this comment.
The prisma.ts file creates a standalone Prisma instance that is likely unused (the PrismaService should handle connections). This duplicate instance could cause connection pool issues. Consider removing this file or using it consistently instead of PrismaService.
|
|
||
| export const normalizeUser = ({ id, name, email }) => { | ||
| return { id, name, email }; |
There was a problem hiding this comment.
CRITICAL ISSUE - jwt.strategy.ts returns wrong payload: Line 16 returns { userId: payload.sub, username: payload.username } but jwt.ts and normalizeUser.ts confirm JWT tokens are signed with { id, email, name }. The strategy should return { id: payload.id, email: payload.email, name: payload.name } to match the actual token payload structure.
| import { useEffect, useState } from "react"; | ||
| import AuthLinkError from "@/app/components/auth-link-error"; |
There was a problem hiding this comment.
CRITICAL - Missing SERVER_PATH validation: Line 6-7 constructs URL without checking if base is undefined. If SERVER_PATH env var is not set, the URL becomes undefined/auth/activate?activateToken=.... Add validation like in refresh/route.ts: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
|
|
||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
CRITICAL - NOT FIXED FROM PREVIOUS REVIEW: Lines 6-7 construct the URL using process.env.SERVER_PATH without validation. If SERVER_PATH is undefined, url becomes undefined/auth/activate... causing runtime failure. Add validation like: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
|
|
||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
ISSUE NOT FIXED FROM PREVIOUS REVIEW: Line 6-7 uses process.env.SERVER_PATH without validation. If SERVER_PATH is undefined, the URL becomes undefined/auth/activate... causing runtime failures. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
|
|
||
| ```bash |
There was a problem hiding this comment.
CRITICAL - NOT FIXED FROM PREVIOUS REVIEW: Line 6-7: const base = process.env.SERVER_PATH; const url = ${base}/auth/activate...`` doesn't validate if base is undefined. If SERVER_PATH is not set, the URL becomes `undefined/auth/activate...` causing runtime fetch failures. Add validation like: `if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });`
| const base = process.env.SERVER_PATH; | ||
| const url = `${base}/auth/reset-password/confirmed`; |
There was a problem hiding this comment.
CRITICAL - Missing SERVER_PATH validation: Lines 4-5 construct URL without validating base. If SERVER_PATH is undefined, URL becomes undefined/auth/google. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function POST(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
CRITICAL - Missing SERVER_PATH validation: Lines 3-4 construct URL without validating base. If SERVER_PATH is undefined, URL becomes undefined/auth/reset-password/confirmed. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| const base = process.env.SERVER_PATH; | ||
| const url = `${base}/auth/reset-password/confirmed`; |
There was a problem hiding this comment.
CRITICAL - Missing SERVER_PATH validation: Lines 4-5 construct URL without validating base. If SERVER_PATH is undefined, URL becomes undefined/auth/reset-password. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function POST(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
CRITICAL - NOT FIXED FROM PREVIOUS REVIEW: Line 4-5 constructs URL without validating base. If SERVER_PATH is undefined, URL becomes undefined/auth/google. Add validation like refresh/route.ts: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
|
|
||
| export async function POST(request: Request) { |
There was a problem hiding this comment.
CRITICAL - NOT FIXED FROM PREVIOUS REVIEW: Line 3-4 constructs URL without validating base. If SERVER_PATH is undefined, URL becomes undefined/auth/reset-password/confirmed. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function POST(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
CRITICAL - NOT FIXED FROM PREVIOUS REVIEW: Line 4-5 constructs URL without validating base. If SERVER_PATH is undefined, URL becomes undefined/auth/reset-password. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
|
|
||
| export async function POST(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
CRITICAL - NOT FIXED FROM PREVIOUS REVIEW: google/route.ts line 4-5, reset-password/route.ts line 4-5, and reset-password/confirmed/route.ts line 3-5 all use process.env.SERVER_PATH without validation. If undefined, URLs become undefined/auth/.... Add validation like refresh/route.ts (lines 15-21): if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
|
|
||
| async function proxyRefresh(request: Request) { |
There was a problem hiding this comment.
CRITICAL - NOT FIXED: Lines 4-5 construct URL without validating SERVER_PATH. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| import { NextResponse } from 'next/server'; | ||
|
|
There was a problem hiding this comment.
CRITICAL - NOT FIXED: Lines 3-4 construct URL without validating SERVER_PATH. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
|
|
||
| async function proxyRefresh(request: Request) { |
There was a problem hiding this comment.
CRITICAL - NOT FIXED: Lines 4-5 construct URL without validating SERVER_PATH. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| return NextResponse.json(body, { status: res.status }); | ||
| } | ||
|
|
||
| const response = NextResponse.json({ message: 'OK' }, { status: 200 }); |
There was a problem hiding this comment.
Line 47: res.status || 401 is redundant since res.status is always a number (200-599). Remove the || 401 fallback.
| const url = `${base}/user`; | ||
|
|
There was a problem hiding this comment.
CRITICAL - NOT FIXED FROM PREVIOUS REVIEW: sign-in/route.ts lines 6-7 construct URL without validating SERVER_PATH. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function GET(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
CRITICAL - NOT FIXED FROM PREVIOUS REVIEW: sign-out/route.ts lines 4-5 construct URL without validating SERVER_PATH. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function GET(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
CRITICAL - NOT FIXED FROM PREVIOUS REVIEW: user/route.ts lines 4-5 construct URL without validating SERVER_PATH. Add validation: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
|
|
||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
CRITICAL - NOT FIXED: sign-in/route.ts lines 6-7 construct URL without validating base. Add: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function POST(request: Request) { | ||
| const body = await request.json(); |
There was a problem hiding this comment.
CRITICAL - NOT FIXED: sign-out/route.ts lines 4-5 construct URL without validating base. Add: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function POST(request: Request) { | ||
| const body = await request.json(); |
There was a problem hiding this comment.
CRITICAL - NOT FIXED: user/route.ts lines 4-5 construct URL without validating base. Add: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| const url = `${base}/auth/sign-out`; | ||
|
|
There was a problem hiding this comment.
CRITICAL - NOT FIXED: Lines 6-7 construct URL without validating SERVER_PATH. Add: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function GET(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
CRITICAL - NOT FIXED: Lines 4-5 construct URL without validating SERVER_PATH. Add: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| export async function GET(request: Request) { | ||
| const base = process.env.SERVER_PATH; |
There was a problem hiding this comment.
CRITICAL - NOT FIXED: Lines 4-5 construct URL without validating SERVER_PATH. Add: if (!base) return NextResponse.json({ message: 'SERVER_PATH is not configured' }, { status: 500 });
| .object({ | ||
| newPassword: passwordSchema, |
There was a problem hiding this comment.
Line 96-97: The ChangePasswordSchema uses newPassword and confirmPassword but the task requirement #17 explicitly mentions "Reset Password confirmation page (with password and confirmation fields)". Consider renaming to password and confirmation to match the task specification, or ensure the backend CompleteResetPasswordDto uses the same field names.
| }, []); | ||
|
|
||
| return ( | ||
| <GoogleOAuthProvider |
There was a problem hiding this comment.
Line 28: Uses non-null assertion for NEXT_PUBLIC_GOOGLE_CLIENT_ID which will throw at runtime if the environment variable is not set. Consider adding validation or a fallback.
No description provided.