imagerequest.Request holds only Source and Transform, what the variant key covers. SourceURL() became Source.URL(allowHTTP), which returns a *url.URL.
The /v1/image/ handler checks allowlist_hosts and calls Signer.Verify(req, sig, expires) itself, in validateSignature. handlers.New builds the signer and the allowlist from the config, as it builds the Referer blocklist.
Both image handlers keep the URL's expiry as a local value for Cache-Control. encurl's Generate takes it and Parse returns it.
The image service has no signer, allowlist or AllowHTTP. It serves images through Get, and has WaitForProcessing for shutdown.
The fetcher takes the source and builds the URL from its own AllowHTTP. validateURL takes a *url.URL, so a redirect target is not parsed again either.
chooseAutoFormat returns the format it chooses instead of writing into the request.
Not visible in the diff
validateURL lost its branch for a URL string that does not parse. Every URL it gets is now built by Source.URL or parsed by net/http. The localhost, address and dialer checks are unchanged.
The tests of ValidateRequest moved to internal/handlers/validate_signature_internal_test.go with the same assertions. A new fetcher test checks that the scheme follows AllowHTTP.
Disclosures
Judgement call: chooseAutoFormat changed too, since it wrote into the request it was given.
Judgement call: the "fetching from upstream" debug log now logs the host, path and query instead of the URL.
Model: opus-5-5
Implements https://git.eeqj.de/sneak/pixa/issues/314, item 8 of https://git.eeqj.de/sneak/pixa/issues/168.
## What changed
- `imagerequest.Request` holds only `Source` and `Transform`, what the variant key covers. `SourceURL()` became `Source.URL(allowHTTP)`, which returns a `*url.URL`.
- The `/v1/image/` handler checks `allowlist_hosts` and calls `Signer.Verify(req, sig, expires)` itself, in `validateSignature`. `handlers.New` builds the signer and the allowlist from the config, as it builds the Referer blocklist.
- Both image handlers keep the URL's expiry as a local value for `Cache-Control`. `encurl`'s `Generate` takes it and `Parse` returns it.
- The image service has no signer, allowlist or `AllowHTTP`. It serves images through `Get`, and has `WaitForProcessing` for shutdown.
- The fetcher takes the source and builds the URL from its own `AllowHTTP`. `validateURL` takes a `*url.URL`, so a redirect target is not parsed again either.
- `chooseAutoFormat` returns the format it chooses instead of writing into the request.
## Not visible in the diff
- `validateURL` lost its branch for a URL string that does not parse. Every URL it gets is now built by `Source.URL` or parsed by `net/http`. The localhost, address and dialer checks are unchanged.
- The tests of `ValidateRequest` moved to `internal/handlers/validate_signature_internal_test.go` with the same assertions. A new fetcher test checks that the scheme follows `AllowHTTP`.
## Disclosures
- Judgement call: `chooseAutoFormat` changed too, since it wrote into the request it was given.
- Judgement call: the "fetching from upstream" debug log now logs the host, path and query instead of the URL.
Model: opus-5-5
internal/imagerequest/urlparser.go: the comments above the host check (lines 129–131) and the # check (lines 150–152) are now false. They justify those rules by saying Go's URL parser, "which the allowlist check and the fetcher use" (or "which the fetcher uses"), would misread the source. After this change neither the allowlist check nor the fetcher parses the source. validateSignature builds a url.URL from the host, and the fetcher builds its URL with Source.URL. Next to a security check, these comments describe a parse step that no longer exists. Acceptable: each comment gives the reason that still holds in one short, correct sentence, or drops the clause that names the parser.
internal/handlers/image_signature_internal_test.go: generateSignedURL (lines 45–54) still sets Quality and FitMode on the request it is given. It keeps the write-back that #314 names for Service.GenerateSignedURL, and the definition of done says nothing writes back into a request it was given. Acceptable: the helper puts the defaults into its own copy of the request and leaves the caller's request unchanged.
Judgement call: finding 2 reads "nothing" in the definition of done as covering test helpers too.
Model: opus-5-5
**FAIL: needs rework**
1. `internal/imagerequest/urlparser.go`: the comments above the host check (lines 129–131) and the `#` check (lines 150–152) are now false. They justify those rules by saying Go's URL parser, "which the allowlist check and the fetcher use" (or "which the fetcher uses"), would misread the source. After this change neither the allowlist check nor the fetcher parses the source. `validateSignature` builds a `url.URL` from the host, and the fetcher builds its URL with `Source.URL`. Next to a security check, these comments describe a parse step that no longer exists. Acceptable: each comment gives the reason that still holds in one short, correct sentence, or drops the clause that names the parser.
2. `internal/handlers/image_signature_internal_test.go`: `generateSignedURL` (lines 45–54) still sets `Quality` and `FitMode` on the request it is given. It keeps the write-back that https://git.eeqj.de/sneak/pixa/issues/314 names for `Service.GenerateSignedURL`, and the definition of done says nothing writes back into a request it was given. Acceptable: the helper puts the defaults into its own copy of the request and leaves the caller's request unchanged.
Judgement call: finding 2 reads "nothing" in the definition of done as covering test helpers too.
Model: opus-5-5
Both comments in internal/imagerequest/urlparser.go now give a reason that names no parser. The host check says later steps put the host unparsed into the allowlist check, the cache key and the upstream URL. The # check says a # starts a URL's fragment.
generateSignedURL now puts the defaults into its own copy of the request and signs that copy. The caller's request is left unchanged.
Model: opus-5-5
1. Both comments in `internal/imagerequest/urlparser.go` now give a reason that names no parser. The host check says later steps put the host unparsed into the allowlist check, the cache key and the upstream URL. The `#` check says a `#` starts a URL's fragment.
2. `generateSignedURL` now puts the defaults into its own copy of the request and signs that copy. The caller's request is left unchanged.
Model: opus-5-5
The doc comments of TestHandleImage_InvalidHost_Returns400 and TestHandleImage_HashInSourcePathOrQuery_Returns400 now give the reasons the comments in internal/imagerequest/urlparser.go give, and no longer say a ? or # would end the host or source URL.
Model: opus-5-5
The doc comments of `TestHandleImage_InvalidHost_Returns400` and `TestHandleImage_HashInSourcePathOrQuery_Returns400` now give the reasons the comments in `internal/imagerequest/urlparser.go` give, and no longer say a `?` or `#` would end the host or source URL.
Model: opus-5-5
internal/handlers/format_auto_internal_test.go lines 78–79 and internal/handlers/request_id_internal_test.go lines 20–21: both doc comments call testSigningKey "the key of the image service newSignedHostServer builds". This change takes the key out of the image service; newSignedHostServer now gives it to the handlers' signer. Acceptable: each comment names it as the key the handlers that newSignedHostServer builds check signatures with.
internal/imgcache/testutil_internal_test.go lines 34–38 and 46–47 call GoodHost and its files "allowlisted" and OtherHost and its files "non-allowlisted", and TestService_Get_AllowlistedHost (internal/imgcache/service_internal_test.go line 20) is named for an allowlist. SetupTestService no longer sets one and the image service has none, so these describe something that no longer exists in this package. Acceptable: the comments and the test name describe the hosts without an allowlist, for example as two hosts the mock fetcher serves.
internal/imagerequest/urlparser.go line 136: the bracket check still gives as its reason that Go's URL parser refuses brackets around anything but an IPv6 address. After this change no step parses the host with Go's URL parser, as the rewritten comment above it says, so that reason no longer holds. The check still matters: without it, [203.0.113.5] would pass the allowlist entry 203.0.113.5 and be fetched from that address under a cache key of its own. Acceptable: one short sentence giving the reason that holds.
Judgement call: finding 3 is on a line this change does not touch; its reason went stale with the fetcher change.
Model: opus-5-5
**FAIL: needs rework**
1. `internal/handlers/format_auto_internal_test.go` lines 78–79 and `internal/handlers/request_id_internal_test.go` lines 20–21: both doc comments call `testSigningKey` "the key of the image service newSignedHostServer builds". This change takes the key out of the image service; `newSignedHostServer` now gives it to the handlers' signer. Acceptable: each comment names it as the key the handlers that `newSignedHostServer` builds check signatures with.
2. `internal/imgcache/testutil_internal_test.go` lines 34–38 and 46–47 call `GoodHost` and its files "allowlisted" and `OtherHost` and its files "non-allowlisted", and `TestService_Get_AllowlistedHost` (`internal/imgcache/service_internal_test.go` line 20) is named for an allowlist. `SetupTestService` no longer sets one and the image service has none, so these describe something that no longer exists in this package. Acceptable: the comments and the test name describe the hosts without an allowlist, for example as two hosts the mock fetcher serves.
3. `internal/imagerequest/urlparser.go` line 136: the bracket check still gives as its reason that Go's URL parser refuses brackets around anything but an IPv6 address. After this change no step parses the host with Go's URL parser, as the rewritten comment above it says, so that reason no longer holds. The check still matters: without it, `[203.0.113.5]` would pass the allowlist entry `203.0.113.5` and be fetched from that address under a cache key of its own. Acceptable: one short sentence giving the reason that holds.
Judgement call: finding 3 is on a line this change does not touch; its reason went stale with the fetcher change.
Model: opus-5-5
The image request now holds only its source and its transform, exactly what
the variant key covers. The /v1/image/ handler checks the allowlist and
verifies the signature itself with Signer.Verify(req, sig, expires). Both
image handlers keep the URL's expiry as a local value for Cache-Control. The
image service holds no signer, allowlist or AllowHTTP, and ValidateRequest is
gone. The fetcher takes the source and builds its URL with Source.URL from its
own AllowHTTP, so no URL string is built and parsed again. chooseAutoFormat
returns the format it chooses instead of writing into the request. The signed
URL format and every route's answers are unchanged.
Model: opus-5-5
The doc comments of autoPhotoURLs and signedPhotoURL now call testSigningKey the key the handlers that newSignedHostServer builds check signatures with.
The TestFixtures comments now describe GoodHost and OtherHost as two hosts the mock fetcher serves. The two tests are now TestService_Get_GoodHost and TestService_Get_OtherHost, and the second one's doc comment no longer mentions an allowlist.
The bracket check's comment now says that the allowlist check and the fetcher drop the brackets, so brackets around anything but an IPv6 address would give a host a second spelling.
Two more comments that this change had made false are fixed: photoVariant no longer says Get writes to its request, and newPhotoFetcher now answers "any other source" instead of "any other URL".
Model: opus-5-5
1. The doc comments of `autoPhotoURLs` and `signedPhotoURL` now call `testSigningKey` the key the handlers that `newSignedHostServer` builds check signatures with.
2. The `TestFixtures` comments now describe `GoodHost` and `OtherHost` as two hosts the mock fetcher serves. The two tests are now `TestService_Get_GoodHost` and `TestService_Get_OtherHost`, and the second one's doc comment no longer mentions an allowlist.
3. The bracket check's comment now says that the allowlist check and the fetcher drop the brackets, so brackets around anything but an IPv6 address would give a host a second spelling.
Two more comments that this change had made false are fixed: `photoVariant` no longer says `Get` writes to its request, and `newPhotoFetcher` now answers "any other source" instead of "any other URL".
Model: opus-5-5
PASS: the image request now holds only its source and transform, the /v1/image/ handler checks the allowlist and verifies the signature itself, the fetcher builds its own URL with every check it had before, and the stale comments and test helper from the earlier reviews are fixed.
Model: opus-5-5
PASS: the image request now holds only its source and transform, the `/v1/image/` handler checks the allowlist and verifies the signature itself, the fetcher builds its own URL with every check it had before, and the stale comments and test helper from the earlier reviews are fixed.
Model: opus-5-5
clawbot
merged commit 7b6712538e into next2026-10-26 13:49:13 +01: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.
Implements #314, item 8 of #168.
What changed
imagerequest.Requestholds onlySourceandTransform, what the variant key covers.SourceURL()becameSource.URL(allowHTTP), which returns a*url.URL./v1/image/handler checksallowlist_hostsand callsSigner.Verify(req, sig, expires)itself, invalidateSignature.handlers.Newbuilds the signer and the allowlist from the config, as it builds the Referer blocklist.Cache-Control.encurl'sGeneratetakes it andParsereturns it.AllowHTTP. It serves images throughGet, and hasWaitForProcessingfor shutdown.AllowHTTP.validateURLtakes a*url.URL, so a redirect target is not parsed again either.chooseAutoFormatreturns the format it chooses instead of writing into the request.Not visible in the diff
validateURLlost its branch for a URL string that does not parse. Every URL it gets is now built bySource.URLor parsed bynet/http. The localhost, address and dialer checks are unchanged.ValidateRequestmoved tointernal/handlers/validate_signature_internal_test.gowith the same assertions. A new fetcher test checks that the scheme followsAllowHTTP.Disclosures
chooseAutoFormatchanged too, since it wrote into the request it was given.Model: opus-5-5
FAIL: needs rework
internal/imagerequest/urlparser.go: the comments above the host check (lines 129–131) and the#check (lines 150–152) are now false. They justify those rules by saying Go's URL parser, "which the allowlist check and the fetcher use" (or "which the fetcher uses"), would misread the source. After this change neither the allowlist check nor the fetcher parses the source.validateSignaturebuilds aurl.URLfrom the host, and the fetcher builds its URL withSource.URL. Next to a security check, these comments describe a parse step that no longer exists. Acceptable: each comment gives the reason that still holds in one short, correct sentence, or drops the clause that names the parser.internal/handlers/image_signature_internal_test.go:generateSignedURL(lines 45–54) still setsQualityandFitModeon the request it is given. It keeps the write-back that #314 names forService.GenerateSignedURL, and the definition of done says nothing writes back into a request it was given. Acceptable: the helper puts the defaults into its own copy of the request and leaves the caller's request unchanged.Judgement call: finding 2 reads "nothing" in the definition of done as covering test helpers too.
Model: opus-5-5
08028e4560tocb3439da3dinternal/imagerequest/urlparser.gonow give a reason that names no parser. The host check says later steps put the host unparsed into the allowlist check, the cache key and the upstream URL. The#check says a#starts a URL's fragment.generateSignedURLnow puts the defaults into its own copy of the request and signs that copy. The caller's request is left unchanged.Model: opus-5-5
cb3439da3dto8a03d0dbe7The doc comments of
TestHandleImage_InvalidHost_Returns400andTestHandleImage_HashInSourcePathOrQuery_Returns400now give the reasons the comments ininternal/imagerequest/urlparser.gogive, and no longer say a?or#would end the host or source URL.Model: opus-5-5
FAIL: needs rework
internal/handlers/format_auto_internal_test.golines 78–79 andinternal/handlers/request_id_internal_test.golines 20–21: both doc comments calltestSigningKey"the key of the image service newSignedHostServer builds". This change takes the key out of the image service;newSignedHostServernow gives it to the handlers' signer. Acceptable: each comment names it as the key the handlers thatnewSignedHostServerbuilds check signatures with.internal/imgcache/testutil_internal_test.golines 34–38 and 46–47 callGoodHostand its files "allowlisted" andOtherHostand its files "non-allowlisted", andTestService_Get_AllowlistedHost(internal/imgcache/service_internal_test.goline 20) is named for an allowlist.SetupTestServiceno longer sets one and the image service has none, so these describe something that no longer exists in this package. Acceptable: the comments and the test name describe the hosts without an allowlist, for example as two hosts the mock fetcher serves.internal/imagerequest/urlparser.goline 136: the bracket check still gives as its reason that Go's URL parser refuses brackets around anything but an IPv6 address. After this change no step parses the host with Go's URL parser, as the rewritten comment above it says, so that reason no longer holds. The check still matters: without it,[203.0.113.5]would pass the allowlist entry203.0.113.5and be fetched from that address under a cache key of its own. Acceptable: one short sentence giving the reason that holds.Judgement call: finding 3 is on a line this change does not touch; its reason went stale with the fetcher change.
Model: opus-5-5
8a03d0dbe7to2670d9002fautoPhotoURLsandsignedPhotoURLnow calltestSigningKeythe key the handlers thatnewSignedHostServerbuilds check signatures with.TestFixturescomments now describeGoodHostandOtherHostas two hosts the mock fetcher serves. The two tests are nowTestService_Get_GoodHostandTestService_Get_OtherHost, and the second one's doc comment no longer mentions an allowlist.Two more comments that this change had made false are fixed:
photoVariantno longer saysGetwrites to its request, andnewPhotoFetchernow answers "any other source" instead of "any other URL".Model: opus-5-5
PASS: the image request now holds only its source and transform, the
/v1/image/handler checks the allowlist and verifies the signature itself, the fetcher builds its own URL with every check it had before, and the stale comments and test helper from the earlier reviews are fixed.Model: opus-5-5