From 19bb0637821708db14e00c1a9e6c065d565a4e4a Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 28 Sep 2026 14:44:25 +0000 Subject: [PATCH] Refuse a q outside 1-100 on /v1/image/ with 400 (closes #134) The route ignored a q that was not a number or was outside 1-100 and used 85, so q=banana or q=500 was served as if q were absent and verified against a signature made for 85. It now reads q with the check the URL generator uses for its quality field (parseFormInt with minQuality and maxQuality, default encurl.DefaultQuality) and answers anything else with a 400 naming q and the value. That check takes an empty value as missing, so an empty q in the URL is refused before it. The query string is read with url.ParseQuery, since r.URL.Query() drops a pair it cannot decode, such as q=80%; one that cannot be decoded is a 400 showing it. Only a q missing from the URL is 85. README.md states the range. Model: opus-5-5 --- README.md | 5 +++-- TODO.md | 7 ++++++ internal/handlers/auth.go | 17 +++++++------- internal/handlers/image.go | 46 +++++++++++++++++++++++++++----------- 4 files changed, 52 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index 7e21964..a2dd813 100644 --- a/README.md +++ b/README.md @@ -125,8 +125,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..e158978 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,13 @@ 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; `README.md` states the range. - 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/image.go b/internal/handlers/image.go index 7411e56..d8bfa6a 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,17 @@ 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. + 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 + } + req.Signature = query.Get("sig") if expStr := query.Get("exp"); expStr != "" { @@ -100,23 +112,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