Neither q (quality) nor fit is included. But internal/handlers/image.go:50-63 reads both from the query string after signature validation, and both are part of the variant cache key (internal/imgcache/storage.go:327-328 — req.Quality, req.FitMode).
Consequence: a single valid signed URL can be replayed with q=1..100 and fit in {cover,contain,fill,inside,outside}, yielding 500 distinct cache entries and 500 libvips transcodes that the signature was never meant to authorize. That is disk amplification against the cache budget #51 just introduced, plus CPU/memory amplification against the processing path — from one URL, by anyone who has seen it.
This also makes the README false. README.md:69-73 states signatures are "exact match only: every component (host, path, query, dimensions, format, expiration) must match exactly what was signed", and the repo has an explicit exact-match-only enforcement commitment from #40.
Definition of done
Decide and implement one of:
(a) include quality and fit in the signed payload (preferred — makes the README's claim true), or
(b) reject any non-default q/fit on signature-authenticated requests with a 400.
If (a): the signed-data format changes, so update the format string, README.md's signature specification section (both the field list and the worked example), and regenerate/extend the golden vectors in internal/signature/golden_test.go. Note this is a breaking change to the URL signing scheme — any external signer must be updated in lockstep. Call that out in the PR body.
Failing test first per repo TDD rules: a test proving that a URL signed for q=85 is rejected (or produces a different signature) when replayed with q=40.
Existing exact-match tests from #40 must still pass unmodified.
make check green.
Notes
The /v1/e/ encrypted-URL path is unaffected by this specific issue (the whole payload is sealed), but see the separate issue on /v1/e/ input validation.
Option (a) is recommended: option (b) leaves the documented guarantee narrower than it reads and keeps quality/fit unauthenticated-but-ignored, which is easy to regress later.
Verified against `main` at `61f42e6`.
`Signer.buildSignatureData` (`internal/signature/signature.go:99-111`) signs exactly:
```
host:path:query:width:height:format:expiration
```
Neither `q` (quality) nor `fit` is included. But `internal/handlers/image.go:50-63` reads both from the query string *after* signature validation, and both are part of the variant cache key (`internal/imgcache/storage.go:327-328` — `req.Quality`, `req.FitMode`).
Consequence: a single valid signed URL can be replayed with `q=1..100` and `fit` in {`cover`,`contain`,`fill`,`inside`,`outside`}, yielding **500 distinct cache entries and 500 libvips transcodes** that the signature was never meant to authorize. That is disk amplification against the cache budget #51 just introduced, plus CPU/memory amplification against the processing path — from one URL, by anyone who has seen it.
This also makes the README false. `README.md:69-73` states signatures are "**exact match only**: every component (host, path, query, dimensions, format, expiration) must match exactly what was signed", and the repo has an explicit exact-match-only enforcement commitment from #40.
## Definition of done
1. Decide and implement one of:
- **(a)** include quality and fit in the signed payload (preferred — makes the README's claim true), or
- **(b)** reject any non-default `q`/`fit` on signature-authenticated requests with a 400.
2. If (a): the signed-data format changes, so update the format string, `README.md`'s signature specification section (both the field list and the worked example), and regenerate/extend the golden vectors in `internal/signature/golden_test.go`. Note this is a **breaking change to the URL signing scheme** — any external signer must be updated in lockstep. Call that out in the PR body.
3. Failing test first per repo TDD rules: a test proving that a URL signed for `q=85` is rejected (or produces a different signature) when replayed with `q=40`.
4. Existing exact-match tests from #40 must still pass unmodified.
5. `make check` green.
## Notes
- The `/v1/e/` encrypted-URL path is unaffected by this specific issue (the whole payload is sealed), but see the separate issue on `/v1/e/` input validation.
- Option (a) is recommended: option (b) leaves the documented guarantee narrower than it reads and keeps quality/fit unauthenticated-but-ignored, which is easy to regress later.
clawbot
added this to the 1.0.0 milestone 2026-08-09 03:42:53 +02:00
The effective (post-default) values are signed. The handler already normalizes an omitted q to 85 and an omitted fit to cover before signature verification, so a URL that omits them verifies against a signature over 85/cover.
TDD: a failing test first (a URL signed for q=85 is rejected when replayed with q=40), committed separately, then the fix.
README signature spec updated (field list + worked example); PR first line states the breaking change and lists what an external signer must change.
One blocker I will surface rather than resolve myself: because the signed bytes change, the three pinned constants in the existing internal/signature/golden_test.go can no longer match. Updating them edits an existing test, which needs owner approval. Per my brief I will put the new known-answer vectors in a new test file, leave golden_test.go unedited (so make check is red on that one test), state the exact new constant values on the PR, and stop there for a decision.
model: claude-opus-4-8
Plan (option (a) — quality and fit inside the signed payload):
- Add `Quality`/`FitMode` to `signature.Request` and extend the signed string by appending `:quality:fit` after the expiration:
`host:path:query:width:height:format:expiration:quality:fit`
The effective (post-default) values are signed. The handler already normalizes an omitted `q` to `85` and an omitted `fit` to `cover` before signature verification, so a URL that omits them verifies against a signature over `85`/`cover`.
- TDD: a failing test first (a URL signed for `q=85` is rejected when replayed with `q=40`), committed separately, then the fix.
- README signature spec updated (field list + worked example); PR first line states the breaking change and lists what an external signer must change.
One blocker I will surface rather than resolve myself: because the signed bytes change, the three pinned constants in the existing `internal/signature/golden_test.go` can no longer match. Updating them edits an existing test, which needs owner approval. Per my brief I will put the new known-answer vectors in a new test file, leave `golden_test.go` unedited (so `make check` is red on that one test), state the exact new constant values on the PR, and stop there for a decision.
model: claude-opus-4-8
Decision requested (sneak). #116 implements option (a): quality and fit join the signed payload, so the signed-data format becomes host:path:query:width:height:format:expiration:quality:fit. That necessarily invalidates the three pinned constants in the existing internal/signature/golden_test.go, whose whole purpose is to pin the format; the review failed the PR only because make check is red on that test, and changing an existing test needs your approval.
Options:
(1) Approve updating the three constants in golden_test.go to the new format (the new-format vectors are already added in a separate file by the PR). Recommended: the format change is the deliberate point of the fix, and the old vectors describe a scheme that let one signed URL authorize 500 unauthorized variants.
(2) Switch to option (b) from the issue: keep the old format and reject any non-default q/fit on signature-authenticated requests with a 400. No existing test changes, no external signer changes, but the documented "exact match" guarantee stays narrower than it reads.
Rework is dispatched as soon as you rule. If you take (1), a reply of "approved" here is enough.
model: claude-fable-5
Decision requested (sneak). https://git.eeqj.de/sneak/pixa/pulls/116 implements option (a): `quality` and `fit` join the signed payload, so the signed-data format becomes `host:path:query:width:height:format:expiration:quality:fit`. That necessarily invalidates the three pinned constants in the existing `internal/signature/golden_test.go`, whose whole purpose is to pin the format; the review failed the PR only because `make check` is red on that test, and changing an existing test needs your approval.
Options:
- (1) Approve updating the three constants in `golden_test.go` to the new format (the new-format vectors are already added in a separate file by the PR). Recommended: the format change is the deliberate point of the fix, and the old vectors describe a scheme that let one signed URL authorize 500 unauthorized variants.
- (2) Switch to option (b) from the issue: keep the old format and reject any non-default `q`/`fit` on signature-authenticated requests with a 400. No existing test changes, no external signer changes, but the documented "exact match" guarantee stays narrower than it reads.
Rework is dispatched as soon as you rule. If you take (1), a reply of "approved" here is enough.
model: claude-fable-5
sneak
was assigned by clawbot2026-09-21 10:02:54 +02:00
Ruling recorded (sneak, 2026-09-21, on #116): "This is a greenfield project with no instances running. It's still a prototype. There is no breaking change anywhere. There are no URLs anywhere. No one has used this software yet."
Reading taken: option (1). Nothing depends on the old signed format, so the three pinned constants in internal/signature/golden_test.go are updated to the new format, and the PR and README drop the breaking-change and external-signer-migration language. Rework of #116 proceeds on that basis.
Model: fable-5-1
Ruling recorded (sneak, 2026-09-21, on https://git.eeqj.de/sneak/pixa/pulls/116): "This is a greenfield project with no instances running. It's still a prototype. There is no breaking change anywhere. There are no URLs anywhere. No one has used this software yet."
Reading taken: option (1). Nothing depends on the old signed format, so the three pinned constants in `internal/signature/golden_test.go` are updated to the new format, and the PR and README drop the breaking-change and external-signer-migration language. Rework of https://git.eeqj.de/sneak/pixa/pulls/116 proceeds on that basis.
Model: fable-5-1
sneak
was unassigned by clawbot2026-09-22 00:43:53 +02:00
clawbot
self-assigned this 2026-09-22 00:43:53 +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.
Verified against
mainat61f42e6.Signer.buildSignatureData(internal/signature/signature.go:99-111) signs exactly:Neither
q(quality) norfitis included. Butinternal/handlers/image.go:50-63reads both from the query string after signature validation, and both are part of the variant cache key (internal/imgcache/storage.go:327-328—req.Quality,req.FitMode).Consequence: a single valid signed URL can be replayed with
q=1..100andfitin {cover,contain,fill,inside,outside}, yielding 500 distinct cache entries and 500 libvips transcodes that the signature was never meant to authorize. That is disk amplification against the cache budget #51 just introduced, plus CPU/memory amplification against the processing path — from one URL, by anyone who has seen it.This also makes the README false.
README.md:69-73states signatures are "exact match only: every component (host, path, query, dimensions, format, expiration) must match exactly what was signed", and the repo has an explicit exact-match-only enforcement commitment from #40.Definition of done
q/fiton signature-authenticated requests with a 400.README.md's signature specification section (both the field list and the worked example), and regenerate/extend the golden vectors ininternal/signature/golden_test.go. Note this is a breaking change to the URL signing scheme — any external signer must be updated in lockstep. Call that out in the PR body.q=85is rejected (or produces a different signature) when replayed withq=40.make checkgreen.Notes
/v1/e/encrypted-URL path is unaffected by this specific issue (the whole payload is sealed), but see the separate issue on/v1/e/input validation.Plan (option (a) — quality and fit inside the signed payload):
Add
Quality/FitModetosignature.Requestand extend the signed string by appending:quality:fitafter the expiration:host:path:query:width:height:format:expiration:quality:fitThe effective (post-default) values are signed. The handler already normalizes an omitted
qto85and an omittedfittocoverbefore signature verification, so a URL that omits them verifies against a signature over85/cover.TDD: a failing test first (a URL signed for
q=85is rejected when replayed withq=40), committed separately, then the fix.README signature spec updated (field list + worked example); PR first line states the breaking change and lists what an external signer must change.
One blocker I will surface rather than resolve myself: because the signed bytes change, the three pinned constants in the existing
internal/signature/golden_test.gocan no longer match. Updating them edits an existing test, which needs owner approval. Per my brief I will put the new known-answer vectors in a new test file, leavegolden_test.gounedited (somake checkis red on that one test), state the exact new constant values on the PR, and stop there for a decision.model: claude-opus-4-8
Decision requested (sneak). #116 implements option (a):
qualityandfitjoin the signed payload, so the signed-data format becomeshost:path:query:width:height:format:expiration:quality:fit. That necessarily invalidates the three pinned constants in the existinginternal/signature/golden_test.go, whose whole purpose is to pin the format; the review failed the PR only becausemake checkis red on that test, and changing an existing test needs your approval.Options:
golden_test.goto the new format (the new-format vectors are already added in a separate file by the PR). Recommended: the format change is the deliberate point of the fix, and the old vectors describe a scheme that let one signed URL authorize 500 unauthorized variants.q/fiton signature-authenticated requests with a 400. No existing test changes, no external signer changes, but the documented "exact match" guarantee stays narrower than it reads.Rework is dispatched as soon as you rule. If you take (1), a reply of "approved" here is enough.
model: claude-fable-5
clawbot referenced this issue2026-09-21 19:26:34 +02:00
Ruling recorded (sneak, 2026-09-21, on #116): "This is a greenfield project with no instances running. It's still a prototype. There is no breaking change anywhere. There are no URLs anywhere. No one has used this software yet."
Reading taken: option (1). Nothing depends on the old signed format, so the three pinned constants in
internal/signature/golden_test.goare updated to the new format, and the PR and README drop the breaking-change and external-signer-migration language. Rework of #116 proceeds on that basis.Model: fable-5-1
clawbot referenced this issue2026-09-28 13:02:34 +02:00