Label request metrics by route, not by path (closes #254)

The metrics middleware gave the handler label each request's path, so
every new path added series that were never removed, and /metrics
listed encrypted URL tokens. The label is now the pattern of the chi
route the request matches, or "unmatched" for one that matches none.
The metrics library takes the label before calling the handler, so the
middleware asks the router with chi's Find which route matches.

Model: opus-5-5

README.md

@@ -26,6 +26,11 @@ make docker
 docker run -p 8080:8080 -e PIXA_SIGNING_KEY="$(openssl rand -base64 32)" pixa:latest
 ```
 
+The example config allowlists no host (`allowlist_hosts`), so pixa serves an
+image only for a signed or encrypted URL. Every image on an allowlisted host is
+served to anyone without a signature, at any size and in any format, so
+allowlist only hosts you control, where nobody else can put images.
+
 A container takes its settings from environment variables (see Configuration
 below for the list). Only `PIXA_SIGNING_KEY` is required; if it is unset the
 container exits at startup naming the variable. Everything else has a built-in
@@ -115,7 +120,9 @@ What the [upaas](https://git.eeqj.de/sneak/upaas) app for pixa needs:
     - `PIXA_SIGNING_KEY` (required): secret for signed and encrypted URLs and
       login, 32+ characters, for example from `openssl rand -base64 32`
     - `PIXA_ALLOWLIST_HOSTS`: upstream hosts served without a signature,
-      comma-separated
+      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;
       default 75% of (free space + what the cache holds)
     - the rest are in the table under Configuration below
@@ -412,8 +419,11 @@ HMAC-SHA256(secret, "host:path:query:width:height:format:expiration:quality:fit"
 Where:
 
 - `host` — source origin hostname (e.g. `cdn.example.com`)
-- `path` — source path (e.g. `/photos/cat.jpg`)
+- `path` — source path (e.g. `/photos/cat.jpg`), with each `%` written as `%25`
-- `query` — source query string, empty string if none
+  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
+  as in `path`; neither then holds a `:`, so path `/a:b` with no query and path
+  `/a` with query `b:` are signed differently
 - `width` — requested width in pixels, `0` for original
 - `height` — requested height in pixels, `0` for original
 - `format` — output format, one of those listed under Routes, with `original`
@@ -621,7 +631,10 @@ See `configs/config.example.yml` for all options with defaults.
 - **Metrics**: Prometheus, at `/metrics`: generic HTTP request metrics
   (duration, response size, requests in flight) and the Go runtime and process
   metrics; requests are measured and `/metrics` is served only when
-  `metrics.username` and `metrics.password` are set
+  `metrics.username` and `metrics.password` are set. A request's `handler` label
+  is the pattern of the route it matches, such as `/v1/image/*`, never its path.
+  Requests that match no route share the label `unmatched`: those the router
+  answers 404 or 405, and CORS preflights
 - **Logging**: stdlib slog
 
 ## Entrypoints

TODO.md

@@ -30,6 +30,40 @@ P2: security: per-origin rate limiting
 
 # Completed Steps
 
+- 2026-10-15 the request metrics are labelled by route, not by path (closes
+  #254): their `handler` label was each request's path, so every new path, a 404
+  or an encrypted URL, added series to the Prometheus registry that were never
+  removed, and `/metrics` listed every encrypted URL's token. The label is now
+  the pattern of the chi route the request matches, such as `/v1/image/*`, or
+  `unmatched` for a request that matches none: one the router answers 404 or
+  405, or a CORS preflight. The metrics library takes the label before the
+  request is routed, so the metrics middleware asks the router which route
+  matches.
+- 2026-10-15 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
+  `https://h/a:b` verified for `https://h/a?b:`; the two also shared a cache
+  key, and whichever was fetched first was served for both. In both, the path
+  and the query now have each `%` written as `%25` and each `:` as `%3A`; a path
+  and query holding neither are signed and keyed as before. Signature
+  Specification in `README.md` says so.
+- 2026-10-15 the example config allowlists no host (closes #249): it allowlisted
+  `github.com` and `user-images.githubusercontent.com`, where anyone can put an
+  image, so a deployment set up as Getting Started says served those images to
+  anyone without a signature, at any size and in any format.
+  `configs/config.example.yml` now leaves `allowlist_hosts` out, with one
+  commented entry under `example.com`. Getting Started and "Running under upaas"
+  in `README.md` say that every image on an allowlisted host is served without a
+  signature, so an allowlisted host must be one the operator controls.
+- 2026-10-15 a chunked upstream body of exactly `upstream_max_response_size`
+  bytes is accepted (closes #248): once the limit was used up, the fetcher's
+  next read refused the body without looking for its end. Go's HTTP client
+  returns the end of a body sent with a `Content-Length` together with its last
+  bytes, but the end of a chunked body can come in a later read, so such a body
+  was refused after it had been downloaded whole, and the image routes
+  answered 500. Once the limit is used up, the fetcher now reads one more byte:
+  if the body has ended it is accepted, and if there is more it is refused with
+  `ErrResponseTooLarge`, as before.
 - 2026-10-13 `fit=outside` no longer makes an output above 8192 on a side
   (closes #257): it scales the image to cover the requested size, and the side
   scaled past that size was held to no maximum, so `1x8192?fit=outside` of a

configs/config.example.yml

@@ -45,15 +45,14 @@ state_dir: ./data
 signing_key: "CHANGE_ME_generate_with_openssl_rand_base64_32"
 
 # Hosts that don't require signatures (default: none)
+# Every image on an allowlisted host is served to anyone without a
+# signature, at any size and in any format, so list only hosts you
+# control, where nobody else can put images.
 # Use "." prefix for wildcard subdomain matching (e.g., ".example.com" matches "cdn.example.com")
 # An entry that is neither a host name nor an IP address (IPv6 without
 # brackets), such as one with a port or a "*." wildcard, aborts startup.
-allowlist_hosts:
+# allowlist_hosts:
-  - s3.sneak.cloud
+#   - images.example.com
-  - static.sneak.cloud
-  - sneak.berlin
-  - github.com
-  - user-images.githubusercontent.com
 
 # Hosts whose pages may not show pixa's images, written as for
 # allowlist_hosts. A request to /v1/image/ or /v1/e/ whose Referer header

internal/config/config_internal_test.go

@@ -92,6 +92,32 @@ func TestGetStringSlice_Empty(t *testing.T) {
 	}
 }
 
+// TestExampleConfigAllowlistsNoHost loads configs/config.example.yml
+// with a real signing key, as Getting Started in README.md has it copied
+// and edited, and checks that it allowlists no host: every image on an
+// allowlisted host is served to anyone without a signature.
+func TestExampleConfigAllowlistsNoHost(t *testing.T) {
+	t.Setenv("PIXA_SIGNING_KEY", validTestSigningKey)
+
+	examplePath := filepath.Join("..", "..", "configs", "config.example.yml")
+
+	sc, err := loadTestConfig(examplePath)
+	if err != nil {
+		t.Fatalf("failed to load %s: %v", examplePath, err)
+	}
+
+	c, err := newFromSmartConfig(sc)
+	if err != nil {
+		t.Fatalf("%s is not a valid config: %v", examplePath, err)
+	}
+
+	t.Logf("allowlist_hosts in %s: %v", examplePath, c.AllowlistHosts)
+
+	if len(c.AllowlistHosts) != 0 {
+		t.Errorf("%s allowlists %v, want no host", examplePath, c.AllowlistHosts)
+	}
+}
+
 // loadTestConfig is a helper to load a config file for testing.
 func loadTestConfig(path string) (*smartconfig.Config, error) {
 	return smartconfig.NewFromConfigPath(path)

internal/httpfetcher/fetch_internal_test.go

@@ -9,6 +9,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"slices"
+	"strconv"
 	"strings"
 	"sync"
 	"testing"
@@ -297,6 +298,139 @@ func TestFetchMaxResponseSizeEnforced(t *testing.T) {
 	}
 }
 
+// fetchWithMaxResponseSize serves handler as the fake upstream and fetches
+// its /image route with MaxResponseSize set to maxResponseSize.
+func fetchWithMaxResponseSize(
+	t *testing.T,
+	maxResponseSize int64,
+	handler http.HandlerFunc,
+) *FetchResult {
+	t.Helper()
+
+	srv := httptest.NewServer(handler)
+	t.Cleanup(srv.Close)
+
+	cfg := DefaultConfig()
+	cfg.MaxResponseSize = maxResponseSize
+
+	f, _ := newServerFetcher(t, srv, cfg)
+
+	res := fetchImage(t, f, "/image")
+	t.Cleanup(func() { _ = res.Content.Close() })
+
+	return res
+}
+
+// TestFetchContentLengthBodyAtMaxResponseSize checks that a body sent with a
+// Content-Length is read whole at exactly MaxResponseSize bytes and refused
+// at one byte more.
+func TestFetchContentLengthBodyAtMaxResponseSize(t *testing.T) {
+	t.Parallel()
+
+	const maxResponseSize = 8
+
+	for _, tc := range []struct {
+		size    int
+		wantErr error
+	}{
+		{maxResponseSize, nil},
+		{maxResponseSize + 1, ErrResponseTooLarge},
+	} {
+		t.Run(strconv.Itoa(tc.size)+" bytes", func(t *testing.T) {
+			t.Parallel()
+
+			body := strings.Repeat("x", tc.size)
+
+			res := fetchWithMaxResponseSize(t, maxResponseSize,
+				func(w http.ResponseWriter, _ *http.Request) {
+					w.Header().Set("Content-Type", contentTypeJPEG)
+					w.Header().Set("Content-Length", strconv.Itoa(tc.size))
+					_, _ = io.WriteString(w, body)
+				})
+
+			if res.ContentLength != int64(tc.size) {
+				t.Fatalf("ContentLength = %d, want %d", res.ContentLength, tc.size)
+			}
+
+			data, err := io.ReadAll(res.Content)
+			t.Logf("body of %d bytes: read %d, error %v", tc.size, len(data), err)
+
+			if !errors.Is(err, tc.wantErr) {
+				t.Fatalf("read error = %v, want %v", err, tc.wantErr)
+			}
+
+			if err == nil && string(data) != body {
+				t.Errorf("read %q, want %q", data, body)
+			}
+		})
+	}
+}
+
+// TestFetchChunkedBodyAtMaxResponseSize checks that a chunked body is read
+// whole at exactly MaxResponseSize bytes and refused at one byte more. The
+// upstream sends the end of the body only once the test has read
+// MaxResponseSize bytes, so the read after them comes before the end of the
+// body has arrived, as it can from a real upstream.
+func TestFetchChunkedBodyAtMaxResponseSize(t *testing.T) {
+	t.Parallel()
+
+	const maxResponseSize = 8
+
+	for _, tc := range []struct {
+		size    int
+		wantErr error
+	}{
+		{maxResponseSize, nil},
+		{maxResponseSize + 1, ErrResponseTooLarge},
+	} {
+		t.Run(strconv.Itoa(tc.size)+" bytes", func(t *testing.T) {
+			t.Parallel()
+
+			body := strings.Repeat("x", tc.size)
+			endBody := make(chan struct{})
+
+			res := fetchWithMaxResponseSize(t, maxResponseSize,
+				func(w http.ResponseWriter, r *http.Request) {
+					w.Header().Set("Content-Type", contentTypeJPEG)
+					_, _ = io.WriteString(w, body)
+					// With no Content-Length, flushing sends the body
+					// chunked; its end is sent when the handler returns.
+					_ = http.NewResponseController(w).Flush()
+
+					select {
+					case <-endBody:
+					case <-r.Context().Done():
+					}
+				})
+
+			if res.ContentLength != -1 {
+				t.Fatalf("ContentLength = %d, want -1 (chunked)", res.ContentLength)
+			}
+
+			head := make([]byte, maxResponseSize)
+
+			_, err := io.ReadFull(res.Content, head)
+			if err != nil {
+				t.Fatalf("read the first %d bytes: %v", maxResponseSize, err)
+			}
+
+			close(endBody)
+
+			rest, err := io.ReadAll(res.Content)
+			data := string(head) + string(rest)
+			t.Logf("body of %d bytes: read %d, error %v", tc.size, len(data), err)
+
+			if !errors.Is(err, tc.wantErr) {
+				t.Fatalf("read error = %v, want %v", err, tc.wantErr)
+			}
+
+			if err == nil && data != body {
+				t.Errorf("read %q, want %q", data, body)
+			}
+		})
+	}
+}
+
 func TestFetchSemaphoreReleasedOnError(t *testing.T) {
 	t.Parallel()
 

internal/httpfetcher/httpfetcher.go

@@ -660,14 +660,33 @@ func dialSSRFSafe(
 	return nil, fmt.Errorf("%w to %s", errConnectFailed, host)
 }
 
-// limitedReader wraps a reader and limits the number of bytes read.
+// limitedReader wraps a reader and refuses a body longer than remaining
+// bytes with ErrResponseTooLarge. remaining goes below 0 once the body is
+// found to be longer.
 type limitedReader struct {
 	reader    io.Reader
 	remaining int64
 }
 
+// Read reads at most the remaining bytes. Once they are read, it reads one
+// more byte to tell the end of the body from more data: at the end it
+// returns the wrapped reader's error, io.EOF; with more data it returns
+// ErrResponseTooLarge, as does every later Read.
 func (r *limitedReader) Read(p []byte) (int, error) {
-	if r.remaining <= 0 {
+	if r.remaining < 0 {
+		return 0, ErrResponseTooLarge
+	}
+
+	if r.remaining == 0 {
+		var extra [1]byte
+
+		n, err := r.reader.Read(extra[:])
+		r.remaining -= int64(n)
+
+		if n == 0 {
+			return 0, err
+		}
+
 		return 0, ErrResponseTooLarge
 	}
 

internal/imgcache/storage.go

@@ -10,6 +10,7 @@ import (
 	"log/slog"
 	"os"
 	"path/filepath"
+	"strings"
 	"time"
 )
 
@@ -371,11 +372,13 @@ func HashPath(path string) PathHash {
 
 // CacheKey generates a unique key for a request variant.
 // Format: sha256(host:path:query:width:height:format:quality:fit_mode)
+// The path and query go through escapePercentAndColon, so that a ":" in
+// either cannot be taken for the one between them.
 func CacheKey(req *ImageRequest) VariantKey {
 	data := fmt.Sprintf("%s:%s:%s:%d:%d:%s:%d:%s",
 		req.SourceHost,
-		req.SourcePath,
+		escapePercentAndColon(req.SourcePath),
-		req.SourceQuery,
+		escapePercentAndColon(req.SourceQuery),
 		req.Size.Width,
 		req.Size.Height,
 		req.Format,
@@ -387,6 +390,14 @@ func CacheKey(req *ImageRequest) VariantKey {
 	return VariantKey(hex.EncodeToString(h[:]))
 }
 
+// escapePercentAndColon returns s with each "%" written as "%25" and each
+// ":" as "%3A", so that the result holds no ":".
+func escapePercentAndColon(s string) string {
+	s = strings.ReplaceAll(s, "%", "%25")
+
+	return strings.ReplaceAll(s, ":", "%3A")
+}
+
 // VariantStorage handles key-based file storage for processed image variants.
 // Files are stored at: <basedir>/<ab>/<cd>/<cache_key>
 // Metadata is stored at: <basedir>/<ab>/<cd>/<cache_key>.meta

internal/imgcache/storage_internal_test.go

@@ -407,6 +407,62 @@ func TestCacheKey(t *testing.T) {
 	}
 }
 
+// TestCacheKey_ColonOrPercentInPathOrQuery checks that two different source
+// URLs whose path or query holds ":" or "%" get different keys.
+func TestCacheKey_ColonOrPercentInPathOrQuery(t *testing.T) {
+	t.Parallel()
+
+	cases := []struct {
+		name          string
+		path1, query1 string
+		path2, query2 string
+	}{
+		{
+			name:  "colon moved from path to query",
+			path1: "/a:b", query1: "",
+			path2: "/a", query2: "b:",
+		},
+		{
+			name:  "colon in path and its percent-encoding",
+			path1: "/a:b", query1: "",
+			path2: "/a%3Ab", query2: "",
+		},
+	}
+
+	for _, tt := range cases {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			key1 := CacheKey(&ImageRequest{
+				SourceHost:  testHostCDN,
+				SourcePath:  tt.path1,
+				SourceQuery: tt.query1,
+				Size:        Size{Width: 800, Height: 600},
+				Format:      FormatWebP,
+				Quality:     85,
+				FitMode:     FitCover,
+			})
+			key2 := CacheKey(&ImageRequest{
+				SourceHost:  testHostCDN,
+				SourcePath:  tt.path2,
+				SourceQuery: tt.query2,
+				Size:        Size{Width: 800, Height: 600},
+				Format:      FormatWebP,
+				Quality:     85,
+				FitMode:     FitCover,
+			})
+
+			t.Logf("CacheKey() of path %q query %q = %s", tt.path1, tt.query1, key1)
+			t.Logf("CacheKey() of path %q query %q = %s", tt.path2, tt.query2, key2)
+
+			if key1 == key2 {
+				t.Errorf("CacheKey() gave path %q query %q and path %q query %q "+
+					"the same key", tt.path1, tt.query1, tt.path2, tt.query2)
+			}
+		})
+	}
+}
+
 // TestVariantStorage_StoreLogsFailedMetaWrite verifies that a .meta write
 // that fails is logged at warn and the store still succeeds.
 func TestVariantStorage_StoreLogsFailedMetaWrite(t *testing.T) {

internal/middleware/middleware.go

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

internal/middleware/middleware_internal_test.go

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

internal/signature/escaping_test.go

@@ -0,0 +1,113 @@
+package signature_test
+
+import (
+	"errors"
+	"testing"
+	"time"
+
+	"sneak.berlin/go/pixa/internal/signature"
+)
+
+// TestSigner_Verify_ColonCannotMovePathQueryBoundary checks that a URL
+// signed for path "/a:b" with no query does not verify for path "/a" with
+// query "b:", a different resource on the same host.
+func TestSigner_Verify_ColonCannotMovePathQueryBoundary(t *testing.T) {
+	t.Parallel()
+
+	signer := signature.New(goldenSigningKey)
+
+	signed := &signature.Request{
+		SourceHost: testHost,
+		SourcePath: "/a:b",
+		Width:      800,
+		Height:     600,
+		Format:     testFormatWebP,
+		Quality:    85,
+		FitMode:    testFitCover,
+		Expires:    time.Now().Add(1 * time.Hour),
+	}
+	signed.Signature = signer.Sign(signed)
+
+	replayed := *signed
+	replayed.SourcePath = "/a"
+	replayed.SourceQuery = "b:"
+
+	t.Logf("signature for path %q query %q: %s",
+		signed.SourcePath, signed.SourceQuery, signed.Signature)
+	t.Logf("signature for path %q query %q: %s",
+		replayed.SourcePath, replayed.SourceQuery, signer.Sign(&replayed))
+
+	err := signer.Verify(&replayed)
+	if !errors.Is(err, signature.ErrInvalid) {
+		t.Errorf("Verify() of path %q query %q with the signature for "+
+			"path %q = %v, want %v",
+			replayed.SourcePath, replayed.SourceQuery, signed.SourcePath,
+			err, signature.ErrInvalid)
+	}
+}
+
+// TestSigner_EscapedGoldenVectors pins the signature of requests whose path
+// or query holds ":" or "%", which are signed with "%" written as "%25" and
+// ":" as "%3A". The expected values were computed once, outside pixa, from
+// the signed data in each comment.
+func TestSigner_EscapedGoldenVectors(t *testing.T) {
+	t.Parallel()
+
+	signer := signature.New(goldenSigningKey)
+
+	vectors := []struct {
+		name          string
+		path          string
+		query         string
+		wantSignature string
+	}{
+		{
+			name: "colon in path",
+			path: "/photos/a:b.jpg",
+			// Signed data:
+			// "cdn.example.com:/photos/a%3Ab.jpg::800:600:webp:1704067200:85:cover"
+			wantSignature: "WnEpZ7bQp1Zvw8WDatlj2vu-4wYKWE3BFBizV8m__6k=",
+		},
+		{
+			name:  "colon in query",
+			path:  testPath,
+			query: "t=1:2",
+			// Signed data:
+			// "cdn.example.com:/photos/cat.jpg:t=1%3A2:800:600:webp:1704067200:85:cover"
+			wantSignature: "85kRoN8hhr4cCUkEUjNvUsvVYKjYYfWdeffx5TjJjqA=",
+		},
+		{
+			name:  "percent in path and query",
+			path:  "/photos/a%20b.jpg",
+			query: "q=50%",
+			// Signed data:
+			// "cdn.example.com:/photos/a%2520b.jpg:q=50%25:800:600:webp:1704067200:85:cover"
+			wantSignature: "MjDiiRdgXMrsjqPMsENtD4aG1hp1e-a6zy67vKTX4CQ=",
+		},
+	}
+
+	for _, tt := range vectors {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			req := &signature.Request{
+				SourceHost:  testHost,
+				SourcePath:  tt.path,
+				SourceQuery: tt.query,
+				Width:       800,
+				Height:      600,
+				Format:      testFormatWebP,
+				Quality:     85,
+				FitMode:     testFitCover,
+				Expires:     time.Unix(goldenExpiresUnix, 0),
+			}
+
+			gotSignature := signer.Sign(req)
+			t.Logf("Sign() of path %q query %q = %s", tt.path, tt.query, gotSignature)
+
+			if gotSignature != tt.wantSignature {
+				t.Errorf("Sign() = %q, want %q", gotSignature, tt.wantSignature)
+			}
+		})
+	}
+}

internal/signature/signature.go

@@ -10,6 +10,7 @@ import (
 	"fmt"
 	"net/url"
 	"strconv"
+	"strings"
 	"time"
 )
 
@@ -154,15 +155,17 @@ func (s *Signer) GenerateSignedURL(
 
 // buildSignatureData creates the string to be signed.
 // Format: "host:path:query:width:height:format:expiration:quality:fit"
-// All components are used verbatim (exact match). No normalization,
+// The path and query go through escapePercentAndColon, so that a ":" in
-// suffix matching, or wildcard expansion is performed. Quality and fit
+// either cannot be taken for the one between them; every other component
-// are the effective transform values, so replaying a signed URL with a
+// is used verbatim. No normalization, suffix matching, or wildcard
-// different quality or fit mode fails verification.
+// expansion is performed. Quality and fit are the effective transform
+// values, so replaying a signed URL with a different quality or fit mode
+// fails verification.
 func (s *Signer) buildSignatureData(req *Request) string {
 	return fmt.Sprintf("%s:%s:%s:%d:%d:%s:%d:%d:%s",
 		req.SourceHost,
-		req.SourcePath,
+		escapePercentAndColon(req.SourcePath),
-		req.SourceQuery,
+		escapePercentAndColon(req.SourceQuery),
 		req.Width,
 		req.Height,
 		req.Format,
@@ -172,6 +175,14 @@ func (s *Signer) buildSignatureData(req *Request) string {
 	)
 }
 
+// escapePercentAndColon returns s with each "%" written as "%25" and each
+// ":" as "%3A", so that the result holds no ":".
+func escapePercentAndColon(s string) string {
+	s = strings.ReplaceAll(s, "%", "%25")
+
+	return strings.ReplaceAll(s, ":", "%3A")
+}
+
 // ParseParams extracts signature and expiration from query parameters.
 func ParseParams(sig, expStr string) (string, time.Time, error) {
 	if expStr == "" {