Add missing security headers (X-Content-Type-Options, X-Frame-Options, CSP, Referrer-Policy, Permissions-Policy)#431
Add missing security headers (X-Content-Type-Options, X-Frame-Options, CSP, Referrer-Policy, Permissions-Policy)#431
Conversation
Add X-Content-Type-Options, X-Frame-Options, Content-Security-Policy, Referrer-Policy, and Permissions-Policy headers to all responses. - X-Content-Type-Options: nosniff — prevents MIME-sniffing attacks - X-Frame-Options: DENY — prevents clickjacking - Content-Security-Policy — mitigates XSS and injection attacks - Referrer-Policy: strict-origin-when-cross-origin — prevents referrer leaks - Permissions-Policy — restricts unnecessary browser API access Expand security header tests from 2 to 8 covering each new header. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Completed Working on "Code Review"✅ Workflow completed successfully. |
|
🚅 Deployed to the QueryWeaver-pr-431 environment in queryweaver
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Summary
- 2 MAJOR findings identified (CSP regression and incomplete permissions-policy test coverage).
- Theme: security header hardening needs to avoid breaking existing cross-origin calls and tests must enforce every promised directive.
Next steps
- Expand the CSP
connect-srcdirective to include the GitHub API endpoint (or make it configurable) so existing browser requests continue to work. - Update
test_permissions_policyto assert thepayment=()directive alongside the other restrictions to keep the test aligned with the middleware behavior.
Once those adjustments are in place the PR should be ready to merge.
api/app_factory.py
Outdated
| "style-src 'self' 'unsafe-inline'; " | ||
| "img-src 'self' data:; " | ||
| "font-src 'self'; " | ||
| "connect-src 'self'; " |
There was a problem hiding this comment.
[major]: The new CSP limits connect-src to 'self', but the SPA already makes client-side fetches to https://api.github.com (e.g., showing GitHub star counts). With this directive, browsers will now block those requests and previously working UI functionality will break. Please allow the required origin (or make it configurable) in the connect-src directive so existing cross-origin calls continue to function.
| policy = response.headers.get("permissions-policy") | ||
| assert policy is not None | ||
| assert "camera=()" in policy | ||
| assert "microphone=()" in policy |
There was a problem hiding this comment.
[major]: Permissions-Policy directives not fully asserted. The new permissions-policy test only checks camera, microphone, and geolocation, so if the middleware stops sending the required payment=() directive the regression would go unnoticed; please assert that the payment control is present to keep the test aligned with the middleware contract.
Suggested fix: Add assert "payment=()" in policy alongside the other directive checks in test_permissions_policy.
There was a problem hiding this comment.
Pull request overview
This PR extends the existing SecurityMiddleware to add several common security headers (beyond HSTS) and expands the test suite to assert their presence on responses.
Changes:
- Add
X-Content-Type-Options,X-Frame-Options,Content-Security-Policy,Referrer-Policy, andPermissions-Policyheaders inSecurityMiddleware. - Refactor HSTS header assignment slightly (inline string instead of intermediate variable).
- Expand header-related tests to cover all added headers and verify presence on an API endpoint.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
api/app_factory.py |
Adds additional security headers (including CSP) in SecurityMiddleware. |
tests/test_hsts_header.py |
Renames/expands tests to validate the new headers on / and /graphs. |
api/app_factory.py
Outdated
| # HSTS: prevent man-in-the-middle attacks | ||
| response.headers["Strict-Transport-Security"] = ( | ||
| "max-age=31536000; includeSubDomains; preload" | ||
| ) | ||
|
|
||
| # Prevent MIME-sniffing attacks | ||
| response.headers["X-Content-Type-Options"] = "nosniff" | ||
|
|
||
| # Prevent clickjacking |
There was a problem hiding this comment.
SecurityMiddleware returns early for forbidden static paths (403 JSONResponse) before the header-setting block, so those responses won’t get HSTS/CSP/etc. Since this PR is expanding security headers, consider refactoring header application into a small helper and calling it both on early-return responses and on the normal call_next response, so the security headers are consistently present on all responses (including 403/401/etc.).
api/app_factory.py
Outdated
| response.headers["Content-Security-Policy"] = ( | ||
| "default-src 'self'; " | ||
| "script-src 'self'; " | ||
| "style-src 'self' 'unsafe-inline'; " | ||
| "img-src 'self' data:; " | ||
| "font-src 'self'; " | ||
| "connect-src 'self'; " | ||
| "frame-ancestors 'none'; " | ||
| "object-src 'none'; " | ||
| "base-uri 'self'" | ||
| ) | ||
|
|
There was a problem hiding this comment.
The CSP is currently too restrictive for existing frontend behavior: the React app fetches GitHub stars from https://api.github.com (blocked by connect-src 'self'), and app/src/index.css imports Google Fonts from https://fonts.googleapis.com / https://fonts.gstatic.com (blocked by style-src 'self' and font-src 'self'). Also, FastAPI’s default /docs and /redoc use CDN assets and inline scripts/styles, which will be blocked by script-src 'self'.
Consider either (a) expanding the CSP directives to include the required origins (e.g., allow api.github.com for connect-src and Google Fonts for style/font), and/or (b) applying a stricter CSP only to the SPA routes while using a different policy (or disabling docs) for /docs//redoc.
| response.headers["Content-Security-Policy"] = ( | |
| "default-src 'self'; " | |
| "script-src 'self'; " | |
| "style-src 'self' 'unsafe-inline'; " | |
| "img-src 'self' data:; " | |
| "font-src 'self'; " | |
| "connect-src 'self'; " | |
| "frame-ancestors 'none'; " | |
| "object-src 'none'; " | |
| "base-uri 'self'" | |
| ) | |
| path = request.url.path | |
| # Apply a more permissive CSP for FastAPI's interactive API docs and schema, | |
| # which rely on CDN assets and inline scripts/styles. | |
| if path.startswith("/docs") or path.startswith("/redoc") or path.startswith( | |
| "/openapi" | |
| ): | |
| response.headers["Content-Security-Policy"] = ( | |
| "default-src 'self'; " | |
| "script-src 'self' 'unsafe-inline' 'unsafe-eval' https://cdn.jsdelivr.net https://unpkg.com; " | |
| "style-src 'self' 'unsafe-inline' https://cdn.jsdelivr.net https://fonts.googleapis.com; " | |
| "img-src 'self' data: https://cdn.jsdelivr.net; " | |
| "font-src 'self' https://fonts.gstatic.com data:; " | |
| "connect-src 'self' https://api.github.com; " | |
| "frame-ancestors 'none'; " | |
| "object-src 'none'; " | |
| "base-uri 'self'" | |
| ) | |
| else: | |
| # Default CSP for the SPA and other routes: | |
| # - Allow GitHub API for fetching repository stars. | |
| # - Allow Google Fonts styles and font files. | |
| response.headers["Content-Security-Policy"] = ( | |
| "default-src 'self'; " | |
| "script-src 'self'; " | |
| "style-src 'self' 'unsafe-inline' https://fonts.googleapis.com; " | |
| "img-src 'self' data:; " | |
| "font-src 'self' https://fonts.gstatic.com; " | |
| "connect-src 'self' https://api.github.com; " | |
| "frame-ancestors 'none'; " | |
| "object-src 'none'; " | |
| "base-uri 'self'" | |
| ) |
- Fix connect-src to allow https://api.github.com (GitHub star counts) - Fix style-src/font-src to allow Google Fonts (googleapis/gstatic) - Add separate permissive CSP for /docs, /redoc, /openapi routes - Refactor header application into helper method so early-return 403 responses also get all security headers - Add missing payment=() assertion in permissions-policy test - Add tests for GitHub API, Google Fonts, docs CSP, and 403 headers Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| """Test that CSP connect-src allows GitHub API for star counts.""" | ||
| response = client.get("/") | ||
| csp = response.headers.get("content-security-policy") | ||
| assert "https://api.github.com" in csp |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
General fix: Instead of using a raw substring search over the entire CSP header value, parse the CSP into directives and check that https://api.github.com appears as an allowed source within the appropriate directive (e.g., connect-src). This aligns with the recommendation: “parse” and reason about structure rather than doing arbitrary substring matching.
Best fix here: Add a small helper in the test file to parse the CSP header into a dictionary of {directive: [sources...]} and then assert that "https://api.github.com" is present in the connect-src sources. This preserves the intent of the test (CSP must allow GitHub API connections) but avoids a generic substring check across the whole header, and directly encodes the requirement about the correct directive.
Concretely:
- In
tests/test_hsts_header.py, above theTestSecurityHeadersclass, add a helper functionparse_csp_header(csp_header: str) -> dict[str, list[str]]that:- Splits the header on
;to get directives. - Splits each directive on whitespace to get the directive name and its sources.
- Returns a mapping from directive name to list of sources.
- Splits the header on
- Update
test_csp_allows_github_api:- Instead of
assert "https://api.github.com" in csp, parse withparse_csp_headerand then assert:"connect-src" in directives"https://api.github.com" in directives["connect-src"].
- Instead of
- No external dependencies are needed; the helper uses only basic string operations.
| @@ -6,6 +6,26 @@ | ||
| from api.index import app | ||
|
|
||
|
|
||
| def parse_csp_header(csp_header: str) -> dict: | ||
| """ | ||
| Parse a Content-Security-Policy header into a mapping of directive -> list of sources. | ||
| This avoids relying on arbitrary substring checks when validating CSP contents. | ||
| """ | ||
| directives = {} | ||
| if not csp_header: | ||
| return directives | ||
| for directive_part in csp_header.split(";"): | ||
| directive_part = directive_part.strip() | ||
| if not directive_part: | ||
| continue | ||
| tokens = directive_part.split() | ||
| if not tokens: | ||
| continue | ||
| name, *sources = tokens | ||
| directives[name] = sources | ||
| return directives | ||
|
|
||
|
|
||
| class TestSecurityHeaders: | ||
| """Test security headers.""" | ||
|
|
||
| @@ -82,7 +102,9 @@ | ||
| """Test that CSP connect-src allows GitHub API for star counts.""" | ||
| response = client.get("/") | ||
| csp = response.headers.get("content-security-policy") | ||
| assert "https://api.github.com" in csp | ||
| directives = parse_csp_header(csp) | ||
| assert "connect-src" in directives | ||
| assert "https://api.github.com" in directives["connect-src"] | ||
|
|
||
| def test_csp_allows_google_fonts(self, client): | ||
| """Test that CSP allows Google Fonts for styles and font files.""" |
| """Test that CSP allows Google Fonts for styles and font files.""" | ||
| response = client.get("/") | ||
| csp = response.headers.get("content-security-policy") | ||
| assert "https://fonts.googleapis.com" in csp |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
Copilot Autofix
AI 3 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| response = client.get("/") | ||
| csp = response.headers.get("content-security-policy") | ||
| assert "https://fonts.googleapis.com" in csp | ||
| assert "https://fonts.gstatic.com" in csp |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
Copilot Autofix
AI 3 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| """Test that /docs gets a permissive CSP for CDN assets.""" | ||
| response = client.get("/docs") | ||
| csp = response.headers.get("content-security-policy") | ||
| assert "https://cdn.jsdelivr.net" in csp |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
Copilot Autofix
AI 3 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
Summary
Adds 5 missing security headers to the
SecurityMiddlewareinapi/app_factory.py, complementing the existing HSTS header.Headers Added
X-Content-Type-OptionsnosniffX-Frame-OptionsDENYContent-Security-Policydefault-src 'self'; script-src 'self'; ...Referrer-Policystrict-origin-when-cross-originPermissions-Policycamera=(), microphone=(), geolocation=(), payment=()CSP Details
The Content-Security-Policy is tuned for this app:
style-src 'self' 'unsafe-inline'— safe concession for CSS librariesimg-src 'self' data:— allows data URI images (icons, etc.)frame-ancestors 'none'— reinforces X-Frame-Options: DENYobject-src 'none'— blocks plugin contentbase-uri 'self'— prevents base tag hijackingTests
Expanded
tests/test_hsts_header.pyfrom 2 tests to 8, covering each header individually plus a combined API endpoint check. All tests pass.