Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions src/users/avatar-upload.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -86,14 +86,30 @@ export class AvatarUploadService {

async deleteAvatar(userId: string, filename: string): Promise<void> {
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
}
Expand All @@ -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');
}
}
Expand Down
19 changes: 19 additions & 0 deletions test/users/avatar-upload.spec.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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',
Expand Down
Loading