Issue #1356 - Improve detection of the issuer of the request

src/plugin/PluginWbPassThrough.cpp

Index: src/plugin/PluginWbPassThrough.cpp
===================================================================
--- a/src/plugin/PluginWbPassThrough.cpp
+++ b/src/plugin/PluginWbPassThrough.cpp
@@ -9,6 +9,7 @@
 #include "PluginSystem.h"
 #include <WinInet.h>
 #include "wtypes.h"
+#include "../shared/Utils.h"
 
 namespace
 {
@@ -125,150 +126,7 @@
                                 IInternetProtocol* pTargetProtocol, bool& handled)
 {
   m_pTargetProtocol = pTargetProtocol;
-  bool isBlocked = false;
-  CString src = szUrl;
-  DEBUG_GENERAL(src);
-  CPluginClient::UnescapeUrl(src);
-
-  // call the impl of the base class as soon as possible because it initializes the base class
-  // members, used by this method. It queries for the required interfaces.
-  HRESULT hr = BaseClass::OnStart(szUrl, pOIProtSink, pOIBindInfo, grfPI, dwReserved, pTargetProtocol);
-  if (FAILED(hr))
-  {
-    return hr;
-  }
-
-  CString mimeType;
-  if (pOIBindInfo)
-  {
-    ULONG resLen = 0;
-
-    // Apparently IE will report random mime type if there's more then 1 in the list.
-    // So we get the whole list and just use the first one (top priority one)
-    LPOLESTR mime[10];
-    pOIBindInfo->GetBindString(BINDSTRING_ACCEPT_MIMES, mime, 10, &resLen);
-    if (mime && resLen > 0)
-    {
-      mimeType.SetString(mime[0]);
-    }
-    LPOLESTR bindString = nullptr;
-    pOIBindInfo->GetBindString(BINDSTRING_FLAG_BIND_TO_OBJECT, &bindString, 1, &resLen);
-    LPOLESTR domainRetrieved = nullptr;
-    if (resLen == 0 || wcscmp(bindString, L"FALSE") == 0)
-    {
-      HRESULT hr = pOIBindInfo->GetBindString(BINDSTRING_XDR_ORIGIN, &domainRetrieved, 1, &resLen);
-      if ((hr == S_OK) && domainRetrieved && (resLen > 0))
-      {
-        m_boundDomain = domainRetrieved;
-      }
-    }
-    // We can obtain IBindCtx* here, but IEnumString obtained via IBindCtx::EnumObjectParam
-    // does not return any parameter, so it's useless.
-  }
-
-  CString cookie;
-  ULONG len1 = 2048;
-  ULONG len2 = 2048;
-
-  CPluginTab* tab = CPluginClass::GetTab(::GetCurrentThreadId());
-  CPluginClient* client = CPluginClient::GetInstance();
-
-  if (tab && client)
-  {
-    CString documentUrl = tab->GetDocumentUrl();
-    // Page is identical to document => don't block
-    if (documentUrl == src)
-    {
-      // fall through
-    }
-    else if (CPluginSettings::GetInstance()->IsPluginEnabled() && !client->IsWhitelistedUrl(std::wstring(documentUrl)))
-    {
-      m_boundDomain = tab->GetDocumentUrl();
-      m_contentType = CFilter::contentTypeAny;
-      if (tab != nullptr && tab->IsFrameCached(src))
-      {
-        m_contentType = CFilter::contentTypeSubdocument;
-      }
-      else
-      {
-        m_contentType = GetContentType(mimeType, m_boundDomain, src);
-      }
-    }
-  }
-
-  if (tab == nullptr)
-  {
-    m_contentType = GetContentType(mimeType, m_boundDomain, src);
-  }
-
-  {
-    // Here is the heuristic which detects the requests issued by Flash.ocx.
-    // It turned out that the implementation from ''Flash.ocx'' (tested version is 15.0.0.152)
-    // returns quite minimal configuration in comparison with the implementation from Microsofts'
-    // libraries (see grfBINDF and bindInfo.dwOptions). The impl from MS often includes something
-    // else.
-    ATL::CComPtr<IBindStatusCallback> bscb;
-    if (SUCCEEDED(QueryServiceFromClient(&bscb)) && !!bscb)
-    {
-      DWORD grfBINDF = 0;
-      BINDINFO bindInfo = {};
-      bindInfo.cbSize = sizeof(bindInfo);
-      if (SUCCEEDED(bscb->GetBindInfo(&grfBINDF, &bindInfo))
-        && (BINDF_ASYNCHRONOUS | BINDF_ASYNCSTORAGE| BINDF_PULLDATA) == grfBINDF
-        && (BINDINFO_OPTIONS_ENABLE_UTF8 | BINDINFO_OPTIONS_USE_IE_ENCODING) == bindInfo.dwOptions
-        )
-      {
-        m_contentType = CFilter::EContentType::contentTypeObjectSubrequest;
-      }
-    }
-  }
-
-  // The descision about EContentType::contentTypeAny is made later in
-  // WBPassthruSink::BeginningTransaction. Sometimes here we cannot detect the request type, but
-  // in WBPassthruSink::BeginningTransaction the header Accept is available which allows to
-  // obtain the "request type" in our terminology.
-  if (nullptr != client
-    && CFilter::EContentType::contentTypeAny != m_contentType
-    && client->ShouldBlock(static_cast<const wchar_t*>(src), m_contentType, m_boundDomain, true))
-  {
-    isBlocked = true;
-  }
-
-  // For IE6 and earlier there is iframe back button issue, so avoid it.
-  if (isBlocked && client->GetIEVersion() > 6)
-  {
-    handled = true;
-    if (CFilter::EContentType::contentTypeImage == m_contentType)
-    {
-      // IE shows a cross that img is not loaded
-      return INET_E_REDIRECT_FAILED;
-    }
-    if (CFilter::EContentType::contentTypeSubdocument == m_contentType)
-    {
-      PassthroughAPP::CustomSinkStartPolicy<WBPassthru, WBPassthruSink>::GetProtocol(this)->m_shouldSupplyCustomContent = true;
-      m_spInternetProtocolSink->ReportProgress(BINDSTATUS_MIMETYPEAVAILABLE, L"text/html");
-      m_spInternetProtocolSink->ReportData(BSCF_FIRSTDATANOTIFICATION, 0, static_cast<ULONG>(g_blockedByABPPage.size()));
-      return S_OK;
-    } 
-    if (CFilter::EContentType::contentTypeScript == m_contentType)
-    {
-      m_spInternetProtocolSink->ReportProgress(BINDSTATUS_MIMETYPEAVAILABLE, L"text/javascript");
-      m_spInternetProtocolSink->ReportResult(INET_E_REDIRECTING, 301, L"data:");
-      return INET_E_REDIRECT_FAILED;
-    }
-    if (CFilter::EContentType::contentTypeXmlHttpRequest == m_contentType)
-    {
-      m_spInternetProtocolSink->ReportResult(INET_E_REDIRECTING, 301, L"data:");
-      return INET_E_REDIRECT_FAILED;
-    }
-    if (CFilter::EContentType::contentTypeAny != m_contentType)
-    {
-      m_spInternetProtocolSink->ReportResult(INET_E_REDIRECTING, 301, L"data:");
-      return INET_E_REDIRECT_FAILED;
-    }
-  }
-
-  return isBlocked ? S_FALSE : hr;
+  return BaseClass::OnStart(szUrl, pOIProtSink, pOIBindInfo, grfPI, dwReserved, pTargetProtocol);
 }
 
 HRESULT WBPassthruSink::OnRead(void* pv, ULONG cb, ULONG* pcbRead)
