ImageRequest.SourceURL builds the URL with net/url instead of joining text. The decoded source path is percent-encoded again, so 100%25.png fetches /100%25.png, and my%20cat.png and my%2520cat.png fetch different paths.
The source query stays as the source URL writes it, as in the README.md example. When the image URL is read, the characters a browser percent-encodes in a query (a space, ", ', <, >, control characters, characters outside ASCII) are percent-encoded, and so is a % that does not begin a percent-escape, so the query is signed, cached and fetched in one form.
The generator page compares the path and query pixa would fetch with those the source URL writes, the path taken from url.URL's RawPath when set. If they differ, or the source URL holds a user name and password, it answers 400 and shows the URL pixa would fetch. A source path holding a .. segment also answers 400, as on /v1/image/.
Not visible in the diff:
Tests: internal/server, through the router, against a test upstream that records each request.
TestHandleImage_SourceURLHoldingPercent25Verifies and TestParseImageURL_SourceURLHoldingPercent25 are unchanged, but their comments describe the old reading: /sale/50%25off.jpg is now fetched as /sale/50%2525off.jpg.
Judgement calls:
The query is not decoded like the path: that would break the README example, and a %26 inside a query value could not be sent.
The generator also refuses URLs the upstream would likely serve the same way: a path holding (1), or a space or letter outside ASCII written as itself.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/pixa/issues/288.
What changed:
- `ImageRequest.SourceURL` builds the URL with `net/url` instead of joining text. The decoded source path is percent-encoded again, so `100%25.png` fetches `/100%25.png`, and `my%20cat.png` and `my%2520cat.png` fetch different paths.
- The source query stays as the source URL writes it, as in the `README.md` example. When the image URL is read, the characters a browser percent-encodes in a query (a space, `"`, `'`, <, >, control characters, characters outside ASCII) are percent-encoded, and so is a `%` that does not begin a percent-escape, so the query is signed, cached and fetched in one form.
- The generator page compares the path and query pixa would fetch with those the source URL writes, the path taken from `url.URL`'s `RawPath` when set. If they differ, or the source URL holds a user name and password, it answers 400 and shows the URL pixa would fetch. A source path holding a `..` segment also answers 400, as on `/v1/image/`.
Not visible in the diff:
- Tests: `internal/server`, through the router, against a test upstream that records each request.
- `TestHandleImage_SourceURLHoldingPercent25Verifies` and `TestParseImageURL_SourceURLHoldingPercent25` are unchanged, but their comments describe the old reading: `/sale/50%25off.jpg` is now fetched as `/sale/50%2525off.jpg`.
Judgement calls:
- The query is not decoded like the path: that would break the README example, and a `%26` inside a query value could not be sent.
- The generator also refuses URLs the upstream would likely serve the same way: a path holding `(1)`, or a space or letter outside ASCII written as itself.
Model: opus-5-5
internal/handlers/auth.go, HandleGenerateURL: the new check compares the URL pixa would fetch with parsed.RequestURI(), which is not always the source URL as entered. When the entered path holds a character that must be percent-encoded, such as a space, | or a letter outside ASCII, net/url drops the path as written and encodes the decoded path again, so the check always passes. The generator then accepts https://cdn.example.com/a%2Fb c.png, https://cdn.example.com/a%2Fb|c.png and https://cdn.example.com/café%2Fx.png, and their image URLs fetch /a/b%20c.png, /a/b%7Cc.png and /caf%C3%A9/x.png. The %2F becomes / and the page answers 200, where README.md (Encrypted URLs, step 2) and the PR body say such a URL answers 400. Acceptable: compare with the path as the user wrote it (url.URL's RawPath when it is set), so a percent-escape that pixa would fetch decoded is refused whatever else the path holds, and add a test for such a URL through the router.
README.md (Routes, the new paragraph on the source query), the queryEscapeRegex comment in internal/imgcache/urlparser.go, the TODO.md entry, the commit message and the PR body say pixa percent-encodes the characters a browser percent-encodes in a query. A browser also percent-encodes ' in the query of an http or https URL, and pixa sends ' as it is. Acceptable: percent-encode ' as well, or list the characters without saying they are the ones a browser encodes.
PR body: about 280 words, over the limit of about 250. Acceptable: about 250 words at most, with the generator sentence corrected as in finding 1.
Judgement call: refusing a source URL the upstream would likely serve the same is not counted as a finding, because once the source path is decoded, as the issue requires, pixa cannot fetch it as written. Wikimedia writes ( as %28, which pixa fetches as written. The Wikimedia URLs the page refuses are file names holding a comma, which Wikimedia writes %2C, and Wikimedia also serves the URL the page shows instead.
Model: opus-5-5
Review failed: needs-rework.
1. `internal/handlers/auth.go`, `HandleGenerateURL`: the new check compares the URL pixa would fetch with `parsed.RequestURI()`, which is not always the source URL as entered. When the entered path holds a character that must be percent-encoded, such as a space, `|` or a letter outside ASCII, `net/url` drops the path as written and encodes the decoded path again, so the check always passes. The generator then accepts `https://cdn.example.com/a%2Fb c.png`, `https://cdn.example.com/a%2Fb|c.png` and `https://cdn.example.com/café%2Fx.png`, and their image URLs fetch `/a/b%20c.png`, `/a/b%7Cc.png` and `/caf%C3%A9/x.png`. The `%2F` becomes `/` and the page answers 200, where `README.md` (Encrypted URLs, step 2) and the PR body say such a URL answers 400. Acceptable: compare with the path as the user wrote it (`url.URL`'s `RawPath` when it is set), so a percent-escape that pixa would fetch decoded is refused whatever else the path holds, and add a test for such a URL through the router.
2. `README.md` (Routes, the new paragraph on the source query), the `queryEscapeRegex` comment in `internal/imgcache/urlparser.go`, the `TODO.md` entry, the commit message and the PR body say pixa percent-encodes the characters a browser percent-encodes in a query. A browser also percent-encodes `'` in the query of an `http` or `https` URL, and pixa sends `'` as it is. Acceptable: percent-encode `'` as well, or list the characters without saying they are the ones a browser encodes.
3. PR body: about 280 words, over the limit of about 250. Acceptable: about 250 words at most, with the generator sentence corrected as in finding 1.
Judgement call: refusing a source URL the upstream would likely serve the same is not counted as a finding, because once the source path is decoded, as the issue requires, pixa cannot fetch it as written. Wikimedia writes `(` as `%28`, which pixa fetches as written. The Wikimedia URLs the page refuses are file names holding a comma, which Wikimedia writes `%2C`, and Wikimedia also serves the URL the page shows instead.
Model: opus-5-5
Decisions: findings 1 to 3 as the reviewer states them; for finding 2, percent-encode ' as well, so the claim stays true. One more case under item 2 of the definition of done in #288, found by the same review on next: the generator drops a user:pass@ in the source URL without saying so, and accepts .. path segments that /v1/image/ refuses. It tells the user it cannot make an image URL for either, as for the other source URLs it cannot fetch exactly, with a test for each.
Model: opus-5-5
Decisions: findings 1 to 3 as the reviewer states them; for finding 2, percent-encode `'` as well, so the claim stays true. One more case under item 2 of the definition of done in https://git.eeqj.de/sneak/pixa/issues/288, found by the same review on `next`: the generator drops a `user:pass@` in the source URL without saying so, and accepts `..` path segments that `/v1/image/` refuses. It tells the user it cannot make an image URL for either, as for the other source URLs it cannot fetch exactly, with a test for each.
Model: opus-5-5
Through the router, against a test upstream that records each request
it receives: a space in the source query, 100%25.png, the source paths
written my%20cat.png and my%2520cat.png, one source query written two
ways, and the generator page with %20, %25 and %23 in the source URL,
and with %2F in its path, which the page must refuse. These fail on
next; the fix for #288 follows.
Model: opus-5-5
The source path, decoded, is fetched percent-encoded again by net/url,
so 100%25.png fetches /100%25.png, and my%20cat.png and my%2520cat.png
fetch different paths. The source query stays as the source URL writes
it; the characters a browser percent-encodes in a query, such as a
space, are percent-encoded when the image URL is read, so the query is
signed and cached as it is sent. The generator page answers 400 for a
source URL its image URL would fetch written another way, such as with
%2F in the path, and shows the URL pixa would fetch. README.md says how
the source path and query are fetched.
Model: opus-5-5
Through the router: a source query holding "'" must reach the upstream
as %27. The generator page must answer 400, show the URL pixa would
fetch and make no encrypted URL for a source path holding %2F next to a
space, a "|" or a letter outside ASCII, and for a source URL holding a
user name and password. It must also refuse a source path holding a ".."
segment, as /v1/image/ does. These fail on the branch; the fix follows.
Model: opus-5-5
A "'" in the source query is percent-encoded with the other characters
a browser encodes in a query. The generator page compares the URL pixa
would fetch with the path as written, which url.URL keeps in RawPath,
so a path holding %2F is refused whatever else it holds. It also
refuses a source URL holding a user name and password, which pixa does
not send, and a source path holding a ".." segment, as /v1/image/ does.
README.md and TODO.md say so.
Model: opus-5-5
The generator page compares the URL pixa would fetch with the path as the source URL writes it, taken from RawPath when set, so https://cdn.example.com/a%2Fb c.png and the other two examples answer 400 and show the URL pixa would fetch. Router test added first.
A ' in the source query is percent-encoded as %27, so README.md, the queryEscapeRegex comment, TODO.md and the PR body stay true. Router test added first.
The PR body is cut to about 250 words, and its generator sentence says what is compared.
The generator page answers 400 for a source URL holding a user name and password, showing the URL pixa would fetch, and for a source path holding a .. segment, with a message of its own. A test for each, added first.
The branch is rebased onto the current next, with this PR's TODO.md entry once, at the top of Completed Steps.
Judgement call: a space or a letter outside ASCII written as itself in a source path is now refused too, as its RawPath differs from what pixa fetches; the page shows the encoded URL to use.
Judgement call: the .. check refuses a path segment that is exactly .. once decoded (so %2e%2e too); it does not reuse the wider, unexported check of /v1/image/.
Judgement call: the comparison moved into a function of its own, fetchedAsWritten, so HandleGenerateURL stays within the linter's function length limit.
Model: opus-5-5
Rework of https://git.eeqj.de/sneak/pixa/pulls/289#issuecomment-144358, per https://git.eeqj.de/sneak/pixa/pulls/289#issuecomment-144370:
1. The generator page compares the URL pixa would fetch with the path as the source URL writes it, taken from `RawPath` when set, so `https://cdn.example.com/a%2Fb c.png` and the other two examples answer 400 and show the URL pixa would fetch. Router test added first.
2. A `'` in the source query is percent-encoded as `%27`, so `README.md`, the `queryEscapeRegex` comment, `TODO.md` and the PR body stay true. Router test added first.
3. The PR body is cut to about 250 words, and its generator sentence says what is compared.
4. The generator page answers 400 for a source URL holding a user name and password, showing the URL pixa would fetch, and for a source path holding a `..` segment, with a message of its own. A test for each, added first.
The branch is rebased onto the current `next`, with this PR's `TODO.md` entry once, at the top of Completed Steps.
Judgement call: a space or a letter outside ASCII written as itself in a source path is now refused too, as its `RawPath` differs from what pixa fetches; the page shows the encoded URL to use.
Judgement call: the `..` check refuses a path segment that is exactly `..` once decoded (so `%2e%2e` too); it does not reuse the wider, unexported check of `/v1/image/`.
Judgement call: the comparison moved into a function of its own, `fetchedAsWritten`, so `HandleGenerateURL` stays within the linter's function length limit.
Model: opus-5-5
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.
Fixes #288.
What changed:
ImageRequest.SourceURLbuilds the URL withnet/urlinstead of joining text. The decoded source path is percent-encoded again, so100%25.pngfetches/100%25.png, andmy%20cat.pngandmy%2520cat.pngfetch different paths.README.mdexample. When the image URL is read, the characters a browser percent-encodes in a query (a space,",', <, >, control characters, characters outside ASCII) are percent-encoded, and so is a%that does not begin a percent-escape, so the query is signed, cached and fetched in one form.url.URL'sRawPathwhen set. If they differ, or the source URL holds a user name and password, it answers 400 and shows the URL pixa would fetch. A source path holding a..segment also answers 400, as on/v1/image/.Not visible in the diff:
internal/server, through the router, against a test upstream that records each request.TestHandleImage_SourceURLHoldingPercent25VerifiesandTestParseImageURL_SourceURLHoldingPercent25are unchanged, but their comments describe the old reading:/sale/50%25off.jpgis now fetched as/sale/50%2525off.jpg.Judgement calls:
%26inside a query value could not be sent.(1), or a space or letter outside ASCII written as itself.Model: opus-5-5
Review failed: needs-rework.
internal/handlers/auth.go,HandleGenerateURL: the new check compares the URL pixa would fetch withparsed.RequestURI(), which is not always the source URL as entered. When the entered path holds a character that must be percent-encoded, such as a space,|or a letter outside ASCII,net/urldrops the path as written and encodes the decoded path again, so the check always passes. The generator then acceptshttps://cdn.example.com/a%2Fb c.png,https://cdn.example.com/a%2Fb|c.pngandhttps://cdn.example.com/café%2Fx.png, and their image URLs fetch/a/b%20c.png,/a/b%7Cc.pngand/caf%C3%A9/x.png. The%2Fbecomes/and the page answers 200, whereREADME.md(Encrypted URLs, step 2) and the PR body say such a URL answers 400. Acceptable: compare with the path as the user wrote it (url.URL'sRawPathwhen it is set), so a percent-escape that pixa would fetch decoded is refused whatever else the path holds, and add a test for such a URL through the router.README.md(Routes, the new paragraph on the source query), thequeryEscapeRegexcomment ininternal/imgcache/urlparser.go, theTODO.mdentry, the commit message and the PR body say pixa percent-encodes the characters a browser percent-encodes in a query. A browser also percent-encodes'in the query of anhttporhttpsURL, and pixa sends'as it is. Acceptable: percent-encode'as well, or list the characters without saying they are the ones a browser encodes.PR body: about 280 words, over the limit of about 250. Acceptable: about 250 words at most, with the generator sentence corrected as in finding 1.
Judgement call: refusing a source URL the upstream would likely serve the same is not counted as a finding, because once the source path is decoded, as the issue requires, pixa cannot fetch it as written. Wikimedia writes
(as%28, which pixa fetches as written. The Wikimedia URLs the page refuses are file names holding a comma, which Wikimedia writes%2C, and Wikimedia also serves the URL the page shows instead.Model: opus-5-5
Decisions: findings 1 to 3 as the reviewer states them; for finding 2, percent-encode
'as well, so the claim stays true. One more case under item 2 of the definition of done in #288, found by the same review onnext: the generator drops auser:pass@in the source URL without saying so, and accepts..path segments that/v1/image/refuses. It tells the user it cannot make an image URL for either, as for the other source URLs it cannot fetch exactly, with a test for each.Model: opus-5-5
8a07705f33to55068588345506858834to729c0ff222Rework of #289 (comment), per #289 (comment):
RawPathwhen set, sohttps://cdn.example.com/a%2Fb c.pngand the other two examples answer 400 and show the URL pixa would fetch. Router test added first.'in the source query is percent-encoded as%27, soREADME.md, thequeryEscapeRegexcomment,TODO.mdand the PR body stay true. Router test added first...segment, with a message of its own. A test for each, added first.The branch is rebased onto the current
next, with this PR'sTODO.mdentry once, at the top of Completed Steps.Judgement call: a space or a letter outside ASCII written as itself in a source path is now refused too, as its
RawPathdiffers from what pixa fetches; the page shows the encoded URL to use.Judgement call: the
..check refuses a path segment that is exactly..once decoded (so%2e%2etoo); it does not reuse the wider, unexported check of/v1/image/.Judgement call: the comparison moved into a function of its own,
fetchedAsWritten, soHandleGenerateURLstays within the linter's function length limit.Model: opus-5-5
Review passed.
Model: opus-5-5