Point to the proxy's access log for a login flood's source

No log line records the client address taken from X-Forwarded-For,
so the login endpoint section no longer says the source shows in the
failure logs; it names the proxy's access log instead.

Model: opus-5-5

Dockerfile

@@ -88,9 +88,7 @@ RUN CGO_ENABLED=1 make build VERSION="$VERSION" GO_LDFLAGS='-extldflags "-static
 # alpine:3.21, 2026-03-17
 FROM alpine:3.21@sha256:c3f8e73fdb79deaebaa2037150150191b9dcbfba68b4a46d70103204c53f4709
 
-# su-exec 0.2-r3 (Alpine 3.21), 2026-09-29: the entrypoint runs the app
+RUN apk --no-cache add ca-certificates
-# as webhooker with it.
-RUN apk --no-cache add ca-certificates su-exec=0.2-r3
 
 # Create non-root user
 RUN addgroup -g 1000 -S webhooker && \
@@ -101,17 +99,13 @@ WORKDIR /app
 # Copy binary from builder
 COPY --from=builder /build/bin/webhooker /app/webhooker
 
-# Not under /app, which belongs to webhooker: this script runs as root.
-COPY deploy/docker-entrypoint.sh /usr/local/bin/docker-entrypoint.sh
-
 # Create data directory for all SQLite databases (main app DB +
 # per-webhook event DBs). DATA_DIR defaults to /var/lib/webhooker.
 RUN mkdir -p /var/lib/webhooker
 
 RUN chown -R webhooker:webhooker /app /var/lib/webhooker
 
-# No USER: the entrypoint starts as root to make the data directory
+USER webhooker
-# webhooker's, then runs the app as webhooker.
 
 EXPOSE 8080
 
@@ -130,5 +124,4 @@ ENV BIND_ADDRESS=0.0.0.0
 HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \
     CMD wget --no-verbose --tries=1 --spider http://localhost:8080/.well-known/healthcheck || exit 1
 
-ENTRYPOINT ["/usr/local/bin/docker-entrypoint.sh"]
 CMD ["/app/webhooker"]

README.md

@@ -545,12 +545,6 @@ its Argon2id hash. There is no second account and no forgot-password
 flow, so the banner and the reset command below are the only two ways
 in.
 
-A start that finds no `webhooker.db` in `DATA_DIR` also logs
-`created a new, empty database` at `WARN`, with the file's path,
-shortly before the banner. On a deployment that has run before, that
-line means `DATA_DIR` was empty, most often because its volume is not
-mounted.
-
 #### Recovering a lost admin password
 
 `webhooker resetpw` sets an existing account's password from the
@@ -565,9 +559,8 @@ printf '%s' "$NEW_PASSWORD" | \
     DATA_DIR=/var/lib/webhooker webhooker resetpw admin
 ```
 
-In a container it is the same binary. The image's `CMD` is
+In a container it is the same binary, which the image sets as `CMD`
-`/app/webhooker`, and a command given to `docker run` replaces all of
+rather than `ENTRYPOINT`, so the whole command has to be given:
-it, so the whole command has to be given:
 
 ```bash
 docker run --rm -v webhooker-data:/var/lib/webhooker \
@@ -704,22 +697,38 @@ those three values rather than trusting the figure. Measured at 65s on
 Docker 29.7.2.) A container `unhealthy` with `connection refused` in
 its health log, or a published port that resets connections, is this.
 
-The app runs as a non-root user (`webhooker`, UID 1000), exposes port
+The container runs as a non-root user (`webhooker`, UID 1000), exposes
-8080, and includes a health check against `/.well-known/healthcheck`.
+port 8080, and includes a health check against
-The `/var/lib/webhooker` volume holds all SQLite databases: the main
+`/.well-known/healthcheck`. The `/var/lib/webhooker` volume holds all
-application database (`webhooker.db`), the per-webhook event databases
+SQLite databases: the main application database (`webhooker.db`), the
-(`events-{uuid}.db`), and any archive databases written by `database`
+per-webhook event databases (`events-{uuid}.db`), and any archive
-targets (`archive-{uuid}.db`). Mount this as a persistent volume to
+databases written by `database` targets (`archive-{uuid}.db`). Mount
-preserve data across container restarts.
+this as a persistent volume to preserve data across container
+restarts.
 
