Issue 4951 - Restrict request headers in XMLHttpRequest.Also test Accept-Encoding with th…

test/WebRequest.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 #include <sstream>
 #include "BaseJsTest.h"
 #include "../src/Thread.h"
 
 namespace
 {
   class MockWebRequest : public AdblockPlus::WebRequest
   {
   public:
     AdblockPlus::ServerResponse GET(const std::string& url, const AdblockPlus::HeaderList& requestHeaders) const
     {
+      lastRequestHeaders.clear();

[sergei, 2017/03/13 09:56:57]

Sorry, I forgot to say it again when my comments had been removed. I think we
should use some unique URL in test requests to distinguish them from another
requests sent from FilterEngine. May be we don't observe it on practice
currently but there is a race condition because the instance of MockWebRequest
is shared by all requests. BTW, there is also a data race on lastRequestHeaders
because the access to it is not synchronized.

[sergei, 2017/03/16 11:08:25]

What about this point? Actually I find it very important.

+      for (auto header : requestHeaders)
+      {
+        lastRequestHeaders.insert(header.first);
+      }
+
       AdblockPlus::Sleep(50);
 
       AdblockPlus::ServerResponse result;
       result.status = NS_OK;
       result.responseStatus = 123;
       result.responseHeaders.push_back(std::pair<std::string, std::string>("Foo", "Bar"));
-      result.responseText = url + "\n" + requestHeaders[0].first + "\n" + requestHeaders[0].second;
+      result.responseText = url + "\n";
+      if (!requestHeaders.empty())
+      {
+        result.responseText += requestHeaders[0].first + "\n" + requestHeaders[0].second;
+      }
       return result;
     }
+
+    // mutable. Very Ugly. But we are testing and need to change this in GET which is const.
+    mutable std::set<std::string> lastRequestHeaders;
   };
 
   template<class T>
   class WebRequestTest : public BaseJsTest
   {
   protected:
     void SetUp()
     {
       BaseJsTest::SetUp();
       jsEngine->SetWebRequest(AdblockPlus::WebRequestPtr(new T));
       jsEngine->SetFileSystem(AdblockPlus::FileSystemPtr(new LazyFileSystem));
     }
   };
 
   typedef WebRequestTest<MockWebRequest> MockWebRequestTest;
   typedef WebRequestTest<AdblockPlus::DefaultWebRequest> DefaultWebRequestTest;
-  // This test doesn't need a real WebRequest.
   typedef WebRequestTest<MockWebRequest> XMLHttpRequestTest;
+
+  void

[Felix Dahlke, 2017/03/09 15:10:58]

Nit: Why the new line? Not really consistent with the rest of the code.

[hub, 2017/03/09 15:33:02]

probably habit. I'll remove it when I send you the patch for checkin.

[sergei, 2017/03/13 09:56:56]

Just in case for future, I would rather prefer to have the final version in the
review tool.

+  ResetTestXHR(const AdblockPlus::JsEnginePtr& jsEngine)
+  {
+    jsEngine->Evaluate("\
+      var result;\
+      var request = new XMLHttpRequest();\
+      request.open('GET', 'https://easylist-downloads.adblockplus.org/easylist.txt');\
+      request.overrideMimeType('text/plain');\
+      request.addEventListener('load', function() {result = request.responseText;}, false);\
+      request.addEventListener('error', function() {result = 'error';}, false);\
+    ");
+  }
+
+  void WaitForVariable(const std::string& variable, const AdblockPlus::JsEnginePtr& jsEngine)
+  {
+    do
+    {
+      AdblockPlus::Sleep(60);

[sergei, 2017/03/13 09:56:56]

Since we touched it here I would say that I personally don't like these sleeps
and would like to get rid of them at some point. In this particular case I would
rather prefer something like the following:
inject a C++ function which sets a mutex and call it when either 'load' or
'error' event is fired, in the test body wait for that mutex. Instead of mutex
it would be better to use std::promise but it's broken in currently used visual
studio. BTW, as a positive side effect we will get rid of global result
variable.

[hub, 2017/03/13 14:16:51]

I'll file an issue for that.

[hub, 2017/03/13 14:29:32]

filed https://issues.adblockplus.org/ticket/4983

+    } while (jsEngine->Evaluate(variable)->IsUndefined());
+  }
+
 }
 
 TEST_F(MockWebRequestTest, BadCall)
 {
   ASSERT_ANY_THROW(jsEngine->Evaluate("_webRequest.GET()"));
   ASSERT_ANY_THROW(jsEngine->Evaluate("_webRequest.GET('', {}, function(){})"));
   ASSERT_ANY_THROW(jsEngine->Evaluate("_webRequest.GET({toString: false}, {}, function(){})"));
   ASSERT_ANY_THROW(jsEngine->Evaluate("_webRequest.GET('http://example.com/', null, function(){})"));
   ASSERT_ANY_THROW(jsEngine->Evaluate("_webRequest.GET('http://example.com/', {}, null)"));
   ASSERT_ANY_THROW(jsEngine->Evaluate("_webRequest.GET('http://example.com/', {}, function(){}, 0)"));
 }
 
 TEST_F(MockWebRequestTest, SuccessfulRequest)
 {
   jsEngine->Evaluate("_webRequest.GET('http://example.com/', {X: 'Y'}, function(result) {foo = result;} )");
   ASSERT_TRUE(jsEngine->Evaluate("this.foo")->IsUndefined());
   AdblockPlus::Sleep(200);
   ASSERT_EQ(AdblockPlus::WebRequest::NS_OK, jsEngine->Evaluate("foo.status")->AsInt());
   ASSERT_EQ(123, jsEngine->Evaluate("foo.responseStatus")->AsInt());
   ASSERT_EQ("http://example.com/\nX\nY", jsEngine->Evaluate("foo.responseText")->AsString());
   ASSERT_EQ("{\"Foo\":\"Bar\"}", jsEngine->Evaluate("JSON.stringify(foo.responseHeaders)")->AsString());
 }
 
 #if defined(HAVE_CURL) || defined(_WIN32)
 TEST_F(DefaultWebRequestTest, RealWebRequest)
 {
   // This URL should redirect to easylist-downloads.adblockplus.org and we
   // should get the actual filter list back.
   jsEngine->Evaluate("_webRequest.GET('https://easylist-downloads.adblockplus.org/easylist.txt', {}, function(result) {foo = result;} )");
-  do
-  {
-    AdblockPlus::Sleep(200);
-  } while (jsEngine->Evaluate("this.foo")->IsUndefined());
+  WaitForVariable("this.foo", jsEngine);
   ASSERT_EQ("text/plain", jsEngine->Evaluate("foo.responseHeaders['content-type'].substr(0, 10)")->AsString());
   ASSERT_EQ(AdblockPlus::WebRequest::NS_OK, jsEngine->Evaluate("foo.status")->AsInt());
   ASSERT_EQ(200, jsEngine->Evaluate("foo.responseStatus")->AsInt());
   ASSERT_EQ("[Adblock Plus ", jsEngine->Evaluate("foo.responseText.substr(0, 14)")->AsString());
   ASSERT_EQ("text/plain", jsEngine->Evaluate("foo.responseHeaders['content-type'].substr(0, 10)")->AsString());
 #if defined(HAVE_CURL)
   ASSERT_EQ("gzip", jsEngine->Evaluate("foo.responseHeaders['content-encoding'].substr(0, 4)")->AsString());
 #endif
   ASSERT_TRUE(jsEngine->Evaluate("foo.responseHeaders['location']")->IsUndefined());
 }
 
 TEST_F(DefaultWebRequestTest, XMLHttpRequest)
 {
   AdblockPlus::FilterEngine filterEngine(jsEngine);
 
-  jsEngine->Evaluate("\
-    var result;\
-    var request = new XMLHttpRequest();\
-    request.open('GET', 'https://easylist-downloads.adblockplus.org/easylist.txt');\
+  ResetTestXHR(jsEngine);
+  jsEngine->Evaluate("\
     request.setRequestHeader('X', 'Y');\
     request.setRequestHeader('X2', 'Y2');\
-    request.overrideMimeType('text/plain');\
-    request.addEventListener('load', function() {result = request.responseText;}, false);\
-    request.addEventListener('error', function() {result = 'error';}, false);\
     request.send(null);");
-  do
-  {
-    AdblockPlus::Sleep(200);
-  } while (jsEngine->Evaluate("result")->IsUndefined());
+  WaitForVariable("result", jsEngine);
   ASSERT_EQ(AdblockPlus::WebRequest::NS_OK, jsEngine->Evaluate("request.channel.status")->AsInt());
   ASSERT_EQ(200, jsEngine->Evaluate("request.status")->AsInt());
   ASSERT_EQ("[Adblock Plus ", jsEngine->Evaluate("result.substr(0, 14)")->AsString());
   ASSERT_EQ("text/plain", jsEngine->Evaluate("request.getResponseHeader('Content-Type').substr(0, 10)")->AsString());
 #if defined(HAVE_CURL)

[sergei, 2017/03/13 09:56:56]

It seems, it should be in commit
https://github.com/adblockplus/libadblockplus/commit/8ba0148ea850a278a2a9c659...
or at least in a new commit but not in this one.

   ASSERT_EQ("gzip", jsEngine->Evaluate("request.getResponseHeader('Content-Encoding').substr(0, 4)")->AsString());
 #endif
   ASSERT_TRUE(jsEngine->Evaluate("request.getResponseHeader('Location')")->IsNull());
 }
 #else
 TEST_F(DefaultWebRequestTest, DummyWebRequest)
 {
   jsEngine->Evaluate("_webRequest.GET('https://easylist-downloads.adblockplus.org/easylist.txt', {}, function(result) {foo = result;} )");
-  do
-  {
-    AdblockPlus::Sleep(200);
-  } while (jsEngine->Evaluate("this.foo")->IsUndefined());
+  WaitForVariable("this.foo", jsEngine);
   ASSERT_EQ(AdblockPlus::WebRequest::NS_ERROR_FAILURE, jsEngine->Evaluate("foo.status")->AsInt());
   ASSERT_EQ(0, jsEngine->Evaluate("foo.responseStatus")->AsInt());
   ASSERT_EQ("", jsEngine->Evaluate("foo.responseText")->AsString());
   ASSERT_EQ("{}", jsEngine->Evaluate("JSON.stringify(foo.responseHeaders)")->AsString());
 }
 
 TEST_F(DefaultWebRequestTest, XMLHttpRequest)
 {
   AdblockPlus::FilterEngine filterEngine(jsEngine);
 
-  jsEngine->Evaluate("\
-    var result;\
-    var request = new XMLHttpRequest();\
-    request.open('GET', 'https://easylist-downloads.adblockplus.org/easylist.txt');\
+  ResetTestXHR(jsEngine);
+  jsEngine->Evaluate("\
     request.setRequestHeader('X', 'Y');\
-    request.overrideMimeType('text/plain');\
-    request.addEventListener('load', function() {result = request.responseText;}, false);\
-    request.addEventListener('error', function() {result = 'error';}, false);\
     request.send(null);");
-  do
-  {
-    AdblockPlus::Sleep(200);
-  } while (jsEngine->Evaluate("result")->IsUndefined());
+  WaitForVariable("result", jsEngine);
   ASSERT_EQ(AdblockPlus::WebRequest::NS_ERROR_FAILURE, jsEngine->Evaluate("request.channel.status")->AsInt());
   ASSERT_EQ(0, jsEngine->Evaluate("request.status")->AsInt());
   ASSERT_EQ("error", jsEngine->Evaluate("result")->AsString());
   ASSERT_TRUE(jsEngine->Evaluate("request.getResponseHeader('Content-Type')")->IsNull());
 }
 
 #endif
 
+namespace
+{
+  class CatchLogSystem : public AdblockPlus::LogSystem
+  {
+  public:
+    AdblockPlus::LogSystem::LogLevel lastLogLevel;
+    std::string lastMessage;
+
+    CatchLogSystem()
+      : AdblockPlus::LogSystem(),
+        lastLogLevel(AdblockPlus::LogSystem::LOG_LEVEL_TRACE)
+    {
+    }
+
+    void operator()(AdblockPlus::LogSystem::LogLevel logLevel,
+        const std::string& message, const std::string&)
+    {
+      lastLogLevel = logLevel;
+      lastMessage = message;
+    }
+
+    void clear()
+    {
+      lastLogLevel = AdblockPlus::LogSystem::LOG_LEVEL_TRACE;
+      lastMessage.clear();
+    }
+  };
+
+  typedef std::shared_ptr<CatchLogSystem> CatchLogSystemPtr;
+}
+
 TEST_F(XMLHttpRequestTest, RequestHeaderValidation)
 {
+  auto catchLogSystem = CatchLogSystemPtr(new CatchLogSystem);

[sergei, 2017/03/13 09:56:56]

Could we please always use either () or {} for constructors? BTW, {} is not
fully supported by currently used Visual Studio.

[sergei, 2017/03/13 09:56:57]

I would prefer to have std::make_shared<CatchLogSystem>() or at least
CatchLogSystemPtr catchLogSystem = new CatchLogSystem(); than
CatchLogSystemPtr(new CatchLogSystem);.

[Felix Dahlke, 2017/03/13 10:05:47]

I definitely prefer omitting these when not necessary, our (i.e. Mozilla's)
coding style just leaves it up to the author AFAIK.

[sergei, 2017/03/13 10:31:02]

It seems Mozilla's coding style does not mention it but I'm afraid it can lead
to some uninitialized plain members of objects, so I would like to always have
value initialization.

[Felix Dahlke, 2017/03/13 11:05:42]

We are talking a bout `new CatchLogSystem` vs `new CatchLogSystem()`? I'm a bit
rusty so I may be missing something, but I don't think there's any semantic
difference here. {} is of course a different story.

[sergei, 2017/03/13 11:39:16]

Yes.
In C++98 and C++03 `new T` and `new T()` were different and the definition of
default initialization has been changed between C++98, C++03 and C++11 (I cannot
say anything about C++14 and C++17 but I am ready to expect it to be changed
there again). However, if we always use value initialization (either `()` or
`{}`) then we are definitely on the safe side.

[Felix Dahlke, 2017/03/13 11:55:06]

I see - rusty after all :P Looking at our existing code, it appears we have just
a single occurrence without parentheses (also in the Libadblockplus tests), so
we could add a rule about that. Feel free to create a patch for
adblockplus.org/coding-style.

[sergei, 2017/03/13 12:19:58]

On 2017/03/13 11:55:06, Felix Dahlke wrote:
...
I would like to firstly upgrade compilers (in particular android NDK and Visual
Studio) and then we can define a subset of new features and techniques which
should be used.

[hub, 2017/03/13 14:16:51]

I see two occurences, line 60 and 61 of this file.

[hub, 2017/03/13 14:16:51]

I did submit a follow up patch to address these. In a new review.

https://codereview.adblockplus.org/29382567

+  jsEngine->SetLogSystem(catchLogSystem);
+
   AdblockPlus::FilterEngine filterEngine(jsEngine);
+  auto webRequest =
+    std::static_pointer_cast<MockWebRequest>(jsEngine->GetWebRequest());

[sergei, 2017/03/13 09:56:56]

Although it's fine here, I would rather prefer to have MockWebRequestPtr as a
member of test fixture and avoid any unnecessary down-casting.

+
+  ASSERT_TRUE(webRequest);
 
   const std::string msg = "Attempt to set a forbidden header was denied: ";
-  // The test will override console.warning so that the

[sergei, 2017/03/02 22:25:31]

I think we should rather check in WebRequest::GET that there are no forbidden
headers and other headers are indeed present.

In addition I think we should leave the check of the warnings and although I
have some concerns about overriding of console.warn here instead of using
LogSystem I think it's fine in tests.

[hub, 2017/03/02 23:30:13]

I didn't realize there was LogSystem to check for that. And given the other
mistake (console.warning instead of console.warn), I would never have realized.
Will definitely fix that.

[sergei, 2017/03/03 08:38:59]

What about check in WebRequest::GET?

-  // header rejection cause result to be set.
-  // While this is an implementation detail, since the DOM API

[sergei, 2017/03/02 22:25:31]

Could you please remove "DOM" because it's not a DOM API.

[hub, 2017/03/02 23:30:13]

Acknowledged.

-  // doesn't seem to return anything, we have no other way to check
-  // the failure.
-  jsEngine->Evaluate("\
-    var result;\
-    console.warning = function(msg) { result = msg; };\
-    var request = new XMLHttpRequest();\
-    request.open('GET', 'https://easylist-downloads.adblockplus.org/easylist.txt');");
+
+  // The test will check that console.warn has been called when the
+  // header is rejected. While this is an implementation detail, we
+  // have no other way to check this
 
   // test 'Accept-Encoding' is rejected

[sergei, 2017/03/02 22:25:31]

What about having several tests, at least checking forbidden headers and any
other headers? BTW, comments look very good for test name, although
unfortunately we are not using this approach.

-  jsEngine->Evaluate("\
-    result = undefined;\
-    request.setRequestHeader('Accept-Encoding', 'gzip');");
-  auto value = jsEngine->Evaluate("result");
-  ASSERT_FALSE(value->IsUndefined());
-  ASSERT_EQ(msg + "Accept-Encoding", value->AsString());
+  catchLogSystem->clear();
+  ResetTestXHR(jsEngine);
+  jsEngine->Evaluate("\
+    request.setRequestHeader('Accept-Encoding', 'gzip');\nrequest.send();");
+  EXPECT_EQ(AdblockPlus::LogSystem::LOG_LEVEL_WARN, catchLogSystem->lastLogLevel);
+  EXPECT_EQ(msg + "Accept-Encoding", catchLogSystem->lastMessage);
+  WaitForVariable("result", jsEngine);
+  EXPECT_TRUE(webRequest->lastRequestHeaders.cend() ==
+              webRequest->lastRequestHeaders.find("Accept-Encoding"));
+
+  // test 'DNT' is rejected
+  catchLogSystem->clear();
+  ResetTestXHR(jsEngine);
+  jsEngine->Evaluate("\
+    request.setRequestHeader('DNT', '1');\nrequest.send();");
+  EXPECT_EQ(AdblockPlus::LogSystem::LOG_LEVEL_WARN, catchLogSystem->lastLogLevel);
+  EXPECT_EQ(msg + "DNT", catchLogSystem->lastMessage);
+  WaitForVariable("result", jsEngine);
+  EXPECT_TRUE(webRequest->lastRequestHeaders.cend() ==
+              webRequest->lastRequestHeaders.find("DNT"));
 
   // test random 'X' header is accepted
-  jsEngine->Evaluate("\
-    result = undefined;\
-    request.setRequestHeader('X', 'y');");
-  value = value = jsEngine->Evaluate("result");

[sergei, 2017/03/02 22:25:31]

value = value =

[hub, 2017/03/02 23:30:13]

Acknowledged.
Cut&paste error. I missed it.

-  ASSERT_TRUE(value->IsUndefined());

[sergei, 2017/03/02 22:25:31]

I think we should use EXPECT_* when the test can continue the execution.

[hub, 2017/03/02 23:30:12]

Acknowledged.

+  catchLogSystem->clear();
+  ResetTestXHR(jsEngine);
+  jsEngine->Evaluate("\
+    request.setRequestHeader('X', 'y');\nrequest.send();");
+  EXPECT_EQ(AdblockPlus::LogSystem::LOG_LEVEL_TRACE, catchLogSystem->lastLogLevel);
+  EXPECT_EQ("", catchLogSystem->lastMessage);
+  WaitForVariable("result", jsEngine);
+  EXPECT_FALSE(webRequest->lastRequestHeaders.cend() ==
+               webRequest->lastRequestHeaders.find("X"));
 
   // test /^Proxy-/ is rejected.
-  jsEngine->Evaluate("\
-    result = undefined;\
-    request.setRequestHeader('Proxy-foo', 'bar');");
-  value = value = jsEngine->Evaluate("result");
-  ASSERT_FALSE(value->IsUndefined());
-  ASSERT_EQ(msg + "Proxy-foo", value->AsString());
+  catchLogSystem->clear();
+  ResetTestXHR(jsEngine);
+  jsEngine->Evaluate("\
+    request.setRequestHeader('Proxy-foo', 'bar');\nrequest.send();");
+  EXPECT_EQ(AdblockPlus::LogSystem::LOG_LEVEL_WARN, catchLogSystem->lastLogLevel);
+  EXPECT_EQ(msg + "Proxy-foo", catchLogSystem->lastMessage);
+  WaitForVariable("result", jsEngine);
+  EXPECT_TRUE(webRequest->lastRequestHeaders.cend() ==
+              webRequest->lastRequestHeaders.find("Proxy-foo"));
 
   // test /^Sec-/ is rejected.
-  jsEngine->Evaluate("\
-    result = undefined;\
-    request.setRequestHeader('Sec-foo', 'bar');");
-  value = value = jsEngine->Evaluate("result");
-  ASSERT_FALSE(value->IsUndefined());
-  ASSERT_EQ(msg + "Sec-foo", value->AsString());
-
-  // test 'Security' is rejected.
-  jsEngine->Evaluate("\
-    result = undefined;\
-    request.setRequestHeader('Security', 'theater');");
-  value = value = jsEngine->Evaluate("result");
-  ASSERT_TRUE(value->IsUndefined());
-}
+  catchLogSystem->clear();
+  ResetTestXHR(jsEngine);
+  jsEngine->Evaluate("\
+    request.setRequestHeader('Sec-foo', 'bar');\nrequest.send();");
+  EXPECT_EQ(AdblockPlus::LogSystem::LOG_LEVEL_WARN, catchLogSystem->lastLogLevel);
+  EXPECT_EQ(msg + "Sec-foo", catchLogSystem->lastMessage);
+  WaitForVariable("result", jsEngine);
+  EXPECT_TRUE(webRequest->lastRequestHeaders.cend() ==
+              webRequest->lastRequestHeaders.find("Sec-foo"));
+
+  // test 'Security' is accepted.
+  catchLogSystem->clear();
+  ResetTestXHR(jsEngine);
+  jsEngine->Evaluate("\
+    request.setRequestHeader('Security', 'theater');\nrequest.send();");
+  EXPECT_EQ(AdblockPlus::LogSystem::LOG_LEVEL_TRACE, catchLogSystem->lastLogLevel);
+  EXPECT_EQ("", catchLogSystem->lastMessage);
+  WaitForVariable("result", jsEngine);
+  EXPECT_FALSE(webRequest->lastRequestHeaders.cend() ==
+               webRequest->lastRequestHeaders.find("Security"));
+}