DNR: Prevent web request extensions from adding removed headers.

r652224 added initial support for removing headers through the Declarative Net
Request API. This CL fixes an existing TODO by preventing web request extensions
from adding back headers removed by DNR.

BUG=947591

Change-Id: I8a6eb4bd3ae8d9bdd556feefb454cbd30944aa74
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1565613
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Auto-Submit: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#652646}

chrome/test/data/extensions/api_test/declarative_net_request/header_removal/background.js

@@ -27,6 +27,17 @@ function checkHasHeader(headers, headerName) {
   return !!headers.find(header => header.name.toLowerCase() == headerName);
 }
 
+// Adds or updates the given header name/value to |headers|.
+function addOrUpdateHeader(headers, headerName, headerValue) {
+  var index =
+      headers.findIndex(header => header.name.toLowerCase() == headerName);
+  if (index != -1) {
+    headers[index].value = headerValue;
+  } else {
+    headers.push({name: headerName, value: headerValue});
+  }
+}
+
 // Checks whether the cookie request header was removed from the request and
 // that it isn't visible to web request listeners. Then proceeds to the next
 // test.
@@ -65,6 +76,38 @@ function checkCookieHeaderRemoved(expectRemoved) {
   });
 }
 
+// Removes all the cookies and optionally checks if |optCurrentCookiesSet|
+// corresponds to the current cookies. Returns a promise.
+function checkAndResetCookies(optCurrentCookiesSet) {
+  var removeCookiesPromise =
+      function(cookieParams) {
+    return new Promise((resolve, reject) => {
+      chrome.cookies.remove(cookieParams, function(details) {
+        chrome.test.assertNoLastError();
+        resolve();
+      });
+    });
+  }
+
+  var url = getServerURL('');
+
+  return new Promise((resolve, reject) => {
+    chrome.cookies.getAll({url: url}, function(cookies) {
+      if (optCurrentCookiesSet) {
+        chrome.test.assertEq(cookies.length, optCurrentCookiesSet.size);
+        for (var i = 0; i < cookies.length; ++i)
+          chrome.test.assertTrue(optCurrentCookiesSet.has(cookies[i].name));
+      }
+
+      var promises = [];
+      for (var i = 0; i < cookies.length; ++i)
+        promises.push(removeCookiesPromise({url: url, name: cookies[i].name}));
+
+      Promise.all(promises).then(resolve, reject);
+    });
+  });
+}
+
 // Checks whether the set-cookie response header was removed from the request
 // and that it isn't visible to web request listeners. Then proceeds to the next
 // test.
@@ -90,42 +133,111 @@ function checkSetCookieHeaderRemoved(expectRemoved) {
         !expectRemoved, checkHasHeader(details.responseHeaders, 'set-cookie'));
   }, filter, extraInfoSpec);
 
-  var removeCookiesPromise =
-      function(cookieParams) {
-    return new Promise((resolve, reject) => {
-      chrome.cookies.remove(cookieParams, function(details) {
-        chrome.test.assertNoLastError();
-        resolve();
-      });
-    });
-  }
-
-  var removeCookie1 =
-      removeCookiesPromise({url: getServerURL(''), 'name': 'foo1'});
-  var removeCookie2 =
-      removeCookiesPromise({url: getServerURL(''), 'name': 'foo2'});
 
   // Clear cookies from existing tests.
