Bind the app port deliberately and document the proxy deployment (closes #268) #277

Merged
clawbot merged 1 commits from issue-268-bind-address-proxy-docs into next 2026-08-24 03:38:44 +02:00
Collaborator

Closes #268 and #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 #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 #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 #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/login200, HTTP/2, strict-transport-security: max-age=63072000; includeSubDomains; preload.
  • Login POST303 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 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 #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.
clawbot added 1 commit 2026-08-24 02:45:09 +02:00
Bind the app port deliberately and document the proxy deployment (closes #268)
All checks were successful
check / check (push) Successful in 3m1s
eb6ef01742
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.
clawbot added the needs-review label 2026-08-24 02:45:14 +02:00
clawbot self-assigned this 2026-08-24 02:45:15 +02:00
clawbot force-pushed issue-268-bind-address-proxy-docs from eb6ef01742 to 38c72bcbcc 2026-08-24 02:57:57 +02:00 Compare
Author
Collaborator

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 #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 #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`.
Author
Collaborator

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'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 #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.
clawbot added needs-rework and removed needs-review labels 2026-08-24 03:16:00 +02:00
clawbot force-pushed issue-268-bind-address-proxy-docs from 38c72bcbcc to 4e6f6ce1d3 2026-08-24 03:25:55 +02:00 Compare
Author
Collaborator

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.
clawbot added needs-review and removed needs-rework labels 2026-08-24 03:28:35 +02:00
Author
Collaborator

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 /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 (#282), not counted. gomodguard deprecation warning ignored as tracked. I agree with declining the t.Skipt.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.
clawbot merged commit 62576f6fc6 into next 2026-08-24 03:38:44 +02:00
clawbot deleted branch issue-268-bind-address-proxy-docs 2026-08-24 03:38:44 +02:00
Sign in to join this conversation.
No Reviewers
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#277