next -> main (1.0.0 milestone) (#118 )

Reviewed-on: #118
Strip metadata from processed images (closes #82 )
2026-09-29 03:01:06 +02:00 · 2026-09-29 02:18:25 +02:00 · 2026-09-29 01:03:37 +02:00 · 2026-09-28 19:59:39 +02:00 · 2026-09-28 18:07:11 +02:00 · 2026-09-28 17:46:49 +02:00
23 changed files with 1178 additions and 63 deletions
@@ -94,9 +94,38 @@ In-process caching of request-to-output mappings targets 1-5k r/s.

 Images are only fetched from origins using TLS with valid certificates.

+A request whose query string cannot be decoded, or gives any parameter more
+than once, is refused with 400.
+
 - `<format>`: one of `orig`, `png`, `jpeg`, `webp`
 - `<size>`: `orig` or `<width>x<height>` (e.g. `800x600`)

+The login form (`POST /`) is limited to 5 attempts per minute per client
+address, counting an IPv6 client by its /64; an attempt over the limit is
+refused with 429 and a `Retry-After` header. Behind a reverse proxy the client
+address comes from `X-Forwarded-For` only when the proxy's address is in
+`trusted_proxies`; otherwise all users behind the proxy are counted as one
+client. With the default `trusted_proxies` (the RFC 1918 ranges), a client
+with a private address can choose the address it is counted by through its own
+`X-Forwarded-For`, whether it connects directly or through the proxy, because
+its own address is trusted too. Setting `trusted_proxies` to the proxy's own
+address closes this.
+
+### Image Metadata
+
+pixa decodes and re-encodes every image it serves, and removes all metadata from
+the output: EXIF (GPS position, camera make, model and serial number, capture
+time, embedded thumbnail), XMP, IPTC and the ICC colour profile. This cannot be
+turned off.
+
+- The `orig` format means the source's own format, not the source's bytes: an
+  `orig` image is re-encoded and stripped like any other.
+- An image with an EXIF orientation is turned upright first, so it displays the
+  same without the tag; a requested size applies to the upright image.
+- An image with an ICC profile is converted to sRGB first, since clients show an
+  image with no profile as sRGB. Colours outside sRGB, such as the most
+  saturated ones in a Display P3 photo, are clipped.
+
 ### Source Hosts

 Source hosts may be allowlisted in the configuration. Non-allowlisted
@@ -124,11 +153,15 @@ Where:
 - `width` — requested width in pixels, `0` for original
 - `height` — requested height in pixels, `0` for original
 - `format` — output format (jpeg, png, webp, avif, gif, orig)
- `expiration` — Unix timestamp when signature expires
- `quality` — the URL's `q` query parameter (1-100), or `85` when the URL
-  has no `q`
+- `expiration` — the URL's `exp` query parameter, the Unix timestamp when
+  the signature expires; a request whose `exp` is not a whole number, an
+  empty `exp=` included, is refused with 400
+- `quality` — the URL's `q` query parameter, a whole number from 1 to 100,
+  or `85` when the URL has no `q`; a request whose `q` is anything else is
+  refused with 400
 - `fit` — the URL's `fit` query parameter (cover, contain, fill, inside,
-  outside), or `cover` when the URL has no `fit`
+  outside), or `cover` when the URL has no `fit`; a request whose `fit` is
+  anything else, an empty `fit=` included, is refused with 400

 **Example:** resize `https://cdn.example.com/photos/cat.jpg` to 800x600
 WebP with expiration 1704067200, default quality and fit:
@@ -161,7 +194,11 @@ process was started with and the file's own key. A variable's value is
 parsed as the same text in the file would be. The three lists take
 comma-separated entries, with the spaces around each trimmed; an empty
 variable is an empty list. A value that does not parse or is invalid aborts
-startup, naming the variable.
+startup, naming the variable. A variable whose name starts with `PIXA_` but
+is not in the table below, such as a misspelled one or `PIXA_PORT`, aborts
+startup naming it, as an unknown config key does. The one other accepted
+name is `PIXA_CONFIG_PATH`, the config file's path (like `--config`). The
+variables set by the file's `env:` section are checked the same way.

 | Variable                             | Config key                      | Meaning                                                                      |
 | ------------------------------------ | ------------------------------- | ---------------------------------------------------------------------------- |
@@ -194,7 +231,8 @@ Key settings in more detail:
  inside one of these ranges; the logged and login-recorded client
  address is then the rightmost forwarded entry that is not itself a
  trusted proxy. Otherwise the direct peer address is used and the header
-  is ignored, so a client connecting directly cannot spoof its address.
+  is ignored, so a client connecting directly from an address outside
+  these ranges cannot spoof its address.
  An omitted key defaults to the RFC 1918 private ranges (`10.0.0.0/8`,
  `172.16.0.0/12`, `192.168.0.0/16`), since pixa is deployed behind a
  proxy on a private network; an explicitly empty list (`[]`) trusts no
@@ -30,6 +30,53 @@ exhaustion

 # Completed Steps

