diff --git a/server/src/nest/trips/trips.controller.ts b/server/src/nest/trips/trips.controller.ts index cae1d961b..27d8c54bd 100644 --- a/server/src/nest/trips/trips.controller.ts +++ b/server/src/nest/trips/trips.controller.ts @@ -20,11 +20,10 @@ import { FileInterceptor } from '@nestjs/platform-express'; import { isDemoWriteBlocked, DEMO_WRITE_ERROR } from '../common/demo-write'; import { RuntimeEnvService } from '../app-config/runtime-env.service'; import type { Request, Response } from 'express'; -import { diskStorage } from 'multer'; +import type { Options } from 'multer'; import path from 'path'; -import fs from 'fs'; -import { v4 as uuidv4 } from 'uuid'; import type { ActiveTripResponse } from '@trek/shared'; +import { StorageService } from '../storage/storage.service'; import type { User } from '../../types'; import { TripsService } from './trips.service'; import { JwtAuthGuard } from '../auth/jwt-auth.guard'; @@ -38,23 +37,17 @@ import { UnsplashService } from '../unsplash/unsplash.service'; import { CalendarService } from '../calendar/calendar.service'; import { TripReadModelService } from '../trip-read-model/trip-read-model.service'; -const MAX_COVER_SIZE = 20 * 1024 * 1024; +export const MAX_COVER_SIZE = 20 * 1024 * 1024; +// Still needed by the Unsplash cover download (a raw-fs writer until the +// caches/downloads slice); the multer destination moved to the storage spool. const coversDir = path.join(__dirname, '../../../uploads/covers'); -const COVER_UPLOAD = { - storage: diskStorage({ - destination: (_req, _file, cb) => { - if (!fs.existsSync(coversDir)) fs.mkdirSync(coversDir, { recursive: true }); - cb(null, coversDir); - }, - filename: (_req, file, cb) => cb(null, `${uuidv4()}${path.extname(file.originalname)}`), - }), - limits: { fileSize: MAX_COVER_SIZE }, - fileFilter: (_req: Request, file: Express.Multer.File, cb: (err: Error | null, accept: boolean) => void) => { - const ext = path.extname(file.originalname).toLowerCase(); - const allowed = ['.jpg', '.jpeg', '.png', '.gif', '.webp']; - if (file.mimetype.startsWith('image/') && !file.mimetype.includes('svg') && allowed.includes(ext)) cb(null, true); - else cb(new Error('Only jpg, png, gif, webp images allowed'), false); - }, +// Consumed by trips.module.ts's MulterModule factory. Quirk preserved on +// purpose: a plain Error without statusCode maps to 500, not 400 (parity). +export const TRIP_COVER_FILE_FILTER: Options['fileFilter'] = (_req, file, cb) => { + const ext = path.extname(file.originalname).toLowerCase(); + const allowed = ['.jpg', '.jpeg', '.png', '.gif', '.webp']; + if (file.mimetype.startsWith('image/') && !file.mimetype.includes('svg') && allowed.includes(ext)) cb(null, true); + else cb(new Error('Only jpg, png, gif, webp images allowed')); }; const toDateStr = (d: Date) => d.toISOString().slice(0, 10); @@ -74,7 +67,7 @@ const addDays = (d: Date, n: number) => { const r = new Date(d); r.setDate(r.get export class TripsController { // calendar last: the hand-wired construction sites in the tests stay positional, // so a new dependency does not touch the ones that never reach the ICS route. - constructor(private readonly trips: TripsService, private readonly audit: AuditService, private readonly env: RuntimeEnvService, private readonly unsplash: UnsplashService, private readonly calendar: CalendarService, private readonly readModel: TripReadModelService) {} + constructor(private readonly trips: TripsService, private readonly audit: AuditService, private readonly env: RuntimeEnvService, private readonly unsplash: UnsplashService, private readonly calendar: CalendarService, private readonly readModel: TripReadModelService, private readonly storage: StorageService) {} @Get() list(@CurrentUser() user: User, @Query('archived') archived?: string) { @@ -194,8 +187,8 @@ export class TripsController { } @Post(':id/cover') - @UseInterceptors(FileInterceptor('cover', COVER_UPLOAD)) - cover(@CurrentUser() user: User, @Param('id') id: string, @UploadedFile() file: Express.Multer.File | undefined) { + @UseInterceptors(FileInterceptor('cover')) + async cover(@CurrentUser() user: User, @Param('id') id: string, @UploadedFile() file: Express.Multer.File | undefined) { if (isDemoWriteBlocked(this.env, user.email)) { throw new HttpException(DEMO_WRITE_ERROR, 403); } @@ -213,6 +206,9 @@ export class TripsController { if (!file) { throw new HttpException({ error: 'No image uploaded' }, 400); } + // Commit the spooled upload to its final storage location (atomic + // same-volume rename) before anything references the final path. + await this.storage.put('covers', file.filename, { tmpPath: file.path }); this.trips.deleteOldCover(trip.cover_image); const coverUrl = `/uploads/covers/${file.filename}`; this.trips.updateCoverImage(id, coverUrl); diff --git a/server/src/nest/trips/trips.module.ts b/server/src/nest/trips/trips.module.ts index 2b678c21b..a849ab1ac 100644 --- a/server/src/nest/trips/trips.module.ts +++ b/server/src/nest/trips/trips.module.ts @@ -27,11 +27,28 @@ import { TripMembersModule } from '../trip-members/trip-members.module'; import { TripReadModelModule } from '../trip-read-model/trip-read-model.module'; import { AddonsModule } from '../addons/addons.module'; import { McpSharedModule } from '../mcp-shared/mcp-shared.module'; +import { MulterModule } from '@nestjs/platform-express'; +import { StorageModule } from '../storage/storage.module'; +import { StorageService } from '../storage/storage.service'; +import { buildStorageUploadOptions } from '../storage/storage-upload.factory'; +import { MAX_COVER_SIZE, TRIP_COVER_FILE_FILTER } from './trips.controller'; /** Trips aggregate root (C1 — Phase 3). Uses exact strangler prefixes so it does * not capture the nested sub-domain mounts (collab, files, ...). */ @Module({ - imports: [McpSharedModule, TodoModule, PackingModule, FilesModule, ReservationsModule, DaysModule, PermissionsModule, AuditModule, BudgetModule, CollabModule, VacayModule, PlacesModule, AuthModule, AppConfigModule, UnsplashModule, RealtimeModule, PluginGuardsModule, AddonsModule, TripMembershipModule, CalendarModule, AccommodationsModule, TripMembersModule, TripReadModelModule], + imports: [ + MulterModule.registerAsync({ + imports: [StorageModule], + inject: [StorageService], + useFactory: (storage: StorageService) => + buildStorageUploadOptions(storage, { + category: 'covers', + maxSize: MAX_COVER_SIZE, + fileFilter: TRIP_COVER_FILE_FILTER, + }), + }), + StorageModule, + McpSharedModule, TodoModule, PackingModule, FilesModule, ReservationsModule, DaysModule, PermissionsModule, AuditModule, BudgetModule, CollabModule, VacayModule, PlacesModule, AuthModule, AppConfigModule, UnsplashModule, RealtimeModule, PluginGuardsModule, AddonsModule, TripMembershipModule, CalendarModule, AccommodationsModule, TripMembersModule, TripReadModelModule], controllers: [TripsController], providers: [TripsService, TripsMcp, TripPromptsMcp, TripsRpc], // Exported for FeedsModule (ICS feeds) and PluginsModule (RPC host injection). diff --git a/server/tests/integration/trips.test.ts b/server/tests/integration/trips.test.ts index 4fd5d5c6e..b878a1eab 100644 --- a/server/tests/integration/trips.test.ts +++ b/server/tests/integration/trips.test.ts @@ -1279,3 +1279,84 @@ describe('Trip bundle', () => { expect(memberView.body.packingItems.map((i: { name: string }) => i.name)).toEqual(['Tent']); }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// Cover upload parity (TRIP-P01…P05) — written BEFORE the storage-upload swap. +// ───────────────────────────────────────────────────────────────────────────── + +describe('Trip cover upload parity', () => { + const fsMod = require('fs') as typeof import('fs'); + const pathMod = require('path') as typeof import('path'); + const FIXTURE_IMG = pathMod.join(__dirname, '../fixtures/small-image.jpg'); + const coversDir = pathMod.join(__dirname, '../../uploads/covers'); + + afterAll(() => { + fsMod.rmSync(coversDir, { recursive: true, force: true }); + }); + + it('TRIP-P01 — cover upload returns /uploads/covers/ and writes the file', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + + const res = await request(app) + .post(`/api/trips/${trip.id}/cover`) + .set('Cookie', authCookie(user.id)) + .attach('cover', FIXTURE_IMG, 'cover.png'); + expect(res.status).toBe(201); + expect(res.body.cover_image).toMatch(/^\/uploads\/covers\/[0-9a-f-]{36}\.png$/); + const diskName = res.body.cover_image.replace('/uploads/covers/', ''); + expect(fsMod.existsSync(pathMod.join(coversDir, diskName))).toBe(true); + }); + + it('TRIP-P02 — re-upload deletes the previous cover file', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + + const first = await request(app) + .post(`/api/trips/${trip.id}/cover`) + .set('Cookie', authCookie(user.id)) + .attach('cover', FIXTURE_IMG, 'one.jpg'); + const firstName = first.body.cover_image.replace('/uploads/covers/', ''); + expect(fsMod.existsSync(pathMod.join(coversDir, firstName))).toBe(true); + + const second = await request(app) + .post(`/api/trips/${trip.id}/cover`) + .set('Cookie', authCookie(user.id)) + .attach('cover', FIXTURE_IMG, 'two.jpg'); + expect(second.status).toBe(201); + expect(fsMod.existsSync(pathMod.join(coversDir, firstName))).toBe(false); + }); + + it('TRIP-P03 — no file → 400 "No image uploaded"', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + + const res = await request(app) + .post(`/api/trips/${trip.id}/cover`) + .set('Cookie', authCookie(user.id)); + expect(res.status).toBe(400); + expect(res.body.error).toBe('No image uploaded'); + }); + + it('TRIP-P04 — non-image upload is 500 (plain-Error filter quirk — pinned, do not "fix")', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + + const res = await request(app) + .post(`/api/trips/${trip.id}/cover`) + .set('Cookie', authCookie(user.id)) + .attach('cover', Buffer.from('plain text'), { filename: 'doc.txt', contentType: 'text/plain' }); + expect(res.status).toBe(500); + }); + + it('TRIP-P05 — unknown trip → 404 "Trip not found"', async () => { + const { user } = createUser(testDb); + + const res = await request(app) + .post('/api/trips/999999/cover') + .set('Cookie', authCookie(user.id)) + .attach('cover', FIXTURE_IMG); + expect(res.status).toBe(404); + expect(res.body.error).toBe('Trip not found'); + }); +}); diff --git a/server/tests/unit/nest/trips.controller.test.ts b/server/tests/unit/nest/trips.controller.test.ts index 1b09a4fac..a26a8c935 100644 --- a/server/tests/unit/nest/trips.controller.test.ts +++ b/server/tests/unit/nest/trips.controller.test.ts @@ -2,9 +2,6 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import type { UnsplashService } from '../../../src/nest/unsplash/unsplash.service'; import { RuntimeEnvService } from '../../../src/nest/app-config/runtime-env.service'; import { HttpException } from '@nestjs/common'; -import { INTERCEPTORS_METADATA } from '@nestjs/common/constants'; -import fs from 'fs'; -import path from 'path'; import type { Request } from 'express'; vi.mock('../../../src/nest/audit/client-ip', () => ({ getClientIp: vi.fn(() => '1.2.3.4') })); @@ -19,7 +16,7 @@ const unsplashStub = { saveUnsplashCover: vi.fn().mockResolvedValue('mock-cover.jpg'), } as unknown as UnsplashService; -import { TripsController } from '../../../src/nest/trips/trips.controller'; +import { TripsController, MAX_COVER_SIZE, TRIP_COVER_FILE_FILTER } from '../../../src/nest/trips/trips.controller'; import type { AuditService } from '../../../src/nest/audit/audit.service'; import type { TripsService } from '../../../src/nest/trips/trips.service'; import type { CalendarService } from '../../../src/nest/calendar/calendar.service'; @@ -39,8 +36,9 @@ const audit = { writeAudit } as unknown as AuditService; // they were; the two that do pass a stub. // calendar and readModel are optional so the call sites that reach neither the // ICS route nor the bundle stay as they were; the ones that do pass a stub. +const storageStub = { put: vi.fn().mockResolvedValue(undefined) } as unknown as import('../../../src/nest/storage/storage.service').StorageService; const tc = (s: TripsService, calendar?: Partial, readModel?: Partial) => - new TripsController(s, audit, new RuntimeEnvService(), unsplashStub, (calendar ?? {}) as CalendarService, (readModel ?? {}) as TripReadModelService); + new TripsController(s, audit, new RuntimeEnvService(), unsplashStub, (calendar ?? {}) as CalendarService, (readModel ?? {}) as TripReadModelService, storageStub); function svc(o: Partial = {}): TripsService { return { @@ -307,24 +305,25 @@ describe('TripsController (parity with the legacy /api/trips route)', () => { describe('POST /:id/cover', () => { const file = { filename: 'abc.jpg' } as Express.Multer.File; - it('404 without access, 403 without permission, 404 raw trip, 400 no file, else returns url', () => { - expect(thrown(() => tc(svc({ canAccessTrip: vi.fn().mockReturnValue(undefined) })).cover(user, '9', file))).toEqual({ status: 404, body: { error: 'Trip not found' } }); - expect(thrown(() => tc(svc({ can: vi.fn().mockReturnValue(false) })).cover(user, '9', file))).toEqual({ status: 403, body: { error: 'No permission to change the cover image' } }); - expect(thrown(() => tc(svc({ getRaw: vi.fn().mockReturnValue(undefined) } as Partial)).cover(user, '9', file))).toEqual({ status: 404, body: { error: 'Trip not found' } }); - expect(thrown(() => tc(svc({ getRaw: vi.fn().mockReturnValue({ cover_image: null }) } as Partial)).cover(user, '9', undefined))).toEqual({ status: 400, body: { error: 'No image uploaded' } }); + it('404 without access, 403 without permission, 404 raw trip, 400 no file, else commits + returns url', async () => { + expect(await thrownAsync(() => tc(svc({ canAccessTrip: vi.fn().mockReturnValue(undefined) })).cover(user, '9', file))).toEqual({ status: 404, body: { error: 'Trip not found' } }); + expect(await thrownAsync(() => tc(svc({ can: vi.fn().mockReturnValue(false) })).cover(user, '9', file))).toEqual({ status: 403, body: { error: 'No permission to change the cover image' } }); + expect(await thrownAsync(() => tc(svc({ getRaw: vi.fn().mockReturnValue(undefined) } as Partial)).cover(user, '9', file))).toEqual({ status: 404, body: { error: 'Trip not found' } }); + expect(await thrownAsync(() => tc(svc({ getRaw: vi.fn().mockReturnValue({ cover_image: null }) } as Partial)).cover(user, '9', undefined))).toEqual({ status: 400, body: { error: 'No image uploaded' } }); const deleteOldCover = vi.fn(); const updateCoverImage = vi.fn(); const s = svc({ getRaw: vi.fn().mockReturnValue({ cover_image: '/old.jpg' }), deleteOldCover, updateCoverImage } as Partial); - expect(tc(s).cover(user, '9', file)).toEqual({ cover_image: '/uploads/covers/abc.jpg' }); + expect(await tc(s).cover(user, '9', file)).toEqual({ cover_image: '/uploads/covers/abc.jpg' }); + expect(storageStub.put).toHaveBeenCalledWith('covers', 'abc.jpg', { tmpPath: undefined }); expect(deleteOldCover).toHaveBeenCalledWith('/old.jpg'); expect(updateCoverImage).toHaveBeenCalledWith('9', '/uploads/covers/abc.jpg'); }); - it('403 in demo mode for a demo account', () => { + it('403 in demo mode for a demo account', async () => { const prev = process.env.DEMO_MODE; process.env.DEMO_MODE = 'true'; isDemoEmail.mockReturnValueOnce(true); try { - expect(thrown(() => tc(svc()).cover(user, '9', file))).toEqual({ + expect(await thrownAsync(() => tc(svc()).cover(user, '9', file))).toEqual({ status: 403, body: { error: 'Uploads are disabled in demo mode. Self-host TREK for full functionality.' }, }); } finally { @@ -334,69 +333,15 @@ describe('TripsController (parity with the legacy /api/trips route)', () => { }); /** - * The multer options are a module-private const wired into the route through - * @UseInterceptors, so the handler tests above never reach them: a broken - * destination/filename/filter would still let every case in this file pass. - * Pulling the interceptor back off the route and reading the multer instance - * it built (multer keeps storage, limits and fileFilter verbatim) exercises - * the real callbacks instead of asserting an object literal. + * The multer options moved into trips.module.ts's MulterModule factory + * (storage engine + spool destination are covered by the storage-upload + * factory unit suite and TRIP-P01/P02 integration parity); the trips-owned + * piece that remains testable here is the exported cover fileFilter. */ - type StorageCb = (err: Error | null, value: string) => void; - type CoverUpload = { - storage: { - getDestination: (req: unknown, file: Express.Multer.File, cb: StorageCb) => void; - getFilename: (req: unknown, file: Express.Multer.File, cb: StorageCb) => void; - }; - limits: { fileSize: number }; - fileFilter: (req: unknown, file: Express.Multer.File, cb: (err: Error | null, accept: boolean) => void) => void; - }; - function coverUpload(): CoverUpload { - const [Interceptor] = Reflect.getMetadata(INTERCEPTORS_METADATA, TripsController.prototype.cover) as Array { multer: CoverUpload }>; - return new Interceptor().multer; - } - const coversDir = path.join('uploads', 'covers'); - - it('creates uploads/covers on the first upload and reuses it afterwards', () => { - // A fresh container has no uploads/covers, and multer does not create the - // destination itself, so without the mkdir the very first cover upload on - // a new instance fails with ENOENT. - const exists = vi.spyOn(fs, 'existsSync').mockReturnValue(false); - const mkdir = vi.spyOn(fs, 'mkdirSync').mockImplementation(() => undefined); - try { - const { getDestination } = coverUpload().storage; - const cb = vi.fn(); - getDestination(req, file, cb); - expect(mkdir).toHaveBeenCalledWith(expect.stringContaining(coversDir), { recursive: true }); - expect(cb).toHaveBeenCalledWith(null, expect.stringContaining(coversDir)); - exists.mockReturnValue(true); - mkdir.mockClear(); - getDestination(req, file, cb); - expect(mkdir).not.toHaveBeenCalled(); - expect(cb).toHaveBeenLastCalledWith(null, expect.stringContaining(coversDir)); - } finally { - exists.mockRestore(); - mkdir.mockRestore(); - } - }); - - it('gives every stored cover a fresh name and keeps the original extension', () => { - const { getFilename } = coverUpload().storage; - const cb = vi.fn(); - const upload = { originalname: 'sunset.JPG' } as Express.Multer.File; - getFilename(req, upload, cb); - getFilename(req, upload, cb); - const [first, second] = cb.mock.calls.map((c) => c[1] as string); - // Keeping the client's name would let one trip's upload overwrite another's; - // dropping the extension leaves the static handler with no mime to serve. - expect(first).toMatch(/^[0-9a-f-]{36}\.JPG$/); - expect(second).not.toBe(first); - }); - it('accepts the four allowed image types and rejects svg, a faked extension and a non-image mime', () => { - const { fileFilter, limits } = coverUpload(); const verdict = (originalname: string, mimetype: string) => { const cb = vi.fn(); - fileFilter(req, { originalname, mimetype } as Express.Multer.File, cb); + TRIP_COVER_FILE_FILTER!(req, { originalname, mimetype } as Express.Multer.File, cb); return cb.mock.calls[0]; }; expect(verdict('a.jpg', 'image/jpeg')).toEqual([null, true]); @@ -404,11 +349,14 @@ describe('TripsController (parity with the legacy /api/trips route)', () => { expect(verdict('a.WEBP', 'image/webp')).toEqual([null, true]); // svg passes the image/* prefix but runs script when the browser opens the // stored cover, so it is rejected even under an allowed extension. - expect(verdict('logo.png', 'image/svg+xml')).toEqual([expect.any(Error), false]); + expect(verdict('logo.png', 'image/svg+xml')).toEqual([expect.any(Error)]); // A binary renamed to an image mime must still fail on the extension. - expect(verdict('payload.exe', 'image/png')).toEqual([expect.any(Error), false]); - expect(verdict('a.png', 'application/octet-stream')).toEqual([expect.any(Error), false]); - expect(limits.fileSize).toBe(20 * 1024 * 1024); + expect(verdict('payload.exe', 'image/png')).toEqual([expect.any(Error)]); + expect(verdict('a.png', 'application/octet-stream')).toEqual([expect.any(Error)]); + // The quirk stays quirky: the rejection error carries NO statusCode, so + // the route answers 500 (pinned by TRIP-P04), not 400. + expect((verdict('logo.png', 'image/svg+xml')[0] as Error & { statusCode?: number }).statusCode).toBeUndefined(); + expect(MAX_COVER_SIZE).toBe(20 * 1024 * 1024); }); });