531267 - Autoconfig: Make XML config file reading more liberal

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect, P1)

Description

[Ben Bucksch (:BenB)]

Context: Autoconfig in account creation wizard, fetch config from our config DB (or later ISP directly).

readFromXML.js is currently very strict when reading the XML. It follows the principle "bail when there's something not conforming to what I expect". This sounded good at the time when I wrote this, esp. from a security perspective, but I entirely forgot about the future-compatibility - what happens when we need to revise and extend the spec? (This is my fault, I very well know this problem, I just didn't think of it.)

Potentional problems:
1. New elements or attributes
   This is not a problem, the code just ignores them (DOM-style reading).
2. New values for enums (string or int)
   Currently: This bail, as I use enum() and translate() from sanitze,
   and they are written to bail when a non-expected value appears.
   Fix: They should be changed to accept a default value somehow and if
   that's passed by the caller, return that. If no default value given,
   keep throwing as now.
3. Elements turned from 1 to n, e.g. the code assumes there's only one
   <incomingServer>, but a new, future revision of the spec allows several,
   as siblings.
   Currently: I think the code will fall on its nose in this case,
   due to details of E4X. We'd have to check.
   Fix: Always use [0], e.g. incomingServer[0] instead of incomingServer
   (I think).

Severity:
If this is not fixed before 3.0, we will be forever stuck with one of 3 options:
* On server, detect client version and use different XML for 3.0 clients.
  Problem: Will likely be maintenance problem and break 3.0 at some point.
* In spec, work around the problems by creating new elements with the same
  purpose, and always pass both. Or half-logical variations of that.
  Problem: Very ugly, please avoid this and keep the format clean.

So, I think this should be fixed before 3.0 release. I realize it's late, but I'd rather fix this now than be stuck with it later.

Comment 1

[Ben Bucksch (:BenB)]

https://wiki.mozilla.org/Thunderbird:Autoconfiguration:ConfigFileFormat
http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/readFromXML.js

Comment 2

[David Ascher (:davida)]

I'd rather not have to make any changes at this stage in the game.  We can come up with a new scheme with a new API endpoint, new schema, etc.

Comment 3

[Ben Bucksch (:BenB)]

Yes, of course we can, but we'll have to support the old version forever (or break 3.0, which we shouldn't).

Comment 4

[David Ascher (:davida)]

We'll support the 3.0-style API until 3.0 is EOL'ed.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

We're resetting the blocking flag for 3.1 on this bug and instead setting the wanted-thunderbird+ flag. We have too many blocking-3.1 bugs, to the point where it doesn't mean much, and managing the list is making it hard to actually work on closing bugs, which helps no one.

Thunderbird 3.1's primary purpose is to allow us to offer a prompted major update to Thunderbird 2 users, to ensure their continued ability to safely use Thunderbird.  Thunderbird 2 is built on an outdated version of Gecko, and our long-term ability to maintain the users' safety for Thunderbird 2 users is limited.

If you think this bug meets the requirements below, please renominate with a detailed explanation of how it meets the following two criteria, and we will reconsider.  To qualify, this bug must either:

a) make the upgrade experience from TB2 very painful for a large number of users

or

b) be a new, reproducible, severe quality issue (eg dataloss, frequent crashes)

Just because this bug doesn't block TB3.1 doesn't mean it can't or won't make the release.  Once they're done with their blockers (if any), we encourage developers to keep working on non-blocking bugs, and to try to land them as early in the cycle as possible, as non-blocking bugs will become increasingly difficult to land in the later stages of the cycle.

Comment 6

[Ben Bucksch (:BenB)]

