Unescape request URL for ShouldBlock in WBPassthruSink::BeginningTransaction

[sergz]

Background

we are not unescaping URL in WBPassthruSink::BeginningTransaction before sending it into ShouldBlock, however it was here ​before and then it seems accidentally ​lost. It seems logical to firstly unescape URL otherwise it's very easy to circumvent us.

What to change

Pass src.c_str() instead of szURL into client->ShouldBlock.

[sergz]

On 2015/11/16 04:10:54, Oleksandr wrote:

After looking into why this isn't an issue in Chrome, I think we should solve it
differently. We should only be unescaping the address part of the URL, while
leaving the parameters escaped. That way #3300 will be fixed as well.

I think to be consistent we should commit this issue as is now.

I completely agree that it should be in the core, however I'm sure it's much faster to get the bug fixed as it's proposed in the issue description. Also pay attention that we have already several calls to ShouldBlock with decoded URL, I think we could fix it here and there at once after it's done in the core as another issue. Otherwise, to be consistent we should change those calls with decoded URL to calls with the original URL now (introducing more bugs) and wait for the implementation in the core.

[oleksandr]

I am not suggesting we should wait for it to be implemented in core. I just think we shouldn't be unescaping the whole URL as it is suggested here. Unescaping only the address part is enough, parameters should not be unescaped.

[sergz]

Another option to fix this issue (not #3300) now is to decode URL only inside ShouldBlock and change other places to pass the original URL. Later we can merely drop decoding code in ShouldBlock.

[oleksandr]

My understanding is that only #3300 is the issue that needs fixing. I don't see any reason to unescape the URL just for the sake of it. We should, however, be consistent and either unescape in both APP and traverser or in neither one. Seeing that #3311 was rejected, I think we should just not unescape the URLs for the Matches call at all.

[sergz]

Replying to oleksandr:

My understanding is that only #3300 is the issue that needs fixing. I don't see any reason to unescape the URL just for the sake of it. We should, however, be consistent and either unescape in both APP and traverser or in neither one. Seeing that #3311 was rejected, I think we should just not unescape the URLs for the Matches call at all.

I agree, let's drop unescaping/decoding for now. If there are some issues we will fix it.

Merely for reference, my opinion is that if we decode only unreserved characters in "​hierarchical part", it won't be a trouble. Anyway Chrome already does it widely (without ABP) and it seems there are no complains.

A quote from ​​https://tools.ietf.org/html/rfc3986

URIs that differ in the replacement of an unreserved character with its corresponding percent-encoded US-ASCII octet are equivalent: they identify the same resource.