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
@@ -46,16 +46,18 @@ namespace
       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;
 }
 
 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(){})"));
@@ -112,16 +114,19 @@ TEST_F(DefaultWebRequestTest, XMLHttpReq
   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
   {
@@ -152,8 +157,63 @@ TEST_F(DefaultWebRequestTest, XMLHttpReq
   } 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
+
+TEST_F(XMLHttpRequestTest, RequestHeaderValidation)
+{
+  AdblockPlus::FilterEngine filterEngine(jsEngine);
+
+  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');");
+
+  // 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());
+
+  // 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.

+
+  // 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());
+
+  // 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());
+}