PR 6232

[yau-wd]

#6232 to verify the changes in a real environment.

[gemini-code-assist]

Code Review

This pull request refactors the iframe security header logic by centralizing it into a new getIframeSecurityHeaders function and improves the parsing of allowed origins by filtering empty strings. Feedback focuses on a security regression where X-Frame-Options: DENY was omitted for custom origins, which could leave legacy browsers unprotected. Additionally, it is recommended to move the header calculation back inside the middleware to ensure robustness against environment changes and to update the test suite to verify the presence of fallback headers.

[gemini-code-assist]

The current implementation omits the X-Frame-Options: DENY header when custom origins are provided. This is a security regression for legacy browsers (like IE11) that do not support the Content-Security-Policy: frame-ancestors directive. To maintain the previous security posture, X-Frame-Options: DENY should be included as a fallback for any configuration other than 'self' (which uses SAMEORIGIN) or * (which allows all).

export function getIframeSecurityHeaders(): Record<string, string> {
    const allowedOrigins = getAllowedIframeOrigins()
    const headers: Record<string, string> = {
        'Content-Security-Policy': 'frame-ancestors ' + allowedOrigins
    }

    if (allowedOrigins === "'self'") {
        headers['X-Frame-Options'] = 'SAMEORIGIN'
    } else if (allowedOrigins !== '*') {
        headers['X-Frame-Options'] = 'DENY'
    }

    return headers
}

References

1. Bypassing user-based authorization is acceptable for API endpoints that are intentionally public (e.g., whitelisted for embedding). This access should not expose sensitive data or allow privilege escalation.

[gemini-code-assist]

Moving the call to getIframeSecurityHeaders() outside the middleware means the headers are calculated only once at startup. While this is a minor optimization, it makes the middleware less robust to environment changes (e.g., during testing or in environments where process.env might be updated). For consistency with the previous implementation and other middleware like CORS, it is recommended to call this function inside the middleware.

Suggested change

	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()
	})
	this.app.use((req, res, next) => {
	const iframeSecurityHeaders = getIframeSecurityHeaders()
	for (const [headerName, headerValue] of Object.entries(iframeSecurityHeaders)) {
	res.setHeader(headerName, headerValue)
	}
	next()
	})

[gemini-code-assist]

Update this test case to reflect the inclusion of the X-Frame-Options: DENY header for custom allowlists, ensuring legacy browser protection is verified.

    it('includes X-Frame-Options: DENY 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',
            'X-Frame-Options': 'DENY'
        })
    })