Fix: CORS-related issues #5310
Fix: CORS-related issues #5310
Conversation
|
here's PR opened by vasu: #5297 |
|
Overall logic looks good. Added few minor comments. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThese changes refactor CORS handling to perform chatflow-specific domain validation for prediction requests, add domain validation utilities (extracting chatflow IDs and checking allowed origins), and remove sensitive origin fields from the public chatbot config response; a comment was added to prediction route docs. Changes
Sequence DiagramsequenceDiagram
participant Client
participant CORS_Handler as CORS Handler
participant Classifier as Request Classifier
participant Validator as Domain Validator
participant ChatflowDB as Chatflow DB
Client->>CORS_Handler: HTTP request (Origin header)
CORS_Handler->>Classifier: isPredictionRequest(req.url)?
alt Prediction request
Classifier-->>CORS_Handler: true
CORS_Handler->>Validator: extractChatflowId(req.url)
Validator-->>CORS_Handler: chatflowId
CORS_Handler->>Validator: validateChatflowDomain(chatflowId, origin)
Validator->>ChatflowDB: fetch chatflow config
ChatflowDB-->>Validator: chatbotConfig (allowedOrigins)
alt Origin allowed
CORS_Handler-->>Client: Allow (CORS OK)
else Origin denied
Validator-->>CORS_Handler: unauthorized message
CORS_Handler-->>Client: Deny (CORS error)
end
else Non-prediction request
Classifier-->>CORS_Handler: false
CORS_Handler-->>Client: Allow (standard origin handling)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/server/src/utils/domainValidation.ts (1)
52-68: Make chatflowId extraction robust and path-agnosticCurrent split logic is brittle and tied to 'prediction'. Use a regex that handles both '/prediction' and '/predictions', optional prefixes, and query strings.
-function extractChatflowId(url: string): string | null { - try { - const urlParts = url.split('/') - const predictionIndex = urlParts.indexOf('prediction') - if (predictionIndex !== -1 && urlParts.length > predictionIndex + 1) { - const chatflowId = urlParts[predictionIndex + 1] - // Remove query parameters if present - return chatflowId.split('?')[0] - } - return null - } catch (error) { - logger.error('Error extracting chatflow ID from URL:', error) - return null - } -} +function extractChatflowId(url: string): string | null { + try { + const m = url.match(/\/predictions?\/([^/?#]+)/i) + return m ? m[1] : null + } catch (error) { + logger.error('Error extracting chatflow ID from URL:', error) + return null + } +}
🧹 Nitpick comments (5)
packages/server/src/services/chatflows/index.ts (1)
377-379: Good: sensitive CORS fields removed from public configHiding allowedOrigins and allowedOriginsError is correct.
- Guard against non-object configs to avoid odd spreads:
-const parsedConfig = dbResponse.chatbotConfig ? JSON.parse(dbResponse.chatbotConfig) : {} +const raw = dbResponse.chatbotConfig ? JSON.parse(dbResponse.chatbotConfig) : {} +const parsedConfig = raw && typeof raw === 'object' ? raw : {}
- Consider documenting other sensitive keys to keep private for future additions.
packages/server/src/utils/XSS.ts (2)
28-39: Clarify and normalize CORS_ORIGINS parsingIf CORS_ORIGINS is documented as FQDNs, comparing them to full Origin values (scheme+host+port) will never match. Either document that CORS_ORIGINS must be full origins (e.g., https://example.com:3000), or normalize both sides to host[:port].
Example normalization:
-function parseAllowedOrigins(allowedOrigins: string): string[] { +function parseAllowedOrigins(allowedOrigins: string): string[] { if (!allowedOrigins) { return [] } if (allowedOrigins === '*') { return ['*'] } - return allowedOrigins - .split(',') - .map((origin) => origin.trim().toLowerCase()) - .filter((origin) => origin.length > 0) + return allowedOrigins + .split(',') + .map((v) => v.trim().toLowerCase()) + .filter((v) => v.length > 0) }Then ensure the caller lowercases the incoming origin (see previous diff).
64-81: checkRequestType: minor hardening
- Lowercase origin before passing to validateChatflowDomain.
- Add a fast path for missing chatflowId to avoid unnecessary lookups.
No separate diff needed if you apply the main origin handler change above.
packages/server/src/utils/domainValidation.ts (2)
85-101: Unused parameter and CORS messaging caveat
- Rename workspaceId to _workspaceId to silence lints until used.
- Note: CORS rejections happen at the browser; custom messages won’t surface on preflight. Use this helper where you return 403 from application endpoints.
-async function getUnauthorizedOriginError(chatflowId: string, workspaceId?: string): Promise<string> { +async function getUnauthorizedOriginError(chatflowId: string, _workspaceId?: string): Promise<string> {
102-102: Export a shared route segment constant to avoid driftConsider exporting PREDICTION_ROUTE_REGEX or a segment constant from here and reuse in XSS.ts and the router.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/server/src/routes/predictions/index.ts(1 hunks)packages/server/src/services/chatflows/index.ts(1 hunks)packages/server/src/utils/XSS.ts(2 hunks)packages/server/src/utils/domainValidation.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: build (ubuntu-latest, 18.15.0)
packages/server/src/utils/domainValidation.ts
[warning] 85-85:
'workspaceId' is defined but never used. Allowed unused args must match /^_/u
[warning] 11-11:
'workspaceId' is defined but never used. Allowed unused args must match /^_/u
🔇 Additional comments (2)
packages/server/src/utils/XSS.ts (1)
23-26: Default '' is a behavior change; confirm intendedReturning '' instead of '' makes cross-origin requests require explicit CORS_ORIGINS or per-chatflow allow (after the fix above). Confirm this is desired for backward compatibility; otherwise keep '' default.
If you intend restrictive-by-default, please update docs/env samples to reflect the change.
packages/server/src/routes/predictions/index.ts (1)
7-8: ****The mount path is
/prediction(singular) perroutes/index.ts:110, not/predictions(plural). All utility functions (isPredictionRequest,extractChatflowId) and constants consistently use the singular form, matching the comments. The folder naming convention (predictions) differs from the mount path, but this causes no functional mismatch or validation bypass. The code is correct as-is.Likely an incorrect or invalid review comment.
| export function getCorsOptions(): any { | ||
| const corsOptions = { | ||
| origin: function (origin: string | undefined, callback: (err: Error | null, allow?: boolean) => void) { | ||
| const allowedOrigins = getAllowedCorsOrigins() | ||
| if (!origin || allowedOrigins == '*' || allowedOrigins.indexOf(origin) !== -1) { | ||
| callback(null, true) | ||
| } else { | ||
| callback(null, false) | ||
| return (req: any, callback: (err: Error | null, options?: any) => void) => { | ||
| const corsOptions = { | ||
| origin: async (origin: string | undefined, originCallback: (err: Error | null, allow?: boolean) => void) => { | ||
| const allowedOrigins = getAllowedCorsOrigins() | ||
| const isPredictionReq = isPredictionRequest(req.url) | ||
|
|
||
| if (!origin || allowedOrigins === '*') { | ||
| await checkRequestType(isPredictionReq, req, origin, originCallback) | ||
| } else { | ||
| const allowedOriginsList = parseAllowedOrigins(allowedOrigins) | ||
| if (origin && allowedOriginsList.includes(origin)) { | ||
| await checkRequestType(isPredictionReq, req, origin, originCallback) | ||
| } else { | ||
| originCallback(null, false) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| callback(null, corsOptions) | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: global CORS gate blocks chatflow-level validation for predictions
With allowedOrigins unset (''), any request with an Origin header is denied before checkRequestType runs. This breaks prediction requests that rely on per-chatflow allowedOrigins.
Apply this diff to evaluate chatflow rules first for prediction requests, and use OR semantics with the global allowlist:
export function getCorsOptions(): any {
- return (req: any, callback: (err: Error | null, options?: any) => void) => {
+ return (req: any, callback: (err: Error | null, options?: any) => void) => {
const corsOptions = {
- origin: async (origin: string | undefined, originCallback: (err: Error | null, allow?: boolean) => void) => {
- const allowedOrigins = getAllowedCorsOrigins()
- const isPredictionReq = isPredictionRequest(req.url)
-
- if (!origin || allowedOrigins === '*') {
- await checkRequestType(isPredictionReq, req, origin, originCallback)
- } else {
- const allowedOriginsList = parseAllowedOrigins(allowedOrigins)
- if (origin && allowedOriginsList.includes(origin)) {
- await checkRequestType(isPredictionReq, req, origin, originCallback)
- } else {
- originCallback(null, false)
- }
- }
- }
+ origin: async (origin: string | undefined, originCallback: (err: Error | null, allow?: boolean) => void) => {
+ const allowedOrigins = getAllowedCorsOrigins()
+ const isPredictionReq = isPredictionRequest(req.url)
+ const allowedList = parseAllowedOrigins(allowedOrigins)
+ const originLc = origin?.toLowerCase()
+
+ // Always allow no-Origin requests (same-origin, server-to-server)
+ if (!originLc) return originCallback(null, true)
+
+ // Global allow: '*' or exact match
+ const globallyAllowed = allowedOrigins === '*' || allowedList.includes(originLc)
+
+ if (isPredictionReq) {
+ // Per-chatflow allowlist OR globally allowed
+ const chatflowAllowed = await (async () => {
+ const chatflowId = extractChatflowId(req.url)
+ return chatflowId ? await validateChatflowDomain(chatflowId, originLc, req.user?.activeWorkspaceId) : true
+ })()
+ return originCallback(null, globallyAllowed || chatflowAllowed)
+ }
+
+ // Non-prediction: rely on global policy only
+ return originCallback(null, globallyAllowed)
+ }
}
callback(null, corsOptions)
}
}Also normalize the comparison by lowercasing origin before includes().
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/server/src/utils/XSS.ts around lines 41-62, the origin handler
currently blocks prediction requests when the global allowedOrigins is unset and
runs the global check before the chatflow-level check; change the logic so that
for prediction requests you evaluate the chatflow rule (checkRequestType) first
and accept if it allows the request, otherwise fall back to the global
allowlist; when validating against lists normalize origin to lowercase and
compare against a lowercased allowedOrigins list, and treat the global allowlist
OR the chatflow allowlist as permissive (i.e., allow if either permits) so
prediction requests aren’t rejected prematurely.
Summary by CodeRabbit
New Features
Changes