src/plugin/PluginWbPassThrough.cpp - Issue 4974480757620736: Issue #1356 - Improve detection of the issuer of the request

Delta Between Two Patch Sets: src/plugin/PluginWbPassThrough.cpp

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

Left Patch Set: Coding style changes Created Nov. 5, 2014, 12:25 a.m.

Right Patch Set: Rename the parameter Created Nov. 5, 2014, 4:51 p.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

Left: Side by side diff | Download
Right: Side by side diff | Download

LEFT	RIGHT
1 #include "PluginStdAfx.h"	1 #include "PluginStdAfx.h"

2	2

3 #include "PluginWbPassThrough.h"	3 #include "PluginWbPassThrough.h"

4 #include "PluginClient.h"	4 #include "PluginClient.h"

5 #include "PluginClientFactory.h"	5 #include "PluginClientFactory.h"

6 #include "PluginFilter.h"	6 #include "PluginFilter.h"

7 #include "PluginSettings.h"	7 #include "PluginSettings.h"

8 #include "PluginClass.h"	8 #include "PluginClass.h"

9 #include "PluginSystem.h"	9 #include "PluginSystem.h"

10 #include <WinInet.h>	10 #include <WinInet.h>

11 #include "wtypes.h"	11 #include "wtypes.h"

12 #include "../shared/Utils.h"	12 #include "../shared/Utils.h"

13	13

14 namespace	14 namespace

15 {	15 {

16 std::string g_blockedByABPPage = "<!DOCTYPE html>"	16 std::string g_blockedByABPPage = "<!DOCTYPE html>"

17 "<html>"	17 "<html>"

18 "<body>"	18 "<body>"

19 "<!-- blocked by AdblockPlus -->"	19 "<!-- blocked by AdblockPlus -->"

20 "</body>"	20 "</body>"

21 "</html>";	21 "</html>";

22	22

23 template <class T>	23 template <class T>

24 T ExtractHttpHeader(const T& allHeaders, const T& targetHeaderName, const T& d elimiter)	24 T ExtractHttpHeader(const T& allHeaders, const T& targetHeaderNameWithColon, c onst T& delimiter)

25 {	25 {

26 auto targetHeaderBeginsAt = allHeaders.find(targetHeaderName);	26 auto targetHeaderBeginsAt = allHeaders.find(targetHeaderNameWithColon);
Felix Dahlke 2014/11/05 09:04:52 I still find it unintuitive that the colon is part 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? Oleksandr 2014/11/05 10:01:58 1. Colon is part of header name because this is a Show quoted text 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. sergei 2014/11/05 10:28:59 1. T is std::string or std::wstring. The variant w Show quoted text 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 ;) Oleksandr 2014/11/05 16:42:50 Created https://issues.adblockplus.org/ticket/1529 Show quoted text 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.
27 if (targetHeaderBeginsAt == T::npos)	27 if (targetHeaderBeginsAt == T::npos)

28 {	28 {

29 return T();	29 return T();

30 }	30 }

31 targetHeaderBeginsAt += targetHeaderName.length();	31 targetHeaderBeginsAt += targetHeaderNameWithColon.length();

32 auto targetHeaderEndsAt = allHeaders.find(delimiter, targetHeaderBeginsAt);	32 auto targetHeaderEndsAt = allHeaders.find(delimiter, targetHeaderBeginsAt);

33 if (targetHeaderEndsAt == T::npos)	33 if (targetHeaderEndsAt == T::npos)

34 {	34 {

35 return T();	35 return T();

36 }	36 }

37 return allHeaders.substr(targetHeaderBeginsAt, targetHeaderEndsAt - targetHe aderBeginsAt);	37 return allHeaders.substr(targetHeaderBeginsAt, targetHeaderEndsAt - targetHe aderBeginsAt);

38 }	38 }

39	39

40 std::string ExtractHttpAcceptHeader(IInternetProtocol* internetProtocol)	40 std::string ExtractHttpAcceptHeader(IInternetProtocol* internetProtocol)

41 {	41 {

(...skipping 215 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
257 )	257 )

258 {	258 {

259 return true;	259 return true;

260 }	260 }

261 }	261 }

262 return false;	262 return false;

263 }	263 }

264	264

