Tell upaas operators to set the cache size limit (closes #251)

"Running under upaas" listed PIXA_CACHE_MAX_BYTES as optional. Under
upaas the volume is a directory on the host's filesystem. The default,
75% of the sum of that filesystem's free space and what the cache
holds, and at least 500 MiB, lets one pixa take three quarters or more
of the space the host and every other app have left, and all of it
when that is 500 MiB or less. The section now says to set it and why.
The default is unchanged.

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,8 +123,12 @@ 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;
+    - `PIXA_CACHE_MAX_BYTES`: disk cache limit in bytes; `0` disables it. Set
-      default 75% of (free space + what the cache holds)
+      it: the volume is a directory on the host's filesystem, shared with the
+      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
@@ -185,11 +188,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
@@ -205,10 +204,9 @@ path under `/v1/` answers 200, in maintenance mode too.
   `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
   whose first bytes libvips reads as another format; 502 when the upstream
-  answered with an error status, and for 5 minutes after that for an image of
+  answered with an error status, and for 5 minutes after that for the same
-  the same source URL that pixa can neither serve from its cache nor make from
+  source URL; 503 when pixa is busy or in maintenance mode; 500 for any other
-  the source image still in its cache; 503 when pixa is busy or in maintenance
+  failure.
-  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
@@ -237,7 +235,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
@@ -358,14 +356,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
@@ -431,9 +422,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
@@ -488,12 +477,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

TODO.md

@@ -30,54 +30,13 @@ P2: security: per-origin rate limiting
 
 # Completed Steps
 
-- 2026-10-15 logging out ends every login session (closes #256): a login session
+- 2026-10-15 "Running under upaas" in `README.md` tells the operator to set
-  was a cookie with nothing kept on the server, so `/logout` removed it from one
+  `PIXA_CACHE_MAX_BYTES` (closes #251): it listed the variable as optional, but
-  browser and a copy of it kept working for its 30 days. Every login is made
+  under upaas the volume is a directory on the host's filesystem. The default,
-  with the one signing key, so logging out now ends them all: the session
+  75% of the sum of that filesystem's free space and what the cache holds, and
-  manager counts logouts, saves the count in the new `logout` table so that it
+  at least 500 MiB, let one pixa take three quarters or more of the space the
-  outlasts a restart, puts the count at login in each session, and refuses a
+  host and every other app had left, and all of it when that was 500 MiB or
-  session whose count is not the current one. No time is compared, so a login
+  less. The section now says so. The default is unchanged.
-  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/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/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()
 

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/server/logout_internal_test.go

@@ -1,232 +0,0 @@
-package server
-
-import (
-	"net/http"
-	"net/http/httptest"
-	"net/url"
-	"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")
-	}
-}

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}}