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

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. The binary defaults to
127.0.0.1, which is the safe answer for a bare host: reaching
webhooker from elsewhere becomes a deliberate act. The image sets
0.0.0.0, which is the correct answer inside a container, where the
network namespace is already the boundary and exposure is decided by
the publish flag instead — so `-p 127.0.0.1:8080:8080` is what the
README shows. Existing container deployments are unaffected. 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, and sentryEnabled is atomic. Both fields were written by
the serving goroutine and read by the fx stop hook with nothing
ordering them, and the OnStart hook returns before that goroutine has
necessarily run: cleanShutdown could dereference a nil httpServer on
an early SIGTERM, and both reads raced. No test started and stopped
the server, so nothing observed it.

Closes #226.

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.

Dockerfile

@@ -95,6 +95,18 @@ USER webhooker
 
 EXPOSE 8080
 
+# The binary defaults BIND_ADDRESS to 127.0.0.1, which is right for a
+# bare host: the cleartext listener serves the admin UI and the
+# unauthenticated receiver, so it must not appear on every interface
+# of a machine that configured nothing. A container is the other case.
+# Its network namespace is already the isolation boundary, so binding
+# every address inside it exposes nothing; what decides exposure is
+# the publish flag, and `-p 127.0.0.1:8080:8080` is the operator's
+# control there. Shipping the image on loopback would buy no security
+# and would make the process unreachable through its own published
+# port.
+ENV BIND_ADDRESS=0.0.0.0
+
 HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \
     CMD wget --no-verbose --tries=1 --spider http://localhost:8080/.well-known/healthcheck || exit 1
 

README.md

@@ -104,7 +104,7 @@ TTY detection, and security headers are always applied.
 | ----------------------- | ----------------------------------- | -------- |
 | `WEBHOOKER_ENVIRONMENT` | `dev` or `prod`                     | `dev`    |
 | `PORT`                  | HTTP listen port                    | `8080`   |
