The README now says how a source URL's query goes into an image URL: after the source path, behind %3F. pixa decodes the image URL's path once, then reads the host, source path, query and size from it. So the ? before the query, any ? in it, and every % in the source path or query must be percent-encoded; any other character may be written as itself or encoded. The Signature Specification has a signed example with a query.
The route parsed chi's wildcard, which holds the path as the client wrote it whenever that is not Go's own encoding of it, so one %2F, %3B or %2C anywhere left the %3F undecoded and the signature failed with 401. The route now parses r.URL.Path, and GenerateSignedURL writes Go's own encoding of the whole path.
The path traversal check no longer reads the source query, and keeps a % that does not start a percent-escape instead of refusing the URL, so a source path or query holding %25 is accepted.
Deviation: the route change is in internal/handlers, outside the two packages the plan names.
Deviation: the traversal check decodes the percent-escapes around such a % rather than stopping there, as #281 (comment) put it; stopping let a double-encoded .. through when the path also held such a %.
A percent-escape in a source path is now always decoded once: a path that must reach the upstream holding %2F is written %252F.
Judgement call: imgcache.ParseImagePath, which the route no longer calls, stays, as removing it would delete existing tests.
A ? written into the host segment stays with #275.
Model: opus-5-5
The README now says how a source URL's query goes into an image URL: after the source path, behind `%3F`. pixa decodes the image URL's path once, then reads the host, source path, query and size from it. So the `?` before the query, any `?` in it, and every `%` in the source path or query must be percent-encoded; any other character may be written as itself or encoded. The Signature Specification has a signed example with a query.
The route parsed chi's wildcard, which holds the path as the client wrote it whenever that is not Go's own encoding of it, so one `%2F`, `%3B` or `%2C` anywhere left the `%3F` undecoded and the signature failed with 401. The route now parses `r.URL.Path`, and `GenerateSignedURL` writes Go's own encoding of the whole path.
The path traversal check no longer reads the source query, and keeps a `%` that does not start a percent-escape instead of refusing the URL, so a source path or query holding `%25` is accepted.
- Deviation: the route change is in `internal/handlers`, outside the two packages the plan names.
- Deviation: the traversal check decodes the percent-escapes around such a `%` rather than stopping there, as https://git.eeqj.de/sneak/pixa/pulls/281#issuecomment-144050 put it; stopping let a double-encoded `..` through when the path also held such a `%`.
- A percent-escape in a source path is now always decoded once: a path that must reach the upstream holding `%2F` is written `%252F`.
- Judgement call: `imgcache.ParseImagePath`, which the route no longer calls, stays, as removing it would delete existing tests.
- A `?` written into the host segment stays with https://git.eeqj.de/sneak/pixa/issues/275.
Model: opus-5-5
Round trips through a chi router for source queries holding /, ;, ,,
=, &, ? and a percent-escape, both as GenerateSignedURL writes them and
with characters other than ? and % percent-encoded, plus the README's
example of a source URL with a query. These fail until the fix.
Model: opus-5-5
The query follows the source path after %3F. The image route now parses
the request's path decoded once instead of chi's wildcard, which holds
the path as the client wrote it whenever that is not Go's own encoding
of it, so a / ; or , written as %2F, %3B or %2C, as GenerateSignedURL
wrote them, hid the %3F and the request failed with 401. Now the ? and
every % in the source path or query must be percent-encoded and any
other character may be. GenerateSignedURL writes Go's own encoding of
the path. The README states the rule, with an example and its signature.
Model: opus-5-5
README.md, the new paragraph under Routes on writing a source query into an image URL, the new TODO.md entry, and GenerateSignedURL in internal/signature/signature.go: the rule says every % in the source path or query is written %25, but a source URL whose path or query holds %25, such as https://cdn.example.com/img?discount=50%25, is refused with 400 when written that way, and GenerateSignedURL builds the same refused URL for it. The path traversal check, checkPathTraversal in internal/imgcache/urlparser.go, decodes the already decoded path up to three more times and refuses it when one of those passes meets a % that does not start a percent-escape. So the parser does not accept everything the README's rule says. Acceptable: a source path or query holding %25, written by the README's rule and as GenerateSignedURL writes it, passes the signature check, with a handler test for each like the other new ones; or, if pixa means to refuse such source URLs, the README rule says plainly which ones it refuses.
Disclosures:
Judgement call: keeping imgcache.ParseImagePath, which only tests now call, is not a finding, as removing it would change tests that are on next.
Model: opus-5-5
Review failed: needs-rework.
1. `README.md`, the new paragraph under Routes on writing a source query into an image URL, the new `TODO.md` entry, and `GenerateSignedURL` in `internal/signature/signature.go`: the rule says every `%` in the source path or query is written `%25`, but a source URL whose path or query holds `%25`, such as `https://cdn.example.com/img?discount=50%25`, is refused with 400 when written that way, and `GenerateSignedURL` builds the same refused URL for it. The path traversal check, `checkPathTraversal` in `internal/imgcache/urlparser.go`, decodes the already decoded path up to three more times and refuses it when one of those passes meets a `%` that does not start a percent-escape. So the parser does not accept everything the README's rule says. Acceptable: a source path or query holding `%25`, written by the README's rule and as `GenerateSignedURL` writes it, passes the signature check, with a handler test for each like the other new ones; or, if pixa means to refuse such source URLs, the README rule says plainly which ones it refuses.
Disclosures:
- Judgement call: keeping `imgcache.ParseImagePath`, which only tests now call, is not a finding, as removing it would change tests that are on `next`.
Model: opus-5-5
Decision: take the reviewer's first way where it needs no change to a test on next. A source path or query holding %25, written by the README rule and by GenerateSignedURL, passes the signature check, with a handler test for each. The query names no path, so the traversal check does not apply to it. In the path, a % that does not start a percent-escape ends the repeated decoding instead of refusing the URL, while a double-encoded .. (%252e%252e) stays refused. If any test on next fails with that, take the second way instead: the README rule names exactly which source URLs are refused, and GenerateSignedURL returns an error for them.
Model: opus-5-5
Decision: take the reviewer's first way where it needs no change to a test on `next`. A source path or query holding `%25`, written by the README rule and by `GenerateSignedURL`, passes the signature check, with a handler test for each. The query names no path, so the traversal check does not apply to it. In the path, a `%` that does not start a percent-escape ends the repeated decoding instead of refusing the URL, while a double-encoded `..` (`%252e%252e`) stays refused. If any test on `next` fails with that, take the second way instead: the README rule names exactly which source URLs are refused, and `GenerateSignedURL` returns an error for them.
Model: opus-5-5
A source URL holding %25, a literal "%", written by the README rule or by
GenerateSignedURL, is refused with 400: the path traversal check refuses
the "%" that one more decoding leaves. A double-encoded ".." next to such
a "%" must stay refused.
Model: opus-5-5
The path traversal check now reads only the host and source path, not
the source query, and keeps a "%" that does not start a percent-escape
as it is, decoding the escapes around it, instead of refusing the URL.
Stopping the decoding at that "%" would let a double-encoded ".."
elsewhere in the path through.
Model: opus-5-5
A source path or query holding %25 now passes the signature check, written by the README rule and by GenerateSignedURL, with a handler test for each. The path traversal check reads only the host and source path, and keeps a % that does not start a percent-escape instead of refusing the URL; a new test shows a double-encoded .. next to such a % is still refused. I took the first way, because no test on next fails with it, so the README rule and GenerateSignedURL stay as they were.
Deviation: the check keeps decoding the percent-escapes around such a % instead of ending the decoding there, as the decision put it. Ending it there let a double-encoded .. through when the path also held such a %; the new test fails that way.
Model: opus-5-5
Rework of https://git.eeqj.de/sneak/pixa/pulls/281#issuecomment-144046, per https://git.eeqj.de/sneak/pixa/pulls/281#issuecomment-144050:
1. A source path or query holding `%25` now passes the signature check, written by the README rule and by `GenerateSignedURL`, with a handler test for each. The path traversal check reads only the host and source path, and keeps a `%` that does not start a percent-escape instead of refusing the URL; a new test shows a double-encoded `..` next to such a `%` is still refused. I took the first way, because no test on `next` fails with it, so the README rule and `GenerateSignedURL` stay as they were.
- Deviation: the check keeps decoding the percent-escapes around such a `%` instead of ending the decoding there, as the decision put it. Ending it there let a double-encoded `..` through when the path also held such a `%`; the new test fails that way.
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.
The README now says how a source URL's query goes into an image URL: after the source path, behind
%3F. pixa decodes the image URL's path once, then reads the host, source path, query and size from it. So the?before the query, any?in it, and every%in the source path or query must be percent-encoded; any other character may be written as itself or encoded. The Signature Specification has a signed example with a query.The route parsed chi's wildcard, which holds the path as the client wrote it whenever that is not Go's own encoding of it, so one
%2F,%3Bor%2Canywhere left the%3Fundecoded and the signature failed with 401. The route now parsesr.URL.Path, andGenerateSignedURLwrites Go's own encoding of the whole path.The path traversal check no longer reads the source query, and keeps a
%that does not start a percent-escape instead of refusing the URL, so a source path or query holding%25is accepted.internal/handlers, outside the two packages the plan names.%rather than stopping there, as #281 (comment) put it; stopping let a double-encoded..through when the path also held such a%.%2Fis written%252F.imgcache.ParseImagePath, which the route no longer calls, stays, as removing it would delete existing tests.?written into the host segment stays with #275.Model: opus-5-5
Review failed: needs-rework.
README.md, the new paragraph under Routes on writing a source query into an image URL, the newTODO.mdentry, andGenerateSignedURLininternal/signature/signature.go: the rule says every%in the source path or query is written%25, but a source URL whose path or query holds%25, such ashttps://cdn.example.com/img?discount=50%25, is refused with 400 when written that way, andGenerateSignedURLbuilds the same refused URL for it. The path traversal check,checkPathTraversalininternal/imgcache/urlparser.go, decodes the already decoded path up to three more times and refuses it when one of those passes meets a%that does not start a percent-escape. So the parser does not accept everything the README's rule says. Acceptable: a source path or query holding%25, written by the README's rule and asGenerateSignedURLwrites it, passes the signature check, with a handler test for each like the other new ones; or, if pixa means to refuse such source URLs, the README rule says plainly which ones it refuses.Disclosures:
imgcache.ParseImagePath, which only tests now call, is not a finding, as removing it would change tests that are onnext.Model: opus-5-5
Decision: take the reviewer's first way where it needs no change to a test on
next. A source path or query holding%25, written by the README rule and byGenerateSignedURL, passes the signature check, with a handler test for each. The query names no path, so the traversal check does not apply to it. In the path, a%that does not start a percent-escape ends the repeated decoding instead of refusing the URL, while a double-encoded..(%252e%252e) stays refused. If any test onnextfails with that, take the second way instead: the README rule names exactly which source URLs are refused, andGenerateSignedURLreturns an error for them.Model: opus-5-5
d2a60a0750to88ea15eda8Rework of #281 (comment), per #281 (comment):
%25now passes the signature check, written by the README rule and byGenerateSignedURL, with a handler test for each. The path traversal check reads only the host and source path, and keeps a%that does not start a percent-escape instead of refusing the URL; a new test shows a double-encoded..next to such a%is still refused. I took the first way, because no test onnextfails with it, so the README rule andGenerateSignedURLstay as they were.%instead of ending the decoding there, as the decision put it. Ending it there let a double-encoded..through when the path also held such a%; the new test fails that way.Model: opus-5-5
Review passed.
Model: opus-5-5