From 1e21e61dad441de9f61bf22c6cc33ae4e1684d58 Mon Sep 17 00:00:00 2001 From: Junghwan <70629228+shaun0927@users.noreply.github.com> Date: Tue, 21 Apr 2026 17:31:13 +0900 Subject: [PATCH] fix(server): preserve custom iframe allowlists in frame headers (#6232) * Preserve explicit iframe allowlists when hardening frame headers The clickjacking fix correctly added frame-ancestors controls, but custom allowlists still emitted X-Frame-Options: DENY. This keeps SAMEORIGIN for 'self' and DENY for 'none', while letting explicit allowlists rely on CSP alone so supported embeds continue to work. Constraint: Legacy X-Frame-Options cannot express arbitrary allowlists Rejected: Keep DENY for all non-self values | breaks documented IFRAME_ORIGINS allowlists Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep CSP as the source of truth for custom iframe allowlists; do not reintroduce contradictory XFO headers Tested: Added unit coverage for wildcard, self, none, and custom allowlists; local header reproduction for custom origin Not-tested: Full browser iframe E2E across legacy browsers * Avoid malformed iframe CSP values while removing per-request header recomputation Gemini's review pointed out two low-risk improvements that strengthen the existing iframe allowlist fix without changing its intent: normalize away empty CSV tokens before building frame-ancestors, and compute the static header set once during app startup instead of rebuilding it on every request. Constraint: IFRAME_ORIGINS is startup configuration, not a dynamic per-request input Rejected: Larger header-construction refactor | not necessary to address the concrete review feedback Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep CSP token normalization aligned with explicit allowlists so empty CSV segments never leak into frame-ancestors Tested: pnpm --dir packages/server test -- src/utils/XSS.test.ts --runInBand Not-tested: Full browser iframe embed verification against a running server --------- Co-authored-by: yau-wd --- packages/server/src/index.ts | 18 +++------- packages/server/src/utils/XSS.test.ts | 50 ++++++++++++++++++++++++--- packages/server/src/utils/XSS.ts | 29 ++++++++++++++++ 3 files changed, 79 insertions(+), 18 deletions(-) diff --git a/packages/server/src/index.ts b/packages/server/src/index.ts index 91c88fafc..897f67af4 100644 --- a/packages/server/src/index.ts +++ b/packages/server/src/index.ts @@ -33,7 +33,7 @@ import { RateLimiterManager } from './utils/rateLimit' import { SSEStreamer } from './utils/SSEStreamer' import { Telemetry } from './utils/telemetry' import { validateAPIKey } from './utils/validateKey' -import { getAllowedIframeOrigins, getCorsOptions, sanitizeMiddleware } from './utils/XSS' +import { getCorsOptions, getIframeSecurityHeaders, sanitizeMiddleware } from './utils/XSS' declare global { namespace Express { @@ -187,20 +187,10 @@ export class App { this.app.use(cookieParser()) // Allow embedding from specified domains. + const iframeSecurityHeaders = getIframeSecurityHeaders() this.app.use((req, res, next) => { - const allowedOrigins = getAllowedIframeOrigins() - 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) - // X-Frame-Options for legacy browser support - if (allowedOrigins === "'self'") { - res.setHeader('X-Frame-Options', 'SAMEORIGIN') - } else { - res.setHeader('X-Frame-Options', 'DENY') - } + for (const [headerName, headerValue] of Object.entries(iframeSecurityHeaders)) { + res.setHeader(headerName, headerValue) } next() }) diff --git a/packages/server/src/utils/XSS.test.ts b/packages/server/src/utils/XSS.test.ts index 2b004f1cd..8fd5cd197 100644 --- a/packages/server/src/utils/XSS.test.ts +++ b/packages/server/src/utils/XSS.test.ts @@ -6,7 +6,7 @@ jest.mock('./domainValidation', () => ({ validateChatflowDomain: jest.fn() })) -import { getAllowedIframeOrigins } from './XSS' +import { getAllowedIframeOrigins, getIframeSecurityHeaders } from './XSS' describe('getAllowedIframeOrigins', () => { const originalEnv = process.env.IFRAME_ORIGINS @@ -101,17 +101,17 @@ describe('getAllowedIframeOrigins', () => { it('should handle trailing comma', () => { process.env.IFRAME_ORIGINS = 'https://example.com,' - expect(getAllowedIframeOrigins()).toBe('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') + 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') + expect(getAllowedIframeOrigins()).toBe('https://domain1.com https://domain2.com') }) }) @@ -132,3 +132,45 @@ describe('getAllowedIframeOrigins', () => { }) }) }) + +describe('getIframeSecurityHeaders', () => { + const originalEnv = process.env.IFRAME_ORIGINS + + afterEach(() => { + if (originalEnv !== undefined) { + process.env.IFRAME_ORIGINS = originalEnv + } else { + delete process.env.IFRAME_ORIGINS + } + }) + + it('returns CSP only for wildcard allowlists', () => { + process.env.IFRAME_ORIGINS = '*' + expect(getIframeSecurityHeaders()).toEqual({ + 'Content-Security-Policy': 'frame-ancestors *' + }) + }) + + it("returns SAMEORIGIN only for 'self'", () => { + process.env.IFRAME_ORIGINS = "'self'" + expect(getIframeSecurityHeaders()).toEqual({ + 'Content-Security-Policy': "frame-ancestors 'self'", + 'X-Frame-Options': 'SAMEORIGIN' + }) + }) + + it("returns DENY for 'none'", () => { + process.env.IFRAME_ORIGINS = "'none'" + expect(getIframeSecurityHeaders()).toEqual({ + 'Content-Security-Policy': "frame-ancestors 'none'", + 'X-Frame-Options': 'DENY' + }) + }) + + it('omits X-Frame-Options for custom allowlists', () => { + process.env.IFRAME_ORIGINS = 'https://embed.example.com,https://admin.example.com' + expect(getIframeSecurityHeaders()).toEqual({ + 'Content-Security-Policy': 'frame-ancestors https://embed.example.com https://admin.example.com' + }) + }) +}) diff --git a/packages/server/src/utils/XSS.ts b/packages/server/src/utils/XSS.ts index c6d28ee5b..3cbacd78a 100644 --- a/packages/server/src/utils/XSS.ts +++ b/packages/server/src/utils/XSS.ts @@ -113,5 +113,34 @@ export function getAllowedIframeOrigins(): string { return origins .split(',') .map((s) => s.trim()) + .filter(Boolean) .join(' ') } + +export function getIframeSecurityHeaders(): Record { + const allowedOrigins = getAllowedIframeOrigins() + + if (allowedOrigins === '*') { + return { + 'Content-Security-Policy': 'frame-ancestors *' + } + } + + if (allowedOrigins === "'self'") { + return { + 'Content-Security-Policy': `frame-ancestors ${allowedOrigins}`, + 'X-Frame-Options': 'SAMEORIGIN' + } + } + + if (allowedOrigins === "'none'") { + return { + 'Content-Security-Policy': `frame-ancestors ${allowedOrigins}`, + 'X-Frame-Options': 'DENY' + } + } + + return { + 'Content-Security-Policy': `frame-ancestors ${allowedOrigins}` + } +}