Opened 4 years ago

Closed 4 years ago

#3303 closed change (rejected)

Unescape request URL for ShouldBlock in WBPassthruSink::BeginningTransaction

Reported by: sergz Assignee: sergz
Priority: P3 Milestone:
Module: Adblock-Plus-for-Internet-Explorer Keywords:
Cc: oleksandr Blocked By:
Blocking: #3300 Platform: Internet Explorer
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29330053/

Description

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.

Change History (9)

comment:1 Changed 4 years ago by sergz

  • Review URL(s) modified (diff)

comment:2 Changed 4 years ago by oleksandr

  • Priority changed from Unknown to P3
  • Status changed from new to reviewing

comment:3 Changed 4 years ago by 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.

comment:4 Changed 4 years ago by 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.

comment:5 Changed 4 years ago by 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.

comment:6 follow-up: Changed 4 years ago by 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.

comment:7 Changed 4 years ago by oleksandr

  • Blocking 3300 added

comment:8 in reply to: ↑ 6 Changed 4 years ago by 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.

Last edited 4 years ago by sergz (previous) (diff)

comment:9 Changed 4 years ago by sergz

  • Resolution set to rejected
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.