Fix non-ASCII recipients #166
Conversation
|
Thanks for your help @berelian1 ! Would you be willing to add a unit test for this? Ideally, add a new unit test to |
|
Hi! Yes, I will create a new pull request with the changes reflected |
|
I had to remove some of the white space in other test cases in test_main.py as the pylint was giving me issues about the number of lines in the file, however, test case functionalities remain the same. Added test test_utf8smtp_trigger in test_main.py and test_utf8smtp_trigger in test_sendmail_client.py |
awdeorio
changed the title
awdeorio
left a comment
awdeorio
left a comment
There was a problem hiding this comment.
I did a more detailed review. This is a great start!
tests/test_main.py
Outdated
| """) # noqa: E501 | ||
|
|
||
|
|
||
| def test_utf8smtp_trigger(tmpdir): |
There was a problem hiding this comment.
How about renaming to test_utf8_recipient and cut-paste right after test_utf8_headers?
| [smtp_server] | ||
| host = open-smtp.example.com | ||
| """), encoding="utf8") | ||
|
|
There was a problem hiding this comment.
Instead of changing the whitespace, how about making a new file test_database.py and cut-paste the test_database_* unit tests into there?
tests/test_main.py
Outdated
| SUBJECT: UTF8SMTP Test | ||
| Hello{{name}}! | ||
| """), encoding="utf8") | ||
| database_path = Path(tmpdir/"mailmerge_database.csv") |
There was a problem hiding this comment.
Adding some comments would be nice
| username = awdeorio | ||
| """)) | ||
|
|
||
| sendmail_client = SendmailClient(config_path, dry_run=False) |
| sendmail_client = SendmailClient(config_path, dry_run=False) | ||
|
|
||
| message = email.message_from_string(textwrap.dedent("""\ | ||
| TO: mü[email protected] |
There was a problem hiding this comment.
Include a different CC and BCC to check those too
tests/test_main.py
Outdated
| """Verify triggers to rcpt_options=['UTF8SMTP'].""" | ||
| template_path = Path(tmpdir/"mailmerge_template.txt") | ||
| template_path.write_text(textwrap.dedent("""\ | ||
| TO: mü[email protected] |
There was a problem hiding this comment.
Include CC and BCC to check those too
tests/test_main.py
Outdated
| template_path = Path(tmpdir/"mailmerge_template.txt") | ||
| template_path.write_text(textwrap.dedent("""\ | ||
| TO: mü[email protected] | ||
| FROM: [email protected] |
There was a problem hiding this comment.
Could you advise what should happen if the FROM address contains a non-ASCII character?
There was a problem hiding this comment.
Hi, I have been reading on the SMTP documentation, and although it seems somewhat unclear, I think the sender must be checked along with the recipients.
3.2. The SMTPUTF8 Extension
" An SMTP server that announces the SMTPUTF8 extension MUST be prepared to accept a UTF-8 string [RFC3629] in any position in which RFC 5321 specifies that a can appear."
"An SMTP client that receives the SMTPUTF8 extension keyword in response to the EHLO command MAY transmit mailbox names within SMTP commands as internationalized strings in UTF-8 form. "
3.2. Client Initiation
"Once the server has sent the greeting (welcoming) message and the
client has received it, the client normally sends the EHLO command to
the server, indicating the client's identity. In addition to opening
the session, use of EHLO indicates that the client is able to process
service extensions and requests that the server provide a list of the
extensions it supports."
Since the sender must also initiate itself and verify identity which includes indicating its service extensions, this includes verifying whether its server can support non-ascii chars
mailmerge/sendmail_client.py
Outdated
| with smtplib.SMTP_SSL(host, port, context=ctx) as smtp: | ||
| smtp.login(self.config.username, self.password) | ||
| smtp.sendmail(sender, recipients, message_flattened) | ||
| if needs_smtputf8(sender, *recipients): |
There was a problem hiding this comment.
See earlier comment about whether sender should be here or not. I'm not sure.
mailmerge/sendmail_client.py
Outdated
| host, port = (self.config.host, self.config.port) | ||
|
|
||
| def needs_smtputf8(*string): | ||
| return any(any(ord(c) > 127 for c in part) for part in string) |
There was a problem hiding this comment.
Leave a note in a comment that this can be improve if/when we move to Python 3.7. Currently mailmerge supports Python 3.6+
https://stackoverflow.com/questions/196345/how-to-check-if-a-string-in-python-is-in-ascii
mailmerge/sendmail_client.py
Outdated
| raise exceptions.MailmergeError(f"SSL Error: {err}") | ||
| host, port = (self.config.host, self.config.port) | ||
|
|
||
| def needs_smtputf8(*string): |
There was a problem hiding this comment.
How about any_non_ascci?
Move to "vanilla" function outside the class.
2 participants
Handle non-ASCII recipients by including
includes rcpt_options=("SMTPUTF8")More detail: issue #143 brings up a bug where mailmerge would crash with a UnicodeEncodeError if an email address contained non-ASCII characters. I added logic to automatically detect non-ASCII characters in sender or recipient email addresses using a needs_smtputf8() helper function. If non-ASCII characters are detected, the sendmail() call includes rcpt_options=("SMTPUTF8"), as some smtp servers support the UTF8SMTP extension and are able to support non-ASCII characters. This ensures that mailmerge no longer crashes, as problematic emails can be skipped or handled in the future. In terms of the code, I updated the sendmail_ssltls() method to call needs_smtputf8(sender, *recipients) before sending.
Closes #143