-
-
Notifications
You must be signed in to change notification settings - Fork 24k
Support CORS credentials (cookies) via env var #6029
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
Changes from all commits
0a92632
82f43bd
6b1045c
e3f1604
22a1803
6e84604
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -205,11 +205,6 @@ export class App { | |
| // Add the sanitizeMiddleware to guard against XSS | ||
| this.app.use(sanitizeMiddleware) | ||
|
|
||
| this.app.use((req, res, next) => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the approach in this PR in general (better controls with env var and the change is made at the precise area
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will this affect users that have embedded chatbot on their website? https://docs.flowiseai.com/using-flowise/embed#cors
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| res.header('Access-Control-Allow-Credentials', 'true') // Allow credentials (cookies, etc.) | ||
| if (next) next() | ||
| }) | ||
|
|
||
| const denylistURLs = process.env.DENYLIST_URLS ? process.env.DENYLIST_URLS.split(',') : [] | ||
| const whitelistURLs = WHITELIST_URLS.filter((url) => !denylistURLs.includes(url)) | ||
| const URL_CASE_INSENSITIVE_REGEX: RegExp = /\/api\/v1\//i | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -25,6 +25,10 @@ export function getAllowedCorsOrigins(): string { | |||||||||
| return process.env.CORS_ORIGINS ?? '' | ||||||||||
| } | ||||||||||
|
|
||||||||||
| export function getAllowCredentials(): boolean { | ||||||||||
| return process.env.CORS_ALLOW_CREDENTIALS === 'true' | ||||||||||
jhead marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
| } | ||||||||||
|
|
||||||||||
| function parseAllowedOrigins(allowedOrigins: string): string[] { | ||||||||||
| if (!allowedOrigins) { | ||||||||||
| return [] | ||||||||||
|
|
@@ -41,6 +45,7 @@ function parseAllowedOrigins(allowedOrigins: string): string[] { | |||||||||
| export function getCorsOptions(): any { | ||||||||||
| return (req: any, callback: (err: Error | null, options?: any) => void) => { | ||||||||||
|
Comment on lines
45
to
46
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To improve type safety and maintainability, it's best to avoid using
Suggested change
|
||||||||||
| const corsOptions = { | ||||||||||
| credentials: getAllowCredentials(), | ||||||||||
| origin: async (origin: string | undefined, originCallback: (err: Error | null, allow?: boolean) => void) => { | ||||||||||
| const allowedOrigins = getAllowedCorsOrigins() | ||||||||||
| const isPublicChatflowReq = isPublicChatflowRequest(req.url) | ||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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