Revert 176406

> Do not pass URLs in onUpdated events to extensions unless they have the
> "tabs" permission.
> 
> BUG=168442
> 
> 
> Review URL: https://chromiumcodereview.appspot.com/11824004

TBR=mvrable@chromium.org
Review URL: https://codereview.chromium.org/11866012

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@176426 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 53 insertions, 133 deletions

diff --git a/chrome/browser/extensions/api/messaging/message_service.cc b/chrome/browser/extensions/api/messaging/message_service.cc
index ae8a504..af6bb51 100644
--- a/chrome/browser/extensions/api/messaging/message_service.cc
+++ b/chrome/browser/extensions/api/messaging/message_service.cc
@@ -162,7 +162,7 @@ void MessageService::OpenChannelToExtension(
   std::string tab_json = "null";
   if (source_contents) {
     scoped_ptr<DictionaryValue> tab_value(ExtensionTabUtil::CreateTabValue(
-        source_contents));
+        source_contents, ExtensionTabUtil::INCLUDE_PRIVACY_SENSITIVE_FIELDS));
     base::JSONWriter::Write(tab_value.get(), &tab_json);
   }
 
@@ -202,7 +202,7 @@ void MessageService::OpenChannelToNativeApp(
   std::string tab_json = "null";
   if (source_contents) {
     scoped_ptr<DictionaryValue> tab_value(ExtensionTabUtil::CreateTabValue(
-        source_contents));
+        source_contents, ExtensionTabUtil::INCLUDE_PRIVACY_SENSITIVE_FIELDS));
     base::JSONWriter::Write(tab_value.get(), &tab_json);
   }
 
@@ -287,7 +287,7 @@ void MessageService::OpenChannelToTab(
   std::string tab_json = "null";
   if (source_contents) {
     scoped_ptr<DictionaryValue> tab_value(ExtensionTabUtil::CreateTabValue(
-        source_contents));
+        source_contents, ExtensionTabUtil::INCLUDE_PRIVACY_SENSITIVE_FIELDS));
     base::JSONWriter::Write(tab_value.get(), &tab_json);
   }
 