+- 2026-09-28 strip metadata from processed images (closes #82): every output is
+  exported with govips' `StripMetadata`, so it carries no EXIF, XMP, IPTC or ICC
+  profile; the image is first turned upright with `AutoRotate` (before sizes are
+  worked out) and, when it has an ICC profile, converted to sRGB; the `orig`
+  format is re-encoded and stripped like any other, as pixa never serves the
+  source bytes; there is no setting to keep metadata; documented in `README.md`.
+- 2026-09-28 rate limit the login form (closes #66): `POST /` is limited to 5
+  attempts per minute per client address, and an attempt over the limit is
+  refused with 429 and a `Retry-After` header; the address is the one
+  `internal/clientip` resolves through `trusted_proxies`, an IPv6 client is
+  counted by its /64, and an IPv4-mapped address as the IPv4 address it
+  carries; the limit is a `RateLimit` middleware in `internal/middleware` on
+  `github.com/go-chi/httprate`, which the image routes can reuse; the library
+  keeps counts for the current and the previous minute only; documented in
+  `README.md`.
+- 2026-09-28 refuse an unparseable `exp` on `/v1/image/` and log swallowed
+  cache errors (closes #72): an `exp` in the URL that is not a whole
+  number, an empty `exp=` included, is a 400 naming `exp` and the value,
+  instead of being ignored and answered with 401 as if the URL had no
+  `exp`; only an `exp` missing from the URL is unchanged; `README.md` says
+  so where it documents `exp`. A failed variant `.meta` write, source
+  metadata JSON write, `Stats` count query, stats counter update, negative
+  cache write or expired negative cache delete is now logged at `warn`
+  with the path or key and the error, and stays non-fatal.
+- 2026-09-28 refuse an empty `fit` on `/v1/image/` (closes #139): a
+  `fit` in the URL with an empty value (`fit=`) is a 400 naming `fit`,
+  instead of being served as `cover` and verified against a signature
+  made for `cover`; only a `fit` missing from the URL is still `cover`;
+  any other value still goes through the existing fit-mode check;
+  `README.md` says so where it documents `fit`.
+- 2026-09-28 refuse an invalid `q` on `/v1/image/` (closes #134): a `q`
+  that is not a whole number from 1 to 100, an empty `q` included, is a
+  400 naming `q` and the value, instead of being served at the default
+  85; the route reads `q` with the generator's quality check
+  (`parseFormInt` with `minQuality` and `maxQuality`); only a `q` missing
+  from the URL is still 85; a query string that cannot be decoded, such
+  as `q=80%`, is a 400 showing it; any query parameter given more than
+  once (`q`, `fit`, `sig`, `exp` alike) is a 400 naming it, so none is
+  read from its first value only; `README.md` states the range and both
+  query-string rules.
+- 2026-09-28 unknown `PIXA_` environment variables abort startup (closes
+  #133): a variable whose name starts with `PIXA_` but is neither a
+  setting's variable nor `PIXA_CONFIG_PATH` aborts startup naming it, as
+  an unknown config key does, and `PIXA_PORT` is named with a pointer to
+  `PORT`; the check runs after the config file loads, so the variables
+  the file's `env:` section sets are checked too; documented in
+  `README.md`.
 - 2026-09-28 start on a fresh upaas volume (closes #129): the image
  starts as root only to give `/var/lib/pixa` to `pixad` when `pixad`
  does not own it (`deploy/docker-entrypoint.sh`), then runs the server
@@ -210,10 +257,9 @@ exhaustion

 # Future Steps

- P1: strip EXIF and other metadata from processed images (privacy)
 - P2: security
    - referer blacklist
-    - per-IP rate limiting
+    - per-IP rate limiting on the image routes
    - per-origin rate limiting
 - P2: HTTP response handling
    - Last-Modified headers
@@ -11,6 +11,7 @@ require (
 	github.com/getsentry/sentry-go v0.40.0
 	github.com/go-chi/chi/v5 v5.2.3
 	github.com/go-chi/cors v1.2.2
+	github.com/go-chi/httprate v0.16.0
 	github.com/gorilla/csrf v1.7.3
 	github.com/gorilla/securecookie v1.1.2
 	github.com/prometheus/client_golang v1.23.2
@@ -91,6 +92,7 @@ require (
 	github.com/inconshreveable/mousetrap v1.1.0 // indirect
 	github.com/josharian/intern v1.0.0 // indirect
 	github.com/json-iterator/go v1.1.12 // indirect
+	github.com/klauspost/cpuid/v2 v2.2.10 // indirect
 	github.com/kylelemons/godebug v1.1.0 // indirect
 	github.com/mailru/easyjson v0.7.7 // indirect
 	github.com/mattn/go-colorable v0.1.13 // indirect
@@ -113,6 +115,7 @@ require (
 	github.com/tidwall/match v1.1.1 // indirect
 	github.com/tidwall/pretty v1.2.0 // indirect
 	github.com/x448/float16 v0.8.4 // indirect
+	github.com/zeebo/xxh3 v1.0.2 // indirect
 	go.etcd.io/etcd/api/v3 v3.6.2 // indirect
 	go.etcd.io/etcd/client/pkg/v3 v3.6.2 // indirect
 	go.etcd.io/etcd/client/v3 v3.6.2 // indirect
@@ -114,6 +114,8 @@ github.com/go-chi/chi/v5 v5.2.3 h1:WQIt9uxdsAbgIYgid+BpYc+liqQZGMHRaUwp0JUcvdE=
 github.com/go-chi/chi/v5 v5.2.3/go.mod h1:L2yAIGWB3H+phAw1NxKwWM+7eUH/lU8pOMm5hHcoops=
 github.com/go-chi/cors v1.2.2 h1:Jmey33TE+b+rB7fT8MUy1u0I4L+NARQlK6LhzKPSyQE=
 github.com/go-chi/cors v1.2.2/go.mod h1:sSbTewc+6wYHBBCW7ytsFSn836hqM7JxpglAy2Vzc58=
+github.com/go-chi/httprate v0.16.0 h1:8V5DH9j6pSK6UQoBsTpvMyFxycqaKEIToyPKzHJjUa8=
+github.com/go-chi/httprate v0.16.0/go.mod h1:A8lo+qRhk+s9LiuP5saS7XCGDXRXMcrueq0NfIuCa/I=
 github.com/go-errors/errors v1.4.2 h1:J6MZopCL4uSllY1OfXM374weqZFFItUbrImctkmUxIA=
 github.com/go-errors/errors v1.4.2/go.mod h1:sIVyrIiJhuEF+Pj9Ebtd6P/rEYROXFi3BopGUQ5a5Og=
 github.com/go-jose/go-jose/v4 v4.0.5 h1:M6T8+mKZl/+fNNuFHvGIzDz7BTLQPIounk/b9dw3AaE=
@@ -251,6 +253,8 @@ github.com/kisielk/errcheck v1.5.0/go.mod h1:pFxgyoBC7bSaBwPgfKdkLd5X25qrDl4LWUI
 github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck=
 github.com/klauspost/compress v1.18.0 h1:c/Cqfb0r+Yi+JtIEq73FWXVkRonBlf0CRNYc8Zttxdo=
 github.com/klauspost/compress v1.18.0/go.mod h1:2Pp+KzxcywXVXMr50+X0Q/Lsb43OQHYWRCY2AiWywWQ=
+github.com/klauspost/cpuid/v2 v2.2.10 h1:tBs3QSyvjDyFTq3uoc/9xFpCuOsJQFNPiAhYdw2skhE=
+github.com/klauspost/cpuid/v2 v2.2.10/go.mod h1:hqwkgyIinND0mEev00jJYCxPNVRVXFQeu1XKlok6oO0=
 github.com/konsorten/go-windows-terminal-sequences v1.0.1/go.mod h1:T0+1ngSBFLxvqU3pZ+m/2kptfBszLMUkC4ZK/EgS/cQ=
 github.com/kr/logfmt v0.0.0-20140226030751-b84e30acd515/go.mod h1:+0opPa2QZZtGFBFZlji/RkVcI2GknAs/DXo4wKdlNEc=
 github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo=
@@ -396,6 +400,10 @@ github.com/x448/float16 v0.8.4/go.mod h1:14CWIYCyZA/cWjXOioeEpHeN/83MdbZDRQHoFcY
 github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
 github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
 github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY=
+github.com/zeebo/assert v1.3.0 h1:g7C04CbJuIDKNPFHmsk4hwZDO5O+kntRxzaUoNXj+IQ=
+github.com/zeebo/assert v1.3.0/go.mod h1:Pq9JiuJQpG8JLJdtkwrJESF0Foym2/D9XMU5ciN/wJ0=
+github.com/zeebo/xxh3 v1.0.2 h1:xZmwmqxHZA8AI603jOQ0tMqmBr9lPeFwGg6d+xy9DC0=
+github.com/zeebo/xxh3 v1.0.2/go.mod h1:5NWz9Sef7zIDm2JHfFlcQvNekmcEl9ekUZQQKCYaDcA=
 go.etcd.io/etcd/api/v3 v3.6.2 h1:25aCkIMjUmiiOtnBIp6PhNj4KdcURuBak0hU2P1fgRc=
 go.etcd.io/etcd/api/v3 v3.6.2/go.mod h1:eFhhvfR8Px1P6SEuLT600v+vrhdDTdcfMzmnxVXXSbk=
 go.etcd.io/etcd/client/pkg/v3 v3.6.2 h1:zw+HRghi/G8fKpgKdOcEKpnBTE4OO39T6MegA0RopVU=
@@ -58,6 +58,7 @@ var (
 	errValueRequired     = errors.New("a value is required")
 	errValueEmpty        = errors.New("value must not be empty")
 	errUnknownConfigKeys = errors.New("unknown config keys")
+	errUnknownEnvVars    = errors.New("unknown environment variables")
 	errNotAString        = errors.New("not a string")
 	errNotAnInteger      = errors.New("not an integer")
 	errNotABoolean       = errors.New("not a boolean")
@@ -154,6 +155,13 @@ func New(_ fx.Lifecycle, params Params) (*Config, error) {
 		return nil, err
 	}

+	// Loading the config file sets the variables in its env section,
+	// so this also checks their names.
+	err = validateKnownEnvVars()
+	if err != nil {
+		return nil, err
+	}
+
 	if sc == nil {
 		log.Info("no config file found, using environment variables and defaults")
 	}
@@ -377,6 +385,43 @@ func envVarNames() map[string]string {
 	}
 }

+// validateKnownEnvVars rejects environment variables whose names start
+// with PIXA_ but that are neither a setting's variable nor
+// PIXA_CONFIG_PATH, so a misspelled variable fails at startup instead of
+// being silently ignored, as validateKnownKeys does for config file keys.
+// New calls it after loading the config file, so the variables the
+// file's env section sets are checked too.
+func validateKnownEnvVars() error {
+	known := map[string]bool{"PIXA_CONFIG_PATH": true}
+
+	for _, name := range envVarNames() {
+		known[name] = true
+	}
+
+	var unknown []string
+
+	for _, entry := range os.Environ() {
+		name, _, _ := strings.Cut(entry, "=")
+
+		switch {
+		case !strings.HasPrefix(name, "PIXA_") || known[name]:
+			continue
+		case name == "PIXA_PORT":
+			unknown = append(unknown, name+" (use PORT for the port)")
+		default:
+			unknown = append(unknown, name)
+		}
+	}
+
+	if len(unknown) > 0 {
+		sort.Strings(unknown)
+
+		return fmt.Errorf("%w: %s", errUnknownEnvVars, strings.Join(unknown, ", "))
+	}
+
+	return nil
+}
+
 // lookupValue returns the value set for key and whether one is set. The
 // key's environment variable wins when it is present, even when empty;
 // its value is a string, read exactly as the same text quoted in the
@@ -3,10 +3,14 @@ package config
 import (
 	"net/netip"
 	"os"
+	"path/filepath"
 	"reflect"
 	"slices"
 	"strings"
 	"testing"
+
+	"sneak.berlin/go/pixa/internal/globals"
+	"sneak.berlin/go/pixa/internal/logger"
 )

 // TestMain unsets PORT and every PIXA_ environment variable before the
@@ -95,6 +99,114 @@ func TestEnvironmentSetsEveryKey(t *testing.T) {
 	}
 }

+// TestUnknownPixaVariableAbortsStartup checks that a PIXA_ variable that
+// is not a setting's variable, such as a misspelled one, aborts startup
+// naming it, as an unknown config key does, instead of being ignored.
+func TestUnknownPixaVariableAbortsStartup(t *testing.T) {
+	t.Setenv("PIXA_TRUSTED_PROXY", "192.0.2.0/24")
+	t.Setenv("PIXA_SIGNINGKEY", validTestSigningKey)
+
+	err := validateKnownEnvVars()
+	wantStartupError(t, err, "PIXA_TRUSTED_PROXY", "PIXA_SIGNINGKEY")
+}
+
+// TestPixaPortAbortsStartupPointingToPort checks that PIXA_PORT aborts
+// startup with a message saying to use PORT, which sets the port.
+func TestPixaPortAbortsStartupPointingToPort(t *testing.T) {
+	t.Setenv("PIXA_PORT", "9090")
+
+	err := validateKnownEnvVars()
+	wantStartupError(t, err, "PIXA_PORT", "use PORT")
+}
+
+// TestSettingVariablesAndConfigPathAreAccepted checks that every
+// setting's variable and PIXA_CONFIG_PATH pass the check for unknown
+// PIXA_ variables. TestEnvironmentSetsEveryKey pins the names in the list.
+func TestSettingVariablesAndConfigPathAreAccepted(t *testing.T) {
+	// A config file's env section loaded by another test can leave a
+	// PIXA_ variable set for the whole process, so every one is unset
+	// here first; t.Setenv restores each when the test ends.
+	for _, entry := range os.Environ() {
+		name, _, _ := strings.Cut(entry, "=")
+		if !strings.HasPrefix(name, "PIXA_") {
+			continue
+		}
+
+		t.Setenv(name, "")
+
+		err := os.Unsetenv(name)
+		if err != nil {
+			t.Fatalf("failed to unset %s: %v", name, err)
+		}
+	}
+
+	t.Setenv("PIXA_CONFIG_PATH", "/etc/pixa/config.yml")
+
+	for _, name := range envVarNames() {
+		t.Setenv(name, "")
+	}
+
+	err := validateKnownEnvVars()
+	if err != nil {
+		t.Fatalf("PIXA_CONFIG_PATH and every setting's variable "+
+			"must be accepted: %v", err)
+	}
+}
+
+// configFromNew writes yamlContent to a temporary config file, points
+// PIXA_CONFIG_PATH at it, and runs New, as the server does at startup.
+// The state directory is a temporary one and the disk cache is off, so
+// New succeeds unless something in the test is wrong.
+func configFromNew(t *testing.T, yamlContent string) (*Config, error) {
+	t.Helper()
+
+	tmpDir := t.TempDir()
+	configPath := filepath.Join(tmpDir, "config.yml")
+
+	err := os.WriteFile(configPath, []byte(yamlContent), 0o600)
+	if err != nil {
+		t.Fatalf("failed to write test config: %v", err)
+	}
+
+	t.Setenv("PIXA_CONFIG_PATH", configPath)
+	t.Setenv("PIXA_STATE_DIR", filepath.Join(tmpDir, "state"))
+	t.Setenv("PIXA_CACHE_MAX_BYTES", "0")
+
+	testLogger, err := logger.New(nil, logger.Params{Globals: &globals.Globals{}})
+	if err != nil {
+		t.Fatalf("failed to create logger: %v", err)
+	}
+
+	return New(nil, Params{Logger: testLogger})
+}
+
+// TestUnknownPixaVariableAbortsNew checks that New, which the server
+// calls at startup, aborts on a misspelled PIXA_ variable.
+func TestUnknownPixaVariableAbortsNew(t *testing.T) {
+	t.Setenv("PIXA_TRUSTED_PROXY", "192.0.2.0/24")
+
+	_, err := configFromNew(t, signingKeyLine)
+	wantStartupError(t, err, "PIXA_TRUSTED_PROXY")
+}
+
+// TestUnknownPixaVariableInEnvSectionAbortsNew checks that New aborts
+// on a misspelled PIXA_ name in the config file's env section, which
+// loading the file sets as an environment variable.
+func TestUnknownPixaVariableInEnvSectionAbortsNew(t *testing.T) {
+	// The variable must be absent until the file loads. t.Setenv makes
+	// sure the one the file sets is removed when the test ends.
+	t.Setenv("PIXA_TRUSTED_PROXY", "")
+
+	err := os.Unsetenv("PIXA_TRUSTED_PROXY")
+	if err != nil {
+		t.Fatalf("failed to unset PIXA_TRUSTED_PROXY: %v", err)
+	}
+
+	_, err = configFromNew(t, signingKeyLine+
+		"env:\n  PIXA_TRUSTED_PROXY: 192.0.2.0/24\n")
+	wantStartupError(t, err, "PIXA_TRUSTED_PROXY")
+}
+
 // TestPortFromEnvironmentOverridesConfigFile checks that PORT wins over
 // the port in the config file.
 func TestPortFromEnvironmentOverridesConfigFile(t *testing.T) {
@@ -18,13 +18,14 @@ import (
 	"sneak.berlin/go/pixa/internal/templates"
 )

-// errInvalidFormField reports a generator form field whose value is
-// non-numeric or out of range. The offending field name is wrapped in so the
-// response can name it.
+// errInvalidFormField reports a generator form field, or the q or exp
+// parameter of /v1/image/, whose value is non-numeric or out of range. The
+// offending field name is wrapped in so the response can name it.
 var errInvalidFormField = errors.New("invalid")

-// Bounds for the generator's quality and ttl fields. maxTTL is in seconds:
-// the expiry calculation time.Duration(ttl) * time.Second overflows above it.
+// Bounds for the generator's quality and ttl fields; the quality bounds also
+// apply to the q parameter of /v1/image/. maxTTL is in seconds: the expiry
+// calculation time.Duration(ttl) * time.Second overflows above it.
 const (
 	minQuality = 1
 	maxQuality = 100
@@ -248,9 +249,9 @@ func parseFormDimension(form url.Values, field string) (int, error) {
 	return value, nil
 }

-// parseFormInt reads an optional integer form field, returning def when the
-// field is empty and an error naming the field when the value is non-numeric
-// or outside minValue to maxValue.
+// parseFormInt reads an optional integer form field or URL query parameter,
+// returning def when the field is empty and an error naming the field when the
+// value is non-numeric or outside minValue to maxValue.
 func parseFormInt(
 	form url.Values, field string, def, minValue, maxValue int,
 ) (int, error) {
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"context"
 	"database/sql"
+	"encoding/json"
 	"image"
 	"image/color"
 	"image/jpeg"
@@ -291,3 +292,105 @@ func TestHandleImage_InvalidFitMode_Returns400(t *testing.T) {
 		t.Fatalf("status = %d, want %d", status, http.StatusBadRequest)
 	}
 }
+
+// TestHandleImage_InvalidQuery_Returns400 verifies that the plain image route
+// answers a q that is not a whole number from 1 to 100, an empty one
+// included, with 400 naming q and the value, a parameter given more than once
+// with 400 naming it, and a query string that cannot be decoded with 400
+// showing it, instead of serving the image at the default quality 85 or at
+// the first value given.
+func TestHandleImage_InvalidQuery_Returns400(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		query, wantError string
+	}{
+		{"q=banana", `invalid q: not a number, got "banana"`},
+		{"q=0", `invalid q: must be from 1 to 100, got "0"`},
+		{"q=101", `invalid q: must be from 1 to 100, got "101"`},
+		{"q=", `invalid q: not a number, got ""`},
+		{"q=80&q=500", `invalid q: given more than once`},
+		{"q=80&q=", `invalid q: given more than once`},
+		{"fit=cover&fit=contain", `invalid fit: given more than once`},
+		{"q=80%", `invalid query string "q=80%": invalid URL escape "%"`},
+		{
+			"q=50;fit=contain",
+			`invalid query string "q=50;fit=contain": ` +
+				`invalid semicolon separator in query`,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.query, func(t *testing.T) {
+			t.Parallel()
+
+			fix := setupTestHandler(t)
+
+			r := chi.NewRouter()
+			r.Get("/v1/image/*", fix.handler.HandleImage())
+
+			req := httptest.NewRequestWithContext(t.Context(), http.MethodGet,
+				"/v1/image/"+fix.goodHost+"/images/photo.jpg/50x50.jpeg?"+tt.query, nil)
+			rec := httptest.NewRecorder()
+
+			r.ServeHTTP(rec, req)
+
+			if rec.Code != http.StatusBadRequest {
+				t.Fatalf("status = %d, want %d", rec.Code, http.StatusBadRequest)
+			}
+
+			t.Logf("GET %s: %d %s", req.URL, rec.Code, rec.Body)
+
+			var body struct {
+				Error string `json:"error"`
+			}
+
+			err := json.NewDecoder(rec.Body).Decode(&body)
+			if err != nil {
+				t.Fatalf("decoding response body: %v", err)
+			}
+
+			if body.Error != tt.wantError {
+				t.Errorf("error = %q, want %q", body.Error, tt.wantError)
+			}
+		})
+	}
+}
+
+// TestHandleImage_EmptyFit_Returns400 verifies that the plain image route
+// answers a fit that is in the URL but empty with 400 naming fit, instead of
+// serving the image as cover. Only a fit missing from the URL means cover.
+func TestHandleImage_EmptyFit_Returns400(t *testing.T) {
+	t.Parallel()
+
+	fix := setupTestHandler(t)
+
+	r := chi.NewRouter()
+	r.Get("/v1/image/*", fix.handler.HandleImage())
+
+	req := httptest.NewRequestWithContext(t.Context(), http.MethodGet,
+		"/v1/image/"+fix.goodHost+"/images/photo.jpg/50x50.jpeg?fit=", nil)
+	rec := httptest.NewRecorder()
+
+	r.ServeHTTP(rec, req)
+
+	if rec.Code != http.StatusBadRequest {
+		t.Fatalf("status = %d, want %d", rec.Code, http.StatusBadRequest)
+	}
+
+	t.Logf("GET %s: %d %s", req.URL, rec.Code, rec.Body)
+
+	var body struct {
+		Error string `json:"error"`
+	}
+
+	err := json.NewDecoder(rec.Body).Decode(&body)
+	if err != nil {
+		t.Fatalf("decoding response body: %v", err)
+	}
+
+	wantError := `invalid fit: not a fit mode, got ""`
+	if body.Error != wantError {
+		t.Errorf("error = %q, want %q", body.Error, wantError)
+	}
+}
@@ -2,12 +2,15 @@ package handlers

 import (
 	"errors"
+	"fmt"
 	"io"
 	"net/http"
+	"net/url"
 	"strconv"
 	"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"
 )
@@ -89,32 +92,66 @@ func (s *Handlers) parseImageRequest(
 	// Convert to ImageRequest
 	req := parsed.ToImageRequest()

-	// Parse signature params from query string
-	query := r.URL.Query()
+	// Parse signature params from query string. r.URL.Query() would silently
+	// drop a pair it cannot decode, such as q=80%, so that q would be served
+	// at 85; a query string that cannot be decoded is refused instead. A
+	// parameter given more than once is refused too, as only its first value
+	// would be read.
+	query, err := url.ParseQuery(r.URL.RawQuery)
+	if err != nil {
+		s.respondError(w, fmt.Sprintf("invalid query string %q: %v",
+			r.URL.RawQuery, err), http.StatusBadRequest)
+
+		return nil, false
+	}
+
+	for name, values := range query {
+		if len(values) > 1 {
+			s.respondError(w, fmt.Sprintf("invalid %s: given more than once",
+				name), http.StatusBadRequest)
+
+			return nil, false
+		}
+	}
+
 	req.Signature = query.Get("sig")

-	if expStr := query.Get("exp"); expStr != "" {
-		exp, parseErr := strconv.ParseInt(expStr, 10, 64)
-		if parseErr == nil {
-			req.Expires = time.Unix(exp, 0)
-		}
+	req.Expires, err = parseExpires(query)
+	if err != nil {
+		s.respondError(w, err.Error(), http.StatusBadRequest)
+
+		return nil, false
 	}

-	// Parse optional quality and fit params
-	if qStr := query.Get("q"); qStr != "" {
-		q, parseErr := strconv.Atoi(qStr)
-		if parseErr == nil && q > 0 && q <= 100 {
-			req.Quality = q
-		}
+	// Parse optional quality and fit params. Only a q missing from the URL is
+	// 85. A q in the URL that is not a whole number from 1 to 100, an empty
+	// one included, is refused, checked as the generator checks its quality
+	// field; that check alone would take an empty q as missing.
+	qStr := query.Get("q")
+	if query.Has("q") && qStr == "" {
+		s.respondError(w, `invalid q: not a number, got ""`,
+			http.StatusBadRequest)
+
+		return nil, false
 	}

-	if fit := query.Get("fit"); fit != "" {
-		req.FitMode = imgcache.FitMode(fit)
+	req.Quality, err = parseFormInt(query, "q",
+		encurl.DefaultQuality, minQuality, maxQuality)
+	if err != nil {
+		s.respondError(w, fmt.Sprintf("%v, got %q", err, qStr),
+			http.StatusBadRequest)
+
+		return nil, false
 	}

-	// Default quality if not set
-	if req.Quality == 0 {
-		req.Quality = 85
+	// Only a fit missing from the URL is cover. A fit in the URL that is not a
+	// fit mode is refused by the fit-mode check below; that check would take an
+	// empty fit as missing, so an empty one is refused here.
+	req.FitMode = imgcache.FitMode(query.Get("fit"))
+	if query.Has("fit") && req.FitMode == "" {
+		s.respondError(w, `invalid fit: not a fit mode, got ""`, http.StatusBadRequest)
+
+		return nil, false
 	}

 	// Default fit mode if not set
@@ -137,6 +174,26 @@ func (s *Handlers) parseImageRequest(
 	return req, true
 }

+// parseExpires reads the exp query parameter, a Unix time in seconds. An exp
+// missing from the URL gives the zero time, which the signature check takes
+// as no expiration. An exp in the URL that is not a whole number, an empty
+// one included, is an error naming exp and the value.
+func parseExpires(query url.Values) (time.Time, error) {
+	if !query.Has("exp") {
+		return time.Time{}, nil
+	}
+
+	expStr := query.Get("exp")
+
+	exp, err := strconv.ParseInt(expStr, 10, 64)
+	if err != nil {
+		return time.Time{}, fmt.Errorf("%w exp: not a number, got %q",
+			errInvalidFormField, expStr)
+	}
+
+	return time.Unix(exp, 0), nil
+}
+
 // respondImageError maps image retrieval errors to HTTP responses.
 func (s *Handlers) respondImageError(
 	w http.ResponseWriter, req *imgcache.ImageRequest, err error,
@@ -1,6 +1,7 @@
 package handlers

 import (
+	"encoding/json"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
@@ -118,3 +119,53 @@ func TestHandleImage_GeneratedSignedURLVerifies(t *testing.T) {
 		})
 	}
 }
+
+// TestHandleImage_InvalidExp_Returns400 sends a signed-host URL whose exp is
+// not a whole number, and one whose exp is empty. Each is refused with 400
+// naming exp and the value, not with the 401 a URL without exp still gets.
+func TestHandleImage_InvalidExp_Returns400(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		query      string
+		wantStatus int
+		wantError  string
+	}{
+		{"sig=x&exp=banana", http.StatusBadRequest,
+			`invalid exp: not a number, got "banana"`},
+		{"sig=x&exp=", http.StatusBadRequest, `invalid exp: not a number, got ""`},
+		{"sig=x", http.StatusUnauthorized, "unauthorized"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.query, func(t *testing.T) {
+			t.Parallel()
+
+			fix := setupTestHandler(t)
+
+			r := chi.NewRouter()
+			r.Get("/v1/image/*", fix.handler.HandleImage())
+
+			req := httptest.NewRequestWithContext(t.Context(), http.MethodGet,
+				"/v1/image/"+signedHost+"/images/photo.jpg/50x50.jpeg?"+tt.query, nil)
+			rec := httptest.NewRecorder()
+
+			r.ServeHTTP(rec, req)
+			t.Logf("GET %s: %d %s", req.URL, rec.Code, rec.Body)
+
+			var body struct {
+				Error string `json:"error"`
+			}
+
+			err := json.NewDecoder(rec.Body).Decode(&body)
+			if err != nil {
+				t.Fatalf("decoding response body: %v", err)
+			}
+
+			if rec.Code != tt.wantStatus || body.Error != tt.wantError {
+				t.Errorf("got %d %q, want %d %q",
+					rec.Code, body.Error, tt.wantStatus, tt.wantError)
+			}
+		})
+	}
+}
@@ -161,6 +161,13 @@ func (p *ImageProcessor) Process(
 	}
 	defer img.Close()

+	// Turn the image upright now: encode strips the EXIF orientation tag,
+	// and sizes below must be worked out on the upright image.
+	err = img.AutoRotate()
+	if err != nil {
+		return nil, fmt.Errorf("failed to auto-rotate: %w", err)
+	}
+
 	// Get original dimensions
 	origWidth := img.Width()
 	origHeight := img.Height()
@@ -404,6 +411,21 @@ func (p *ImageProcessor) encode(
 		return nil, fmt.Errorf("%w: %s", ErrUnsupportedOutputFormat, format)
 	}

+	// Stripping drops the ICC profile as well, and clients show an image
+	// with no profile as sRGB, so convert to sRGB first. "srgb" names
+	// libvips' built-in profile; govips' own sRGB path variable is set on
+	// first use but read without a lock, so concurrent requests race on it.
+	if img.HasICCProfile() {
+		err := img.TransformICCProfileWithFallback("srgb", "srgb")
+		if err != nil {
+			return nil, fmt.Errorf("failed to convert to sRGB: %w", err)
+		}
+	}
+
+	// Drop EXIF, XMP, IPTC and the ICC profile. govips ignores this for
+	// GIF, which carries none of them.
+	params.StripMetadata = true
+
 	output, _, err := img.Export(&params)
 	if err != nil {
 		return nil, err
@@ -9,7 +9,9 @@ import (
 	"image/jpeg"
 	"image/png"
 	"io"
+	"math"
 	"os"
+	"slices"
 	"testing"

 	"github.com/davidbyttow/govips/v2/vips"
@@ -561,3 +563,152 @@ func TestImageProcessor_EncodeAVIF(t *testing.T) {

 	encodeAndCheck(t, FormatAVIF, 85, mimeAVIF)
 }
+
+// processAndDecode runs input through Process and decodes the output with
+// vips, so a test can inspect the image a client would receive.
+func processAndDecode(t *testing.T, input []byte, req *Request) *vips.ImageRef {
+	t.Helper()
+
+	result, err := New(Params{}).Process(
+		context.Background(), bytes.NewReader(input), req,
+	)
+	if err != nil {
+		t.Fatalf("Process() error = %v", err)
+	}
+
+	defer func() { _ = result.Content.Close() }()
+
+	data, err := io.ReadAll(result.Content)
+	if err != nil {
+		t.Fatalf("failed to read result: %v", err)
+	}
+
+	output, err := vips.NewImageFromBuffer(data)
+	if err != nil {
+		t.Fatalf("failed to decode output: %v", err)
+	}
+
+	t.Cleanup(output.Close)
+
+	return output
+}
+
+func TestImageProcessor_StripsEXIF(t *testing.T) {
+	t.Parallel()
+
+	// gps-exif.jpg carries GPS coordinates, a camera make, model and serial
+	// number, and a capture time.
+	input, err := os.ReadFile("testdata/gps-exif.jpg")
+	if err != nil {
+		t.Fatalf("failed to read test JPEG: %v", err)
+	}
+
+	fixture, err := vips.NewImageFromBuffer(input)
+	if err != nil {
+		t.Fatalf("failed to decode test JPEG: %v", err)
+	}
+
+	t.Cleanup(fixture.Close)
+
+	if !slices.Contains(fixture.GetFields(), "exif-ifd3-GPSLatitude") {
+		t.Fatal("testdata/gps-exif.jpg has no GPS latitude")
+	}
+
+	formats := []Format{
+		FormatJPEG, FormatPNG, FormatWebP, FormatAVIF, FormatGIF, FormatOriginal,
+	}
+
+	for _, format := range formats {
+		t.Run(string(format), func(t *testing.T) {
+			t.Parallel()
+
+			output := processAndDecode(t, input, &Request{Format: format})
+
+			if output.HasExif() {
+				t.Errorf("output has EXIF: %v", output.GetExif())
+			}
+		})
+	}
+}
+
+func TestImageProcessor_AppliesEXIFOrientation(t *testing.T) {
+	t.Parallel()
+
+	// orientation-6.jpg is stored 16x8, red on the left and blue on the
+	// right, with EXIF orientation 6 (turn 90 degrees clockwise to view).
+	// Upright it is 8x16, red on top and blue below.
+	input, err := os.ReadFile("testdata/orientation-6.jpg")
+	if err != nil {
+		t.Fatalf("failed to read test JPEG: %v", err)
+	}
+
+	tests := []struct {
+		name  string
+		size  Size
+		wantW int
+		wantH int
+	}{
+		{name: "original size", size: Size{}, wantW: 8, wantH: 16},
+		{name: "width only", size: Size{Width: 4}, wantW: 4, wantH: 8},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			output := processAndDecode(t, input, &Request{
+				Size:   tt.size,
+				Format: FormatPNG,
+			})
+
+			if output.Width() != tt.wantW || output.Height() != tt.wantH {
+				t.Fatalf("output is %dx%d, want %dx%d",
+					output.Width(), output.Height(), tt.wantW, tt.wantH)
+			}
+
+			top, err := output.GetPoint(tt.wantW/2, 0)
+			if err != nil {
+				t.Fatalf("GetPoint() error = %v", err)
+			}
+
+			bottom, err := output.GetPoint(tt.wantW/2, tt.wantH-1)
+			if err != nil {
+				t.Fatalf("GetPoint() error = %v", err)
+			}
+
+			if top[0] <= top[2] || bottom[2] <= bottom[0] {
+				t.Errorf("top pixel = %v, bottom pixel = %v, want red above blue",
+					top, bottom)
+			}
+		})
+	}
+}
+
+func TestImageProcessor_ConvertsWideGamutToSRGB(t *testing.T) {
+	t.Parallel()
+
+	// display-p3.jpg is a flat 8x8 image with the Display P3 profile
+	// embedded, filled with Display P3 (234, 51, 35), which is sRGB red.
+	input, err := os.ReadFile("testdata/display-p3.jpg")
+	if err != nil {
+		t.Fatalf("failed to read test JPEG: %v", err)
+	}
+
+	output := processAndDecode(t, input, &Request{Format: FormatPNG})
+
+	if output.HasICCProfile() {
+		t.Error("output has an ICC profile")
+	}
+
+	pixel, err := output.GetPoint(4, 4)
+	if err != nil {
+		t.Fatalf("GetPoint() error = %v", err)
+	}
+
+	want := []float64{255, 0, 0}
+	for i := range want {
+		if math.Abs(pixel[i]-want[i]) > 5 {
+			t.Fatalf("pixel = %v, want within 5 of %v", pixel, want)
+		}
+	}
+}
@@ -125,7 +125,7 @@ func NewCache(db *sql.DB, config CacheConfig) (*Cache, error) {
 	}

 	variants, err := NewVariantStorage(
-		filepath.Join(config.StateDir, "cache", "variants"),
+		filepath.Join(config.StateDir, "cache", "variants"), log,
 	)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create variant storage: %w", err)
@@ -263,23 +263,7 @@ func (c *Cache) StoreSource(
 		return "", fmt.Errorf("failed to insert source metadata: %w", err)
 	}

-	// Store metadata JSON file
-	meta := &SourceMetadata{
-		Host:            req.SourceHost,
-		Path:            req.SourcePath,
-		Query:           req.SourceQuery,
-		ContentHash:     string(contentHash),
-		StatusCode:      result.StatusCode,
-		ContentType:     result.ContentType,
-		ContentLength:   result.ContentLength,
-		ResponseHeaders: result.Headers,
-		FetchedAt:       time.Now().UTC().Unix(),
-		FetchDurationMs: result.FetchDurationMs,
-		RemoteAddr:      result.RemoteAddr,
-	}
-
-	// A failure here is non-fatal; the metadata is in the database.
-	_ = c.srcMetadata.Store(req.SourceHost, pathHash, meta)
+	c.writeMetadataSidecar(req, pathHash, contentHash, result)

 	c.notifyWritePressure()

@@ -436,12 +420,19 @@ func (c *Cache) Stats(ctx context.Context) (*CacheStats, error) {
 	}

 	// Get actual item count and total size from content tables
-	_ = c.db.QueryRowContext(ctx,
+	err = c.db.QueryRowContext(ctx,
 		`SELECT COUNT(*) FROM request_cache`,
 	).Scan(&stats.TotalItems)
-	_ = c.db.QueryRowContext(ctx,
+	if err != nil {
+		c.log.Warn("failed to count cache items for stats", "error", err)
+	}
+
+	err = c.db.QueryRowContext(ctx,
 		`SELECT COALESCE(SUM(size_bytes), 0) FROM output_content`,
 	).Scan(&stats.TotalSizeBytes)
+	if err != nil {
+		c.log.Warn("failed to sum cache size for stats", "error", err)
+	}

 	// Compute hit rate as a ratio
 	if stats.HitCount+stats.MissCount > 0 {
@@ -453,15 +444,17 @@ func (c *Cache) Stats(ctx context.Context) (*CacheStats, error) {

 // IncrementStats increments cache statistics.
 func (c *Cache) IncrementStats(ctx context.Context, hit bool, fetchBytes int64) {
+	var err error
+
 	if hit {
-		_, _ = c.db.ExecContext(ctx, `
+		_, err = c.db.ExecContext(ctx, `
 			UPDATE cache_stats
 			SET hit_count = hit_count + 1,
 			    last_updated_at = CURRENT_TIMESTAMP
 			WHERE id = 1
 		`)
 	} else {
-		_, _ = c.db.ExecContext(ctx, `
+		_, err = c.db.ExecContext(ctx, `
 			UPDATE cache_stats
 			SET miss_count = miss_count + 1,
 			    last_updated_at = CURRENT_TIMESTAMP
@@ -469,14 +462,52 @@ func (c *Cache) IncrementStats(ctx context.Context, hit bool, fetchBytes int64)
 		`)
 	}

+	if err != nil {
+		c.log.Warn("failed to count cache hit or miss", "hit", hit, "error", err)
+	}
+
 	if fetchBytes > 0 {
-		_, _ = c.db.ExecContext(ctx, `
+		_, err = c.db.ExecContext(ctx, `
 			UPDATE cache_stats
 			SET upstream_fetch_count = upstream_fetch_count + 1,
 			    upstream_fetch_bytes = upstream_fetch_bytes + ?,
 			    last_updated_at = CURRENT_TIMESTAMP
 			WHERE id = 1
 		`, fetchBytes)
+		if err != nil {
+			c.log.Warn("failed to count upstream fetch",
+				"fetch_bytes", fetchBytes, "error", err)
+		}
+	}
+}
+
+// writeMetadataSidecar writes the JSON metadata sidecar of a stored source.
+// A failure is logged and is otherwise non-fatal; the metadata is in the
+// database.
+func (c *Cache) writeMetadataSidecar(
+	req *ImageRequest,
+	pathHash PathHash,
+	contentHash ContentHash,
+	result *httpfetcher.FetchResult,
+) {
+	meta := &SourceMetadata{
+		Host:            req.SourceHost,
+		Path:            req.SourcePath,
+		Query:           req.SourceQuery,
+		ContentHash:     string(contentHash),
+		StatusCode:      result.StatusCode,
+		ContentType:     result.ContentType,
+		ContentLength:   result.ContentLength,
+		ResponseHeaders: result.Headers,
+		FetchedAt:       time.Now().UTC().Unix(),
+		FetchDurationMs: result.FetchDurationMs,
+		RemoteAddr:      result.RemoteAddr,
+	}
+
+	err := c.srcMetadata.Store(req.SourceHost, pathHash, meta)
+	if err != nil {
+		c.log.Warn("failed to write metadata sidecar",
+			"host", req.SourceHost, "path_hash", pathHash, "error", err)
 	}
 }

@@ -528,10 +559,14 @@ func (c *Cache) checkNegativeCache(
 	// Check if expired
 	if time.Now().After(expiresAt) {
 		// Clean up expired entry
-		_, _ = c.db.ExecContext(ctx, `
+		_, err = c.db.ExecContext(ctx, `
 			DELETE FROM negative_cache
 			WHERE source_host = ? AND source_path = ? AND source_query = ?
 		`, req.SourceHost, req.SourcePath, req.SourceQuery)
+		if err != nil {
+			c.log.Warn("failed to delete expired negative cache entry",
+				"host", req.SourceHost, "path", req.SourcePath, "error", err)
+		}

 		return false, nil
 	}
@@ -322,7 +322,12 @@ func (s *Service) fetchAndProcess(
 		// Store negative cache for certain errors
 		if isNegativeCacheable(err) {
 			statusCode := extractStatusCode(err)
-			_ = s.cache.StoreNegative(ctx, req, statusCode, 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)
+			}
 		}

 		return nil, fmt.Errorf("upstream fetch failed: %w", err)
@@ -1,9 +1,12 @@
 package imgcache

 import (
+	"bytes"
 	"context"
 	"database/sql"
+	"log/slog"
 	"math"
+	"strings"
 	"testing"
 	"time"

@@ -101,3 +104,82 @@ func TestStats_ZeroCounts(t *testing.T) {
 		t.Errorf("HitRate = %f, want 0.0 for zero counts", stats.HitRate)
 	}
 }
+
+// TestStats_LogsFailedCountQueries verifies that a failed item count query
+// and a failed size query are each logged at warn and Stats still succeeds.
+func TestStats_LogsFailedCountQueries(t *testing.T) {
+	t.Parallel()
+
+	db := setupStatsTestDB(t)
+
+	var logBuf bytes.Buffer
+
+	cache, err := NewCache(db, CacheConfig{
+		StateDir:    t.TempDir(),
+		CacheTTL:    time.Hour,
+		NegativeTTL: 5 * time.Minute,
+		Logger:      slog.New(slog.NewJSONHandler(&logBuf, nil)),
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	_, err = db.ExecContext(t.Context(),
+		`DROP TABLE request_cache; DROP TABLE output_content`)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	_, err = cache.Stats(t.Context())
+	if err != nil {
+		t.Fatalf("Stats() error = %v, want nil", err)
+	}
+
+	for _, msg := range []string{
+		"failed to count cache items for stats",
+		"failed to sum cache size for stats",
+	} {
+		want := `"level":"WARN","msg":"` + msg + `"`
+		if !strings.Contains(logBuf.String(), want) {
+			t.Errorf("log missing %s; got %q", want, logBuf.String())
+		}
+	}
+}
+
+// TestIncrementStats_LogsFailedUpdates verifies that a failed hit or miss
+// count update and a failed upstream fetch count update are each logged at
+// warn.
+func TestIncrementStats_LogsFailedUpdates(t *testing.T) {
+	t.Parallel()
+
+	db := setupStatsTestDB(t)
+
+	var logBuf bytes.Buffer
+
+	cache, err := NewCache(db, CacheConfig{
+		StateDir:    t.TempDir(),
+		CacheTTL:    time.Hour,
+		NegativeTTL: 5 * time.Minute,
+		Logger:      slog.New(slog.NewJSONHandler(&logBuf, nil)),
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	_, err = db.ExecContext(t.Context(), `DROP TABLE cache_stats`)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	cache.IncrementStats(t.Context(), false, 1024)
+
+	for _, msg := range []string{
+		"failed to count cache hit or miss",
+		"failed to count upstream fetch",
+	} {
+		want := `"level":"WARN","msg":"` + msg + `"`
+		if !strings.Contains(logBuf.String(), want) {
+			t.Errorf("log missing %s; got %q", want, logBuf.String())
+		}
+	}
+}
@@ -7,6 +7,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"log/slog"
 	"os"
 	"path/filepath"
 	"time"
@@ -392,6 +393,7 @@ func CacheKey(req *ImageRequest) VariantKey {
 // Unlike ContentStorage, the key is provided by the caller (not computed from content).
 type VariantStorage struct {
 	baseDir string
+	log     *slog.Logger
 }

 // VariantMeta contains metadata about a cached variant.
@@ -404,13 +406,14 @@ type VariantMeta struct {
 }

 // NewVariantStorage creates a new variant storage at the given base directory.
-func NewVariantStorage(baseDir string) (*VariantStorage, error) {
+// A failed .meta write is logged to log.
+func NewVariantStorage(baseDir string, log *slog.Logger) (*VariantStorage, error) {
 	err := os.MkdirAll(baseDir, StorageDirPerm)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create variant storage directory: %w", err)
 	}

-	return &VariantStorage{baseDir: baseDir}, nil
+	return &VariantStorage{baseDir: baseDir, log: log}, nil
 }

 // Store writes content and metadata to storage at the given key.
@@ -478,7 +481,11 @@ func (s *VariantStorage) Store(
 	}

 	// Metadata write failure is non-fatal; content is already stored.
-	_ = os.WriteFile(metaPath, metaData, StorageFilePerm)
+	err = os.WriteFile(metaPath, metaData, StorageFilePerm)
+	if err != nil {
+		s.log.Warn("failed to write variant metadata sidecar",
+			"path", metaPath, "error", err)
+	}

 	return size, nil
 }
@@ -4,8 +4,10 @@ import (
 	"bytes"
 	"errors"
 	"io"
+	"log/slog"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 )

@@ -404,3 +406,35 @@ func TestCacheKey(t *testing.T) {
 		t.Error("CacheKey() produced same key for different quality")
 	}
 }
+
+// TestVariantStorage_StoreLogsFailedMetaWrite verifies that a .meta write
+// that fails is logged at warn and the store still succeeds.
+func TestVariantStorage_StoreLogsFailedMetaWrite(t *testing.T) {
+	t.Parallel()
+
+	var logBuf bytes.Buffer
+
+	storage, err := NewVariantStorage(
+		t.TempDir(), slog.New(slog.NewJSONHandler(&logBuf, nil)))
+	if err != nil {
+		t.Fatalf("NewVariantStorage() error = %v", err)
+	}
+
+	key := CacheKey(&ImageRequest{SourceHost: testHostCDN, SourcePath: testPathCat})
+
+	// A directory where the .meta file goes makes the .meta write fail.
+	err = os.MkdirAll(storage.keyToPath(key)+".meta", StorageDirPerm)
+	if err != nil {
+		t.Fatalf("failed to create directory: %v", err)
+	}
+
+	_, err = storage.Store(key, bytes.NewReader([]byte("variant data")), "image/webp")
+	if err != nil {
+		t.Fatalf("Store() error = %v, want nil", err)
+	}
+
+	want := `"level":"WARN","msg":"failed to write variant metadata sidecar"`
+	if !strings.Contains(logBuf.String(), want) {
+		t.Errorf("log missing %s; got %q", want, logBuf.String())
+	}
+}
@@ -4,11 +4,13 @@ package middleware
 import (
 	"log/slog"
 	"net/http"
+	"net/netip"
 	"time"

 	basicauth "github.com/99designs/basicauth-go"
 	"github.com/go-chi/chi/v5/middleware"
 	"github.com/go-chi/cors"
+	"github.com/go-chi/httprate"
 	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"
@@ -88,6 +90,31 @@ func (s *Middleware) ClientIP() func(http.Handler) http.Handler {
 	}
 }

+// RateLimit returns a middleware that limits each client to requestLimit
+// requests per window and refuses a request over the limit with 429 Too Many
+// Requests and a Retry-After header. Clients are told apart by the address
+// the ClientIP middleware stored in the request context, so ClientIP must
+// run first. An IPv6 client is counted by its /64, which one client usually
+// holds whole; an IPv4-mapped address (::ffff:a.b.c.d) is counted as the
+// IPv4 address it carries, since every such address falls in the same /64.
+// Counts are kept only for the current and the previous window, so memory
+// stays bounded.
+func (s *Middleware) RateLimit(
+	requestLimit int, window time.Duration,
+) func(http.Handler) http.Handler {
+	return httprate.LimitBy(requestLimit, window,
+		func(r *http.Request) (string, error) {
+			ip := clientip.FromContext(r.Context())
+
+			addr, err := netip.ParseAddr(ip)
+			if err == nil {
+				ip = addr.Unmap().String()
+			}
+
+			return httprate.CanonicalizeIP(ip), nil
+		})
+}
+
 type loggingResponseWriter struct {
 	http.ResponseWriter

@@ -0,0 +1,280 @@
+package server
+
+import (
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"net/netip"
+	"net/url"
+	"path/filepath"
+	"regexp"
+	"strconv"
+	"strings"
+	"testing"
+
+	"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/logger"
+	"sneak.berlin/go/pixa/internal/middleware"
+)
+
+// testSigningKey is a throwaway signing key; submitting it logs in.
+const testSigningKey = "test-signing-key-0123456789abcdef"
+
+// wrongKey is submitted for a failed login.
+const wrongKey = "not-the-signing-key"
+
+// Addresses for the login rate limit tests. The test server trusts
+// 10.0.0.0/8 as its proxies, so the X-Forwarded-For sent by proxyPeer is
+// believed and the one sent by firstClient or secondClient is ignored.
+const (
+	firstClient     = "198.51.100.1:40000"
+	secondClient    = "198.51.100.2:40000"
+	proxyPeer       = "10.0.0.1:40000"
+	firstForwarded  = "203.0.113.1"
+	secondForwarded = "203.0.113.2"
+)
+
+// csrfFieldPattern extracts the CSRF token rendered into the login form.
+var csrfFieldPattern = regexp.MustCompile(
+	`name="gorilla\.csrf\.Token" value="([^"]+)"`)
+
+// newTestServer builds the server's real routes from the constructors
+// cmd/pixad uses, with a throwaway state directory. Debug marks requests
+// as plain HTTP, so the CSRF check runs without an https Referer.
+func newTestServer(t *testing.T) *Server {
+	t.Helper()
+
+	stateDir := t.TempDir()
+	cfg := &config.Config{
+		Debug:          true,
+		SigningKey:     testSigningKey,
+		StateDir:       stateDir,
+		DBURL:          "file:" + filepath.Join(stateDir, "state.sqlite3"),
+		TrustedProxies: []netip.Prefix{netip.MustParsePrefix("10.0.0.0/8")},
+	}
+
+	lc := fxtest.NewLifecycle(t)
+
+	log, err := logger.New(lc, logger.Params{Globals: &globals.Globals{}})
+	if err != nil {
+		t.Fatalf("logger.New() error = %v", err)
+	}
+
+	db, err := database.New(lc, database.Params{Logger: log, Config: cfg})
+	if err != nil {
+		t.Fatalf("database.New() error = %v", err)
+	}
+
+	h, err := handlers.New(lc, handlers.Params{
+		Logger: log, Database: db, Config: cfg,
+	})
+	if err != nil {
+		t.Fatalf("handlers.New() error = %v", err)
+	}
+
+	mw, err := middleware.New(lc, middleware.Params{Logger: log, Config: cfg})
+	if err != nil {
+		t.Fatalf("middleware.New() error = %v", err)
+	}
+
+	lc.RequireStart()
+	t.Cleanup(lc.RequireStop)
+
+	s := &Server{config: cfg, mw: mw, h: h}
+	s.SetupRoutes()
+
+	return s
+}
+
+// clientRequest builds a request for / arriving from remoteAddr, carrying
+// forwardedFor as its X-Forwarded-For header when that is not empty.
+func clientRequest(
+	t *testing.T, method string, body io.Reader, remoteAddr, forwardedFor string,
+) *http.Request {
+	t.Helper()
+
+	req := httptest.NewRequestWithContext(t.Context(), method, "/", body)
+	req.RemoteAddr = remoteAddr
+
+	if forwardedFor != "" {
+		req.Header.Set("X-Forwarded-For", forwardedFor)
+	}
+
+	return req
+}
+
+// postLogin loads the login form with GET / and submits key in it with
+// POST /, as a browser does, both from the same client. GET / is not rate
+// limited, so the form must load even for a client over the limit.
+func postLogin(
+	t *testing.T, s *Server, remoteAddr, forwardedFor, key string,
+) *httptest.ResponseRecorder {
+	t.Helper()
+
+	page := httptest.NewRecorder()
+	s.ServeHTTP(page,
+		clientRequest(t, http.MethodGet, nil, remoteAddr, forwardedFor))
+
+	if page.Code != http.StatusOK {
+		t.Fatalf("GET / status = %d, want %d", page.Code, http.StatusOK)
+	}
+
+	match := csrfFieldPattern.FindStringSubmatch(page.Body.String())
+	if match == nil {
+		t.Fatalf("no CSRF token field found in the login form")
+	}
+
+	form := url.Values{"key": {key}, "gorilla.csrf.Token": {match[1]}}
+	req := clientRequest(t, http.MethodPost,
+		strings.NewReader(form.Encode()), remoteAddr, forwardedFor)
+	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+
+	for _, c := range page.Result().Cookies() {
+		req.AddCookie(c)
+	}
+
+	rec := httptest.NewRecorder()
+	s.ServeHTTP(rec, req)
+
+	return rec
+}
+
+// tripLoginRateLimit makes LoginAttemptsPerMinute failed logins from one
+// client, each answered with the login form again, then one more, which
+// must be refused with 429. It returns the response to that last attempt.
+func tripLoginRateLimit(
+	t *testing.T, s *Server, remoteAddr, forwardedFor string,
+) *httptest.ResponseRecorder {
+	t.Helper()
+
+	for attempt := range LoginAttemptsPerMinute {
+		rec := postLogin(t, s, remoteAddr, forwardedFor, wrongKey)
+		if rec.Code != http.StatusOK {
+			t.Fatalf("failed login %d status = %d, want %d",
+				attempt+1, rec.Code, http.StatusOK)
+		}
+	}
+
+	rec := postLogin(t, s, remoteAddr, forwardedFor, wrongKey)
+	if rec.Code != http.StatusTooManyRequests {
+		t.Fatalf("login over the limit status = %d, want %d",
+			rec.Code, http.StatusTooManyRequests)
+	}
+
+	return rec
+}
+
+// TestLoginRateLimitRefusesAttemptOverLimit verifies the login attempt
+// after LoginAttemptsPerMinute failed ones from one client is refused with
+// 429 and a Retry-After header, and that the client cannot get around the
+// limit by sending X-Forwarded-For: from a peer that is not a trusted
+// proxy, the header is ignored.
+func TestLoginRateLimitRefusesAttemptOverLimit(t *testing.T) {
+	t.Parallel()
+
+	s := newTestServer(t)
+
+	rec := tripLoginRateLimit(t, s, firstClient, "")
+
+	retryAfter := rec.Header().Get("Retry-After")
+
+	seconds, err := strconv.Atoi(retryAfter)
+	if err != nil || seconds <= 0 {
+		t.Errorf("Retry-After = %q, want a positive number of seconds",
+			retryAfter)
+	}
+
+	rec = postLogin(t, s, firstClient, secondForwarded, wrongKey)
+	if rec.Code != http.StatusTooManyRequests {
+		t.Errorf("login with X-Forwarded-For from an untrusted peer "+
+			"status = %d, want %d", rec.Code, http.StatusTooManyRequests)
+	}
+}
+
+// TestLoginRateLimitLeavesOtherClientsAlone verifies one client going over
+// the limit does not limit another: a failed login from a different
+// address is answered with the login form, and the signing key still logs
+// it in.
+func TestLoginRateLimitLeavesOtherClientsAlone(t *testing.T) {
+	t.Parallel()
+
+	s := newTestServer(t)
+
+	tripLoginRateLimit(t, s, firstClient, "")
+
+	rec := postLogin(t, s, secondClient, "", wrongKey)
+	if rec.Code != http.StatusOK {
+		t.Errorf("failed login from another client status = %d, want %d",
+			rec.Code, http.StatusOK)
+	}
+
+	rec = postLogin(t, s, secondClient, "", testSigningKey)
+	if rec.Code != http.StatusSeeOther {
+		t.Errorf("login with the signing key from another client "+
+			"status = %d, want %d", rec.Code, http.StatusSeeOther)
+	}
+}
+
+// TestLoginRateLimitCountsClientsBehindProxySeparately verifies the limit
+// counts the client address resolved from X-Forwarded-For, not the address
+// of the trusted proxy the requests arrive from, so two clients behind the
+// same proxy are counted separately.
+func TestLoginRateLimitCountsClientsBehindProxySeparately(t *testing.T) {
+	t.Parallel()
+
+	s := newTestServer(t)
+
+	tripLoginRateLimit(t, s, proxyPeer, firstForwarded)
+
+	rec := postLogin(t, s, proxyPeer, secondForwarded, wrongKey)
+	if rec.Code != http.StatusOK {
+		t.Errorf("failed login from a second client behind the proxy "+
+			"status = %d, want %d", rec.Code, http.StatusOK)
+	}
+}
+
+// TestLoginRateLimitCountsIPv6ClientsByPrefix verifies an IPv6 client is
+// counted by its /64: another address in the same /64 is refused too,
+// while an address in a different /64 is not.
+func TestLoginRateLimitCountsIPv6ClientsByPrefix(t *testing.T) {
+	t.Parallel()
+
+	s := newTestServer(t)
+
+	tripLoginRateLimit(t, s, proxyPeer, "2001:db8::1")
+
+	rec := postLogin(t, s, proxyPeer, "2001:db8::2", wrongKey)
+	if rec.Code != http.StatusTooManyRequests {
+		t.Errorf("login from the same /64 status = %d, want %d",
+			rec.Code, http.StatusTooManyRequests)
+	}
+
+	rec = postLogin(t, s, proxyPeer, "2001:db8:0:1::1", wrongKey)
+	if rec.Code != http.StatusOK {
+		t.Errorf("login from another /64 status = %d, want %d",
+			rec.Code, http.StatusOK)
+	}
+}
+
+// TestLoginRateLimitCountsIPv4MappedClientsSeparately verifies an IPv4
+// client that the proxy forwards in IPv4-mapped IPv6 form (::ffff:a.b.c.d)
+// is counted by its IPv4 address, not by the /64 that every such address
+// shares, so two of them behind the proxy are counted separately.
+func TestLoginRateLimitCountsIPv4MappedClientsSeparately(t *testing.T) {
+	t.Parallel()
+
+	s := newTestServer(t)
+
+	tripLoginRateLimit(t, s, proxyPeer, "::ffff:"+firstForwarded)
+
+	rec := postLogin(t, s, proxyPeer, "::ffff:"+secondForwarded, wrongKey)
+	if rec.Code != http.StatusOK {
+		t.Errorf("failed login from a second IPv4-mapped client "+
+			"status = %d, want %d", rec.Code, http.StatusOK)
+	}
+}
@@ -2,6 +2,7 @@ package server

 import (
 	"net/http"
+	"time"

 	sentryhttp "github.com/getsentry/sentry-go/http"
 	"github.com/go-chi/chi/v5"
@@ -12,6 +13,10 @@ import (
 	"sneak.berlin/go/pixa/internal/static"
 )

+// LoginAttemptsPerMinute is how many login attempts (POST /) one client may
+// make per minute; the next is refused with 429 Too Many Requests.
+const LoginAttemptsPerMinute = 5
+
 // SetupRoutes configures all HTTP routes.
 func (s *Server) SetupRoutes() {
 	s.router = chi.NewRouter()
@@ -50,11 +55,14 @@ func (s *Server) SetupRoutes() {
 	// token cookie is independent of the session cookie, so it also
 	// covers the login POST, where no session exists yet. LimitBody caps
 	// the POST body ahead of CSRF, which reads its token from that body.
+	// The login POST is rate limited per client after both, so every
+	// attempt that reaches the signing key comparison is counted.
 	s.router.Group(func(r chi.Router) {
 		r.Use(s.h.LimitBody(handlers.MaxFormBytes))
 		r.Use(s.h.CSRF())
 		r.Get("/", s.h.HandleRoot())
-		r.Post("/", s.h.HandleRoot())
+		r.With(s.mw.RateLimit(LoginAttemptsPerMinute, time.Minute)).
+			Post("/", s.h.HandleRoot())
 		r.Post("/generate", s.h.HandleGenerateURL())
 	})
Author	SHA1	Message	Date
sneak	05678eaae5	next -> main (1.0.0 milestone) (#118 ) check / check (push) Successful in 12s Details Reviewed-on: #118	2026-09-29 03:01:06 +02:00
clawbot	be060a8305	Strip metadata from processed images (closes #82 ) check / check (push) Successful in 13s Details Every output is exported with govips' StripMetadata, so it carries no EXIF (GPS, serial numbers, embedded thumbnails), XMP, IPTC or ICC profile, the orig format included: it is always re-encoded, and pixa never serves the source bytes. The image is turned upright with AutoRotate right after decoding, so dropping the orientation tag does not leave it rotated, and a requested size applies to the upright image. An image with an ICC profile is converted to sRGB before export. No setting turns this off. README.md documents it. Model: opus-5-5	2026-09-29 02:18:25 +02:00
clawbot	e410146fb6	Rate limit login attempts per client address (closes #66 ) check / check (push) Successful in 12s Details POST / had no limit, so the signing key could be guessed at no cost. It is now limited to 5 attempts per minute per client by a new RateLimit middleware on github.com/go-chi/httprate; an attempt over the limit gets 429 with Retry-After. It counts by the address the ClientIP middleware resolved through trusted_proxies (an IPv4-mapped address as its IPv4 address, IPv6 by its /64) and runs after the body-size and CSRF checks, so every attempt that reaches the key comparison is counted. README says that with the default trusted_proxies a client with a private address can choose its counted address, and how to close that. Model: opus-5-5	2026-09-29 01:03:37 +02:00
clawbot	6010f5beb0	Refuse an unparseable exp with 400; log swallowed cache errors (closes #72 ) check / check (push) Successful in 12s Details An exp that was not a whole number, or empty, was ignored, so a URL for a host that needs a signature got 401 as if it had no exp. It is now a 400 naming exp and the value, on every host; only an exp missing from the URL is unchanged. A failed variant .meta write, source metadata JSON write, Stats count query, stats counter update, negative cache write or expired negative cache delete was discarded without a trace. Each is now logged at warn with the path or key and the error, and stays non-fatal, with tests for those that can be made to fail. VariantStorage takes the cache's logger. Model: opus-5-5	2026-09-28 19:59:39 +02:00
clawbot	f149813c7e	Refuse an empty fit on /v1/image/ with 400 (closes #139 ) check / check (push) Successful in 13s Details A fit in the URL with an empty value (fit=) was treated as missing, so it was served as cover and verified against a signature made for cover. It is now a 400 naming fit, the same rule the route applies to an empty q. It is checked before the existing fit-mode check, which takes an empty fit as missing; any other value still goes through that check unchanged. Only a fit missing from the URL is cover. Model: opus-5-5	2026-09-28 18:07:11 +02:00
clawbot	45869572ff	Refuse a q outside 1-100 on /v1/image/ with 400 (closes #134 ) check / check (push) Successful in 13s Details A q that was not a number or was outside 1-100 was dropped and 85 used, so q=500 was served and verified against a signature made for 85. It is now a 400 naming q and the value, read with the generator's quality check; only a q missing from the URL is 85. The route also refuses with 400 a query string that cannot be decoded (r.URL.Query() drops such a pair, so q=80% arrived as no q) and any parameter given more than once, which was read from its first value only (q=80&q=500 was served at 80). Model: opus-5-5	2026-09-28 17:46:49 +02:00
clawbot	0f3700f7f5	Abort startup on an unknown PIXA_ environment variable (closes #133 ) check / check (push) Successful in 11s Details A variable whose name starts with PIXA_ but is neither a setting's variable, from the list pairing each config key with its variable, nor PIXA_CONFIG_PATH now aborts startup naming it, as an unknown config key does. PIXA_PORT is named with a pointer to PORT. The check runs after the config file loads, so the variables the file's env section sets are checked too. README.md says so under Configuration. Model: opus-5-5	2026-09-28 16:12:57 +02:00