PS - Adjusting webRequest API for use in Public Sessions

extensions/browser/api/web_request/web_request_api.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "extensions/browser/api/web_request/web_request_api.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <memory>
 #include <utility>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/json/json_writer.h"
 #include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "base/values.h"
+#include "chromeos/login/login_state.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/browser/user_metrics.h"
 #include "content/public/common/browser_side_navigation_policy.h"
 #include "content/public/common/child_process_host.h"
 #include "extensions/browser/api/activity_log/web_request_constants.h"
 #include "extensions/browser/api/declarative/rules_registry_service.h"
 #include "extensions/browser/api/declarative_webrequest/request_stage.h"
 #include "extensions/browser/api/declarative_webrequest/webrequest_constants.h"
 #include "extensions/browser/api/declarative_webrequest/webrequest_rules_registry.h"
@@ skipping 296 matching lines @@
 // We hide events from the system context as well as sensitive requests.
 bool ShouldHideEvent(void* browser_context,
                      const InfoMap* extension_info_map,
                      const net::URLRequest* request,
                      ExtensionNavigationUIData* navigation_ui_data) {
   return (!browser_context ||
           WebRequestPermissions::HideRequest(extension_info_map, request,
                                              navigation_ui_data));
 }
 
+// Returns true if we're in a Public Session.
+bool IsPublicSession() {
+#if defined(OS_CHROMEOS)
+  if (chromeos::LoginState::IsInitialized()) {
+    return chromeos::LoginState::Get()->IsPublicSessionUser();
+  }
+#endif
+  return false;
+}
+
 }  // namespace
 
 WebRequestAPI::WebRequestAPI(content::BrowserContext* context)
     : browser_context_(context) {
   EventRouter* event_router = EventRouter::Get(browser_context_);
   for (size_t i = 0; i < arraysize(kWebRequestEvents); ++i) {
     // Observe the webRequest event.
     std::string event_name = kWebRequestEvents[i];
     event_router->RegisterObserver(this, event_name);
 
@@ skipping 759 matching lines @@
   std::string event_name =
       EventRouter::GetBaseEventName((*listener_ids)[0].sub_event_name);
   DCHECK(IsWebRequestEvent(event_name));
 
   Listeners& event_listeners = listeners_[browser_context][event_name];
   void* cross_browser_context = GetCrossBrowserContext(browser_context);
   Listeners* cross_event_listeners =
       cross_browser_context ? &listeners_[cross_browser_context][event_name]
                             : nullptr;
 
+  // In Public Sessions we want to restrict access to security or privacy
+  // sensitive data.
+  if (IsPublicSession()) {
+    event_details = event_details->WhitelistedCopyForPublicSession();

[Devlin, 2016/11/14 17:44:38]

This sanitizes the input for *all* listeners, not just the extension that might
be force-installed.  Is that desired behavior?  If so, we should comment to make
it explicit.

Additionally, if that's the case, we don't need to make a copy of event details;
we can just modify the current object.

[Ivan Šandrk, 2016/11/14 20:16:13]

Yes.

I guess I could also use some help here. I couldn't assign the new object inside
the WhitelistedCopyForPublicSession because the assignment operator is deleted
and I wanted to make the smallest change to the existing code, plus I was trying
to go with the approach of a clean copy, and then just copying over the safe
fields (future-proofing). Got any suggestions here?

[Devlin, 2016/11/15 01:19:37]

If we're okay with sanitizing for all listeners, which it sounds like we are,
then I'd say we should just make a ScrubForPublicSession() method or similar on
WebRequestEventDetails that clears those fields on itself.  Would that work?

[Ivan Šandrk, 2016/11/15 13:49:57]

Sure, done.

+  }
+
   for (const EventListener::ID& id : *listener_ids) {
     // It's possible that the listener is no longer present. Check to make sure
     // it's still there.
     const EventListener* listener =
         FindEventListenerInContainer(id, event_listeners);
     if (!listener && cross_event_listeners) {
       listener = FindEventListenerInContainer(id, *cross_event_listeners);
     }
     if (!listener)
       continue;
@@ skipping 990 matching lines @@
             APIPermission::kWebRequestBlocking)) {
       return RespondNow(Error(keys::kBlockingPermissionRequired));
     }
 
     // We allow to subscribe to patterns that are broader than the host
     // permissions. E.g., we could subscribe to http://www.example.com/*
     // while having host permissions for http://www.example.com/foo/* and
     // http://www.example.com/bar/*.
     // For this reason we do only a coarse check here to warn the extension
     // developer if they do something obviously wrong.
-    if (extension->permissions_data()
+    // When we are in a Public Session, allow all URL's for webRequests

[Devlin, 2016/11/14 17:44:38]

s/URL's/URLs

[Ivan Šandrk, 2016/11/14 20:16:13]

Done.

+    // initiated by a regular extension.
+    if (!(IsPublicSession() && extension->is_extension()) &&
+        extension->permissions_data()
             ->GetEffectiveHostPermissions()
             .is_empty() &&
         extension->permissions_data()
             ->withheld_permissions()
             .explicit_hosts()
             .is_empty()) {
       return RespondNow(Error(keys::kHostPermissionsRequired));
     }
   }
 
@@ skipping 61 matching lines @@
       if (value->size() != 1) {
         OnError(event_name, sub_event_name, request_id, std::move(response));
         return RespondNow(Error(keys::kInvalidBlockingResponse));
       }
 
       bool cancel = false;
       EXTENSION_FUNCTION_VALIDATE(value->GetBoolean("cancel", &cancel));
       response->cancel = cancel;
     }
 
