Issue 1793 - check whether the frame is whitelisted before injecting CSS

src/plugin/PluginTabBase.cpp

Index: src/plugin/PluginTabBase.cpp
diff --git a/src/plugin/PluginTabBase.cpp b/src/plugin/PluginTabBase.cpp
index 9e7128041f9eb4adc9a9b674c3ab1dd249675654..6836cfa25f3c201920e1001eb176d3f7c7b570d7 100644
--- a/src/plugin/PluginTabBase.cpp
+++ b/src/plugin/PluginTabBase.cpp
@@ -8,6 +8,7 @@
 #include "PluginTabBase.h"
 #include "PluginUtil.h"
 #include "../shared/IE_version.h"
+#include "../shared/Utils.h"
 #include <dispex.h>
 #include <Mshtmhst.h>
 
@@ -116,6 +117,7 @@ void CPluginTabBase::InjectABP(IWebBrowser2* browser)
     DEBUG_ERROR_LOG(0, PLUGIN_ERROR_CREATE_SETTINGS_JAVASCRIPT, PLUGIN_ERROR_CREATE_SETTINGS_JAVASCRIPT_INVOKE, "CPluginTabBase::InjectABP - Failed to QI document");
     return;
   }
+
   CComPtr<IHTMLWindow2> pWnd2;
   pDoc2->get_parentWindow(&pWnd2);
   if (!pWnd2)
@@ -155,6 +157,95 @@ void CPluginTabBase::InjectABP(IWebBrowser2* browser)
   }
 }
 
+namespace
+{
+  void InjectABPCSS(IHTMLDocument2& htmlDocument2, const std::vector<std::wstring>& hideFilters)

[Eric, 2015/01/13 19:52:52]

We're not distinguishing between fatal errors and simple non-support here. If IE
9 is required, then simply check for it at the start and be done with it. Then
if the QI calls fail (for example, when converting from IHTMLStyleSheet to
IHTMLStyleSheet4) it can be properly designated as a fatal error by throwing an
exception.

I don't see anything that should fail here, assuming version prerequisites are
met. Thus the 'void' return type is appropriate. What's not correct, though, is
for failures to return rather than to throw. Alternatively, and it's not one I
like, is to declare the function 'bool' and return success/failure. Since this
is brand new code, there's no good reason not to do it right and move forward
(void/throw) rather than remain mired in the past (bool/return).

Overall, this function would be far easier to read with two modifications. (1)
Declare all the [out] argument variables immediately before use. (2) Add a blank
line separating each individual operation and its error handing.

[Oleksandr, 2015/03/13 10:00:27]

On 2015/01/13 19:52:52, Eric wrote:
What's not correct, though, is
I guess this code made into this codereview by accident, since InjectABPCSS is
not actually used anywhere. But still I disagree with the void/throw. In this
case in an unlikely scenario of anything going wrong it's better to just ignore
the error and keep doing what's next in the code rather than dropping everything
and unwinding the stack. This method should not throw, IMO.

[sergei, 2015/04/13 08:06:41]

I agree this function should not be in this code review.

I also think that throwing would hurt more than help here, so far let's discuss
it in http://codereview.adblockplus.org/6567422169448448/.

+  {
+    ATL::CComPtr<IHTMLElement> stylePureHtmlElement;
+    ATL::CComQIPtr<IHTMLStyleElement> styleHtmlElement;
+    if (FAILED(htmlDocument2.createElement(ATL::CComBSTR(L"style"), &stylePureHtmlElement)))
+    {
+      DEBUG_GENERAL(L"Cannot create style element");
+      return;
+    }
+    if (!(styleHtmlElement = stylePureHtmlElement))
+    {
+      DEBUG_GENERAL(L"Cannot obtain IHTMLStyleElement from IHTMLElement");
+      return;
+    }
+    if (FAILED(styleHtmlElement->put_type(ATL::CComBSTR("text/css"))))
+    {
+      DEBUG_GENERAL(L"Cannot set type text/css");
+      return;
+    }
+    ATL::CComQIPtr<IHTMLStyleSheet> styleSheet;
+    // IHTMLStyleElement2 is availabe starting from IE9, Vista
+    ATL::CComQIPtr<IHTMLStyleElement2> styleHtmlElement2 = styleHtmlElement;
+    if (!styleHtmlElement2)
+    {
+      DEBUG_GENERAL(L"Cannot obtain IHTMLStyleElement2 from IHTMLStyleElement");
+      return;
+    }
+    if (FAILED(styleHtmlElement2->get_sheet(&styleSheet)) || !styleSheet)
+    {
+      DEBUG_GENERAL(L"Cannot obtain IHTMLStyleSheet");
+      return;
+    }
+    // IHTMLStyleSheet4 is availabe starting from IE9, Vista
+    ATL::CComQIPtr<IHTMLStyleSheet4> styleSheet4 = styleSheet;
+    if (!styleSheet4)
+    {
+      DEBUG_GENERAL(L"Cannot obtain IHTMLStyleSheet4");
+      return;
+    }
+    long newIndex = 0;
+    std::wstring cssValue = L"{ display: none !important; }";
+    for (const auto& selector : hideFilters)
+    {
+      auto cssRule = selector + cssValue;
+      ATL::CComBSTR selector(cssRule.size(), cssRule.c_str());
+      if (SUCCEEDED(styleSheet4->insertRule(selector, newIndex, &newIndex)))
+      {
+        ++newIndex;
+      }
+      else
+      {
+        DEBUG_GENERAL(ToCString(L"Cannot add rule for selector " + cssRule));
+      }
+    }
+
+    ATL::CComQIPtr<IHTMLDOMNode> styleNode = stylePureHtmlElement;
+    if (!styleNode)
+    {
+      DEBUG_GENERAL(L"Cannot obtain IHTMLDOMNode from stylePureHtmlElement");
+      return;
+    }
+    ATL::CComPtr<IHTMLElement> headHtmlElement;
+    // IHTMLDocument7 is availabe starting from IE9, Vista
+    ATL::CComQIPtr<IHTMLDocument7> htmlDocument7 = &htmlDocument2;
+    if (!htmlDocument7)
+    {
+      DEBUG_GENERAL(L"Cannot obtain IHTMLDocument7 from htmlDocument2");
+      return;
+    }
+    if (FAILED(htmlDocument7->get_head(&headHtmlElement)))
+    {
+      DEBUG_GENERAL(L"Cannot obtain head from pDoc7");
+      return;
+    }
+    ATL::CComQIPtr<IHTMLDOMNode> headNode = headHtmlElement;
+    if (!headNode)
+    {
+      DEBUG_GENERAL(L"Cannot obtain headNode from headHtmlElement");
+      return;
+    }
+    if (FAILED(headNode->appendChild(styleNode, nullptr)))
+    {
+      DEBUG_GENERAL(L"Cannot append blocking style");
+    }
+  }
+}
+
 void CPluginTabBase::OnDownloadComplete(IWebBrowser2* browser)
 {
   CPluginClient* client = CPluginClient::GetInstance();
@@ -166,6 +257,52 @@ void CPluginTabBase::OnDownloadComplete(IWebBrowser2* browser)
   InjectABP(browser);
 }
 