-**The container sets its data directory's owner and mode itself
+**The bind-mounted directory must be owned by UID 1000, or the
-before the app starts**, so a host directory can be mounted as it is,
+container does not start.** Docker creates a `-v` source path that
-whoever owns it. The image's `ENTRYPOINT`,
+does not exist yet as `root:root`, and the process runs as UID 1000,
-`deploy/docker-entrypoint.sh`, starts as root, creates `DATA_DIR` if
+so it cannot take its `DATA_DIR` lock:
-it is missing, gives the directory and anything in it that belongs to
+
-another user to `webhooker`, sets the directory to `0750`, and only
+```
-then runs the app as `webhooker`. Started with `--user`, it changes
+webhooker: locking data directory /var/lib/webhooker: open
-nothing and runs the app as that user.
+/var/lib/webhooker/webhooker.lock: permission denied
+```
+
+It exits non-zero at that point, before opening any database. Create
+the directory ahead of the first `docker run`:
+
+```bash
+mkdir -p /path/to/data
+chown 1000:1000 /path/to/data
+chmod 750 /path/to/data
+```
+
+The same `chown` is what a restore needs — see step 4 of
+[Restore](#restore). A **named volume** does not have this problem:
+Docker copies the image's ownership onto a volume it initializes, and
+the image creates `/var/lib/webhooker` owned by `webhooker`.
 
 **The file modes are not yours to set, and do not depend on the
 directory.** `webhooker.db` holds target configuration in plaintext —
@@ -727,10 +736,13 @@ 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. Files an earlier build left `0644` are tightened when
-they are opened. The directory's `0750` is defence in depth — it stops
+they are opened. A `DATA_DIR` webhooker creates itself is `0750`, but
-other local users listing the directory and learning your webhook
+a bind mount supplies its own directory and Docker's default for one
-UUIDs from the `events-{uuid}.db` filenames — not the barrier
+it creates is `0755`; the `0600` files hold there regardless. The
-protecting the credentials.
+`chmod 750` above 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
 
@@ -746,6 +758,17 @@ repository's `Dockerfile` and runs it. The app needs:
   app name, port `8080`. Leave `PORT` unset: the image's health check
   probes `8080`.
 - **Volume:** one host directory mounted at `/var/lib/webhooker`.
+  upaas bind-mounts the host path it is given and does not create it,
+  and the container does not start unless UID 1000 owns it (see
+  [Running with Docker](#running-with-docker)). Create it before the
+  first deploy:
+
+    ```bash
+    mkdir -p /path/to/data
+    chown 1000:1000 /path/to/data
+    chmod 750 /path/to/data
+    ```
+
 - **Environment variables:**
     - `WEBHOOKER_ENVIRONMENT=prod`
     - `TRUSTED_PROXIES`: Docker networks use private addresses, so the
@@ -1012,12 +1035,12 @@ done
 `.backup` reads through the WAL and writes a single consistent file with
 no sidecars of its own, so the destination is complete as it stands.
 Two caveats. First, the runtime image is `alpine:3.21` with only
-`ca-certificates` and `su-exec` added — the `sqlite3` CLI is **not** in
+`ca-certificates` added — the `sqlite3` CLI is **not** in it, so run
-it, so run this on the host against the volume path, or from a
+this on the host against the volume path, or from a throwaway container
-throwaway container that mounts the volume. Second, each file is
+that mounts the volume. Second, each file is captured at its own
-captured at its own instant, so a webhook created or an event delivered
+instant, so a webhook created or an event delivered between two files
-between two files being copied lands in one and not the other. If you
+being copied lands in one and not the other. If you need the whole set
-need the whole set coherent as of a single moment, stop the service.
+coherent as of a single moment, stop the service.
 
 Note that `sqlite3 <db> .dump` is **not** one of these procedures: it is
 an export, it holds a read transaction open for as long as it runs, and
@@ -1071,11 +1094,21 @@ with any `-wal`/`-shm` beside it, or wait until there are none.
    archive not opened since a crash. A copy salvaged from a crashed
    instance has them for everything, and needs all of them.
 
-4. Start the service. The container gives the directory and the
+4. **Fix ownership.** The container runs as the non-root `webhooker`
-   restored files to the `webhooker` user before the app starts,
+   user, UID 1000 / GID 1000. Restored files must be owned by (or
-   whoever restored them (see
+   writable by) that UID, and so must the directory itself — SQLite
-   [Running with Docker](#running-with-docker)). `AutoMigrate` runs
+   creates the `-wal` and `-shm` sidecars beside the database, so a
-   against each restored database as it is opened.
+   writable file inside a directory it cannot write is not enough:
+
+    ```bash
+    chown -R 1000:1000 /path/to/data
+    ```
+
+    Restoring as `root` on the host and forgetting this step is the
+    usual way a restore fails.
+
+5. Start the service. `AutoMigrate` runs against each restored database
+   as it is opened.
 
 ### Upgrades
 
@@ -2554,7 +2587,7 @@ arrived uncut — never the numbers.
 
 Every limiter here — receiver, login, and password change — identifies
 the client the same way, through one shared key function: the
-connection's own address, unless the peer is inside
+connection's own address, unless the peer is listed in
 `TRUSTED_PROXIES`, in which case the forwarded client address is used
 instead. That address becomes a bucket by family: IPv4 keys on the full
 address, IPv6 on its `/64` prefix. A routed `/64` is the normal
@@ -2592,8 +2625,7 @@ opposite directions:
 
 The login `POST` is the one endpoint with no pre-emptive limiter in
 front of it, and that is deliberate. A limiter that spends budget on
-arrival is a lockout wherever clients share one bucket, as they do
+arrival is a lockout in this deployment shape: sharing one bucket, a
-behind a reverse proxy that `TRUSTED_PROXIES` does not cover: a
 stranger sending five POSTs a minute — about 0.08 requests per second,
 from anywhere — keeps it permanently full, and the operator has no
 second administrative path. So the handler inverts the order:
@@ -2707,7 +2739,7 @@ abuse limit later; they are tracked as future work.
 | ------ | --------------------------- | ----------- |
 | `GET`  | `/`                         | Root redirect, 303 (authenticated → `/sources`, unauthenticated → `/pages/login`) |
 | `GET`  | `/.well-known/healthcheck`  | Health check (JSON: `status`, `now`, `uptimeSeconds`, `uptimeHuman`, `version`, `appname`, `maintenanceMode`) |
-| `GET`, `HEAD` | `/s/*`              | Static file serving (embedded CSS, JS). `GET` and `HEAD` only — `POST`, `PUT`, `PATCH`, `DELETE`, `OPTIONS`, `TRACE` and `CONNECT` are answered `405 Method Not Allowed` with `Allow: GET, HEAD`. Any other method (such as `PROPFIND`) is refused by chi before it reaches this route, and gets `405` without an `Allow` header. Pinned by `TestStaticServesOnlyGetAndHead` |
+| any    | `/s/*`                      | Static file serving (embedded CSS, JS). Mounted for every method, not just `GET`/`HEAD`: chi's `Mount` registers all methods and `http.FileServer` special-cases only `HEAD` (by omitting the body), so a `POST` or `DELETE` to an asset is answered `200` with the file. Pinned by `TestStaticServesEveryMethod` |
 | `POST` | `/webhook/{uuid}`           | Webhook receiver endpoint. `POST` only — every other method is answered `405 Method Not Allowed` with `Allow: POST`. Rate limited (see [Rate Limiting](#rate-limiting)) |
 
 #### Authentication Endpoints
@@ -3056,11 +3088,7 @@ check, see [The login endpoint](#the-login-endpoint).
 - Prometheus metrics behind basic auth
 - Static assets embedded in binary (no filesystem access needed at
   runtime)
-- The app runs as the non-root `webhooker` user (UID 1000) in the
+- Container runs as non-root user (UID 1000)
-  container. The image sets no `USER`, so these run as root: the
-  `ENTRYPOINT` script, which sets the data directory's owner and mode
-  before the app starts; the image's health check; and `docker exec`,
-  unless given `--user`
 - GORM soft deletes on every entity that carries `BaseModel`, which is
   all of them but `Setting` (data preserved for audit)
 
@@ -3187,13 +3215,10 @@ version is fixed independently of the compiler's:
    `GO_LDFLAGS`, so neither can drop the `-X` that stamps the version.
    The version arrives as the `VERSION` build arg, since the context
    has no `.git` (see [Version stamping](#version-stamping)).
-3. **Runtime stage** (`alpine:3.21`) — copies the static binary and
+3. **Runtime stage** (`alpine:3.21`) — copies the static binary,
-   `deploy/docker-entrypoint.sh`, creates the `/var/lib/webhooker`
+   creates the `/var/lib/webhooker` directory for all SQLite databases,
-   directory for all SQLite databases, exposes port 8080, and includes
+   runs as the non-root `webhooker` user (UID 1000), exposes port 8080,
-   a health check against `/.well-known/healthcheck`. It sets no
+   and includes a health check against `/.well-known/healthcheck`.
-   `USER`: the `ENTRYPOINT` script starts as root, sets the data
-   directory's owner and mode, and runs the app as the non-root
-   `webhooker` user (UID 1000) through `su-exec`.
 
 The lint stage invokes `golangci-lint` directly rather than `make lint`:
 it is already the pinned linter image, and `make lint` builds
@@ -3272,5 +3297,3 @@ MIT
 ## Author
 
 [@sneak](https://sneak.berlin)
-
-

deploy/docker-entrypoint.sh

@@ -1,22 +0,0 @@
-#!/bin/sh
-# deploy/docker-entrypoint.sh: the image's ENTRYPOINT. A bind-mounted
-# data directory keeps its owner from the host, often root, and the app
-# could not write to it. Started as root, this creates DATA_DIR if
-# needed, gives it and everything in it to webhooker, sets its mode, and
-# runs the command as webhooker, so the app never runs as root. Started
-# as another user, it only runs the command.
-set -eu
-
-main() {
-    if [ "$(id -u)" != 0 ]; then
-        exec "$@"
-    fi
-
-    dir="${DATA_DIR:-/var/lib/webhooker}"
-    mkdir -p "$dir"
-    find "$dir" ! -user webhooker -exec chown -h webhooker:webhooker {} +
-    chmod 750 "$dir"
-    exec su-exec webhooker "$@"
-}
-
-main "$@"

internal/database/bootstrap_banner_test.go

@@ -3,8 +3,6 @@ package database_test
 import (
 	"bytes"
 	"context"
-	"log/slog"
-	"path/filepath"
 	"strings"
 	"testing"
 
@@ -85,37 +83,3 @@ func TestFirstBoot_PrintsTheAdminPasswordAsABanner(t *testing.T) {
 		t, ok, "the printed password must open the seeded account",
 	)
 }
-
-// TestNewDatabase_IsLoggedWithItsPath is the log half of
-// https://git.eeqj.de/sneak/webhooker/issues/359. A DATA_DIR that is
-// unexpectedly empty boots exactly like a first start, so the start
-// that creates the database must say so, and where. Opening that
-// database again must not.
-func TestNewDatabase_IsLoggedWithItsPath(t *testing.T) {
-	t.Parallel()
-
-	dir := t.TempDir()
-
-	open := func() string {
-		var out bytes.Buffer
-
-		db, err := database.Open(dir, slog.New(slog.NewTextHandler(&out, nil)))
-		require.NoError(t, err)
-		require.NoError(t, db.Close())
-
-		return out.String()
-	}
-
-	const created = `level=WARN msg="created a new, empty database"`
-
-	first := open()
-	second := open()
-
-	assert.Contains(
-		t, first,
-		created+" path="+filepath.Join(dir, database.MainDBFileName),
-	)
-	assert.NotContains(
-		t, second, created, "an existing database is not new",
-	)
-}

internal/database/database.go

@@ -8,7 +8,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"io/fs"
 	"log/slog"
 	"os"
 	"path/filepath"
@@ -200,12 +199,6 @@ func (d *Database) connectTo(dataDir string) error {
 	// Construct the main application database path inside DATA_DIR.
 	dbPath := filepath.Join(dataDir, MainDBFileName)
 
-	// Checked before opening, which creates the file. A DATA_DIR that
-	// is unexpectedly empty -- its volume not mounted, say -- looks
-	// exactly like a first start, so a new database is a warning.
-	_, statErr := os.Stat(dbPath)
-	created := errors.Is(statErr, fs.ErrNotExist)
-
 	// Opened through OpenSQLite so this handle carries the same WAL
 	// journaling, busy timeout, immediate-transaction locking, and pool
 	// bounds as every other database file. See sqlite_open.go.
@@ -236,12 +229,7 @@ func (d *Database) connectTo(dataDir string) error {
 	}
 
 	d.db = db
-
+	d.log.Info("connected to database", "path", dbPath)
-	if created {
-		d.log.Warn("created a new, empty database", "path", dbPath)
-	} else {
-		d.log.Info("connected to database", "path", dbPath)
-	}
 
 	// Run migrations
 	return d.migrate()

internal/middleware/ratelimit_test.go

@@ -384,8 +384,8 @@ const (
 	// trustedProxyCIDR is the proxy network the forwarded-path
 	// tests configure, and trustedPeer an address inside it. A
 	// production deployment is required to run behind a reverse
-	// proxy that TRUSTED_PROXIES covers, either by the default or by
+	// proxy with TRUSTED_PROXIES set, so this is the shape the
-	// a set value, so this is the shape the bucketing has to hold in.
+	// bucketing has to hold in.
 	trustedProxyCIDR = "10.0.0.0/8"
 	trustedPeer      = "10.0.0.1:44444"
 )
@@ -1097,9 +1097,8 @@ func TestPostRateLimit_IPv4IndependentPerAddress(t *testing.T) {
 // that arrives from trustedPeer — a configured trusted proxy — and
 // names forwarded as its client in X-Forwarded-For. That is the
 // production path: a deployment is required to run behind a reverse
-// proxy that TRUSTED_PROXIES covers, either by the default or by a
+// proxy with TRUSTED_PROXIES set, so the forwarded address, not the
-// set value, so the forwarded address, not the peer, is what the
+// peer, is what the limiters bucket on there.
-// limiters bucket on there.
 func forwardedKeyFor(
 	t *testing.T, m *middleware.Middleware, forwarded string,
 ) string {
@@ -1179,9 +1178,9 @@ func TestRateLimitKey_ForwardedIPv6BucketsByPrefix(t *testing.T) {
 //
 // Every existing test of this fallback uses an IPv4 proxy, where
 // bucketKey is the identity function, so replacing the call with
-// peer.String() leaves the whole suite green. Only addresses inside
+// peer.String() leaves the whole suite green. Only operator-listed
-// TRUSTED_PROXIES reach this line and the fallback is fail-closed, so
+// addresses reach this line and the fallback is fail-closed, so this
-// this pins behaviour rather than fixing a defect.
+// pins behaviour rather than fixing a defect.
 func TestRateLimitKey_TrustedPeerUnusableForwardedMasksPeer(
 	t *testing.T,
 ) {

internal/server/routes.go

@@ -92,25 +92,11 @@ func (s *Server) setupGlobalMiddleware() {
 func (s *Server) setupRoutes() {
 	s.router.Get("/", s.h.HandleIndex())
 
-	// Static assets answer GET and HEAD only. chi's default 405
+	s.router.Mount(
-	// carries no Allow header, so this group supplies its own.
+		"/s",
-	staticFiles := http.StripPrefix(
+		http.StripPrefix("/s", http.FileServer(http.FS(static.Static))),
-		"/s", http.FileServer(http.FS(static.Static)),
 	)
 
-	s.router.Route("/s", func(r chi.Router) {
-		r.MethodNotAllowed(func(w http.ResponseWriter, _ *http.Request) {
-			w.Header().Set("Allow", "GET, HEAD")
-			http.Error(
-				w,
-				"Method Not Allowed",
-				http.StatusMethodNotAllowed,
-			)
-		})
-		r.Method(http.MethodGet, "/*", staticFiles)
-		r.Method(http.MethodHead, "/*", staticFiles)
-	})
-
 	s.router.Route("/api/v1", func(_ chi.Router) {
 		// API routes will be added here.
 	})

internal/server/routes_test.go

@@ -7,7 +7,6 @@ import (
 	"net/http/httptest"
 	"net/url"
 	"regexp"
-	"slices"
 	"strconv"
 	"strings"
 	"testing"
@@ -221,21 +220,9 @@ func (e *testEnv) csrfFrom(
 	// out of the markup has to be unescaped before it is submitted.
 	token := html.UnescapeString(match[1])
 
-	// A cookie the page sets replaces the one of the same name, as in
+	combined := make([]*http.Cookie, 0, len(cookies))
-	// a browser. Sent both, the server would read the first, older one.
+	combined = append(combined, cookies...)
-	set := w.Result().Cookies()
+	combined = append(combined, w.Result().Cookies()...)
-	combined := make([]*http.Cookie, 0, len(cookies)+len(set))
-
-	for _, c := range cookies {
-		replaced := slices.ContainsFunc(set, func(n *http.Cookie) bool {
-			return n.Name == c.Name
-		})
-		if !replaced {
-			combined = append(combined, c)
-		}
-	}
-
-	combined = append(combined, set...)
 
 	return token, combined
 }
@@ -409,15 +396,13 @@ func (e *testEnv) storedHash(t *testing.T, username string) string {
 
 // --- /s static group ---
 
-// TestStaticServesOnlyGetAndHead pins the methods the static group
+// TestStaticServesEveryMethod pins what the static mount actually
-// answers: GET and HEAD are served the asset, and the other methods
+// answers. chi's Mount registers the handler for all methods and
-// chi routes (POST, PUT, DELETE and the rest) are refused with 405
+// http.FileServer only special-cases HEAD (by suppressing the body),
-// and an Allow header naming those two. A method chi does not route,
+// so a POST or a DELETE to an asset is served the file rather than
-// such as PROPFIND, is refused with 405 by the top-level router
+// refused. The README documents this; the test is what keeps the two
-// before it reaches the static group, so it gets no Allow header.
+// from drifting.
-// The README documents this; the test is what keeps the two from
+func TestStaticServesEveryMethod(t *testing.T) {
-// drifting.
-func TestStaticServesOnlyGetAndHead(t *testing.T) {
 	t.Parallel()
 
 	env := newTestEnv(t)
@@ -432,7 +417,6 @@ func TestStaticServesOnlyGetAndHead(t *testing.T) {
 		http.MethodPost,
 		http.MethodPut,
 		http.MethodDelete,
-		"PROPFIND",
 	} {
 		t.Run(method, func(t *testing.T) {
 			t.Parallel()
@@ -444,38 +428,18 @@ func TestStaticServesOnlyGetAndHead(t *testing.T) {
 			w := httptest.NewRecorder()
 			env.router.ServeHTTP(w, req)
 
-			switch method {
+			assert.Equal(t, http.StatusOK, w.Code,
-			case http.MethodGet:
+				"static mount answers every method")
-				assert.Equal(t, http.StatusOK, w.Code)
+
-				assert.Equal(t, body, w.Body.Bytes(),
+			if method == http.MethodHead {
-					"the asset itself is returned")
-			case http.MethodHead:
-				assert.Equal(t, http.StatusOK, w.Code)
 				assert.Empty(t, w.Body.Bytes(),
 					"HEAD must not carry a body")
-			case "PROPFIND":
+
-				assert.Equal(
+				return
-					t, http.StatusMethodNotAllowed, w.Code,
-				)
-				assert.Empty(t, w.Header().Get("Allow"),
-					"chi refuses a method it does not route "+
-						"before the static group runs")
-				assert.NotContains(
-					t, w.Body.String(), string(body),
-					"a refused method must not get the asset",
-				)
-			default:
-				assert.Equal(
-					t, http.StatusMethodNotAllowed, w.Code,
-				)
-				assert.Equal(
-					t, "GET, HEAD", w.Header().Get("Allow"),
-				)
-				assert.NotContains(
-					t, w.Body.String(), string(body),
-					"a refused method must not get the asset",
-				)
 			}
+
+			assert.Equal(t, body, w.Body.Bytes(),
+				"the asset itself is returned")
 		})
 	}
 }
@@ -627,59 +591,6 @@ func TestPagesLogin_CorrectPasswordSurvivesASpentBudget(
 	)
 }
 
-// TestPagesLogin_CookiesFromAnEarlierDatabase is
-// https://git.eeqj.de/sneak/webhooker/issues/359. A new database
-// brings a new session key, and the operator's browser still holds
-// the session and CSRF cookies signed with the old one. Logging in
-// must work as from a fresh browser and leave cookies the new key
-// accepts.
-func TestPagesLogin_CookiesFromAnEarlierDatabase(t *testing.T) {
-	t.Parallel()
-
-	const (
-		username = "operator"
-		password = "correct-horse-battery-staple"
-	)
-
-	earlier := newTestEnv(t)
-	earlierID, _ := earlier.seedUser(t, username, password)
-	_, stale := earlier.csrfFrom(t, "/pages/login", nil)
-	stale = append(stale, earlier.authCookies(t, earlierID, username)...)
-
-	env := newTestEnv(t)
-	env.seedUser(t, username, password)
-
-	token, cookies := env.csrfFrom(t, "/pages/login", stale)
-
-	form := url.Values{}
-	form.Set("csrf_token", token)
-	form.Set("username", username)
-	form.Set("password", password)
-
-	w := env.post("/pages/login", form, cookies)
-	require.Equal(
-		t, http.StatusSeeOther, w.Code,
-		"a session cookie from another key must not fail the login",
-	)
-
-	// The response deletes the old session cookie and then sets the
-	// new one; a browser keeps the last.
-	var fresh *http.Cookie
-
-	for _, c := range w.Result().Cookies() {
-		if c.Name == session.SessionName {
-			fresh = c
-		}
-	}
-
-	require.NotNil(t, fresh, "login must set a session cookie")
-	assert.Equal(
-		t, "/sources",
-		env.get("/", []*http.Cookie{fresh}).Header().Get("Location"),
-		"the new session cookie must authenticate",
-	)
-}
-
 // --- /user/{username} group ---
 
 // TestPasswordChange_OversizeBody_RejectedAndPasswordUnchanged

internal/session/codec_test.go

@@ -19,8 +19,8 @@ import (
 )
 
 // The tests below exercise the securecookie codecs underneath the
-// store and nothing else: they decode through the store itself, so no
+// store and nothing else: Session.Get only decodes, so no server-side
-// server-side expiry check takes part in the result. They exist because
+// expiry check takes part in the result. They exist because
 // NewCookieStore gives its codecs a 30-day max age that assigning
 // store.Options does not override, which would let the codec accept a
 // cookie weeks past the cap the cookie attribute advertises.
@@ -75,11 +75,10 @@ func restamp(
 	return base64.URLEncoding.EncodeToString(payload)
 }
 
-// decodeCookie feeds value back through the store's decode path. It
+// decodeCookie feeds value back through the store's decode path.
-// asks the store rather than Session.Get, which treats a cookie that
-// does not decode as absent and so hides the codec's reason.
 func decodeCookie(
 	t *testing.T,
+	s *session.Session,
 	value string,
 ) (*sessions.Session, error) {
 	t.Helper()
@@ -95,7 +94,7 @@ func decodeCookie(
 		SameSite: http.SameSiteLaxMode,
 	})
 
-	sess, err := session.NewStore(testKey()).Get(req, session.SessionName)
+	sess, err := s.Get(req)
 	require.NotNil(t, sess)
 
 	return sess, err
@@ -106,7 +105,7 @@ func TestCodec_AcceptsCookieInsideAbsoluteCap(t *testing.T) {
 
 	s := testSession(t)
 
-	sess, err := decodeCookie(t, restamp(
+	sess, err := decodeCookie(t, s, restamp(
 		t,
 		issuedCookie(t, s),
 		time.Now().Add(-(testAbsoluteMaxAge-time.Hour)),
@@ -127,7 +126,7 @@ func TestCodec_RejectsCookiePastAbsoluteCap(t *testing.T) {
 
 	s := testSession(t)
 
-	sess, err := decodeCookie(t, restamp(
+	sess, err := decodeCookie(t, s, restamp(
 		t,
 		issuedCookie(t, s),
 		time.Now().Add(-(testAbsoluteMaxAge+time.Hour)),

internal/session/session.go

@@ -224,22 +224,10 @@ func New(
 }
 
 // Get retrieves a session for the request.
-//
-// A session cookie that does not decode -- one signed with an earlier
-// session key, say, because the database was made anew -- is treated
-// as absent: the caller gets a new, empty session and no error, and
-// the next save replaces the cookie.
 func (s *Session) Get(
 	r *http.Request,
 ) (*sessions.Session, error) {
-	sess, err := s.store.Get(r, SessionName)
+	return s.store.Get(r, SessionName)
-	if sess == nil {
-		return nil, err
-	}
-
-	// For a cookie that does not decode, gorilla/sessions returns a
-	// new, empty session alongside the error that is dropped here.
-	return sess, nil
 }
 
 // GetKey returns the raw 32-byte authentication key used for