From c58d797117f83e995acbddec378389d4440fe22f Mon Sep 17 00:00:00 2001 From: Maurice Date: Mon, 10 Aug 2026 17:04:03 +0200 Subject: [PATCH] Write the 404 Immich streaming only returned MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit streamImmichAsset answered { error, status } when the effective owner had no Immich connection stored, and left the response untouched. All four callers ignored that return, so the client got no status line and waited out its own timeout. Its Synology counterpart has always written through handleServiceResult; this is the same shape. Every other exit already went through pipeAsset, which writes on all paths including its catch, so the method is void all the way down now — and the PhotoProvider interface drops the error return it was carrying purely to describe this defect. Three cases pin it, including the one that matters for shared albums: the connection that gets read is the OWNER's, so a viewer who happens to have Immich configured must not paper over an owner who does not. --- server/src/nest/journey/journey.service.ts | 2 +- server/src/nest/memories/immich.service.ts | 22 +++++++-- server/src/nest/memories/photo-provider.ts | 11 ++--- .../memories/providers/immich.provider.ts | 7 +-- server/tests/unit/nest/immich.service.test.ts | 47 +++++++++++++++++++ server/tests/unit/nest/photo-provider.test.ts | 13 ----- 6 files changed, 74 insertions(+), 28 deletions(-) diff --git a/server/src/nest/journey/journey.service.ts b/server/src/nest/journey/journey.service.ts index e9921b673..7398abcdc 100644 --- a/server/src/nest/journey/journey.service.ts +++ b/server/src/nest/journey/journey.service.ts @@ -112,7 +112,7 @@ export class JourneyService { provider: string, ref: { userId: number; ownerId: number; assetId: string }, kind: 'thumbnail' | 'original', - ): Promise | null { + ): Promise | null { const backend = this.providers.get(provider); if (!backend) return null; return backend.streamAsset(res, ref, kind); diff --git a/server/src/nest/memories/immich.service.ts b/server/src/nest/memories/immich.service.ts index 0f930539b..511d29546 100644 --- a/server/src/nest/memories/immich.service.ts +++ b/server/src/nest/memories/immich.service.ts @@ -6,7 +6,7 @@ import { checkSsrf, safeFetch } from '../../utils/ssrfGuard'; import { AuditService } from '../audit/audit.service'; import { DatabaseService } from '../database/database.service'; import { MemoriesAccessService } from './memories-access.service'; -import { pipeAsset, type Selection } from './memories.helpers'; +import { fail, handleServiceResult, pipeAsset, type Selection } from './memories.helpers'; import { UPLOADS_ROOT } from './uploads-root'; const ALBUM_PAGE_SIZE = 1000; @@ -317,6 +317,19 @@ export class ImmichService { } } + /** + * Proxy one asset to the response. + * + * The credential guard WRITES the 404 rather than returning it. It used to + * answer `{ error, status }` and leave the response untouched, and all four + * callers ignored that return — so a user whose Immich connection had been + * removed got no status line at all and the request hung until the client + * gave up. Its Synology counterpart has always written through + * `handleServiceResult`; this is the same shape. + * + * Every other exit goes through `pipeAsset`, which writes on all paths + * including its catch, so the method is `void` all the way down now. + */ async streamImmichAsset( response: Response, userId: number, @@ -324,10 +337,13 @@ export class ImmichService { kind: 'thumbnail' | 'original', ownerUserId?: number, opts?: { mediaType?: string | null; range?: string }, - ): Promise<{ error?: string; status?: number } | void> { + ): Promise { const effectiveUserId = ownerUserId ?? userId; const creds = this.getImmichCredentials(effectiveUserId); - if (!creds) return { error: 'Not found', status: 404 }; + if (!creds) { + handleServiceResult(response, fail('Not found', 404)); + return; + } const isVideo = opts?.mediaType === 'video'; const headers: Record = { 'x-api-key': creds.immich_api_key }; diff --git a/server/src/nest/memories/photo-provider.ts b/server/src/nest/memories/photo-provider.ts index a6fc80fd8..296fda5d8 100644 --- a/server/src/nest/memories/photo-provider.ts +++ b/server/src/nest/memories/photo-provider.ts @@ -52,13 +52,12 @@ export interface PhotoProvider { /** * Write the asset to the response, headers and status included. * - * The error return is Immich's: it answers `{ error, status }` without - * touching the response when the owner has no credentials stored. Every - * caller has always ignored that — which leaves the request open until the - * client times out — so the shape is carried here rather than quietly - * dropped, and the pre-existing handling is left alone. + * Void, and that is a contract rather than an oversight: a provider that + * cannot serve the asset writes its own error status. Immich used to return + * `{ error, status }` on a missing connection and leave the response + * untouched, which hung the request in every one of its four callers. */ - streamAsset(res: Response, ref: PhotoAssetRef, kind: PhotoAssetKind): Promise; + streamAsset(res: Response, ref: PhotoAssetRef, kind: PhotoAssetKind): Promise; /** Thumbnail bytes for the cache, rather than straight to a response. */ fetchThumbnailBytes(ref: PhotoAssetRef): Promise; diff --git a/server/src/nest/memories/providers/immich.provider.ts b/server/src/nest/memories/providers/immich.provider.ts index b8a4572da..e3a462cad 100644 --- a/server/src/nest/memories/providers/immich.provider.ts +++ b/server/src/nest/memories/providers/immich.provider.ts @@ -20,14 +20,11 @@ export class ImmichPhotoProvider implements PhotoProvider { constructor(private readonly immich: ImmichService) {} - async streamAsset(res: Response, ref: PhotoAssetRef, kind: PhotoAssetKind): Promise { - const result = await this.immich.streamImmichAsset(res, ref.userId, ref.assetId, kind, ref.ownerId, { + streamAsset(res: Response, ref: PhotoAssetRef, kind: PhotoAssetKind): Promise { + return this.immich.streamImmichAsset(res, ref.userId, ref.assetId, kind, ref.ownerId, { mediaType: ref.mediaType, range: ref.range, }); - // The service types both fields optional; the one path that returns at all - // always fills them. Normalised here so the interface can require them. - if (result && result.error) return { error: result.error, status: result.status ?? 500 }; } fetchThumbnailBytes(ref: PhotoAssetRef): Promise { diff --git a/server/tests/unit/nest/immich.service.test.ts b/server/tests/unit/nest/immich.service.test.ts index 5fa94ea9f..4493c5f39 100644 --- a/server/tests/unit/nest/immich.service.test.ts +++ b/server/tests/unit/nest/immich.service.test.ts @@ -397,6 +397,53 @@ describe('fetchImmichThumbnailBytes', () => { }); }); +describe('streamImmichAsset', () => { + function makeRes() { + return { status: vi.fn().mockReturnThis(), json: vi.fn(), set: vi.fn(), end: vi.fn() }; + } + + it('IMMICH-041b: answers 404 when the owner has no connection, instead of leaving the request open', async () => { + // It used to return { error, status } and touch nothing. All four callers + // ignored that return, so the client got no status line at all and waited + // out its own timeout. Synology's counterpart always wrote; this matches. + seedUser(11, null, null); + const res = makeRes(); + + await svc.streamImmichAsset(res as never, USER, 'a1', 'thumbnail', 11); + + expect(res.status).toHaveBeenCalledWith(404); + expect(res.json).toHaveBeenCalledWith({ error: 'Not found' }); + expect(safeFetch).not.toHaveBeenCalled(); + }); + + it('IMMICH-041c: reads the connection of the OWNER, not of the viewer', async () => { + // A shared album is proxied with the owner's key on the viewer's behalf, so + // a viewer who happens to have Immich configured must not paper over an + // owner who does not. + seedUser(USER, 'https://immich.example', 'viewer-key'); + seedUser(12, null, null); + const res = makeRes(); + + await svc.streamImmichAsset(res as never, USER, 'a1', 'original', 12); + + expect(res.status).toHaveBeenCalledWith(404); + expect(safeFetch).not.toHaveBeenCalled(); + }); + + it('IMMICH-041d: proxies with the owner key once a connection exists', async () => { + seedUser(13, 'https://immich.example', 'owner-key'); + safeFetch.mockResolvedValue(upstream({ contentType: 'image/jpeg', body: 'bytes' })); + const res = makeRes(); + + await svc.streamImmichAsset(res as never, USER, 'a1', 'thumbnail', 13); + + expect(safeFetch).toHaveBeenCalledTimes(1); + const [url, init] = safeFetch.mock.calls[0]; + expect(url).toContain('/api/assets/a1/thumbnail'); + expect((init.headers as Record)['x-api-key']).toBe('owner-key'); + }); +}); + describe('collectAlbumSelection', () => { it('IMMICH-042: 404s when the album link does not resolve', async () => { access.getAlbumIdFromLink.mockReturnValue({ success: false }); diff --git a/server/tests/unit/nest/photo-provider.test.ts b/server/tests/unit/nest/photo-provider.test.ts index 280c5c42d..442508a49 100644 --- a/server/tests/unit/nest/photo-provider.test.ts +++ b/server/tests/unit/nest/photo-provider.test.ts @@ -93,19 +93,6 @@ describe('ImmichPhotoProvider', () => { expect(immich.fetchImmichThumbnailBytes).toHaveBeenCalledWith(7, 'asset-1', 42); }); - it('PROV-005: normalises the credential-less answer, which the service types as optional', async () => { - immich.streamImmichAsset.mockResolvedValue({ error: 'Not found', status: 404 }); - expect(await immichProvider.streamAsset(res, ref, 'original')).toEqual({ error: 'Not found', status: 404 }); - - immich.streamImmichAsset.mockResolvedValue({ error: 'Broken' }); - expect(await immichProvider.streamAsset(res, ref, 'original')).toEqual({ error: 'Broken', status: 500 }); - }); - - it('PROV-006: a written response reports nothing', async () => { - immich.streamImmichAsset.mockResolvedValue(undefined); - expect(await immichProvider.streamAsset(res, ref, 'thumbnail')).toBeUndefined(); - }); - it('PROV-007: turns the service\'s {data,error} into a ServiceResult', async () => { immich.getAssetInfo.mockResolvedValue({ data: { id: 'a' } }); expect(await immichProvider.getAssetInfo(ref)).toEqual({ success: true, data: { id: 'a' } });