diff --git a/chrome/browser/extensions/browser_event_router.cc b/chrome/browser/extensions/browser_event_router.cc
index e647b8f..7b38ef9 100644
--- a/chrome/browser/extensions/browser_event_router.cc
+++ b/chrome/browser/extensions/browser_event_router.cc
@@ -339,17 +339,17 @@ void BrowserEventRouter::TabMoved(WebContents* contents,
 
 void BrowserEventRouter::TabUpdated(WebContents* contents, bool did_navigate) {
   TabEntry* entry = GetTabEntry(contents);
-  scoped_ptr<DictionaryValue> changed_properties;
+  DictionaryValue* changed_properties = NULL;
 
   DCHECK(entry);
 
   if (did_navigate)
-    changed_properties.reset(entry->DidNavigate(contents));
+    changed_properties = entry->DidNavigate(contents);
   else
-    changed_properties.reset(entry->UpdateLoadState(contents));
+    changed_properties = entry->UpdateLoadState(contents);
 
   if (changed_properties)
-    DispatchTabUpdatedEvent(contents, changed_properties.Pass());
+    DispatchTabUpdatedEvent(contents, changed_properties);
 }
 
 void BrowserEventRouter::DispatchEvent(
@@ -396,27 +396,18 @@ void BrowserEventRouter::DispatchSimpleBrowserEvent(
                 EventRouter::USER_GESTURE_UNKNOWN);
 }
 
-static void WillDispatchTabUpdatedEvent(
-    WebContents* contents,
-    const DictionaryValue* changed_properties,
-    Profile* profile,
-    const Extension* extension,
-    ListValue* event_args) {
-  // Overwrite the second argument with the appropriate properties dictionary,
-  // depending on extension permissions.
-  DictionaryValue* properties_value = changed_properties->DeepCopy();
-  ExtensionTabUtil::ScrubTabValueForExtension(contents, extension,
-                                              properties_value);
-  event_args->Set(1, properties_value);
-
-  // Overwrite the third arg with our tab value as seen by this extension.
+static void WillDispatchTabUpdatedEvent(WebContents* contents,
+                                        Profile* profile,
+                                        const Extension* extension,
+                                        ListValue* event_args) {
   DictionaryValue* tab_value = ExtensionTabUtil::CreateTabValue(
       contents, extension);
+  // Overwrite the third arg with our tab value as seen by this extension.
   event_args->Set(2, tab_value);
 }
 
 void BrowserEventRouter::DispatchTabUpdatedEvent(
-    WebContents* contents, scoped_ptr<DictionaryValue> changed_properties) {
+    WebContents* contents, DictionaryValue* changed_properties) {
   DCHECK(changed_properties);
   DCHECK(contents);
 
@@ -427,9 +418,8 @@ void BrowserEventRouter::DispatchTabUpdatedEvent(
   // First arg: The id of the tab that changed.
   args_base->AppendInteger(ExtensionTabUtil::GetTabId(contents));
 
-  // Second arg: An object containing the changes to the tab state.  Filled in
-  // by WillDispatchTabUpdatedEvent as a copy of changed_properties, if the
-  // extension has the tabs permission.
+  // Second arg: An object containing the changes to the tab state.
+  args_base->Append(changed_properties);
 
   // Third arg: An object containing the state of the tab. Filled in by
   // WillDispatchTabUpdatedEvent.
@@ -439,8 +429,7 @@ void BrowserEventRouter::DispatchTabUpdatedEvent(
   event->restrict_to_profile = profile;
   event->user_gesture = EventRouter::USER_GESTURE_NOT_ENABLED;
   event->will_dispatch_callback =
-      base::Bind(&WillDispatchTabUpdatedEvent,
-                 contents, changed_properties.get());
+      base::Bind(&WillDispatchTabUpdatedEvent, contents);
   ExtensionSystem::Get(profile)->event_router()->BroadcastEvent(event.Pass());
 }
 
@@ -512,10 +501,10 @@ void BrowserEventRouter::TabPinnedStateChanged(WebContents* contents,
   int tab_index;
 
   if (ExtensionTabUtil::GetTabStripModel(contents, &tab_strip, &tab_index)) {
-    scoped_ptr<DictionaryValue> changed_properties(new DictionaryValue());
+    DictionaryValue* changed_properties = new DictionaryValue();
     changed_properties->SetBoolean(tab_keys::kPinnedKey,
                                    tab_strip->IsTabPinned(tab_index));
-    DispatchTabUpdatedEvent(contents, changed_properties.Pass());
+    DispatchTabUpdatedEvent(contents, changed_properties);
   }
 }
 
@@ -602,7 +591,8 @@ void BrowserEventRouter::ExtensionActionExecuted(
   if (event_name) {
     scoped_ptr<ListValue> args(new ListValue());
     DictionaryValue* tab_value = ExtensionTabUtil::CreateTabValue(
-        web_contents);
+        web_contents,
+        ExtensionTabUtil::INCLUDE_PRIVACY_SENSITIVE_FIELDS);
     args->Append(tab_value);
 
     DispatchEventToExtension(profile,
diff --git a/chrome/browser/extensions/browser_event_router.h b/chrome/browser/extensions/browser_event_router.h
index 24aed86..0f40c36 100644
--- a/chrome/browser/extensions/browser_event_router.h
+++ b/chrome/browser/extensions/browser_event_router.h
@@ -124,7 +124,7 @@ class BrowserEventRouter : public TabStripModelObserver,
   // Packages |changed_properties| as a tab updated event for the tab |contents|
   // and dispatches the event to the extension.
   void DispatchTabUpdatedEvent(content::WebContents* contents,
-                               scoped_ptr<DictionaryValue> changed_properties);
+                               DictionaryValue* changed_properties);
 
   // Called to dispatch a deprecated style page action click event that was
   // registered like:
diff --git a/chrome/browser/extensions/extension_tab_util.cc b/chrome/browser/extensions/extension_tab_util.cc
index 5d475b0..c654bf1 100644
--- a/chrome/browser/extensions/extension_tab_util.cc
+++ b/chrome/browser/extensions/extension_tab_util.cc
@@ -64,9 +64,14 @@ DictionaryValue* ExtensionTabUtil::CreateTabValue(
     TabStripModel* tab_strip,
     int tab_index,
     const Extension* extension) {
-  DictionaryValue *result = CreateTabValue(contents, tab_strip, tab_index);
-  ScrubTabValueForExtension(contents, extension, result);
-  return result;
+  // Only add privacy-sensitive data if the requesting extension has the tabs
+  // permission.
+  bool has_permission = extension && extension->HasAPIPermissionForTab(
+      GetTabId(contents), APIPermission::kTab);
+
+  return CreateTabValue(contents, tab_strip, tab_index,
+                        has_permission ? INCLUDE_PRIVACY_SENSITIVE_FIELDS :
+                            OMIT_PRIVACY_SENSITIVE_FIELDS);
 }
 
 ListValue* ExtensionTabUtil::CreateTabList(
@@ -87,7 +92,8 @@ ListValue* ExtensionTabUtil::CreateTabList(
 DictionaryValue* ExtensionTabUtil::CreateTabValue(
     const WebContents* contents,
     TabStripModel* tab_strip,
-    int tab_index) {
+    int tab_index,
+    IncludePrivacySensitiveFields include_privacy_sensitive_fields) {
   if (!tab_strip)
     ExtensionTabUtil::GetTabStripModel(contents, &tab_strip, &tab_index);
 
@@ -108,14 +114,14 @@ DictionaryValue* ExtensionTabUtil::CreateTabValue(
   result->SetBoolean(keys::kIncognitoKey,
                      contents->GetBrowserContext()->IsOffTheRecord());
 
-  // Privacy-sensitive fields: these should be stripped off by
-  // ScrubTabValueForExtension if the extension should not see them.
-  result->SetString(keys::kUrlKey, contents->GetURL().spec());
-  result->SetString(keys::kTitleKey, contents->GetTitle());
-  if (!is_loading) {
-    NavigationEntry* entry = contents->GetController().GetActiveEntry();
-    if (entry && entry->GetFavicon().valid)
-      result->SetString(keys::kFaviconUrlKey, entry->GetFavicon().url.spec());
+  if (include_privacy_sensitive_fields == INCLUDE_PRIVACY_SENSITIVE_FIELDS) {
+    result->SetString(keys::kUrlKey, contents->GetURL().spec());
+    result->SetString(keys::kTitleKey, contents->GetTitle());
+    if (!is_loading) {
+      NavigationEntry* entry = contents->GetController().GetActiveEntry();
+      if (entry && entry->GetFavicon().valid)
+        result->SetString(keys::kFaviconUrlKey, entry->GetFavicon().url.spec());
+    }
   }
 
   if (tab_strip) {
@@ -127,19 +133,6 @@ DictionaryValue* ExtensionTabUtil::CreateTabValue(
   return result;
 }
 
-void ExtensionTabUtil::ScrubTabValueForExtension(const WebContents* contents,
-                                                 const Extension* extension,
-                                                 DictionaryValue* tab_info) {
-  bool has_permission = extension && extension->HasAPIPermissionForTab(
-      GetTabId(contents), APIPermission::kTab);
-
-  if (!has_permission) {
-    tab_info->Remove(keys::kUrlKey, NULL);
-    tab_info->Remove(keys::kTitleKey, NULL);
-    tab_info->Remove(keys::kFaviconUrlKey, NULL);
-  }
-}
-
 bool ExtensionTabUtil::GetTabStripModel(const WebContents* web_contents,
                                         TabStripModel** tab_strip_model,
                                         int* tab_index) {
diff --git a/chrome/browser/extensions/extension_tab_util.h b/chrome/browser/extensions/extension_tab_util.h
index 3a8aa22..4e8e5c6 100644
--- a/chrome/browser/extensions/extension_tab_util.h
+++ b/chrome/browser/extensions/extension_tab_util.h
@@ -45,10 +45,6 @@ class ExtensionTabUtil {
       const Browser* browser,
       const extensions::Extension* extension);
 
-  // Creates a Tab object (see chrome/common/extensions/api/tabs.json) with
-  // information about the state of a browser tab.  Depending on the
-  // permissions of the extension, the object may or may not include sensitive
-  // data such as the tab's URL.
   static base::DictionaryValue* CreateTabValue(
       const content::WebContents* web_contents,
       const extensions::Extension* extension) {
@@ -60,23 +56,21 @@ class ExtensionTabUtil {
       int tab_index,
       const extensions::Extension* extension);
 
-  // Creates a Tab object but performs no extension permissions checks; the
-  // returned object will contain privacy-sensitive data.
+  enum IncludePrivacySensitiveFields {
+    INCLUDE_PRIVACY_SENSITIVE_FIELDS,
+    OMIT_PRIVACY_SENSITIVE_FIELDS
+  };
   static base::DictionaryValue* CreateTabValue(
-      const content::WebContents* web_contents) {
-    return CreateTabValue(web_contents, NULL, -1);
+      const content::WebContents* web_contents,
+      IncludePrivacySensitiveFields include_privacy_sensitive_fields) {
+    return CreateTabValue(web_contents, NULL, -1,
+                          include_privacy_sensitive_fields);
   }
   static base::DictionaryValue* CreateTabValue(
       const content::WebContents* web_contents,
       TabStripModel* tab_strip,
-      int tab_index);
-
-  // Removes any privacy-sensitive fields from a Tab object if appropriate,
-  // given the permissions of the extension and the tab in question.  The
-  // tab_info object is modified in place.
-  static void ScrubTabValueForExtension(const content::WebContents* contents,
-                                        const extensions::Extension* extension,
-                                        base::DictionaryValue* tab_info);
+      int tab_index,
+      IncludePrivacySensitiveFields include_privacy_sensitive_fields);
 
   // Gets the |tab_strip_model| and |tab_index| for the given |web_contents|.
   static bool GetTabStripModel(const content::WebContents* web_contents,
diff --git a/chrome/browser/extensions/extension_tab_util_android.cc b/chrome/browser/extensions/extension_tab_util_android.cc
index 6167685..6210724 100644
--- a/chrome/browser/extensions/extension_tab_util_android.cc
+++ b/chrome/browser/extensions/extension_tab_util_android.cc
@@ -51,7 +51,8 @@ ListValue* ExtensionTabUtil::CreateTabList(
 DictionaryValue* ExtensionTabUtil::CreateTabValue(
     const WebContents* contents,
     TabStripModel* tab_strip,
-    int tab_index) {
+    int tab_index,
+    IncludePrivacySensitiveFields include_privacy_sensitive_fields) {
   NOTIMPLEMENTED();
   return NULL;
 }
diff --git a/chrome/browser/extensions/extension_tabs_apitest.cc b/chrome/browser/extensions/extension_tabs_apitest.cc
index 93828b3..29a8fce 100644
--- a/chrome/browser/extensions/extension_tabs_apitest.cc
+++ b/chrome/browser/extensions/extension_tabs_apitest.cc
@@ -191,10 +191,6 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, TabsOnUpdated) {
   ASSERT_TRUE(RunExtensionTest("tabs/on_updated")) << message_;
 }
 
-IN_PROC_BROWSER_TEST_F(ExtensionApiTest, TabsNoPermissions) {
-  ASSERT_TRUE(RunExtensionTest("tabs/no_permissions")) << message_;
-}
-
 IN_PROC_BROWSER_TEST_F(ExtensionApiTest,
                        MAYBE_FocusWindowDoesNotExitFullscreen) {
   browser()->window()->EnterFullscreen(
diff --git a/chrome/browser/extensions/menu_manager.cc b/chrome/browser/extensions/menu_manager.cc
index 077c564..b20afaf 100644
--- a/chrome/browser/extensions/menu_manager.cc
+++ b/chrome/browser/extensions/menu_manager.cc
@@ -644,7 +644,8 @@ void MenuManager::ExecuteCommand(Profile* profile,
   if (!extension || !extension->is_platform_app()) {
     // Note: web_contents are NULL in unit tests :(
     if (web_contents) {
-      args->Append(ExtensionTabUtil::CreateTabValue(web_contents));
+      args->Append(ExtensionTabUtil::CreateTabValue(
+          web_contents, ExtensionTabUtil::INCLUDE_PRIVACY_SENSITIVE_FIELDS));
     } else {
       args->Append(new DictionaryValue());
     }
diff --git a/chrome/test/data/extensions/api_test/tabs/no_permissions/manifest.json b/chrome/test/data/extensions/api_test/tabs/no_permissions/manifest.json
deleted file mode 100644
index 323946c..0000000
--- a/chrome/test/data/extensions/api_test/tabs/no_permissions/manifest.json
+++ /dev/null
@@ -1,10 +0,0 @@
-{
-  "name": "tabs/no_permissions",
-  "version": "0.1",
-  "manifest_version": 2,
-  "description": "Tests access to the tabs API from extensions lacking the tabs permission",
-  "background": {
-    "scripts": ["test.js"]
-  },
-  "permissions": []
-}
diff --git a/chrome/test/data/extensions/api_test/tabs/no_permissions/test.js b/chrome/test/data/extensions/api_test/tabs/no_permissions/test.js
deleted file mode 100644
index c28c29e..0000000
--- a/chrome/test/data/extensions/api_test/tabs/no_permissions/test.js
+++ /dev/null
@@ -1,45 +0,0 @@
-// Copyright (c) 2013 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.
-
-function assertNoSensitiveFields(tab) {
-  ['url', 'title', 'faviconUrl'].forEach(function(field) {
-    chrome.test.assertEq(undefined, tab[field],
-                         'Sensitive property ' + field + ' is visible')
-  });
-}
-
-chrome.test.runTests([
-  function testOnUpdated() {
-    var expectedEventData = [{status: 'loading'}, {status: 'complete'},
-                             {status: 'loading'}, {status: 'complete'}];
-    var capturedEventData = [];
-
-    var onUpdateListener = function(tabId, info, tab) {
-      capturedEventData.push(info);
-      if (capturedEventData.length < expectedEventData.length)
-        return;
-      chrome.tabs.onUpdated.removeListener(onUpdateListener);
-      chrome.test.assertEq(expectedEventData, capturedEventData);
-      chrome.test.succeed();
-    }
-
-    chrome.tabs.onUpdated.addListener(onUpdateListener);
-    chrome.tabs.create({url: 'chrome://newtab/'}, function(tab) {
-      assertNoSensitiveFields(tab);
-      chrome.tabs.update(tab.id, {url: 'about:blank'}, function(tab) {
-        assertNoSensitiveFields(tab);
-        chrome.tabs.create({url: 'chrome://newtab/'});
-      });
-    });
-  },
-
-  function testQuery() {
-    chrome.tabs.create({url: 'chrome://newtab/'});
-    chrome.tabs.query({active: true}, chrome.test.callbackPass(function(tabs) {
-      chrome.test.assertEq(1, tabs.length);
-      assertNoSensitiveFields(tabs[0]);
-    }));
-  },
-
-]);