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.
|
WalkthroughAdds chatflow-aware CORS handling and domain validation utilities, removes Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CORS as CORS Handler
participant Classifier as Request Classifier
participant Validator as Domain Validator
participant DB as Chatflow DB
Client->>CORS: HTTP request (with Origin)
CORS->>Classifier: isPredictionRequest(req.url)?
alt prediction request
Classifier-->>CORS: true
CORS->>Validator: extractChatflowId(req.url)
Validator-->>CORS: chatflowId
CORS->>Validator: validateChatflowDomain(chatflowId, origin)
Validator->>DB: fetch chatflow config
DB-->>Validator: chatbotConfig (allowedOrigins)
alt origin allowed
CORS-->>Client: Allow
else origin denied
Validator-->>CORS: unauthorized message
CORS-->>Client: Deny (CORS error)
end
else non-prediction request
Classifier-->>CORS: false
CORS-->>Client: Allow/deny per global allowed origins
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/server/src/utils/XSS.ts (1)
50-51: Document the security rationale for allowing no-origin requests.Allowing all requests without an
Originheader is correct for same-origin requests and server-to-server calls, but this behavior should be documented to clarify the security model.Add a more detailed comment:
- // Always allow no-Origin requests (same-origin, server-to-server) + // Always allow no-Origin requests: + // - Same-origin requests (browser doesn't send Origin header) + // - Server-to-server requests (no browser Origin) + // - Legacy clients that don't support CORS if (!originLc) return originCallback(null, true)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/server/src/utils/XSS.ts(2 hunks)
⏰ 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)
- GitHub Check: build (ubuntu-latest, 18.15.0)
- GitHub Check: build
🔇 Additional comments (3)
packages/server/src/utils/XSS.ts (3)
23-26: Security improvement: tighter default CORS policy.Changing the default from
'*'to''significantly tightens CORS security. This means origins are now denied by default unless explicitly configured viaCORS_ORIGINSenvironment variable. While this is a security improvement, it's a breaking change for existing deployments that relied on the permissive default.Ensure this change is documented in the release notes or migration guide, and consider whether existing deployments need to be notified.
28-39: LGTM! Clean normalization logic.The
parseAllowedOriginshelper correctly handles the wildcard case, normalizes origins to lowercase for case-insensitive comparison, and filters out empty entries from malformed configuration.
3-3: All imported domain validation utilities are correctly implemented.Verification confirms that
isPredictionRequest,extractChatflowId, andvalidateChatflowDomainare properly exported from domainValidation.ts and include:
- Comprehensive error handling with try-catch blocks
- UUID validation, URL parsing, and safe JSON parsing
- Proper edge case handling (invalid URLs, missing configs, query parameters)
- Appropriate logging and safe fallback values
The imports in XSS.ts line 3 are valid and the underlying implementations are sound.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/server/src/utils/XSS.ts (2)
28-39: Well-structured normalization helper.The function correctly handles all cases and normalizes origins for case-insensitive matching. The logic is clean and defensive.
Consider adding basic origin format validation to catch configuration errors early:
function parseAllowedOrigins(allowedOrigins: string): string[] { if (!allowedOrigins) { return [] } if (allowedOrigins === '*') { return ['*'] } return allowedOrigins .split(',') .map((origin) => origin.trim().toLowerCase()) - .filter((origin) => origin.length > 0) + .filter((origin) => { + if (origin.length === 0) return false + // Warn if origin doesn't start with http:// or https:// + if (origin !== '*' && !origin.startsWith('http://') && !origin.startsWith('https://')) { + console.warn(`Potentially invalid CORS origin format: ${origin}`) + } + return true + }) }This would help catch misconfigured origins like
example.com(missing protocol) in the environment variables.
41-78: Excellent fixes addressing all previous critical issues.The refactored implementation correctly:
- Uses OR logic to allow prediction requests via either global or chatflow-specific rules (line 69)
- Wraps async validation in try-catch with safe deny-on-error fallback (lines 61-67)
- Securely denies requests when chatflowId cannot be extracted (line 59 defaults to false)
The async origin handler pattern is correctly implemented for the cors middleware.
Consider sanitizing error logs to prevent information leakage.
Line 65 logs the full error object, which might include sensitive details like database connection strings, internal paths, or stack traces visible to administrators.
} catch (error) { // Log error and deny on failure - console.error('Domain validation error:', error) + console.error('Domain validation error:', error instanceof Error ? error.message : 'Unknown error') chatflowAllowed = false }Alternatively, use a structured logger that can redact sensitive fields in production environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/server/src/utils/XSS.ts(2 hunks)
⏰ 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)
- GitHub Check: build (ubuntu-latest, 18.15.0)
- GitHub Check: build
🔇 Additional comments (2)
packages/server/src/utils/XSS.ts (2)
3-3: LGTM! Import additions support the new domain validation logic.The new imports are correctly used in the refactored CORS handling logic.
23-26: Good security improvement: secure-by-default CORS policy.Changing the default from
'*'to''ensures CORS must be explicitly configured, preventing accidental exposure.
Summary by CodeRabbit
New Features
Changes