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
@@ -127,11 +127,6 @@
                                 IInternetProtocol* pTargetProtocol, bool& handled)
 {
   m_pTargetProtocol = pTargetProtocol;
-  bool isBlocked = false;
-  CString src = szUrl;

[Eric, 2014/10/15 17:02:33]

The review I just posted converts this CString to std::wstring.
http://codereview.adblockplus.org/4912420225024000/

Since you're planning on rewriting this anyway, it would be useful to get more
of CString occurrences out of the way quickly.

-  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);
@@ -139,142 +134,7 @@
   {
     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;
-
-#ifdef SUPPORT_FILTER
-  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;
-#ifdef SUPPORT_FRAME_CACHING
-      if (nullptr != tab && tab->IsFrameCached(src))
-      {
-        m_contentType = CFilter::contentTypeSubdocument;
-      }
-      else
-#endif // SUPPORT_FRAME_CACHING
-      {
-        m_contentType = GetContentType(mimeType, m_boundDomain, src);
-      }
-    }
-  }
-
-  if (nullptr == tab)
-  {
-    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(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;
-    }
-  }
-#endif // SUPPORT_FILTER
-
-  return isBlocked ? S_FALSE : hr;
+  return hr;
 }
 
 HRESULT WBPassthruSink::OnRead(void* pv, ULONG cb, ULONG* pcbRead)
@@ -341,6 +201,37 @@
   */
   return m_spInternetProtocolSink ? m_spInternetProtocolSink->Switch(pProtocolData) : E_UNEXPECTED;
 }
+std::wstring ExtractHTTPHeader(const std::wstring& allHeaders, const std::wstring& targetHeaderName)
+{
+  auto targetHeaderBeginsAt = allHeaders.find(targetHeaderName);
+  if (std::string::npos == targetHeaderBeginsAt )
+  {
+    return L"";
+  }
+  targetHeaderBeginsAt += targetHeaderName.length();
+  auto targetHeaderEndsAt = allHeaders.find(L"\n", targetHeaderBeginsAt);
+  if (std::string::npos == targetHeaderEndsAt)
+  {
+    return L"";
+  }
+  return allHeaders.substr(targetHeaderBeginsAt, targetHeaderEndsAt - targetHeaderBeginsAt);
+}
+

[Eric, 2014/10/15 17:02:33]

If we really need both standard and wide versions of this function, at least use
a template to eliminate purely duplicated code.

+std::string ExtractHTTPHeader(const std::string& allHeaders, const std::string& targetHeaderName)
+{
+  auto targetHeaderBeginsAt = allHeaders.find(targetHeaderName);
+  if (std::string::npos == targetHeaderBeginsAt )
+  {
+    return "";
+  }
+  targetHeaderBeginsAt += targetHeaderName.length();
+  auto targetHeaderEndsAt = allHeaders.find("\r\n", targetHeaderBeginsAt);
+  if (std::string::npos == targetHeaderEndsAt)
+  {
+    return "";
+  }
+  return allHeaders.substr(targetHeaderBeginsAt, targetHeaderEndsAt - targetHeaderBeginsAt);
+}
 
 STDMETHODIMP WBPassthruSink::BeginningTransaction(LPCWSTR szURL, LPCWSTR szHeaders, DWORD dwReserved, LPWSTR* pszAdditionalHeaders)
 {
@@ -349,6 +240,15 @@
     *pszAdditionalHeaders = nullptr;
   }
 

[Eric, 2014/10/15 17:02:33]

I'm assuming this is new code that's not showing up as such because of the way
the patch set was generated.

+  CComPtr<IHttpNegotiate> spHttpNegotiate;
+  QueryServiceFromClient(&spHttpNegotiate);
+  HRESULT nativeHresult = spHttpNegotiate ? spHttpNegotiate->BeginningTransaction(szURL, szHeaders,dwReserved, pszAdditionalHeaders) : S_OK;

[Eric, 2014/10/15 17:02:33]

It would seem to me that if the service query fails that we have an error. I
realize this expression is a legacy from previous code but it's fairly opaque as
to why it's correct.

+
+  bool isBlocked = false;
+  CString src = szURL;
+  DEBUG_GENERAL(src);
+  CPluginClient::UnescapeUrl(src);
+
   CPluginClient* client = nullptr;
   if (CFilter::EContentType::contentTypeAny == m_contentType && (client = CPluginClient::GetInstance()))
   {
@@ -377,31 +277,104 @@
       {
         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);
+      return ExtractHTTPHeader(buf, "Accept:");
     }();
     m_contentType = GetContentTypeFromMimeType(ATL::CString(acceptHeader.c_str()));
-    bool isBlocked = client->ShouldBlock(szURL, m_contentType, m_boundDomain, /*debug flag but must be set*/true);
+
+    m_boundDomain = ExtractHTTPHeader(std::wstring(*pszAdditionalHeaders), L"Referer:").c_str();

[Eric, 2014/10/15 17:02:33]

In general, use 'ToCString()' here preferably. It makes explicit the boundary
between the old type and the new. In this case, however, there's a local
variable 'boundDomain' that's already been converted. The name 'm_boundDomain'
seems to be a new member variable, but I see no code outside the present
function that would use its value.

[Eric, 2014/10/15 17:02:33]

Is it the case the "Referer" only ever appears in the additional headers from
PassthroughAPP and not in the headers provided through QueryInfo? It seems oddly
inconsistent.

[sergei, 2014/10/28 14:08:46]

It indeed looks inconsistent although there is may be some logic, but I
completely agree that no one can guarantee that there is no Referer in the
string obtained through QueryInfo.
We could put some debug tracing here which is triggered when Referer is
available via QueryInfo.

[Oleksandr, 2014/10/30 23:40:48]

In my tests the Referer is available through QueryInfo only in ReportResult. I
have never seen it available before the request was made.

[sergei, 2014/10/31 12:47:57]

It's OK for me.

+    m_boundDomain = m_boundDomain.Trim(L" \r");
+    CPluginClient::UnescapeUrl(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 == src)
+      {
+        return nativeHresult;
+      }
+      else if (CPluginSettings::GetInstance()->IsPluginEnabled() && !client->IsWhitelistedUrl(std::wstring(documentUrl)))
+      {

[Eric, 2014/10/15 17:02:33]

FYI. A solid reason to need a rewrite. The change set that removes
SUPPORT_FRAME_CACHING has now been pushed.

+  #ifdef SUPPORT_FRAME_CACHING
+        if (tab->IsFrameCached(src))
+        {
+          m_contentType = CFilter::contentTypeSubdocument;
+        }
+  #endif // SUPPORT_FRAME_CACHING
+      }
+    }
+
+    {
+      // 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;
+        }
+      }
+    }
+
+    if (nullptr != client
+      && client->ShouldBlock(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)
+    {
+      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;
+      }
+    }
     if (isBlocked)
     {
       m_blockedInTransaction = true;
       return E_ABORT;
     }
   }
-  CComPtr<IHttpNegotiate> spHttpNegotiate;
-  QueryServiceFromClient(&spHttpNegotiate);
-  return spHttpNegotiate ? spHttpNegotiate->BeginningTransaction(szURL, szHeaders,dwReserved, pszAdditionalHeaders) : S_OK;
+
+  return  nativeHresult;
 }
 
 STDMETHODIMP WBPassthruSink::OnResponse(DWORD dwResponseCode, LPCWSTR szResponseHeaders, LPCWSTR szRequestHeaders, LPWSTR *pszAdditionalRequestHeaders)