From 07b8f1234cda6aa11d3c15a9492775eda6bcf545 Mon Sep 17 00:00:00 2001 From: jubnl Date: Mon, 27 Jul 2026 00:27:12 +0200 Subject: [PATCH] fix(server): fix the quirks preserved by the collab DI migration --- server/src/nest/collab/collab.controller.ts | 13 +-- server/src/nest/collab/collab.mcp.ts | 6 +- server/src/nest/collab/collab.service.ts | 45 +++++++---- server/tests/integration/collab.test.ts | 13 +++ .../unit/mcp/tools-collab-polls-chat.test.ts | 18 +++++ .../tests/unit/nest/collab.controller.test.ts | 3 +- server/tests/unit/nest/collab.service.test.ts | 81 +++++++++++++++++-- 7 files changed, 149 insertions(+), 30 deletions(-) diff --git a/server/src/nest/collab/collab.controller.ts b/server/src/nest/collab/collab.controller.ts index 9ce625fe..67c2488b 100644 --- a/server/src/nest/collab/collab.controller.ts +++ b/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); } diff --git a/server/src/nest/collab/collab.mcp.ts b/server/src/nest/collab/collab.mcp.ts index 8b50d33c..7a3e049c 100644 --- a/server/src/nest/collab/collab.mcp.ts +++ b/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); diff --git a/server/src/nest/collab/collab.service.ts b/server/src/nest/collab/collab.service.ts index 7f4e5b67..630ad2be 100644 --- a/server/src/nest/collab/collab.service.ts +++ b/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('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('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('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 { 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 }; diff --git a/server/tests/integration/collab.test.ts b/server/tests/integration/collab.test.ts index 51214117..bf3a9de6 100644 --- a/server/tests/integration/collab.test.ts +++ b/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); diff --git a/server/tests/unit/mcp/tools-collab-polls-chat.test.ts b/server/tests/unit/mcp/tools-collab-polls-chat.test.ts index 997720ac..7abeb5a0 100644 --- a/server/tests/unit/mcp/tools-collab-polls-chat.test.ts +++ b/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); diff --git a/server/tests/unit/nest/collab.controller.test.ts b/server/tests/unit/nest/collab.controller.test.ts index b2cc99e0..7d703525 100644 --- a/server/tests/unit/nest/collab.controller.test.ts +++ b/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)).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); diff --git a/server/tests/unit/nest/collab.service.test.ts b/server/tests/unit/nest/collab.service.test.ts index 5a09f784..ee04d79d 100644 --- a/server/tests/unit/nest/collab.service.test.ts +++ b/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(); + }); +});