Bug 670935 - Fix handling of news URIs when group names contain `&' and other characters. r=bienvenu,a=Standard8
authorJoshua Cranmer <Pidgeot18@gmail.com>
Sat, 26 Nov 2011 21:54:33 -0600
changeset 9386 0146a1f973e2269d9781012ec4435ca0b8b4a7ea
parent 9385 a03506c2ab9fbd2004f0583a14df33fb0f7dc8fa
child 9387 da477a14d2ce2dc5a6810d1e3d4703c20915c49a
child 9392 afdd55bc88bdb0f4b2da3ac2e35920cd4ff66b6a
push idunknown
push userunknown
push dateunknown
reviewersbienvenu, Standard8
bugs670935
Bug 670935 - Fix handling of news URIs when group names contain `&' and other characters. r=bienvenu,a=Standard8
mailnews/news/public/nsINntpIncomingServer.idl
mailnews/news/public/nsINntpUrl.idl
mailnews/news/src/nsNewsUtils.cpp
mailnews/news/src/nsNntpIncomingServer.cpp
mailnews/news/test/unit/postings/bug670935.eml
mailnews/news/test/unit/test_internalUris.js
--- a/mailnews/news/public/nsINntpIncomingServer.idl
+++ b/mailnews/news/public/nsINntpIncomingServer.idl
@@ -128,18 +128,25 @@ interface nsINntpIncomingServer : nsISup
 
     /**
      * Load the next URI in the queue to the given connection.
      *
      * @param aNntpConnection The newly-freed connection.
      */
     [noscript] void prepareForNextUrl(in nsNNTPProtocol aNntpConnection);
 
-    /* used for auto subscribing */
-    boolean containsNewsgroup(in AUTF8String name);
+    /**
+     * Returns whether or not the server has subscribed to the given newsgroup.
+     *
+     * Note that the name here is intended to be escaped; however, since `%' is
+     * not a legal newsgroup name, it is possibly safe to pass in an unescaped
+     * newsgroup name.
+     */
+    boolean containsNewsgroup(in AUTF8String escapedName);
+
     void subscribeToNewsgroup(in AUTF8String name);
 
     /* used for the subscribe dialog.
        name is encoded in |charset|  (attribute declared above) */
     [noscript] void addNewsgroupToList(in string name);
 
     attribute boolean supportsExtensions;
     void addExtension(in string extension);
@@ -153,16 +160,24 @@ interface nsINntpIncomingServer : nsISup
     string queryPropertyForGet(in string name);
     
     void addSearchableGroup(in AString name);
     boolean querySearchableGroup(in AString name);
     
     void addSearchableHeader(in string headerName);
     boolean querySearchableHeader(in string headerName);
 
+    /**
+     * Returns the folder corresponding to the given group.
+     *
+     * Note that this name is expected to be unescaped.
+     * @note If the group does not exist, a bogus news folder will be returned.
+     *       DO NOT call this method unless you are sure that the newsgroup
+     *       is subscribed to (e.g., by containsNewsgroup)
+     */
     nsIMsgNewsFolder findGroup(in AUTF8String name);
 
     readonly attribute AUTF8String firstGroupNeedingExtraInfo;
     void setGroupNeedsExtraInfo(in AUTF8String name, in boolean needsExtraInfo);
 
     void groupNotFound(in nsIMsgWindow window, in AString group,
                        in boolean opening);
 
