Work on security by boiboi796 · Pull Request #8 · SkillsForge-Community/Expense-Tracker-Backend-Node

boiboi796 · 2026-02-13T11:23:43Z

##What I did
I installed JWT and upgraded the security of each endpoint of the expense tracker

… to accept users credentials compulsorily

Copilot

Pull request overview

This PR implements JWT-based authentication and authorization for the expense tracker application. It introduces a complete auth system with user management, password hashing, JWT token generation, and role-based access control to secure transaction and user endpoints.

Changes:

Added authentication module with JWT strategy, guards, and login endpoint
Created users module with user registration, CRUD operations, and password hashing
Enhanced transaction endpoints with ownership validation and admin-only access controls
Updated database schema to make user fields required (name, email, passwordHash)

Reviewed changes

Copilot reviewed 24 out of 25 changed files in this pull request and generated 26 comments.

Show a summary per file

File	Description
package.json	Added authentication dependencies (bcrypt, passport, jsonwebtoken, @nestjs/passport)
src/auth/auth.module.ts	Created authentication module with JWT strategy and passport integration
src/auth/auth.service.ts	Implemented login logic with password verification and JWT token generation
src/auth/auth.controller.ts	Added login and profile endpoints
src/auth/guards/jwt-auth/jwt-auth.guard.ts	Created JWT authentication guard for protecting endpoints
src/auth/strategies/jwt.strategy/jwt.strategy.ts	Implemented JWT validation strategy using passport-jwt
src/auth/dto/login.dto.ts	Created DTO for login requests with email and password validation
src/users/users.module.ts	Created users module with service, repository, and controller
src/users/users.service.ts	Implemented user creation with password hashing and admin check
src/users/users.controller.ts	Added user CRUD endpoints with admin-only access control
src/users/users.repository.ts	Created repository for user database operations
src/users/dto/users-dto.ts	Created DTO for user creation with validation
src/transactions/transactions.service.ts	Added ownership validation to prevent users from accessing others' transactions
src/transactions/transactions.controller.ts	Protected all endpoints with JWT auth and added admin checks for list endpoint
src/transactions/dto/create-transaction.dto.ts	Removed unused Length import
src/db/schema.ts	Made user fields required and improved formatting
src/config/env.ts	Added JWT_SECRET configuration
src/app.module.ts	Imported AuthModule

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-13T11:30:30Z

+    const token = jwt.sign(payload, env.JWT_SECRET, {
+      expiresIn: '24h',
+    });


The auth implementation uses the low-level jsonwebtoken library directly instead of leveraging NestJS's @nestjs/jwt module. This bypasses NestJS's built-in JWT integration and makes the code less maintainable. Replace the manual jwt.sign call with JwtService from @nestjs/jwt, and add JwtModule to the AuthModule imports with proper configuration.

Copilot · 2026-02-13T11:30:30Z

+    @IsString()
+    @IsNotEmpty()
+    @Length(8, 20)
+    passwordHash: string;


The passwordHash field in the DTO is poorly named and misleading. At this point in the request lifecycle, it contains the plain-text password from the user, not a hashed password. Rename this field to 'password' to accurately reflect its content. The hashing happens later in the service layer.

Suggested change

passwordHash: string;

password: string;

Copilot · 2026-02-13T11:30:31Z

+    const isAdmin = req.user.userType === 'ADMIN';
+    if (!isAdmin) {
+      throw new ForbiddenException('Only admins can access this resource');
+    }
+    return this.usersService.findAll();


The admin authorization logic is duplicated across multiple controller methods in both UsersController and TransactionsController. This code duplication makes the codebase harder to maintain and increases the risk of inconsistent authorization checks. Create a reusable RolesGuard or AdminGuard that can be applied via a decorator (e.g., @roles('ADMIN')) to eliminate this duplication.

Copilot · 2026-02-13T11:30:31Z

+    return this.dbClient.select()
+      .from(users)
+      .where(eq(users.id, id));


The findById method returns user records including the passwordHash field. Password hashes should never be exposed in API responses. Filter out the passwordHash field from these query results using .select() with explicit field selection, or map the results to exclude the sensitive field.

Suggested change

return this.dbClient.select()

.from(users)

.where(eq(users.id, id));

const result = await this.dbClient.select()

.from(users)

.where(eq(users.id, id));

return result.map(({ passwordHash, ...userWithoutPassword }) => userWithoutPassword);

Copilot · 2026-02-13T11:30:31Z

