Conversation
CC_FORWARDED_ALLOW_IPS appends trusted proxy IPs (e.g. a same-host Caddy reached via a non-loopback address) to the uvicorn proxy-headers allowlist.
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: df67b07f29
ℹ️ 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 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 address that feedback".
|
This is useful for custom reverse-proxy setups. Before merging, please add regression coverage for:
Please test the middleware behavior, not just the generated allowlist string. Also document the variable and its trust boundary: configured addresses must belong to trusted reverse proxies, with direct relay access restricted appropriately. Update the existing loopback-only documentation to match. |
FORWARDED_ALLOW_IPS appends operator-supplied IPs or CIDR networks to the built-in loopback proxy defaults, so a same-host reverse proxy that cannot reach the relay over loopback (Tailscale, LAN, Docker bridge gateway) is trusted instead of silently distrusted. - Move the allowlist into RelayConfig alongside the other uvicorn options and validate it: wildcard, /0, and malformed entries were previously accepted by the env-only patch and then silently never matched a peer, which is the same undiagnosable failure this change exists to fix. - Add middleware-level regression tests that drive uvicorn's ProxyHeadersMiddleware end to end: unset/blank stays loopback-only, extra addresses are trusted while the defaults survive, a trusted proxy forwards the client address, and an untrusted peer cannot assert a scheme or choose its own rate-limit bucket through forwarded headers. - Document the variable and its trust boundary in both configuration tables and the relay env templates, and update the loopback-only prose in the deployment docs to match. FORWARDED_ALLOW_IPS is unprefixed to match every other relay tunable; the earlier fork-only CC_FORWARDED_ALLOW_IPS name is dropped rather than kept as an alias, since it was never documented and nothing outside this branch reads it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da1ba5195c
ℹ️ 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 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 address that feedback".
- require the relay to listen on the container interface in bridge mode - bind the published host port to loopback before trusting the bridge gateway
|
Addressed the requested follow-up coverage and trust-boundary documentation.
Current head: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4130bdb9e
ℹ️ 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 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 address that feedback".
| | `PUBLIC_ORIGIN` | empty | Exact browser origin allowed to connect, e.g. `https://remote.example.com`; **required**, and non-loopback origins must use HTTPS unless `ALLOW_INSECURE_HTTP` is enabled. | | ||
| | `ALLOW_PRIVATE_ORIGINS` | `0` | Set to `1` to retain `PUBLIC_ORIGIN` while also accepting literal private/loopback IP origins on `RELAY_PORT`: `127/8`, `10/8`, `172.16/12`, `192.168/16`, Tailscale `100.64/10`, IPv6 loopback, and ULA. The Origin scheme/host/port must also exactly match the effective request target; hostnames, public IPs, and other ports remain rejected. Private HTTP is unencrypted and normally cannot install a PWA. | | ||
| | `ALLOW_INSECURE_HTTP` | `0` | Escape hatch for a bare public IPv4 address: allows plain `http://`/`ws://` outside loopback. Off by default; login credentials, cookies, wrapper tokens, and all session traffic are unencrypted while enabled. Prefer TLS whenever possible. | | ||
| | `FORWARDED_ALLOW_IPS` | empty | Extra peers trusted to supply `X-Forwarded-Proto` / `X-Forwarded-For`: a comma-separated list of IPs or CIDR networks, appended after the built-in `127.0.0.1,::1`. Only needed when the reverse proxy does not reach the relay over loopback (Tailscale, LAN, or the Docker bridge gateway): uvicorn ignores the proxy headers from an untrusted peer, so the relay computes an `http:80` request target while the browser's Origin says `https:443`, and page loads and `/api/*` keep working while every WebSocket is rejected with 403. List **only the addresses your own reverse proxy connects from** — a trusted peer may claim any client address and any scheme, so the relay must stay unreachable from anywhere else, and `*`, `0.0.0.0/0`, and `::/0` are rejected at startup. Malformed entries are rejected at startup too, rather than silently never matching. | |
There was a problem hiding this comment.
In this untrusted-proxy scenario, the browser's POST /api/login includes an Origin and is rejected by _request_origin_allowed before authentication (cc_remote/relay/server.py:721-728); the new regression test likewise expects a 403 (tests/test_relay_forwarded_headers.py:207-214). Therefore the claim that /api/* keeps working and only WebSockets fail is misleading for an operator who cannot even log in; describe both origin-checked API requests and WebSocket upgrades as affected.
Useful? React with 👍 / 👎.
2 participants
Problem
forwarded_allow_ipsis hardcoded to127.0.0.1,::1. That only covers a reverse proxy reaching the relay over loopback. Any same-host proxy reached via a non-loopback address — a Tailscale IP, a LAN address, a docker bridge gateway — is silently distrusted.The failure mode is nasty to diagnose: uvicorn then ignores
X-Forwarded-Proto, so_request_target_parts()computeshttp:80while the browser's Origin ishttps:443, and every WebSocket upgrade is rejected with 403 by the origin check. Page loads fine,/api/*works, only WS fails.Fix
Read extra trusted proxies from
FORWARDED_ALLOW_IPS(comma-separated IPs or CIDR networks) and append them to the built-in loopback defaults.Fully backwards compatible: with the env unset the behavior is byte-identical to today.
The variable is unprefixed to match every other relay tunable (
RELAY_HOST,RELAY_PORT,ALLOW_PRIVATE_ORIGINS,ALLOW_INSECURE_HTTP,WS_MAX_SIZE_BYTES) and uvicorn's own--forwarded-allow-ipsflag;CC_-prefixed names are wrapper-side in this repo.Review follow-ups
Regression coverage (
tests/test_relay_forwarded_headers.py, 24 tests). These drive uvicorn's ownProxyHeadersMiddlewareend to end —TestClient(client=(peer, port))sets the ASGI peer address the middleware inspects — rather than asserting on the allowlist string, since a correct string is not what makes the feature work:"", whitespace, and bare commas all yield127.0.0.1,::1, and the behavior is confirmed through the middleware: a non-loopback proxy still gets 403, loopback still gets 200.X-Forwarded-Forselects the rate-limit bucket and itsX-Forwarded-Protosatisfies the origin gate; an untrusted peer forging both cannot claim a scheme it does not terminate, cannot pick its own rate-limit bucket, and is closed with 1008 on the WebSocket route.A
ProxyHeadersMiddlewarecase covers the reported bug directly: same peer, same headers, only the allowlist changes — 403 before, 200 after.Docs.
FORWARDED_ALLOW_IPSis documented in both configuration tables and all three relay env templates, each stating the trust boundary: listed addresses must belong to your own reverse proxy, because a trusted peer can claim any client IP and any scheme, so the relay must stay unreachable from anywhere else. The three places that claimed forwarded metadata is trusted only from loopback (deploy/README.md,deploy/nginx-reverse-proxy.conf.example,CLAUDE.md/AGENTS.md) now describe the real boundary.Validation beyond the tests
The allowlist is validated at startup, because uvicorn's parser classifies anything that is not a valid IP or CIDR — a hostname, a typo, a malformed CIDR — as a string literal compared against the peer address, so it would start cleanly and then silently never match. Upstream documents this hazard itself in
_TrustedHosts.__init__:That is the same undiagnosable failure this change exists to fix, so it is rejected instead:
*,0.0.0.0/0, and::/0are rejected as trusting every peer._request_ip()'s own loopback check in the relay is a second, independent trust boundary (rate-limit bucketing) and was verified correct as-is — it needed coverage, not a code change.