Edgewall Software
Modify

Opened 13 years ago

Closed 12 years ago

#3212 closed defect (fixed)

notification email check too strict

Reported by: Dinko Korunic <dinko.korunic@…> Owned by: Emmanuel Blot
Priority: low Milestone: 0.11
Component: ticket system Version: devel
Severity: normal Keywords: notification review
Cc: david.l.jones@…, pzn@…, nielsen@… Branch:
Release Notes:
API Changes:

Description

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).

Attachments (1)

trac_notification_addr_fmt_broken_in_r3406.patch (549 bytes ) - added by david.l.jones@… 13 years ago.
Patch to allow + and = in email addresses

Download all attachments as: .zip

Change History (22)

comment:1 by Emmanuel Blot, 13 years ago

Milestone: 0.11
Owner: changed from Jonas Borgström to Emmanuel Blot
Status: newassigned
Version: 0.9.5devel

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

comment:2 by Matthew Good, 13 years ago

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

comment:3 by Dinko Korunic <dinko.korunic@…>, 13 years ago

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).

comment:4 by Emmanuel Blot, 13 years ago

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.

by david.l.jones@…, 13 years ago

Patch to allow + and = in email addresses

comment:5 by david.l.jones@…, 13 years ago

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.

comment:6 by david.l.jones@…, 13 years ago

Cc: david.l.jones@… added

comment:7 by Emmanuel Blot, 13 years ago

Milestone: 0.110.10
Resolution: fixed
Status: assignedclosed

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.

comment:8 by pzn@…, 13 years ago

Cc: pzn@… added
Resolution: fixed
Status: closedreopened

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_\.\-]+")

in reply to:  8 ; comment:9 by anonymous, 13 years ago

Keywords: notification review added

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 ?

in reply to:  9 comment:10 by pzn@…, 13 years ago

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.

comment:11 by Tim Hatch, 13 years ago

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.

in reply to:  11 ; comment:12 by Emmanuel Blot, 13 years ago

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.

in reply to:  12 ; comment:13 by Dinko Korunic <dinko.korunic@…>, 13 years ago

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.

in reply to:  13 ; comment:14 by pzn@…, 13 years ago

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@…

in reply to:  14 ; comment:15 by Tim Hatch, 13 years ago

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.

in reply to:  15 comment:16 by Dinko Korunic <dinko.korunic@…>, 13 years ago

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.

comment:17 by nielsen@…, 13 years ago

Cc: nielsen@… added

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

comment:18 by anonymous, 12 years ago

Milestone: 0.100.11

comment:19 by dge@…, 12 years ago

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.

in reply to:  19 comment:20 by Emmanuel Blot, 12 years ago

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.

comment:21 by Emmanuel Blot, 12 years ago

Resolution: fixed
Status: reopenedclosed

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Emmanuel Blot.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.