@@ -342,60 +200,96 @@
   {
     *pszAdditionalHeaders = nullptr;
   }
+  std::wstring src = szURL;

[Felix Dahlke, 2014/11/03 04:58:16]

Shouldn't this be stored as a CString? It's only ever used as a CString it
seems.

[Oleksandr, 2014/11/03 11:37:37]

There is a folow-up review where we will be removing the CString occurences.
Since I was rewriting this code it made sense to use std::wstring off the bat.
See
http://codereview.adblockplus.org/4974480757620736/diff/5629499534213120/src/...

+  DEBUG_GENERAL(ToCString(src));
 
-  CPluginClient* client = nullptr;
-  if (CFilter::EContentType::contentTypeAny == m_contentType && (client = CPluginClient::GetInstance()))
+  CComPtr<IHttpNegotiate> spHttpNegotiate;

[Felix Dahlke, 2014/11/03 04:58:16]

Other local variables here don't use hungarian notation, why this one?

+  QueryServiceFromClient(&spHttpNegotiate);
+  // This fills the pszAdditionalHeaders with more headers. One of which is the Referer header, which we need.
+  // There doesn't seem to be any other way to get this header before the request has been made.
+  HRESULT nativeHr = spHttpNegotiate ? spHttpNegotiate->BeginningTransaction(szURL, szHeaders,dwReserved, pszAdditionalHeaders) : S_OK;

