Describe code in its own terms, not by an earlier build's data (closes #522 )

Several comments justified code by what an earlier build would have left in a database or on disk. Each now says what the code does: reserveSQLiteFile sets the mode on every open whoever created the file, and expired treats a session lacking a deadline's timestamp as expired. The comments on retention, resubmitted events, archive pruning, target headers, the redactor and the delivery result view drop their mentions of older rows. Comments only. Model: opus-5-5
2026-10-13 05:45:14 +00:00
9 changed files with 94 additions and 150 deletions
@@ -214,22 +214,16 @@ RUN version="$(make version VERSION="$VERSION")"; \
 # (Alpine.js, extracted from its tarball in 3p/) first.
 RUN make build VERSION="$VERSION"

-# Rebuild with static linking for the Alpine runtime. The cgo binary the
-# step above builds is dynamically linked against glibc, which Alpine
-# (musl) does not have.
+# Rebuild with static linking for Alpine runtime.
+# make build already verified compilation.
+# The CGO binary from `make build` is dynamically linked against glibc,
+# which doesn't exist on Alpine (musl). Rebuild with static linking so
+# the binary runs on Alpine without glibc.
 #
-# A statically linked glibc still loads shared libraries at run time for
-# getaddrinfo and dlopen, and Alpine has none to load, so the build tags
-# keep both out of the binary: netgo and osusergo select Go's own DNS
-# resolver and user lookup instead of glibc's, and
-# sqlite_omit_load_extension leaves out SQLite extension loading, which
-# webhooker never uses.
-#
-# The static flags and tags go in through GO_LDFLAGS and GO_TAGS rather
-# than a -ldflags of their own: the build target composes them with the
-# -X that stamps the version, so this relink cannot silently drop the
-# stamp.
-RUN CGO_ENABLED=1 make build VERSION="$VERSION" GO_TAGS=netgo,osusergo,sqlite_omit_load_extension GO_LDFLAGS='-extldflags "-static"'
+# The static flags go in through GO_LDFLAGS rather than a -ldflags of
+# their own: the build target composes them with the -X that stamps the
+# version, so this relink cannot silently drop the stamp.
+RUN CGO_ENABLED=1 make build VERSION="$VERSION" GO_LDFLAGS='-extldflags "-static"'

 # Runtime stage
 # alpine:3.21, 2026-03-17
@@ -16,16 +16,14 @@ VERSION ?= $(shell script/version)
 # exists to correct.
 override VERSION := $(or $(strip $(VERSION)),$(shell script/version))

-# Extra linker flags and build tags for the build target. The static
-# relink in the Dockerfile passes -extldflags and its build tags through
-# these rather than a -ldflags of its own, so composing flags cannot drop
-# the version stamp.
+# Extra linker flags for the build target. The static relink in the
+# Dockerfile adds -extldflags here rather than passing its own -ldflags,
+# so composing flags cannot drop the version stamp.
 #
 # The build target itself always passes -trimpath and -s -w, as the Go
 # Dockerfile in REPO_POLICIES.md does: no build paths, symbol table or
 # debug information in the binary.
 GO_LDFLAGS ?=
-GO_TAGS ?=

 bootstrap:
 	@script/bootstrap
@@ -55,7 +53,7 @@ check:
 	@script/check

 build: assets
-	go build -trimpath -tags '$(GO_TAGS)' -ldflags '$(strip -s -w -X main.version=$(VERSION) $(GO_LDFLAGS))' -o bin/webhooker ./cmd/webhooker
+	go build -trimpath -ldflags '$(strip -s -w -X main.version=$(VERSION) $(GO_LDFLAGS))' -o bin/webhooker ./cmd/webhooker

 run: build
 	./bin/webhooker
@@ -465,7 +465,10 @@ idle timestamp is rewritten at most once per tenth of the idle window rather
 than on every request, which means a session may expire up to 10% early relative
 to the user's true last request, but never late.

-Both clocks are anchored by timestamps stored in the session cookie.
+Both clocks are anchored by timestamps stored in the session cookie. Sessions
+issued before this feature existed carry neither, so they are treated as
+expired: upgrading to a build that has it logs every existing session out once,
+and those users sign in again.

 #### Invalid values abort startup

@@ -699,11 +702,11 @@ app as that user.
 `webhooker.db` holds target configuration in plaintext — bearer tokens, API
 keys, Slack webhook URLs — along with the session encryption key, so webhooker
 creates every SQLite file it owns `0600`: each database and both of its `-wal`