-  Promise.all([removeCookie1, removeCookie2]).then(function() {
+  checkAndResetCookies().then(function() {
     navigateTab(setCookieUrl, function(tab) {
       chrome.test.assertTrue(onHeadersReceivedSeen);
       chrome.test.assertTrue(onResponseStartedSeen);
-      chrome.cookies.getAll({url: getServerURL('')}, function(cookies) {
-        if (expectRemoved) {
-          chrome.test.assertEq(0, cookies.length);
-        } else {
-          chrome.test.assertEq(2, cookies.length);
-          chrome.test.assertTrue(
-              !!cookies.find(cookie => cookie.name === 'foo1'));
-          chrome.test.assertTrue(
-              !!cookies.find(cookie => cookie.name === 'foo2'));
-        }
+
+      var expectedCookies = expectRemoved ? [] : ['foo1', 'foo2'];
+      checkAndResetCookies(new Set(expectedCookies)).then(chrome.test.succeed);
+    });
+  });
+}
+
+// Checks whether the cookie request header added by Web request extension was
+// removed. Then proceeds to the next test.
+function checkAddWebRequestCookie(expectRemoved) {
+  var echoCookieUrl = getServerURL('echoheader?cookie');
+
+  // Register web request listeners for |echoCookieUrl|.
+  var filter = {urls: [echoCookieUrl]};
+  var extraInfoSpec = ['requestHeaders', 'extraHeaders', 'blocking'];
+  var onBeforeSendHeadersSeen = false;
+  var onBeforeSendHeadersListener = function listener(details) {
+    onBeforeSendHeadersSeen = true;
+    addOrUpdateHeader(details.requestHeaders, 'cookie', 'webRequest=true');
+    return {requestHeaders: details.requestHeaders};
+  };
+  chrome.webRequest.onBeforeSendHeaders.addListener(
+      onBeforeSendHeadersListener, filter, extraInfoSpec);
+
+  var onActionIgnoredCalled = false;
+  var onActionIgnoredListener = function(details) {
+    onActionIgnoredCalled = true;
+    chrome.test.assertEq('request_headers', details.action);
+  };
+  chrome.webRequest.onActionIgnored.addListener(onActionIgnoredListener);
+
+  navigateTab(echoCookieUrl, function(tab) {
+    chrome.webRequest.onBeforeSendHeaders.removeListener(
+        onBeforeSendHeadersListener);
+    chrome.webRequest.onActionIgnored.removeListener(onActionIgnoredListener);
+
+    chrome.test.assertTrue(onBeforeSendHeadersSeen);
+    chrome.test.assertEq(expectRemoved, onActionIgnoredCalled);
+
+    chrome.tabs.executeScript(
+        tab.id, {code: 'document.body.innerText'}, function(results) {
+          chrome.test.assertNoLastError();
+          chrome.test.assertEq(
+              expectRemoved ? 'None' : 'webRequest=true', results[0]);
           chrome.test.succeed();
         });
   });
+}
+
+// Checks whether the set-cookie request header added by Web request extension
+// was removed.
+function checkAddWebRequestSetCookie(expectRemoved) {
+  var url = getServerURL('echo');
+
+  // Register web request listeners for |url|.
+  var filter = {urls: [url]};
+  var extraInfoSpec = ['responseHeaders', 'extraHeaders', 'blocking'];
+  var onHeadersReceivedSeen = false;
+  var onHeadersReceivedListener = function listener(details) {
+    onHeadersReceivedSeen = true;
+    addOrUpdateHeader(details.responseHeaders, 'set-cookie', 'webRequest=true');
+    return {responseHeaders: details.responseHeaders};
+  };
+  chrome.webRequest.onHeadersReceived.addListener(
+      onHeadersReceivedListener, filter, extraInfoSpec);
+
+  var onActionIgnoredCalled = false;
+  var onActionIgnoredListener = function(details) {
+    onActionIgnoredCalled = true;
+    chrome.test.assertEq('response_headers', details.action);
+  };
+  chrome.webRequest.onActionIgnored.addListener(onActionIgnoredListener);
+
+  checkAndResetCookies().then(function() {
+    navigateTab(url, function(tab) {
+      chrome.webRequest.onHeadersReceived.removeListener(
+          onHeadersReceivedListener);
+      chrome.webRequest.onActionIgnored.removeListener(onActionIgnoredListener);
+
+      chrome.test.assertTrue(onHeadersReceivedSeen);
+      chrome.test.assertEq(expectRemoved, onActionIgnoredCalled);
+
+      var expectedCookies = expectRemoved ? [] : ['webRequest']
+      checkAndResetCookies(new Set(expectedCookies)).then(chrome.test.succeed);
+    });
   });
 }
 
+var removeCookieRule = {
+  id: 1,
+  condition: {urlFilter: host, resourceTypes: ['main_frame']},
+  action: {type: 'removeHeaders', removeHeadersList: ['cookie']}
+};
+var removeSetCookieRule = {
+  id: 2,
+  condition: {urlFilter: host, resourceTypes: ['main_frame']},
+  action: {type: 'removeHeaders', removeHeadersList: ['setCookie']}
+};
+
 var tests = [
   function testCookieWithoutRules() {
     navigateTab(getServerURL('set-cookie?foo1=bar1&foo2=bar2'), function() {
@@ -134,11 +246,7 @@ var tests = [
   },
 
   function addRulesAndTestCookieRemoval() {
-    var rules = [{
-      id: 1,
-      condition: {urlFilter: host, resourceTypes: ['main_frame']},
-      action: {type: 'removeHeaders', removeHeadersList: ['cookie']}
-    }];
+    var rules = [removeCookieRule];
     chrome.declarativeNetRequest.addDynamicRules(rules, function() {
       chrome.test.assertNoLastError();
       checkCookieHeaderRemoved(true);
@@ -150,15 +258,40 @@ var tests = [
   },
 
   function addRulesAndTestSetCookieRemoval() {
-    var rules = [{
-      id: 2,
-      condition: {urlFilter: host, resourceTypes: ['main_frame']},
-      action: {type: 'removeHeaders', removeHeadersList: ['setCookie']}
-    }];
+    var rules = [removeSetCookieRule];
     chrome.declarativeNetRequest.addDynamicRules(rules, function() {
       chrome.test.assertNoLastError();
       checkSetCookieHeaderRemoved(true);
     });
+  },
+
+  function clearState() {
+    chrome.declarativeNetRequest.removeDynamicRules([1, 2], function() {
+      chrome.test.assertNoLastError();
+      checkAndResetCookies().then(chrome.test.succeed);
+    });
+  },
+
+  function testAddWebRequestCookie() {
+    checkAddWebRequestCookie(false);
+  },
+
+  function testAddWebRequestCookieWithRules() {
+    var rules = [removeCookieRule];
+    chrome.declarativeNetRequest.addDynamicRules(rules, function() {
+      checkAddWebRequestCookie(true);
+    });
+  },
+
+  function testAddWebRequestSetCookie() {
+    checkAddWebRequestSetCookie(false);
+  },
+
+  function testAddWebRequestSetCookieWithRules() {
+    var rules = [removeSetCookieRule];
+    chrome.declarativeNetRequest.addDynamicRules(rules, function() {
+      checkAddWebRequestSetCookie(true);
+    });
   }
 ];
 

chrome/test/data/extensions/api_test/declarative_net_request/header_removal/manifest.json

@@ -9,6 +9,7 @@
   "permissions": [
     "declarativeNetRequest",
     "webRequest",
+    "webRequestBlocking",
     "<all_urls>",
     "tabs",
     "cookies"

extensions/browser/api/web_request/web_request_api.cc

@@ -2388,8 +2388,6 @@ int ExtensionWebRequestEventRouter::ExecuteDeltas(void* browser_context,
   helpers::MergeCancelOfResponses(blocked_request.response_deltas, &canceled,
                                   request->logger.get());
 
-  // TODO(crbug.com/947591): Prevent web request extensions from adding headers
-  // removed by Declarative Net Request.
   extension_web_request_api_helpers::IgnoredActions ignored_actions;
   if (blocked_request.event == kOnBeforeRequest) {
     CHECK(!blocked_request.callback.is_null());
@@ -2399,9 +2397,9 @@ int ExtensionWebRequestEventRouter::ExecuteDeltas(void* browser_context,
   } else if (blocked_request.event == kOnBeforeSendHeaders) {
     CHECK(!blocked_request.before_send_headers_callback.is_null());
     helpers::MergeOnBeforeSendHeadersResponses(
-        request->url, blocked_request.response_deltas,
+        *request, blocked_request.response_deltas,
         blocked_request.request_headers, &ignored_actions,
-        request->logger.get(), &request_headers_removed, &request_headers_set,
+        &request_headers_removed, &request_headers_set,
         &request_headers_modified);
 
     // Also include headers removed by the declarative net request API.
@@ -2412,10 +2410,10 @@ int ExtensionWebRequestEventRouter::ExecuteDeltas(void* browser_context,
   } else if (blocked_request.event == kOnHeadersReceived) {
     CHECK(!blocked_request.callback.is_null());
     helpers::MergeOnHeadersReceivedResponses(
-        request->url, blocked_request.response_deltas,
+        *request, blocked_request.response_deltas,
         blocked_request.filtered_response_headers.get(),
         blocked_request.override_response_headers, blocked_request.new_url,
-        &ignored_actions, request->logger.get(), &response_headers_modified);
+        &ignored_actions, &response_headers_modified);
   } else if (blocked_request.event == kOnAuthRequired) {
     CHECK(blocked_request.callback.is_null());
     CHECK(!blocked_request.auth_callback.is_null());

extensions/browser/api/web_request/web_request_api_helpers.cc

@@ -704,11 +704,10 @@ void MergeCookiesInOnBeforeSendHeadersResponses(
 }
 
 void MergeOnBeforeSendHeadersResponses(
-    const GURL& url,
+    const extensions::WebRequestInfo& request,
     const EventResponseDeltas& deltas,
     net::HttpRequestHeaders* request_headers,
     IgnoredActions* ignored_actions,
-    extensions::WebRequestInfo::Logger* logger,
     std::set<std::string>* removed_headers,
     std::set<std::string>* set_headers,
     bool* request_headers_modified) {
@@ -737,10 +736,23 @@ void MergeOnBeforeSendHeadersResponses(
         const std::string& key = modification.name();
         const std::string& value = modification.value();
 
-        // We must not delete anything that has been modified before.
+        // We must not modify anything that has been deleted before.
         if (removed_headers->find(key) != removed_headers->end() &&
             !extension_conflicts) {
           extension_conflicts = true;
+          break;
+        }
+
+        // Prevent extensions from adding any header removed by the Declarative
+        // Net Request API.
+        if (std::find_if(request.request_headers_to_remove.begin(),
+                         request.request_headers_to_remove.end(),
+                         [&key](const char* header_to_remove) {
+                           return base::EqualsCaseInsensitiveASCII(
+                               header_to_remove, key);
+                         }) != request.request_headers_to_remove.end()) {
+          extension_conflicts = true;
+          break;
         }
 
         // We must not modify anything that has been set to a *different*
@@ -762,13 +774,10 @@ void MergeOnBeforeSendHeadersResponses(
       for (auto key = delta.deleted_request_headers.begin();
            key != delta.deleted_request_headers.end() && !extension_conflicts;
            ++key) {
-        if (set_headers->find(*key) != set_headers->end()) {
-          std::string current_value;
-          request_headers->GetHeader(*key, &current_value);
+        if (set_headers->find(*key) != set_headers->end())
           extension_conflicts = true;
       }
     }
-    }
 
     // Now execute the modifications if there were no conflicts.
     if (!extension_conflicts) {
@@ -789,13 +798,14 @@ void MergeOnBeforeSendHeadersResponses(
           removed_headers->insert(header);
         }
       }
-      logger->LogEvent(net::NetLogEventType::CHROME_EXTENSION_MODIFIED_HEADERS,
+      request.logger->LogEvent(
+          net::NetLogEventType::CHROME_EXTENSION_MODIFIED_HEADERS,
           delta.extension_id);
       *request_headers_modified = true;
     } else {
       ignored_actions->emplace_back(
           delta.extension_id, web_request::IGNORED_ACTION_TYPE_REQUEST_HEADERS);
-      logger->LogEvent(
+      request.logger->LogEvent(
           net::NetLogEventType::CHROME_EXTENSION_IGNORED_DUE_TO_CONFLICT,
           delta.extension_id);
     }
@@ -846,8 +856,8 @@ void MergeOnBeforeSendHeadersResponses(
   }
 
   // Currently, conflicts are ignored while merging cookies.
-  MergeCookiesInOnBeforeSendHeadersResponses(url, deltas, request_headers,
-                                             logger);
+  MergeCookiesInOnBeforeSendHeadersResponses(
+      request.url, deltas, request_headers, request.logger.get());
 }
 
 // Retrieves all cookies from |override_response_headers|.
@@ -1074,13 +1084,12 @@ static ResponseHeader ToLowerCase(const ResponseHeader& header) {
 }
 
 void MergeOnHeadersReceivedResponses(
-    const GURL& url,
+    const extensions::WebRequestInfo& request,
     const EventResponseDeltas& deltas,
     const net::HttpResponseHeaders* original_response_headers,
     scoped_refptr<net::HttpResponseHeaders>* override_response_headers,
     GURL* allowed_unsafe_redirect_url,
     IgnoredActions* ignored_actions,
-    extensions::WebRequestInfo::Logger* logger,
     bool* response_headers_modified) {
   DCHECK(response_headers_modified);
   *response_headers_modified = false;
@@ -1100,8 +1109,9 @@ void MergeOnHeadersReceivedResponses(
     }
 
     // Only create a copy if we really want to modify the response headers.
-    if (override_response_headers->get() == NULL) {
-      *override_response_headers = new net::HttpResponseHeaders(
+    if (override_response_headers->get() == nullptr) {
+      *override_response_headers =
+          base::MakeRefCounted<net::HttpResponseHeaders>(
               original_response_headers->raw_headers());
     }
 
@@ -1111,57 +1121,73 @@ void MergeOnHeadersReceivedResponses(
     // conflict. As deltas is sorted by decreasing extension installation order,
     // this takes care of precedence.
     bool extension_conflicts = false;
-    ResponseHeaders::const_iterator i;
-    for (i = delta.deleted_response_headers.begin();
-         i != delta.deleted_response_headers.end(); ++i) {
-      if (removed_headers.find(ToLowerCase(*i)) != removed_headers.end()) {
+    for (const ResponseHeader& header : delta.deleted_response_headers) {
+      if (removed_headers.find(ToLowerCase(header)) != removed_headers.end()) {
         extension_conflicts = true;
         break;
       }
     }
 
+    // Prevent extensions from adding any response header which was removed by
+    // the Declarative Net Request API.
+    if (!extension_conflicts && !request.response_headers_to_remove.empty()) {
+      for (const ResponseHeader& header : delta.added_response_headers) {
+        if (std::find_if(request.response_headers_to_remove.begin(),
+                         request.response_headers_to_remove.end(),
+                         [&header](const char* header_to_remove) {
+                           return base::EqualsCaseInsensitiveASCII(
+                               header.first, header_to_remove);
+                         }) != request.response_headers_to_remove.end()) {
+          extension_conflicts = true;
+          break;
+        }
+      }
+    }
+
     // Now execute the modifications if there were no conflicts.
     if (!extension_conflicts) {
       // Delete headers
       {
-        for (i = delta.deleted_response_headers.begin();
-             i != delta.deleted_response_headers.end(); ++i) {
-          (*override_response_headers)->RemoveHeaderLine(i->first, i->second);
-          removed_headers.insert(ToLowerCase(*i));
+        for (const ResponseHeader& header : delta.deleted_response_headers) {
+          (*override_response_headers)
+              ->RemoveHeaderLine(header.first, header.second);
+          removed_headers.insert(ToLowerCase(header));
         }
       }
 
       // Add headers.
       {
-        for (i = delta.added_response_headers.begin();
-             i != delta.added_response_headers.end(); ++i) {
-          ResponseHeader lowercase_header(ToLowerCase(*i));
+        for (const ResponseHeader& header : delta.added_response_headers) {
+          ResponseHeader lowercase_header(ToLowerCase(header));
           if (added_headers.find(lowercase_header) != added_headers.end())
             continue;
           added_headers.insert(lowercase_header);
-          (*override_response_headers)->AddHeader(i->first + ": " + i->second);
+          (*override_response_headers)
+              ->AddHeader(header.first + ": " + header.second);
         }
       }
-      logger->LogEvent(net::NetLogEventType::CHROME_EXTENSION_MODIFIED_HEADERS,
+      request.logger->LogEvent(
+          net::NetLogEventType::CHROME_EXTENSION_MODIFIED_HEADERS,
           delta.extension_id);
       *response_headers_modified = true;
     } else {
       ignored_actions->emplace_back(
           delta.extension_id,
           web_request::IGNORED_ACTION_TYPE_RESPONSE_HEADERS);
-      logger->LogEvent(
+      request.logger->LogEvent(
           net::NetLogEventType::CHROME_EXTENSION_IGNORED_DUE_TO_CONFLICT,
           delta.extension_id);
     }
   }
 
   // Currently, conflicts are ignored while merging cookies.
-  MergeCookiesInOnHeadersReceivedResponses(url, deltas,
-                                           original_response_headers,
-                                           override_response_headers, logger);
+  MergeCookiesInOnHeadersReceivedResponses(
+      request.url, deltas, original_response_headers, override_response_headers,
+      request.logger.get());
 
   GURL new_url;
-  MergeRedirectUrlOfResponses(url, deltas, &new_url, ignored_actions, logger);
+  MergeRedirectUrlOfResponses(request.url, deltas, &new_url, ignored_actions,
+                              request.logger.get());
   if (new_url.is_valid()) {
     // Only create a copy if we really want to modify the response headers.
     if (override_response_headers->get() == NULL) {
@@ -1196,7 +1222,6 @@ void MergeOnHeadersReceivedResponses(
   }
   UMA_HISTOGRAM_BOOLEAN("Extensions.WebRequest.SetCookieResponseHeaderRemoved",
                         set_cookie_removed && !set_cookie_modified);
-
 }
 
 bool MergeOnAuthRequiredResponses(const EventResponseDeltas& deltas,

extensions/browser/api/web_request/web_request_api_helpers.h

@@ -327,11 +327,10 @@ void MergeCookiesInOnBeforeSendHeadersResponses(
 // Stores in |request_headers_modified| whether the request headers were
 // modified.
 void MergeOnBeforeSendHeadersResponses(
-    const GURL& url,
+    const extensions::WebRequestInfo& request,
     const EventResponseDeltas& deltas,
     net::HttpRequestHeaders* request_headers,
     IgnoredActions* ignored_actions,
-    extensions::WebRequestInfo::Logger* logger,
     std::set<std::string>* removed_headers,
     std::set<std::string>* set_headers,
     bool* request_headers_modified);
@@ -354,13 +353,12 @@ void MergeCookiesInOnHeadersReceivedResponses(
 // Stores in |response_headers_modified| whether the response headers were
 // modified.
 void MergeOnHeadersReceivedResponses(
-    const GURL& url,
+    const extensions::WebRequestInfo& request,
     const EventResponseDeltas& deltas,
     const net::HttpResponseHeaders* original_response_headers,
     scoped_refptr<net::HttpResponseHeaders>* override_response_headers,
     GURL* allowed_unsafe_redirect_url,
     IgnoredActions* ignored_actions,
-    extensions::WebRequestInfo::Logger* logger,
     bool* response_headers_modified);
 // Merge the responses of blocked onAuthRequired handlers. The first
 // registered listener that supplies authentication credentials in a response,