From 9cebadf67cfa7e771de12e7fcb463332a868984f Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 8 Oct 2026 16:17:57 +0200 Subject: [PATCH] fix(memories): save the Immich connection and auto_upload in one transaction PUT /api/integrations/memories/immich/settings saved the connection through one service call and then wrote auto_upload through a second one from the controller, each committed on its own. A failure between them left a new connection with the old upload choice. saveImmichSettings takes the auto_upload flag and writes it with the connection (or the disconnect) inside one transaction, after the SSRF check that does the DNS lookup. An absent or non-boolean flag still keeps the stored choice, a refused URL still writes nothing, and the response is unchanged. The controller's write sequence leaves the lint:tx baseline, and the MemoriesService facade loses the immichSetAutoUpload pass-through it no longer needs. No MCP tool writes these settings. --- server/scripts/tx-writes-baseline.json | 1 - server/src/nest/memories/immich.controller.ts | 9 ++-- server/src/nest/memories/immich.service.ts | 52 +++++++++++-------- server/src/nest/memories/memories.service.ts | 8 +-- server/tests/e2e/memories.e2e.test.ts | 3 +- server/tests/helpers/mcp-test-controllers.ts | 2 +- server/tests/unit/nest/immich.service.test.ts | 33 +++++++++++- .../unit/nest/memories.controller.test.ts | 18 +++---- .../tests/unit/nest/memories.service.test.ts | 6 +-- 9 files changed, 82 insertions(+), 50 deletions(-) diff --git a/server/scripts/tx-writes-baseline.json b/server/scripts/tx-writes-baseline.json index 2e1bb7a4b..19b3d759f 100644 --- a/server/scripts/tx-writes-baseline.json +++ b/server/scripts/tx-writes-baseline.json @@ -40,7 +40,6 @@ "src/nest/maps/maps.service.ts#MapsService.getPlaceDetails": 2, "src/nest/maps/maps.service.ts#MapsService.getPlaceDetailsExpanded": 2, "src/nest/maps/maps.service.ts#MapsService.getPlacePhoto": 7, - "src/nest/memories/immich.controller.ts#ImmichMemoriesController.putSettings": 2, "src/nest/memories/photo-resolver.service.ts#PhotoResolverService.streamCachedThumbnail": 3, "src/nest/memories/photo-resolver.service.ts#PhotoResolverService.streamPhoto": 2, "src/nest/memories/synology.service.ts#SynologyService._fetchAllSynologyAlbums": 2, diff --git a/server/src/nest/memories/immich.controller.ts b/server/src/nest/memories/immich.controller.ts index 1f62006f8..bc8d4d2bd 100644 --- a/server/src/nest/memories/immich.controller.ts +++ b/server/src/nest/memories/immich.controller.ts @@ -37,15 +37,14 @@ export class ImmichMemoriesController { ): Promise { const { immich_url, immich_api_key, auto_upload, allow_insecure_tls } = body; // Absent stays undefined and leaves the stored choice alone, so an older - // client cannot clear it by saving. - const result = await this.memories.immichSaveSettings(user.id, immich_url, immich_api_key, getClientIp(req), allow_insecure_tls); + // client cannot clear it by saving. auto_upload is written with the + // connection in one transaction, and only when it is a boolean. + const autoUpload = typeof auto_upload === 'boolean' ? auto_upload : undefined; + const result = await this.memories.immichSaveSettings(user.id, immich_url, immich_api_key, getClientIp(req), allow_insecure_tls, autoUpload); if (!result.success) { res.status(400).json({ error: result.error }); return; } - if (typeof auto_upload === 'boolean') { - await this.memories.immichSetAutoUpload(user.id, auto_upload); - } if (result.warning) { res.json({ success: true, warning: result.warning }); return; diff --git a/server/src/nest/memories/immich.service.ts b/server/src/nest/memories/immich.service.ts index 92528f14a..86ecc006b 100644 --- a/server/src/nest/memories/immich.service.ts +++ b/server/src/nest/memories/immich.service.ts @@ -11,6 +11,7 @@ import path from 'node:path'; import { MemoriesAccessService } from './memories-access.service'; import { describeFetchFailure, fail, handleServiceResult, isWithinLocalDayRange, pipeAsset, shiftCalendarDay, sortAssetsByTakenAtDesc, type Selection } from './memories.helpers'; import { Users } from '../../db/entities/Users.entity'; +import { UnitOfWork } from '../database/unit-of-work'; import type { UsersRepository } from '../../db/repositories/Users.repository'; const ALBUM_PAGE_SIZE = 1000; @@ -81,6 +82,7 @@ export class ImmichService { private readonly access: MemoriesAccessService, private readonly storage: StorageService, @InjectRepository(Users) private readonly users: UsersRepository, + private readonly uow: UnitOfWork, ) {} async getImmichCredentials(userId: number): Promise { @@ -138,7 +140,9 @@ export class ImmichService { * `allowInsecureTls` left undefined keeps the stored choice while the URL * stays the same, so a client that does not know the switch cannot clear it * by saving. The switch trusts one server, so a new URL without it starts - * off, and disconnecting (no URL) always turns it off again. + * off, and disconnecting (no URL) always turns it off again. `autoUpload` + * is written with the connection in one transaction, after the URL check; + * left undefined it keeps the stored choice. */ async saveImmichSettings( userId: number, @@ -146,32 +150,38 @@ export class ImmichService { immichApiKey: string | undefined, clientIp: string | null, allowInsecureTls?: boolean, + autoUpload?: boolean, ): Promise<{ success: boolean; warning?: string; error?: string }> { + let ssrf: Awaited> | null = null; + const url = (immichUrl?.endsWith('/') ? immichUrl.slice(0, -1) : immichUrl ?? '').trim(); if (immichUrl) { - if (immichUrl.endsWith('/')) { - immichUrl = immichUrl.slice(0, -1); - } - const ssrf = await checkSsrf(immichUrl.trim()); + ssrf = await checkSsrf(url); if (!ssrf.allowed) { return { success: false, error: `Invalid Immich URL: ${ssrf.error}` }; } - const insecure = allowInsecureTls === undefined ? null : Number(allowInsecureTls); - // The stored URL decides whether an undefined switch keeps its value (IM4). - await this.users.setImmichSettings(userId, immichUrl.trim(), maybe_encrypt_api_key(immichApiKey), insecure); - if (ssrf.isPrivate) { - await this.audit.writeAudit({ - userId, - action: 'immich.private_ip_configured', - ip: clientIp, - details: { immich_url: immichUrl.trim(), resolved_ip: ssrf.resolvedIp }, - }); - return { - success: true, - warning: `Immich URL resolves to a private IP address (${ssrf.resolvedIp}). Make sure this is intentional.`, - }; + } + // The DNS check above is network I/O, so it runs before the transaction opens. + await this.uow.transactional(async () => { + if (immichUrl) { + const insecure = allowInsecureTls === undefined ? null : Number(allowInsecureTls); + // The stored URL decides whether an undefined switch keeps its value (IM4). + await this.users.setImmichSettings(userId, url, maybe_encrypt_api_key(immichApiKey), insecure); + } else { + await this.users.clearImmichSettings(userId, maybe_encrypt_api_key(immichApiKey)); } - } else { - await this.users.clearImmichSettings(userId, maybe_encrypt_api_key(immichApiKey)); + if (typeof autoUpload === 'boolean') await this.setImmichAutoUpload(userId, autoUpload); + }); + if (ssrf?.isPrivate) { + await this.audit.writeAudit({ + userId, + action: 'immich.private_ip_configured', + ip: clientIp, + details: { immich_url: url, resolved_ip: ssrf.resolvedIp }, + }); + return { + success: true, + warning: `Immich URL resolves to a private IP address (${ssrf.resolvedIp}). Make sure this is intentional.`, + }; } return { success: true }; } diff --git a/server/src/nest/memories/memories.service.ts b/server/src/nest/memories/memories.service.ts index 7d90961ef..277c05218 100644 --- a/server/src/nest/memories/memories.service.ts +++ b/server/src/nest/memories/memories.service.ts @@ -67,12 +67,8 @@ export class MemoriesService { return this.immich.getConnectionSettings(userId); } - immichSaveSettings(userId: number, immichUrl: string | undefined, immichApiKey: string | undefined, clientIp: string | null, allowInsecureTls?: boolean) { - return this.immich.saveImmichSettings(userId, immichUrl, immichApiKey, clientIp, allowInsecureTls); - } - - immichSetAutoUpload(userId: number, enabled: boolean): Promise { - return this.immich.setImmichAutoUpload(userId, enabled); + immichSaveSettings(userId: number, immichUrl: string | undefined, immichApiKey: string | undefined, clientIp: string | null, allowInsecureTls?: boolean, autoUpload?: boolean) { + return this.immich.saveImmichSettings(userId, immichUrl, immichApiKey, clientIp, allowInsecureTls, autoUpload); } immichGetConnectionStatus(userId: number) { diff --git a/server/tests/e2e/memories.e2e.test.ts b/server/tests/e2e/memories.e2e.test.ts index 9fbfb0952..d4277abc7 100644 --- a/server/tests/e2e/memories.e2e.test.ts +++ b/server/tests/e2e/memories.e2e.test.ts @@ -206,7 +206,8 @@ describe('Memories e2e (real auth guard + temp SQLite)', () => { const ok = await request(server).put(`${IMMICH}/settings`).set('Cookie', sessionCookie(1)).send({ immich_url: 'https://x', immich_api_key: 'k', auto_upload: true }); expect(ok.status).toBe(200); expect(ok.body).toEqual({ success: true }); - expect(immich.setImmichAutoUpload).toHaveBeenCalledWith(1, true); + expect(immich.saveImmichSettings).toHaveBeenCalledWith(1, 'https://x', 'k', expect.anything(), undefined, true); + expect(immich.setImmichAutoUpload).not.toHaveBeenCalled(); immich.saveImmichSettings.mockResolvedValue({ success: false, error: 'Invalid Immich URL: bad' }); const bad = await request(server).put(`${IMMICH}/settings`).set('Cookie', sessionCookie(1)).send({ immich_url: 'bad' }); diff --git a/server/tests/helpers/mcp-test-controllers.ts b/server/tests/helpers/mcp-test-controllers.ts index 9a3c6e989..7f2f802a8 100644 --- a/server/tests/helpers/mcp-test-controllers.ts +++ b/server/tests/helpers/mcp-test-controllers.ts @@ -403,7 +403,7 @@ export async function createMcpTestRegistry(): Promise { // Plan 3g Task 4 constructor-ripple: MA1/MA2/MA6's journey half. await createTestJourneysRepo(db), await createTestJourneyContributorsRepo(db), await createTestJourneyPhotosRepo(db), ); - const immichService = new ImmichService(new AuditService(auditLogRepo, usersRepo), memoriesAccess, generalStorage, usersRepo); + const immichService = new ImmichService(new AuditService(auditLogRepo, usersRepo), memoriesAccess, generalStorage, usersRepo, await createTestUnitOfWork(db)); const synologyService = new SynologyService(memoriesAccess, notificationsStub(), usersRepo); const trekPhotos = new TrekPhotoRegistrationService(mcpOrm.repo(TrekPhotos), mcpOrm.repo(TripPhotos), await createTestJourneyPhotosRepo(db)); const captureBackfill = new PhotoCaptureBackfillService(new PhotoResolverService(trekPhotos, new ThumbnailService(addonsService, generalStorage, mcpOrm.repo(TrekPhotos)), new TrekPhotoCacheService(mcpOrm.repo(TrekPhotoCacheMeta), generalStorage), new PhotoProviderRegistry([new ImmichPhotoProvider(immichService), new SynologyPhotoProvider(synologyService)]), generalStorage), trekPhotos, generalStorage); diff --git a/server/tests/unit/nest/immich.service.test.ts b/server/tests/unit/nest/immich.service.test.ts index 2a8f4be53..30db60650 100644 --- a/server/tests/unit/nest/immich.service.test.ts +++ b/server/tests/unit/nest/immich.service.test.ts @@ -47,7 +47,7 @@ import type { MemoriesAccessService } from '../../../src/nest/memories/memories- import fs from 'node:fs'; import path from 'node:path'; import { makeStorageFixture } from '../../helpers/storage-fixture'; -import { createTestUsersRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestUsersRepo } from '../../helpers/test-uow'; import { PROVIDER_SELECT_ALL_MAX_PAGES } from '@trek/shared'; const audit = { writeAudit: vi.fn() }; @@ -76,7 +76,7 @@ function upstream(opts: { ok?: boolean; status?: number; json?: unknown; url?: s beforeAll(async () => { const users = await createTestUsersRepo(testDb); - svc = new ImmichService(audit as unknown as AuditService, access as unknown as MemoriesAccessService, journeyFx.storage, users); + svc = new ImmichService(audit as unknown as AuditService, access as unknown as MemoriesAccessService, journeyFx.storage, users, await createTestUnitOfWork(testDb)); }); beforeEach(() => { @@ -176,6 +176,35 @@ describe('saveImmichSettings', () => { expect(await svc.getImmichCredentials(USER)).toBeNull(); }); + it('IMMICH-052: writes auto_upload with the connection, and keeps it when the flag is absent', async () => { + const autoUpload = () => (testDb.prepare('SELECT immich_auto_upload FROM users WHERE id = ?').get(USER) as { immich_auto_upload: number }).immich_auto_upload; + + await svc.saveImmichSettings(USER, 'https://new.test', 'k2', null, undefined, true); + expect(autoUpload()).toBe(1); + await svc.saveImmichSettings(USER, 'https://new.test', 'k2', null); + expect(autoUpload()).toBe(1); + await svc.saveImmichSettings(USER, undefined, undefined, null, undefined, false); + expect(autoUpload()).toBe(0); + }); + + it('IMMICH-053: a refused URL writes no auto_upload either', async () => { + checkSsrf.mockResolvedValue({ allowed: false, error: 'blocked host' }); + + await svc.saveImmichSettings(USER, 'http://169.254.169.254', 'k', null, undefined, true); + + expect(testDb.prepare('SELECT immich_auto_upload FROM users WHERE id = ?').get(USER)).toEqual({ immich_auto_upload: 0 }); + }); + + it('IMMICH-054: a failing auto_upload write rolls the new connection back with it', async () => { + const spy = vi.spyOn(svc, 'setImmichAutoUpload').mockRejectedValueOnce(new Error('boom')); + try { + await expect(svc.saveImmichSettings(USER, 'https://new.test', 'k2', null, undefined, true)).rejects.toThrow('boom'); + expect((await svc.getImmichCredentials(USER))!.immich_url).toBe('https://immich.test'); + } finally { + spy.mockRestore(); + } + }); + // R6 — the encrypt call still runs; this file mocks `maybe_encrypt_api_key` // as identity everywhere else (so cases above can assert on the plaintext // round-trip), so this one case swaps in the REAL crypto for the duration diff --git a/server/tests/unit/nest/memories.controller.test.ts b/server/tests/unit/nest/memories.controller.test.ts index 72f9c3075..cd42dc52f 100644 --- a/server/tests/unit/nest/memories.controller.test.ts +++ b/server/tests/unit/nest/memories.controller.test.ts @@ -236,23 +236,21 @@ describe('ImmichMemoriesController (parity with /api/integrations/memories/immic expect(res.json).toHaveBeenCalledWith({ error: 'Bad URL' }); }); - it('applies auto_upload when it is a boolean and returns success', async () => { + it('hands auto_upload to the one settings save when it is a boolean and returns success', async () => { const immichSaveSettings = vi.fn().mockResolvedValue({ success: true }); - const immichSetAutoUpload = vi.fn(); - const svc = makeService({ immichSaveSettings, immichSetAutoUpload }); + const svc = makeService({ immichSaveSettings }); const res = makeRes(); await new ImmichMemoriesController(svc).putSettings(user, { immich_url: 'x', immich_api_key: 'k', auto_upload: true }, req, res); - expect(immichSaveSettings).toHaveBeenCalledWith(7, 'x', 'k', '1.2.3.4', undefined); - expect(immichSetAutoUpload).toHaveBeenCalledWith(7, true); + expect(immichSaveSettings).toHaveBeenCalledTimes(1); + expect(immichSaveSettings).toHaveBeenCalledWith(7, 'x', 'k', '1.2.3.4', undefined, true); expect(res.json).toHaveBeenCalledWith({ success: true }); }); - it('skips auto_upload when it is not a boolean', async () => { + it('leaves auto_upload undefined when it is not a boolean', async () => { const immichSaveSettings = vi.fn().mockResolvedValue({ success: true }); - const immichSetAutoUpload = vi.fn(); - const svc = makeService({ immichSaveSettings, immichSetAutoUpload }); + const svc = makeService({ immichSaveSettings }); await new ImmichMemoriesController(svc).putSettings(user, { auto_upload: 'yes' as unknown as boolean }, req, makeRes()); - expect(immichSetAutoUpload).not.toHaveBeenCalled(); + expect(immichSaveSettings).toHaveBeenCalledWith(7, undefined, undefined, '1.2.3.4', undefined, undefined); }); it('hands the self-signed switch to the save both ways (#2475)', async () => { @@ -270,7 +268,7 @@ describe('ImmichMemoriesController (parity with /api/integrations/memories/immic const immichSaveSettings = vi.fn().mockResolvedValue({ success: true }); const svc = makeService({ immichSaveSettings }); await new ImmichMemoriesController(svc).putSettings(user, { immich_url: 'x', immich_api_key: 'k' }, req, makeRes()); - expect(immichSaveSettings).toHaveBeenCalledWith(7, 'x', 'k', '1.2.3.4', undefined); + expect(immichSaveSettings).toHaveBeenCalledWith(7, 'x', 'k', '1.2.3.4', undefined, undefined); }); it('returns the warning when the save carries one', async () => { diff --git a/server/tests/unit/nest/memories.service.test.ts b/server/tests/unit/nest/memories.service.test.ts index 4c6517828..35b9ff067 100644 --- a/server/tests/unit/nest/memories.service.test.ts +++ b/server/tests/unit/nest/memories.service.test.ts @@ -121,10 +121,10 @@ describe('MemoriesService (delegation wrapper over services/memories/*)', () => expect(immich.getConnectionSettings).toHaveBeenCalledWith(7); await svc.immichSaveSettings(7, 'u', 'k', '1.2.3.4', true); - expect(immich.saveImmichSettings).toHaveBeenCalledWith(7, 'u', 'k', '1.2.3.4', true); + expect(immich.saveImmichSettings).toHaveBeenCalledWith(7, 'u', 'k', '1.2.3.4', true, undefined); - await svc.immichSetAutoUpload(7, true); - expect(immich.setImmichAutoUpload).toHaveBeenCalledWith(7, true); + await svc.immichSaveSettings(7, 'u', 'k', '1.2.3.4', undefined, false); + expect(immich.saveImmichSettings).toHaveBeenLastCalledWith(7, 'u', 'k', '1.2.3.4', undefined, false); await svc.immichGetConnectionStatus(7); expect(immich.getConnectionStatus).toHaveBeenCalledWith(7);