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' } });