Bug 1358248, r=nechen,evanxd
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Tue, 09 May 2017 13:24:10 +0200
changeset 394027 ce1b967c11f79388cf4c2903f2f655586b378ecb
parent 394026 1b921b7a4df9aa59d6fdff909fd79484b777ca25
child 394028 3563f72474d33d5be8ef013d8b84e457d2fe15be
push id7334
push usercbook@mozilla.com
push dateMon, 22 May 2017 09:54:57 +0000
treeherdermozilla-beta@0386af1703e2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnechen, evanxd
bugs1358248
milestone54.0
Bug 1358248, r=nechen,evanxd MozReview-Commit-ID: 1EBZFcyvmY1
browser/base/content/urlbarBindings.xml
mobile/android/chrome/content/browser.js
toolkit/components/reader/ReaderMode.jsm
--- a/browser/base/content/urlbarBindings.xml
+++ b/browser/base/content/urlbarBindings.xml
@@ -180,19 +180,19 @@ file, You can obtain one at http://mozil
                 break;
               }
               case "extension": {
                 returnValue = action.params.content;
                 break;
               }
             }
           } else {
-            let originalUrl = ReaderMode.getOriginalUrl(aValue);
+            let originalUrl = ReaderMode.getOriginalUrlObjectForDisplay(aValue);
             if (originalUrl) {
-              returnValue = originalUrl;
+              returnValue = originalUrl.spec;
             }
           }
 
           // Set the actiontype only if the user is not overriding actions.
           if (action && this._pressedNoActionKeys.size == 0) {
             this.setAttribute("actiontype", action.type);
           } else {
             this.removeAttribute("actiontype");
@@ -814,26 +814,27 @@ file, You can obtain one at http://mozil
             try {
               uri = uriFixup.createFixupURI(inputVal, Ci.nsIURIFixup.FIXUP_FLAG_NONE);
             } catch (e) {}
             if (!uri)
               return selectedVal;
           }
 
           // Avoid copying 'about:reader?url=', and always provide the original URI:
-          let readerOriginalURL = ReaderMode.getOriginalUrl(uri.spec);
-          if (readerOriginalURL) {
-            uri = uriFixup.createFixupURI(readerOriginalURL, Ci.nsIURIFixup.FIXUP_FLAG_NONE);
+          // Reader mode ensures we call createExposableURI itself.
+          let readerStrippedURI = ReaderMode.getOriginalUrlObjectForDisplay(uri.spec);
+          if (readerStrippedURI) {
+            uri = readerStrippedURI;
+          } else {
+            // Only copy exposable URIs
+            try {
+              uri = uriFixup.createExposableURI(uri);
+            } catch (ex) {}
           }
 
-          // Only copy exposable URIs
-          try {
-            uri = uriFixup.createExposableURI(uri);
-          } catch (ex) {}
-
           // If the entire URL is selected, just use the actual loaded URI,
           // unless we want a decoded URI, or it's a data: or javascript: URI,
           // since those are hard to read when encoded.
           if (inputVal == selectedVal &&
               !uri.schemeIs("javascript") && !uri.schemeIs("data") &&
               !Services.prefs.getBoolPref("browser.urlbar.decodeURLsOnCopy")) {
             return uri.spec;
           }
--- a/mobile/android/chrome/content/browser.js
+++ b/mobile/android/chrome/content/browser.js
@@ -4466,24 +4466,17 @@ Tab.prototype = {
       // browser.contentDocument is changed to the new document we're loading
       this.contentDocumentIsDisplayed = false;
       this.hasTouchListener = false;
       Services.obs.notifyObservers(this.browser, "Session:NotifyLocationChange", null);
     }
   },
 
   _stripAboutReaderURL: function (originalURI) {
-    try {
-      let strippedURL = ReaderMode.getOriginalUrl(originalURI.spec);
-      if(strippedURL){
-        // Continue to create exposable uri if original uri is stripped.
-        originalURI = URIFixup.createExposableURI(Services.io.newURI(strippedURL));
-      }
-    } catch (ex) { }
-    return originalURI;
+    return ReaderMode.getOriginalUrlObjectForDisplay(originalURI.spec) || originalURI;
   },
 
   // Properties used to cache security state used to update the UI
   _state: null,
   _hostChanged: false, // onLocationChange will flip this bit
 
   onSecurityChange: function(aWebProgress, aRequest, aState) {
     // Don't need to do anything if the data we use to update the UI hasn't changed
--- a/toolkit/components/reader/ReaderMode.jsm
+++ b/toolkit/components/reader/ReaderMode.jsm
@@ -161,16 +161,34 @@ this.ReaderMode = {
         let uriObj = Services.io.newURI(originalUrl);
         uriObj = Services.io.newURI("#" + outerHash, null, uriObj);
         originalUrl = uriObj.spec;
       } catch (ex) {}
     }
     return originalUrl;
   },
 
