[superseded] Issue 1265 - Improve detection of mime type of the expected response

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 32 matching lines @@
     return CFilter::contentTypeScript;
   }
   if (mimeType.Find(L"application/x-shockwave-flash") >= 0)
   {
     return CFilter::contentTypeObject;
   }
   if (mimeType.Find(L"text/html") >= 0)
   {
     return CFilter::contentTypeSubdocument;
   }
+  // It is important to have this check last, since it is rather generic, and might overlay text/html, for example
+  if (mimeType.Find(L"xml") >= 0)
+  {
+    return CFilter::contentTypeXmlHttpRequest;
+  }
+
   return CFilter::contentTypeAny;
 }
 
 int WBPassthruSink::GetContentTypeFromURL(const CString& src)
 {
   CString srcExt = src;
 
   int pos = 0;
   if ((pos = src.Find('?')) > 0)
   {
     srcExt = src.Left(pos);
   }
 
   int lastDotIndex = srcExt.ReverseFind('.');
   if (lastDotIndex < 0)
     return CFilter::contentTypeAny;
   CString ext = srcExt.Mid(lastDotIndex);
   if (ext == L".jpg" || ext == L".gif" || ext == L".png" || ext == L".jpeg")
   {
     return CFilter::contentTypeImage;
   }
   else if (ext == L".css")
   {
     return CFilter::contentTypeStyleSheet;
   }
   else if (ext.Right(3) == L".js")
   {
     return CFilter::contentTypeScript;
+  }
+  else if (ext == L".xml")
+  {
+    return CFilter::contentTypeXmlHttpRequest;
   }
   else if (ext == L".swf")
   {
     return CFilter::contentTypeObject;
   }
   else if (ext == L".jsp" || ext == L".php" || ext == L".html")
   {
     return CFilter::contentTypeSubdocument;
   }
   return CFilter::contentTypeAny;
@@ skipping 83 matching lines @@
       }
       else
 #endif // SUPPORT_FRAME_CACHING
       {
         m_contentType = GetContentType(mimeType, m_boundDomain, src);
       }
     }
     
   }
 
   if (nullptr == tab)

[Oleksandr, 2014/10/01 09:15:39]

I personally don't like yoda conditions, and I think we have it in our coding
style not to use them. 

if (tab == nullptr) looks better, imho.

   {
     m_contentType = GetContentType(mimeType, m_boundDomain, src);
   }
 
   if (nullptr != client
     && CFilter::EContentType::contentTypeAny != m_contentType

[Oleksandr, 2014/09/11 08:58:54]

This line is not clear to me. We can in theory block requests that have
contentTypeAny as well.

[sergei, 2014/09/11 09:53:25]

If the type is `contentTypeAny` then the decision is made in
`BeginningTransaction`, it will be blocked there.

     && client->ShouldBlock(src, m_contentType, m_boundDomain, true))

[Oleksandr, 2014/10/01 09:15:39]

Comment about yoda conditions applies here as well. And further below for all
the m_contentType checks.

   {
     isBlocked = true;
-    DEBUG_BLOCKER("Blocker::Blocking Http-request:" + src);

[Oleksandr, 2014/09/11 08:58:54]

DEBUG_BLOCKER is broken here. Either use CPluginDebug::DebugResultBlocking
directly (prefered), or fix the DEBUG_BLOCKER. The debug build should write the
output to debug_result.txt

[sergei, 2014/09/11 09:53:25]

`client->ShouldBlock` already puts into the log. `DEBUG_BLOCKER` is removed.

-  }
-  if (!isBlocked)
-  {
-    DEBUG_BLOCKER("Blocker::Ignoring Http-request:" + src)

[Oleksandr, 2014/09/11 08:58:54]

CPluginDebug::DebugResultIgnoring here accordingly

[sergei, 2014/09/11 09:53:25]

The same as above, deleted.

   }
 
   // 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)
     {
       BaseClass::OnStart(szUrl, pOIProtSink, pOIBindInfo, grfPI, dwReserved, pTargetProtocol);
       // 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;
       BaseClass::OnStart(szUrl, pOIProtSink, pOIBindInfo, grfPI, dwReserved, pTargetProtocol);
       m_spInternetProtocolSink->ReportProgress(BINDSTATUS_MIMETYPEAVAILABLE, L"text/html");
       m_spInternetProtocolSink->ReportData(BSCF_FIRSTDATANOTIFICATION, 0, static_cast<ULONG>(g_myPageBlocked.size()));
       return S_OK;
     } 
     if (CFilter::EContentType::contentTypeScript == m_contentType)
     {
       BaseClass::OnStart(szUrl, pOIProtSink, pOIBindInfo, grfPI, dwReserved, pTargetProtocol);
       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)

