Bug 997030 - Don't encodeURI twice in bookmarks.html. r=mano a=sylvestre
authorMarco Bonardo <mbonardo@mozilla.com>
Tue, 20 May 2014 22:52:48 +0200
changeset 192350 05baa07365d9
parent 192349 ff1925aa6f85
child 192351 77795a696555
push id3578
push usermak77@bonardo.net
push date2014-05-21 13:43 +0000
Treeherderresults
reviewersmano, sylvestre
bugs997030
milestone30.0
Bug 997030 - Don't encodeURI twice in bookmarks.html. r=mano a=sylvestre
toolkit/components/places/BookmarkHTMLUtils.jsm
toolkit/components/places/tests/bookmarks/test_997030-bookmarks-html-encode.js
toolkit/components/places/tests/bookmarks/xpcshell.ini
toolkit/components/places/tests/head_common.js
--- a/toolkit/components/places/BookmarkHTMLUtils.jsm
+++ b/toolkit/components/places/BookmarkHTMLUtils.jsm
@@ -108,16 +108,24 @@ function base64EncodeString(aString) {
 function escapeHtmlEntities(aText) {
   return (aText || "").replace("&", "&amp;", "g")
                       .replace("<", "&lt;", "g")
                       .replace(">", "&gt;", "g")
                       .replace("\"", "&quot;", "g")
                       .replace("'", "&#39;", "g");
 }
 
+/**
+ * Provides URL escaping for use in HTML attributes of the bookmarks file,
+ * compatible with the old bookmarks system.
+ */
+function escapeUrl(aText) {
+  return (aText || "").replace("\"", "%22", "g");
+}
+
 function notifyObservers(aTopic, aInitialImport) {
   Services.obs.notifyObservers(null, aTopic, aInitialImport ? "html-initial"
                                                             : "html");
 }
 
 function promiseSoon() {
   let deferred = Promise.defer();
   Services.tm.mainThread.dispatch(deferred.resolve,
@@ -1082,20 +1090,20 @@ BookmarkExporter.prototype = {
     if (aItem.title)
       this._writeAttribute("NAME", escapeHtmlEntities(aItem.title));
     this._write(">");
   },
 
   _writeLivemark: function (aItem, aIndent) {
     this._write(aIndent + "<DT><A");
     let feedSpec = aItem.annos.find(anno => anno.name == PlacesUtils.LMANNO_FEEDURI).value;
-    this._writeAttribute("FEEDURL", encodeURI(feedSpec));
+    this._writeAttribute("FEEDURL", escapeUrl(feedSpec));
     let siteSpecAnno = aItem.annos.find(anno => anno.name == PlacesUtils.LMANNO_SITEURI);
     if (siteSpecAnno)
-      this._writeAttribute("HREF", encodeURI(siteSpecAnno.value));
+      this._writeAttribute("HREF", escapeUrl(siteSpecAnno.value));
     this._writeLine(">" + escapeHtmlEntities(aItem.title) + "</A>");
     this._writeDescription(aItem, aIndent);
   },
 
   _writeItem: function (aItem, aIndent) {
     // This is a workaround for "too much recursion" error, due to the fact
     // Task.jsm still uses old on-same-tick promises.  It may be removed as
     // soon as bug 887923 is fixed.
@@ -1104,17 +1112,17 @@ BookmarkExporter.prototype = {
     try {
       uri = NetUtil.newURI(aItem.uri);
     } catch (ex) {
       // If the item URI is invalid, skip the item instead of failing later.
       return;
     }
 
     this._write(aIndent + "<DT><A");
-    this._writeAttribute("HREF", encodeURI(aItem.uri));
+    this._writeAttribute("HREF", escapeUrl(aItem.uri));
     this._writeDateAttributes(aItem);
     yield this._writeFaviconAttribute(aItem);
 
     let keyword = PlacesUtils.bookmarks.getKeywordForBookmark(aItem.id);
     if (aItem.keyword)
       this._writeAttribute("SHORTCUTURL", escapeHtmlEntities(keyword));
 
     let postDataAnno = aItem.annos &&
@@ -1147,17 +1155,17 @@ BookmarkExporter.prototype = {
     let favicon;
     try {
       favicon  = yield PlacesUtils.promiseFaviconData(aItem.uri);
     } catch (ex) {
       Components.utils.reportError("Unexpected Error trying to fetch icon data");
       return;
     }
 
-    this._writeAttribute("ICON_URI", encodeURI(favicon.uri.spec));
+    this._writeAttribute("ICON_URI", escapeUrl(favicon.uri.spec));
 
     if (!favicon.uri.schemeIs("chrome") && favicon.dataLen > 0) {
       let faviconContents = "data:image/png;base64," +
         base64EncodeString(String.fromCharCode.apply(String, favicon.data));
       this._writeAttribute("ICON", faviconContents);
     }
   },
 
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/bookmarks/test_997030-bookmarks-html-encode.js
@@ -0,0 +1,37 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * 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/. */
+
+/**
+ * Checks that we don't encodeURI twice when creating bookmarks.html.
+ */
+ 
+function run_test() {
+  run_next_test();
+}
+
+add_task(function() {
+  let uri = NetUtil.newURI("http://bt.ktxp.com/search.php?keyword=%E5%A6%84%E6%83%B3%E5%AD%A6%E7%94%9F%E4%BC%9A");
+  let bm = PlacesUtils.bookmarks.insertBookmark(PlacesUtils.unfiledBookmarksFolderId,
+                                                uri,
+                                                PlacesUtils.bookmarks.DEFAULT_INDEX,
+                                                "bookmark");
+
+  let file =  OS.Path.join(OS.Constants.Path.profileDir, "bookmarks.exported.997030.html");
+  if ((yield OS.File.exists(file))) {
+    yield OS.File.remove(file);
+  }
+  yield BookmarkHTMLUtils.exportToFile(file);
+
+  // Remove the bookmarks, then restore the backup.
+  PlacesUtils.bookmarks.removeItem(bm);
+  yield BookmarkHTMLUtils.importFromFile(file, true);
+
+  do_log_info("Checking first level");
+  let root = PlacesUtils.getFolderContents(PlacesUtils.unfiledBookmarksFolderId).root;
+  let node = root.getChild(0);
+  do_check_eq(node.uri, uri.spec);
+
+  root.containerOpen = false;
+  PlacesUtils.bookmarks.removeFolderChildren(PlacesUtils.unfiledBookmarksFolderId);
+});
--- a/toolkit/components/places/tests/bookmarks/xpcshell.ini
+++ b/toolkit/components/places/tests/bookmarks/xpcshell.ini
@@ -25,8 +25,9 @@ tail =
 [test_nsINavBookmarkObserver.js]
 [test_removeItem.js]
 [test_savedsearches.js]
 [test_675416.js]
 [test_711914.js]
 [test_protectRoots.js]
 [test_818593-store-backup-metadata.js]
 [test_992901-backup-unsorted-hierarchy.js]
+[test_997030-bookmarks-html-encode.js]
--- a/toolkit/components/places/tests/head_common.js
+++ b/toolkit/components/places/tests/head_common.js
@@ -29,16 +29,18 @@ XPCOMUtils.defineLazyModuleGetter(this, 
 XPCOMUtils.defineLazyModuleGetter(this, "Promise",
                                   "resource://gre/modules/commonjs/sdk/core/promise.js");
 XPCOMUtils.defineLazyModuleGetter(this, "Services",
                                   "resource://gre/modules/Services.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "Task",
                                   "resource://gre/modules/Task.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "BookmarkJSONUtils",
                                   "resource://gre/modules/BookmarkJSONUtils.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "BookmarkHTMLUtils",
+                                  "resource://gre/modules/BookmarkHTMLUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "PlacesBackups",
                                   "resource://gre/modules/PlacesBackups.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "PlacesTransactions",
                                   "resource://gre/modules/PlacesTransactions.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "OS",
                                   "resource://gre/modules/osfile.jsm");
 
 // This imports various other objects in addition to PlacesUtils.