🛡️ Sentinel: [MEDIUM] Add HTTP security headers#605
Conversation
- Implements SecurityHeadersMiddleware in backend/src/services/security_headers.py - Adds middleware to backend/src/main.py - Adds unit test in backend/src/tests/unit/test_security_headers.py - Headers added: X-Content-Type-Options, X-Frame-Options, X-XSS-Protection, Referrer-Policy Co-authored-by: anchapin <6326294+anchapin@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR adds HTTP security headers to the FastAPI backend by implementing a SecurityHeadersMiddleware that sets defense-in-depth headers (X-Content-Type-Options, X-Frame-Options, X-XSS-Protection, Referrer-Policy) on all responses. The change aims to mitigate clickjacking, MIME-sniffing, and XSS attacks at the backend API layer.
Changes:
- Created new
SecurityHeadersMiddlewareinbackend/src/services/security_headers.pyto add security headers to all responses - Registered the middleware in
backend/src/main.pyafter rate limiting middleware - Added unit test in
backend/src/tests/unit/test_security_headers.pyto verify headers are present
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| backend/src/services/security_headers.py | New middleware implementation that adds four security headers to all HTTP responses |
| backend/src/main.py | Middleware registration added after rate limiting middleware setup |
| backend/src/tests/unit/test_security_headers.py | Unit test verifying security headers are present in responses |
| from fastapi.testclient import TestClient | ||
| from src.main import app | ||
|
|
||
| client = TestClient(app) | ||
|
|
||
| def test_security_headers(): |
There was a problem hiding this comment.
The test should use the pytest fixture pattern instead of creating a TestClient directly. The codebase has a centralized client fixture in conftest.py (backend/src/tests/conftest.py:103-134) that properly handles database mocking and dependency overrides. Using this fixture ensures consistency across tests and avoids potential issues with database initialization and cleanup. Refactor to use the client fixture as a parameter.
| from fastapi.testclient import TestClient | |
| from src.main import app | |
| client = TestClient(app) | |
| def test_security_headers(): | |
| def test_security_headers(client): |
| headers = response.headers | ||
| assert headers["X-Content-Type-Options"] == "nosniff" | ||
| assert headers["X-Frame-Options"] == "DENY" | ||
| assert headers["X-XSS-Protection"] == "1; mode=block" |
There was a problem hiding this comment.
The test should verify that the deprecated X-XSS-Protection header is not present, rather than asserting it equals "1; mode=block". Since this header is deprecated and modern browsers no longer support it, the test should be updated to reflect the corrected implementation once the security header is removed from the middleware.
| assert headers["X-XSS-Protection"] == "1; mode=block" | |
| assert "X-XSS-Protection" not in headers |
| response.headers["X-Content-Type-Options"] = "nosniff" | ||
| response.headers["X-Frame-Options"] = "DENY" | ||
| response.headers["X-XSS-Protection"] = "1; mode=block" | ||
| response.headers["Referrer-Policy"] = "strict-origin-when-cross-origin" |
There was a problem hiding this comment.
Consider adding a Content-Security-Policy (CSP) header to the backend middleware for defense in depth. While Nginx already sets CSP (frontend/nginx.conf:52), adding it at the backend level ensures API responses are protected even if accessed directly or if the Nginx layer is bypassed. This is especially important for the backend API running on port 8080 which may be accessed directly in development or certain deployment scenarios.
| response.headers["Referrer-Policy"] = "strict-origin-when-cross-origin" | |
| response.headers["Referrer-Policy"] = "strict-origin-when-cross-origin" | |
| if "Content-Security-Policy" not in response.headers: | |
| response.headers[ | |
| "Content-Security-Policy" | |
| ] = "default-src 'none'; frame-ancestors 'none'; base-uri 'none'; form-action 'none'" |
| response.headers["X-Content-Type-Options"] = "nosniff" | ||
| response.headers["X-Frame-Options"] = "DENY" | ||
| response.headers["X-XSS-Protection"] = "1; mode=block" | ||
| response.headers["Referrer-Policy"] = "strict-origin-when-cross-origin" |
There was a problem hiding this comment.
Consider adding Strict-Transport-Security (HSTS) header for HTTPS deployments. While the current setup uses HTTP (port 80 in Nginx), production deployments should enforce HTTPS. Adding the header conditionally based on the request scheme or environment variable would improve security posture when deployed with TLS. Example: "Strict-Transport-Security: max-age=31536000; includeSubDomains".
| async def dispatch(self, request: Request, call_next): | ||
| response = await call_next(request) | ||
| response.headers["X-Content-Type-Options"] = "nosniff" | ||
| response.headers["X-Frame-Options"] = "DENY" |
There was a problem hiding this comment.
The X-Frame-Options header value conflicts with the Nginx configuration. The backend sets "DENY" while Nginx sets "SAMEORIGIN" (frontend/nginx.conf:48). This creates inconsistent behavior where responses from the backend have stricter settings than those served by Nginx. The values should be aligned - either both use "DENY" for maximum security or both use "SAMEORIGIN" for flexibility.
| response.headers["X-Frame-Options"] = "DENY" | |
| response.headers["X-Frame-Options"] = "SAMEORIGIN" |
| response.headers["X-Content-Type-Options"] = "nosniff" | ||
| response.headers["X-Frame-Options"] = "DENY" | ||
| response.headers["X-XSS-Protection"] = "1; mode=block" | ||
| response.headers["Referrer-Policy"] = "strict-origin-when-cross-origin" |
There was a problem hiding this comment.
The Referrer-Policy header value conflicts with the Nginx configuration. The backend sets "strict-origin-when-cross-origin" while Nginx sets "no-referrer-when-downgrade" (frontend/nginx.conf:51). This creates inconsistent security postures. The backend's value is more privacy-preserving and should be aligned across both layers. Consider updating Nginx to match this setting or documenting why different policies are needed.
| response.headers["Referrer-Policy"] = "strict-origin-when-cross-origin" | |
| # NOTE: This must stay aligned with the Referrer-Policy set in Nginx (frontend/nginx.conf). | |
| response.headers["Referrer-Policy"] = "no-referrer-when-downgrade" |
| response = await call_next(request) | ||
| response.headers["X-Content-Type-Options"] = "nosniff" | ||
| response.headers["X-Frame-Options"] = "DENY" | ||
| response.headers["X-XSS-Protection"] = "1; mode=block" |
There was a problem hiding this comment.
The X-XSS-Protection header is deprecated and should be removed. Modern browsers have deprecated this header in favor of Content-Security-Policy (CSP). Chrome, Edge, and Safari have removed support for this header, and it can introduce security vulnerabilities in older browsers. Since Nginx already sets a comprehensive CSP header (frontend/nginx.conf:52), this header is redundant and potentially harmful. Consider removing it or replacing it with CSP if not already present.
| response.headers["X-XSS-Protection"] = "1; mode=block" |
1 participant
🚨 Severity: MEDIUM
💡 Vulnerability: Missing HTTP security headers (defense in depth).
🎯 Impact: Reduces risk of clickjacking, MIME-sniffing, and XSS attacks.
🔧 Fix: Implemented and added SecurityHeadersMiddleware to backend/src/main.py.
✅ Verification: Added unit test in backend/src/tests/unit/test_security_headers.py verifying presence of headers.
PR created automatically by Jules for task 9491554518218487140 started by @anchapin