Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1239)

Issue 4974480757620736: Issue #1356 - Improve detection of the issuer of the request (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 9 months ago by Oleksandr
Modified:
4 years, 8 months ago
Reviewers:
sergei, Eric, Felix Dahlke
Visibility:
Public.

Description

This builds on http://codereview.adblockplus.org/6390087684194304/, and will very likely have tons of conflicts when merging. Which is why I submit this as an overview of the idea in general. There will most likely be another patch with exact code to be committed after we push the patches from the above code review. The idea is that we can get the real Referer header, which is available on BeginningTransaction AFTER the original BeginningTransaction is called. Doing that leaves no reason to have our ShouldBlock check running in OnStart, so all that code is moved to BeginningTransaction as well. It works and it finally fixes the issue.

Patch Set 1 #

Total comments: 16

Patch Set 2 : Update to the repositorory tip #

Total comments: 11

Patch Set 3 : Cleanup #

Total comments: 2

Patch Set 4 : Revert the incorrect cleanup #

Total comments: 16

Patch Set 5 : Refactoring lambda and ExtractHttpHeaders functions. Coding style cleanup. #

Total comments: 5

Patch Set 6 : Revert refactoring of ExtractHttpHeaders. Keeping it simple. Typo fixes. #

Total comments: 3

Patch Set 7 : Coding style changes #

Total comments: 6

Patch Set 8 : Reorder stuff in BeginningTransaction for clarity. #

Patch Set 9 : Rename the parameter #

