MailboxAddress.TryParse not respecting parser options

[jjxtra]

The following code is returning true when it should be returning false:

bool result = MailboxAddress.TryParse(ParserOptions.Default, "asdasdadsasd", out _);
result = MailboxAddress.TryParse("asdasdadsasd", out _);
// result is true for both, however ParserOptions.Default.AllowAddressesWithoutDomain is false by default.

Issue #331 was closed, but this bug remains for over a year.

[jstedfast]

Apply the following patch to your local checkout:

diff --git a/MimeKit/InternetAddress.cs b/MimeKit/InternetAddress.cs
index 9bf27526..db22262b 100644
--- a/MimeKit/InternetAddress.cs
+++ b/MimeKit/InternetAddress.cs
@@ -662,6 +662,13 @@ namespace MimeKit {
                                        return false;
                                }
 
+                               if (!options.AllowAddressesWithoutDomain) {
+                                       if (throwOnError)
+                                               throw new ParseException (string.Format ("Incomplete addr-spec token at offset {0}", startIndex), startIndex, index);
+
+                                       return false;
+                               }
+
                                // rewind back to the beginning of the local-part
                                index = startIndex;
 

Now try running the unit tests and see all the things that break.

This is a non-trivial problem to fix. Dare I say, impossible to fix without introducing far worse breakages.

[jstedfast]

I never should have added that option because I can't make it work the way you expect it to work due to real-world inputs that MimeKit must handle.

[jstedfast]

You can VERY EASILY change your code to do the following:

if (addr.IndexOf ('@') != -1 && MailboxAddress.TryParse (addr, out mailbox)) {
    // do stuff
}

[jjxtra]

Sounds good, I will make an extension method for it. I suppose we could remove that flag from the parser options since it is not supported. That would be a breaking compile change but shouldn't be a breaking behavior change. Thoughts?

…

-- Jeff

On Wed, Feb 20, 2019 at 3:29 PM Jeffrey Stedfast ***@***.***> wrote: You can VERY EASILY change your code to do the following: if (addr.IndexOf ('@') != -1 && MailboxAddress.TryParse (addr, out mailbox)) { // do stuff } — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#465 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABNoGJ3L_ogjfpL5K9Js0LbW87DrUGRUks5vPcw3gaJpZM4bBrDa> .

[jstedfast]

The option is still used to make adjustments to the parser behavior, just not the way you are expecting.

I would say the option is misleadingly named.

Here's the unit test:

[Test]
public void TestParseMailboxWithUnquotedCommaInName ()
{
	const string text = "Worthington, Warren <[email protected]>";
	InternetAddress addr;

	AssertParse (text);

	// default options should parse this as a single mailbox address
	addr = InternetAddress.Parse (text);
	Assert.AreEqual ("Worthington, Warren", addr.Name);

	// this should fail when we allow mailbox addresses w/o a domain
	var options = ParserOptions.Default.Clone ();
	options.AllowAddressesWithoutDomain = true;

	try {
		addr = InternetAddress.Parse (options, text);
		Assert.Fail ("Should not have parsed \"{0}\" with AllowAddressesWithoutDomain = true", text);
	} catch (ParseException pex) {
		Assert.AreEqual (text.IndexOf (','), pex.TokenIndex, "TokenIndex");
		Assert.AreEqual (text.IndexOf (','), pex.ErrorIndex, "ErrorIndex");
	} catch (Exception ex) {
		Assert.Fail ("Should not have thrown {0}", ex.GetType ().Name);
	}
}