This ensure email having non-ascii characters are properly encoded and displayed in the email clients. Fixes https://pagure.io/pagure/issue/1496
I do wonder if this actually fixes the issue.
The problem is, if user_from or from_email aren't UTF-8-encoded already, we're going to crash and burn here:
user_from
from_email
>>> latin1_from = 'Kamil Páral'.decode('utf-8').encode('latin1') >>> x = '%s <%s>' % (latin1_from, 'pagure@pagure.io') >>> x.encode('utf-8') Traceback (most recent call last): File "<stdin>", line 1, in <module> UnicodeDecodeError: 'ascii' codec can't decode byte 0xe1 in position 7: ordinal not in range(128) >>> print(x) Kamil Pal <pagure@pagure.io> >>> print(x.decode('latin1')) Kamil Páral <pagure@pagure.io>
There are three scenarios here:
The parameters are UTF-8 encoded. The string literal is also UTF-8 encoded since the coding is declared at the top of this Python file. The end result (in Python 2) is a UTF-8 encoded byte array, so there's no need to encode it.
The parameters are of type unicode. The string literal is decoded to unicode and the resulting string is unicode. It's then encoded to UTF-8. From the sound of the email docs, this is done automatically if the charset is declared (which it is), so this may be pointless.
unicode
The parameters are neither unicode nor a UTF-8 encoded byte array. They could be, for example, latin-1. The result may or may not be a UnicodeDecodeError as Python attempts to decode and re-encode the string.
A good thing to do would be to document the types of the arguments provided (I recommend strictly requiring unicode in Python 2 and str in Python 3) and making sure every user of the API adheres to that requirement. My suspicion is that some user of the API is not providing UTF-8 or unicode input.
str
rebased
This PR has been rebased and adjusted, my tests still show this as fixing the issue at hand
This introduces a subtle bug where it breaks if the from_email contains non-7-bit-ASCII characters, which is possible for both the user and domain portion. It would make more sense to format the string and pass the whole thing to Header. You do not need to explicitly call header.encode, you just need to set the message's header field to be this header object (which I recommend renaming).
Header
header.encode
header
Other than the comment in-line, I have two other things:
This is definitely something we want unit tests for.
There are other headers that will fall into the same trap. Any project with non-ASCII symbols will cause the subject header to break in the same way, for example.
I strongly recommend breaking this send_email function into two functions. One is responsible for building the message, and the other should be responsible for sending the message.
send_email
As a side note, there's a as_string method on messages, which would show the whole email as it's formatted by the email module, which I think would be better than calling print with each header and the message body.
as_string
print
Subject header seems to be converted automatically without the need for us to worry.
What I think is occurring is that when the From header is something like From: =?utf-8?b?RnLDpG7Dp2lzIDxwYWd1cmVAcGFndXJlLm9yZz4=?= the smtp server is appending an @<hostname> to it just like when you send an email from your terminal using mail the From field is: <username>@<hostname>.
From: =?utf-8?b?RnLDpG7Dp2lzIDxwYWd1cmVAcGFndXJlLm9yZz4=?=
@<hostname>
<username>@<hostname>
For the as_string we could expand the debugging info but we wouldn't cover everything since we would be missing smtp piece and that's not really something we have the hand on.
If I give the entire string, then it gets entirely encoded and we're back to square one where @<hostname> is appended.
3 new commits added
Also regarding this:
This introduces a subtle bug where it breaks if the from_email contains non-7-bit-ASCII characters, which is possible for both the user and domain portion.
Do note that it would be the To email not the From, so having it not so nice looking is imho a lesser problem.
To
From
Let's not encode to bytes here since this is to a function call within Pagure. It's much simpler to leave text data as unicode and encode it at the edge of the application (that is, when you're working with an interface that demands bytes)
bytes
Same encoding comment as above
It does indeed look like the email module handles headers correctly if they're set to a unicode string rather than an encoded string. However, if a user of the API provides an encoded string, it'll break. The API needs to be very clearly documented as requiring a unicode string.
Interesting. However, with the current implementation, it seems like it'll violate the email RFC. I think we need to figure out why this is happening and fix that.
I'm also really not comfortable approving this without unit tests, and I definitely think this function needs to be broken into two separate functions.
While the unit-tests might be nice, I still think they only give part of the picture and thus might not be entirely satisfying and real testing is still needed.
For the code style, that's imho personal preferences, and while it can be discussed I don't think it should be a blocker.
Did you run tests on your side? Did you find a suitable solution or can you point me to what you think is the right solution? Because I've been testing this for a few days now and what I am proposing is what seems to work based on my empirical evidences, not a RFC that may not be widely supported or may be.
Well, we are very much on the edge of the application here :) We're in the API and the next line is basically sending the data back to the user.
And here as well we're on the edge since the line REDIS.publish() is sending the data to redis for the services to use :)
REDIS.publish()
For that one we could move the check into pagure.lib if we want indeed.
Hm actually, moving this check into pagure.lib should fix all three comments, but it isn't quite the edge of the application, at least not as I understood it at first.
edge
This is currently including the commits from https://pagure.io/pagure/pull-request/1542 they will disappear when it is merged.
1 new commit added
Sure, I understand that unit tests don't cover everything. I also understand not everything makes sense to unit test, and somethings are difficult/impossible to test. This is not one of those cases. Unit tests catch regressions, but they are also a great way to document and demonstrate to the reviewer what the problem was and why it's fixed. Without them, review is painful and slow since I have to go fiddle with the code myself and run through all the various scenarios. I also have no idea what scenarios you considered because there is no paper trail.
I'm new and I'm still trying to figure out the expectations of everyone when it comes to coding standards. I obviously bring my own history and set of expectations to the table and I'm trying to not let those rule here. I'm working off the documented coding standard for Pagure, which leads me to believe that unit test coverage for changes and additions is expected, as is PEP8-compliant code. Those expectations were not met here. If these are not actually the expectations for code contributions, we need to update the documentation. Personally I think those are good expectations and that we should abide by them.
Sure, which is why I said it was a recommendation. I think it would have made the code much clearer and easier to test (which I think is very valuable), but I left it up to you. I'm sad that you don't agree, but I'm not blocking this PR based on that.
I did run some basic tests. I don't know why the hostname is being appended. I didn't investigate because I didn't want to step on your toes and I didn't hear back from my offer to investigate. I am, however, confident that everyone in the world expects emails to abide by RFC 2822 and that this PR violates that RFC. You cannot use non-ASCII characters in email. The Python email documentation is also clear on that front.
This is a incredibly difficult problem to navigate and people have been getting it wrong for decades (I mean, just look how many RFCs for email there are). I don't expect us to get it right the first time, either, but I don't think we should move ahead with a change that we know breaks the standard.
All of this has me thinking though, surely this is a solved problem. Django has an API to send emails and it probably has to deal with all this nitty-gritty stuff. Surely Flask has the same sort of deal as an extension somewhere?
The RFC 2822 also says, just the paragraph you're linking to: There are other documents, specifically the MIME document series [RFC2045, RFC2046, RFC2047, RFC2048, RFC2049], that extend this standard to allow for values outside of that range. and while I see what you mean, I was able to send emails and receive them fine and the only case where things didn't look pretty was when the user's email contained non-ascii characters which is in the To field, thus less important (to me) than the From field we're trying to fix here.
There are other documents, specifically the MIME document series [RFC2045, RFC2046, RFC2047, RFC2048, RFC2049], that extend this standard to allow for values outside of that range.
I don't believe this current PR is breaking the standard, I do think we have something here that is better than what we have now in prod and that will work for most.
For your offer to help, I will never turn down a helping hand :) At that time, I thought that we could probably solve this quicker than we are but if you are curious and want to test things, please go right ahead, no need for a permission or anything, quite the contrary, we're a team if one of us has the proper answer that's perfect. Let's just be sure to tell each other what we're doing (ie: which angle we're looking at) or if we're close to find something that works so we eventually avoid double work, but even then, I kinda prefer we both end up with two solutions that we can compare or (better) at the same solution than preventing anyone from diving into an issue that interest them :)
It also makes the discussion much easier because it's no longer a proposal/refusal, but a proposal/counter-proposal scheme :)
Finally, you are right about the demand for unit-tests in our coding standards but there are a few things here:
Hope this helps keeping us going and trying to find the best solution :)
The RFC 2822 also says, just the paragraph you're linking to: There are other documents, specifically the MIME document series [RFC2045, RFC2046, RFC2047, RFC2048, RFC2049], that extend this standard to allow for values outside of that range. and while I see what you mean, I was able to send emails and receive them fine and the only case where things didn't look pretty was when the user's email contained non-ascii characters which is in the To field, thus less important (to me) than the From field we're trying to fix here. I don't believe this current PR is breaking the standard, I do think we have something here that is better than what we have now in prod and that will work for most.
Yes, but did you read those RFCs? Specifically, you should look at RFC2047 which defines how to encode headers containing non-ASCII characters using the us-ASCII character set. It still holds to the strict us-ASCII requirements of email. Given that, this is clearly a violation, and most of those other headers (the From header included) have the potential to have non-ASCII characters in them.
Yes, it worked in your test. The mail agents you tested with are apparently kind to violators of the standard, but that doesn't mean all MUAs, MTAs, and MDAs in the world will be.
Okay, I'll investigate this afternoon, then.
Finally, you are right about the demand for unit-tests in our coding standards but there are a few things here: I've been pretty bad at following them myself
I've been pretty bad at following them myself
Unfortunately, since you're the benevolent dictator of this project, this sort of change needs to start with you. I can't really ask everyone (myself included) to write tests unless you're not just happy to see them, but absolutely require them.
we have (currently) no way to enforce them (in other words, it might be worth looking at porting what was done in bodhi to pagure to enforce pep8 and coverage)
I'm all for adding a style test and strictly requiring tests to pass before merging anything.
sometime I test things manually and prefer to push the fix and move on to the next one than spending the time to write the test, which means, sometime I need to just seat done and do only tests for a little bit to remove the bugs I introduced :-p
This tends to lead to lots of regressions, pain, and gnashing of teeth. I'm begging that we not do this from now on. This method might work on a personal project that doesn't see much outside contribution, but it doesn't scale to more than one person.
Do one of RFC2045, RFC2046, RFC2047, RFC2048, or RFC2049 allow UTF-8 encoding? I do believe that MIME e-mails can be UTF-8. I went to look at a few example MIME e-mails from my own inbox, and I see a few that have UTF-8 by using a header like:
Content-Type: multipart/alternative; boundary=001a1148a594eb161e054147d4f4
and then under that boundary, the first thing I see is:
Content-Type: text/plain; charset=UTF-8
Is this what the Pagure e-mails are doing?
That's not me quoting an RFC of course, just me looking at some example e-mails I've got in my own inbox.
Of course, my example really only pertains to the body of the e-mail, not the headers. I'm not sure what is allowed for the headers, since the MIME content seems to usually be related to the body not the headers.
Unfortunately, since you're the benevolent dictator of this project
Such a friendly wording...
Given that, this is clearly a violation, and most of those other headers (the From header included) have the potential to have non-ASCII characters in them.
Agreed, yet if I pass it through the same mechanism as the other headers, I end up with: =?utf-8?b?cHlfw6fDtsOpQHBpbmdvdXJlZC5mcg==?=@flame.pingoured.fr, ie: back to square one.
=?utf-8?b?cHlfw6fDtsOpQHBpbmdvdXJlZC5mcg==?=@flame.pingoured.fr
After discussing with @puiterwijk he pointed me to https://tools.ietf.org/html/rfc6532#section-3.2 which seems to imply that the local part of the address and the domain should both be encoded separately, something like:
user, domain = mailto.split('@', 1) msg['To'] = '%s@%s' % (Header(user, 'utf-8'), Header(domain, 'utf-8'))
We tested it, mutt is still not happy with the address but @puiterwijk 's client seems to handle it fine.
I'll try to improve and clean a little bit the code in this part of the project tomorrow and push an adjusted PR with unit-tests checking for non-ascii chars all over the places :)
I got about halfway done with a patch that splits the function up into something that makes the mail and something that sends the mail, but then it'll be simple to see if smtp.sendmail lives up to what it says and does not modify the headers in any way.
I know this is confusing, but I believe RFC6532 is for email that actually does do real UTF-8 in email (not UTF-8 encoded as ASCII) and as far as I know Python added support for that in Python 3.5.
Anyway, in case you want to see what I was up to this afternoon, this is the patch I've got so far (I have done no prettying up or anything, I stopped for dinner):
diff --git a/pagure/lib/notify.py b/pagure/lib/notify.py index 49d1290..5adf987 100644 --- a/pagure/lib/notify.py +++ b/pagure/lib/notify.py @@ -24,7 +24,9 @@ import warnings import flask import pagure +import six +from email.header import Header from email.mime.text import MIMEText @@ -175,6 +177,59 @@ def _build_url(*args): return '/'.join(items) +def create_email(subject, message, from_email, recipient, project_name=None, + encoding='utf-8', mail_id=None, in_reply_to=None): + """ + Create an email Message. + + Email needs to handle non-ASCII characters carefully. This creates a + plain email using the encoding provided on both the message body and + the message headers. + + :param subject: The message subject; the project name will be prepended. + :type subject: unicode str + :param message: The text you would like to send in the message body. + :type message: unicode str + :param from_email: The string to use in the 'From' field + :type from_email: unicode str + """ + for arg in [subject, message, from_email, recipient, project_name, mail_id, in_reply_to]: + if not isinstance(arg, six.text_type): + raise ValueError('"{arg}" must be a unicode str'.format(arg=arg)) + + if project_name: + subject = u'[{tag}] {subj}'.format(tag=project_name, subj=subject) + else: + subject = u'[Pagure] {subj}'.format(tag=project_name, subj=subject) + + email_message = MIMEText(message, 'plain', encoding) + email_message['Subject'] = Header(subject, encoding) + email_message['From'] = Header(from_email, encoding) + email_message['To'] = Header(recipient, encoding) + if mail_id: + email_message['mail-id'] = Header(mail_id) + email_message['Message-Id'] = Header(u'<{0}>'.format(mail_id)) + + if in_reply_to: + in_reply_to = u'<{0}>'.format(in_reply_to) + email_message['In-Reply-To'] = Header(in_reply_to, encoding) + + salt = pagure.APP.config.get('SALT_EMAIL') + mhash = hashlib.sha512('<{id}>{salt}{to}'.format(id=mail_id, salt=salt, to=recipient)) + reply = u'reply+{hash}@{domain}'.format( + hash=mhash.hexdigest(), + domain=pagure.APP.config['DOMAIN_EMAIL_NOTIFICATIONS'] + ) + email_message['Reply-To'] = Header(reply, encoding) + email_message['Mail-Followup-To'] = Header(reply, encoding) + + email_message['X-pagure'] = Header(pagure.APP.config['APP_URL'], encoding) + if project_name is not None: + email_message['X-pagure-project'] = Header(project_name, encoding) + + return email_message + + def send_email(text, subject, to_mail, mail_id=None, in_reply_to=None, project_name=None, user_from=None): # pragma: no cover @@ -196,25 +251,19 @@ def send_email(text, subject, to_mail, from_email = pagure.APP.config.get( 'FROM_EMAIL', 'pagure@fedoraproject.org') if user_from: - from_email = '%s <%s>' % (user_from, from_email) + from_email = u'%s <%s>' % (user_from, from_email) + + messages = [] + for recipient in to_mail.split(','): + messages.append(create_email(subject, text, from_email, recipient, project_name, + mail_id=mail_id, in_reply_to=in_reply_to)) if not pagure.APP.config.get('EMAIL_SEND', True): - print '******EMAIL******' - print 'From: %s' % from_email - print 'To: %s' % to_mail - print 'Subject: %s' % subject - print 'in_reply_to: %s' % in_reply_to - print 'mail_id: %s' % mail_id - print 'Contents:' - print text.encode('utf-8') - print '*****/EMAIL******' + print(u'Would have sent the following emails:') + for m in messages: + print(m.as_string()) return - if project_name is not None: - subject_tag = project_name - else: - subject_tag = 'Pagure' - if pagure.APP.config['SMTP_SSL']: smtp = smtplib.SMTP_SSL( pagure.APP.config['SMTP_SERVER'], pagure.APP.config['SMTP_PORT']) @@ -222,31 +271,6 @@ def send_email(text, subject, to_mail, smtp = smtplib.SMTP( pagure.APP.config['SMTP_SERVER'], pagure.APP.config['SMTP_PORT']) - for mailto in to_mail.split(','): - msg = MIMEText(text.encode('utf-8'), 'plain', 'utf-8') - msg['Subject'] = '[%s] %s' % (subject_tag, subject) - msg['From'] = from_email - - if mail_id: - msg['mail-id'] = mail_id - msg['Message-Id'] = '<%s>' % mail_id - - if in_reply_to: - msg['In-Reply-To'] = '<%s>' % in_reply_to - - msg['X-pagure'] = pagure.APP.config['APP_URL'] - if project_name is not None: - msg['X-pagure-project'] = project_name - - # Send the message via our own SMTP server, but don't include the - # envelope header. - msg['To'] = mailto - salt = pagure.APP.config.get('SALT_EMAIL') - mhash = hashlib.sha512('<%s>%s%s' % (mail_id, salt, mailto)) - msg['Reply-To'] = 'reply+%s@%s' % ( - mhash.hexdigest(), - pagure.APP.config['DOMAIN_EMAIL_NOTIFICATIONS']) - msg['Mail-Followup-To'] = msg['Reply-To'] try: if pagure.APP.config['SMTP_USERNAME'] \ and pagure.APP.config['SMTP_PASSWORD']: @@ -255,14 +279,14 @@ def send_email(text, subject, to_mail, pagure.APP.config['SMTP_PASSWORD'] ) - smtp.sendmail( - from_email, - [mailto], - msg.as_string()) + for m in messages: + from_email = pagure.APP.config.get('FROM_EMAIL', 'pagure@fedoraproject.org') + to = m['To'] # TODO does this work with RFC 2047 encoded messages? + # Sadly we can't use smtp.send_message since it's Python 3.2+ + smtp.sendmail(from_email, [str(to)], m.as_string()) except smtplib.SMTPException as err: pagure.LOG.exception(err) smtp.quit() - return msg def notify_new_comment(comment, user=None):
So, the full definition of addr-spec, taking into account the updates from RFC6532, basing on 5322 is:
addr-spec = local-part "@" domain local-part = dot-atom / quoted-string / obs-local-part domain = dot-atom / domain-literal / obs-domain domain-literal = [CFWS] "[" ([FWS] dtext) [FWS] "]" [CFWS] quoted-string = [CFWS] DQUOTE ([FWS] qcontent) [FWS] DQUOTE [CFWS] qcontent = qtext / quoted-pair qtext =/ UTF8-non-ascii dtext =/ UTF8-non-ascii UTF8-non-ascii = UTF8-2 / UTF8-3 / UTF8-4 UTF8-non-ascii = UTF8-2 / UTF8-3 / UTF8-4 UTF8-2 = %xC2-DF UTF8-tail
Note that both local-part and domain fall down to qtext and dtext respectively, and RFC6532 sets both of those to UTF8-non-ascii. Also, explicitly note that the @ symbol was NOT overridden.
As a consequence, that means that both local-part and domain need to be unicode-encoded seperately. (and for domain part, you might want to use xn--mumble style, but that's something else entirely. Look at IDNA2003/IDNA2008).
As Jeremy hinted in his last reply, this indicates that it is just plain unicode and not the =?utf?.... encoded variant indeed.
I am starting to have the following reasoning:
I am thinking to just allow UTF-8 headers.
This means that the PR as is would be valid, we could even likely just simplify it a little more and the refactoring that @jcline is working on could come in another PR.
Thoughts?
Okay, I read RFC2047 a bit more carefully, and it does NOT apply to the To or From addresses, to them only RFC 6532 applies (which I pointed out in detail in my last comment, so go read that for what you should do), I'm going to here explain why RFC2047 does not apply to the addresses:
An 'encoded-word' may appear in a message header or body part header according to the following rules: (1) An 'encoded-word' may replace a 'text' token (as defined by RFC 822) in any Subject or Comments header field, any extension message header field, or any MIME body part field for which the field body is defined as '*text'. An 'encoded-word' may also appear in any user-defined ("X-") message or body part header field. (2) An 'encoded-word' may appear within a 'comment' delimited by "(" and ")", i.e., wherever a 'ctext' is allowed. More precisely, the RFC 822 ABNF definition for 'comment' is amended as follows: (3) As a replacement for a 'word' entity within a 'phrase', for example, one that precedes an address in a From, To, or Cc header. The ABNF definition for 'phrase' from RFC 822 thus becomes:
However, an excerpt from RFC822's ABNF (note I'm leaving out some alternatives that I think don't match anyway):
address = mailbox ; one addressee mailbox = addr-spec ; simple address / phrase route-addr ; name & addr-spec route-addr = "<" [route] addr-spec ">" addr-spec = local-part "@" domain ; global address local-part = word *("." word) ; uninterpreted domain = sub-domain *("." sub-domain) sub-domain = domain-ref / domain-literal domain-ref = atom ; symbolic reference atom = 1*<any CHAR except specials, SPACE and CTLs> CHAR = <any ASCII character> ; ( 0-177, 0.-127.)
Note explicitly that no path in addr-spec ever falls down to either a "text token in subject, comment, extension header or mime body part", "comment (qtext)", or "word entity within a phrase". Note that mailbox can allow phrase (which is the name in Patrick me@whatever.com), and that part CAN be =?utf?.... encoded, but addr-spec cannot.
This is even marked as an explicit sentence in RFC#2047: + An 'encoded-word' MUST NOT appear in any portion of an 'addr-spec'.
As far as I can see, the current code DOES satisfy the RFCs ("+ From: =?utf-8?b?WsO2w6k=?= pagure@pagure.org" encodes the name per 2047 and "To: zöé@foo.net" encodes the addr-spec per 6532), and the code itself looks reasonable to me. I think the variable "openid" is a misnomer (if it's really an openid identity, that would be a problem in itself as that can't be utf8-encoded), but other than that I'm fine with it.
I think the variable "openid" is a misnomer (if it's really an openid identity, that would be a problem in itself as that can't be utf8-encoded).
Yeah, that's a little technical debt that was laying around. I have renamed the function and the variable properly. PR #1542 is up to date :)
Rebased on the top of master, now that #1542 has been merged.
As far as I can see, the current code DOES satisfy the RFCs ("+ From: =?utf-8?b?WsO2w6k=?= pagure@pagure.org" encodes the name per 2047 and "To: zöé @foo.net" encodes the addr-spec per 6532), and the code itself looks reasonable to me. I think the variable "openid" is a misnomer (if it's really an openid identity, that would be a problem in itself as that can't be utf8-encoded), but other than that I'm fine with it.
Isn't it supposed to include the message/global media type? I assumed that was part of what Python added in 3.5's email module.
message/global
I honestly don't know anymore. "An 'encoded-word' MUST NOT appear in any portion of an 'addr-spec'" is pretty clear, but the way I'm reading the 6532 spec, what is being produced now doesn't match that either. Maybe it is just better to throw it out the door and see what happens. That makes me nervous, but I guess that's my problem.
@jcline: so, "encoded-word" is the =?utf?B?.....?= encoded unicode stuff from RFC 2047. If you look at my earlier comment (as was indicated in the one you just quoted from), in the RFC 6532 defines that you can use plain UTF8-non-ascii in the addr-spec, which is the normal unicode-encoded strings. So that is NOT an encoded-word as per 2047, but utf-8 as per 6532 (only the 2-, 3- or 4-byte variants, but that's pretty much all current characters).
This means that the NAME in the envelope addresses can be an encoded-word, but the addr-spec (email address) cannot be encoded-word, but only utf8.
I did not look at the MIME media types, as I was only looking at the envelope (To/From headers).
@jcline: also, 6532 says it must only be transferred if either it's marked as message/global, OR it's allowed per 6531. And postfix makes sure we adhere to 6531 by disallowing sending utf8-encoded email to systems that don't support SMTPUTF8: http://www.postfix.org/SMTPUTF8_README.html: " When a message is received with the SMTPUTF8 request, Postfix will deliver the message to a non-SMTPUTF8 SMTP or LMTP server ONLY if: .... ".
Okay, I'm satisfied.
Pull-Request has been merged by pingou
This ensure email having non-ascii characters are properly encoded
and displayed in the email clients.
Fixes https://pagure.io/pagure/issue/1496