[Felix Dahlke, 2014/11/03 04:58:16]

Space after "szHeaders,".

+
+  auto acceptHeader = [&]() -> std::string

[Felix Dahlke, 2014/11/03 04:58:16]

Looks like this could move up to be the first thing in the function, since it's
the first thing we do and it doesn't need pszAdditionalHeaders, nativeHr etc.

I suggest this:

std::wstring src = szURL;
DEBUG_GENERAL(ToCString(src));

auto acceptHeader = ...

if (pszAdditionalHeaders)
{
  *pszAdditionalHeaders = nullptr;
}
CComPtr<IHTTPNegotiate> spHttpNegotiate;
...

That said, I'd find this even easier to follow if the lambda was a named
function, ExtractAcceptHeader or something.

   {
-    auto acceptHeader = [&]() -> std::string
+    // Despite there is HTTP_QUERY_ACCEPT and other query info flags, they don't work here,

[Felix Dahlke, 2014/11/03 04:58:16]

s/is/being/?

+    // only HTTP_QUERY_RAW_HEADERS_CRLF | HTTP_QUERY_FLAG_REQUEST_HEADERS does dork.
+    ATL::CComPtr<IWinInetHttpInfo> winInetHttpInfo;
+    HRESULT hr = m_spTargetProtocol->QueryInterface(&winInetHttpInfo);
+    if(FAILED(hr))

[Felix Dahlke, 2014/11/03 04:58:16]

Space before "(" please, also below.

     {
-      // Despite there is HTTP_QUERY_ACCEPT and other query info flags, they don't work here,
-      // only HTTP_QUERY_RAW_HEADERS_CRLF | HTTP_QUERY_FLAG_REQUEST_HEADERS does dork.
-      ATL::CComPtr<IWinInetHttpInfo> winInetHttpInfo;
-      HRESULT hr = m_spTargetProtocol->QueryInterface(&winInetHttpInfo);
-      if(FAILED(hr))
+      return "";
+    }
+    DWORD size = 0;
+    DWORD flags = 0;
+    hr = winInetHttpInfo->QueryInfo(HTTP_QUERY_RAW_HEADERS_CRLF | HTTP_QUERY_FLAG_REQUEST_HEADERS,
+        /*buffer*/nullptr, /* get size */&size, &flags, /*reserved*/ 0);

[Felix Dahlke, 2014/11/03 04:58:16]

How about /*get size*/ for consistency?

+    if(FAILED(hr))
+    {
+      return "";
+    }
+    std::string buf(size, '\0');
+    hr = winInetHttpInfo->QueryInfo(HTTP_QUERY_RAW_HEADERS_CRLF | HTTP_QUERY_FLAG_REQUEST_HEADERS,

[Felix Dahlke, 2014/11/03 04:58:16]

Can you store the headers to retrieve in a variable to avoid duplication here?

+      &buf[0], &size, &flags, 0);
+    if(FAILED(hr))
+    {
+      return "";
+    }
+    return ExtractHTTPHeader<std::string>(buf, "Accept:", "\r\n");
+  }();
+  m_contentType = GetContentTypeFromMimeType(ATL::CString(acceptHeader.c_str()));
+  if (*pszAdditionalHeaders != 0)
+  {
+    m_boundDomain = ExtractHTTPHeader<std::wstring>(std::wstring(*pszAdditionalHeaders), L"Referer:", L"\n").c_str();
+  }
+  m_boundDomain = TrimString(m_boundDomain);
+  CPluginTab* tab = CPluginClass::GetTab(::GetCurrentThreadId());
+  CPluginClient* client = CPluginClient::GetInstance();
+
+  if (tab && client)
+  {
+    CString documentUrl = tab->GetDocumentUrl();
+    // Page is identical to document => don't block
+    if (documentUrl == ToCString(src))
+    {
+      return nativeHr;
+    }
+    else if (CPluginSettings::GetInstance()->IsPluginEnabled() && !client->IsWhitelistedUrl(std::wstring(documentUrl)))
+    {
+      if (tab->IsFrameCached(ToCString(src)))
+      {
+        m_contentType = CFilter::contentTypeSubdocument;
+      }
+    }
+  }
+
+  {

[Felix Dahlke, 2014/11/03 04:58:16]

Also something I'd suggest to move to a dedicated function, the comment shows it
wants to be free IMO :) Up to you too.

