Issue 29330053: Issue 3303 - Unescape request URL for ShouldBlock in WBPassthruSink::BeginningTransaction (Closed)
Patch Set 1 #
Total comments: 2
Patch Set 2 : address comment #MessagesTotal messages: 13
LGTM. Weird we've overlooked this before.
https://codereview.adblockplus.org/29330053/diff/29330054/src/plugin/PluginWb... File src/plugin/PluginWbPassThrough.cpp (right): https://codereview.adblockplus.org/29330053/diff/29330054/src/plugin/PluginWb... src/plugin/PluginWbPassThrough.cpp:381: if (client->ShouldBlock(src.c_str(), m_contentType, m_boundDomain, /*debug flag but must be set*/true)) The first argument for 'ShouldBlock' is "const std::wstring&". There's no need to convert to C-String.
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.
https://codereview.adblockplus.org/29330053/diff/29330054/src/plugin/PluginWb... File src/plugin/PluginWbPassThrough.cpp (right): https://codereview.adblockplus.org/29330053/diff/29330054/src/plugin/PluginWb... src/plugin/PluginWbPassThrough.cpp:381: if (client->ShouldBlock(src.c_str(), m_contentType, m_boundDomain, /*debug flag but must be set*/true)) On 2015/11/14 18:11:25, Eric wrote: > The first argument for 'ShouldBlock' is "const std::wstring&". There's no need > to convert to C-String. Good catch!
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. Let's move this discussion into the issue.
LGTM I've read the discussion in #3311. Given that, this change seems fine, particularly for now. We're unescaping URL's immediately upon all of the protocol entry points, including the one in this review. We're only calling CAdblockPlusClient::ShouldBlock() in three places: -- in the traverser "OnIFrame", unescaped by the sole caller -- in the traverser "OnElement" for <img> "src" attributes, unescaped in the statement before the call -- (here) in WBPassthruSink::BeginningTransaction, not unescaped For consistency with the rest of the code, we should probably make this change so that all the calls to ShouldBlock() are the same. Now it may be that this unescaping behavior is not what we want, but if it's not what we want here, we likely want to change it elsewhere as well.
Thanks for review but I would like to close this issue as rejected, see my comment https://issues.adblockplus.org/ticket/3303#comment:8. It would be better to drop decoding as another issue and as another codereview.
On 2015/11/18 10:23:51, sergei wrote: > Thanks for review but I would like to close this issue as rejected, see my > comment https://issues.adblockplus.org/ticket/3303#comment:8. It would be better > to drop decoding as another issue and as another codereview. The whole discussion has me wondering whether any of the calls to UnescapeUrl() are appropriate. That said, it might be good to commit the present change set as "Noissue - Call UnescapeUrl() consistent with other calls to ShouldBlock()".
On 2015/11/18 14:01:37, Eric wrote: > On 2015/11/18 10:23:51, sergei wrote: > > Thanks for review but I would like to close this issue as rejected, see my > > comment https://issues.adblockplus.org/ticket/3303#comment:8. It would be > better > > to drop decoding as another issue and as another codereview. > > The whole discussion has me wondering whether any of the calls to UnescapeUrl() > are appropriate. > > That said, it might be good to commit the present change set as "Noissue - Call > UnescapeUrl() consistent with other calls to ShouldBlock()". I think we should not commit this and instead be consistent by removing the UnescapeUrl() calls. I think that's what Sergei has suggested. This codereview should probably be just closed.
NOT LGTM
On 2015/11/19 11:48:14, Oleksandr wrote: > I think we should not commit this and instead be consistent by removing the > UnescapeUrl() calls. Does IE ever rewrite URI's, causing them to become escaped?
On 2015/11/20 15:47:24, Eric wrote: > On 2015/11/19 11:48:14, Oleksandr wrote: > > I think we should not commit this and instead be consistent by removing the > > UnescapeUrl() calls. > > Does IE ever rewrite URI's, causing them to become escaped? No. I have never seen it doing that. Maybe the underlying original HTTP/HTTPS Asynhronous Pluggable Protocols do that, but on our level we shouldn't encounter escaping. So far only Chrome is guilty with this, it seems. 
|
