Isolate config tests from the shell; any out-of-range PORT is ErrInvalidPort (closes #94)

Config tests, and the first-boot debug log test that builds a Config,
start from an empty environment: config.ClearEnvForTest unsets every
variable the process has and restores each when the test ends, so
nothing exported in the developer's shell changes a result.
TestEnvPositiveInt and TestEnvPort share one table runner, and
TestEnvPort checks that the bad value appears in each error.

Every out-of-range PORT now wraps ErrInvalidPort: zero, negatives,
above 65535, and numbers too large or too small for an int.

The README configuration table and the Settings page say that a
RETENTION_SWEEP_INTERVAL that does not parse, or is zero or negative,
fails startup.

Model: opus-5-5

README.md

@@ -139,7 +139,7 @@ TTY detection, and security headers are always applied.
 | `METRICS_USERNAME`      | Basic auth username for `/metrics`. Must be set together with `METRICS_PASSWORD`; one without the other fails startup | `""`     |
 | `METRICS_PASSWORD`      | Basic auth password for `/metrics`. Must be set together with `METRICS_USERNAME`; one without the other fails startup | `""`     |
 | `SENTRY_DSN`            | Sentry error reporting DSN. Unset leaves error reporting off; a value the Sentry SDK cannot parse fails startup rather than serving with reporting silently off | `""`     |
-| `RETENTION_SWEEP_INTERVAL` | How often the retention reaper and archive sweeper run (Go duration, must be positive) | `1h` |
+| `RETENTION_SWEEP_INTERVAL` | How often the retention reaper and archive sweeper run (Go duration, must be positive). A value that does not parse, or is zero or negative, fails startup | `1h` |
 | `SESSION_IDLE_TIMEOUT`  | Idle session timeout (Go duration)  | `24h`    |
 | `RECEIVER_RATE_LIMIT`   | Receiver requests/minute per IP per entrypoint (10x that per IP across the route) | `120` |
 | `TRUSTED_PROXIES`       | CIDRs whose forwarded headers are trusted. A set value replaces the default. If any client can reach webhooker, or the proxy in front of it, from an RFC 1918 source address, set it to the proxy's address alone. See [Trusted proxies](#trusted-proxies) | `10.0.0.0/8,172.16.0.0/12,192.168.0.0/16` (RFC 1918) |
@@ -457,19 +457,6 @@ Your proxy must therefore **append** the peer address to
 `option forwardfor`, Caddy and AWS ALB by default), and must append a
 bare address with no port.
 
-Every log line that names a client carries two addresses: `remoteIP`,
-the connecting peer, which behind a proxy is the proxy; and `clientIP`,
-the client the rate limiters identify by the rules above, which is the
-field to read when tracing who sent what. Those lines are the
-`http request` access log line, the rate-limit rejection lines
-(`login failure limit exceeded` among them), the
-`csrf: token validation failed` warning and the receiver's
-`webhook request received` line. `clientIP` is only as trustworthy as
-`TRUSTED_PROXIES`: for a request from a peer inside the list, it is
-read out of the `X-Forwarded-For` that peer sent, so a peer that does
-not belong in the list can make it name any address it likes. For a
-request from any other peer, both fields name the peer.
-
 #### Sessions
 
 Sessions are bounded by two independent clocks, and end at whichever
@@ -854,9 +841,9 @@ reports.
    was given, so on any port other than 443 `$host` makes every form
    POST — including login — fail with `403 origin invalid`, with
    nothing in the error naming the cause.
-5. **Keep the proxy's access log.** webhooker's own access log names
-   the client in its `clientIP` field only while `TRUSTED_PROXIES`
-   covers the proxy; the proxy's log names it regardless. nginx's
+5. **Keep the proxy's access log.** webhooker's own access log records
+   the peer address, which behind a proxy is always the proxy. The
+   proxy's log is the only record of which client sent what. nginx's
    default `combined` format already logs `$remote_addr`; do not
    replace it with one that drops the client address, and retain those
    logs as long as you would want to answer a question about traffic.
