From bb25e306a3a53bfa896f5837dcba46f6a673388f Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 15 Oct 2026 22:25:02 +0000 Subject: [PATCH 1/4] Test that the image route refuses a host no URL host can be A host written with a percent-encoded ?, #, %, \ or space, and an IPv6 address without brackets, must be refused with 400; a host name, an IPv4 address, an IPv6 address in brackets and a host with a port, or a : and no port, must still be served. The first test fails until the fix for https://git.eeqj.de/sneak/pixa/issues/275 lands. Model: opus-5-5 --- .../handlers/image_errors_internal_test.go | 72 +++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/internal/handlers/image_errors_internal_test.go b/internal/handlers/image_errors_internal_test.go index fbbcf6f..27f1d4f 100644 --- a/internal/handlers/image_errors_internal_test.go +++ b/internal/handlers/image_errors_internal_test.go @@ -264,3 +264,75 @@ func TestHandleImage_AllowlistOrSignature(t *testing.T) { }) } } + +// TestHandleImage_InvalidHost_Returns400 sends image URLs whose host is +// allowlistedHost with a character after it that a URL host cannot hold, +// percent-encoded, and checks that each is refused with 400 naming the host +// as decoded. A "?" or "#" there would otherwise end the host when the source +// URL is read, so the request would pass the allowlist and be fetched and +// cached under a host that is not the one fetched. +func TestHandleImage_InvalidHost_Returns400(t *testing.T) { + t.Parallel() + + photos := newPhotoFetcher(t, allowlistedHost) + + tests := []struct { + name string + writtenHost string + decodedHost string + }{ + {"? written as %3F", allowlistedHost + "%3Fx=1", allowlistedHost + "?x=1"}, + {"# written as %23", allowlistedHost + "%23x", allowlistedHost + "#x"}, + {"% written as %25", allowlistedHost + "%253F", allowlistedHost + "%3F"}, + {`\ written as %5C`, allowlistedHost + "%5Cx", allowlistedHost + `\x`}, + {"space written as %20", allowlistedHost + "%20", allowlistedHost + " "}, + {"IPv6 address without brackets", "2001:db8::1", "2001:db8::1"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + rec := sendGet(t, newImageRoute(t, photos), photoURL(tt.writtenHost)) + checkErrorBody(t, rec, http.StatusBadRequest, + fmt.Sprintf("invalid image URL: invalid source host: %q", + tt.decodedHost)) + }) + } +} + +// TestHandleImage_HostForms_Served checks that the image route serves, with +// a valid signature, the image at photoPath on a host name, an IPv4 address, +// an IPv6 address in brackets, and a host followed by ":" with or without a +// port. +func TestHandleImage_HostForms_Served(t *testing.T) { + t.Parallel() + + hosts := []string{ + allowlistedHost, + "img_1.example.com", + "203.0.113.5", + "[2001:db8::1]", + allowlistedHost + ":8443", + "[2001:db8::1]:8443", + allowlistedHost + ":", + } + + photos := newPhotoFetcher(t, hosts...) + exp := time.Now().Add(time.Hour) + + for _, host := range hosts { + t.Run(host, func(t *testing.T) { + t.Parallel() + + sig := photoSignature(testSigningKey, host, exp) + + rec := sendGet(t, newImageRoute(t, photos), + photoURLWithSig(host, sig, exp)) + if rec.Code != http.StatusOK { + t.Errorf("status = %d, want %d: %s", + rec.Code, http.StatusOK, rec.Body) + } + }) + } +} -- 2.54.0 From c6ee690e836a71b14b084c5e3ad7e08244e31206 Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 15 Oct 2026 22:30:24 +0000 Subject: [PATCH 2/4] Refuse an image URL host that no URL host can be Only @ was refused in the host of a /v1/image/ URL, so a ? written as %3F gave the host cdn.example.com?x=1, which Go's URL parser reads as cdn.example.com with a query: it passed the allowlist and each text after the ? was cached under keys of its own. The host must now be a host name or an IPv4 address (letters, digits, -, _ and .) or an IPv6 address in brackets, either one optionally followed by : and a port; anything else is refused with 400. An IPv6 address without brackets, which needed a signature, is refused too. Model: opus-5-5 --- README.md | 6 +++++- TODO.md | 11 +++++++++++ internal/imgcache/urlparser.go | 16 ++++++++++++---- 3 files changed, 28 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 4fc97e9..85d08c0 100644 --- a/README.md +++ b/README.md @@ -575,7 +575,11 @@ pixa lower-cases the host of a `/v1/image/` URL before anything else uses it, so `CDN.Example.com` is matched, fetched, cached and counted against `upstream_connections_per_host` as `cdn.example.com`. Such a URL whose host has a port, as in `cdn.example.com:8443`, or a `:` with no port, needs a signature -even when the host is allowlisted. A host holding `@` is refused with 400. +even when the host is allowlisted. The host must be a host name or an IPv4 +address (letters, digits, hyphens, underscores and dots) or an IPv6 address in +brackets, either one optionally followed by `:` and a port. Any other host, such +as one holding `@`, or a `?`, `#`, `%`, `\` or space written percent-encoded, is +refused with 400. ### Configuration diff --git a/TODO.md b/TODO.md index 0ef9f6b..6b6417e 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,17 @@ P2: security: per-origin rate limiting # Completed Steps +- 2026-10-15 the host of a `/v1/image/` URL must be a host name or an IPv4 + address (letters, digits, hyphens, underscores and dots) or an IPv6 address in + brackets, either one optionally followed by `:` and a port, and any other host + is refused with 400 (closes #275): only `@` was refused, so a `?` written as + `%3F` gave the host `cdn.example.com?x=1`, which Go's URL parser reads as + `cdn.example.com` with a query. Such a URL passed the allowlist, and each text + after the `?` fetched the host's root path and cached the answer under keys of + its own. A `#`, `%`, `\` or space written percent-encoded is refused the same + way, and so is an IPv6 address without brackets, which needed a signature + before. Underscores are kept because `allowlist_hosts` accepts host names + holding them. - 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 diff --git a/internal/imgcache/urlparser.go b/internal/imgcache/urlparser.go index 1794d1f..d109025 100644 --- a/internal/imgcache/urlparser.go +++ b/internal/imgcache/urlparser.go @@ -50,6 +50,12 @@ var sizeFormatRegex = regexp.MustCompile( // percentEscapeRegex matches one percent-escape, such as "%2e" var percentEscapeRegex = regexp.MustCompile(`%[0-9A-Fa-f]{2}`) +// hostRegex matches the host of an image URL: a host name or an IPv4 address +// (letters, digits, "-", "_" and "."), or an IPv6 address in brackets, either +// one optionally followed by ":" and a port, which may be empty +var hostRegex = regexp.MustCompile( + `^([A-Za-z0-9_.-]+|\[[0-9A-Fa-f:.]+\])(:[0-9]*)?$`) + // ParsedURL contains the parsed components of an image proxy URL. type ParsedURL struct { // Host is the source origin host (e.g., "cdn.example.com") @@ -141,10 +147,12 @@ func parseImageComponents(remainder string) (*ParsedURL, error) { return nil, ErrMissingHost } - // A URL reads what comes before "@" as a user name and password, and the - // rest as the host - if strings.Contains(host, "@") { - return nil, fmt.Errorf("%w: %q holds @", ErrInvalidHost, host) + // Go's URL parser, which the allowlist check and the fetcher use, reads a + // host that hostRegex does not match as a different host, or refuses it: + // what comes before an "@" is a user name and password, and what comes + // after a "?" or "#" is a query or a fragment + if !hostRegex.MatchString(host) { + return nil, fmt.Errorf("%w: %q", ErrInvalidHost, host) } // Host names do not depend on letter case, so the allowlist, the upstream -- 2.54.0 From 46ee9fa1cac35982f7fb3854913782018eef0411 Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 15 Oct 2026 22:53:42 +0000 Subject: [PATCH 3/4] Test that the image route refuses brackets around no IPv6 address An IPv4 address in brackets, such as [203.0.113.5], and brackets around no address at all, such as [abc], must be refused with 400. They now pass the host check, and Go's URL parser refuses them later, so the route answers 401. The new rows fail until the fix lands. Model: opus-5-5 --- internal/handlers/image_errors_internal_test.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/internal/handlers/image_errors_internal_test.go b/internal/handlers/image_errors_internal_test.go index 27f1d4f..eac170f 100644 --- a/internal/handlers/image_errors_internal_test.go +++ b/internal/handlers/image_errors_internal_test.go @@ -265,12 +265,14 @@ func TestHandleImage_AllowlistOrSignature(t *testing.T) { } } -// TestHandleImage_InvalidHost_Returns400 sends image URLs whose host is -// allowlistedHost with a character after it that a URL host cannot hold, -// percent-encoded, and checks that each is refused with 400 naming the host -// as decoded. A "?" or "#" there would otherwise end the host when the source -// URL is read, so the request would pass the allowlist and be fetched and -// cached under a host that is not the one fetched. +// TestHandleImage_InvalidHost_Returns400 sends image URLs whose host no URL +// host can be, and checks that each is refused with 400 naming the host as +// decoded: allowlistedHost with a character after it that a URL host cannot +// hold, percent-encoded, an IPv6 address without brackets, and brackets +// around something that is not an IPv6 address. A "?" or "#" after +// allowlistedHost would otherwise end the host when the source URL is read, +// so the request would pass the allowlist and be fetched and cached under a +// host that is not the one fetched. func TestHandleImage_InvalidHost_Returns400(t *testing.T) { t.Parallel() @@ -287,6 +289,8 @@ func TestHandleImage_InvalidHost_Returns400(t *testing.T) { {`\ written as %5C`, allowlistedHost + "%5Cx", allowlistedHost + `\x`}, {"space written as %20", allowlistedHost + "%20", allowlistedHost + " "}, {"IPv6 address without brackets", "2001:db8::1", "2001:db8::1"}, + {"IPv4 address in brackets", "[203.0.113.5]", "[203.0.113.5]"}, + {"no address in brackets", "[abc]", "[abc]"}, } for _, tt := range tests { -- 2.54.0 From 42afa539de072aa8d77034fe8e25994ebad354ea Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 15 Oct 2026 22:55:37 +0000 Subject: [PATCH 4/4] Refuse brackets around anything but an IPv6 address in an image URL host The host check accepted any hex digits, : and . in brackets, so a host such as [203.0.113.5] or [abc] got through it, and Go's URL parser then refused the source URL, which the route answered with 401. The text in brackets must now parse as an IPv6 address, as Go's URL parser asks, or the host is refused with 400. The comment above the check now says only what is true of it. Model: opus-5-5 --- internal/imgcache/urlparser.go | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/internal/imgcache/urlparser.go b/internal/imgcache/urlparser.go index d109025..467c8d9 100644 --- a/internal/imgcache/urlparser.go +++ b/internal/imgcache/urlparser.go @@ -3,6 +3,7 @@ package imgcache import ( "errors" "fmt" + "net/netip" "net/url" "regexp" "strconv" @@ -50,11 +51,13 @@ var sizeFormatRegex = regexp.MustCompile( // percentEscapeRegex matches one percent-escape, such as "%2e" var percentEscapeRegex = regexp.MustCompile(`%[0-9A-Fa-f]{2}`) -// hostRegex matches the host of an image URL: a host name or an IPv4 address -// (letters, digits, "-", "_" and "."), or an IPv6 address in brackets, either -// one optionally followed by ":" and a port, which may be empty +// hostRegex matches the forms the host of an image URL may take: a host name +// or an IPv4 address (letters, digits, "-", "_" and "."), or an IPv6 address +// in brackets (hex digits, ":" and "."), either one optionally followed by ":" +// and a port, which may be empty. Its one group holds the text in brackets, +// which parseImageComponents also checks is an IPv6 address. var hostRegex = regexp.MustCompile( - `^([A-Za-z0-9_.-]+|\[[0-9A-Fa-f:.]+\])(:[0-9]*)?$`) + `^(?:[A-Za-z0-9_.-]+|\[([0-9A-Fa-f:.]+)\])(?::[0-9]*)?$`) // ParsedURL contains the parsed components of an image proxy URL. type ParsedURL struct { @@ -147,14 +150,22 @@ func parseImageComponents(remainder string) (*ParsedURL, error) { return nil, ErrMissingHost } - // Go's URL parser, which the allowlist check and the fetcher use, reads a - // host that hostRegex does not match as a different host, or refuses it: - // what comes before an "@" is a user name and password, and what comes - // after a "?" or "#" is a query or a fragment - if !hostRegex.MatchString(host) { + // Only the forms hostRegex describes are accepted. A "?", "#" or "@" in + // the host would make Go's URL parser, which the allowlist check and the + // fetcher use, read a different host from the one allowlisted and cached + hostMatch := hostRegex.FindStringSubmatch(host) + if hostMatch == nil { return nil, fmt.Errorf("%w: %q", ErrInvalidHost, host) } + // Go's URL parser refuses brackets around anything but an IPv6 address + if inBrackets := hostMatch[1]; inBrackets != "" { + addr, err := netip.ParseAddr(inBrackets) + if err != nil || !addr.Is6() { + return nil, fmt.Errorf("%w: %q", ErrInvalidHost, host) + } + } + // Host names do not depend on letter case, so the allowlist, the upstream // connection limits, the cache and the signature see one spelling of each // host -- 2.54.0