Conversation
Summary of ChangesHello, 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 Flowise's ability to handle Cross-Origin Resource Sharing (CORS) credentials by introducing a dedicated environment variable. This change resolves an issue where an existing custom middleware failed to properly enable credential support, leading to improved compatibility with authenticating proxies. The core modification involves streamlining the CORS configuration to correctly apply credential settings. Highlights
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. 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 correctly implements support for CORS credentials by adding a CORS_ALLOW_CREDENTIALS environment variable. The change properly moves the credentials configuration into the main cors middleware options, which is the correct approach, and removes the previous, ineffective middleware. The implementation is sound. The suggestion to improve type safety in the getCorsOptions function by replacing any types with specific types from the cors package has been retained, as it enhances maintainability and aligns with general good practices.
| export function getCorsOptions(): any { | ||
| return (req: any, callback: (err: Error | null, options?: any) => void) => { |
There was a problem hiding this comment.
To improve type safety and maintainability, it's best to avoid using any. You can use specific types from the cors package. Using import('cors').CorsOptionsDelegate and import('cors').CorsOptions allows for type-safe code without needing to add a new import statement at the top of the file. The Request type is already available from the existing express import.
| export function getCorsOptions(): any { | |
| return (req: any, callback: (err: Error | null, options?: any) => void) => { | |
| export function getCorsOptions(): import('cors').CorsOptionsDelegate { | |
| return (req: Request, callback: (err: Error | null, options?: import('cors').CorsOptions) => void) => { |
| // Add the sanitizeMiddleware to guard against XSS | ||
| this.app.use(sanitizeMiddleware) | ||
|
|
||
| this.app.use((req, res, next) => { |
There was a problem hiding this comment.
I like the approach in this PR in general (better controls with env var and the change is made at the precise area corsOptions), just wondering if anything would be impacted with the removal of this block
There was a problem hiding this comment.
The removed middleware was only ever partially effective. For any request that triggers a preflight (POST with application/json, custom headers like content-type/sentry-trace) — which is the vast majority of Flowise UI traffic — it never worked at all, because cors() intercepts OPTIONS and ends the response before the manual header middleware ran. The header only reached the client for "simple" requests (GET/HEAD, or POST with form-encoded/plain-text bodies), which is a narrow slice that doesn't cover authenticated API flows from what I can tell.
In practice, any integration that needed credentialed cross-origin access to non-simple requests was already broken. Defaulting to false and requiring explicit opt-in via CORS_ALLOW_CREDENTIALS=true is a stricter, more intentional posture with hopefully negligible regression for existing users.
But I'm open to feedback from others if anyone disagrees.
There was a problem hiding this comment.
will this affect users that have embedded chatbot on their website? https://docs.flowiseai.com/using-flowise/embed#cors
There was a problem hiding this comment.
it probably will, i.e. users need to set the env var to true going forward.
can I suggest that I open a PR to update the documentation?
defaulting to false seems safer because not all Flowise users will be using the embedded chat? so it seems safer to assume we don't want credentials to be passed cross-origin.
I'm open to other POVs though.
| ############################################################################################################ | ||
|
|
||
| # NUMBER_OF_PROXIES= 1 | ||
| # CORS_ALLOW_CREDENTIALS=false |
There was a problem hiding this comment.
It'd be great if a PR could also be added to updated this documentation: https://github.com/FlowiseAI/FlowiseDocs/blob/main/en/configuration/environment-variables.md?plain=1#L181-L190
5 participants
Adds support for enabling
Access-Control-Allow-Credentialsvia a newCORS_ALLOW_CREDENTIALSenvironment variable. This is useful when running Flowise behind an authenticating proxy that requires cookies, etc.Removes an existing middleware that attempted to enable this feature but never worked because the
cors()middleware always intercepted and responded to the preflight requests first, so instead I've moved the credentials config to the main cors middleware.Env var logic defaults to false.