+ATL::CComPtr<IWebBrowser2> GetParent(const ATL::CComPtr<IWebBrowser2>& browser)

[Eric, 2015/01/13 19:52:52]

Since you're not copying the CComPtr and incurring an Add()/Release() pair, you
can just pass this argument as "IWebBrowser2*".

[sergei, 2015/04/13 08:06:41]

fixed to use `IWebBrowser2&`.

+{
+  ATL::CComPtr<IWebBrowser2> retValue;
+  ATL::CComPtr<IDispatch> browserParentDispatch;
+  if (FAILED(browser->get_Parent(&browserParentDispatch)) || !browserParentDispatch)
+  {
+    return retValue;

[Eric, 2015/01/13 19:52:52]

"return nullptr" is much clearer here.

[sergei, 2015/04/13 08:06:41]

fixed

+  }
+  // The InternetExplorer application always returns a pointer to itself.
+  if (browserParentDispatch.IsEqualObject(browser))
+  {
+    return retValue;

[Eric, 2015/01/13 19:52:52]

As before "return nullptr"

[sergei, 2015/04/13 08:06:41]

fixed

+  }
+  ATL::CComQIPtr<IServiceProvider> parentDocumentServiceProvider = browserParentDispatch;
+  if (!parentDocumentServiceProvider)
+  {
+    return retValue;

[Eric, 2015/01/13 19:52:52]

return nullptr

[sergei, 2015/04/13 08:06:41]

fixed

+  }

[Eric, 2015/01/13 19:52:52]

This is the right place to declare 'retValue'.

As a rule, it's best to declare variables that receive [out] arguments
immediately before they're used, if only to ensure that operator& doesn't fail.

[sergei, 2015/04/13 08:06:41]

fixed

+  if (FAILED(parentDocumentServiceProvider->QueryService(IID_IWebBrowserApp, &retValue)) || !retValue)

[Oleksandr, 2015/03/13 10:00:27]

Why is this querying for IWebBrowserApp and not IWebBrowser2? Output expects
IWebBrowser2.

[sergei, 2015/04/13 08:06:41]

fixed

+  {
+    // error

[Eric, 2015/01/13 19:52:52]

You've got to do something here, since a failed COM call leaves an [out]
argument in an undefined state.

Best would be to throw an exception.

[Oleksandr, 2015/03/13 10:00:27]

return nullptr would be better IMO

[sergei, 2015/04/13 08:06:41]

I don't think we should expect undefined state of out-parameter, it will
complicate the things to unusable state, check the rules of COM world
https://msdn.microsoft.com/en-us/library/windows/desktop/ms686638(v=vs.85).aspx.

+  }
+  return retValue;
+}
+
+bool IsFrameWhiteListed(ATL::CComPtr<IWebBrowser2> frame)
+{
+  if (!frame)
+  {
+    return false;
+  }
+  CPluginClient* client = CPluginClient::GetInstance();
+  if (!client)
+  {
+    return false;

[Eric, 2015/01/13 19:52:52]

This is really a pretty serious error.

Personally, I'd prefer that GetInstance() throw in case of error (and thus never
return nullptr), but that's perhaps too drastic a change for the moment. At
least throw here.

+  }
+  auto url = GetLocationUrl(*frame);
+  std::vector<std::string> frameHierarchy;
+  for(frame = GetParent(frame); frame; frame = GetParent(frame))
+  {
+    frameHierarchy.push_back(ToUtf8String(GetLocationUrl(*frame)));
+  }
+  return client->IsWhitelistedUrl(url, frameHierarchy)
+      || client->IsElemhideWhitelistedOnDomain(url, frameHierarchy);
+}
+
 void CPluginTabBase::OnDocumentComplete(IWebBrowser2* browser, const CString& url, bool isDocumentBrowser)
 {
   CString documentUrl = GetDocumentUrl();