From 63338f0c409fa559eca2d3aaaa2acc7425c961c9 Mon Sep 17 00:00:00 2001 From: Ricardo Garim Date: Wed, 7 Jan 2026 22:28:45 -0300 Subject: [PATCH 01/32] refactor: remove router-level body parsing on form-data requests --- apps/meteor/app/api/server/definition.ts | 2 ++ apps/meteor/app/api/server/router.ts | 2 ++ packages/http-router/src/Router.ts | 6 ++++-- .../http-router/src/middlewares/honoAdapterForExpress.ts | 8 +++++++- 4 files changed, 15 insertions(+), 3 deletions(-) diff --git a/apps/meteor/app/api/server/definition.ts b/apps/meteor/app/api/server/definition.ts index 9684b99c798c8..b9384f1d13c04 100644 --- a/apps/meteor/app/api/server/definition.ts +++ b/apps/meteor/app/api/server/definition.ts @@ -184,6 +184,7 @@ export type ActionThis>; readonly request: Request; + readonly rawRequest?: any; readonly queryOperations: TOptions extends { queryOperations: infer T } ? T : never; readonly queryFields: TOptions extends { queryFields: infer T } ? T : never; @@ -310,6 +311,7 @@ export type TypedThis requestIp?: string; route: string; response: Response; + rawRequest?: any; }; type PromiseOrValue = T | Promise; diff --git a/apps/meteor/app/api/server/router.ts b/apps/meteor/app/api/server/router.ts index 1f18ea4c47228..f32260d1cb9aa 100644 --- a/apps/meteor/app/api/server/router.ts +++ b/apps/meteor/app/api/server/router.ts @@ -18,6 +18,7 @@ export type APIActionContext = { queryParams: Record; bodyParams: Record; request: Request; + rawRequest?: any; path: string; response: any; route: string; @@ -47,6 +48,7 @@ export class RocketChatAPIRouter< queryParams, bodyParams, request, + rawRequest: c.env?.incoming, path: req.path, response: res, route: req.routePath, diff --git a/packages/http-router/src/Router.ts b/packages/http-router/src/Router.ts index 7585cd0090224..46cd1e8bb3fd5 100644 --- a/packages/http-router/src/Router.ts +++ b/packages/http-router/src/Router.ts @@ -157,11 +157,13 @@ export class Router< if (contentType?.includes('application/json')) { parsedBody = await request.raw.clone().json(); - } else if (contentType?.includes('multipart/form-data')) { - parsedBody = await request.raw.clone().formData(); } else if (contentType?.includes('application/x-www-form-urlencoded')) { const req = await request.raw.clone().formData(); parsedBody = Object.fromEntries(req.entries()); + } else if (contentType?.includes('multipart/form-data')) { + // Don't parse multipart here, routes handle it manually via UploadService.parse() + // since multipart/form-data is only used for file uploads + parsedBody = {}; } else { parsedBody = await request.raw.clone().text(); } diff --git a/packages/http-router/src/middlewares/honoAdapterForExpress.ts b/packages/http-router/src/middlewares/honoAdapterForExpress.ts index b6f306bffecf9..65bc7bf543349 100644 --- a/packages/http-router/src/middlewares/honoAdapterForExpress.ts +++ b/packages/http-router/src/middlewares/honoAdapterForExpress.ts @@ -12,11 +12,17 @@ export const honoAdapterForExpress = (hono: Hono) => async (expressReq: Request, const { body, ...req } = expressReq; + // Don't pass body for multipart/form-data to avoid consuming the stream + // Routes will parse it manually via UploadService.parse() using rawRequest + const contentType = expressReq.headers['content-type']; + const isMultipart = contentType?.includes('multipart/form-data'); + const shouldPassBody = ['POST', 'PUT', 'DELETE'].includes(expressReq.method) && !isMultipart; + const honoRes = await hono.request( expressReq.originalUrl, { ...req, - ...(['POST', 'PUT', 'DELETE'].includes(expressReq.method) && { body: expressReq as unknown as ReadableStream }), + ...(shouldPassBody && { body: expressReq as unknown as ReadableStream }), headers: new Headers(Object.fromEntries(Object.entries(expressReq.headers)) as Record), }, { From 555290a89fae16d86b6b5c82267b3427897c8b6b Mon Sep 17 00:00:00 2001 From: Ricardo Garim Date: Wed, 7 Jan 2026 22:29:56 -0300 Subject: [PATCH 02/32] refactor: remove unnecessary buffering for file size check --- apps/meteor/server/ufs/ufs-store.ts | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/apps/meteor/server/ufs/ufs-store.ts b/apps/meteor/server/ufs/ufs-store.ts index 5eb44fdd2a137..e3237340b4a95 100644 --- a/apps/meteor/server/ufs/ufs-store.ts +++ b/apps/meteor/server/ufs/ufs-store.ts @@ -166,25 +166,16 @@ export class Store { }; const finishHandler = async () => { - let size = 0; - const readStream = await this.getReadStream(fileId, file); + if (file.complete) { + return; + } - readStream.on('error', (error: Error) => { - callback.call(this, error); - }); - readStream.on('data', (data) => { - size += data.length; - }); - readStream.on('end', async () => { - if (file.complete) { - return; - } + try { // Set file attribute file.complete = true; file.etag = UploadFS.generateEtag(); file.path = await this.getFileRelativeURL(fileId); file.progress = 1; - file.size = size; file.token = this.generateToken(); file.uploading = false; file.uploadedAt = new Date(); @@ -217,7 +208,9 @@ export class Store { // Return file info callback.call(this, undefined, file); - }); + } catch (error) { + callback.call(this, error as Error); + } }; const ws = await this.getWriteStream(fileId, file); From ae491a4a3349cda1663e69097fea938c41aefb47 Mon Sep 17 00:00:00 2001 From: Ricardo Garim Date: Wed, 7 Jan 2026 22:31:01 -0300 Subject: [PATCH 03/32] refactor: avoid re-buffering before file validation --- .../app/file-upload/server/lib/FileUpload.ts | 84 +++++++++++++------ 1 file changed, 57 insertions(+), 27 deletions(-) diff --git a/apps/meteor/app/file-upload/server/lib/FileUpload.ts b/apps/meteor/app/file-upload/server/lib/FileUpload.ts index 73f67fb473744..4413e867f57eb 100644 --- a/apps/meteor/app/file-upload/server/lib/FileUpload.ts +++ b/apps/meteor/app/file-upload/server/lib/FileUpload.ts @@ -789,51 +789,81 @@ export class FileUploadClass { return store.delete(file._id); } + private async _streamToTmpFile(inputStream: stream.Readable, targetPath: string): Promise { + const writeStream = fs.createWriteStream(targetPath); + + return new Promise((resolve, reject) => { + if ('isPaused' in inputStream && inputStream.isPaused()) { + inputStream.resume(); + } + inputStream.pipe(writeStream); + writeStream.on('finish', () => resolve()); + writeStream.on('error', (err) => reject(err)); + inputStream.on('error', (err) => reject(err)); + }); + } + + private async _validateFile( + fileData: OptionalId, + content: stream.Readable | Buffer | string, + ): Promise { + const filter = this.store.getFilter(); + if (!filter?.check) { + return content; + } + + if (content instanceof stream.Readable) { + // Currently, only the Slack Adapter passes a stream.Readable here + // We can't use _streamToTmpFile at this stage since the file hasn't been validated yet, + // and for security reasons we must not write it to disk before validation + content = await streamToBuffer(content); + } else if (content instanceof Uint8Array && !(content instanceof Buffer)) { + // Services compat - convert Uint8Array to Buffer + content = Buffer.from(content); + } + + try { + // TODO: Make filter.check accept file path as string - check Apps Engine IPreFileUpload hook + await filter.check(fileData, typeof content === 'string' ? undefined : content); + return content; + } catch (e) { + throw e; + } + } + async _doInsert( fileData: OptionalId, - streamOrBuffer: ReadableStream | stream | Buffer, + content: stream.Readable | Buffer | string, options?: { session?: ClientSession }, ): Promise { const fileId = await this.store.create(fileData, { session: options?.session }); const tmpFile = UploadFS.getTempFilePath(fileId); try { - if (streamOrBuffer instanceof stream) { - streamOrBuffer.pipe(fs.createWriteStream(tmpFile)); - } else if (streamOrBuffer instanceof Buffer) { - fs.writeFileSync(tmpFile, streamOrBuffer); + if (typeof content === 'string') { + await fs.promises.rename(content, tmpFile); + } else if (content instanceof Buffer) { + await fs.promises.writeFile(tmpFile, content); + } else if (content instanceof Uint8Array) { + await fs.promises.writeFile(tmpFile, Buffer.from(content)); + } else if (content instanceof stream.Readable) { + await this._streamToTmpFile(content, tmpFile); } else { throw new Error('Invalid file type'); } - const file = await ufsComplete(fileId, this.name, { session: options?.session }); - - return file; - } catch (e: any) { + return ufsComplete(fileId, this.name, { session: options?.session }); + } catch (e) { throw e; } } async insert( fileData: OptionalId, - streamOrBuffer: ReadableStream | stream.Readable | Buffer, + streamOrBuffer: stream.Readable | Buffer | string, options?: { session?: ClientSession }, - ) { - if (streamOrBuffer instanceof stream) { - streamOrBuffer = await streamToBuffer(streamOrBuffer); - } - - if (streamOrBuffer instanceof Uint8Array) { - // Services compat :) - streamOrBuffer = Buffer.from(streamOrBuffer); - } - - // Check if the fileData matches store filter - const filter = this.store.getFilter(); - if (filter?.check) { - await filter.check(fileData, streamOrBuffer); - } - - return this._doInsert(fileData, streamOrBuffer, { session: options?.session }); + ): Promise { + streamOrBuffer = await this._validateFile(fileData, streamOrBuffer); + return this._doInsert(fileData, streamOrBuffer, options); } } From 6e94ffcb9bb518c6e3d65e6fb985d8db7fbee758 Mon Sep 17 00:00:00 2001 From: Ricardo Garim Date: Wed, 7 Jan 2026 22:33:15 -0300 Subject: [PATCH 04/32] refactor: remove unneeded bufferToStream from initialData --- apps/meteor/server/startup/initialData.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/apps/meteor/server/startup/initialData.ts b/apps/meteor/server/startup/initialData.ts index be73e6ea2c79d..2503f4d47bae1 100644 --- a/apps/meteor/server/startup/initialData.ts +++ b/apps/meteor/server/startup/initialData.ts @@ -6,7 +6,6 @@ import { Accounts } from 'meteor/accounts-base'; import { Meteor } from 'meteor/meteor'; import { addCallHistoryTestData } from './callHistoryTestData'; -import { RocketChatFile } from '../../app/file/server'; import { FileUpload } from '../../app/file-upload/server'; import { addUserToDefaultChannels } from '../../app/lib/server/functions/addUserToDefaultChannels'; import { checkUsernameAvailability } from '../../app/lib/server/functions/checkUsernameAvailability'; @@ -146,7 +145,6 @@ Meteor.startup(async () => { if (asset) { const buffer = Buffer.from(asset); - const rs = RocketChatFile.bufferToStream(buffer); const fileStore = FileUpload.getStore('Avatars'); await fileStore.deleteByName('rocket.cat'); @@ -156,7 +154,7 @@ Meteor.startup(async () => { size: buffer.length, }; - const upload = await fileStore.insert(file, rs); + const upload = await fileStore.insert(file, buffer); await Users.setAvatarData('rocket.cat', 'local', upload.etag); } } From c266d9eb5304dbab4824ec074bba90aa52ee606f Mon Sep 17 00:00:00 2001 From: Ricardo Garim Date: Wed, 7 Jan 2026 22:34:29 -0300 Subject: [PATCH 05/32] refactor: add UploadService --- .../app/api/server/lib/UploadService.ts | 215 ++++++++++++++++++ 1 file changed, 215 insertions(+) create mode 100644 apps/meteor/app/api/server/lib/UploadService.ts diff --git a/apps/meteor/app/api/server/lib/UploadService.ts b/apps/meteor/app/api/server/lib/UploadService.ts new file mode 100644 index 0000000000000..d2f384b5ef4e5 --- /dev/null +++ b/apps/meteor/app/api/server/lib/UploadService.ts @@ -0,0 +1,215 @@ +import fs from 'fs'; +import type { IncomingMessage } from 'http'; +import type { Stream, Transform } from 'stream'; +import { Readable, pipeline } from 'stream'; +import { promisify } from 'util'; + +import { MeteorError } from '@rocket.chat/core-services'; +import { Random } from '@rocket.chat/random'; +import busboy from 'busboy'; +import ExifTransformer from 'exif-be-gone'; + +import { UploadFS } from '../../../../server/ufs'; +import { getMimeType } from '../../../utils/lib/mimeTypes'; + +const pipelineAsync = promisify(pipeline); + +export type ParsedUpload = { + tempFilePath: string; + filename: string; + mimetype: string; + size: number; + fieldname: string; +}; + +export type ParseOptions = { + field: string; + maxSize?: number; + allowedMimeTypes?: string[]; + transforms?: Transform[]; // Optional transform pipeline (e.g., EXIF stripping) + fileOptional?: boolean; +}; + +export class UploadService { + static transforms = { + stripExif(): Transform { + return new ExifTransformer(); + }, + }; + + static async cleanup(tempFilePath: string): Promise { + try { + await fs.promises.unlink(tempFilePath); + } catch (error: any) { + if (error.code !== 'ENOENT') { + console.warn(`[UploadService] Failed to cleanup temp file: ${tempFilePath}`, error); + } + } + } + + static async stripExifFromFile(tempFilePath: string): Promise { + const strippedPath = `${tempFilePath}.stripped`; + + try { + await pipelineAsync(fs.createReadStream(tempFilePath), new ExifTransformer(), fs.createWriteStream(strippedPath)); + + const stats = await fs.promises.stat(strippedPath); + + await fs.promises.rename(strippedPath, tempFilePath); + + return stats.size; + } catch (error) { + try { + await fs.promises.unlink(strippedPath); + } catch { + // Ignore cleanup errors + } + throw error; + } + } + + static async parse( + requestOrStream: IncomingMessage | Request, + options: ParseOptions, + ): Promise<{ file: ParsedUpload | null; fields: Record }> { + const limits: any = { files: 1 }; + if (options.maxSize && options.maxSize > 0) { + limits.fileSize = options.maxSize; + } + + const isIncomingMessage = 'socket' in requestOrStream; + const headers = isIncomingMessage + ? ((requestOrStream as IncomingMessage).headers as Record) + : Object.fromEntries((requestOrStream as Request).headers.entries()); + + const bb = busboy({ + headers, + defParamCharset: 'utf8', + limits, + }); + + const fields: Record = {}; + let parsedFile: ParsedUpload | null = null; + let busboyFinished = false; + let writeStreamFinished = options.fileOptional === true; + + const { promise, resolve, reject } = Promise.withResolvers<{ + file: ParsedUpload | null; + fields: Record; + }>(); + + const cleanupTempFile = (tempFilePath: string) => { + fs.unlink(tempFilePath, (err) => { + if (err) { + console.error(`[UploadService] Failed to cleanup temp file: ${tempFilePath}`, err); + } + }); + }; + + const tryResolve = () => { + if (busboyFinished && writeStreamFinished) { + if (!parsedFile && !options.fileOptional) { + return reject(new MeteorError('error-no-file', 'No file uploaded')); + } + resolve({ file: parsedFile, fields }); + } + }; + + bb.on('field', (fieldname: string, value: string) => { + fields[fieldname] = value; + }); + + bb.on('file', (fieldname, file, info) => { + const { filename, mimeType } = info; + + writeStreamFinished = false; + + if (options.field && fieldname !== options.field) { + file.resume(); + return reject(new MeteorError('invalid-field')); + } + + if (options.allowedMimeTypes && !options.allowedMimeTypes.includes(mimeType)) { + file.resume(); + return reject(new MeteorError('error-invalid-file-type', `File type ${mimeType} not allowed`)); + } + + const fileId = Random.id(); + const tempFilePath = UploadFS.getTempFilePath(fileId); + + const writeStream = fs.createWriteStream(tempFilePath); + let fileSize = 0; + + file.on('data', (chunk: Buffer) => { + fileSize += chunk.length; + }); + + let currentStream: Stream = file; + if (options.transforms?.length) { + for (const transform of options.transforms) { + currentStream = currentStream.pipe(transform); + } + } + + currentStream.pipe(writeStream); + + writeStream.on('finish', () => { + parsedFile = { + tempFilePath, + filename, + mimetype: getMimeType(mimeType, filename), + size: fileSize, + fieldname, + }; + writeStreamFinished = true; + tryResolve(); + }); + + writeStream.on('error', (err) => { + cleanupTempFile(tempFilePath); + reject(new MeteorError('error-file-upload', err.message)); + }); + + file.on('error', (err) => { + writeStream.destroy(); + cleanupTempFile(tempFilePath); + reject(err); + }); + }); + + bb.on('finish', () => { + busboyFinished = true; + tryResolve(); + }); + + bb.on('error', (err: any) => { + reject(new MeteorError('error-upload-failed', err.message)); + }); + + bb.on('filesLimit', () => { + reject('Just 1 file is allowed'); + }); + + bb.on('partsLimit', () => { + reject(new MeteorError('error-too-many-parts', 'Too many parts in upload')); + }); + + bb.on('fieldsLimit', () => { + reject(new MeteorError('error-too-many-fields', 'Too many fields in upload')); + }); + + if (isIncomingMessage) { + (requestOrStream as IncomingMessage).pipe(bb); + } else { + const fetchRequest = requestOrStream as Request; + if (!fetchRequest.body) { + return Promise.reject(new MeteorError('error-no-body', 'Request has no body')); + } + + const nodeStream = Readable.fromWeb(fetchRequest.body as any); + nodeStream.pipe(bb); + } + + return promise; + } +} From 164ba271e79d92c5ebf73cc89572abbbab3fa629 Mon Sep 17 00:00:00 2001 From: Ricardo Garim Date: Wed, 7 Jan 2026 22:34:59 -0300 Subject: [PATCH 06/32] refactor: adjust POST /rooms.media/:rid to use UploadService --- apps/meteor/app/api/server/v1/rooms.ts | 34 +++++++++----------------- 1 file changed, 12 insertions(+), 22 deletions(-) diff --git a/apps/meteor/app/api/server/v1/rooms.ts b/apps/meteor/app/api/server/v1/rooms.ts index f0c4bd6d51009..40be5ccdbd784 100644 --- a/apps/meteor/app/api/server/v1/rooms.ts +++ b/apps/meteor/app/api/server/v1/rooms.ts @@ -1,4 +1,4 @@ -import { FederationMatrix, Media, MeteorError, Team } from '@rocket.chat/core-services'; +import { FederationMatrix, MeteorError, Team } from '@rocket.chat/core-services'; import type { IRoom, IUpload } from '@rocket.chat/core-typings'; import { isPrivateRoom, isPublicRoom } from '@rocket.chat/core-typings'; import { Messages, Rooms, Users, Uploads, Subscriptions } from '@rocket.chat/models'; @@ -55,7 +55,7 @@ import { API } from '../api'; import { composeRoomWithLastMessage } from '../helpers/composeRoomWithLastMessage'; import { getPaginationItems } from '../helpers/getPaginationItems'; import { getUserFromParams } from '../helpers/getUserFromParams'; -import { getUploadFormData } from '../lib/getUploadFormData'; +import { UploadService } from '../lib/UploadService'; import { findAdminRoom, findAdminRooms, @@ -197,24 +197,20 @@ API.v1.addRoute( return API.v1.forbidden(); } - const file = await getUploadFormData( - { - request: this.request, - }, - { field: 'file', sizeLimit: settings.get('FileUpload_MaxFileSize') }, - ); + const stripExif = settings.get('Message_Attachments_Strip_Exif'); + const { file, fields } = await UploadService.parse(this.rawRequest, { + field: 'file', + maxSize: settings.get('FileUpload_MaxFileSize'), + ...(stripExif && { transforms: [UploadService.transforms.stripExif()] }), + }); if (!file) { - throw new Meteor.Error('invalid-field'); + throw new Meteor.Error('error-no-file-uploaded', 'No file was uploaded'); } - let { fileBuffer } = file; - const expiresAt = new Date(); expiresAt.setHours(expiresAt.getHours() + 24); - const { fields } = file; - let content; if (fields.content) { @@ -228,7 +224,7 @@ API.v1.addRoute( const details = { name: file.filename, - size: fileBuffer.length, + size: file.size, type: file.mimetype, rid: this.urlParams.rid, userId: this.userId, @@ -236,15 +232,9 @@ API.v1.addRoute( expiresAt, }; - const stripExif = settings.get('Message_Attachments_Strip_Exif'); - if (stripExif) { - // No need to check mime. Library will ignore any files without exif/xmp tags (like BMP, ico, PDF, etc) - fileBuffer = await Media.stripExifFromBuffer(fileBuffer); - details.size = fileBuffer.length; - } - + // TODO: In the future, we should isolate file receival from storage and post-processing. const fileStore = FileUpload.getStore('Uploads'); - const uploadedFile = await fileStore.insert(details, fileBuffer); + const uploadedFile = await fileStore.insert(details, file.tempFilePath); uploadedFile.path = FileUpload.getPath(`${uploadedFile._id}/${encodeURI(uploadedFile.name || '')}`); From cfbe5f693a60e1c2f70388217b92ca9e964ef1c5 Mon Sep 17 00:00:00 2001 From: Ricardo Garim Date: Wed, 7 Jan 2026 22:35:16 -0300 Subject: [PATCH 07/32] refactor: adjust rest of upload routes to use UploadService --- apps/meteor/app/api/server/v1/assets.ts | 23 +++++---- apps/meteor/app/api/server/v1/emoji-custom.ts | 51 ++++++++++--------- apps/meteor/app/api/server/v1/users.ts | 20 ++++---- .../livechat/imports/server/rest/upload.ts | 35 +++++-------- .../ee/server/apps/communication/rest.ts | 36 ++++++------- 5 files changed, 81 insertions(+), 84 deletions(-) diff --git a/apps/meteor/app/api/server/v1/assets.ts b/apps/meteor/app/api/server/v1/assets.ts index 2843cf8627d51..94af49b064bf4 100644 --- a/apps/meteor/app/api/server/v1/assets.ts +++ b/apps/meteor/app/api/server/v1/assets.ts @@ -1,3 +1,5 @@ +import { readFile } from 'fs/promises'; + import { Settings } from '@rocket.chat/models'; import { isAssetsUnsetAssetProps } from '@rocket.chat/rest-typings'; @@ -6,7 +8,7 @@ import { RocketChatAssets, refreshClients } from '../../../assets/server'; import { notifyOnSettingChangedById } from '../../../lib/server/lib/notifyListener'; import { settings } from '../../../settings/server'; import { API } from '../api'; -import { getUploadFormData } from '../lib/getUploadFormData'; +import { UploadService } from '../lib/UploadService'; API.v1.addRoute( 'assets.setAsset', @@ -16,18 +18,18 @@ API.v1.addRoute( }, { async post() { - const asset = await getUploadFormData( - { - request: this.request, - }, - { field: 'asset', sizeLimit: settings.get('FileUpload_MaxFileSize') }, - ); + const { file, fields } = await UploadService.parse(this.rawRequest, { + field: 'asset', + maxSize: settings.get('FileUpload_MaxFileSize'), + }); - const { fileBuffer, fields, filename, mimetype } = asset; + if (!file) { + throw new Error('No file was uploaded'); + } const { refreshAllClients, assetName: customName } = fields; - const assetName = customName || filename; + const assetName = customName || file.filename; const assetsKeys = Object.keys(RocketChatAssets.assets); const isValidAsset = assetsKeys.includes(assetName); @@ -35,7 +37,8 @@ API.v1.addRoute( throw new Error('Invalid asset'); } - const { key, value } = await RocketChatAssets.setAssetWithBuffer(fileBuffer, mimetype, assetName); + const fileBuffer = await readFile(file.tempFilePath); + const { key, value } = await RocketChatAssets.setAssetWithBuffer(fileBuffer, file.mimetype, assetName); const { modifiedCount } = await updateAuditedByUser({ _id: this.userId, diff --git a/apps/meteor/app/api/server/v1/emoji-custom.ts b/apps/meteor/app/api/server/v1/emoji-custom.ts index fa84d3cc6b995..b917db2fb76ce 100644 --- a/apps/meteor/app/api/server/v1/emoji-custom.ts +++ b/apps/meteor/app/api/server/v1/emoji-custom.ts @@ -1,3 +1,5 @@ +import { readFile } from 'fs/promises'; + import { Media } from '@rocket.chat/core-services'; import type { IEmojiCustom } from '@rocket.chat/core-typings'; import { EmojiCustom } from '@rocket.chat/models'; @@ -13,8 +15,8 @@ import { deleteEmojiCustom } from '../../../emoji-custom/server/methods/deleteEm import { settings } from '../../../settings/server'; import { API } from '../api'; import { getPaginationItems } from '../helpers/getPaginationItems'; +import { UploadService } from '../lib/UploadService'; import { findEmojisCustom } from '../lib/emoji-custom'; -import { getUploadFormData } from '../lib/getUploadFormData'; function validateDateParam(paramName: string, paramValue: string | undefined): Date | undefined { if (!paramValue) { @@ -108,21 +110,23 @@ API.v1.addRoute( { authRequired: true }, { async post() { - const emoji = await getUploadFormData( - { - request: this.request, - }, - { field: 'emoji', sizeLimit: settings.get('FileUpload_MaxFileSize') }, - ); + const { file, fields } = await UploadService.parse(this.rawRequest, { + field: 'emoji', + maxSize: settings.get('FileUpload_MaxFileSize'), + }); - const { fields, fileBuffer, mimetype } = emoji; + if (!file) { + throw new Meteor.Error('error-no-file-uploaded', 'No file was uploaded'); + } + + const fileBuffer = await readFile(file.tempFilePath); const isUploadable = await Media.isImage(fileBuffer); if (!isUploadable) { throw new Meteor.Error('emoji-is-not-image', "Emoji file provided cannot be uploaded since it's not an image"); } - const [, extension] = mimetype.split('/'); + const [, extension] = file.mimetype.split('/'); fields.extension = extension; try { @@ -134,7 +138,7 @@ API.v1.addRoute( extension: fields.extension, }); - await uploadEmojiCustomWithBuffer(this.userId, fileBuffer, mimetype, emojiData); + await uploadEmojiCustomWithBuffer(this.userId, fileBuffer, file.mimetype, emojiData); } catch (e) { SystemLogger.error(e); return API.v1.failure(); @@ -150,14 +154,11 @@ API.v1.addRoute( { authRequired: true }, { async post() { - const emoji = await getUploadFormData( - { - request: this.request, - }, - { field: 'emoji', sizeLimit: settings.get('FileUpload_MaxFileSize'), fileOptional: true }, - ); - - const { fields, fileBuffer, mimetype } = emoji; + const { file, fields } = await UploadService.parse(this.rawRequest, { + field: 'emoji', + maxSize: settings.get('FileUpload_MaxFileSize'), + fileOptional: true, + }); if (!fields._id) { throw new Meteor.Error('The required "_id" query param is missing.'); @@ -180,24 +181,26 @@ API.v1.addRoute( newFile: false, }; - const isNewFile = fileBuffer?.length && !!mimetype; + const isNewFile = !!file; if (isNewFile) { + const fileBuffer = await readFile(file.tempFilePath); + emojiData.newFile = isNewFile; const isUploadable = await Media.isImage(fileBuffer); if (!isUploadable) { throw new Meteor.Error('emoji-is-not-image', "Emoji file provided cannot be uploaded since it's not an image"); } - const [, extension] = mimetype.split('/'); + const [, extension] = file.mimetype.split('/'); emojiData.extension = extension; + + const updatedEmojiData = await insertOrUpdateEmoji(this.userId, emojiData); + await uploadEmojiCustomWithBuffer(this.userId, fileBuffer, file.mimetype, updatedEmojiData); } else { emojiData.extension = emojiToUpdate.extension; + await insertOrUpdateEmoji(this.userId, emojiData); } - const updatedEmojiData = await insertOrUpdateEmoji(this.userId, emojiData); - if (isNewFile) { - await uploadEmojiCustomWithBuffer(this.userId, fileBuffer, mimetype, updatedEmojiData); - } return API.v1.success(); }, }, diff --git a/apps/meteor/app/api/server/v1/users.ts b/apps/meteor/app/api/server/v1/users.ts index fd546426e84b5..a15a9ec19bf64 100644 --- a/apps/meteor/app/api/server/v1/users.ts +++ b/apps/meteor/app/api/server/v1/users.ts @@ -1,3 +1,5 @@ +import { readFile } from 'fs/promises'; + import { MeteorError, Team, api, Calendar } from '@rocket.chat/core-services'; import { type IExportOperation, type ILoginToken, type IPersonalAccessToken, type IUser, type UserStatus } from '@rocket.chat/core-typings'; import { Users, Subscriptions, Sessions } from '@rocket.chat/models'; @@ -75,7 +77,7 @@ import { API } from '../api'; import { getPaginationItems } from '../helpers/getPaginationItems'; import { getUserFromParams } from '../helpers/getUserFromParams'; import { isUserFromParams } from '../helpers/isUserFromParams'; -import { getUploadFormData } from '../lib/getUploadFormData'; +import { UploadService } from '../lib/UploadService'; import { isValidQuery } from '../lib/isValidQuery'; import { findPaginatedUsersByStatus, findUsersToAutocomplete, getInclusiveFields, getNonEmptyFields, getNonEmptyQuery } from '../lib/users'; @@ -270,19 +272,15 @@ API.v1.addRoute( return API.v1.success(); } - const image = await getUploadFormData( - { - request: this.request, - }, - { field: 'image', sizeLimit: settings.get('FileUpload_MaxFileSize') }, - ); + const { file, fields } = await UploadService.parse(this.rawRequest, { + field: 'image', + maxSize: settings.get('FileUpload_MaxFileSize'), + }); - if (!image) { + if (!file) { return API.v1.failure("The 'image' param is required"); } - const { fields, fileBuffer, mimetype } = image; - const sentTheUserByFormData = fields.userId || fields.username; if (sentTheUserByFormData) { if (fields.userId) { @@ -303,7 +301,7 @@ API.v1.addRoute( } } - await setUserAvatar(user, fileBuffer, mimetype, 'rest'); + await setUserAvatar(user, await readFile(file.tempFilePath), file.mimetype, 'rest'); return API.v1.success(); }, diff --git a/apps/meteor/app/livechat/imports/server/rest/upload.ts b/apps/meteor/app/livechat/imports/server/rest/upload.ts index 8cb0a0511eade..9558132b9663d 100644 --- a/apps/meteor/app/livechat/imports/server/rest/upload.ts +++ b/apps/meteor/app/livechat/imports/server/rest/upload.ts @@ -1,8 +1,7 @@ import { LivechatVisitors, LivechatRooms } from '@rocket.chat/models'; -import filesize from 'filesize'; import { API } from '../../../../api/server'; -import { getUploadFormData } from '../../../../api/server/lib/getUploadFormData'; +import { UploadService } from '../../../../api/server/lib/UploadService'; import { FileUpload } from '../../../../file-upload/server'; import { settings } from '../../../../settings/server'; import { fileUploadIsValidContentType } from '../../../../utils/server/restrictions'; @@ -36,42 +35,34 @@ API.v1.addRoute('livechat/upload/:rid', { const maxFileSize = settings.get('FileUpload_MaxFileSize') || 104857600; - const file = await getUploadFormData( - { - request: this.request, - }, - { field: 'file', sizeLimit: maxFileSize }, - ); + const { file, fields } = await UploadService.parse(this.rawRequest, { + field: 'file', + maxSize: maxFileSize > -1 ? maxFileSize : undefined, + }); - const { fields, fileBuffer, filename, mimetype } = file; - - if (!fileUploadIsValidContentType(mimetype)) { + if (!file) { return API.v1.failure({ - reason: 'error-type-not-allowed', + reason: 'error-no-file-uploaded', }); } - const buffLength = fileBuffer.length; - - // -1 maxFileSize means there is no limit - if (maxFileSize > -1 && buffLength > maxFileSize) { + if (!fileUploadIsValidContentType(file.mimetype)) { return API.v1.failure({ - reason: 'error-size-not-allowed', - sizeAllowed: filesize(maxFileSize), + reason: 'error-type-not-allowed', }); } const fileStore = FileUpload.getStore('Uploads'); const details = { - name: filename, - size: buffLength, - type: mimetype, + name: file.filename, + size: file.size, + type: file.mimetype, rid: this.urlParams.rid, visitorToken, }; - const uploadedFile = await fileStore.insert(details, fileBuffer); + const uploadedFile = await fileStore.insert(details, file.tempFilePath); if (!uploadedFile) { return API.v1.failure('Invalid file'); } diff --git a/apps/meteor/ee/server/apps/communication/rest.ts b/apps/meteor/ee/server/apps/communication/rest.ts index 95436789a11f6..7a3d31f12fc6f 100644 --- a/apps/meteor/ee/server/apps/communication/rest.ts +++ b/apps/meteor/ee/server/apps/communication/rest.ts @@ -1,3 +1,5 @@ +import { readFile } from 'fs/promises'; + import { AppStatus, AppStatusUtils } from '@rocket.chat/apps-engine/definition/AppStatus'; import type { IAppInfo } from '@rocket.chat/apps-engine/definition/metadata'; import type { AppManager } from '@rocket.chat/apps-engine/server/AppManager'; @@ -19,7 +21,7 @@ import { registerAppLogsHandler } from './endpoints/appLogsHandler'; import { registerAppsCountHandler } from './endpoints/appsCountHandler'; import { API } from '../../../../app/api/server'; import type { APIClass } from '../../../../app/api/server/ApiClass'; -import { getUploadFormData } from '../../../../app/api/server/lib/getUploadFormData'; +import { UploadService } from '../../../../app/api/server/lib/UploadService'; import { loggerMiddleware } from '../../../../app/api/server/middlewares/logger'; import { metricsMiddleware } from '../../../../app/api/server/middlewares/metrics'; import { tracerSpanMiddleware } from '../../../../app/api/server/middlewares/tracer'; @@ -326,16 +328,16 @@ export class AppsRestApi { return API.v1.failure({ error: message }); } } else { - const app = await getUploadFormData( - { - request: this.request, - }, - { field: 'app', sizeLimit: settings.get('FileUpload_MaxFileSize') }, - ); + const { file, fields: formData } = await UploadService.parse(this.rawRequest, { + field: 'app', + maxSize: settings.get('FileUpload_MaxFileSize'), + }); - const { fields: formData } = app; + if (!file) { + return API.v1.failure({ error: 'No file was uploaded' }); + } - buff = app.fileBuffer; + buff = await readFile(file.tempFilePath); permissionsGranted = (() => { try { const permissions = JSON.parse(formData?.permissions || ''); @@ -831,16 +833,16 @@ export class AppsRestApi { } else { isPrivateAppUpload = true; - const app = await getUploadFormData( - { - request: this.request, - }, - { field: 'app', sizeLimit: settings.get('FileUpload_MaxFileSize') }, - ); + const { file, fields: formData } = await UploadService.parse(this.rawRequest, { + field: 'app', + maxSize: settings.get('FileUpload_MaxFileSize'), + }); - const { fields: formData } = app; + if (!file) { + return API.v1.failure({ error: 'No file was uploaded' }); + } - buff = app.fileBuffer; + buff = await readFile(file.tempFilePath); permissionsGranted = (() => { try { const permissions = JSON.parse(formData?.permissions || ''); From 8f55d5236e314d7245e7cb94cb248acee2d3cda0 Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Thu, 8 Jan 2026 15:14:47 -0300 Subject: [PATCH 08/32] refactor: use builtin finished api to await the pipe --- .../app/file-upload/server/lib/FileUpload.ts | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/apps/meteor/app/file-upload/server/lib/FileUpload.ts b/apps/meteor/app/file-upload/server/lib/FileUpload.ts index 4413e867f57eb..58b3bcd5be250 100644 --- a/apps/meteor/app/file-upload/server/lib/FileUpload.ts +++ b/apps/meteor/app/file-upload/server/lib/FileUpload.ts @@ -5,6 +5,7 @@ import { unlink, rename, writeFile } from 'fs/promises'; import type * as http from 'http'; import type * as https from 'https'; import stream from 'stream'; +import { finished } from 'stream/promises'; import URL from 'url'; import { hashLoginToken } from '@rocket.chat/account-utils'; @@ -789,20 +790,6 @@ export class FileUploadClass { return store.delete(file._id); } - private async _streamToTmpFile(inputStream: stream.Readable, targetPath: string): Promise { - const writeStream = fs.createWriteStream(targetPath); - - return new Promise((resolve, reject) => { - if ('isPaused' in inputStream && inputStream.isPaused()) { - inputStream.resume(); - } - inputStream.pipe(writeStream); - writeStream.on('finish', () => resolve()); - writeStream.on('error', (err) => reject(err)); - inputStream.on('error', (err) => reject(err)); - }); - } - private async _validateFile( fileData: OptionalId, content: stream.Readable | Buffer | string, @@ -847,7 +834,7 @@ export class FileUploadClass { } else if (content instanceof Uint8Array) { await fs.promises.writeFile(tmpFile, Buffer.from(content)); } else if (content instanceof stream.Readable) { - await this._streamToTmpFile(content, tmpFile); + await finished(content.pipe(fs.createWriteStream(tmpFile)), { cleanup: true }); } else { throw new Error('Invalid file type'); } From c4af8ce517ecef22a02ae7e950666acfdc5b88fe Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Thu, 8 Jan 2026 15:21:03 -0300 Subject: [PATCH 09/32] refactor: streamline removeTempFile in ufsComplete --- apps/meteor/server/ufs/ufs-methods.ts | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/apps/meteor/server/ufs/ufs-methods.ts b/apps/meteor/server/ufs/ufs-methods.ts index 768aefdcee3dc..f1c495e98ef36 100644 --- a/apps/meteor/server/ufs/ufs-methods.ts +++ b/apps/meteor/server/ufs/ufs-methods.ts @@ -19,14 +19,10 @@ export async function ufsComplete(fileId: string, storeName: string, options?: { const tmpFile = UploadFS.getTempFilePath(fileId); - const removeTempFile = function () { - fs.stat(tmpFile, (err) => { - !err && - fs.unlink(tmpFile, (err2) => { - err2 && console.error(`ufs: cannot delete temp file "${tmpFile}" (${err2.message})`); - }); + const removeTempFile = () => + fs.promises.unlink(tmpFile).catch(() => { + console.warn(`[ufsComplete] Failed to remove temp file: ${tmpFile}`); }); - }; return new Promise(async (resolve, reject) => { try { @@ -61,7 +57,7 @@ export async function ufsComplete(fileId: string, storeName: string, options?: { rs, fileId, (err, file) => { - removeTempFile(); + void removeTempFile(); if (err) { return reject(err); @@ -76,7 +72,7 @@ export async function ufsComplete(fileId: string, storeName: string, options?: { } catch (err: any) { // If write failed, remove the file await store.removeById(fileId, { session: options?.session }); - // removeTempFile(); // todo remove temp file on error or try again ? + void removeTempFile(); reject(new Meteor.Error('ufs: cannot upload file', err)); } }); From 1e8f712ff016f5da47de63ce162941444d47ad13 Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Thu, 8 Jan 2026 15:26:11 -0300 Subject: [PATCH 10/32] refactor: stripExifFromFile stop calling fs.stat --- apps/meteor/app/api/server/lib/UploadService.ts | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/apps/meteor/app/api/server/lib/UploadService.ts b/apps/meteor/app/api/server/lib/UploadService.ts index d2f384b5ef4e5..bac56bfff7c8d 100644 --- a/apps/meteor/app/api/server/lib/UploadService.ts +++ b/apps/meteor/app/api/server/lib/UploadService.ts @@ -51,19 +51,16 @@ export class UploadService { const strippedPath = `${tempFilePath}.stripped`; try { - await pipelineAsync(fs.createReadStream(tempFilePath), new ExifTransformer(), fs.createWriteStream(strippedPath)); + const writeStream = fs.createWriteStream(strippedPath); - const stats = await fs.promises.stat(strippedPath); + await pipelineAsync(fs.createReadStream(tempFilePath), new ExifTransformer(), writeStream); await fs.promises.rename(strippedPath, tempFilePath); - return stats.size; + return writeStream.bytesWritten; } catch (error) { - try { - await fs.promises.unlink(strippedPath); - } catch { - // Ignore cleanup errors - } + void this.cleanup(strippedPath); + throw error; } } From 2c141d36f2f6c2704e94b74bdf3dda878e8fd0b8 Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Thu, 8 Jan 2026 15:26:40 -0300 Subject: [PATCH 11/32] refactor: always warn if cleanup fails --- apps/meteor/app/api/server/lib/UploadService.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/apps/meteor/app/api/server/lib/UploadService.ts b/apps/meteor/app/api/server/lib/UploadService.ts index bac56bfff7c8d..e0c39b9688738 100644 --- a/apps/meteor/app/api/server/lib/UploadService.ts +++ b/apps/meteor/app/api/server/lib/UploadService.ts @@ -41,9 +41,7 @@ export class UploadService { try { await fs.promises.unlink(tempFilePath); } catch (error: any) { - if (error.code !== 'ENOENT') { - console.warn(`[UploadService] Failed to cleanup temp file: ${tempFilePath}`, error); - } + console.warn(`[UploadService] Failed to cleanup temp file: ${tempFilePath}`, error); } } From 14aa0cdb145e4efa991e5807413ca01763dd0894 Mon Sep 17 00:00:00 2001 From: Ricardo Garim Date: Thu, 8 Jan 2026 18:52:39 -0300 Subject: [PATCH 12/32] test: adjust router test to comply with new form-data behavior --- packages/http-router/src/Router.spec.ts | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/packages/http-router/src/Router.spec.ts b/packages/http-router/src/Router.spec.ts index cf7616d2d89ec..cedaa083925fb 100644 --- a/packages/http-router/src/Router.spec.ts +++ b/packages/http-router/src/Router.spec.ts @@ -446,7 +446,7 @@ describe('Router', () => { }); describe('Content types', () => { - it('should handle different content types for requests', async () => { + it('should not auto-parse multipart/form-data and provide raw request for manual parsing', async () => { const app = express(); const api = new Router('/api'); @@ -458,12 +458,18 @@ describe('Router', () => { }, }, async (c) => { - const formData = await c.req.formData(); - const name = formData.get('name'); + // For multipart/form-data, routes use c.env.incoming for manual parsing + // In production, routes call UploadService.parse(c.env.incoming) + const hasIncoming = !!c.env.incoming; + const contentType = c.env.incoming?.headers['content-type']; + const isMultipart = contentType?.includes('multipart/form-data'); return { statusCode: 200, - body: { received: { name } }, + body: { + receivedRawRequest: hasIncoming, + receivedIncoming: isMultipart, + }, }; }, ); @@ -473,7 +479,10 @@ describe('Router', () => { const response = await request(app).post('/api/form-data').field('name', 'Test User'); expect(response.status).toBe(200); - expect(response.body).toEqual({ received: { name: 'Test User' } }); + expect(response.body).toEqual({ + receivedRawRequest: true, + receivedIncoming: true, + }); }); it('should set custom response headers', async () => { From 99779e9c6bbae696dc4820eddbd1582f6e235543 Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Thu, 8 Jan 2026 21:36:04 -0300 Subject: [PATCH 13/32] refactor: remove unnecessary stat call in ufs-store --- apps/meteor/server/ufs/ufs-local.spec.ts | 4 ++-- apps/meteor/server/ufs/ufs-store.ts | 7 ++----- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/apps/meteor/server/ufs/ufs-local.spec.ts b/apps/meteor/server/ufs/ufs-local.spec.ts index b2eba7bf473e5..6fed2a129fae7 100644 --- a/apps/meteor/server/ufs/ufs-local.spec.ts +++ b/apps/meteor/server/ufs/ufs-local.spec.ts @@ -99,8 +99,8 @@ describe('LocalStore', () => { it('should not throw if file does not exist (ENOENT)', async () => { unlinkStub.rejects(Object.assign(new Error('not found'), { code: 'ENOENT' })); await expect(store.delete('test')).to.be.fulfilled; - // Should only call unlink once - expect(unlinkStub.calledOnce).to.be.true; + // unlink is called twice: once for the temp file, once for the actual file + expect(unlinkStub.calledTwice).to.be.true; }); it('should throw if unlink fails with non-ENOENT error', async () => { diff --git a/apps/meteor/server/ufs/ufs-store.ts b/apps/meteor/server/ufs/ufs-store.ts index e3237340b4a95..ab775a4a4f304 100644 --- a/apps/meteor/server/ufs/ufs-store.ts +++ b/apps/meteor/server/ufs/ufs-store.ts @@ -231,11 +231,8 @@ export class Store { const tmpFile = UploadFS.getTempFilePath(fileId); // Delete the temp file - fs.stat(tmpFile, (err) => { - !err && - fs.unlink(tmpFile, (err2) => { - err2 && console.error(`ufs: cannot delete temp file at ${tmpFile} (${err2.message})`); - }); + await fs.promises.unlink(tmpFile).catch((err) => { + err?.code !== 'ENOENT' && console.error(`ufs: cannot delete temp file at ${tmpFile} (${err.message})`); }); await this.getCollection().removeById(fileId, { session: options?.session }); From 0a9f08cc704ba431227d1a3b3466c50f75a17f1a Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Thu, 8 Jan 2026 22:24:39 -0300 Subject: [PATCH 14/32] refactor: use pipeline promises --- apps/meteor/app/api/server/lib/UploadService.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/apps/meteor/app/api/server/lib/UploadService.ts b/apps/meteor/app/api/server/lib/UploadService.ts index e0c39b9688738..a616a8b572817 100644 --- a/apps/meteor/app/api/server/lib/UploadService.ts +++ b/apps/meteor/app/api/server/lib/UploadService.ts @@ -1,8 +1,8 @@ import fs from 'fs'; import type { IncomingMessage } from 'http'; import type { Stream, Transform } from 'stream'; -import { Readable, pipeline } from 'stream'; -import { promisify } from 'util'; +import { Readable } from 'stream'; +import { pipeline } from 'stream/promises'; import { MeteorError } from '@rocket.chat/core-services'; import { Random } from '@rocket.chat/random'; @@ -12,8 +12,6 @@ import ExifTransformer from 'exif-be-gone'; import { UploadFS } from '../../../../server/ufs'; import { getMimeType } from '../../../utils/lib/mimeTypes'; -const pipelineAsync = promisify(pipeline); - export type ParsedUpload = { tempFilePath: string; filename: string; @@ -51,7 +49,7 @@ export class UploadService { try { const writeStream = fs.createWriteStream(strippedPath); - await pipelineAsync(fs.createReadStream(tempFilePath), new ExifTransformer(), writeStream); + await pipeline(fs.createReadStream(tempFilePath), new ExifTransformer(), writeStream); await fs.promises.rename(strippedPath, tempFilePath); From 135d4b4acf2805977336fb49a7f5ef4d6c66e26e Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Thu, 8 Jan 2026 22:25:41 -0300 Subject: [PATCH 15/32] refactor: get file size from fs.WriteStream --- apps/meteor/app/api/server/lib/UploadService.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/apps/meteor/app/api/server/lib/UploadService.ts b/apps/meteor/app/api/server/lib/UploadService.ts index a616a8b572817..fc8939045a18d 100644 --- a/apps/meteor/app/api/server/lib/UploadService.ts +++ b/apps/meteor/app/api/server/lib/UploadService.ts @@ -131,11 +131,6 @@ export class UploadService { const tempFilePath = UploadFS.getTempFilePath(fileId); const writeStream = fs.createWriteStream(tempFilePath); - let fileSize = 0; - - file.on('data', (chunk: Buffer) => { - fileSize += chunk.length; - }); let currentStream: Stream = file; if (options.transforms?.length) { @@ -151,7 +146,7 @@ export class UploadService { tempFilePath, filename, mimetype: getMimeType(mimeType, filename), - size: fileSize, + size: writeStream.bytesWritten, fieldname, }; writeStreamFinished = true; From c85eabce4f31915de7d4652d77fe380c24920f41 Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Thu, 8 Jan 2026 22:26:25 -0300 Subject: [PATCH 16/32] fix: reject on file truncated as previous implementation --- apps/meteor/app/api/server/lib/UploadService.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/apps/meteor/app/api/server/lib/UploadService.ts b/apps/meteor/app/api/server/lib/UploadService.ts index fc8939045a18d..b12007e271722 100644 --- a/apps/meteor/app/api/server/lib/UploadService.ts +++ b/apps/meteor/app/api/server/lib/UploadService.ts @@ -142,6 +142,11 @@ export class UploadService { currentStream.pipe(writeStream); writeStream.on('finish', () => { + if (file.truncated) { + cleanupTempFile(tempFilePath); + return reject(new MeteorError('error-file-too-large', 'File size exceeds the allowed limit')); + } + parsedFile = { tempFilePath, filename, From 96654125fc6b35113df6ca97321f1cbc526c932b Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Thu, 8 Jan 2026 22:34:42 -0300 Subject: [PATCH 17/32] refactor: reuse static cleanup function --- apps/meteor/app/api/server/lib/UploadService.ts | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/apps/meteor/app/api/server/lib/UploadService.ts b/apps/meteor/app/api/server/lib/UploadService.ts index b12007e271722..f511004b39a0a 100644 --- a/apps/meteor/app/api/server/lib/UploadService.ts +++ b/apps/meteor/app/api/server/lib/UploadService.ts @@ -91,14 +91,6 @@ export class UploadService { fields: Record; }>(); - const cleanupTempFile = (tempFilePath: string) => { - fs.unlink(tempFilePath, (err) => { - if (err) { - console.error(`[UploadService] Failed to cleanup temp file: ${tempFilePath}`, err); - } - }); - }; - const tryResolve = () => { if (busboyFinished && writeStreamFinished) { if (!parsedFile && !options.fileOptional) { @@ -143,7 +135,7 @@ export class UploadService { writeStream.on('finish', () => { if (file.truncated) { - cleanupTempFile(tempFilePath); + void this.cleanup(tempFilePath); return reject(new MeteorError('error-file-too-large', 'File size exceeds the allowed limit')); } @@ -159,13 +151,13 @@ export class UploadService { }); writeStream.on('error', (err) => { - cleanupTempFile(tempFilePath); + void this.cleanup(tempFilePath); reject(new MeteorError('error-file-upload', err.message)); }); file.on('error', (err) => { writeStream.destroy(); - cleanupTempFile(tempFilePath); + void this.cleanup(tempFilePath); reject(err); }); }); From f202c2242f0b5369a48a971d9765970a54b2d550 Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Fri, 9 Jan 2026 13:43:55 -0300 Subject: [PATCH 18/32] refactor: remove `rawRequest` from route context in favor of existing request --- apps/meteor/app/api/server/definition.ts | 2 -- apps/meteor/app/api/server/router.ts | 2 -- apps/meteor/app/api/server/v1/assets.ts | 2 +- apps/meteor/app/api/server/v1/emoji-custom.ts | 4 ++-- apps/meteor/app/api/server/v1/rooms.ts | 2 +- apps/meteor/app/api/server/v1/users.ts | 2 +- apps/meteor/app/livechat/imports/server/rest/upload.ts | 2 +- apps/meteor/ee/server/apps/communication/rest.ts | 4 ++-- .../http-router/src/middlewares/honoAdapterForExpress.ts | 8 +------- 9 files changed, 9 insertions(+), 19 deletions(-) diff --git a/apps/meteor/app/api/server/definition.ts b/apps/meteor/app/api/server/definition.ts index b9384f1d13c04..9684b99c798c8 100644 --- a/apps/meteor/app/api/server/definition.ts +++ b/apps/meteor/app/api/server/definition.ts @@ -184,7 +184,6 @@ export type ActionThis>; readonly request: Request; - readonly rawRequest?: any; readonly queryOperations: TOptions extends { queryOperations: infer T } ? T : never; readonly queryFields: TOptions extends { queryFields: infer T } ? T : never; @@ -311,7 +310,6 @@ export type TypedThis requestIp?: string; route: string; response: Response; - rawRequest?: any; }; type PromiseOrValue = T | Promise; diff --git a/apps/meteor/app/api/server/router.ts b/apps/meteor/app/api/server/router.ts index f32260d1cb9aa..1f18ea4c47228 100644 --- a/apps/meteor/app/api/server/router.ts +++ b/apps/meteor/app/api/server/router.ts @@ -18,7 +18,6 @@ export type APIActionContext = { queryParams: Record; bodyParams: Record; request: Request; - rawRequest?: any; path: string; response: any; route: string; @@ -48,7 +47,6 @@ export class RocketChatAPIRouter< queryParams, bodyParams, request, - rawRequest: c.env?.incoming, path: req.path, response: res, route: req.routePath, diff --git a/apps/meteor/app/api/server/v1/assets.ts b/apps/meteor/app/api/server/v1/assets.ts index 94af49b064bf4..2d30a545a471e 100644 --- a/apps/meteor/app/api/server/v1/assets.ts +++ b/apps/meteor/app/api/server/v1/assets.ts @@ -18,7 +18,7 @@ API.v1.addRoute( }, { async post() { - const { file, fields } = await UploadService.parse(this.rawRequest, { + const { file, fields } = await UploadService.parse(this.request, { field: 'asset', maxSize: settings.get('FileUpload_MaxFileSize'), }); diff --git a/apps/meteor/app/api/server/v1/emoji-custom.ts b/apps/meteor/app/api/server/v1/emoji-custom.ts index b917db2fb76ce..0cfb74f19b660 100644 --- a/apps/meteor/app/api/server/v1/emoji-custom.ts +++ b/apps/meteor/app/api/server/v1/emoji-custom.ts @@ -110,7 +110,7 @@ API.v1.addRoute( { authRequired: true }, { async post() { - const { file, fields } = await UploadService.parse(this.rawRequest, { + const { file, fields } = await UploadService.parse(this.request, { field: 'emoji', maxSize: settings.get('FileUpload_MaxFileSize'), }); @@ -154,7 +154,7 @@ API.v1.addRoute( { authRequired: true }, { async post() { - const { file, fields } = await UploadService.parse(this.rawRequest, { + const { file, fields } = await UploadService.parse(this.request, { field: 'emoji', maxSize: settings.get('FileUpload_MaxFileSize'), fileOptional: true, diff --git a/apps/meteor/app/api/server/v1/rooms.ts b/apps/meteor/app/api/server/v1/rooms.ts index 40be5ccdbd784..3c4a11f9e31cd 100644 --- a/apps/meteor/app/api/server/v1/rooms.ts +++ b/apps/meteor/app/api/server/v1/rooms.ts @@ -198,7 +198,7 @@ API.v1.addRoute( } const stripExif = settings.get('Message_Attachments_Strip_Exif'); - const { file, fields } = await UploadService.parse(this.rawRequest, { + const { file, fields } = await UploadService.parse(this.request, { field: 'file', maxSize: settings.get('FileUpload_MaxFileSize'), ...(stripExif && { transforms: [UploadService.transforms.stripExif()] }), diff --git a/apps/meteor/app/api/server/v1/users.ts b/apps/meteor/app/api/server/v1/users.ts index a15a9ec19bf64..0c9c701932f3d 100644 --- a/apps/meteor/app/api/server/v1/users.ts +++ b/apps/meteor/app/api/server/v1/users.ts @@ -272,7 +272,7 @@ API.v1.addRoute( return API.v1.success(); } - const { file, fields } = await UploadService.parse(this.rawRequest, { + const { file, fields } = await UploadService.parse(this.request, { field: 'image', maxSize: settings.get('FileUpload_MaxFileSize'), }); diff --git a/apps/meteor/app/livechat/imports/server/rest/upload.ts b/apps/meteor/app/livechat/imports/server/rest/upload.ts index 9558132b9663d..eb79babf1635b 100644 --- a/apps/meteor/app/livechat/imports/server/rest/upload.ts +++ b/apps/meteor/app/livechat/imports/server/rest/upload.ts @@ -35,7 +35,7 @@ API.v1.addRoute('livechat/upload/:rid', { const maxFileSize = settings.get('FileUpload_MaxFileSize') || 104857600; - const { file, fields } = await UploadService.parse(this.rawRequest, { + const { file, fields } = await UploadService.parse(this.request, { field: 'file', maxSize: maxFileSize > -1 ? maxFileSize : undefined, }); diff --git a/apps/meteor/ee/server/apps/communication/rest.ts b/apps/meteor/ee/server/apps/communication/rest.ts index 7a3d31f12fc6f..1160f57c4b9c2 100644 --- a/apps/meteor/ee/server/apps/communication/rest.ts +++ b/apps/meteor/ee/server/apps/communication/rest.ts @@ -328,7 +328,7 @@ export class AppsRestApi { return API.v1.failure({ error: message }); } } else { - const { file, fields: formData } = await UploadService.parse(this.rawRequest, { + const { file, fields: formData } = await UploadService.parse(this.request, { field: 'app', maxSize: settings.get('FileUpload_MaxFileSize'), }); @@ -833,7 +833,7 @@ export class AppsRestApi { } else { isPrivateAppUpload = true; - const { file, fields: formData } = await UploadService.parse(this.rawRequest, { + const { file, fields: formData } = await UploadService.parse(this.request, { field: 'app', maxSize: settings.get('FileUpload_MaxFileSize'), }); diff --git a/packages/http-router/src/middlewares/honoAdapterForExpress.ts b/packages/http-router/src/middlewares/honoAdapterForExpress.ts index 65bc7bf543349..b6f306bffecf9 100644 --- a/packages/http-router/src/middlewares/honoAdapterForExpress.ts +++ b/packages/http-router/src/middlewares/honoAdapterForExpress.ts @@ -12,17 +12,11 @@ export const honoAdapterForExpress = (hono: Hono) => async (expressReq: Request, const { body, ...req } = expressReq; - // Don't pass body for multipart/form-data to avoid consuming the stream - // Routes will parse it manually via UploadService.parse() using rawRequest - const contentType = expressReq.headers['content-type']; - const isMultipart = contentType?.includes('multipart/form-data'); - const shouldPassBody = ['POST', 'PUT', 'DELETE'].includes(expressReq.method) && !isMultipart; - const honoRes = await hono.request( expressReq.originalUrl, { ...req, - ...(shouldPassBody && { body: expressReq as unknown as ReadableStream }), + ...(['POST', 'PUT', 'DELETE'].includes(expressReq.method) && { body: expressReq as unknown as ReadableStream }), headers: new Headers(Object.fromEntries(Object.entries(expressReq.headers)) as Record), }, { From 2e21cea60e1cb24819c70fe3347d590df35d27a1 Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Fri, 9 Jan 2026 17:47:20 -0300 Subject: [PATCH 19/32] refactor: array view validation before writeFile --- apps/meteor/app/file-upload/server/lib/FileUpload.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/apps/meteor/app/file-upload/server/lib/FileUpload.ts b/apps/meteor/app/file-upload/server/lib/FileUpload.ts index 58b3bcd5be250..96db59db4c661 100644 --- a/apps/meteor/app/file-upload/server/lib/FileUpload.ts +++ b/apps/meteor/app/file-upload/server/lib/FileUpload.ts @@ -7,6 +7,7 @@ import type * as https from 'https'; import stream from 'stream'; import { finished } from 'stream/promises'; import URL from 'url'; +import { isArrayBufferView } from 'util/types'; import { hashLoginToken } from '@rocket.chat/account-utils'; import { Apps, AppEvents } from '@rocket.chat/apps'; @@ -829,10 +830,8 @@ export class FileUploadClass { try { if (typeof content === 'string') { await fs.promises.rename(content, tmpFile); - } else if (content instanceof Buffer) { + } else if (isArrayBufferView(content)) { await fs.promises.writeFile(tmpFile, content); - } else if (content instanceof Uint8Array) { - await fs.promises.writeFile(tmpFile, Buffer.from(content)); } else if (content instanceof stream.Readable) { await finished(content.pipe(fs.createWriteStream(tmpFile)), { cleanup: true }); } else { From 9b92e5aaf58ab042ab0a30caf8f1e00f924cec83 Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Fri, 9 Jan 2026 18:49:15 -0300 Subject: [PATCH 20/32] fix: move exif stripping to after image rotation --- apps/meteor/app/api/server/v1/rooms.ts | 2 -- .../file-upload/server/lib/FileUpload.spec.ts | 1 + .../app/file-upload/server/lib/FileUpload.ts | 33 ++++++++++++------- apps/meteor/tests/e2e/image-upload.spec.ts | 11 +++++-- 4 files changed, 31 insertions(+), 16 deletions(-) diff --git a/apps/meteor/app/api/server/v1/rooms.ts b/apps/meteor/app/api/server/v1/rooms.ts index 3c4a11f9e31cd..efc8e12a0de07 100644 --- a/apps/meteor/app/api/server/v1/rooms.ts +++ b/apps/meteor/app/api/server/v1/rooms.ts @@ -197,11 +197,9 @@ API.v1.addRoute( return API.v1.forbidden(); } - const stripExif = settings.get('Message_Attachments_Strip_Exif'); const { file, fields } = await UploadService.parse(this.request, { field: 'file', maxSize: settings.get('FileUpload_MaxFileSize'), - ...(stripExif && { transforms: [UploadService.transforms.stripExif()] }), }); if (!file) { diff --git a/apps/meteor/app/file-upload/server/lib/FileUpload.spec.ts b/apps/meteor/app/file-upload/server/lib/FileUpload.spec.ts index 73249fea904a0..7d8fa682ff42a 100644 --- a/apps/meteor/app/file-upload/server/lib/FileUpload.spec.ts +++ b/apps/meteor/app/file-upload/server/lib/FileUpload.spec.ts @@ -31,6 +31,7 @@ const { FileUpload, FileUploadClass } = proxyquire.noCallThru().load('./FileUplo '../../../utils/lib/mimeTypes': sinon.stub(), '../../../utils/server/lib/JWTHelper': sinon.stub(), '../../../utils/server/restrictions': sinon.stub(), + '../../../api/server/lib/UploadService': sinon.stub(), }); describe('FileUpload', () => { diff --git a/apps/meteor/app/file-upload/server/lib/FileUpload.ts b/apps/meteor/app/file-upload/server/lib/FileUpload.ts index 96db59db4c661..d5f26dc4222f6 100644 --- a/apps/meteor/app/file-upload/server/lib/FileUpload.ts +++ b/apps/meteor/app/file-upload/server/lib/FileUpload.ts @@ -31,6 +31,7 @@ import { roomCoordinator } from '../../../../server/lib/rooms/roomCoordinator'; import { UploadFS } from '../../../../server/ufs'; import { ufsComplete } from '../../../../server/ufs/ufs-methods'; import type { Store, StoreOptions } from '../../../../server/ufs/ufs-store'; +import { UploadService } from '../../../api/server/lib/UploadService'; import { canAccessRoomAsync, canAccessRoomIdAsync } from '../../../authorization/server/functions/canAccessRoom'; import { settings } from '../../../settings/server'; import { mime } from '../../../utils/lib/mimeTypes'; @@ -371,10 +372,6 @@ export const FileUpload = { const s = sharp(tmpFile); const metadata = await s.metadata(); - // if (err != null) { - // SystemLogger.error(err); - // return fut.return(); - // } const rotated = typeof metadata.orientation !== 'undefined' && metadata.orientation !== 1; const width = rotated ? metadata.height : metadata.width; @@ -391,22 +388,36 @@ export const FileUpload = { : undefined, }; + const shouldRotate = settings.get('FileUpload_RotateImages') === true; + const shouldStripExif = settings.get('Message_Attachments_Strip_Exif') === true; + + let size = file.size || 0; + const reorientation = async () => { - if (!rotated || settings.get('FileUpload_RotateImages') !== true) { - return; + // sharp rotates AND removes metadata + const transform = s.rotate(); + + if (!shouldStripExif) { + transform.withMetadata(); } - await s.rotate().toFile(`${tmpFile}.tmp`); + const result = await transform.toFile(`${tmpFile}.sharp`); + + size = result.size; await unlink(tmpFile); - await rename(`${tmpFile}.tmp`, tmpFile); - // SystemLogger.error(err); + await rename(`${tmpFile}.sharp`, tmpFile); }; - await reorientation(); + if (rotated && shouldRotate) { + // If there is EXIF orientation and the setting is enabled, rotate the image (which removes metadata) + await reorientation(); + } else if (settings.get('Message_Attachments_Strip_Exif')) { + // If there is no EXIF orientation but the setting is enabled, still strip any metadata + size = await UploadService.stripExifFromFile(tmpFile); + } - const { size } = await fs.lstatSync(tmpFile); await this.getCollection().updateOne( { _id: file._id }, { diff --git a/apps/meteor/tests/e2e/image-upload.spec.ts b/apps/meteor/tests/e2e/image-upload.spec.ts index 8a55c5ad38c0e..509f9630225ab 100644 --- a/apps/meteor/tests/e2e/image-upload.spec.ts +++ b/apps/meteor/tests/e2e/image-upload.spec.ts @@ -6,12 +6,14 @@ import { test, expect } from './utils/test'; test.use({ storageState: Users.user1.state }); test.describe('image-upload', () => { - let settingDefaultValue: unknown; + let defaultStripSetting: unknown; + let defaultRotateSetting: unknown; let poHomeChannel: HomeChannel; let targetChannel: string; test.beforeAll(async ({ api }) => { - settingDefaultValue = await getSettingValueById(api, 'Message_Attachments_Strip_Exif'); + defaultStripSetting = await getSettingValueById(api, 'Message_Attachments_Strip_Exif'); + defaultRotateSetting = await getSettingValueById(api, 'FileUpload_RotateImages'); targetChannel = await createTargetChannel(api, { members: ['user1'] }); }); @@ -22,7 +24,8 @@ test.describe('image-upload', () => { }); test.afterAll(async ({ api }) => { - await setSettingValueById(api, 'Message_Attachments_Strip_Exif', settingDefaultValue); + await setSettingValueById(api, 'Message_Attachments_Strip_Exif', defaultStripSetting); + await setSettingValueById(api, 'FileUpload_RotateImages', defaultRotateSetting); expect((await api.post('/channels.delete', { roomName: targetChannel })).status()).toBe(200); }); @@ -44,6 +47,8 @@ test.describe('image-upload', () => { test.describe('strip exif enabled', () => { test.beforeAll(async ({ api }) => { await setSettingValueById(api, 'Message_Attachments_Strip_Exif', true); + // Image rotation now happens before EXIF stripping, so we need to disable it to properly test it + await setSettingValueById(api, 'FileUpload_RotateImages', false); }); test('should succeed upload of bad-orientation.jpeg', async () => { From 6495005aba375f87ff4cefe128a30532b62e030c Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Fri, 9 Jan 2026 19:29:23 -0300 Subject: [PATCH 21/32] chore: remove redundant call to unlink temp file --- apps/meteor/server/ufs/ufs-methods.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/apps/meteor/server/ufs/ufs-methods.ts b/apps/meteor/server/ufs/ufs-methods.ts index f1c495e98ef36..55c18f6ca066c 100644 --- a/apps/meteor/server/ufs/ufs-methods.ts +++ b/apps/meteor/server/ufs/ufs-methods.ts @@ -72,7 +72,6 @@ export async function ufsComplete(fileId: string, storeName: string, options?: { } catch (err: any) { // If write failed, remove the file await store.removeById(fileId, { session: options?.session }); - void removeTempFile(); reject(new Meteor.Error('ufs: cannot upload file', err)); } }); From 8a81284dd820a6151759ebffcbdf15901d6e482b Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Sun, 11 Jan 2026 18:51:33 -0300 Subject: [PATCH 22/32] fix: prevent body consumption in logger middleware --- apps/meteor/app/api/server/middlewares/logger.ts | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/apps/meteor/app/api/server/middlewares/logger.ts b/apps/meteor/app/api/server/middlewares/logger.ts index a9a733de86c4c..1188556d0a263 100644 --- a/apps/meteor/app/api/server/middlewares/logger.ts +++ b/apps/meteor/app/api/server/middlewares/logger.ts @@ -10,10 +10,15 @@ export const loggerMiddleware = let payload = {}; - try { - payload = await c.req.raw.clone().json(); - // eslint-disable-next-line no-empty - } catch {} + // We don't want to consume the request body stream for multipart requests + if (!c.req.header('content-type')?.includes('multipart/form-data')) { + try { + payload = await c.req.raw.clone().json(); + // eslint-disable-next-line no-empty + } catch {} + } else { + payload = '[multipart/form-data]'; + } const log = logger.logger.child({ method: c.req.method, From 0d11da3b549e062999bed2e678eeb7703512c28b Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Sun, 11 Jan 2026 18:57:07 -0300 Subject: [PATCH 23/32] fix: increase maxSize by 1 byte so we don't block files exactly as big as the limit --- apps/meteor/app/api/server/lib/UploadService.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/apps/meteor/app/api/server/lib/UploadService.ts b/apps/meteor/app/api/server/lib/UploadService.ts index f511004b39a0a..d45de21fc0b8e 100644 --- a/apps/meteor/app/api/server/lib/UploadService.ts +++ b/apps/meteor/app/api/server/lib/UploadService.ts @@ -67,7 +67,9 @@ export class UploadService { ): Promise<{ file: ParsedUpload | null; fields: Record }> { const limits: any = { files: 1 }; if (options.maxSize && options.maxSize > 0) { - limits.fileSize = options.maxSize; + // We add an extra byte to the configured limit so we don't fail the upload + // of a file that is EXACTLY maxSize + limits.fileSize = options.maxSize + 1; } const isIncomingMessage = 'socket' in requestOrStream; From 400b8e83ebf507b1ad1a394defdb21950f5a2e67 Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Sun, 11 Jan 2026 18:58:37 -0300 Subject: [PATCH 24/32] refactor: simplify request type checks in UploadService --- .../app/api/server/lib/UploadService.ts | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/apps/meteor/app/api/server/lib/UploadService.ts b/apps/meteor/app/api/server/lib/UploadService.ts index d45de21fc0b8e..90a4e80bb88ae 100644 --- a/apps/meteor/app/api/server/lib/UploadService.ts +++ b/apps/meteor/app/api/server/lib/UploadService.ts @@ -1,5 +1,5 @@ import fs from 'fs'; -import type { IncomingMessage } from 'http'; +import { IncomingMessage } from 'http'; import type { Stream, Transform } from 'stream'; import { Readable } from 'stream'; import { pipeline } from 'stream/promises'; @@ -72,10 +72,10 @@ export class UploadService { limits.fileSize = options.maxSize + 1; } - const isIncomingMessage = 'socket' in requestOrStream; - const headers = isIncomingMessage - ? ((requestOrStream as IncomingMessage).headers as Record) - : Object.fromEntries((requestOrStream as Request).headers.entries()); + const headers = + requestOrStream instanceof IncomingMessage + ? (requestOrStream.headers as Record) + : Object.fromEntries(requestOrStream.headers.entries()); const bb = busboy({ headers, @@ -185,15 +185,14 @@ export class UploadService { reject(new MeteorError('error-too-many-fields', 'Too many fields in upload')); }); - if (isIncomingMessage) { - (requestOrStream as IncomingMessage).pipe(bb); + if (requestOrStream instanceof IncomingMessage) { + requestOrStream.pipe(bb); } else { - const fetchRequest = requestOrStream as Request; - if (!fetchRequest.body) { + if (!requestOrStream.body) { return Promise.reject(new MeteorError('error-no-body', 'Request has no body')); } - const nodeStream = Readable.fromWeb(fetchRequest.body as any); + const nodeStream = Readable.fromWeb(requestOrStream.body as any); nodeStream.pipe(bb); } From cf8c71c6ea19604471fa47917fff7611414f8a71 Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Sun, 11 Jan 2026 19:01:43 -0300 Subject: [PATCH 25/32] refactor: expose internal http request to route handler --- apps/meteor/app/api/server/definition.ts | 3 +++ apps/meteor/app/api/server/router.ts | 23 ++++++++++++++--------- apps/meteor/app/api/server/v1/rooms.ts | 2 +- 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/apps/meteor/app/api/server/definition.ts b/apps/meteor/app/api/server/definition.ts index 9684b99c798c8..f8deb68d55261 100644 --- a/apps/meteor/app/api/server/definition.ts +++ b/apps/meteor/app/api/server/definition.ts @@ -1,3 +1,5 @@ +import type { IncomingMessage } from 'http'; + import type { IUser, LicenseModule } from '@rocket.chat/core-typings'; import type { Logger } from '@rocket.chat/logger'; import type { Method, MethodOf, OperationParams, OperationResult, PathPattern, UrlParams } from '@rocket.chat/rest-typings'; @@ -184,6 +186,7 @@ export type ActionThis>; readonly request: Request; + readonly incoming: IncomingMessage; readonly queryOperations: TOptions extends { queryOperations: infer T } ? T : never; readonly queryFields: TOptions extends { queryFields: infer T } ? T : never; diff --git a/apps/meteor/app/api/server/router.ts b/apps/meteor/app/api/server/router.ts index 1f18ea4c47228..0fba18a206093 100644 --- a/apps/meteor/app/api/server/router.ts +++ b/apps/meteor/app/api/server/router.ts @@ -1,16 +1,18 @@ -/* eslint-disable @typescript-eslint/naming-convention */ +import type { IncomingMessage } from 'node:http'; + import type { ResponseSchema } from '@rocket.chat/http-router'; import { Router } from '@rocket.chat/http-router'; -import type { Context as HonoContext } from 'hono'; +import type { Context } from 'hono'; import type { TypedOptions } from './definition'; -declare module 'hono' { - interface ContextVariableMap { - 'route': string; +type HonoContext = Context<{ + Bindings: { incoming: IncomingMessage }; + Variables: { + 'remoteAddress': string; 'bodyParams-override'?: Record; - } -} + }; +}>; export type APIActionContext = { requestIp: string; @@ -21,6 +23,7 @@ export type APIActionContext = { path: string; response: any; route: string; + incoming: IncomingMessage; }; export type APIActionHandler = (this: APIActionContext, request: Request) => Promise>; @@ -39,9 +42,10 @@ export class RocketChatAPIRouter< request: req, extra: { bodyParamsOverride: c.var['bodyParams-override'] || {} }, }); + const request = req.raw.clone(); - const context = { + const context: APIActionContext = { requestIp: c.get('remoteAddress'), urlParams: req.param(), queryParams, @@ -50,7 +54,8 @@ export class RocketChatAPIRouter< path: req.path, response: res, route: req.routePath, - } as APIActionContext; + incoming: c.env.incoming, + }; return action.apply(context, [request]); }; diff --git a/apps/meteor/app/api/server/v1/rooms.ts b/apps/meteor/app/api/server/v1/rooms.ts index efc8e12a0de07..4f33f1150b804 100644 --- a/apps/meteor/app/api/server/v1/rooms.ts +++ b/apps/meteor/app/api/server/v1/rooms.ts @@ -197,7 +197,7 @@ API.v1.addRoute( return API.v1.forbidden(); } - const { file, fields } = await UploadService.parse(this.request, { + const { file, fields } = await UploadService.parse(this.incoming, { field: 'file', maxSize: settings.get('FileUpload_MaxFileSize'), }); From 40951edb786a1da1910bf876c2c070b1f2e67588 Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Tue, 13 Jan 2026 14:07:43 -0300 Subject: [PATCH 26/32] refactor: revert endpoints that require buffer --- apps/meteor/app/api/server/v1/assets.ts | 23 ++++----- apps/meteor/app/api/server/v1/emoji-custom.ts | 51 +++++++++---------- apps/meteor/app/api/server/v1/users.ts | 20 ++++---- .../ee/server/apps/communication/rest.ts | 36 +++++++------ 4 files changed, 62 insertions(+), 68 deletions(-) diff --git a/apps/meteor/app/api/server/v1/assets.ts b/apps/meteor/app/api/server/v1/assets.ts index 2d30a545a471e..2843cf8627d51 100644 --- a/apps/meteor/app/api/server/v1/assets.ts +++ b/apps/meteor/app/api/server/v1/assets.ts @@ -1,5 +1,3 @@ -import { readFile } from 'fs/promises'; - import { Settings } from '@rocket.chat/models'; import { isAssetsUnsetAssetProps } from '@rocket.chat/rest-typings'; @@ -8,7 +6,7 @@ import { RocketChatAssets, refreshClients } from '../../../assets/server'; import { notifyOnSettingChangedById } from '../../../lib/server/lib/notifyListener'; import { settings } from '../../../settings/server'; import { API } from '../api'; -import { UploadService } from '../lib/UploadService'; +import { getUploadFormData } from '../lib/getUploadFormData'; API.v1.addRoute( 'assets.setAsset', @@ -18,18 +16,18 @@ API.v1.addRoute( }, { async post() { - const { file, fields } = await UploadService.parse(this.request, { - field: 'asset', - maxSize: settings.get('FileUpload_MaxFileSize'), - }); + const asset = await getUploadFormData( + { + request: this.request, + }, + { field: 'asset', sizeLimit: settings.get('FileUpload_MaxFileSize') }, + ); - if (!file) { - throw new Error('No file was uploaded'); - } + const { fileBuffer, fields, filename, mimetype } = asset; const { refreshAllClients, assetName: customName } = fields; - const assetName = customName || file.filename; + const assetName = customName || filename; const assetsKeys = Object.keys(RocketChatAssets.assets); const isValidAsset = assetsKeys.includes(assetName); @@ -37,8 +35,7 @@ API.v1.addRoute( throw new Error('Invalid asset'); } - const fileBuffer = await readFile(file.tempFilePath); - const { key, value } = await RocketChatAssets.setAssetWithBuffer(fileBuffer, file.mimetype, assetName); + const { key, value } = await RocketChatAssets.setAssetWithBuffer(fileBuffer, mimetype, assetName); const { modifiedCount } = await updateAuditedByUser({ _id: this.userId, diff --git a/apps/meteor/app/api/server/v1/emoji-custom.ts b/apps/meteor/app/api/server/v1/emoji-custom.ts index 0cfb74f19b660..fa84d3cc6b995 100644 --- a/apps/meteor/app/api/server/v1/emoji-custom.ts +++ b/apps/meteor/app/api/server/v1/emoji-custom.ts @@ -1,5 +1,3 @@ -import { readFile } from 'fs/promises'; - import { Media } from '@rocket.chat/core-services'; import type { IEmojiCustom } from '@rocket.chat/core-typings'; import { EmojiCustom } from '@rocket.chat/models'; @@ -15,8 +13,8 @@ import { deleteEmojiCustom } from '../../../emoji-custom/server/methods/deleteEm import { settings } from '../../../settings/server'; import { API } from '../api'; import { getPaginationItems } from '../helpers/getPaginationItems'; -import { UploadService } from '../lib/UploadService'; import { findEmojisCustom } from '../lib/emoji-custom'; +import { getUploadFormData } from '../lib/getUploadFormData'; function validateDateParam(paramName: string, paramValue: string | undefined): Date | undefined { if (!paramValue) { @@ -110,23 +108,21 @@ API.v1.addRoute( { authRequired: true }, { async post() { - const { file, fields } = await UploadService.parse(this.request, { - field: 'emoji', - maxSize: settings.get('FileUpload_MaxFileSize'), - }); - - if (!file) { - throw new Meteor.Error('error-no-file-uploaded', 'No file was uploaded'); - } + const emoji = await getUploadFormData( + { + request: this.request, + }, + { field: 'emoji', sizeLimit: settings.get('FileUpload_MaxFileSize') }, + ); - const fileBuffer = await readFile(file.tempFilePath); + const { fields, fileBuffer, mimetype } = emoji; const isUploadable = await Media.isImage(fileBuffer); if (!isUploadable) { throw new Meteor.Error('emoji-is-not-image', "Emoji file provided cannot be uploaded since it's not an image"); } - const [, extension] = file.mimetype.split('/'); + const [, extension] = mimetype.split('/'); fields.extension = extension; try { @@ -138,7 +134,7 @@ API.v1.addRoute( extension: fields.extension, }); - await uploadEmojiCustomWithBuffer(this.userId, fileBuffer, file.mimetype, emojiData); + await uploadEmojiCustomWithBuffer(this.userId, fileBuffer, mimetype, emojiData); } catch (e) { SystemLogger.error(e); return API.v1.failure(); @@ -154,11 +150,14 @@ API.v1.addRoute( { authRequired: true }, { async post() { - const { file, fields } = await UploadService.parse(this.request, { - field: 'emoji', - maxSize: settings.get('FileUpload_MaxFileSize'), - fileOptional: true, - }); + const emoji = await getUploadFormData( + { + request: this.request, + }, + { field: 'emoji', sizeLimit: settings.get('FileUpload_MaxFileSize'), fileOptional: true }, + ); + + const { fields, fileBuffer, mimetype } = emoji; if (!fields._id) { throw new Meteor.Error('The required "_id" query param is missing.'); @@ -181,26 +180,24 @@ API.v1.addRoute( newFile: false, }; - const isNewFile = !!file; + const isNewFile = fileBuffer?.length && !!mimetype; if (isNewFile) { - const fileBuffer = await readFile(file.tempFilePath); - emojiData.newFile = isNewFile; const isUploadable = await Media.isImage(fileBuffer); if (!isUploadable) { throw new Meteor.Error('emoji-is-not-image', "Emoji file provided cannot be uploaded since it's not an image"); } - const [, extension] = file.mimetype.split('/'); + const [, extension] = mimetype.split('/'); emojiData.extension = extension; - - const updatedEmojiData = await insertOrUpdateEmoji(this.userId, emojiData); - await uploadEmojiCustomWithBuffer(this.userId, fileBuffer, file.mimetype, updatedEmojiData); } else { emojiData.extension = emojiToUpdate.extension; - await insertOrUpdateEmoji(this.userId, emojiData); } + const updatedEmojiData = await insertOrUpdateEmoji(this.userId, emojiData); + if (isNewFile) { + await uploadEmojiCustomWithBuffer(this.userId, fileBuffer, mimetype, updatedEmojiData); + } return API.v1.success(); }, }, diff --git a/apps/meteor/app/api/server/v1/users.ts b/apps/meteor/app/api/server/v1/users.ts index 0c9c701932f3d..fd546426e84b5 100644 --- a/apps/meteor/app/api/server/v1/users.ts +++ b/apps/meteor/app/api/server/v1/users.ts @@ -1,5 +1,3 @@ -import { readFile } from 'fs/promises'; - import { MeteorError, Team, api, Calendar } from '@rocket.chat/core-services'; import { type IExportOperation, type ILoginToken, type IPersonalAccessToken, type IUser, type UserStatus } from '@rocket.chat/core-typings'; import { Users, Subscriptions, Sessions } from '@rocket.chat/models'; @@ -77,7 +75,7 @@ import { API } from '../api'; import { getPaginationItems } from '../helpers/getPaginationItems'; import { getUserFromParams } from '../helpers/getUserFromParams'; import { isUserFromParams } from '../helpers/isUserFromParams'; -import { UploadService } from '../lib/UploadService'; +import { getUploadFormData } from '../lib/getUploadFormData'; import { isValidQuery } from '../lib/isValidQuery'; import { findPaginatedUsersByStatus, findUsersToAutocomplete, getInclusiveFields, getNonEmptyFields, getNonEmptyQuery } from '../lib/users'; @@ -272,15 +270,19 @@ API.v1.addRoute( return API.v1.success(); } - const { file, fields } = await UploadService.parse(this.request, { - field: 'image', - maxSize: settings.get('FileUpload_MaxFileSize'), - }); + const image = await getUploadFormData( + { + request: this.request, + }, + { field: 'image', sizeLimit: settings.get('FileUpload_MaxFileSize') }, + ); - if (!file) { + if (!image) { return API.v1.failure("The 'image' param is required"); } + const { fields, fileBuffer, mimetype } = image; + const sentTheUserByFormData = fields.userId || fields.username; if (sentTheUserByFormData) { if (fields.userId) { @@ -301,7 +303,7 @@ API.v1.addRoute( } } - await setUserAvatar(user, await readFile(file.tempFilePath), file.mimetype, 'rest'); + await setUserAvatar(user, fileBuffer, mimetype, 'rest'); return API.v1.success(); }, diff --git a/apps/meteor/ee/server/apps/communication/rest.ts b/apps/meteor/ee/server/apps/communication/rest.ts index 1160f57c4b9c2..95436789a11f6 100644 --- a/apps/meteor/ee/server/apps/communication/rest.ts +++ b/apps/meteor/ee/server/apps/communication/rest.ts @@ -1,5 +1,3 @@ -import { readFile } from 'fs/promises'; - import { AppStatus, AppStatusUtils } from '@rocket.chat/apps-engine/definition/AppStatus'; import type { IAppInfo } from '@rocket.chat/apps-engine/definition/metadata'; import type { AppManager } from '@rocket.chat/apps-engine/server/AppManager'; @@ -21,7 +19,7 @@ import { registerAppLogsHandler } from './endpoints/appLogsHandler'; import { registerAppsCountHandler } from './endpoints/appsCountHandler'; import { API } from '../../../../app/api/server'; import type { APIClass } from '../../../../app/api/server/ApiClass'; -import { UploadService } from '../../../../app/api/server/lib/UploadService'; +import { getUploadFormData } from '../../../../app/api/server/lib/getUploadFormData'; import { loggerMiddleware } from '../../../../app/api/server/middlewares/logger'; import { metricsMiddleware } from '../../../../app/api/server/middlewares/metrics'; import { tracerSpanMiddleware } from '../../../../app/api/server/middlewares/tracer'; @@ -328,16 +326,16 @@ export class AppsRestApi { return API.v1.failure({ error: message }); } } else { - const { file, fields: formData } = await UploadService.parse(this.request, { - field: 'app', - maxSize: settings.get('FileUpload_MaxFileSize'), - }); + const app = await getUploadFormData( + { + request: this.request, + }, + { field: 'app', sizeLimit: settings.get('FileUpload_MaxFileSize') }, + ); - if (!file) { - return API.v1.failure({ error: 'No file was uploaded' }); - } + const { fields: formData } = app; - buff = await readFile(file.tempFilePath); + buff = app.fileBuffer; permissionsGranted = (() => { try { const permissions = JSON.parse(formData?.permissions || ''); @@ -833,16 +831,16 @@ export class AppsRestApi { } else { isPrivateAppUpload = true; - const { file, fields: formData } = await UploadService.parse(this.request, { - field: 'app', - maxSize: settings.get('FileUpload_MaxFileSize'), - }); + const app = await getUploadFormData( + { + request: this.request, + }, + { field: 'app', sizeLimit: settings.get('FileUpload_MaxFileSize') }, + ); - if (!file) { - return API.v1.failure({ error: 'No file was uploaded' }); - } + const { fields: formData } = app; - buff = await readFile(file.tempFilePath); + buff = app.fileBuffer; permissionsGranted = (() => { try { const permissions = JSON.parse(formData?.permissions || ''); From 89e2bcac9798999f38e1dfec386e055b3a0cee5e Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Tue, 13 Jan 2026 14:08:44 -0300 Subject: [PATCH 27/32] refactor: settings usage refactor --- apps/meteor/app/file-upload/server/lib/FileUpload.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/meteor/app/file-upload/server/lib/FileUpload.ts b/apps/meteor/app/file-upload/server/lib/FileUpload.ts index d5f26dc4222f6..32351e2100686 100644 --- a/apps/meteor/app/file-upload/server/lib/FileUpload.ts +++ b/apps/meteor/app/file-upload/server/lib/FileUpload.ts @@ -413,7 +413,7 @@ export const FileUpload = { if (rotated && shouldRotate) { // If there is EXIF orientation and the setting is enabled, rotate the image (which removes metadata) await reorientation(); - } else if (settings.get('Message_Attachments_Strip_Exif')) { + } else if (shouldStripExif) { // If there is no EXIF orientation but the setting is enabled, still strip any metadata size = await UploadService.stripExifFromFile(tmpFile); } From dee52dce05cd0e5629d9afa765eed7fcbcd9500e Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Tue, 13 Jan 2026 14:09:12 -0300 Subject: [PATCH 28/32] refactor: remove unnecessary if on finishHandler --- apps/meteor/server/ufs/ufs-store.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/apps/meteor/server/ufs/ufs-store.ts b/apps/meteor/server/ufs/ufs-store.ts index ab775a4a4f304..3c6d6c983850f 100644 --- a/apps/meteor/server/ufs/ufs-store.ts +++ b/apps/meteor/server/ufs/ufs-store.ts @@ -166,10 +166,6 @@ export class Store { }; const finishHandler = async () => { - if (file.complete) { - return; - } - try { // Set file attribute file.complete = true; From fca5ce4aee0d6d2142fe2b7d477e29a94ff29f61 Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Tue, 13 Jan 2026 16:56:22 -0300 Subject: [PATCH 29/32] refactor: rename UploadService to MultipartUploadHandler --- ...adService.ts => MultipartUploadHandler.ts} | 28 +++++++++---------- apps/meteor/app/api/server/v1/rooms.ts | 4 +-- .../file-upload/server/lib/FileUpload.spec.ts | 2 +- .../app/file-upload/server/lib/FileUpload.ts | 4 +-- .../livechat/imports/server/rest/upload.ts | 4 +-- apps/meteor/tests/end-to-end/api/rooms.ts | 4 +-- 6 files changed, 23 insertions(+), 23 deletions(-) rename apps/meteor/app/api/server/lib/{UploadService.ts => MultipartUploadHandler.ts} (87%) diff --git a/apps/meteor/app/api/server/lib/UploadService.ts b/apps/meteor/app/api/server/lib/MultipartUploadHandler.ts similarity index 87% rename from apps/meteor/app/api/server/lib/UploadService.ts rename to apps/meteor/app/api/server/lib/MultipartUploadHandler.ts index 90a4e80bb88ae..75ee816e6ab03 100644 --- a/apps/meteor/app/api/server/lib/UploadService.ts +++ b/apps/meteor/app/api/server/lib/MultipartUploadHandler.ts @@ -6,7 +6,7 @@ import { pipeline } from 'stream/promises'; import { MeteorError } from '@rocket.chat/core-services'; import { Random } from '@rocket.chat/random'; -import busboy from 'busboy'; +import busboy, { type BusboyConfig } from 'busboy'; import ExifTransformer from 'exif-be-gone'; import { UploadFS } from '../../../../server/ufs'; @@ -28,7 +28,7 @@ export type ParseOptions = { fileOptional?: boolean; }; -export class UploadService { +export class MultipartUploadHandler { static transforms = { stripExif(): Transform { return new ExifTransformer(); @@ -61,11 +61,12 @@ export class UploadService { } } - static async parse( - requestOrStream: IncomingMessage | Request, + static async parseRequest( + request: IncomingMessage | Request, options: ParseOptions, ): Promise<{ file: ParsedUpload | null; fields: Record }> { - const limits: any = { files: 1 }; + const limits: BusboyConfig['limits'] = { files: 1 }; + if (options.maxSize && options.maxSize > 0) { // We add an extra byte to the configured limit so we don't fail the upload // of a file that is EXACTLY maxSize @@ -73,9 +74,7 @@ export class UploadService { } const headers = - requestOrStream instanceof IncomingMessage - ? (requestOrStream.headers as Record) - : Object.fromEntries(requestOrStream.headers.entries()); + request instanceof IncomingMessage ? (request.headers as Record) : Object.fromEntries(request.headers.entries()); const bb = busboy({ headers, @@ -153,6 +152,7 @@ export class UploadService { }); writeStream.on('error', (err) => { + file.destroy(); void this.cleanup(tempFilePath); reject(new MeteorError('error-file-upload', err.message)); }); @@ -160,7 +160,7 @@ export class UploadService { file.on('error', (err) => { writeStream.destroy(); void this.cleanup(tempFilePath); - reject(err); + reject(new MeteorError('error-file-upload', err.message)); }); }); @@ -174,7 +174,7 @@ export class UploadService { }); bb.on('filesLimit', () => { - reject('Just 1 file is allowed'); + reject(new MeteorError('error-too-many-files', 'Too many files in upload')); }); bb.on('partsLimit', () => { @@ -185,14 +185,14 @@ export class UploadService { reject(new MeteorError('error-too-many-fields', 'Too many fields in upload')); }); - if (requestOrStream instanceof IncomingMessage) { - requestOrStream.pipe(bb); + if (request instanceof IncomingMessage) { + request.pipe(bb); } else { - if (!requestOrStream.body) { + if (!request.body) { return Promise.reject(new MeteorError('error-no-body', 'Request has no body')); } - const nodeStream = Readable.fromWeb(requestOrStream.body as any); + const nodeStream = Readable.fromWeb(request.body as any); nodeStream.pipe(bb); } diff --git a/apps/meteor/app/api/server/v1/rooms.ts b/apps/meteor/app/api/server/v1/rooms.ts index 4f33f1150b804..b89be1304b7c5 100644 --- a/apps/meteor/app/api/server/v1/rooms.ts +++ b/apps/meteor/app/api/server/v1/rooms.ts @@ -55,7 +55,7 @@ import { API } from '../api'; import { composeRoomWithLastMessage } from '../helpers/composeRoomWithLastMessage'; import { getPaginationItems } from '../helpers/getPaginationItems'; import { getUserFromParams } from '../helpers/getUserFromParams'; -import { UploadService } from '../lib/UploadService'; +import { MultipartUploadHandler } from '../lib/MultipartUploadHandler'; import { findAdminRoom, findAdminRooms, @@ -197,7 +197,7 @@ API.v1.addRoute( return API.v1.forbidden(); } - const { file, fields } = await UploadService.parse(this.incoming, { + const { file, fields } = await MultipartUploadHandler.parseRequest(this.incoming, { field: 'file', maxSize: settings.get('FileUpload_MaxFileSize'), }); diff --git a/apps/meteor/app/file-upload/server/lib/FileUpload.spec.ts b/apps/meteor/app/file-upload/server/lib/FileUpload.spec.ts index 7d8fa682ff42a..e5bad2f55d626 100644 --- a/apps/meteor/app/file-upload/server/lib/FileUpload.spec.ts +++ b/apps/meteor/app/file-upload/server/lib/FileUpload.spec.ts @@ -31,7 +31,7 @@ const { FileUpload, FileUploadClass } = proxyquire.noCallThru().load('./FileUplo '../../../utils/lib/mimeTypes': sinon.stub(), '../../../utils/server/lib/JWTHelper': sinon.stub(), '../../../utils/server/restrictions': sinon.stub(), - '../../../api/server/lib/UploadService': sinon.stub(), + '../../../api/server/lib/MultipartUploadHandler': sinon.stub(), }); describe('FileUpload', () => { diff --git a/apps/meteor/app/file-upload/server/lib/FileUpload.ts b/apps/meteor/app/file-upload/server/lib/FileUpload.ts index 32351e2100686..2b58ca82cd87c 100644 --- a/apps/meteor/app/file-upload/server/lib/FileUpload.ts +++ b/apps/meteor/app/file-upload/server/lib/FileUpload.ts @@ -31,7 +31,7 @@ import { roomCoordinator } from '../../../../server/lib/rooms/roomCoordinator'; import { UploadFS } from '../../../../server/ufs'; import { ufsComplete } from '../../../../server/ufs/ufs-methods'; import type { Store, StoreOptions } from '../../../../server/ufs/ufs-store'; -import { UploadService } from '../../../api/server/lib/UploadService'; +import { MultipartUploadHandler } from '../../../api/server/lib/MultipartUploadHandler'; import { canAccessRoomAsync, canAccessRoomIdAsync } from '../../../authorization/server/functions/canAccessRoom'; import { settings } from '../../../settings/server'; import { mime } from '../../../utils/lib/mimeTypes'; @@ -415,7 +415,7 @@ export const FileUpload = { await reorientation(); } else if (shouldStripExif) { // If there is no EXIF orientation but the setting is enabled, still strip any metadata - size = await UploadService.stripExifFromFile(tmpFile); + size = await MultipartUploadHandler.stripExifFromFile(tmpFile); } await this.getCollection().updateOne( diff --git a/apps/meteor/app/livechat/imports/server/rest/upload.ts b/apps/meteor/app/livechat/imports/server/rest/upload.ts index eb79babf1635b..14cd2ae29e077 100644 --- a/apps/meteor/app/livechat/imports/server/rest/upload.ts +++ b/apps/meteor/app/livechat/imports/server/rest/upload.ts @@ -1,7 +1,7 @@ import { LivechatVisitors, LivechatRooms } from '@rocket.chat/models'; import { API } from '../../../../api/server'; -import { UploadService } from '../../../../api/server/lib/UploadService'; +import { MultipartUploadHandler } from '../../../../api/server/lib/MultipartUploadHandler'; import { FileUpload } from '../../../../file-upload/server'; import { settings } from '../../../../settings/server'; import { fileUploadIsValidContentType } from '../../../../utils/server/restrictions'; @@ -35,7 +35,7 @@ API.v1.addRoute('livechat/upload/:rid', { const maxFileSize = settings.get('FileUpload_MaxFileSize') || 104857600; - const { file, fields } = await UploadService.parse(this.request, { + const { file, fields } = await MultipartUploadHandler.parseRequest(this.request, { field: 'file', maxSize: maxFileSize > -1 ? maxFileSize : undefined, }); diff --git a/apps/meteor/tests/end-to-end/api/rooms.ts b/apps/meteor/tests/end-to-end/api/rooms.ts index 8ab8a3bb20917..1f0b8ef63003a 100644 --- a/apps/meteor/tests/end-to-end/api/rooms.ts +++ b/apps/meteor/tests/end-to-end/api/rooms.ts @@ -153,7 +153,7 @@ describe('[Rooms]', () => { .expect(400) .expect((res) => { expect(res.body).to.have.property('success', false); - expect(res.body).to.have.property('error', res.body.error); + expect(res.body).to.have.property('error'); }) .end(done); }); @@ -167,7 +167,7 @@ describe('[Rooms]', () => { .expect(400) .expect((res) => { expect(res.body).to.have.property('success', false); - expect(res.body).to.have.property('error', 'Just 1 file is allowed'); + expect(res.body).to.have.property('errorType', 'error-too-many-files'); }) .end(done); }); From 396ebd566754364b131d41700e325943d7f9c2c6 Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Tue, 13 Jan 2026 18:02:18 -0300 Subject: [PATCH 30/32] fix: properly handle errors during transform pipeline --- apps/meteor/app/api/server/lib/MultipartUploadHandler.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/apps/meteor/app/api/server/lib/MultipartUploadHandler.ts b/apps/meteor/app/api/server/lib/MultipartUploadHandler.ts index 75ee816e6ab03..dafa07b85cc01 100644 --- a/apps/meteor/app/api/server/lib/MultipartUploadHandler.ts +++ b/apps/meteor/app/api/server/lib/MultipartUploadHandler.ts @@ -127,7 +127,9 @@ export class MultipartUploadHandler { let currentStream: Stream = file; if (options.transforms?.length) { + const fileDestroyer = file.destroy.bind(file); for (const transform of options.transforms) { + transform.on('error', fileDestroyer); currentStream = currentStream.pipe(transform); } } From 0e9c0287dea293bb1880d17052cb1e28ad7897df Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Wed, 14 Jan 2026 11:48:14 -0300 Subject: [PATCH 31/32] fix(apps): reduce memory usage on apps-engine IPreFileUpload event (#38068) --- .changeset/spotty-steaks-notice.md | 6 ++ .../app/apps/server/bridges/listeners.js | 38 +++++++++ apps/meteor/ee/server/apps/orchestrator.js | 17 ++++ .../tests/data/apps/app-packages/README.md | 54 +++++++++++++ .../app-packages/file-upload-test_0.0.1.zip | Bin 0 -> 2640 bytes .../tests/data/apps/app-packages/index.ts | 3 + apps/meteor/tests/data/apps/helper.ts | 21 +++-- .../end-to-end/apps/apps-hooks-file-upload.ts | 56 ++++++++++++++ packages/apps-engine/deno-runtime/deno.jsonc | 1 + packages/apps-engine/deno-runtime/deno.lock | 12 +++ .../handlers/app/handleUploadEvents.ts | 73 ++++++++++++++++++ .../deno-runtime/handlers/app/handler.ts | 67 +++++++--------- .../deno-runtime/handlers/lib/assertions.ts | 33 ++++++++ .../definition/uploads/IFileUploadContext.ts | 5 ++ packages/apps-engine/src/server/AppManager.ts | 21 ++++- packages/apps-engine/src/server/ProxiedApp.ts | 4 +- .../src/server/compiler/AppImplements.ts | 17 ++-- .../src/server/compiler/AppPackageParser.ts | 2 +- .../src/server/managers/AppListenerManager.ts | 8 +- .../runtime/deno/AppsEngineDenoRuntime.ts | 14 +++- .../server/compiler/AppImplements.spec.ts | 7 +- .../apps-engine/tests/test-data/utilities.ts | 1 + 22 files changed, 399 insertions(+), 61 deletions(-) create mode 100644 .changeset/spotty-steaks-notice.md create mode 100644 apps/meteor/tests/data/apps/app-packages/README.md create mode 100644 apps/meteor/tests/data/apps/app-packages/file-upload-test_0.0.1.zip create mode 100644 apps/meteor/tests/data/apps/app-packages/index.ts create mode 100644 apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.ts create mode 100644 packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts create mode 100644 packages/apps-engine/deno-runtime/handlers/lib/assertions.ts diff --git a/.changeset/spotty-steaks-notice.md b/.changeset/spotty-steaks-notice.md new file mode 100644 index 0000000000000..6a47834f69d47 --- /dev/null +++ b/.changeset/spotty-steaks-notice.md @@ -0,0 +1,6 @@ +--- +'@rocket.chat/apps-engine': patch +'@rocket.chat/meteor': patch +--- + +Fixes an issue that caused a spike in memory usage when apps handled the IPreFileUpload event diff --git a/apps/meteor/app/apps/server/bridges/listeners.js b/apps/meteor/app/apps/server/bridges/listeners.js index 31aa2c0052695..8f2b5b78a113d 100644 --- a/apps/meteor/app/apps/server/bridges/listeners.js +++ b/apps/meteor/app/apps/server/bridges/listeners.js @@ -1,8 +1,15 @@ +import crypto from 'crypto'; +import fs from 'fs'; +import path from 'path'; + import { LivechatTransferEventType } from '@rocket.chat/apps-engine/definition/livechat'; import { AppInterface } from '@rocket.chat/apps-engine/definition/metadata'; export class AppListenerBridge { constructor(orch) { + /** + * @type {import('@rocket.chat/apps').IAppServerOrchestrator} + */ this.orch = orch; } @@ -10,6 +17,8 @@ export class AppListenerBridge { // eslint-disable-next-line complexity const method = (() => { switch (event) { + case AppInterface.IPreFileUpload: + return 'uploadEvent'; case AppInterface.IPostSystemMessageSent: case AppInterface.IPreMessageSentPrevent: case AppInterface.IPreMessageSentExtend: @@ -72,6 +81,35 @@ export class AppListenerBridge { return this.orch.getManager().getListenerManager().executeListener(inte, payload); } + /** + * + * @param {{ file: import('@rocket.chat/core-typings').IUpload; content: Buffer }} payload + * @return Promise + */ + async uploadEvent(_, payload) { + const { file, content } = payload; + + const tmpfile = path.join(this.orch.getManager().getTempFilePath(), crypto.randomUUID()); + await fs.promises.writeFile(tmpfile, content).catch((err) => { + this.orch.getRocketChatLogger().error({ msg: `AppListenerBridge: Could not write temporary file at ${tmpfile}`, err }); + + throw new Error('Error sending file to apps', { cause: err }); + }); + + try { + const appFile = await this.orch.getConverters().get('uploads').convertToApp(file); + + // Execute both events for backward compatibility + await this.orch.getManager().getListenerManager().executeListener(AppInterface.IPreFileUpload, { file: appFile, path: tmpfile }); + } finally { + await fs.promises + .unlink(tmpfile) + .catch((err) => + this.orch.getRocketChatLogger().warn({ msg: `AppListenerBridge: Could not delete temporary file at ${tmpfile}`, err }), + ); + } + } + async messageEvent(inte, message, ...payload) { const msg = await this.orch.getConverters().get('messages').convertMessage(message); diff --git a/apps/meteor/ee/server/apps/orchestrator.js b/apps/meteor/ee/server/apps/orchestrator.js index b4fed41ec05c0..d72ffb9e4f6d6 100644 --- a/apps/meteor/ee/server/apps/orchestrator.js +++ b/apps/meteor/ee/server/apps/orchestrator.js @@ -1,3 +1,7 @@ +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; + import { registerOrchestrator } from '@rocket.chat/apps'; import { EssentialAppDisabledException } from '@rocket.chat/apps-engine/definition/exceptions'; import { AppManager } from '@rocket.chat/apps-engine/server/AppManager'; @@ -68,11 +72,24 @@ export class AppServerOrchestrator { this._bridges = new RealAppBridges(this); + const tempFilePath = path.join(os.tmpdir(), 'apps-engine-temp'); + + try { + // We call this only once at server startup, so using the synchronous version is fine + fs.mkdirSync(tempFilePath); + } catch (err) { + // If the temp directory already exists, we can continue + if (err.code !== 'EEXIST') { + throw new Error('Failed to initialize the Apps-Engine', { cause: err }); + } + } + this._manager = new AppManager({ metadataStorage: this._storage, logStorage: this._logStorage, bridges: this._bridges, sourceStorage: this._appSourceStorage, + tempFilePath, }); this._communicators = new Map(); diff --git a/apps/meteor/tests/data/apps/app-packages/README.md b/apps/meteor/tests/data/apps/app-packages/README.md new file mode 100644 index 0000000000000..03da4595a87c5 --- /dev/null +++ b/apps/meteor/tests/data/apps/app-packages/README.md @@ -0,0 +1,54 @@ +# Test App Packages + +Includes pre-built app packages that are designed to test specific APIs exposed by the Apps-engine. + +## How to use + +In your tests, add a `before` step and call the `installLocalTestPackage` function, passing the path of your desired package. For instance: + +```javascript +import { appImplementsIPreFileUpload } from '../../data/apps/app-packages'; +import { installLocalTestPackage } from '../../data/apps/helper'; + +describe('My tests', () => { + before(async () => { + await installLocalTestPackage(appImplementsIPreFileUpload); + }); +}); +``` + +### Available apps + +#### IPreFileUpload handler + +File name: `file-upload-test_0.0.1.zip` + +An app that handles the `IPreFileUpload` event. If the file name starts with `"test-should-reject"`, the app will prevent the upload from happening. The error message will contain the contents of the uploaded file as evidence that the app could successfully read them. + +
+App source code + +```typescript +import { + IHttp, + IModify, + IPersistence, + IRead, +} from '@rocket.chat/apps-engine/definition/accessors'; +import { App } from '@rocket.chat/apps-engine/definition/App'; +import { FileUploadNotAllowedException } from '@rocket.chat/apps-engine/definition/exceptions'; +import { IPreFileUpload } from '@rocket.chat/apps-engine/definition/uploads'; +import { IFileUploadContext } from '@rocket.chat/apps-engine/definition/uploads/IFileUploadContext'; + +export class TestIPreFileUpload extends App implements IPreFileUpload { + public async executePreFileUpload(context: IFileUploadContext, read: IRead, http: IHttp, persis: IPersistence, modify: IModify): Promise { + if (context.file.name.startsWith('test-should-reject')) { + console.log('[executePreFileUpload] Rejecting file which name starts with "test-should-reject"'); + throw new FileUploadNotAllowedException(`Test file rejected ${context.content.toString()}`); + } + console.log('[executePreFileUpload] Did not reject file'); + } +} +``` + +
diff --git a/apps/meteor/tests/data/apps/app-packages/file-upload-test_0.0.1.zip b/apps/meteor/tests/data/apps/app-packages/file-upload-test_0.0.1.zip new file mode 100644 index 0000000000000000000000000000000000000000..05183f3f05cb2d9138f8f8dea4e7f6b1332ec6cc GIT binary patch literal 2640 zcmZ`*2T&7O6HZ9zNEZbWkf=erp~nM65s_dB1Q3uCYNVJ@#88#aQKbawAjNVgK{^~# zgd8n|L{WNCP(&$;5kgSPAMbweyqo{uH#@ugX7}xT-@KXqPzVrM901_q0gM+y9qV;V zh-d%+V9i#3wyOHOy8F6fJw4n)AKEGo(CL3mQ`88G>f2{LDgGL1FYFZNPfi zQW2q|NTbrxbylz!8eGZu zc0eNvm-(*oTOmR2c;O6iiM%S&@0f!-bgu?Dg^GB^92FY{@n)RgHJ7RdBNUAyN3PtG z>4<$)1h6G`*{$)!O+x56WeGWah;ZW5nrza6-lwgtqb;j?#R>(A7CnL0R}=;ojD>6$ z^y(HgYpT&g#)n95ZOK3p~_Y1{wXOWHanCs`Y|(so^LJB z+z&Q+D>#!sCE0RQQqt4fQmxWZ!yapJNaYKe))y@jRy)s+nLV3#0S_mKV%1sKheOuw zR^sP6szwgaq|O-Ktb1p1ykeky5gFuJYG|Sft_2)vu>)JnT%De~ihaeD05wm^BEKGk zP@0@gW6zmJVnh{5({tBGB?aIb1kEb-yXJ~&0{UWg8eb9IrC)Y2mDQD=lLjK+q$}QZH|cy$CvFGt|C;Z}{FP+PvXetoX zUv^f`=tyGnys`7K#)jSW56 zl(lLrgb`JvPZxSwd#Av0p!<=wHJnJ&1)e&b7}3sUbcMOFWnelGe^j|JV`4z2m{IoN zwPz4CDlsJO_6(Y{`2|-MQr!L{(ict*2VX5?i0{*Mo*cXyisL_HMQdoBLfv%d6bABF z_Skd5uah)_frf61a_@}SwVT@T-J5K9%*;cZ8Eemb8k_+b3T$hI_YOD;l@u?%BWKNkxMe zqaWTq85C~wL{q))rzjh~p6V*2jniLZOnx=d&`nBXzM0symb6(ZFU$LId(gIxa(rr# zh;B>}**7fSF6`X@b38xxu|>$zgKeRpa*8HGK$lQW_#^$N(1@6os=)F zPH;I>DNH`+TK3s|?IZJ!I6& zLGzy#%wCQrV{*@WS1mKQRc4F%o*!a1m8;XW@1YOLrI@9BavBWFB`&$xE*QC!Sh?@7 z*aZ`>&Eno}X<^QCG=$8Exm&*Mmf%7&CkkNi47W0D(*hCUl!E-6c(lX&k;S&gIpf|; zz4_G_m(ib0D>#}mYa_uY+O;woM(_D!M<6GeBuG~A!)8j8!?F~;bkZU{>6ORy5Z-yl z@{z^t-g4>KUIz`*3;`~+sRkB*=?^aIAU-koUNW`}5UEa`gVvdh&$<{h(EFo}J9*mZ z-VsNElO5^3O(ngfCwF}NZJ+{M)d^1?6Q8{1j3#J-myXB=jrAQp-9Gl)>_bBPVnq27 zYDGqP7eBZob6+=eh%*|m1$O4>81RmO0s~#nlS%jI;5a68(07a^VpGQ|sLOMzVL6cJ zIj8NBZWlSkFX=;XWVKAB7WZ^p-~(k(@GD%F6;#0HUA!5x+lz^N?anF}66xm#>^W zgt;VOAQ2mIl2sz8iR+aAE#+mGPcC(Tk)-KDI%iS(CB1gTY~F5WZo}wyiz$PCd_c_N zLq5Jsj8z+>QN%c$P%W&hBz0!;v^inF*^RqCu~K}MVQM-*3C3=62;_40zCZOGrjnM3 zf$MzPsrE?=_suFmH8lxD>>9vQ>?Y(#(|=8~X{3A{*S@u+)>*B5#ao^jjZF~FD~uVq z#%ZpXY>{)vFRjLT{`$m%N!p;klwImWY1MdKo~+D`+GB>3ojXE*&PbmB&S&;m{7ZP) zp6ee|5s$l=p^qoa2`6lB^Zo89ylXlg_Sx!}%Xe=m+dwQM0y5McfrKQ+OC@(mILC+3 z4@k9U(2+RQ%=6n%&3Qourj=5lOE07EYUJMc)x>YWO=#ypLDK4G@A86|j z7V6AqJ|#Rb)LC#`dkr~YKTMgxUd5PvF0MEX%SN}sEiMNlK3dlYn&T2p<>Pp6G>Wyi zil77sYG~ah@x0HrTI9e5{gF;Nea;O)SXn@w#q0O!o%!DVf|sS62*k`IZ5 zYR|qo=~0__r>(bTw_kc;K4S8I^Lm&u2r_=~3ZkP)TJ4cc(|bhfNAiv=3c=0e48Iew z&c>k=2mt(z6g39w_!mrU=NnSq?s%N4KMuPruokqqkTvgy!Mxg7_vd9>4A@mrn~RGp z^ckcW!+W4FgRl3f+Q+?@3z7#9?(U7LJy&;UCxI@gYt%Sn;o*2MGbs=V6np<+=^*@m z+;MheZXN?A4b(T{WXDz%0?1(q`v0Vgo$`k?M1M0sP^^C!{+ki~Sq1 new Promise((resolve) => { @@ -23,10 +23,7 @@ const removeAppById = (id: App['id']) => export const cleanupApps = async () => { const apps = await getApps(); - const testApp = apps.find((app) => app.name === APP_NAME); - if (testApp) { - await removeAppById(testApp.id); - } + await Promise.all(apps.map((testApp) => removeAppById(testApp.id))); }; export const installTestApp = () => @@ -41,3 +38,17 @@ export const installTestApp = () => resolve(res.body.app); }); }); + +export const installLocalTestPackage = (path: string) => + new Promise((resolve, reject) => { + void request + .post(apps()) + .set(credentials) + .attach('app', path) + .end((err, res) => { + if (err) { + return reject(err); + } + return resolve(res.body.app); + }); + }); diff --git a/apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.ts b/apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.ts new file mode 100644 index 0000000000000..9cd3576705f52 --- /dev/null +++ b/apps/meteor/tests/end-to-end/apps/apps-hooks-file-upload.ts @@ -0,0 +1,56 @@ +import type { IRoom } from '@rocket.chat/core-typings'; +import { expect } from 'chai'; +import { after, before, describe, it } from 'mocha'; +import type { Response } from 'supertest'; + +import { getCredentials, request, credentials, api } from '../../data/api-data'; +import { appImplementsIPreFileUpload } from '../../data/apps/app-packages'; +import { cleanupApps, installLocalTestPackage } from '../../data/apps/helper'; +import { createRoom, deleteRoom } from '../../data/rooms.helper'; +import { IS_EE } from '../../e2e/config/constants'; + +(IS_EE ? describe : describe.skip)('[Apps Hooks - File Upload]', () => { + before((done) => getCredentials(done)); + + describe('IPreFileUpload', () => { + let room: IRoom; + + before(async () => { + await cleanupApps(); + await installLocalTestPackage(appImplementsIPreFileUpload); + room = await createRoom({ type: 'c', name: `file-upload-hook-${Date.now()}` }).then((res) => res.body.channel); + }); + + after(() => Promise.all([deleteRoom({ type: 'c' as const, roomId: room._id }), cleanupApps()])); + + it('should be capable of rejecting an upload based on app logic', async () => { + const fileContents = 'I want to be rejected by the app'; + + await request + .post(api(`rooms.media/${room._id}`)) + .set(credentials) + .attach('file', Buffer.from(fileContents), { filename: 'test-should-reject' }) + .expect('Content-Type', 'application/json') + .expect(400) + .expect((res: Response) => { + expect(res.body).to.have.property('success', false); + expect(res.body).to.have.property('errorType', 'error-app-prevented'); + expect(res.body).to.have.property('error').that.is.string(fileContents); + }); + }); + + it('should not prevent an unrelated file upload', async () => { + const fileContents = 'I should not be rejected'; + + await request + .post(api(`rooms.media/${room._id}`)) + .set(credentials) + .attach('file', Buffer.from(fileContents), { filename: 'test-file' }) + .expect('Content-Type', 'application/json') + .expect(200) + .expect((res: Response) => { + expect(res.body).to.have.property('success', true); + }); + }); + }); +}); diff --git a/packages/apps-engine/deno-runtime/deno.jsonc b/packages/apps-engine/deno-runtime/deno.jsonc index cefd268cf8856..525eda492feb4 100644 --- a/packages/apps-engine/deno-runtime/deno.jsonc +++ b/packages/apps-engine/deno-runtime/deno.jsonc @@ -4,6 +4,7 @@ "@rocket.chat/apps-engine/": "./../src/", "@rocket.chat/ui-kit": "npm:@rocket.chat/ui-kit@^0.31.22", "@std/cli": "jsr:@std/cli@^1.0.9", + "@std/streams": "jsr:@std/streams@^1.0.16", "acorn": "npm:acorn@8.10.0", "acorn-walk": "npm:acorn-walk@8.2.0", "astring": "npm:astring@1.8.6", diff --git a/packages/apps-engine/deno-runtime/deno.lock b/packages/apps-engine/deno-runtime/deno.lock index 61763f056cce3..6dbfd05882fe2 100644 --- a/packages/apps-engine/deno-runtime/deno.lock +++ b/packages/apps-engine/deno-runtime/deno.lock @@ -2,7 +2,9 @@ "version": "3", "packages": { "specifiers": { + "jsr:@std/bytes@^1.0.6": "jsr:@std/bytes@1.0.6", "jsr:@std/cli@^1.0.9": "jsr:@std/cli@1.0.13", + "jsr:@std/streams@^1.0.16": "jsr:@std/streams@1.0.16", "npm:@msgpack/msgpack@3.0.0-beta2": "npm:@msgpack/msgpack@3.0.0-beta2", "npm:@rocket.chat/ui-kit@^0.31.22": "npm:@rocket.chat/ui-kit@0.31.25_@rocket.chat+icons@0.32.0", "npm:acorn-walk@8.2.0": "npm:acorn-walk@8.2.0", @@ -14,8 +16,17 @@ "npm:uuid@8.3.2": "npm:uuid@8.3.2" }, "jsr": { + "@std/bytes@1.0.6": { + "integrity": "f6ac6adbd8ccd99314045f5703e23af0a68d7f7e58364b47d2c7f408aeb5820a" + }, "@std/cli@1.0.13": { "integrity": "5db2d95ab2dca3bca9fb6ad3c19908c314e93d6391c8b026725e4892d4615a69" + }, + "@std/streams@1.0.16": { + "integrity": "85030627befb1767c60d4f65cb30fa2f94af1d6ee6e5b2515b76157a542e89c4", + "dependencies": [ + "jsr:@std/bytes@^1.0.6" + ] } }, "npm": { @@ -102,6 +113,7 @@ "workspace": { "dependencies": [ "jsr:@std/cli@^1.0.9", + "jsr:@std/streams@^1.0.16", "npm:@msgpack/msgpack@3.0.0-beta2", "npm:@rocket.chat/ui-kit@^0.31.22", "npm:acorn-walk@8.2.0", diff --git a/packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts b/packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts new file mode 100644 index 0000000000000..f5dd11a442a1f --- /dev/null +++ b/packages/apps-engine/deno-runtime/handlers/app/handleUploadEvents.ts @@ -0,0 +1,73 @@ +import { Buffer } from 'node:buffer'; + +import type { App } from '@rocket.chat/apps-engine/definition/App.ts'; +import { AppsEngineException } from '@rocket.chat/apps-engine/definition/exceptions/AppsEngineException.ts'; +import type { IFileUploadContext } from '@rocket.chat/apps-engine/definition/uploads/IFileUploadContext.ts' +import type { IUpload } from '@rocket.chat/apps-engine/definition/uploads/IUpload.ts' +import { toArrayBuffer } from '@std/streams'; +import { Defined, JsonRpcError } from 'jsonrpc-lite'; + +import { AppObjectRegistry } from '../../AppObjectRegistry.ts'; +import { assertAppAvailable, assertHandlerFunction, isRecord } from '../lib/assertions.ts'; +import { AppAccessorsInstance } from '../../lib/accessors/mod.ts'; + +export const uploadEvents = ['executePreFileUpload'] as const; + +function assertIsUpload(v: unknown): asserts v is IUpload { + if (isRecord(v) && isRecord(v.user) && isRecord(v.room)) return; + + throw JsonRpcError.invalidParams({ err: `Invalid 'file' parameter. Expected IUpload, got`, value: v }); +} + +function assertString(v: unknown): asserts v is string { + if (v && typeof v === 'string') return; + + throw JsonRpcError.invalidParams({ err: `Invalid 'path' parameter. Expected string, got`, value: v }); +} + +export default async function handleUploadEvents(method: typeof uploadEvents[number], params: unknown): Promise { + const [{ file, path }] = params as [{ file?: IUpload, path?: string }]; + + const app = AppObjectRegistry.get('app'); + const handlerFunction = app?.[method as keyof App] as unknown; + + try { + assertAppAvailable(app); + assertHandlerFunction(handlerFunction); + assertIsUpload(file); + assertString(path); + + using tempFile = await Deno.open(path, { read: true, create: false }); + let context: IFileUploadContext; + + switch (method) { + case 'executePreFileUpload': { + const fileContents = await toArrayBuffer(tempFile.readable); + context = { file, content: Buffer.from(fileContents) }; + break; + } + } + + return await handlerFunction.call( + app, + context, + AppAccessorsInstance.getReader(), + AppAccessorsInstance.getHttp(), + AppAccessorsInstance.getPersistence(), + AppAccessorsInstance.getModifier(), + ); + } catch(e) { + if (e?.name === AppsEngineException.name) { + return new JsonRpcError(e.message, AppsEngineException.JSONRPC_ERROR_CODE, { name: e.name }); + } + + if (e instanceof JsonRpcError) { + return e; + } + + return JsonRpcError.internalError({ + err: e.message, + ...(e.code && { code: e.code }), + }); + } +} diff --git a/packages/apps-engine/deno-runtime/handlers/app/handler.ts b/packages/apps-engine/deno-runtime/handlers/app/handler.ts index 171d179a2d05d..cfb2df08cfb69 100644 --- a/packages/apps-engine/deno-runtime/handlers/app/handler.ts +++ b/packages/apps-engine/deno-runtime/handlers/app/handler.ts @@ -15,6 +15,8 @@ import handleListener from '../listener/handler.ts'; import handleUIKitInteraction, { uikitInteractions } from '../uikit/handler.ts'; import { AppObjectRegistry } from '../../AppObjectRegistry.ts'; import handleOnUpdate from './handleOnUpdate.ts'; +import handleUploadEvents, { uploadEvents } from './handleUploadEvents.ts'; +import { isOneOf } from '../lib/assertions.ts'; export default async function handleApp(method: string, params: unknown): Promise { const [, appMethod] = method.split(':'); @@ -30,46 +32,35 @@ export default async function handleApp(method: string, params: unknown): Promis app?.getLogger().debug({ msg: `A method is being called...`, appMethod }); - if (uikitInteractions.includes(appMethod)) { - return handleUIKitInteraction(appMethod, params).then((result) => { - if (result instanceof JsonRpcError) { - app?.getLogger().debug({ - msg: `Method call was unsuccessful.`, - appMethod, - err: result, - errorMessage: result.message, - }); - } else { - app?.getLogger().debug({ - msg: `Method was successfully called! The result is:`, - appMethod, - result, - }); - } - - return result; - }); + const formatResult = (result: Defined | JsonRpcError): Defined | JsonRpcError => { + if (result instanceof JsonRpcError) { + app?.getLogger().debug({ + msg: `'${appMethod}' was unsuccessful.`, + appMethod, + err: result, + errorMessage: result.message, + }); + } else { + app?.getLogger().debug({ + msg: `'${appMethod}' was successfully called! The result is:`, + appMethod, + result, + }); + } + + return result; + }; + + if (app && isOneOf(appMethod, uploadEvents)) { + return handleUploadEvents(appMethod, params).then(formatResult); } - if (appMethod.startsWith('check') || appMethod.startsWith('execute')) { - return handleListener(appMethod, params).then((result) => { - if (result instanceof JsonRpcError) { - app?.getLogger().debug({ - msg: `'${appMethod}' was unsuccessful.`, - appMethod, - err: result, - errorMessage: result.message, - }); - } else { - app?.getLogger().debug({ - msg: `'${appMethod}' was successfully called! The result is:`, - appMethod, - result, - }); - } - - return result; - }); + if (app && isOneOf(appMethod, uikitInteractions)) { + return handleUIKitInteraction(appMethod, params).then(formatResult); + } + + if (app && (appMethod.startsWith('check') || appMethod.startsWith('execute'))) { + return handleListener(appMethod, params).then(formatResult); } let result: Defined | JsonRpcError; diff --git a/packages/apps-engine/deno-runtime/handlers/lib/assertions.ts b/packages/apps-engine/deno-runtime/handlers/lib/assertions.ts new file mode 100644 index 0000000000000..c154015d24b02 --- /dev/null +++ b/packages/apps-engine/deno-runtime/handlers/lib/assertions.ts @@ -0,0 +1,33 @@ +import type { App } from '@rocket.chat/apps-engine/definition/App.ts'; +import { JsonRpcError } from 'jsonrpc-lite'; + +export function isRecord(v: unknown): v is Record { + if (!v || typeof v !== 'object') { + return false; + } + + const prototype = Object.getPrototypeOf(v); + + return prototype === null || prototype.constructor === Object; +} + +/** + * Type guard function to check if a value is included in a readonly array + * and narrow its type accordingly. + */ +export function isOneOf(value: unknown, array: readonly T[]): value is T { + return array.includes(value as T); +} + +export function assertAppAvailable(v: unknown): asserts v is App { + if (v && typeof (v as App)['extendConfiguration'] === 'function') return; + + throw JsonRpcError.internalError({ err: 'App object not available' }); +} + +// deno-lint-ignore ban-types -- Function is the best we can do at this time +export function assertHandlerFunction(v: unknown): asserts v is Function { + if (v instanceof Function) return; + + throw JsonRpcError.internalError({ err: `Expected handler function, got ${v}` }); +} diff --git a/packages/apps-engine/src/definition/uploads/IFileUploadContext.ts b/packages/apps-engine/src/definition/uploads/IFileUploadContext.ts index cd0de6127029b..3aa78e24bcc03 100644 --- a/packages/apps-engine/src/definition/uploads/IFileUploadContext.ts +++ b/packages/apps-engine/src/definition/uploads/IFileUploadContext.ts @@ -1,5 +1,10 @@ import type { IUploadDetails } from './IUploadDetails'; +export interface IFileUploadInternalContext { + file: IUploadDetails; + path: string; +} + export interface IFileUploadContext { file: IUploadDetails; content: Buffer; diff --git a/packages/apps-engine/src/server/AppManager.ts b/packages/apps-engine/src/server/AppManager.ts index 4c8348b726653..826b2b1b95148 100644 --- a/packages/apps-engine/src/server/AppManager.ts +++ b/packages/apps-engine/src/server/AppManager.ts @@ -54,6 +54,12 @@ export interface IAppManagerDeps { logStorage: AppLogStorage; bridges: AppBridges; sourceStorage: AppSourceStorage; + /** + * Path to temporary file storage. + * + * Needs to be accessible for reading and writing. + */ + tempFilePath: string; } interface IPurgeAppConfigOpts { @@ -106,9 +112,11 @@ export class AppManager { private readonly runtime: AppRuntimeManager; + private readonly tempFilePath: string; + private isLoaded: boolean; - constructor({ metadataStorage, logStorage, bridges, sourceStorage }: IAppManagerDeps) { + constructor({ metadataStorage, logStorage, bridges, sourceStorage, tempFilePath }: IAppManagerDeps) { // Singleton style. There can only ever be one AppManager instance if (typeof AppManager.Instance !== 'undefined') { throw new Error('There is already a valid AppManager instance'); @@ -138,6 +146,8 @@ export class AppManager { throw new Error('Invalid instance of the AppSourceStorage'); } + this.tempFilePath = tempFilePath; + this.apps = new Map(); this.parser = new AppPackageParser(); @@ -160,6 +170,15 @@ export class AppManager { AppManager.Instance = this; } + /** + * Gets the path to the temporary file storage. + * + * Mainly used for upload events + */ + public getTempFilePath(): string { + return this.tempFilePath; + } + /** Gets the instance of the storage connector. */ public getStorage(): AppMetadataStorage { return this.appMetadataStorage; diff --git a/packages/apps-engine/src/server/ProxiedApp.ts b/packages/apps-engine/src/server/ProxiedApp.ts index fb49615b736dc..d4eb00bbb6666 100644 --- a/packages/apps-engine/src/server/ProxiedApp.ts +++ b/packages/apps-engine/src/server/ProxiedApp.ts @@ -1,3 +1,5 @@ +import { inspect } from 'util'; + import * as mem from 'mem'; import type { AppManager } from './AppManager'; @@ -78,7 +80,7 @@ export class ProxiedApp { // Range of JSON-RPC error codes: https://www.jsonrpc.org/specification#error_object if (e.code >= -32999 || e.code <= -32000) { // we really need to receive a logger from rocket.chat - console.error('JSON-RPC error received: ', e); + console.error('JSON-RPC error received: ', inspect(e, { depth: 10 })); } } } diff --git a/packages/apps-engine/src/server/compiler/AppImplements.ts b/packages/apps-engine/src/server/compiler/AppImplements.ts index eafbb2d2dd411..ba9be27b678bc 100644 --- a/packages/apps-engine/src/server/compiler/AppImplements.ts +++ b/packages/apps-engine/src/server/compiler/AppImplements.ts @@ -2,26 +2,31 @@ import { AppInterface } from '../../definition/metadata/AppInterface'; import { Utilities } from '../misc/Utilities'; export class AppImplements { - private implemented: { [key: string]: boolean }; + private implemented: Record; constructor() { - this.implemented = {}; - Object.keys(AppInterface).forEach((int) => { + this.implemented = {} as Record; + + Object.keys(AppInterface).forEach((int: AppInterface) => { this.implemented[int] = false; }); } - public doesImplement(int: string): void { + public setImplements(int: AppInterface): void { if (int in AppInterface) { this.implemented[int] = true; } } - public getValues(): { [int: string]: boolean } { + public doesImplement(int: AppInterface): boolean { + return this.implemented[int]; + } + + public getValues(): Record { return Utilities.deepCloneAndFreeze(this.implemented); } - public toJSON(): { [int: string]: boolean } { + public toJSON(): Record { return this.getValues(); } } diff --git a/packages/apps-engine/src/server/compiler/AppPackageParser.ts b/packages/apps-engine/src/server/compiler/AppPackageParser.ts index 073d3dbffd646..757fe60878bed 100644 --- a/packages/apps-engine/src/server/compiler/AppPackageParser.ts +++ b/packages/apps-engine/src/server/compiler/AppPackageParser.ts @@ -85,7 +85,7 @@ export class AppPackageParser { const implemented = new AppImplements(); if (Array.isArray(info.implements)) { - info.implements.forEach((interfaceName) => implemented.doesImplement(interfaceName)); + info.implements.forEach((interfaceName) => implemented.setImplements(interfaceName)); } return { diff --git a/packages/apps-engine/src/server/managers/AppListenerManager.ts b/packages/apps-engine/src/server/managers/AppListenerManager.ts index 273f969ce1045..82f8a628a560e 100644 --- a/packages/apps-engine/src/server/managers/AppListenerManager.ts +++ b/packages/apps-engine/src/server/managers/AppListenerManager.ts @@ -25,7 +25,7 @@ import type { IUIKitIncomingInteractionModalContainer, } from '../../definition/uikit/UIKitIncomingInteractionContainer'; import type { IUIKitLivechatBlockIncomingInteraction, IUIKitLivechatIncomingInteraction } from '../../definition/uikit/livechat'; -import type { IFileUploadContext } from '../../definition/uploads/IFileUploadContext'; +import type { IFileUploadInternalContext } from '../../definition/uploads/IFileUploadContext'; import type { IUser, IUserContext, IUserStatusContext, IUserUpdateContext } from '../../definition/users'; import type { AppManager } from '../AppManager'; import type { ProxiedApp } from '../ProxiedApp'; @@ -205,7 +205,7 @@ interface IListenerExecutor { }; // FileUpload [AppInterface.IPreFileUpload]: { - args: [IFileUploadContext]; + args: [IFileUploadInternalContext]; result: void; }; // Email @@ -448,7 +448,7 @@ export class AppListenerManager { return this.executePostLivechatGuestSaved(data as IVisitor); // FileUpload case AppInterface.IPreFileUpload: - return this.executePreFileUpload(data as IFileUploadContext); + return this.executePreFileUpload(data as IFileUploadInternalContext); // Email case AppInterface.IPreEmailSent: return this.executePreEmailSent(data as IPreEmailSentContext); @@ -1170,7 +1170,7 @@ export class AppListenerManager { } // FileUpload - private async executePreFileUpload(data: IFileUploadContext): Promise { + private async executePreFileUpload(data: IFileUploadInternalContext): Promise { for (const appId of this.listeners.get(AppInterface.IPreFileUpload)) { const app = this.manager.getOneById(appId); diff --git a/packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts b/packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts index 5b62d31051557..8b4eb385a88f8 100644 --- a/packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts +++ b/packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts @@ -11,7 +11,7 @@ import { ProcessMessenger } from './ProcessMessenger'; import { bundleLegacyApp } from './bundler'; import { newDecoder } from './codec'; import { AppStatus, AppStatusUtils } from '../../../definition/AppStatus'; -import type { AppMethod } from '../../../definition/metadata'; +import { AppInterface, AppMethod } from '../../../definition/metadata'; import type { AppManager } from '../../AppManager'; import type { AppBridges } from '../../bridges'; import type { IParseAppPackageResult } from '../../compiler'; @@ -115,6 +115,8 @@ export class DenoRuntimeSubprocessController extends EventEmitter implements IRu private readonly livenessManager: LivenessManager; + private readonly tempFilePath: string; + // We need to keep the appSource around in case the Deno process needs to be restarted constructor( manager: AppManager, @@ -137,6 +139,7 @@ export class DenoRuntimeSubprocessController extends EventEmitter implements IRu this.api = manager.getApiManager(); this.logStorage = manager.getLogStorage(); this.bridges = manager.getBridges(); + this.tempFilePath = manager.getTempFilePath(); } public spawnProcess(): void { @@ -151,10 +154,17 @@ export class DenoRuntimeSubprocessController extends EventEmitter implements IRu // process must be able to read in order to include files that use NPM packages const parentNodeModulesDir = path.dirname(path.join(appsEngineDir, '..')); + const allowedDirs = [appsEngineDir, parentNodeModulesDir]; + + // If the app handles file upload events, it needs to be able to read the temp dir + if (this.appPackage.implemented.doesImplement(AppInterface.IPreFileUpload)) { + allowedDirs.push(this.tempFilePath); + } + const options = [ 'run', '--cached-only', - `--allow-read=${appsEngineDir},${parentNodeModulesDir}`, + `--allow-read=${allowedDirs.join(',')}`, `--allow-env=${ALLOWED_ENVIRONMENT_VARIABLES.join(',')}`, denoWrapperPath, '--subprocess', diff --git a/packages/apps-engine/tests/server/compiler/AppImplements.spec.ts b/packages/apps-engine/tests/server/compiler/AppImplements.spec.ts index a848fbd7a0769..e3e6240ffba93 100644 --- a/packages/apps-engine/tests/server/compiler/AppImplements.spec.ts +++ b/packages/apps-engine/tests/server/compiler/AppImplements.spec.ts @@ -11,9 +11,10 @@ export class AppImplementsTestFixture { const impls = new AppImplements(); Expect(impls.getValues()).toBeDefined(); - Expect(() => impls.doesImplement(AppInterface.IPreMessageSentPrevent)).not.toThrow(); + Expect(() => impls.setImplements(AppInterface.IPreMessageSentPrevent)).not.toThrow(); + Expect(impls.doesImplement(AppInterface.IPreMessageSentPrevent)).toBe(true); + Expect(impls.doesImplement(AppInterface.IPostMessageDeleted)).toBe(false); Expect(impls.getValues()[AppInterface.IPreMessageSentPrevent]).toBe(true); - Expect(() => impls.doesImplement('Something')).not.toThrow(); - Expect(impls.getValues().Something).not.toBeDefined(); + Expect(impls.getValues()[AppInterface.IPostMessageDeleted]).toBe(false); } } diff --git a/packages/apps-engine/tests/test-data/utilities.ts b/packages/apps-engine/tests/test-data/utilities.ts index 9cd3135ed061c..7d976d08113e2 100644 --- a/packages/apps-engine/tests/test-data/utilities.ts +++ b/packages/apps-engine/tests/test-data/utilities.ts @@ -130,6 +130,7 @@ export class TestInfastructureSetup { getRuntime: () => { return this.runtimeManager; }, + getTempFilePath: () => 'temp-file-path', } as unknown as AppManager; } From 91601f7667a6f1abfdb5e5d5900dd67276855493 Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Sat, 10 Jan 2026 19:05:26 -0300 Subject: [PATCH 32/32] feat: adapt app upload event trigger to new upload changes --- .../app/apps/server/bridges/listeners.js | 21 ++++++++++++++----- .../app/file-upload/server/lib/FileUpload.ts | 11 +++++----- apps/meteor/server/ufs/ufs-filter.ts | 6 +++--- 3 files changed, 24 insertions(+), 14 deletions(-) diff --git a/apps/meteor/app/apps/server/bridges/listeners.js b/apps/meteor/app/apps/server/bridges/listeners.js index 8f2b5b78a113d..b62016c239670 100644 --- a/apps/meteor/app/apps/server/bridges/listeners.js +++ b/apps/meteor/app/apps/server/bridges/listeners.js @@ -83,18 +83,29 @@ export class AppListenerBridge { /** * - * @param {{ file: import('@rocket.chat/core-typings').IUpload; content: Buffer }} payload + * @param {{ file: import('@rocket.chat/core-typings').IUpload; content: Buffer | string }} payload * @return Promise */ async uploadEvent(_, payload) { const { file, content } = payload; const tmpfile = path.join(this.orch.getManager().getTempFilePath(), crypto.randomUUID()); - await fs.promises.writeFile(tmpfile, content).catch((err) => { - this.orch.getRocketChatLogger().error({ msg: `AppListenerBridge: Could not write temporary file at ${tmpfile}`, err }); - throw new Error('Error sending file to apps', { cause: err }); - }); + if (typeof content === 'string') { + // If content is a string, we assume it's a path and create a symlink to avoid file duplication + await fs.promises.symlink(content, tmpfile, 'file').catch((err) => { + this.orch.getRocketChatLogger().error({ msg: `AppListenerBridge: Could not create symlink at ${tmpfile}`, err }); + + throw new Error('Error sending file to apps', { cause: err }); + }); + } else { + // Otherwise, we write the buffer content to a temporary file + await fs.promises.writeFile(tmpfile, content).catch((err) => { + this.orch.getRocketChatLogger().error({ msg: `AppListenerBridge: Could not write temporary file at ${tmpfile}`, err }); + + throw new Error('Error sending file to apps', { cause: err }); + }); + } try { const appFile = await this.orch.getConverters().get('uploads').convertToApp(file); diff --git a/apps/meteor/app/file-upload/server/lib/FileUpload.ts b/apps/meteor/app/file-upload/server/lib/FileUpload.ts index 2b58ca82cd87c..060c2158b6f37 100644 --- a/apps/meteor/app/file-upload/server/lib/FileUpload.ts +++ b/apps/meteor/app/file-upload/server/lib/FileUpload.ts @@ -136,7 +136,7 @@ export const FileUpload = { ); }, - async validateFileUpload(file: IUpload, content?: Buffer) { + async validateFileUpload(file: IUpload, content?: Buffer | string) { if (!Match.test(file.rid, String)) { return false; } @@ -191,7 +191,7 @@ export const FileUpload = { // App IPreFileUpload event hook try { - await Apps.self?.triggerEvent(AppEvents.IPreFileUpload, { file, content: content || Buffer.from([]) }); + await Apps.self?.triggerEvent(AppEvents.IPreFileUpload, { file, content }); } catch (error: any) { if (error.name === AppsEngineException.name) { throw new Meteor.Error('error-app-prevented', error.message); @@ -817,13 +817,12 @@ export class FileUploadClass { // and for security reasons we must not write it to disk before validation content = await streamToBuffer(content); } else if (content instanceof Uint8Array && !(content instanceof Buffer)) { - // Services compat - convert Uint8Array to Buffer - content = Buffer.from(content); + // Services compat - create a view into the underlying ArrayBuffer without copying the data + content = Buffer.from(content.buffer); } try { - // TODO: Make filter.check accept file path as string - check Apps Engine IPreFileUpload hook - await filter.check(fileData, typeof content === 'string' ? undefined : content); + await filter.check(fileData, content); return content; } catch (e) { throw e; diff --git a/apps/meteor/server/ufs/ufs-filter.ts b/apps/meteor/server/ufs/ufs-filter.ts index d73055c7e1f65..071402ff676a1 100644 --- a/apps/meteor/server/ufs/ufs-filter.ts +++ b/apps/meteor/server/ufs/ufs-filter.ts @@ -7,7 +7,7 @@ type IFilterOptions = { extensions?: string[]; minSize?: number; maxSize?: number; - onCheck?: (file: IUpload, content?: Buffer) => Promise; + onCheck?: (file: IUpload, content?: Buffer | string) => Promise; invalidFileError?: () => Meteor.Error; fileTooSmallError?: (fileSize: number, minFileSize: number) => Meteor.Error; fileTooLargeError?: (fileSize: number, maxFileSize: number) => Meteor.Error; @@ -59,7 +59,7 @@ export class Filter { } } - async check(file: OptionalId, content?: ReadableStream | Buffer) { + async check(file: OptionalId, content?: Buffer | string) { let error = null; if (typeof file !== 'object' || !file) { error = this.options.invalidFileError(); @@ -137,7 +137,7 @@ export class Filter { return result; } - async onCheck(_file: OptionalId, _content?: ReadableStream | Buffer) { + async onCheck(_file: OptionalId, _content?: Buffer | string): Promise { return true; } }