diff --git a/README.md b/README.md index 85d08c0..b84f4a1 100644 --- a/README.md +++ b/README.md @@ -323,8 +323,9 @@ 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, +itself or percent-encoded: `/`, `;`, `,`, `=` and `&` work either way. Neither +the source path nor the source query may hold a `#`, and an image URL that +writes one as `%23` is refused with 400. 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`, diff --git a/TODO.md b/TODO.md index 6b6417e..4b27407 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,11 @@ P2: security: per-origin rate limiting # Completed Steps +- 2026-10-15 a `/v1/image/` URL whose source path or source query holds a `#`, + written as `%23`, is refused with 400 (closes #286): the source URL built from + it ended at the `#` when fetched, so each text after it fetched the same + upstream image, without a signature for an allowlisted host, and cached it + under keys of its own. `README.md` says neither may hold a `#`. - 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 diff --git a/internal/handlers/image_errors_internal_test.go b/internal/handlers/image_errors_internal_test.go index eac170f..25fe061 100644 --- a/internal/handlers/image_errors_internal_test.go +++ b/internal/handlers/image_errors_internal_test.go @@ -340,3 +340,51 @@ func TestHandleImage_HostForms_Served(t *testing.T) { }) } } + +// TestHandleImage_HashInSourcePathOrQuery_Returns400 sends image URLs whose +// source path or source query holds a "#", written as %23, and checks that +// each is refused with 400, the one signed for that source path included. The +// source URL would otherwise end at the "#" when it is fetched, so each text +// after it would fetch the image at photoPath and cache it under keys of its +// own. +func TestHandleImage_HashInSourcePathOrQuery_Returns400(t *testing.T) { + t.Parallel() + + photos := newPhotoFetcher(t, allowlistedHost, signedHost) + exp := time.Now().Add(time.Hour) + + sig := signature.New(testSigningKey).Sign(&signature.Request{ + SourceHost: signedHost, + SourcePath: photoPath + "#x", + Width: 50, + Height: 50, + Format: string(imgcache.FormatJPEG), + Quality: 85, + FitMode: string(imgcache.FitCover), + Expires: exp, + }) + + tests := []struct { + name string + target string + }{ + {"# in the source path", + "/v1/image/" + allowlistedHost + photoPath + "%23x/50x50.jpeg"}, + {"# in the source query", + "/v1/image/" + allowlistedHost + photoPath + "%3Fv=1%23x/50x50.jpeg"}, + {"# in the source path, signed", + fmt.Sprintf("/v1/image/%s%s%%23x/50x50.jpeg?sig=%s&exp=%d", + signedHost, photoPath, sig, exp.Unix())}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + rec := sendGet(t, newImageRoute(t, photos), tt.target) + checkErrorBody(t, rec, http.StatusBadRequest, + `invalid image URL: invalid image path: `+ + `the source path or query holds a "#"`) + }) + } +} diff --git a/internal/imgcache/urlparser.go b/internal/imgcache/urlparser.go index 467c8d9..59d95fb 100644 --- a/internal/imgcache/urlparser.go +++ b/internal/imgcache/urlparser.go @@ -171,6 +171,14 @@ func parseImageComponents(remainder string) (*ParsedURL, error) { // host host = strings.ToLower(host) + // Go's URL parser, which the fetcher uses, reads a "#" as the start of a + // fragment and drops the rest, so each text after it would be another + // spelling, cached under its own keys, of one upstream request + if strings.Contains(path, "#") { + return nil, fmt.Errorf( + `%w: the source path or query holds a "#"`, ErrInvalidPath) + } + // Extract query string if present in path if qIndex := strings.Index(path, "?"); qIndex != -1 { query = path[qIndex+1:]