[Oleksandr, 2014/10/01 09:15:39]

Looks like we don't need a special case for contentTypeXmlHttpRequest do we? The
'if' below will do the same anyway.

+    {
+      BaseClass::OnStart(szUrl, pOIProtSink, pOIBindInfo, grfPI, dwReserved, pTargetProtocol);
+      m_spInternetProtocolSink->ReportResult(INET_E_REDIRECTING, 301, L"data:");
+      return INET_E_REDIRECT_FAILED;
+    }
+    if (CFilter::EContentType::contentTypeAny != m_contentType)
     {
       BaseClass::OnStart(szUrl, pOIProtSink, pOIBindInfo, grfPI, dwReserved, pTargetProtocol);
       m_spInternetProtocolSink->ReportResult(INET_E_REDIRECTING, 301, L"data:");

[Oleksandr, 2014/10/01 09:15:39]

I don't think redirecting all requests to 'data:' is a good idea. This should be
done only for predefined types (contentTypeObject and
contentTypeObjectSubrequest, I think). Otherwise IE might present a download
prompt to the user, if we block CSS, for example. Redirecting to
res://mshtml.dll/blank.htm is a safer option here from my experience.

       return INET_E_REDIRECT_FAILED;
     }
   }
 #endif // SUPPORT_FILTER
 
   return isBlocked ? S_FALSE : BaseClass::OnStart(szUrl, pOIProtSink, pOIBindInfo, grfPI, dwReserved, pTargetProtocol);
 }
 
 HRESULT WBPassthruSink::OnRead(void* pv, ULONG cb, ULONG* pcbRead)
 {
@@ skipping 63 matching lines @@
 STDMETHODIMP WBPassthruSink::BeginningTransaction(LPCWSTR szURL, LPCWSTR szHeaders, DWORD dwReserved, LPWSTR* pszAdditionalHeaders)
 {
   if (pszAdditionalHeaders)
   {
     *pszAdditionalHeaders = nullptr;
   }
 
   CPluginClient* client = nullptr;
   if (CFilter::EContentType::contentTypeAny == m_contentType && (client = CPluginClient::GetInstance()))
   {
     auto acceptHeader = [&]() -> std::string

[Oleksandr, 2014/09/11 08:58:54]

I'd just like to say this looks too cool :D

[sergei, 2014/09/11 09:53:25]

Thanks!

     {
       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,
@@ skipping 17 matching lines @@
       }
       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,
-      //client->GetHostFromUrl(m_boundDomain.operator LPCWSTR()).c_str(), /*debug flag but must be set*/true);

[Oleksandr, 2014/09/11 08:58:54]

Leftover code here?

[sergei, 2014/09/11 09:53:25]

Thanks, removed.

-      m_boundDomain, /*debug flag but must be set*/true);
+    bool isBlocked = client->ShouldBlock(szURL, m_contentType, m_boundDomain, /*debug flag but must be set*/true);
     if (isBlocked)
     {

[Oleksandr, 2014/09/11 08:58:54]

Log blocking here as well

[sergei, 2014/09/11 09:53:25]

`client->ShouldBlock` puts into log

       return E_ABORT;
     }
   }
   CComPtr<IHttpNegotiate> spHttpNegotiate;
   QueryServiceFromClient(&spHttpNegotiate);
   return spHttpNegotiate ? spHttpNegotiate->BeginningTransaction(szURL, szHeaders,dwReserved, pszAdditionalHeaders) : S_OK;
 }
 
 STDMETHODIMP WBPassthruSink::OnResponse(DWORD dwResponseCode, LPCWSTR szResponseHeaders, LPCWSTR szRequestHeaders, LPWSTR *pszAdditionalRequestHeaders)
 {
@@ skipping 46 matching lines @@
 }
 
 STDMETHODIMP WBPassthru::UnlockRequest()
 {
   if (m_shouldSupplyCustomContent)
   {
     return S_OK;
   }
   return PassthroughAPP::CInternetProtocol<WBStartPolicy>::UnlockRequest();
 }