feat(api): pre-release security hardening bundle - #620
Conversation
- Add SecurityHeadersMiddleware (X-Content-Type-Options, X-Frame-Options, Referrer-Policy, and HSTS over HTTPS) applied to every response. - Reject a wildcard CORS origin when credentials are enabled (sanitize_cors_origins), so a misconfigured CORS_EXTRA_ORIGINS=* cannot reflect credentialed any-origin access. - Parse RATE_LIMIT_* only when rate limiting is enabled, so a malformed value no longer crashes boot when the feature is off. Adds unit tests for each; full unit suite green.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds CORS origin sanitization, security headers middleware and wiring, and a rate-limit startup guard so disabled rate limiting skips parsing malformed config. ChangesAPI Security Hardening
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93c6e166ae
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| app.add_middleware(InstrumentationMiddleware) | ||
| # Registered last among the app stack so it wraps outermost and stamps the | ||
| # baseline security headers on every response that flows out. | ||
| app.add_middleware(SecurityHeadersMiddleware) |
There was a problem hiding this comment.
When a browser sends a CORS preflight, the CORSMiddleware added later at line 305 is the outermost middleware and short-circuits OPTIONS requests before they reach this inner SecurityHeadersMiddleware (the surrounding comment already notes CORS preflights bypass the inner stack). In that scenario the new baseline headers, including HSTS for HTTPS preflights, are omitted even though the middleware is intended to cover every response; register the security middleware after CORS or otherwise wrap CORS with it.
Useful? React with 👍 / 👎.
| app.add_middleware(InstrumentationMiddleware) | ||
| # Registered last among the app stack so it wraps outermost and stamps the | ||
| # baseline security headers on every response that flows out. | ||
| app.add_middleware(SecurityHeadersMiddleware) |
There was a problem hiding this comment.
When ENABLE_RAY_SERVE=true and Chainlit is enabled, api.main imports chainlit_api.app and starts it in a separate uvicorn process on CHAINLIT_PORT, while openrag/chainlit_api.py defines its own FastAPI app serving /chainlit, /static, and /assets. This middleware is only installed on the main API app here, so that public Ray Serve Chainlit origin still lacks the new nosniff, frame, referrer, and HSTS hardening; add the same middleware to the standalone Chainlit app as well.
Useful? React with 👍 / 👎.
| app.add_middleware(InstrumentationMiddleware) | ||
| # Registered last among the app stack so it wraps outermost and stamps the | ||
| # baseline security headers on every response that flows out. | ||
| app.add_middleware(SecurityHeadersMiddleware) |
There was a problem hiding this comment.
For exceptions handled by the generic Exception handler, Starlette routes the response through its outer ServerErrorMiddleware, which wraps all user-added middleware rather than passing back through this SecurityHeadersMiddleware. As a result, an unexpected route failure still returns the app's JSON 500 without the new baseline headers; cover this path by applying the headers in the 500 handler or with an ASGI wrapper outside Starlette's error middleware.
Useful? React with 👍 / 👎.
Addresses review feedback on the SecurityHeadersMiddleware coverage: - Register the middleware after CORS (outermost) so it also stamps CORS preflight responses. - Add the same middleware to the standalone Chainlit app so the Ray Serve Chainlit origin gets the baseline headers. - Apply the headers in the unhandled-500 handler, since Starlette generates those responses in the outer ServerErrorMiddleware, outside the user stack. Header logic is extracted into a shared apply_security_headers() used by both the middleware and the 500 handler. Adds a test for the 500 path.
|
Thanks — all three verified as valid and addressed in the latest commit:
The header logic is extracted into a shared |
hedhoud
left a comment
hedhoud
left a comment
There was a problem hiding this comment.
I found one small but real gap before I can approve.
Normal responses from the standalone Chainlit app get the new headers, but unhandled 500 responses can still bypass them. The main API has the extra 500-handler path that calls apply_security_headers(), but chainlit_api.py only adds the middleware.
Simple example: if /chainlit/ raises unexpectedly in Ray Serve mode, that standalone origin can still return a 500 without X-Content-Type-Options, X-Frame-Options, or Referrer-Policy.
Could we add the same 500-header path for the standalone Chainlit app, or a small Chainlit-specific exception handler that calls apply_security_headers()?
Register the shared error handlers on the standalone Chainlit app so its unhandled-500 responses (produced by Starlette's outer ServerErrorMiddleware, outside the user middleware stack) get the same baseline security headers via apply_security_headers. Also gives the Chainlit app consistent OpenRAGError/500 shaping.
|
Good catch — verified and fixed in 14d1b67. You're right: the standalone Chainlit app had the middleware but no 500-handler path, so its unhandled-500s (generated by Starlette's outer Fix: |
hedhoud
left a comment
hedhoud
left a comment
There was a problem hiding this comment.
Rechecked the latest update. The previous gaps around CORS preflights, standalone Chainlit, and unhandled 500 responses are covered now, and CI plus the targeted local checks are green. Looks good to me.
3 participants
What
Three small, independent pre-release hardening changes on the API surface.
Changes
Security response headers — new
SecurityHeadersMiddlewaresetsX-Content-Type-Options: nosniff,X-Frame-Options: SAMEORIGIN, andReferrer-Policy: strict-origin-when-cross-originon every response, plusStrict-Transport-Securityonly when the request is HTTPS (so local http development and http health probes are unaffected).setdefaultpreserves a stricter header a route sets for itself. A full CSP is intentionally omitted — the mounted admin UI / Chainlit rely on inline scripts/styles and would need per-UI tuning.Reject a wildcard CORS origin with credentials —
sanitize_cors_originsdrops any*from the allowlist whenallow_credentials=True, so a misconfiguredCORS_EXTRA_ORIGINS=*can't cause Starlette to reflect the requestOriginwith credentials.Rate limiter parses only when enabled —
RATE_LIMIT_*values are now parsed only when the limiter is enabled, so a malformed value no longer crashes boot when rate limiting is turned off.Not included (by design)
is_safe_urlon the model-endpoint validate probe is handled in fix(admin): redact saved model endpoint secrets #615 — adding it to the config-endpointhealthcheck.pywould break legitimate probing of internal inference endpoints.PREFERRED_URL_SCHEME=httpsis already the default ininfra/compose/.env.example; adding it to the Helm values is a small follow-up (that chart is being reworked in Replace the indexer-ui submodule with the same-origin React admin-ui #616).Tests
test_security_headers.py,test_cors_config.py, and a rate-limit test asserting a malformed config with limiting disabled doesn't crash.Note: the
main.pyCORS block is also touched by #616 (theINDEXERUI→ADMIN_UIrewire); the two compose —sanitize_cors_originsapplies to whichever origin list that PR produces — so expect a trivial merge.Summary by CodeRabbit
New Features
Bug Fixes
Tests