fix: validate mime type list when updating chatflow configuration#5768
fix: validate mime type list when updating chatflow configuration#5768
Conversation
Summary of ChangesHello @0xi4o, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the security of chatflow configurations by introducing robust validation and sanitization for allowed file upload MIME types. It prevents the configuration of potentially malicious or dangerous file types by filtering user-provided MIME type lists against a predefined safe list during chatflow updates. This ensures that only approved file types can be uploaded, mitigating security risks associated with arbitrary file uploads. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an important security enhancement by validating the list of allowed MIME types for file uploads in chatflow configurations. A server-side allowlist (ALLOWED_UPLOAD_MIME_TYPES) is added, and the chatbotConfig is sanitized upon updates to ensure only permitted file types are configured.
My review includes two main points:
- A suggestion to use a
Setinstead of an array for theALLOWED_UPLOAD_MIME_TYPESlist inpackages/components/src/utils.tsfor better performance and code clarity. - A high-severity issue in
packages/server/src/services/chatflows/index.tswhere an emptycatchblock could allow bypassing the new sanitization logic if a malformedchatbotConfigis provided.
The overall approach is solid, and with the suggested changes, the implementation will be more robust and secure.
| } catch { | ||
| // If parsing fails, leave chatbotConfig unchanged | ||
| } |
There was a problem hiding this comment.
The empty catch block that silently ignores JSON parsing errors is a security risk. If a malicious user provides a malformed chatbotConfig JSON string, the parsing will fail, the catch block will be executed, and the sanitization logic for allowedUploadFileTypes will be bypassed. The application will then proceed to save the unsanitized chatbotConfig from the request. This could lead to security vulnerabilities if other parts of the system trust this configuration.
Instead of silently ignoring the error, you should at least log it. For better security, consider rejecting the update with a BAD_REQUEST error if the chatbotConfig is not valid JSON.
} catch (error) {
logger.error(`[server]: Error parsing chatbotConfig in updateChatflow: ${getErrorMessage(error)}`);
// To prevent saving a potentially malicious malformed config, you might want to
// throw an error here to abort the update.
// e.g., throw new InternalFlowiseError(StatusCodes.BAD_REQUEST, `Invalid chatbotConfig: ${getErrorMessage(error)}`);
}| export const ALLOWED_UPLOAD_MIME_TYPES: readonly string[] = [ | ||
| 'text/css', | ||
| 'text/csv', | ||
| 'text/html', | ||
| 'application/json', | ||
| 'text/markdown', | ||
| 'application/x-yaml', | ||
| 'application/pdf', | ||
| 'application/sql', | ||
| 'text/plain', | ||
| 'application/xml', | ||
| 'application/msword', | ||
| 'application/vnd.openxmlformats-officedocument.wordprocessingml.document', | ||
| 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet', | ||
| 'application/vnd.openxmlformats-officedocument.presentationml.presentation' | ||
| ] | ||
|
|
||
| /** | ||
| * Returns true if the MIME type is allowed for file upload config. | ||
| * Must be in ALLOWED_UPLOAD_MIME_TYPES and have a mapping in mapMimeTypeToExt. | ||
| * @param {string} mime | ||
| * @returns {boolean} | ||
| */ | ||
| export const isAllowedUploadMimeType = (mime: string): boolean => { | ||
| if (!mime || typeof mime !== 'string') return false | ||
| const trimmed = mime.trim() | ||
| if (!trimmed) return false | ||
| return ALLOWED_UPLOAD_MIME_TYPES.includes(trimmed) && mapMimeTypeToExt(trimmed) !== '' | ||
| } |
There was a problem hiding this comment.
For improved performance and code clarity, it's better to use a Set for ALLOWED_UPLOAD_MIME_TYPES. A Set provides O(1) average time complexity for lookups (.has()), which is more efficient than an array's includes() O(n) complexity. This also makes the intent of the collection—a unique set of values for quick lookups—more explicit.
export const ALLOWED_UPLOAD_MIME_TYPES: ReadonlySet<string> = new Set([
'text/css',
'text/csv',
'text/html',
'application/json',
'text/markdown',
'application/x-yaml',
'application/pdf',
'application/sql',
'text/plain',
'application/xml',
'application/msword',
'application/vnd.openxmlformats-officedocument.wordprocessingml.document',
'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet',
'application/vnd.openxmlformats-officedocument.presentationml.presentation'
])
/**
* Returns true if the MIME type is allowed for file upload config.
* Must be in ALLOWED_UPLOAD_MIME_TYPES and have a mapping in mapMimeTypeToExt.
* @param {string} mime
* @returns {boolean}
*/
export const isAllowedUploadMimeType = (mime: string): boolean => {
if (!mime || typeof mime !== 'string') return false
const trimmed = mime.trim()
if (!trimmed) return false
return ALLOWED_UPLOAD_MIME_TYPES.has(trimmed) && mapMimeTypeToExt(trimmed) !== ''
}1 participant
No description provided.