Check the negative cache only before a fetch from the upstream (closes #245)

The check moves from Get to the start of fetchAndProcess, so a variant
not in the cache is still made from a cached source while the URL is in
the negative cache. A refusal now counts as a miss.

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
@@ -193,9 +200,10 @@ 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 the same
-  source URL; 503 when pixa is busy or in maintenance mode; 500 for any other
-  failure.
+  answered with an error status, and for 5 minutes after that for an image of
+  the same source URL that pixa can neither serve from its cache nor make from
+  the source image still in its cache; 503 when pixa is busy or in maintenance
+  mode; 500 for any other failure.
 - `GET` or `HEAD` `/v1/e/<token>/<name>` — an image through an encrypted URL
   (see Encrypted URLs). Needs: nothing but the URL. Answers: 200; 304 when
   `If-None-Match` matches the image's `ETag`; 400 for a token that does not
@@ -412,8 +420,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`)
-- `query` — source query string, empty string if none
+- `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
+  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`

TODO.md

@@ -30,6 +30,41 @@ P2: security: per-origin rate limiting
 
 # Completed Steps
 
+- 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 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:
-  - s3.sneak.cloud
-  - static.sneak.cloud
-  - sneak.berlin
-  - github.com
-  - user-images.githubusercontent.com
+# allowlist_hosts:
+#   - images.example.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/concurrency_limits_internal_test.go

@@ -32,28 +32,6 @@ func TestOmittedConcurrencyLimitsUseDefaults(t *testing.T) {
 	}
 }
 
-// TestOmittedMaxConcurrentProcessingIsAtMostFour checks that an omitted
-// max_concurrent_processing is the number of CPUs Go uses, but never more
-// than 4, so a container with no CPU limit on a host with many CPUs does not
-// process one image per CPU at once.
-func TestOmittedMaxConcurrentProcessingIsAtMostFour(t *testing.T) {
-	t.Parallel()
-
-	c, err := configFromYAML(t, signingKeyLine)
-	if err != nil {
-		t.Fatalf("minimal config should be valid, got error: %v", err)
-	}
-
-	want := min(runtime.GOMAXPROCS(0), 4)
-	t.Logf("GOMAXPROCS %d, MaxConcurrentProcessing %d",
-		runtime.GOMAXPROCS(0), c.MaxConcurrentProcessing)
-
-	if c.MaxConcurrentProcessing != want {
-		t.Errorf("MaxConcurrentProcessing = %d, want %d",
-			c.MaxConcurrentProcessing, want)
-	}
-}
-
 // TestExplicitConcurrencyLimitsAreUsed checks that valid values for the
 // two limits are used as given.
 func TestExplicitConcurrencyLimitsAreUsed(t *testing.T) {

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/imageprocessor/max_concurrent_processing_internal_test.go

@@ -132,27 +132,6 @@ func TestNewDefaultsMaxConcurrentProcessingToCPUs(t *testing.T) {
 	}
 }
 
-// TestNewDefaultMaxConcurrentProcessingIsAtMostFour checks that New, given
-// no MaxConcurrentProcessing, makes one slot per CPU Go uses, but never more
-// than 4.
-func TestNewDefaultMaxConcurrentProcessingIsAtMostFour(t *testing.T) {
-	t.Parallel()
-
-	want := min(runtime.GOMAXPROCS(0), 4)
-
-	for _, limit := range []int{0, -1} {
-		proc := New(Params{MaxConcurrentProcessing: limit})
-		got := cap(proc.processingSemaphore)
-		t.Logf("MaxConcurrentProcessing %d, GOMAXPROCS %d: %d slots",
-			limit, runtime.GOMAXPROCS(0), got)
-
-		if got != want {
-			t.Errorf("MaxConcurrentProcessing %d: %d slots, want %d",
-				limit, got, want)
-		}
-	}
-}
-
 // TestProcessNeverExceedsMaxConcurrentProcessing starts more Process calls
 // than MaxConcurrentProcessing allows and holds the first ones inside
 // Process until the test lets them go. No more than the limit may be

internal/imgcache/negative_cache_internal_test.go

@@ -94,6 +94,71 @@ 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)
+	}
+}
+
 func TestService_Get_ReturnsErrorForNegativeCachedURL(t *testing.T) {
 	t.Parallel()
 

internal/imgcache/service.go

@@ -129,25 +129,13 @@ 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 {
@@ -423,12 +411,28 @@ 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.
+// reading the response or a later step fails. It fetches nothing while the
+// 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/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,
-		req.SourceQuery,
+		escapePercentAndColon(req.SourcePath),
+		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/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,
-// suffix matching, or wildcard 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.
+// The path and query go through escapePercentAndColon, so that a ":" in
+// either cannot be taken for the one between them; every other component
+// is used verbatim. No normalization, suffix matching, or wildcard
+// 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,
-		req.SourceQuery,
+		escapePercentAndColon(req.SourcePath),
+		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 == "" {