--- a/mailnews/news/public/nsINntpUrl.idl
+++ b/mailnews/news/public/nsINntpUrl.idl
@@ -83,17 +83,22 @@ interface nsINntpUrl : nsISupports {
    *
    * 3. A non-empty group is found (ActionGetNewNews or ActionListGroups).
    */
   attribute nsNewsAction newsAction;
 
   /// For ActionGetNewNews URIs, whether or not to get older messages.
   attribute boolean getOldMessages;
 
-  /// The group portion of the URI, if one is present
+  /**
+   * The group portion of the URI, if one is present.
+   *
+   * This group name is fully unescaped; if you need to construct news URLs with
+   * this value, be sure to escape it first.
+   */
   readonly attribute ACString group;
 
   /// The message ID portion of the URI, if one is present
   readonly attribute ACString messageID;
 
   /// The message key portion of the URI or nsMsgKey_None if not present
   readonly attribute unsigned long key;
 
--- a/mailnews/news/src/nsNewsUtils.cpp
+++ b/mailnews/news/src/nsNewsUtils.cpp
@@ -55,17 +55,22 @@ nsParseNewsMessageURI(const char* uri, n
   {
     PRInt32 keyEndSeparator = MsgFindCharInSet(uriStr, "?&", keySeparator);
 
     // Grab between the last '/' and the '#' for the key
     group = StringHead(uriStr, keySeparator);
     PRInt32 groupSeparator = group.RFind("/");
     if (groupSeparator == -1)
       return NS_ERROR_FAILURE;
-    group = Substring(group, groupSeparator + 1);
+
+    // Our string APIs don't let us unescape into the same buffer from earlier,
+    // so escape into a temporary
+    nsCAutoString unescapedGroup;
+    MsgUnescapeString(Substring(group, groupSeparator + 1), 0, unescapedGroup);
+    group = unescapedGroup;
 
     nsCAutoString keyStr;
     if (keyEndSeparator != -1)
       keyStr = Substring(uriStr, keySeparator + 1, keyEndSeparator - (keySeparator + 1));
     else
       keyStr = Substring(uriStr, keySeparator + 1);
     nsresult errorCode;
     *key = keyStr.ToInteger(&errorCode);
--- a/mailnews/news/src/nsNntpIncomingServer.cpp
+++ b/mailnews/news/src/nsNntpIncomingServer.cpp
@@ -1552,18 +1552,23 @@ nsNntpIncomingServer::FindGroup(const ns
 
   nsresult rv;
   nsCOMPtr <nsIMsgFolder> serverFolder;
   rv = GetRootMsgFolder(getter_AddRefs(serverFolder));
   NS_ENSURE_SUCCESS(rv,rv);
 
   if (!serverFolder) return NS_ERROR_FAILURE;
 
+  // Escape the name for using FindSubFolder
+  nsCAutoString escapedName;
+  rv = MsgEscapeString(name, nsINetUtil::ESCAPE_URL_PATH, escapedName);
+  NS_ENSURE_SUCCESS(rv, rv);
+
   nsCOMPtr <nsIMsgFolder> subFolder;
-  rv = serverFolder->FindSubFolder(name, getter_AddRefs(subFolder));
+  rv = serverFolder->FindSubFolder(escapedName, getter_AddRefs(subFolder));
   NS_ENSURE_SUCCESS(rv,rv);
   if (!subFolder) return NS_ERROR_FAILURE;
 
   rv = subFolder->QueryInterface(NS_GET_IID(nsIMsgNewsFolder), (void**)result);
   NS_ENSURE_SUCCESS(rv,rv);
   if (!*result) return NS_ERROR_FAILURE;
   return NS_OK;
 }
new file mode 100644
--- /dev/null
+++ b/mailnews/news/test/unit/postings/bug670935.eml
@@ -0,0 +1,7 @@
+From: Robert Kagan <Robert.Kagan@test.invalid>
+Date: Fri, 24 Mar 1989 00:04:18 -0900
+Newsgroups: test.malformed&name
+Subject: I think we have a problem
+
+This is an automated report. There may be some structural damage in
+sectors 10 and 9. Please respond to confirm.
--- a/mailnews/news/test/unit/test_internalUris.js
+++ b/mailnews/news/test/unit/test_internalUris.js
@@ -31,16 +31,17 @@ var nntpService = Cc["@mozilla.org/messe
 
 let tests = [
   test_newMsgs,
   test_cancel,
   test_fetchMessage,
   test_search,
   test_grouplist,
   test_postMessage,
+  test_escapedName,
   cleanUp
 ];
 
 function test_newMsgs() {
   // This tests nsMsgNewsFolder::GetNewsMessages via getNewMessages
   let folder = localserver.rootFolder.getChildNamed("test.filter");
   do_check_eq(folder.getTotalMessages(false), 0);
   folder.getNewMessages(null, asyncUrlListener);
@@ -181,16 +182,48 @@ function test_forwardInline() {
   let composeSvc = Cc['@mozilla.org/messengercompose;1']
                      .getService(Ci.nsIMsgComposeService);
   let folder = localserver.rootFolder.getChildNamed("test.filter");
   let hdr = folder.msgDatabase.GetMsgHdrForKey(1);
   composeSvc.forwardMessage("a@b.c", hdr, null,
     localserver, Ci.nsIMsgComposeService.kForwardInline);
 }
 
+function test_escapedName() {
+  // This does a few tests to make sure our internal URIs work for newsgroups
+  // with names that need escaping
+  let evilName = "test.malformed&name";
+  daemon.addGroup(evilName);
+  daemon.addArticle(make_article(do_get_file("postings/bug670935.eml")));
+  localserver.subscribeToNewsgroup(evilName);
+
+  // Can we access it?
+  let folder = localserver.rootFolder.getChildNamed(evilName);
+  folder.getNewMessages(null, asyncUrlListener);
+  yield false;
+
+  // If we get here, we didn't crash--newsgroups unescape properly.
+  // Load a message, to test news-message: URI unescaping
+  var statuscode = -1;
+  let streamlistener = {
+    onDataAvailable: function() {},
+    onStartRequest: function() {
+    },
+    onStopRequest: function (aRequest, aContext, aStatus) {
+      statuscode = aStatus;
+    },
+    QueryInterface: XPCOMUtils.generateQI([Ci.nsIStreamListener,
+                                           Ci.nsIRequestObserver])
+  };
+  nntpService.fetchMessage(folder, 1, null, streamlistener, asyncUrlListener);
+  yield false;
+  do_check_eq(statuscode, Components.results.NS_OK);
+  yield true;
+}
+
 function run_test() {
   daemon = setupNNTPDaemon();
   localserver = setupLocalServer(NNTP_PORT);
   server = makeServer(NNTP_RFC2980_handler, daemon);
   server.start(NNTP_PORT);
   server.setDebugLevel(fsDebugAll);
 
   // Set up an identity for posting