From 22bfc432b676bf115b5bfc7b818e9d6e581359e9 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Fri, 16 Oct 2026 00:17:33 +0200 Subject: [PATCH] Document how a source query goes into an image URL, and accept it (closes #255) The README gave no way to put a source URL's query into a signed image URL, and GenerateSignedURL built one that failed: the route parsed chi's wildcard, which keeps the path as the client wrote it, so one %2F, %3B or %2C left the %3F undecoded and the signature failed. The route now parses the decoded path, GenerateSignedURL writes Go's own encoding of it, and the README says the ? before the query, any ? in it and every % must be percent-encoded, with a signed example. The path traversal check no longer reads the query and accepts a literal %, while a double-encoded .. stays refused. A percent-escape in a source path is now decoded once. Model: opus-5-5 --- README.md | 25 +++ TODO.md | 14 ++ internal/handlers/image.go | 16 +- .../handlers/image_signature_internal_test.go | 163 ++++++++++++++++++ internal/imgcache/urlparser.go | 47 +++-- internal/imgcache/urlparser_internal_test.go | 65 +++++++ internal/signature/signature.go | 33 ++-- internal/signature/signature_test.go | 38 ++++ 8 files changed, 353 insertions(+), 48 deletions(-) diff --git a/README.md b/README.md index 2666d03..4fc97e9 100644 --- a/README.md +++ b/README.md @@ -316,6 +316,20 @@ once, is refused with 400. both optional (values under Signature Specification). Both are part of what is cached, so each value of either is a separate cached image. +A source URL's query follows its path after `%3F`, the encoded `?`: +`/v1/image//%3F/.`. pixa decodes the +percent-escapes in the image URL's path once, then reads the host up to the +first `/`, the size and format after the last `/`, and between them the source +path up to the first `?` and the source query after it. So the `?` before the +query, any `?` in the query, and every `%` in the source path or query must be +percent-encoded, as `%3F` and `%25`. Any other character may be written as +itself or percent-encoded: `/`, `;`, `,`, `=` and `&` work either way. For +example, +`https://cdn.example.com/img?url=https://origin.example.com/my%20cat.jpg` at +800x600 WebP is +`/v1/image/cdn.example.com/img%3Furl=https://origin.example.com/my%2520cat.jpg/800x600.webp`, +signed as shown under Signature Specification. + A size with one side `0` has that side scaled to keep the source's proportions, and is refused with 400 when that side would come out above 8192, the most either side may be; so is a size with `fit=outside`, which scales the image to @@ -535,6 +549,17 @@ For the same image at quality 40 with fit `contain`, the input ends in and the URL is `/v1/image/cdn.example.com/photos/cat.jpg/800x600.webp?sig=5IwXUx6vf7yefhaUvFzgXZvG2o0Df4RJxPTK3pKq5VU=&exp=1704067200&q=40&fit=contain`. +For a source URL with a query, +`https://cdn.example.com/img?url=https://origin.example.com/my%20cat.jpg`, at +800x600 WebP with the same key and expiration and the default quality and fit: + +1. Build input, with the query's `:` written as `%3A` and its `%` as `%25`: + `cdn.example.com:/img:url=https%3A//origin.example.com/my%2520cat.jpg:800:600:webp:1704067200:85:cover` +2. The signature is `37aFa9GSVCZM1kRY9SzMOk2fOg_M7Wt5gKNR4xM-FYA=` +3. URL, with the `?` before the query written as `%3F` and the query's `%` as + `%25` (see Routes): + `/v1/image/cdn.example.com/img%3Furl=https://origin.example.com/my%2520cat.jpg/800x600.webp?sig=37aFa9GSVCZM1kRY9SzMOk2fOg_M7Wt5gKNR4xM-FYA=&exp=1704067200` + **Allowlist patterns:** - **Exact match**: `cdn.example.com` — matches only that host diff --git a/TODO.md b/TODO.md index 1b59abf..0ef9f6b 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,20 @@ P2: security: per-origin rate limiting # Completed Steps +- 2026-10-15 the README says how a source URL with a query is written into an + image URL, and the image route accepts it (closes #255): the query follows the + source path after `%3F`. pixa now reads the image URL's path decoded once, so + the `?` before the query, any `?` in it and every `%` in the source path or + query must be percent-encoded, and any other character may be. The route used + to parse chi's wildcard, which holds the path as the client wrote it whenever + that is not Go's own encoding of it; a `/`, `;` or `,` in the query written as + `%2F`, `%3B` or `%2C`, as `GenerateSignedURL` wrote them, kept the `%3F` from + being read as `?`, and the request failed with 401. `GenerateSignedURL` now + writes Go's own encoding of the path. The README has an example with a query + and its signature. The path traversal check no longer reads the source query, + and keeps a `%` that does not start a percent-escape as it is instead of + refusing the URL, so a source path or query holding `%25` is accepted, while a + double-encoded `..` next to such a `%` is still refused. - 2026-10-15 `max_concurrent_processing` defaults to half the CPUs pixa sees, rounded down, and at least 1 (closes #242): it defaulted to one image per CPU, and a container with no CPU limit sees every CPU of the host, so on a 48-CPU diff --git a/internal/handlers/image.go b/internal/handlers/image.go index 9b28da1..96dad32 100644 --- a/internal/handlers/image.go +++ b/internal/handlers/image.go @@ -9,7 +9,6 @@ import ( "strconv" "time" - "github.com/go-chi/chi/v5" "github.com/go-chi/chi/v5/middleware" "sneak.berlin/go/pixa/internal/encurl" "sneak.berlin/go/pixa/internal/httpfetcher" @@ -80,20 +79,21 @@ func (s *Handlers) HandleRobotsTxt() http.HandlerFunc { } } -// parseImageRequest parses the wildcard path and query parameters into +// parseImageRequest parses the image URL's path and query parameters into // an ImageRequest. On invalid input it writes an error response and // returns false. func (s *Handlers) parseImageRequest( w http.ResponseWriter, r *http.Request, ) (*imgcache.ImageRequest, bool) { - // Get the wildcard path from chi - pathParam := chi.URLParam(r, "*") - - // Parse the URL path - parsed, err := imgcache.ParseImagePath(pathParam) + // The path is parsed decoded once, as r.URL.Path holds it, and not from + // chi's wildcard: chi routes on the path as the client wrote it whenever + // that is not Go's own encoding of it, so its wildcard would still hold + // the %3F before a source query when, say, a "/" in the query is written + // as %2F + parsed, err := imgcache.ParseImageURL(r.URL.Path) if err != nil { s.log.Warn("failed to parse image URL", - "path", pathParam, + "path", r.URL.Path, "error", err, ) s.respondError(w, "invalid image URL: "+err.Error(), http.StatusBadRequest) diff --git a/internal/handlers/image_signature_internal_test.go b/internal/handlers/image_signature_internal_test.go index c057514..babdb1e 100644 --- a/internal/handlers/image_signature_internal_test.go +++ b/internal/handlers/image_signature_internal_test.go @@ -169,3 +169,166 @@ func TestHandleImage_InvalidExp_Returns400(t *testing.T) { }) } } + +// TestHandleImage_GeneratedSignedURLWithSourceQueryVerifies sends URLs built +// by the service's signed-URL generator for source URLs with a query to the +// route, through a chi router: chi routes on the path as the client wrote it +// whenever that is not Go's own encoding of it. A 502 means the signature +// passed, so the route read the source path and query that were signed. +func TestHandleImage_GeneratedSignedURLWithSourceQueryVerifies(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + query string + }{ + {"slash and colon", "url=https://origin.example.com/cat.jpg"}, + {"semicolon", "a=1;b=2"}, + {"comma", "crop=0,0,50,50"}, + {"equals and ampersand", "a=1&b=2"}, + {"percent-escape", "name=my%20cat"}, + {"question mark", "next=/a?b=1"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + fix := setupTestHandler(t) + + signedURL, err := fix.service.GenerateSignedURL("", &imgcache.ImageRequest{ + SourceHost: signedHost, + SourcePath: photoPath, + SourceQuery: tt.query, + Size: imgcache.Size{Width: 50, Height: 50}, + Format: imgcache.FormatJPEG, + }, time.Hour) + if err != nil { + t.Fatalf("GenerateSignedURL() error = %v", err) + } + + status := getImage(t, fix, signedURL) + if status != http.StatusBadGateway { + t.Errorf("status = %d, want %d", status, http.StatusBadGateway) + } + }) + } +} + +// TestHandleImage_SourceQueryEncodedAnyWay signs one source URL with a query +// and writes it into the image URL in ways the README allows: the "?" before +// the query and each "%" in it percent-encoded, and every other character as +// itself or percent-encoded. A 502 means the signature passed. +func TestHandleImage_SourceQueryEncodedAnyWay(t *testing.T) { + t.Parallel() + + expires := time.Now().Add(time.Hour) + signer := signature.New("test-signing-key-must-be-32-chars") + sig := signer.Sign(&signature.Request{ + SourceHost: signedHost, + SourcePath: photoPath, + SourceQuery: "url=https://origin.example.com/my%20cat.jpg", + Width: 50, + Height: 50, + Format: string(imgcache.FormatJPEG), + Quality: 85, + FitMode: string(imgcache.FitCover), + Expires: expires, + }) + + tests := []struct { + name string + writtenQuery string + }{ + {"only ? and % encoded", + "%3Furl=https://origin.example.com/my%2520cat.jpg"}, + {"every reserved character encoded", + "%3Furl%3Dhttps%3A%2F%2Forigin.example.com%2Fmy%2520cat.jpg"}, + {"lower-case hex digits", + "%3furl=https://origin.example.com/my%2520cat.jpg"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + target := fmt.Sprintf("/v1/image/%s%s%s/50x50.jpeg?sig=%s&exp=%d", + signedHost, photoPath, tt.writtenQuery, sig, expires.Unix()) + + status := getImage(t, setupTestHandler(t), target) + if status != http.StatusBadGateway { + t.Errorf("status = %d, want %d", status, http.StatusBadGateway) + } + }) + } +} + +// TestHandleImage_SourceURLHoldingPercent25Verifies signs a source path and a +// source query that each hold %25, the percent-escape of a literal "%", and +// sends each to the route written as the README says, with that "%" written as +// %25, and as GenerateSignedURL writes it. A 502 means the signature passed. +func TestHandleImage_SourceURLHoldingPercent25Verifies(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + sourcePath string + sourceQuery string + // writtenPathAndQuery is the source path and query as the README says + // to write them into the image URL + writtenPathAndQuery string + }{ + {"path", "/sale/50%25off.jpg", "", "/sale/50%2525off.jpg"}, + {"query", photoPath, "discount=50%25", photoPath + "%3Fdiscount=50%2525"}, + } + + for _, tt := range tests { + t.Run(tt.name+" written by the README rule", func(t *testing.T) { + t.Parallel() + + expires := time.Now().Add(time.Hour) + signer := signature.New("test-signing-key-must-be-32-chars") + sig := signer.Sign(&signature.Request{ + SourceHost: signedHost, + SourcePath: tt.sourcePath, + SourceQuery: tt.sourceQuery, + Width: 50, + Height: 50, + Format: string(imgcache.FormatJPEG), + Quality: 85, + FitMode: string(imgcache.FitCover), + Expires: expires, + }) + + target := fmt.Sprintf("/v1/image/%s%s/50x50.jpeg?sig=%s&exp=%d", + signedHost, tt.writtenPathAndQuery, sig, expires.Unix()) + + status := getImage(t, setupTestHandler(t), target) + if status != http.StatusBadGateway { + t.Errorf("status = %d, want %d", status, http.StatusBadGateway) + } + }) + + t.Run(tt.name+" written by GenerateSignedURL", func(t *testing.T) { + t.Parallel() + + fix := setupTestHandler(t) + + signedURL, err := fix.service.GenerateSignedURL("", &imgcache.ImageRequest{ + SourceHost: signedHost, + SourcePath: tt.sourcePath, + SourceQuery: tt.sourceQuery, + Size: imgcache.Size{Width: 50, Height: 50}, + Format: imgcache.FormatJPEG, + }, time.Hour) + if err != nil { + t.Fatalf("GenerateSignedURL() error = %v", err) + } + + status := getImage(t, fix, signedURL) + if status != http.StatusBadGateway { + t.Errorf("status = %d, want %d", status, http.StatusBadGateway) + } + }) + } +} diff --git a/internal/imgcache/urlparser.go b/internal/imgcache/urlparser.go index 4247be6..1794d1f 100644 --- a/internal/imgcache/urlparser.go +++ b/internal/imgcache/urlparser.go @@ -47,6 +47,9 @@ func ValidateDimension(name string, value int) error { var sizeFormatRegex = regexp.MustCompile( `^(\d+)x(\d+)(?:\.(\w+))?$|^(orig)(?:\.(\w+))?$`) +// percentEscapeRegex matches one percent-escape, such as "%2e" +var percentEscapeRegex = regexp.MustCompile(`%[0-9A-Fa-f]{2}`) + // ParsedURL contains the parsed components of an image proxy URL. type ParsedURL struct { // Host is the source origin host (e.g., "cdn.example.com") @@ -61,9 +64,8 @@ type ParsedURL struct { Format ImageFormat } -// ParseImagePath parses the path captured by chi's wildcard: -// //., or // for JPEG XL -// This is the primary entry point when using chi routing. +// ParseImagePath parses an image URL's path after /v1/image/, decoded once: +// //., or // for JPEG XL. // Examples: // - cdn.example.com/photos/cat.jpg/800x600.webp // - cdn.example.com/photos/cat.jpg/0x0.jpeg @@ -79,9 +81,10 @@ func ParseImagePath(path string) (*ParsedURL, error) { return parseImageComponents(path) } -// ParseImageURL parses a full URL path like /v1/image///., -// or /v1/image/// for JPEG XL -// Use ParseImagePath instead when working with chi's wildcard capture. +// ParseImageURL parses an image URL's path, decoded once as r.URL.Path holds +// it: /v1/image///., or /v1/image/// +// for JPEG XL. A source query follows the source path after a "?", which the +// URL writes as %3F. func ParseImageURL(urlPath string) (*ParsedURL, error) { // Remove the /v1/image/ prefix const prefix = "/v1/image/" @@ -100,12 +103,6 @@ func ParseImageURL(urlPath string) (*ParsedURL, error) { // parseImageComponents parses //., or // // for JPEG XL. func parseImageComponents(remainder string) (*ParsedURL, error) { - // Check for path traversal before any other processing - err := checkPathTraversal(remainder) - if err != nil { - return nil, err - } - // Find the last path segment, which holds "size" or "size.format" lastSlash := strings.LastIndex(remainder, "/") if lastSlash == -1 { @@ -161,6 +158,13 @@ func parseImageComponents(remainder string) (*ParsedURL, error) { path = path[:qIndex] } + // The source query names no file on the upstream, so only the host and + // the source path are checked for path traversal + err = checkPathTraversal(host + path) + if err != nil { + return nil, err + } + // Ensure path starts with / if !strings.HasPrefix(path, "/") { path = "/" + path @@ -186,12 +190,7 @@ func checkPathTraversal(path string) error { // Decode multiple times to catch double-encoding decoded := path for range 3 { - newDecoded, err := url.PathUnescape(decoded) - if err != nil { - // Malformed encoding is suspicious - return ErrPathTraversal - } - + newDecoded := decodePercentEscapes(decoded) if newDecoded == decoded { break } @@ -232,6 +231,18 @@ func checkPathTraversal(path string) error { return nil } +// decodePercentEscapes decodes every percent-escape in s, such as %2e, and +// keeps a "%" that does not start one as it is, where url.PathUnescape would +// refuse the whole string. A source path holding %25, a literal "%", has such +// a "%" after one more decoding. +func decodePercentEscapes(s string) string { + return percentEscapeRegex.ReplaceAllStringFunc(s, func(escape string) string { + decoded, _ := url.PathUnescape(escape) + + return decoded + }) +} + // parseSizeFormat parses strings like "800x600.webp", "orig.png" or "800x600" func parseSizeFormat(s string) (Size, ImageFormat, error) { matches := sizeFormatRegex.FindStringSubmatch(s) diff --git a/internal/imgcache/urlparser_internal_test.go b/internal/imgcache/urlparser_internal_test.go index 518a04e..603d516 100644 --- a/internal/imgcache/urlparser_internal_test.go +++ b/internal/imgcache/urlparser_internal_test.go @@ -407,6 +407,71 @@ func TestParseImagePath_PathTraversal(t *testing.T) { } } +// TestParseImageURL_SourceURLHoldingPercent25 verifies that a source path and +// a source query holding %25, the percent-escape of a literal "%", are +// accepted as they are, although decoding them once more leaves a "%" that +// does not start a percent-escape. +func TestParseImageURL_SourceURLHoldingPercent25(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + input string + want *ParsedURL + }{ + { + name: "path", + input: "/v1/image/cdn.example.com/sale/50%25off.jpg/800x600.webp", + want: &ParsedURL{ + Host: testHostCDN, Path: "/sale/50%25off.jpg", + Size: Size{Width: 800, Height: 600}, Format: FormatWebP, + }, + }, + { + name: "query", + input: "/v1/image/cdn.example.com/photos/cat.jpg?discount=50%25/800x600.webp", + want: &ParsedURL{ + Host: testHostCDN, Path: testPathCat, Query: "discount=50%25", + Size: Size{Width: 800, Height: 600}, Format: FormatWebP, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + got, err := ParseImageURL(tt.input) + if err != nil { + t.Fatalf("ParseImageURL() unexpected error = %v", err) + } + + assertParsedURL(t, got, tt.want) + }) + } +} + +// TestParseImageURL_DoubleEncodedTraversalNextToPercent verifies that a +// double-encoded ".." is still refused when the path also holds a "%" that +// does not start a percent-escape, either as written or after one more +// decoding. +func TestParseImageURL_DoubleEncodedTraversalNextToPercent(t *testing.T) { + t.Parallel() + + for _, input := range []string{ + "/v1/image/cdn.example.com/photos/%252e%252e/secret/50%25off.jpg/800x600.jpeg", + "/v1/image/cdn.example.com/photos/%252e%252e/secret/100%/800x600.jpeg", + } { + _, err := ParseImageURL(input) + t.Logf("ParseImageURL(%q) error = %v", input, err) + + if !errors.Is(err, ErrPathTraversal) { + t.Errorf("ParseImageURL(%q) error = %v, want ErrPathTraversal", + input, err) + } + } +} + // errorIs checks if err matches target (handles wrapped errors). func errorIs(err, target error) bool { if errors.Is(err, target) { diff --git a/internal/signature/signature.go b/internal/signature/signature.go index a683e39..a507240 100644 --- a/internal/signature/signature.go +++ b/internal/signature/signature.go @@ -126,31 +126,20 @@ func (s *Signer) GenerateSignedURL( sizeStr = fmt.Sprintf("%dx%d", req.Width, req.Height) } - // Build the path. - // When a source query is present, it is embedded as a path segment - // (e.g. /host/path?query/size.fmt) so that the URL parser can extract - // it from the last-slash split. The "?" inside a path segment is - // percent-encoded by clients but chi delivers it decoded, which is - // exactly what the URL parser expects. - var path string + // Build the path. A source query follows the source path after a "?". + // The image route reads the path decoded once, so the whole path is + // written in Go's own percent-encoding of a URL path, which writes "?" + // as %3F and "%" as %25, and leaves "/", ";", ",", "=" and "&" as they + // are. + sourcePathAndQuery := req.SourcePath if req.SourceQuery != "" { - path = fmt.Sprintf("/v1/image/%s%s%%3F%s/%s.%s", - req.SourceHost, - req.SourcePath, - url.PathEscape(req.SourceQuery), - sizeStr, - req.Format, - ) - } else { - path = fmt.Sprintf("/v1/image/%s%s/%s.%s", - req.SourceHost, - req.SourcePath, - sizeStr, - req.Format, - ) + sourcePathAndQuery += "?" + req.SourceQuery } - return path, sig, exp + unescapedPath := fmt.Sprintf("/v1/image/%s%s/%s.%s", + req.SourceHost, sourcePathAndQuery, sizeStr, req.Format) + + return (&url.URL{Path: unescapedPath}).EscapedPath(), sig, exp } // buildSignatureData creates the string to be signed. diff --git a/internal/signature/signature_test.go b/internal/signature/signature_test.go index 1712fca..559d19c 100644 --- a/internal/signature/signature_test.go +++ b/internal/signature/signature_test.go @@ -475,6 +475,44 @@ func TestGenerateSignedURL_WithoutQueryString(t *testing.T) { } } +// TestGenerateSignedURL_ReadmeQueryExample checks the README's example of a +// source URL with a query, the image at +// https://cdn.example.com/img?url=https://origin.example.com/my%20cat.jpg: its +// signature with the README's key, and the path GenerateSignedURL writes for +// it, with the "?" written as %3F and the "%" as %25. +func TestGenerateSignedURL_ReadmeQueryExample(t *testing.T) { + t.Parallel() + + signer := signature.New("example-signing-key-for-documentation") + + req := signature.Request{ + SourceHost: testHost, + SourcePath: "/img", + SourceQuery: "url=https://origin.example.com/my%20cat.jpg", + Width: 800, + Height: 600, + Format: testFormatWebP, + Quality: 85, + FitMode: testFitCover, + Expires: time.Unix(1704067200, 0), + } + + const wantSig = "37aFa9GSVCZM1kRY9SzMOk2fOg_M7Wt5gKNR4xM-FYA=" + + gotSig := signer.Sign(&req) + if gotSig != wantSig { + t.Errorf("Sign() = %q, want %q", gotSig, wantSig) + } + + const wantPath = "/v1/image/cdn.example.com" + + "/img%3Furl=https://origin.example.com/my%2520cat.jpg/800x600.webp" + + gotPath, _, _ := signer.GenerateSignedURL(&req, time.Hour) + if gotPath != wantPath { + t.Errorf("GenerateSignedURL() path = %q, want %q", gotPath, wantPath) + } +} + func TestParseParams(t *testing.T) { t.Parallel()