fix(server): fix the quirks preserved by the collab DI migration

server/src/nest/collab/collab.controller.ts

@@ -66,7 +66,9 @@ const NOTE_UPLOAD = {
  * (400 with the standard `{ error }` envelope on mismatch — this replaced the
  * legacy bespoke 'Title is required' / 'Question is required' / '... 2 options
  * ...' / 5000-char / 'Emoji is required' checks; the whitespace-only 'Message
- * text is required' check stays, since min(1) doesn't trim).
+ * text is required' check stays, since min(1) doesn't trim). One deliberate
+ * deviation from the legacy route: link-preview now verifies trip access (404)
+ * like every sibling handler.
  */
 @Controller('api/trips/:tripId/collab')
 @UseGuards(JwtAuthGuard)
@@ -154,7 +156,7 @@ export class CollabController {
     if (!result) {
       throw new HttpException({ error: 'Note not found' }, 404);
     }
-    this.collab.broadcast(tripId, 'collab:note:updated', { note: this.collab.getFormattedNoteById(id) }, socketId);
+    this.collab.broadcast(tripId, 'collab:note:updated', { note: this.collab.getFormattedNoteById(tripId, id) }, socketId);
     return result;
   }
 
@@ -165,7 +167,7 @@ export class CollabController {
     if (!this.collab.deleteNoteFile(tripId, id, fileId)) {
       throw new HttpException({ error: 'File not found' }, 404);
     }
-    this.collab.broadcast(tripId, 'collab:note:updated', { note: this.collab.getFormattedNoteById(id) }, socketId);
+    this.collab.broadcast(tripId, 'collab:note:updated', { note: this.collab.getFormattedNoteById(tripId, id) }, socketId);
     return { success: true };
   }
 
