PS - Adjusting webRequest API for use in Public Sessions

extensions/browser/api/web_request/web_request_api.cc

Index: extensions/browser/api/web_request/web_request_api.cc
diff --git a/extensions/browser/api/web_request/web_request_api.cc b/extensions/browser/api/web_request/web_request_api.cc
index 829ebb512eb72d044537eb4afa30a5f800fcf704..66f611cd49c80cc2ae16bd364592928fb10f31a7 100644
--- a/extensions/browser/api/web_request/web_request_api.cc
+++ b/extensions/browser/api/web_request/web_request_api.cc
@@ -22,6 +22,7 @@
 #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"
@@ -338,6 +339,16 @@ bool ShouldHideEvent(void* browser_context,
                                              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)
@@ -1117,6 +1128,12 @@ void ExtensionWebRequestEventRouter::DispatchEventToListeners(
       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.
@@ -2127,7 +2144,10 @@ WebRequestInternalAddEventListenerFunction::Run() {
     // 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()
@@ -2209,83 +2229,89 @@ WebRequestInternalEventHandledFunction::Run() {
       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));
-      }
-
-      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));
+    // 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));
+        }
       }
 
-      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);
+      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::kInvalidHeader, serialized_header));
+          return RespondNow(Error(keys::kInvalidHeaderKeyCombination));
         }
-        if (!net::HttpUtil::IsValidHeaderName(name)) {
-          OnError(event_name, sub_event_name, request_id, std::move(response));
-          return RespondNow(Error(keys::kInvalidHeaderName));
+
+        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));
         }
-        if (!net::HttpUtil::IsValidHeaderValue(value)) {
-          OnError(event_name, sub_event_name, request_id, std::move(response));
-          return RespondNow(Error(keys::kInvalidHeaderValue, name));
+
+        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)
-          request_headers->SetHeader(name, value);
+          response->request_headers.reset(request_headers.release());
         else
-          response_headers->push_back(helpers::ResponseHeader(name, value));
+          response->response_headers.reset(response_headers.release());
       }
-      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));
+      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));
+      }
     }
   }