From 4634d2dc2fe2b737d86f0ecec75088d7d8ef0698 Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 15 Oct 2026 21:01:41 +0000 Subject: [PATCH 1/4] Test that a source query in an image URL reaches the signature check Round trips through a chi router for source queries holding /, ;, ,, =, &, ? and a percent-escape, both as GenerateSignedURL writes them and with characters other than ? and % percent-encoded, plus the README's example of a source URL with a query. These fail until the fix. Model: opus-5-5 --- .../handlers/image_signature_internal_test.go | 93 +++++++++++++++++++ internal/signature/signature_test.go | 37 ++++++++ 2 files changed, 130 insertions(+) diff --git a/internal/handlers/image_signature_internal_test.go b/internal/handlers/image_signature_internal_test.go index c057514..b74ed18 100644 --- a/internal/handlers/image_signature_internal_test.go +++ b/internal/handlers/image_signature_internal_test.go @@ -169,3 +169,96 @@ 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: "/images/photo.jpg", + 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: "/images/photo.jpg", + 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 + writtenSource string + }{ + {"only ? and % encoded", + "/images/photo.jpg%3Furl=https://origin.example.com/my%2520cat.jpg"}, + {"every reserved character encoded", + "/images/photo.jpg%3Furl%3Dhttps%3A%2F%2Forigin.example.com%2Fmy%2520cat.jpg"}, + {"lower-case hex digits", + "/images/photo.jpg%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/50x50.jpeg?sig=%s&exp=%d", + signedHost, tt.writtenSource, sig, expires.Unix()) + + status := getImage(t, setupTestHandler(t), target) + if status != http.StatusBadGateway { + t.Errorf("status = %d, want %d", status, http.StatusBadGateway) + } + }) + } +} diff --git a/internal/signature/signature_test.go b/internal/signature/signature_test.go index 1712fca..939fda0 100644 --- a/internal/signature/signature_test.go +++ b/internal/signature/signature_test.go @@ -475,6 +475,43 @@ func TestGenerateSignedURL_WithoutQueryString(t *testing.T) { } } +// TestGenerateSignedURL_ReadmeQueryExample checks the README's example of a +// source URL with a query, 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() -- 2.54.0 From f9c878161ac19d519ca775a38846d8f819db8391 Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 15 Oct 2026 21:05:33 +0000 Subject: [PATCH 2/4] Document how a source query goes into an image URL, and accept it (closes #255) The query follows the source path after %3F. The image route now parses the request's path decoded once instead of chi's wildcard, which holds the path as the client wrote it whenever that is not Go's own encoding of it, so a / ; or , written as %2F, %3B or %2C, as GenerateSignedURL wrote them, hid the %3F and the request failed with 401. Now the ? and every % in the source path or query must be percent-encoded and any other character may be. GenerateSignedURL writes Go's own encoding of the path. The README states the rule, with an example and its signature. Model: opus-5-5 --- README.md | 25 ++++++++++++++ TODO.md | 11 +++++++ internal/handlers/image.go | 16 ++++----- .../handlers/image_signature_internal_test.go | 18 +++++----- internal/imgcache/urlparser.go | 12 +++---- internal/signature/signature.go | 33 +++++++------------ internal/signature/signature_test.go | 7 ++-- 7 files changed, 74 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..1fcd110 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,17 @@ 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. - 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 b74ed18..0ddecf2 100644 --- a/internal/handlers/image_signature_internal_test.go +++ b/internal/handlers/image_signature_internal_test.go @@ -198,7 +198,7 @@ func TestHandleImage_GeneratedSignedURLWithSourceQueryVerifies(t *testing.T) { signedURL, err := fix.service.GenerateSignedURL("", &imgcache.ImageRequest{ SourceHost: signedHost, - SourcePath: "/images/photo.jpg", + SourcePath: photoPath, SourceQuery: tt.query, Size: imgcache.Size{Width: 50, Height: 50}, Format: imgcache.FormatJPEG, @@ -226,7 +226,7 @@ func TestHandleImage_SourceQueryEncodedAnyWay(t *testing.T) { signer := signature.New("test-signing-key-must-be-32-chars") sig := signer.Sign(&signature.Request{ SourceHost: signedHost, - SourcePath: "/images/photo.jpg", + SourcePath: photoPath, SourceQuery: "url=https://origin.example.com/my%20cat.jpg", Width: 50, Height: 50, @@ -237,23 +237,23 @@ func TestHandleImage_SourceQueryEncodedAnyWay(t *testing.T) { }) tests := []struct { - name string - writtenSource string + name string + writtenQuery string }{ {"only ? and % encoded", - "/images/photo.jpg%3Furl=https://origin.example.com/my%2520cat.jpg"}, + "%3Furl=https://origin.example.com/my%2520cat.jpg"}, {"every reserved character encoded", - "/images/photo.jpg%3Furl%3Dhttps%3A%2F%2Forigin.example.com%2Fmy%2520cat.jpg"}, + "%3Furl%3Dhttps%3A%2F%2Forigin.example.com%2Fmy%2520cat.jpg"}, {"lower-case hex digits", - "/images/photo.jpg%3furl=https://origin.example.com/my%2520cat.jpg"}, + "%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/50x50.jpeg?sig=%s&exp=%d", - signedHost, tt.writtenSource, sig, expires.Unix()) + 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 { diff --git a/internal/imgcache/urlparser.go b/internal/imgcache/urlparser.go index 4247be6..fa8fd10 100644 --- a/internal/imgcache/urlparser.go +++ b/internal/imgcache/urlparser.go @@ -61,9 +61,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 +78,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/" 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 939fda0..559d19c 100644 --- a/internal/signature/signature_test.go +++ b/internal/signature/signature_test.go @@ -476,9 +476,10 @@ func TestGenerateSignedURL_WithoutQueryString(t *testing.T) { } // TestGenerateSignedURL_ReadmeQueryExample checks the README's example of a -// source URL with a query, 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. +// 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() -- 2.54.0 From c49d509a21085063421adba3f9fbfed4427bbbea Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 15 Oct 2026 21:40:55 +0000 Subject: [PATCH 3/4] Test that a source path or query holding %25 passes the signature check A source URL holding %25, a literal "%", written by the README rule or by GenerateSignedURL, is refused with 400: the path traversal check refuses the "%" that one more decoding leaves. A double-encoded ".." next to such a "%" must stay refused. Model: opus-5-5 --- .../handlers/image_signature_internal_test.go | 70 +++++++++++++++++++ internal/imgcache/urlparser_internal_test.go | 65 +++++++++++++++++ 2 files changed, 135 insertions(+) diff --git a/internal/handlers/image_signature_internal_test.go b/internal/handlers/image_signature_internal_test.go index 0ddecf2..babdb1e 100644 --- a/internal/handlers/image_signature_internal_test.go +++ b/internal/handlers/image_signature_internal_test.go @@ -262,3 +262,73 @@ func TestHandleImage_SourceQueryEncodedAnyWay(t *testing.T) { }) } } + +// 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_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) { -- 2.54.0 From 88ea15eda8af787e17111f8191bb76ec7723e435 Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 15 Oct 2026 21:43:53 +0000 Subject: [PATCH 4/4] Accept a source path or query holding %25 in an image URL The path traversal check now reads only the host and source path, not the source query, and keeps a "%" that does not start a percent-escape as it is, decoding the escapes around it, instead of refusing the URL. Stopping the decoding at that "%" would let a double-encoded ".." elsewhere in the path through. Model: opus-5-5 --- TODO.md | 5 ++++- internal/imgcache/urlparser.go | 35 ++++++++++++++++++++++------------ 2 files changed, 27 insertions(+), 13 deletions(-) diff --git a/TODO.md b/TODO.md index 1fcd110..0ef9f6b 100644 --- a/TODO.md +++ b/TODO.md @@ -40,7 +40,10 @@ P2: security: per-origin rate limiting `%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. + 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/imgcache/urlparser.go b/internal/imgcache/urlparser.go index fa8fd10..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") @@ -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) -- 2.54.0