+  getOriginalUrlObjectForDisplay(url) {
+    let originalUrl = this.getOriginalUrl(url);
+    if (originalUrl) {
+      let uriObj;
+      try {
+        uriObj = Services.uriFixup.createFixupURI(originalUrl, Services.uriFixup.FIXUP_FLAG_NONE);
+      } catch (ex) {
+        return null;
+      }
+      try {
+        return Services.uriFixup.createExposableURI(uriObj);
+      } catch (ex) {
+        return null;
+      }
+    }
+    return null;
+  },
+
   /**
    * Decides whether or not a document is reader-able without parsing the whole thing.
    *
    * @param doc A document to parse.
    * @return boolean Whether or not we should show the reader mode button.
    */
   isProbablyReaderable(doc) {
     // Only care about 'real' HTML documents:
@@ -203,42 +221,39 @@ this.ReaderMode = {
    * Gets an article from a loaded browser's document. This method will not attempt
    * to parse certain URIs (e.g. about: URIs).
    *
    * @param doc A document to parse.
    * @return {Promise}
    * @resolves JS object representing the article, or null if no article is found.
    */
   parseDocument: Task.async(function* (doc) {
-    let documentURI = Services.io.newURI(doc.documentURI);
-    let baseURI = Services.io.newURI(doc.baseURI);
-    if (!this._shouldCheckUri(documentURI) || !this._shouldCheckUri(baseURI, true)) {
+    if (!this._shouldCheckUri(doc.documentURIObject) || !this._shouldCheckUri(doc.baseURIObject, true)) {
       this.log("Reader mode disabled for URI");
       return null;
     }
 
-    return yield this._readerParse(baseURI, doc);
+    return yield this._readerParse(doc);
   }),
 
   /**
    * Downloads and parses a document from a URL.
    *
    * @param url URL to download and parse.
    * @return {Promise}
    * @resolves JS object representing the article, or null if no article is found.
    */
   downloadAndParseDocument: Task.async(function* (url) {
     let doc = yield this._downloadDocument(url);
-    let uri = Services.io.newURI(doc.baseURI);
-    if (!this._shouldCheckUri(uri, true)) {
+    if (!this._shouldCheckUri(doc.documentURIObject) || !this._shouldCheckUri(doc.baseURIObject, true)) {
       this.log("Reader mode disabled for URI");
       return null;
     }
 
-    return yield this._readerParse(uri, doc);
+    return yield this._readerParse(doc);
   }),
 
   _downloadDocument(url) {
     let histogram = Services.telemetry.getHistogramById("READER_MODE_DOWNLOAD_RESULT");
     return new Promise((resolve, reject) => {
       let xhr = new XMLHttpRequest();
       xhr.open("GET", url, true);
       xhr.onerror = evt => reject(evt.error);
@@ -411,38 +426,37 @@ this.ReaderMode = {
 
     return true;
   },
 
   /**
    * Attempts to parse a document into an article. Heavy lifting happens
    * in readerWorker.js.
    *
-   * @param uri The base URI of the article.
    * @param doc The document to parse.
    * @return {Promise}
    * @resolves JS object representing the article, or null if no article is found.
    */
-  _readerParse: Task.async(function* (uri, doc) {
+  _readerParse: Task.async(function* (doc) {
     let histogram = Services.telemetry.getHistogramById("READER_MODE_PARSE_RESULT");
     if (this.parseNodeLimit) {
       let numTags = doc.getElementsByTagName("*").length;
       if (numTags > this.parseNodeLimit) {
-        this.log("Aborting parse for " + uri.spec + "; " + numTags + " elements found");
+        this.log("Aborting parse for " + doc.baseURIObject.spec + "; " + numTags + " elements found");
         histogram.add(PARSE_ERROR_TOO_MANY_ELEMENTS);
         return null;
       }
     }
 
     let uriParam = {
-      spec: uri.spec,
-      host: uri.host,
-      prePath: uri.prePath,
-      scheme: uri.scheme,
-      pathBase: Services.io.newURI(".", null, uri).spec
+      spec: doc.baseURIObject.spec,
+      host: doc.baseURIObject.host,
+      prePath: doc.baseURIObject.prePath,
+      scheme: doc.baseURIObject.scheme,
+      pathBase: Services.io.newURI(".", null, doc.baseURIObject).spec
     };
 
     let serializer = Cc["@mozilla.org/xmlextras/xmlserializer;1"].
                      createInstance(Ci.nsIDOMSerializer);
     let serializedDoc = serializer.serializeToString(doc);
 
     let article = null;
     try {
@@ -453,18 +467,20 @@ this.ReaderMode = {
     }
 
     if (!article) {
       this.log("Worker did not return an article");
       histogram.add(PARSE_ERROR_NO_ARTICLE);
       return null;
     }
 
-    // Readability returns a URI object, but we only care about the URL.
-    article.url = article.uri.spec;
+    // Readability returns a URI object based on the baseURI, but we only care
+    // about the original document's URL from now on. This also avoids spoofing
+    // attempts where the baseURI doesn't match the domain of the documentURI
+    article.url = doc.documentURI;
     delete article.uri;
 
     let flags = Ci.nsIDocumentEncoder.OutputSelectionOnly | Ci.nsIDocumentEncoder.OutputAbsoluteLinks;
     article.title = Cc["@mozilla.org/parserutils;1"].getService(Ci.nsIParserUtils)
                                                     .convertToPlainText(article.title, flags, 0);
     if (gIsFirefoxDesktop) {
       yield this._assignLanguage(article);
       this._maybeAssignTextDirection(article);