@@ -281,8 +283,9 @@ export class CollabController {
   // ── Link preview ──────────────────────────────────────────────────────────
   @Get('link-preview')
   async linkPreview(@CurrentUser() user: User, @Param('tripId') tripId: string, @Query('url') url?: string) {
-    // NB: the legacy route does not verify trip access on link-preview; kept 1:1.
-    void user; void tripId;
+    // Unlike the legacy route, this verifies trip access — any authed user
+    // could otherwise drive the SSRF-guarded fetcher through arbitrary trip URLs.
+    this.requireTrip(tripId, user);
     if (!url) {
       throw new HttpException({ error: 'URL is required' }, 400);
     }

server/src/nest/collab/collab.mcp.ts

@@ -53,8 +53,9 @@ function jsonContent(uri: string, data: unknown) {
  * and broadcasts). The registration-time gates map to the composite `when`
  * thunks (collab addon AND per-sub-feature flag) plus the declarative collab
  * read/write access markers (the legacy `if (R)` / `if (W)` checks, resolved
- * by trekMcpAccessPolicy). Parity quirks kept on purpose: vote_collab_poll has
- * no demo-user gate, and the list tools check only trip access.
+ * by trekMcpAccessPolicy). The list tools check only trip access (as legacy);
+ * vote_collab_poll gained the demo-user gate the legacy registrar was missing,
+ * matching every other collab write tool.
  */
 @McpController()
 export class CollabMcp {
@@ -202,6 +203,7 @@ export class CollabMcp {
     access: { group: 'collab', mode: 'write' },
   })
   async voteCollabPoll({ tripId, pollId, optionIndex }: { tripId: number; pollId: number; optionIndex: number }, ctx: McpContext) {
+    if (isDemoUser(ctx.userId)) return demoDenied();
     if (!this.collab.verifyTripAccess(tripId, ctx.userId)) return noAccess();
     if (!hasTripPermission('collab_edit', tripId, ctx.userId)) return permissionDenied();
     const result = this.collab.votePoll(tripId, pollId, ctx.userId, optionIndex);

server/src/nest/collab/collab.service.ts

@@ -48,11 +48,15 @@ export interface LinkPreviewResult {
 }
 
 /**
- * Collab domain service — owns the collab SQL (moved 1:1 from the legacy
- * services/collabService.ts: identical statements, the `||` falsy-coercion
- * defaults, the mixed COALESCE/CASE update, the post-write re-selects and the
- * sentinel error strings). Trip access, the 'collab_edit' / 'file_upload'
- * permissions and the WebSocket broadcast keep their legacy call paths.
+ * Collab domain service — owns the collab SQL (moved from the legacy
+ * services/collabService.ts: the `||` falsy-coercion defaults, the mixed
+ * COALESCE/CASE update, the post-write re-selects and the sentinel error
+ * strings). Trip access, the 'collab_edit' / 'file_upload' permissions and the
+ * WebSocket broadcast keep their legacy call paths. Post-migration hardening
+ * on top of the 1:1 move: the multi-statement writes (deleteNote, votePoll)
+ * run in db.transaction(), getFormattedNoteById is trip-scoped and null-safe,
+ * votePoll rejects non-integer indexes, and linkPreview absorbs malformed URLs
+ * instead of throwing.
  * Non-Nest consumers (legacy tripService, the legacy MCP trips registrar) go
  * through collab.bridge.ts instead of importing this class directly.
  */
@@ -186,15 +190,17 @@ export class CollabService {
     const existing = this.db.get('SELECT id FROM collab_notes WHERE id = ? AND trip_id = ?', noteId, tripId);
     if (!existing) return false;
 
-    // Clean up attached files from disk
+    // Clean up attached files from disk (unlink-first is intentional — a
+    // failed row delete leaves dangling rows, never orphaned files).
     const noteFiles = this.db.all<NoteFileRow>('SELECT id, filename FROM trip_files WHERE note_id = ?', noteId);
     for (const f of noteFiles) {
       const filePath = path.join(__dirname, '../../../uploads', f.filename);
       try { fs.unlinkSync(filePath); } catch { /* ignore */ }
     }
-    this.db.run('DELETE FROM trip_files WHERE note_id = ?', noteId);
-
-    this.db.run('DELETE FROM collab_notes WHERE id = ?', noteId);
+    this.db.transaction(() => {
+      this.db.run('DELETE FROM trip_files WHERE note_id = ?', noteId);
+      this.db.run('DELETE FROM collab_notes WHERE id = ?', noteId);
+    });
     return true;
   }
 
@@ -215,8 +221,9 @@ export class CollabService {
     return { file: { ...saved, url: `/api/trips/${tripId}/files/${saved.id}/download` } };
   }
 
-  getFormattedNoteById(noteId: string | number) {
-    const note = this.db.get<CollabNote>('SELECT n.*, u.username, u.avatar FROM collab_notes n JOIN users u ON n.user_id = u.id WHERE n.id = ?', noteId)!;
+  getFormattedNoteById(tripId: string | number, noteId: string | number) {
+    const note = this.db.get<CollabNote>('SELECT n.*, u.username, u.avatar FROM collab_notes n JOIN users u ON n.user_id = u.id WHERE n.id = ? AND n.trip_id = ?', noteId, tripId);
+    if (!note) return null;
     return this.formatNote(note);
   }
 
@@ -304,7 +311,7 @@ export class CollabService {
     if (poll.closed) return { error: 'closed' };
 
     const options = JSON.parse(poll.options);
-    if (optionIndex < 0 || optionIndex >= options.length) {
+    if (!Number.isInteger(optionIndex) || optionIndex < 0 || optionIndex >= options.length) {
       return { error: 'invalid_index' };
     }
 
@@ -316,10 +323,12 @@ export class CollabService {
     if (existingVote) {
       this.db.run('DELETE FROM collab_poll_votes WHERE id = ?', existingVote.id);
     } else {
-      if (!poll.multiple) {
-        this.db.run('DELETE FROM collab_poll_votes WHERE poll_id = ? AND user_id = ?', pollId, userId);
-      }
-      this.db.run('INSERT INTO collab_poll_votes (poll_id, user_id, option_index) VALUES (?, ?, ?)', pollId, userId, optionIndex);
+      this.db.transaction(() => {
+        if (!poll.multiple) {
+          this.db.run('DELETE FROM collab_poll_votes WHERE poll_id = ? AND user_id = ?', pollId, userId);
+        }
+        this.db.run('INSERT INTO collab_poll_votes (poll_id, user_id, option_index) VALUES (?, ?, ?)', pollId, userId, optionIndex);
+      });
     }
 
     return { poll: this.getPollWithVotes(pollId) };
@@ -430,7 +439,9 @@ export class CollabService {
   async linkPreview(url: string): Promise<LinkPreviewResult> {
     const fallback: LinkPreviewResult = { title: null, description: null, image: null, url };
 
-    const parsed = new URL(url);
+    // A malformed URL returns the fallback directly (the legacy code let
+    // `new URL` throw and relied on the controller's catch for the same 200).
+    try { new URL(url); } catch { return fallback; }
     const ssrf = await checkSsrf(url, true);
     if (!ssrf.allowed) {
       return { ...fallback, error: ssrf.error } as LinkPreviewResult & { error?: string };

server/tests/integration/collab.test.ts

@@ -669,6 +669,19 @@ describe('Collab validation', () => {
 });
 
 describe('Link preview', () => {
+  it('COLLAB-025 — GET /collab/link-preview as a non-member returns 404 (trip-access check added post-migration)', async () => {
+    const { user: owner } = createUser(testDb);
+    const { user: outsider } = createUser(testDb);
+    const trip = createTrip(testDb, owner.id);
+
+    const res = await request(app)
+      .get(`/api/trips/${trip.id}/collab/link-preview?url=https://example.com`)
+      .set('Cookie', authCookie(outsider.id));
+
+    expect(res.status).toBe(404);
+    expect(res.body).toEqual({ error: 'Trip not found' });
+  });
+
   it('COLLAB-025 — GET /collab/link-preview without url returns 400', async () => {
     const { user } = createUser(testDb);
     const trip = createTrip(testDb, user.id);

server/tests/unit/mcp/tools-collab-polls-chat.test.ts

@@ -178,6 +178,24 @@ describe('Tool: vote_collab_poll', () => {
     });
   });
 
+  it('blocks demo user (gate added with the DI migration — the legacy registrar missed it)', async () => {
+    process.env.DEMO_MODE = 'true';
+    const { user } = createUser(testDb, { email: 'demo@nomad.app' });
+    const trip = createTrip(testDb, user.id);
+    const pollId = (testDb.prepare(
+      `INSERT INTO collab_polls (trip_id, user_id, question, options, created_at) VALUES (?, ?, ?, ?, datetime('now'))`
+    ).run(trip.id, user.id, 'Best city?', JSON.stringify(['Paris', 'Rome'])) as any).lastInsertRowid;
+
+    await withHarness(user.id, async (h) => {
+      const result = await h.client.callTool({
+        name: 'vote_collab_poll',
+        arguments: { tripId: trip.id, pollId: Number(pollId), optionIndex: 0 },
+      });
+      expect(result.isError).toBe(true);
+      expect(testDb.prepare('SELECT COUNT(*) as c FROM collab_poll_votes WHERE poll_id = ?').get(pollId)).toEqual({ c: 0 });
+    });
+  });
+
   it('returns access denied for non-member', async () => {
     const { user } = createUser(testDb);
     const { user: other } = createUser(testDb);

server/tests/unit/nest/collab.controller.test.ts

@@ -154,7 +154,8 @@ describe('CollabController (parity with the legacy /api/trips/:tripId/collab rou
   });
 
   describe('link preview', () => {
-    it('400 without url, maps an error result to 400, else returns the preview', async () => {
+    it('404 without trip access, 400 without url, maps an error result to 400, else returns the preview', async () => {
+      expect(await thrownAsync(() => new CollabController(svc({ verifyTripAccess: vi.fn().mockReturnValue(undefined) })).linkPreview(user, '5', 'http://x'))).toEqual({ status: 404, body: { error: 'Trip not found' } });
       expect(await thrownAsync(() => new CollabController(svc()).linkPreview(user, '5', undefined))).toEqual({ status: 400, body: { error: 'URL is required' } });
       expect(await thrownAsync(() => new CollabController(svc({ linkPreview: vi.fn().mockResolvedValue({ error: 'bad url' }) } as Partial<CollabService>)).linkPreview(user, '5', 'http://x'))).toEqual({ status: 400, body: { error: 'bad url' } });
       const s = svc({ linkPreview: vi.fn().mockResolvedValue({ title: 'T', description: null, image: null, url: 'http://x' }) } as Partial<CollabService>);

server/tests/unit/nest/collab.service.test.ts

@@ -1,10 +1,12 @@
 /**
- * Unit tests for the DI-native CollabService — COLLAB-SVC-001 to COLLAB-SVC-033
+ * Unit tests for the DI-native CollabService — COLLAB-SVC-001 to COLLAB-SVC-038
  * (001–030 moved 1:1 from the legacy tests/unit/services/collabService.test.ts;
- * 031–033 pin the collab.bridge delegation). Covers votePoll edge cases,
- * listMessages pagination, deleteMessage ownership, updateNote partial fields,
- * linkPreview, avatarUrl, createMessage reply validation. Uses a real in-memory
- * SQLite DB so SQL logic is exercised faithfully.
+ * 031–033 pin the collab.bridge delegation; 034–038 pin the post-migration
+ * hardening: transactional writes, trip-scoped getFormattedNoteById, the
+ * integer vote guard and malformed-URL absorption). Covers votePoll edge
+ * cases, listMessages pagination, deleteMessage ownership, updateNote partial
+ * fields, linkPreview, avatarUrl, createMessage reply validation. Uses a real
+ * in-memory SQLite DB so SQL logic is exercised faithfully.
  */
 import { describe, it, expect, vi, beforeAll, beforeEach, afterAll, afterEach } from 'vitest';
 
@@ -432,3 +434,72 @@ describe('collab.bridge', () => {
   });
 });
 
+// ── Post-migration hardening (transactions, scoping, guards) ──────────────────
+
+describe('hardening', () => {
+  it('COLLAB-SVC-034: votePoll switch is atomic — prior vote survives a failed INSERT', () => {
+    const { user1, trip } = setup();
+    const dbs = new DatabaseService(testDb);
+    const failing = new CollabService(dbs);
+    const poll = failing.createPoll(trip.id, user1.id, { question: 'Q?', options: ['A', 'B'] });
+    failing.votePoll(trip.id, poll!.id, user1.id, 0);
+
+    const realRun = dbs.run.bind(dbs);
+    const spy = vi.spyOn(dbs, 'run').mockImplementation((sql: string, ...params: unknown[]) => {
+      if (sql.includes('INSERT INTO collab_poll_votes')) throw new Error('boom');
+      return realRun(sql, ...params);
+    });
+    // Single-choice switch: DELETE prior votes, then the INSERT fails — the
+    // transaction must roll the DELETE back too.
+    expect(() => failing.votePoll(trip.id, poll!.id, user1.id, 1)).toThrow('boom');
+    spy.mockRestore();
+
+    const votes = testDb.prepare('SELECT option_index FROM collab_poll_votes WHERE poll_id = ?').all(poll!.id) as { option_index: number }[];
+    expect(votes).toEqual([{ option_index: 0 }]);
+  });
+
+  it('COLLAB-SVC-035: deleteNote is atomic — trip_files rows survive a failed note DELETE', () => {
+    const { user1, trip } = setup();
+    const dbs = new DatabaseService(testDb);
+    const failing = new CollabService(dbs);
+    const note = failing.createNote(trip.id, user1.id, { title: 'With file' });
+    testDb.prepare('INSERT INTO trip_files (trip_id, note_id, filename, original_name) VALUES (?, ?, ?, ?)')
+      .run(trip.id, note.id, 'files/a.pdf', 'a.pdf');
+
+    const realRun = dbs.run.bind(dbs);
+    const spy = vi.spyOn(dbs, 'run').mockImplementation((sql: string, ...params: unknown[]) => {
+      if (sql.includes('DELETE FROM collab_notes')) throw new Error('boom');
+      return realRun(sql, ...params);
+    });
+    expect(() => failing.deleteNote(trip.id, note.id)).toThrow('boom');
+    spy.mockRestore();
+
+    expect(testDb.prepare('SELECT COUNT(*) as c FROM trip_files WHERE note_id = ?').get(note.id)).toEqual({ c: 1 });
+    expect(testDb.prepare('SELECT COUNT(*) as c FROM collab_notes WHERE id = ?').get(note.id)).toEqual({ c: 1 });
+  });
+
+  it('COLLAB-SVC-036: getFormattedNoteById is trip-scoped and null-safe', () => {
+    const { user1, trip } = setup();
+    const otherTrip = createTrip(testDb, user1.id);
+    const note = svc.createNote(trip.id, user1.id, { title: 'Scoped' });
+
+    expect(svc.getFormattedNoteById(trip.id, note.id)!.title).toBe('Scoped');
+    expect(svc.getFormattedNoteById(otherTrip.id, note.id)).toBeNull();
+    expect(svc.getFormattedNoteById(trip.id, 9999)).toBeNull();
+  });
+
+  it('COLLAB-SVC-037: votePoll rejects a non-integer option index with "invalid_index"', () => {
+    const { user1, trip } = setup();
+    const poll = svc.createPoll(trip.id, user1.id, { question: 'Q?', options: ['A', 'B'] });
+
+    expect(svc.votePoll(trip.id, poll!.id, user1.id, '0' as unknown as number).error).toBe('invalid_index');
+    expect(svc.votePoll(trip.id, poll!.id, user1.id, 0.5).error).toBe('invalid_index');
+    expect(testDb.prepare('SELECT COUNT(*) as c FROM collab_poll_votes WHERE poll_id = ?').get(poll!.id)).toEqual({ c: 0 });
+  });
+
+  it('COLLAB-SVC-038: linkPreview returns the fallback for a malformed URL without throwing', async () => {
+    const result = await svc.linkPreview('not a url');
+    expect(result).toEqual({ title: null, description: null, image: null, url: 'not a url' });
+    expect(mockCheckSsrf).not.toHaveBeenCalled();
+  });
+});