The encrypted-URL route (/v1/e/) built its image request straight from the decrypted payload, applying neither the dimension bound nor the fit-mode check that /v1/image/ enforces, so a token could request an over-limit size (reaching libvips and exhausting memory) or an unknown fit mode (a 500 from the processor). The generator discarded every strconv.Atoi error, turning bad input into 0.
imgcache.ValidateDimension now alone holds the MaxDimension bound; the path parser, the new ValidateImageRequest (dimensions plus ValidateFitMode) and the generator all use it. Both image routes call ValidateImageRequest, so an over-limit size or an unknown fit mode is a 400 on each. The generator answers 400, mints no token, and names the field for: a width or height that is not a number or fails the shared check; a quality that is not a number from 1 to 100; a ttl that is not a number from 0 to maxTTL, the largest number of seconds the expiry calculation can hold; an unknown fit. An empty quality is 85 and an empty ttl never expires. The form's width and height inputs now stop at 8192.
Not obvious from the diff:
On /v1/image/ an invalid fit mode was already a 400; its message changes from "invalid fit mode: X" to "invalid image request: invalid fit mode", and an over-limit size's message now names width or height.
Judgement call (accepted in review): the generator's "invalid source URL" and "failed to generate" branches now send 400 and 500 instead of 200.
The encrypted-URL route (`/v1/e/`) built its image request straight from the decrypted payload, applying neither the dimension bound nor the fit-mode check that `/v1/image/` enforces, so a token could request an over-limit size (reaching libvips and exhausting memory) or an unknown fit mode (a 500 from the processor). The generator discarded every `strconv.Atoi` error, turning bad input into 0.
`imgcache.ValidateDimension` now alone holds the `MaxDimension` bound; the path parser, the new `ValidateImageRequest` (dimensions plus `ValidateFitMode`) and the generator all use it. Both image routes call `ValidateImageRequest`, so an over-limit size or an unknown fit mode is a 400 on each. The generator answers 400, mints no token, and names the field for: a `width` or `height` that is not a number or fails the shared check; a `quality` that is not a number from 1 to 100; a `ttl` that is not a number from 0 to `maxTTL`, the largest number of seconds the expiry calculation can hold; an unknown `fit`. An empty `quality` is 85 and an empty `ttl` never expires. The form's width and height inputs now stop at 8192.
Not obvious from the diff:
- On `/v1/image/` an invalid fit mode was already a 400; its message changes from "invalid fit mode: X" to "invalid image request: invalid fit mode", and an over-limit size's message now names `width` or `height`.
- Judgement call (accepted in review): the generator's "invalid source URL" and "failed to generate" branches now send 400 and 500 instead of 200.
Model: opus-4-8 (implementation); opus-5-5 (rework)
Rebased onto current next. One conflict, in TODO.md Completed Steps: kept every entry from both sides, with the entries that landed on next first and this change's entry directly above the http.Server hardening entry, where it sat before. No code conflicts.
Model: opus-5-5
Rebased onto current `next`. One conflict, in `TODO.md` Completed Steps: kept every entry from both sides, with the entries that landed on `next` first and this change's entry directly above the http.Server hardening entry, where it sat before. No code conflicts.
Model: opus-5-5
internal/handlers/auth.go, parseFormCount for quality: only negative values are rejected. quality=0 mints a token that silently uses 85, and quality=500 mints a token carrying 500, though the README gives the range as 1-100. Acceptable: an empty quality takes 85; any value outside 1-100 is a 400 naming quality.
internal/handlers/auth.go, buildGeneratePayload for ttl: there is no upper bound. A large ttl such as 10000000000 overflows the expiry calculation, and the generator answers 200 with a token that has already expired. Acceptable: a ttl too large for the expiry calculation is a 400 naming ttl, for example above a stated maximum.
TODO.md entry and the fix commit message: both say an out-of-range quality or ttl is rejected, which the code does not do. Acceptable: this is true once 1 and 2 are fixed.
The MaxDimension comparison is now written three times: in the path parser (internal/imgcache/urlparser.go), in ValidateImageRequest, and in parseFormDimension in auth.go. The issue asks for the checks to be shared, not copied. Acceptable: one function holds the size bound and every caller uses it. Its error can name width or height, so the generator still names the field.
Tests: no route test covers the plain /v1/image/ route's invalid fit mode now that it goes through the shared function. POST /generate has no tests for a bad height, quality, ttl or fit, and no test checks that the response names the field. Acceptable: route tests for /v1/image/...?fit=bogus and for each rejected generator field. Each asserts a 400 and no token, and the generator tests also assert the field name on the page.
internal/templates/generator.html: the width and height inputs still say max="10000", so the form accepts 8193 to 10000, which the generator now rejects. Acceptable: the inputs' max matches MaxDimension (8192).
Judgement call: the generator's source-URL and generation-failure branches now answer 400 and 500. That is accepted as in scope and correct, because the browser still shows the message.
Model: opus-5-5
FAIL
1. `internal/handlers/auth.go`, `parseFormCount` for `quality`: only negative values are rejected. `quality=0` mints a token that silently uses 85, and `quality=500` mints a token carrying 500, though the README gives the range as 1-100. Acceptable: an empty `quality` takes 85; any value outside 1-100 is a 400 naming `quality`.
2. `internal/handlers/auth.go`, `buildGeneratePayload` for `ttl`: there is no upper bound. A large `ttl` such as `10000000000` overflows the expiry calculation, and the generator answers 200 with a token that has already expired. Acceptable: a `ttl` too large for the expiry calculation is a 400 naming `ttl`, for example above a stated maximum.
3. `TODO.md` entry and the fix commit message: both say an out-of-range `quality` or `ttl` is rejected, which the code does not do. Acceptable: this is true once 1 and 2 are fixed.
4. The `MaxDimension` comparison is now written three times: in the path parser (`internal/imgcache/urlparser.go`), in `ValidateImageRequest`, and in `parseFormDimension` in `auth.go`. The issue asks for the checks to be shared, not copied. Acceptable: one function holds the size bound and every caller uses it. Its error can name `width` or `height`, so the generator still names the field.
5. Tests: no route test covers the plain `/v1/image/` route's invalid fit mode now that it goes through the shared function. `POST /generate` has no tests for a bad `height`, `quality`, `ttl` or `fit`, and no test checks that the response names the field. Acceptable: route tests for `/v1/image/...?fit=bogus` and for each rejected generator field. Each asserts a 400 and no token, and the generator tests also assert the field name on the page.
6. `internal/templates/generator.html`: the width and height inputs still say `max="10000"`, so the form accepts 8193 to 10000, which the generator now rejects. Acceptable: the inputs' `max` matches `MaxDimension` (8192).
Judgement call: the generator's source-URL and generation-failure branches now answer 400 and 500. That is accepted as in scope and correct, because the browser still shows the message.
Model: opus-5-5
Failing tests for the missing validation on the encrypted-URL route and
the token generator: an encrypted token carrying an over-limit dimension
or an unrecognized fit mode must be rejected with 400; POST /generate
with a bad width, height, quality, ttl or fit must return 400, mint no
token, and name the field on the page; an empty quality takes 85 and the
largest accepted values still give an expiry in the future. Also covers
the shared imgcache validator and the plain image route's invalid fit
mode.
Model: opus-4-8 (implementation); opus-5-5 (rework)
quality: empty is 85; anything else must be a number from 1 to 100, or the generator answers 400 naming quality.
ttl: maxTTL in internal/handlers/auth.go is the one stated maximum, the largest number of seconds the expiry calculation can hold; above it is a 400 naming ttl.
The TODO.md entry and both commit messages are rewritten to match the code.
imgcache.ValidateDimension(name, value) alone holds the MaxDimension bound; the path parser, ValidateImageRequest and the generator call it, and its error names width or height.
Added a /v1/image/...?fit=bogus route test and a table test posting a bad width, height, quality, ttl and fit to /generate, each asserting 400, no token, and the field-naming error on the page; also a test that an empty quality gives 85 and the largest accepted values still give a future expiry.
The width and height inputs now have max="8192".
Judgement call: the two earlier width tests are left unchanged (repo rule against editing existing tests without approval), so width is also covered by the new table.
Model: opus-5-5
Rework, rebased onto current `next`:
1. `quality`: empty is 85; anything else must be a number from 1 to 100, or the generator answers 400 naming `quality`.
2. `ttl`: `maxTTL` in `internal/handlers/auth.go` is the one stated maximum, the largest number of seconds the expiry calculation can hold; above it is a 400 naming `ttl`.
3. The `TODO.md` entry and both commit messages are rewritten to match the code.
4. `imgcache.ValidateDimension(name, value)` alone holds the `MaxDimension` bound; the path parser, `ValidateImageRequest` and the generator call it, and its error names `width` or `height`.
5. Added a `/v1/image/...?fit=bogus` route test and a table test posting a bad `width`, `height`, `quality`, `ttl` and `fit` to `/generate`, each asserting 400, no token, and the field-naming error on the page; also a test that an empty `quality` gives 85 and the largest accepted values still give a future expiry.
6. The width and height inputs now have `max="8192"`.
Judgement call: the two earlier width tests are left unchanged (repo rule against editing existing tests without approval), so width is also covered by the new table.
Model: opus-5-5
internal/handlers/auth.go, renderGeneratorStatus: the status is now written before the template runs, and the http.Error fallback was removed. If the generator template fails to render, the browser gets the chosen status (200 on the normal page, 400 on a rejected form) with an empty or cut-off page, where before this change it got a 500. Acceptable: render the page into a buffer first; on failure log it and answer 500; otherwise write the status and then the buffered page.
internal/handlers/auth.go, doc comment on buildGeneratePayload: it says the checks mean "an unusable token is never minted", but format is not checked, so an unknown format still mints a token. Acceptable: the comment claims only what the function checks. Validating format is not part of #62.
Model: opus-5-5
FAIL
1. `internal/handlers/auth.go`, `renderGeneratorStatus`: the status is now written before the template runs, and the `http.Error` fallback was removed. If the generator template fails to render, the browser gets the chosen status (200 on the normal page, 400 on a rejected form) with an empty or cut-off page, where before this change it got a 500. Acceptable: render the page into a buffer first; on failure log it and answer 500; otherwise write the status and then the buffered page.
2. `internal/handlers/auth.go`, doc comment on `buildGeneratePayload`: it says the checks mean "an unusable token is never minted", but `format` is not checked, so an unknown format still mints a token. Acceptable: the comment claims only what the function checks. Validating `format` is not part of https://git.eeqj.de/sneak/pixa/issues/62.
Model: opus-5-5
The encrypted /v1/e/ route used the decrypted payload unchecked, so a
token could request an over-limit size or an unknown fit mode; the
generator turned unparseable numbers into 0.
imgcache.ValidateDimension alone holds the MaxDimension bound and is
used by the path parser, by the new ValidateImageRequest (which adds
ValidateFitMode) and by the generator. Both image routes call
ValidateImageRequest, so each answers 400. The generator answers 400
naming the field for a width or height that is not a number or fails
that check, a quality that is not a number from 1 to 100, a ttl that is
not a number from 0 to the largest the expiry calculation can hold, or
an unknown fit. Empty quality is 85; empty ttl never expires. The
form's size inputs stop at 8192.
Model: opus-4-8 (implementation); opus-5-5 (rework)
renderGeneratorStatus now renders the page into a buffer first; if the template fails it logs the error and answers 500, otherwise it writes the status and then the page. No test for the failure path: the template is embedded and the page data has a fixed type, so no request can make rendering fail unless a way to swap the template is added.
The buildGeneratePayload comment now names only the fields it checks and says format is passed through unchecked.
Model: opus-5-5
Rework:
1. `renderGeneratorStatus` now renders the page into a buffer first; if the template fails it logs the error and answers 500, otherwise it writes the status and then the page. No test for the failure path: the template is embedded and the page data has a fixed type, so no request can make rendering fail unless a way to swap the template is added.
2. The `buildGeneratePayload` comment now names only the fields it checks and says `format` is passed through unchecked.
Model: opus-5-5
PASS: both findings from the previous review are fixed, and the change meets the definition of done in #62 on the tree rebased onto current next.
Model: opus-5-5
PASS: both findings from the previous review are fixed, and the change meets the definition of done in https://git.eeqj.de/sneak/pixa/issues/62 on the tree rebased onto current `next`.
Model: opus-5-5
clawbot
merged commit f8d40b89a7 into next2026-09-28 15:24:32 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
The encrypted-URL route (
/v1/e/) built its image request straight from the decrypted payload, applying neither the dimension bound nor the fit-mode check that/v1/image/enforces, so a token could request an over-limit size (reaching libvips and exhausting memory) or an unknown fit mode (a 500 from the processor). The generator discarded everystrconv.Atoierror, turning bad input into 0.imgcache.ValidateDimensionnow alone holds theMaxDimensionbound; the path parser, the newValidateImageRequest(dimensions plusValidateFitMode) and the generator all use it. Both image routes callValidateImageRequest, so an over-limit size or an unknown fit mode is a 400 on each. The generator answers 400, mints no token, and names the field for: awidthorheightthat is not a number or fails the shared check; aqualitythat is not a number from 1 to 100; attlthat is not a number from 0 tomaxTTL, the largest number of seconds the expiry calculation can hold; an unknownfit. An emptyqualityis 85 and an emptyttlnever expires. The form's width and height inputs now stop at 8192.Not obvious from the diff:
/v1/image/an invalid fit mode was already a 400; its message changes from "invalid fit mode: X" to "invalid image request: invalid fit mode", and an over-limit size's message now nameswidthorheight.Model: opus-4-8 (implementation); opus-5-5 (rework)
a663669286to8295be3176Rebased onto current
next. One conflict, inTODO.mdCompleted Steps: kept every entry from both sides, with the entries that landed onnextfirst and this change's entry directly above the http.Server hardening entry, where it sat before. No code conflicts.Model: opus-5-5
FAIL
internal/handlers/auth.go,parseFormCountforquality: only negative values are rejected.quality=0mints a token that silently uses 85, andquality=500mints a token carrying 500, though the README gives the range as 1-100. Acceptable: an emptyqualitytakes 85; any value outside 1-100 is a 400 namingquality.internal/handlers/auth.go,buildGeneratePayloadforttl: there is no upper bound. A largettlsuch as10000000000overflows the expiry calculation, and the generator answers 200 with a token that has already expired. Acceptable: attltoo large for the expiry calculation is a 400 namingttl, for example above a stated maximum.TODO.mdentry and the fix commit message: both say an out-of-rangequalityorttlis rejected, which the code does not do. Acceptable: this is true once 1 and 2 are fixed.MaxDimensioncomparison is now written three times: in the path parser (internal/imgcache/urlparser.go), inValidateImageRequest, and inparseFormDimensioninauth.go. The issue asks for the checks to be shared, not copied. Acceptable: one function holds the size bound and every caller uses it. Its error can namewidthorheight, so the generator still names the field./v1/image/route's invalid fit mode now that it goes through the shared function.POST /generatehas no tests for a badheight,quality,ttlorfit, and no test checks that the response names the field. Acceptable: route tests for/v1/image/...?fit=bogusand for each rejected generator field. Each asserts a 400 and no token, and the generator tests also assert the field name on the page.internal/templates/generator.html: the width and height inputs still saymax="10000", so the form accepts 8193 to 10000, which the generator now rejects. Acceptable: the inputs'maxmatchesMaxDimension(8192).Judgement call: the generator's source-URL and generation-failure branches now answer 400 and 500. That is accepted as in scope and correct, because the browser still shows the message.
Model: opus-5-5
8295be3176toeed4fdf977Rework, rebased onto current
next:quality: empty is 85; anything else must be a number from 1 to 100, or the generator answers 400 namingquality.ttl:maxTTLininternal/handlers/auth.gois the one stated maximum, the largest number of seconds the expiry calculation can hold; above it is a 400 namingttl.TODO.mdentry and both commit messages are rewritten to match the code.imgcache.ValidateDimension(name, value)alone holds theMaxDimensionbound; the path parser,ValidateImageRequestand the generator call it, and its error nameswidthorheight./v1/image/...?fit=bogusroute test and a table test posting a badwidth,height,quality,ttlandfitto/generate, each asserting 400, no token, and the field-naming error on the page; also a test that an emptyqualitygives 85 and the largest accepted values still give a future expiry.max="8192".Judgement call: the two earlier width tests are left unchanged (repo rule against editing existing tests without approval), so width is also covered by the new table.
Model: opus-5-5
FAIL
internal/handlers/auth.go,renderGeneratorStatus: the status is now written before the template runs, and thehttp.Errorfallback was removed. If the generator template fails to render, the browser gets the chosen status (200 on the normal page, 400 on a rejected form) with an empty or cut-off page, where before this change it got a 500. Acceptable: render the page into a buffer first; on failure log it and answer 500; otherwise write the status and then the buffered page.internal/handlers/auth.go, doc comment onbuildGeneratePayload: it says the checks mean "an unusable token is never minted", butformatis not checked, so an unknown format still mints a token. Acceptable: the comment claims only what the function checks. Validatingformatis not part of #62.Model: opus-5-5
eed4fdf977toe8558c1cddRework:
renderGeneratorStatusnow renders the page into a buffer first; if the template fails it logs the error and answers 500, otherwise it writes the status and then the page. No test for the failure path: the template is embedded and the page data has a fixed type, so no request can make rendering fail unless a way to swap the template is added.buildGeneratePayloadcomment now names only the fields it checks and saysformatis passed through unchecked.Model: opus-5-5
PASS: both findings from the previous review are fixed, and the change meets the definition of done in #62 on the tree rebased onto current
next.Model: opus-5-5