On /v1/image/, an exp that was not a whole number, or an empty exp=, was ignored, so a URL for a host that needs a signature got 401 as if it had no exp. It is now a 400 naming exp and the value (invalid exp: not a number, got "banana"), as q and fit already are (#134, #139). A missing exp is unchanged. README.md says so where it documents exp.
These discarded errors are now logged at warn with the path or key and the error, and stay non-fatal: the variant .meta write, the source metadata JSON write, the two Stats count queries (their tables stay with #56), the stats counter updates, the negative cache write and the expired negative cache delete.
Worth knowing:
The exp check also applies to allowlisted hosts, which skip the signature check and used to ignore a bad exp.
VariantStorage had no logger; NewVariantStorage now takes the cache's.
parseImageRequest and StoreSource were at the 80-line limit, so exp parsing moved into parseExpires and the metadata JSON write into writeMetadataSidecar.
Disclosures:
Judgement call: the negative cache write in service.go is logged too; the plan's list did not name it.
Left as they are: closing files and bodies after reading, removing temp files on paths already returning an error, rolling back a committed transaction, writing robots.txt to the client, and setting PIXA_CONFIG_PATH from the command-line flag, which cannot fail for that name.
Untested: the warn lines of the source metadata JSON write, the negative cache write and the expired negative cache delete; no test makes these fail.
Commits: the failing test first, then the fix.
Model: opus-5-5
Closes https://git.eeqj.de/sneak/pixa/issues/72.
On `/v1/image/`, an `exp` that was not a whole number, or an empty `exp=`, was ignored, so a URL for a host that needs a signature got 401 as if it had no `exp`. It is now a 400 naming `exp` and the value (`invalid exp: not a number, got "banana"`), as `q` and `fit` already are (https://git.eeqj.de/sneak/pixa/issues/134, https://git.eeqj.de/sneak/pixa/issues/139). A missing `exp` is unchanged. `README.md` says so where it documents `exp`.
These discarded errors are now logged at `warn` with the path or key and the error, and stay non-fatal: the variant `.meta` write, the source metadata JSON write, the two `Stats` count queries (their tables stay with https://git.eeqj.de/sneak/pixa/issues/56), the stats counter updates, the negative cache write and the expired negative cache delete.
Worth knowing:
- The `exp` check also applies to allowlisted hosts, which skip the signature check and used to ignore a bad `exp`.
- `VariantStorage` had no logger; `NewVariantStorage` now takes the cache's.
- `parseImageRequest` and `StoreSource` were at the 80-line limit, so `exp` parsing moved into `parseExpires` and the metadata JSON write into `writeMetadataSidecar`.
Disclosures:
- Judgement call: the negative cache write in `service.go` is logged too; the plan's list did not name it.
- Left as they are: closing files and bodies after reading, removing temp files on paths already returning an error, rolling back a committed transaction, writing `robots.txt` to the client, and setting `PIXA_CONFIG_PATH` from the command-line flag, which cannot fail for that name.
- Untested: the warn lines of the source metadata JSON write, the negative cache write and the expired negative cache delete; no test makes these fail.
- Commits: the failing test first, then the fix.
Model: opus-5-5
An exp that is not a whole number, or an empty exp=, is ignored today,
so a URL for a host that needs a signature gets 401 as if it had no exp.
This test asks for a 400 naming exp and the value instead; it fails
until the route refuses it. A URL without exp still gets 401.
Model: opus-5-5
An exp that was not a whole number, or empty, was ignored, so a URL for
a host that needs a signature got 401 as if it had no exp. It is now a
400 naming exp and the value; only an exp missing from the URL is
unchanged.
A failed variant .meta write, source metadata JSON write, Stats count
query, stats counter update, negative cache write or expired negative
cache delete was discarded without a trace. Each is now logged at warn
with the path or key and the error, and stays non-fatal. VariantStorage
takes the cache's logger for this. The metadata JSON write moved out of
StoreSource into writeMetadataSidecar to keep StoreSource within the
function length limit.
Model: opus-5-5
Untested warn lines, internal/imgcache/cache.go and internal/imgcache/storage.go: the PR body says nothing forces these writes or queries to fail, but the variant .meta write, the two Stats count queries and the stats counter updates can each be made to fail with the existing test helpers (a directory where the .meta file goes; a dropped table in the test database). Acceptable: a test for each that hands the cache or NewVariantStorage a logger writing to a buffer, asserts the warn line, and asserts the call still succeeds; the PR body's untested line then names only what is left.
Stale comment, internal/handlers/auth.go line 21: errInvalidFormField is described as reporting a generator form field or the q parameter of /v1/image/, but parseExpires now wraps it for exp too. Acceptable: the comment names exp as well.
Model: opus-5-5
FAIL
1. Untested warn lines, `internal/imgcache/cache.go` and `internal/imgcache/storage.go`: the PR body says nothing forces these writes or queries to fail, but the variant `.meta` write, the two `Stats` count queries and the stats counter updates can each be made to fail with the existing test helpers (a directory where the `.meta` file goes; a dropped table in the test database). Acceptable: a test for each that hands the cache or `NewVariantStorage` a logger writing to a buffer, asserts the warn line, and asserts the call still succeeds; the PR body's untested line then names only what is left.
2. Stale comment, `internal/handlers/auth.go` line 21: `errInvalidFormField` is described as reporting a generator form field or the `q` parameter of `/v1/image/`, but `parseExpires` now wraps it for `exp` too. Acceptable: the comment names `exp` as well.
Model: opus-5-5
A directory where a variant .meta file goes, and dropped tables in the
test database, make the variant .meta write, the two Stats count queries
and the stats counter updates fail. Each test hands the cache or
NewVariantStorage a logger writing to a buffer, checks the warn line, and
checks the store and Stats calls still succeed.
Model: opus-5-5
Done: a test each for the variant .meta write, the two Stats count queries and the stats counter updates. IncrementStats returns nothing, so its test checks the warn lines only. The PR body's untested line now names what is left.
Done.
Model: opus-5-5
Rework for the review at https://git.eeqj.de/sneak/pixa/pulls/141#issuecomment-104000:
1. Done: a test each for the variant `.meta` write, the two `Stats` count queries and the stats counter updates. `IncrementStats` returns nothing, so its test checks the warn lines only. The PR body's untested line now names what is left.
2. Done.
Model: opus-5-5
PASS: an exp that is set but not a whole number is now refused with 400 on every host, and each discarded cache error is logged at warn and stays non-fatal, with tests that catch a missing or wrong-level warn line.
Model: opus-5-5
PASS: an `exp` that is set but not a whole number is now refused with 400 on every host, and each discarded cache error is logged at `warn` and stays non-fatal, with tests that catch a missing or wrong-level warn line.
Model: opus-5-5
clawbot
merged commit 6010f5beb0 into next2026-09-28 19:59:39 +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.
Closes #72.
On
/v1/image/, anexpthat was not a whole number, or an emptyexp=, was ignored, so a URL for a host that needs a signature got 401 as if it had noexp. It is now a 400 namingexpand the value (invalid exp: not a number, got "banana"), asqandfitalready are (#134, #139). A missingexpis unchanged.README.mdsays so where it documentsexp.These discarded errors are now logged at
warnwith the path or key and the error, and stay non-fatal: the variant.metawrite, the source metadata JSON write, the twoStatscount queries (their tables stay with #56), the stats counter updates, the negative cache write and the expired negative cache delete.Worth knowing:
expcheck also applies to allowlisted hosts, which skip the signature check and used to ignore a badexp.VariantStoragehad no logger;NewVariantStoragenow takes the cache's.parseImageRequestandStoreSourcewere at the 80-line limit, soexpparsing moved intoparseExpiresand the metadata JSON write intowriteMetadataSidecar.Disclosures:
service.gois logged too; the plan's list did not name it.robots.txtto the client, and settingPIXA_CONFIG_PATHfrom the command-line flag, which cannot fail for that name.Model: opus-5-5
FAIL
Untested warn lines,
internal/imgcache/cache.goandinternal/imgcache/storage.go: the PR body says nothing forces these writes or queries to fail, but the variant.metawrite, the twoStatscount queries and the stats counter updates can each be made to fail with the existing test helpers (a directory where the.metafile goes; a dropped table in the test database). Acceptable: a test for each that hands the cache orNewVariantStoragea logger writing to a buffer, asserts the warn line, and asserts the call still succeeds; the PR body's untested line then names only what is left.Stale comment,
internal/handlers/auth.goline 21:errInvalidFormFieldis described as reporting a generator form field or theqparameter of/v1/image/, butparseExpiresnow wraps it forexptoo. Acceptable: the comment namesexpas well.Model: opus-5-5
Rework for the review at #141 (comment):
.metawrite, the twoStatscount queries and the stats counter updates.IncrementStatsreturns nothing, so its test checks the warn lines only. The PR body's untested line now names what is left.Model: opus-5-5
PASS: an
expthat is set but not a whole number is now refused with 400 on every host, and each discarded cache error is logged atwarnand stays non-fatal, with tests that catch a missing or wrong-level warn line.Model: opus-5-5