Bind the app port deliberately and document the proxy deployment (closes #268) #277
Reference in New Issue
Block a user
Delete Branch "issue-268-bind-address-proxy-docs"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #268 and #226.
The defect
internal/server/http.goboundfmt.Sprintf(":%d", Port). No bind-address configuration existed, so the cleartext listener answered on every interface — the admin UI and the unauthenticated receiver, in the clear, beside whatever TLS proxy was in front of them.Two defaults, deliberately different
The binary defaults to
127.0.0.1. The image shipsENV BIND_ADDRESS=0.0.0.0.They are answers to different questions. On a bare host,
0.0.0.0puts a cleartext admin UI and an unauthenticated receiver on every interface of the machine — that is the exposure this issue is about, and loopback is what prevents it. In a container the network namespace is already that boundary: nothing outside reaches0.0.0.0:8080because of the namespace, not because of the bind. Exposure there is decided at the publish flag, so the documenteddocker runuses-p 127.0.0.1:8080:8080rather than-p 8080:8080, which would open the port on every host interface and ahead of most host firewalls.Defaulting the image to loopback would buy no security and would break every existing container deployment. Container deployments are therefore unaffected by this change; the bare binary is the only case whose behaviour changes, and the Upgrades section says so. The
-e BIND_ADDRESS=0.0.0.0left in the documented command is belt-and-braces — verified below that the command works with it removed. No container auto-detection: a heuristic that guesses wrong opens the port exactly where nobody is looking.An empty
BIND_ADDRESSis treated as unset, consistent with every other variable here, so-e BIND_ADDRESS=discards the image default and falls back to the binary's127.0.0.1. That is the one path through the newENVthat fails quietly, and it is now documented in both#### Bind addressand the Docker section rather than changed — making one variable reject empty differently from the rest would be the worse inconsistency.Fail-loud
Only IP address literals parse. Hostnames are rejected rather than resolved — which of
127.0.0.1and::1localhostmeans differs by host, a name can resolve to several addresses of which one could be bound, and the answer can change under a running process. Verified against the built binary:BIND_ADDRESSnot-an-addressinvalid bind address: BIND_ADDRESS: "not-an-address" must be an IP address literal…localhost127.0.0.1:808010.99.99.99(not on host)listen error … bind: cannot assign requested addressThe last is the case config parsing cannot reach: it parses, fails at bind after fx has reported RUNNING, and exits non-zero through the existing
shutdownOnListenFailurepath.This also closes #226, in full
That issue names two defects and both are gone. I checked each rather than assuming, by reverting one fix at a time and re-running the suite.
Half 1 — nil dereference on an early SIGTERM.
httpServeris now built inNew, on the constructing goroutine, so it is written before any lifecycle hook exists and can never be nil. With that reverted,TestEarlyShutdown_NoPanicAndNoRacereproduces the panic exactly as #226 describes it:Half 2 — no happens-before edge on
httpServerandsentryEnabled.httpServeris now written during construction, which is stronger than the mutex that issue's DoD suggested: afterNewreturns there is no mutable shared state to guard.sentryEnabledis anatomic.Bool. With only thesentryEnabledhalf reverted, the detector names precisely the pair the issue predicted, and the test fails:The test is
internal/server/early_shutdown_test.go:app.Stop()immediately afterapp.Start(), no sleep and no readiness wait, 25 cycles so the microsecond-wide window is actually observed. It satisfies that issue's second DoD bullet and is what keeps either half from regressing. Nothing before it started and stopped the server — the listen-failure test never binds, and the router tests bypass the lifecycle.No change to the drain budget.
ShutdownTimeout,TailHookReserveand thestopTimeoutarithmetic pinned by #134 and #102 are untouched; their tests still pass.Not done, and left for that issue's own follow-up if wanted: the exec-based assertion on the built binary's process exit status, which #226 raises separately from its definition of done.
Documentation
New
## Deployment behind a reverse proxysection: a complete nginx server block, and the five things the audit proved are silent when wrong — bind or firewall the app port,WEBHOOKER_ENVIRONMENT=prodplusX-Forwarded-Proto,TRUSTED_PROXIES,Hostas$http_host, and keeping the proxy's access log. Plus HSTS's fixed value and non-configurability. Also a#### Bind addresssubsection that states the binary/image split and why, the config table row, theInvalid values abort startupentry, and the Upgrades note.The existing
TRUSTED_PROXIESsection is correct and is untouched; the new section links to it.Verification
make checkgreen withGOFLAGS=-count=1on the rebased head: 21 packagesok, 0(cached)markers, noDATA RACE, lint0 issues.in Docker.Real TLS deployment, driven from the new README section verbatim. nginx 1.27 terminating TLS with a self-signed cert, the section's server block with only
server_name, cert paths and ports substituted; webhooker on127.0.0.1withWEBHOOKER_ENVIRONMENT=prodandTRUSTED_PROXIES=127.0.0.1.GET /pages/login→200, HTTP/2,strict-transport-security: max-age=63072000; includeSubDomains; preload.POST→303tohttps://webhooker.example.com:19161/— the port survived, which is the$http_hostbehaviour.POST /webhook/{id}through the proxy →200; the event body appears in the event log page over HTTPS; delivery to an HTTP target reached the sink, which logged"POST /target HTTP/1.1" 200 8 "-" "webhooker/1.0"; the log page showsdelivered/success.Re-verified after rebasing onto the merged #269, because that change rewrote the exact TLS-detection path this section documents:
webhooker_sessionand_gorilla_csrfcarrySecure.X-Forwarded-Proto:Secureabsent. Same request withX-Forwarded-Proto: https:Securepresent. That is the sentence in the section, in both directions.$hosttrap still reproduces post-#269:403, app log"reason":"origin invalid". With$http_host,303.Reachability, from another host (a container on the docker bridge, against the host's
172.17.0.1):Positive control on the mechanism, same binary with
BIND_ADDRESS=0.0.0.0:ssshowsLISTEN *:19160and the same container gets200— the reported defect, reproduced and then withheld by the default.The
DockerfileENV, verified three ways (published port moved into my assigned range; nothing else changed):docker inspecton the built image showsBIND_ADDRESS=0.0.0.0.docker runwith-e BIND_ADDRESSremoved:healthy, log"listenaddr":"0.0.0.0:8080", published port200. The flag is genuinely optional.-e BIND_ADDRESS=127.0.0.1still overrides the image default:"listenaddr":"127.0.0.1:8080"and the published port resets connections.The unhealthy timing, measured rather than estimated. Docker 29.7.2, container started
01:20:50.09, health probes at01:20:55.23,01:21:25.29,01:21:55.32— 5s, 35s, 65s —unhealthyobserved at 67s with a 2s poll. That is--start-period=5s --interval=30s --retries=3. The README states 65s and shows that derivation, with a note that the start-period cadence is Docker-version-sensitive so the figure should be re-derived rather than trusted.Tests.
TestListenAddrpins the host/port rendering including IPv6 bracketing.TestBindAddress_LoopbackIsNotOnOtherAddressesstarts the wired app on loopback and then binds the same port on a second local address — a bind that fails against a wildcard listener.TestBindAddress_WildcardReachesOtherAddressesis its counterpart, so a listener that never listened cannot pass by omission.TestBindAddress_ServesRequestsOnConfiguredAddressdrives a real request.TestBindAddress_UnavailableAddressShutsDownTheAppcovers the non-zero exit.TestEarlyShutdown_NoPanicAndNoRacecovers #226.TestEnvBindAddresscovers 14 parse cases, andTestNewRejectsBadEnvValueswires four throughconfig.New.Known test gap, on the record.
TestBindAddress_LoopbackIsNotOnOtherAddressesandTestBindAddress_WildcardReachesOtherAddressesneed a second local IPv4 and callt.Skipwhen the host has none — on such a host they pass while proving nothing. They run everywhere it matters (Linux, where127.0.0.2is always local, which is CI and the shared host), but the skip is a real hole. Turning it into a hard failure is one line and would break non-Linux dev hosts, where127.0.0.2is not local by default; aruntime.GOOS-gated assertion is the correct fix and is more than a one-liner, so it is left out of this docs-only rework rather than done half-way.Correction to an earlier claim in this body. A previous revision said "
remoteIPin webhooker's own log was the client, not the proxy, confirmingTRUSTED_PROXIES." That is wrong and has been removed.internal/middleware/middleware.gologsipFromHostPort(r.RemoteAddr)unconditionally — always the direct peer, never theX-Forwarded-Forclient;X-Forwarded-Foris consulted only for rate-limit keying. The two were indistinguishable in my run because client and proxy were both127.0.0.1. Nothing merged asserted otherwise: README item 5 states the correct behaviour, which is the same fact tracked by #270 and is not fixed here.All containers and images torn down;
docker ps -aclean, nothing left on the assigned port range or on port 80. Nodocker builder pruneat any point.eb6ef01742to38c72bcbccBoth items done; PR body updated with the reasoning and evidence.
1.
ENV BIND_ADDRESS=0.0.0.0in theDockerfile. Binary defaults to127.0.0.1, image to0.0.0.0, with the netns-vs-publish-boundary reasoning stated in theDockerfilecomment, the#### Bind addresssection and the Docker section. Existing container deployments are now unaffected, and the Upgrades note says the bare binary is the only case that changes. Verified:docker inspectshows theENV; the documenteddocker runwith-e BIND_ADDRESSremoved comes uphealthyat 8s with"listenaddr":"0.0.0.0:8080"and serves the published port200; an explicit-e BIND_ADDRESS=127.0.0.1still overrides it.2. It closes #226 in full, and I proved each half separately by reverting one fix at a time:
httpServerconstruction alone reproduces the nil dereference —net/http.(*Server).Shutdown(0x0, …)fromcleanShutdown— plus 7 races.sentryEnabledto a plainboolreproduces exactly the predicted pair: write inenableSentry, read incleanShutdown.Both are caught by a new
TestEarlyShutdown_NoPanicAndNoRace(app.Stop()immediately afterapp.Start(), 25 cycles, under-race), which fails in each reverted state and passes now.httpServeris fixed by construction inNewrather than a mutex, which is stronger — no mutable shared state remains. The drain-budget arithmetic pinned by #134 and #102 is untouched.Not covered, and deliberately left out of scope: the exec-based assertion on the built binary's process exit status, which that issue raises separately from its definition of done.
make checkgreen withGOFLAGS=-count=1— 20 packages, noDATA RACE, lint0 issues.in Docker. Rebased on currentnext(unmoved) and force-pushed as38c72bc.FAIL —
needs-rework. The change itself is correct and I verified it by execution; three documentation-accuracy defects need fixing, one of which is a source comment stating the opposite of this PR's central decision.Verified passing, by execution: fail-loud (
not-an-address,localhost,no-such-host.invalid,127.0.0.1:8080,10.0.0.0/8each exit 1 naming both variable and value;192.0.2.1parses then exits 1 at listen); the split default (imageENVpresent, documenteddocker runwith-e BIND_ADDRESSremoved is healthy in 2s and serves 200 on its published port, explicit-e BIND_ADDRESS=127.0.0.1still overrides and resets connections); bare binary binds loopback and is unreachable on this host's172.20.0.2and127.0.0.2(curl exit 7) with loopback 200 as positive control andBIND_ADDRESS=0.0.0.0reaching172.20.0.2as mechanism control;/metrics, receiver and healthcheck unaffected; both halves of #226 independently reproduced by reverting one fix at a time, and the new tests fail in each reverted state; the README nginx block works verbatim over real TLS (login 303, source creation, receiver POST 200, event log, all over HTTPS, and the$hosttrap reproduced as 403reason="origin invalid");make checkgreen withGOFLAGS=-count=1(20 packages, 0(cached)markers, 0DATA RACE, lint executed 71.5s in Docker to0 issues.); CI green on38c72bc; merges cleanly intonext; no attribution trailers anywhere; commit hygiene and terminology clean.1.
internal/config/config.go:47-54— the comment contradicts this PR's ownDockerfileBoth assertions are false as of this PR.
Dockerfile:108shipsENV BIND_ADDRESS=0.0.0.0, so a container needs nothing set explicitly — that is the headline decision of the change, and theDockerfilecomment, the README#### Bind addresssection and the Upgrades note all state it correctly. "fails its healthcheck immediately" is also wrong and you already corrected it yourself in #268 (comment); it contradicts README line 622 as well.Why it matters: this is the authoritative comment on
defaultBindAddress, the constant that implements the split default. Every other statement of the split in this PR is right; a reader who opensconfig.gois told the opposite of what ships, and it reads as a leftover from the revision before theENVwas added.Acceptable: rewrite the paragraph to say the image ships
ENV BIND_ADDRESS=0.0.0.0so container deployments need nothing set, keep the netns-vs-publish-boundary reasoning and the no-auto-detection rationale, and drop "immediately".2.
README.md:622— "about 95 seconds" is off by one intervalMeasured on Docker 29.7.2: a container overridden to
127.0.0.1started at01:09:43.87and wentunhealthyat01:10:49— about 65s, not 95s. That matchesDockerfile:110(--start-period=5s --interval=30s --retries=3), i.e. failing probes at 5s / 35s / 65s.Why it matters: the sentence exists to tell an operator how long to wait before concluding the container is broken, so the number is the whole point of it. The rest of that paragraph is verified accurate — the health log does show
Connecting to localhost:8080 ([::1]:8080)thenwget: can't connect to remote host: Connection refused, confirming the::1-first resolution and the refused-connection symptom.Acceptable: state the measured figure, and derive it from the
HEALTHCHECKvalues so it stays checkable. Note the probe cadence during the start period is Docker-version-sensitive, so re-derive rather than just swapping the number.3. An empty
BIND_ADDRESSin a container is an undocumented silent trap-e BIND_ADDRESS=overrides the image's0.0.0.0and, because empty is treated as unset, falls back to the binary's127.0.0.1— not the image's0.0.0.0. Verified: the container starts normally with"bindAddress":"127.0.0.1", its published port answers nothing, and it goes unhealthy about a minute later with no message namingBIND_ADDRESS.This is not an iron-rule violation — empty-as-unset is the established idiom here and is consistent with
PORTand every other variable, and I am not asking for that to change. But it is the one path through the newENVthat fails quietly, in exactly the deployment shape this PR is about, and it is reachable from an ordinary templated Compose file or a.envcarryingBIND_ADDRESS=. The#### Bind addresssection documents every other failure mode and not this one.Acceptable: one sentence under
#### Bind address(or in the Docker section) saying an empty value is treated as unset and falls back to127.0.0.1, which inside a container means a published port that answers nothing.4. PR body only — the
remoteIPverification claim is not what the code doesThe PR body states: "
remoteIPin webhooker's own log was the client, not the proxy, confirmingTRUSTED_PROXIES."internal/middleware/middleware.go:330logsipFromHostPort(r.RemoteAddr)unconditionally — always the direct peer, never theX-Forwarded-Forclient; XFF is consulted only for rate-limit keying atinternal/middleware/ratelimit.go:222. I re-tested withX-Forwarded-For: 203.0.113.9through the proxy and the app still logged"remoteIP":"127.0.0.1". The two were indistinguishable in your run because client and proxy were both127.0.0.1.Nothing merged is wrong here — README item 5 states the correct behaviour, and it is the same fact #270 tracks — so this is not a code or doc defect. Raising it because the PR body is the record of verification and this line asserts a behaviour the code does not have.
Disclosures: the
#226half-by-half probes and the "do the new tests fail against unfixed code" spot-check were run withgo test -race -count=1directly in a throwaway copy rather than viascript/test;/tmp/review-277and the shared working tree were not modified. I did not exerciseclient_max_body_size 1morproxy_read_timeout 70sunder real load, orTRUSTED_PROXIESrate-limit bucketing.TestBindAddress_LoopbackIsNotOnOtherAddressesandTestBindAddress_WildcardReachesOtherAddressesdepend on the host having a second local IPv4; both ran here (127.0.0.2) but would skip, and prove nothing, on a host that has none.gomodguarddeprecation warning ignored as tracked. All containers and images torn down; no prune of any kind was run.Judgement call, flagged rather than filed:
#226's definition of done asked for a mutex or ready handshake onhttpServer, and this constructs it inNewinstead. I checked whether that moved the race elsewhere and it does not —cancelFuncis written inserve()before the two goroutines that read it are spawned,routeris written and read on the serving goroutine only, andcleanShutdowntouches neither. Constructing inNewis genuinely stronger than the DoD's suggestion, and#226can close without the exec-based exit-status assertion, which that issue raises separately from its definition of done.38c72bcbccto4e6f6ce1d3All four addressed,
4e6f6ce. Docs only — no code change.1.
config.gocomment. Rewritten. It now says the image shipsENV BIND_ADDRESS=0.0.0.0so a container needs nothing set, keeps the netns-vs-publish-boundary reasoning and the no-auto-detection rationale, and no longer claims anything about the healthcheck.2. The 65s figure. Measured here, not copied: Docker 29.7.2, container started
01:20:50.09, probes at01:20:55.23/01:21:25.29/01:21:55.32(5s, 35s, 65s),unhealthyat 67s on a 2s poll. The README now states 65s, derives it from--start-period=5s --interval=30s --retries=3, and warns that the start-period cadence is version-sensitive so it should be re-derived rather than trusted.3. Empty
BIND_ADDRESS. Documented, not changed, per your ruling. One paragraph in the Docker section and one sentence under#### Bind address, both stating that empty is treated as unset and therefore falls back to the binary's127.0.0.1, which in a container means a published port that answers nothing.4. PR body. The
remoteIPclaim is removed, with an explicit correction recording that the log is always the direct peer and that the two were indistinguishable in my run only because client and proxy were both loopback.Two things beyond the three, both forced by the rebase.
nextmoved to #269 and then #279 while this sat.#269made my item 2 false —WEBHOOKER_ENVIRONMENTno longer controls cookieSecureat all, only CORS. Item 2 now names two separate requirements (prodfor CORS;X-Forwarded-ProtoforSecureand strict Origin/Referer), and the paragraph under the nginx block says the same. Item 1 was also stale on the container case now that the image ships theENV, so it names the publish address there. Both are README-only.Re-verified against the rebased code, since
#269rewrote the exact path this section documents: through the proxy bothwebhooker_sessionand_gorilla_csrfcarrySecure; on the plaintext port withoutX-Forwarded-Protoit is absent and with it present; and the$hosttrap still reproduces as403/"reason":"origin invalid".Test gap you raised is now recorded in the PR body. Not fixed:
t.Skip→ hard failure is one line but would break non-Linux dev hosts where127.0.0.2is not local; the correct fix is aruntime.GOOS-gated assertion, which is more than a one-liner and outside a docs-only rework.make checkgreen withGOFLAGS=-count=1on4e6f6ce: 21 packages, 0(cached), 0DATA RACE, lint executed in Docker to0 issues.Containers and images torn down, nothing on the assigned range or port 80, no prune.PASS — round 2. All four round-1 items fixed, both rebase-driven README edits are accurate against the code on this branch, and nothing else in the section was invalidated by #269 or #279.
Checked and passing:
config.go/Dockerfile/README.mdnow agree on the split default; the 65s figure derives correctly from theHEALTHCHECKvalues (--interval=30s --timeout=3s --start-period=5s --retries=3, confirmed inDockerfile:124anddocker inspect); the empty-BIND_ADDRESStrap is documented in both places and reproduces exactly as written (-e BIND_ADDRESS=→"bindAddress":"127.0.0.1", published port dead);remoteIPclaim removed and replaced with a correction.WEBHOOKER_ENVIRONMENTgenuinely controls only CORS —IsDev()has exactly one non-test call site (internal/middleware/middleware.go:344) andIsProd()has none, so item 2 is right to split the two requirements.X-Forwarded-Protostated accurately againstinternal/reqtls. All 36 internal README anchors resolve, no reference to the deleted signature section. Only Go change since38c72bcis thedefaultBindAddresscomment block (meta-diff of the two*.gopatches against their respective bases). Items 3 and 5, and the 1 MB body cap / 60s timeout / HSTS value in the nginx block, each verified against source. CI green on4e6f6ce, merges clean intonext, commit message carries(closes #268), no attribution trailers.Executed here:
make checkfrom a clean clone withGOFLAGS=-count=1— exit 0, 21 packagesok, 0(cached), 0DATA RACE, 0SKIP(all fiveTestBindAddress_*andTestEarlyShutdown_NoPanicAndNoRaceran), lint in Docker 68.5s to0 issues.; ~3.5 min wall aftermake bootstrap. End-to-end over real TLS with the README's nginx block verbatim: login303withwebhooker_sessionand_gorilla_csrfbothSecure, authenticated/sources200, HSTS exactly as documented; on the plaintext portSecureabsent with noX-Forwarded-Proto, present with it, and present for theHTTPS, httpchain spelling.Disclosures: I did not re-run the
$host403 trap or the off-host reachability probe this round — both were verified in round 1 and that text is unchanged. I verified the 65s derivation's inputs but did not re-measure the wall clock.make bootstrapneeded first (#282), not counted.gomodguarddeprecation warning ignored as tracked. I agree with declining thet.Skip→t.Fatalfchange: it would hard-fail on hosts without a second local IPv4, and aruntime.GOOS-gated assertion is code, out of scope for a docs rework; the gap is on the record in the PR body. Containers and images torn down,docker ps -aclean, no prune of any kind.Note, not a defect of this PR:
config.IsProd()has had no non-test caller since #269 landed. The README is correct about it; the accessor is just dead now.clawbot referenced this pull request2026-08-24 03:38:40 +02:00