diff --git a/server/src/nest/collab/collab.controller.ts b/server/src/nest/collab/collab.controller.ts index ef33e6930..4d273d07c 100644 --- a/server/src/nest/collab/collab.controller.ts +++ b/server/src/nest/collab/collab.controller.ts @@ -40,11 +40,30 @@ import { BLOCKED_EXTENSIONS } from '../files/files.constants'; export const MAX_NOTE_FILE_SIZE = 50 * 1024 * 1024; const MAX_CHAT_IMAGES = 4; const CHAT_IMAGE_TYPES = new Set(['image/jpeg', 'image/png', 'image/gif', 'image/webp']); +const CHAT_IMAGE_EXTENSIONS = new Set(['.jpg', '.jpeg', '.png', '.gif', '.webp']); +/** + * The extension is checked as well as the type, and both have to agree. + * + * `file.mimetype` is the Content-Type the client put on the part, so it is a + * claim, not a fact. The stored name keeps the extension of the name the client + * sent (`storage-upload.factory.ts`), and the download route derives the served + * Content-Type from that extension and sends it inline. Believing the header + * alone therefore lets `pwn.html` through as `image/png` and serves it back as + * HTML on our own origin. This filter also fully replaces the module-level one + * on this route, so the blocked-extension list has to be applied here too. + */ export const collabChatImageFilter: Options['fileFilter'] = (_req, file, cb) => { - if (!CHAT_IMAGE_TYPES.has(file.mimetype)) { - const err: Error & { statusCode?: number } = new Error('Only JPEG, PNG, GIF, and WebP images are allowed'); + const reject = (message: string) => { + const err: Error & { statusCode?: number } = new Error(message); err.statusCode = 400; return cb(err); + }; + if (!CHAT_IMAGE_TYPES.has(file.mimetype)) { + return reject('Only JPEG, PNG, GIF, and WebP images are allowed'); + } + const ext = path.extname(file.originalname).toLowerCase(); + if (BLOCKED_EXTENSIONS.includes(ext) || !CHAT_IMAGE_EXTENSIONS.has(ext)) { + return reject('Only JPEG, PNG, GIF, and WebP images are allowed'); } cb(null, true); }; @@ -306,8 +325,10 @@ export class CollabController { return { messages: this.collab.listMessages(tripId, before) }; } - @UseGuards(TripAccessGuard) - @RequirePermission('collab_edit') + // No guard decorators, deliberately, like every other upload route in this + // file: guards run before the interceptor, so a 403 or 404 goes out while the + // client is still streaming the body and the socket dies as ECONNRESET + // instead of carrying the error envelope. Both checks happen in the handler. @Post('messages') @UseInterceptors(FilesInterceptor('images', MAX_CHAT_IMAGES, { fileFilter: collabChatImageFilter, limits: { files: MAX_CHAT_IMAGES, fileSize: 10 * 1024 * 1024 } })) async createMessage(@CurrentUser() user: User, @Param('tripId') tripId: string, @Body() body: CollabMessageCreateDto, @UploadedFiles() files: Express.Multer.File[] | undefined, @Headers('x-socket-id') socketId?: string) { @@ -317,7 +338,14 @@ export class CollabController { cleanupSpool(); throw new HttpException({ error: 'text must be 5000 characters or less' }, 400); } - const trip = this.requireTrip(tripId, user); + let trip; + try { + trip = this.requireTrip(tripId, user); + this.requireEdit(trip, user); + } catch (err) { + cleanupSpool(); + throw err; + } if (uploaded.length && !this.collab.canUploadFiles(trip, user)) { cleanupSpool(); throw new HttpException({ error: 'No permission to upload files' }, 403); @@ -325,7 +353,12 @@ export class CollabController { const text = (body.text || '').trim(); if (!text && !uploaded.length) { cleanupSpool(); - throw new HttpException({ error: 'Message text or image is required' }, 400); + // The old wording is kept for a request that carried no file part at all, + // because that is exactly what a client from before this route took + // images sends, and parity is on the bespoke strings too. A multipart + // request gets the wording that actually describes its options. + const wasMultipart = files !== undefined; + throw new HttpException({ error: wasMultipart ? 'Message text or image is required' : 'Message text is required' }, 400); } const committed: string[] = []; try { diff --git a/server/tests/unit/nest/addons.service.test.ts b/server/tests/unit/nest/addons.service.test.ts index 9bb1aa324..87e3809bb 100644 --- a/server/tests/unit/nest/addons.service.test.ts +++ b/server/tests/unit/nest/addons.service.test.ts @@ -78,7 +78,7 @@ describe('AddonsService.list', () => { feedReads([], [], [], [{ key: 'collab_chat_enabled', value: 'false' }], { value: 'true' }); const res = svc().list(); - expect(res.collabFeatures).toEqual({ chat: false, notes: true, polls: true, whatsnext: true }); + expect(res.collabFeatures).toEqual({ chat: false, notes: true, links: true, polls: true, whatsnext: true }); expect(res.bagTracking).toBe(true); expect(res.addons).toEqual([]); }); @@ -311,7 +311,7 @@ describe('AddonsService addon/feature flags', () => { { key: 'collab_chat_enabled', value: 'false' }, { key: 'collab_polls_enabled', value: 'true' }, ]); - expect(svc().getCollabFeatures()).toEqual({ chat: false, notes: true, polls: true, whatsnext: true }); + expect(svc().getCollabFeatures()).toEqual({ chat: false, notes: true, links: true, polls: true, whatsnext: true }); }); it('updateCollabFeatures writes only the provided flags and reports changed (#1414, ADMIN-SVC-070)', () => { diff --git a/server/tests/unit/nest/collab.controller.test.ts b/server/tests/unit/nest/collab.controller.test.ts index 89f7693bc..63e418d76 100644 --- a/server/tests/unit/nest/collab.controller.test.ts +++ b/server/tests/unit/nest/collab.controller.test.ts @@ -154,17 +154,33 @@ describe('CollabController (parity with the legacy /api/trips/:tripId/collab rou }); describe('messages', () => { - it('POST 400 whitespace-only, 400 reply_not_found, else creates + notifies (length checks now in the Zod pipe)', () => { - expect(thrown(() => new CollabController(svc(), storageStub).createMessage(user, '5', { text: ' ' }))).toEqual({ status: 400, body: { error: 'Message text is required' } }); - expect(thrown(() => new CollabController(svc({ createMessage: vi.fn().mockReturnValue({ error: 'reply_not_found' }) } as Partial), storageStub).createMessage(user, '5', { text: 'hi', reply_to: 99 }))).toEqual({ status: 400, body: { error: 'Reply target message not found' } }); + // Async since the route took its multipart form: it awaits the storage + // commit, so every case here goes through thrownAsync. + it('POST 400 whitespace-only, 400 reply_not_found, else creates + notifies (length checks now in the Zod pipe)', async () => { + // A request with no file part keeps the wording it always had. + expect(await thrownAsync(() => new CollabController(svc(), storageStub).createMessage(user, '5', { text: ' ' }, undefined))) + .toEqual({ status: 400, body: { error: 'Message text is required' } }); + expect(await thrownAsync(() => new CollabController(svc(), storageStub).createMessage(user, '5', { text: ' ' }, []))) + .toEqual({ status: 400, body: { error: 'Message text or image is required' } }); + expect(await thrownAsync(() => new CollabController(svc({ createMessage: vi.fn().mockReturnValue({ error: 'reply_not_found' }) } as Partial), storageStub).createMessage(user, '5', { text: 'hi', reply_to: 99 }, undefined))) + .toEqual({ status: 400, body: { error: 'Reply target message not found' } }); const broadcast = vi.fn(); const notifyCollab = vi.fn(); const s = svc({ createMessage: vi.fn().mockReturnValue({ message: { id: 3 } }), broadcast, notifyCollab } as Partial); - expect(new CollabController(s, storageStub).createMessage(user, '5', { text: 'hello' }, 'sock')).toEqual({ message: { id: 3 } }); + expect(await new CollabController(s, storageStub).createMessage(user, '5', { text: 'hello' }, undefined, 'sock')).toEqual({ message: { id: 3 } }); expect(broadcast).toHaveBeenCalledWith('5', 'collab:message:created', { message: { id: 3 } }, 'sock'); expect(notifyCollab).toHaveBeenCalledWith('5', user, 'hello'); }); + it('refuses a caller who cannot reach the trip or cannot write, from inside the handler', async () => { + // The decorators had to go because of the multipart body, so these two + // refusals are the only thing standing in front of the route now. + expect(await thrownAsync(() => new CollabController(svc({ verifyTripAccess: vi.fn().mockReturnValue(null) } as Partial), storageStub).createMessage(user, '5', { text: 'hi' }, undefined))) + .toEqual({ status: 404, body: { error: 'Trip not found' } }); + expect(await thrownAsync(() => new CollabController(svc({ canEdit: vi.fn().mockReturnValue(false) } as Partial), storageStub).createMessage(user, '5', { text: 'hi' }, undefined))) + .toEqual({ status: 403, body: { error: 'No permission' } }); + }); + it('react 404 unknown, else broadcasts reactions (empty emoji now 400s in the Zod pipe)', () => { expect(thrown(() => new CollabController(svc({ reactMessage: vi.fn().mockReturnValue({ found: false, reactions: [] }) } as Partial), storageStub).react(user, '5', '3', { emoji: '👍' }))).toEqual({ status: 404, body: { error: 'Message not found' } }); const broadcast = vi.fn(); @@ -205,7 +221,19 @@ describe('CollabController (parity with the legacy /api/trips/:tripId/collab rou expect(Reflect.getMetadata(TRIP_PERMISSION_KEY, handler)).toBeUndefined(); } // The write siblings do demand it, so this is a deliberate split, not an omission. - expect(Reflect.getMetadata(TRIP_PERMISSION_KEY, CollabController.prototype.createMessage)).toBe('collab_edit'); + expect(Reflect.getMetadata(TRIP_PERMISSION_KEY, CollabController.prototype.createNote)).toBe('collab_edit'); + }); + + it('the multipart write routes carry no guard decorators, and check inside instead', () => { + // Guards run before the interceptor, so a refusal goes out while the client + // is still streaming the body and the socket dies as ECONNRESET rather than + // carrying the error envelope. Both of these therefore look unguarded here + // and do requireTrip plus the permission check in the handler; the test + // below proves createMessage actually refuses. + for (const handler of [CollabController.prototype.createMessage, CollabController.prototype.addNoteFile]) { + expect(Reflect.getMetadata(TRIP_PERMISSION_KEY, handler)).toBeUndefined(); + expect(guardsOn(handler)).not.toContain(TripAccessGuard); + } }); }); diff --git a/shared/src/admin/admin.schema.ts b/shared/src/admin/admin.schema.ts index 737ba7bbc..fb6c2d9e1 100644 --- a/shared/src/admin/admin.schema.ts +++ b/shared/src/admin/admin.schema.ts @@ -84,6 +84,7 @@ export type AdminAddonUpdateRequest = z.infer