diff --git a/src/users/avatar-upload.service.ts b/src/users/avatar-upload.service.ts index 7a015d6..4e6af48 100644 --- a/src/users/avatar-upload.service.ts +++ b/src/users/avatar-upload.service.ts @@ -3,7 +3,7 @@ import { Injectable, BadRequestException, Logger } from '@nestjs/common'; import { ConfigService } from '@nestjs/config'; import { promises as fs } from 'fs'; -import { join } from 'path'; +import { isAbsolute, join, relative, resolve } from 'path'; import { createHash } from 'crypto'; // Multer type definition @@ -86,14 +86,30 @@ export class AvatarUploadService { async deleteAvatar(userId: string, filename: string): Promise { const userDir = join(this.uploadDir, userId); + const resolvedUserDir = resolve(userDir); + + if (!/^[A-Za-z0-9._-]+$/.test(filename)) { + throw new BadRequestException('Invalid filename'); + } try { // Delete all size variants const sizes = ['small_', 'medium_', 'large_', '']; for (const prefix of sizes) { const filePath = join(userDir, `${prefix}${filename}`); + const resolvedFilePath = resolve(filePath); + const normalizedRelativePath = relative(resolvedUserDir, resolvedFilePath); + + if ( + normalizedRelativePath.startsWith('..') || + isAbsolute(normalizedRelativePath) || + normalizedRelativePath === '' + ) { + throw new BadRequestException('Invalid filename'); + } + try { - await fs.unlink(filePath); + await fs.unlink(resolvedFilePath); } catch (error) { // File might not exist, continue } @@ -109,6 +125,9 @@ export class AvatarUploadService { this.logger.log(`Avatar deleted successfully for user ${userId}`); } catch (error) { this.logger.error(`Error deleting avatar for user ${userId}:`, error); + if (error instanceof BadRequestException) { + throw error; + } throw new BadRequestException('Failed to delete avatar'); } } diff --git a/test/users/avatar-upload.spec.ts b/test/users/avatar-upload.spec.ts index 09c7b33..577a58c 100644 --- a/test/users/avatar-upload.spec.ts +++ b/test/users/avatar-upload.spec.ts @@ -1,4 +1,5 @@ import { Test, TestingModule } from '@nestjs/testing'; +import { ConfigService } from '@nestjs/config'; import { AvatarUploadController } from '../../src/users/avatar-upload.controller'; import { AvatarUploadService } from '../../src/users/avatar-upload.service'; import { UsersService } from '../../src/users/users.service'; @@ -9,6 +10,24 @@ describe('AvatarUploadController', () => { let avatarUploadService: AvatarUploadService; let usersService: UsersService; + describe('AvatarUploadService', () => { + let service: AvatarUploadService; + + beforeEach(() => { + const configService = { + get: jest.fn((key: string, defaultValue?: string | number) => defaultValue), + } as unknown as ConfigService; + + service = new AvatarUploadService(configService); + }); + + it('should reject filenames that attempt path traversal', async () => { + await expect(service.deleteAvatar('user_123', '../../secret.txt')).rejects.toThrow( + BadRequestException, + ); + }); + }); + const mockUser = { id: 'user_123', email: 'test@example.com',