+    // Here is the heuristic which detects the requests issued by Flash.ocx.
+    // It turned out that the implementation from ''Flash.ocx'' (tested version is 15.0.0.152)
+    // returns quite minimal configuration in comparison with the implementation from Microsofts'
+    // libraries (see grfBINDF and bindInfo.dwOptions). The impl from MS often includes something
+    // else.
+    ATL::CComPtr<IBindStatusCallback> bscb;
+    if (SUCCEEDED(QueryServiceFromClient(&bscb)) && !!bscb)
+    {
+      DWORD grfBINDF = 0;
+      BINDINFO bindInfo = {};
+      bindInfo.cbSize = sizeof(bindInfo);
+      if (SUCCEEDED(bscb->GetBindInfo(&grfBINDF, &bindInfo))
+        && (BINDF_ASYNCHRONOUS | BINDF_ASYNCSTORAGE| BINDF_PULLDATA) == grfBINDF

[Felix Dahlke, 2014/11/03 04:58:16]

According to Mozilla's style, we should break after &&/||, not before.

+        && (BINDINFO_OPTIONS_ENABLE_UTF8 | BINDINFO_OPTIONS_USE_IE_ENCODING) == bindInfo.dwOptions
+        )
       {
-        return "";
+        m_contentType = CFilter::EContentType::contentTypeObjectSubrequest;
       }
-      DWORD size = 0;
-      DWORD flags = 0;
-      hr = winInetHttpInfo->QueryInfo(HTTP_QUERY_RAW_HEADERS_CRLF | HTTP_QUERY_FLAG_REQUEST_HEADERS,
-          /*buffer*/nullptr, /* get size */&size, &flags, /*reserved*/ 0);
-      if(FAILED(hr))
-      {
-        return "";
-      }
-      std::string buf(size, '\0');
-      hr = winInetHttpInfo->QueryInfo(HTTP_QUERY_RAW_HEADERS_CRLF | HTTP_QUERY_FLAG_REQUEST_HEADERS,
-        &buf[0], &size, &flags, 0);
-      if(FAILED(hr))
-      {
-        return "";
-      }
-      char acceptHeader[] = "Accept:";
-      auto acceptHeaderBeginsAt = buf.find(acceptHeader);
-      if (std::string::npos == acceptHeaderBeginsAt)
-      {
-        return "";
-      }
-      acceptHeaderBeginsAt += sizeof(acceptHeader);
-      auto acceptHeaderEndsAt = buf.find("\n", acceptHeaderBeginsAt);
-      if (std::string::npos == acceptHeaderEndsAt)
-      {
-        return "";
-      }
-      return buf.substr(acceptHeaderBeginsAt, acceptHeaderEndsAt - acceptHeaderBeginsAt);
-    }();
-    m_contentType = GetContentTypeFromMimeType(ATL::CString(acceptHeader.c_str()));
-    bool isBlocked = client->ShouldBlock(szURL, m_contentType, m_boundDomain, /*debug flag but must be set*/true);
-    if (isBlocked)
-    {
-      m_blockedInTransaction = true;
-      return E_ABORT;
     }
   }
-  CComPtr<IHttpNegotiate> spHttpNegotiate;
-  QueryServiceFromClient(&spHttpNegotiate);
-  return spHttpNegotiate ? spHttpNegotiate->BeginningTransaction(szURL, szHeaders,dwReserved, pszAdditionalHeaders) : S_OK;
+
+  m_blockedInTransaction = client->ShouldBlock(szURL, m_contentType, m_boundDomain, /*debug flag but must be set*/true);
+  if (m_blockedInTransaction)
+  {
+    return E_ABORT;
+  }
+  return nativeHr;
 }
 
 STDMETHODIMP WBPassthruSink::OnResponse(DWORD dwResponseCode, LPCWSTR szResponseHeaders, LPCWSTR szRequestHeaders, LPWSTR *pszAdditionalRequestHeaders)
@@ -431,7 +325,6 @@
 
 WBPassthru::WBPassthru()
   : m_shouldSupplyCustomContent(false)
-  , m_hasOriginalStartCalled(false)
 {
 }
 
@@ -455,18 +348,10 @@
 
 STDMETHODIMP WBPassthru::LockRequest(/* [in] */ DWORD options)
 {
-  if (!m_hasOriginalStartCalled)
-  {
-    return S_OK;
-  }
   return BaseClass::LockRequest(options);
 }
 
 STDMETHODIMP WBPassthru::UnlockRequest()
 {
-  if (!m_hasOriginalStartCalled)
-  {
-    return S_OK;
-  }
   return BaseClass::UnlockRequest();
 }