265 STDMETHODIMP WBPassthruSink::BeginningTransaction(LPCWSTR szURL, LPCWSTR szHeade rs, DWORD dwReserved, LPWSTR* pszAdditionalHeaders)	265 STDMETHODIMP WBPassthruSink::BeginningTransaction(LPCWSTR szURL, LPCWSTR szHeade rs, DWORD dwReserved, LPWSTR* pszAdditionalHeaders)

266 {	266 {

267 if (pszAdditionalHeaders)

268 {

269 *pszAdditionalHeaders = nullptr;

270 }

271 std::wstring src = szURL;	267 std::wstring src = szURL;

272 DEBUG_GENERAL(ToCString(src));	268 DEBUG_GENERAL(ToCString(src));
Felix Dahlke 2014/11/05 09:04:52 Hehe, still not quite what I meant :D Wouldn't it Hehe, still not quite what I meant :D Wouldn't it make more sense to: 1. Have these two lines (i.e. DEBUG_GENERAL) be the first thing the function does 2. Move the if (pszAdditionalHeaders) down to where we actually use pszAdditionalHeaders? Just wanted to bring across what I mean, go with what you think makes most sense. Oleksandr 2014/11/05 10:01:58 That makes sense, yes. Show quoted text On 2014/11/05 09:04:52, Felix H. Dahlke wrote: > Hehe, still not quite what I meant :D Wouldn't it make more sense to: > > 1. Have these two lines (i.e. DEBUG_GENERAL) be the first thing the function > does > 2. Move the if (pszAdditionalHeaders) down to where we actually use > pszAdditionalHeaders? > > Just wanted to bring across what I mean, go with what you think makes most > sense. That makes sense, yes.
273	269

274 std::string acceptHeader = ExtractHttpAcceptHeader(m_spTargetProtocol);	270 std::string acceptHeader = ExtractHttpAcceptHeader(m_spTargetProtocol);

275 m_contentType = GetContentTypeFromMimeType(ATL::CString(acceptHeader.c_str())) ;	271 m_contentType = GetContentTypeFromMimeType(ATL::CString(acceptHeader.c_str())) ;

	272

	273 if (pszAdditionalHeaders)

	274 {

	275 *pszAdditionalHeaders = nullptr;

	276 }

276	277

277 CComPtr<IHttpNegotiate> httpNegotiate;	278 CComPtr<IHttpNegotiate> httpNegotiate;

278 QueryServiceFromClient(&httpNegotiate);	279 QueryServiceFromClient(&httpNegotiate);

279 // This fills the pszAdditionalHeaders with more headers. One of which is the Referer header, which we need.	280 // This fills the pszAdditionalHeaders with more headers. One of which is the Referer header, which we need.

280 // There doesn't seem to be any other way to get this header before the reques t has been made.	281 // There doesn't seem to be any other way to get this header before the reques t has been made.

281 HRESULT nativeHr = httpNegotiate ? httpNegotiate->BeginningTransaction(szURL, szHeaders, dwReserved, pszAdditionalHeaders) : S_OK;	282 HRESULT nativeHr = httpNegotiate ? httpNegotiate->BeginningTransaction(szURL, szHeaders, dwReserved, pszAdditionalHeaders) : S_OK;

282	283

283 if (*pszAdditionalHeaders != 0)	284 if (*pszAdditionalHeaders != 0)

284 {	285 {

285 m_boundDomain = ExtractHttpHeader<std::wstring>(*pszAdditionalHeaders, L"Ref erer:", L"\n").c_str();	286 m_boundDomain = ExtractHttpHeader<std::wstring>(*pszAdditionalHeaders, L"Ref erer:", L"\n").c_str();

(...skipping 88 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
374	375

375 STDMETHODIMP WBPassthru::LockRequest(/* [in] */ DWORD options)	376 STDMETHODIMP WBPassthru::LockRequest(/* [in] */ DWORD options)

376 {	377 {

377 return BaseClass::LockRequest(options);	378 return BaseClass::LockRequest(options);

378 }	379 }

379	380

380 STDMETHODIMP WBPassthru::UnlockRequest()	381 STDMETHODIMP WBPassthru::UnlockRequest()

381 {	382 {

382 return BaseClass::UnlockRequest();	383 return BaseClass::UnlockRequest();

383 }	384 }

LEFT	RIGHT