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
@@ skipping 19 matching lines @@
 
       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;
       return result;
     }
   };
 
+  class XHRTestWebRequest : public AdblockPlus::WebRequest

[Felix Dahlke, 2017/03/06 16:51:07]

IMHO it'd make more sense to add lastRequestHeaders to MockWebRequest rather
than to add a new mock here.

[hub, 2017/03/06 17:26:05]

I can do that too. I was trying to avoid side effect my isolating test fixture,
even though I don't see any obvious one right now.

+  {
+  public:
+    AdblockPlus::ServerResponse GET(const std::string& url, const AdblockPlus::HeaderList& requestHeaders) const
+    {
+      lastRequestHeaders.clear();
+      for (auto header : requestHeaders)
+      {
+        lastRequestHeaders.insert(header.first);
+      }
+
+      AdblockPlus::ServerResponse result;
+      result.status = NS_OK;
+      result.responseStatus = 123;
+      result.responseHeaders.push_back(std::pair<std::string, std::string>("Foo", "Bar"));
+      return result;
+    }
+
+    // mutable. Very Ugly. But we are testing.
+    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<XHRTestWebRequest> XMLHttpRequestTest;
 }
 
 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)"));
@@ skipping 46 matching lines @@
     request.addEventListener('error', function() {result = 'error';}, false);\
     request.send(null);");
   do
   {
     AdblockPlus::Sleep(200);
   } while (jsEngine->Evaluate("result")->IsUndefined());
   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)
+  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());
@@ skipping 20 matching lines @@
   {
     AdblockPlus::Sleep(200);
   } while (jsEngine->Evaluate("result")->IsUndefined());
   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;
+
+  void
+  ResetTestXHR(const AdblockPlus::JsEnginePtr & jsEngine, const CatchLogSystemPtr & logger)

[Felix Dahlke, 2017/03/06 16:51:07]

Nit: `AdblockPlus::JsEnginePtr &` -> `AdblockPlus::JsEnginePtr&` (same
"snuggling" as with pointers).

[hub, 2017/03/06 17:26:05]

Acknowledged.

+  {
+    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);\
+    ");
+    logger->clear();
+  }
+}
+
+TEST_F(XMLHttpRequestTest, RequestHeaderValidation)
+{
+  #define WAIT_FOR_XHR_RESULT do\
+  {\
+    AdblockPlus::Sleep(60);\
+  } while (jsEngine->Evaluate("result")->IsUndefined())

[Felix Dahlke, 2017/03/06 16:51:08]

This is a bit of a footgun, wouldn't it work to just wait 200 ms after each
invocation as we do in the other tests? Since no actual request is happening
that shouldn't be shaky I believe.

[hub, 2017/03/06 17:26:05]

The only difference I see is that we'll check after 200ms rather than 60. Also
the difference is that MockWebRequest has a Sleep(50) in its GET method. But
then when I implement the change above, it will be there, so I can just change
the sleep to 200....

I was trying to avoid having the tests drag for too long needlessly. The while()
loop is here to change on a regular basis.

[Felix Dahlke, 2017/03/07 07:44:22]

Oh sorry, it seems I didn't get what I meant here across :) I surely see no
point in sleeping here for longer. Could probably be shorter than 60 ms the way
the code works now, since it's checking for `result`.

What I meant was: In the other tests we do not have logic for checking that the
XHR is actually finished. We just wait for 200 ms and that always seems to do
the trick. So what I suggested was: Get rid of `WAIT_FOR_XHR_RESULT` and replace
it with a single `AdblockPlus::Sleep(200)` call, which is how the other tests
seem to do it.

I can see the reasoning for not wanting to sleep an arbitrary amount of time and
hoping for the best, but I don't see (I might be missing something though) how
the problem is different in this test than it is in the others.

(What I meant by footgun: If someone calls `WAIT_FOR_XHR_RESULT` without first
calling `request.send()`, this will just hang indefinitely.)

[hub, 2017/03/07 15:52:32]

The other test have a similar loop. That's where I took it.
see lines: 100, 129, 146, 169, etc.

WAIT_FOR_XHR_RESULT is just to shorten typing. Not to do it differently. A
simple wait is IMHO doomed to fail. Either by just being too long or being too
short some of the time and fail.
Like any other programming error. Like when I actually forget to call send() and
had errors...
I'm not worried about that. 

If we are worried for indefinite hang we can set a global timeout and abort the
test with a fail in that case. But that's a different issue.

[Felix Dahlke, 2017/03/07 17:03:42]

Oh indeed, for some reason I missed that. Like I said, rusty :)

