Bug 1329457, r=bz,dao

MozReview-Commit-ID: 9WNuDkQnyM0

--HG--
extra : rebase_source : 2e82c422eee1b70e6833be41b41378a3a71e70db
extra : amend_source : c8557f4955ea7a5548842cb9bd31935159f7106e

browser/components/newtab/PlacesProvider.jsm

@@ -2,7 +2,7 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-/* global XPCOMUtils, Services, PlacesUtils, gPrincipal, EventEmitter */
+/* global XPCOMUtils, Services, PlacesUtils, EventEmitter */
 /* global gLinks */
 /* exported PlacesProvider */
 
@@ -22,52 +22,14 @@ XPCOMUtils.defineLazyGetter(this, "EventEmitter", function() {
   return EventEmitter;
 });
 
-XPCOMUtils.defineLazyGetter(this, "gPrincipal", function() {
-  let uri = Services.io.newURI("about:newtab");
-  return Services.scriptSecurityManager.getNoAppCodebasePrincipal(uri);
-});
-
 XPCOMUtils.defineLazyModuleGetter(this, "Task",
                                   "resource://gre/modules/Task.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "NewTabUtils",
+                                  "resource://gre/modules/NewTabUtils.jsm");
 
 // The maximum number of results PlacesProvider retrieves from history.
 const HISTORY_RESULTS_LIMIT = 100;
 
-/**
- * Singleton that checks if a given link should be displayed on about:newtab
- * or if we should rather not do it for security reasons. URIs that inherit
- * their caller's principal will be filtered.
- */
-let LinkChecker = {
-  _cache: new Map(),
-
-  get flags() {
-    return Ci.nsIScriptSecurityManager.DISALLOW_INHERIT_PRINCIPAL |
-           Ci.nsIScriptSecurityManager.DONT_REPORT_ERRORS;
-  },
-
-  checkLoadURI: function LinkChecker_checkLoadURI(aURI) {
-    if (!this._cache.has(aURI)) {
-      this._cache.set(aURI, this._doCheckLoadURI(aURI));
-    }
-
-    return this._cache.get(aURI);
-  },
-
-  _doCheckLoadURI: function LinkChecker_doCheckLoadURI(aURI) {
-    let result = false;
-    try {
-      Services.scriptSecurityManager.
-        checkLoadURIStrWithPrincipal(gPrincipal, aURI, this.flags);
-      result = true;
-    } catch (e) {
-      // We got a weird URI or one that would inherit the caller's principal.
-      Cu.reportError(e);
-    }
-    return result;
-  }
-};
-
 /* Queries history to retrieve the most visited sites. Emits events when the
  * history changes.
  * Implements the EventEmitter interface.
@@ -105,7 +67,8 @@ Links.prototype = {
                            aNewFrecency, aGUID, aHidden, aLastVisitDate) { // jshint ignore:line
       // The implementation of the query in getLinks excludes hidden and
       // unvisited pages, so it's important to exclude them here, too.
-      if (!aHidden && aLastVisitDate) {
+      if (!aHidden && aLastVisitDate &&
+          NewTabUtils.linkChecker.checkLoadURI(aURI.spec)) {
         gLinks.emit("linkChanged", {
           url: aURI.spec,
           frecency: aNewFrecency,
@@ -122,10 +85,12 @@ Links.prototype = {
     },
 
     onTitleChanged: function historyObserver_onTitleChanged(aURI, aNewTitle) {
+      if (NewTabUtils.linkChecker.checkLoadURI(aURI.spec)) {
         gLinks.emit("linkChanged", {
           url: aURI.spec,
           title: aNewTitle
         });
+      }
     },
 
     QueryInterface: XPCOMUtils.generateQI([Ci.nsINavHistoryObserver,
@@ -184,7 +149,7 @@ Links.prototype = {
                   params: {limit: this.maxNumLinks}
                 });
 
-    return links.filter(link => LinkChecker.checkLoadURI(link.url));
+    return links.filter(link => NewTabUtils.linkChecker.checkLoadURI(link.url));
   }),
 
   /**
@@ -247,6 +212,10 @@ Links.prototype = {
 const gLinks = new Links(); // jshint ignore:line
 
 let PlacesProvider = {
-  LinkChecker,
   links: gLinks,
 };
+
+// Kept only for backwards-compatibility
+XPCOMUtils.defineLazyGetter(PlacesProvider, "LinkChecker",
+  () => NewTabUtils.linkChecker);
+

browser/components/newtab/tests/xpcshell/test_PlacesProvider.js

@@ -59,13 +59,13 @@ function makeVisit(index, daysAgo, isTyped, domain = TEST_URL) {
 add_task(function test_LinkChecker_securityCheck() {
 
   let urls = [
-    {url: "file://home/file/image.png", expected: false},
-    {url: "resource:///modules/PlacesProvider.jsm", expected: false},
     {url: "javascript:alert('hello')", expected: false}, // jshint ignore:line
     {url: "data:image/png;base64,XXX", expected: false},
     {url: "about:newtab", expected: true},
     {url: "https://example.com", expected: true},
     {url: "ftp://example.com", expected: true},
+    {url: "file://home/file/image.png", expected: true},
+    {url: "resource:///modules/PlacesProvider.jsm", expected: true},
   ];
   for (let {url, expected} of urls) {
     let observed = PlacesProvider.LinkChecker.checkLoadURI(url);

toolkit/modules/NewTabUtils.jsm

@@ -23,11 +23,6 @@ XPCOMUtils.defineLazyModuleGetter(this, "PageThumbs",
 XPCOMUtils.defineLazyModuleGetter(this, "BinarySearch",
   "resource://gre/modules/BinarySearch.jsm");
 
-XPCOMUtils.defineLazyGetter(this, "gPrincipal", function() {
-  let uri = Services.io.newURI("about:newtab");
-  return Services.scriptSecurityManager.createCodebasePrincipal(uri, {});
-});
-
 XPCOMUtils.defineLazyGetter(this, "gCryptoHash", function() {
   return Cc["@mozilla.org/security/hash;1"].createInstance(Ci.nsICryptoHash);
 });
@@ -1351,8 +1346,12 @@ var LinkChecker = {
 
   _doCheckLoadURI: function Links_doCheckLoadURI(aURI) {
     try {
+      // about:newtab is currently privileged. In any case, it should be
+      // possible for tiles to point to pretty much everything - but not
+      // to stuff that inherits the system principal, so we check:
+      let systemPrincipal = Services.scriptSecurityManager.getSystemPrincipal();
       Services.scriptSecurityManager.
-        checkLoadURIStrWithPrincipal(gPrincipal, aURI, this.flags);
+        checkLoadURIStrWithPrincipal(systemPrincipal, aURI, this.flags);
       return true;
     } catch (e) {
       // We got a weird URI or one that would inherit the caller's principal.