Fix clickjacking (#6185)

* fix(server): clickjacking

* fix(XSS.ts): handle CSV and null values in getAllowedIframeOrigins

Author: yau-wd

packages/server/src/index.ts

@@ -189,13 +189,20 @@ export class App {
         // Allow embedding from specified domains.
         this.app.use((req, res, next) => {
             const allowedOrigins = getAllowedIframeOrigins()
-            if (allowedOrigins == '*') {
-                next()
+            if (allowedOrigins === '*') {
+                // Explicitly allow all origins (only when user opts in)
+                res.setHeader('Content-Security-Policy', 'frame-ancestors *')
             } else {
                 const csp = `frame-ancestors ${allowedOrigins}`
                 res.setHeader('Content-Security-Policy', csp)
-                next()
+                // X-Frame-Options for legacy browser support
+                if (allowedOrigins === "'self'") {
+                    res.setHeader('X-Frame-Options', 'SAMEORIGIN')
+                } else {
+                    res.setHeader('X-Frame-Options', 'DENY')
+                }
             }
+            next()
         })
 
         // Switch off the default 'X-Powered-By: Express' header

packages/server/src/utils/XSS.test.ts

@@ -0,0 +1,134 @@
+// At the top of XSS.test.ts, before importing getAllowedIframeOrigins
+jest.mock('./domainValidation', () => ({
+    extractChatflowId: jest.fn(),
+    isPublicChatflowRequest: jest.fn(),
+    isTTSGenerateRequest: jest.fn(),
+    validateChatflowDomain: jest.fn()
+}))
+
+import { getAllowedIframeOrigins } from './XSS'
+
+describe('getAllowedIframeOrigins', () => {
+    const originalEnv = process.env.IFRAME_ORIGINS
+
+    afterEach(() => {
+        // Restore original environment variable after each test
+        if (originalEnv !== undefined) {
+            process.env.IFRAME_ORIGINS = originalEnv
+        } else {
+            delete process.env.IFRAME_ORIGINS
+        }
+    })
+
+    describe('default behavior', () => {
+        it("should return 'self' when IFRAME_ORIGINS is not set", () => {
+            delete process.env.IFRAME_ORIGINS
+            expect(getAllowedIframeOrigins()).toBe("'self'")
+        })
+
+        it("should return 'self' when IFRAME_ORIGINS is empty string", () => {
+            process.env.IFRAME_ORIGINS = ''
+            expect(getAllowedIframeOrigins()).toBe("'self'")
+        })
+
+        it("should return 'self' when IFRAME_ORIGINS is only whitespace", () => {
+            process.env.IFRAME_ORIGINS = '   '
+            expect(getAllowedIframeOrigins()).toBe("'self'")
+        })
+    })
+
+    describe('CSP special values', () => {
+        it("should handle 'self' value", () => {
+            process.env.IFRAME_ORIGINS = "'self'"
+            expect(getAllowedIframeOrigins()).toBe("'self'")
+        })
+
+        it("should handle 'none' value", () => {
+            process.env.IFRAME_ORIGINS = "'none'"
+            expect(getAllowedIframeOrigins()).toBe("'none'")
+        })
+
+        it('should handle wildcard * value', () => {
+            process.env.IFRAME_ORIGINS = '*'
+            expect(getAllowedIframeOrigins()).toBe('*')
+        })
+    })
+
+    describe('single domain', () => {
+        it('should handle a single FQDN', () => {
+            process.env.IFRAME_ORIGINS = 'https://example.com'
+            expect(getAllowedIframeOrigins()).toBe('https://example.com')
+        })
+
+        it('should trim whitespace from single domain', () => {
+            process.env.IFRAME_ORIGINS = '  https://example.com  '
+            expect(getAllowedIframeOrigins()).toBe('https://example.com')
+        })
+    })
+
+    describe('multiple domains', () => {
+        it('should convert comma-separated domains to space-separated', () => {
+            process.env.IFRAME_ORIGINS = 'https://domain1.com,https://domain2.com'
+            expect(getAllowedIframeOrigins()).toBe('https://domain1.com https://domain2.com')
+        })
+
+        it('should handle multiple domains with spaces', () => {
+            process.env.IFRAME_ORIGINS = 'https://domain1.com, https://domain2.com, https://domain3.com'
+            expect(getAllowedIframeOrigins()).toBe('https://domain1.com https://domain2.com https://domain3.com')
+        })
+
+        it('should trim individual domains in comma-separated list', () => {
+            process.env.IFRAME_ORIGINS = '  https://app.com  ,  https://admin.com  '
+            expect(getAllowedIframeOrigins()).toBe('https://app.com https://admin.com')
+        })
+    })
+
+    describe('edge cases', () => {
+        it('should handle domains with ports', () => {
+            process.env.IFRAME_ORIGINS = 'https://localhost:3000,https://localhost:4000'
+            expect(getAllowedIframeOrigins()).toBe('https://localhost:3000 https://localhost:4000')
+        })
+
+        it('should handle domains with paths (though not typical for CSP)', () => {
+            process.env.IFRAME_ORIGINS = 'https://example.com/path1,https://example.com/path2'
+            expect(getAllowedIframeOrigins()).toBe('https://example.com/path1 https://example.com/path2')
+        })
+
+        it('should handle mixed protocols', () => {
+            process.env.IFRAME_ORIGINS = 'http://example.com,https://secure.com'
+            expect(getAllowedIframeOrigins()).toBe('http://example.com https://secure.com')
+        })
+
+        it('should handle trailing comma', () => {
+            process.env.IFRAME_ORIGINS = 'https://example.com,'
+            expect(getAllowedIframeOrigins()).toBe('https://example.com ')
+        })
+
+        it('should handle leading comma', () => {
+            process.env.IFRAME_ORIGINS = ',https://example.com'
+            expect(getAllowedIframeOrigins()).toBe(' https://example.com')
+        })
+
+        it('should handle multiple consecutive commas', () => {
+            process.env.IFRAME_ORIGINS = 'https://domain1.com,,https://domain2.com'
+            expect(getAllowedIframeOrigins()).toBe('https://domain1.com  https://domain2.com')
+        })
+    })
+
+    describe('real-world scenarios', () => {
+        it('should handle typical production configuration', () => {
+            process.env.IFRAME_ORIGINS = 'https://app.example.com,https://admin.example.com,https://dashboard.example.com'
+            expect(getAllowedIframeOrigins()).toBe('https://app.example.com https://admin.example.com https://dashboard.example.com')
+        })
+
+        it('should handle development configuration with localhost', () => {
+            process.env.IFRAME_ORIGINS = 'http://localhost:3000,http://localhost:3001,http://127.0.0.1:3000'
+            expect(getAllowedIframeOrigins()).toBe('http://localhost:3000 http://localhost:3001 http://127.0.0.1:3000')
+        })
+
+        it("should handle mix of 'self' and domains", () => {
+            process.env.IFRAME_ORIGINS = "'self',https://trusted.com"
+            expect(getAllowedIframeOrigins()).toBe("'self' https://trusted.com")
+        })
+    })
+})

packages/server/src/utils/XSS.ts

@@ -1,6 +1,6 @@
-import { Request, Response, NextFunction } from 'express'
+import { NextFunction, Request, Response } from 'express'
 import sanitizeHtml from 'sanitize-html'
-import { extractChatflowId, validateChatflowDomain, isPublicChatflowRequest, isTTSGenerateRequest } from './domainValidation'
+import { extractChatflowId, isPublicChatflowRequest, isTTSGenerateRequest, validateChatflowDomain } from './domainValidation'
 
 export function sanitizeMiddleware(req: Request, res: Response, next: NextFunction): void {
     // decoding is necessary as the url is encoded by the browser
@@ -87,8 +87,31 @@ export function getCorsOptions(): any {
     }
 }
 
+/**
+ * Retrieves and normalizes allowed iframe embedding origins for CSP frame-ancestors directive.
+ *
+ * Reads `IFRAME_ORIGINS` environment variable (comma-separated FQDNs) and converts it to
+ * space-separated format required by Content Security Policy specification.
+ *
+ * Input format:
+ * - Comma-separated: `https://domain1.com,https://domain2.com`
+ * - Special values: `'self'`, `'none'`, or `*`
+ * - Default: `'self'` (same-origin only)
+ *
+ * Output examples:
+ * - `https://app.com,https://admin.com` → `https://app.com https://admin.com`
+ * - `'self'` → `'self'`
+ * - `*` → `*`
+ *
+ * @returns Space-separated string for CSP frame-ancestors directive
+ */
 export function getAllowedIframeOrigins(): string {
     // Expects FQDN separated by commas, otherwise nothing or * for all.
     // Also CSP allowed values: self or none
-    return process.env.IFRAME_ORIGINS ?? '*'
+    const origins = (process.env.IFRAME_ORIGINS?.trim() || undefined) ?? "'self'"
+    // Convert CSV to space-separated for CSP frame-ancestors directive
+    return origins
+        .split(',')
+        .map((s) => s.trim())
+        .join(' ')
 }