Given how we have that code in a few other test cases, how about we add a
function such as `WaitForVariable` (e.g. `WaitForVariable("result")`) or
something to that effect?
I guess we're fine, we can solve that issue when it occurs.

+
+  auto catchLogSystem = CatchLogSystemPtr(new CatchLogSystem);
+  jsEngine->SetLogSystem(catchLogSystem);
+
+  AdblockPlus::FilterEngine filterEngine(jsEngine);
+  auto webRequest =
+    std::static_pointer_cast<XHRTestWebRequest>(jsEngine->GetWebRequest());
+
+  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
+  ResetTestXHR(jsEngine, catchLogSystem);
+  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);
+  WAIT_FOR_XHR_RESULT;
+  EXPECT_TRUE(webRequest->lastRequestHeaders.cend() ==
+              webRequest->lastRequestHeaders.find("Accept-Encoding"));
+
+  // test 'DNT' is rejected
+  ResetTestXHR(jsEngine, catchLogSystem);
+  jsEngine->Evaluate("\
+    request.setRequestHeader('DNT', '1');\nrequest.send();");
+  EXPECT_EQ(AdblockPlus::LogSystem::LOG_LEVEL_WARN, catchLogSystem->lastLogLevel);
+  EXPECT_EQ(msg + "DNT", catchLogSystem->lastMessage);
+  WAIT_FOR_XHR_RESULT;
+  EXPECT_TRUE(webRequest->lastRequestHeaders.cend() ==
+              webRequest->lastRequestHeaders.find("DNT"));
+
+  // test random 'X' header is accepted
+  ResetTestXHR(jsEngine, catchLogSystem);
+  jsEngine->Evaluate("\
+    request.setRequestHeader('X', 'y');\nrequest.send();");
+  EXPECT_EQ(AdblockPlus::LogSystem::LOG_LEVEL_TRACE, catchLogSystem->lastLogLevel);
+  EXPECT_EQ("", catchLogSystem->lastMessage);
+  WAIT_FOR_XHR_RESULT;
+  EXPECT_FALSE(webRequest->lastRequestHeaders.cend() ==
+               webRequest->lastRequestHeaders.find("X"));
+
+  // test /^Proxy-/ is rejected.
+  ResetTestXHR(jsEngine, catchLogSystem);
+  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);
+  WAIT_FOR_XHR_RESULT;
+  EXPECT_TRUE(webRequest->lastRequestHeaders.cend() ==
+              webRequest->lastRequestHeaders.find("Proxy-foo"));
+
+  // test /^Sec-/ is rejected.
+  ResetTestXHR(jsEngine, catchLogSystem);
+  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);
+  WAIT_FOR_XHR_RESULT;
+  EXPECT_TRUE(webRequest->lastRequestHeaders.cend() ==
+              webRequest->lastRequestHeaders.find("Sec-foo"));
+
+  // test 'Security' is accepted.
+  ResetTestXHR(jsEngine, catchLogSystem);
+  jsEngine->Evaluate("\
+    request.setRequestHeader('Security', 'theater');\nrequest.send();");
+  EXPECT_EQ(AdblockPlus::LogSystem::LOG_LEVEL_TRACE, catchLogSystem->lastLogLevel);
+  EXPECT_EQ("", catchLogSystem->lastMessage);
+  WAIT_FOR_XHR_RESULT;
+  EXPECT_FALSE(webRequest->lastRequestHeaders.cend() ==
+               webRequest->lastRequestHeaders.find("Security"));
+}