From 94490b7d31f028da8355e2968043c7a2e4cebe9f Mon Sep 17 00:00:00 2001 From: jubnl Date: Sun, 16 Aug 2026 11:16:17 +0200 Subject: [PATCH] fix(share): answer place-photo cache miss with 204 like the maps proxy --- server/src/nest/share/share.controller.ts | 23 +++++++++---- server/tests/integration/share.test.ts | 28 +++++++++------- .../tests/unit/nest/share.controller.test.ts | 33 ++++++++++++------- 3 files changed, 56 insertions(+), 28 deletions(-) diff --git a/server/src/nest/share/share.controller.ts b/server/src/nest/share/share.controller.ts index a8efd9a61..dd5ad41c6 100644 --- a/server/src/nest/share/share.controller.ts +++ b/server/src/nest/share/share.controller.ts @@ -89,14 +89,17 @@ export class SharedController { * image URLs to this route so thumbnails load without a session cookie (the * /api/maps bytes endpoint is JwtAuthGuard'd). The service validates the token * and that the place belongs to its trip; a miss streams nothing and answers - * 404. Declared before the bare ':token' read route. Streaming mirrors - * MapsController.placePhotoBytes (cached photos are always JPEG). + * an empty 204, mirroring MapsController.placePhotoBytes (#1727's rationale + * extended to shared pages: shared payloads keep this URL in place + * image_urls, so evicted cache entries would otherwise 404 once per place + * per render). Declared before the bare ':token' read route. Cached photos + * are always JPEG. */ @Get(':token/place-photo/:placeId/bytes') async placePhotoBytes(@Param('token') token: string, @Param('placeId') placeId: string, @Res() res: Response): Promise { const key = this.share.getSharedPlacePhotoKey(token, placeId); if (!key) { - res.status(404).json({ error: 'Photo not cached' }); + this.emptyPhoto(res); return; } // Bytes are stream-piped like the maps handler; headers go on before the @@ -108,16 +111,24 @@ export class SharedController { try { ({ stream } = await this.storage.getStream('photos-google', key)); } catch { - // Cache-delete race — same terminal state as the old stream error path. - if (!res.headersSent) res.status(404).json({ error: 'Photo not cached' }); + // Cache-delete race — same terminal state as the stream error path. + if (!res.headersSent) this.emptyPhoto(res); return; } stream.on('error', () => { - if (!res.headersSent) res.status(404).json({ error: 'Photo not cached' }); + if (!res.headersSent) this.emptyPhoto(res); }); stream.pipe(res); } + // 204 for "no bytes to serve". Overrides the immutable Cache-Control the hit + // path already set — a photo that reappears in the cache must not stay hidden + // behind a month-old empty response. + private emptyPhoto(res: Response): void { + res.set('Cache-Control', 'no-store'); + res.status(204).end(); + } + @Get(':token') read(@Param('token') token: string) { const data = this.share.getSharedTripData(token); diff --git a/server/tests/integration/share.test.ts b/server/tests/integration/share.test.ts index b8484d8cf..69a993214 100644 --- a/server/tests/integration/share.test.ts +++ b/server/tests/integration/share.test.ts @@ -570,24 +570,30 @@ describe('Shared trip — place photos in shared links (issue #1100)', () => { expect(Buffer.from(res.body)).toEqual(photoBytes); }); - it('SHARE-019 — public proxy 404s for a placeId not in the shared trip', async () => { + // Every miss shape answers the same empty 204 (fix: mirrors the maps proxy, + // #1727 extended to shared pages). The service still collapses "not in + // trip" / "bad token" / "map disabled" into one indistinguishable null, so + // no authorization detail leaks through the status change. + it('SHARE-019 — public proxy answers an empty 204 for a placeId not in the shared trip', async () => { const { token } = await setupSharedPlaceWithPhoto(); const res = await request(app).get(`/api/shared/${token}/place-photo/ChIJnotInTrip/bytes`); - expect(res.status).toBe(404); - expect(res.body).toEqual({ error: 'Photo not cached' }); + expect(res.status).toBe(204); + expect(res.headers['cache-control']).toBe('no-store'); + expect(res.text ?? '').toBe(''); }); - it('SHARE-020 — public proxy 404s for an invalid token', async () => { + it('SHARE-020 — public proxy answers an empty 204 for an invalid token', async () => { await setupSharedPlaceWithPhoto(); const res = await request(app).get(`/api/shared/bad-token/place-photo/${encodeURIComponent(PLACE_ID)}/bytes`); - expect(res.status).toBe(404); - expect(res.body).toEqual({ error: 'Photo not cached' }); + expect(res.status).toBe(204); + expect(res.headers['cache-control']).toBe('no-store'); + expect(res.text ?? '').toBe(''); }); // Regression — GHSA-9hc8 sibling: place photos are part of the map/itinerary, - // so the proxy must 404 when the owner disabled the map, even with a valid - // token + cached bytes. - it('SHARE-025 — public place-photo proxy 404s when share_map=false', async () => { + // so the proxy must stream nothing when the owner disabled the map, even + // with a valid token + cached bytes. + it('SHARE-025 — public place-photo proxy streams nothing when share_map=false', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); const place = createPlace(testDb, trip.id, { name: 'Hidden Photo Place' }); @@ -600,7 +606,7 @@ describe('Shared trip — place photos in shared links (issue #1100)', () => { cachedFilePath = cached.filePath; const res = await request(app).get(`/api/shared/${token}/place-photo/${encodeURIComponent(PLACE_ID)}/bytes`); - expect(res.status).toBe(404); - expect(res.body).toEqual({ error: 'Photo not cached' }); + expect(res.status).toBe(204); + expect(res.text ?? '').toBe(''); }); }); diff --git a/server/tests/unit/nest/share.controller.test.ts b/server/tests/unit/nest/share.controller.test.ts index c867b2621..c1b329a6d 100644 --- a/server/tests/unit/nest/share.controller.test.ts +++ b/server/tests/unit/nest/share.controller.test.ts @@ -89,8 +89,9 @@ describe('SharedController', () => { json: vi.fn(), set: vi.fn(), type: vi.fn(), + end: vi.fn(), }; - return r as unknown as Response & { status: ReturnType; json: ReturnType; set: ReturnType; type: ReturnType }; + return r as unknown as Response & { status: ReturnType; json: ReturnType; set: ReturnType; type: ReturnType; end: ReturnType }; } // Braced body on purpose: mockReset() returns the mock, and a function @@ -105,11 +106,16 @@ describe('SharedController', () => { return new SharedController(svc({ getSharedPlacePhotoKey: vi.fn().mockReturnValue(key) } as Partial), storageStub); } - it('404 without streaming when the photo is not cached for the token', async () => { + // #1727's rationale extended to public share pages: shared payloads keep + // this URL in place image_urls, so an evicted cache entry means one request + // per place per shared-page render — an empty 204 instead of 404 noise. + it('204 without a body when the photo is not cached for the token', async () => { const res = photoRes(); await controller(null).placePhotoBytes('tok', 'p1', res); - expect(res.status).toHaveBeenCalledWith(404); - expect(res.json).toHaveBeenCalledWith({ error: 'Photo not cached' }); + expect(res.status).toHaveBeenCalledWith(204); + expect(res.end).toHaveBeenCalled(); + expect(res.json).not.toHaveBeenCalled(); + expect(res.set).toHaveBeenLastCalledWith('Cache-Control', 'no-store'); expect(getStream).not.toHaveBeenCalled(); }); @@ -124,26 +130,30 @@ describe('SharedController', () => { expect(stream.pipe).toHaveBeenCalledWith(res); }); - it('falls back to 404 when the stream cannot be opened (cache-delete race)', async () => { + it('falls back to an empty 204 when the stream cannot be opened (cache-delete race)', async () => { getStream.mockRejectedValue(new Error('storage object not found')); const res = photoRes(); await controller('abc.jpg').placePhotoBytes('tok', 'p1', res); - expect(res.status).toHaveBeenCalledWith(404); - expect(res.json).toHaveBeenCalledWith({ error: 'Photo not cached' }); + expect(res.status).toHaveBeenCalledWith(204); + expect(res.end).toHaveBeenCalled(); + // The hit path already asked for a month of immutable caching — that + // header must not survive onto the empty answer. + expect(res.set).toHaveBeenLastCalledWith('Cache-Control', 'no-store'); }); - it('falls back to 404 when the stream errors before headers were sent', async () => { + it('falls back to an empty 204 when the stream errors before headers were sent', async () => { let onError: () => void = () => {}; const stream = { on: vi.fn((ev: string, cb: () => void) => { if (ev === 'error') onError = cb; return stream; }), pipe: vi.fn() }; getStream.mockResolvedValue({ stream, stat: { key: 'photos/google/abc.jpg', size: 5, mtimeMs: 0 } }); const res = photoRes(); await controller('abc.jpg').placePhotoBytes('tok', 'p1', res); onError(); - expect(res.status).toHaveBeenCalledWith(404); - expect(res.json).toHaveBeenCalledWith({ error: 'Photo not cached' }); + expect(res.status).toHaveBeenCalledWith(204); + expect(res.end).toHaveBeenCalled(); + expect(res.set).toHaveBeenLastCalledWith('Cache-Control', 'no-store'); }); - it('does not re-send a 404 when the stream errors after headers were flushed', async () => { + it('does not re-send a 204 when the stream errors after headers were flushed', async () => { let onError: () => void = () => {}; const stream = { on: vi.fn((ev: string, cb: () => void) => { if (ev === 'error') onError = cb; return stream; }), pipe: vi.fn() }; getStream.mockResolvedValue({ stream, stat: { key: 'photos/google/abc.jpg', size: 5, mtimeMs: 0 } }); @@ -153,6 +163,7 @@ describe('SharedController', () => { onError(); expect(res.status).not.toHaveBeenCalled(); expect(res.json).not.toHaveBeenCalled(); + expect(res.end).not.toHaveBeenCalled(); }); }); });