Unified diffs Side-by-side diffs Delta from patch set Stats (+118 lines, -207 lines) Patch
M src/plugin/PluginFilter.cpp View 1 1 chunk +1 line, -1 line 0 comments Download
M src/plugin/PluginWbPassThrough.h View 1 2 3 4 2 chunks +1 line, -1 line 0 comments Download
M src/plugin/PluginWbPassThrough.cpp View 1 2 3 4 5 6 7 8 6 chunks +116 lines, -204 lines 0 comments Download
M src/plugin/SinkPolicy.inl View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 23
Oleksandr
4 years, 9 months ago (2014-10-14 23:34:41 UTC) #1
Eric
Looks OK. Merging this directly does indeed look difficult, given the number of changes since ...
4 years, 9 months ago (2014-10-15 17:02:32 UTC) #2
Oleksandr
http://codereview.adblockplus.org/4974480757620736/diff/5629499534213120/src/plugin/PluginWbPassThrough.cpp File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/4974480757620736/diff/5629499534213120/src/plugin/PluginWbPassThrough.cpp#newcode255 src/plugin/PluginWbPassThrough.cpp:255: auto acceptHeader = [&]() -> std::string On 2014/10/15 17:02:33, ...
4 years, 9 months ago (2014-10-24 09:59:18 UTC) #3
sergei
- http://www.netzwelt.de still does not work as expected. - BTW, as a privacy feature we ...
4 years, 8 months ago (2014-10-28 14:08:45 UTC) #4
sergei
On 2014/10/28 14:08:45, sergei wrote: > - http://www.netzwelt.de still does not work as expected. Awesome, ...
4 years, 8 months ago (2014-10-28 14:19:52 UTC) #5
Oleksandr
http://codereview.adblockplus.org/4974480757620736/diff/5629499534213120/src/plugin/PluginWbPassThrough.cpp File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/4974480757620736/diff/5629499534213120/src/plugin/PluginWbPassThrough.cpp#newcode284 src/plugin/PluginWbPassThrough.cpp:284: m_boundDomain = ExtractHTTPHeader(std::wstring(*pszAdditionalHeaders), L"Referer:").c_str(); On 2014/10/28 14:08:46, sergei wrote: ...
4 years, 8 months ago (2014-10-30 23:40:48 UTC) #6
sergei
http://codereview.adblockplus.org/4974480757620736/diff/5629499534213120/src/plugin/PluginWbPassThrough.cpp File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/4974480757620736/diff/5629499534213120/src/plugin/PluginWbPassThrough.cpp#newcode284 src/plugin/PluginWbPassThrough.cpp:284: m_boundDomain = ExtractHTTPHeader(std::wstring(*pszAdditionalHeaders), L"Referer:").c_str(); On 2014/10/30 23:40:48, Oleksandr wrote: ...
4 years, 8 months ago (2014-10-31 12:47:57 UTC) #7
Oleksandr
http://codereview.adblockplus.org/4974480757620736/diff/5700735861784576/src/plugin/SinkPolicy.inl File src/plugin/SinkPolicy.inl (left): http://codereview.adblockplus.org/4974480757620736/diff/5700735861784576/src/plugin/SinkPolicy.inl#oldcode387 src/plugin/SinkPolicy.inl:387: if (E_ABORT == hr && pSink->m_blockedInTransaction) On 2014/10/31 12:47:57, ...
4 years, 8 months ago (2014-10-31 15:22:07 UTC) #8
sergei
LGTM
4 years, 8 months ago (2014-10-31 15:40:54 UTC) #9
Felix Dahlke
http://codereview.adblockplus.org/4974480757620736/diff/5681717746597888/src/plugin/PluginWbPassThrough.cpp File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/4974480757620736/diff/5681717746597888/src/plugin/PluginWbPassThrough.cpp#newcode203 src/plugin/PluginWbPassThrough.cpp:203: std::wstring src = szURL; Shouldn't this be stored as ...
4 years, 8 months ago (2014-11-03 04:58:16 UTC) #10
Oleksandr
http://codereview.adblockplus.org/4974480757620736/diff/5681717746597888/src/plugin/PluginWbPassThrough.cpp File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/4974480757620736/diff/5681717746597888/src/plugin/PluginWbPassThrough.cpp#newcode203 src/plugin/PluginWbPassThrough.cpp:203: std::wstring src = szURL; On 2014/11/03 04:58:16, Felix H. ...
4 years, 8 months ago (2014-11-03 11:37:37 UTC) #11
sergei
http://codereview.adblockplus.org/4974480757620736/diff/5681717746597888/src/shared/Utils.h File src/shared/Utils.h (right): http://codereview.adblockplus.org/4974480757620736/diff/5681717746597888/src/shared/Utils.h#newcode54 src/shared/Utils.h:54: T ExtractHTTPHeader(const T& allHeaders, const T& targetHeaderName, const T& ...
4 years, 8 months ago (2014-11-03 14:17:46 UTC) #12
Oleksandr
http://codereview.adblockplus.org/4974480757620736/diff/5681717746597888/src/shared/Utils.h File src/shared/Utils.h (right): http://codereview.adblockplus.org/4974480757620736/diff/5681717746597888/src/shared/Utils.h#newcode54 src/shared/Utils.h:54: T ExtractHTTPHeader(const T& allHeaders, const T& targetHeaderName, const T& ...
4 years, 8 months ago (2014-11-04 12:02:42 UTC) #13
Felix Dahlke
http://codereview.adblockplus.org/4974480757620736/diff/5681717746597888/src/shared/Utils.h File src/shared/Utils.h (right): http://codereview.adblockplus.org/4974480757620736/diff/5681717746597888/src/shared/Utils.h#newcode54 src/shared/Utils.h:54: T ExtractHTTPHeader(const T& allHeaders, const T& targetHeaderName, const T& ...
4 years, 8 months ago (2014-11-04 16:13:25 UTC) #14
Oleksandr
4 years, 8 months ago (2014-11-04 23:47:03 UTC) #15
sergei
http://codereview.adblockplus.org/4974480757620736/diff/5678807906254848/src/plugin/PluginWbPassThrough.cpp File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/4974480757620736/diff/5678807906254848/src/plugin/PluginWbPassThrough.cpp#newcode39 src/plugin/PluginWbPassThrough.cpp:39: std::string ExtractHttpAcceptHeader(IInternetProtocol* internetProtocol) Would you shift this function one ...
4 years, 8 months ago (2014-11-05 00:19:36 UTC) #16
Oleksandr
4 years, 8 months ago (2014-11-05 00:27:23 UTC) #17
sergei
On 2014/11/05 00:27:23, Oleksandr wrote: LGTM
4 years, 8 months ago (2014-11-05 07:17:15 UTC) #18
Felix Dahlke
http://codereview.adblockplus.org/4974480757620736/diff/5630721452408832/src/plugin/PluginWbPassThrough.cpp File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/4974480757620736/diff/5630721452408832/src/plugin/PluginWbPassThrough.cpp#newcode26 src/plugin/PluginWbPassThrough.cpp:26: auto targetHeaderBeginsAt = allHeaders.find(targetHeaderName); I still find it unintuitive ...
4 years, 8 months ago (2014-11-05 09:04:52 UTC) #19
Oleksandr
http://codereview.adblockplus.org/4974480757620736/diff/5630721452408832/src/plugin/PluginWbPassThrough.cpp File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/4974480757620736/diff/5630721452408832/src/plugin/PluginWbPassThrough.cpp#newcode26 src/plugin/PluginWbPassThrough.cpp:26: auto targetHeaderBeginsAt = allHeaders.find(targetHeaderName); On 2014/11/05 09:04:52, Felix H. ...
4 years, 8 months ago (2014-11-05 10:01:57 UTC) #20
Felix Dahlke
LGTM!
4 years, 8 months ago (2014-11-05 10:03:42 UTC) #21
sergei
http://codereview.adblockplus.org/4974480757620736/diff/5630721452408832/src/plugin/PluginWbPassThrough.cpp File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/4974480757620736/diff/5630721452408832/src/plugin/PluginWbPassThrough.cpp#newcode26 src/plugin/PluginWbPassThrough.cpp:26: auto targetHeaderBeginsAt = allHeaders.find(targetHeaderName); On 2014/11/05 10:01:58, Oleksandr wrote: ...
4 years, 8 months ago (2014-11-05 10:28:59 UTC) #22
Oleksandr
4 years, 8 months ago (2014-11-05 16:42:49 UTC) #23
Renamed the parameter. Closing this and pushing.

