fix(collab): close the image upload hole and the guard trap

The chat image filter believed the Content-Type the client put on the part.
The stored name keeps the extension of the name the client sent, and the
download route derives what it serves from that extension and sends it inline,
so pwn.html declared as image/png came back as HTML on our own origin. The
filter now checks the extension as well, against the blocked list and against
the types it claims to accept, because a per-route filter fully replaces the
module-level one it looked like it was adding to.

The multipart message route also kept its guard decorators. Guards run before
the interceptor, so a refusal went out while the client was still streaming up
to four images and the socket died as ECONNRESET rather than carrying the
error. The checks moved into the handler, where the other upload route in this
file already does them, and a test pins both refusals.

The three collab:link events were missing from the shared registry, which the
server typecheck refuses, and the admin Links switch could never be saved
because the Zod contract had no key for it. Both are in now. Migrations moved
to the end of the list and the column add uses the pragma guard rather than a
caught duplicate-column error.

The message-required string keeps its old wording for a request without a file
part, so a client from before this route took images sees what it always saw.

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 {

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)', () => {

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<CollabService>), 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<CollabService>), 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<CollabService>);
-      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<CollabService>), 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<CollabService>), 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<CollabService>), 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);
+      }
     });
   });
 

shared/src/admin/admin.schema.ts

@@ -84,6 +84,7 @@ export type AdminAddonUpdateRequest = z.infer<typeof adminAddonUpdateRequestSche
 export const adminCollabFeaturesRequestSchema = z.object({
   chat: z.boolean().optional(),
   notes: z.boolean().optional(),
+  links: z.boolean().optional(),
   polls: z.boolean().optional(),
   whatsnext: z.boolean().optional(),
 });

shared/src/realtime/events.schema.ts

@@ -215,6 +215,9 @@ export const TREK_WS_EVENTS = {
   'collab:note:created': { scope: 'trip', payload: z.object({ note: entity }) },
   'collab:note:updated': { scope: 'trip', payload: z.object({ note: entity }) },
   'collab:note:deleted': { scope: 'trip', payload: z.object({ noteId: id }) },
+  'collab:link:created': { scope: 'trip', payload: z.object({ link: entity }) },
+  'collab:link:updated': { scope: 'trip', payload: z.object({ link: entity }) },
+  'collab:link:deleted': { scope: 'trip', payload: z.object({ linkId: id }) },
   'collab:poll:created': { scope: 'trip', payload: z.object({ poll: entity }) },
   'collab:poll:voted': { scope: 'trip', payload: z.object({ poll: entity }) },
   'collab:poll:closed': { scope: 'trip', payload: z.object({ poll: entity }) },