Bug 1911951 - Fix hostname validity check - don't create servers with hostnames that could not be used. r=babolivier,tobyp

Instead, use <key>.invalid as hostname.
For CreateIncomingServer(), error out directly.

Differential Revision: https://phabricator.services.mozilla.com/D219747

--HG--
extra : rebase_source : d4e2a1ea642a50cc39f2ec6b1be77c3b85269e20

mailnews/base/src/nsMsgAccountManager.cpp

@@ -410,6 +410,28 @@ nsMsgAccountManager::CreateIncomingServer(const nsACString& username,
                                           nsIMsgIncomingServer** _retval) {
   NS_ENSURE_ARG_POINTER(_retval);
 
+  // Make sure the hostname is usable when creating a new incoming server.
+
+  if (hostname.Equals("Local%20Folders") ||
+      hostname.Equals("smart%20mailboxes")) {
+    return NS_ERROR_MALFORMED_URI;
+  }
+  if (hostname.Equals("Local Folders") || hostname.Equals("smart mailboxes")) {
+    // Allow these special hostnames, but only for "none" servers.
+    if (!type.Equals("none")) {
+      return NS_ERROR_MALFORMED_URI;
+    }
+  } else {
+    nsAutoCString unused;
+    nsresult rv = NS_DomainToASCII(hostname, unused);
+    NS_ENSURE_SUCCESS(rv, NS_ERROR_MALFORMED_URI);
+    nsCOMPtr<nsIURL> url;
+    rv = NS_MutateURI(NS_STANDARDURLMUTATOR_CONTRACTID)
+             .SetSpec("imap://"_ns + hostname)
+             .Finalize(url);
+    NS_ENSURE_SUCCESS(rv, NS_ERROR_MALFORMED_URI);
+  }
+
   nsresult rv = LoadAccounts();
   NS_ENSURE_SUCCESS(rv, rv);
 
@@ -535,16 +557,17 @@ nsMsgAccountManager::RemoveIncomingServer(nsIMsgIncomingServer* aServer,
   return rv;
 }
 
-/*
- * create a server when you know the key and the type
+/**
+ * Create a server when you know the key and the type
  */
 nsresult nsMsgAccountManager::createKeyedServer(
     const nsACString& key, const nsACString& username,
-    const nsACString& hostname, const nsACString& type,
+    const nsACString& hostnameIn, const nsACString& type,
     nsIMsgIncomingServer** aServer) {
   nsresult rv;
   *aServer = nullptr;
 
+  nsAutoCString hostname(hostnameIn);
   if (hostname.Equals("Local Folders") || hostname.Equals("smart mailboxes")) {
     // Allow these special hostnames, but only for "none" servers.
     if (type != "none") {
@@ -558,7 +581,17 @@ nsresult nsMsgAccountManager::createKeyedServer(
     // Check the hostname is valid.
     nsAutoCString unused;
     rv = NS_DomainToASCII(hostname, unused);
-    NS_ENSURE_SUCCESS(rv, rv);
+    if (NS_SUCCEEDED(rv)) {
+      nsCOMPtr<nsIURL> url;
+      rv = NS_MutateURI(NS_STANDARDURLMUTATOR_CONTRACTID)
+               .SetSpec("imap://"_ns + hostname)
+               .Finalize(url);
+    }
+    if (NS_FAILED(rv)) {
+      // In case of failure, use a <key>.invalid hostname instead
+      // so that access to the account is not lost.
+      hostname = key + ".invalid"_ns;
+    }
   }
 
   // construct the contractid

mailnews/base/test/unit/test_accountMgr2.js

@@ -258,6 +258,8 @@ add_task(async function () {
     "should find server by uri for normalized hostname '2'"
   );
 
+  // Now, general Testing of bad hostnames.
+
   MailServices.accounts.createIncomingServer(
     "nobody",
     "smart mailboxes",
@@ -279,17 +281,55 @@ add_task(async function () {
     );
   }
 
+  // Valid for "none" type, but already exists.
+  checkBadHostname("Local Folders", /NS_ERROR_FAILURE/);
+  checkBadHostname("Local%20Folders");
+  // Valid for "none" type, but already exists.
+  checkBadHostname("smart mailboxes", /NS_ERROR_FAILURE/);
+  checkBadHostname("smart%20mailboxes");
+
+  checkBadHostname("Local Folders 2");
   checkBadHostname(" bad.test ");
   checkBadHostname("b%61d.test");
   checkBadHostname("b/d.test");
   checkBadHostname("b:d.test");
   checkBadHostname("b@d.test");
   checkBadHostname("b d.test");
-  // Valid for "none" type, but already exists.
-  checkBadHostname("Local Folders", /NS_ERROR_FAILURE/);
-  checkBadHostname("Local Folders 2");
-  checkBadHostname("Local%20Folders");
-  // Valid for "none" type, but already exists.
-  checkBadHostname("smart mailboxes", /NS_ERROR_FAILURE/);
-  checkBadHostname("smart%20mailboxes");
+
+  // non-IPv4 hostnames that end in numbers are not valid.
+  checkBadHostname("invalid.192.168.1.2");
+
+  /**
+   * Check that servers created with these invalid hostnames get created, but
+   * as a <key>.invalid hostname instead (e.g. "server1.invalid").
+   * This is for the case where bad data is already in the prefs.
+   */
+  function checkHostnameRescue(hostname) {
+    const server = MailServices.accounts.createIncomingServer(
+      "nobody",
+      "tobechanged",
+      "none"
+    );
+    Services.prefs.setCharPref(`mail.server.${server.key}.hostname`, hostname);
+
+    MailServices.accounts.unloadAccounts();
+    MailServices.accounts.loadAccounts();
+
+    const serverLater = MailServices.accounts.getIncomingServer(server.key);
+
+    Assert.equal(
+      serverLater.hostName,
+      `${server.key}.invalid`,
+      `invalid hostname ${hostname} should turn into <key>.invalid"`
+    );
+    MailServices.accounts.removeIncomingServer(serverLater, false);
+  }
+  // Only try rescue for a few. If we try to many we get a crash.
+  // Seems loadAccounts/unloadAccounts has some bug.
+
+  checkHostnameRescue("b%61d.test");
+  checkHostnameRescue("b/d.test");
+
+  // non-IPv4 hostnames that end in numbers are not valid.
+  checkHostnameRescue("invalid.192.168.1.2");
 });