http://codereview.adblockplus.org/4974480757620736/diff/5630721452408832/src/...
File src/plugin/PluginWbPassThrough.cpp (right):

http://codereview.adblockplus.org/4974480757620736/diff/5630721452408832/src/...
src/plugin/PluginWbPassThrough.cpp:26: auto targetHeaderBeginsAt =
allHeaders.find(targetHeaderName);
On 2014/11/05 10:28:59, sergei wrote:
> On 2014/11/05 10:01:58, Oleksandr wrote:
> > On 2014/11/05 09:04:52, Felix H. Dahlke wrote:
> > > I still find it unintuitive that the colon is part of "header name" here.
> Why
> > > not just append the colon here?
> > > 
> > > More importantly, header names are generally case insensitive, are we sure
> > both
> > > HTTP_QUERY_RAW_HEADERS_CRLF and pszAdditionalHeaders will always return
them
> > in
> > > a way our case sensitive comparison expects?
> > 
> > 1. Colon is part of header name because this is a templated function and
> > otherwise we'll end up with stuff CreateConstString (see Patchset 5). This
is
> to
> > keep it simple.
> > 2. The headers we get from IE are constructed by IE (not some third party
> code).
> > So it is always in the same format. Besides, if we wanted to convert
> everything
> > to lower case ::tolower wouldn't work (since it's meant to be used for chars
> > only, not wchars. So we'll make things unnecessarily complex again.
> 
> 1. T is std::string or std::wstring. The variant with template specialization
> looks like overengineering here. May be such functions like
> `ConvertToResult<From, To>::Type ConvertTo<To>(const From&)` could help but we
> don't have them as well as we can use something like `static_cast<typename
> T::value_type>(':')`, but it still smells. What about
> `targetHeaderNameWithColon`?
> 2. It works now, but I also think it's important. I would say that content
type
> should also be case-insensitive.
> BTW, the headers are not always constructed by IE. `BeginningTransaction` is
> provided by the client and the client sets or modifies the header.  (as I
> pointed before we can actually use it to implement additional features like
> hiding the real user agent). A real example is flash addon, as I see it adds
its
> own header which we can use for better object-subrequest detection as well
> (should be as another issue).
> I think that we can discuss the details how it should be implemented in the
> issue tracker. I would like to have it as a good-first-bug, it's an excellent
> challenging task which can wait for now I think ;)

Created https://issues.adblockplus.org/ticket/1529 on this.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5