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()