+        const userId = (req as any).user.id; 
        return this.transactionsService.createTransaction(createTransactionDto, userId);
    }


    @Get()
-    getAllTransactions() {
+    getAllTransactions(@Request() req: ExpressRequest) {
+        const isAdmin = (req as any).user.userType === 'ADMIN';
+        if (!isAdmin) {
+            throw new ForbiddenException('Only admins can access all transactions');
+        }
        return this.transactionsService.getAllTransactions();
    }

    // GET transaction by id
    @Get(':id')
-    getTransactionById(@Param('id', ParseIntPipe) id: number) {
-        return this.transactionsService.getTransactionById(id);
+    getTransactionById(@Param('id', ParseIntPipe) id: number, @Request() req: ExpressRequest) {
+        const userId = (req as any).user.id;
+        return this.transactionsService.getTransactionById(id, userId);
    }

    // UPDATE transaction by id
    @Put(':id')
    updateTransaction(@Param('id',ParseIntPipe) id: number,
-                     @Body() updateTransactionDto: UpdateTransactionDto) {
-        return this.transactionsService.updateTransaction(id, updateTransactionDto);
+                     @Body() updateTransactionDto: UpdateTransactionDto,
+                     @Request() req: ExpressRequest) {
+        const userId = (req as any).user.id;
+        return this.transactionsService.updateTransaction(id, updateTransactionDto, userId);
    }

    // DELETE transaction by id
    @Delete(':id')
-    deleteTransaction(@Param('id',ParseIntPipe) id: number){
-        return this.transactionsService.deleteTransaction(id);
+    deleteTransaction(@Param('id',ParseIntPipe) id: number, @Request() req: ExpressRequest){
+        const userId = (req as any).user.id;
+        return this.transactionsService.deleteTransaction(id, userId);


Type casting req as 'any' bypasses TypeScript's type safety and is repeated throughout the controller. Define a proper type or interface that extends Express.Request with a user property, and use it consistently to eliminate these unsafe type casts.

Copilot · 2026-02-13T11:30:36Z

 @Controller('transactions')
+@UseGuards(JwtAuthGuard)
 export class TransactionsController {
    constructor(private readonly transactionsService: TransactionsService) {}


    // CREATE transaction
    @Post()
-    createTransaction(@Body() createTransactionDto: CreateTransactionDto) {
-        const userId = 1; 
+    createTransaction(@Body() createTransactionDto: CreateTransactionDto, @Request() req: ExpressRequest) {
+        const userId = (req as any).user.id; 
        return this.transactionsService.createTransaction(createTransactionDto, userId);
    }


    @Get()
-    getAllTransactions() {
+    getAllTransactions(@Request() req: ExpressRequest) {
+        const isAdmin = (req as any).user.userType === 'ADMIN';
+        if (!isAdmin) {
+            throw new ForbiddenException('Only admins can access all transactions');
+        }
        return this.transactionsService.getAllTransactions();
    }

    // GET transaction by id
    @Get(':id')
-    getTransactionById(@Param('id', ParseIntPipe) id: number) {
-        return this.transactionsService.getTransactionById(id);
+    getTransactionById(@Param('id', ParseIntPipe) id: number, @Request() req: ExpressRequest) {
+        const userId = (req as any).user.id;
+        return this.transactionsService.getTransactionById(id, userId);
    }

    // UPDATE transaction by id
    @Put(':id')
    updateTransaction(@Param('id',ParseIntPipe) id: number,
-                     @Body() updateTransactionDto: UpdateTransactionDto) {
-        return this.transactionsService.updateTransaction(id, updateTransactionDto);
+                     @Body() updateTransactionDto: UpdateTransactionDto,
+                     @Request() req: ExpressRequest) {
+        const userId = (req as any).user.id;
+        return this.transactionsService.updateTransaction(id, updateTransactionDto, userId);
    }

    // DELETE transaction by id
    @Delete(':id')
-    deleteTransaction(@Param('id',ParseIntPipe) id: number){
-        return this.transactionsService.deleteTransaction(id);
+    deleteTransaction(@Param('id',ParseIntPipe) id: number, @Request() req: ExpressRequest){
+        const userId = (req as any).user.id;
+        return this.transactionsService.deleteTransaction(id, userId);
    } 


The authentication and authorization logic in the controller lacks comprehensive test coverage. The test file only contains a basic 'should be defined' test. Add unit tests for the JWT guard integration, admin authorization checks, and user ownership validation across all endpoints.

Copilot · 2026-02-13T11:30:36Z

+@Injectable()
+export class AuthService {
+  constructor(private readonly usersRepository: UsersRepository) {}
+
+  async login(loginDto: LoginDto) {
+
+
+    const user = await this.usersRepository.findByEmail(loginDto.email);
+
+    if (!user) {
+      throw new UnauthorizedException('Invalid credentials');
+    }
+
+    //check pass
+    const isPasswordValid = await bcrypt.compare(
+      loginDto.password,
+      user.passwordHash
+    );
+
+    if (!isPasswordValid) {
+      throw new UnauthorizedException('Invalid credentials');
+    }
+
+    const payload = {
+      sub: user.id,
+      email: user.email,
+      userType: user.userType,
+    };
+
+    if (!env.JWT_SECRET) {
+      throw new Error('JWT_SECRET is not configured');
+    }
+
+    const token = jwt.sign(payload, env.JWT_SECRET, {
+      expiresIn: '24h',
+    });
+
+    // 4. Return token and user info
+    return {
+      access_token: token,
+      user: {
+        id: user.id,
+        name: user.name,
+        email: user.email,
+        userType: user.userType,
+      },
+    };
+  }
+}


The authentication logic including login, password verification, and JWT token generation lacks comprehensive test coverage. The test file only contains a basic 'should be defined' test. Add unit tests for the login method covering successful authentication, invalid email, invalid password, and JWT token generation scenarios.

Copilot · 2026-02-13T11:30:36Z

+  async create(createUserDto: CreateUserDto) {
+    const hashedPassword = await bcrypt.hash(createUserDto.passwordHash, 10);
+    return this.usersRepository.create({
+      ...createUserDto,
+      passwordHash: hashedPassword,
+    });
+  }


The user creation endpoint does not check if a user with the provided email already exists before attempting to create a new user. This will result in a database constraint violation error when duplicate emails are provided, which may expose internal database details. Add a check in the service to verify email uniqueness and return a clear error message (e.g., 'User with this email already exists') before attempting the database insert.

Copilot · 2026-02-13T11:30:36Z

+  async findAll(@Request() req: any) {
+    const isAdmin = req.user.userType === 'ADMIN';
+    if (!isAdmin) {
+      throw new ForbiddenException('Only admins can access this resource');
+    }
+    return this.usersService.findAll();
+  }
+
+  @UseGuards(JwtAuthGuard)
+  @Get(':id')
+  async findById(@Request() req: any, @Param('id', ParseIntPipe) id: number) {
+    const isAdmin = req.user.userType === 'ADMIN';
+    if (!isAdmin) {
+      throw new ForbiddenException('Only admins can access this resource');
+    }
+      return this.usersService.findById(id);


Type casting req as 'any' bypasses TypeScript's type safety. Define a proper type or interface that extends Express.Request with a user property, and use it consistently throughout the controller to eliminate these unsafe type casts.

Copilot · 2026-02-13T11:30:36Z

@@ -0,0 +1,34 @@
+import { Controller, Get, Param, Body, Post, ParseIntPipe, Query, ForbiddenException, UseGuards, Request } from '@nestjs/common';


Unused import Query.

Suggested change

import { Controller, Get, Param, Body, Post, ParseIntPipe, Query, ForbiddenException, UseGuards, Request } from '@nestjs/common';

import { Controller, Get, Param, Body, Post, ParseIntPipe, ForbiddenException, UseGuards, Request } from '@nestjs/common';

a-k-o-r-e-d-e · 2026-02-14T15:07:00Z

+    @Post('login')
+    async login(@Body() loginDto: LoginDto) {
+        return this.authService.login(loginDto);
+    }


@boiboi796 Just study rate limiting but you don't have to implement it rn

a-k-o-r-e-d-e · 2026-02-14T15:08:27Z

+    }
+
+    const token = jwt.sign(payload, env.JWT_SECRET, {
+      expiresIn: '24h',


Use one hour for now, we can implement refresh tokens in a different PR

a-k-o-r-e-d-e · 2026-02-14T15:09:04Z

-    createTransaction(@Body() createTransactionDto: CreateTransactionDto) {
-        const userId = 1; 
+    createTransaction(@Body() createTransactionDto: CreateTransactionDto, @Request() req: ExpressRequest) {
+        const userId = (req as any).user.id; 


See the comment about creating an interface

a-k-o-r-e-d-e · 2026-02-14T15:09:27Z

+        const isAdmin = (req as any).user.userType === 'ADMIN';
+        if (!isAdmin) {
+            throw new ForbiddenException('Only admins can access all transactions');
+        }
        return this.transactionsService.getAllTransactions();


Create a RolesGuard

a-k-o-r-e-d-e · 2026-02-14T15:12:38Z

+    if (!transaction) {
+      throw new NotFoundException('Transaction not found');
+    }
+    if (transaction.userId !== userId) {


Could bypass this check by just fetching from the db by transactionId and userId.

create a repo func for that. If it returns a transaction, then the transaction muct belong to the user

a-k-o-r-e-d-e · 2026-02-14T15:14:19Z

-  async findAll(@Query('adminId', ParseIntPipe) adminId: number) {
-    const isAdmin = await this.usersService.isAdmin(adminId);
+  async findAll(@Request() req: any) {
+    const isAdmin = req.user.userType === 'ADMIN';


a-k-o-r-e-d-e · 2026-02-14T15:15:33Z

  async create(createUserDto: CreateUserDto) {
-    return this.usersRepository.create(createUserDto);
+    const hashedPassword = await bcrypt.hash(createUserDto.passwordHash, 10);
+    return this.usersRepository.create({


Create a response dto or User dto

a-k-o-r-e-d-e · 2026-02-14T15:16:24Z

  async create(createUserDto: CreateUserDto) {
-    return this.usersRepository.create(createUserDto);
+    const hashedPassword = await bcrypt.hash(createUserDto.passwordHash, 10);
+    return this.usersRepository.create({


Check if the user exists first, do not depend on the db returning an error as the error might not be user friendly

a-k-o-r-e-d-e · 2026-02-14T15:21:02Z

  }

  async isAdmin(id: number) {
    const [user] = await this.usersRepository.findById(id);


findOneById should not return an array, but rather the first item in an array or null.

refactor the repo function to do this.

You can use a util function such as

export const returnFirst = (rows: T[]): T | null => rows[0] ?? null;

wrap this around the response of the repo such as

static async findOneById(whereClause: SQL | undefined, tx: Transaction): Promise<IDojo | null> {
const dojo = returnFirst(await tx.select().from(dojos).where(eq(dojos.id, id)).limit(1).execute());

return dojo || null;

}

a-k-o-r-e-d-e · 2026-02-14T15:21:31Z

+
+
+  async getTransactionById(id: number, userId: number) {
+    const [transaction] = await this.transactionsRepo.findById(id);


findOneById should not return an array, but rather the first item in an array or null.

refactor the repo function to do this.

You can use a util function such as

export const returnFirst = (rows: T[]): T | null => rows[0] ?? null;

wrap this around the response of the repo such as

static async findOneById(whereClause: SQL | undefined, tx: Transaction): Promise<IDojo | null> {
const dojo = returnFirst(await tx.select().from(dojos).where(eq(dojos.id, id)).limit(1).execute());

return dojo || null;

}

… class

boiboi796 added 3 commits February 10, 2026 14:21

created users module service and repository, also modified the schema…

c0090b9

… to accept users credentials compulsorily

Defined the user controller

c1b1b2f

Created User Authorization

8adf517

Copilot AI review requested due to automatic review settings February 13, 2026 11:23

Copilot started reviewing on behalf of boiboi796 February 13, 2026 11:24 View session

Copilot AI reviewed Feb 13, 2026

View reviewed changes

a-k-o-r-e-d-e requested changes Feb 14, 2026

View reviewed changes

boiboi796 added 7 commits February 18, 2026 12:38

Added password class validator

c4b3ca1

Defined the user controller

246a9f2

Added the use of RolesGuard to the Users controller admin function

a61faa5

Created User Authorization

157ab06

Resolved rebase conflicts

c7ffbfc

Added class validator, removed the jwt falback and prepared a userDto…

bb56507

… class

Implemented JWT and improved the security of the expense tracker

56fcef3

boiboi796 force-pushed the Work-on-Security branch from 738ec9b to 56fcef3 Compare February 19, 2026 11:50

boiboi796 added 2 commits February 19, 2026 14:19

Added an authenticated request interface

994808c

fixed other securities problem stated in the comment

53fb99c

		@@ -0,0 +1,34 @@
		import { Controller, Get, Param, Body, Post, ParseIntPipe, Query, ForbiddenException, UseGuards, Request } from '@nestjs/common';



		async getTransactionById(id: number, userId: number) {
		const [transaction] = await this.transactionsRepo.findById(id);

Conversation

boiboi796 commented Feb 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants