notification email check too strict

[Dinko Korunic <dinko.korunic@…>]

Email checks in notification.py are too strict. For instance '+' and '=' delimiters can be used in username part of RFC822 address but they are not allowed - see the code:

File notification.py, revision 3356, line 138:

addrfmt = r"[\w\d_\.\-]+\@(([\w\d\-])+\.)+([\w\d]{2,4})+"

I'd suggest adding '+' and '=', as well as allowing \% as username/hostname delimiter too. And there is possibility of someone using user%host@host2 addresses too (think relaying).

[eblot]

You're right, the email address validation needs to be revisited to comply with RFC2822.

[mgood]

Well, I don't know how much the address format changed from RFC 822 to RFC 2822, but apparently it's extremely complicated to write a regex to validate RFC 822 addresses: http://www.ex-parrot.com/~pdw/Mail-RFC822-Address.html

[Dinko Korunic <dinko.korunic@…>]

Unfortunately yes. It's up to you to decide either to:

• implement only some of the features (which I've mentioned),
• to ignore the mail checking at all (or to allow disabling the mail validity checks - eventually the bad headers will be dropped by MTA) or
• to implement all of this evil regex (which is probably quite CPU consuming, regarding the complexity of a such state machine).

[eblot]

The trouble is that the regex is not used to validate an email address, but used to extract email addresses from login and other kinds of "username" strings: I don't see an easy way to distinguish an email address from another simple username string. Up to now, Trac allows both kinds in email-related fields.

Another option would be to ask the MTA to validate each 'suspected' email address, but this solution seems quite complex and may report false negatives.

The RFC(2)822 email address regex is not really usable.
IMHO, we could improve the existing regex so that it supports + and = chars in email addresses.

[david.l.jones@…]

Patch to allow + and = in email addresses

[david.l.jones@…]

I've attached a patch to simply allow + and = symbols in the email address. I use "username+trac@host"-style filtering prolifically, so this was a crude awakening when it broke!

As for the relay portion (the % character)... it's not in this patch. I'm not sure how my MTA at work (where I am writing this, natch) will handle that, i.e. if it will actually relay, so I am going to hold off on testing that.

[eblot]

Patch applied in [3423].

I've searched for better regular expressions for email address detection, but it seems there are no useful alternative but the overly complex solution as pointed out by Matt.

Closing this ticket for 0.10, feel free to reopen it if other kind of email addresses are in trouble.

[pzn@…]

I also have problems with email checking. It rejects some internal-only mail. I need it to accept @localhost and @servername.companyname

The following patch works for me, I added "localhost" and changed "2,4" to "2," to allow addresses like pzn@…

Can you apply it? My Trac version is 0.10.2

Thanks.

--- notification.py.orig        2006-12-22 13:50:54.000000000 -0200
+++ notification.py     2006-12-22 14:05:18.000000000 -0200
@@ -150,7 +150,7 @@
     server = None
     email_map = None
     template_name = None
-    addrfmt = r"[\w\d_\.\-\+=]+\@(([\w\d\-])+\.)+([\w\d]{2,4})+"
+    addrfmt = r"[\w\d_\.\-\+=]+\@((([\w\d\-])+\.)+([\w\d]{2,})|localhost)+"
     shortaddr_re = re.compile(addrfmt)
     longaddr_re = re.compile(r"^\s*(.*)\s+<(" + addrfmt + ")>\s*$");
     nodomaddr_re = re.compile(r"[\w\d_\.\-]+")

[anonymous]

Replying to pzn@terra.com.br:

The following patch works for me, I added "localhost" and changed "2,4" to "2," to allow addresses like pzn@… Can you apply it? My Trac version is 0.10.2

The RE is not valid (it would match localhostlocalhost for example). Anyway we have an issue here: there will never be a valid RE for all possible cases.

I don't think supporting "localhost" is really required BTW, as the notification options already allow to use a default domain and/or domainless email addresses.

Waiting for feedback from other developers:
what do we do about email domain validation: should we drop it ?

[pzn@…]

Replying to anonymous:

what do we do about email domain validation: should we drop it ?

You could add an option to trac.ini, "email_validation" that can be set to "true" or "false".

Another option that could be added to Ini file is "email_validation_regex" that could be used for specifying a custom regular expression to validate mail addresses, when not set, trac will use its internal regex to validate.

[thatch]

IF the "add option to trac.ini" angle is pursued, I would favor something like allowed_local_domains that would be allowed as if they were tld's. This could contain 'localhost,local' by default, and would still allow the "does this look like an email?" bit of code to function as expected.

[eblot]

Replying to thatch:

IF the "add option to trac.ini" angle is pursued, I would favor something like allowed_local_domains that would be allowed as if they were tld's. This could contain 'localhost,local' by default, and would still allow the "does this look like an email?" bit of code to function as expected.

Agreed. I don't see how we can get rid of the domain validation as we need to "guess" whether the user field contains a name or an email address. I'm +1 with the local domain proposal. If other developers are ok with this addition, I'll implement it. IMHO the current regex is fine (as it works with all Internet FQDNs), and local domain option should fullfil need for host/domain names that are not registered on the internet.

[Dinko Korunic <dinko.korunic@…>]

Replying to eblot:

Replying to thatch:

IF the "add option to trac.ini" angle is pursued, I would favor something like allowed_local_domains that would be allowed as if they were tld's. This could contain 'localhost,local' by default, and would still allow the "does this look like an email?" bit of code to function as expected.

Agreed. I don't see how we can get rid of the domain validation as we need to "guess" whether the user field contains a name or an email address. I'm +1 with the local domain proposal. If other developers are ok with this addition, I'll implement it. IMHO the current regex is fine (as it works with all Internet FQDNs), and local domain option should fullfil need for host/domain names that are not registered on the internet.

Variable allowed_local_domains should contain localhost and localdomain. Keyword local itself bares no meaning for MTA or any DNS system by default.

[pzn@…]

Replying to Dinko Korunic <dinko.korunic@gmail.com>:

Variable allowed_local_domains should contain localhost and localdomain. Keyword local itself bares no meaning for MTA or any DNS system by default.

Please note in the implementation that this variable should also accept values with dots, like this: allowed_local_domains = localhost,hostname.localdomain

Check comment 8, I needed to allow an address user@…

[thatch]

Replying to pzn@terra.com.br:

Please note in the implementation that this variable should also accept values with dots, like this: allowed_local_domains = localhost,hostname.localdomain Check comment 8, I needed to allow an address user@…

Yes, and this is compatible with what I said "as if they were tld's". Setting "vztech" enabled using allowed_local_domains=vztech would allow <valid hostname>.vztech. I see no need to validate the hostnames themselves using a config option, just the domain portion. Elbot is welcome to overrule me here, but the regexp is simpler if we only replace the last segment's rule. This errs on the side of allowing too much, which I feel better about than too little.

Replying to Dinko Korunic:

Keyword local itself bares no meaning...

My comment about allowing local is related to OS-X and MS Small Business Server, which make dns records internally for <computer name>.local. This would also solve #4372, by allowing <valid hostname>.local if local was in the list by default.

[Dinko Korunic <dinko.korunic@…>]

My comment about allowing local is related to OS-X and MS Small Business Server, which make dns records internally for <computer name>.local. This would also solve #4372, by allowing <valid hostname>.local if local was in the list by default.

Well, zone with special local meaning can be absolutely anything including .lan, .local and whatever other. Sufficiently widely recognized and used (a large set of Unices, at least) is only .localdomain, AFAIK, but adding .local won't hurt I guess.

[nielsen@…]

This broke a large Trac deployment when upgrading to 0.10. The patch by pzn@… worked nicely.

[dge@…]

Since iniatial release of trac, we also experience difficulties with that code. The issue for us, is not in validating the email, but in the too simplistic condition that check if username to email resolution is required. Currently, I mean in 0.10, it is a simple check on the presence of an "@" sign. This is really annoying for us, since we support user from different domain in an AD, and that username are composed by user@domain, which is not acceptable as an email for us, but should be considered to be converted to an email adresse using the usual process. So we have always patch the simple "@" check into a more complex regex check. Currently, in 0.10, we duplicated the email bottom check at the begining of the process. Hope that this helps in thinking deeper baout this issue, and at least show that the current solution is far from perfect.

[eblot]

Replying to dge@softec.st:

This is really annoying for us, since we support user from different domain in an AD, and that username are composed by user@domain, which is not acceptable as an email for us, but should be considered to be converted to an email adresse using the usual process.

A ticket has been dedicated to this specific issue, see #2766.

[eblot]

Closing this ticket as the initial issue has been addressed; remaining issues are duplicates:

1. ActiveDirectory domain vs. SMTP domain mismatch is tracked down through #2766
2. Support for localhost, localdomain and the like is tracked down through #4372