Bug 1088729 - Only allow http(s) directory links. r=adw, a=sledru
authorEd Lee <edilee@mozilla.com>
Fri, 24 Oct 2014 09:33:03 -0700
changeset 218117 137b543a1ec4
parent 218116 e4f020cdef25
child 218120 9383b778f7f6
push id549
push userryanvm@gmail.com
push date2014-10-26 22:38 +0000
treeherdermozilla-release@137b543a1ec4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw, sledru
bugs1088729
milestone33.0.1
Bug 1088729 - Only allow http(s) directory links. r=adw, a=sledru
toolkit/modules/DirectoryLinksProvider.jsm
toolkit/modules/tests/xpcshell/test_DirectoryLinksProvider.js
--- a/toolkit/modules/DirectoryLinksProvider.jsm
+++ b/toolkit/modules/DirectoryLinksProvider.jsm
@@ -42,16 +42,22 @@ const PREF_SELECTED_LOCALE = "general.us
 const PREF_DIRECTORY_SOURCE = "browser.newtabpage.directory.source";
 
 // The preference that tells where to send click/view pings
 const PREF_DIRECTORY_PING = "browser.newtabpage.directory.ping";
 
 // The preference that tells if newtab is enhanced
 const PREF_NEWTAB_ENHANCED = "browser.newtabpage.enhanced";
 
+// Only allow link urls that are http(s)
+const ALLOWED_LINK_SCHEMES = new Set(["http", "https"]);
+
+// Only allow link image urls that are https or data
+const ALLOWED_IMAGE_SCHEMES = new Set(["https", "data"]);
+
 // The frecency of a directory link
 const DIRECTORY_FRECENCY = 1000;
 
 // Divide frecency by this amount for pings
 const PING_SCORE_DIVISOR = 10000;
 
 // Allowed ping actions remotely stored as columns: case-insensitive [a-z0-9_]
 const PING_ACTIONS = ["block", "click", "pin", "sponsored", "sponsored_link", "unpin", "view"];
