Use AnyP::Uri::Decode() for urlpath checks

[kinkie]

No description provided.

[kinkie]

This was runtime-tested as well, with configuration:

acl test1 urlpath_regex ^/foo
acl test2 urlpath_regex ^/bar
http_access allow test1
http_access deny test2

and commands

$ http_proxy=http://localhost:3128/ curl -v http://www.squid-cache.org/bar
$ http_proxy=http://localhost:3128/ curl -v http://www.squid-cache.org/foo

[rousskov]

What happens when url.path() contains an invalid encoding? Will this Decode() call throw or do we filter out those cases elsewhere? If this Decode() call throws, how does the overall transaction outcome differ compared to the outcome produced by official code?

[yadij]

In the current code the exception will jump out to the exception handler for the async Dialer, terminating whichever access checks were being performed and the transaction.

[kinkie]

Good question!
With this code in, the transaction is aborted. What current-squid does is try to interpret it let it pass, so that then Apache can throw a 400 bad request error. I guess what squid SHOULD do is just return a 400 bad request error

[rousskov]

Faced with an invalid encoding, Squid code should do several things, depending on that code location.

• Uri::Decode() should throw. That is already done.

• Acl::Node::match() caller probably should convert exceptions to "I do not know" answers/errors. I cannot check all the details right now, so I cannot tell whether that is already done. It might be. If it is not done, it should be done in a dedicated PR.

• URI parsing code should probably decode paths and either throw on invalid ones (and store decoded ones) or mark them specially so that they are matched and forwarded "as is". Handling invalid path encoding feels similar to handling URI path whitespace, so we may want to be consistent with that feature, especially if we like it's default behavior. Any related changes probably deserve a dedicated PR (at least).

• This particular Decode() caller should not call Decode() at all or should continue to call Decode() as it does in the current PR code, depending on whether the input path is guaranteed to be decoded already (see the bullet above).

We may have a situation where PR-modified code lines are correct, but the PR should not be merged until other PRs prepare the rest of Squid for this PR change.

In either case, an invalid HTTP request (target path) should result in an error response; it should not kill the transaction.

Please do keep these caveats and complexities in mind as you select Decode() upgrade targets. Not all upgrades will be as easy as the first one. As the PR author, you are responsible for thinking about and, where applicable, testing exceptions introduced by Decode(). Mentioning, in the PR description, why they are not relevant or how they are handled is usually a good idea and may reduce the number of review iterations.

[rousskov]

Amos: In the current code the exception will jump out to the exception handler for the async Dialer, terminating whichever access checks were being performed and the transaction.

The exception will be called by JobDialer::dial() only if the current Acl::UrlPathCheck::match() call was triggered by firing of an async job call. Not all tasks have been converted to async jobs. Not all job callbacks have been converted to AsyncCalls. Legacy tasks/callbacks do not have a general exception handling mechanism like JobDialer::dial(). I have not checked carefully, but it is likely that these new match() exceptions thrown while processing some ACL-driven directives will kill Squid worker (because they will not be caught at the transaction or lower level).

Alex: Acl::Node::match() caller probably should convert exceptions to "I do not know" answers/errors. I cannot check all the details right now, so I cannot tell whether that is already done. It might be. If it is not done, it should be done in a dedicated PR.

I checked a bit more and could not find that conversion code in Acl::Node::matches() or nearby. It is probably missing and should be added in a dedicated PR.