-and `-shm` sidecars, across all three tiers. Any of them found at another mode
-is set to `0600` when the database is opened. The directory's `0750` is defence
-in depth — it stops other local users listing the directory and learning your
-webhook UUIDs from the `events-{uuid}.db` filenames — not the barrier protecting
-the credentials.
+and `-shm` sidecars, across all three tiers. Files an earlier build left `0644`
+are tightened when they are opened. The directory's `0750` is defence in depth —
+it stops other local users listing the directory and learning your webhook UUIDs
+from the `events-{uuid}.db` filenames — not the barrier protecting the
+credentials.

 ### Running under upaas

@@ -915,7 +918,7 @@ replaces the `max-age` it stored, so to end the policy sooner, set a short
 `HSTS_MAX_AGE` such as `1s` and keep it until returning browsers have picked it
 up, or have a reverse proxy send `max-age=0`.

-## Backup and Restore
+## Backup, Restore, and Upgrades

 ### What to back up

@@ -1068,14 +1071,60 @@ beside it, or wait until there are none.

 4. Start the service. The container gives the directory and the restored files
   to the `webhooker` user before the app starts, whoever restored them (see
-   [Running with Docker](#running-with-docker)).
+   [Running with Docker](#running-with-docker)). Each restored database gets any
+   of its SQL migrations it has not had as it is opened (see
+   [Upgrades](#upgrades)).
+
+### Upgrades
+
+**Back up before every upgrade.** Each kind of database file has its schema in
+SQL files built into the binary, one directory per kind under
+`internal/db/migrations/`. In each, `000_migration.sql` creates the
+`schema_migrations` table, which records the number of every file the database
+has had, and `001_schema.sql` holds the whole schema. Opening a database runs,
+in one transaction, every file in its directory that `schema_migrations` does
+not list:
+
+- `main/` for the main database, on connect — settings, users, API keys,
+  webhooks, entrypoints and targets
+- `events/` for each event database, when it is lazily opened — events,
+  deliveries, delivery results and their running totals
+- `archive/` for each archive database, on every open and reopen
+
+There are no down migrations.
+
+Upgrade procedure:
+
+1. Stop the service.
+2. Back up `DATA_DIR` using one of the procedures above.
+3. Pull the new image and start it.
+4. Confirm `database migrations completed` in the logs before putting traffic
+   back on it.
+5. Confirm the new build is the one running:
+   `curl -s http://host:8080/.well-known/healthcheck` reports the version it was
+   stamped with (see [Version stamping](#version-stamping)).
+
+**Upgrading past the introduction of `BIND_ADDRESS`:** earlier versions always
+bound every interface. **Container deployments are unaffected** — the image
+ships `ENV BIND_ADDRESS=0.0.0.0`, so a `docker run` or Compose service that
+worked before still works with nothing changed.
+
+A **bare binary** is the case that changes: the listener now binds `127.0.0.1`
+unless `BIND_ADDRESS` says otherwise, so a deployment that relied on reaching it
+from another host becomes unreachable until it sets the address the proxy
+connects to. Check the `bindAddress` field of the startup log to see what a
+running process bound. See [Bind address](#bind-address).
+
+**Downgrade is unsupported.** Once a newer binary has migrated the files there
+is no way to move them back. The only supported way back to an older version is
+restoring the pre-upgrade backup, which discards everything received since that
+backup was taken.

 ### Version stamping

 The binary reports its version at `/.well-known/healthcheck` (the `version`
 field), in the UI footer, and in the startup log line (`msg=starting`,
 `version=...`). It is also the Sentry release name, as `webhooker-{version}`.
-`curl -s http://host:8080/.well-known/healthcheck` shows which build is running.
 The value is stamped in at build time by the linker; it is not read from a file
 at runtime, so it identifies the build itself.

@@ -1534,7 +1583,8 @@ The cap is not arbitrary: the reaper computes its cutoff as a `time.Duration`,
 an int64 nanosecond count, and a longer period overflows it. An overflowed
 cutoff lands in the future, where it matches every row, so the sweep would
 delete every event the webhook has instead of none. The reaper also clamps the
-value it reads to the cap, so no stored value can trigger that either.
+value it is given, so a row written by an older version cannot trigger that
+either.

 #### Entrypoint

@@ -1917,7 +1967,7 @@ and application state:
 - **APIKeys** — programmatic access credentials

 On first startup the main database is created from its SQL files (see
-[Schema](#schema)), a session encryption key is generated and stored, and an
+[Upgrades](#upgrades)), a session encryption key is generated and stored, and an
 `admin` user is created.

 **Per-Webhook Event Databases** (`{DATA_DIR}/events-{webhook_uuid}.db`) — each
@@ -2114,21 +2164,6 @@ The database uses the
 though CGO is required at build time due to the transitive `mattn/go-sqlite3`
 dependency from `gorm.io/driver/sqlite`.

-#### Schema
-
-Each kind of database file has its schema in SQL files built into the binary,
-one directory per kind under `internal/db/migrations/`. In each,
-`000_migration.sql` creates the `schema_migrations` table, which records the
-number of every file the database has had, and `001_schema.sql` holds the whole
-schema. Opening a database runs, in one transaction, every file in its directory
-that `schema_migrations` does not list:
-
- `main/` for the main database, on connect — settings, users, API keys,
-  webhooks, entrypoints and targets
- `events/` for each event database, when it is lazily opened — events,
-  deliveries, delivery results and their running totals
- `archive/` for each archive database, on every open and reopen
-
 ### Request Flow

 ```
@@ -3419,14 +3454,12 @@ fixed independently of the compiler's:
   phases and the `css-check` and `markdown-check` stages passing (it copies a
   file from each), runs `make build` (which extracts Alpine.js from `3p/`
   first), and then rebuilds the binary with `CGO_ENABLED=1` and static linking
-   so it runs on musl, with the build tags `netgo`, `osusergo` and
-   `sqlite_omit_load_extension` so it needs none of glibc's shared libraries at
-   run time. Both builds go through `make build`, the relink adding its
-   `-extldflags` via `GO_LDFLAGS` and its tags via `GO_TAGS`, so neither can
-   drop the `-X` that stamps the version. The version is the `VERSION` build arg
-   if one is given, otherwise derived from the `.git` in the context, and the
-   stage fails if a context with `.git` would stamp an empty version, `dev` or
-   `unknown` (see [Version stamping](#version-stamping)).
+   so it runs on musl. Both builds go through `make build`, the relink adding
+   its `-extldflags` via `GO_LDFLAGS`, so neither can drop the `-X` that stamps
+   the version. The version is the `VERSION` build arg if one is given,
+   otherwise derived from the `.git` in the context, and the stage fails if a
+   context with `.git` would stamp an empty version, `dev` or `unknown` (see
+   [Version stamping](#version-stamping)).
 7. **Runtime stage** (`alpine:3.21`) — copies the static binary and
   `deploy/docker-entrypoint.sh`, creates the `/var/lib/webhooker` directory for
   all SQLite databases, exposes port 8080, and includes a health check against
@@ -82,7 +82,7 @@ type Webhook struct {
 // value while building the insert statement, which runs after
 // BeforeSave; rewriting any later than this loses that race and the
 // row lands at 30 days. Living on the model also means a future call
-// site, such as a REST API or a fixture, cannot bypass it.
+// site — a REST API, a fixture, a migration — cannot bypass it.
 func (w *Webhook) BeforeSave(_ *gorm.DB) error {
 	if w.RetentionDays <= 0 {
 		w.RetentionDays = RetentionForeverDays
@@ -241,19 +241,7 @@ func SQLiteDSN(path, mode string) string {
 	)
 	q.Add("_pragma", "journal_mode(WAL)")

-	// SQLite reads a file: URI's path only up to the first ? or #, and
-	// decodes %XX escapes in it, so url.URL escapes the path. OmitHost
-	// keeps a relative path as "file:data/x.db"; written
-	// "file://data/x.db", SQLite would reject "data" as the URI's
-	// authority.
-	dsn := url.URL{
-		Scheme:   "file",
-		Path:     path,
-		OmitHost: true,
-		RawQuery: q.Encode(),
-	}
-
-	return dsn.String()
+	return "file:" + path + "?" + q.Encode()
 }

 // OpenSQLite opens the SQLite file at path with the service's
@@ -2,8 +2,6 @@ package database_test

 import (
 	"context"
-	"database/sql"
-	"os"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -114,79 +112,6 @@ func TestMainDBAppliesPragmasOnALiveHandle(t *testing.T) {
 	assert.Equal(t, "10000", busy)
 }

-// TestOpenSQLiteOpensThePathAskedFor uses a data directory whose name
-// holds ?, # and %. SQLite reads a file: URI's path only up to a ? or
-// #, and decodes %41 to A, so an unescaped path sent every database in
-// that directory to one file outside it.
-func TestOpenSQLiteOpensThePathAskedFor(t *testing.T) {
-	t.Parallel()
-
-	dir := filepath.Join(t.TempDir(), "hooks#1?x%41")
-	require.NoError(t, os.Mkdir(dir, 0o700))
-
-	for _, name := range []string{
-		database.MainDBFileName,
-		"events-" + uuid.New().String() + ".db",
-	} {
-		path := filepath.Join(dir, name)
-
-		sqlDB, err := database.OpenSQLite(
-			path, database.SQLiteModeCreate,
-		)
-		require.NoError(t, err)
-
-		t.Cleanup(func() { assert.NoError(t, sqlDB.Close()) })
-
-		assertOpenedFile(t, sqlDB, path)
-	}
-}
-
-// TestOpenSQLiteOpensARelativePath opens a database at a path relative
-// to the working directory, which is what a relative DATA_DIR such as
-// the README's DATA_DIR=../webhooker-data gives OpenSQLite.
-//
-//nolint:paralleltest // t.Chdir moves the whole process.
-func TestOpenSQLiteOpensARelativePath(t *testing.T) {
-	t.Chdir(t.TempDir())
-	require.NoError(t, os.Mkdir("data", 0o700))
-
-	path := filepath.Join("data", database.MainDBFileName)
-
-	sqlDB, err := database.OpenSQLite(path, database.SQLiteModeCreate)
-	require.NoError(t, err)
-
-	defer func() { require.NoError(t, sqlDB.Close()) }()
-
-	assertOpenedFile(t, sqlDB, path)
-}
-
-// assertOpenedFile checks that sqlDB is the SQLite file at path.
-// database_list names the file SQLite actually opened, as an absolute
-// path with symlinks resolved, so the files are compared, not the names.
-func assertOpenedFile(t *testing.T, sqlDB *sql.DB, path string) {
-	t.Helper()
-
-	var (
-		seq          int
-		schema, file string
-	)
-
-	require.NoError(t, sqlDB.
-		QueryRowContext(t.Context(), "pragma database_list").
-		Scan(&seq, &schema, &file))
-
-	asked, err := os.Stat(path)
-	require.NoError(t, err)
-
-	opened, err := os.Stat(file)
-	require.NoError(t, err)
-
-	assert.True(
-		t, os.SameFile(asked, opened),
-		"asked for %s, SQLite opened %s", path, file,
-	)
-}
-
 // TestConcurrentReaderDoesNotBlockWrites is the unit-scale form of the
 // reproduction in
 // https://git.eeqj.de/sneak/webhooker/issues/256: an operator's
@@ -42,6 +42,9 @@ func TestDeliveryAttempts_ReadInTheTargetTypesOwnTerms(t *testing.T) {
 		{database.TargetTypeLog, true, 0, "", "written to the log", ""},
 	}

+	// Named for type and outcome, so that no two cases share a name:
+	// Go adds "#01" to a repeated one, and a "#" in the temporary
+	// directory's path cuts the path SQLiteDSN gives SQLite short.
 	for _, tc := range cases {
 		t.Run(string(tc.targetType)+" "+tc.outcome, func(t *testing.T) {
 			t.Parallel()
@@ -49,6 +49,13 @@ var (
 // working retry configuration over a typo. A default answers a
 // question that was not asked; it never answers one that was asked
 // badly.
+//
+// A target stored with a count above the ceiling before this
+// validation existed keeps rendering and keeps delivering — nothing
+// clamps the row. Re-saving it from the edit form does have to bring
+// it into range, because the form submits the pre-filled value back
+// and accepting it would be the ceiling not applying to the edit
+// path. The 400 names the ceiling, so the fix is one field.
 func parseMaxRetries(raw string, fallback int) (int, error) {
 	raw = strings.TrimSpace(raw)
 	if raw == "" {
@@ -156,8 +156,7 @@ func TestVersion_IsStableAcrossInvocations(t *testing.T) {
 // The build target is the only place composing linker flags. If a
 // future edit drops either half, the binary silently reports "dev"
 // again (the defect this package exists for) or fails to link on
-// Alpine. If it stops passing GO_TAGS, the image's static binary links
-// glibc's getaddrinfo and dlopen again.
+// Alpine.
 func TestMakefile_BuildComposesVersionAndExtraFlags(t *testing.T) {
 	t.Parallel()

@@ -165,7 +164,6 @@ func TestMakefile_BuildComposesVersionAndExtraFlags(t *testing.T) {

 	require.Contains(t, makefile, "-X main.version=$(VERSION)")
 	require.Contains(t, makefile, "$(GO_LDFLAGS)")
-	require.Contains(t, makefile, "-tags '$(GO_TAGS)'")
 	require.Contains(t, makefile, "VERSION ?= $(shell script/version)")
 }

@@ -235,9 +233,7 @@ func TestDockerfile_BuildsThroughTheMakeTarget(t *testing.T) {
 		"a raw go build bypasses the Makefile's -X flag")
 	require.Contains(t, dockerfile, "ARG VERSION")
 	require.Contains(t, dockerfile,
-		`make build VERSION="$VERSION"`+
-			` GO_TAGS=netgo,osusergo,sqlite_omit_load_extension`+
-			` GO_LDFLAGS='-extldflags "-static"'`)
+		`make build VERSION="$VERSION" GO_LDFLAGS='-extldflags "-static"'`)
 }

 func read(t *testing.T, path string) string {