From afd6d297c0d1169de52254c8a30eccc9677dac0c Mon Sep 17 00:00:00 2001 From: jubnl Date: Sun, 16 Aug 2026 11:12:28 +0200 Subject: [PATCH] refactor(journey): serve public journey assets through storage --- .../nest/journey/journey-public.controller.ts | 19 ++-- .../unit/nest/journey.controller.test.ts | 91 +++++++++++-------- 2 files changed, 64 insertions(+), 46 deletions(-) diff --git a/server/src/nest/journey/journey-public.controller.ts b/server/src/nest/journey/journey-public.controller.ts index f31de1406..d2e492716 100644 --- a/server/src/nest/journey/journey-public.controller.ts +++ b/server/src/nest/journey/journey-public.controller.ts @@ -1,8 +1,8 @@ import { Controller, Get, HttpException, Param, Res } from '@nestjs/common'; import type { Response } from 'express'; import path from 'node:path'; -import fs from 'node:fs'; import { JourneyService } from './journey.service'; +import { StorageService } from '../storage/storage.service'; import { Public } from '../auth/public.decorator'; /** @@ -17,7 +17,10 @@ import { Public } from '../auth/public.decorator'; @Public('share-token validated: the whole point is a link that works without an account') @Controller('api/public/journey') export class JourneyPublicController { - constructor(private readonly journey: JourneyService) {} + constructor( + private readonly journey: JourneyService, + private readonly storage: StorageService, + ) {} @Get(':token') get(@Param('token') token: string) { @@ -54,15 +57,15 @@ export class JourneyPublicController { const wantThumb = kind === 'thumbnail' ? 'thumbnail' : 'original'; if (provider === 'local') { - // Local journey assets are flat filenames; use basename() and confine the - // resolved path to the journey upload directory. - const journeyDir = path.resolve(__dirname, '../../../uploads/journey'); - const resolved = path.resolve(path.join(journeyDir, path.basename(assetId))); - if (!resolved.startsWith(journeyDir + path.sep) || !fs.existsSync(resolved)) { + // Local journey assets are flat filenames. basename() kept for shape; + // containment now lives in central storage key validation + // (storage-keys.ts), which subsumes the old journeyDir startsWith guard. + const name = path.basename(assetId); + if (!(await this.storage.exists('journey', name).catch(() => false))) { throw new HttpException({ error: 'Not found' }, 404); } res.set('Cache-Control', 'public, max-age=86400'); - res.sendFile(resolved); + await this.storage.sendToResponse('journey', name, res); return; } diff --git a/server/tests/unit/nest/journey.controller.test.ts b/server/tests/unit/nest/journey.controller.test.ts index 9563cba61..aa0d996c2 100644 --- a/server/tests/unit/nest/journey.controller.test.ts +++ b/server/tests/unit/nest/journey.controller.test.ts @@ -1,7 +1,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { HttpException } from '@nestjs/common'; import type { Response } from 'express'; -import path from 'node:path'; +// Still spied on by the DELETE-photo cases: those raw unlinks are slice-4 scope. import fs from 'node:fs'; import { JourneyController } from '../../../src/nest/journey/journey.controller'; @@ -16,9 +16,13 @@ function svc(o: Partial = {}): JourneyService { return { journeyAddonEnabled: vi.fn().mockReturnValue(true), ...o } as unknown as JourneyService; } +const storageExists = vi.fn(); +const storageSendToResponse = vi.fn(); const storageStub = { put: vi.fn().mockResolvedValue(undefined), delete: vi.fn().mockResolvedValue(undefined), + exists: storageExists, + sendToResponse: storageSendToResponse, } as unknown as StorageService; function thrown(fn: () => unknown): { status: number; body: unknown } { @@ -293,46 +297,46 @@ describe('JourneyController', () => { describe('JourneyPublicController', () => { it('GET /:token 404 / json', () => { - expect(thrown(() => new JourneyPublicController(svc({ getPublicJourney: vi.fn().mockReturnValue(null) } as Partial)).get('tok'))).toEqual({ status: 404, body: { error: 'Not found' } }); - expect(new JourneyPublicController(svc({ getPublicJourney: vi.fn().mockReturnValue({ id: 1 }) } as Partial)).get('tok')).toEqual({ id: 1 }); + expect(thrown(() => new JourneyPublicController(svc({ getPublicJourney: vi.fn().mockReturnValue(null) } as Partial), storageStub).get('tok'))).toEqual({ status: 404, body: { error: 'Not found' } }); + expect(new JourneyPublicController(svc({ getPublicJourney: vi.fn().mockReturnValue({ id: 1 }) } as Partial), storageStub).get('tok')).toEqual({ id: 1 }); }); it('photo proxy 404 on invalid token, else streams', async () => { - expect(await thrownAsync(() => new JourneyPublicController(svc({ validateShareTokenForPhoto: vi.fn().mockReturnValue(null) } as Partial)).photo('tok', '7', 'thumbnail', {} as Response))).toEqual({ status: 404, body: { error: 'Not found' } }); + expect(await thrownAsync(() => new JourneyPublicController(svc({ validateShareTokenForPhoto: vi.fn().mockReturnValue(null) } as Partial), storageStub).photo('tok', '7', 'thumbnail', {} as Response))).toEqual({ status: 404, body: { error: 'Not found' } }); const streamPhoto = vi.fn().mockResolvedValue(undefined); const s = svc({ validateShareTokenForPhoto: vi.fn().mockReturnValue({ ownerId: 2 }), streamPhoto } as Partial); - await new JourneyPublicController(s).photo('tok', '7', 'original', {} as Response); + await new JourneyPublicController(s, storageStub).photo('tok', '7', 'original', {} as Response); expect(streamPhoto).toHaveBeenCalledWith({}, 2, 7, 'original'); }); it('legacy photo proxy: 404 invalid token, immich path streams', async () => { - expect(await thrownAsync(() => new JourneyPublicController(svc({ validateShareTokenForAsset: vi.fn().mockReturnValue(null) } as Partial)).legacyPhoto('tok', 'immich', 'a1', '2', 'thumbnail', {} as Response))).toEqual({ status: 404, body: { error: 'Not found' } }); + expect(await thrownAsync(() => new JourneyPublicController(svc({ validateShareTokenForAsset: vi.fn().mockReturnValue(null) } as Partial), storageStub).legacyPhoto('tok', 'immich', 'a1', '2', 'thumbnail', {} as Response))).toEqual({ status: 404, body: { error: 'Not found' } }); // One call for every provider now, with the ids in a ref instead of in a // per-provider argument order. const streamProviderAsset = vi.fn().mockResolvedValue(undefined); const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 5 }), streamProviderAsset } as Partial); - await new JourneyPublicController(s).legacyPhoto('tok', 'immich', 'a1', '2', 'original', {} as Response); + await new JourneyPublicController(s, storageStub).legacyPhoto('tok', 'immich', 'a1', '2', 'original', {} as Response); expect(streamProviderAsset).toHaveBeenCalledWith({}, 'immich', { userId: 5, ownerId: 5, assetId: 'a1' }, 'original'); }); it('photo proxy streams thumbnails too', async () => { const streamPhoto = vi.fn().mockResolvedValue(undefined); const s = svc({ validateShareTokenForPhoto: vi.fn().mockReturnValue({ ownerId: 3 }), streamPhoto } as Partial); - await new JourneyPublicController(s).photo('tok', '7', 'thumbnail', {} as Response); + await new JourneyPublicController(s, storageStub).photo('tok', '7', 'thumbnail', {} as Response); expect(streamPhoto).toHaveBeenCalledWith({}, 3, 7, 'thumbnail'); }); it('legacy photo proxy: synology streams, and a failure becomes a 404 json', async () => { const streamProviderAsset = vi.fn().mockResolvedValue(undefined); const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 5 }), streamProviderAsset } as Partial); - await new JourneyPublicController(s).legacyPhoto('tok', 'synologyphotos', 'a1', '2', 'thumbnail', {} as Response); + await new JourneyPublicController(s, storageStub).legacyPhoto('tok', 'synologyphotos', 'a1', '2', 'thumbnail', {} as Response); expect(streamProviderAsset).toHaveBeenCalledWith({}, 'synologyphotos', { userId: 5, ownerId: 5, assetId: 'a1' }, 'thumbnail'); const status = vi.fn().mockReturnThis(); const json = vi.fn(); const res = { status, json } as unknown as Response; const failing = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 0 }), streamProviderAsset: vi.fn().mockRejectedValue(new Error('no synology')) } as Partial); - await new JourneyPublicController(failing).legacyPhoto('tok', 'synologyphotos', 'a1', '6', 'original', res); + await new JourneyPublicController(failing, storageStub).legacyPhoto('tok', 'synologyphotos', 'a1', '6', 'original', res); expect(status).toHaveBeenCalledWith(404); expect(json).toHaveBeenCalledWith({ error: 'Provider not supported' }); }); @@ -346,7 +350,7 @@ describe('JourneyPublicController', () => { const json = vi.fn(); const res = { status, json } as unknown as Response; const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 5 }), streamProviderAsset } as Partial); - await new JourneyPublicController(s).legacyPhoto('tok', 'photoprism', 'a1', '2', 'original', res); + await new JourneyPublicController(s, storageStub).legacyPhoto('tok', 'photoprism', 'a1', '2', 'original', res); expect(status).toHaveBeenCalledWith(404); expect(json).toHaveBeenCalledWith({ error: 'Provider not supported' }); }); @@ -354,40 +358,51 @@ describe('JourneyPublicController', () => { it('legacy photo proxy: falls back to the path ownerId when the token has none', async () => { const streamProviderAsset = vi.fn().mockResolvedValue(undefined); const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 0 }), streamProviderAsset } as Partial); - await new JourneyPublicController(s).legacyPhoto('tok', 'immich', 'a1', '8', 'original', {} as Response); + await new JourneyPublicController(s, storageStub).legacyPhoto('tok', 'immich', 'a1', '8', 'original', {} as Response); expect(streamProviderAsset).toHaveBeenCalledWith({}, 'immich', { userId: 8, ownerId: 8, assetId: 'a1' }, 'original'); }); - it('legacy photo proxy: local provider 404s when the resolved file does not exist', async () => { - const existsSpy = vi.spyOn(fs, 'existsSync').mockReturnValue(false); - try { - const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 5 }) } as Partial); - expect(await thrownAsync(() => new JourneyPublicController(s).legacyPhoto('tok', 'local', 'gone.jpg', '2', 'thumbnail', {} as Response))).toEqual({ status: 404, body: { error: 'Not found' } }); - } finally { - existsSpy.mockRestore(); - } + it('legacy photo proxy: local provider 404s when the object does not exist', async () => { + storageExists.mockResolvedValue(false); + const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 5 }) } as Partial); + expect(await thrownAsync(() => new JourneyPublicController(s, storageStub).legacyPhoto('tok', 'local', 'gone.jpg', '2', 'thumbnail', {} as Response))).toEqual({ status: 404, body: { error: 'Not found' } }); + expect(storageSendToResponse).not.toHaveBeenCalled(); + }); + + it('legacy photo proxy: local provider serves through storage with the day cache header', async () => { + storageExists.mockResolvedValue(true); + storageSendToResponse.mockResolvedValue(undefined); + const set = vi.fn(); + const res = { set } as unknown as Response; + const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 5 }) } as Partial); + await new JourneyPublicController(s, storageStub).legacyPhoto('tok', 'local', 'photo.jpg', '2', 'original', res); + expect(storageExists).toHaveBeenCalledWith('journey', 'photo.jpg'); + expect(set).toHaveBeenCalledWith('Cache-Control', 'public, max-age=86400'); + expect(storageSendToResponse).toHaveBeenCalledWith('journey', 'photo.jpg', res); }); it('legacy photo proxy: local provider cannot escape uploads/journey via a traversal asset id', async () => { - // Pretend any path exists so we can inspect exactly what would be served. - const existsSpy = vi.spyOn(fs, 'existsSync').mockReturnValue(true); - try { - const sendFile = vi.fn(); - const res = { set: vi.fn(), sendFile } as unknown as Response; - const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 5 }) } as Partial); + storageExists.mockResolvedValue(true); + storageSendToResponse.mockResolvedValue(undefined); + const res = { set: vi.fn() } as unknown as Response; + const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 5 }) } as Partial); - // Express decodes %2F in a single path param to '/', so the handler sees this. - await new JourneyPublicController(s).legacyPhoto('tok', 'local', '../../files/secret.pdf', '2', 'original', res); + // Express decodes %2F in a single path param to '/', so the handler sees this. + await new JourneyPublicController(s, storageStub).legacyPhoto('tok', 'local', '../../files/secret.pdf', '2', 'original', res); - expect(sendFile).toHaveBeenCalledTimes(1); - const served = sendFile.mock.calls[0][0] as string; - // basename() collapses the traversal: the served file stays inside - // uploads/journey and never reaches the sibling /uploads/files dir. - expect(path.basename(served)).toBe('secret.pdf'); - expect(served).toMatch(/[\\/]journey[\\/]secret\.pdf$/); - expect(served).not.toMatch(/[\\/]files[\\/]/); - } finally { - existsSpy.mockRestore(); - } + // basename() collapses the traversal before the storage lookup, and central + // key validation (storage-keys.ts) rejects anything that still carries a + // path — the asset can never address the sibling files/ category. + expect(storageExists).toHaveBeenCalledWith('journey', 'secret.pdf'); + expect(storageSendToResponse).toHaveBeenCalledWith('journey', 'secret.pdf', res); + }); + + it('legacy photo proxy: a bare traversal asset id reads as a miss, not a crash', async () => { + // '..' survives basename(); storage.exists rejects it as an invalid key, + // which the handler reads as a miss. + storageExists.mockRejectedValue(new Error('invalid storage key: journey/..')); + const s = svc({ validateShareTokenForAsset: vi.fn().mockReturnValue({ ownerId: 5 }) } as Partial); + expect(await thrownAsync(() => new JourneyPublicController(s, storageStub).legacyPhoto('tok', 'local', '..', '2', 'original', {} as Response))).toEqual({ status: 404, body: { error: 'Not found' } }); + expect(storageSendToResponse).not.toHaveBeenCalled(); }); });