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

test/WebRequest.cpp

Index: test/WebRequest.cpp
===================================================================
--- a/test/WebRequest.cpp
+++ b/test/WebRequest.cpp
@@ -21,41 +21,77 @@
 
 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;
+  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(){})"));
@@ -75,85 +111,181 @@ TEST_F(MockWebRequestTest, SuccessfulReq
 }
 
 #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);
 
+  ResetTestXHR(jsEngine);
   jsEngine->Evaluate("\
-    var result;\
-    var request = new XMLHttpRequest();\
-    request.open('GET', 'https://easylist-downloads.adblockplus.org/easylist.txt');\
     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);
 
+  ResetTestXHR(jsEngine);
   jsEngine->Evaluate("\
-    var result;\
-    var request = new XMLHttpRequest();\
-    request.open('GET', 'https://easylist-downloads.adblockplus.org/easylist.txt');\
     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 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
+  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
+  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.
+  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.
+  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"));
+}