@@ -368,26 +374,48 @@ let DirectoryLinksProvider = {
    */
   getEnhancedLink: function DirectoryLinksProvider_getEnhancedLink(link) {
     // Use the provided link if it's already enhanced
     return link.enhancedImageURI && link ||
            this._enhancedLinks.get(NewTabUtils.extractSite(link.url));
   },
 
   /**
+   * Check if a url's scheme is in a Set of allowed schemes
+   */
+  isURLAllowed: function DirectoryLinksProvider_isURLAllowed(url, allowed) {
+    // Assume no url is an allowed url
+    if (!url) {
+      return true;
+    }
+
+    let scheme = "";
+    try {
+      // A malformed url will not be allowed
+      scheme = Services.io.newURI(url, null, null).scheme;
+    }
+    catch(ex) {}
+    return allowed.has(scheme);
+  },
+
+  /**
    * Gets the current set of directory links.
    * @param aCallback The function that the array of links is passed to.
    */
   getLinks: function DirectoryLinksProvider_getLinks(aCallback) {
     this._readDirectoryLinksFile().then(rawLinks => {
       // Reset the cache of enhanced images for this new set of links
       this._enhancedLinks.clear();
 
-      // all directory links have a frecency of DIRECTORY_FRECENCY
-      return rawLinks.map((link, position) => {
+      return rawLinks.filter(link => {
+        // Make sure the link url is allowed and images too if they exist
+        return this.isURLAllowed(link.url, ALLOWED_LINK_SCHEMES) &&
+               this.isURLAllowed(link.imageURI, ALLOWED_IMAGE_SCHEMES) &&
+               this.isURLAllowed(link.enhancedImageURI, ALLOWED_IMAGE_SCHEMES);
+      }).map((link, position) => {
         // Stash the enhanced image for the site
         if (link.enhancedImageURI) {
           this._enhancedLinks.set(NewTabUtils.extractSite(link.url), link);
         }
 
         link.frecency = DIRECTORY_FRECENCY;
         link.lastVisitDate = rawLinks.length - position;
         return link;
--- a/toolkit/modules/tests/xpcshell/test_DirectoryLinksProvider.js
+++ b/toolkit/modules/tests/xpcshell/test_DirectoryLinksProvider.js
@@ -554,66 +554,129 @@ add_task(function test_DirectoryLinksPro
   let directoryLinksFilePath = OS.Path.join(OS.Constants.Path.profileDir, DIRECTORY_LINKS_FILE);
   yield OS.File.writeAtomic(directoryLinksFilePath, '{"en-US":');
   let data = yield fetchData();
   isIdentical(data, []);
 
   yield promiseCleanDirectoryLinksProvider();
 });
 
+add_task(function test_DirectoryLinksProvider_getAllowedLinks() {
+  let data = {"en-US": [
+    {url: "ftp://example.com"},
+    {url: "http://example.net"},
+    {url: "javascript:5"},
+    {url: "https://example.com"},
+    {url: "httpJUNKjavascript:42"},
+    {url: "data:text/plain,hi"},
+    {url: "http/bork:eh"},
+  ]};
+  let dataURI = 'data:application/json,' + JSON.stringify(data);
+  yield promiseSetupDirectoryLinksProvider({linksURL: dataURI});
+
+  let links = yield fetchData();
+  do_check_eq(links.length, 2);
+
+  // The only remaining url should be http and https
+  do_check_eq(links[0].url, data["en-US"][1].url);
+  do_check_eq(links[1].url, data["en-US"][3].url);
+});
+
+add_task(function test_DirectoryLinksProvider_getAllowedImages() {
+  let data = {"en-US": [
+    {url: "http://example.com", imageURI: "ftp://example.com"},
+    {url: "http://example.com", imageURI: "http://example.net"},
+    {url: "http://example.com", imageURI: "javascript:5"},
+    {url: "http://example.com", imageURI: "https://example.com"},
+    {url: "http://example.com", imageURI: "httpJUNKjavascript:42"},
+    {url: "http://example.com", imageURI: "data:text/plain,hi"},
+    {url: "http://example.com", imageURI: "http/bork:eh"},
+  ]};
+  let dataURI = 'data:application/json,' + JSON.stringify(data);
+  yield promiseSetupDirectoryLinksProvider({linksURL: dataURI});
+
+  let links = yield fetchData();
+  do_check_eq(links.length, 2);
+
+  // The only remaining images should be https and data
+  do_check_eq(links[0].imageURI, data["en-US"][3].imageURI);
+  do_check_eq(links[1].imageURI, data["en-US"][5].imageURI);
+});
+
+add_task(function test_DirectoryLinksProvider_getAllowedEnhancedImages() {
+  let data = {"en-US": [
+    {url: "http://example.com", enhancedImageURI: "ftp://example.com"},
+    {url: "http://example.com", enhancedImageURI: "http://example.net"},
+    {url: "http://example.com", enhancedImageURI: "javascript:5"},
+    {url: "http://example.com", enhancedImageURI: "https://example.com"},
+    {url: "http://example.com", enhancedImageURI: "httpJUNKjavascript:42"},
+    {url: "http://example.com", enhancedImageURI: "data:text/plain,hi"},
+    {url: "http://example.com", enhancedImageURI: "http/bork:eh"},
+  ]};
+  let dataURI = 'data:application/json,' + JSON.stringify(data);
+  yield promiseSetupDirectoryLinksProvider({linksURL: dataURI});
+
+  let links = yield fetchData();
+  do_check_eq(links.length, 2);
+
+  // The only remaining enhancedImages should be http and https and data
+  do_check_eq(links[0].enhancedImageURI, data["en-US"][3].enhancedImageURI);
+  do_check_eq(links[1].enhancedImageURI, data["en-US"][5].enhancedImageURI);
+});
+
 add_task(function test_DirectoryLinksProvider_getEnhancedLink() {
   let data = {"en-US": [
-    {url: "http://example.net", enhancedImageURI: "net1"},
-    {url: "http://example.com", enhancedImageURI: "com1"},
-    {url: "http://example.com", enhancedImageURI: "com2"},
+    {url: "http://example.net", enhancedImageURI: "data:,net1"},
+    {url: "http://example.com", enhancedImageURI: "data:,com1"},
+    {url: "http://example.com", enhancedImageURI: "data:,com2"},
   ]};
   let dataURI = 'data:application/json,' + JSON.stringify(data);
   yield promiseSetupDirectoryLinksProvider({linksURL: dataURI});
 
   let links = yield fetchData();
   do_check_eq(links.length, 3);
 
   function checkEnhanced(url, image) {
     let enhanced = DirectoryLinksProvider.getEnhancedLink({url: url});
     do_check_eq(enhanced && enhanced.enhancedImageURI, image);
   }
 
   // Get the expected image for the same site
-  checkEnhanced("http://example.net/", "net1");
-  checkEnhanced("http://example.net/path", "net1");
-  checkEnhanced("https://www.example.net/", "net1");
-  checkEnhanced("https://www3.example.net/", "net1");
+  checkEnhanced("http://example.net/", "data:,net1");
+  checkEnhanced("http://example.net/path", "data:,net1");
+  checkEnhanced("https://www.example.net/", "data:,net1");
+  checkEnhanced("https://www3.example.net/", "data:,net1");
 
   // Get the image of the last entry
-  checkEnhanced("http://example.com", "com2");
+  checkEnhanced("http://example.com", "data:,com2");
 
   // Get the inline enhanced image
   let inline = DirectoryLinksProvider.getEnhancedLink({
     url: "http://example.com/echo",
-    enhancedImageURI: "echo",
+    enhancedImageURI: "data:,echo",
   });
-  do_check_eq(inline.enhancedImageURI, "echo");
+  do_check_eq(inline.enhancedImageURI, "data:,echo");
   do_check_eq(inline.url, "http://example.com/echo");
 
   // Undefined for not enhanced
   checkEnhanced("http://sub.example.net/", undefined);
   checkEnhanced("http://example.org", undefined);
   checkEnhanced("http://localhost", undefined);
   checkEnhanced("http://127.0.0.1", undefined);
 
   // Make sure old data is not cached
   data = {"en-US": [
-    {url: "http://example.com", enhancedImageURI: "fresh"},
+    {url: "http://example.com", enhancedImageURI: "data:,fresh"},
   ]};
   dataURI = 'data:application/json,' + JSON.stringify(data);
   yield promiseSetupDirectoryLinksProvider({linksURL: dataURI});
   links = yield fetchData();
   do_check_eq(links.length, 1);
   checkEnhanced("http://example.net", undefined);
-  checkEnhanced("http://example.com", "fresh");
+  checkEnhanced("http://example.com", "data:,fresh");
 });
 
 add_task(function test_DirectoryLinksProvider_setDefaultEnhanced() {
   function checkDefault(expected) {
     Services.prefs.clearUserPref(kNewtabEnhancedPref);
     do_check_eq(Services.prefs.getBoolPref(kNewtabEnhancedPref), expected);
   }