From 88afdb986cab2ce9f088b7bb58e2800aa03a93e9 Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 1 Oct 2026 12:39:58 +0200 Subject: [PATCH] feat(mcp): upload_trip_file tool for adding documents to a trip (#1566) --- server/src/nest/files/files.constants.ts | 14 +++ server/src/nest/files/files.controller.ts | 20 +-- server/src/nest/files/files.mcp.ts | 65 +++++++++- server/src/nest/files/files.module.ts | 4 +- server/src/nest/files/files.service.ts | 19 ++- server/tests/helpers/mcp-test-controllers.ts | 2 +- server/tests/unit/mcp/tools-files.test.ts | 126 ++++++++++++++++++- wiki/MCP-Tools-and-Resources.md | 1 + 8 files changed, 228 insertions(+), 23 deletions(-) diff --git a/server/src/nest/files/files.constants.ts b/server/src/nest/files/files.constants.ts index 5178f18c9..589974180 100644 --- a/server/src/nest/files/files.constants.ts +++ b/server/src/nest/files/files.constants.ts @@ -47,3 +47,17 @@ export const BLOCKED_EXTENSIONS = [ // the extra '..' keeps the same absolute /uploads/files under both the // src (vitest) and dist (runtime) layouts. export const filesDir = path.join(__dirname, '../../../uploads/files'); + +/** + * Whether a trip-file upload named `originalname` passes the extension rules: + * never a blocked extension or an SVG, otherwise on the operator's list (or `*`), + * and video regardless of that list (#823). Shared by the multipart filter and + * the MCP upload tool so the two ingestion paths cannot drift apart. + */ +export function isUploadTypeAllowed(originalname: string, mimetype: string, allowedList: string): boolean { + const ext = path.extname(originalname).toLowerCase(); + if (BLOCKED_EXTENSIONS.includes(ext) || mimetype.includes('svg')) return false; + const allowed = allowedList.split(',').map((e) => e.trim().toLowerCase()); + const fileExt = ext.replace('.', ''); + return allowed.includes(fileExt) || isVideoExtension(fileExt) || allowed.includes('*'); +} diff --git a/server/src/nest/files/files.controller.ts b/server/src/nest/files/files.controller.ts index c5077c0a8..cfcbea84d 100644 --- a/server/src/nest/files/files.controller.ts +++ b/server/src/nest/files/files.controller.ts @@ -29,7 +29,7 @@ import { CurrentUser } from '../auth/current-user.decorator'; import { TripAccessGuard } from '../permissions/trip-access.guard'; import type { TripAccess } from '../database/database.service'; import { Trip } from '../permissions/trip.decorator'; -import { MAX_FILE_SIZE, BLOCKED_EXTENSIONS, isVideoExtension } from './files.constants'; +import { MAX_FILE_SIZE, isUploadTypeAllowed, isVideoExtension } from './files.constants'; import { FileUploadDto, FileUpdateDto, FileLinkDto } from './files.dto'; import { AllowedFileTypesService } from './allowed-file-types.service'; @@ -44,19 +44,11 @@ import { AllowedFileTypesService } from './allowed-file-types.service'; */ export function filesUploadFileFilter(allowedTypes: AllowedFileTypesService): Options['fileFilter'] { return (_req, file, cb) => { - const ext = path.extname(file.originalname).toLowerCase(); - const reject = () => { - // i18n key — the client resolves it via t() (see translateApiError). - const err: Error & { statusCode?: number } = new Error('files.uploadErrorType'); - err.statusCode = 400; - cb(err); - }; - if (BLOCKED_EXTENSIONS.includes(ext) || file.mimetype.includes('svg')) return reject(); - const allowed = allowedTypes.get().split(',').map((e) => e.trim().toLowerCase()); - const fileExt = ext.replace('.', ''); - // Video is accepted as media regardless of the admin doc-types allowlist (#823). - if (allowed.includes(fileExt) || isVideoExtension(fileExt) || (allowed.includes('*') && !BLOCKED_EXTENSIONS.includes(ext))) return cb(null, true); - reject(); + if (isUploadTypeAllowed(file.originalname, file.mimetype, allowedTypes.get())) return cb(null, true); + // i18n key — the client resolves it via t() (see translateApiError). + const err: Error & { statusCode?: number } = new Error('files.uploadErrorType'); + err.statusCode = 400; + cb(err); }; } diff --git a/server/src/nest/files/files.mcp.ts b/server/src/nest/files/files.mcp.ts index 3858c2485..081ed3021 100644 --- a/server/src/nest/files/files.mcp.ts +++ b/server/src/nest/files/files.mcp.ts @@ -10,9 +10,21 @@ import { noAccess, permissionDenied } from '../../mcp/tools/_shared'; import { AuthService } from '../auth/auth.service'; import { McpToolGuardsService } from '../mcp-shared/mcp-tool-guards.service'; import { FilesService, FileContentError, FILE_CONTENT_MAX } from './files.service'; +import { AllowedFileTypesService } from './allowed-file-types.service'; +import { MAX_FILE_SIZE, isUploadTypeAllowed } from './files.constants'; +import { contentTypeFor } from '../storage/content-type'; const CONTENT_MAX_MB = Math.round(FILE_CONTENT_MAX / (1024 * 1024)); +/** + * The upload tool's cap: the same 10 MB a read hands back, or the operator's + * upload limit when that is lower. The bytes travel base64 inside one JSON-RPC + * message, so the multipart route's 500 MB video allowance has no place here. + */ +const UPLOAD_MAX = Math.min(FILE_CONTENT_MAX, MAX_FILE_SIZE); +const UPLOAD_MAX_MB = Math.max(1, Math.floor(UPLOAD_MAX / (1024 * 1024))); +const BASE64 = /^[A-Za-z0-9+/]*={0,2}$/; + /** * Bytes worth handing over as text. Everything else goes back base64: a model * can pass those on to a converter, but it cannot read them, and decoding a PDF @@ -42,9 +54,10 @@ function contentRefusal(err: FileContentError): string { * two listing tools carry no file_* right because the routes they mirror carry * none either; trip access is the whole gate there. * - * There is no upload tool on purpose. MCP has no file transport, and an upload - * faked through base64 would be a second, unpoliced ingestion path beside the - * multipart route with its extension filter and per-type size caps. + * upload_trip_file (#1566) takes the bytes base64-encoded, since MCP has no file + * transport. It is policed like the multipart route: the same extension rules + * through isUploadTypeAllowed, the file_upload right, the demo block and the + * same-trip check on what it attaches to, plus a tighter size cap. */ @McpController() export class FilesMcp { @@ -52,6 +65,7 @@ export class FilesMcp { private readonly files: FilesService, private readonly auth: AuthService, private readonly guards: McpToolGuardsService, + private readonly allowedTypes: AllowedFileTypesService, ) {} @Tool({ @@ -103,6 +117,51 @@ export class FilesMcp { } } + @Tool({ + name: 'upload_trip_file', + description: `Add a document to a trip, e.g. a booking confirmation PDF or a ticket, and optionally attach it to a booking or a place in the same call. Pass the bytes base64-encoded in content, up to ${UPLOAD_MAX_MB} MB decoded. The file name has to carry an extension the trip's file manager accepts; the type is derived from it. Returns the new file, whose ID works with read_trip_file and link_trip_file.`, + inputSchema: { + tripId: z.number().int().positive(), + filename: z.string().trim().min(1).max(255).describe('File name including its extension, e.g. "hotel-confirmation.pdf"'), + content: z.string().min(1).max(Math.ceil(UPLOAD_MAX / 3) * 4).describe('The file bytes, base64-encoded'), + description: z.string().max(1000).optional().describe('Free-text description'), + reservation_id: z.number().int().positive().optional().describe('Booking on the same trip to attach the file to'), + place_id: z.number().int().positive().optional().describe('Place on the same trip to attach the file to'), + }, + annotations: TOOL_ANNOTATIONS_WRITE, + access: { group: 'files', mode: 'write' }, + }) + async uploadTripFile( + { tripId, filename, content, description, reservation_id, place_id }: { + tripId: number; filename: string; content: string; description?: string; reservation_id?: number; place_id?: number; + }, + ctx: McpContext, + ) { + if (!this.files.verifyTripAccess(tripId, ctx.userId)) return noAccess(); + if (this.auth.isDemoUser(ctx.userId)) return demoDenied(); + if (!this.guards.hasTripPermission('file_upload', tripId, ctx.userId)) return permissionDenied(); + // A path someone pasted in is not a name; keep only the last segment. + const originalname = filename.split(/[\\/]/).pop()!.trim(); + const mimetype = contentTypeFor(originalname); + if (!originalname || !isUploadTypeAllowed(originalname, mimetype, this.allowedTypes.get())) { + return errorResult('This file type is not allowed on this TREK instance.'); + } + const clean = content.replace(/\s+/g, ''); + if (!BASE64.test(clean)) return errorResult('content is not valid base64.'); + const bytes = Buffer.from(clean, 'base64'); + if (bytes.length === 0) return errorResult('The file is empty.'); + if (bytes.length > UPLOAD_MAX) return errorResult(`File is too large (over ${UPLOAD_MAX_MB} MB). Ask the user to upload it in TREK instead.`); + const foreign = this.files.findForeignLinkTarget(tripId, { reservation_id, place_id }); + if (foreign) return errorResult(`Linked item does not belong to this trip (${foreign}).`); + const file = await this.files.createFileFromBytes(tripId, { originalname, mimetype, bytes }, ctx.userId, { + reservation_id: reservation_id ?? null, + place_id: place_id ?? null, + description: description ?? null, + }); + this.guards.safeBroadcast(tripId, 'file:created', { file }); + return ok({ file }); + } + @Tool({ name: 'update_trip_file', description: 'Set a file\'s description and attach it to a booking or a place. Fields left out keep their current value, null detaches. place_id and reservation_id are the file\'s primary attachment, the one the file manager shows next to it. To attach one document to several bookings or places at once, use link_trip_file instead.', diff --git a/server/src/nest/files/files.module.ts b/server/src/nest/files/files.module.ts index 5637bdd92..b40e5bb60 100644 --- a/server/src/nest/files/files.module.ts +++ b/server/src/nest/files/files.module.ts @@ -40,7 +40,9 @@ import { MAX_FILE_SIZE, MAX_VIDEO_SIZE } from './files.constants'; // AuthModule + McpSharedModule feed FilesMcp's demo and RBAC guards. Neither is // @Global, and AuthModule reaches this domain only through the leaf // AllowedFileTypesModule, so importing it here stays cycle-free. - EphemeralTokenModule, PermissionsModule, AppConfigModule, RealtimeModule, PluginGuardsModule, AuthModule, McpSharedModule], + EphemeralTokenModule, PermissionsModule, AppConfigModule, RealtimeModule, PluginGuardsModule, AuthModule, McpSharedModule, + // FilesMcp's upload tool checks the same extension list as the multipart filter. + AllowedFileTypesModule], controllers: [FilesController, FilesDownloadController], providers: [FilesService, FilesRpc, FilesMcp], exports: [FilesService], diff --git a/server/src/nest/files/files.service.ts b/server/src/nest/files/files.service.ts index 43aa44029..ad5281c6d 100644 --- a/server/src/nest/files/files.service.ts +++ b/server/src/nest/files/files.service.ts @@ -1,6 +1,7 @@ import { Injectable } from '@nestjs/common'; import path from 'path'; -import type { Readable } from 'node:stream'; +import { randomUUID } from 'crypto'; +import { Readable } from 'node:stream'; import type { Request } from 'express'; import type { TrekWsPayload, TrekWsTripEventName } from '@trek/shared'; import { RealtimeService } from '../realtime/realtime.service'; @@ -282,6 +283,22 @@ export class FilesService { return formatFile(created); } + /** + * Store bytes that arrived without multipart (the MCP upload tool, #1566) and + * record them like a multipart upload: same random storage key, same row. The + * caller has already run the type and size checks. + */ + async createFileFromBytes( + tripId: string | number, + upload: { originalname: string; mimetype: string; bytes: Buffer }, + uploadedBy: number, + opts: { place_id?: number | null; reservation_id?: number | null; description?: string | null }, + ) { + const filename = `${randomUUID()}${path.extname(upload.originalname)}`; + await this.storage.put('files', filename, Readable.from([upload.bytes]), { contentType: upload.mimetype }); + return this.createFile(tripId, { filename, originalname: upload.originalname, size: upload.bytes.length, mimetype: upload.mimetype }, uploadedBy, opts); + } + updateFile( id: string | number, current: TripFile, diff --git a/server/tests/helpers/mcp-test-controllers.ts b/server/tests/helpers/mcp-test-controllers.ts index 2f5a31aed..a8da2d421 100644 --- a/server/tests/helpers/mcp-test-controllers.ts +++ b/server/tests/helpers/mcp-test-controllers.ts @@ -233,7 +233,7 @@ export function createMcpTestRegistry(): McpRegistry { new DaysMcp(daysService, authService, guards, dayRemovalService), new RoadtripMcp(new RoadtripService(dbService, realtimeService), dbService, guards, authService, addonsService), new RoadtripPreferencesMcp(new RoadtripPreferencesService(dbService, realtimeService), authService, addonsService, dbService, guards), - new FilesMcp(new FilesService(dbService, permissionsService, realtimeService, new EphemeralTokenService(), generalStorage), authService, guards), + new FilesMcp(new FilesService(dbService, permissionsService, realtimeService, new EphemeralTokenService(), generalStorage), authService, guards, new AllowedFileTypesService(dbService)), new AccommodationsMcp(accommodationsService, dbService, placesService, authService, guards), new AssignmentsMcp(assignmentsService, daysService, authService, guards), new CollabMcp(collabService, authService, addonsService, guards), diff --git a/server/tests/unit/mcp/tools-files.test.ts b/server/tests/unit/mcp/tools-files.test.ts index 2fbcbb9e3..c767328d8 100644 --- a/server/tests/unit/mcp/tools-files.test.ts +++ b/server/tests/unit/mcp/tools-files.test.ts @@ -107,7 +107,7 @@ interface FileRowOverrides { deleted_at: string | null; } -/** A trip_files row, straight in: there is no upload path through MCP to make one. */ +/** A trip_files row, straight in, so each case controls every column. */ function insertFile(tripId: number, overrides: Partial = {}) { const info = testDb.prepare(` INSERT INTO trip_files (trip_id, filename, original_name, file_size, mime_type, description, uploaded_by, place_id, reservation_id, starred, deleted_at) @@ -501,6 +501,126 @@ describe('Tool: update_trip_file', () => { }); }); +// --------------------------------------------------------------------------- +// upload_trip_file (#1566) +// --------------------------------------------------------------------------- + +describe('Tool: upload_trip_file', () => { + const pdf = Buffer.from('%PDF-1.4 booking').toString('base64'); + + it('stores the bytes, records the row and attaches it to a booking', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const booking = createReservation(testDb, trip.id); + + await withHarness(user.id, async (h) => { + const data = parseToolResult(await h.client.callTool({ + name: 'upload_trip_file', + arguments: { tripId: trip.id, filename: 'C:\\mail\\hotel.pdf', content: pdf, description: 'Hotel', reservation_id: booking.id }, + })) as any; + expect(data.file).toMatchObject({ original_name: 'hotel.pdf', mime_type: 'application/pdf', file_size: 16, description: 'Hotel', reservation_id: booking.id, uploaded_by: user.id }); + expect(data.file.filename).toMatch(/^[0-9a-f-]{36}\.pdf$/); + expect(fs.readFileSync(nodePath.join(fixture.root, data.file.filename), 'utf8')).toBe('%PDF-1.4 booking'); + // And the read tool hands the same bytes back. + const read = parseToolResult(await h.client.callTool({ name: 'read_trip_file', arguments: { tripId: trip.id, fileId: data.file.id } })) as any; + expect(Buffer.from(read.file.content, 'base64').toString()).toBe('%PDF-1.4 booking'); + }); + expect(broadcastMock).toHaveBeenCalledWith(trip.id, 'file:created', expect.objectContaining({ _source: 'mcp' })); + }); + + it('refuses an extension the file manager would refuse', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + await withHarness(user.id, async (h) => { + for (const filename of ['evil.html', 'logo.svg', 'tool.exe', 'noext']) { + const result = await h.client.callTool({ name: 'upload_trip_file', arguments: { tripId: trip.id, filename, content: pdf } }); + expect(result.isError).toBe(true); + expect(JSON.stringify(result.content)).toContain('not allowed'); + } + }); + expect(testDb.prepare('SELECT COUNT(*) AS n FROM trip_files').get()).toEqual({ n: 0 }); + }); + + it('follows the operator list, including the wildcard', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + testDb.prepare("INSERT OR REPLACE INTO app_settings (key, value) VALUES ('allowed_file_types', 'pdf')").run(); + await withHarness(user.id, async (h) => { + const refused = await h.client.callTool({ name: 'upload_trip_file', arguments: { tripId: trip.id, filename: 'notes.txt', content: pdf } }); + expect(refused.isError).toBe(true); + testDb.prepare("UPDATE app_settings SET value = '*' WHERE key = 'allowed_file_types'").run(); + const data = parseToolResult(await h.client.callTool({ name: 'upload_trip_file', arguments: { tripId: trip.id, filename: 'route.gpx', content: pdf } })) as any; + expect(data.file.mime_type).toBe('application/gpx+xml'); + }); + }); + + it('refuses content that is not base64, or decodes to nothing', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + await withHarness(user.id, async (h) => { + const bad = await h.client.callTool({ name: 'upload_trip_file', arguments: { tripId: trip.id, filename: 'a.pdf', content: 'not base64!' } }); + expect(JSON.stringify(bad.content)).toContain('not valid base64'); + const empty = await h.client.callTool({ name: 'upload_trip_file', arguments: { tripId: trip.id, filename: 'a.pdf', content: '==' } }); + expect(JSON.stringify(empty.content)).toContain('empty'); + }); + }); + + it('refuses a file over the cap', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + // Just past the cap after decoding, still inside the schema's base64 length. + const big = Buffer.alloc(FILE_CONTENT_MAX + 1, 1).toString('base64'); + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ name: 'upload_trip_file', arguments: { tripId: trip.id, filename: 'big.pdf', content: big } }); + expect(result.isError).toBe(true); + expect(JSON.stringify(result.content)).toContain('too large'); + }); + }); + + it('refuses a place from another trip', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const foreignTrip = createTrip(testDb, user.id); + const foreignPlace = createPlace(testDb, foreignTrip.id); + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ name: 'upload_trip_file', arguments: { tripId: trip.id, filename: 'a.pdf', content: pdf, place_id: foreignPlace.id } }); + expect(result.isError).toBe(true); + expect(JSON.stringify(result.content)).toContain('does not belong to this trip'); + }); + expect(testDb.prepare('SELECT COUNT(*) AS n FROM trip_files').get()).toEqual({ n: 0 }); + }); + + it('refuses a member once file_upload is owner-only', async () => { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, member.id); + setPermission('file_upload', 'trip_owner'); + await withHarness(member.id, async (h) => { + const result = await h.client.callTool({ name: 'upload_trip_file', arguments: { tripId: trip.id, filename: 'a.pdf', content: pdf } }); + expect(result.isError).toBe(true); + }); + }); + + it('denies a non-member, and blocks a demo user', async () => { + const { user } = createUser(testDb); + const { user: other } = createUser(testDb); + const foreign = createTrip(testDb, other.id); + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ name: 'upload_trip_file', arguments: { tripId: foreign.id, filename: 'a.pdf', content: pdf } }); + expect(JSON.stringify(result.content)).toContain('Trip not found or access denied.'); + }); + process.env.DEMO_MODE = 'true'; + const { user: demo } = createUser(testDb, { email: 'demo@trek.app' }); + const own = createTrip(testDb, demo.id); + await withHarness(demo.id, async (h) => { + const result = await h.client.callTool({ name: 'upload_trip_file', arguments: { tripId: own.id, filename: 'a.pdf', content: pdf } }); + expect(result.isError).toBe(true); + }); + expect(testDb.prepare('SELECT COUNT(*) AS n FROM trip_files').get()).toEqual({ n: 0 }); + }); +}); + // --------------------------------------------------------------------------- // link_trip_file // --------------------------------------------------------------------------- @@ -818,7 +938,7 @@ describe('Tool: list_trip_file_links', () => { describe('File tools: scope gating', () => { const READ_TOOLS = ['list_trip_files', 'list_trip_file_links']; - const WRITE_TOOLS = ['update_trip_file', 'link_trip_file', 'unlink_trip_file']; + const WRITE_TOOLS = ['upload_trip_file', 'update_trip_file', 'link_trip_file', 'unlink_trip_file']; async function listToolNames(userId: number, scopes: string[] | null): Promise { const h = await createMcpHarness({ userId, withResources: false, scopes }); @@ -829,7 +949,7 @@ describe('File tools: scope gating', () => { } } - it('registers all six tools for a full-access session', async () => { + it('registers all seven tools for a full-access session', async () => { const { user } = createUser(testDb); const names = await listToolNames(user.id, null); for (const tool of [...READ_TOOLS, ...WRITE_TOOLS, 'read_trip_file']) expect(names).toContain(tool); diff --git a/wiki/MCP-Tools-and-Resources.md b/wiki/MCP-Tools-and-Resources.md index cc88f1258..0a7693092 100644 --- a/wiki/MCP-Tools-and-Resources.md +++ b/wiki/MCP-Tools-and-Resources.md @@ -214,6 +214,7 @@ Requires `files:read` or `files:write`. Reading what is inside a document needs |---|---| | `list_trip_files` | List a trip's documents: name, type, size, uploader, description, what they are linked to, starred and trash state. Pass `trash` to list the trash instead. | | `read_trip_file` | Read one document's contents. Text comes back as text, anything else base64, with an `encoding` field saying which. Files over 10 MB are refused; use the download link in the app. | +| `upload_trip_file` | Add a document to a trip, base64-encoded, up to 10 MB (or the instance's upload limit if lower). The name needs an extension the file manager accepts. Optionally attach it to a booking or place right away. Needs the file upload right. | | `update_trip_file` | Set a file's description and the booking, place or expense (`budget_item_id`, a receipt) it belongs to. Pass null to detach. | | `link_trip_file` | Link a file to one more booking, place, day assignment or expense (`budget_item_id`, a receipt). | | `unlink_trip_file` | Remove one link. The file stays. |