diff --git a/server/src/nest/collab/collab.controller.ts b/server/src/nest/collab/collab.controller.ts index a8a54bb7e..4f35faf4b 100644 --- a/server/src/nest/collab/collab.controller.ts +++ b/server/src/nest/collab/collab.controller.ts @@ -17,6 +17,7 @@ import { import { FileInterceptor } from '@nestjs/platform-express'; import type { Options } from 'multer'; import path from 'path'; +import fs from 'fs'; import type { User } from '../../types'; import { CollabService } from './collab.service'; import { StorageService } from '../storage/storage.service'; @@ -146,9 +147,20 @@ export class CollabController { @Post('notes/:id/files') @UseInterceptors(FileInterceptor('file')) async addNoteFile(@CurrentUser() user: User, @Param('tripId') tripId: string, @Param('id') id: string, @UploadedFile() file: Express.Multer.File | undefined, @Headers('x-socket-id') socketId?: string) { - const trip = this.requireTrip(tripId, user); - if (!this.collab.canUploadFiles(trip, user)) { - throw new HttpException({ error: 'No permission to upload files' }, 403); + // multer has already written the upload to the spool dir by the time any of + // these checks run, and nothing sweeps orphans, so every refusal takes the + // bytes back out, with the same closure the trip-file upload uses. + const cleanupSpool = () => { + if (file?.path) { try { fs.unlinkSync(file.path); } catch { /* best-effort */ } } + }; + try { + const trip = this.requireTrip(tripId, user); + if (!this.collab.canUploadFiles(trip, user)) { + throw new HttpException({ error: 'No permission to upload files' }, 403); + } + } catch (err) { + cleanupSpool(); + throw err; } if (!file) { throw new HttpException({ error: 'No file uploaded' }, 400); @@ -158,6 +170,8 @@ export class CollabController { await this.storage.put('files', file.filename, { tmpPath: file.path }); const result = this.collab.addNoteFile(tripId, id, file); if (!result) { + // Already committed, so the spool file is gone; drop the final object. + await this.storage.delete('files', file.filename).catch(() => {}); throw new HttpException({ error: 'Note not found' }, 404); } this.collab.broadcast(tripId, 'collab:note:updated', { note: this.collab.getFormattedNoteById(tripId, id) }, socketId); diff --git a/server/src/nest/collab/collab.service.ts b/server/src/nest/collab/collab.service.ts index efd1b2d1b..173b5cdab 100644 --- a/server/src/nest/collab/collab.service.ts +++ b/server/src/nest/collab/collab.service.ts @@ -6,12 +6,17 @@ import { RealtimeService } from '../realtime/realtime.service'; import { PermissionsService } from '../permissions/permissions.service'; import { avatarUrl } from '../common/avatarUrl'; import { checkSsrf, createPinnedDispatcher } from '../../utils/ssrfGuard'; +import { exceedsDeclaredLength, readCappedText } from '../../utils/cappedFetch'; import type { CollabNote, CollabPoll, CollabMessage, TripFile, User } from '../../types'; import { NotificationsService } from '../notifications/notifications.service'; import { StorageService } from '../storage/storage.service'; type Trip = TripAccess; +// Half a megabyte of markup is far more than any og:/ block needs, and +// the URL is caller-supplied, so read no further than that. +const MAX_PREVIEW_BYTES = 512 * 1024; + export interface ReactionRow { emoji: string; user_id: number; @@ -374,7 +379,8 @@ export class CollabService { listMessages(tripId: string | number, before?: string | number) { const query = ` SELECT m.*, u.username, u.avatar, - rm.text AS reply_text, ru.username AS reply_username + CASE WHEN rm.deleted = 1 THEN '' ELSE rm.text END AS reply_text, + ru.username AS reply_username FROM collab_messages m JOIN users u ON m.user_id = u.id LEFT JOIN collab_messages rm ON m.reply_to = rm.id @@ -390,6 +396,12 @@ export class CollabService { messages.reverse(); + // A deleted message keeps its row so the client can render the placeholder + // off the flag, but the original text must not leave the server. REST and + // both MCP surfaces read through this method, so blanking here covers all + // three. + for (const m of messages) if (m.deleted) m.text = ''; + const msgIds = messages.map(m => m.id); const reactionsByMsg: Record<number, ReactionRow[]> = {}; if (msgIds.length > 0) { @@ -468,8 +480,11 @@ export class CollabService { } as any); clearTimeout(timeout); if (!r.ok) throw new Error('Fetch failed'); + if (exceedsDeclaredLength(r, MAX_PREVIEW_BYTES)) return fallback; - const html = await r.text(); + // A truncated head still carries the tags we scrape, so a page over the + // budget degrades to fewer fields rather than to an error. + const { text: html } = await readCappedText(r, MAX_PREVIEW_BYTES); const get = (prop: string) => { const m = html.match(new RegExp(`<meta[^>]*property=["']og:${prop}["'][^>]*content=["']([^"']*)["']`, 'i')) || html.match(new RegExp(`<meta[^>]*content=["']([^"']*)["'][^>]*property=["']og:${prop}["']`, 'i')); diff --git a/server/tests/unit/nest/collab.controller.test.ts b/server/tests/unit/nest/collab.controller.test.ts index 29292f8ac..08da31f29 100644 --- a/server/tests/unit/nest/collab.controller.test.ts +++ b/server/tests/unit/nest/collab.controller.test.ts @@ -1,5 +1,8 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { HttpException } from '@nestjs/common'; +import fs from 'fs'; +import os from 'os'; +import path from 'path'; import { CollabController } from '../../../src/nest/collab/collab.controller'; import type { CollabService } from '../../../src/nest/collab/collab.service'; @@ -19,7 +22,10 @@ function svc(o: Partial<CollabService> = {}): CollabService { } as unknown as CollabService; } -const storageStub = { put: vi.fn().mockResolvedValue(undefined) } as unknown as StorageService; +const storageStub = { + put: vi.fn().mockResolvedValue(undefined), + delete: vi.fn().mockResolvedValue(undefined), +} as unknown as StorageService; function thrown(fn: () => unknown): { status: number; body: unknown } { try { fn(); } catch (err) { @@ -89,6 +95,26 @@ describe('CollabController (parity with the legacy /api/trips/:tripId/collab rou expect(broadcast).toHaveBeenCalledWith('5', 'collab:note:updated', { note: { id: 9 } }, 'sock'); }); + it('reclaims the upload on every refusal, spooled or already committed', async () => { + // multer has written the file before any check runs, and nothing sweeps + // orphans, so a loop of rejected POSTs would otherwise fill the disk. + const spool = path.join(os.tmpdir(), `trek-collab-${Date.now()}-${Math.random().toString(16).slice(2)}.pdf`); + fs.writeFileSync(spool, 'x'); + const spooled = { filename: 'a.pdf', path: spool } as Express.Multer.File; + expect(await thrownAsync(() => new CollabController(svc({ canUploadFiles: vi.fn().mockReturnValue(false) }), storageStub).addNoteFile(user, '5', '9', spooled))).toEqual({ status: 403, body: { error: 'No permission to upload files' } }); + expect(fs.existsSync(spool)).toBe(false); + + const spool2 = path.join(os.tmpdir(), `trek-collab-${Date.now()}-${Math.random().toString(16).slice(2)}.pdf`); + fs.writeFileSync(spool2, 'x'); + expect(await thrownAsync(() => new CollabController(svc({ verifyTripAccess: vi.fn().mockReturnValue(null) }), storageStub).addNoteFile(user, '5', '9', { filename: 'b.pdf', path: spool2 } as Express.Multer.File))).toEqual({ status: 404, body: { error: 'Trip not found' } }); + expect(fs.existsSync(spool2)).toBe(false); + + // Past the commit the spool file is gone, so the final object is what has + // to go instead. + await thrownAsync(() => new CollabController(svc({ addNoteFile: vi.fn().mockReturnValue(null) } as Partial<CollabService>), storageStub).addNoteFile(user, '5', '9', file)); + expect(storageStub.delete).toHaveBeenCalledWith('files', 'a.pdf'); + }); + it('DELETE file 404 when missing, else success', async () => { expect(await thrownAsync(() => new CollabController(svc({ deleteNoteFile: vi.fn().mockResolvedValue(false) } as Partial<CollabService>), storageStub).deleteNoteFile(user, '5', '9', '3'))).toEqual({ status: 404, body: { error: 'File not found' } }); const s = svc({ deleteNoteFile: vi.fn().mockResolvedValue(true), getFormattedNoteById: vi.fn().mockReturnValue({ id: 9 }), broadcast: vi.fn() } as Partial<CollabService>); diff --git a/server/tests/unit/nest/collab.service.test.ts b/server/tests/unit/nest/collab.service.test.ts index d85d84854..651561ea3 100644 --- a/server/tests/unit/nest/collab.service.test.ts +++ b/server/tests/unit/nest/collab.service.test.ts @@ -209,6 +209,28 @@ describe('listMessages', () => { expect(msgs[2].text).toBe('C'); }); + it('COLLAB-SVC-013a: blanks the text of a deleted message but keeps the flag', () => { + const { user1, trip } = setup(); + const r = svc.createMessage(trip.id, user1.id, 'Secret plans'); + svc.deleteMessage(trip.id, r.message!.id, user1.id); + + const msgs = svc.listMessages(trip.id); + expect(msgs).toHaveLength(1); + expect(msgs[0].text).toBe(''); + expect(msgs[0].deleted).toBe(1); + }); + + it('COLLAB-SVC-013b: blanks reply_text when the quoted message was deleted', () => { + const { user1, trip } = setup(); + const original = svc.createMessage(trip.id, user1.id, 'Original secret'); + svc.createMessage(trip.id, user1.id, 'Quoting it', original.message!.id); + svc.deleteMessage(trip.id, original.message!.id, user1.id); + + const msgs = svc.listMessages(trip.id); + const reply = msgs.find(m => m.text === 'Quoting it')!; + expect(reply.reply_text).toBe(''); + }); + it('COLLAB-SVC-013: includes reactions grouped by emoji', () => { const { user1, trip } = setup(); const r = svc.createMessage(trip.id, user1.id, 'React me'); @@ -398,6 +420,42 @@ describe('linkPreview', () => { expect(result.url).toBe('https://example.com/net-error'); }); + it('COLLAB-SVC-030a: returns the fallback when the page declares a body over the cap', async () => { + vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ + ok: true, + headers: { get: (h: string) => (h === 'content-length' ? String(10 * 1024 * 1024) : null) }, + text: async () => '<html><head><title>Huge', + })); + + const result = await svc.linkPreview('https://example.com/huge'); + expect(result.title).toBeNull(); + }); + + it('COLLAB-SVC-030b: reads only up to the cap and still scrapes the head tags', async () => { + let cancelled = false; + const chunks = [ + new TextEncoder().encode('Head Title'), + new TextEncoder().encode('x'.repeat(1024 * 1024)), + ]; + let i = 0; + vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ + ok: true, + headers: { get: () => null }, + body: { + getReader: () => ({ + read: async () => (i < chunks.length ? { done: false, value: chunks[i++] } : { done: true }), + cancel: async () => { cancelled = true; }, + }), + }, + })); + + const result = await svc.linkPreview('https://example.com/streamed'); + // The head arrives first, so the scrape still works; the megabyte of padding + // past the 512KB budget is dropped and the reader is cancelled. + expect(result.title).toBe('Head Title'); + expect(cancelled).toBe(true); + }); + it('COLLAB-SVC-030: falls back to meta description tag when no og:description', async () => { vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ ok: true,