fix: Fix worker class and proxy headers in the web entrypoint - #338
fix: Fix worker class and proxy headers in the web entrypoint#338Uxío (Uxio0) wants to merge 1 commit into
Conversation
uvicorn.workers.UvicornWorker is deprecated since uvicorn 0.30 and the module is going away. uvicorn-worker was already a dependency but unused, so use its worker class instead. Over a unix socket uvicorn leaves scope["client"] as None, so its ProxyHeadersMiddleware never trusted the connection with the default forwarded_allow_ips=127.0.0.1 and dropped both X-Forwarded-For and X-Forwarded-Proto. The same request over the TCP bind got them applied, so request.client and the scheme depended on whether the request came through nginx or hit the port directly. "*" is the only value that works on a socket: the connection reports no peer address to match a trusted host against. Both binds are pod-local and the headers on either come from the nginx sidecar. The worker count was already WEB_CONCURRENCY or 1 through the gunicorn default. Spell it out in the command so it is visible. Same change as safe-queue-service#260.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 8d5e7170eb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| -b unix:$DOCKER_SHARED_DIR/uvicorn.socket \ | ||
| -b 0.0.0.0:8888 \ | ||
| --workers "${WEB_CONCURRENCY:-1}" \ | ||
| --forwarded-allow-ips "*" |
There was a problem hiding this comment.
When the TCP listener is reachable directly—as it is via the checked docker-compose.yml mapping 8888:8888—requests bypass nginx and can supply arbitrary forwarding headers. Gunicorn documents * as disabling frontend-IP checking, so the Uvicorn worker will accept a direct caller's X-Forwarded-For and X-Forwarded-Proto, making request.client, the request scheme, and logged URLs attacker-controlled. Remove or isolate the TCP bind, or avoid wildcard trust on any listener reachable by untrusted clients.
Useful? React with 👍 / 👎.
1 participant
Linear: PLA-1938
What was wrong? 👾
Same three things as safe-global/safe-queue-service#260, which ships an identical entrypoint. Full analysis there.
uvicorn.workers.UvicornWorkerhas been deprecated since uvicorn 0.30 and the module is going away.uvicorn-workerwas already a dependency inpyproject.toml, just unused. (TheDeprecationWarningis filtered out by default, so this is about the removal, not log noise.)request.clientand the request scheme depended on how the request reached the app. Over a unix socket uvicorn leavesscope["client"]asNone, soProxyHeadersMiddlewarenever trusted the connection under the defaultforwarded_allow_ips=127.0.0.1and dropped bothX-Forwarded-ForandX-Forwarded-Proto. The same request over the TCP bind got them applied. Since nginx talks to the socket, everything coming through nginx lost the real client and scheme.The worker count was invisible: it was
WEB_CONCURRENCYor 1 through the gunicorn default.*is the only value that works on a unix socket — the connection reports no peer address to match a trusted host against. Both binds are pod-local and the headers on either come from the nginx sidecar.How was it fixed? 🎯
${WEB_CONCURRENCY:-1}keeps the current behaviour, just spelled out.Verification
Built the image and ran
web+nginxfromdocker-compose.yml::8888serves/health200 and/api/v1/about→1.16.1. Through nginx on:8000:/health,/,/static/favicon.icoand/api/v1/aboutall 200.Sending
X-Forwarded-Proto: httpsstraight to the unix socket from inside the container, the app now logs the request ashttps://localhost/health. It loggedhttp://before.Out of scope
max-requests): no measured leak to justify it.webservice indocker-compose.ymlusesenv_file: .env, whose hosts arelocalhost, so the container cannot reachdb/redis/rabbitmq..env.dockerexists with the right hostnames and is whatsafe-queue-serviceuses. Worth a follow-up; I worked around it with a compose override to verify this PR.