@@ -885,8 +872,9 @@ server {
     # webhooker's message.
     client_max_body_size 1m;
 
-    # $remote_addr is the client. webhooker's own log names it, as
-    # clientIP, only while TRUSTED_PROXIES covers this proxy.
+    # $remote_addr is the client. webhooker's own log records this
+    # proxy and nothing else, so this file is the only place the
+    # client's address is written down.
     access_log /var/log/nginx/webhooker.access.log combined;
 
     location / {
@@ -2029,29 +2017,6 @@ Because each `database` target has its own archive file, a target's
 webhook with different expiries keep two archives, each pruned on its
 own schedule.
 
-Each `database` target on the webhook page has a **Download** button,
-which returns its archive as one gzipped JSON file,
-`archive-{webhook_name}-{target_name}-{YYYYMMDDTHHMMSSZ}.json.gz`, the
-names made safe as above and the time in UTC. The file holds one
-object: `webhook` and `target`, each an `id` and a `name`;
-`exported_at`; and `archived_events`, one object per archived row with
-every column, keyed by column name. A body that is not valid UTF-8 is
-written in base64, with `"body_encoding": "base64"` beside it. An
-archive that does not exist yet, or was moved away, downloads with an
-empty `archived_events`; the download never creates the file.
-
-The download streams: each row is read and written out compressed
-before the next is read, so neither the archive nor the JSON is held in
-memory. It reads on a connection of its own, inside one read-only
-transaction, so the file holds the archive as it stood when the
-download started, and archive writes go on meanwhile, since under WAL a
-reader never blocks a writer. While it runs, the `-wal` cannot be
-checkpointed past what it reads, so a long download lets the `-wal`
-grow. It finds the file by the stored names under the lock that webhook
-edits, target edits and target creation hold, and lets go once the file
-is open: a rename during the download moves the file without affecting
-it.
-
 Deleting a webhook releases its archives: the delivery engine's cached
 archive writers are dropped and their file handles closed, so nothing
 lingers after the webhook is gone. The archive **files themselves are
@@ -2508,21 +2473,20 @@ trade.
 Net: **one `INFO` line per request, of at most 2,560 bytes.** That
 ceiling is arithmetic, not an observation: 3 × (512 + 11) for `url`,
 `useragent` and `referer`, plus 128 + 11 for `request_id`, plus 32 + 11
-for `method`, plus a 405-byte fixed portion (the field names, the
-punctuation, both timestamps at their longest, `remoteIP` and
-`clientIP` each charged as an IPv6 address with a zone, the status and
-the latency) — 2,156 bytes, stated at 2,560 so the figure has headroom.
-`internal/middleware/accesslog_test.go` asserts it against 8 KB of
-client-chosen text in the path, in the query, and in each of
-`User-Agent`, `Referer`, `X-Request-Id` and `X-Forwarded-For`,
+for `method`, plus a 336-byte fixed portion (the field names, the
+punctuation, both timestamps at their longest, an IPv6 `remoteIP` with
+a zone, the status and the latency) — 2,087 bytes, stated at 2,560 so
+the figure has headroom. `internal/middleware/accesslog_test.go`
+asserts it against 8 KB of client-chosen text in the path, in the
+query, and in each of `User-Agent`, `Referer` and `X-Request-Id`,
 including cases built from the characters the handlers escape, and
-against a 5xx that keeps its concrete path while all three header fields
-are also at their budget and an `X-Forwarded-For` sent from a trusted
-proxy ends in an IPv6 client address at its longest followed by an 8 KB
-zone, where `clientIP` must name the address without the zone. Every
-case runs through both handlers `internal/logger` can select — the JSON
-one and the text one it installs on a tty — since the two do not escape
-alike and the ceiling is quoted unqualified.
+against the widest access log line the service can be made to write: a
+5xx that keeps its concrete path while all three header fields are also
+at their budget. Every case runs through both handlers
+`internal/logger` can select — the JSON one and the text one it installs
+on a tty — since the two do not escape alike and the ceiling is quoted
+unqualified. Measured over a real connection, the widest access log line
+is 1,972 bytes.
 
 Multiply that ceiling by the request rate to size log storage. Note
 that the rate is not bounded by the limits above on every route:
@@ -2860,9 +2824,9 @@ remedies are to block the source at the reverse proxy, or to
 rate-limit `POST /pages/login` there — the one place a limit can be
 applied without reintroducing the lockout, because the proxy sees the
 real client address. `TRUSTED_PROXIES` does not stop the saturation.
-The flood's source is in the `clientIP` field of webhooker's access
-log while `TRUSTED_PROXIES` covers the proxy, and in the proxy's own
-access log either way (see [Trusted proxies](#trusted-proxies)).
+The flood's source is in the proxy's access log: webhooker's own logs
+record the proxy's address, not the client's (see
+[Deployment behind a reverse proxy](#deployment-behind-a-reverse-proxy)).
 
 Finer-grained per-webhook rate limits (configured in the web UI and
 enforced in the webhook handler) can layer on top of this env-level
@@ -2915,7 +2879,6 @@ returns to the page that was asked for.
 | `POST` | `/hook/{id}/targets`     | Add target to webhook |
 | `GET`  | `/hook/{id}/targets/{targetID}/edit` | Edit target form. The one page that renders a target's destination URL and header values in full, rather than masked |
 | `POST` | `/hook/{id}/targets/{targetID}/edit` | Edit target submission |
-| `GET`  | `/hook/{id}/targets/{targetID}/download` | Download a `database` target's archive as one gzipped JSON file. See [Database Architecture](#database-architecture) |
 | `POST` | `/hook/{id}/targets/{targetID}/delete` | Delete a target |
 | `POST` | `/hook/{id}/targets/{targetID}/toggle` | Enable or disable a target |
 
@@ -2961,7 +2924,8 @@ webhooker/
 │   ├── resetpw/
 │   │   └── resetpw.go         # `webhooker resetpw`: set an account's password, stopped deployments only
 │   ├── config/
-│   │   └── config.go          # Configuration loading from environment variables
+│   │   ├── config.go          # Configuration loading from environment variables
+│   │   └── testing.go         # ClearEnvForTest: an empty environment for one test
 │   ├── database/
 │   │   ├── base_model.go      # BaseModel with UUID primary keys
 │   │   ├── database.go        # GORM connection, migrations, admin seed
@@ -2996,7 +2960,6 @@ webhooker/
 │   │   ├── target_slack.go    # Slack/Mattermost incoming-webhook target
 │   │   ├── target_database.go # Database archive target
 │   │   ├── target_database_archive.go  # Archive file lifecycle and pruning
-│   │   ├── target_database_export.go   # Archive download as gzipped JSON
 │   │   ├── target_log.go      # Log target (stdout)
 │   │   ├── target_config_view.go  # Masked target config for templates
 │   │   ├── archive_sweeper.go # Periodic pruning of idle archives
@@ -3102,7 +3065,7 @@ Applied to all routes in this order:
    (HSTS, X-Content-Type-Options, X-Frame-Options, CSP, Referrer-Policy,
    Permissions-Policy)
 3. **Logging** — Structured request logging (method, URL, status,
-   latency, remote IP, client IP, user agent, request ID)
+   latency, remote IP, user agent, request ID)
 4. **Metrics** — Prometheus HTTP metrics (if `METRICS_USERNAME` and
    `METRICS_PASSWORD` are both set)
 5. **CORS** — Cross-origin resource sharing headers

internal/config/config.go

@@ -80,8 +80,7 @@ const (
 	// process over a Docker network or a private LAN connects from.
 	defaultTrustedProxies = "10.0.0.0/8,172.16.0.0/12,192.168.0.0/16"
 
-	// maxPort is the highest valid TCP port number. The lower
-	// bound (at least 1) is enforced by envPositiveInt.
+	// maxPort is the highest valid TCP port number.
 	maxPort = 65535
 
 	// mappedV4Offset is the number of leading bits an IPv4-mapped
@@ -105,7 +104,7 @@ var ErrInvalidEnvironment = errors.New("invalid environment")
 var ErrNonPositiveValue = errors.New("value must be positive")
 
 // ErrInvalidPort is returned when an environment variable holding a
-// TCP port number is set above the valid port range.
+// TCP port number is set to a number outside 1 to 65535.
 var ErrInvalidPort = errors.New("invalid port")
 
 // ErrInvalidCIDR is returned when an environment variable holding a
@@ -363,17 +362,27 @@ func envPositiveInt(
 // envPort returns the value of the named environment variable parsed
 // as a TCP port number. Returns defaultValue if not set. A set value
 // that is unparseable, below 1, or above maxPort is a hard error
-// naming the key and the bad value.
+// naming the key and the bad value; every out-of-range value wraps
+// ErrInvalidPort, including one too large or too small for an int.
 func envPort(key string, defaultValue int) (int, error) {
-	port, err := envPositiveInt(key, defaultValue)
-	if err != nil {
-		return 0, err
+	v := os.Getenv(key)
+	if v == "" {
+		return defaultValue, nil
 	}
 
-	if port > maxPort {
+	// strconv.ErrRange means a number too large or too small for an
+	// int, which is outside the port range as well.
+	port, err := strconv.Atoi(v)
+	if err != nil && !errors.Is(err, strconv.ErrRange) {
 		return 0, fmt.Errorf(
-			"%w: %s must be at most %d, got %d",
-			ErrInvalidPort, key, maxPort, port,
+			"invalid integer for %s: %q: %w", key, v, err,
+		)
+	}
+
+	if err != nil || port < 1 || port > maxPort {
+		return 0, fmt.Errorf(
+			"%w: %s must be from 1 to %d, got %q",
+			ErrInvalidPort, key, maxPort, v,
 		)
 	}
 

internal/config/config_test.go

@@ -3,7 +3,6 @@ package config_test
 import (
 	"bytes"
 	"log/slog"
-	"os"
 	"testing"
 	"time"
 
@@ -71,14 +70,12 @@ func TestEnvironmentConfig(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
+			config.ClearEnvForTest(t)
+
 			if tt.envValue != "" {
 				t.Setenv(
 					"WEBHOOKER_ENVIRONMENT", tt.envValue,
 				)
-			} else {
-				require.NoError(t, os.Unsetenv(
-					"WEBHOOKER_ENVIRONMENT",
-				))
 			}
 
 			for k, v := range tt.envVars {
@@ -199,14 +196,11 @@ func TestRetentionSweepInterval(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
+			config.ClearEnvForTest(t)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 
 			if tt.set {
 				t.Setenv("RETENTION_SWEEP_INTERVAL", tt.value)
-			} else {
-				require.NoError(t, os.Unsetenv(
-					"RETENTION_SWEEP_INTERVAL",
-				))
 			}
 
 			if tt.expectError {
@@ -341,14 +335,11 @@ func TestSessionIdleTimeout(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
+			config.ClearEnvForTest(t)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 
 			if tt.set {
 				t.Setenv("SESSION_IDLE_TIMEOUT", tt.value)
-			} else {
-				require.NoError(t, os.Unsetenv(
-					"SESSION_IDLE_TIMEOUT",
-				))
 			}
 
 			if tt.expectError {
@@ -397,16 +388,12 @@ func TestDefaultDataDir(t *testing.T) {
 		t.Run("env="+name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
+			config.ClearEnvForTest(t)
+
 			if env != "" {
 				t.Setenv("WEBHOOKER_ENVIRONMENT", env)
-			} else {
-				require.NoError(t, os.Unsetenv(
-					"WEBHOOKER_ENVIRONMENT",
-				))
 			}
 
-			require.NoError(t, os.Unsetenv("DATA_DIR"))
-
 			var cfg *config.Config
 
 			app := fxtest.New(
@@ -446,9 +433,9 @@ func TestDataDirHelper(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
-			if set == "" {
-				require.NoError(t, os.Unsetenv("DATA_DIR"))
-			} else {
+			config.ClearEnvForTest(t)
+
+			if set != "" {
 				t.Setenv("DATA_DIR", set)
 			}
 
@@ -511,14 +498,11 @@ func TestReceiverRateLimit(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
+			config.ClearEnvForTest(t)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 
 			if tt.set {
 				t.Setenv("RECEIVER_RATE_LIMIT", tt.value)
-			} else {
-				require.NoError(t, os.Unsetenv(
-					"RECEIVER_RATE_LIMIT",
-				))
 			}
 
 			if tt.expectError {
@@ -630,12 +614,11 @@ func TestTrustedProxies(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
+			config.ClearEnvForTest(t)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 
 			if tt.set {
 				t.Setenv("TRUSTED_PROXIES", tt.value)
-			} else {
-				require.NoError(t, os.Unsetenv("TRUSTED_PROXIES"))
 			}
 
 			if tt.expectError {
@@ -742,14 +725,11 @@ func TestAllowedEgressCIDRs(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
+			config.ClearEnvForTest(t)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 
 			if tt.set {
 				t.Setenv("ALLOWED_EGRESS_CIDRS", tt.value)
-			} else {
-				require.NoError(
-					t, os.Unsetenv("ALLOWED_EGRESS_CIDRS"),
-				)
 			}
 
 			if tt.expectError {
@@ -817,13 +797,10 @@ func TestEgressAllowlistWarning(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
+			config.ClearEnvForTest(t)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", config.EnvironmentDev)
 
-			if tt.allowed == "" {
-				require.NoError(
-					t, os.Unsetenv("ALLOWED_EGRESS_CIDRS"),
-				)
-			} else {
+			if tt.allowed != "" {
 				t.Setenv("ALLOWED_EGRESS_CIDRS", tt.allowed)
 			}
 
@@ -956,20 +933,14 @@ func TestMetricsAuthConfig(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
+			config.ClearEnvForTest(t)
+
 			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 {

internal/config/dotenv_test.go

@@ -22,17 +22,6 @@ const malformedDotEnv = "PORT 19615\n" +
 	"this is not = valid ! syntax\n" +
 	"\"unclosed\n"
 
-// unsetDotEnvKey makes dotEnvKey genuinely absent for the duration of
-// the test and restores it afterwards. t.Setenv registers the restore;
-// the Unsetenv that follows is what the test actually needs, because a
-// variable set to the empty string is still present in os.Environ and
-// godotenv would refuse to overwrite it.
-func unsetDotEnvKey(t *testing.T) {
-	t.Helper()
-	t.Setenv(dotEnvKey, "placeholder")
-	require.NoError(t, os.Unsetenv(dotEnvKey))
-}
-
 // writeDotEnv writes contents to a .env file in a fresh temporary
 // directory and returns its path.
 func writeDotEnv(t *testing.T, contents string) string {
@@ -50,9 +39,9 @@ func writeDotEnv(t *testing.T, contents string) string {
 // normally rather than be refused for a file it was never meant to
 // have.
 //
-//nolint:paralleltest // unsetDotEnvKey uses t.Setenv.
+//nolint:paralleltest // ClearEnvForTest uses t.Setenv.
 func TestLoadDotEnv_MissingFileIsFine(t *testing.T) {
-	unsetDotEnvKey(t)
+	config.ClearEnvForTest(t)
 
 	absent := filepath.Join(t.TempDir(), config.DotEnvPath)
 	require.NoError(t, config.LoadDotEnvFileForTest(absent))
@@ -65,9 +54,9 @@ func TestLoadDotEnv_MissingFileIsFine(t *testing.T) {
 // reaches the environment, which is the whole reason the file is read
 // at all.
 //
-//nolint:paralleltest // unsetDotEnvKey uses t.Setenv.
+//nolint:paralleltest // ClearEnvForTest uses t.Setenv.
 func TestLoadDotEnv_AppliesValues(t *testing.T) {
-	unsetDotEnvKey(t)
+	config.ClearEnvForTest(t)
 
 	path := writeDotEnv(t, "# a comment\n"+dotEnvKey+"=from-dot-env\n")
 
@@ -93,9 +82,9 @@ func TestLoadDotEnv_RealEnvironmentWins(t *testing.T) {
 // reverts to its default; the process used to start that way with no
 // log line naming the file at all.
 //
-//nolint:paralleltest // unsetDotEnvKey uses t.Setenv.
+//nolint:paralleltest // ClearEnvForTest uses t.Setenv.
 func TestLoadDotEnv_MalformedFileAborts(t *testing.T) {
-	unsetDotEnvKey(t)
+	config.ClearEnvForTest(t)
 
 	path := writeDotEnv(
 		t, malformedDotEnv+dotEnvKey+"=from-dot-env\n",
@@ -143,7 +132,7 @@ func TestLoadDotEnv_UnreadableFileAborts(t *testing.T) {
 //
 //nolint:paralleltest // t.Chdir moves the whole process.
 func TestLoadDotEnv_ReadsTheWorkingDirectory(t *testing.T) {
-	unsetDotEnvKey(t)
+	config.ClearEnvForTest(t)
 
 	dir := t.TempDir()
 	require.NoError(t, os.WriteFile(

internal/config/env_test.go

@@ -1,7 +1,6 @@
 package config_test
 
 import (
-	"os"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -121,10 +120,10 @@ func TestEnvBool(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
+			config.ClearEnvForTest(t)
+
 			if tt.set {
 				t.Setenv(testEnvKey, tt.value)
-			} else {
-				require.NoError(t, os.Unsetenv(testEnvKey))
 			}
 
 			got, err := config.EnvBoolForTest(
@@ -145,17 +144,62 @@ func TestEnvBool(t *testing.T) {
 	}
 }
 
+// envIntCase is one row of the envPositiveInt and envPort tables.
+type envIntCase struct {
+	name        string
+	set         bool
+	value       string
+	expectError bool
+	errIs       error
+	expected    int
+}
+
+// runEnvIntCases runs each row through parse, which is
+// envPositiveInt or envPort, with testEnvKey set to the row's value
+// or left unset.
+func runEnvIntCases(
+	t *testing.T,
+	parse func(key string, defaultValue int) (int, error),
+	defaultValue int,
+	tests []envIntCase,
+) {
+	t.Helper()
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Cannot use t.Parallel() here because t.Setenv
+			// is incompatible with parallel subtests.
+			config.ClearEnvForTest(t)
+
+			if tt.set {
+				t.Setenv(testEnvKey, tt.value)
+			}
+
+			got, err := parse(testEnvKey, defaultValue)
+
+			if tt.expectError {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), testEnvKey)
+				assert.Contains(t, err.Error(), tt.value)
+
+				if tt.errIs != nil {
+					require.ErrorIs(t, err, tt.errIs)
+				}
+
+				return
+			}
+
+			require.NoError(t, err)
+			assert.Equal(t, tt.expected, got)
+		})
+	}
+}
+
+//nolint:paralleltest // runEnvIntCases uses t.Setenv.
 func TestEnvPositiveInt(t *testing.T) {
 	const defaultValue = 7
 
-	tests := []struct {
-		name        string
-		set         bool
-		value       string
-		expectError bool
-		errIs       error
-		expected    int
-	}{
+	runEnvIntCases(t, config.EnvPositiveIntForTest, defaultValue, []envIntCase{
 		{
 			name:     "unset returns the default integer",
 			expected: defaultValue,
@@ -192,51 +236,14 @@ func TestEnvPositiveInt(t *testing.T) {
 			expectError: true,
 			errIs:       config.ErrNonPositiveValue,
 		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			// Cannot use t.Parallel() here because t.Setenv
-			// is incompatible with parallel subtests.
-			if tt.set {
-				t.Setenv(testEnvKey, tt.value)
-			} else {
-				require.NoError(t, os.Unsetenv(testEnvKey))
-			}
-
-			got, err := config.EnvPositiveIntForTest(
-				testEnvKey, defaultValue,
-			)
-
-			if tt.expectError {
-				require.Error(t, err)
-				assert.Contains(t, err.Error(), testEnvKey)
-				assert.Contains(t, err.Error(), tt.value)
-
-				if tt.errIs != nil {
-					require.ErrorIs(t, err, tt.errIs)
-				}
-
-				return
-			}
-
-			require.NoError(t, err)
-			assert.Equal(t, tt.expected, got)
-		})
-	}
+	})
 }
 
+//nolint:paralleltest // runEnvIntCases uses t.Setenv.
 func TestEnvPort(t *testing.T) {
 	const defaultValue = 8080
 
-	tests := []struct {
-		name        string
-		set         bool
-		value       string
-		expectError bool
-		errIs       error
-		expected    int
-	}{
+	runEnvIntCases(t, config.EnvPortForTest, defaultValue, []envIntCase{
 		{
 			name:     "unset returns the default port",
 			expected: defaultValue,
@@ -264,7 +271,14 @@ func TestEnvPort(t *testing.T) {
 			set:         true,
 			value:       "0",
 			expectError: true,
-			errIs:       config.ErrNonPositiveValue,
+			errIs:       config.ErrInvalidPort,
+		},
+		{
+			name:        "negative is rejected",
+			set:         true,
+			value:       "-1",
+			expectError: true,
+			errIs:       config.ErrInvalidPort,
 		},
 		{
 			name:        "above the port range is rejected",
@@ -273,37 +287,14 @@ func TestEnvPort(t *testing.T) {
 			expectError: true,
 			errIs:       config.ErrInvalidPort,
 		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			// Cannot use t.Parallel() here because t.Setenv
-			// is incompatible with parallel subtests.
-			if tt.set {
-				t.Setenv(testEnvKey, tt.value)
-			} else {
-				require.NoError(t, os.Unsetenv(testEnvKey))
-			}
-
-			got, err := config.EnvPortForTest(
-				testEnvKey, defaultValue,
-			)
-
-			if tt.expectError {
-				require.Error(t, err)
-				assert.Contains(t, err.Error(), testEnvKey)
-
-				if tt.errIs != nil {
-					require.ErrorIs(t, err, tt.errIs)
-				}
-
-				return
-			}
-
-			require.NoError(t, err)
-			assert.Equal(t, tt.expected, got)
-		})
-	}
+		{
+			name:        "too large for an int is rejected",
+			set:         true,
+			value:       "99999999999999999999",
+			expectError: true,
+			errIs:       config.ErrInvalidPort,
+		},
+	})
 }
 
 // TestEnvBindAddress covers BIND_ADDRESS parsing.
@@ -319,10 +310,10 @@ func TestEnvBindAddress(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
+			config.ClearEnvForTest(t)
+
 			if tt.set {
 				t.Setenv(testEnvKey, tt.value)
-			} else {
-				require.NoError(t, os.Unsetenv(testEnvKey))
 			}
 
 			got, err := config.EnvBindAddressForTest(
@@ -485,6 +476,7 @@ func TestNewRejectsBadEnvValues(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
+			config.ClearEnvForTest(t)
 			t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 			t.Setenv(tt.key, tt.value)
 
@@ -646,14 +638,9 @@ func sentryEnvValueCases() []badEnvValueCase {
 // break the legitimate unset case: absent variables still get their
 // documented defaults.
 func TestNewUsesDefaultsWhenUnset(t *testing.T) {
+	config.ClearEnvForTest(t)
 	t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
 
-	for _, key := range []string{
-		envKeyPort, envKeyDebug, envKeyBindAddress, envKeySentryDSN,
-	} {
-		require.NoError(t, os.Unsetenv(key))
-	}
-
 	cfg, err := buildConfig(t)
 	require.NoError(t, err)
 	require.NotNil(t, cfg)

internal/config/sentry_test.go

@@ -1,7 +1,6 @@
 package config_test
 
 import (
-	"os"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -101,10 +100,10 @@ func TestEnvSentryDSN(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			// Cannot use t.Parallel() here because t.Setenv
 			// is incompatible with parallel subtests.
+			config.ClearEnvForTest(t)
+
 			if tt.set {
 				t.Setenv(envKeySentryDSN, tt.value)
-			} else {
-				require.NoError(t, os.Unsetenv(envKeySentryDSN))
 			}
 
 			got, err := config.EnvSentryDSNForTest(envKeySentryDSN)

internal/config/testing.go

@@ -0,0 +1,30 @@
+package config
+
+import (
+	"os"
+	"strings"
+	"testing"
+)
+
+// ClearEnvForTest unsets every variable in the process environment
+// for the rest of the test and puts each back when the test ends, so
+// a test sees only the variables it sets itself, not whatever the
+// developer's shell exports.
+func ClearEnvForTest(t *testing.T) {
+	t.Helper()
+
+	for _, entry := range os.Environ() {
+		key, _, _ := strings.Cut(entry, "=")
+
+		// t.Setenv registers the restore; the Unsetenv after it is
+		// what makes the key absent, since a key set to the empty
+		// string is still present, and godotenv will not overwrite a
+		// present key.
+		t.Setenv(key, "")
+
+		err := os.Unsetenv(key)
+		if err != nil {
+			t.Fatalf("unsetting %s: %v", key, err)
+		}
+	}
+}

internal/database/retention.go

@@ -184,6 +184,16 @@ func (r *RetentionReaper) sweep(ctx context.Context) {
 
 		wh := webhooks[i]
 
+		// Skip retain-forever webhooks before building any query.
+		// RetainsForever covers both the RetentionForeverDays
+		// sentinel and the non-positive values that predate it: the
+		// sentinel is a positive number, so without this the reaper
+		// would compute a cutoff a thousand years in the past and
+		// issue a DELETE matching nothing on every single sweep.
+		if wh.RetainsForever() {
+			continue
+		}
+
 		// Nothing to reap if the per-webhook database has never
 		// been created.
 		if !r.dbManager.DBExists(wh.ID) {
@@ -202,13 +212,6 @@ func (r *RetentionReaper) reapWebhook(
 	webhookID string,
 	retentionDays int,
 ) {
-	// A retain-forever webhook has no cutoff, so its database is not
-	// even opened.
-	cutoff, ok := retentionCutoff(time.Now(), retentionDays)
-	if !ok {
-		return
-	}
-
 	db, err := r.dbManager.GetDB(webhookID)
 	if err != nil {
 		r.log.Error(
@@ -220,6 +223,11 @@ func (r *RetentionReaper) reapWebhook(
 		return
 	}
 
+	cutoff, ok := retentionCutoff(time.Now(), retentionDays)
+	if !ok {
+		return
+	}
+
 	deleted, err := reapExpired(ctx, db, cutoff)
 	if err != nil {
 		r.log.Error(

internal/database/retention_test.go

@@ -362,7 +362,7 @@ func TestRetentionReaper_HugeFiniteRetentionRetainsRecentEvents(
 		t,
 		overflowingRetentionDays,
 		database.RetentionForeverDays,
-		"the test value must not be treated as retain-forever",
+		"the test value must not be rescued by the forever skip",
 	)
 
 	webhookID := createWebhook(

internal/delivery/target_database.go

@@ -3,6 +3,7 @@ package delivery
 import (
 	"context"
 	"fmt"
+	"path/filepath"
 	"strings"
 	"sync"
 	"time"
@@ -276,9 +277,10 @@ func (t *databaseTarget) releaseSweepWriter(
 }
 
 // newWriter builds the writer for a database target's archive. The
-// file is the one ArchivePath gives for the webhook and the target as
-// the main database names them now; from then on only rename changes
-// the name the writer uses. It does not touch the archive file.
+// file lives beside the webhook's event database in the data
+// directory and is named for the webhook and the target as the main
+// database has them now; from then on only rename changes the name
+// the writer uses. It does not touch the archive file.
 func (t *databaseTarget) newWriter(
 	targetID string,
 ) (*archiveWriter, error) {
@@ -297,10 +299,12 @@ func (t *databaseTarget) newWriter(
 		)
 	}
 
-	w := newArchiveWriter(
-		ArchivePath(t.eng.dbManager, &target.Webhook, &target),
-		t.eng.log,
+	dir := filepath.Dir(t.eng.dbManager.DBPath(target.WebhookID))
+	name := ArchiveFileName(
+		target.Webhook.Name, target.Name, target.ID,
 	)
+
+	w := newArchiveWriter(filepath.Join(dir, name), t.eng.log)
 	w.webhookID = target.WebhookID
 
 	return w, nil

internal/delivery/target_database_export.go

@@ -1,275 +0,0 @@
-package delivery
-
-import (
-	"compress/gzip"
-	"context"
-	"database/sql"
-	"encoding/base64"
-	"encoding/json"
-	"fmt"
-	"io"
-	"log/slog"
-	"path/filepath"
-	"time"
-	"unicode/utf8"
-
-	"gorm.io/driver/sqlite"
-	"gorm.io/gorm"
-	"sneak.berlin/go/webhooker/internal/database"
-	"sneak.berlin/go/webhooker/internal/gormlog"
-)
-
-// archiveTableQuery counts the archive's table: 0 when the archive
-// writer has created the file but not yet the table in it.
-const archiveTableQuery = "SELECT count(*) FROM sqlite_master " +
-	"WHERE type = 'table' AND name = 'archived_events'"
-
-// ArchivePath returns where a database target's archive file is: in
-// the data directory, beside the webhook's event database, under the
-// name ArchiveFileName gives it.
-func ArchivePath(
-	dbMgr *database.WebhookDBManager,
-	webhook *database.Webhook,
-	target *database.Target,
-) string {
-	return filepath.Join(
-		filepath.Dir(dbMgr.DBPath(webhook.ID)),
-		ArchiveFileName(webhook.Name, target.Name, target.ID),
-	)
-}
-
-// ArchiveExportFileName returns the name a database target's archive
-// downloads under:
-// archive-WEBHOOKNAME-TARGETNAME-YYYYMMDDTHHMMSSZ.json.gz, the names
-// made safe as in ArchiveFileName and the time in UTC.
-func ArchiveExportFileName(
-	webhookName, targetName string, at time.Time,
-) string {
-	return "archive-" + archiveNamePart(webhookName) + "-" +
-		archiveNamePart(targetName) + "-" +
-		at.UTC().Format("20060102T150405Z") + ".json.gz"
-}
-
-// ArchiveExport is a database target's archive opened for download.
-// It reads the file on its own connection, inside one read-only
-// transaction, so it writes out the archive as it stood when
-// OpenArchiveExport returned.
-//
-// Archives are in WAL mode, where a reader works from a snapshot and
-// never blocks a writer: archive writes go on while an export is open,
-// and the export does not see them. SQLite cannot checkpoint the -wal
-// past an open snapshot, so the -wal grows until the export is closed.
-type ArchiveExport struct {
-	db *sql.DB
-	tx *gorm.DB
-
-	// empty is true when there is nothing to read: no file, or a file
-	// without the archive's table yet.
-	empty bool
-}
-
-// exportedName is how an export names its webhook and its target.
-type exportedName struct {
-	ID   string `json:"id"`
-	Name string `json:"name"`
-}
-
-// OpenArchiveExport opens the archive file at path for export and
-// takes the snapshot the export reads. It never creates the file: with
-// no file at path, the export has no rows.
-//
-// Once it has returned, the file is open, so a rename or a move of it
-// does not affect the export, which reads the same file under its new
-// name.
-//
-// The transaction lasts as long as ctx does, so ctx must last for the
-// whole export.
-func OpenArchiveExport(
-	ctx context.Context, path string, log *slog.Logger,
-) (*ArchiveExport, error) {
-	if !fileExists(path) {
-		return &ArchiveExport{empty: true}, nil
-	}
-
-	db, err := database.OpenSQLite(path, archiveModeExisting)
-	if err != nil {
-		return nil, fmt.Errorf("opening archive %s: %w", path, err)
-	}
-
-	gdb, err := gorm.Open(
-		sqlite.Dialector{Conn: db}, &gorm.Config{
-			// Never leave this at GORM's default. See
-			// internal/gormlog.
-			Logger: gormlog.New(log),
-		},
-	)
-	if err != nil {
-		_ = db.Close()
-
-		return nil, fmt.Errorf("opening archive %s: %w", path, err)
-	}
-
-	// ReadOnly makes the driver begin a deferred transaction in place
-	// of the BEGIN IMMEDIATE the connection string asks for, so the
-	// export never takes the archive's write lock.
-	tx := gdb.WithContext(ctx).Begin(&sql.TxOptions{ReadOnly: true})
-	if tx.Error != nil {
-		_ = db.Close()
-
-		return nil, fmt.Errorf("reading archive %s: %w", path, tx.Error)
-	}
-
-	// The transaction's first read is what takes the snapshot.
-	var tables int
-
-	err = tx.Raw(archiveTableQuery).Row().Scan(&tables)
-	if err != nil {
-		_ = tx.Rollback()
-		_ = db.Close()
-
-		return nil, fmt.Errorf("reading archive %s: %w", path, err)
-	}
-
-	return &ArchiveExport{db: db, tx: tx, empty: tables == 0}, nil
-}
-
-// WriteGzipJSON writes the export to w as one gzipped JSON object:
-// webhook and target, each an id and a name; exported_at; and
-// archived_events, one object per archived row, keyed by column name.
-// A body that is not valid UTF-8 cannot be a JSON string, so it is
-// written in base64, with "body_encoding": "base64" beside it.
-//
-// Each row is written out before the next is read, so neither the
-// archive nor its JSON is ever held in memory whole. After an error
-// the gzip stream is left unfinished, so what was written does not
-// decompress as a whole file.
-func (x *ArchiveExport) WriteGzipJSON(
-	ctx context.Context,
-	w io.Writer,
-	webhook *database.Webhook,
-	target *database.Target,
-	exportedAt time.Time,
-) error {
-	head, err := json.Marshal(map[string]any{
-		"webhook":     exportedName{ID: webhook.ID, Name: webhook.Name},
-		"target":      exportedName{ID: target.ID, Name: target.Name},
-		"exported_at": exportedAt.UTC(),
-	})
-	if err != nil {
-		return fmt.Errorf("encoding archive export: %w", err)
-	}
-
-	zw := gzip.NewWriter(w)
-
-	err = x.writeJSON(ctx, zw, head)
-	if err != nil {
-		return fmt.Errorf("writing archive export: %w", err)
-	}
-
-	return zw.Close()
-}
-
-// Close ends the export's transaction and closes its connection.
-func (x *ArchiveExport) Close() error {
-	if x.db == nil {
-		return nil
-	}
-
-	_ = x.tx.Rollback()
-
-	return x.db.Close()
-}
-
-// writeJSON writes head with archived_events added as its last key,
-// the rows going into it one at a time.
-func (x *ArchiveExport) writeJSON(
-	ctx context.Context, w io.Writer, head []byte,
-) error {
-	// head goes out without its closing brace, so that
-	// archived_events can follow it.
-	_, err := w.Write(head[:len(head)-1])
-	if err != nil {
-		return err
-	}
-
-	_, err = io.WriteString(w, `,"archived_events":[`)
-	if err != nil {
-		return err
-	}
-
-	err = x.writeRows(ctx, w)
-	if err != nil {
-		return err
-	}
-
-	_, err = io.WriteString(w, "\n]}\n")
-
-	return err
-}
-
-// writeRows writes each archived row to w, oldest first, one per line,
-// separated by commas.
-func (x *ArchiveExport) writeRows(ctx context.Context, w io.Writer) error {
-	if x.empty {
-		return nil
-	}
-
-	rows, err := x.tx.WithContext(ctx).
-		Model(&archivedEvent{}).Order("id").Rows()
-	if err != nil {
-		return err
-	}
-
-	defer func() { _ = rows.Close() }()
-
-	for sep := "\n"; rows.Next(); sep = ",\n" {
-		var ev archivedEvent
-
-		err = x.tx.ScanRows(rows, &ev)
-		if err != nil {
-			return err
-		}
-
-		_, err = io.WriteString(w, sep)
-		if err != nil {
-			return err
-		}
-
-		err = writeRow(w, &ev)
-		if err != nil {
-			return err
-		}
-	}
-
-	return rows.Err()
-}
-
-// writeRow writes an archived row to w as a JSON object keyed by
-// column name, its body in base64 when it is not valid UTF-8.
-func writeRow(w io.Writer, ev *archivedEvent) error {
-	row := map[string]any{
-		"id":            ev.ID,
-		"event_id":      ev.EventID,
-		"webhook_id":    ev.WebhookID,
-		"entrypoint_id": ev.EntrypointID,
-		"method":        ev.Method,
-		"headers":       ev.Headers,
-		"body":          ev.Body,
-		"content_type":  ev.ContentType,
-		"archived_at":   ev.ArchivedAt.UTC(),
-	}
-
-	if !utf8.ValidString(ev.Body) {
-		row["body"] = base64.StdEncoding.EncodeToString([]byte(ev.Body))
-		row["body_encoding"] = "base64"
-	}
-
-	line, err := json.Marshal(row)
-	if err != nil {
-		return err
-	}
-
-	_, err = w.Write(line)
-
-	return err
-}

internal/delivery/target_database_export_test.go

@@ -1,388 +0,0 @@
-package delivery_test
-
-import (
-	"bytes"
-	"compress/gzip"
-	"crypto/rand"
-	"encoding/base64"
-	"encoding/json"
-	"fmt"
-	"io"
-	"os"
-	"path/filepath"
-	"runtime"
-	"runtime/debug"
-	"testing"
-	"time"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/webhooker/internal/database"
-	"sneak.berlin/go/webhooker/internal/delivery"
-)
-
-// The webhook and the target the export tests' archives belong to.
-const (
-	exportWebhookID   = "wh-export"
-	exportWebhookName = "Orders (EU)"
-	exportTargetID    = "tgt-export"
-	exportTargetName  = "Long-term archive"
-)
-
-const (
-	// binaryBody is a body that is not valid UTF-8.
-	binaryBody = "\xff\xfe\x00\x01binary\x80"
-
-	// openedEventID is the event the snapshot tests archive before
-	// they open the export.
-	openedEventID = "opened"
-)
-
-// writeExportTo writes export to w as the archive of the export tests'
-// webhook and target, exported at 2026-10-02T12:03:04Z.
-func writeExportTo(
-	t *testing.T, export *delivery.ArchiveExport, w io.Writer,
-) error {
-	t.Helper()
-
-	return export.WriteGzipJSON(
-		t.Context(), w,
-		&database.Webhook{
-			BaseModel: database.BaseModel{ID: exportWebhookID},
-			Name:      exportWebhookName,
-		},
-		&database.Target{
-			BaseModel: database.BaseModel{ID: exportTargetID},
-			Name:      exportTargetName,
-		},
-		time.Date(2026, 10, 2, 12, 3, 4, 0, time.UTC),
-	)
-}
-
-// exportArchive runs a whole export of the archive at path and returns
-// its JSON, decompressed and parsed.
-func exportArchive(t *testing.T, path string) map[string]any {
-	t.Helper()
-
-	export, err := delivery.OpenArchiveExport(
-		t.Context(), path, archiveTestLogger(),
-	)
-	require.NoError(t, err)
-
-	defer func() { require.NoError(t, export.Close()) }()
-
-	return writeExport(t, export)
-}
-
-// writeExport writes an opened export and returns its JSON,
-// decompressed and parsed. Reading to the end makes the gzip reader
-// check that the stream was finished.
-func writeExport(
-	t *testing.T, export *delivery.ArchiveExport,
-) map[string]any {
-	t.Helper()
-
-	var buf bytes.Buffer
-
-	require.NoError(t, writeExportTo(t, export, &buf))
-
-	zr, err := gzip.NewReader(&buf)
-	require.NoError(t, err)
-
-	raw, err := io.ReadAll(zr)
-	require.NoError(t, err)
-
-	var got map[string]any
-
-	require.NoError(t, json.Unmarshal(raw, &got))
-
-	return got
-}
-
-// exportedEvents returns an export's archived_events.
-func exportedEvents(t *testing.T, got map[string]any) []map[string]any {
-	t.Helper()
-
-	list, ok := got["archived_events"].([]any)
-	require.True(t, ok, "archived_events must be an array: %v", got)
-
-	events := make([]map[string]any, len(list))
-
-	for i, v := range list {
-		events[i], ok = v.(map[string]any)
-		require.True(t, ok, "an archived event must be an object: %v", v)
-	}
-
-	return events
-}
-
-// exportedEventIDs returns the event_id of each of an export's
-// archived_events.
-func exportedEventIDs(t *testing.T, got map[string]any) []string {
-	t.Helper()
-
-	events := exportedEvents(t, got)
-	ids := make([]string, 0, len(events))
-
-	for _, ev := range events {
-		ids = append(ids, fmt.Sprint(ev["event_id"]))
-	}
-
-	return ids
-}
-
-// TestArchiveExport_MatchesStoredRows proves an export holds the
-// webhook, the target, the time, and every column of every stored
-// row: a body that is valid UTF-8 as a string, and one that is not in
-// base64, marked as such.
-func TestArchiveExport_MatchesStoredRows(t *testing.T) {
-	t.Parallel()
-
-	path := filepath.Join(t.TempDir(), "archive.db")
-	w := delivery.NewExportArchiveWriter(path, archiveTestLogger(), 0)
-	bodies := []string{`{"order":1}`, "plain text", "", binaryBody}
-
-	for i, body := range bodies {
-		require.NoError(t, w.Write(delivery.ExportArchivedEvent{
-			EventID:      fmt.Sprintf("ev-%d", i),
-			WebhookID:    exportWebhookID,
-			EntrypointID: "ep-1",
-			Method:       "POST",
-			Headers:      `{"X-Test":["yes"]}`,
-			Body:         body,
-			ContentType:  testContentType,
-		}, 0))
-	}
-
-	var stored []delivery.ExportArchivedEvent
-
-	require.NoError(t, openArchiveDBForRead(t, path).
-		Order("id").Find(&stored).Error)
-
-	got := exportArchive(t, path)
-
-	assert.Equal(t,
-		map[string]any{"id": exportWebhookID, "name": exportWebhookName},
-		got["webhook"],
-	)
-	assert.Equal(t,
-		map[string]any{"id": exportTargetID, "name": exportTargetName},
-		got["target"],
-	)
-	assert.Equal(t, "2026-10-02T12:03:04Z", got["exported_at"])
-
-	events := exportedEvents(t, got)
-	require.Len(t, events, len(bodies))
-
-	for i, row := range stored {
-		assertExportedRow(t, row, events[i])
-	}
-}
-
-// assertExportedRow checks that ev, from an export, holds every column
-// of the stored row.
-func assertExportedRow(
-	t *testing.T, row delivery.ExportArchivedEvent, ev map[string]any,
-) {
-	t.Helper()
-
-	archivedAt, err := time.Parse(
-		time.RFC3339Nano, fmt.Sprint(ev["archived_at"]),
-	)
-	require.NoError(t, err)
-	assert.True(t, archivedAt.Equal(row.ArchivedAt))
-
-	assert.EqualValues(t, row.ID, ev["id"])
-	assert.Equal(t, row.EventID, ev["event_id"])
-	assert.Equal(t, row.WebhookID, ev["webhook_id"])
-	assert.Equal(t, row.EntrypointID, ev["entrypoint_id"])
-	assert.Equal(t, row.Method, ev["method"])
-	assert.Equal(t, row.Headers, ev["headers"])
-	assert.Equal(t, row.ContentType, ev["content_type"])
-
-	if row.Body != binaryBody {
-		assert.Equal(t, row.Body, ev["body"])
-		assert.Len(t, ev, 9, "the nine columns and nothing else: %v", ev)
-
-		return
-	}
-
-	body, err := base64.StdEncoding.DecodeString(fmt.Sprint(ev["body"]))
-	require.NoError(t, err)
-	assert.Equal(t, binaryBody, string(body))
-	assert.Equal(t, "base64", ev["body_encoding"])
-	assert.Len(t, ev, 10, "the nine columns and body_encoding: %v", ev)
-}
-
-// TestArchiveExport_Empty proves an archive with nothing in it exports
-// as an empty archived_events: no file, which the export must not
-// create; a file the archive writer has not yet put its table in; and
-// a table with no rows.
-func TestArchiveExport_Empty(t *testing.T) {
-	t.Parallel()
-
-	dir := t.TempDir()
-	missing := filepath.Join(dir, "missing.db")
-	noTable := filepath.Join(dir, "no-table.db")
-	noRows := filepath.Join(dir, "no-rows.db")
-
-	require.NoError(t, os.WriteFile(noTable, nil, 0o600))
-	require.NoError(t,
-		delivery.NewExportArchiveWriter(noRows, archiveTestLogger(), 0).
-			Open(0),
-	)
-
-	for _, path := range []string{missing, noTable, noRows} {
-		assert.Empty(t, exportedEvents(t, exportArchive(t, path)), path)
-	}
-
-	for _, suffix := range archiveFileSuffixes() {
-		assert.NoFileExists(t, missing+suffix)
-	}
-}
-
-// TestArchiveExport_ReadsOneSnapshot proves an export writes the
-// archive as it was when it was opened, and holds up no archive
-// write: a row written while the export is open is stored, and is not
-// in the export. A write held up for the whole busy timeout would
-// fail.
-func TestArchiveExport_ReadsOneSnapshot(t *testing.T) {
-	t.Parallel()
-
-	path := filepath.Join(t.TempDir(), "archive.db")
-	w := delivery.NewExportArchiveWriter(path, archiveTestLogger(), 0)
-
-	require.NoError(t, w.Write(delivery.ExportArchivedEvent{EventID: openedEventID}, 0))
-
-	export, err := delivery.OpenArchiveExport(
-		t.Context(), path, archiveTestLogger(),
-	)
-	require.NoError(t, err)
-
-	defer func() { require.NoError(t, export.Close()) }()
-
-	require.NoError(t, w.Write(delivery.ExportArchivedEvent{EventID: "during"}, 0))
-
-	assert.Equal(t,
-		[]string{openedEventID}, exportedEventIDs(t, writeExport(t, export)),
-	)
-
-	var stored int64
-
-	require.NoError(t, openArchiveDBForRead(t, path).
-		Model(&delivery.ExportArchivedEvent{}).Count(&stored).Error)
-	assert.Equal(t, int64(2), stored)
-}
-
-// TestArchiveExport_SurvivesRename proves that renaming the archive
-// while an export of it is open, as renaming its webhook or target
-// does, leaves the export reading the same file.
-func TestArchiveExport_SurvivesRename(t *testing.T) {
-	t.Parallel()
-
-	path := filepath.Join(t.TempDir(), "archive-old.db")
-	w := delivery.NewExportArchiveWriter(path, archiveTestLogger(), 0)
-
-	require.NoError(t, w.Write(delivery.ExportArchivedEvent{EventID: openedEventID}, 0))
-
-	export, err := delivery.OpenArchiveExport(
-		t.Context(), path, archiveTestLogger(),
-	)
-	require.NoError(t, err)
-
-	defer func() { require.NoError(t, export.Close()) }()
-
-	require.NoError(t, w.Rename("archive-new.db"))
-	require.NoError(t, w.Write(delivery.ExportArchivedEvent{EventID: "after"}, 0))
-	require.NoFileExists(t, path)
-
-	assert.Equal(t,
-		[]string{openedEventID}, exportedEventIDs(t, writeExport(t, export)),
-	)
-}
-
-// heapPeak is an io.Writer that discards what it is given and records
-// the largest heap it saw at a write.
-type heapPeak struct {
-	max uint64
-}
-
-func (p *heapPeak) Write(b []byte) (int, error) {
-	var m runtime.MemStats
-
-	runtime.ReadMemStats(&m)
-	p.max = max(p.max, m.HeapAlloc)
-
-	return len(b), nil
-}
-
-// TestArchiveExport_Streams proves an export holds neither the archive
-// nor its output in memory whole: exporting a 16 MiB archive grows the
-// heap by less than half of that. The bodies are random bytes in
-// base64, which gzip shrinks by only a quarter, so an export that read
-// every row before writing, or built the JSON or the gzipped file
-// before writing it, would hold at least 12 MiB at a write.
-//
-//nolint:paralleltest // It measures the heap, which tests share.
-func TestArchiveExport_Streams(t *testing.T) {
-	const (
-		rows     = 64
-		bodySize = 256 << 10
-		limit    = rows * bodySize / 2
-	)
-
-	path := filepath.Join(t.TempDir(), "archive.db")
-	w := delivery.NewExportArchiveWriter(path, archiveTestLogger(), 0)
-
-	// Base64 makes four characters of every three bytes.
-	random := make([]byte, bodySize/4*3)
-
-	for range rows {
-		_, _ = rand.Read(random)
-
-		require.NoError(t, w.Write(delivery.ExportArchivedEvent{
-			Body: base64.StdEncoding.EncodeToString(random),
-		}, 0))
-	}
-
-	export, err := delivery.OpenArchiveExport(
-		t.Context(), path, archiveTestLogger(),
-	)
-	require.NoError(t, err)
-
-	defer func() { require.NoError(t, export.Close()) }()
-
-	// A low GC target collects garbage soon after it is made, so the
-	// heap at each write is close to what the export is holding.
-	defer debug.SetGCPercent(debug.SetGCPercent(10))
-
-	runtime.GC()
-
-	var start runtime.MemStats
-
-	runtime.ReadMemStats(&start)
-
-	peak := &heapPeak{}
-
-	require.NoError(t, writeExportTo(t, export, peak))
-	assert.Less(t, peak.max, start.HeapAlloc+limit,
-		"heap at the start %d, at its peak %d", start.HeapAlloc, peak.max,
-	)
-}
-
-// TestArchiveExportFileName proves the download is named for the
-// webhook and the target, with the names made safe as for the archive
-// file, and the export time in UTC.
-func TestArchiveExportFileName(t *testing.T) {
-	t.Parallel()
-
-	cest := time.FixedZone("CEST", int((2 * time.Hour).Seconds()))
-
-	assert.Equal(t,
-		"archive-orders-eu-long-term-archive-20261002T120304Z.json.gz",
-		delivery.ArchiveExportFileName(
-			exportWebhookName, exportTargetName,
-			time.Date(2026, 10, 2, 14, 3, 4, 0, cest),
-		),
-	)
-}

internal/gormlog/firstboot_test.go

@@ -117,8 +117,8 @@ func readFirstBootSecrets(
 }
 
 // bootAtDebug starts and stops the real application graph against
-// dataDir with DEBUG=true, and returns everything it wrote to standard
-// output.
+// dataDir with DEBUG=true and nothing else set, and returns everything
+// it wrote to standard output.
 //
 // config.New reads DEBUG from the environment exactly as the binary
 // does, internal/logger builds the handler it builds in production,
@@ -128,6 +128,7 @@ func readFirstBootSecrets(
 func bootAtDebug(t *testing.T, dataDir string) string {
 	t.Helper()
 
+	config.ClearEnvForTest(t)
 	t.Setenv("DEBUG", "true")
 	t.Setenv("DATA_DIR", dataDir)
 

internal/handlers/handlers.go

@@ -97,8 +97,7 @@ type Handlers struct {
 	// names through the archive rename, the save and any move back.
 	// Interleaved, one could rename an archive between another's
 	// rename and save, leaving the file named for one edit and the
-	// stored names from the other. An archive download holds it while
-	// it reads the stored names and opens the file they give.
+	// stored names from the other.
 	renameMu sync.Mutex
 
 	// dummyVerifications counts the equivalent-cost verifications

internal/handlers/settings.go

@@ -77,7 +77,8 @@ func settingRows(cfg *config.Config) []settingRow {
 		{
 			"RETENTION_SWEEP_INTERVAL",
 			"How often the retention reaper and archive sweeper run " +
-				"(Go duration, must be positive)",
+				"(Go duration, must be positive). A value that does " +
+				"not parse, or is zero or negative, fails startup",
 			cfg.RetentionSweepInterval.String(),
 		},
 		{

internal/handlers/source_management.go

@@ -41,51 +41,71 @@ type WebhookListItem struct {
 // errMissingURL signals that a required URL was not provided.
 var errMissingURL = errors.New("missing URL")
 
-// parseRetentionDays interprets a retention_days form value. It
-// returns the number of days, or, for a value it refuses, the message
-// the create and edit forms show; the message is empty when the value
-// is accepted.
+// errInvalidRetention signals a retention_days form value that is not
+// a non-negative whole number.
+var errInvalidRetention = errors.New("invalid retention days")
+
+// errRetentionTooLarge signals a retention_days form value that is a
+// whole number but larger than the reaper's cutoff arithmetic can
+// represent. It is distinguished from errInvalidRetention so the form
+// can tell the user the actual ceiling instead of implying their input
+// was not a number.
+var errRetentionTooLarge = errors.New("retention days out of range")
+
+// retentionErrorMessage returns the message the create and edit forms
+// show the user for a rejected retention_days value. Any error other
+// than errRetentionTooLarge falls back to the generic wording, so an
+// unrecognised parse failure still produces a sensible 400 rather than
+// an empty alert.
+func retentionErrorMessage(err error) string {
+	if errors.Is(err, errRetentionTooLarge) {
+		return "Retention must be at most " +
+			strconv.Itoa(database.MaxFiniteRetentionDays) +
+			" days, or 0 to retain events forever."
+	}
+
+	return "Retention must be a whole number of days, or 0 to " +
+		"retain events forever."
+}
+
+// parseRetentionDays interprets a retention_days form value.
 //
 // An empty value yields fallback, which lets the create path apply the
 // default and the edit path leave the stored value unchanged. A value
 // of 0 is returned as 0 and is rewritten to the retain-forever
 // sentinel by database.Webhook's BeforeSave hook. Anything unparseable
-// or negative is refused rather than silently given a default.
+// or negative is an error rather than a silently substituted default.
 //
 // The upper bound is not cosmetic. The reaper computes its cutoff as a
 // time.Duration, an int64 nanosecond count, so a day count above
 // database.MaxFiniteRetentionDays overflows, puts the cutoff in the
 // future, and deletes every event the webhook has. A finite value
-// above that ceiling is therefore refused, and the message names the
-// ceiling rather than implying the input was not a number.
+// above that ceiling is therefore a 400.
 //
 // A value at or above the retain-forever sentinel is not out of range:
 // it is what the edit form pre-fills for a retain-forever webhook, so
 // submitting the form back unchanged has to keep meaning "forever"
 // rather than being rejected.
-func parseRetentionDays(raw string, fallback int) (int, string) {
+func parseRetentionDays(raw string, fallback int) (int, error) {
 	raw = strings.TrimSpace(raw)
 	if raw == "" {
-		return fallback, ""
+		return fallback, nil
 	}
 
 	v, err := strconv.Atoi(raw)
 	if err != nil || v < 0 {
-		return 0, "Retention must be a whole number of days, or 0 to " +
-			"retain events forever."
+		return 0, errInvalidRetention
 	}
 
 	if v >= database.RetentionForeverDays {
-		return database.RetentionForeverDays, ""
+		return database.RetentionForeverDays, nil
 	}
 
 	if v > database.MaxFiniteRetentionDays {
-		return 0, "Retention must be at most " +
-			strconv.Itoa(database.MaxFiniteRetentionDays) +
-			" days, or 0 to retain events forever."
+		return 0, errRetentionTooLarge
 	}
 
-	return v, ""
+	return v, nil
 }
 
 // DeliveryView is the display-safe projection of a delivery
@@ -341,13 +361,16 @@ func (h *Handlers) HandleSourceCreateSubmit() http.HandlerFunc {
 			return
 		}
 
-		retentionDays, errMsg := parseRetentionDays(
+		retentionDays, retErr := parseRetentionDays(
 			retentionStr, database.DefaultRetentionDays,
 		)
-		if errMsg != "" {
+		if retErr != nil {
 			h.renderTemplateStatus(
 				w, r, "sources_new.html",
-				newSourceFormData(errMsg, name, description),
+				newSourceFormData(
+					retentionErrorMessage(retErr),
+					name, description,
+				),
 				http.StatusBadRequest,
 			)
 
@@ -632,13 +655,13 @@ func (h *Handlers) applyWebhookEdit(
 
 	// An empty field falls back to the stored value, so submitting the
 	// form without touching retention leaves the policy alone.
-	retentionDays, errMsg := parseRetentionDays(
+	retentionDays, retErr := parseRetentionDays(
 		r.PostFormValue("retention_days"), webhook.RetentionDays,
 	)
-	if errMsg != "" {
+	if retErr != nil {
 		data := map[string]any{
 			tmplKeyWebhook: webhook,
-			tmplKeyError:   errMsg,
+			tmplKeyError:   retentionErrorMessage(retErr),
 		}
 
 		h.renderTemplateStatus(w, r, "source_edit.html", data, http.StatusBadRequest)

internal/handlers/source_management_test.go

@@ -191,7 +191,6 @@ func storedRetentionDays(
 type sourceTestEnv struct {
 	handlers *handlers.Handlers
 	db       *database.Database
-	dbMgr    *database.WebhookDBManager
 	archives *recordingArchives
 	cookies  []*http.Cookie
 }
@@ -205,11 +204,9 @@ func setupSourceTest(t *testing.T) *sourceTestEnv {
 
 	var db *database.Database
 
-	var dbMgr *database.WebhookDBManager
-
 	var archives *recordingArchives
 
-	app := newTestApp(t, &h, &sess, &db, &dbMgr, &archives)
+	app := newTestApp(t, &h, &sess, &db, &archives)
 	app.RequireStart()
 
 	t.Cleanup(app.RequireStop)
@@ -217,7 +214,6 @@ func setupSourceTest(t *testing.T) *sourceTestEnv {
 	return &sourceTestEnv{
 		handlers: h,
 		db:       db,
-		dbMgr:    dbMgr,
 		archives: archives,
 		cookies: authenticatedCookies(
 			t, sess, sourceTestUserID, "sourceuser",
@@ -372,42 +368,31 @@ func TestHandleSourceCreateSubmit_OverflowingRetentionIsRejected(
 // boundary between "too large to represent" and "retain forever": the
 // sentinel is above MaxFiniteRetentionDays, but it is the value the
 // edit form pre-fills, so it must be accepted rather than rejected as
-// out of range. A value above the sentinel is stored as the sentinel.
+// out of range.
 func TestHandleSourceCreateSubmit_SentinelIsAcceptedAsForever(
 	t *testing.T,
 ) {
 	t.Parallel()
 
-	for _, days := range []int{
+	env := setupSourceTest(t)
+	sentinel := strconv.Itoa(database.RetentionForeverDays)
+
+	w := submitCreate(t, env.handlers, env.cookies, "forever", &sentinel)
+	require.Equal(t, http.StatusSeeOther, w.Code)
+
+	wh := onlyWebhook(t, env.db)
+	assert.Equal(
+		t,
 		database.RetentionForeverDays,
-		database.RetentionForeverDays + 1,
-	} {
-		raw := strconv.Itoa(days)
-
-		t.Run(raw, func(t *testing.T) {
-			t.Parallel()
-
-			env := setupSourceTest(t)
-
-			w := submitCreate(t, env.handlers, env.cookies, "forever", &raw)
-			require.Equal(t, http.StatusSeeOther, w.Code)
-
-			wh := onlyWebhook(t, env.db)
-			assert.Equal(
-				t,
-				database.RetentionForeverDays,
-				storedRetentionDays(t, env.db, wh.ID),
-			)
-		})
-	}
+		storedRetentionDays(t, env.db, wh.ID),
+	)
 }
 
 // TestHandleSourceCreateSubmit_RejectedFormKeepsUserInput checks that a
 // validation failure hands the user's typing back, matching what the
 // edit form already does. Losing a long description to a mistyped
 // retention value is the kind of thing that makes people give up on a
-// form. Both values carry HTML-special characters, which must come
-// back escaped rather than as markup.
+// form.
 func TestHandleSourceCreateSubmit_RejectedFormKeepsUserInput(
 	t *testing.T,
 ) {
@@ -416,8 +401,8 @@ func TestHandleSourceCreateSubmit_RejectedFormKeepsUserInput(
 	env := setupSourceTest(t)
 
 	const (
-		name        = `kept"><b>name`
-		description = `a </textarea> worth not losing`
+		name        = "kept-name"
+		description = "a description worth not losing"
 	)
 
 	form := url.Values{}
@@ -434,10 +419,8 @@ func TestHandleSourceCreateSubmit_RejectedFormKeepsUserInput(
 
 	body := w.Body.String()
 
-	assert.Contains(t, body, `value="kept&#34;&gt;&lt;b&gt;name"`)
-	assert.Contains(t, body, `a &lt;/textarea&gt; worth not losing`)
-	assert.NotContains(t, body, name)
-	assert.NotContains(t, body, description)
+	assert.Contains(t, body, `value="`+name+`"`)
+	assert.Contains(t, body, description)
 }
 
 // submitEdit posts the webhook edit form for the given webhook.

internal/handlers/target_download.go

@@ -1,121 +0,0 @@
-package handlers
-
-import (
-	"context"
-	"errors"
-	"net/http"
-	"time"
-
-	"sneak.berlin/go/webhooker/internal/database"
-	"sneak.berlin/go/webhooker/internal/delivery"
-)
-
-// downloadWriteTimeout is how long one write of a download may wait
-// for a client that has stopped reading.
-const downloadWriteTimeout = 60 * time.Second
-
-// HandleTargetDownload serves a database target's archive as one
-// gzipped JSON file, named for the webhook, the target and the time;
-// see delivery.ArchiveExport.WriteGzipJSON for what it holds. Other
-// target types have no archive and are a 404.
-//
-// A download runs for as long as the client keeps reading: it reads
-// under a context the request limit does not cancel, and gives each
-// write its own deadline in place of the server's write timeout. It
-// stops when a write fails.
-func (h *Handlers) HandleTargetDownload() http.HandlerFunc {
-	return func(w http.ResponseWriter, r *http.Request) {
-		ctx := context.WithoutCancel(r.Context())
-
-		webhook, target, export, ok := h.openTargetArchive(ctx, w, r)
-		if !ok {
-			return
-		}
-
-		defer func() { _ = export.Close() }()
-
-		now := time.Now()
-
-		w.Header().Set("Content-Type", "application/gzip")
-		w.Header().Set(
-			"Content-Disposition",
-			`attachment; filename="`+delivery.ArchiveExportFileName(
-				webhook.Name, target.Name, now,
-			)+`"`,
-		)
-
-		err := export.WriteGzipJSON(
-			ctx,
-			downloadWriter{w: w, rc: http.NewResponseController(w)},
-			&webhook, target, now,
-		)
-		if err != nil {
-			h.log.Error(
-				"failed to export archive",
-				"target_id", target.ID,
-				"error", err,
-			)
-
-			// The 200 has gone out. Aborting the connection is what
-			// tells the client the file is incomplete.
-			panic(http.ErrAbortHandler)
-		}
-	}
-}
-
-// downloadWriter writes a download to the client, giving each write
-// downloadWriteTimeout to finish.
-type downloadWriter struct {
-	w  http.ResponseWriter
-	rc *http.ResponseController
-}
-
-func (d downloadWriter) Write(b []byte) (int, error) {
-	// A writer that has no write deadline, such as a test's recorder,
-	// answers http.ErrNotSupported and needs none extended.
-	err := d.rc.SetWriteDeadline(time.Now().Add(downloadWriteTimeout))
-	if err != nil && !errors.Is(err, http.ErrNotSupported) {
-		return 0, err
-	}
-
-	return d.w.Write(b)
-}
-
-// openTargetArchive opens the archive of the request's database target
-// for export, with its reads under ctx. It reports false once it has
-// written the response.
-//
-// It holds renameMu, which every archive rename runs under, while it
-// reads the stored names and opens the file, so the file it opens is
-// the one those names give. It lets go before the export is streamed:
-// once the file is open, a rename does not affect the export.
-func (h *Handlers) openTargetArchive(
-	ctx context.Context,
-	w http.ResponseWriter,
-	r *http.Request,
-) (database.Webhook, *database.Target, *delivery.ArchiveExport, bool) {
-	h.renameMu.Lock()
-	defer h.renameMu.Unlock()
-
-	webhook, target, ok := h.ownedTarget(w, r)
-	if !ok {
-		return database.Webhook{}, nil, nil, false
-	}
-
-	if target.Type != database.TargetTypeDatabase {
-		h.renderError(w, r, http.StatusNotFound)
-
-		return database.Webhook{}, nil, nil, false
-	}
-
-	export, err := delivery.OpenArchiveExport(
-		ctx, delivery.ArchivePath(h.dbMgr, &webhook, target), h.log,
-	)
-	if err != nil {
-		h.serverError(w, r, "failed to open archive for export", err)
-
-		return database.Webhook{}, nil, nil, false
-	}
-
-	return webhook, target, export, true
-}

internal/handlers/target_download_test.go

@@ -1,328 +0,0 @@
-package handlers_test
-
-import (
-	"compress/gzip"
-	"crypto/rand"
-	"encoding/json"
-	"errors"
-	"io"
-	"net/http"
-	"net/http/httptest"
-	"net/url"
-	"sync"
-	"testing"
-	"time"
-
-	chimw "github.com/go-chi/chi/middleware"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/webhooker/internal/database"
-	"sneak.berlin/go/webhooker/internal/delivery"
-)
-
-// errClientGone is the write failure of a client that has gone away.
-var errClientGone = errors.New("client gone")
-
-// downloadPath is the archive download route of a target.
-func downloadPath(webhookID, targetID string) string {
-	return "/hook/" + webhookID + "/targets/" + targetID + "/download"
-}
-
-// renameTarget submits the edit form renaming a target to Renamed.
-func renameTarget(
-	env *sourceTestEnv, webhookID, targetID string,
-) *httptest.ResponseRecorder {
-	form := url.Values{}
-	form.Set("name", "Renamed")
-
-	return submitTargetEdit(env, webhookID, targetID, form)
-}
-
-// TestHandleTargetDownload proves a database target's archive
-// downloads as a gzipped JSON attachment named for the webhook, the
-// target and the time, here with no archive file yet, so with no
-// rows; and that a target of another type has no download.
-func TestHandleTargetDownload(t *testing.T) {
-	t.Parallel()
-
-	env := setupSourceTest(t)
-	wh := seedWebhookWithRetention(t, env.db, 7)
-	archive := seedTarget(t, env.db, wh.ID, database.TargetTypeDatabase)
-	logTarget := seedTarget(t, env.db, wh.ID, database.TargetTypeLog)
-
-	w := serveTarget(
-		env, http.MethodGet, downloadPath(wh.ID, archive.ID), nil,
-	)
-	require.Equal(t, http.StatusOK, w.Code, w.Body.String())
-	assert.Equal(t, "application/gzip", w.Header().Get("Content-Type"))
-	assert.Regexp(t,
-		`^attachment; filename="archive-seeded-t-database-`+
-			`\d{8}T\d{6}Z\.json\.gz"$`,
-		w.Header().Get("Content-Disposition"),
-	)
-
-	zr, err := gzip.NewReader(w.Body)
-	require.NoError(t, err)
-
-	var got map[string]json.RawMessage
-
-	require.NoError(t, json.NewDecoder(zr).Decode(&got))
-	assert.JSONEq(t,
-		`{"id":"`+archive.ID+`","name":"t-database"}`,
-		string(got["target"]),
-	)
-	assert.JSONEq(t, `[]`, string(got["archived_events"]))
-
-	w = serveTarget(
-		env, http.MethodGet, downloadPath(wh.ID, logTarget.ID), nil,
-	)
-	assert.Equal(t, http.StatusNotFound, w.Code)
-}
-
-// TestHandleTargetDownload_WaitsForRename proves a download reads the
-// target's names and opens its archive under the lock a rename holds:
-// started while an edit is renaming the archive, it waits, and is
-// named for the target's new name.
-func TestHandleTargetDownload_WaitsForRename(t *testing.T) {
-	t.Parallel()
-
-	env := setupSourceTest(t)
-	wh := seedWebhookWithRetention(t, env.db, 7)
-	archive := seedTarget(t, env.db, wh.ID, database.TargetTypeDatabase)
-
-	renaming, release := env.archives.BlockNextRename()
-	edited := make(chan *httptest.ResponseRecorder, 1)
-
-	go func() {
-		edited <- renameTarget(env, wh.ID, archive.ID)
-	}()
-
-	<-renaming
-
-	downloaded := make(chan *httptest.ResponseRecorder, 1)
-
-	go func() {
-		downloaded <- serveTarget(
-			env, http.MethodGet, downloadPath(wh.ID, archive.ID), nil,
-		)
-	}()
-
-	select {
-	case <-downloaded:
-		release()
-		t.Fatal("the download did not wait for the rename")
-	case <-time.After(100 * time.Millisecond):
-	}
-
-	release()
-	require.Equal(t, http.StatusSeeOther, (<-edited).Code)
-
-	w := <-downloaded
-	require.Equal(t, http.StatusOK, w.Code)
-	assert.Contains(t,
-		w.Header().Get("Content-Disposition"), "archive-seeded-renamed-",
-	)
-}
-
-// stalledWriter is a response writer whose first write waits until
-// resume is closed, closing writing when it starts to wait.
-type stalledWriter struct {
-	*httptest.ResponseRecorder
-
-	once    sync.Once
-	writing chan struct{}
-	resume  chan struct{}
-}
-
-func (s *stalledWriter) Write(b []byte) (int, error) {
-	s.once.Do(func() {
-		close(s.writing)
-		<-s.resume
-	})
-
-	return s.ResponseRecorder.Write(b)
-}
-
-// TestHandleTargetDownload_StreamsWithoutTheLock proves a download
-// lets go of the rename lock once its archive is open: while the
-// download is stalled writing, an edit can still rename the target.
-func TestHandleTargetDownload_StreamsWithoutTheLock(t *testing.T) {
-	t.Parallel()
-
-	env := setupSourceTest(t)
-	wh := seedWebhookWithRetention(t, env.db, 7)
-	archive := seedTarget(t, env.db, wh.ID, database.TargetTypeDatabase)
-
-	req := httptest.NewRequestWithContext(
-		t.Context(), http.MethodGet, downloadPath(wh.ID, archive.ID), nil,
-	)
-	for _, c := range env.cookies {
-		req.AddCookie(c)
-	}
-
-	sw := &stalledWriter{
-		ResponseRecorder: httptest.NewRecorder(),
-		writing:          make(chan struct{}),
-		resume:           make(chan struct{}),
-	}
-	downloaded := make(chan struct{})
-
-	go func() {
-		targetRouter(env).ServeHTTP(sw, req)
-		close(downloaded)
-	}()
-
-	<-sw.writing
-
-	edited := make(chan *httptest.ResponseRecorder, 1)
-
-	go func() {
-		edited <- renameTarget(env, wh.ID, archive.ID)
-	}()
-
-	select {
-	case w := <-edited:
-		assert.Equal(t, http.StatusSeeOther, w.Code)
-	case <-time.After(10 * time.Second):
-		t.Error("the rename waited for the download")
-	}
-
-	close(sw.resume)
-	<-downloaded
-	assert.Equal(t, http.StatusOK, sw.Code)
-}
-
-// seedArchive writes rows to the archive file at path, each with a
-// body of bodySize random bytes, which do not compress. Its table has
-// only the columns the test fills; an export writes the others empty.
-func seedArchive(t *testing.T, path string, rows, bodySize int) {
-	t.Helper()
-
-	db, err := database.OpenSQLite(path, database.SQLiteModeCreate)
-	require.NoError(t, err)
-
-	defer func() { require.NoError(t, db.Close()) }()
-
-	_, err = db.ExecContext(t.Context(),
-		"CREATE TABLE archived_events (id INTEGER PRIMARY KEY, body TEXT)",
-	)
-	require.NoError(t, err)
-
-	body := make([]byte, bodySize)
-
-	for range rows {
-		_, _ = rand.Read(body)
-
-		_, err = db.ExecContext(t.Context(),
-			"INSERT INTO archived_events (body) VALUES (?)", string(body),
-		)
-		require.NoError(t, err)
-	}
-}
-
-// TestHandleTargetDownload_OutlastsTheRequestLimit proves a download
-// runs for as long as the client keeps reading. Behind a request limit
-// and a server write timeout of a tenth of a second, the client stops
-// reading once the response has started, waits three times as long,
-// and still gets the whole file. The archive is larger than a
-// connection holds, so the download is still being written while the
-// client waits.
-func TestHandleTargetDownload_OutlastsTheRequestLimit(t *testing.T) {
-	t.Parallel()
-
-	const (
-		limit    = 100 * time.Millisecond
-		rows     = 12
-		bodySize = 1 << 20
-	)
-
-	env := setupSourceTest(t)
-	wh := seedWebhookWithRetention(t, env.db, 7)
-	archive := seedTarget(t, env.db, wh.ID, database.TargetTypeDatabase)
-	seedArchive(
-		t, delivery.ArchivePath(env.dbMgr, &wh, archive), rows, bodySize,
-	)
-
-	srv := httptest.NewUnstartedServer(
-		chimw.Timeout(limit)(targetRouter(env)),
-	)
-	srv.Config.WriteTimeout = limit
-	srv.Start()
-	t.Cleanup(srv.Close)
-
-	req, err := http.NewRequestWithContext(
-		t.Context(), http.MethodGet,
-		srv.URL+downloadPath(wh.ID, archive.ID), nil,
-	)
-	require.NoError(t, err)
-
-	for _, c := range env.cookies {
-		req.AddCookie(c)
-	}
-
-	resp, err := srv.Client().Do(req)
-	require.NoError(t, err)
-
-	defer func() { _ = resp.Body.Close() }()
-
-	require.Equal(t, http.StatusOK, resp.StatusCode)
-
-	time.Sleep(3 * limit)
-
-	zr, err := gzip.NewReader(resp.Body)
-	require.NoError(t, err)
-
-	var (
-		got    map[string]json.RawMessage
-		events []json.RawMessage
-	)
-
-	require.NoError(t, json.NewDecoder(zr).Decode(&got))
-	require.NoError(t, json.Unmarshal(got["archived_events"], &events))
-	assert.Len(t, events, rows)
-
-	// Reading to the end makes the gzip reader check that the file was
-	// finished.
-	_, err = io.ReadAll(zr)
-	require.NoError(t, err)
-}
-
-// brokenWriter is a response writer whose writes fail once the
-// response has started, as they do when the client goes away.
-type brokenWriter struct {
-	*httptest.ResponseRecorder
-}
-
-func (b brokenWriter) Write(p []byte) (int, error) {
-	if b.Body.Len() > 0 {
-		return 0, errClientGone
-	}
-
-	return b.ResponseRecorder.Write(p)
-}
-
-// TestHandleTargetDownload_AbortsWhenItFails proves a download that
-// fails after its response has started aborts the connection, so the
-// client sees a failed download rather than a file that looks
-// complete and does not decompress.
-func TestHandleTargetDownload_AbortsWhenItFails(t *testing.T) {
-	t.Parallel()
-
-	env := setupSourceTest(t)
-	wh := seedWebhookWithRetention(t, env.db, 7)
-	archive := seedTarget(t, env.db, wh.ID, database.TargetTypeDatabase)
-
-	req := httptest.NewRequestWithContext(
-		t.Context(), http.MethodGet, downloadPath(wh.ID, archive.ID), nil,
-	)
-	for _, c := range env.cookies {
-		req.AddCookie(c)
-	}
-
-	w := brokenWriter{ResponseRecorder: httptest.NewRecorder()}
-
-	assert.PanicsWithValue(t, http.ErrAbortHandler, func() {
-		targetRouter(env).ServeHTTP(w, req)
-	})
-	assert.Equal(t, http.StatusOK, w.Code)
-}

internal/handlers/target_edit_test.go

@@ -37,18 +37,14 @@ const (
 	editAuthHeader   = "Authorization: Bearer " + editBearerSecret
 )
 
-// targetRouter mounts the target create, edit and download routes on
-// a chi router so the handlers see the URL parameters they read.
+// targetRouter mounts the target create and edit routes on a chi
+// router so the handlers see the URL parameters they read.
 func targetRouter(env *sourceTestEnv) *chi.Mux {
 	router := chi.NewRouter()
 	router.Post(
 		"/hook/{sourceID}/targets",
 		env.handlers.HandleTargetCreate(),
 	)
-	router.Get(
-		"/hook/{sourceID}/targets/{targetID}/download",
-		env.handlers.HandleTargetDownload(),
-	)
 	router.Get(
 		"/hook/{sourceID}/targets/{targetID}/edit",
 		env.handlers.HandleTargetEdit(),

internal/handlers/webhook.go

@@ -11,7 +11,6 @@ import (
 	"sneak.berlin/go/webhooker/internal/database"
 	"sneak.berlin/go/webhooker/internal/delivery"
 	"sneak.berlin/go/webhooker/internal/logfield"
-	"sneak.berlin/go/webhooker/internal/middleware"
 )
 
 const (
@@ -58,8 +57,7 @@ func (h *Handlers) HandleWebhook() http.HandlerFunc {
 		h.log.Info("webhook request received",
 			"entrypoint_uuid", entrypointUUID,
 			"method", r.Method,
-			"remoteIP", middleware.RemoteIP(r),
-			"clientIP", middleware.ClientIP(r),
+			"remote_addr", r.RemoteAddr,
 		)
 
 		if !entrypoint.Active {

internal/handlers/webhook_test.go

@@ -1,124 +0,0 @@
-package handlers_test
-
-import (
-	"bytes"
-	"context"
-	"encoding/json"
-	"log/slog"
-	"net/http"
-	"net/http/httptest"
-	"net/netip"
-	"strings"
-	"testing"
-
-	"github.com/go-chi/chi"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/webhooker/internal/config"
-	"sneak.berlin/go/webhooker/internal/database"
-	"sneak.berlin/go/webhooker/internal/handlers"
-	"sneak.berlin/go/webhooker/internal/middleware"
-)
-
-// TestHandleWebhook_LogsClientNextToThePeer checks that the
-// receiver's "webhook request received" line carries both addresses:
-// remoteIP, the connecting peer, and clientIP, the client the access
-// log attributes the request to.
-func TestHandleWebhook_LogsClientNextToThePeer(t *testing.T) {
-	t.Parallel()
-
-	// untrustedPeer is outside the trusted 10.0.0.0/8, so its
-	// X-Forwarded-For is ignored and it is the client.
-	const untrustedPeer = "192.0.2.10"
-
-	cases := map[string]struct {
-		peer       string
-		wantRemote string
-		wantClient string
-	}{
-		"trusted proxy with a forwarded chain": {
-			peer:       "10.0.0.1:44444",
-			wantRemote: "10.0.0.1",
-			wantClient: "198.51.100.7",
-		},
-		"untrusted peer": {
-			peer:       untrustedPeer + ":5555",
-			wantRemote: untrustedPeer,
-			wantClient: untrustedPeer,
-		},
-	}
-
-	for name, tc := range cases {
-		t.Run(name, func(t *testing.T) {
-			t.Parallel()
-
-			var (
-				h  *handlers.Handlers
-				mw *middleware.Middleware
-				db *database.Database
-			)
-
-			app := newTestAppWithConfig(t, &config.Config{
-				DataDir: t.TempDir(),
-				TrustedProxies: []netip.Prefix{
-					netip.MustParsePrefix("10.0.0.0/8"),
-				},
-			}, &h, &mw, &db)
-			app.RequireStart()
-
-			t.Cleanup(app.RequireStop)
-
-			buf := new(bytes.Buffer)
-			h.SetLogForTest(slog.New(slog.NewJSONHandler(buf, nil)))
-
-			webhook := seedWebhook(t, db)
-			seedEntrypoint(t, db, webhook.ID)
-
-			// Logging is what works the client address out, so the
-			// request goes through it as it does in production.
-			router := chi.NewRouter()
-			router.Use(mw.Logging())
-			router.Post("/h/{uuid}", h.HandleWebhook())
-
-			req := httptest.NewRequestWithContext(
-				context.Background(), http.MethodPost,
-				"/h/ep-"+webhook.ID, strings.NewReader("{}"),
-			)
-			req.RemoteAddr = tc.peer
-			req.Header.Set("X-Forwarded-For", "198.51.100.7, 10.0.0.2")
-
-			w := httptest.NewRecorder()
-			router.ServeHTTP(w, req)
-
-			require.Equal(t, http.StatusOK, w.Code)
-
-			line := receivedLine(t, buf)
-			assert.Equal(t, tc.wantRemote, line["remoteIP"])
-			assert.Equal(t, tc.wantClient, line["clientIP"])
-		})
-	}
-}
-
-// receivedLine returns the one "webhook request received" line in the
-// captured JSON log.
-func receivedLine(t *testing.T, buf *bytes.Buffer) map[string]any {
-	t.Helper()
-
-	var found []map[string]any
-
-	for line := range strings.SplitSeq(
-		strings.TrimSpace(buf.String()), "\n",
-	) {
-		var entry map[string]any
-
-		require.NoError(t, json.Unmarshal([]byte(line), &entry))
-
-		if entry["msg"] == "webhook request received" {
-			found = append(found, entry)
-		}
-	}
-
-	require.Len(t, found, 1)
-
-	return found[0]
-}

internal/middleware/accesslog_test.go

@@ -63,12 +63,6 @@ const (
 // capturingMiddleware returns a Middleware whose logger writes JSON
 // lines into the returned buffer, so the access log can be asserted
 // on directly.
-//
-// It trusts 192.0.2.1, the peer address httptest.NewRequestWithContext
-// gives a request, as a proxy, the way a deployment trusts its reverse
-// proxy: a request built that way and carrying X-Forwarded-For is
-// logged with the client that header names as clientIP, and one
-// without it with the peer.
 func capturingMiddleware(t *testing.T) (*middleware.Middleware, *bytes.Buffer) {
 	t.Helper()
 
@@ -78,10 +72,7 @@ func capturingMiddleware(t *testing.T) (*middleware.Middleware, *bytes.Buffer) {
 		&slog.HandlerOptions{Level: slog.LevelInfo},
 	))
 
-	cfg := &config.Config{
-		Environment:    config.EnvironmentDev,
-		TrustedProxies: trustedProxies("192.0.2.1/32"),
-	}
+	cfg := &config.Config{Environment: config.EnvironmentDev}
 
 	return middleware.NewForTest(log, cfg, nil), buf
 }
@@ -90,7 +81,7 @@ func capturingMiddleware(t *testing.T) (*middleware.Middleware, *bytes.Buffer) {
 // internal/logger can select: slog's text handler, which
 // internal/logger/logger.go installs when stderr is a tty. It escapes
 // differently from the JSON one, so the line bound has to be asserted
-// against both. It trusts the same peer.
+// against both.
 func capturingTextMiddleware(
 	t *testing.T,
 ) (*middleware.Middleware, *bytes.Buffer) {
@@ -102,10 +93,7 @@ func capturingTextMiddleware(
 		&slog.HandlerOptions{Level: slog.LevelInfo},
 	))
 
-	cfg := &config.Config{
-		Environment:    config.EnvironmentDev,
-		TrustedProxies: trustedProxies("192.0.2.1/32"),
-	}
+	cfg := &config.Config{Environment: config.EnvironmentDev}
 
 	return middleware.NewForTest(log, cfg, nil), buf
 }
@@ -346,12 +334,11 @@ func oversizedHeaders(value string) map[string]string {
 // sizeCase is one way of pointing 8 KB of client-chosen text at the
 // access log.
 type sizeCase struct {
-	target       string
-	headers      map[string]string
-	wantStatus   int
-	wantURL      string
-	wantClientIP string
-	bound        int
+	target     string
+	headers    map[string]string
+	wantStatus int
+	wantURL    string
+	bound      int
 }
 
 // lineSizeCases enumerates every part of a request that reaches the
@@ -388,7 +375,8 @@ func lineSizeCases() map[string]sizeCase {
 	}
 
 	// The url field on a 5xx keeps the concrete path, so it reaches its
-	// own budget on the same line as the three header fields.
+	// own budget on the same line as the three header fields. That is
+	// the widest access log line the service can be made to write.
 	longPath := "/boom/" + strings.Repeat("x", oversizedSegmentBytes)
 	wantLongURL := longPath[:maxFieldBytes] + truncationSuffix
 
@@ -432,29 +420,6 @@ func lineSizeCases() map[string]sizeCase {
 			}
 	}
 
-	// From a trusted proxy, clientIP is read out of X-Forwarded-For,
-	// which the client writes. What bounds the field is that only one
-	// address from the header is written, and it is written parsed, with
-	// no zone. An IPv6 address with all eight groups at four digits is
-	// the longest such address; here it carries an 8 KB zone, which must
-	// not reach the line. It goes on the 5xx line with all three header
-	// fields at their budget.
-	const longestIPv6 = "ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff"
-
-	forwarded := oversizedHeaders(oversizedValue("h"))
-	forwarded[headerXFF] = oversizedValue("h") + ", " +
-		longestIPv6 + "%" + oversizedValue("h")
-
-	cases["oversized X-Forwarded-For from a trusted proxy "+
-		"with a 5xx concrete url"] = sizeCase{
-		target:       longPath,
-		headers:      forwarded,
-		wantStatus:   http.StatusInternalServerError,
-		wantURL:      wantLongURL,
-		wantClientIP: longestIPv6,
-		bound:        maxCappedLineBytes,
-	}
-
 	return cases
 }
 
@@ -495,14 +460,6 @@ func TestAccessLog_LineSizeDoesNotTrackInputSize(t *testing.T) {
 			require.Len(t, entries, 1)
 			assert.Equal(t, tc.wantURL, entries[0]["url"])
 
-			// Set only by the X-Forwarded-For case, where it proves
-			// the header was read rather than ignored.
-			if tc.wantClientIP != "" {
-				assert.Equal(
-					t, tc.wantClientIP, entries[0]["clientIP"],
-				)
-			}
-
 			// The markers sit at the far end of the client-chosen
 			// text, so their absence is what proves the redaction and
 			// the truncation actually ran.
@@ -691,8 +648,7 @@ func TestAccessLog_RetainsEveryOtherField(t *testing.T) {
 
 	for _, key := range []string{
 		"request_start", "method", "url", "useragent", "request_id",
-		"referer", "proto", "remoteIP", "clientIP", "status",
-		"latency_ms",
+		"referer", "proto", "remoteIP", "status", "latency_ms",
 	} {
 		assert.Contains(t, entries[0], key)
 	}

internal/middleware/clientip_test.go

@@ -1,200 +0,0 @@
-package middleware_test
-
-import (
-	"bytes"
-	"context"
-	"log/slog"
-	"net/http"
-	"net/http/httptest"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/webhooker/internal/config"
-	"sneak.berlin/go/webhooker/internal/middleware"
-)
-
-const (
-	// forwardedChain is the X-Forwarded-For a request arrives with:
-	// the client, then a second proxy inside trustedProxyCIDR that the
-	// request passed through before reaching trustedPeer.
-	forwardedChain = clientIPv4 + ", 10.0.0.2"
-
-	// untrustedPeer is a peer outside trustedProxyCIDR, so its
-	// X-Forwarded-For is ignored and the peer is the client.
-	untrustedPeer = "192.0.2.10:5555"
-
-	// oneRequestPerMinute is the receiver limit these tests install:
-	// the second request on a path is rejected, and the aggregate
-	// limit is ReceiverAggregateMultiplierConst.
-	oneRequestPerMinute = 1
-)
-
-// clientLogSite is one log line that names the client. build wraps the
-// middleware that writes it around a handler, and requests is how many
-// identical requests it takes before the line is written.
-type clientLogSite struct {
-	build    func(m *middleware.Middleware) http.Handler
-	requests int
-}
-
-// clientLogSites maps the message of each line that names the client
-// to the way to make it be written.
-func clientLogSites() map[string]clientLogSite {
-	served := func(*middleware.Middleware) http.Handler {
-		return okHandler()
-	}
-
-	receiver := func(m *middleware.Middleware) http.Handler {
-		return m.ReceiverRateLimit()(okHandler())
-	}
-
-	login := func(m *middleware.Middleware) http.Handler {
-		return http.HandlerFunc(
-			func(w http.ResponseWriter, r *http.Request) {
-				m.RecordLoginFailure(r, "someone")
-				w.WriteHeader(http.StatusUnauthorized)
-			},
-		)
-	}
-
-	csrf := func(m *middleware.Middleware) http.Handler {
-		return m.CSRF(http.HandlerFunc(forbidden))(okHandler())
-	}
-
-	passwordChange := func(m *middleware.Middleware) http.Handler {
-		return m.PasswordChangeRateLimit()(okHandler())
-	}
-
-	replay := func(m *middleware.Middleware) http.Handler {
-		return m.ReplayRateLimit()(okHandler())
-	}
-
-	resubmit := func(m *middleware.Middleware) http.Handler {
-		return m.ResubmitRateLimit()(okHandler())
-	}
-
-	return map[string]clientLogSite{
-		"http request": {
-			build:    served,
-			requests: 1,
-		},
-		"webhook receiver rate limit exceeded": {
-			build:    receiver,
-			requests: oneRequestPerMinute + 1,
-		},
-		// The aggregate limit sits in front of the per-entrypoint
-		// one, so the requests that one rejects count towards it.
-		"webhook receiver aggregate rate limit exceeded": {
-			build: receiver,
-			requests: middleware.ReceiverAggregateMultiplierConst*
-				oneRequestPerMinute + 1,
-		},
-		"login failure limit exceeded": {
-			build:    login,
-			requests: middleware.LoginRateLimitConst + 1,
-		},
-		"csrf: token validation failed": {
-			build:    csrf,
-			requests: 1,
-		},
-		"password change rate limit exceeded": {
-			build:    passwordChange,
-			requests: middleware.PasswordChangeRateLimitConst + 1,
-		},
-		"delivery replay rate limit exceeded": {
-			build:    replay,
-			requests: middleware.ReplayRateLimitConst + 1,
-		},
-		"event resubmit rate limit exceeded": {
-			build:    resubmit,
-			requests: middleware.ResubmitRateLimitConst + 1,
-		},
-	}
-}
-
-// clientLogLines sends the site's requests from peer, each carrying
-// forwardedChain, through Logging and then the site, as production
-// does, and returns the logged lines whose message is msg.
-func clientLogLines(
-	t *testing.T, site clientLogSite, msg, peer string,
-) []map[string]any {
-	t.Helper()
-
-	buf := new(bytes.Buffer)
-	log := slog.New(slog.NewJSONHandler(
-		buf,
-		&slog.HandlerOptions{Level: slog.LevelDebug},
-	))
-
-	cfg := &config.Config{
-		Environment:       config.EnvironmentDev,
-		ReceiverRateLimit: oneRequestPerMinute,
-		TrustedProxies:    trustedProxies(trustedProxyCIDR),
-	}
-
-	m := middleware.NewForTest(
-		log, cfg, newTestSessionManager(cfg, log, nil),
-	)
-	handler := m.Logging()(site.build(m))
-
-	for range site.requests {
-		req := httptest.NewRequestWithContext(
-			context.Background(), http.MethodPost, "/h/x", nil,
-		)
-		req.RemoteAddr = peer
-		req.Header.Set(headerXFF, forwardedChain)
-
-		handler.ServeHTTP(httptest.NewRecorder(), req)
-	}
-
-	var lines []map[string]any
-
-	for _, entry := range accessLogEntries(t, buf) {
-		if entry["msg"] == msg {
-			lines = append(lines, entry)
-		}
-	}
-
-	return lines
-}
-
-// TestClientIP_LoggedNextToThePeer checks that every line that names
-// the client carries both addresses: remoteIP, the connecting peer,
-// and clientIP, the client the rate limiters key on.
-func TestClientIP_LoggedNextToThePeer(t *testing.T) {
-	t.Parallel()
-
-	cases := map[string]struct {
-		peer       string
-		wantRemote string
-		wantClient string
-	}{
-		"trusted proxy with a forwarded chain": {
-			peer:       trustedPeer,
-			wantRemote: "10.0.0.1",
-			wantClient: clientIPv4,
-		},
-		"untrusted peer": {
-			peer:       untrustedPeer,
-			wantRemote: "192.0.2.10",
-			wantClient: "192.0.2.10",
-		},
-	}
-
-	for msg, site := range clientLogSites() {
-		for name, tc := range cases {
-			t.Run(msg+"/"+name, func(t *testing.T) {
-				t.Parallel()
-
-				lines := clientLogLines(t, site, msg, tc.peer)
-				require.NotEmpty(t, lines, "%q was never logged", msg)
-
-				for _, line := range lines {
-					assert.Equal(t, tc.wantRemote, line["remoteIP"])
-					assert.Equal(t, tc.wantClient, line["clientIP"])
-				}
-			})
-		}
-	}
-}

internal/middleware/csrf.go

@@ -45,10 +45,10 @@ func (m *Middleware) CSRF(
 		// unauthenticated client: a POST with no token to
 		// /hook/<any length of any text>/edit lands here. The
 		// method and path are capped against the same budgets as
-		// the access log. remoteIP and clientIP are the same
-		// addresses the access log carries, and
+		// the access log. remote_addr is set by net/http from the
+		// accepted connection rather than by the client, and
 		// csrf.FailureReason returns one of gorilla/csrf's own
-		// fixed error values, so none of them is client-sized.
+		// fixed error values, so neither is client-sized.
 		m.log.Warn("csrf: token validation failed",
 			"method", logfield.Truncate(
 				r.Method, maxLogMethodBytes,
@@ -56,8 +56,7 @@ func (m *Middleware) CSRF(
 			"path", logfield.Truncate(
 				r.URL.Path, logfield.MaxBytes,
 			),
-			"remoteIP", RemoteIP(r),
-			"clientIP", ClientIP(r),
+			"remote_addr", r.RemoteAddr,
 			"reason", csrf.FailureReason(r),
 		)
 		forbidden.ServeHTTP(w, r)

internal/middleware/export_test.go

@@ -132,12 +132,6 @@ func (g *LoginGuard) TrackedKeysForTest() (int, int) {
 // passwordChangeRateLimit constant.
 const PasswordChangeRateLimitConst = passwordChangeRateLimit
 
-// ReplayRateLimitConst exposes the replayRateLimit constant.
-const ReplayRateLimitConst = replayRateLimit
-
-// ResubmitRateLimitConst exposes the resubmitRateLimit constant.
-const ResubmitRateLimitConst = resubmitRateLimit
-
 // ReceiverAggregateMultiplierConst exposes the
 // receiverAggregateMultiplier constant.
 const ReceiverAggregateMultiplierConst = receiverAggregateMultiplier

internal/middleware/loginguard.go

@@ -385,8 +385,6 @@ func (m *Middleware) RecordLoginFailure(
 			"path", logfield.Truncate(
 				r.URL.Path, logfield.MaxBytes,
 			),
-			"remoteIP", RemoteIP(r),
-			"clientIP", ClientIP(r),
 		)
 	}
 

internal/middleware/middleware.go

@@ -3,7 +3,6 @@
 package middleware
 
 import (
-	"context"
 	"log/slog"
 	"net"
 	"net/http"
@@ -70,19 +69,18 @@ const (
 	//	url, useragent, referer   3*(512+11) = 1569
 	//	request_id                    128+11 =  139
 	//	method                         32+11 =   43
-	//	fixed portion                        =  405
+	//	fixed portion                        =  336
 	//	                                        ----
-	//	                                        2156
+	//	                                        2087
 	//
 	// The 512 is logfield.MaxBytes; the 11 is the truncation marker,
 	// charged on top of each budget rather than inside it.
 	//
 	// The fixed portion is the JSON punctuation, the field names, the
-	// level and the message, both timestamps at their longest, remoteIP
-	// and clientIP each charged as an IPv6 address with a zone, a
-	// three-digit status and a full-width int64 latency. Stated at 2560
-	// so the figure carries headroom rather than sitting on the
-	// arithmetic.
+	// level and the message, both timestamps at their longest, an IPv6
+	// remoteIP with a zone, a three-digit status and a full-width int64
+	// latency. Stated at 2560 so the figure carries headroom rather
+	// than sitting on the arithmetic.
 	//
 	// The tty text handler in internal/logger is covered by the same
 	// figure. logfield.EncodedBytes charges every rune at least what
@@ -90,8 +88,8 @@ const (
 	// bytes strconv.Quote spends on a non-printable rune at or above
 	// U+10000, which is four more than the JSON handler ever spends —
 	// so each budget bounds the encoded field under either handler.
-	// The text handler's fixed portion is 351, the smaller of the two,
-	// which puts its worst case at 2102.
+	// The text handler's fixed portion is 286, the smaller of the two,
+	// which puts its worst case at 2037.
 	//
 	// It is also the ceiling on every OTHER line this service writes
 	// THROUGH SLOG that carries text an UNAUTHENTICATED client
@@ -217,28 +215,6 @@ func ipFromHostPort(hp string) string {
 	return h
 }
 
-// RemoteIP returns the address of the connecting peer, without its
-// port. Behind a reverse proxy it is the proxy. Every log line that
-// names the client logs it as remoteIP, next to clientIP.
-func RemoteIP(r *http.Request) string {
-	return ipFromHostPort(r.RemoteAddr)
-}
-
-// clientIPKey is the request context key under which Logging stores
-// the value ClientIP returns.
-type clientIPKey struct{}
-
-// ClientIP returns the address the request is attributed to, which
-// Logging works out once per request with clientAddr in ratelimit.go
-// and logs as clientIP. The other lines that name the client read it
-// from here, so all of them agree. It is empty for a request Logging
-// has not seen.
-func ClientIP(r *http.Request) string {
-	ip, _ := r.Context().Value(clientIPKey{}).(string)
-
-	return ip
-}
-
 type loggingResponseWriter struct {
 	http.ResponseWriter
 
@@ -340,13 +316,6 @@ func (s *Middleware) Logging() func(http.Handler) http.Handler {
 			lrw := newLoggingResponseWriter(w)
 			ctx := r.Context()
 
-			// When RemoteAddr is not an address, the peer's own
-			// text is all the request can be attributed to.
-			clientIP := RemoteIP(r)
-			if addr, ok := s.clientAddr(r); ok {
-				clientIP = addr.String()
-			}
-
 			defer func() {
 				latency := time.Since(start)
 				requestID := ""
@@ -381,16 +350,13 @@ func (s *Middleware) Logging() func(http.Handler) http.Handler {
 						r.Referer(), logfield.MaxBytes,
 					),
 					"proto", r.Proto,
-					"remoteIP", RemoteIP(r),
-					"clientIP", clientIP,
+					"remoteIP", ipFromHostPort(r.RemoteAddr),
 					"status", lrw.statusCode,
 					"latency_ms", latency.Milliseconds(),
 				)
 			}()
 
-			next.ServeHTTP(lrw, r.WithContext(
-				context.WithValue(ctx, clientIPKey{}, clientIP),
-			))
+			next.ServeHTTP(lrw, r)
 		})
 	}
 }

internal/middleware/ratelimit.go

@@ -202,17 +202,24 @@ func (m *Middleware) forwardedClientAddr(
 }
 
 // rateLimitKey is the client identity every rate limiter in this
-// package buckets on: the address clientAddr attributes the request
-// to, reduced to a bucket by bucketKey — full address for IPv4, /64
-// prefix for IPv6.
+// package buckets on. Forwarded headers are honoured only when the
+// direct peer (RemoteAddr) is inside the configured trusted-proxy
+// set; otherwise the peer address itself is the key. Without that
+// gate any client could mint a fresh bucket per request, or starve
+// another client's bucket, by picking an X-Forwarded-For value —
+// which makes every limit here decorative against a deliberate
+// attacker.
+//
+// The address that identifies the client is then reduced to a bucket
+// by bucketKey: full address for IPv4, /64 prefix for IPv6.
 func (m *Middleware) rateLimitKey(r *http.Request) (string, error) {
 	return m.clientKey(r), nil
 }
 
 // clientKey computes the bucket key described on rateLimitKey.
 func (m *Middleware) clientKey(r *http.Request) string {
-	addr, ok := m.clientAddr(r)
-	if !ok {
+	peer, err := netip.ParseAddr(ipFromHostPort(r.RemoteAddr))
+	if err != nil {
 		// Not an address we can reason about; key on the raw
 		// value, the most specific identity left. Distinct
 		// RemoteAddr values stay in distinct buckets, so this
@@ -223,36 +230,16 @@ func (m *Middleware) clientKey(r *http.Request) string {
 		return r.RemoteAddr
 	}
 
-	return bucketKey(addr)
-}
-
-// clientAddr is the address a request is attributed to. The rate
-// limiters key on it and the logs name it as clientIP.
-//
-// Forwarded headers are honoured only when the direct peer
-// (RemoteAddr) is inside the configured trusted-proxy set; otherwise
-// the peer address itself is the client. Without that gate any client
-// could mint a fresh bucket per request, or starve another client's
-// bucket, by picking an X-Forwarded-For value — which makes every
-// limit here decorative against a deliberate attacker.
-//
-// ok is false when RemoteAddr is not an address at all.
-func (m *Middleware) clientAddr(r *http.Request) (netip.Addr, bool) {
-	peer, err := netip.ParseAddr(ipFromHostPort(r.RemoteAddr))
-	if err != nil {
-		return netip.Addr{}, false
-	}
-
 	peer = normalizeAddr(peer)
 	if !m.isTrustedProxy(peer) {
-		return peer, true
+		return bucketKey(peer)
 	}
 
 	if addr, ok := m.forwardedClientAddr(r); ok {
-		return addr, true
+		return bucketKey(addr)
 	}
 
-	return peer, true
+	return bucketKey(peer)
 }
 
 // tooManyRequests returns the 429 handler used by the
@@ -275,8 +262,6 @@ func (m *Middleware) tooManyRequests(
 			"path", logfield.Truncate(
 				r.URL.Path, logfield.MaxBytes,
 			),
-			"remoteIP", RemoteIP(r),
-			"clientIP", ClientIP(r),
 		)
 		http.Error(w, responseMessage, http.StatusTooManyRequests)
 	}
@@ -301,12 +286,8 @@ func (m *Middleware) tooManyRequests(
 func (m *Middleware) floodTooManyRequests(
 	logMessage, responseMessage string,
 ) http.HandlerFunc {
-	return func(w http.ResponseWriter, r *http.Request) {
-		m.log.Debug(
-			logMessage,
-			"remoteIP", RemoteIP(r),
-			"clientIP", ClientIP(r),
-		)
+	return func(w http.ResponseWriter, _ *http.Request) {
+		m.log.Debug(logMessage)
 		http.Error(w, responseMessage, http.StatusTooManyRequests)
 	}
 }

internal/server/archive_download_test.go

@@ -1,69 +0,0 @@
-package server_test
-
-import (
-	"compress/gzip"
-	"encoding/json"
-	"net/http"
-	"regexp"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"gorm.io/gorm/clause"
-	"sneak.berlin/go/webhooker/internal/database"
-)
-
-// TestHook_DownloadArchive follows the Download link the webhook page
-// shows for a database target, and only for it, and gets the archive
-// as a gzipped JSON file. Signed out, the link leads to the login page.
-func TestHook_DownloadArchive(t *testing.T) {
-	t.Parallel()
-
-	env := newTestEnv(t)
-
-	userID, _ := env.seedUser(t, "archivist", "somepassword")
-	cookies := env.authCookies(t, userID, "archivist")
-	wh := env.seedWebhook(t, userID)
-	env.seedTarget(t, wh.ID)
-
-	archive := &database.Target{
-		WebhookID: wh.ID,
-		Name:      "kept",
-		Type:      database.TargetTypeDatabase,
-		Active:    true,
-	}
-	require.NoError(t,
-		env.db.DB().Omit(clause.Associations).Create(archive).Error,
-	)
-
-	page := env.get("/hook/"+wh.ID, cookies)
-	require.Equal(t, http.StatusOK, page.Code)
-
-	links := regexp.MustCompile(
-		`href="(/hook/[^/"]+/targets/[^/"]+/download)"`,
-	).FindAllStringSubmatch(page.Body.String(), -1)
-	require.Len(t, links, 1, "only the database target has a Download")
-
-	link := links[0][1]
-	assert.Equal(t,
-		"/hook/"+wh.ID+"/targets/"+archive.ID+"/download", link,
-	)
-
-	w := env.get(link, cookies)
-	require.Equal(t, http.StatusOK, w.Code)
-	assert.Equal(t, "application/gzip", w.Header().Get("Content-Type"))
-
-	zr, err := gzip.NewReader(w.Body)
-	require.NoError(t, err)
-
-	var got map[string]json.RawMessage
-
-	require.NoError(t, json.NewDecoder(zr).Decode(&got))
-	assert.JSONEq(t,
-		`{"id":"`+wh.ID+`","name":"routed"}`, string(got["webhook"]),
-	)
-
-	w = env.get(link, nil)
-	assert.Equal(t, http.StatusSeeOther, w.Code)
-	assert.Contains(t, w.Header().Get("Location"), "/pages/login")
-}

internal/server/routes.go

@@ -312,10 +312,6 @@ func (s *Server) setupSourceRoutes() {
 			"/targets/{targetID}/edit",
 			s.h.HandleTargetEditSubmit(),
 		)
-		r.Get(
-			"/targets/{targetID}/download",
-			s.h.HandleTargetDownload(),
-		)
 		r.Post(
 			"/targets/{targetID}/delete",
 			s.h.HandleTargetDelete(),

templates/source_detail.html

@@ -157,9 +157,6 @@
                             {{else}}
                             <span class="badge-error">Inactive</span>
                             {{end}}
-                            {{if eq .Type "database"}}
-                            <a href="/hook/{{$.Webhook.ID}}/targets/{{.ID}}/download" class="text-xs text-gray-500 hover:text-primary-600" title="Download the archive as gzipped JSON">Download</a>
-                            {{end}}
                             <a href="/hook/{{$.Webhook.ID}}/targets/{{.ID}}/edit" class="text-xs text-gray-500 hover:text-primary-600" title="Edit">Edit</a>
                             <form method="POST" action="/hook/{{$.Webhook.ID}}/targets/{{.ID}}/toggle" class="inline">
                                 <input type="hidden" name="csrf_token" value="{{$.CSRFToken}}">