Need this to allow both IMAP and POP configs in the same file (problem 3) above. Current code will break on such files :( (that's exactly what this bug is about).

http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/readFromXML.js#71

Comment 7

[Ben Bucksch (:BenB)]

We also allow new values for the authentication. That's already commited as part of bug 525238, but if we support the new format in 3.0.5, we need to backport that readFromXML.js change.

Comment 8

[Ben Bucksch (:BenB)]

Attachment: Fix, v2

The XML reading code is not anywhere near as pretty anymore :-( , but it's passing the initially mentioned criteria.

It's now supporting several <incomingServer> and <outgoingServer> elements in the XML, and the first one which can be processed without error will be used. That means, we can, if we absolutely need to (should be avoided to avoid crap and large files), add a fallback configuration as second one.

The same idea is used for the authentication and socket enums. We support several <authentication> elements. If the value of the first is unknown, we'll skip it and use the second one. That allows us to add values like "smtp-after-pop" and a fallback "no" (auth), in case we add specific support for that.

I have tested this with various configs, and they still work.
I have not tested the error/fallback cases yet.

Comment 9

[Ben Bucksch (:BenB)]

Attachment: Fix, v3

ops

Comment 10

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 434222 [details] [diff] [review]
Fix, v3

>+++ b/mailnews/base/prefs/content/accountcreation/readFromXML.js
>@@ -46,96 +46,165 @@
>+  d.displayName = d.id;
>   if ("displayName" in xml)
>-    d.displayName = sanitize.label(xml.displayName);
>+  {
>+    try {
>+      d.displayName = sanitize.label(xml.displayName);
>+    } catch (e) { logException(e); }
>+  }

So, you're catching the exception because there might not be a label?  It
seems to me that it would be cheaper and more understandable to just check
for that beforehand.

>   for each (var domain in xml.domain)
>-    d.domains.push(sanitize.hostname(domain));
>+  {
>+    try {
>+      d.domains.push(sanitize.hostname(domain));
>+    } catch (e) { logException(e); }
>+  }

This makes more sense to me, because we may want to continue to add the
rest of the domains even if one fails.

>   // incoming server
>+  for each (let iX in xml.incomingServer) // input (XML)
>   {
>+    let iO = d.incoming; // output (object)
>+    try {
>+      // throws if not supported
[snip…]
>+      for each (let iXsocketType in iX.socketType)
>+      {
>+        try {
>+          iO.socketType = sanitize.translate(iXsocketType,
>+              { plain : 1, SSL: 2, STARTTLS: 3 });
>+          break; // take first that we support
>+        } catch (e) { logException(e); exception = e; }
>+      }
>+      if (!iO.socketType && exception)
>+        throw exception;

You seem to do this a lot, and it confused me for a bit.  I think it might
be useful to wrap it in a function with a name like "addAllValid", or
something.

And then, does it make sense to do the same thing for d.domains up above?
I don't think having xml with no domains really makes sense…

>+      for each (let iXauth in iX.authentication)
>+      {
>+        try {
>+          iO.auth = sanitize.translate(iXauth,
>+              { "password-clear" : Ci.nsMsgAuthMethod.passwordCleartext,
>+                plain : Ci.nsMsgAuthMethod.passwordCleartext,
>+                "password-encrypted" : Ci.nsMsgAuthMethod.passwordEncrypted,
>+                secure : Ci.nsMsgAuthMethod.passwordEncrypted,
>+                GSSAPI : Ci.nsMsgAuthMethod.GSSAPI,
>+                NTLM : Ci.nsMsgAuthMethod.NTLM });

Please wrap all the keys in "s, for consistency.

>+      try {
>+        // defaults are in accountConfig.js
>+        if (iO.type == "pop3" && "pop3" in iX)
>+        {
>+          if ("leaveMessagesOnServer" in iX.pop3)
>+            iO.leaveMessagesOnServer = sanitize.boolean(iX.pop3.leaveMessagesOnServer);
>+          if ("daysToLeaveMessagesOnServer" in iX.pop3)
>+            iO.daysToLeaveMessagesOnServer = sanitize.integer(iX.pop3.daysToLeaveMessagesOnServer);
>+          if ("downloadOnBiff" in iX.pop3)
>+            iO.downloadOnBiff = sanitize.boolean(iX.pop3.downloadOnBiff);
>+        }
>+      } catch (e) { logException(e); }

Do we really want to not set the downloadOnBiff if the
daysToLeaveMessagesOnServer is invalid?  And you don't consider these
errors worthy of blocking the server?  (I'm a little concerned about them,
because they could lead to dataloss, if the user and ISP isn't expecting
the emails to be deleted after 7 days, because of a typo in the config
file.)

>+      break; // take first server which was processed without error

Would you mind looping through all the incoming servers, and stuffing the
later ones into an "incoming.alternates" array?  (It makes my life easier
later. ;)

>+    } catch (e) { logException(e); exception = e; }
>   }
>+  if (exception)
>+    throw exception; // throw exception for last server

Hmm...  You don't clear the exception when you start a new server, so if
you have a failing server followed by a successful server, it will still
throw the exception, I think.

(You could set exception to null before the "break", to fix this, I think.)

>   // outgoing server
>+  for each (let oX in xml.outgoingServer) // input (XML)
>   {
>+    let oO = d.outgoing; // output (object)
>+    try {
>+      if (!(oX.@type == "smtp"))

Isn't that just:
      if (oX.@type != "smtp")
?  ;)

>+++ b/mailnews/base/prefs/content/accountcreation/sanitizeDatatypes.js
>@@ -157,45 +157,72 @@ var sanitize =
>+  /**
>+   * Allows only certain (string) values as input, otherwise throw.
>+   *
>+   * @param allowedValues {Array} List of values that |unchecked| may have.
>+   * @param defaultValue {Any} (Optional) If |unchecked| does not match

Might as well throw an "@param unchecked {Any} The value to check." in
there for completeness.

>+  enum : function(unchecked, allowedValues, defaultValue)

Also, we don't seem to use the defaultValue anywhere.  Did you just add it
in for fun?  ;)

>+++ b/mailnews/base/prefs/content/accountcreation/util.js
>@@ -38,16 +38,18 @@
>+Cu.import("resource://app/modules/errUtils.js");

Finally, do you need this import?  It doesn't seem to be used down below.

I'm going to go with an r-, mostly so that I get pinged by the answers to
the questions, and have a chance to review the code again before I use it.  I expect the next patch to get a pretty quick r+, though.

Thanks,
Blake.

Comment 11

[Ben Bucksch (:BenB)]

> you're catching the exception because there might not be a label?

No, but because sanitize may always throw.
Note that I set the name to a default value before.

> we may want to continue to add the rest of the domains even if one fails.

That's the idea, yes.

> wrap it in a function

How? (nicely)

> Do we really want to not set the downloadOnBiff if the
> daysToLeaveMessagesOnServer is invalid?  And you don't consider these
> errors worthy of blocking the server?  (I'm a little concerned about them,
> because they could lead to dataloss, if the user and ISP isn't expecting
> the emails to be deleted after 7 days, because of a typo in the config
> file.)

hm. My thinking was that basically the ISP shouldn't set this at all (I fail to understand why this was added to the file at all), so I considered them not important.
I didn't wrap each in its own try/catch, because I find them very annoying and cluttering the code a lot.
You have a point, though.

> Would you mind looping through all the incoming servers, and stuffing the
> later ones into an "incoming.alternates" array?  (It makes my life easier
> later. ;)

No, I already did that :).

> Hmm...  You don't clear the exception when you start a new server, so if
> you have a failing server followed by a successful server, it will still
> throw the exception, I think.

Oops.

> I expect the next patch to get a pretty quick r+, though.

Thanks. New patch will follow shortly.

Comment 12

[Ben Bucksch (:BenB)]

> Do we really want to not set the downloadOnBiff if the
> daysToLeaveMessagesOnServer is invalid?

Well, the way it's written, nothing bad will happen: downloadOnBiff defaults to true, so we're fine. I'll wrap it into individual try-s anyways.

Comment 13

[Ben Bucksch (:BenB)]

Attachment: Fix, v4

- Fixed above remove comments
- Implemented alternative server configs
- Fixed other problems
This should be working much better.
Tested also the fallback case (with [e-mail]) and what I tested works.

Comment 14

[Ben Bucksch (:BenB)]

Attachment: Fix, v5

Better

> Also, we don't seem to use the defaultValue anywhere.  Did you just add it
> in for fun?  ;)

Although I do like to do stuff just for fun, it is actually used :).

Comment 15

[Ben Bucksch (:BenB)]

Attachment: Fix, v6

Comment 16

[Ben Bucksch (:BenB)]

Attachment: Fix, v7, with unit test

Comment 17

[Ben Bucksch (:BenB)]

Attachment: Fix, v8, with unit test

Comment 18

[Ben Bucksch (:BenB)]

ping? I keep making some of the changes I make here (sanitize, util) in all my other patches, because I need them there as well.

Comment 19

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 435653 [details] [diff] [review]
Fix, v8, with unit test

>+++ b/mailnews/base/prefs/content/accountcreation/readFromXML.js
>@@ -46,96 +46,197 @@
>+        } catch (e) { exception = e; }

Why aren't you logging the exceptions here anymore?  (Your previous patch logged them, and that seems useful for ISPs trying to figure out what's happening with their newly-created configs.)

>+++ b/mailnews/base/test/unit/test_autoconfigXML.js
>@@ -0,0 +1,220 @@
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Messaging.

I've heard that this should be "The Mozilla Foundation."

>+ * Portions created by the Initial Developer are Copyright (C) 2009

That should probably be "(C) 2010".

>+ * Contributor(s):
>+ *   Blake Winton <[e-mail]>

What?  I wrote this file?  Neat!  ;)

>+/*
>+ * Test suite for the autoconfigUtils class
>+ *
>+ * Currently tested:
>+ * - getHostEntry function.
>+ * - getIncomingTryOrder function.
>+ * - getOutgoingTryOrder function.
>+ *
>+ * TODO:
>+ * - Test the returned CMDS.
>+ * - Figure out what else to test.
>+ */

I think we might want to update this comment with what it actually tests.

>+  var clientConfigXML =
>+    <clientConfig>
>+      <emailProvider id="example.com">
>+        <domain>example.com</domain>
>+        <domain>example.net</domain>
>+        <displayName>Example</displayName>
>+        <displayShortName>Example Mail</displayShortName>
>+
>+        // 1. - protocol not supported
>+        <incomingServer type="imap5">

?!?  Javascript-style comments in an XML literal?  Does that actually work?

>+          <hostname>badprotocol.example.com</hostname>
>+          <port>993</port>
>+          <socketType>SSL</socketType>
>+          <username>%EMAILLOCALPART%</username>
>+          <authentication>ssl-client-cert</authentication>
>+        </incomingServer>
[snip…]
>+  do_check_eq(config instanceof xmlReader.AccountConfig, true);
>+  do_check_eq("example.com", config.id);
>+  do_check_eq("Example", config.displayName);
>+  do_check_neq(-1, config.domains.indexOf("example.com"));

Could we do better, and assert that it is 0?

>+function run_test()
>+{
>+  if (!xmlReader)
>+  {

I think the { for the if should go on the previous line, as per our coding standards.

>+    // if you see this and this is Thunderbird, then it's an error
>+    dump("test_autoconfigXML.js not running, because this is SeaMonkey.");
>+    return;

Uh, I might run this past the SeaMonkey folk, to see if they're okay with that message being displayed.

With their approval, r=me.

Thanks,
Blake.

Comment 20

[Ben Bucksch (:BenB)]

> Javascript-style comments in an XML literal?  Does that actually work?

Yes. But XML-style comments also work, and I changed to that.

> I think we might want to update this comment with what it actually tests.

Definitely. Done.

> Could we do better, and assert that it is 0?

The order of the domains doesn't matter. So, while check for 0 would pass now, the correct test is for "does it exist?", not "is it first?".

> I think the { for the if should go on the previous line, as per our coding
> standards.

Our code is not consistent with regard to that, and I may be confused, but from *my* understanding, our coding standard is { on new line (apart from try/catch).

>+        } catch (e) { exception = e; }

> Why aren't you logging the exceptions here anymore?  (Your previous patch
> logged them, and that seems useful for ISPs trying to figure out what's
> happening with their newly-created configs.)

Good memory :)

The exceptions where logged twice. We store the exception here, and then, if we couldn't parse any <authentication> (or <socketType>), we'll re-throw it, and then catch it for the whole server, and log it there.

So, before, it would be logged twice, if you have one <auth> and it's invalid. Now it's logged once. Before, if you had an <auth> that's not supported, and another fallback <auth> that we do support, you'd get the logged exception, although I don't think you want that (after all, that's what the fallback is for).

Comment 21

[Ben Bucksch (:BenB)]

Attachment: Fix, v10

Commited as <http://hg.mozilla.org/comm-central/rev/97304e9d44d2>

FIXED

Comment 22

[Ben Bucksch (:BenB)]

See bug 556140 to create a new server URL, so that we can make use of the new features without breaking TB 3.0.

Comment 23

[Ben Bucksch (:BenB)]

> I might run this past the SeaMonkey folk, to see if they're okay with that
> message being displayed.

The message won't be displayed by default. You only see that the test passed, not any output of the test (many tests are quite spammy, and rightly so, for debugging). You see the output only when it fails or you run the test manually.

Comment 24

[Ben Bucksch (:BenB)]

For followup, see bug 759422 comment 4.