Skip to content

Sanitize flowData in getSinglePublicChatflow#5938

Open
christopherholland-workday wants to merge 1 commit intomainfrom
chatflow-data-leak
Open

Sanitize flowData in getSinglePublicChatflow#5938
christopherholland-workday wants to merge 1 commit intomainfrom
chatflow-data-leak

Conversation

@christopherholland-workday
Copy link
Contributor

Sanitize flowData using sanitizeFlowDataForPublicEndpoint. FLOWISE-293

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello, 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 focuses on improving the security and privacy of public chatflows by ensuring that sensitive flowData is properly sanitized before being sent to clients. This change prevents the exposure of internal or potentially sensitive configuration details when public chatflows are accessed, thereby enhancing the overall robustness of the application's public-facing APIs.

Highlights

  • Security Enhancement: Implemented data sanitization for flowData when exposing public chatflows to prevent potential information leakage.
  • New Utility Function: Introduced sanitizeFlowDataForPublicEndpoint to handle the sanitization process.
  • API Modification: Modified the getSinglePublicChatflow controller to utilize the new sanitization function before returning public chatflow data.
Changelog
  • packages/server/src/controllers/chatflows/index.ts
    • Imported the new sanitizeFlowDataForPublicEndpoint utility function.
    • Applied the sanitizeFlowDataForPublicEndpoint function to the flowData property of public chatflows before returning them in the getSinglePublicChatflow endpoint.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request aims to improve security by sanitizing flowData in the getSinglePublicChatflow public endpoint, using sanitizeFlowDataForPublicEndpoint to prevent sensitive information leakage. However, the current sanitizeFlowDataForPublicEndpoint implementation has a bypass when the inputParams property is missing from a node's data, potentially exposing credentials. Additionally, the controller returns the entire chatflow object, which exposes internal metadata and unsanitized configuration. It is recommended to refine the sanitizer logic and limit the fields returned by the public endpoint. There is also a minor code style suggestion to improve readability and maintainability, aligning with repository rules.

Comment on lines +221 to +222
if (chatflow.isPublic)
return res.status(StatusCodes.OK).json({ ...chatflow, flowData: sanitizeFlowDataForPublicEndpoint(chatflow.flowData) })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

The sanitizeFlowDataForPublicEndpoint function used here has a potential bypass. If the flowData is missing the inputParams property for a node, the sanitizer skips the node entirely, leaving sensitive inputs (including passwords and authentication headers) exposed in the public response. The sanitizer implementation in sanitizeFlowData.ts should be updated to handle cases where inputParams is missing, ensuring that at least header sanitization is always performed. Additionally, for improved readability and to prevent potential future bugs (like the dangling else problem), it's a good practice to always use curly braces {} with if statements, especially when the enclosed logic is long enough to be broken into multiple lines, as shown in the suggestion.

Suggested change
if (chatflow.isPublic)
return res.status(StatusCodes.OK).json({ ...chatflow, flowData: sanitizeFlowDataForPublicEndpoint(chatflow.flowData) })
if (chatflow.isPublic) {
return res.status(StatusCodes.OK).json({ ...chatflow, flowData: sanitizeFlowDataForPublicEndpoint(chatflow.flowData) })
}
References
  1. The comment highlights a potential bypass in a public endpoint's sanitizer that could expose sensitive data, directly contradicting the rule's requirement that public access should not expose sensitive data.
  2. The suggestion to always use curly braces with if statements aligns with prioritizing readability and preventing potential bugs, which improves understandability and reduces future errors.

if (!chatflow) return res.status(StatusCodes.NOT_FOUND).json({ message: 'Chatflow not found' })
if (chatflow.isPublic) return res.status(StatusCodes.OK).json(chatflow)
if (chatflow.isPublic)
return res.status(StatusCodes.OK).json({ ...chatflow, flowData: sanitizeFlowDataForPublicEndpoint(chatflow.flowData) })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

Returning the entire chatflow object via { ...chatflow } exposes internal metadata such as apikeyid and workspaceId to the public. Additionally, the chatbotConfig field is returned without any sanitization, which could leak sensitive configuration settings (e.g., allowedOrigins). It is recommended to return only the necessary fields and ensure all returned data is properly sanitized.

Suggested change
return res.status(StatusCodes.OK).json({ ...chatflow, flowData: sanitizeFlowDataForPublicEndpoint(chatflow.flowData) })
return res.status(StatusCodes.OK).json({ id: chatflow.id, name: chatflow.name, type: chatflow.type, flowData: sanitizeFlowDataForPublicEndpoint(chatflow.flowData) })
References
  1. The comment points out that returning the entire chatflow object exposes internal metadata and unsanitized configuration to the public, which violates the rule's condition that public endpoints should not expose sensitive data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants