From 45869572ff996abf1b4a3807cc6c47f7c6506c69 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 28 Sep 2026 17:46:49 +0200 Subject: [PATCH] Refuse a q outside 1-100 on /v1/image/ with 400 (closes #134) A q that was not a number or was outside 1-100 was dropped and 85 used, so q=500 was served and verified against a signature made for 85. It is now a 400 naming q and the value, read with the generator's quality check; only a q missing from the URL is 85. The route also refuses with 400 a query string that cannot be decoded (r.URL.Query() drops such a pair, so q=80% arrived as no q) and any parameter given more than once, which was read from its first value only (q=80&q=500 was served at 80). Model: opus-5-5 --- README.md | 8 ++- TODO.md | 10 ++++ internal/handlers/auth.go | 17 +++--- internal/handlers/handlers_internal_test.go | 65 +++++++++++++++++++++ internal/handlers/image.go | 57 +++++++++++++----- 5 files changed, 134 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index 7e21964..8f1e261 100644 --- a/README.md +++ b/README.md @@ -94,6 +94,9 @@ In-process caching of request-to-output mappings targets 1-5k r/s. Images are only fetched from origins using TLS with valid certificates. +A request whose query string cannot be decoded, or gives any parameter more +than once, is refused with 400. + - ``: one of `orig`, `png`, `jpeg`, `webp` - ``: `orig` or `x` (e.g. `800x600`) @@ -125,8 +128,9 @@ Where: - `height` — requested height in pixels, `0` for original - `format` — output format (jpeg, png, webp, avif, gif, orig) - `expiration` — Unix timestamp when signature expires -- `quality` — the URL's `q` query parameter (1-100), or `85` when the URL - has no `q` +- `quality` — the URL's `q` query parameter, a whole number from 1 to 100, + or `85` when the URL has no `q`; a request whose `q` is anything else is + refused with 400 - `fit` — the URL's `fit` query parameter (cover, contain, fill, inside, outside), or `cover` when the URL has no `fit` diff --git a/TODO.md b/TODO.md index e726796..736c4e1 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,16 @@ exhaustion # Completed Steps +- 2026-09-28 refuse an invalid `q` on `/v1/image/` (closes #134): a `q` + that is not a whole number from 1 to 100, an empty `q` included, is a + 400 naming `q` and the value, instead of being served at the default + 85; the route reads `q` with the generator's quality check + (`parseFormInt` with `minQuality` and `maxQuality`); only a `q` missing + from the URL is still 85; a query string that cannot be decoded, such + as `q=80%`, is a 400 showing it; any query parameter given more than + once (`q`, `fit`, `sig`, `exp` alike) is a 400 naming it, so none is + read from its first value only; `README.md` states the range and both + query-string rules. - 2026-09-28 unknown `PIXA_` environment variables abort startup (closes #133): a variable whose name starts with `PIXA_` but is neither a setting's variable nor `PIXA_CONFIG_PATH` aborts startup naming it, as diff --git a/internal/handlers/auth.go b/internal/handlers/auth.go index 4edf22f..171b900 100644 --- a/internal/handlers/auth.go +++ b/internal/handlers/auth.go @@ -18,13 +18,14 @@ import ( "sneak.berlin/go/pixa/internal/templates" ) -// errInvalidFormField reports a generator form field whose value is -// non-numeric or out of range. The offending field name is wrapped in so the -// response can name it. +// errInvalidFormField reports a generator form field, or the q parameter of +// /v1/image/, whose value is non-numeric or out of range. The offending field +// name is wrapped in so the response can name it. var errInvalidFormField = errors.New("invalid") -// Bounds for the generator's quality and ttl fields. maxTTL is in seconds: -// the expiry calculation time.Duration(ttl) * time.Second overflows above it. +// Bounds for the generator's quality and ttl fields; the quality bounds also +// apply to the q parameter of /v1/image/. maxTTL is in seconds: the expiry +// calculation time.Duration(ttl) * time.Second overflows above it. const ( minQuality = 1 maxQuality = 100 @@ -248,9 +249,9 @@ func parseFormDimension(form url.Values, field string) (int, error) { return value, nil } -// parseFormInt reads an optional integer form field, returning def when the -// field is empty and an error naming the field when the value is non-numeric -// or outside minValue to maxValue. +// parseFormInt reads an optional integer form field or URL query parameter, +// returning def when the field is empty and an error naming the field when the +// value is non-numeric or outside minValue to maxValue. func parseFormInt( form url.Values, field string, def, minValue, maxValue int, ) (int, error) { diff --git a/internal/handlers/handlers_internal_test.go b/internal/handlers/handlers_internal_test.go index bfcb01a..780a46c 100644 --- a/internal/handlers/handlers_internal_test.go +++ b/internal/handlers/handlers_internal_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "database/sql" + "encoding/json" "image" "image/color" "image/jpeg" @@ -291,3 +292,67 @@ func TestHandleImage_InvalidFitMode_Returns400(t *testing.T) { t.Fatalf("status = %d, want %d", status, http.StatusBadRequest) } } + +// TestHandleImage_InvalidQuery_Returns400 verifies that the plain image route +// answers a q that is not a whole number from 1 to 100, an empty one +// included, with 400 naming q and the value, a parameter given more than once +// with 400 naming it, and a query string that cannot be decoded with 400 +// showing it, instead of serving the image at the default quality 85 or at +// the first value given. +func TestHandleImage_InvalidQuery_Returns400(t *testing.T) { + t.Parallel() + + tests := []struct { + query, wantError string + }{ + {"q=banana", `invalid q: not a number, got "banana"`}, + {"q=0", `invalid q: must be from 1 to 100, got "0"`}, + {"q=101", `invalid q: must be from 1 to 100, got "101"`}, + {"q=", `invalid q: not a number, got ""`}, + {"q=80&q=500", `invalid q: given more than once`}, + {"q=80&q=", `invalid q: given more than once`}, + {"fit=cover&fit=contain", `invalid fit: given more than once`}, + {"q=80%", `invalid query string "q=80%": invalid URL escape "%"`}, + { + "q=50;fit=contain", + `invalid query string "q=50;fit=contain": ` + + `invalid semicolon separator in query`, + }, + } + + for _, tt := range tests { + t.Run(tt.query, func(t *testing.T) { + t.Parallel() + + fix := setupTestHandler(t) + + r := chi.NewRouter() + r.Get("/v1/image/*", fix.handler.HandleImage()) + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, + "/v1/image/"+fix.goodHost+"/images/photo.jpg/50x50.jpeg?"+tt.query, nil) + rec := httptest.NewRecorder() + + r.ServeHTTP(rec, req) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want %d", rec.Code, http.StatusBadRequest) + } + + t.Logf("GET %s: %d %s", req.URL, rec.Code, rec.Body) + + var body struct { + Error string `json:"error"` + } + + err := json.NewDecoder(rec.Body).Decode(&body) + if err != nil { + t.Fatalf("decoding response body: %v", err) + } + + if body.Error != tt.wantError { + t.Errorf("error = %q, want %q", body.Error, tt.wantError) + } + }) + } +} diff --git a/internal/handlers/image.go b/internal/handlers/image.go index 7411e56..f8a1e64 100644 --- a/internal/handlers/image.go +++ b/internal/handlers/image.go @@ -2,12 +2,15 @@ package handlers import ( "errors" + "fmt" "io" "net/http" + "net/url" "strconv" "time" "github.com/go-chi/chi/v5" + "sneak.berlin/go/pixa/internal/encurl" "sneak.berlin/go/pixa/internal/httpfetcher" "sneak.berlin/go/pixa/internal/imgcache" ) @@ -89,8 +92,28 @@ func (s *Handlers) parseImageRequest( // Convert to ImageRequest req := parsed.ToImageRequest() - // Parse signature params from query string - query := r.URL.Query() + // Parse signature params from query string. r.URL.Query() would silently + // drop a pair it cannot decode, such as q=80%, so that q would be served + // at 85; a query string that cannot be decoded is refused instead. A + // parameter given more than once is refused too, as only its first value + // would be read. + query, err := url.ParseQuery(r.URL.RawQuery) + if err != nil { + s.respondError(w, fmt.Sprintf("invalid query string %q: %v", + r.URL.RawQuery, err), http.StatusBadRequest) + + return nil, false + } + + for name, values := range query { + if len(values) > 1 { + s.respondError(w, fmt.Sprintf("invalid %s: given more than once", + name), http.StatusBadRequest) + + return nil, false + } + } + req.Signature = query.Get("sig") if expStr := query.Get("exp"); expStr != "" { @@ -100,23 +123,31 @@ func (s *Handlers) parseImageRequest( } } - // Parse optional quality and fit params - if qStr := query.Get("q"); qStr != "" { - q, parseErr := strconv.Atoi(qStr) - if parseErr == nil && q > 0 && q <= 100 { - req.Quality = q - } + // Parse optional quality and fit params. Only a q missing from the URL is + // 85. A q in the URL that is not a whole number from 1 to 100, an empty + // one included, is refused, checked as the generator checks its quality + // field; that check alone would take an empty q as missing. + qStr := query.Get("q") + if query.Has("q") && qStr == "" { + s.respondError(w, `invalid q: not a number, got ""`, + http.StatusBadRequest) + + return nil, false + } + + req.Quality, err = parseFormInt(query, "q", + encurl.DefaultQuality, minQuality, maxQuality) + if err != nil { + s.respondError(w, fmt.Sprintf("%v, got %q", err, qStr), + http.StatusBadRequest) + + return nil, false } if fit := query.Get("fit"); fit != "" { req.FitMode = imgcache.FitMode(fit) } - // Default quality if not set - if req.Quality == 0 { - req.Quality = 85 - } - // Default fit mode if not set if req.FitMode == "" { req.FitMode = imgcache.FitCover