-
Notifications
You must be signed in to change notification settings - Fork 13.1k
fix: prevent file content buffering in user uploads #38071
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
38 commits
Select commit
Hold shift + click to select a range
76f5afd
refactor: remove router-level body parsing on form-data requests
ricardogarim 06628de
refactor: remove unnecessary buffering for file size check
ricardogarim 9c1bdd2
refactor: avoid re-buffering before file validation
ricardogarim 35ccddf
refactor: remove unneeded bufferToStream from initialData
ricardogarim 60555df
refactor: add UploadService
ricardogarim 027c99e
refactor: adjust POST /rooms.media/:rid to use UploadService
ricardogarim 0c2facf
refactor: adjust rest of upload routes to use UploadService
ricardogarim fbc8e35
refactor: use builtin finished api to await the pipe
d-gubert eb257ec
refactor: streamline removeTempFile in ufsComplete
d-gubert 0de765c
refactor: stripExifFromFile stop calling fs.stat
d-gubert 34ac403
refactor: always warn if cleanup fails
d-gubert dd7afaf
test: adjust router test to comply with new form-data behavior
ricardogarim 69f9ea6
refactor: remove unnecessary stat call in ufs-store
d-gubert d22a57e
refactor: use pipeline promises
d-gubert bf2ac8d
refactor: get file size from fs.WriteStream
d-gubert fabf791
fix: reject on file truncated as previous implementation
d-gubert a29a3ae
refactor: reuse static cleanup function
d-gubert c6370fe
refactor: remove `rawRequest` from route context in favor of existing…
d-gubert f1dff4f
refactor: array view validation before writeFile
d-gubert b3669a8
fix: move exif stripping to after image rotation
d-gubert b386694
chore: remove redundant call to unlink temp file
d-gubert 52555d9
fix: prevent body consumption in logger middleware
d-gubert f1b8d81
fix: increase maxSize by 1 byte so we don't block files exactly as bi…
d-gubert 7584815
refactor: simplify request type checks in UploadService
d-gubert 1aee7a0
refactor: expose internal http request to route handler
d-gubert 4de8fcd
refactor: revert endpoints that require buffer
d-gubert 64e7839
refactor: settings usage refactor
d-gubert f68aab1
refactor: remove unnecessary if on finishHandler
d-gubert 78649c6
refactor: rename UploadService to MultipartUploadHandler
d-gubert 6ba1d78
fix: properly handle errors during transform pipeline
d-gubert 975e795
feat: adapt app upload event trigger to new upload changes
d-gubert 72b8ae8
refactor: changed way of identifying finished files
d-gubert 27f875a
fix: types on ufs-filter
d-gubert 2de5cea
refactor: more correct buffer creation from view
d-gubert 89c8fc2
refactor: improve multipart skipping in parseBodyParams
d-gubert 1d5dc95
add changeset
d-gubert 92bada3
refactor: suggestion from review
d-gubert 0b8781e
Merge branch 'develop' into refactor/s3-large-uploads
kodiakhq[bot] File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| '@rocket.chat/http-router': patch | ||
| '@rocket.chat/meteor': patch | ||
| --- | ||
|
|
||
| Improves file upload flow to prevent buffering of contents in memory |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
205 changes: 205 additions & 0 deletions
205
apps/meteor/app/api/server/lib/MultipartUploadHandler.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 { | ||
d-gubert marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| static transforms = { | ||
| stripExif(): Transform { | ||
| return new ExifTransformer(); | ||
| }, | ||
| }; | ||
|
|
||
| static async cleanup(tempFilePath: string): Promise<void> { | ||
| 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<number> { | ||
| 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<string, string> }> { | ||
| 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<string, string>) : Object.fromEntries(request.headers.entries()); | ||
|
|
||
| const bb = busboy({ | ||
| headers, | ||
| defParamCharset: 'utf8', | ||
| limits, | ||
| }); | ||
|
|
||
| const fields: Record<string, string> = {}; | ||
| let parsedFile: ParsedUpload | null = null; | ||
| let busboyFinished = false; | ||
| let filePendingCount = 0; | ||
|
|
||
| const { promise, resolve, reject } = Promise.withResolvers<{ | ||
| file: ParsedUpload | null; | ||
| fields: Record<string, string>; | ||
| }>(); | ||
|
|
||
| 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); | ||
| } | ||
| } | ||
d-gubert marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| currentStream.pipe(writeStream); | ||
d-gubert marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| 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')); | ||
| }); | ||
d-gubert marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| 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; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.