mirror of
https://github.com/bookorbit/bookorbit.git
synced 2026-11-03 08:21:39 -06:00
feat(server): add safe directory creation to path service
Introduces PathService.createDirectory with comprehensive path validation to prevent directory traversal and symlink attacks. It validates parent paths against a blocked list, ensures the parent is a real directory, and blocks unsafe characters in folder names. Filesystem errors like EEXIST and EACCES are mapped to standard HTTP exceptions to provide clear feedback to the client. Closes #332
This commit is contained in:
@@ -0,0 +1,13 @@
|
||||
import { IsNotEmpty, IsString, MaxLength } from 'class-validator';
|
||||
|
||||
export class CreateFolderDto {
|
||||
@IsString()
|
||||
@IsNotEmpty()
|
||||
@MaxLength(4096)
|
||||
parentPath: string;
|
||||
|
||||
@IsString()
|
||||
@IsNotEmpty()
|
||||
@MaxLength(255)
|
||||
name: string;
|
||||
}
|
||||
@@ -3,6 +3,7 @@ import { PathController } from './path.controller';
|
||||
describe('PathController', () => {
|
||||
const pathService = {
|
||||
listDirectories: vi.fn(),
|
||||
createDirectory: vi.fn(),
|
||||
};
|
||||
|
||||
const controller = new PathController(pathService as never);
|
||||
@@ -22,4 +23,10 @@ describe('PathController', () => {
|
||||
|
||||
expect(pathService.listDirectories).toHaveBeenCalledWith('/books');
|
||||
});
|
||||
|
||||
it('forwards folder creation to the service', async () => {
|
||||
await controller.createFolder({ parentPath: '/books', name: 'scifi' });
|
||||
|
||||
expect(pathService.createDirectory).toHaveBeenCalledWith('/books', 'scifi');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,7 +1,8 @@
|
||||
import { Permission } from '@bookorbit/types';
|
||||
import { Controller, Get, Query } from '@nestjs/common';
|
||||
import { Body, Controller, Get, Post, Query } from '@nestjs/common';
|
||||
|
||||
import { RequirePermission } from '../../common/decorators/require-permission.decorator';
|
||||
import { CreateFolderDto } from './dto/create-folder.dto';
|
||||
import { PathService } from './path.service';
|
||||
|
||||
@Controller('path')
|
||||
@@ -13,4 +14,10 @@ export class PathController {
|
||||
listDirectories(@Query('path') path: string) {
|
||||
return this.pathService.listDirectories(path || '/');
|
||||
}
|
||||
|
||||
@Post()
|
||||
@RequirePermission(Permission.ManageLibraries)
|
||||
createFolder(@Body() dto: CreateFolderDto) {
|
||||
return this.pathService.createDirectory(dto.parentPath, dto.name);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,14 +1,21 @@
|
||||
vi.mock('fs/promises', () => ({
|
||||
readdir: vi.fn(),
|
||||
lstat: vi.fn(),
|
||||
mkdir: vi.fn(),
|
||||
}));
|
||||
|
||||
import { readdir, lstat } from 'fs/promises';
|
||||
import { BadRequestException, ConflictException, ForbiddenException } from '@nestjs/common';
|
||||
import { readdir, lstat, mkdir } from 'fs/promises';
|
||||
|
||||
import { PathService } from './path.service';
|
||||
|
||||
const readdirMock = vi.mocked(readdir);
|
||||
const lstatMock = vi.mocked(lstat);
|
||||
const mkdirMock = vi.mocked(mkdir);
|
||||
|
||||
function dirStat() {
|
||||
return { isSymbolicLink: () => false, isDirectory: () => true } as never;
|
||||
}
|
||||
|
||||
function entry(name: string, options: { directory?: boolean; symbolicLink?: boolean } = {}) {
|
||||
return {
|
||||
@@ -92,4 +99,82 @@ describe('PathService', () => {
|
||||
|
||||
await expect(service.listDirectories('/does/not/exist')).resolves.toEqual([]);
|
||||
});
|
||||
|
||||
describe('createDirectory', () => {
|
||||
it('creates a folder under an accessible parent and returns its name and path', async () => {
|
||||
lstatMock.mockResolvedValue(dirStat());
|
||||
mkdirMock.mockResolvedValue(undefined as never);
|
||||
|
||||
await expect(service.createDirectory('/tmp/books', 'scifi')).resolves.toEqual({
|
||||
name: 'scifi',
|
||||
path: '/tmp/books/scifi',
|
||||
});
|
||||
expect(mkdirMock).toHaveBeenCalledWith('/tmp/books/scifi');
|
||||
});
|
||||
|
||||
it('trims the folder name before creating', async () => {
|
||||
lstatMock.mockResolvedValue(dirStat());
|
||||
mkdirMock.mockResolvedValue(undefined as never);
|
||||
|
||||
await expect(service.createDirectory('/tmp/books', ' scifi ')).resolves.toEqual({
|
||||
name: 'scifi',
|
||||
path: '/tmp/books/scifi',
|
||||
});
|
||||
expect(mkdirMock).toHaveBeenCalledWith('/tmp/books/scifi');
|
||||
});
|
||||
|
||||
it('rejects blocked parent paths without touching the filesystem', async () => {
|
||||
await expect(service.createDirectory('/etc', 'evil')).rejects.toBeInstanceOf(ForbiddenException);
|
||||
expect(lstatMock).not.toHaveBeenCalled();
|
||||
expect(mkdirMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('rejects a symlinked parent', async () => {
|
||||
lstatMock.mockResolvedValue({ isSymbolicLink: () => true, isDirectory: () => false } as never);
|
||||
|
||||
await expect(service.createDirectory('/tmp/link', 'scifi')).rejects.toBeInstanceOf(ForbiddenException);
|
||||
expect(mkdirMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('rejects a parent that is not a directory', async () => {
|
||||
lstatMock.mockResolvedValue({ isSymbolicLink: () => false, isDirectory: () => false } as never);
|
||||
|
||||
await expect(service.createDirectory('/tmp/file.txt', 'scifi')).rejects.toBeInstanceOf(BadRequestException);
|
||||
expect(mkdirMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it.each(['', ' ', '..', '.', '.hidden', 'a/b', 'a\\b'])('rejects unsafe folder name %j before any filesystem access', async (name) => {
|
||||
await expect(service.createDirectory('/tmp/books', name)).rejects.toBeInstanceOf(BadRequestException);
|
||||
expect(lstatMock).not.toHaveBeenCalled();
|
||||
expect(mkdirMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('maps EEXIST to a conflict', async () => {
|
||||
lstatMock.mockResolvedValue(dirStat());
|
||||
mkdirMock.mockRejectedValue(Object.assign(new Error('exists'), { code: 'EEXIST' }));
|
||||
|
||||
await expect(service.createDirectory('/tmp/books', 'scifi')).rejects.toBeInstanceOf(ConflictException);
|
||||
});
|
||||
|
||||
it('maps EACCES to forbidden', async () => {
|
||||
lstatMock.mockResolvedValue(dirStat());
|
||||
mkdirMock.mockRejectedValue(Object.assign(new Error('denied'), { code: 'EACCES' }));
|
||||
|
||||
await expect(service.createDirectory('/tmp/books', 'scifi')).rejects.toBeInstanceOf(ForbiddenException);
|
||||
});
|
||||
|
||||
it('maps EROFS to a bad request', async () => {
|
||||
lstatMock.mockResolvedValue(dirStat());
|
||||
mkdirMock.mockRejectedValue(Object.assign(new Error('read-only'), { code: 'EROFS' }));
|
||||
|
||||
await expect(service.createDirectory('/tmp/books', 'scifi')).rejects.toBeInstanceOf(BadRequestException);
|
||||
});
|
||||
|
||||
it('maps a missing parent (ENOENT) to a bad request', async () => {
|
||||
lstatMock.mockRejectedValue(Object.assign(new Error('missing'), { code: 'ENOENT' }));
|
||||
|
||||
await expect(service.createDirectory('/tmp/gone', 'scifi')).rejects.toBeInstanceOf(BadRequestException);
|
||||
expect(mkdirMock).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,14 +1,27 @@
|
||||
import { Injectable } from '@nestjs/common';
|
||||
import { lstat, readdir } from 'fs/promises';
|
||||
import type { CreateFolderResult, DirectoryEntry } from '@bookorbit/types';
|
||||
import {
|
||||
BadRequestException,
|
||||
ConflictException,
|
||||
ForbiddenException,
|
||||
HttpException,
|
||||
Injectable,
|
||||
InternalServerErrorException,
|
||||
Logger,
|
||||
} from '@nestjs/common';
|
||||
import { lstat, mkdir, readdir } from 'fs/promises';
|
||||
import { join, resolve, sep } from 'path';
|
||||
|
||||
import { sanitizeLogValue } from '../../common/utils/log-sanitize.utils';
|
||||
|
||||
const BLOCKED = ['/proc', '/sys', '/dev', '/run', '/var/run', '/etc', '/root'];
|
||||
|
||||
@Injectable()
|
||||
export class PathService {
|
||||
async listDirectories(rawPath: string): Promise<{ name: string; path: string }[]> {
|
||||
private readonly logger = new Logger(PathService.name);
|
||||
|
||||
async listDirectories(rawPath: string): Promise<DirectoryEntry[]> {
|
||||
const resolved = resolve(rawPath || '/');
|
||||
if (BLOCKED.some((b) => resolved === b || resolved.startsWith(b + sep))) {
|
||||
if (this.isBlocked(resolved)) {
|
||||
return [];
|
||||
}
|
||||
try {
|
||||
@@ -16,7 +29,7 @@ export class PathService {
|
||||
if (rootStat.isSymbolicLink()) return [];
|
||||
|
||||
const entries = await readdir(resolved, { withFileTypes: true }); // codeql[js/path-injection]
|
||||
const dirs: { name: string; path: string }[] = [];
|
||||
const dirs: DirectoryEntry[] = [];
|
||||
for (const entry of entries) {
|
||||
if (!entry.isDirectory()) continue;
|
||||
if (entry.name.startsWith('.')) continue;
|
||||
@@ -33,4 +46,83 @@ export class PathService {
|
||||
return [];
|
||||
}
|
||||
}
|
||||
|
||||
async createDirectory(rawParent: string, rawName: string): Promise<CreateFolderResult> {
|
||||
const name = this.assertSafeName(rawName);
|
||||
const parent = resolve(rawParent || '/');
|
||||
if (this.isBlocked(parent)) {
|
||||
throw new ForbiddenException('Cannot create a folder in this location');
|
||||
}
|
||||
|
||||
const startedAt = Date.now();
|
||||
this.logger.log(`[path.create_folder] [start] parentPath="${sanitizeLogValue(parent)}" name="${sanitizeLogValue(name)}" - create folder started`);
|
||||
try {
|
||||
const parentStat = await lstat(parent); // codeql[js/path-injection]
|
||||
if (parentStat.isSymbolicLink()) throw new ForbiddenException('Cannot create a folder under a symbolic link');
|
||||
if (!parentStat.isDirectory()) throw new BadRequestException('Parent path is not a directory');
|
||||
|
||||
const target = join(parent, name);
|
||||
const prefix = parent.endsWith(sep) ? parent : parent + sep;
|
||||
if (!resolve(target).startsWith(prefix)) {
|
||||
throw new BadRequestException('Invalid folder name');
|
||||
}
|
||||
|
||||
await mkdir(target); // codeql[js/path-injection]
|
||||
this.logger.log(`[path.create_folder] [end] path="${sanitizeLogValue(target)}" durationMs=${Date.now() - startedAt} - create folder completed`);
|
||||
return { name, path: target };
|
||||
} catch (error) {
|
||||
this.logger.warn(
|
||||
`[path.create_folder] [fail] parentPath="${sanitizeLogValue(parent)}" name="${sanitizeLogValue(name)}" durationMs=${Date.now() - startedAt} errorClass=${getErrorClass(error)} error="${sanitizeLogValue(getErrorMessage(error))}" - create folder failed`,
|
||||
);
|
||||
throw this.mapCreateError(error);
|
||||
}
|
||||
}
|
||||
|
||||
private isBlocked(resolved: string): boolean {
|
||||
return BLOCKED.some((b) => resolved === b || resolved.startsWith(b + sep));
|
||||
}
|
||||
|
||||
private assertSafeName(rawName: string): string {
|
||||
const name = (rawName ?? '').trim();
|
||||
if (!name) throw new BadRequestException('Folder name is required');
|
||||
if (name.includes('/') || name.includes('\\') || name.includes(sep)) {
|
||||
throw new BadRequestException('Folder name cannot contain path separators');
|
||||
}
|
||||
if (name === '.' || name === '..' || name.startsWith('.')) {
|
||||
throw new BadRequestException('Folder name cannot start with a dot');
|
||||
}
|
||||
if (name.includes('\0')) throw new BadRequestException('Folder name contains invalid characters');
|
||||
return name;
|
||||
}
|
||||
|
||||
private mapCreateError(error: unknown): HttpException {
|
||||
if (error instanceof HttpException) return error;
|
||||
switch (getErrorCode(error)) {
|
||||
case 'EEXIST':
|
||||
return new ConflictException('A folder with that name already exists');
|
||||
case 'EACCES':
|
||||
case 'EPERM':
|
||||
return new ForbiddenException('Permission denied');
|
||||
case 'EROFS':
|
||||
return new BadRequestException('The filesystem is read-only');
|
||||
case 'ENOENT':
|
||||
return new BadRequestException('Parent folder no longer exists');
|
||||
default:
|
||||
return new InternalServerErrorException('Could not create the folder');
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
function getErrorCode(error: unknown): string | undefined {
|
||||
if (typeof error !== 'object' || error === null || !('code' in error)) return undefined;
|
||||
const code = (error as { code?: unknown }).code;
|
||||
return typeof code === 'string' ? code : undefined;
|
||||
}
|
||||
|
||||
function getErrorClass(error: unknown): string {
|
||||
return error instanceof Error ? error.constructor.name : 'Error';
|
||||
}
|
||||
|
||||
function getErrorMessage(error: unknown): string {
|
||||
return error instanceof Error ? error.message : String(error);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user