fix: stop chainlit's Socket.IO transport from tripping the rate limiter#654
Open
Ahmath-Gadji wants to merge 5 commits into
Open
fix: stop chainlit's Socket.IO transport from tripping the rate limiter#654Ahmath-Gadji wants to merge 5 commits into
Ahmath-Gadji wants to merge 5 commits into
Conversation
/chainlit and /assets/ are bypassed by AuthMiddleware (Chainlit runs its own header-auth callback), so request.state.user is never populated there. The rate limiter therefore could not see the admin flag and fell back to keying on request.client.host — which behind the admin-ui reverse proxy is one container IP shared by every chat user. A single bucket for the whole deployment. Chainlit's Socket.IO transport lives at /chainlit/ws/socket.io and issues one HTTP request per emitted packet whenever it long-polls, so a streamed answer alone exhausts the 600/minute default and the resulting 429s trigger engine.io reconnects, which issue more requests. Skip the limiter on those prefixes, overridable via RATE_LIMIT_EXEMPT_PATHS (set-but-empty disables the exemption). /auth/* stays limited: it is the brute-force surface and IP keying is exactly what it wants.
The `location /` block proxied over nginx's default HTTP/1.0 and dropped the Upgrade/Connection headers, so chainlit's Socket.IO could never leave its long-polling bootstrap transport. Every emitted packet then cost one HTTP request through the API's middleware stack. Add proxy_http_version 1.1 plus the standard $connection_upgrade map. The map is load-bearing: a hardcoded `Connection: upgrade` would be sent on ordinary requests too and break them. With the upgrade succeeding, chainlit's chat traffic becomes a single WebSocket whose ASGI scope is "websocket", which Starlette's BaseHTTPMiddleware skips — so it no longer traverses the rate limiter at all. Validated with `nginx -t` against nginx:alpine.
…I path The documented UVICORN_FORWARDED_ALLOW_IPS was only read by api.main's __main__ block, which entrypoint.sh executes solely when ENABLE_RAY_SERVE=true. The default container path is the bare `uvicorn api.main:app` CLI, and uvicorn's own config reads FORWARDED_ALLOW_IPS instead — so setting the documented variable did nothing there. Consequence: the admin-ui reverse proxy sits outside loopback, is never trusted, X-Forwarded-For is dropped, and request.client.host stays the proxy's container address. Every user shares one rate-limit bucket, and OIDC cookies ship Secure=False behind HTTPS. Pass --forwarded-allow-ips (and --proxy-headers explicitly) from the same variable so one documented name covers both entrypoints. Guard it with a regression test alongside the existing api.main one, and document the rate-limit consequence plus RATE_LIMIT_EXEMPT_PATHS in .env.example (mirrored to docs/assets) and env_vars.md.
|
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 (8)
📝 WalkthroughWalkthroughRate-limit path exemptions are configurable and tested, Uvicorn now trusts explicitly configured proxy addresses, and nginx forwards WebSocket upgrades. Environment examples and documentation describe the new settings and reverse-proxy requirements. ChangesRate limiting and proxy integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
The exempt prefixes were matched with `startswith(("/chainlit", "/assets/"))`,
so any path merely *beginning* with `/chainlit` — e.g. `/chainlithack` — was
silently skipped by the limiter while AuthMiddleware still required a token for
it. Tighten the prefix to `/chainlit/` so only the real subtree is exempt,
mirroring the prefix arm of `api.middleware.auth.is_bypass_path`. Bare
`/chainlit` (the 307 to `/chainlit/`) is now metered at the default tier, which
is one request per page load and never bites.
Add a boundary regression test asserting `/chainlithack` stays rate-limited.
…ion boundary Sync the documented `RATE_LIMIT_EXEMPT_PATHS` default to `/chainlit/,/assets/` and note that prefixes are matched with `startswith`, so the trailing slash is what keeps siblings like `/chainlithack` limited. Add proxy-trust guidance for `UVICORN_FORWARDED_ALLOW_IPS`: prefer the proxy's subnet over `*`. `*` trusts `X-Forwarded-For` from any peer, so when the API port is reachable directly a caller can forge a rotating client IP and evade the per-IP `/auth/*` brute-force limit. The `.env.example` now shows a subnet instead of `*`, and env_vars.md gains a caution covering the too-narrow (collapsed bucket) vs too-wide (spoofable) failure modes plus the nginx `X-Forwarded-For $remote_addr` overwrite and the real-ip module alternative.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A chat session floods the API log with:
Three defects compound here.
1. Chainlit's Socket.IO traffic is rate limited, but can't be attributed.
mount_chainlit()mounts Chainlit's whole ASGI app — including its Socket.IO server at/chainlit/ws/socket.io— inside the API. That subtree is auth-bypassed (is_bypass_path), because Chainlit runs its own header-auth callback, soAuthMiddlewarereturns without settingrequest.state.user.RateLimitMiddlewarethen finds no user:_is_admin()isFalseeven for an admin's browser, and_identity()falls back torequest.client.host.2. That IP is the proxy, not the browser.
entrypoint.shruns the bareuvicornCLI, which readsFORWARDED_ALLOW_IPS— not theUVICORN_FORWARDED_ALLOW_IPSthatapi/main.pyreads and thatenv_vars.mddocuments. The documented variable was a no-op on the default container path. So nginx'sX-Forwarded-Foris untrusted,request.client.hostis the admin-ui container (172.20.0.3), and every chat user in the deployment shares one 600/minute bucket.3. The transport should never have been HTTP.
openrag-admin.conf'slocation /proxied over nginx's default HTTP/1.0 with noUpgrade/Connectionheaders, so Socket.IO's WebSocket upgrade always failed and it stayed on long-polling. Under long-polling each emitted packet terminates the openGETand the client re-polls, so a single streamed answer costs hundreds of requests. The resulting 429s look like transport errors to engine.io, which reconnects — and issues more requests.Note
SUPER_ADMIN_MODEis unrelated: the limiter never reads it, and theis_adminbypass it resembles cannot fire on a path where auth never ran.Changes
One commit each:
fix(rate-limit): exempt the auth-bypassed chainlit subtree— skip the limiter on/chainlitand/assets/, overridable via the newRATE_LIMIT_EXEMPT_PATHS(set-but-empty disables the exemption). A per-identity limit that cannot distinguish identities only throttles everyone at once./auth/*stays limited — it's the brute-force surface and IP keying is the point there.fix(nginx): forward WebSocket upgrades to the API—proxy_http_version 1.1plus the standard$connection_upgrademap. With the upgrade succeeding, chainlit chat becomes one WebSocket, whose ASGI scope iswebsocket, which Starlette'sBaseHTTPMiddlewareskips — so it stops traversing the limiter entirely. This fixes the load; the exemption fixes the accounting.fix(entrypoint): honour UVICORN_FORWARDED_ALLOW_IPS on the uvicorn CLI path— forward the documented variable to--forwarded-allow-ipsso per-user rate limits key on real clients. This gap also made OIDC session cookies shipSecure=Falsebehind a TLS-terminating proxy, so it's worth fixing independently of the above.Verification
pytest tests/unit/api/— 261 passed. The rate-limit suite grew from 10 to 15 tests: chainlit and/assets/exempt under a 2/minute budget, non-exempt routes still 429, env override replaces the defaults, set-but-empty disables them.nginx -tagainstnginx:alpinewith the new conf mounted — syntax OK.entrypoint.shpasses--forwarded-allow-ipsfromUVICORN_FORWARDED_ALLOW_IPS, mirroring the existing guard onapi/main.py.nginx -t, not by watching a browser negotiate the upgrade.Operator notes
UVICORN_FORWARDED_ALLOW_IPSnow takes effect where it previously didn't. It stays defaulted to127.0.0.1(unchanged behaviour, still ignores the proxy) — deployments wanting real client IPs must set it to their subnet or*..env.exampledocuments this, mirrored todocs/assets/env_example.env.Summary by CodeRabbit
New Features
Documentation
Tests