Add an egress CIDR allowlist to the SSRF guard (closes #204)

The SSRF blocklist had no escape hatch, so the thing webhooker is
mostly for — taking a public webhook and forwarding it to something
on your own network — could not be configured at all. Every private
address, Docker sibling and loopback service was permanently
unreachable as a delivery destination.

ALLOWED_EGRESS_CIDRS (default empty) names blocks that delivery
targets may reach despite the default blocklist. It is an allowlist
and only ever adds destinations: there is no boolean, and no value
disables SSRF protection wholesale. Empty, the guard behaves exactly
as before.

Link-local (169.254.0.0/16, fe80::/10) is refused before the
allowlist is consulted, so no supplied CIDR can open it — not the
exact address, not a supernet, not 0.0.0.0/0. Reaching cloud
instance metadata is credential theft rather than delivery to an
internal service.

The policy now lives in one function, Guard.checkIP, which both
target-creation validation and the delivery dialer call. The two
paths previously decided separately, which is how they came to
disagree about a destination. The guard is built once from config
and injected via fx into both the handlers and the delivery engine,
so there is a single instance and a single answer.

A set-but-unparseable value aborts startup naming the variable,
reusing the existing envPrefixList parser. A non-empty list is
logged at startup with the blocks spelled out, not counted, so the
hole is visible in the log of any deployment that has one.

Tests: an allowlisted loopback CIDR both validates and delivers to a
live server (and the same URL still fails without the allowlist); a
private address outside the listed block stays refused on both
paths; metadata stays refused under six different covering CIDRs;
public addresses are unaffected either way; and config coverage for
parsing, startup abort, and the warning's contents.

README.md

@@ -107,8 +107,8 @@ TTY detection, and security headers are always applied.
 | `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`  |
-| `METRICS_USERNAME`      | Basic auth username for `/metrics`. Must be set together with `METRICS_PASSWORD`; one without the other fails startup | `""`     |
+| `METRICS_USERNAME`      | Basic auth username for `/metrics`  | `""`     |
-| `METRICS_PASSWORD`      | Basic auth password for `/metrics`. Must be set together with `METRICS_USERNAME`; one without the other fails startup | `""`     |
+| `METRICS_PASSWORD`      | Basic auth password for `/metrics`  | `""`     |
 | `SENTRY_DSN`            | Sentry error reporting DSN          | `""`     |
 | `RETENTION_SWEEP_INTERVAL` | How often the retention reaper and archive sweeper run (Go duration, must be positive) | `1h` |
 | `SESSION_IDLE_TIMEOUT`  | Idle session timeout (Go duration)  | `24h`    |
@@ -147,41 +147,17 @@ cover the destinations you actually deliver to — prefer
 `10.1.2.3/32` over `10.0.0.0/8` — and never list a block wider than the
 network you are willing to expose.
 
-Listing `0.0.0.0/0` or `::/0` opens **every** other private and
-reserved range at once — loopback, RFC 1918, CGNAT, ULA, the lot. It is
-a functional off switch for everything except the addresses listed as
-unconditionally blocked below, and it makes any delivery target a probe
-into your entire network and this host's own loopback services. Do not
-list it.
-
 Two things this setting cannot do:
 
 - **It cannot turn the guard off.** There is no boolean, and no value
   that disables SSRF protection wholesale. The guard is always on and
   the list is always an allowlist; an empty list (the default) means
-  every private and reserved range stays refused. Note that
+  every private and reserved range stays refused.
-  `0.0.0.0/0` gets you most of the way there anyway, per above.
+- **It cannot open link-local.** `169.254.0.0/16` and `fe80::/10` stay
-- **It cannot open link-local or a known cloud metadata endpoint.**
+  blocked no matter what you list, including when you list them
-  These stay blocked no matter what you list, including when you list
+  outright or list a supernet such as `0.0.0.0/0`. That range serves
-  them outright or list a supernet such as `0.0.0.0/0`, `::/0`,
+  cloud instance metadata (`169.254.169.254`), where reaching it is
-  `fd00::/8` or `100.64.0.0/10`:
+  credential theft rather than delivery to an internal service.
-
-  | Blocked unconditionally | What it is |
-  | ----------------------- | ---------- |
-  | `169.254.0.0/16` | IPv4 link-local, carrying `169.254.169.254` (AWS, GCP, Azure and others) |
-  | `fe80::/10` | IPv6 link-local |
-  | `fd00:ec2::254/128` | AWS IPv6 IMDS, a host route inside ULA space |
-  | `100.100.100.200/32` | Alibaba Cloud metadata, a host route inside CGNAT |
-  | `::a9fe:a9fe/128` | `169.254.169.254` as an IPv4-compatible IPv6 address |
-  | `64:ff9b::a9fe:a9fe/128` | `169.254.169.254` behind the NAT64 well-known prefix |
-
-  The IPv4-mapped form `::ffff:169.254.169.254` is covered by the
-  `169.254.0.0/16` entry. Reaching any of these is credential theft
-  rather than delivery to an internal service. The two host routes are
-  single addresses, so blocking them costs you nothing else on the ULA
-  or CGNAT networks around them. This list is not exhaustive of every
-  cloud's metadata address — if yours is not here, do not allowlist the
-  block that contains it.
 
 The list is applied at one place in the code, which both target
 creation and delivery consult, so a URL that the target form accepts is
@@ -195,18 +171,6 @@ A set but unparseable value aborts startup. When the list is non-empty
 webhooker logs it at startup, blocks and all, so the hole is visible in
 the log of any deployment that has one.
 
-#### Metrics credentials
-
-`METRICS_USERNAME` and `METRICS_PASSWORD` are set together or not at
-all. With both set, `/metrics` is served behind basic auth. With
-neither set, the route is not registered and returns 404. With one set
-and the other empty or unset, the process refuses to start and exits
-non-zero with an error naming both variables — mounting the endpoint
-on the username alone would publish it behind a password that is the
-empty string, and quietly withholding it would deny an endpoint that
-was asked for. The `hasMetricsAuth` field in the startup log and the
-existence of the route are the same value, so they cannot disagree.
-
 #### Trusted proxies
 
 `TRUSTED_PROXIES` is a comma-separated list of CIDR blocks (a bare
@@ -1775,7 +1739,7 @@ abuse limit later; they are tracked as future work.
 
 | Method | Path       | Description |
 | ------ | ---------- | ----------- |
-| `GET`  | `/metrics` | Prometheus metrics, behind basic auth. The route is registered only when `METRICS_USERNAME` and `METRICS_PASSWORD` are both set; with neither set it does not exist and returns 404, and with only one set the process refuses to start |
+| `GET`  | `/metrics` | Prometheus metrics, behind basic auth. The route is registered only when `METRICS_USERNAME` is set; otherwise it does not exist and returns 404 |
 
 #### API (Planned)
 
@@ -1930,8 +1894,7 @@ Applied to all routes in this order:
    Permissions-Policy)
 3. **Logging** — Structured request logging (method, URL, status,
    latency, remote IP, user agent, request ID)
-4. **Metrics** — Prometheus HTTP metrics (if `METRICS_USERNAME` and
+4. **Metrics** — Prometheus HTTP metrics (if `METRICS_USERNAME` is set)
-   `METRICS_PASSWORD` are both set)
 5. **CORS** — Cross-origin resource sharing headers
 6. **Timeout** — 60-second request timeout
 7. **Recoverer** — Panic recovery: one `ERROR` record through
@@ -1963,9 +1926,8 @@ being read and without reaching CSRF, the route group's remaining
 middleware, or the handler. It is not rejected before *any* other
 middleware, though: the global entries listed above all run first, so
 such a request is still logged and given the security headers — and
-counted in the metrics, on a deployment where the `/metrics`
+counted in the metrics, on a deployment where `METRICS_USERNAME` is
-credentials are set and the Metrics middleware is therefore registered
+set and the Metrics middleware is therefore registered at all. The
-at all. The
 rejection itself is logged at `WARN` with the method, path and
 declared length. A chunked request, or
 one that lies about its length, is hard-capped by
@@ -2019,9 +1981,8 @@ check, see [The login endpoint](#the-login-endpoint).
   route through a single decision function, so they cannot disagree
   about a destination. An operator can permit specific blocks with
   [`ALLOWED_EGRESS_CIDRS`](#allowing-egress-to-your-own-network); the
-  guard cannot be switched off, and link-local plus the known cloud
+  guard cannot be switched off, and link-local stays blocked whatever
-  metadata endpoints stay blocked whatever is listed — though listing
+  is listed
-  `0.0.0.0/0` or `::/0` does open every other private range
 - **Login limiting is inverted, deliberately.** The login `POST` has
   no pre-emptive rate limiter in front of it. Credentials are
   verified first and only a _failed_ attempt spends budget, so a

internal/config/config.go

@@ -71,16 +71,6 @@ var ErrInvalidPort = errors.New("invalid port")
 // nor a bare IP address.
 var ErrInvalidCIDR = errors.New("invalid CIDR")
 
-// ErrIncompleteMetricsAuth is returned when exactly one of
-// METRICS_USERNAME and METRICS_PASSWORD carries a value. Neither
-// fallback is acceptable: serving /metrics on the username alone
-// publishes an endpoint whose password is the empty string, and
-// silently leaving it unmounted withholds an endpoint the operator
-// asked for. Half-set is a configuration error, so startup fails.
-var ErrIncompleteMetricsAuth = errors.New(
-	"incomplete metrics credentials",
-)
-
 //nolint:revive // ConfigParams is a standard fx naming convention.
 type ConfigParams struct {
 	fx.In
@@ -149,21 +139,6 @@ func (c *Config) IsProd() bool {
 	return c.Environment == EnvironmentProd
 }
 
-// MetricsAuthEnabled reports whether /metrics is served behind basic
-// auth. It is the only answer to that question in the codebase: the
-// route mount, the Prometheus recording middleware and the startup
-// log's hasMetricsAuth field all read this one method, so the log
-// cannot report auth as off while the route is mounted.
-//
-// It requires both credentials rather than the username alone.
-// loadFromEnv already rejects a half-set pair, but a Config built in
-// code bypasses that, and the failure mode this guards is an endpoint
-// mounted with a credential map whose only password is the empty
-// string.
-func (c *Config) MetricsAuthEnabled() bool {
-	return c.MetricsUsername != "" && c.MetricsPassword != ""
-}
-
 // envString returns the value of the named environment variable,
 // or an empty string if not set.
 func envString(key string) string {
@@ -365,30 +340,6 @@ func envPrefixList(key string) ([]netip.Prefix, error) {
 	return prefixes, nil
 }
 
-// resolveMetricsAuth reads the /metrics basic-auth credentials and
-// rejects a half-set pair, naming both variables either way. The
-// error carries neither value: the password is a secret.
-func resolveMetricsAuth() (string, string, error) {
-	username := envString("METRICS_USERNAME")
-	password := envString("METRICS_PASSWORD")
-
-	if (username == "") == (password == "") {
-		return username, password, nil
-	}
-
-	set, empty := "METRICS_USERNAME", "METRICS_PASSWORD"
-	if username == "" {
-		set, empty = empty, set
-	}
-
-	return "", "", fmt.Errorf(
-		"%w: %s is set but %s is empty; METRICS_USERNAME and "+
-			"METRICS_PASSWORD must both be set to serve /metrics, "+
-			"or both be empty to leave it unmounted",
-		ErrIncompleteMetricsAuth, set, empty,
-	)
-}
-
 // resolveEnvironment reads WEBHOOKER_ENVIRONMENT, defaulting to
 // dev, and rejects unrecognised values.
 func resolveEnvironment() (string, error) {
@@ -471,18 +422,13 @@ func loadFromEnv() (*Config, error) {
 		return nil, err
 	}
 
-	metricsUsername, metricsPassword, err := resolveMetricsAuth()
-	if err != nil {
-		return nil, err
-	}
-
 	return &Config{
 		DataDir:                envString("DATA_DIR"),
 		Debug:                  debug,
 		MaintenanceMode:        maintenanceMode,
 		Environment:            environment,
-		MetricsUsername:        metricsUsername,
+		MetricsUsername:        envString("METRICS_USERNAME"),
-		MetricsPassword:        metricsPassword,
+		MetricsPassword:        envString("METRICS_PASSWORD"),
 		Port:                   port,
 		SentryDSN:              envString("SENTRY_DSN"),
 		RetentionSweepInterval: retentionSweepInterval,
@@ -622,7 +568,8 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) {
 		"trustedProxies", len(s.TrustedProxies),
 		"allowedEgressCIDRs", len(s.AllowedEgressCIDRs),
 		"hasSentryDSN", s.SentryDSN != "",
-		"hasMetricsAuth", s.MetricsAuthEnabled(),
+		"hasMetricsAuth",
+		s.MetricsUsername != "" && s.MetricsPassword != "",
 	)
 
 	s.warnSharedRateLimitBucket(log)

internal/config/config_test.go

@@ -26,12 +26,6 @@ const (
 	// cidrPrivateV4 is the sample trusted-proxy block the
 	// TRUSTED_PROXIES cases are built from.
 	cidrPrivateV4 = "10.0.0.0/8"
-
-	// metricsAuthValue is the sample METRICS_PASSWORD the metrics
-	// credential cases are built from. It is asserted absent from
-	// the startup error, so it must not be a substring of either
-	// variable name that error prints.
-	metricsAuthValue = "s3cret"
 )
 
 func TestEnvironmentConfig(t *testing.T) {
@@ -909,168 +903,3 @@ func TestSharedRateLimitBucketWarning(t *testing.T) {
 		})
 	}
 }
-
-// metricsEnv describes what one subtest below puts in the
-// environment for a single METRICS_ variable. A variable that is
-// set to the empty string and one that is not set at all are
-// distinct inputs here, because the reported bug arrived through
-// the first of them.
-type metricsEnv struct {
-	set   bool
-	value string
-}
-
-// unset leaves the variable out of the environment entirely.
-func unset() metricsEnv {
-	return metricsEnv{set: false, value: ""}
-}
-
-// setTo sets the variable, including to the empty string.
-func setTo(value string) metricsEnv {
-	return metricsEnv{set: true, value: value}
-}
-
-// metricsAuthCase is one row of the table in TestMetricsAuthConfig,
-// named so the table can live in its own function and keep the test
-// itself short.
-type metricsAuthCase struct {
-	name        string
-	username    metricsEnv
-	password    metricsEnv
-	expectError bool
-	expectAuth  bool
-}
-
-// metricsAuthCases enumerates every combination of the two
-// credentials, counting "set to the empty string" and "not set at
-// all" as separate inputs on each side.
-func metricsAuthCases() []metricsAuthCase {
-	return []metricsAuthCase{
-		{
-			name:     "both unset leaves metrics unmounted",
-			username: unset(),
-			password: unset(),
-		},
-		{
-			name:     "both empty leaves metrics unmounted",
-			username: setTo(""),
-			password: setTo(""),
-		},
-		{
-			name:       "both set enables metrics auth",
-			username:   setTo("metrics"),
-			password:   setTo(metricsAuthValue),
-			expectAuth: true,
-		},
-		{
-			name:        "username with unset password fails",
-			username:    setTo("metrics"),
-			password:    unset(),
-			expectError: true,
-		},
-		{
-			name:        "username with empty password fails",
-			username:    setTo("metrics"),
-			password:    setTo(""),
-			expectError: true,
-		},
-		{
-			name:        "password with unset username fails",
-			username:    unset(),
-			password:    setTo(metricsAuthValue),
-			expectError: true,
-		},
-		{
-			name:        "password with empty username fails",
-			username:    setTo(""),
-			password:    setTo(metricsAuthValue),
-			expectError: true,
-		},
-	}
-}
-
-// TestMetricsAuthConfig covers every combination of METRICS_USERNAME
-// and METRICS_PASSWORD. Either both carry a value, in which case
-// /metrics is served behind basic auth, or neither does, in which
-// case the route is never mounted. One without the other is a
-// startup error rather than a fallback: mounting on the username
-// alone published /metrics behind a credential map that accepted an
-// empty password, which is the defect this test exists to pin. See
-// https://git.eeqj.de/sneak/webhooker/issues/205.
-func TestMetricsAuthConfig(t *testing.T) {
-	for _, tt := range metricsAuthCases() {
-		t.Run(tt.name, func(t *testing.T) {
-			// Cannot use t.Parallel() here because t.Setenv
-			// is incompatible with parallel subtests.
-			if tt.username.set {
-				t.Setenv("METRICS_USERNAME", tt.username.value)
-			} else {
-				require.NoError(
-					t, os.Unsetenv("METRICS_USERNAME"),
-				)
-			}
-
-			if tt.password.set {
-				t.Setenv("METRICS_PASSWORD", tt.password.value)
-			} else {
-				require.NoError(
-					t, os.Unsetenv("METRICS_PASSWORD"),
-				)
-			}
-
-			if tt.expectError {
-				assertMetricsAuthRejected(t)
-
-				return
-			}
-
-			assertMetricsAuthAccepted(t, tt.expectAuth)
-		})
-	}
-}
-
-// assertMetricsAuthRejected requires that fx refused to build the
-// graph, that the failure is ErrIncompleteMetricsAuth, and that the
-// operator is told both variable names — the point of failing here
-// rather than degrading is that the message says what to fix.
-func assertMetricsAuthRejected(t *testing.T) {
-	t.Helper()
-
-	var cfg *config.Config
-
-	app := fx.New(
-		fx.NopLogger,
-		fx.Provide(globals.New, logger.New, config.New),
-		fx.Populate(&cfg),
-	)
-
-	err := app.Err()
-	require.Error(t, err)
-	require.ErrorIs(t, err, config.ErrIncompleteMetricsAuth)
-	assert.Contains(t, err.Error(), "METRICS_USERNAME")
-	assert.Contains(t, err.Error(), "METRICS_PASSWORD")
-	// The password is a secret and must not reach a startup error.
-	assert.NotContains(t, err.Error(), metricsAuthValue)
-}
-
-// assertMetricsAuthAccepted requires that startup succeeded and that
-// MetricsAuthEnabled — the single value the /metrics mount and the
-// startup log both read — reports what the environment asked for.
-func assertMetricsAuthAccepted(t *testing.T, expectAuth bool) {
-	t.Helper()
-
-	var cfg *config.Config
-
-	app := fxtest.New(
-		t,
-		fx.Provide(globals.New, logger.New, config.New),
-		fx.Populate(&cfg),
-	)
-	require.NoError(t, app.Err())
-
-	app.RequireStart()
-
-	defer app.RequireStop()
-
-	assert.Equal(t, expectAuth, cfg.MetricsAuthEnabled())
-}

internal/delivery/ssrf.go

@@ -28,9 +28,9 @@ var (
 	errBlockedIP = errors.New(
 		"blocked private/reserved IP range",
 	)
-	errBlockedMetadata = errors.New(
+	errBlockedLinkLocal = errors.New(
-		"blocked link-local or cloud instance metadata " +
+		"blocked link-local range, which serves cloud instance " +
-			"address: ALLOWED_EGRESS_CIDRS cannot open it",
+			"metadata: ALLOWED_EGRESS_CIDRS cannot open it",
 	)
 	errInvalidScheme = errors.New(
 		"only http and https are allowed",
@@ -46,16 +46,14 @@ var (
 var blockedNetworks []*net.IPNet
 
 // alwaysBlockedNetworks are the ranges no configuration can
-// open: the link-local blocks and the cloud instance metadata
+// open. They are the link-local blocks, which carry the cloud
-// endpoints that live outside them. Reaching one is credential
+// instance metadata services (169.254.169.254 and its IPv6
-// theft rather than delivery to an internal service, so a
+// equivalents). Reaching one is credential theft rather than
-// supplied CIDR that covers such an address still leaves it
+// delivery to an internal service, so a supplied CIDR that
-// blocked.
+// covers a link-local address still leaves it blocked.
 //
-// Some of these are also in blockedNetworks and this list is
+// These addresses are also in blockedNetworks; this list is what
-// what makes them unconditional; the metadata endpoints outside
+// makes them unconditional.
-// the link-local range are host routes, so blocking them costs
-// an operator nothing else on the surrounding network.
 //
 //nolint:gochecknoglobals // package-level network list is appropriate here
 var alwaysBlockedNetworks []*net.IPNet
@@ -82,34 +80,9 @@ func init() {
 		"fe80::/10",
 	})
 
-	// Every entry is named. The set must not grow or shrink
-	// without a matching change to
-	// TestAlwaysBlockedNetworks_PinnedSet.
-	//
-	// The IPv4-mapped form ::ffff:169.254.169.254 needs no
-	// entry: net.IPNet.Contains normalises it via To4() before
-	// comparing, so 169.254.0.0/16 already matches it. To4()
-	// does not normalise the IPv4-compatible or NAT64 forms,
-	// which is why those are listed separately.
 	alwaysBlockedNetworks = mustParseCIDRs([]string{
-		// IPv4 link-local, carrying the 169.254.169.254
-		// metadata service used by AWS, GCP, Azure and others.
 		"169.254.0.0/16",
-		// IPv6 link-local, its IPv6 counterpart.
 		"fe80::/10",
-		// AWS IPv6 IMDS. It sits in fc00::/7, so an operator
-		// allowlisting their own ULA block (fd00::/8 is an
-		// ordinary entry) would otherwise reopen it.
-		"fd00:ec2::254/128",
-		// Alibaba Cloud metadata. It sits in CGNAT
-		// 100.64.0.0/10, which Tailscale also uses, so an
-		// operator allowlisting a Tailscale peer's range would
-		// otherwise reopen it.
-		"100.100.100.200/32",
-		// 169.254.169.254 as an IPv4-compatible IPv6 address.
-		"::a9fe:a9fe/128",
-		// 169.254.169.254 behind the NAT64 well-known prefix.
-		"64:ff9b::a9fe:a9fe/128",
 	})
 }
 
@@ -248,16 +221,15 @@ func (g *Guard) allows(ip net.IP) bool {
 //
 // The order is the policy:
 //
-//  1. alwaysBlockedNetworks is refused before the allowlist is
+//  1. Link-local is refused before the allowlist is consulted,
-//     consulted, so no configured CIDR reaches link-local or a
+//     so no configured CIDR can reach cloud instance metadata.
-//     cloud instance metadata endpoint.
 //  2. The allowlist is consulted next, so a listed private
 //     network becomes reachable.
 //  3. Everything else keeps the default blocklist's answer.
 func (g *Guard) checkIP(ip net.IP) error {
 	if matchesAny(alwaysBlockedNetworks, ip) {
 		return fmt.Errorf(
-			"target IP %s: %w", ip, errBlockedMetadata,
+			"target IP %s: %w", ip, errBlockedLinkLocal,
 		)
 	}
 

internal/delivery/ssrf_allowlist_test.go

@@ -31,13 +31,6 @@ const (
 	// publicIP is an ordinary public address, which the guard
 	// permits with or without an allowlist.
 	publicIP = "93.184.216.34"
-
-	// allowAllIPv4 and allowAllIPv6 are the widest allowlist
-	// entries expressible: the whole internet, in each family.
-	// Nothing unconditionally blocked may be reachable under
-	// them.
-	allowAllIPv4 = "0.0.0.0/0"
-	allowAllIPv6 = "::/0"
 )
 
 // TestGuardAllowlist_PermittedCIDRDelivers proves the escape
@@ -152,57 +145,19 @@ func TestGuardAllowlist_UnlistedPrivateStillRefused(t *testing.T) {
 }
 
 // TestGuardAllowlist_MetadataAlwaysRefused is the load-bearing
-// case: cloud instance metadata endpoints are credential theft
+// case: link-local carries cloud instance metadata, so reaching
-// rather than delivery to an internal service, so no allowlist
+// it is credential theft rather than delivery to an internal
-// reaches one. Every guard below names a CIDR that covers its
+// service. Every one of these guards names a CIDR that covers
-// target — including 0.0.0.0/0, ::/0, and the ordinary ULA and
+// 169.254.169.254 — including 0.0.0.0/0 — and the address must
-// CGNAT blocks an operator would really list — and the address
+// stay refused anyway, on both paths.
-// must stay refused anyway, on both the validation and the
-// delivery path.
 func TestGuardAllowlist_MetadataAlwaysRefused(t *testing.T) {
 	t.Parallel()
 
-	for _, tt := range metadataAlwaysRefusedCases() {
+	tests := []struct {
-		t.Run(tt.name, func(t *testing.T) {
+		name   string
-			t.Parallel()
+		allow  string
-
+		target string
-			guard := delivery.NewTestGuard(
+	}{
-				netip.MustParsePrefix(tt.allow),
-			)
-
-			err := guard.ValidateTargetURL(
-				context.Background(), tt.target,
-			)
-			require.Error(t,
-				err,
-				"%s must stay blocked even though %s covers it",
-				tt.target, tt.allow,
-			)
-			assert.Contains(t,
-				err.Error(),
-				"ALLOWED_EGRESS_CIDRS cannot open it",
-				"the refusal must say why it cannot be opened",
-			)
-
-			assertDialRefused(t, guard, tt.target)
-		})
-	}
-}
-
-// metadataAlwaysRefusedCase is one (allowlist, target) pair that
-// must be refused: allow covers target, and target must stay
-// blocked regardless.
-type metadataAlwaysRefusedCase struct {
-	name   string
-	allow  string
-	target string
-}
-
-// metadataAlwaysRefusedCases enumerates every unconditionally
-// blocked address together with an allowlist entry that would
-// otherwise reach it.
-func metadataAlwaysRefusedCases() []metadataAlwaysRefusedCase {
-	return []metadataAlwaysRefusedCase{
 		{
 			name:   "exact metadata host",
 			allow:  "169.254.169.254/32",
@@ -220,54 +175,43 @@ func metadataAlwaysRefusedCases() []metadataAlwaysRefusedCase {
 		},
 		{
 			name:   "the entire IPv4 internet",
-			allow:  allowAllIPv4,
+			allow:  "0.0.0.0/0",
 			target: metadataURL,
 		},
 		{
 			name:   "other link-local address",
-			allow:  allowAllIPv4,
+			allow:  "0.0.0.0/0",
 			target: "http://169.254.1.1/",
 		},
 		{
 			name:   "IPv6 link-local",
-			allow:  allowAllIPv6,
+			allow:  "::/0",
 			target: "http://[fe80::1]/",
 		},
-		{
+	}
-			// fd00::/8 is an ordinary block for an operator to
+
-			// allowlist, and AWS's IPv6 IMDS sits inside it.
+	for _, tt := range tests {
-			name:   "AWS IPv6 IMDS under an allowlisted ULA block",
+		t.Run(tt.name, func(t *testing.T) {
-			allow:  "fd00::/8",
+			t.Parallel()
-			target: "http://[fd00:ec2::254]/latest/meta-data/",
+
-		},
+			guard := delivery.NewTestGuard(
-		{
+				netip.MustParsePrefix(tt.allow),
-			// Tailscale uses 100.64.0.0/10, so an operator
+			)
-			// forwarding to a Tailscale peer lists exactly this.
+
-			name:   "Alibaba metadata under allowlisted CGNAT",
+			err := guard.ValidateTargetURL(
-			allow:  "100.64.0.0/10",
+				context.Background(), tt.target,
-			target: "http://100.100.100.200/latest/meta-data/",
+			)
-		},
+			require.Error(t,
-		{
+				err,
-			// To4() does not normalise the IPv4-compatible form,
+				"%s must stay blocked even though %s covers it",
-			// so this needs its own always-blocked entry.
+				tt.target, tt.allow,
-			name:   "IPv4-compatible IPv6 form of the metadata IP",
+			)
-			allow:  allowAllIPv6,
+			assert.Contains(t, err.Error(), "link-local",
-			target: "http://[::a9fe:a9fe]/latest/meta-data/",
+				"the refusal must say why it cannot be opened",
-		},
+			)
-		{
+
-			// Nor the NAT64 well-known prefix form.
+			assertDialRefused(t, guard, tt.target)
-			name:   "NAT64 form of the metadata IP",
+		})
-			allow:  allowAllIPv6,
-			target: "http://[64:ff9b::a9fe:a9fe]/latest/meta-data/",
-		},
-		{
-			// Already refused before this change: IPNet.Contains
-			// calls To4() first, so the mapped form matches
-			// 169.254.0.0/16. Pinned so it cannot regress.
-			name:   "IPv4-mapped IPv6 form of the metadata IP",
-			allow:  allowAllIPv6,
-			target: "http://[::ffff:169.254.169.254]/latest/meta-data/",
-		},
 	}
 }
 
@@ -348,15 +292,11 @@ func TestGuardCheckIP_BothPathsShareOneDecision(t *testing.T) {
 	}
 }
 
-// TestAlwaysBlockedNetworks_PinnedSet pins the unconditional set
+// TestAlwaysBlockedNetworks_AreLinkLocal pins the unconditional
-// exactly, so it cannot quietly grow or shrink.
+// set to link-local only. It is deliberately small: everything
-//
+// else in the default blocklist is an operator's own network and
-// It stays deliberately small. Everything else in the default
+// must remain openable, or the escape hatch would not work.
-// blocklist is an operator's own network and must remain
+func TestAlwaysBlockedNetworks_AreLinkLocal(t *testing.T) {
-// openable, or the escape hatch would not work — which is why
-// the metadata endpoints outside the link-local range are host
-// routes rather than the blocks that contain them.
-func TestAlwaysBlockedNetworks_PinnedSet(t *testing.T) {
 	t.Parallel()
 
 	nets := delivery.ExportAlwaysBlockedNetworks()
@@ -366,24 +306,9 @@ func TestAlwaysBlockedNetworks_PinnedSet(t *testing.T) {
 		got = append(got, n.String())
 	}
 
-	want := []string{
+	assert.Equal(t,
-		// IPv4 link-local: the 169.254.169.254 metadata
+		[]string{"169.254.0.0/16", "fe80::/10"}, got,
-		// service on AWS, GCP, Azure and others.
+	)
-		"169.254.0.0/16",
-		// IPv6 link-local.
-		"fe80::/10",
-		// AWS IPv6 IMDS, inside the ULA space an operator may
-		// legitimately allowlist.
-		"fd00:ec2::254/128",
-		// Alibaba Cloud metadata, inside CGNAT.
-		"100.100.100.200/32",
-		// 169.254.169.254 as an IPv4-compatible IPv6 address.
-		"::a9fe:a9fe/128",
-		// 169.254.169.254 behind the NAT64 well-known prefix.
-		"64:ff9b::a9fe:a9fe/128",
-	}
-
-	assert.Equal(t, want, got)
 }
 
 // requireLoopback fails the test unless rawURL's host is a

internal/server/http.go

@@ -43,7 +43,10 @@ func (s *Server) serveUntilShutdown() {
 	err := s.httpServer.ListenAndServe()
 	if err != nil && !errors.Is(err, http.ErrServerClosed) {
 		s.log.Error("listen error", "error", err)
-		s.shutdownOnListenFailure()
+
+		if s.cancelFunc != nil {
+			s.cancelFunc()
+		}
 	}
 }
 

internal/server/listen_failure_test.go

@@ -1,95 +0,0 @@
-package server_test
-
-import (
-	"context"
-	"net"
-	"testing"
-	"time"
-
-	"github.com/stretchr/testify/require"
-	"go.uber.org/fx"
-	"sneak.berlin/go/webhooker/internal/globals"
-	"sneak.berlin/go/webhooker/internal/server"
-)
-
-// listenFailureDeadline is how long the app gets to give up after a
-// listen it cannot satisfy. The defect this pins left the process
-// reporting RUNNING for 183 seconds with nothing bound; a bind error
-// is known instantly, so anything past a moment here is that defect
-// back.
-const listenFailureDeadline = 2 * time.Second
-
-// lifecycleTimeout bounds the app's start and stop sequences so a
-// wedged hook fails the test instead of hanging it.
-const lifecycleTimeout = 15 * time.Second
-
-// TestListenFailure_ShutsDownTheApp pins that a listener the server
-// cannot bind terminates the application with a non-zero status.
-//
-// The fx OnStart hook returns as soon as the serving goroutine is
-// spawned, so a bind failure is discovered after fx has already
-// reported RUNNING. Nothing else in the graph observes it, and the
-// process used to stay alive with no listener: down, but indis-
-// tinguishable from healthy to systemd's Restart=on-failure and to
-// Docker's restart policies, which is the state this test exists to
-// keep from returning.
-//
-// The port is occupied by a listener this test holds open, on a
-// kernel-chosen port, so the failure is the real EADDRINUSE the
-// operator hits when a second instance starts. Loopback is enough to
-// collide with the server's wildcard bind: a listening socket on a
-// specific address blocks the wildcard from claiming the same port.
-func TestListenFailure_ShutsDownTheApp(t *testing.T) {
-	t.Parallel()
-
-	var listenCfg net.ListenConfig
-
-	occupied, err := listenCfg.Listen(
-		t.Context(), "tcp", "127.0.0.1:0",
-	)
-	require.NoError(t, err)
-
-	t.Cleanup(func() { _ = occupied.Close() })
-
-	addr, ok := occupied.Addr().(*net.TCPAddr)
-	require.True(t, ok, "listener is not TCP")
-
-	// The collaborators come from the wired graph rather than stubs,
-	// so the Server under test is the one that ships. Only the port
-	// is test-specific.
-	env := newTestEnv(t)
-	env.cfg.Port = addr.Port
-
-	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))
-
-	select {
-	case sig := <-app.Wait():
-		require.Equal(
-			t, server.ListenFailureExitCode, sig.ExitCode,
-			"listen failure must exit non-zero",
-		)
-	case <-time.After(listenFailureDeadline):
-		t.Fatal("listen failure left the app running")
-	}
-
-	// The stop sequence still has to complete: the fix must reach
-	// shutdown through fx rather than around it.
-	stopCtx, cancelStop := context.WithTimeout(
-		context.Background(), lifecycleTimeout,
-	)
-	defer cancelStop()
-
-	require.NoError(t, app.Stop(stopCtx))
-}

internal/server/routes.go

@@ -55,11 +55,8 @@ func (s *Server) setupGlobalMiddleware() {
 	s.router.Use(s.mw.SecurityHeaders())
 	s.router.Use(s.mw.Logging())
 
-	// Metrics recording middleware, registered only when the
+	// Metrics middleware (only if credentials are configured)
-	// endpoint that exposes what it records is served. The
+	if s.params.Config.MetricsUsername != "" {
-	// condition is the same MetricsAuthEnabled the /metrics mount
-	// in setupRoutes reads.
-	if s.params.Config.MetricsAuthEnabled() {
 		s.router.Use(s.mw.Metrics())
 	}
 
@@ -106,14 +103,8 @@ func (s *Server) setupRoutes() {
 		s.h.HandleHealthCheck(),
 	)
 
-	// Authenticated /metrics route. The condition is
+	// set up authenticated /metrics route:
-	// Config.MetricsAuthEnabled and never the username alone: a
+	if s.params.Config.MetricsUsername != "" {
-	// username with an empty password would otherwise mount the
-	// endpoint behind a credential map that accepts an empty
-	// password. Config rejects that combination at startup, and
-	// this reads the same value the startup log reports, so the
-	// two cannot disagree about whether the route exists.
-	if s.params.Config.MetricsAuthEnabled() {
 		s.router.Group(func(r chi.Router) {
 			r.Use(s.mw.MetricsAuth())
 			r.Get(

internal/server/routes_test.go

@@ -34,13 +34,6 @@ import (
 // the CSRF middleware executed.
 const csrfCookieName = "_gorilla_csrf"
 
-const (
-	// metricsUser and metricsAuthValue are the /metrics basic-auth
-	// credentials the metrics routing tests below configure.
-	metricsUser      = "metrics"
-	metricsAuthValue = "s3cret"
-)
-
 type noopNotifier struct{}
 
 func (n *noopNotifier) Notify([]delivery.Task) {}
@@ -76,23 +69,9 @@ type testEnv struct {
 func newTestEnv(t *testing.T) *testEnv {
 	t.Helper()
 
-	return newTestEnvWithConfig(t, &config.Config{
-		DataDir:     t.TempDir(),
-		Environment: config.EnvironmentDev,
-	})
-}
-
-// newTestEnvWithConfig is newTestEnv over a caller-supplied Config,
-// for the routes whose existence the configuration decides. The same
-// pointer reaches the router and every middleware, so a test cannot
-// accidentally configure one and not the other.
-func newTestEnvWithConfig(
-	t *testing.T, cfg *config.Config,
-) *testEnv {
-	t.Helper()
-
 	var (
 		log   *logger.Logger
+		cfg   *config.Config
 		mw    *middleware.Middleware
 		hnd   *handlers.Handlers
 		sess  *session.Session
@@ -105,7 +84,12 @@ func newTestEnvWithConfig(
 		fx.Provide(
 			globals.New,
 			logger.New,
-			func() *config.Config { return cfg },
+			func() *config.Config {
+				return &config.Config{
+					DataDir:     t.TempDir(),
+					Environment: config.EnvironmentDev,
+				}
+			},
 			database.New,
 			database.NewWebhookDBManager,
 			healthcheck.New,
@@ -116,7 +100,7 @@ func newTestEnvWithConfig(
 			delivery.NewGuard,
 			handlers.New,
 		),
-		fx.Populate(&log, &mw, &hnd, &sess, &db, &dbMgr),
+		fx.Populate(&log, &cfg, &mw, &hnd, &sess, &db, &dbMgr),
 	)
 	app.RequireStart()
 	t.Cleanup(app.RequireStop)
@@ -674,119 +658,3 @@ func TestSourceLogsBody_OtherUser404s(t *testing.T) {
 	assert.Equal(t, http.StatusSeeOther, anon.Code)
 	assert.Equal(t, "/pages/login", anon.Header().Get("Location"))
 }
-
-// metricsConfig is a Config differing from the routing default only
-// in the two /metrics credentials.
-func metricsConfig(
-	t *testing.T, username, password string,
-) *config.Config {
-	t.Helper()
-
-	return &config.Config{
-		DataDir:         t.TempDir(),
-		Environment:     config.EnvironmentDev,
-		MetricsUsername: username,
-		MetricsPassword: password,
-	}
-}
-
-// metricsRequest asks the real router for /metrics with the given
-// basic-auth credentials, or with no Authorization header when
-// username is empty.
-func (e *testEnv) metricsRequest(
-	username, password string,
-) *httptest.ResponseRecorder {
-	req := httptest.NewRequestWithContext(
-		context.Background(), http.MethodGet, "/metrics", nil,
-	)
-
-	if username != "" {
-		req.SetBasicAuth(username, password)
-	}
-
-	w := httptest.NewRecorder()
-	e.router.ServeHTTP(w, req)
-
-	return w
-}
-
-// TestMetricsRouteUnmountedWithoutCredentials pins that with neither
-// credential configured the route does not exist, which is the
-// documented behaviour and the only valid way for /metrics to be
-// absent.
-func TestMetricsRouteUnmountedWithoutCredentials(t *testing.T) {
-	t.Parallel()
-
-	env := newTestEnvWithConfig(t, metricsConfig(t, "", ""))
-
-	assert.Equal(
-		t, http.StatusNotFound,
-		env.metricsRequest("", "").Code,
-	)
-}
-
-// TestMetricsRouteRequiresCredentials pins that with both credentials
-// configured the route exists and every request that does not carry
-// the configured pair is refused — including the empty password that
-// a half-set configuration used to make sufficient.
-func TestMetricsRouteRequiresCredentials(t *testing.T) {
-	t.Parallel()
-
-	env := newTestEnvWithConfig(
-		t, metricsConfig(t, metricsUser, metricsAuthValue),
-	)
-
-	assert.Equal(
-		t, http.StatusUnauthorized,
-		env.metricsRequest("", "").Code,
-		"no credentials must not reach the metrics handler",
-	)
-	assert.Equal(
-		t, http.StatusUnauthorized,
-		env.metricsRequest(metricsUser, "").Code,
-		"an empty password must not reach the metrics handler",
-	)
-	assert.Equal(
-		t, http.StatusUnauthorized,
-		env.metricsRequest(metricsUser, "wrong").Code,
-	)
-
-	ok := env.metricsRequest(metricsUser, metricsAuthValue)
-	assert.Equal(t, http.StatusOK, ok.Code)
-	assert.Contains(t, ok.Body.String(), "go_goroutines")
-}
-
-// TestMetricsRouteUnmountedOnHalfSetConfig pins the defect from
-// https://git.eeqj.de/sneak/webhooker/issues/205 at the routing
-// layer. Config rejects a half-set pair at startup, so this Config
-// cannot be reached from the environment; the assertion is that the
-// route tree does not publish an endpoint accepting an empty
-// password even when handed one anyway, because the mount and the
-// startup log's hasMetricsAuth read the same value.
-func TestMetricsRouteUnmountedOnHalfSetConfig(t *testing.T) {
-	t.Parallel()
-
-	for _, tc := range []struct {
-		name     string
-		username string
-		password string
-	}{
-		{name: "username only", username: metricsUser},
-		{name: "password only", password: metricsAuthValue},
-	} {
-		t.Run(tc.name, func(t *testing.T) {
-			t.Parallel()
-
-			cfg := metricsConfig(t, tc.username, tc.password)
-			env := newTestEnvWithConfig(t, cfg)
-
-			assert.False(t, cfg.MetricsAuthEnabled())
-			assert.Equal(
-				t, http.StatusNotFound,
-				env.metricsRequest(
-					tc.username, tc.password,
-				).Code,
-			)
-		})
-	}
-}

internal/server/server.go

@@ -50,13 +50,6 @@ const (
 	minSentryFlush = 250 * time.Millisecond
 )
 
-// ListenFailureExitCode is the status the process exits with when the
-// HTTP listener cannot be established, or dies for a reason other
-// than a requested shutdown. It must stay non-zero: systemd
-// `Restart=on-failure` and Docker's restart policies key off it, and a
-// zero exit would read as a deliberate stop.
-const ListenFailureExitCode = 1
-
 // SentryFlushBudget reports how long the Sentry flush may run when
 // remaining is the time left on the fx stop context after the HTTP
 // drain. sentry.Flush takes a bare duration and honours no context,
@@ -82,13 +75,13 @@ type ServerParams struct {
 	Config     *config.Config
 	Middleware *middleware.Middleware
 	Handlers   *handlers.Handlers
-	Shutdowner fx.Shutdowner
 }
 
 // Server is the main HTTP server that wires up routes and manages
 // graceful shutdown.
 type Server struct {
 	startupTime   time.Time
+	exitCode      int
 	sentryEnabled bool
 	log           *slog.Logger
 	cancelFunc    context.CancelFunc
@@ -166,12 +159,7 @@ func (s *Server) enableSentry() {
 	s.sentryEnabled = true
 }
 
-// serve installs the signal watcher, starts the listener and blocks
+func (s *Server) serve() int {
-// until the server's context is cancelled. The process exit status is
-// fx's to decide — from a signal, or from the code
-// shutdownOnListenFailure hands the Shutdowner — so this reports
-// nothing back to its caller.
-func (s *Server) serve() {
 	ctx, cancelFunc := context.WithCancel(context.Background())
 	s.cancelFunc = cancelFunc
 
@@ -197,30 +185,7 @@ func (s *Server) serve() {
 	<-ctx.Done()
 	// Shutdown is handled by the fx OnStop hook (cleanShutdown).
 	// Do not call cleanShutdown() here to avoid double invocation.
-}
+	return s.exitCode
-
-// shutdownOnListenFailure ends the application after the HTTP
-// listener failed. The fx OnStart hook returns as soon as the serving
-// goroutine is spawned, so nothing downstream of it ever learns that
-// the listen failed: fx reports RUNNING and the process sits alive
-// with nothing bound, which is invisible to systemd and Docker
-// restart policies. Asking the Shutdowner to stop the app with a
-// non-zero code is what turns that into a visible failure.
-//
-// The context cancel that follows only unwinds serve()'s own wait.
-// The shutdown itself runs through fx's normal stop sequence, so the
-// clean-shutdown drain in cleanShutdown is reached unchanged.
-func (s *Server) shutdownOnListenFailure() {
-	err := s.params.Shutdowner.Shutdown(
-		fx.ExitCode(ListenFailureExitCode),
-	)
-	if err != nil {
-		s.log.Error("shutdown request failed", "error", err)
-	}
-
-	if s.cancelFunc != nil {
-		s.cancelFunc()
-	}
 }
 
 func (s *Server) cleanupForExit() {
@@ -228,6 +193,9 @@ func (s *Server) cleanupForExit() {
 }
 
 func (s *Server) cleanShutdown(ctx context.Context) {
+	// initiate clean shutdown
+	s.exitCode = 0
+
 	ctxShutdown, shutdownCancel := context.WithTimeout(
 		ctx, ShutdownTimeout,
 	)