From af5ce499b9cb4515fffdd17aef29a17c04966fc6 Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 16 Oct 2026 00:03:39 +0000 Subject: [PATCH 1/4] Test that the source URL is fetched with its path and query encoded Through the router, against a test upstream that records each request it receives: a space in the source query, 100%25.png, the source paths written my%20cat.png and my%2520cat.png, one source query written two ways, and the generator page with %20, %25 and %23 in the source URL, and with %2F in its path, which the page must refuse. These fail on next; the fix for https://git.eeqj.de/sneak/pixa/issues/288 follows. Model: opus-5-5 --- internal/server/source_url_internal_test.go | 334 ++++++++++++++++++++ 1 file changed, 334 insertions(+) create mode 100644 internal/server/source_url_internal_test.go diff --git a/internal/server/source_url_internal_test.go b/internal/server/source_url_internal_test.go new file mode 100644 index 0000000..06858b1 --- /dev/null +++ b/internal/server/source_url_internal_test.go @@ -0,0 +1,334 @@ +package server + +import ( + "net/http" + "net/http/httptest" + "net/url" + "regexp" + "slices" + "strings" + "sync" + "testing" + + "sneak.berlin/go/pixa/internal/imgcache" + "sneak.berlin/go/pixa/internal/session" +) + +// percentSignPath is a source path holding a "%", which a URL writes %25. +const percentSignPath = "/100%25.png" + +// generateFormPattern extracts the CSRF token rendered into the generator +// page's form. +var generateFormPattern = regexp.MustCompile( + `(?s)action="/generate".*?name="gorilla\.csrf\.Token" value="([^"]+)"`) + +// generatedURLPattern extracts the path of the encrypted URL the generator +// page shows. +var generatedURLPattern = regexp.MustCompile(`value="[^"]*(/v1/e/[^"]+)"`) + +// recordingUpstream is a test upstream that answers every request with a PNG +// and records the request target, the path and query as sent, of each request +// it receives. +type recordingUpstream struct { + *httptest.Server + + mu sync.Mutex + received []string +} + +// startRecordingUpstream starts a recordingUpstream, which is closed when the +// test ends. +func startRecordingUpstream(t *testing.T) *recordingUpstream { + t.Helper() + + source := encodeTestPNG(t, 64, 48) + upstream := &recordingUpstream{} + + upstream.Server = httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + upstream.mu.Lock() + upstream.received = append(upstream.received, r.RequestURI) + upstream.mu.Unlock() + + w.Header().Set("Content-Type", "image/png") + _, _ = w.Write(source) + })) + t.Cleanup(upstream.Close) + + return upstream +} + +// requestTargets returns the request targets the upstream has received, in +// the order it received them. +func (u *recordingUpstream) requestTargets() []string { + u.mu.Lock() + defer u.mu.Unlock() + + return slices.Clone(u.received) +} + +// pageOrigin is the origin of the login and generator pages below, opened +// over HTTPS; httptest gives every request the host example.com. +const pageOrigin = "https://example.com" + +// openRoot sends GET / to s over HTTPS, with cookie when it is not nil, and +// returns the page: the login page, or the generator page for a login session. +func openRoot( + t *testing.T, s *Server, cookie *http.Cookie, +) *httptest.ResponseRecorder { + t.Helper() + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, + pageOrigin+"/", nil) + if cookie != nil { + req.AddCookie(cookie) + } + + page := httptest.NewRecorder() + s.ServeHTTP(page, req) + + return page +} + +// submitForm posts form to path on s as a browser submits it from page over +// HTTPS: with the CSRF token tokenPattern finds in page, the cookies page set, +// cookie when it is not nil, and a Referer naming the page, which pixa needs +// while debug is off, as it is in startImageProxy's config. +func submitForm( + t *testing.T, s *Server, page *httptest.ResponseRecorder, + tokenPattern *regexp.Regexp, path string, form url.Values, + cookie *http.Cookie, +) *httptest.ResponseRecorder { + t.Helper() + + match := tokenPattern.FindStringSubmatch(page.Body.String()) + if match == nil { + t.Fatalf("the page has no form for %s with a CSRF token", path) + } + + form.Set("gorilla.csrf.Token", match[1]) + + req := httptest.NewRequestWithContext(t.Context(), http.MethodPost, + pageOrigin+path, strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.Header.Set("Referer", pageOrigin+"/") + + for _, c := range page.Result().Cookies() { + req.AddCookie(c) + } + + if cookie != nil { + req.AddCookie(cookie) + } + + rec := httptest.NewRecorder() + s.ServeHTTP(rec, req) + + return rec +} + +// postGenerate logs in to s with the signing key, submits the generator page's +// form with sourceURL, and returns the response. +func postGenerate( + t *testing.T, s *Server, sourceURL string, +) *httptest.ResponseRecorder { + t.Helper() + + login := submitForm(t, s, openRoot(t, s, nil), csrfFieldPattern, "/", + url.Values{"key": {testSigningKey}}, nil) + + var sessionCookie *http.Cookie + + for _, c := range login.Result().Cookies() { + if c.Name == session.CookieName { + sessionCookie = c + } + } + + if sessionCookie == nil { + t.Fatalf("logging in answered %d and set no session cookie", login.Code) + } + + rec := submitForm(t, s, openRoot(t, s, sessionCookie), generateFormPattern, + "/generate", url.Values{"url": {sourceURL}}, sessionCookie) + t.Logf("POST /generate with url %s: %d", sourceURL, rec.Code) + + return rec +} + +// TestImageURLFetchesItsSourcePathAndQuery requests image URLs whose source +// query holds a space and whose source path holds a "%", each written +// percent-encoded, and checks the one request the upstream receives: the +// source path percent-encoded again, and the source query with the space +// written %20. +func TestImageURLFetchesItsSourcePathAndQuery(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + // pathAndQuery is the source path and query as the image URL writes + // them + pathAndQuery string + // wantRequest is the path and query the upstream must receive + wantRequest string + }{ + {"space in the query", "/photo.png%3Fname=my%20cat", + "/photo.png?name=my%20cat"}, + {"percent sign in the path", percentSignPath, percentSignPath}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + upstream := startRecordingUpstream(t) + s, _, _ := startImageProxy(t, upstream.Server) + + getImage(t, s, "/v1/image/"+upstreamHost+tt.pathAndQuery+"/orig.png") + + got := upstream.requestTargets() + t.Logf("upstream received %q", got) + + if !slices.Equal(got, []string{tt.wantRequest}) { + t.Errorf("upstream received %q, want [%q]", got, tt.wantRequest) + } + }) + } +} + +// TestSourcePathsHoldingSpaceAndPercent20AreFetchedApart requests twice each +// of the source paths /my cat.png and /my%20cat.png, which the image URL +// writes my%20cat.png and my%2520cat.png. Each is fetched once, as a request +// of its own, and its second request is served from the cache. +func TestSourcePathsHoldingSpaceAndPercent20AreFetchedApart(t *testing.T) { + t.Parallel() + + upstream := startRecordingUpstream(t) + s, _, _ := startImageProxy(t, upstream.Server) + + imageURLs := []string{ + "/v1/image/" + upstreamHost + "/my%20cat.png/orig.png", + "/v1/image/" + upstreamHost + "/my%2520cat.png/orig.png", + } + + for _, imageURL := range imageURLs { + getImage(t, s, imageURL) + } + + for _, imageURL := range imageURLs { + cacheStatus := getImage(t, s, imageURL).Header().Get("X-Pixa-Cache") + if cacheStatus != string(imgcache.CacheHit) { + t.Errorf("second GET %s: X-Pixa-Cache = %q, want HIT", + imageURL, cacheStatus) + } + } + + got := upstream.requestTargets() + t.Logf("upstream received %q", got) + + want := []string{"/my%20cat.png", "/my%2520cat.png"} + if !slices.Equal(got, want) { + t.Errorf("upstream received %q, want %q", got, want) + } +} + +// TestSourceQueryWrittenTwoWaysIsOneUpstreamRequest requests the source query +// name=my%20cat written as the README says, name=my%2520cat, and with the +// space written %20, name=my%20cat. Both are one source query: the upstream +// receives one request, and the second image URL is served from the cache. +func TestSourceQueryWrittenTwoWaysIsOneUpstreamRequest(t *testing.T) { + t.Parallel() + + upstream := startRecordingUpstream(t) + s, _, _ := startImageProxy(t, upstream.Server) + + getImage(t, s, "/v1/image/"+upstreamHost+"/photo.png%3Fname=my%2520cat/orig.png") + + second := getImage(t, s, + "/v1/image/"+upstreamHost+"/photo.png%3Fname=my%20cat/orig.png") + if got := second.Header().Get("X-Pixa-Cache"); got != string(imgcache.CacheHit) { + t.Errorf("second X-Pixa-Cache = %q, want HIT", got) + } + + got := upstream.requestTargets() + t.Logf("upstream received %q", got) + + want := []string{"/photo.png?name=my%20cat"} + if !slices.Equal(got, want) { + t.Errorf("upstream received %q, want %q", got, want) + } +} + +// TestGeneratedURLFetchesTheSourceURLAsEntered makes an encrypted URL on the +// generator page for source URLs holding %20, %25 and %23 in the path or the +// query, requests it, and checks that the upstream receives the path and +// query exactly as entered. +func TestGeneratedURLFetchesTheSourceURLAsEntered(t *testing.T) { + t.Parallel() + + // The names hold no "%": each test's state directory is named after it + // and goes into the database URL, which decodes percent-escapes + tests := []struct { + name string + pathAndQuery string + }{ + {"space in the path", "/my%20cat.png"}, + {"percent sign in the path", percentSignPath}, + {"number sign in the path", "/a%23b.png"}, + {"all three in the query", "/photo.png?name=my%20cat%25%23"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + upstream := startRecordingUpstream(t) + s, _, _ := startImageProxy(t, upstream.Server) + + rec := postGenerate(t, s, "https://"+upstreamHost+tt.pathAndQuery) + + match := generatedURLPattern.FindStringSubmatch(rec.Body.String()) + if rec.Code != http.StatusOK || match == nil { + t.Fatalf("POST /generate: %d with no encrypted URL; body %s", + rec.Code, rec.Body.String()) + } + + getImage(t, s, match[1]) + + got := upstream.requestTargets() + t.Logf("upstream received %q", got) + + if !slices.Equal(got, []string{tt.pathAndQuery}) { + t.Errorf("upstream received %q, want [%q]", got, tt.pathAndQuery) + } + }) + } +} + +// TestGeneratorRefusesSourceURLItWouldFetchAnotherWay submits a source URL +// whose path holds %2F. pixa keeps the path decoded and would fetch it with a +// "/" there, which is another URL, so the generator page answers 400, shows +// the URL pixa would fetch, and makes no encrypted URL. +func TestGeneratorRefusesSourceURLItWouldFetchAnotherWay(t *testing.T) { + t.Parallel() + + s, _, _ := startImageProxy(t, startRecordingUpstream(t).Server) + + rec := postGenerate(t, s, "https://"+upstreamHost+"/a%2Fb.png") + body := rec.Body.String() + + if rec.Code != http.StatusBadRequest { + t.Errorf("status = %d, want %d", rec.Code, http.StatusBadRequest) + } + + if strings.Contains(body, "/v1/e/") { + t.Error("the page shows an encrypted URL") + } + + // The test config sets allow_http, so pixa would fetch over plain HTTP + fetchURL := "http://" + upstreamHost + "/a/b.png" + if !strings.Contains(body, fetchURL) { + t.Errorf("the page does not show the URL pixa would fetch, %s; body %s", + fetchURL, body) + } +} -- 2.54.0 From 545507e9d4a5699260a1cac3c71fdb9c10feb527 Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 16 Oct 2026 00:07:08 +0000 Subject: [PATCH 2/4] Fetch the source path and query URL-encoded, not joined as text (closes #288) The source path, decoded, is fetched percent-encoded again by net/url, so 100%25.png fetches /100%25.png, and my%20cat.png and my%2520cat.png fetch different paths. The source query stays as the source URL writes it; the characters a browser percent-encodes in a query, such as a space, are percent-encoded when the image URL is read, so the query is signed and cached as it is sent. The generator page answers 400 for a source URL its image URL would fetch written another way, such as with %2F in the path, and shows the URL pixa would fetch. README.md says how the source path and query are fetched. Model: opus-5-5 --- README.md | 32 ++++++++++++++++++++++++++------ TODO.md | 11 +++++++++++ internal/handlers/auth.go | 16 ++++++++++++++++ internal/imgcache/imgcache.go | 24 ++++++++++++++++-------- internal/imgcache/urlparser.go | 27 +++++++++++++++++++++++++-- 5 files changed, 94 insertions(+), 16 deletions(-) diff --git a/README.md b/README.md index b84f4a1..d6f3a3a 100644 --- a/README.md +++ b/README.md @@ -331,6 +331,20 @@ writes one as `%23` is refused with 400. For example, `/v1/image/cdn.example.com/img%3Furl=https://origin.example.com/my%2520cat.jpg/800x600.webp`, signed as shown under Signature Specification. +The source path is the path with its percent-escapes decoded, and pixa fetches +it percent-encoded again: every character other than letters, digits, `-`, `.`, +`_`, `~`, `/`, `$`, `&`, `+`, `,`, `:`, `;`, `=` and `@` is sent as a +percent-escape. So `my%20cat.jpg` in the image URL is the source path +`/my cat.jpg`, fetched as `/my%20cat.jpg`; `my%2520cat.jpg` is the source path +`/my%20cat.jpg`, fetched as `/my%2520cat.jpg`; and `100%25.png` is fetched as +`/100%25.png`. The source query is the source URL's query as that URL writes it, +its percent-escapes included, and is sent as it is, except that the characters a +browser percent-encodes in a query (a space, `"`, `<`, `>`, control characters +and characters outside ASCII) and a `%` that does not begin a percent-escape are +percent-encoded. pixa signs and caches the query in that form, so +`img%3Fname=my%20cat` and `img%3Fname=my%2520cat` in the image URL are the same +source query, `name=my%20cat`. + 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 @@ -445,7 +459,11 @@ nor change what it asks for. sent with an empty format, or none, also makes a JPEG XL URL. Width and height both empty or `0` keep the original size; if only one of them is empty or `0`, that side is scaled to keep the image's proportions. A URL whose - scaled side would come out above 8192 answers 400. + scaled side would come out above 8192 answers 400. So does a source URL that + the image URL would fetch written another way, such as one whose path holds + `%2F`, fetched as `/`, or `(`, fetched as `%28`, as pixa keeps the source + path decoded and fetches it percent-encoded again (see Routes); the page then + shows the URL pixa would fetch. 3. The page shows the URL, `https:///v1/e//img.`, and when it expires. `` is the host the page was opened on, and the URL starts with `http` instead while `debug` is on. The name after the token is ignored @@ -507,11 +525,13 @@ Where: - `host` — source host as the URL writes it up to the first `/`, in lower case, keeping an IPv6 address's brackets and any `:` and port after it (e.g. `cdn.example.com`, `cdn.example.com:8443`, `[2001:db8::1]:8443`) -- `path` — source path (e.g. `/photos/cat.jpg`), with each `%` written as `%25` - and each `:` as `%3A`, so `/photos/a:b.jpg` is signed as `/photos/a%3Ab.jpg` -- `query` — source query string, empty string if none, with `%` and `:` written - as in `path`; neither then holds a `:`, so path `/a:b` with no query and path - `/a` with query `b:` are signed differently +- `path` — source path, with its percent-escapes decoded as under Routes (e.g. + `/photos/cat.jpg`, or `/photos/my cat.jpg` for `/photos/my%20cat.jpg`), with + each `%` written as `%25` and each `:` as `%3A`, so `/photos/a:b.jpg` is + signed as `/photos/a%3Ab.jpg` +- `query` — source query string as under Routes, empty string if none, with `%` + and `:` written as in `path`; neither then holds a `:`, so path `/a:b` with no + query and path `/a` with query `b:` are signed differently - `width` — requested width in pixels, `0` for original - `height` — requested height in pixels, `0` for original - `format` — output format, one of those listed under Routes, with `original` diff --git a/TODO.md b/TODO.md index cc2a818..79c5fdb 100644 --- a/TODO.md +++ b/TODO.md @@ -42,6 +42,17 @@ P2: security: per-origin rate limiting resident memory peaked at about 242 MiB for the 8192x64 source and 431 MiB for the 8192x1 one. The output can sit a fraction of an output pixel from where it was. +- 2026-10-16 the source URL a request fetches is built with `net/url` from the + source host, path and query, not joined as text (closes #288). The source + path, decoded, is fetched percent-encoded again, so `100%25.png` fetches + `/100%25.png` where it answered 401, and `my%20cat.png` and `my%2520cat.png` + fetch different paths where both fetched `/my%20cat.png` under two cache keys. + The source query stays as the source URL writes it; the characters a browser + percent-encodes in a query, such as a space, which was sent raw and answered + 502, are percent-encoded when the image URL is read, so the query is signed + and cached as it is sent. The generator page answers 400, showing the URL pixa + would fetch, for a source URL its image URL would fetch written another way, + such as with `%2F` in the path. - 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 diff --git a/internal/handlers/auth.go b/internal/handlers/auth.go index dff2763..173f2c7 100644 --- a/internal/handlers/auth.go +++ b/internal/handlers/auth.go @@ -141,6 +141,22 @@ func (s *Handlers) HandleGenerateURL() http.HandlerFunc { return } + // The image URL must fetch the source URL as it was written. The + // token keeps the path decoded and pixa fetches it percent-encoded + // again, so a path written another way, such as with %2F, would fetch + // another URL. The scheme is not kept: only path and query are compared + sourceRequest := payload.ToImageRequest() + sourceRequest.AllowHTTP = s.config.AllowHTTP + + fetchURL, err := url.Parse(sourceRequest.SourceURL()) + if err != nil || fetchURL.RequestURI() != parsed.RequestURI() { + s.renderGeneratorWithForm(w, r, "Source URL cannot be fetched as "+ + "written: pixa would fetch "+sourceRequest.SourceURL(), + r.Form, http.StatusBadRequest) + + return + } + // Generate encrypted token token, err := s.encGen.Generate(payload) if err != nil { diff --git a/internal/imgcache/imgcache.go b/internal/imgcache/imgcache.go index 47acca6..51b33b3 100644 --- a/internal/imgcache/imgcache.go +++ b/internal/imgcache/imgcache.go @@ -5,6 +5,7 @@ import ( "context" "errors" "io" + "net/url" "time" ) @@ -86,9 +87,11 @@ func ValidateImageRequest(req *ImageRequest) error { type ImageRequest struct { // SourceHost is the origin host (e.g., "cdn.example.com") SourceHost string - // SourcePath is the path on the origin (e.g., "/photos/cat.jpg") + // SourcePath is the path on the origin with its percent-escapes decoded, + // as url.URL's Path holds it (e.g., "/photos/my cat.jpg") SourcePath string - // SourceQuery is the optional query string for the origin URL + // SourceQuery is the optional query string for the origin URL as the URL + // writes it, percent-escapes included, as url.URL's RawQuery holds it SourceQuery string // Size is the requested output dimensions Size Size @@ -107,20 +110,25 @@ type ImageRequest struct { AllowHTTP bool } -// SourceURL returns the full upstream URL to fetch. -// Uses http:// scheme when AllowHTTP is true, otherwise https://. +// SourceURL returns the full upstream URL to fetch: the source path +// percent-encoded again, as net/url writes a path, so each source path is one +// upstream path, and the source query as it is, with the characters +// escapeQuery encodes percent-encoded. Uses http:// scheme when AllowHTTP is +// true, otherwise https://. func (r *ImageRequest) SourceURL() string { scheme := "https" if r.AllowHTTP { scheme = "http" } - url := scheme + "://" + r.SourceHost + r.SourcePath - if r.SourceQuery != "" { - url += "?" + r.SourceQuery + sourceURL := url.URL{ + Scheme: scheme, + Host: r.SourceHost, + Path: r.SourcePath, + RawQuery: escapeQuery(r.SourceQuery), } - return url + return sourceURL.String() } // ImageResponse represents a processed image ready to serve diff --git a/internal/imgcache/urlparser.go b/internal/imgcache/urlparser.go index 59d95fb..edcd344 100644 --- a/internal/imgcache/urlparser.go +++ b/internal/imgcache/urlparser.go @@ -51,6 +51,12 @@ var sizeFormatRegex = regexp.MustCompile( // percentEscapeRegex matches one percent-escape, such as "%2e" var percentEscapeRegex = regexp.MustCompile(`%[0-9A-Fa-f]{2}`) +// queryEscapeRegex matches a percent-escape, or one character that a browser +// percent-encodes in a URL's query: one outside the printable ASCII characters +// "!" to "~", such as a space, a control character or a character outside +// ASCII, or one of `"#<>`. A "%" that does not begin a percent-escape matches too. +var queryEscapeRegex = 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 ":" @@ -179,9 +185,12 @@ func parseImageComponents(remainder string) (*ParsedURL, error) { `%w: the source path or query holds a "#"`, ErrInvalidPath) } - // Extract query string if present in path + // Extract query string if present in path. It is the source URL's query + // as that URL writes it. A character a URL's query cannot hold as it is, + // such as a space written %20 in the image URL, is percent-encoded here, + // so the query is signed and cached as it is sent upstream if qIndex := strings.Index(path, "?"); qIndex != -1 { - query = path[qIndex+1:] + query = escapeQuery(path[qIndex+1:]) path = path[:qIndex] } @@ -270,6 +279,20 @@ func decodePercentEscapes(s string) string { }) } +// escapeQuery returns a source query as a URL's query holds it: each +// character that queryEscapeRegex names, such as a space, percent-encoded, +// and the rest, the query's percent-escapes included, kept as they are. The +// result is unchanged by a second escapeQuery. +func escapeQuery(query string) string { + return queryEscapeRegex.ReplaceAllStringFunc(query, func(match string) string { + if percentEscapeRegex.MatchString(match) { + return match + } + + return url.PathEscape(match) + }) +} + // parseSizeFormat parses strings like "800x600.webp", "orig.png" or "800x600" func parseSizeFormat(s string) (Size, ImageFormat, error) { matches := sizeFormatRegex.FindStringSubmatch(s) -- 2.54.0 From 5bfec71b611803c74d4f6ea2a8cae5114de1be27 Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 16 Oct 2026 00:56:05 +0000 Subject: [PATCH 3/4] Test the generator refusals and a "'" in the source query Through the router: a source query holding "'" must reach the upstream as %27. The generator page must answer 400, show the URL pixa would fetch and make no encrypted URL for a source path holding %2F next to a space, a "|" or a letter outside ASCII, and for a source URL holding a user name and password. It must also refuse a source path holding a ".." segment, as /v1/image/ does. These fail on the branch; the fix follows. Model: opus-5-5 --- internal/server/source_url_internal_test.go | 89 +++++++++++++++------ 1 file changed, 63 insertions(+), 26 deletions(-) diff --git a/internal/server/source_url_internal_test.go b/internal/server/source_url_internal_test.go index 06858b1..beaf1d5 100644 --- a/internal/server/source_url_internal_test.go +++ b/internal/server/source_url_internal_test.go @@ -157,10 +157,11 @@ func postGenerate( } // TestImageURLFetchesItsSourcePathAndQuery requests image URLs whose source -// query holds a space and whose source path holds a "%", each written -// percent-encoded, and checks the one request the upstream receives: the -// source path percent-encoded again, and the source query with the space -// written %20. +// query holds a space, written percent-encoded, or a "'", and whose source +// path holds a "%", written percent-encoded, and checks the one request the +// upstream receives: the source path percent-encoded again, and the source +// query with the space written %20 and the "'" written %27, as a browser +// writes them in a query. func TestImageURLFetchesItsSourcePathAndQuery(t *testing.T) { t.Parallel() @@ -174,6 +175,8 @@ func TestImageURLFetchesItsSourcePathAndQuery(t *testing.T) { }{ {"space in the query", "/photo.png%3Fname=my%20cat", "/photo.png?name=my%20cat"}, + {"apostrophe in the query", "/photo.png%3Fname=it's", + "/photo.png?name=it%27s"}, {"percent sign in the path", percentSignPath, percentSignPath}, } @@ -305,30 +308,64 @@ func TestGeneratedURLFetchesTheSourceURLAsEntered(t *testing.T) { } } -// TestGeneratorRefusesSourceURLItWouldFetchAnotherWay submits a source URL -// whose path holds %2F. pixa keeps the path decoded and would fetch it with a -// "/" there, which is another URL, so the generator page answers 400, shows -// the URL pixa would fetch, and makes no encrypted URL. -func TestGeneratorRefusesSourceURLItWouldFetchAnotherWay(t *testing.T) { +// TestGeneratorRefusesSourceURLItCannotFetchAsWritten submits source URLs the +// generator page must make no encrypted URL for, and checks that it answers +// 400 and shows why. pixa keeps the source path decoded and fetches it +// percent-encoded again, so a path holding %2F would be fetched with a "/" +// there, whatever else the path holds. pixa sends no user name or password, +// and no "?" with no query after it. For these the page shows the URL pixa +// would fetch. A source path holding a ".." segment is refused, as +// /v1/image/ refuses it. +func TestGeneratorRefusesSourceURLItCannotFetchAsWritten(t *testing.T) { t.Parallel() - s, _, _ := startImageProxy(t, startRecordingUpstream(t).Server) - - rec := postGenerate(t, s, "https://"+upstreamHost+"/a%2Fb.png") - body := rec.Body.String() - - if rec.Code != http.StatusBadRequest { - t.Errorf("status = %d, want %d", rec.Code, http.StatusBadRequest) - } - - if strings.Contains(body, "/v1/e/") { - t.Error("the page shows an encrypted URL") - } - // The test config sets allow_http, so pixa would fetch over plain HTTP - fetchURL := "http://" + upstreamHost + "/a/b.png" - if !strings.Contains(body, fetchURL) { - t.Errorf("the page does not show the URL pixa would fetch, %s; body %s", - fetchURL, body) + const fetchedFrom = "http://" + upstreamHost + + tests := []struct { + name string + // sourceURL is the source URL after "https://" + sourceURL string + // wantShown is what the page must show: the URL pixa would fetch, or + // why it makes no encrypted URL + wantShown string + }{ + {"encoded slash", upstreamHost + "/a%2Fb.png", + fetchedFrom + "/a/b.png"}, + {"encoded slash and a space", upstreamHost + "/a%2Fb c.png", + fetchedFrom + "/a/b%20c.png"}, + {"encoded slash and a bar", upstreamHost + "/a%2Fb|c.png", + fetchedFrom + "/a/b%7Cc.png"}, + {"encoded slash and a letter outside ASCII", upstreamHost + "/café%2Fx.png", + fetchedFrom + "/caf%C3%A9/x.png"}, + {"user name and password", "user:pass@" + upstreamHost + "/a.png", + fetchedFrom + "/a.png"}, + {"question mark with no query", upstreamHost + "/a.png?", + fetchedFrom + "/a.png"}, + {"two-dot segment", upstreamHost + "/a/../b.png", + "cannot hold a .. segment"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + s, _, _ := startImageProxy(t, startRecordingUpstream(t).Server) + + rec := postGenerate(t, s, "https://"+tt.sourceURL) + body := rec.Body.String() + + if rec.Code != http.StatusBadRequest { + t.Errorf("status = %d, want %d", rec.Code, http.StatusBadRequest) + } + + if strings.Contains(body, "/v1/e/") { + t.Error("the page shows an encrypted URL") + } + + if !strings.Contains(body, tt.wantShown) { + t.Errorf("the page does not show %s; body %s", tt.wantShown, body) + } + }) } } -- 2.54.0 From 729c0ff222cb460487fb0652e3b7c0ccfad78eb1 Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 16 Oct 2026 01:02:08 +0000 Subject: [PATCH 4/4] Encode ' in the source query; the generator compares the path as written A "'" in the source query is percent-encoded with the other characters a browser encodes in a query. The generator page compares the URL pixa would fetch with the path as written, which url.URL keeps in RawPath, so a path holding %2F is refused whatever else it holds. It also refuses a source URL holding a user name and password, which pixa does not send, and a source path holding a ".." segment, as /v1/image/ does. README.md and TODO.md say so. Model: opus-5-5 --- README.md | 22 ++++++++-------- TODO.md | 24 ++++++++++-------- internal/handlers/auth.go | 46 ++++++++++++++++++++++++++++------ internal/imgcache/urlparser.go | 9 ++++--- 4 files changed, 69 insertions(+), 32 deletions(-) diff --git a/README.md b/README.md index d6f3a3a..a8a9242 100644 --- a/README.md +++ b/README.md @@ -339,11 +339,11 @@ percent-escape. So `my%20cat.jpg` in the image URL is the source path `/my%20cat.jpg`, fetched as `/my%2520cat.jpg`; and `100%25.png` is fetched as `/100%25.png`. The source query is the source URL's query as that URL writes it, its percent-escapes included, and is sent as it is, except that the characters a -browser percent-encodes in a query (a space, `"`, `<`, `>`, control characters -and characters outside ASCII) and a `%` that does not begin a percent-escape are -percent-encoded. pixa signs and caches the query in that form, so -`img%3Fname=my%20cat` and `img%3Fname=my%2520cat` in the image URL are the same -source query, `name=my%20cat`. +browser percent-encodes in a query (a space, `"`, `'`, `<`, `>`, control +characters and characters outside ASCII) and a `%` that does not begin a +percent-escape are percent-encoded. pixa signs and caches the query in that +form, so `img%3Fname=my%20cat` and `img%3Fname=my%2520cat` in the image URL are +the same source query, `name=my%20cat`. 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 @@ -459,11 +459,13 @@ nor change what it asks for. sent with an empty format, or none, also makes a JPEG XL URL. Width and height both empty or `0` keep the original size; if only one of them is empty or `0`, that side is scaled to keep the image's proportions. A URL whose - scaled side would come out above 8192 answers 400. So does a source URL that - the image URL would fetch written another way, such as one whose path holds - `%2F`, fetched as `/`, or `(`, fetched as `%28`, as pixa keeps the source - path decoded and fetches it percent-encoded again (see Routes); the page then - shows the URL pixa would fetch. + scaled side would come out above 8192 answers 400, and so does a source URL + whose path holds a `..` segment. So does a source URL that the image URL + would fetch written another way, and the page then shows the URL pixa would + fetch: one with a user name and password, which pixa does not send, or one + whose path holds `%2F`, fetched as `/`, or `(`, fetched as `%28`, as pixa + keeps the source path decoded and fetches it percent-encoded again (see + Routes). 3. The page shows the URL, `https:///v1/e//img.`, and when it expires. `` is the host the page was opened on, and the URL starts with `http` instead while `debug` is on. The name after the token is ignored diff --git a/TODO.md b/TODO.md index 79c5fdb..4c56de7 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,19 @@ P2: security: per-origin rate limiting # Completed Steps +- 2026-10-16 the source URL a request fetches is built with `net/url` from the + source host, path and query, not joined as text (closes #288). The source + path, decoded, is fetched percent-encoded again, so `100%25.png` fetches + `/100%25.png` where it answered 401, and `my%20cat.png` and `my%2520cat.png` + fetch different paths where both fetched `/my%20cat.png` under two cache keys. + The source query stays as the source URL writes it; the characters a browser + percent-encodes in a query, such as `'` or a space, which was sent raw and + answered 502, are percent-encoded when the image URL is read, so the query is + signed and cached as it is sent. The generator page answers 400, showing the + URL pixa would fetch, for a source URL its image URL would fetch written + another way, such as with `%2F` in the path, whatever else the path holds, or + with a user name and password. It also answers 400 for a source path holding a + `..` segment, as `/v1/image/` refuses one. - 2026-10-15 a thin source requested with `fit=cover` no longer makes libvips build a huge image (closes #282): libvips' thumbnail scaled the whole source to cover the requested size and copied that into memory before it cut out the @@ -42,17 +55,6 @@ P2: security: per-origin rate limiting resident memory peaked at about 242 MiB for the 8192x64 source and 431 MiB for the 8192x1 one. The output can sit a fraction of an output pixel from where it was. -- 2026-10-16 the source URL a request fetches is built with `net/url` from the - source host, path and query, not joined as text (closes #288). The source - path, decoded, is fetched percent-encoded again, so `100%25.png` fetches - `/100%25.png` where it answered 401, and `my%20cat.png` and `my%2520cat.png` - fetch different paths where both fetched `/my%20cat.png` under two cache keys. - The source query stays as the source URL writes it; the characters a browser - percent-encodes in a query, such as a space, which was sent raw and answered - 502, are percent-encoded when the image URL is read, so the query is signed - and cached as it is sent. The generator page answers 400, showing the URL pixa - would fetch, for a source URL its image URL would fetch written another way, - such as with `%2F` in the path. - 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 diff --git a/internal/handlers/auth.go b/internal/handlers/auth.go index 173f2c7..d51b569 100644 --- a/internal/handlers/auth.go +++ b/internal/handlers/auth.go @@ -9,7 +9,9 @@ import ( "math" "net/http" "net/url" + "slices" "strconv" + "strings" "time" "sneak.berlin/go/pixa/internal/clientip" @@ -133,6 +135,16 @@ func (s *Handlers) HandleGenerateURL() http.HandlerFunc { return } + // /v1/image/ refuses a ".." segment in the source path, and so does + // the generator. The path is decoded, so %2e%2e counts too + if slices.Contains(strings.Split(parsed.Path, "/"), "..") { + s.renderGeneratorWithForm(w, r, + "Source URL path cannot hold a .. segment", r.Form, + http.StatusBadRequest) + + return + } + payload, expiresAt, ttl, err := buildGeneratePayload(parsed, r.Form) if err != nil { s.renderGeneratorWithForm(w, r, err.Error(), r.Form, @@ -141,17 +153,14 @@ func (s *Handlers) HandleGenerateURL() http.HandlerFunc { return } - // The image URL must fetch the source URL as it was written. The - // token keeps the path decoded and pixa fetches it percent-encoded - // again, so a path written another way, such as with %2F, would fetch - // another URL. The scheme is not kept: only path and query are compared + // The image URL must fetch the source URL as it was written sourceRequest := payload.ToImageRequest() sourceRequest.AllowHTTP = s.config.AllowHTTP + fetchURL := sourceRequest.SourceURL() - fetchURL, err := url.Parse(sourceRequest.SourceURL()) - if err != nil || fetchURL.RequestURI() != parsed.RequestURI() { + if !fetchedAsWritten(parsed, fetchURL) { s.renderGeneratorWithForm(w, r, "Source URL cannot be fetched as "+ - "written: pixa would fetch "+sourceRequest.SourceURL(), + "written: pixa would fetch "+fetchURL, r.Form, http.StatusBadRequest) return @@ -189,6 +198,29 @@ func (s *Handlers) HandleGenerateURL() http.HandlerFunc { } } +// fetchedAsWritten reports whether fetchURL, the URL pixa fetches for an +// image URL made from the source URL source, is source as it was written, +// apart from the scheme, which the token does not keep. It is not when source +// holds a user name and password, which the token does not keep either, or a +// "?" with no query after it. The token keeps the source path decoded and +// pixa fetches it percent-encoded again, so a path written another way, such +// as with %2F, is fetched as another path. url.URL keeps the path as written +// in RawPath when it is not the decoded path percent-encoded again. +// EscapedPath ignores RawPath when it also holds a character net/url encodes, +// such as a space, so RawPath is compared itself. +func fetchedAsWritten(source *url.URL, fetchURL string) bool { + writtenPath := source.RawPath + if writtenPath == "" { + writtenPath = source.EscapedPath() + } + + fetched, err := url.Parse(fetchURL) + + return err == nil && source.User == nil && !source.ForceQuery && + fetched.EscapedPath() == writtenPath && + fetched.RawQuery == source.RawQuery +} + // buildGeneratePayload parses the numeric form fields and assembles the // encrypted URL payload. ttl=0 means never expires (ExpiresAt stays 0). A // non-numeric or out-of-range width, height, quality or ttl, or an diff --git a/internal/imgcache/urlparser.go b/internal/imgcache/urlparser.go index edcd344..6c58c20 100644 --- a/internal/imgcache/urlparser.go +++ b/internal/imgcache/urlparser.go @@ -52,10 +52,11 @@ var sizeFormatRegex = regexp.MustCompile( var percentEscapeRegex = regexp.MustCompile(`%[0-9A-Fa-f]{2}`) // queryEscapeRegex matches a percent-escape, or one character that a browser -// percent-encodes in a URL's query: one outside the printable ASCII characters -// "!" to "~", such as a space, a control character or a character outside -// ASCII, or one of `"#<>`. A "%" that does not begin a percent-escape matches too. -var queryEscapeRegex = regexp.MustCompile(`%[0-9A-Fa-f]{2}|[^!-~]|["#<>%]`) +// percent-encodes in the query of an http or https URL: one outside the +// printable ASCII characters "!" to "~", such as a space, a control character +// or a character outside ASCII, or one of `"#'<>`. A "%" that does not begin a +// percent-escape matches too. +var queryEscapeRegex = 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 -- 2.54.0