From daf61cb4669039477d31fb7975b1e13736f6e88b Mon Sep 17 00:00:00 2001 From: neon <18559658+neonsolstice@users.noreply.github.com> Date: Thu, 10 Sep 2026 12:17:02 +0000 Subject: [PATCH] fix(scanner): support uploads to ACL-controlled storage Stream cross-device moves into exclusive destination-side temporary files to avoid copying permissions onto ACL-controlled shares. Publish completed files with rename and clean up failed copies while preserving the source. Cover storage failures, staging collisions, concurrent moves, and source preservation with real-file unit tests. Fixes #1342 --- .../upload/upload-storage.service.test.ts | 418 ++++++++++++------ .../modules/upload/upload-storage.service.ts | 32 +- 2 files changed, 318 insertions(+), 132 deletions(-) diff --git a/server/src/modules/upload/upload-storage.service.test.ts b/server/src/modules/upload/upload-storage.service.test.ts index 0d388c211..876a94d89 100644 --- a/server/src/modules/upload/upload-storage.service.test.ts +++ b/server/src/modules/upload/upload-storage.service.test.ts @@ -1,150 +1,322 @@ -vi.mock('fs/promises', () => ({ - copyFile: vi.fn(), - mkdir: vi.fn(), - rename: vi.fn(), - stat: vi.fn(), - unlink: vi.fn(), -})); - -vi.mock('fs', () => ({ createWriteStream: vi.fn() })); -vi.mock('os', () => ({ tmpdir: () => '/tmp' })); -vi.mock('crypto', () => ({ randomUUID: () => 'unit-test-id' })); - -import { PassThrough } from 'stream'; +import { Test, type TestingModule } from '@nestjs/testing'; import { PayloadTooLargeException } from '@nestjs/common'; -import { copyFile, mkdir, rename, stat, unlink } from 'fs/promises'; -import { createWriteStream } from 'fs'; +import { createReadStream, type WriteStream } from 'fs'; +import { copyFile, mkdir, mkdtemp, open, readFile, readdir, rename, rm, stat, symlink, unlink, writeFile } from 'fs/promises'; +import { join } from 'path'; +import { PassThrough, Readable, Writable } from 'stream'; +import { randomUUID } from 'crypto'; +import { AppSettingsService } from '../app-settings/app-settings.service'; import { UploadStorageService } from './upload-storage.service'; -const mockCopyFile = copyFile as MockedFunction; -const mockMkdir = mkdir as MockedFunction; -const mockRename = rename as MockedFunction; -const mockStat = stat as MockedFunction; -const mockUnlink = unlink as MockedFunction; -const mockCreateWriteStream = createWriteStream as MockedFunction; +const control = vi.hoisted(() => ({ tempDir: '' })); + +vi.mock('fs/promises', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + copyFile: vi.fn(actual.copyFile), + open: vi.fn(actual.open), + rename: vi.fn(actual.rename), + stat: vi.fn(actual.stat), + unlink: vi.fn(actual.unlink), + }; +}); + +vi.mock('fs', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, createReadStream: vi.fn(actual.createReadStream) }; +}); + +vi.mock('crypto', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, randomUUID: vi.fn(actual.randomUUID) }; +}); + +vi.mock('os', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, tmpdir: () => control.tempDir }; +}); + +const realFs = await vi.importActual('fs/promises'); +const realOs = await vi.importActual('os'); +const ioError = (code: string) => Object.assign(new Error(`Injected ${code}`), { code }); describe('UploadStorageService', () => { + let module: TestingModule; let service: UploadStorageService; + let source: string; + let destination: string; + let destinationDir: string; + const payload = Buffer.from('Book upload content\n'); + const settings = { getMaxUploadSizeMb: vi.fn().mockResolvedValue(500) }; - beforeEach(() => { + beforeEach(async () => { vi.resetAllMocks(); - const mockAppSettings = { - getMaxUploadSizeMb: vi.fn().mockResolvedValue(500), - } as any; - service = new UploadStorageService(mockAppSettings); - mockMkdir.mockResolvedValue(undefined); - mockRename.mockResolvedValue(undefined); - mockCopyFile.mockResolvedValue(undefined); - mockUnlink.mockResolvedValue(undefined); - mockStat.mockResolvedValue({ size: 123 } as Awaited>); + control.tempDir = await mkdtemp(join(realOs.tmpdir(), 'bookorbit-upload-test-')); + source = join(control.tempDir, 'source.epub'); + destinationDir = join(control.tempDir, 'dock'); + destination = join(destinationDir, 'book.epub'); + await writeFile(source, payload); + module = await Test.createTestingModule({ + providers: [UploadStorageService, { provide: AppSettingsService, useValue: settings }], + }).compile(); + service = module.get(UploadStorageService); }); - it('streams multipart input to temp file and reports size', async () => { - const source = new PassThrough(); - const sink = new PassThrough(); - mockCreateWriteStream.mockReturnValue(sink as unknown as ReturnType); - - const p = service.streamToTemp(source); - source.end(Buffer.from('abc')); - - await expect(p).resolves.toEqual({ tempPath: '/tmp/bookorbit-upload-unit-test-id', sizeBytes: 123 }); + afterEach(async () => { + await module.close(); + await rm(control.tempDir, { recursive: true, force: true }); + vi.restoreAllMocks(); }); - it('throws PayloadTooLargeException and cleans up when busboy truncates stream', async () => { - const source = new PassThrough() as PassThrough & { truncated?: boolean }; - source.truncated = true; - const sink = new PassThrough(); - mockCreateWriteStream.mockReturnValue(sink as unknown as ReturnType); + function crossDeviceMove() { + vi.mocked(rename).mockRejectedValueOnce(ioError('EXDEV')); + return service.moveToPath(source, destination); + } - const p = service.streamToTemp(source); - source.end(Buffer.from('x')); + async function expectPreservedSource() { + expect(await readFile(source)).toEqual(payload); + expect(vi.mocked(unlink).mock.calls.flat()).not.toContain(source); + } - await expect(p).rejects.toBeInstanceOf(PayloadTooLargeException); - expect(mockUnlink).toHaveBeenCalledWith('/tmp/bookorbit-upload-unit-test-id'); + async function expectEmptyDestination() { + expect(await readdir(destinationDir)).toEqual([]); + } + + it('streams multipart input to a temporary file and reports its actual size', async () => { + const result = await service.streamToTemp(Readable.from(payload)); + expect(result.sizeBytes).toBe(payload.length); + expect(await readFile(result.tempPath)).toEqual(payload); }); - it('cleans up temp file when the source stream errors', async () => { - const source = new PassThrough(); - const sink = new PassThrough(); - mockCreateWriteStream.mockReturnValue(sink as unknown as ReturnType); - - const p = service.streamToTemp(source); - source.destroy(new Error('stream read failed')); - - await expect(p).rejects.toThrow('stream read failed'); - expect(mockUnlink).toHaveBeenCalledWith('/tmp/bookorbit-upload-unit-test-id'); + it('rejects a truncated multipart stream and removes its temporary file', async () => { + const input = Object.assign(Readable.from(payload), { truncated: true }); + await expect(service.streamToTemp(input)).rejects.toBeInstanceOf(PayloadTooLargeException); + expect(await readdir(control.tempDir)).toEqual(['source.epub']); + expect(settings.getMaxUploadSizeMb).toHaveBeenCalledOnce(); }); - it('falls back to copy+unlink when rename fails with EXDEV', async () => { - mockRename.mockRejectedValue(Object.assign(new Error('cross-device'), { code: 'EXDEV' })); - - await service.moveToPath('/tmp/a', '/books/x/file.epub'); - - expect(mockMkdir).toHaveBeenCalledWith('/books/x', { recursive: true }); - expect(mockCopyFile).toHaveBeenCalledWith('/tmp/a', '/books/x/file.epub'); - expect(mockUnlink).toHaveBeenCalledWith('/tmp/a'); + it('preserves the stream error and removes the incomplete temporary upload', async () => { + const failure = ioError('ECONNRESET'); + const input = Readable.from( + (function* () { + yield payload; + throw failure; + })(), + ); + await expect(service.streamToTemp(input)).rejects.toBe(failure); + expect(await readdir(control.tempDir)).toEqual(['source.epub']); }); - it('rethrows non-EXDEV rename errors', async () => { - mockRename.mockRejectedValue(new Error('permission denied')); - - await expect(service.moveToPath('/tmp/a', '/books/x/file.epub')).rejects.toThrow('permission denied'); - expect(mockCopyFile).not.toHaveBeenCalled(); + it('uses rename directly on the same filesystem, without opening a staging file', async () => { + await service.moveToPath(source, destination); + expect(await readFile(destination)).toEqual(payload); + await expect(stat(source)).rejects.toMatchObject({ code: 'ENOENT' }); + expect(rename).toHaveBeenCalledExactlyOnceWith(source, destination); + expect(open).not.toHaveBeenCalled(); + expect(createReadStream).not.toHaveBeenCalled(); }); - it('cleanup ignores ENOENT and warns on other unlink failures', async () => { - const warn = vi.spyOn((service as unknown as { logger: { warn: (m: string) => void } }).logger, 'warn').mockImplementation(); + it.each(['EACCES', 'EPERM', 'ENOENT', 'EIO'])('propagates %s from rename without attempting a copy', async (code) => { + const failure = ioError(code); + vi.mocked(rename).mockRejectedValueOnce(failure); + await expect(service.moveToPath(source, destination)).rejects.toBe(failure); + expect(open).not.toHaveBeenCalled(); + await expectPreservedSource(); + await expectEmptyDestination(); + }); - mockUnlink.mockRejectedValueOnce(Object.assign(new Error('gone'), { code: 'ENOENT' })); - await service.cleanup('/tmp/missing'); + it('does not move or delete the source when the destination directory cannot be created', async () => { + await writeFile(destinationDir, 'not a directory'); + await expect(service.moveToPath(source, destination)).rejects.toMatchObject({ code: 'EEXIST' }); + expect(rename).not.toHaveBeenCalled(); + await expectPreservedSource(); + }); + + it('copies bytes across devices without requiring permission-copying support', async () => { + vi.mocked(copyFile).mockRejectedValue(ioError('EPERM')); + await crossDeviceMove(); + expect(await readFile(destination)).toEqual(payload); + await expect(stat(source)).rejects.toMatchObject({ code: 'ENOENT' }); + expect(await readdir(destinationDir)).toEqual(['book.epub']); + expect(copyFile).not.toHaveBeenCalled(); + expect(open).toHaveBeenCalledWith(expect.stringMatching(/\.bookorbit-upload-.*\.tmp$/), 'wx'); + expect(unlink).toHaveBeenCalledExactlyOnceWith(source); + expect(vi.mocked(rename).mock.invocationCallOrder[1]).toBeLessThan(vi.mocked(unlink).mock.invocationCallOrder[0]); + }); + + it('keeps the final filename absent and the source intact until the stream has finished', async () => { + const input = new PassThrough(); + vi.mocked(createReadStream).mockReturnValueOnce(input as ReturnType); + const moving = crossDeviceMove(); + try { + await vi.waitFor(() => expect(input.listenerCount('data')).toBeGreaterThan(0)); + input.write(payload); + await expect(stat(destination)).rejects.toMatchObject({ code: 'ENOENT' }); + await expectPreservedSource(); + expect(await readdir(destinationDir)).toEqual([expect.stringMatching(/^\.bookorbit-upload-.*\.tmp$/)]); + expect(rename).toHaveBeenCalledOnce(); + } finally { + input.end(); + await moving; + } + expect(await readFile(destination)).toEqual(payload); + }); + + it('copies a multi-chunk binary file without altering its contents', async () => { + const bytes = Buffer.alloc(2 * 1024 * 1024); + for (let i = 0; i < bytes.length; i++) bytes[i] = i % 251; + await writeFile(source, bytes); + await crossDeviceMove(); + expect((await readFile(destination)).equals(bytes)).toBe(true); + }); + + it('supports empty files', async () => { + await writeFile(source, ''); + await crossDeviceMove(); + expect((await stat(destination)).size).toBe(0); + await expect(stat(source)).rejects.toMatchObject({ code: 'ENOENT' }); + }); + + it('does not copy restrictive source permissions to the destination', async () => { + await realFs.chmod(source, 0o400); + await mkdir(destinationDir); + const reference = join(destinationDir, 'reference'); + await writeFile(reference, ''); + const expectedMode = (await stat(reference)).mode & 0o777; + await crossDeviceMove(); + expect((await stat(destination)).mode & 0o777).toBe(expectedMode); + }); + + it.each(['EACCES', 'ENOSPC'])('preserves the source when staging creation fails with %s', async (code) => { + const failure = ioError(code); + vi.mocked(open).mockRejectedValueOnce(failure); + await expect(crossDeviceMove()).rejects.toBe(failure); + await expectPreservedSource(); + await expectEmptyDestination(); + expect(unlink).not.toHaveBeenCalled(); + }); + + it.each(['file', 'symlink'])('does not overwrite or delete a colliding staging %s', async (kind) => { + const id = '00000000-0000-4000-8000-000000000000'; + vi.mocked(randomUUID).mockReturnValueOnce(id); + await mkdir(destinationDir); + const staging = join(destinationDir, `.bookorbit-upload-${id}.tmp`); + if (kind === 'symlink') await symlink(source, staging); + else await writeFile(staging, 'existing data'); + await expect(crossDeviceMove()).rejects.toMatchObject({ code: 'EEXIST' }); + expect(await readFile(staging)).toEqual(kind === 'symlink' ? payload : Buffer.from('existing data')); + await expectPreservedSource(); + expect(unlink).not.toHaveBeenCalled(); + }); + + it('removes staging and preserves the source after a read error mid-copy', async () => { + const failure = ioError('EIO'); + const input = Readable.from( + (function* () { + yield payload; + throw failure; + })(), + ); + vi.mocked(createReadStream).mockReturnValueOnce(input as ReturnType); + await expect(crossDeviceMove()).rejects.toBe(failure); + await expectPreservedSource(); + await expectEmptyDestination(); + expect(rename).toHaveBeenCalledOnce(); + }); + + it('closes staging and preserves the source when opening the read stream throws', async () => { + const failure = ioError('EIO'); + vi.mocked(createReadStream).mockImplementationOnce(() => { + throw failure; + }); + await expect(crossDeviceMove()).rejects.toBe(failure); + const handle = await vi.mocked(open).mock.results[0].value; + expect(handle.fd).toBe(-1); + await expectPreservedSource(); + await expectEmptyDestination(); + }); + + it('closes staging and preserves the source when the destination stream runs out of space', async () => { + const failure = ioError('ENOSPC'); + vi.mocked(open).mockImplementationOnce(async (...args) => { + const handle = await realFs.open(...args); + vi.spyOn(handle, 'createWriteStream').mockReturnValueOnce( + new Writable({ + write(_chunk, _encoding, callback) { + callback(failure); + }, + }) as WriteStream, + ); + return handle; + }); + await expect(crossDeviceMove()).rejects.toBe(failure); + const handle = await vi.mocked(open).mock.results[0].value; + expect(handle.fd).toBe(-1); + await expectPreservedSource(); + await expectEmptyDestination(); + expect(rename).toHaveBeenCalledOnce(); + }); + + it('preserves an existing destination and source when final placement fails', async () => { + await mkdir(destinationDir); + await writeFile(destination, 'existing book'); + const failure = ioError('EACCES'); + vi.mocked(rename).mockRejectedValueOnce(ioError('EXDEV')).mockRejectedValueOnce(failure); + await expect(service.moveToPath(source, destination)).rejects.toBe(failure); + expect(await readFile(destination, 'utf8')).toBe('existing book'); + expect(await readdir(destinationDir)).toEqual(['book.epub']); + await expectPreservedSource(); + }); + + it('retains rename replacement semantics after a successful cross-device copy', async () => { + await mkdir(destinationDir); + await writeFile(destination, 'old content'); + await crossDeviceMove(); + expect(await readFile(destination)).toEqual(payload); + expect(await readdir(destinationDir)).toEqual(['book.epub']); + }); + + it('preserves the primary error and logs a staging cleanup failure', async () => { + const warn = vi.spyOn(service['logger'], 'warn').mockImplementation(() => {}); + const failure = ioError('EACCES'); + vi.mocked(rename).mockRejectedValueOnce(ioError('EXDEV')).mockRejectedValueOnce(failure); + vi.mocked(unlink).mockRejectedValueOnce(ioError('EBUSY')); + await expect(service.moveToPath(source, destination)).rejects.toBe(failure); + await expectPreservedSource(); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('EBUSY')); + }); + + it('keeps the completed destination and logs when source cleanup fails', async () => { + const warn = vi.spyOn(service['logger'], 'warn').mockImplementation(() => {}); + vi.mocked(unlink).mockRejectedValueOnce(ioError('EBUSY')); + await crossDeviceMove(); + expect(await readFile(destination)).toEqual(payload); + expect(await readFile(source)).toEqual(payload); + expect(await readdir(destinationDir)).toEqual(['book.epub']); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('EBUSY')); + }); + + it('handles concurrent cross-device moves using distinct staging files', async () => { + const otherSource = join(control.tempDir, 'other.epub'); + const otherDestination = join(destinationDir, 'other.epub'); + await writeFile(otherSource, 'another book'); + vi.mocked(rename).mockImplementation(async (from, to) => { + if (from === source || from === otherSource) throw ioError('EXDEV'); + return realFs.rename(from, to); + }); + await Promise.all([service.moveToPath(source, destination), service.moveToPath(otherSource, otherDestination)]); + expect(await readFile(destination)).toEqual(payload); + expect(await readFile(otherDestination, 'utf8')).toBe('another book'); + expect((await readdir(destinationDir)).sort()).toEqual(['book.epub', 'other.epub']); + expect(new Set(vi.mocked(open).mock.calls.map(([path]) => path)).size).toBe(2); + }); + + it('ignores missing files during concurrent cleanup', async () => { + const warn = vi.spyOn(service['logger'], 'warn').mockImplementation(() => {}); + await Promise.all([service.cleanup(source), service.cleanup(source), service.cleanup(source)]); + await expect(stat(source)).rejects.toMatchObject({ code: 'ENOENT' }); expect(warn).not.toHaveBeenCalled(); - - mockUnlink.mockRejectedValueOnce(Object.assign(new Error('io fail'), { code: 'EIO' })); - await service.cleanup('/tmp/bad'); - expect(warn).toHaveBeenCalledWith(expect.stringContaining('io fail')); - }); - - it('propagates stat error after successful write in streamToTemp', async () => { - const source = new PassThrough(); - const sink = new PassThrough(); - mockCreateWriteStream.mockReturnValue(sink as unknown as ReturnType); - mockStat.mockRejectedValue(new Error('stat failed')); - - const p = service.streamToTemp(source); - source.end(Buffer.from('data')); - - await expect(p).rejects.toThrow('stat failed'); - }); - - it('propagates mkdir error in moveToPath', async () => { - mockMkdir.mockRejectedValue(new Error('mkdir failed')); - - await expect(service.moveToPath('/tmp/a', '/books/x/file.epub')).rejects.toThrow('mkdir failed'); - expect(mockRename).not.toHaveBeenCalled(); - }); - - it('propagates copyFile error during EXDEV fallback', async () => { - mockRename.mockRejectedValue(Object.assign(new Error('cross-device'), { code: 'EXDEV' })); - mockCopyFile.mockRejectedValue(new Error('copy failed')); - - await expect(service.moveToPath('/tmp/a', '/books/x/file.epub')).rejects.toThrow('copy failed'); - }); - - it('propagates unlink error after copy in EXDEV fallback', async () => { - mockRename.mockRejectedValue(Object.assign(new Error('cross-device'), { code: 'EXDEV' })); - mockCopyFile.mockResolvedValue(undefined); - mockUnlink.mockRejectedValue(Object.assign(new Error('unlink failed'), { code: 'EBUSY' })); - - await service.moveToPath('/tmp/a', '/books/x/file.epub'); - expect(mockCopyFile).toHaveBeenCalledWith('/tmp/a', '/books/x/file.epub'); - expect(mockUnlink).toHaveBeenCalledWith('/tmp/a'); - }); - - it('concurrent cleanup calls are safe', async () => { - mockUnlink.mockRejectedValue(Object.assign(new Error('gone'), { code: 'ENOENT' })); - - await Promise.all([service.cleanup('/tmp/file'), service.cleanup('/tmp/file'), service.cleanup('/tmp/file')]); - expect(mockUnlink).toHaveBeenCalledTimes(3); }); }); diff --git a/server/src/modules/upload/upload-storage.service.ts b/server/src/modules/upload/upload-storage.service.ts index 34655e9ab..8f48052bb 100644 --- a/server/src/modules/upload/upload-storage.service.ts +++ b/server/src/modules/upload/upload-storage.service.ts @@ -1,6 +1,6 @@ import { Injectable, Logger, PayloadTooLargeException } from '@nestjs/common'; -import { copyFile, mkdir, rename, stat, unlink } from 'fs/promises'; -import { createWriteStream } from 'fs'; +import { mkdir, open, rename, stat, unlink } from 'fs/promises'; +import { createReadStream, createWriteStream } from 'fs'; import { tmpdir } from 'os'; import { dirname, join } from 'path'; import { Readable } from 'stream'; @@ -46,7 +46,7 @@ export class UploadStorageService { /** * Moves the temp file to an already-resolved absolute destination path. * Creates parent directories as needed. - * Uses rename() and falls back to copy+unlink for cross-device moves. + * Uses rename() and stages cross-device copies on the destination filesystem. */ async moveToPath(tempPath: string, absolutePath: string): Promise { await mkdir(dirname(absolutePath), { recursive: true }); @@ -54,15 +54,29 @@ export class UploadStorageService { try { await rename(tempPath, absolutePath); } catch (err) { - if ((err as NodeJS.ErrnoException).code === 'EXDEV') { - await copyFile(tempPath, absolutePath); - await this.cleanup(tempPath); - } else { - throw err; - } + if ((err as NodeJS.ErrnoException).code !== 'EXDEV') throw err; + await this.copyAcrossDevices(tempPath, absolutePath); } } + private async copyAcrossDevices(sourcePath: string, destinationPath: string): Promise { + const stagingPath = join(dirname(destinationPath), `.bookorbit-upload-${randomUUID()}.tmp`); + const stagingFile = await open(stagingPath, 'wx'); + + try { + // copyFile also copies permissions, which ACL-backed shares may forbid despite allowing writes. + // Stage without a book extension so scanners cannot ingest a partially written file. + await pipeline(createReadStream(sourcePath), stagingFile.createWriteStream()); + await rename(stagingPath, destinationPath); + } catch (err) { + await stagingFile.close().catch(() => {}); + await this.cleanup(stagingPath); + throw err; + } + + await this.cleanup(sourcePath); + } + async cleanup(tempPath: string): Promise { await unlink(tempPath).catch((err: NodeJS.ErrnoException) => { if (err.code !== 'ENOENT') {