mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-18 21:16:47 +00:00
fix(collab): stop serving the text of deleted chat messages
A deleted message keeps its row so the client can draw the placeholder off the flag, but listMessages still returned the original text - in the list itself and in the reply preview joined onto whatever quoted it. Both REST and the MCP resource read through that one method, so blanking it there covers every surface. Also cap the link-preview read, and delete the spooled upload when addNoteFile refuses the request instead of leaving it on disk.
This commit is contained in:
@@ -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);
|
||||
|
||||
@@ -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:/<title> 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'));
|
||||
|
||||
@@ -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>);
|
||||
|
||||
@@ -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</title></head></html>',
|
||||
}));
|
||||
|
||||
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('<html><head><title>Head Title</title></head><body>'),
|
||||
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,
|
||||
|
||||
Reference in New Issue
Block a user