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/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/lib/MultipartUploadHandler.ts b/apps/meteor/app/api/server/lib/MultipartUploadHandler.ts new file mode 100644 index 0000000000000..dafa07b85cc01 --- /dev/null +++ b/apps/meteor/app/api/server/lib/MultipartUploadHandler.ts @@ -0,0 +1,203 @@ +import fs from 'fs'; +import { IncomingMessage } from 'http'; +import type { Stream, Transform } from 'stream'; +import { Readable } from 'stream'; +import { pipeline } from 'stream/promises'; + +import { MeteorError } from '@rocket.chat/core-services'; +import { Random } from '@rocket.chat/random'; +import busboy, { type BusboyConfig } from 'busboy'; +import ExifTransformer from 'exif-be-gone'; + +import { UploadFS } from '../../../../server/ufs'; +import { getMimeType } from '../../../utils/lib/mimeTypes'; + +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 MultipartUploadHandler { + static transforms = { + stripExif(): Transform { + return new ExifTransformer(); + }, + }; + + static async cleanup(tempFilePath: string): Promise { + try { + await fs.promises.unlink(tempFilePath); + } catch (error: any) { + console.warn(`[UploadService] Failed to cleanup temp file: ${tempFilePath}`, error); + } + } + + static async stripExifFromFile(tempFilePath: string): Promise { + const strippedPath = `${tempFilePath}.stripped`; + + try { + const writeStream = fs.createWriteStream(strippedPath); + + await pipeline(fs.createReadStream(tempFilePath), new ExifTransformer(), writeStream); + + await fs.promises.rename(strippedPath, tempFilePath); + + return writeStream.bytesWritten; + } catch (error) { + void this.cleanup(strippedPath); + + throw error; + } + } + + static async parseRequest( + request: IncomingMessage | Request, + options: ParseOptions, + ): Promise<{ file: ParsedUpload | null; fields: Record }> { + 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 + limits.fileSize = options.maxSize + 1; + } + + const headers = + request instanceof IncomingMessage ? (request.headers as Record) : Object.fromEntries(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 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 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); + } + } + + currentStream.pipe(writeStream); + + writeStream.on('finish', () => { + if (file.truncated) { + void this.cleanup(tempFilePath); + return reject(new MeteorError('error-file-too-large', 'File size exceeds the allowed limit')); + } + + parsedFile = { + tempFilePath, + filename, + mimetype: getMimeType(mimeType, filename), + size: writeStream.bytesWritten, + fieldname, + }; + writeStreamFinished = true; + tryResolve(); + }); + + writeStream.on('error', (err) => { + file.destroy(); + void this.cleanup(tempFilePath); + reject(new MeteorError('error-file-upload', err.message)); + }); + + file.on('error', (err) => { + writeStream.destroy(); + void this.cleanup(tempFilePath); + reject(new MeteorError('error-file-upload', err.message)); + }); + }); + + bb.on('finish', () => { + busboyFinished = true; + tryResolve(); + }); + + bb.on('error', (err: any) => { + reject(new MeteorError('error-upload-failed', err.message)); + }); + + bb.on('filesLimit', () => { + reject(new MeteorError('error-too-many-files', 'Too many files in upload')); + }); + + 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 (request instanceof IncomingMessage) { + request.pipe(bb); + } else { + if (!request.body) { + return Promise.reject(new MeteorError('error-no-body', 'Request has no body')); + } + + const nodeStream = Readable.fromWeb(request.body as any); + nodeStream.pipe(bb); + } + + return promise; + } +} 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, 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 f0c4bd6d51009..b89be1304b7c5 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 { MultipartUploadHandler } from '../lib/MultipartUploadHandler'; import { findAdminRoom, findAdminRooms, @@ -197,24 +197,18 @@ API.v1.addRoute( return API.v1.forbidden(); } - const file = await getUploadFormData( - { - request: this.request, - }, - { field: 'file', sizeLimit: settings.get('FileUpload_MaxFileSize') }, - ); + const { file, fields } = await MultipartUploadHandler.parseRequest(this.incoming, { + field: 'file', + maxSize: settings.get('FileUpload_MaxFileSize'), + }); 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 +222,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 +230,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 || '')}`); diff --git a/apps/meteor/app/apps/server/bridges/listeners.js b/apps/meteor/app/apps/server/bridges/listeners.js index 31aa2c0052695..b62016c239670 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,46 @@ export class AppListenerBridge { return this.orch.getManager().getListenerManager().executeListener(inte, 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()); + + 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); + + // 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/app/file-upload/server/lib/FileUpload.spec.ts b/apps/meteor/app/file-upload/server/lib/FileUpload.spec.ts index 73249fea904a0..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,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/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 73f67fb473744..060c2158b6f37 100644 --- a/apps/meteor/app/file-upload/server/lib/FileUpload.ts +++ b/apps/meteor/app/file-upload/server/lib/FileUpload.ts @@ -5,7 +5,9 @@ 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 { isArrayBufferView } from 'util/types'; import { hashLoginToken } from '@rocket.chat/account-utils'; import { Apps, AppEvents } from '@rocket.chat/apps'; @@ -29,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 { 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'; @@ -133,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; } @@ -188,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); @@ -369,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; @@ -389,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 (shouldStripExif) { + // If there is no EXIF orientation but the setting is enabled, still strip any metadata + size = await MultipartUploadHandler.stripExifFromFile(tmpFile); + } - const { size } = await fs.lstatSync(tmpFile); await this.getCollection().updateOne( { _id: file._id }, { @@ -789,51 +802,64 @@ export class FileUploadClass { return store.delete(file._id); } + 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 - create a view into the underlying ArrayBuffer without copying the data + content = Buffer.from(content.buffer); + } + + try { + await filter.check(fileData, 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 (isArrayBufferView(content)) { + await fs.promises.writeFile(tmpFile, content); + } else if (content instanceof stream.Readable) { + await finished(content.pipe(fs.createWriteStream(tmpFile)), { cleanup: true }); } 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); } } diff --git a/apps/meteor/app/livechat/imports/server/rest/upload.ts b/apps/meteor/app/livechat/imports/server/rest/upload.ts index 8cb0a0511eade..14cd2ae29e077 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 { MultipartUploadHandler } from '../../../../api/server/lib/MultipartUploadHandler'; 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 MultipartUploadHandler.parseRequest(this.request, { + 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/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/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); } } 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; } } 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-methods.ts b/apps/meteor/server/ufs/ufs-methods.ts index 768aefdcee3dc..55c18f6ca066c 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,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 }); - // removeTempFile(); // todo remove temp file on error or try again ? reject(new Meteor.Error('ufs: cannot upload file', err)); } }); diff --git a/apps/meteor/server/ufs/ufs-store.ts b/apps/meteor/server/ufs/ufs-store.ts index 5eb44fdd2a137..3c6d6c983850f 100644 --- a/apps/meteor/server/ufs/ufs-store.ts +++ b/apps/meteor/server/ufs/ufs-store.ts @@ -166,25 +166,12 @@ export class Store { }; const finishHandler = async () => { - let size = 0; - const readStream = await this.getReadStream(fileId, file); - - 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 +204,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); @@ -238,11 +227,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 }); 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 0000000000000..05183f3f05cb2 Binary files /dev/null and b/apps/meteor/tests/data/apps/app-packages/file-upload-test_0.0.1.zip differ diff --git a/apps/meteor/tests/data/apps/app-packages/index.ts b/apps/meteor/tests/data/apps/app-packages/index.ts new file mode 100644 index 0000000000000..d221164b6a5cb --- /dev/null +++ b/apps/meteor/tests/data/apps/app-packages/index.ts @@ -0,0 +1,3 @@ +import * as path from 'path'; + +export const appImplementsIPreFileUpload = path.resolve(__dirname, './file-upload-test_0.0.1.zip'); diff --git a/apps/meteor/tests/data/apps/helper.ts b/apps/meteor/tests/data/apps/helper.ts index dfa945a470dc1..7871b51dafb3d 100644 --- a/apps/meteor/tests/data/apps/helper.ts +++ b/apps/meteor/tests/data/apps/helper.ts @@ -1,7 +1,7 @@ import type { App } from '@rocket.chat/core-typings'; import { request, credentials } from '../api-data'; -import { apps, APP_URL, APP_NAME, installedApps } from './apps-data'; +import { apps, APP_URL, installedApps } from './apps-data'; const getApps = () => 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/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 () => { 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); }); 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; } 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 () => { 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(); }