-    if (value->HasKey("redirectUrl")) {
-      std::string new_url_str;
-      EXTENSION_FUNCTION_VALIDATE(value->GetString("redirectUrl",
-                                                   &new_url_str));
-      response->new_url = GURL(new_url_str);
-      if (!response->new_url.is_valid()) {
-        OnError(event_name, sub_event_name, request_id, std::move(response));
-        return RespondNow(Error(keys::kInvalidRedirectUrl, new_url_str));
-      }
-    }
-
-    const bool has_request_headers = value->HasKey("requestHeaders");
-    const bool has_response_headers = value->HasKey("responseHeaders");
-    if (has_request_headers || has_response_headers) {
-      if (has_request_headers && has_response_headers) {
-        // Allow only one of the keys, not both.
-        OnError(event_name, sub_event_name, request_id, std::move(response));
-        return RespondNow(Error(keys::kInvalidHeaderKeyCombination));
+    // In Public Session we only want to allow "cancel".
+    if (!IsPublicSession()) {

[Devlin, 2016/11/14 17:44:38]

Could we instead have this be an early-out error?  i.e. something like:
if (IsPublicSession() &&
    (value->HasKey("redirectUrl") ||
     value->HasKey(keys::kAuthCredentialsKey) ||
     value->HasKey("requestHeaders") ||
     value->HasKey("responseHeaders")) {
  return RespondNow(Error("Only cancel allowed in public sessions"));
}

I think it would make it a bit easier to read (and the diff a lot cleaner)

[Ivan Šandrk, 2016/11/14 20:16:13]

Agreed completely and done. But now I broke the redirectIsBlocked test, and I'm
unsure how to restructure it. It gives the error message "Unchecked
runtime.lastError while running webRequestInternal.eventHandled: Only cancel
allowed in Public Sessions.", and then timeouts.

I guess I was relying on the silent ignore that was previously observed. Got any
ideas/pointers/hints?

[Devlin, 2016/11/15 01:19:37]

The basic approach here will be that you'll need to check the last error (e.g.
chrome.test.assertTrue(!!chrome.runtime.lastError);
chrome.test.assertEq('Only cancel allowed in Public Sessions.',
                     chrome.runtime.lastError.message);

It would probably be easiest to do this bit without the framework.js, but
probably doable either way.

[Ivan Šandrk, 2016/11/15 13:49:57]

That's the first approach which I tried, but couldn't get it working. lastError
wasn't being set until after the test.js script was run (for reasons I'm not
aware of).

I managed to get it working today, turns out I forgot to call OnError() before
returning RespondNow(Error()). Simple as that heh.

+      if (value->HasKey("redirectUrl")) {
+        std::string new_url_str;
+        EXTENSION_FUNCTION_VALIDATE(value->GetString("redirectUrl",
+                                                     &new_url_str));
+        response->new_url = GURL(new_url_str);
+        if (!response->new_url.is_valid()) {
+          OnError(event_name, sub_event_name, request_id, std::move(response));
+          return RespondNow(Error(keys::kInvalidRedirectUrl, new_url_str));
+        }
       }
 
-      base::ListValue* headers_value = NULL;
-      std::unique_ptr<net::HttpRequestHeaders> request_headers;
-      std::unique_ptr<helpers::ResponseHeaders> response_headers;
-      if (has_request_headers) {
-        request_headers.reset(new net::HttpRequestHeaders());
-        EXTENSION_FUNCTION_VALIDATE(value->GetList(keys::kRequestHeadersKey,
-                                                   &headers_value));
-      } else {
-        response_headers.reset(new helpers::ResponseHeaders());
-        EXTENSION_FUNCTION_VALIDATE(value->GetList(keys::kResponseHeadersKey,
-                                                   &headers_value));
+      const bool has_request_headers = value->HasKey("requestHeaders");
+      const bool has_response_headers = value->HasKey("responseHeaders");
+      if (has_request_headers || has_response_headers) {
+        if (has_request_headers && has_response_headers) {
+          // Allow only one of the keys, not both.
+          OnError(event_name, sub_event_name, request_id, std::move(response));
+          return RespondNow(Error(keys::kInvalidHeaderKeyCombination));
+        }
+
+        base::ListValue* headers_value = NULL;
+        std::unique_ptr<net::HttpRequestHeaders> request_headers;
+        std::unique_ptr<helpers::ResponseHeaders> response_headers;
+        if (has_request_headers) {
+          request_headers.reset(new net::HttpRequestHeaders());
+          EXTENSION_FUNCTION_VALIDATE(value->GetList(keys::kRequestHeadersKey,
+                                                     &headers_value));
+        } else {
+          response_headers.reset(new helpers::ResponseHeaders());
+          EXTENSION_FUNCTION_VALIDATE(value->GetList(keys::kResponseHeadersKey,
+                                                     &headers_value));
+        }
+
+        for (size_t i = 0; i < headers_value->GetSize(); ++i) {
+          base::DictionaryValue* header_value = NULL;
+          std::string name;
+          std::string value;
+          EXTENSION_FUNCTION_VALIDATE(
+              headers_value->GetDictionary(i, &header_value));
+          if (!FromHeaderDictionary(header_value, &name, &value)) {
+            std::string serialized_header;
+            base::JSONWriter::Write(*header_value, &serialized_header);
+            OnError(event_name, sub_event_name, request_id,
+                    std::move(response));
+            return RespondNow(Error(keys::kInvalidHeader, serialized_header));
+          }
+          if (!net::HttpUtil::IsValidHeaderName(name)) {
+            OnError(event_name, sub_event_name, request_id,
+                    std::move(response));
+            return RespondNow(Error(keys::kInvalidHeaderName));
+          }
+          if (!net::HttpUtil::IsValidHeaderValue(value)) {
+            OnError(event_name, sub_event_name, request_id,
+                    std::move(response));
+            return RespondNow(Error(keys::kInvalidHeaderValue, name));
+          }
+          if (has_request_headers)
+            request_headers->SetHeader(name, value);
+          else
+            response_headers->push_back(helpers::ResponseHeader(name, value));
+        }
+        if (has_request_headers)
+          response->request_headers.reset(request_headers.release());
+        else
+          response->response_headers.reset(response_headers.release());
       }
 
-      for (size_t i = 0; i < headers_value->GetSize(); ++i) {
-        base::DictionaryValue* header_value = NULL;
-        std::string name;
-        std::string value;
+      if (value->HasKey(keys::kAuthCredentialsKey)) {
+        base::DictionaryValue* credentials_value = NULL;
+        EXTENSION_FUNCTION_VALIDATE(value->GetDictionary(
+            keys::kAuthCredentialsKey,
+            &credentials_value));
+        base::string16 username;
+        base::string16 password;
         EXTENSION_FUNCTION_VALIDATE(
-            headers_value->GetDictionary(i, &header_value));
-        if (!FromHeaderDictionary(header_value, &name, &value)) {
-          std::string serialized_header;
-          base::JSONWriter::Write(*header_value, &serialized_header);
-          OnError(event_name, sub_event_name, request_id, std::move(response));
-          return RespondNow(Error(keys::kInvalidHeader, serialized_header));
-        }
-        if (!net::HttpUtil::IsValidHeaderName(name)) {
-          OnError(event_name, sub_event_name, request_id, std::move(response));
-          return RespondNow(Error(keys::kInvalidHeaderName));
-        }
-        if (!net::HttpUtil::IsValidHeaderValue(value)) {
-          OnError(event_name, sub_event_name, request_id, std::move(response));
-          return RespondNow(Error(keys::kInvalidHeaderValue, name));
-        }
-        if (has_request_headers)
-          request_headers->SetHeader(name, value);
-        else
-          response_headers->push_back(helpers::ResponseHeader(name, value));
+            credentials_value->GetString(keys::kUsernameKey, &username));
+        EXTENSION_FUNCTION_VALIDATE(
+            credentials_value->GetString(keys::kPasswordKey, &password));
+        response->auth_credentials.reset(
+            new net::AuthCredentials(username, password));
       }
-      if (has_request_headers)
-        response->request_headers.reset(request_headers.release());
-      else
-        response->response_headers.reset(response_headers.release());
-    }
-
-    if (value->HasKey(keys::kAuthCredentialsKey)) {
-      base::DictionaryValue* credentials_value = NULL;
-      EXTENSION_FUNCTION_VALIDATE(value->GetDictionary(
-          keys::kAuthCredentialsKey,
-          &credentials_value));
-      base::string16 username;
-      base::string16 password;
-      EXTENSION_FUNCTION_VALIDATE(
-          credentials_value->GetString(keys::kUsernameKey, &username));
-      EXTENSION_FUNCTION_VALIDATE(
-          credentials_value->GetString(keys::kPasswordKey, &password));
-      response->auth_credentials.reset(
-          new net::AuthCredentials(username, password));
     }
   }
 
   ExtensionWebRequestEventRouter::GetInstance()->OnEventHandled(
       profile_id(), extension_id_safe(), event_name, sub_event_name, request_id,
       response.release());
 
   return RespondNow(NoArguments());
 }
 
@@ skipping 61 matching lines @@
   // Since EventListeners are segmented by browser_context, check that
   // last, as it is exceedingly unlikely to be different.
   return extension_id == that.extension_id &&
          sub_event_name == that.sub_event_name &&
          web_view_instance_id == that.web_view_instance_id &&
          embedder_process_id == that.embedder_process_id &&
          browser_context == that.browser_context;
 }
 
 }  // namespace extensions