-| `BIND_ADDRESS`          | IP address the HTTP listener binds. Loopback by default, so the cleartext listener is not published on every interface. Containers must set `0.0.0.0`. See [Bind address](#bind-address) | `127.0.0.1` |
+| `BIND_ADDRESS`          | IP address the HTTP listener binds. Loopback by default, so the cleartext listener is not published on every interface. The Docker image ships `0.0.0.0` instead. See [Bind address](#bind-address) | `127.0.0.1` (image: `0.0.0.0`) |
 | `DATA_DIR`              | Directory for all SQLite databases  | `/var/lib/webhooker` |
 | `DEBUG`                 | Enable debug logging                | `false`  |
 | `MAINTENANCE_MODE`      | Report `maintenanceMode: true` in the healthcheck JSON. It does not change how any request is served — no maintenance page exists | `false`  |
@@ -228,9 +228,10 @@ the log of any deployment that has one.
 
 #### Bind address
 
-`BIND_ADDRESS` is the IP address the HTTP listener binds. It defaults
-to `127.0.0.1`, so out of the box webhooker is reachable only from the
-host it runs on.
+`BIND_ADDRESS` is the IP address the HTTP listener binds. The binary
+defaults to `127.0.0.1`, so a bare webhooker is reachable only from the
+host it runs on. The Docker image ships `ENV BIND_ADDRESS=0.0.0.0`
+instead — see below for why the two differ.
 
 That listener speaks **cleartext**, and it serves both the admin UI and
 the unauthenticated webhook receiver. webhooker terminates no TLS
@@ -244,20 +245,23 @@ clear, on a port nobody chose to publish. Reaching webhooker from
 another host is therefore something you configure, not something you
 get by default.
 
-**A container must set `BIND_ADDRESS=0.0.0.0`.** A process bound to
-loopback inside a container is unreachable from outside its network
-namespace even with `-p`, because the published port maps to the
-container's external address and nothing is listening there. The
-container's namespace is its own boundary, and `-p` is the exposure
-decision. The `docker run` command in
-[Running with Docker](#running-with-docker) sets it.
+**In a container the answer is `0.0.0.0`, which is why the image ships
+that.** A container's network namespace is already the boundary the
+loopback default is reaching for: nothing outside the container gets to
+`0.0.0.0:8080` because of the namespace, whatever the process bound.
+Exposure is decided at the publish flag instead — `-p
+127.0.0.1:8080:8080` rather than `-p 8080:8080` — which is the
+operator's to choose and is what
+[Running with Docker](#running-with-docker) shows. A loopback bind
+inside a container buys nothing and makes the process unreachable
+through its own published port.
 
 The value must be an IP address literal:
 
-- `127.0.0.1` — loopback only (the default). Use this with a reverse
-  proxy on the same host.
-- `0.0.0.0` — every IPv4 address. Required in a container; on a bare
-  host, only with a firewall in front of the port.
+- `127.0.0.1` — loopback only (the binary's default). Use this with a
+  reverse proxy on the same host.
+- `0.0.0.0` — every IPv4 address. The image's default; on a bare host,
+  only behind a firewall on the port.
 - `::` — every address, IPv6 and (on Linux, with the default
   `net.ipv6.bindv6only=0`) IPv4 as well.
 - A specific address such as `10.0.0.5` — that interface only.
@@ -591,24 +595,33 @@ docker run -d \
   webhooker:latest
 ```
 
-`BIND_ADDRESS=0.0.0.0` is **required** in a container and is not the
-default. webhooker binds loopback unless told otherwise (see
-[Bind address](#bind-address)), and a loopback-bound process inside a
-container is unreachable from outside its network namespace even with
-`-p`: the port is published and nothing answers on it. The container's
-health check fails too — it requests `http://localhost:8080`, and
-`localhost` resolves to `::1` first, which a `127.0.0.1` bind is not
-listening on — so the container goes `unhealthy` about 95 seconds
-after start. A container that is `unhealthy` with `connection refused`
-in its health log, or a published port that resets connections, is
-this.
+**The image and the bare binary default `BIND_ADDRESS` differently, on
+purpose.** The binary defaults to `127.0.0.1`; the image ships
+`ENV BIND_ADDRESS=0.0.0.0`, so the `-e BIND_ADDRESS=0.0.0.0` above is
+belt-and-braces and the command works without it.
 
-Publishing to `127.0.0.1:8080` rather than `8080` keeps Docker from
-opening the cleartext port on every interface of the host, which is
-what a bare `-p 8080:8080` does — including through firewall rules,
-since Docker's forwarding rules are inserted ahead of most host
-firewalls. Bind it to the host address your reverse proxy connects
-from, and nothing wider.
+The two cases are not the same question. On a bare host, `0.0.0.0`
+puts the cleartext admin UI and the unauthenticated receiver on every
+interface of the machine, which is what the loopback default exists to
+prevent. 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. What decides exposure there is the
+**publish flag**, and that is the line to get right.
+
+So publish to `127.0.0.1:8080` rather than `8080`. A bare
+`-p 8080:8080` opens the port on every interface of the host — through
+firewall rules too, since Docker's forwarding rules are inserted ahead
+of most host firewalls. Publish to the host address your reverse proxy
+connects from, and nothing wider.
+
+If you override `BIND_ADDRESS` to a loopback address in a container,
+the container is unreachable from outside its namespace even with
+`-p`: the published port answers nothing, and the health check fails
+as well — it requests `http://localhost:8080`, `localhost` resolves to
+`::1` first, and a `127.0.0.1` bind is not listening there, so the
+container goes `unhealthy` about 95 seconds after start. A container
+`unhealthy` with `connection refused` in its health log, or a
+published port that resets connections, is this.
 
 The container runs as a non-root user (`webhooker`, UID 1000), exposes
 port 8080, and includes a health check against
@@ -886,13 +899,15 @@ Upgrade procedure:
    traffic back on it.
 
 **Upgrading past the introduction of `BIND_ADDRESS`:** earlier versions
-always bound every interface. The listener now binds `127.0.0.1` unless
-`BIND_ADDRESS` says otherwise, so a deployment that relied on the old
-behaviour becomes unreachable from other hosts until it sets one. In a
-container that means `BIND_ADDRESS=0.0.0.0`; without it the container
-also goes `unhealthy`, since its health check reaches the app over
-`localhost`, which resolves to `::1` before `127.0.0.1`. On a bare
-host, set the address the proxy connects to. See
+always bound every interface. **Container deployments are unaffected**
+— the image ships `ENV BIND_ADDRESS=0.0.0.0`, so a `docker run` or
+Compose service that worked before still works with nothing changed.
+
+A **bare binary** is the case that changes: the listener now binds
+`127.0.0.1` unless `BIND_ADDRESS` says otherwise, so a deployment that
+relied on reaching it from another host becomes unreachable until it
+sets the address the proxy connects to. Check the `bindAddress` field
+of the startup log to see what a running process bound. See
 [Bind address](#bind-address).
 
 **Downgrade is unsupported.** Once a newer binary has migrated the files

internal/server/early_shutdown_test.go

@@ -0,0 +1,91 @@
+package server_test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"go.uber.org/fx"
+	"sneak.berlin/go/webhooker/internal/globals"
+	"sneak.berlin/go/webhooker/internal/server"
+)
+
+// earlyStopIterations is how many start/stop cycles the race test
+// runs. The window it aims at is the gap between the OnStart hook
+// returning and the serving goroutine reaching its first field
+// access, which is microseconds wide. The race detector reports an
+// unsynchronised pair whenever it observes one, but it has to observe
+// one, so a single cycle can miss purely on scheduling. Repetition
+// makes the observation reliable; the collaborators are built once,
+// so the cycles themselves are cheap.
+const earlyStopIterations = 25
+
+// TestEarlyShutdown_NoPanicAndNoRace stops the application
+// immediately after starting it, before the serving goroutine has
+// necessarily run at all.
+//
+// Two defects live in that window. The OnStart hook returns as soon
+// as it has spawned the serving goroutine, so fx runs the stop
+// sequence against a Server whose serving goroutine may not have
+// executed a single line. cleanShutdown called Shutdown on an
+// httpServer that goroutine was supposed to assign, which was a nil
+// dereference on an early SIGTERM; and it read httpServer and
+// sentryEnabled with nothing ordering those reads against the
+// goroutine's writes, which is a data race that only surfaces once
+// something both starts and stops the server. Nothing did before this
+// test: the listen-failure test never binds, and the router tests
+// bypass the lifecycle entirely.
+//
+// httpServer is now built in New, on the constructing goroutine, so
+// it is written before any hook exists and can never be nil.
+// sentryEnabled is atomic. This test is what catches either one
+// coming back — under -race, which is how the suite runs.
+func TestEarlyShutdown_NoPanicAndNoRace(t *testing.T) {
+	t.Parallel()
+
+	// Built once: the collaborators are not what is under test, and
+	// standing up a database per iteration would make repetition too
+	// expensive to be worth having.
+	env := newTestEnv(t)
+	env.cfg.BindAddress = loopbackV4
+
+	for range earlyStopIterations {
+		requireStartStopIsClean(t, env)
+	}
+}
+
+// requireStartStopIsClean runs one start/stop cycle with no wait in
+// between, failing the test if either half errors.
+//
+// Each cycle gets a fresh fx app, so the Server under test is
+// constructed anew every time — that construction is where the
+// httpServer write now happens, and reusing one Server would test it
+// only once.
+func requireStartStopIsClean(t *testing.T, env *testEnv) {
+	t.Helper()
+
+	env.cfg.Port = freePort(t)
+
+	app := fx.New(
+		fx.NopLogger,
+		fx.Supply(env.log, env.cfg, env.mw, env.hnd),
+		fx.Provide(globals.New, server.New),
+		fx.Invoke(func(*server.Server) {}),
+	)
+
+	startCtx, cancelStart := context.WithTimeout(
+		context.Background(), lifecycleTimeout,
+	)
+	defer cancelStart()
+
+	require.NoError(t, app.Start(startCtx))
+
+	// No sleep and no readiness wait: stopping while the serving
+	// goroutine is still in flight is the whole point.
+	stopCtx, cancelStop := context.WithTimeout(
+		context.Background(), lifecycleTimeout,
+	)
+	defer cancelStop()
+
+	require.NoError(t, app.Stop(stopCtx))
+}

internal/server/export_test.go

@@ -90,12 +90,12 @@ func NewRouterWithProbeForTest(
 	probe http.HandlerFunc,
 ) http.Handler {
 	s := &Server{
-		log:           log,
-		mw:            mw,
-		h:             h,
-		params:        ServerParams{Config: cfg},
-		sentryEnabled: sentryEnabled,
+		log:    log,
+		mw:     mw,
+		h:      h,
+		params: ServerParams{Config: cfg},
 	}
+	s.sentryEnabled.Store(sentryEnabled)
 	s.SetupRoutes()
 	s.router.Handle(ProbePattern, probe)
 

internal/server/routes.go

@@ -81,7 +81,7 @@ func (s *Server) setupGlobalMiddleware() {
 	// Sentry error reporting (if SENTRY_DSN is set). Repanic is
 	// true so panics still bubble up to the Recoverer middleware
 	// registered immediately above.
-	if s.sentryEnabled {
+	if s.sentryEnabled.Load() {
 		sentryHandler := sentryhttp.New(sentryhttp.Options{
 			Repanic: true,
 		})

internal/server/server.go

@@ -9,6 +9,7 @@ import (
 	"net/http"
 	"os"
 	"os/signal"
+	"sync/atomic"
 	"syscall"
 	"time"
 
@@ -88,8 +89,15 @@ type ServerParams struct {
 // Server is the main HTTP server that wires up routes and manages
 // graceful shutdown.
 type Server struct {
-	startupTime   time.Time
-	sentryEnabled bool
+	startupTime time.Time
+
+	// sentryEnabled is written by the serving goroutine, in
+	// enableSentry, and read by the fx stop hook in cleanShutdown.
+	// Nothing orders those two: the OnStart hook returns as soon as
+	// the goroutine is spawned, so a stop can be running while
+	// enableSentry is still deciding. It is atomic to supply the
+	// edge the goroutines do not.
+	sentryEnabled atomic.Bool
 	log           *slog.Logger
 	cancelFunc    context.CancelFunc
 	httpServer    *http.Server
@@ -143,7 +151,7 @@ func (s *Server) MaintenanceMode() bool {
 }
 
 func (s *Server) enableSentry() {
-	s.sentryEnabled = false
+	s.sentryEnabled.Store(false)
 
 	if s.params.Config.SentryDSN == "" {
 		return
@@ -164,7 +172,7 @@ func (s *Server) enableSentry() {
 	}
 
 	s.log.Info("sentry error reporting activated")
-	s.sentryEnabled = true
+	s.sentryEnabled.Store(true)
 }
 
 // serve installs the signal watcher, starts the listener and blocks
@@ -243,7 +251,7 @@ func (s *Server) cleanShutdown(ctx context.Context) {
 
 	s.cleanupForExit()
 
-	if s.sentryEnabled {
+	if s.sentryEnabled.Load() {
 		s.flushSentry(ctx)
 	}
 }