From df45ef0e35732a1a0537660a94073188797d5942 Mon Sep 17 00:00:00 2001 From: Carsten Otto Date: Tue, 13 Aug 2024 17:39:24 +0200 Subject: [PATCH] fix(server): follow symlinks when zipping assets (#11685) * follow symlinks when zipping assets fixes #9335 * chore: clean up --------- Co-authored-by: Jason Rasmussen --- server/src/interfaces/storage.interface.ts | 1 + server/src/repositories/storage.repository.ts | 6 ++++- server/src/services/download.service.spec.ts | 27 ++++++++++++++++++- server/src/services/download.service.ts | 14 ++++++++-- .../repositories/storage.repository.mock.ts | 1 + 5 files changed, 45 insertions(+), 4 deletions(-) diff --git a/server/src/interfaces/storage.interface.ts b/server/src/interfaces/storage.interface.ts index 1bd49a3f20..f27edaccc9 100644 --- a/server/src/interfaces/storage.interface.ts +++ b/server/src/interfaces/storage.interface.ts @@ -36,6 +36,7 @@ export interface IStorageRepository { createReadStream(filepath: string, mimeType?: string | null): Promise; readFile(filepath: string, options?: FileReadOptions): Promise; writeFile(filepath: string, buffer: Buffer): Promise; + realpath(filepath: string): Promise; unlink(filepath: string): Promise; unlinkDir(folder: string, options?: { recursive?: boolean; force?: boolean }): Promise; removeEmptyDirs(folder: string, self?: boolean): Promise; diff --git a/server/src/repositories/storage.repository.ts b/server/src/repositories/storage.repository.ts index 0d0be5c062..b310f2e110 100644 --- a/server/src/repositories/storage.repository.ts +++ b/server/src/repositories/storage.repository.ts @@ -24,6 +24,10 @@ export class StorageRepository implements IStorageRepository { this.logger.setContext(StorageRepository.name); } + realpath(filepath: string) { + return fs.realpath(filepath); + } + readdir(folder: string): Promise { return fs.readdir(folder); } @@ -52,7 +56,7 @@ export class StorageRepository implements IStorageRepository { const archive = archiver('zip', { store: true }); const addFile = (input: string, filename: string) => { - archive.file(input, { name: filename }); + archive.file(input, { name: filename, mode: 0o644 }); }; const finalize = () => archive.finalize(); diff --git a/server/src/services/download.service.spec.ts b/server/src/services/download.service.spec.ts index 6216a4dc3a..2d3c11a6f1 100644 --- a/server/src/services/download.service.spec.ts +++ b/server/src/services/download.service.spec.ts @@ -2,12 +2,14 @@ import { BadRequestException } from '@nestjs/common'; import { DownloadResponseDto } from 'src/dtos/download.dto'; import { AssetEntity } from 'src/entities/asset.entity'; import { IAssetRepository } from 'src/interfaces/asset.interface'; +import { ILoggerRepository } from 'src/interfaces/logger.interface'; import { IStorageRepository } from 'src/interfaces/storage.interface'; import { DownloadService } from 'src/services/download.service'; import { assetStub } from 'test/fixtures/asset.stub'; import { authStub } from 'test/fixtures/auth.stub'; import { IAccessRepositoryMock, newAccessRepositoryMock } from 'test/repositories/access.repository.mock'; import { newAssetRepositoryMock } from 'test/repositories/asset.repository.mock'; +import { newLoggerRepositoryMock } from 'test/repositories/logger.repository.mock'; import { newStorageRepositoryMock } from 'test/repositories/storage.repository.mock'; import { Readable } from 'typeorm/platform/PlatformTools.js'; import { Mocked, vitest } from 'vitest'; @@ -26,6 +28,7 @@ describe(DownloadService.name, () => { let sut: DownloadService; let accessMock: IAccessRepositoryMock; let assetMock: Mocked; + let loggerMock: Mocked; let storageMock: Mocked; it('should work', () => { @@ -35,9 +38,10 @@ describe(DownloadService.name, () => { beforeEach(() => { accessMock = newAccessRepositoryMock(); assetMock = newAssetRepositoryMock(); + loggerMock = newLoggerRepositoryMock(); storageMock = newStorageRepositoryMock(); - sut = new DownloadService(accessMock, assetMock, storageMock); + sut = new DownloadService(accessMock, assetMock, loggerMock, storageMock); }); describe('downloadArchive', () => { @@ -109,6 +113,27 @@ describe(DownloadService.name, () => { expect(archiveMock.addFile).toHaveBeenNthCalledWith(1, 'upload/library/IMG_123.jpg', 'IMG_123.jpg'); expect(archiveMock.addFile).toHaveBeenNthCalledWith(2, 'upload/library/IMG_123.jpg', 'IMG_123+1.jpg'); }); + + it('should resolve symlinks', async () => { + const archiveMock = { + addFile: vitest.fn(), + finalize: vitest.fn(), + stream: new Readable(), + }; + + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1'])); + assetMock.getByIds.mockResolvedValue([ + { ...assetStub.noResizePath, id: 'asset-1', originalPath: '/path/to/symlink.jpg' }, + ]); + storageMock.realpath.mockResolvedValue('/path/to/realpath.jpg'); + storageMock.createZipStream.mockReturnValue(archiveMock); + + await expect(sut.downloadArchive(authStub.admin, { assetIds: ['asset-1'] })).resolves.toEqual({ + stream: archiveMock.stream, + }); + + expect(archiveMock.addFile).toHaveBeenCalledWith('/path/to/realpath.jpg', 'IMG_123.jpg'); + }); }); describe('getDownloadInfo', () => { diff --git a/server/src/services/download.service.ts b/server/src/services/download.service.ts index 07ef03efb5..11e4de83d9 100644 --- a/server/src/services/download.service.ts +++ b/server/src/services/download.service.ts @@ -7,7 +7,8 @@ import { DownloadArchiveInfo, DownloadInfoDto, DownloadResponseDto } from 'src/d import { AssetEntity } from 'src/entities/asset.entity'; import { IAccessRepository } from 'src/interfaces/access.interface'; import { IAssetRepository } from 'src/interfaces/asset.interface'; -import { IStorageRepository, ImmichReadStream } from 'src/interfaces/storage.interface'; +import { ILoggerRepository } from 'src/interfaces/logger.interface'; +import { ImmichReadStream, IStorageRepository } from 'src/interfaces/storage.interface'; import { HumanReadableSize } from 'src/utils/bytes'; import { usePagination } from 'src/utils/pagination'; @@ -18,9 +19,11 @@ export class DownloadService { constructor( @Inject(IAccessRepository) accessRepository: IAccessRepository, @Inject(IAssetRepository) private assetRepository: IAssetRepository, + @Inject(ILoggerRepository) private logger: ILoggerRepository, @Inject(IStorageRepository) private storageRepository: IStorageRepository, ) { this.access = AccessCore.create(accessRepository); + this.logger.setContext(DownloadService.name); } async getDownloadInfo(auth: AuthDto, dto: DownloadInfoDto): Promise { @@ -83,7 +86,14 @@ export class DownloadService { filename = `${parsedFilename.name}+${count}${parsedFilename.ext}`; } - zip.addFile(originalPath, filename); + let realpath = originalPath; + try { + realpath = await this.storageRepository.realpath(originalPath); + } catch { + this.logger.warn('Unable to resolve realpath', { originalPath }); + } + + zip.addFile(realpath, filename); } void zip.finalize(); diff --git a/server/test/repositories/storage.repository.mock.ts b/server/test/repositories/storage.repository.mock.ts index 615fd5d8c9..5c2951e097 100644 --- a/server/test/repositories/storage.repository.mock.ts +++ b/server/test/repositories/storage.repository.mock.ts @@ -56,6 +56,7 @@ export const newStorageRepositoryMock = (reset = true): Mocked Promise.resolve(filepath)), stat: vitest.fn(), crawl: vitest.fn(), walk: vitest.fn().mockImplementation(async function* () {}),