internal/server/http.go bound fmt.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 ships ENV BIND_ADDRESS=0.0.0.0.
They are answers to different questions. On a bare host, 0.0.0.0 puts 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 reaches 0.0.0.0:8080 because of the namespace, not because of the bind. Exposure there is decided at the publish flag, so the documented docker run uses -p 127.0.0.1:8080:8080 rather 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.0 left 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 emptyBIND_ADDRESS is treated as unset, consistent with every other variable here, so -e BIND_ADDRESS= discards the image default and falls back to the binary's 127.0.0.1. That is the one path through the new ENV that fails quietly, and it is now documented in both #### Bind address and 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.1 and ::1localhost means 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_ADDRESS
result
not-an-address
exit 1, invalid bind address: BIND_ADDRESS: "not-an-address" must be an IP address literal…
The 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 shutdownOnListenFailure path.
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.httpServer is now built in New, on the constructing goroutine, so it is written before any lifecycle hook exists and can never be nil. With that reverted, TestEarlyShutdown_NoPanicAndNoRace reproduces the panic exactly as #226 describes it:
Half 2 — no happens-before edge on httpServer and sentryEnabled.httpServer is now written during construction, which is stronger than the mutex that issue's DoD suggested: after New returns there is no mutable shared state to guard. sentryEnabled is an atomic.Bool. With only the sentryEnabled half reverted, the detector names precisely the pair the issue predicted, and the test fails:
WARNING: DATA RACE
Write at … by goroutine 609: (*Server).enableSentry() server.go:153
Previous read at … by goroutine 615: (*Server).cleanShutdown() server.go:253
The test is internal/server/early_shutdown_test.go: app.Stop() immediately after app.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, TailHookReserve and the stopTimeout arithmetic 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 proxy section: a complete nginx server block, and the five things the audit proved are silent when wrong — bind or firewall the app port, WEBHOOKER_ENVIRONMENT=prod plus X-Forwarded-Proto, TRUSTED_PROXIES, Host as $http_host, and keeping the proxy's access log. Plus HSTS's fixed value and non-configurability. Also a #### Bind address subsection that states the binary/image split and why, the config table row, the Invalid values abort startup entry, and the Upgrades note.
The existing TRUSTED_PROXIES section is correct and is untouched; the new section links to it.
Verification
make check green with GOFLAGS=-count=1 on the rebased head: 21 packages ok, 0(cached) markers, no DATA RACE, lint 0 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 on 127.0.0.1 with WEBHOOKER_ENVIRONMENT=prod and TRUSTED_PROXIES=127.0.0.1.
HTTPS GET /pages/login → 200, HTTP/2, strict-transport-security: max-age=63072000; includeSubDomains; preload.
Login POST → 303 to https://webhooker.example.com:19161/ — the port survived, which is the $http_host behaviour.
Source created, entrypoint used, 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 shows delivered / success.
Re-verified after rebasing onto the merged #269, because that change rewrote the exact TLS-detection path this section documents:
Through the proxy, both webhooker_session and _gorilla_csrf carry Secure.
Directly on the plaintext port with no X-Forwarded-Proto: Secure absent. Same request with X-Forwarded-Proto: https: Secure present. That is the sentence in the section, in both directions.
The $host trap 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):
app port 19160, default bind → exit 7, "Failed to connect to 172.17.0.1 port 19160"
proxy port 19161 (control) → 200
Positive control on the mechanism, same binary with BIND_ADDRESS=0.0.0.0: ss shows LISTEN *:19160 and the same container gets 200 — 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 inspect on the built image shows BIND_ADDRESS=0.0.0.0.
The documented docker runwith -e BIND_ADDRESS removed: healthy, log "listenaddr":"0.0.0.0:8080", published port 200. The flag is genuinely optional.
An explicit -e BIND_ADDRESS=127.0.0.1 still 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 at 01:20:55.23, 01:21:25.29, 01:21:55.32 — 5s, 35s, 65s — unhealthy observed 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.TestListenAddr pins the host/port rendering including IPv6 bracketing. TestBindAddress_LoopbackIsNotOnOtherAddresses starts 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_WildcardReachesOtherAddresses is its counterpart, so a listener that never listened cannot pass by omission. TestBindAddress_ServesRequestsOnConfiguredAddress drives a real request. TestBindAddress_UnavailableAddressShutsDownTheApp covers the non-zero exit. TestEarlyShutdown_NoPanicAndNoRace covers #226. TestEnvBindAddress covers 14 parse cases, and TestNewRejectsBadEnvValues wires four through config.New.
Known test gap, on the record.TestBindAddress_LoopbackIsNotOnOtherAddresses and TestBindAddress_WildcardReachesOtherAddresses need a second local IPv4 and call t.Skip when the host has none — on such a host they pass while proving nothing. They run everywhere it matters (Linux, where 127.0.0.2 is 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, where 127.0.0.2 is not local by default; a runtime.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 "remoteIP in webhooker's own log was the client, not the proxy, confirming TRUSTED_PROXIES." That is wrong and has been removed. internal/middleware/middleware.go logs ipFromHostPort(r.RemoteAddr) unconditionally — always the direct peer, never the X-Forwarded-For client; X-Forwarded-For is consulted only for rate-limit keying. The two were indistinguishable in my run because client and proxy were both 127.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 -a clean, nothing left on the assigned port range or on port 80. No docker builder prune at any point.
Closes https://git.eeqj.de/sneak/webhooker/issues/268 and https://git.eeqj.de/sneak/webhooker/issues/226.
## The defect
`internal/server/http.go` bound `fmt.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 ships `ENV BIND_ADDRESS=0.0.0.0`.**
They are answers to different questions. On a bare host, `0.0.0.0` puts 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 reaches `0.0.0.0:8080` because of the namespace, not because of the bind. Exposure there is decided at the **publish flag**, so the documented `docker run` uses `-p 127.0.0.1:8080:8080` rather 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.0` left 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_ADDRESS` is treated as unset, consistent with every other variable here, so `-e BIND_ADDRESS=` discards the image default and falls back to the binary's `127.0.0.1`. That is the one path through the new `ENV` that fails quietly, and it is now documented in both `#### Bind address` and 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.1` and `::1` `localhost` means 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_ADDRESS` | result |
| --- | --- |
| `not-an-address` | exit 1, `invalid bind address: BIND_ADDRESS: "not-an-address" must be an IP address literal…` |
| `localhost` | exit 1 |
| `127.0.0.1:8080` | exit 1 |
| `10.99.99.99` (not on host) | exit 1, `listen error … bind: cannot assign requested address` |
The 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 `shutdownOnListenFailure` path.
## This also closes https://git.eeqj.de/sneak/webhooker/issues/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.** `httpServer` is now built in `New`, on the constructing goroutine, so it is written before any lifecycle hook exists and can never be nil. With that reverted, `TestEarlyShutdown_NoPanicAndNoRace` reproduces the panic exactly as https://git.eeqj.de/sneak/webhooker/issues/226 describes it:
```
panic: runtime error: invalid memory address or nil pointer dereference
net/http.(*Server).Shutdown(0x0, {…})
sneak.berlin/go/webhooker/internal/server.(*Server).cleanShutdown(…)
```
**Half 2 — no happens-before edge on `httpServer` and `sentryEnabled`.** `httpServer` is now written during construction, which is stronger than the mutex that issue's DoD suggested: after `New` returns there is no mutable shared state to guard. `sentryEnabled` is an `atomic.Bool`. With **only** the `sentryEnabled` half reverted, the detector names precisely the pair the issue predicted, and the test fails:
```
WARNING: DATA RACE
Write at … by goroutine 609: (*Server).enableSentry() server.go:153
Previous read at … by goroutine 615: (*Server).cleanShutdown() server.go:253
```
**The test** is `internal/server/early_shutdown_test.go`: `app.Stop()` immediately after `app.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`, `TailHookReserve` and the `stopTimeout` arithmetic pinned by https://git.eeqj.de/sneak/webhooker/issues/134 and https://git.eeqj.de/sneak/webhooker/issues/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 https://git.eeqj.de/sneak/webhooker/issues/226 raises separately from its definition of done.
## Documentation
New `## Deployment behind a reverse proxy` section: a complete nginx server block, and the five things the audit proved are silent when wrong — bind or firewall the app port, `WEBHOOKER_ENVIRONMENT=prod` plus `X-Forwarded-Proto`, `TRUSTED_PROXIES`, `Host` as `$http_host`, and keeping the proxy's access log. Plus HSTS's fixed value and non-configurability. Also a `#### Bind address` subsection that states the binary/image split and why, the config table row, the `Invalid values abort startup` entry, and the Upgrades note.
The existing `TRUSTED_PROXIES` section is correct and is untouched; the new section links to it.
## Verification
`make check` green with `GOFLAGS=-count=1` on the rebased head: 21 packages `ok`, **0** `(cached)` markers, no `DATA RACE`, lint `0 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 on `127.0.0.1` with `WEBHOOKER_ENVIRONMENT=prod` and `TRUSTED_PROXIES=127.0.0.1`.
- HTTPS `GET /pages/login` → `200`, HTTP/2, `strict-transport-security: max-age=63072000; includeSubDomains; preload`.
- Login `POST` → `303` to `https://webhooker.example.com:19161/` — the **port survived**, which is the `$http_host` behaviour.
- Source created, entrypoint used, `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 shows `delivered` / `success`.
**Re-verified after rebasing onto the merged https://git.eeqj.de/sneak/webhooker/issues/269**, because that change rewrote the exact TLS-detection path this section documents:
- Through the proxy, both `webhooker_session` and `_gorilla_csrf` carry `Secure`.
- Directly on the plaintext port with no `X-Forwarded-Proto`: `Secure` absent. Same request with `X-Forwarded-Proto: https`: `Secure` present. That is the sentence in the section, in both directions.
- The `$host` trap 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`):
```
app port 19160, default bind → exit 7, "Failed to connect to 172.17.0.1 port 19160"
proxy port 19161 (control) → 200
```
Positive control on the mechanism, same binary with `BIND_ADDRESS=0.0.0.0`: `ss` shows `LISTEN *:19160` and the same container gets `200` — the reported defect, reproduced and then withheld by the default.
**The `Dockerfile` `ENV`, verified three ways** (published port moved into my assigned range; nothing else changed):
- `docker inspect` on the built image shows `BIND_ADDRESS=0.0.0.0`.
- The documented `docker run` **with `-e BIND_ADDRESS` removed**: `healthy`, log `"listenaddr":"0.0.0.0:8080"`, published port `200`. The flag is genuinely optional.
- An explicit `-e BIND_ADDRESS=127.0.0.1` still 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 at `01:20:55.23`, `01:21:25.29`, `01:21:55.32` — 5s, 35s, 65s — `unhealthy` observed 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.** `TestListenAddr` pins the host/port rendering including IPv6 bracketing. `TestBindAddress_LoopbackIsNotOnOtherAddresses` starts 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_WildcardReachesOtherAddresses` is its counterpart, so a listener that never listened cannot pass by omission. `TestBindAddress_ServesRequestsOnConfiguredAddress` drives a real request. `TestBindAddress_UnavailableAddressShutsDownTheApp` covers the non-zero exit. `TestEarlyShutdown_NoPanicAndNoRace` covers https://git.eeqj.de/sneak/webhooker/issues/226. `TestEnvBindAddress` covers 14 parse cases, and `TestNewRejectsBadEnvValues` wires four through `config.New`.
**Known test gap, on the record.** `TestBindAddress_LoopbackIsNotOnOtherAddresses` and `TestBindAddress_WildcardReachesOtherAddresses` need a second local IPv4 and call `t.Skip` when the host has none — on such a host they pass while proving nothing. They run everywhere it matters (Linux, where `127.0.0.2` is 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, where `127.0.0.2` is not local by default; a `runtime.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 "`remoteIP` in webhooker's own log was the client, not the proxy, confirming `TRUSTED_PROXIES`." That is **wrong** and has been removed. `internal/middleware/middleware.go` logs `ipFromHostPort(r.RemoteAddr)` unconditionally — always the direct peer, never the `X-Forwarded-For` client; `X-Forwarded-For` is consulted only for rate-limit keying. The two were indistinguishable in my run because client and proxy were both `127.0.0.1`. Nothing merged asserted otherwise: README item 5 states the correct behaviour, which is the same fact tracked by https://git.eeqj.de/sneak/webhooker/issues/270 and is not fixed here.
All containers and images torn down; `docker ps -a` clean, nothing left on the assigned port range or on port 80. No `docker builder prune` at any point.
The plaintext listener bound `:PORT`, so it answered on every
interface with no way to say otherwise. That published the admin UI
and the unauthenticated receiver in cleartext beside whatever TLS
proxy was in front of them, reachable from any host that could route
to the machine.
BIND_ADDRESS now selects the address, defaulting to 127.0.0.1.
Loopback is the only default that does not silently expose that
listener; reaching webhooker from another host becomes a deliberate
act. A container must set BIND_ADDRESS=0.0.0.0, since a loopback bind
inside a network namespace is unreachable even with -p, and the
documented `docker run` does. Only IP address literals are accepted:
hostnames, host:port and CIDR blocks abort startup naming the variable
and the value, and a literal that is not an address of this host fails
at listen and exits non-zero.
The http.Server is now built in New rather than in the serving
goroutine. Two goroutines reached that field with nothing ordering
them — the serving goroutine assigning it, the fx stop hook calling
Shutdown on it — which is a data race the race detector reports as
soon as anything starts and stops the server, and a nil dereference if
a stop arrived first.
README gains a "Deployment behind a reverse proxy" section: a working
nginx server block, and the five things that are silent when wrong —
bind or firewall the app port, WEBHOOKER_ENVIRONMENT=prod,
TRUSTED_PROXIES, Host as $http_host rather than $host, and keeping the
proxy's access log because webhooker's own records only the proxy.
Both items done; PR body updated with the reasoning and evidence.
1. ENV BIND_ADDRESS=0.0.0.0 in the Dockerfile. Binary defaults to 127.0.0.1, image to 0.0.0.0, with the netns-vs-publish-boundary reasoning stated in the Dockerfile comment, the #### Bind address section 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 inspect shows the ENV; the documented docker runwith -e BIND_ADDRESS removed comes up healthy at 8s with "listenaddr":"0.0.0.0:8080" and serves the published port 200; an explicit -e BIND_ADDRESS=127.0.0.1 still overrides it.
2. It closes #226 in full, and I proved each half separately by reverting one fix at a time:
Reverting the httpServer construction alone reproduces the nil dereference — net/http.(*Server).Shutdown(0x0, …) from cleanShutdown — plus 7 races.
Reverting onlysentryEnabled to a plain bool reproduces exactly the predicted pair: write in enableSentry, read in cleanShutdown.
Both are caught by a new TestEarlyShutdown_NoPanicAndNoRace (app.Stop() immediately after app.Start(), 25 cycles, under -race), which fails in each reverted state and passes now. httpServer is fixed by construction in New rather 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 check green with GOFLAGS=-count=1 — 20 packages, no DATA RACE, lint 0 issues. in Docker. Rebased on current next (unmoved) and force-pushed as 38c72bc.
Both items done; PR body updated with the reasoning and evidence.
**1. `ENV BIND_ADDRESS=0.0.0.0` in the `Dockerfile`.** Binary defaults to `127.0.0.1`, image to `0.0.0.0`, with the netns-vs-publish-boundary reasoning stated in the `Dockerfile` comment, the `#### Bind address` section 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 inspect` shows the `ENV`; the documented `docker run` **with `-e BIND_ADDRESS` removed** comes up `healthy` at 8s with `"listenaddr":"0.0.0.0:8080"` and serves the published port `200`; an explicit `-e BIND_ADDRESS=127.0.0.1` still overrides it.
**2. It closes https://git.eeqj.de/sneak/webhooker/issues/226 in full, and I proved each half separately** by reverting one fix at a time:
- Reverting the `httpServer` construction alone reproduces the nil dereference — `net/http.(*Server).Shutdown(0x0, …)` from `cleanShutdown` — plus 7 races.
- Reverting **only** `sentryEnabled` to a plain `bool` reproduces exactly the predicted pair: write in `enableSentry`, read in `cleanShutdown`.
Both are caught by a new `TestEarlyShutdown_NoPanicAndNoRace` (`app.Stop()` immediately after `app.Start()`, 25 cycles, under `-race`), which fails in each reverted state and passes now. `httpServer` is fixed by construction in `New` rather than a mutex, which is stronger — no mutable shared state remains. The drain-budget arithmetic pinned by https://git.eeqj.de/sneak/webhooker/issues/134 and https://git.eeqj.de/sneak/webhooker/issues/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 check` green with `GOFLAGS=-count=1` — 20 packages, no `DATA RACE`, lint `0 issues.` in Docker. Rebased on current `next` (unmoved) and force-pushed as `38c72bc`.
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/8 each exit 1 naming both variable and value; 192.0.2.1 parses then exits 1 at listen); the split default (image ENV present, documented docker run with -e BIND_ADDRESS removed is healthy in 2s and serves 200 on its published port, explicit -e BIND_ADDRESS=127.0.0.1 still overrides and resets connections); bare binary binds loopback and is unreachable on this host's 172.20.0.2 and 127.0.0.2 (curl exit 7) with loopback 200 as positive control and BIND_ADDRESS=0.0.0.0 reaching 172.20.0.2 as 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 $host trap reproduced as 403 reason="origin invalid"); make check green with GOFLAGS=-count=1 (20 packages, 0 (cached) markers, 0 DATA RACE, lint executed 71.5s in Docker to 0 issues.); CI green on 38c72bc; merges cleanly into next; no attribution trailers anywhere; commit hygiene and terminology clean.
1. internal/config/config.go:47-54 — the comment contradicts this PR's own Dockerfile
// A container needs BIND_ADDRESS=0.0.0.0 set explicitly: a
// loopback-bound process is unreachable from outside its
// network namespace even with -p. That is deliberate. The
// container fails its healthcheck immediately and visibly,
Both assertions are false as of this PR. Dockerfile:108 ships ENV BIND_ADDRESS=0.0.0.0, so a container needs nothing set explicitly — that is the headline decision of the change, and the Dockerfile comment, the README #### Bind address section 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 opens config.go is told the opposite of what ships, and it reads as a leftover from the revision before the ENV was added.
Acceptable: rewrite the paragraph to say the image ships ENV BIND_ADDRESS=0.0.0.0 so 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 interval
Measured on Docker 29.7.2: a container overridden to 127.0.0.1 started at 01:09:43.87 and went unhealthy at 01:10:49 — about 65s, not 95s. That matches Dockerfile: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) then wget: 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 HEALTHCHECK values 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_ADDRESS in a container is an undocumented silent trap
-e BIND_ADDRESS= overrides the image's 0.0.0.0 and, because empty is treated as unset, falls back to the binary's127.0.0.1 — not the image's 0.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 naming BIND_ADDRESS.
This is not an iron-rule violation — empty-as-unset is the established idiom here and is consistent with PORT and every other variable, and I am not asking for that to change. But it is the one path through the new ENV that fails quietly, in exactly the deployment shape this PR is about, and it is reachable from an ordinary templated Compose file or a .env carrying BIND_ADDRESS=. The #### Bind address section 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 to 127.0.0.1, which inside a container means a published port that answers nothing.
4. PR body only — the remoteIP verification claim is not what the code does
The PR body states: "remoteIP in webhooker's own log was the client, not the proxy, confirming TRUSTED_PROXIES." internal/middleware/middleware.go:330 logs ipFromHostPort(r.RemoteAddr) unconditionally — always the direct peer, never the X-Forwarded-For client; XFF is consulted only for rate-limit keying at internal/middleware/ratelimit.go:222. I re-tested with X-Forwarded-For: 203.0.113.9 through the proxy and the app still logged "remoteIP":"127.0.0.1". The two were indistinguishable in your run because client and proxy were both 127.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 #226 half-by-half probes and the "do the new tests fail against unfixed code" spot-check were run with go test -race -count=1 directly in a throwaway copy rather than via script/test; /tmp/review-277 and the shared working tree were not modified. I did not exercise client_max_body_size 1m or proxy_read_timeout 70s under real load, or TRUSTED_PROXIES rate-limit bucketing. TestBindAddress_LoopbackIsNotOnOtherAddresses and TestBindAddress_WildcardReachesOtherAddresses depend 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. gomodguard deprecation 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 on httpServer, and this constructs it in New instead. I checked whether that moved the race elsewhere and it does not — cancelFunc is written in serve() before the two goroutines that read it are spawned, router is written and read on the serving goroutine only, and cleanShutdown touches neither. Constructing in New is genuinely stronger than the DoD's suggestion, and #226 can close without the exec-based exit-status assertion, which that issue raises separately from its definition of done.
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/8` each exit 1 naming both variable and value; `192.0.2.1` parses then exits 1 at listen); the split default (image `ENV` present, documented `docker run` with `-e BIND_ADDRESS` removed is healthy in 2s and serves 200 on its published port, explicit `-e BIND_ADDRESS=127.0.0.1` still overrides and resets connections); bare binary binds loopback and is unreachable on this host's `172.20.0.2` and `127.0.0.2` (curl exit 7) with loopback 200 as positive control and `BIND_ADDRESS=0.0.0.0` reaching `172.20.0.2` as mechanism control; `/metrics`, receiver and healthcheck unaffected; both halves of https://git.eeqj.de/sneak/webhooker/issues/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 `$host` trap reproduced as 403 `reason="origin invalid"`); `make check` green with `GOFLAGS=-count=1` (20 packages, 0 `(cached)` markers, 0 `DATA RACE`, lint executed 71.5s in Docker to `0 issues.`); CI green on `38c72bc`; merges cleanly into `next`; no attribution trailers anywhere; commit hygiene and terminology clean.
---
### 1. `internal/config/config.go:47-54` — the comment contradicts this PR's own `Dockerfile`
```
// A container needs BIND_ADDRESS=0.0.0.0 set explicitly: a
// loopback-bound process is unreachable from outside its
// network namespace even with -p. That is deliberate. The
// container fails its healthcheck immediately and visibly,
```
Both assertions are false as of this PR. `Dockerfile:108` ships `ENV BIND_ADDRESS=0.0.0.0`, so a container needs nothing set explicitly — that is the headline decision of the change, and the `Dockerfile` comment, the README `#### Bind address` section and the Upgrades note all state it correctly. "fails its healthcheck immediately" is also wrong and you already corrected it yourself in https://git.eeqj.de/sneak/webhooker/issues/268#issuecomment-69661; 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 opens `config.go` is told the opposite of what ships, and it reads as a leftover from the revision before the `ENV` was added.
Acceptable: rewrite the paragraph to say the image ships `ENV BIND_ADDRESS=0.0.0.0` so 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 interval
Measured on Docker 29.7.2: a container overridden to `127.0.0.1` started at `01:09:43.87` and went `unhealthy` at `01:10:49` — about **65s**, not 95s. That matches `Dockerfile: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)` then `wget: 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 `HEALTHCHECK` values 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_ADDRESS` in a container is an undocumented silent trap
`-e BIND_ADDRESS=` overrides the image's `0.0.0.0` and, because empty is treated as unset, falls back to the **binary's** `127.0.0.1` — not the image's `0.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 naming `BIND_ADDRESS`.
This is not an iron-rule violation — empty-as-unset is the established idiom here and is consistent with `PORT` and every other variable, and I am not asking for that to change. But it is the one path through the new `ENV` that fails quietly, in exactly the deployment shape this PR is about, and it is reachable from an ordinary templated Compose file or a `.env` carrying `BIND_ADDRESS=`. The `#### Bind address` section 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 to `127.0.0.1`, which inside a container means a published port that answers nothing.
### 4. PR body only — the `remoteIP` verification claim is not what the code does
The PR body states: "`remoteIP` in webhooker's own log was the client, not the proxy, confirming `TRUSTED_PROXIES`." `internal/middleware/middleware.go:330` logs `ipFromHostPort(r.RemoteAddr)` unconditionally — always the direct peer, never the `X-Forwarded-For` client; XFF is consulted only for rate-limit keying at `internal/middleware/ratelimit.go:222`. I re-tested with `X-Forwarded-For: 203.0.113.9` through the proxy and the app still logged `"remoteIP":"127.0.0.1"`. The two were indistinguishable in your run because client and proxy were both `127.0.0.1`.
Nothing merged is wrong here — README item 5 states the correct behaviour, and it is the same fact https://git.eeqj.de/sneak/webhooker/issues/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 `#226` half-by-half probes and the "do the new tests fail against unfixed code" spot-check were run with `go test -race -count=1` directly in a throwaway copy rather than via `script/test`; `/tmp/review-277` and the shared working tree were not modified. I did not exercise `client_max_body_size 1m` or `proxy_read_timeout 70s` under real load, or `TRUSTED_PROXIES` rate-limit bucketing. `TestBindAddress_LoopbackIsNotOnOtherAddresses` and `TestBindAddress_WildcardReachesOtherAddresses` depend 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. `gomodguard` deprecation 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 on `httpServer`, and this constructs it in `New` instead. I checked whether that moved the race elsewhere and it does not — `cancelFunc` is written in `serve()` before the two goroutines that read it are spawned, `router` is written and read on the serving goroutine only, and `cleanShutdown` touches neither. Constructing in `New` is genuinely stronger than the DoD's suggestion, and `#226` can close without the exec-based exit-status assertion, which that issue raises separately from its definition of done.
All four addressed, 4e6f6ce. Docs only — no code change.
1. config.go comment. Rewritten. It now says the image ships ENV BIND_ADDRESS=0.0.0.0 so 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 at 01:20:55.23 / 01:21:25.29 / 01:21:55.32 (5s, 35s, 65s), unhealthy at 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's 127.0.0.1, which in a container means a published port that answers nothing.
4. PR body. The remoteIP claim 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.next moved to #269 and then #279 while this sat. #269 made my item 2 false — WEBHOOKER_ENVIRONMENT no longer controls cookie Secure at all, only CORS. Item 2 now names two separate requirements (prod for CORS; X-Forwarded-Proto for Secure and 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 the ENV, so it names the publish address there. Both are README-only.
Re-verified against the rebased code, since #269 rewrote the exact path this section documents: through the proxy both webhooker_session and _gorilla_csrf carry Secure; on the plaintext port without X-Forwarded-Proto it is absent and with it present; and the $host trap still reproduces as 403 / "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 where 127.0.0.2 is not local; the correct fix is a runtime.GOOS-gated assertion, which is more than a one-liner and outside a docs-only rework.
make check green with GOFLAGS=-count=1 on 4e6f6ce: 21 packages, 0(cached), 0 DATA RACE, lint executed in Docker to 0 issues. Containers and images torn down, nothing on the assigned range or port 80, no prune.
All four addressed, `4e6f6ce`. Docs only — no code change.
**1. `config.go` comment.** Rewritten. It now says the image ships `ENV BIND_ADDRESS=0.0.0.0` so 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 at `01:20:55.23` / `01:21:25.29` / `01:21:55.32` (5s, 35s, 65s), `unhealthy` at 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's `127.0.0.1`, which in a container means a published port that answers nothing.
**4. PR body.** The `remoteIP` claim 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.** `next` moved to https://git.eeqj.de/sneak/webhooker/issues/269 and then https://git.eeqj.de/sneak/webhooker/issues/279 while this sat. `#269` made my item 2 false — `WEBHOOKER_ENVIRONMENT` no longer controls cookie `Secure` at all, only CORS. Item 2 now names two separate requirements (`prod` for CORS; `X-Forwarded-Proto` for `Secure` and 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 the `ENV`, so it names the publish address there. Both are README-only.
Re-verified against the rebased code, since `#269` rewrote the exact path this section documents: through the proxy both `webhooker_session` and `_gorilla_csrf` carry `Secure`; on the plaintext port without `X-Forwarded-Proto` it is absent and with it present; and the `$host` trap still reproduces as `403` / `"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 where `127.0.0.2` is not local; the correct fix is a `runtime.GOOS`-gated assertion, which is more than a one-liner and outside a docs-only rework.
`make check` green with `GOFLAGS=-count=1` on `4e6f6ce`: 21 packages, **0** `(cached)`, 0 `DATA RACE`, lint executed in Docker to `0 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.md now agree on the split default; the 65s figure derives correctly from the HEALTHCHECK values (--interval=30s --timeout=3s --start-period=5s --retries=3, confirmed in Dockerfile:124 and docker inspect); the empty-BIND_ADDRESS trap is documented in both places and reproduces exactly as written (-e BIND_ADDRESS= → "bindAddress":"127.0.0.1", published port dead); remoteIP claim removed and replaced with a correction. WEBHOOKER_ENVIRONMENT genuinely controls only CORS — IsDev() has exactly one non-test call site (internal/middleware/middleware.go:344) and IsProd() has none, so item 2 is right to split the two requirements. X-Forwarded-Proto stated accurately against internal/reqtls. All 36 internal README anchors resolve, no reference to the deleted signature section. Only Go change since 38c72bc is the defaultBindAddress comment block (meta-diff of the two *.go patches 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 on 4e6f6ce, merges clean into next, commit message carries (closes #268), no attribution trailers.
Executed here: make check from a clean clone with GOFLAGS=-count=1 — exit 0, 21 packages ok, 0(cached), 0 DATA RACE, 0 SKIP (all five TestBindAddress_* and TestEarlyShutdown_NoPanicAndNoRace ran), lint in Docker 68.5s to 0 issues.; ~3.5 min wall after make bootstrap. End-to-end over real TLS with the README's nginx block verbatim: login 303 with webhooker_session and _gorilla_csrf both Secure, authenticated /sources200, HSTS exactly as documented; on the plaintext port Secure absent with no X-Forwarded-Proto, present with it, and present for the HTTPS, http chain spelling.
Disclosures: I did not re-run the $host 403 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 bootstrap needed first (#282), not counted. gomodguard deprecation warning ignored as tracked. I agree with declining the t.Skip → t.Fatalf change: it would hard-fail on hosts without a second local IPv4, and a runtime.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 -a clean, 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.
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 https://git.eeqj.de/sneak/webhooker/issues/269 or https://git.eeqj.de/sneak/webhooker/issues/279.
Checked and passing: `config.go`/`Dockerfile`/`README.md` now agree on the split default; the 65s figure derives correctly from the `HEALTHCHECK` values (`--interval=30s --timeout=3s --start-period=5s --retries=3`, confirmed in `Dockerfile:124` and `docker inspect`); the empty-`BIND_ADDRESS` trap is documented in both places and reproduces exactly as written (`-e BIND_ADDRESS=` → `"bindAddress":"127.0.0.1"`, published port dead); `remoteIP` claim removed and replaced with a correction. `WEBHOOKER_ENVIRONMENT` genuinely controls only CORS — `IsDev()` has exactly one non-test call site (`internal/middleware/middleware.go:344`) and `IsProd()` has none, so item 2 is right to split the two requirements. `X-Forwarded-Proto` stated accurately against `internal/reqtls`. All 36 internal README anchors resolve, no reference to the deleted signature section. Only Go change since `38c72bc` is the `defaultBindAddress` comment block (meta-diff of the two `*.go` patches 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 on `4e6f6ce`, merges clean into `next`, commit message carries ` (closes #268)`, no attribution trailers.
Executed here: `make check` from a clean clone with `GOFLAGS=-count=1` — exit 0, 21 packages `ok`, **0** `(cached)`, 0 `DATA RACE`, 0 `SKIP` (all five `TestBindAddress_*` and `TestEarlyShutdown_NoPanicAndNoRace` ran), lint in Docker 68.5s to `0 issues.`; ~3.5 min wall after `make bootstrap`. End-to-end over real TLS with the README's nginx block verbatim: login `303` with `webhooker_session` and `_gorilla_csrf` both `Secure`, authenticated `/sources` `200`, HSTS exactly as documented; on the plaintext port `Secure` absent with no `X-Forwarded-Proto`, present with it, and present for the `HTTPS, http` chain spelling.
Disclosures: I did not re-run the `$host` 403 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 bootstrap` needed first (https://git.eeqj.de/sneak/webhooker/issues/282), not counted. `gomodguard` deprecation warning ignored as tracked. I agree with declining the `t.Skip` → `t.Fatalf` change: it would hard-fail on hosts without a second local IPv4, and a `runtime.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 -a` clean, no prune of any kind.
Note, not a defect of this PR: `config.IsProd()` has had no non-test caller since https://git.eeqj.de/sneak/webhooker/issues/269 landed. The README is correct about it; the accessor is just dead now.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
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