Survive a missing client address, and surface the degraded default - #138
Merged
Conversation
…default Setting ADDRESS_HEADER on a real deployment took the site down, and the failure generalises to anyone running behind a reverse proxy. With ADDRESS_HEADER set, adapter-node *throws* when that header is absent instead of falling back — and it is legitimately absent on every request that does not come through the proxy, starting with the container health check. The proxy handler called getClientAddress() unguarded, so the exception propagated: health check failed, orchestrator marked the container unhealthy, pulled it from the pool, and the site 404'd. Nothing in the logs pointed at the header. It now degrades instead of throwing. When the address cannot be resolved the incoming X-Forwarded-For is *deleted* rather than passed through: falling back to it would hand the rate limiter a value the client chose, which is the spoofing this proxy exists to prevent. Both paths are tested, including the health-check case that caused the outage. The deeper problem was that the broken-by-default state was invisible. In the bundled image every browser request reaches the API through the web app's proxy, so the peer is always 127.0.0.1; without TRUSTED_PROXIES the API correctly refuses to believe the forwarded address and falls back to the peer — meaning every user shares one rate-limit bucket. The limits still apply, they just apply to the whole instance at once, and no operator would notice. The API now says so at startup, naming the three variables that fix it. docs/self-hosting.md gains a section covering them, why XFF_DEPTH matters (X-Forwarded-For is a client-prefillable list; only the entries your own proxies appended can be trusted), the Nginx directives Caddy and Traefik set on their own, and a command to verify the health check still passes afterwards — the exact check I skipped.
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
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.
Found by deploying the previous work to a real instance behind Traefik. Both problems affect any self-hosted deployment behind a reverse proxy, not one particular setup.
1. Setting
ADDRESS_HEADERtook the site downWith
ADDRESS_HEADERset,adapter-nodethrows when the header is absent rather than falling back. And it is legitimately absent on every request that does not come through the proxy — starting with the container health check.The proxy handler called
getClientAddress()unguarded, so:Two minutes of downtime on a real instance, and nothing in the logs pointed at the header.
It now degrades instead of throwing. Note what happens on the fallback path:
Deleting rather than passing through is deliberate: falling back to the incoming header would hand the rate limiter a value the client chose — the spoofing this proxy exists to prevent. Both branches are tested, including the health-check case that caused the outage.
2. The broken-by-default state was invisible
In the bundled image every browser request reaches the API through the web app's proxy, so the API's peer is always
127.0.0.1. WithoutTRUSTED_PROXIESit correctly refuses to believe the forwarded address and falls back to the peer — so every user shares a single rate-limit bucket.The limits still apply. They just apply to the whole instance at once, so one busy visitor exhausts them for everybody. No operator would notice: nothing fails, nothing is logged, and the README says "per-IP rate limiting".
The API now says so at startup:
Silent when a proxy is trusted, and explicit that a directly-exposed API can ignore it.
Documentation
docs/self-hosting.mdgains Making rate limiting work behind a proxy: the three variables, whyXFF_DEPTHmatters (X-Forwarded-Foris a list the client can prefill — only the entries your own proxies appended are trustworthy, so too high lets a client pick its bucket and too low puts everyone back in one), theproxy_set_headerdirectives Nginx needs and Caddy/Traefik set on their own, and a command to confirm the health check still passes:That last one is the check I should have run before declaring the config change good.
.env.examplehadTRUSTED_PROXIESdocumented as "leave empty when the server is exposed directly (recommended default)" — accurate for a bare API, misleading for this image, where a proxy is always involved. Corrected, withADDRESS_HEADER/XFF_DEPTHdocumented alongside since all three are needed together.843 tests at 100% coverage; lint, typecheck and build pass.