diff --git a/.changeset/gold-kids-move.md b/.changeset/gold-kids-move.md new file mode 100644 index 0000000000000..9a3c402b231e5 --- /dev/null +++ b/.changeset/gold-kids-move.md @@ -0,0 +1,7 @@ +--- +"@rocket.chat/meteor": patch +"@rocket.chat/core-services": patch +"@rocket.chat/i18n": patch +--- + +Changes OEmbed URL processing. Now, the processing is done asynchronously and has a configurable timeout for each request. Additionally, the `API_EmbedIgnoredHosts` setting now accepts wildcard domains. diff --git a/.changeset/twelve-sheep-accept.md b/.changeset/twelve-sheep-accept.md new file mode 100644 index 0000000000000..92c7ec130d1d3 --- /dev/null +++ b/.changeset/twelve-sheep-accept.md @@ -0,0 +1,6 @@ +--- +'@rocket.chat/http-router': patch +'@rocket.chat/meteor': patch +--- + +Improves file upload flow to prevent buffering of contents in memory 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..467cf016d0d68 --- /dev/null +++ b/apps/meteor/app/api/server/lib/MultipartUploadHandler.ts @@ -0,0 +1,205 @@ +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 filePendingCount = 0; + + const { promise, resolve, reject } = Promise.withResolvers<{ + file: ParsedUpload | null; + fields: Record; + }>(); + + const tryResolve = () => { + if (busboyFinished && filePendingCount < 1) { + 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; + + ++filePendingCount; + + 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, + }; + + --filePendingCount; + + 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/chat.ts b/apps/meteor/app/api/server/v1/chat.ts index 9884afc7e7367..f289960f4f411 100644 --- a/apps/meteor/app/api/server/v1/chat.ts +++ b/apps/meteor/app/api/server/v1/chat.ts @@ -1,3 +1,4 @@ +import { Message } from '@rocket.chat/core-services'; import type { IMessage, IThreadMainMessage } from '@rocket.chat/core-typings'; import { MessageTypes } from '@rocket.chat/message-types'; import { Messages, Users, Rooms, Subscriptions } from '@rocket.chat/models'; @@ -48,7 +49,6 @@ import { executeUpdateMessage } from '../../../lib/server/methods/updateMessage' import { applyAirGappedRestrictionsValidation } from '../../../license/server/airGappedRestrictionsWrapper'; import { pinMessage, unpinMessage } from '../../../message-pin/server/pinMessage'; import { starMessage } from '../../../message-star/server/starMessage'; -import { OEmbed } from '../../../oembed/server/server'; import { executeSetReaction } from '../../../reactions/server/setReaction'; import { settings } from '../../../settings/server'; import { followMessage } from '../../../threads/server/methods/followMessage'; @@ -914,7 +914,7 @@ API.v1.addRoute( throw new Meteor.Error('error-not-allowed', 'Not allowed'); } - const { urlPreview } = await OEmbed.parseUrl(url); + const { urlPreview } = await Message.parseOEmbedUrl(url); urlPreview.ignoreParse = true; return API.v1.success({ urlPreview }); 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.ts b/apps/meteor/app/apps/server/bridges/listeners.ts index 4e1c899072851..6c3b97e81f0a4 100644 --- a/apps/meteor/app/apps/server/bridges/listeners.ts +++ b/apps/meteor/app/apps/server/bridges/listeners.ts @@ -171,7 +171,7 @@ type HandleDefaultEvent = type HandleFileUploadEvent = { event: AppInterface.IPreFileUpload; - payload: [{ file: IUpload; content: Buffer }]; + payload: [{ file: IUpload; content: Buffer | string }]; }; type HandleEvent = @@ -253,11 +253,22 @@ export class AppListenerBridge { const [{ file, content }] = args.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 uploadDetails = { 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..d7c91da115048 100644 --- a/apps/meteor/app/file-upload/server/lib/FileUpload.spec.ts +++ b/apps/meteor/app/file-upload/server/lib/FileUpload.spec.ts @@ -21,7 +21,7 @@ const { FileUpload, FileUploadClass } = proxyquire.noCallThru().load('./FileUplo 'meteor/ostrio:cookies': { Cookies: sinon.stub() }, 'sharp': sinon.stub(), 'stream-buffers': sinon.stub(), - './streamToBuffer': sinon.stub(), + '@rocket.chat/tools': sinon.stub(), '../../../../server/lib/i18n': sinon.stub(), '../../../../server/lib/logger/system': sinon.stub(), '../../../../server/lib/rooms/roomCoordinator': sinon.stub(), @@ -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..f7a8890e5bf58 100644 --- a/apps/meteor/app/file-upload/server/lib/FileUpload.ts +++ b/apps/meteor/app/file-upload/server/lib/FileUpload.ts @@ -5,13 +5,16 @@ 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'; import { AppsEngineException } from '@rocket.chat/apps-engine/definition/exceptions'; import { isE2EEUpload, type IUpload } from '@rocket.chat/core-typings'; import { Users, Avatars, UserDataFiles, Uploads, Settings, Subscriptions, Messages, Rooms } from '@rocket.chat/models'; +import { streamToBuffer } from '@rocket.chat/tools'; import type { NextFunction } from 'connect'; import filesize from 'filesize'; import { Match } from 'meteor/check'; @@ -22,13 +25,13 @@ import sharp from 'sharp'; import type { WritableStreamBuffer } from 'stream-buffers'; import streamBuffers from 'stream-buffers'; -import { streamToBuffer } from './streamToBuffer'; import { i18n } from '../../../../server/lib/i18n'; import { SystemLogger } from '../../../../server/lib/logger/system'; 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'); + 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, content.byteOffset, content.byteLength); + } + + 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/file-upload/server/lib/streamToBuffer.ts b/apps/meteor/app/file-upload/server/lib/streamToBuffer.ts deleted file mode 100644 index 110d364705f39..0000000000000 --- a/apps/meteor/app/file-upload/server/lib/streamToBuffer.ts +++ /dev/null @@ -1,13 +0,0 @@ -import type { Readable } from 'stream'; - -export const streamToBuffer = (stream: Readable): Promise => - new Promise((resolve, reject) => { - const chunks: Array = []; - - stream - .on('data', (data) => chunks.push(data)) - .on('end', () => resolve(Buffer.concat(chunks))) - .on('error', (error) => reject(error)) - // force stream to resume data flow in case it was explicitly paused before - .resume(); - }); diff --git a/apps/meteor/app/lib/server/functions/sendMessage.ts b/apps/meteor/app/lib/server/functions/sendMessage.ts index 184c2d9ce62ee..c41b52d959ad4 100644 --- a/apps/meteor/app/lib/server/functions/sendMessage.ts +++ b/apps/meteor/app/lib/server/functions/sendMessage.ts @@ -11,7 +11,7 @@ import { hasPermissionAsync } from '../../../authorization/server/functions/hasP import { FileUpload } from '../../../file-upload/server'; import { settings } from '../../../settings/server'; import { afterSaveMessage } from '../lib/afterSaveMessage'; -import { notifyOnRoomChangedById, notifyOnMessageChange } from '../lib/notifyListener'; +import { notifyOnRoomChangedById } from '../lib/notifyListener'; import { validateCustomMessageFields } from '../lib/validateCustomMessageFields'; // TODO: most of the types here are wrong, but I don't want to change them now @@ -285,11 +285,8 @@ export const sendMessage = async function (user: any, message: any, room: any, u void Apps.self?.triggerEvent(messageEvent, message); } - // TODO: is there an opportunity to send returned data to notifyOnMessageChange? await afterSaveMessage(message, room, user); - void notifyOnMessageChange({ id: message._id }); - void notifyOnRoomChangedById(message.rid); return message; diff --git a/apps/meteor/app/lib/server/functions/updateMessage.ts b/apps/meteor/app/lib/server/functions/updateMessage.ts index 73894d136ce03..baf2628e73394 100644 --- a/apps/meteor/app/lib/server/functions/updateMessage.ts +++ b/apps/meteor/app/lib/server/functions/updateMessage.ts @@ -7,7 +7,7 @@ import { Meteor } from 'meteor/meteor'; import { parseUrlsInMessage } from './parseUrlsInMessage'; import { settings } from '../../../settings/server'; import { afterSaveMessage } from '../lib/afterSaveMessage'; -import { notifyOnRoomChangedById, notifyOnMessageChange } from '../lib/notifyListener'; +import { notifyOnRoomChangedById } from '../lib/notifyListener'; import { validateCustomMessageFields } from '../lib/validateCustomMessageFields'; export const updateMessage = async function ( @@ -97,14 +97,7 @@ export const updateMessage = async function ( return; } - // although this is an "afterSave" kind callback, we know they can extend message's properties - // so we wait for it to run before broadcasting - const data = await afterSaveMessage(msg, room, user); - - void notifyOnMessageChange({ - id: msg._id, - data, - }); + await afterSaveMessage(msg, room, user); if (room?.lastMessage?._id === msg._id) { void notifyOnRoomChangedById(message.rid); diff --git a/apps/meteor/app/lib/server/lib/afterSaveMessage.ts b/apps/meteor/app/lib/server/lib/afterSaveMessage.ts index b320c2d87e8d6..3ab96e1ab7478 100644 --- a/apps/meteor/app/lib/server/lib/afterSaveMessage.ts +++ b/apps/meteor/app/lib/server/lib/afterSaveMessage.ts @@ -1,3 +1,4 @@ +import { Message } from '@rocket.chat/core-services'; import type { IMessage, IUser, IRoom } from '@rocket.chat/core-typings'; import type { Updater } from '@rocket.chat/models'; import { Rooms } from '@rocket.chat/models'; @@ -6,14 +7,16 @@ import { callbacks } from '../../../../server/lib/callbacks'; export async function afterSaveMessage(message: IMessage, room: IRoom, user: IUser, roomUpdater?: Updater): Promise { const updater = roomUpdater ?? Rooms.getUpdater(); - const data = await callbacks.run('afterSaveMessage', message, { room, user, roomUpdater: updater }); + const data: IMessage = (await callbacks.run('afterSaveMessage', message, { room, user, roomUpdater: updater })) as unknown as IMessage; if (!roomUpdater && updater.hasChanges()) { await Rooms.updateFromUpdater({ _id: room._id }, updater); } + void Message.afterSave({ message: data }); + // TODO: Fix type - callback configuration needs to be updated - return data as unknown as IMessage; + return data; } export function afterSaveMessageAsync(message: IMessage, room: IRoom, user: IUser, roomUpdater: Updater = Rooms.getUpdater()): void { @@ -22,4 +25,6 @@ export function afterSaveMessageAsync(message: IMessage, room: IRoom, user: IUse if (roomUpdater.hasChanges()) { void Rooms.updateFromUpdater({ _id: room._id }, roomUpdater); } + + void Message.afterSave({ message }); } 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/app/oembed/server/index.ts b/apps/meteor/app/oembed/server/index.ts deleted file mode 100644 index 245f608c6b94e..0000000000000 --- a/apps/meteor/app/oembed/server/index.ts +++ /dev/null @@ -1,2 +0,0 @@ -import './providers'; -import './server'; diff --git a/apps/meteor/ee/server/apps/storage/AppGridFSSourceStorage.ts b/apps/meteor/ee/server/apps/storage/AppGridFSSourceStorage.ts index 6f887971f0d89..2fd4232ee6d35 100644 --- a/apps/meteor/ee/server/apps/storage/AppGridFSSourceStorage.ts +++ b/apps/meteor/ee/server/apps/storage/AppGridFSSourceStorage.ts @@ -1,11 +1,10 @@ import type { IAppStorageItem } from '@rocket.chat/apps-engine/server/storage'; import { AppSourceStorage } from '@rocket.chat/apps-engine/server/storage'; +import { streamToBuffer } from '@rocket.chat/tools'; import { MongoInternals } from 'meteor/mongo'; import { NpmModuleMongodb } from 'meteor/npm-mongo'; import { ObjectId } from 'mongodb'; -import { streamToBuffer } from '../../../../app/file-upload/server/lib/streamToBuffer'; - export class AppGridFSSourceStorage extends AppSourceStorage { private pathPrefix = 'GridFS:/'; diff --git a/apps/meteor/server/importPackages.ts b/apps/meteor/server/importPackages.ts index ad07a20c2a8a5..61cb32f15958d 100644 --- a/apps/meteor/server/importPackages.ts +++ b/apps/meteor/server/importPackages.ts @@ -42,7 +42,6 @@ import '../app/message-pin/server'; import '../app/message-star/server'; import '../app/nextcloud/server'; import '../app/oauth2-server-config/server'; -import '../app/oembed/server'; import '../app/push-notifications/server'; import '../app/retention-policy/server'; import '../app/slackbridge/server'; diff --git a/apps/meteor/server/lib/callbacks.ts b/apps/meteor/server/lib/callbacks.ts index 791df468f5da0..d9fa18e0491b7 100644 --- a/apps/meteor/server/lib/callbacks.ts +++ b/apps/meteor/server/lib/callbacks.ts @@ -9,8 +9,6 @@ import type { ILivechatInquiryRecord, ILivechatVisitor, VideoConference, - OEmbedMeta, - OEmbedUrlContent, IOmnichannelRoom, ILivechatTag, ILivechatTagRecord, @@ -164,16 +162,6 @@ type ChainedCallbackSignatures = { BusinessHourBehaviorClass: { new (): IBusinessHourBehavior }; }; 'renderMessage': (message: T) => T; - 'oembed:beforeGetUrlContent': (data: { urlObj: URL }) => { - urlObj: URL; - headerOverrides?: { [k: string]: string }; - }; - 'oembed:afterParseContent': (data: { url: string; meta: OEmbedMeta; headers: { [k: string]: string }; content: OEmbedUrlContent }) => { - url: string; - meta: OEmbedMeta; - headers: { [k: string]: string }; - content: OEmbedUrlContent; - }; 'livechat.beforeListTags': () => ILivechatTag[]; 'livechat.offlineMessage': (data: { name: string; email: string; message: string; department?: string; host?: string }) => void; 'livechat.leadCapture': (room: IOmnichannelRoom) => IOmnichannelRoom; diff --git a/apps/meteor/server/services/image/service.ts b/apps/meteor/server/services/image/service.ts index 16992853139b1..85b4d2d05f979 100644 --- a/apps/meteor/server/services/image/service.ts +++ b/apps/meteor/server/services/image/service.ts @@ -3,6 +3,7 @@ import stream from 'stream'; import { ServiceClassInternal } from '@rocket.chat/core-services'; import type { IMediaService, ResizeResult } from '@rocket.chat/core-services'; +import { streamToBuffer } from '@rocket.chat/tools'; import ExifTransformer from 'exif-be-gone'; import ft from 'file-type'; import isSvg from 'is-svg'; @@ -91,7 +92,7 @@ export class MediaService extends ServiceClassInternal implements IMediaService } stripExifFromBuffer(buffer: Buffer): Promise { - return this.streamToBuffer(this.stripExifFromImageStream(this.bufferToStream(buffer))); + return streamToBuffer(this.stripExifFromImageStream(this.bufferToStream(buffer))); } stripExifFromImageStream(stream: stream.Stream): Readable { @@ -103,11 +104,4 @@ export class MediaService extends ServiceClassInternal implements IMediaService bufferStream.end(buffer); return bufferStream; } - - private streamToBuffer(stream: stream.Stream): Promise { - return new Promise((resolve) => { - const chunks: Array = []; - stream.on('data', (data) => chunks.push(data)).on('end', () => resolve(Buffer.concat(chunks))); - }); - } } diff --git a/apps/meteor/app/oembed/server/server.ts b/apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts similarity index 82% rename from apps/meteor/app/oembed/server/server.ts rename to apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts index 1018e7426d1d9..3623b3ddd90ae 100644 --- a/apps/meteor/app/oembed/server/server.ts +++ b/apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts @@ -1,25 +1,25 @@ import type { OEmbedUrlContentResult, + MessageUrl, OEmbedUrlWithMetadata, - IMessage, - MessageAttachment, OEmbedMeta, - MessageUrl, + IMessage, + OEmbedUrlContent, } from '@rocket.chat/core-typings'; import { isOEmbedUrlWithMetadata } from '@rocket.chat/core-typings'; import { Logger } from '@rocket.chat/logger'; -import { Messages, OEmbedCache } from '@rocket.chat/models'; +import { OEmbedCache, Messages } from '@rocket.chat/models'; import { serverFetch as fetch } from '@rocket.chat/server-fetch'; -import { camelCase } from 'change-case'; import he from 'he'; import iconv from 'iconv-lite'; import ipRangeCheck from 'ip-range-check'; import jschardet from 'jschardet'; +import { camelCase } from 'lodash'; -import { isURL } from '../../../lib/utils/isURL'; -import { callbacks } from '../../../server/lib/callbacks'; -import { settings } from '../../settings/server'; -import { Info } from '../../utils/rocketchat.info'; +import { settings } from '../../../../app/settings/server'; +import { Info } from '../../../../app/utils/rocketchat.info'; +import { isURL } from '../../../../lib/utils/isURL'; +import { afterParseUrlContent, beforeGetUrlContent } from '../lib/oembed/providers'; const MAX_EXTERNAL_URL_PREVIEWS = 5; const log = new Logger('OEmbed'); @@ -65,7 +65,7 @@ const toUtf8 = function (contentType: string, body: Buffer): string { return iconv.decode(body, getCharset(contentType, body)); }; -const getUrlContent = async (urlObj: URL, redirectCount = 5): Promise => { +const getUrlContent = async (urlObj: URL, redirectCount = 5): Promise => { const portsProtocol = new Map( Object.entries({ 80: 'http:', @@ -74,9 +74,48 @@ const getUrlContent = async (urlObj: URL, redirectCount = 5): Promise('API_EmbedIgnoredHosts').replace(/\s/g, '').split(',') || []; - if (urlObj.hostname && (ignoredHosts.includes(urlObj.hostname) || ipRangeCheck(urlObj.hostname, ignoredHosts))) { - throw new Error('invalid host'); + const ignoredHosts = + settings + .get('API_EmbedIgnoredHosts') + .replace(/\s/g, '') + .split(',') + .filter(Boolean) + .map((host) => host.toLowerCase()) || []; + + const isIgnoredHost = (hostname: string | undefined): boolean => { + hostname = hostname?.toLowerCase(); + if (!hostname || !ignoredHosts.length) { + return false; + } + + const exactHosts = ignoredHosts.filter((h) => !h.includes('*')); + if (exactHosts.includes(hostname) || ipRangeCheck(hostname, exactHosts)) { + return true; + } + + return ignoredHosts + .filter((h) => h.includes('*')) + .some((pattern) => { + const validationRegex = /^(?:\*\.)?(?:\*|[a-z0-9-]+)(?:\.(?:\*|[a-z0-9-]+))*$/i; + if (!validationRegex.test(pattern) || pattern === '*') { + return false; + } + + const escaped = pattern.replace(/[-/\\^$+?.()|[\]{}]/g, '\\$&'); + const source = `^${escaped.replace(/\*/g, '[^.]*')}$`; + + try { + const regex = new RegExp(source, 'i'); + return regex.test(hostname); + } catch { + // fail safe on invalid patterns + return false; + } + }); + }; + + if (isIgnoredHost(urlObj.hostname)) { + throw new Error('host is ignored'); } const safePorts = settings.get('API_EmbedSafePorts').replace(/\s/g, '').split(',') || []; @@ -91,14 +130,13 @@ const getUrlContent = async (urlObj: URL, redirectCount = 5): Promise('API_EmbedTimeout') * 1000, size: sizeLimit, // max size of the response body, this was not working as expected so I'm also manually verifying that on the iterator }, settings.get('Allow_Invalid_SelfSigned_Certs'), ); + const end = Date.now(); let totalSize = 0; const chunks = []; @@ -126,13 +166,14 @@ const getUrlContent = async (urlObj: URL, redirectCount = 5): Promise { @@ -233,7 +274,7 @@ const getUrlMeta = async function ( if (content && content.statusCode !== 200) { return; } - return callbacks.run('oembed:afterParseContent', { + return afterParseUrlContent({ url, meta: metas, headers, @@ -289,6 +330,10 @@ const insertMaxWidthInOembedHtml = (oembedHtml?: string): string | undefined => const rocketUrlParser = async function (message: IMessage): Promise { log.debug({ msg: 'Parsing message URLs' }); + if (!settings.get('API_Embed')) { + return message; + } + if (!Array.isArray(message.urls)) { return message; } @@ -303,8 +348,6 @@ const rocketUrlParser = async function (message: IMessage): Promise { return message; } - const attachments: MessageAttachment[] = []; - let changed = false; for await (const item of message.urls) { if (item.ignoreParse === true) { @@ -318,10 +361,6 @@ const rocketUrlParser = async function (message: IMessage): Promise { changed = changed || foundMeta; } - if (attachments.length) { - await Messages.setMessageAttachments(message._id, attachments); - } - if (changed === true) { await Messages.setUrlsById(message._id, message.urls); } @@ -329,23 +368,10 @@ const rocketUrlParser = async function (message: IMessage): Promise { return message; }; -const OEmbed: { - getUrlMeta: (url: string, withFragment?: boolean) => Promise; - getUrlMetaWithCache: (url: string, withFragment?: boolean) => Promise; +export const OEmbed: { rocketUrlParser: (message: IMessage) => Promise; parseUrl: (url: string) => Promise<{ urlPreview: MessageUrl; foundMeta: boolean }>; } = { rocketUrlParser, - getUrlMetaWithCache, - getUrlMeta, parseUrl, }; - -settings.watch('API_Embed', (value) => { - if (value) { - return callbacks.add('afterSaveMessage', (message) => OEmbed.rocketUrlParser(message), callbacks.priority.LOW, 'API_Embed'); - } - return callbacks.remove('afterSaveMessage', 'API_Embed'); -}); - -export { OEmbed }; diff --git a/apps/meteor/app/oembed/server/providers.ts b/apps/meteor/server/services/messages/lib/oembed/providers.ts similarity index 67% rename from apps/meteor/app/oembed/server/providers.ts rename to apps/meteor/server/services/messages/lib/oembed/providers.ts index 5feb6b8ff078c..cb6dfc7cdb013 100644 --- a/apps/meteor/app/oembed/server/providers.ts +++ b/apps/meteor/server/services/messages/lib/oembed/providers.ts @@ -1,10 +1,9 @@ import type { OEmbedMeta, OEmbedUrlContent, OEmbedProvider } from '@rocket.chat/core-typings'; import { camelCase } from 'change-case'; -import { callbacks } from '../../../server/lib/callbacks'; -import { SystemLogger } from '../../../server/lib/logger/system'; -import { settings } from '../../settings/server'; -import { Info } from '../../utils/rocketchat.info'; +import { settings } from '../../../../../app/settings/server'; +import { Info } from '../../../../../app/utils/rocketchat.info'; +import { SystemLogger } from '../../../../lib/logger/system'; class Providers { private providers: OEmbedProvider[]; @@ -101,32 +100,32 @@ providers.registerProvider({ endPoint: 'https://www.loom.com/v1/oembed?format=json', }); -callbacks.add( - 'oembed:beforeGetUrlContent', - (data) => { - if (!data.urlObj) { - return data; - } +export const beforeGetUrlContent = (data: { + urlObj: URL; +}): { + urlObj: URL; + headerOverrides?: { [k: string]: string }; +} => { + if (!data.urlObj) { + return data; + } - const url = data.urlObj.toString(); - const provider = providers.getProviderForUrl(url); + const url = data.urlObj.toString(); + const provider = providers.getProviderForUrl(url); - if (!provider) { - return data; - } + if (!provider) { + return data; + } - const consumerUrl = Providers.getConsumerUrl(provider, url); + const consumerUrl = Providers.getConsumerUrl(provider, url); - const headerOverrides = Providers.getCustomHeaders(provider); - if (!consumerUrl) { - return { ...data, headerOverrides }; - } + const headerOverrides = Providers.getCustomHeaders(provider); + if (!consumerUrl) { + return { ...data, headerOverrides }; + } - return { ...data, headerOverrides, urlObj: new URL(consumerUrl) }; - }, - callbacks.priority.MEDIUM, - 'oembed-providers-before', -); + return { ...data, headerOverrides, urlObj: new URL(consumerUrl) }; +}; const cleanupOembed = (data: { url: string; @@ -135,7 +134,7 @@ const cleanupOembed = (data: { content: OEmbedUrlContent; }): { url: string; - meta: Omit; + meta: OEmbedMeta; headers: { [k: string]: string }; content: OEmbedUrlContent; } => { @@ -148,37 +147,42 @@ const cleanupOembed = (data: { return { ...data, - meta, + meta: meta as OEmbedMeta, }; }; -callbacks.add( - 'oembed:afterParseContent', - (data) => { - if (!data?.url || !data.content?.body) { - return cleanupOembed(data); - } +export const afterParseUrlContent = (data: { + url: string; + meta: OEmbedMeta; + headers: { [k: string]: string }; + content: OEmbedUrlContent; +}): { + url: string; + meta: OEmbedMeta; + headers: { [k: string]: string }; + content: OEmbedUrlContent; +} => { + if (!data?.url || !data.content?.body) { + return cleanupOembed(data); + } - const provider = providers.getProviderForUrl(data.url); + const provider = providers.getProviderForUrl(data.url); - if (!provider) { - return cleanupOembed(data); - } + if (!provider) { + return cleanupOembed(data); + } - data.meta.oembedUrl = data.url; - - try { - const metas = JSON.parse(data.content.body); - Object.entries(metas).forEach(([key, value]) => { - if (value && typeof value === 'string') { - data.meta[camelCase(`oembed_${key}`)] = value; - } - }); - } catch (error) { - SystemLogger.error(error); - } - return data; - }, - callbacks.priority.MEDIUM, - 'oembed-providers-after', -); + data.meta.oembedUrl = data.url; + + try { + const metas = JSON.parse(data.content.body); + Object.entries(metas).forEach(([key, value]) => { + if (value && typeof value === 'string') { + data.meta[camelCase(`oembed_${key}`)] = value; + } + }); + } catch (error) { + SystemLogger.error(error); + } + return data; +}; diff --git a/apps/meteor/server/services/messages/service.ts b/apps/meteor/server/services/messages/service.ts index df64348d00a99..30dbe92b518ca 100644 --- a/apps/meteor/server/services/messages/service.ts +++ b/apps/meteor/server/services/messages/service.ts @@ -1,9 +1,11 @@ import { AppEvents, Apps } from '@rocket.chat/apps'; import type { IMessageService } from '@rocket.chat/core-services'; import { Authorization, ServiceClassInternal } from '@rocket.chat/core-services'; -import { type IMessage, type MessageTypesValues, type IUser, type IRoom, isEditedMessage, type AtLeast } from '@rocket.chat/core-typings'; +import { isEditedMessage } from '@rocket.chat/core-typings'; +import type { MessageUrl, IMessage, MessageTypesValues, IUser, IRoom, AtLeast } from '@rocket.chat/core-typings'; import { Messages, Rooms } from '@rocket.chat/models'; +import { OEmbed } from './hooks/AfterSaveOEmbed'; import { deleteMessage } from '../../../app/lib/server/functions/deleteMessage'; import { sendMessage } from '../../../app/lib/server/functions/sendMessage'; import { updateMessage } from '../../../app/lib/server/functions/updateMessage'; @@ -253,6 +255,17 @@ export class MessageService extends ServiceClassInternal implements IMessageServ return message; } + // The actions made on this event should be asynchronous + // That means, caller should not expect to receive updated message + // after calling + async afterSave({ message }: { message: IMessage }): Promise { + await OEmbed.rocketUrlParser(message); + + // Since this will happen after the message is sent and ack on the UI + // we'll notify until after these hooks are finished + void notifyOnMessageChange({ id: message._id }); + } + private getMarkdownConfig() { const customDomains = settings.get('Message_CustomDomain_AutoLink') ? settings @@ -308,4 +321,11 @@ export class MessageService extends ServiceClassInternal implements IMessageServ throw new FederationMatrixInvalidConfigurationError('Unable to delete message'); } } + + async parseOEmbedUrl(url: string): Promise<{ + urlPreview: MessageUrl; + foundMeta: boolean; + }> { + return OEmbed.parseUrl(url); + } } diff --git a/apps/meteor/server/settings/message.ts b/apps/meteor/server/settings/message.ts index 520af87d2333a..85a829a6f06e1 100644 --- a/apps/meteor/server/settings/message.ts +++ b/apps/meteor/server/settings/message.ts @@ -166,6 +166,10 @@ export const createMessageSettings = () => await this.add('API_EmbedSafePorts', '80, 443', { type: 'string', }); + await this.add('API_EmbedTimeout', 10, { + type: 'int', + enableQuery: { _id: 'API_Embed', value: true }, + }); await this.add('Message_TimeFormat', 'LT', { type: 'string', public: true, 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..37002260a4ed5 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) { 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/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/chat.ts b/apps/meteor/tests/end-to-end/api/chat.ts index 87242f88b058e..d7012f1347055 100644 --- a/apps/meteor/tests/end-to-end/api/chat.ts +++ b/apps/meteor/tests/end-to-end/api/chat.ts @@ -5,6 +5,7 @@ import { expect } from 'chai'; import { after, before, beforeEach, describe, it } from 'mocha'; import type { Response } from 'supertest'; +import { sleep } from '../../../lib/utils/sleep'; import { getCredentials, api, request, credentials, apiUrl } from '../../data/api-data'; import { followMessage, sendSimpleMessage, deleteMessage } from '../../data/chat.helper'; import { updatePermission, updateSetting } from '../../data/permissions.helper'; @@ -1284,6 +1285,9 @@ describe('[Chat]', () => { msgId = res.body.message._id; }); + // process is async now so wait for a sec + await sleep(1000); + await request .get(api('chat.getMessage')) .set(credentials) 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/packages/core-services/src/types/IMessageService.ts b/packages/core-services/src/types/IMessageService.ts index c9490e81bb01e..4be232027707c 100644 --- a/packages/core-services/src/types/IMessageService.ts +++ b/packages/core-services/src/types/IMessageService.ts @@ -1,4 +1,4 @@ -import type { IMessage, MessageTypesValues, IUser, IRoom, AtLeast } from '@rocket.chat/core-typings'; +import type { IMessage, MessageTypesValues, IUser, IRoom, AtLeast, MessageUrl } from '@rocket.chat/core-typings'; export interface IMessageService { sendMessage({ fromId, rid, msg }: { fromId: string; rid: string; msg: string }): Promise; @@ -49,4 +49,9 @@ export interface IMessageService { reactToMessage(userId: string, reaction: string, messageId: IMessage['_id'], shouldReact?: boolean): Promise; beforeReacted(message: IMessage, room: AtLeast): Promise; beforeDelete(message: IMessage, room: IRoom): Promise; + afterSave(param: { message: IMessage }): Promise; + parseOEmbedUrl(url: string): Promise<{ + urlPreview: MessageUrl; + foundMeta: boolean; + }>; } 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..4ebec2f5e02ff 100644 --- a/packages/http-router/src/Router.ts +++ b/packages/http-router/src/Router.ts @@ -5,10 +5,10 @@ import express from 'express'; import type { Context, HonoRequest, MiddlewareHandler } from 'hono'; import { Hono } from 'hono'; import type { StatusCode } from 'hono/utils/http-status'; -import qs from 'qs'; // Using qs specifically to keep express compatibility import type { ResponseSchema, TypedOptions } from './definition'; import { honoAdapterForExpress } from './middlewares/honoAdapterForExpress'; +import { parseQueryParams } from './parseQueryParams'; const logger = new Logger('HttpRouter'); @@ -155,10 +155,14 @@ export class Router< let parsedBody = {}; const contentType = request.header('content-type'); + 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 + return parsedBody; + } + 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()); @@ -182,7 +186,7 @@ export class Router< } protected parseQueryParams(request: HonoRequest) { - return qs.parse(request.raw.url.split('?')?.[1] || ''); + return parseQueryParams(request.raw.url.split('?')?.[1] || ''); } protected method( @@ -197,7 +201,14 @@ export class Router< this.innerRouter[method.toLowerCase() as Lowercase](`/${subpath}`.replace('//', '/'), ...middlewares, async (c) => { const { req, res } = c; - const queryParams = this.parseQueryParams(req); + let queryParams: Record; + try { + queryParams = this.parseQueryParams(req); + } catch (e) { + logger.warn({ msg: 'Error parsing query params for request', path: req.path, err: e }); + + return c.json({ success: false, error: 'Invalid query parameters' }, 400); + } if (options.query) { const validatorFn = options.query; diff --git a/packages/http-router/src/parseQueryParams.test.ts b/packages/http-router/src/parseQueryParams.test.ts new file mode 100644 index 0000000000000..9b7eaf2c5124a --- /dev/null +++ b/packages/http-router/src/parseQueryParams.test.ts @@ -0,0 +1,60 @@ +import { parseQueryParams } from './parseQueryParams'; + +describe('parseQueryParams', () => { + it('should parse simple query string', () => { + const result = parseQueryParams('foo=bar'); + expect(result).toEqual({ foo: 'bar' }); + }); + + it('should parse multiple query parameters', () => { + const result = parseQueryParams('foo=bar&baz=qux'); + expect(result).toEqual({ foo: 'bar', baz: 'qux' }); + }); + + it('should parse array parameters', () => { + const result = parseQueryParams('ids[]=1&ids[]=2&ids[]=3'); + expect(result).toEqual({ ids: ['1', '2', '3'] }); + }); + + it('should parse nested objects', () => { + const result = parseQueryParams('user[name]=john&user[age]=30'); + expect(result).toEqual({ user: { name: 'john', age: '30' } }); + }); + + it('should handle empty query string', () => { + const result = parseQueryParams(''); + expect(result).toEqual({}); + }); + + it('should decode URL encoded values', () => { + const result = parseQueryParams('name=John%20Doe'); + expect(result).toEqual({ name: 'John Doe' }); + }); + + it('should handle boolean-like values as strings', () => { + const result = parseQueryParams('active=true&disabled=false'); + expect(result).toEqual({ active: 'true', disabled: 'false' }); + }); + + it('should throw error when array limit is exceeded', () => { + const largeArray = Array(501) + .fill(0) + .map((_, i) => `ids[]=${i}`) + .join('&'); + expect(() => parseQueryParams(largeArray)).toThrow(); + }); + + it('should parse arrays within the limit', () => { + const array = Array(500) + .fill(0) + .map((_, i) => `ids[]=${i}`) + .join('&'); + const result = parseQueryParams(array); + expect(result.ids).toHaveLength(500); + }); + + it('should parse as array even without brackets', () => { + const result = parseQueryParams('ids=1&ids=2'); + expect(result.ids).toHaveLength(2); + }); +}); diff --git a/packages/http-router/src/parseQueryParams.ts b/packages/http-router/src/parseQueryParams.ts new file mode 100644 index 0000000000000..2bef5c58dede4 --- /dev/null +++ b/packages/http-router/src/parseQueryParams.ts @@ -0,0 +1,5 @@ +import qs from 'qs'; // Using qs specifically to keep express compatibility + +export function parseQueryParams(url: string) { + return qs.parse(url, { arrayLimit: 500, throwOnLimitExceeded: true }); +} diff --git a/packages/i18n/src/locales/en.i18n.json b/packages/i18n/src/locales/en.i18n.json index 9ae2d019be0ea..02ae126a8d8cc 100644 --- a/packages/i18n/src/locales/en.i18n.json +++ b/packages/i18n/src/locales/en.i18n.json @@ -107,6 +107,7 @@ "API_EmbedSafePorts_Description": "Comma-separated list of ports allowed for previewing.", "API_Embed_Description": "Whether embedded link previews are enabled or not when a user posts a link to a website.", "API_Embed_UserAgent": "Embed Request User Agent", + "API_EmbedTimeout": "Embed Request default timeout (in seconds)", "API_Enable_CORS": "Enable CORS", "API_Enable_Direct_Message_History_EndPoint": "Enable Direct Message History Endpoint", "API_Enable_Direct_Message_History_EndPoint_Description": "This enables the `/api/v1/im.messages.others` which allows the viewing of direct messages sent by other users that the caller is not part of.",