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/handlers/image_errors_internal_test.go b/internal/handlers/image_errors_internal_test.go index fbbcf6f..eac170f 100644 --- a/internal/handlers/image_errors_internal_test.go +++ b/internal/handlers/image_errors_internal_test.go @@ -264,3 +264,79 @@ func TestHandleImage_AllowlistOrSignature(t *testing.T) { }) } } + +// 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() + + 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"}, + {"IPv4 address in brackets", "[203.0.113.5]", "[203.0.113.5]"}, + {"no address in brackets", "[abc]", "[abc]"}, + } + + 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) + } + }) + } +} diff --git a/internal/imgcache/urlparser.go b/internal/imgcache/urlparser.go index 1794d1f..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,6 +51,14 @@ var sizeFormatRegex = regexp.MustCompile( // percentEscapeRegex matches one percent-escape, such as "%2e" var percentEscapeRegex = regexp.MustCompile(`%[0-9A-Fa-f]{2}`) +// 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]*)?$`) + // 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 +150,20 @@ 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) + // 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