Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in

#21795: smtpd.SMTPServer should announce 8BITMIME when supported and accept SMTPUTF8 without it

Can't Edit
Can't Publish+Mail
Start Review
5 years, 3 months ago by milan.py
5 years, 3 months ago
barry, AntoinePitrou, r.david.murray, jesstess, devnull_psf.upfronthosting.co.za, zvyn

Patch Set 1 #

Total comments: 11

Patch Set 2 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Doc/library/smtpd.rst View 1 3 chunks +20 lines, -15 lines 0 comments Download
Lib/smtpd.py View 1 10 chunks +53 lines, -39 lines 0 comments Download
Lib/test/test_smtpd.py View 1 4 chunks +72 lines, -21 lines 0 comments Download


Total messages: 1
5 years, 3 months ago #1
I haven't reviewed the tests yet, but I wanted to publish this before the

File Doc/library/smtpd.rst (right):

Doc/library/smtpd.rst:57: When *decode_data* is set to ``True`` the server
accepts ``BODY=8BITMIME``
When *decode_data* is set to ``False`` and the client has executed the EHLO

Doc/library/smtpd.rst:61: .. method:: process_message(peer, mailfrom, rcpttos,
data, \*\*kwargs)
I don't think the escapes are required here, did you try it?

Doc/library/smtpd.rst:76: unless ``decode_data=False`` or
``enable_SMTPUTF8=True`` was given as
unless at least one of

Doc/library/smtpd.rst:77: init parameter. Otherwise it contains the following
an init parameter.

Doc/library/smtpd.rst:81: *rcpt_options*: same as *mail_options* but for the
``RCPT`` command.
This should note that currently no RCPT TO options are supported.

File Lib/smtpd.py (right):

Lib/smtpd.py:389: } if not self._decode_data else {}
Again, I would prefer an if here rather than a multiline expression:

kwargs = {}
if not self._decode_data:
   kwargs = dict(mail_options=self.mail_options, rcpt_options=self.rcpt_options)

Lib/smtpd.py:539: params_dict = self._getparams(self.mail_options)
I don't see the point in renaming this params_dict.  In python we generally
don't use any sort of hungarian notation.

Lib/smtpd.py:541: self.push(syntaxerr)
This was bad logic.  It depended on the fact that we only recognized *one*
parameter, SIZE, so if we had params and we got none back, that meant what we
did get was an invalid parameter and we pushed it.  Now we can have multiple
parameters, so if we have one good one and one bad one, we will this test will
pass and we will fail to generate a syntax error for the bad parameter.  So
_getparams needs to be changed to fix this.  The simplest fix is probably to
have it return None if there is an invalid parameter (something that doesn't
pass the isalnum test).

Lib/smtpd.py:545: params_dict.update({'BODY': body})
Shouldn't you instead be checking that BODY is one of 7BIT or 8BITMIME and
generating a syntax error if it is not?  And generating a syntax error in any
case if self.enable_8BITMIME is False?

Lib/smtpd.py:550: params_dict.update({'SMTPUTF8': True})
Please add a comment here that says "push the parameter back to produce an
unknown parameter error below".

Lib/smtpd.py:711: - 'rcpt_options': same for the rcpt command.
To be consistent with the code and the docs, this should say "will contain".  I
think that's better for backward compatibility (if/when we do support rctp to
options, an application can depend on the empty list existing if run using an
older python that does not support them).
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+