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
     {
-      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;
-      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();

[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";
+      if (!requestHeaders.empty())
+      {
+        result.responseText += requestHeaders[0].first + "\n" + requestHeaders[0].second;
+      }
       return result;
     }
 
-    // mutable. Very Ugly. But we are testing.
+    // 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<XHRTestWebRequest> XMLHttpRequestTest;
+  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
 {
@@ skipping 17 matching lines @@
     }
 
     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);

[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<XHRTestWebRequest>(jsEngine->GetWebRequest());
+    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
-  ResetTestXHR(jsEngine, catchLogSystem);
+  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);
-  WAIT_FOR_XHR_RESULT;
+  WaitForVariable("result", jsEngine);
   EXPECT_TRUE(webRequest->lastRequestHeaders.cend() ==
               webRequest->lastRequestHeaders.find("Accept-Encoding"));
 
   // test 'DNT' is rejected
-  ResetTestXHR(jsEngine, catchLogSystem);
+  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);
-  WAIT_FOR_XHR_RESULT;
+  WaitForVariable("result", jsEngine);
   EXPECT_TRUE(webRequest->lastRequestHeaders.cend() ==
               webRequest->lastRequestHeaders.find("DNT"));
 
   // test random 'X' header is accepted
-  ResetTestXHR(jsEngine, catchLogSystem);
+  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);
-  WAIT_FOR_XHR_RESULT;
+  WaitForVariable("result", jsEngine);
   EXPECT_FALSE(webRequest->lastRequestHeaders.cend() ==
                webRequest->lastRequestHeaders.find("X"));
 
   // test /^Proxy-/ is rejected.
-  ResetTestXHR(jsEngine, catchLogSystem);
+  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);
-  WAIT_FOR_XHR_RESULT;
+  WaitForVariable("result", jsEngine);
   EXPECT_TRUE(webRequest->lastRequestHeaders.cend() ==
               webRequest->lastRequestHeaders.find("Proxy-foo"));
 
   // test /^Sec-/ is rejected.
-  ResetTestXHR(jsEngine, catchLogSystem);
+  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);
-  WAIT_FOR_XHR_RESULT;
+  WaitForVariable("result", jsEngine);
   EXPECT_TRUE(webRequest->lastRequestHeaders.cend() ==
               webRequest->lastRequestHeaders.find("Sec-foo"));
 
   // test 'Security' is accepted.
-  ResetTestXHR(jsEngine, catchLogSystem);
+  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);
-  WAIT_FOR_XHR_RESULT;
+  WaitForVariable("result", jsEngine);
   EXPECT_FALSE(webRequest->lastRequestHeaders.cend() ==
                webRequest->lastRequestHeaders.find("Security"));
 }