Answer 415 for a source whose first bytes do not match its Content-Type (closes #253)

A body whose first bytes are not those of the format its Content-Type
names answered 500 on both image routes, was logged as pixa's own
error, and was fetched again on every request. Both routes now answer
415 for each error the first-bytes check returns, and the image service
logs the refused source at warning level. The failure also gets a
negative cache entry, as an upstream error status does, so the source
URL is not fetched for 5 minutes and answers 502 meanwhile. README.md
says so.

TestService_Get_ConcurrentMissesShareAFailure still expects such a
source to be fetched again and fails until its case is changed.

Model: opus-5-5

README.md

@@ -71,10 +71,9 @@ The state directory (`state_dir`, `/var/lib/pixa` in the container) holds the
 database and the disk cache:
 
 - It needs a persistent volume: without one, every restart starts with an empty
-  cache and forgets the count of logouts, so login sessions that a logout ended
+  cache. In the container, the startup script gives the directory to the user
-  can work again (see Encrypted URLs). In the container, the startup script
+  pixa runs as (uid 65532) and sets its mode to `750`; outside it, that user
-  gives the directory to the user pixa runs as (uid 65532) and sets its mode to
+  must be able to write the directory.
-  `750`; outside it, that user must be able to write the directory.
 - `cache_max_bytes` limits the source and transformed images together. The
   database, the metadata files, the `.meta` file beside each transformed image
   and files still being written come on top, and eviction runs in the
@@ -124,12 +123,8 @@ What the [upaas](https://git.eeqj.de/sneak/upaas) app for pixa needs:
       comma-separated; default none. Every image on a listed host is served to
       anyone without a signature, so list only hosts you control, where nobody
       else can put images.
-    - `PIXA_CACHE_MAX_BYTES`: disk cache limit in bytes; `0` disables it. Set
+    - `PIXA_CACHE_MAX_BYTES`: disk cache limit in bytes; `0` disables it;
-      it: the volume is a directory on the host's filesystem, shared with the
+      default 75% of (free space + what the cache holds)
-      host and every other app. The default is 75% of the sum of that
-      filesystem's free space and what the cache holds, and at least 500 MiB, so
-      the cache can grow to three quarters or more of the space they all have
-      left, and to all of it when that is 500 MiB or less.
     - the rest are in the table under Configuration below
 - **Health check:** the image's `HEALTHCHECK` requests
   `/.well-known/healthcheck.json`. upaas reads the container's health 60 seconds
@@ -189,11 +184,7 @@ path under `/v1/` answers 200, in maintenance mode too.
   session it answers 303 to `/`. Answers: 200 with the page showing the URL; 400
   with the page naming a field that is not valid; 500 when the URL cannot be
   made.
-- `POST /logout` — log out with the generator page's Logout button: end every
+- `GET /logout` — end the login session. Needs: nothing. Answers: 303 to `/`.
-  login session, in every browser (see Encrypted URLs), and remove the session
-  cookie. Needs: a login session and the generator page's form (below); without
-  a login session it only removes the cookie. Answers: 303 to `/`; 500 when the
-  logout cannot be saved in the database.
 - `GET` or `HEAD` `/v1/image/<host>/<path>/<size>.<format>`, or
   `/v1/image/<host>/<path>/<size>` with no format — an image, fetched, resized
   and converted (below). Needs: a signature, unless the host is allowlisted (see
@@ -207,12 +198,13 @@ path under `/v1/` answers 200, in maintenance mode too.
   redirects to, is `localhost`, ends in `.localhost` or `.local`, or has an
   address in a blocked network (see `blocked_networks`); 429 over
   `image_requests_per_minute` (below); 415 when the upstream answers with a
-  `Content-Type` that is not one of the source formats (below), or with an image
+  `Content-Type` that is not one of the source formats (below), with a body
-  whose first bytes libvips reads as another format; 502 when the upstream
+  whose first bytes are not those of the format its `Content-Type` names, or
-  answered with an error status, and for 5 minutes after that for an image of
+  with an image whose first bytes libvips reads as another format; 502 when the
-  the same source URL that pixa can neither serve from its cache nor make from
+  upstream answered with an error status, and for 5 minutes after that, or after
-  the source image still in its cache; 503 when pixa is busy or in maintenance
+  a body whose first bytes are not those of its `Content-Type`, for the same
-  mode; 500 for any other failure.
+  source URL; 503 when pixa is busy or in maintenance mode; 500 for any other
+  failure.
 - `GET` or `HEAD` `/v1/e/<token>/<name>` — an image through an encrypted URL
   (see Encrypted URLs). Needs: nothing but the URL. Answers: 200; 304 when
   `If-None-Match` matches the image's `ETag`; 400 for a token that does not
@@ -241,7 +233,7 @@ the request carries the same ID as `request_id`, and so do the lines it logs
 when it fetches, converts and serves an image; the fetch sends it to the
 upstream host as `X-Request-ID`.
 
-The `POST` routes accept only a form that pixa's own page served: the page puts
+Both `POST` routes accept only a form that pixa's own page served: the page puts
 a token in the form and sets a cookie to match, and a request without both is
 refused with 403, so another site cannot submit the form from a visitor's
 browser. The login and generator pages are meant to be opened over HTTPS: while
@@ -289,8 +281,12 @@ above 8192.
 The source image may be JPEG, PNG, GIF, WebP, AVIF or JPEG XL. The upstream's
 `Content-Type` must name its format, and the image's first bytes must match it.
 An upstream answering with any other `Content-Type`, SVG included, is refused
-with 415, and so is an image whose first bytes libvips reads as another format,
+with 415. So is a body whose first bytes are not those of the format its
-such as an SVG whose first bytes pass the check for AVIF.
+`Content-Type` names, such as a JPEG sent as `image/png`, text sent as an image,
+or an empty body. pixa then does not fetch that source URL again for 5 minutes,
+and answers 502 for it meanwhile, as after an upstream error status. An image
+whose first bytes libvips reads as another format, such as an SVG whose first
+bytes pass the check for AVIF, is refused with 415 too.
 
 With the format `auto`, pixa chooses the format for each request from its
 `Accept` header, in this order:
@@ -362,14 +358,7 @@ nor change what it asks for.
 
 1. Open `/` in a browser over HTTPS (or over plain HTTP while `debug` is on, see
    Routes) and log in with the signing key (`signing_key`). The login session
-   lasts 30 days, or until someone logs out with the generator page's Logout
+   lasts 30 days, or until `/logout`.
-   button. Every login is made with the one signing key, so logging out ends
-   every login session, in every browser, a copy of the session cookie included.
-   pixa keeps a count of logouts in its database: each logout adds one, a login
-   session carries the count at its login, and a login session whose count is
-   not the current one is refused, also after a restart that keeps the state
-   directory (see Deployment). No time is compared, so a login after a logout
-   works whatever the host's clock says.
 2. On the generator page, give the source image's URL, the width and height, the
    format, quality and fit, and how long the URL lasts, then submit the form
    (`POST /generate`). The format is JPEG XL unless another is chosen; a form
@@ -435,9 +424,7 @@ HMAC-SHA256(secret, "host:path:query:width:height:format:expiration:quality:fit"
 
 Where:
 
-- `host` — source host as the URL writes it up to the first `/`, in lower case,
+- `host` — source origin hostname (e.g. `cdn.example.com`)
-  keeping an IPv6 address's brackets and any `:` and port after it (e.g.
-  `cdn.example.com`, `cdn.example.com:8443`, `[2001:db8::1]:8443`)
 - `path` — source path (e.g. `/photos/cat.jpg`), with each `%` written as `%25`
   and each `:` as `%3A`, so `/photos/a:b.jpg` is signed as `/photos/a%3Ab.jpg`
 - `query` — source query string, empty string if none, with `%` and `:` written
@@ -492,12 +479,6 @@ entry that is neither a host name (letters, digits, hyphens, underscores and
 dots, with at most one leading dot) nor an IP address, such as one with a port
 or a `*.` wildcard, aborts startup.
 
-pixa lower-cases the host of a `/v1/image/` URL before anything else uses it, so
-`CDN.Example.com` is matched, fetched, cached and counted against
-`upstream_connections_per_host` as `cdn.example.com`. Such a URL whose host has
-a port, as in `cdn.example.com:8443`, or a `:` with no port, needs a signature
-even when the host is allowlisted. A host holding `@` is refused with 400.
-
 ### Configuration
 
 Every setting can be given as an environment variable, in a YAML config file
@@ -656,10 +637,7 @@ See `configs/config.example.yml` for all options with defaults.
 - **Metrics**: Prometheus, at `/metrics`: generic HTTP request metrics
   (duration, response size, requests in flight) and the Go runtime and process
   metrics; requests are measured and `/metrics` is served only when
-  `metrics.username` and `metrics.password` are set. A request's `handler` label
+  `metrics.username` and `metrics.password` are set
-  is the pattern of the route it matches, such as `/v1/image/*`, never its path.
-  Requests that match no route share the label `unmatched`: those the router
-  answers 404 or 405, and CORS preflights
 - **Logging**: stdlib slog
 
 ## Entrypoints

TODO.md

@@ -30,70 +30,15 @@ P2: security: per-origin rate limiting
 
 # Completed Steps
 
-- 2026-10-15 "Running under upaas" in `README.md` tells the operator to set
+- 2026-10-15 a source whose first bytes do not match its `Content-Type` answers
-  `PIXA_CACHE_MAX_BYTES` (closes #251): it listed the variable as optional, but
+  415 (closes #253): a body whose first bytes are not those of the format its
-  under upaas the volume is a directory on the host's filesystem. The default,
+  `Content-Type` names, such as a JPEG sent as `image/png`, text, or a body
-  75% of the sum of that filesystem's free space and what the cache holds, and
+  under 12 bytes, answered 500 on both image routes and was logged as an error
-  at least 500 MiB, let one pixa take three quarters or more of the space the
+  of pixa's, and each request for it fetched the whole body again. Both routes
-  host and every other app had left, and all of it when that was 500 MiB or
+  now answer 415, and the image service logs it at warning level as a refused
-  less. The section now says so. The default is unchanged.
+  source. It also gets a negative cache entry, as an upstream error status does,
-- 2026-10-15 the request metrics are labelled by route, not by path (closes
+  so the source URL is not fetched again for 5 minutes and answers 502
-  #254): their `handler` label was each request's path, so every new path, a 404
+  meanwhile.
-  or an encrypted URL, added series to the Prometheus registry that were never
-  removed, and `/metrics` listed every encrypted URL's token. The label is now
-  the pattern of the chi route the request matches, such as `/v1/image/*`, or
-  `unmatched` for a request that matches none: one the router answers 404 or
-  405, or a CORS preflight. The metrics library takes the label before the
-  request is routed, so the metrics middleware asks the router which route
-  matches.
-- 2026-10-15 logging out ends every login session (closes #256): a login session
-  was a cookie with nothing kept on the server, so `/logout` removed it from one
-  browser and a copy of it kept working for its 30 days. Every login is made
-  with the one signing key, so logging out now ends them all: the session
-  manager counts logouts, saves the count in the new `logout` table so that it
-  outlasts a restart, puts the count at login in each session, and refuses a
-  session whose count is not the current one. No time is compared, so a login
-  after a logout works whatever the host's clock does. A restart without a
-  persistent volume for the state directory forgets the count. Logging out is
-  now `POST /logout` from a form on the generator page, behind the CSRF check
-  the other forms have, so a link or image elsewhere on the same site cannot end
-  the sessions. A logout without a valid login session ends none and only
-  removes the cookie, as anyone can take a CSRF token from the login page. A
-  logout that cannot be saved answers 500. `session.NewManager`, which the tests
-  use, keeps the count in memory only; pixad uses
-  `session.NewManagerWithDatabase`.
-- 2026-10-15 expired negative cache rows are removed on a schedule (closes
-  #244): the row of a failed fetch, which makes pixa answer 502 for that source
-  URL for 5 minutes, was deleted only when the same URL was requested again
-  after that, so a client asking for a new missing path each time added rows
-  that stayed for good. The eviction goroutine now calls `CleanExpired` at
-  startup and on each periodic tick, every 5 minutes in pixad, so a row stays at
-  most about 10 minutes. `CleanExpired` deletes at most 1000 rows per statement,
-  so a request waits behind 1000 deletions at most, however many rows have
-  expired. The goroutine runs with the disk cache disabled
-  (`cache_max_bytes: 0`) too, where it does nothing else, as the negative cache
-  is used either way.
-- 2026-10-15 a cached variant is served while its source URL is in the negative
-  cache (closes #245): `Get` checked the negative cache before the variant
-  cache, so once a fetch of the source failed, as one does when another variant
-  misses after the source was evicted and the upstream is down, every variant of
-  that source answered 502 for 5 minutes, also one on disk or one that could be
-  made from the cached source. The negative cache is now checked only just
-  before a fetch from the upstream, so a cached variant is served and a variant
-  not in the cache is made from the cached source when there is one. `README.md`
-  says so under `/v1/image/`. A refusal from the negative cache now counts as a
-  miss, as any other request that misses the variant cache does.
-- 2026-10-15 the host of a `/v1/image/` URL is lower-cased, and one with a port
-  needs a signature (closes #246): with `cdn.example.com` allowlisted,
-  `CDN.Example.com` and `cdn.example.com:8443` passed the allowlist too, and
-  each spelling was fetched and cached on its own and had connections of its own
-  under `upstream_connections_per_host`. The image URL parser now lower-cases
-  the host and refuses one holding `@`, which a URL reads as a user name and
-  password, with 400. A host with a port, or with a `:` and no port, needs a
-  signature even when it is allowlisted. That is checked in the image service,
-  not in `allowlist.IsAllowed`, which also matches `referer_blocklist`, where a
-  `Referer` is blocked whatever its port. A URL is signed over its host as it
-  writes it, in lower case, with any IPv6 brackets, `:` and port kept.
 - 2026-10-15 a `:` in the source path or query no longer moves where the path
   ends and the query begins (closes #247): the signed data and the cache key
   joined host, path and query with `:` and no escaping, so a URL signed for

internal/db/migrations/001_schema.sql

@@ -176,12 +176,3 @@ CREATE TABLE IF NOT EXISTS cache_stats (
     last_updated_at DATETIME DEFAULT CURRENT_TIMESTAMP
 );
 INSERT OR IGNORE INTO cache_stats (id) VALUES (1);
-
--- Count of logouts. Each logout adds one; a login session carries the count
--- at its login and is refused once the count has changed, so logging out ends
--- every login session.
-CREATE TABLE IF NOT EXISTS logout (
-    id INTEGER PRIMARY KEY CHECK (id = 1),
-    logout_count INTEGER NOT NULL DEFAULT 0
-);
-INSERT OR IGNORE INTO logout (id) VALUES (1);

internal/handlers/auth.go

@@ -89,18 +89,10 @@ func (s *Handlers) handleLoginPost(w http.ResponseWriter, r *http.Request) {
 	http.Redirect(w, r, "/", http.StatusSeeOther)
 }
 
-// HandleLogout logs out, which ends every login session (see
+// HandleLogout clears the session and redirects to login.
-// session.Manager.Logout), and redirects to login.
 func (s *Handlers) HandleLogout() http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {
-		err := s.sessMgr.Logout(w, r)
+		s.sessMgr.ClearSession(w)
-		if err != nil {
-			s.log.Error("failed to log out", "error", err)
-			http.Error(w, "Failed to log out", http.StatusInternalServerError)
-
-			return
-		}
-
 		http.Redirect(w, r, "/", http.StatusSeeOther)
 	}
 }

internal/handlers/auth_session_internal_test.go

@@ -185,98 +185,6 @@ func TestHandleLogout_ClearsSessionCookie(t *testing.T) {
 	}
 }
 
-// TestHandleLogout_EndsCopiedSession verifies that after POST /logout a copy
-// of the session cookie taken before it shows the login page, not the
-// generator page.
-func TestHandleLogout_EndsCopiedSession(t *testing.T) {
-	t.Parallel()
-
-	h, srv := newCSRFTestRouter(t)
-
-	copied := newSessionCookie(t, h)
-
-	req := httptest.NewRequestWithContext(
-		t.Context(), http.MethodPost, "/logout", nil)
-	req.AddCookie(copied)
-
-	rec := httptest.NewRecorder()
-	h.HandleLogout().ServeHTTP(rec, req)
-
-	if rec.Code != http.StatusSeeOther {
-		t.Fatalf("POST /logout status = %d, want %d",
-			rec.Code, http.StatusSeeOther)
-	}
-
-	req = httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil)
-	req.AddCookie(copied)
-
-	rec = httptest.NewRecorder()
-	srv.ServeHTTP(rec, req)
-
-	body := rec.Body.String()
-	t.Logf("GET / with the copied cookie: status %d, login page %v",
-		rec.Code, strings.Contains(body, loginForm))
-
-	if !strings.Contains(body, loginForm) || strings.Contains(body, generatorForm) {
-		t.Error("a cookie copied before /logout still opens the generator page")
-	}
-}
-
-// TestHandleLogout_SaveFails_Answers500 verifies that when the logout cannot
-// be saved in the database, POST /logout answers 500, leaves the session
-// cookie in place and ends no session.
-func TestHandleLogout_SaveFails_Answers500(t *testing.T) {
-	t.Parallel()
-
-	h, srv := newCSRFTestRouter(t)
-
-	db := setupTestDB(t)
-
-	sessMgr, err := session.NewManagerWithDatabase(
-		t.Context(), testSigningKey, db)
-	if err != nil {
-		t.Fatalf("session.NewManagerWithDatabase() error = %v", err)
-	}
-
-	h.sessMgr = sessMgr
-	cookie := newSessionCookie(t, h)
-
-	// Saving the logout fails on a closed database.
-	err = db.Close()
-	if err != nil {
-		t.Fatalf("closing the database: %v", err)
-	}
-
-	req := httptest.NewRequestWithContext(
-		t.Context(), http.MethodPost, "/logout", nil)
-	req.AddCookie(cookie)
-
-	rec := httptest.NewRecorder()
-	h.HandleLogout().ServeHTTP(rec, req)
-
-	t.Logf("POST /logout: status %d, Set-Cookie %q",
-		rec.Code, rec.Header().Get("Set-Cookie"))
-
-	if rec.Code != http.StatusInternalServerError {
-		t.Errorf("POST /logout status = %d, want %d",
-			rec.Code, http.StatusInternalServerError)
-	}
-
-	if c := findSessionCookie(rec); c != nil {
-		t.Errorf("a logout that failed set the session cookie: %s", c)
-	}
-
-	req = httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil)
-	req.AddCookie(cookie)
-
-	rec = httptest.NewRecorder()
-	srv.ServeHTTP(rec, req)
-
-	if !strings.Contains(rec.Body.String(), generatorForm) {
-		t.Error("a logout that failed ended the session")
-	}
-}
-
 // TestGeneratePost_NoSession_RedirectsToLogin verifies that POST /generate
 // with a valid CSRF token but no login session answers 303 to / and makes no
 // URL.

internal/handlers/first_bytes_internal_test.go

@@ -0,0 +1,180 @@
+package handlers
+
+import (
+	"bytes"
+	"encoding/json"
+	"image/color"
+	"log/slog"
+	"net/http"
+	"slices"
+	"testing"
+	"testing/fstest"
+	"time"
+
+	"github.com/go-chi/chi/v5"
+	"sneak.berlin/go/pixa/internal/encurl"
+	"sneak.berlin/go/pixa/internal/httpfetcher"
+	"sneak.berlin/go/pixa/internal/imgcache"
+)
+
+// mismatchedSourcePath is the path of the source that
+// TestFirstBytesNotMatchingContentType serves on allowlistedHost. The mock
+// fetcher names a file's Content-Type from its extension, so the source is
+// sent as image/png.
+const mismatchedSourcePath = "/images/source.png"
+
+// newRoutesFetchingThrough returns both image routes, and the Handlers behind
+// them, whose image service fetches through fetcher and has allowlistedHost on
+// its allowlist. The handlers and the image service log to log.
+func newRoutesFetchingThrough(
+	t *testing.T, fetcher httpfetcher.Fetcher, log *slog.Logger,
+) (*Handlers, http.Handler) {
+	t.Helper()
+
+	cache, err := imgcache.NewCache(setupTestDB(t), imgcache.CacheConfig{
+		StateDir:    t.TempDir(),
+		NegativeTTL: 5 * time.Minute,
+	})
+	if err != nil {
+		t.Fatalf("imgcache.NewCache() error = %v", err)
+	}
+
+	svc, err := imgcache.NewService(&imgcache.ServiceConfig{
+		Cache:      cache,
+		Fetcher:    fetcher,
+		SigningKey: testSigningKey,
+		Allowlist:  []string{allowlistedHost},
+		Logger:     log,
+	})
+	if err != nil {
+		t.Fatalf("imgcache.NewService() error = %v", err)
+	}
+
+	encGen, err := encurl.NewGenerator(testSigningKey)
+	if err != nil {
+		t.Fatalf("encurl.NewGenerator() error = %v", err)
+	}
+
+	h := &Handlers{log: log, imgSvc: svc, encGen: encGen}
+
+	r := chi.NewRouter()
+	r.Get("/v1/image/*", h.HandleImage())
+	r.Get("/v1/e/{token}/*", h.HandleImageEnc())
+
+	return h, r
+}
+
+// mismatchedSourcePlainURL returns the /v1/image/ URL of mismatchedSourcePath
+// on allowlistedHost at 50x50 as a JPEG.
+func mismatchedSourcePlainURL(*testing.T, *Handlers) string {
+	return "/v1/image/" + allowlistedHost + mismatchedSourcePath + "/50x50.jpeg"
+}
+
+// mismatchedSourceEncURL returns an encrypted /v1/e/ URL, made with h's
+// generator, of mismatchedSourcePath on allowlistedHost at 50x50 as a JPEG.
+func mismatchedSourceEncURL(t *testing.T, h *Handlers) string {
+	t.Helper()
+
+	token, err := h.encGen.Generate(&encurl.Payload{
+		SourceHost: allowlistedHost,
+		SourcePath: mismatchedSourcePath,
+		Width:      50,
+		Height:     50,
+		Format:     imgcache.FormatJPEG,
+	})
+	if err != nil {
+		t.Fatalf("Generate() error = %v", err)
+	}
+
+	return "/v1/e/" + token + "/img.jpg"
+}
+
+// loggedLevels reads the JSON log lines in logs and returns the messages
+// logged at each level.
+func loggedLevels(t *testing.T, logs *bytes.Buffer) map[string][]string {
+	t.Helper()
+
+	t.Logf("logged:\n%s", logs.String())
+
+	messages := make(map[string][]string)
+
+	dec := json.NewDecoder(logs)
+	for dec.More() {
+		var line struct {
+			Level string `json:"level"`
+			Msg   string `json:"msg"`
+		}
+
+		err := dec.Decode(&line)
+		if err != nil {
+			t.Fatalf("decoding log line: %v", err)
+		}
+
+		messages[line.Level] = append(messages[line.Level], line.Msg)
+	}
+
+	return messages
+}
+
+// TestFirstBytesNotMatchingContentType checks that both image routes answer
+// 415 for an upstream body whose first bytes are not those of the format its
+// Content-Type names, and log it as a refused source, not as an error of
+// pixa's. The next request for that source must answer 502 without fetching
+// it again, as after an upstream error status.
+func TestFirstBytesNotMatchingContentType(t *testing.T) {
+	t.Parallel()
+
+	bodies := map[string][]byte{
+		"JPEG sent as image/png":          generateTestJPEG(t, 10, 10, color.Black),
+		"text sent as image/png":          []byte("This is just text, not a PNG"),
+		"body too short to name a format": []byte("short"),
+	}
+
+	// The source's URL on each image route.
+	sourceURLs := map[string]func(*testing.T, *Handlers) string{
+		"plain URL":     mismatchedSourcePlainURL,
+		"encrypted URL": mismatchedSourceEncURL,
+	}
+
+	for bodyName, body := range bodies {
+		for urlName, sourceURL := range sourceURLs {
+			t.Run(urlName+", "+bodyName, func(t *testing.T) {
+				t.Parallel()
+
+				fetcher := &countingFetcher{
+					Fetcher: httpfetcher.NewMock(fstest.MapFS{
+						allowlistedHost + mismatchedSourcePath: &fstest.MapFile{
+							Data: body,
+						},
+					}),
+				}
+
+				var logs bytes.Buffer
+
+				h, routes := newRoutesFetchingThrough(t, fetcher,
+					slog.New(slog.NewJSONHandler(&logs, nil)))
+				target := sourceURL(t, h)
+
+				checkErrorBody(t, sendGet(t, routes, target),
+					http.StatusUnsupportedMediaType, "unsupported source format")
+
+				levels := loggedLevels(t, &logs)
+				if len(levels[slog.LevelError.String()]) != 0 {
+					t.Errorf("logged at error level: %q",
+						levels[slog.LevelError.String()])
+				}
+
+				if !slices.Contains(levels[slog.LevelWarn.String()], "source refused") {
+					t.Error(`no "source refused" line logged at warning level`)
+				}
+
+				checkErrorBody(t, sendGet(t, routes, target),
+					http.StatusBadGateway, "upstream error")
+
+				if n := fetcher.fetches.Load(); n != 1 {
+					t.Errorf("upstream fetched %d times, want 1", n)
+				}
+			})
+		}
+	}
+}

internal/handlers/handlers.go

@@ -124,8 +124,7 @@ func (s *Handlers) initImageService() error {
 	s.imgCache = cache
 
 	// Background eviction: startup reconciliation, then periodic and
-	// write-pressure passes, and the removal of expired negative cache
+	// write-pressure passes. No-op when the disk cache is disabled.
-	// rows, which is all it does when the disk cache is disabled.
 	cache.StartEviction(imgcache.DefaultEvictionInterval)
 
 	// Writes the counts requests could not write by their deadline
@@ -164,10 +163,7 @@ func (s *Handlers) initImageService() error {
 
 	// Initialize session manager (signing key is validated at config load
 	// time). Session cookies are always Secure/HttpOnly/SameSite=Strict.
-	// The count of logouts is saved in the database, so it outlasts a
+	sessMgr, err := session.NewManager(s.config.SigningKey)
-	// restart.
-	sessMgr, err := session.NewManagerWithDatabase(
-		context.Background(), s.config.SigningKey, s.db.DB())
 	if err != nil {
 		return err
 	}

internal/handlers/image.go

@@ -15,6 +15,7 @@ import (
 	"sneak.berlin/go/pixa/internal/httpfetcher"
 	"sneak.berlin/go/pixa/internal/imageprocessor"
 	"sneak.berlin/go/pixa/internal/imgcache"
+	"sneak.berlin/go/pixa/internal/magic"
 )
 
 // HandleImage handles the main image proxy route:
@@ -220,6 +221,18 @@ func (s *Handlers) HandleTooManyRequests() http.HandlerFunc {
 func (s *Handlers) respondImageError(
 	w http.ResponseWriter, req *imgcache.ImageRequest, err error,
 ) {
+	// An upstream body whose first bytes are not those of the format its
+	// Content-Type names is the upstream's error, not pixa's: the image
+	// service has logged it as a refused source.
+	if errors.Is(err, magic.ErrMagicByteMismatch) ||
+		errors.Is(err, magic.ErrUnknownFormat) ||
+		errors.Is(err, magic.ErrNotEnoughData) {
+		s.respondError(w, "unsupported source format",
+			http.StatusUnsupportedMediaType)
+
+		return
+	}
+
 	s.log.Error("failed to get image",
 		"host", req.SourceHost,
 		"path", req.SourcePath,

internal/handlers/imageenc.go

@@ -15,6 +15,7 @@ import (
 	"sneak.berlin/go/pixa/internal/httpfetcher"
 	"sneak.berlin/go/pixa/internal/imageprocessor"
 	"sneak.berlin/go/pixa/internal/imgcache"
+	"sneak.berlin/go/pixa/internal/magic"
 )
 
 // HandleImageEnc handles requests to /v1/e/{token}/* for encrypted
@@ -162,7 +163,10 @@ func (s *Handlers) handleImageError(w http.ResponseWriter, err error) {
 		s.respondError(w, "server busy, try again later",
 			http.StatusServiceUnavailable)
 	case errors.Is(err, httpfetcher.ErrInvalidContentType),
-		errors.Is(err, imageprocessor.ErrUnsupportedInputFormat):
+		errors.Is(err, imageprocessor.ErrUnsupportedInputFormat),
+		errors.Is(err, magic.ErrMagicByteMismatch),
+		errors.Is(err, magic.ErrUnknownFormat),
+		errors.Is(err, magic.ErrNotEnoughData):
 		s.respondError(w, "unsupported source format",
 			http.StatusUnsupportedMediaType)
 	case errors.Is(err, imageprocessor.ErrOutputTooLarge):

internal/imgcache/cache.go

@@ -32,12 +32,6 @@ const httpStatusOK = 200
 // variant not among them is served as before, reading its .meta file.
 const metaCacheSize = 10000
 
-// cleanExpiredBatchSize is the most expired negative cache rows one
-// statement of CleanExpired deletes. Each batch is a statement of its
-// own, so a request waits behind one batch at most, however many rows
-// have expired.
-const cleanExpiredBatchSize = 1000
-
 // CacheConfig holds cache configuration.
 type CacheConfig struct {
 	StateDir    string
@@ -56,9 +50,8 @@ type CacheConfig struct {
 
 	// DisableDiskCache turns the disk cache off entirely: no cache
 	// directories are created, lookups always miss, stores are
-	// no-ops, and the eviction goroutine only removes the expired
+	// no-ops, and no eviction machinery runs. The config layer sets
-	// negative cache rows. The config layer sets this when the
+	// this when the operator configures cache_max_bytes: 0.
-	// operator configures cache_max_bytes: 0.
 	DisableDiskCache bool
 
 	// Logger receives accounting and eviction log output. A nil
@@ -489,27 +482,17 @@ func (c *Cache) GetSourceContent(
 	return c.srcContent.LoadWithSize(contentHash)
 }
 
-// CleanExpired removes the negative cache rows past their expiry, one
+// CleanExpired removes expired entries from the cache.
-// batch per statement, until none is left. Once ctx ends it starts no
-// further batch and returns ctx's error. The eviction goroutine calls it
-// at startup and on every periodic tick, so a row stays about
-// NegativeTTL plus one eviction interval, whether or not its URL is
-// requested again.
 func (c *Cache) CleanExpired(ctx context.Context) error {
-	for {
+	// Clean expired negative cache entries
-		if ctx.Err() != nil {
+	_, err := c.db.ExecContext(ctx, `
-			return ctx.Err()
+		DELETE FROM negative_cache WHERE expires_at < CURRENT_TIMESTAMP
-		}
+	`)
-
+	if err != nil {
-		deleted, err := c.cleanExpiredBatch(ctx)
+		return fmt.Errorf("failed to clean negative cache: %w", err)
-		if err != nil {
-			return err
-		}
-
-		if deleted < cleanExpiredBatchSize {
-			return nil
-		}
 	}
+
+	return nil
 }
 
 // Stats returns cache statistics.
@@ -773,27 +756,3 @@ func (c *Cache) checkNegativeCache(
 
 	return true, nil
 }
-
-// cleanExpiredBatch deletes up to cleanExpiredBatchSize negative cache
-// rows past their expiry in one statement, and returns how many it
-// deleted. DELETE ... LIMIT needs a SQLite build option, so the subquery
-// picks the rows.
-func (c *Cache) cleanExpiredBatch(ctx context.Context) (int64, error) {
-	result, err := c.db.ExecContext(ctx, `
-		DELETE FROM negative_cache WHERE id IN (
-			SELECT id FROM negative_cache
-			WHERE expires_at < CURRENT_TIMESTAMP
-			LIMIT ?
-		)
-	`, cleanExpiredBatchSize)
-	if err != nil {
-		return 0, fmt.Errorf("failed to clean negative cache: %w", err)
-	}
-
-	deleted, err := result.RowsAffected()
-	if err != nil {
-		return 0, fmt.Errorf("failed to count cleaned negative cache rows: %w", err)
-	}
-
-	return deleted, nil
-}

internal/imgcache/eviction.go

@@ -437,16 +437,14 @@ func (c *Cache) notifyWritePressure() {
 }
 
 // StartEviction launches the background eviction goroutine, which
-// reconciles the database accounting with the cache directories and
+// reconciles the database accounting with the cache directories at
-// removes the expired negative cache rows at startup and again on every
+// startup and again on every periodic tick thereafter, and evicts to
-// periodic tick thereafter, and evicts to the configured limit on the
+// the configured limit on the given periodic interval and on
-// given periodic interval and on write-pressure notifications. On a
+// write-pressure notifications. It is a no-op on a disabled cache or
-// disabled cache it only removes the expired negative cache rows, as
+// when already started. The goroutine outlives the caller, so it runs
-// the negative cache is used either way. It is a no-op when already
+// with its own context, which StopEviction cancels.
-// started. The goroutine outlives the caller, so it runs with its own
-// context, which StopEviction cancels.
 func (c *Cache) StartEviction(interval time.Duration) {
-	if c.evictionCancel != nil {
+	if c.disabled || c.evictionCancel != nil {
 		return
 	}
 
@@ -482,7 +480,6 @@ func (c *Cache) evictionLoop(ctx context.Context, interval time.Duration) {
 	defer close(c.evictionDone)
 
 	c.runReconciliationPass(ctx)
-	c.runCleanExpired(ctx)
 	c.runEvictionPass(ctx)
 
 	ticker := time.NewTicker(interval)
@@ -503,7 +500,6 @@ func (c *Cache) evictionLoop(ctx context.Context, interval time.Duration) {
 			// unaccounted for to one interval, on a process that is
 			// already running this loop regardless.
 			c.runReconciliationPass(ctx)
-			c.runCleanExpired(ctx)
 		case <-c.evictionPressure:
 		}
 
@@ -539,20 +535,6 @@ func (c *Cache) runReconciliationPass(ctx context.Context) {
 	}
 }
 
-// runCleanExpired runs CleanExpired, logging a failure instead of
-// propagating it (the loop must keep running). It does nothing once ctx
-// is cancelled.
-func (c *Cache) runCleanExpired(ctx context.Context) {
-	if ctx.Err() != nil {
-		return
-	}
-
-	err := c.CleanExpired(ctx)
-	if err != nil {
-		c.log.Warn("negative cache cleanup failed", "error", err)
-	}
-}
-
 // reconcileAccounting synchronizes the database size accounting with
 // the actual contents of the cache directories. It runs at startup and
 // again on every periodic eviction tick thereafter, off the request

internal/imgcache/negative_cache_internal_test.go

@@ -3,7 +3,6 @@ package imgcache
 import (
 	"context"
 	"errors"
-	"strconv"
 	"testing"
 	"time"
 )
@@ -95,307 +94,6 @@ func TestNegativeCache_Expired(t *testing.T) {
 	}
 }
 
-// TestService_Get_ServesCachedVariantOfNegativeCachedURL caches a variant of
-// the test photo, then keeps a failed fetch of the photo's URL in the negative
-// cache, as a failed fetch for another variant does once the source has been
-// evicted. The cached variant is still served, and a variant not in the cache
-// is made from the cached source. Once the source is evicted, a variant not in
-// the cache needs a fetch and is refused.
-func TestService_Get_ServesCachedVariantOfNegativeCachedURL(t *testing.T) {
-	t.Parallel()
-
-	svc, fixtures := SetupTestService(t)
-
-	resp, err := svc.Get(t.Context(), photoVariant(fixtures, 85, FitCover))
-	if err != nil {
-		t.Fatalf("Get() first request error = %v", err)
-	}
-
-	_ = resp.Content.Close()
-
-	err = svc.cache.StoreNegative(t.Context(), photoVariant(fixtures, 85, FitCover),
-		httpStatusBadGateway, "upstream answered 503")
-	if err != nil {
-		t.Fatalf("StoreNegative() error = %v", err)
-	}
-
-	resp, err = svc.Get(t.Context(), photoVariant(fixtures, 85, FitCover))
-	if err != nil {
-		t.Fatalf("Get() of the cached variant error = %v", err)
-	}
-
-	_ = resp.Content.Close()
-
-	if resp.CacheStatus != CacheHit {
-		t.Errorf("cached variant CacheStatus = %q, want %q",
-			resp.CacheStatus, CacheHit)
-	}
-
-	resp, err = svc.Get(t.Context(), photoVariant(fixtures, 40, FitCover))
-	t.Logf("Get() of a variant made from the cached source: error = %v", err)
-
-	if err != nil {
-		t.Fatalf("Get() of a variant made from the cached source error = %v", err)
-	}
-
-	_ = resp.Content.Close()
-
-	contentHash, _, err := svc.cache.LookupSource(t.Context(),
-		photoVariant(fixtures, 85, FitCover))
-	if err != nil || contentHash == "" {
-		t.Fatalf("LookupSource() = %q, %v, want the cached source", contentHash, err)
-	}
-
-	err = svc.cache.evictSourceBlob(t.Context(), contentHash)
-	if err != nil {
-		t.Fatalf("evictSourceBlob() error = %v", err)
-	}
-
-	_, err = svc.Get(t.Context(), photoVariant(fixtures, 60, FitCover))
-	t.Logf("Get() of a variant whose source was evicted: error = %v", err)
-
-	if !errors.Is(err, ErrNegativeCached) {
-		t.Errorf("Get() of a variant whose source was evicted error = %v, want %v",
-			err, ErrNegativeCached)
-	}
-}
-
-// TestEvictionLoopRemovesExpiredNegativeCacheRows stores a failed fetch
-// that expires a second later and starts the eviction loop with a short
-// interval: a later tick must remove its row although its URL is never
-// requested again, and must keep a row that has not expired. It runs
-// with the disk cache enabled and disabled, as the negative cache is
-// used either way.
-func TestEvictionLoopRemovesExpiredNegativeCacheRows(t *testing.T) {
-	t.Parallel()
-
-	tests := []struct {
-		name             string
-		disableDiskCache bool
-	}{
-		{name: "disk cache enabled", disableDiskCache: false},
-		{name: "disk cache disabled", disableDiskCache: true},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
-
-			cache, err := NewCache(evictionTestDB(t), CacheConfig{
-				StateDir:         t.TempDir(),
-				NegativeTTL:      time.Second,
-				DisableDiskCache: tt.disableDiskCache,
-			})
-			if err != nil {
-				t.Fatalf("failed to create cache: %v", err)
-			}
-
-			expired := &ImageRequest{
-				SourceHost: testHostExample, SourcePath: "/gone.jpg",
-			}
-
-			err = cache.StoreNegative(t.Context(), expired, 404, "not found")
-			if err != nil {
-				t.Fatalf("StoreNegative failed: %v", err)
-			}
-
-			const notExpiredPath = "/later.jpg"
-
-			_, err = cache.db.ExecContext(t.Context(), `
-				INSERT INTO negative_cache
-					(source_host, source_path, status_code, expires_at)
-				VALUES (?, ?, 404, datetime('now', '+1 hour'))
-			`, testHostExample, notExpiredPath)
-			if err != nil {
-				t.Fatalf("failed to insert a row that has not expired: %v", err)
-			}
-
-			started := time.Now()
-
-			cache.StartEviction(100 * time.Millisecond)
-			defer func() { _ = cache.StopEviction(t.Context()) }()
-
-			for negativeCacheRowCount(t, cache, expired.SourcePath) > 0 {
-				if time.Since(started) > 10*time.Second {
-					t.Fatal("the expired row is still in negative_cache")
-				}
-
-				time.Sleep(50 * time.Millisecond)
-			}
-
-			t.Logf("the expired row was removed %s after the loop started",
-				time.Since(started))
-
-			if negativeCacheRowCount(t, cache, notExpiredPath) != 1 {
-				t.Error("the row that has not expired was removed")
-			}
-		})
-	}
-}
-
-// negativeCacheRowCount returns how many negative_cache rows hold
-// sourcePath.
-func negativeCacheRowCount(tb testing.TB, cache *Cache, sourcePath string) int {
-	tb.Helper()
-
-	var count int
-
-	err := cache.db.QueryRowContext(tb.Context(),
-		`SELECT COUNT(*) FROM negative_cache WHERE source_path = ?`,
-		sourcePath,
-	).Scan(&count)
-	if err != nil {
-		tb.Fatalf("failed to count negative_cache rows: %v", err)
-	}
-
-	return count
-}
-
-// TestCleanExpiredDeletesInBatches stores one expired negative cache row
-// more than one batch holds, and one row that has not expired. One
-// statement must delete one batch and no more, so a request waits behind
-// one batch at most; CleanExpired must then remove the expired row left
-// over and keep the row that has not expired.
-func TestCleanExpiredDeletesInBatches(t *testing.T) {
-	t.Parallel()
-
-	cache, err := NewCache(evictionTestDB(t), CacheConfig{StateDir: t.TempDir()})
-	if err != nil {
-		t.Fatalf("failed to create cache: %v", err)
-	}
-
-	ctx := t.Context()
-
-	insertExpiredNegativeCacheRows(t, cache, cleanExpiredBatchSize+1)
-
-	const notExpiredPath = "/later.jpg"
-
-	_, err = cache.db.ExecContext(ctx, `
-		INSERT INTO negative_cache
-			(source_host, source_path, status_code, expires_at)
-		VALUES (?, ?, 404, datetime('now', '+1 hour'))
-	`, testHostExample, notExpiredPath)
-	if err != nil {
-		t.Fatalf("failed to insert a row that has not expired: %v", err)
-	}
-
-	deleted, err := cache.cleanExpiredBatch(ctx)
-	if err != nil {
-		t.Fatalf("cleanExpiredBatch failed: %v", err)
-	}
-
-	t.Logf("one statement deleted %d of %d expired rows",
-		deleted, cleanExpiredBatchSize+1)
-
-	if deleted != cleanExpiredBatchSize {
-		t.Errorf("one statement deleted %d rows, want %d",
-			deleted, cleanExpiredBatchSize)
-	}
-
-	err = cache.CleanExpired(ctx)
-	if err != nil {
-		t.Fatalf("CleanExpired failed: %v", err)
-	}
-
-	if count := negativeCacheTableRowCount(t, cache); count != 1 {
-		t.Errorf("negative_cache holds %d rows after CleanExpired, want 1", count)
-	}
-
-	if negativeCacheRowCount(t, cache, notExpiredPath) != 1 {
-		t.Error("the row that has not expired was removed")
-	}
-}
-
-// TestCleanExpiredRemovesMoreThanOneBatch stores one expired negative
-// cache row more than one batch holds and calls CleanExpired once: it
-// must remove them all, so it fails if CleanExpired stops after its
-// first batch.
-func TestCleanExpiredRemovesMoreThanOneBatch(t *testing.T) {
-	t.Parallel()
-
-	cache, err := NewCache(evictionTestDB(t), CacheConfig{StateDir: t.TempDir()})
-	if err != nil {
-		t.Fatalf("failed to create cache: %v", err)
-	}
-
-	insertExpiredNegativeCacheRows(t, cache, cleanExpiredBatchSize+1)
-
-	before := negativeCacheTableRowCount(t, cache)
-
-	err = cache.CleanExpired(t.Context())
-	if err != nil {
-		t.Fatalf("CleanExpired failed: %v", err)
-	}
-
-	after := negativeCacheTableRowCount(t, cache)
-
-	t.Logf("CleanExpired removed %d of %d expired rows, %d per batch",
-		before-after, before, cleanExpiredBatchSize)
-
-	if after != 0 {
-		t.Errorf("negative_cache holds %d rows after CleanExpired, want 0", after)
-	}
-}
-
-// TestCleanExpiredStopsOnceContextEnds runs CleanExpired with a context
-// that has already ended: it must return the context's error and delete
-// no row.
-func TestCleanExpiredStopsOnceContextEnds(t *testing.T) {
-	t.Parallel()
-
-	cache, err := NewCache(evictionTestDB(t), CacheConfig{StateDir: t.TempDir()})
-	if err != nil {
-		t.Fatalf("failed to create cache: %v", err)
-	}
-
-	insertExpiredNegativeCacheRows(t, cache, 1)
-
-	ctx, cancel := context.WithCancel(t.Context())
-	cancel()
-
-	err = cache.CleanExpired(ctx)
-	if !errors.Is(err, context.Canceled) {
-		t.Errorf("CleanExpired() error = %v, want context.Canceled", err)
-	}
-
-	if count := negativeCacheTableRowCount(t, cache); count != 1 {
-		t.Errorf("negative_cache holds %d rows, want the expired row kept", count)
-	}
-}
-
-// insertExpiredNegativeCacheRows inserts count negative_cache rows that
-// expired an hour ago, each for a different source path.
-func insertExpiredNegativeCacheRows(t *testing.T, cache *Cache, count int) {
-	t.Helper()
-
-	for i := range count {
-		_, err := cache.db.ExecContext(t.Context(), `
-			INSERT INTO negative_cache
-				(source_host, source_path, status_code, expires_at)
-			VALUES (?, ?, 404, datetime('now', '-1 hour'))
-		`, testHostExample, "/expired/"+strconv.Itoa(i)+".jpg")
-		if err != nil {
-			t.Fatalf("failed to insert an expired row: %v", err)
-		}
-	}
-}
-
-// negativeCacheTableRowCount returns how many rows negative_cache holds.
-func negativeCacheTableRowCount(tb testing.TB, cache *Cache) int {
-	tb.Helper()
-
-	var count int
-
-	err := cache.db.QueryRowContext(tb.Context(),
-		`SELECT COUNT(*) FROM negative_cache`,
-	).Scan(&count)
-	if err != nil {
-		tb.Fatalf("failed to count negative_cache rows: %v", err)
-	}
-
-	return count
-}
-
 func TestService_Get_ReturnsErrorForNegativeCachedURL(t *testing.T) {
 	t.Parallel()
 

internal/imgcache/service.go

@@ -9,7 +9,6 @@ import (
 	"log/slog"
 	"net/url"
 	"runtime/debug"
-	"strings"
 	"time"
 
 	"github.com/dustin/go-humanize"
@@ -130,13 +129,25 @@ func NewService(cfg *ServiceConfig) (*Service, error) {
 var ErrNegativeCached = errors.New("request is in negative cache (recently failed)")
 
 // Get retrieves a processed image, fetching and processing if necessary.
-// While its source URL is in the negative cache, a cached variant is still
-// served and a variant not in the cache is still made from a cached source;
-// only a request that needs the source fetched is refused.
 func (s *Service) Get(ctx context.Context, req *ImageRequest) (*ImageResponse, error) {
 	// Propagate AllowHTTP setting to the request
 	req.AllowHTTP = s.allowHTTP
 
+	// Check negative cache first - skip fetching for recently-failed URLs
+	negHit, err := s.cache.checkNegativeCache(ctx, req)
+	if err != nil {
+		s.log.Warn("negative cache check failed", "error", err)
+	}
+
+	if negHit {
+		s.log.Debug("negative cache hit",
+			"host", req.SourceHost,
+			"path", req.SourcePath,
+		)
+
+		return nil, fmt.Errorf("%w: %w", httpfetcher.ErrUpstreamError, ErrNegativeCached)
+	}
+
 	// Check variant cache first (disk only, no DB)
 	result, err := s.cache.Lookup(ctx, req)
 	if err != nil {
@@ -207,16 +218,7 @@ func (s *Service) ValidateRequest(req *ImageRequest) error {
 		return fmt.Errorf("invalid source URL: %w", err)
 	}
 
-	// An allowlist entry names no port, so a host followed by ":", with or
+	if s.allowlist.IsAllowed(parsedURL) {
-	// without a port, needs a signature. The port is read from the parsed
-	// URL, as the fetcher reads it: in an IPv6 address written without
-	// brackets, such as 2001:db8::1:8443, it is what follows the last ":".
-	// This is checked here and not in allowlist.IsAllowed, which also matches
-	// referer_blocklist, where a Referer must be blocked whatever its port.
-	hostHasPort := parsedURL.Port() != "" ||
-		strings.HasSuffix(parsedURL.Host, ":")
-
-	if !hostHasPort && s.allowlist.IsAllowed(parsedURL) {
 		return nil
 	}
 
@@ -421,28 +423,12 @@ func (s *Service) processFromSourceOrFetch(
 
 // fetchAndProcess fetches from upstream, processes, and caches the result.
 // It counts the fetch with the bytes read from upstream, including when
-// reading the response or a later step fails. It fetches nothing while the
+// reading the response or a later step fails.
-// source URL is in the negative cache.
 func (s *Service) fetchAndProcess(
 	ctx context.Context,
 	req *ImageRequest,
 	cacheKey VariantKey,
 ) (*processedVariant, error) {
-	// Check negative cache - skip fetching for recently-failed URLs
-	negHit, err := s.cache.checkNegativeCache(ctx, req)
-	if err != nil {
-		s.log.Warn("negative cache check failed", "error", err)
-	}
-
-	if negHit {
-		s.log.Debug("negative cache hit",
-			"host", req.SourceHost,
-			"path", req.SourcePath,
-		)
-
-		return nil, fmt.Errorf("%w: %w", httpfetcher.ErrUpstreamError, ErrNegativeCached)
-	}
-
 	// Fetch from upstream
 	sourceURL := req.SourceURL()
 
@@ -452,13 +438,7 @@ func (s *Service) fetchAndProcess(
 	if err != nil {
 		// Store negative cache for certain errors
 		if isNegativeCacheable(err) {
-			statusCode := extractStatusCode(err)
+			s.storeNegative(ctx, req, extractStatusCode(err), err)
-
-			storeErr := s.cache.StoreNegative(ctx, req, statusCode, err.Error())
-			if storeErr != nil {
-				s.log.Warn("failed to store negative cache entry",
-					"host", req.SourceHost, "path", req.SourcePath, "error", storeErr)
-			}
 		}
 
 		return nil, fmt.Errorf("upstream fetch failed: %w", err)
@@ -504,9 +484,16 @@ func (s *Service) fetchAndProcess(
 		"cipher", fetchResult.TLSCipherSuite,
 	)
 
-	// Validate magic bytes match content type
+	// Validate magic bytes match content type. A body that does not is the
+	// upstream's error, so it gets a negative cache entry, as an upstream
+	// error status does.
 	err = magic.ValidateMagicBytes(sourceData, fetchResult.ContentType)
 	if err != nil {
+		s.log.Warn("source refused", "request_id", middleware.GetReqID(ctx),
+			"host", req.SourceHost, "path", req.SourcePath,
+			"content_type", fetchResult.ContentType, "error", err)
+		s.storeNegative(ctx, req, httpStatusUnsupportedMediaType, err)
+
 		return nil, fmt.Errorf("content validation failed: %w", err)
 	}
 
@@ -522,6 +509,19 @@ func (s *Service) fetchAndProcess(
 	)
 }
 
+// storeNegative stores a negative cache entry for req's source, whose fetch
+// failed with err, so the source is not fetched again until the entry
+// expires. A failure to store the entry is only logged.
+func (s *Service) storeNegative(
+	ctx context.Context, req *ImageRequest, statusCode int, err error,
+) {
+	storeErr := s.cache.StoreNegative(ctx, req, statusCode, err.Error())
+	if storeErr != nil {
+		s.log.Warn("failed to store negative cache entry",
+			"host", req.SourceHost, "path", req.SourcePath, "error", storeErr)
+	}
+}
+
 // processedVariant is a variant as processAndStore made it. Each request that
 // shared its processing serves it through a reader of its own.
 type processedVariant struct {
@@ -630,8 +630,9 @@ func signatureRequest(req *ImageRequest) *signature.Request {
 
 // HTTP status codes for error responses.
 const (
-	httpStatusBadGateway    = 502
+	httpStatusBadGateway           = 502
-	httpStatusInternalError = 500
+	httpStatusInternalError        = 500
+	httpStatusUnsupportedMediaType = 415
 )
 
 // isNegativeCacheable returns true if the error should be cached.

internal/imgcache/service_internal_test.go

@@ -250,97 +250,6 @@ func TestService_ValidateRequest_SignatureExactHostMatch(t *testing.T) {
 	}
 }
 
-// TestService_ValidateRequest_HostWithPortNeedsSignature verifies that a host
-// followed by a port, or by a colon with no port, matches no allowlist entry,
-// so that a request for it without a signature is refused, while the same
-// host with no port is allowlisted.
-func TestService_ValidateRequest_HostWithPortNeedsSignature(t *testing.T) {
-	t.Parallel()
-
-	svc, _ := SetupTestService(t,
-		WithSigningKey(testSigningKey),
-		WithAllowlist(testHostCDN, "203.0.113.5", "2001:db8::1"),
-	)
-
-	tests := []struct {
-		host        string
-		allowlisted bool
-	}{
-		{testHostCDN, true},
-		{"cdn.example.com:8443", false},
-		{"cdn.example.com:443", false},
-		{"cdn.example.com:", false},
-		{"203.0.113.5", true},
-		{"203.0.113.5:8080", false},
-		{"[2001:db8::1]", true},
-		{"[2001:db8::1]:8443", false},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.host, func(t *testing.T) {
-			t.Parallel()
-
-			req := &ImageRequest{
-				SourceHost: tt.host,
-				SourcePath: testPathCat,
-				Size:       Size{Width: 50, Height: 50},
-				Format:     FormatJPEG,
-				Quality:    85,
-				FitMode:    FitCover,
-			}
-
-			err := svc.ValidateRequest(req)
-			t.Logf("ValidateRequest() for host %q with no signature: %v",
-				tt.host, err)
-
-			if allowlisted := err == nil; allowlisted != tt.allowlisted {
-				t.Errorf("host %q served without a signature = %v, want %v",
-					tt.host, allowlisted, tt.allowlisted)
-			}
-		})
-	}
-}
-
-// TestService_ValidateRequest_UnbracketedIPv6PortNeedsSignature verifies that
-// an allowlisted IPv6 address written without brackets and followed by a
-// port, or by a colon with no port, needs a signature. Go's URL parser, which
-// the fetcher uses, reads 2001:db8::1:8443 as the host 2001:db8::1 with the
-// port 8443.
-func TestService_ValidateRequest_UnbracketedIPv6PortNeedsSignature(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	svc, _ := SetupTestService(t,
-		WithSigningKey(testSigningKey),
-		WithAllowlist("2001:db8::1"),
-	)
-
-	for _, host := range []string{"2001:db8::1:8443", "2001:db8::1:"} {
-		t.Run(host, func(t *testing.T) {
-			t.Parallel()
-
-			req := &ImageRequest{
-				SourceHost: host,
-				SourcePath: testPathCat,
-				Size:       Size{Width: 50, Height: 50},
-				Format:     FormatJPEG,
-				Quality:    85,
-				FitMode:    FitCover,
-			}
-
-			err := svc.ValidateRequest(req)
-			t.Logf("ValidateRequest() for host %q with no signature: %v",
-				host, err)
-
-			if err == nil {
-				t.Errorf("host %q served without a signature, want refused",
-					host)
-			}
-		})
-	}
-}
-
 func TestService_Get_InvalidFile(t *testing.T) {
 	t.Parallel()
 

internal/imgcache/urlparser.go

@@ -15,7 +15,6 @@ import (
 var (
 	ErrInvalidPath       = errors.New("invalid image path")
 	ErrMissingHost       = errors.New("missing source host")
-	ErrInvalidHost       = errors.New("invalid source host")
 	ErrMissingSize       = errors.New("missing size specification")
 	ErrInvalidSize       = errors.New("invalid size format")
 	ErrInvalidFormat     = errors.New("invalid or unsupported format")
@@ -144,17 +143,6 @@ func parseImageComponents(remainder string) (*ParsedURL, error) {
 		return nil, ErrMissingHost
 	}
 
-	// A URL reads what comes before "@" as a user name and password, and the
-	// rest as the host
-	if strings.Contains(host, "@") {
-		return nil, fmt.Errorf("%w: %q holds @", ErrInvalidHost, host)
-	}
-
-	// Host names do not depend on letter case, so the allowlist, the upstream
-	// connection limits, the cache and the signature see one spelling of each
-	// host
-	host = strings.ToLower(host)
-
 	// Extract query string if present in path
 	if qIndex := strings.Index(path, "?"); qIndex != -1 {
 		query = path[qIndex+1:]

internal/imgcache/urlparser_internal_test.go

@@ -234,49 +234,6 @@ func TestParseImagePath(t *testing.T) {
 	}
 }
 
-// TestParseImagePath_LowerCasesHost verifies that the host is lower-cased, so
-// that every spelling of its letter case is the same host to the allowlist,
-// the upstream connection limits and the cache, and that the path keeps its
-// case.
-func TestParseImagePath_LowerCasesHost(t *testing.T) {
-	t.Parallel()
-
-	got, err := ParseImagePath("CDN.Example.COM/Photos/Cat.jpg/800x600.webp")
-	if err != nil {
-		t.Fatalf("ParseImagePath() error = %v", err)
-	}
-
-	t.Logf("parsed host %q, path %q", got.Host, got.Path)
-
-	if got.Host != testHostCDN {
-		t.Errorf("Host = %q, want %q", got.Host, testHostCDN)
-	}
-
-	if got.Path != "/Photos/Cat.jpg" {
-		t.Errorf("Path = %q, want %q", got.Path, "/Photos/Cat.jpg")
-	}
-}
-
-// TestParseImagePath_RefusesUserinfo verifies that a host holding "@" is
-// refused: a URL reads what comes before the "@" as a user name and password
-// and the rest as the host.
-func TestParseImagePath_RefusesUserinfo(t *testing.T) {
-	t.Parallel()
-
-	for _, input := range []string{
-		"user@cdn.example.com/photos/cat.jpg/800x600.webp",
-		"user:password@cdn.example.com/photos/cat.jpg/800x600.webp",
-	} {
-		got, err := ParseImagePath(input)
-		t.Logf("ParseImagePath(%q) = %+v, error = %v", input, got, err)
-
-		if !errors.Is(err, ErrInvalidHost) {
-			t.Errorf("ParseImagePath(%q) error = %v, want %v",
-				input, err, ErrInvalidHost)
-		}
-	}
-}
-
 func TestParsedURL_ToImageRequest(t *testing.T) {
 	t.Parallel()
 

internal/middleware/middleware.go

@@ -8,15 +8,12 @@ import (
 	"net/http"
 	"net/netip"
 	"regexp"
-	"strings"
 	"time"
 
 	basicauth "github.com/99designs/basicauth-go"
-	"github.com/go-chi/chi/v5"
 	"github.com/go-chi/chi/v5/middleware"
 	"github.com/go-chi/cors"
 	"github.com/go-chi/httprate"
-	"github.com/prometheus/client_golang/prometheus"
 	metrics "github.com/slok/go-http-metrics/metrics/prometheus"
 	ghmm "github.com/slok/go-http-metrics/middleware"
 	"github.com/slok/go-http-metrics/middleware/std"
@@ -66,10 +63,6 @@ type Middleware struct {
 	log      *slog.Logger
 	config   *config.Config
 	clientIP *clientip.Resolver
-
-	// metricsRegistry is the registry Metrics registers its metrics with.
-	// nil is Prometheus' default registry, the one /metrics serves.
-	metricsRegistry prometheus.Registerer
 }
 
 // New creates a new Middleware instance.
@@ -216,65 +209,17 @@ func (s *Middleware) CORS() func(http.Handler) http.Handler {
 	})
 }
 
-// Metrics returns a Prometheus metrics middleware. Its handler label is the
+// Metrics returns a Prometheus metrics middleware.
-// pattern of the chi route a request matches, such as /v1/image/*, or
-// "unmatched" for a request that matches none, so the routes fix how many
-// values the label has. It is never the path, with which any client could
-// add series to the registry without limit.
 func (s *Middleware) Metrics() func(http.Handler) http.Handler {
 	mdlw := ghmm.New(ghmm.Config{
-		Recorder: metrics.NewRecorder(metrics.Config{
+		Recorder: metrics.NewRecorder(metrics.Config{}),
-			Registry: s.metricsRegistry,
-		}),
 	})
 
 	return func(next http.Handler) http.Handler {
-		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		return std.Handler("", mdlw, next)
-			std.Handler(routePattern(r), mdlw, next).ServeHTTP(w, r)
-		})
 	}
 }
 
-// routePattern returns the pattern of the chi route r matches, or
-// "unmatched" when it matches none or is not served by a chi router. The
-// metrics library takes the label before the request is routed, so the
-// router is asked which route matches, with the path it routes on:
-// r.URL.RawPath when set, else r.URL.Path, else /.
-func routePattern(r *http.Request) string {
-	pattern := ""
-
-	rctx := chi.RouteContext(r.Context())
-	if rctx != nil {
-		path := r.URL.RawPath
-		if path == "" {
-			path = r.URL.Path
-		}
-
-		if path == "" {
-			path = "/"
-		}
-
-		pattern = rctx.Routes.Find(chi.NewRouteContext(), r.Method, path)
-
-		// For the path a subrouter is mounted at, such as /v1 or /v1/, Find
-		// answers that path without asking the subrouter, which chi hands
-		// the request as /.
-		for _, route := range rctx.Routes.Routes() {
-			if route.SubRoutes != nil &&
-				route.Pattern == strings.TrimSuffix(pattern, "/")+"/*" &&
-				!route.SubRoutes.Match(chi.NewRouteContext(), r.Method, "/") {
-				pattern = ""
-			}
-		}
-	}
-
-	if pattern == "" {
-		return "unmatched"
-	}
-
-	return pattern
-}
-
 // MetricsAuth returns a basic auth middleware for the metrics endpoint.
 func (s *Middleware) MetricsAuth() func(http.Handler) http.Handler {
 	return basicauth.New(

internal/middleware/middleware_internal_test.go

@@ -3,15 +3,12 @@ package middleware
 import (
 	"bytes"
 	"log/slog"
-	"maps"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"strings"
 	"testing"
 
-	"github.com/go-chi/chi/v5"
-	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
 
 	"sneak.berlin/go/pixa/internal/config"
@@ -201,7 +198,7 @@ func TestMetricsRecordsServedRequest(t *testing.T) {
 
 	// The line /metrics shows once one GET /test has been served.
 	const want = `http_request_duration_seconds_count{` +
-		`code="200",handler="unmatched",method="GET",service=""} 1`
+		`code="200",handler="/test",method="GET",service=""} 1`
 
 	mw := &Middleware{log: slog.Default(), config: &config.Config{}}
 
@@ -223,97 +220,6 @@ func TestMetricsRecordsServedRequest(t *testing.T) {
 	}
 }
 
-// TestMetricsLabelsRequestsByRoute checks that the metrics middleware labels
-// each request with the pattern of the route that answers it, never with its
-// path: two paths one route answers share that route's pattern, and requests
-// no route answers, a 404 or a 405, share "unmatched", among them requests for
-// the path the subrouter is mounted at, /v1 and /v1/. A request with an empty
-// path is answered by the / route, as chi routes it as /, and is labelled /.
-// With a label per path, any client could add series to the registry without
-// limit. The middleware registers with a registry of its own here, as another
-// test in this package registers with Prometheus' default registry, which can
-// be done only once.
-func TestMetricsLabelsRequestsByRoute(t *testing.T) {
-	t.Parallel()
-
-	registry := prometheus.NewRegistry()
-	mw := &Middleware{
-		log:             slog.Default(),
-		config:          &config.Config{},
-		metricsRegistry: registry,
-	}
-
-	// The image route is in a subrouter, as in pixa's router.
-	router := chi.NewRouter()
-	router.Use(mw.Metrics())
-	router.Get("/", func(w http.ResponseWriter, _ *http.Request) {
-		w.WriteHeader(http.StatusOK)
-	})
-	router.Route("/v1", func(r chi.Router) {
-		r.Get("/image/*", func(w http.ResponseWriter, _ *http.Request) {
-			w.WriteHeader(http.StatusOK)
-		})
-	})
-
-	for _, req := range []struct{ method, path string }{
-		{http.MethodGet, "/v1/image/a.example.com/one.jpg/1x1.png"},
-		{http.MethodGet, "/v1/image/b.example.com/two.jpg/1x1.png"},
-		{http.MethodGet, "/no-such-route"},
-		{http.MethodGet, "/v1/no-such-route"},
-		{http.MethodGet, "/v1"},
-		{http.MethodGet, "/v1/"},
-		{http.MethodPost, "/v1/image/a.example.com/one.jpg/1x1.png"},
-		// The request line GET http://pixa.example HTTP/1.1 leaves both
-		// r.URL.Path and r.URL.RawPath empty.
-		{http.MethodGet, "http://pixa.example"},
-	} {
-		r := httptest.NewRequestWithContext(
-			t.Context(), req.method, req.path, nil)
-		t.Logf("%s %s: r.URL.Path %q, r.URL.RawPath %q",
-			req.method, req.path, r.URL.Path, r.URL.RawPath)
-		router.ServeHTTP(httptest.NewRecorder(), r)
-	}
-
-	got := requestCountsByHandler(t, registry)
-	t.Logf("requests counted by handler label: %v", got)
-
-	want := map[string]uint64{"/": 1, "/v1/image/*": 2, "unmatched": 5}
-	if !maps.Equal(got, want) {
-		t.Errorf("requests counted by handler label = %v, want %v", got, want)
-	}
-}
-
-// requestCountsByHandler returns how many requests the request duration
-// histogram in registry counted under each value of its handler label.
-func requestCountsByHandler(
-	t *testing.T, registry *prometheus.Registry,
-) map[string]uint64 {
-	t.Helper()
-
-	families, err := registry.Gather()
-	if err != nil {
-		t.Fatalf("registry.Gather() error = %v", err)
-	}
-
-	counts := map[string]uint64{}
-
-	for _, family := range families {
-		if family.GetName() != "http_request_duration_seconds" {
-			continue
-		}
-
-		for _, metric := range family.GetMetric() {
-			for _, label := range metric.GetLabel() {
-				if label.GetName() == "handler" {
-					counts[label.GetValue()] += metric.GetHistogram().GetSampleCount()
-				}
-			}
-		}
-	}
-
-	return counts
-}
-
 // TestLoggingLeavesOutSubmittedSigningKey checks that a login, a POST /
 // whose form carries the signing key, leaves no trace of the key in the
 // request's log line.

internal/server/logout_internal_test.go

@@ -1,269 +0,0 @@
-package server
-
-import (
-	"net/http"
-	"net/http/httptest"
-	"net/url"
-	"path/filepath"
-	"regexp"
-	"strings"
-	"testing"
-
-	"go.uber.org/fx"
-	"go.uber.org/fx/fxtest"
-
-	"sneak.berlin/go/pixa/internal/config"
-	"sneak.berlin/go/pixa/internal/database"
-	"sneak.berlin/go/pixa/internal/globals"
-	"sneak.berlin/go/pixa/internal/handlers"
-	"sneak.berlin/go/pixa/internal/healthcheck"
-	"sneak.berlin/go/pixa/internal/logger"
-	"sneak.berlin/go/pixa/internal/middleware"
-	"sneak.berlin/go/pixa/internal/session"
-)
-
-// logoutFormPattern extracts the CSRF token rendered into the generator
-// page's logout form.
-var logoutFormPattern = regexp.MustCompile(
-	`(?s)action="/logout".*?name="gorilla\.csrf\.Token" value="([^"]+)"`)
-
-// startPixa starts the components pixad's fx app builds, from cfg, and
-// returns the server with its routes, as newTestServer does.
-func startPixa(t *testing.T, cfg *config.Config) *Server {
-	t.Helper()
-
-	var (
-		h  *handlers.Handlers
-		mw *middleware.Middleware
-	)
-
-	app := fxtest.New(t,
-		fx.Supply(cfg),
-		fx.Provide(
-			globals.New,
-			logger.New,
-			database.New,
-			healthcheck.New,
-			handlers.New,
-			middleware.New,
-		),
-		fx.Populate(&h, &mw),
-	)
-	app.RequireStart()
-	t.Cleanup(app.RequireStop)
-
-	s := &Server{config: cfg, mw: mw, h: h}
-	s.SetupRoutes()
-
-	return s
-}
-
-// logIn logs in to s with the signing key, as postLogin does, and returns
-// the session cookie it sets.
-func logIn(t *testing.T, s *Server) *http.Cookie {
-	t.Helper()
-
-	rec := postLogin(t, s, firstClient, "", testSigningKey)
-
-	for _, c := range rec.Result().Cookies() {
-		if c.Name == session.CookieName {
-			return c
-		}
-	}
-
-	t.Fatal("logging in set no session cookie")
-
-	return nil
-}
-
-// opensGeneratorPage reports whether GET / with cookie shows s's generator
-// page, which only a valid login session opens.
-func opensGeneratorPage(t *testing.T, s *Server, cookie *http.Cookie) bool {
-	t.Helper()
-
-	req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil)
-	req.AddCookie(cookie)
-
-	rec := httptest.NewRecorder()
-	s.ServeHTTP(rec, req)
-
-	return rec.Code == http.StatusOK &&
-		strings.Contains(rec.Body.String(), `action="/generate"`)
-}
-
-// postLogout loads the generator page with GET / and sessionCookie and
-// submits its logout form with POST /logout, as a browser does.
-func postLogout(
-	t *testing.T, s *Server, sessionCookie *http.Cookie,
-) *httptest.ResponseRecorder {
-	t.Helper()
-
-	req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil)
-	req.AddCookie(sessionCookie)
-
-	page := httptest.NewRecorder()
-	s.ServeHTTP(page, req)
-
-	match := logoutFormPattern.FindStringSubmatch(page.Body.String())
-	if match == nil {
-		t.Fatal("the generator page has no logout form with a CSRF token")
-	}
-
-	form := url.Values{"gorilla.csrf.Token": {match[1]}}
-	req = httptest.NewRequestWithContext(t.Context(), http.MethodPost,
-		"/logout", strings.NewReader(form.Encode()))
-	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
-	req.AddCookie(sessionCookie)
-
-	for _, c := range page.Result().Cookies() {
-		req.AddCookie(c)
-	}
-
-	rec := httptest.NewRecorder()
-	s.ServeHTTP(rec, req)
-
-	return rec
-}
-
-// TestLogoutWithoutCSRFTokenEndsNoSession verifies that a request to /logout
-// carrying the session cookie but not the logout form's CSRF token ends no
-// session: GET /logout, as an image on another page of the same site makes a
-// browser send, answers 405, and POST /logout without the token answers 403.
-func TestLogoutWithoutCSRFTokenEndsNoSession(t *testing.T) {
-	t.Parallel()
-
-	tests := []struct {
-		method string
-		want   int
-	}{
-		{http.MethodGet, http.StatusMethodNotAllowed},
-		{http.MethodPost, http.StatusForbidden},
-	}
-
-	for _, tc := range tests {
-		t.Run(tc.method, func(t *testing.T) {
-			t.Parallel()
-
-			s := newTestServer(t)
-			cookie := logIn(t, s)
-
-			req := httptest.NewRequestWithContext(
-				t.Context(), tc.method, "/logout", nil)
-			req.AddCookie(cookie)
-
-			rec := httptest.NewRecorder()
-			s.ServeHTTP(rec, req)
-			t.Logf("%s /logout without the CSRF token: status %d",
-				tc.method, rec.Code)
-
-			if rec.Code != tc.want {
-				t.Errorf("%s /logout without the CSRF token status = %d, "+
-					"want %d", tc.method, rec.Code, tc.want)
-			}
-
-			if !opensGeneratorPage(t, s, cookie) {
-				t.Errorf("%s /logout without the CSRF token ended the "+
-					"login session", tc.method)
-			}
-		})
-	}
-}
-
-// TestLoginAfterLogoutOpensGeneratorPage verifies that after a logout from
-// the generator page a copy of the session cookie taken before it is
-// refused, and that a login after it opens the generator page.
-func TestLoginAfterLogoutOpensGeneratorPage(t *testing.T) {
-	t.Parallel()
-
-	s := newTestServer(t)
-
-	copied := logIn(t, s)
-	if !opensGeneratorPage(t, s, copied) {
-		t.Fatal("logging in did not open the generator page")
-	}
-
-	logout := postLogout(t, s, copied)
-	if logout.Code != http.StatusSeeOther {
-		t.Fatalf("POST /logout status = %d, want %d",
-			logout.Code, http.StatusSeeOther)
-	}
-
-	if opensGeneratorPage(t, s, copied) {
-		t.Error("a cookie copied before the logout still opens the " +
-			"generator page")
-	}
-
-	opens := opensGeneratorPage(t, s, logIn(t, s))
-	t.Logf("a login after the logout opens the generator page: %v", opens)
-
-	if !opens {
-		t.Error("a login after the logout does not open the generator page")
-	}
-}
-
-// TestLogoutOutlastsRestart verifies that a copy of the session cookie taken
-// before a logout is still refused by a pixa started again on the same state
-// directory, as pixad is after a restart.
-func TestLogoutOutlastsRestart(t *testing.T) {
-	t.Parallel()
-
-	s := newTestServer(t)
-
-	copied := logIn(t, s)
-	if !opensGeneratorPage(t, s, copied) {
-		t.Fatal("logging in did not open the generator page")
-	}
-
-	logout := postLogout(t, s, copied)
-	if logout.Code != http.StatusSeeOther {
-		t.Fatalf("POST /logout status = %d, want %d",
-			logout.Code, http.StatusSeeOther)
-	}
-
-	restarted := startPixa(t, s.config)
-
-	opens := opensGeneratorPage(t, restarted, copied)
-	t.Logf("after the restart, the copied cookie opens the generator page: %v",
-		opens)
-
-	if opens {
-		t.Error("after a restart, a cookie copied before the logout " +
-			"still opens the generator page")
-	}
-}
-
-// TestLostStateDirectoryEndsEveryLoginSession verifies that a pixa started on
-// an empty state directory, as pixad is after a restart that lost its state
-// directory, refuses a session cookie from before, and that a login on it
-// opens the generator page.
-func TestLostStateDirectoryEndsEveryLoginSession(t *testing.T) {
-	t.Parallel()
-
-	s := newTestServer(t)
-
-	before := logIn(t, s)
-	if !opensGeneratorPage(t, s, before) {
-		t.Fatal("logging in did not open the generator page")
-	}
-
-	emptyStateDir := *s.config
-	emptyStateDir.StateDir = t.TempDir()
-	emptyStateDir.DBURL = "file:" +
-		filepath.Join(emptyStateDir.StateDir, "state.sqlite3")
-
-	restarted := startPixa(t, &emptyStateDir)
-
-	opens := opensGeneratorPage(t, restarted, before)
-	t.Logf("on an empty state directory, the cookie from before opens the "+
-		"generator page: %v", opens)
-
-	if opens {
-		t.Error("on an empty state directory, a cookie from before still " +
-			"opens the generator page")
-	}
-
-	if !opensGeneratorPage(t, restarted, logIn(t, restarted)) {
-		t.Error("a login on an empty state directory does not open the " +
-			"generator page")
-	}
-}

internal/server/routes.go

@@ -69,9 +69,10 @@ func (s *Server) SetupRoutes() {
 		r.With(s.mw.RateLimit(LoginAttemptsPerMinute, time.Minute, nil)).
 			Post("/", s.h.HandleRoot())
 		r.Post("/generate", s.h.HandleGenerateURL())
-		r.Post("/logout", s.h.HandleLogout())
 	})
 
+	s.router.Get("/logout", s.h.HandleLogout())
+
 	// Image routes, the only ones that send CORS headers, as pages on other
 	// sites read them. They are a subrouter rather than a group: a group's
 	// middleware runs only for a request that matches one of its routes,

internal/session/session.go

@@ -1,15 +1,9 @@
-// Package session provides encrypted session cookie management, and keeps a
+// Package session provides encrypted session cookie management.
-// count of logouts in the database so that a logout ends every session made
-// before it.
 package session
 
 import (
-	"context"
-	"database/sql"
 	"errors"
-	"fmt"
 	"net/http"
-	"sync"
 	"time"
 
 	"github.com/gorilla/securecookie"
@@ -38,34 +32,14 @@ type Data struct {
 	Authenticated bool      `json:"auth"`
 	CreatedAt     time.Time `json:"created"`
 	ExpiresAt     time.Time `json:"expires"`
-
-	// LogoutCount is the count of logouts when the session was made.
-	LogoutCount int64 `json:"logoutCount"`
 }
 
 // Manager handles session creation and validation using encrypted cookies.
-//
-// Every login is made with the one signing key, so a logout ends every
-// session made before it, in every browser. The Manager counts logouts: each
-// logout adds one, a session carries the count at its login, and a session
-// whose count is not the current one is refused. No clock is compared, so a
-// login after a logout works whatever the host's clock does. The Manager
-// keeps the count in memory, so checking a session reads no database; a
-// Manager from NewManagerWithDatabase also saves it in the database, so that
-// it outlasts a restart.
 type Manager struct {
 	sc *securecookie.SecureCookie
-	db *sql.DB
-
-	// mu guards logoutCount, and is held while a logout is saved, so that
-	// the database and logoutCount agree.
-	mu          sync.Mutex
-	logoutCount int64
 }
 
 // NewManager creates a session manager with keys derived from the signing key.
-// It keeps the count of logouts in memory only, so a restart forgets it;
-// NewManagerWithDatabase keeps it in the database.
 //
 // Session cookies always carry the Secure, HttpOnly, and SameSite=Strict
 // attributes; this cannot be configured. Browsers treat http://localhost as a
@@ -93,29 +67,6 @@ func NewManager(signingKey string) (*Manager, error) {
 	}, nil
 }
 
-// NewManagerWithDatabase creates a session manager like NewManager that saves
-// the count of logouts in db, which must have pixa's schema, and starts from
-// the count saved there.
-func NewManagerWithDatabase(
-	ctx context.Context, signingKey string, db *sql.DB,
-) (*Manager, error) {
-	m, err := NewManager(signingKey)
-	if err != nil {
-		return nil, err
-	}
-
-	err = db.QueryRowContext(ctx,
-		"SELECT logout_count FROM logout WHERE id = 1",
-	).Scan(&m.logoutCount)
-	if err != nil {
-		return nil, fmt.Errorf("failed to read the count of logouts: %w", err)
-	}
-
-	m.db = db
-
-	return m, nil
-}
-
 // CreateSession creates a new authenticated session and sets the cookie.
 func (m *Manager) CreateSession(w http.ResponseWriter) error {
 	now := time.Now()
@@ -123,7 +74,6 @@ func (m *Manager) CreateSession(w http.ResponseWriter) error {
 		Authenticated: true,
 		CreatedAt:     now,
 		ExpiresAt:     now.Add(SessionTTL),
-		LogoutCount:   m.currentLogoutCount(),
 	}
 
 	encoded, err := m.sc.Encode(CookieName, data)
@@ -172,30 +122,9 @@ func (m *Manager) ValidateSession(r *http.Request) (*Data, error) {
 		return nil, ErrInvalidSession
 	}
 
-	// A logout since the session was made has ended it.
-	if data.LogoutCount != m.currentLogoutCount() {
-		return nil, ErrInvalidSession
-	}
-
 	return &data, nil
 }
 
-// Logout removes the session cookie and, when r has a valid session, ends
-// every session made until now. A request without one ends no session, so
-// someone who has not logged in cannot end the sessions of those who have.
-func (m *Manager) Logout(w http.ResponseWriter, r *http.Request) error {
-	if m.IsAuthenticated(r) {
-		err := m.saveLogout(r.Context())
-		if err != nil {
-			return err
-		}
-	}
-
-	m.ClearSession(w)
-
-	return nil
-}
-
 // ClearSession removes the session cookie.
 func (m *Manager) ClearSession(w http.ResponseWriter) {
 	http.SetCookie(w, &http.Cookie{
@@ -216,31 +145,3 @@ func (m *Manager) IsAuthenticated(r *http.Request) bool {
 
 	return err == nil && data != nil && data.Authenticated
 }
-
-// saveLogout adds one to the count of logouts, saving the new count in the
-// database first when the Manager has one.
-func (m *Manager) saveLogout(ctx context.Context) error {
-	m.mu.Lock()
-	defer m.mu.Unlock()
-
-	if m.db != nil {
-		_, err := m.db.ExecContext(ctx,
-			"UPDATE logout SET logout_count = ? WHERE id = 1",
-			m.logoutCount+1)
-		if err != nil {
-			return fmt.Errorf("failed to save the logout: %w", err)
-		}
-	}
-
-	m.logoutCount++
-
-	return nil
-}
-
-// currentLogoutCount returns the count of logouts.
-func (m *Manager) currentLogoutCount() int64 {
-	m.mu.Lock()
-	defer m.mu.Unlock()
-
-	return m.logoutCount
-}

internal/session/session_logout_test.go

@@ -1,148 +0,0 @@
-package session_test
-
-import (
-	"database/sql"
-	"errors"
-	"net/http"
-	"net/http/httptest"
-	"testing"
-
-	"sneak.berlin/go/pixa/internal/database"
-	"sneak.berlin/go/pixa/internal/session"
-)
-
-const logoutTestSigningKey = "test-signing-key-12345"
-
-// newTestDatabase opens an empty in-memory database with pixa's schema. It
-// has one connection, as pixad's has: each connection to ":memory:" would
-// open a database of its own.
-func newTestDatabase(t *testing.T) *sql.DB {
-	t.Helper()
-
-	db, err := sql.Open("sqlite", ":memory:")
-	if err != nil {
-		t.Fatalf("sql.Open() error = %v", err)
-	}
-
-	db.SetMaxOpenConns(1)
-	t.Cleanup(func() { _ = db.Close() })
-
-	err = database.ApplyMigrations(t.Context(), db, nil)
-	if err != nil {
-		t.Fatalf("ApplyMigrations() error = %v", err)
-	}
-
-	return db
-}
-
-// createSessionCookie logs in with mgr and returns the session cookie it
-// sets.
-func createSessionCookie(t *testing.T, mgr *session.Manager) *http.Cookie {
-	t.Helper()
-
-	rec := httptest.NewRecorder()
-
-	err := mgr.CreateSession(rec)
-	if err != nil {
-		t.Fatalf("CreateSession() error = %v", err)
-	}
-
-	for _, c := range rec.Result().Cookies() {
-		if c.Name == session.CookieName {
-			return c
-		}
-	}
-
-	t.Fatalf("CreateSession() did not set a %q cookie", session.CookieName)
-
-	return nil
-}
-
-// requestWithCookie returns a GET request carrying cookie, or no cookie when
-// cookie is nil.
-func requestWithCookie(t *testing.T, cookie *http.Cookie) *http.Request {
-	t.Helper()
-
-	req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil)
-	if cookie != nil {
-		req.AddCookie(cookie)
-	}
-
-	return req
-}
-
-// TestLogout_EndsEverySessionMadeBeforeIt verifies that after a logout a
-// copy of the session cookie taken before it is refused and a login after it
-// works, and that both still hold for a new Manager on the same database, as
-// after a restart.
-func TestLogout_EndsEverySessionMadeBeforeIt(t *testing.T) {
-	t.Parallel()
-
-	db := newTestDatabase(t)
-
-	mgr, err := session.NewManagerWithDatabase(
-		t.Context(), logoutTestSigningKey, db)
-	if err != nil {
-		t.Fatalf("NewManagerWithDatabase() error = %v", err)
-	}
-
-	copied := createSessionCookie(t, mgr)
-
-	err = mgr.Logout(httptest.NewRecorder(), requestWithCookie(t, copied))
-	if err != nil {
-		t.Fatalf("Logout() error = %v", err)
-	}
-
-	_, err = mgr.ValidateSession(requestWithCookie(t, copied))
-	t.Logf("cookie copied before the logout: ValidateSession() error = %v", err)
-
-	if !errors.Is(err, session.ErrInvalidSession) {
-		t.Errorf("cookie copied before the logout: error = %v, want %v",
-			err, session.ErrInvalidSession)
-	}
-
-	loginAfter := createSessionCookie(t, mgr)
-
-	_, err = mgr.ValidateSession(requestWithCookie(t, loginAfter))
-	if err != nil {
-		t.Errorf("login after the logout: ValidateSession() error = %v", err)
-	}
-
-	restarted, err := session.NewManagerWithDatabase(
-		t.Context(), logoutTestSigningKey, db)
-	if err != nil {
-		t.Fatalf("NewManagerWithDatabase() after restart error = %v", err)
-	}
-
-	if restarted.IsAuthenticated(requestWithCookie(t, copied)) {
-		t.Error("after a restart, a cookie copied before the logout is accepted")
-	}
-
-	if !restarted.IsAuthenticated(requestWithCookie(t, loginAfter)) {
-		t.Error("after a restart, a login made after the logout is refused")
-	}
-}
-
-// TestLogout_WithoutSession_EndsNoSession verifies that a logout from a
-// request without a session cookie ends no session, so someone who has not
-// logged in cannot end the sessions of those who have.
-func TestLogout_WithoutSession_EndsNoSession(t *testing.T) {
-	t.Parallel()
-
-	mgr, err := session.NewManagerWithDatabase(
-		t.Context(), logoutTestSigningKey, newTestDatabase(t))
-	if err != nil {
-		t.Fatalf("NewManagerWithDatabase() error = %v", err)
-	}
-
-	cookie := createSessionCookie(t, mgr)
-
-	err = mgr.Logout(httptest.NewRecorder(), requestWithCookie(t, nil))
-	if err != nil {
-		t.Fatalf("Logout() error = %v", err)
-	}
-
-	if !mgr.IsAuthenticated(requestWithCookie(t, cookie)) {
-		t.Error("a logout without a session cookie ended a session")
-	}
-}

internal/static/style.css

@@ -122,20 +122,12 @@ header {
     margin-bottom: 2rem;
 }
 
-/* The logout button, drawn as a link. */
+header a {
-
-header button {
-    width: auto;
-    margin: 0;
-    padding: 0;
-    background: none;
     font-size: 0.875rem;
     color: #4b5563;
-    text-decoration: underline;
 }
 
-header button:hover {
+header a:hover {
-    background: none;
     color: #1f2937;
 }
 

internal/templates/generator.html

@@ -10,12 +10,9 @@
     <div class="page">
         <header>
             <h1>Pixa URL Generator</h1>
-            <form method="POST" action="/logout">
+            <a href="/logout">
-                {{ .CSRFField }}
+                Logout
-                <button type="submit">
+            </a>
-                    Logout
-                </button>
-            </form>
         </header>
 
         {{if .GeneratedURL}}