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

src/plugin/PluginWbPassThrough.cpp

 #include "PluginStdAfx.h"
 
 #include "PluginWbPassThrough.h"
 #include "PluginClient.h"
 #include "PluginClientFactory.h"
 #ifdef SUPPORT_FILTER
 #include "PluginFilter.h"
 #endif
 #include "PluginSettings.h"
 #include "PluginClass.h"
@@ skipping 109 matching lines @@
 ////////////////////////////////////////////////////////////////////////////////////////
 //WBPassthruSink
 //Monitor and/or cancel every request and responde
 //WB makes, including images, sounds, scripts, etc
 ////////////////////////////////////////////////////////////////////////////////////////
 HRESULT WBPassthruSink::OnStart(LPCWSTR szUrl, IInternetProtocolSink *pOIProtSink,
                                 IInternetBindInfo *pOIBindInfo, DWORD grfPI, HANDLE_PTR dwReserved,
                                 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);
   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;
-
-#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)
 {
   if (nullptr == pv)
   {
     return E_POINTER;
   }
   if (nullptr == pcbRead)
   {
@@ skipping 46 matching lines @@
   IInternetProtocolSink::Switch with PD_FORCE_SWITCH flag in 
   PROTOCOLDATA::grfFlags, eventually URLMon will turn around and call 
   IInternetProtocol::Continue on the main thread. 
 
   Or, if you happen to have a window handy that was created on the main 
   thread, you can post yourself a message.
   "
   */
   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)
 {
   if (pszAdditionalHeaders)
   {
     *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()))
   {
     auto acceptHeader = [&]() -> std::string

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

This function does not look like a good candidate for a lambda. Since you're
invoking it immediately, there's little advantage defining a function. Simply
declare the variable 'acceptHeader' and initialize it in place of the 'return'
statement.

It also seems reasonable to define a separate function to retrieve the headers,
given how verbose the implementation for such a simple-looking function.

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

Also, it would be better generally to have 'acceptHeader' be 'std::wstring' for
consistency with the rest of the code. It's used as an argument to
'GetContentTypeFromMimeType', whose 'CString' argument is slated for conversion.

[Oleksandr, 2014/10/24 09:59:18]

I think it's good to leave this as is, since it helps us avoid a lot of the
nested if's and also the code looks cleaner this way, IMHO.

     {
       // 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.

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

Doesn't HTTP_QUERY_CUSTOM work? If so, that would greatly simplify things,
including not needing to define a std::string version of ExtractHTTPHeader.

"Causes HttpQueryInfo to search for the header name specified in lpvBuffer and
store the header data in lpvBuffer."

[Oleksandr, 2014/10/24 09:59:18]

No, it doesn't work. Added a comment about that.

       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);
       if(FAILED(hr))
       {
         return "";
       }
       std::string buf(size, '\0');

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

I'll simply register my distaste for writing into an internal string buffer
that's declared read-only. It works today, but it may not work later.

       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);
+      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)
 {
   if (pszAdditionalRequestHeaders)
   {
     *pszAdditionalRequestHeaders = 0;
   }
 
   CComPtr<IHttpNegotiate> spHttpNegotiate;
@@ skipping 54 matching lines @@
 }
 
 STDMETHODIMP WBPassthru::UnlockRequest()
 {
   if (!m_hasOriginalStartCalled)
   {
     return S_OK;
   }
   return BaseClass::UnlockRequest();
 }