classification
Title: smtplib: verify breaks with Postfix servers
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.2, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: catalin.iacob, felipecruz, mpg, pablomouzo, python-dev, r.david.murray
Priority: normal Keywords: easy, patch

Created on 2009-12-12 06:55 by mpg, last changed 2011-07-19 02:21 by r.david.murray. This issue is now closed.

Files
File name Uploaded Description Edit
bsl.py mpg, 2009-12-12 06:55 demonstration
issue7484-trunk.diff pablomouzo, 2009-12-25 17:41 trunk patch review
issue7484-py3k.diff pablomouzo, 2009-12-25 17:41 py3k patch review
issue7484-py3k.diff felipecruz, 2011-04-13 01:22 py3k patch review
issue7484-27.diff felipecruz, 2011-04-13 01:23 review
issue7484-py3k_2.diff felipecruz, 2011-04-13 18:48 improved patch review
issue7484-27_2.diff felipecruz, 2011-04-13 18:48 improved patch review
88b5c7ab7a03.diff catalin.iacob, 2011-07-17 12:57 review
unnamed felipecruz, 2011-07-19 02:17
Repositories containing patches
http://bitbucket.org/cataliniacob/cpython#issue7484
Messages (13)
msg96284 - (view) Author: Manuel Pégourié-Gonnard (mpg) * Date: 2009-12-12 06:55
Hi,

The verify method of SMTP objects created with smtplib doesn't work
properly with servers running Postfix, due to quoting problems: the
address is enclosed in pointed brackets by the method, which changes the
way it is interpreted by the server.

The attached demo file uses the mx1.nic.fr server, which runs Postfix
and exhibits the problem at the time of the writing.

RFC 5321 says the argument of VRFY is a string representing a "user
name", without saying much about what a "user name" is, but nothing
suggests it should be quoted in pointed brackets. Moreover, the example
in D.4 doens't use any quoting. 

Anyway, even if Postfix was wrong, I think it would be worth trying to
support it, since it is quite widely used.

Thanks!
msg96298 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2009-12-12 15:57
I agree.  My reading of the rfc is that the form without the brackets
*must* be supported by the MTA, while any other form is optional.  So
smtplib should use the required form for its VRFY query.  Any MTA that
doesn't recognize that form would be broken.

2.5 is in security-fix-only mode, so this can only be fixed in 2.6 and
above.
msg96884 - (view) Author: Pablo Mouzo (pablomouzo) Date: 2009-12-25 17:41
This patch solves the problem with the VRFY command, but I'm still 
wondering if this happens with other commands too.
msg133629 - (view) Author: Felipe Cruz (felipecruz) * Date: 2011-04-13 01:22
I've rewrote those patches to 'default' and 2.7
msg133637 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2011-04-13 01:59
Thanks for working on this.

The tests seem to be missing, as is the line that adds 'clean' to the def, so the patches won't work as is.

However, now that I've looked at the patch in more detail, adding a parameter to a public method is not something we can do in a bug fix release.  So, this solution would work for 3.3, but not for 2.7 and 3.2.  In any case, Guido thinks that parameters that have only two values should be replaced by methods with two different names.  In this case that makes a lot of sense.  I've checked the RFC and the code, and there are two cases: MAIL FROM and RCPT TO, which require the address to be in <>s, and VRFY and EXPN, which prefer that it not be in <>s.  So I think we should introduce a new, private function for use in the VRFY and EXPN cases:

    def _addronly(addr):
        (fullname, email) = email.utils.parseaddr(addr)
        return email

Can you do a new patch, adding the above function and using it at the right places?  Tests are also needed...it should be possible to modify the test that the original patch modified so that it checks to make sure the <> are not added.  If you need help with that let me know.
msg133684 - (view) Author: Felipe Cruz (felipecruz) * Date: 2011-04-13 18:48
David..

I extracted quoteaddr code to _addrformat and now quoteaddr and _addronly call _addrformat passing a format (<%s> or %s).

I've also created quoteaddr and _addronly test functions as well modified VRFY and EXPN tests to make sure they call _addronly and pointed brackets aren't added.

Let me know if those patches still need improvements.
msg140260 - (view) Author: Felipe Cruz (felipecruz) * Date: 2011-07-13 14:40
Can anyone take a loot at those patches?

Do they need more tests?
msg140527 - (view) Author: Catalin Iacob (catalin.iacob) * Date: 2011-07-17 12:56
I looked at the Felipe's patch and hopefully made some improvements.

Unlike Felipe's patch I didn't change the reply of the SMTP server in the tests but instead use what VRFY and EXPN actually send to index the users and lists dictionaries. If <> would be sent the lookup would fail. Similarly, when VRFY return 550 it echoed the address as received and now it's tested to be equal to something without <>.

By the way, but I was wondering:
* is the try/except really needed or just a historical artifact (why would email.utils.parseaddr raise AttributeError?)
* is the test to None correct? It was added by the fix to issue1430298 but does email.utils.parseaddr ever return None for the address? (I could only get it to return '')
I kept quoteaddr as is to make it easier to review the patch but if David confirms the above points are valid I can create new issues for them and simplify/fix quoteaddr.
msg140643 - (view) Author: Roundup Robot (python-dev) Date: 2011-07-19 01:43
New changeset c4d884d5d86c by R David Murray in branch '2.7':
#7484: no more <> around addresses in VRFY or EXPN
http://hg.python.org/cpython/rev/c4d884d5d86c

New changeset f8c4ac9aa9e2 by R David Murray in branch '3.2':
#7484: no more <> around addresses in VRFY or EXPN
http://hg.python.org/cpython/rev/f8c4ac9aa9e2

New changeset 0d9216de8f05 by R David Murray in branch 'default':
Merge #7484: no more <> around addresses in VRFY or EXPN
http://hg.python.org/cpython/rev/0d9216de8f05
msg140644 - (view) Author: Roundup Robot (python-dev) Date: 2011-07-19 02:00
New changeset 50b6c3053c30 by R David Murray in branch 'default':
#7484: simplify quoteaddr: if parseaddr throws an error it is a bug.
http://hg.python.org/cpython/rev/50b6c3053c30
msg140645 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2011-07-19 02:09
Thank you both for your work on this.  The patch I committed is a combination of my _addr_only, Filipe's tests, and Catalin's modifications to those tests.  quoteaddr, although in the __all__, is not documented and is really an implementation detail, as is the new _addr_only.  So I am only testing them indirectly through the documented parts of the API (I added a test for <> address, and one for an IDNA encoded address).

Catalin, I think you are correct about the try/except/None stuff.  As far as I can tell it is left over from the old days before the email package and its philosophy of never throwing parsing errors.  Nowadays if parseaddr throws an error, it is a bug.  That's a refactoring not a bug fix, though, so I didn't backport it.
msg140647 - (view) Author: Felipe Cruz (felipecruz) * Date: 2011-07-19 02:17
You're very kind David.

Hope I can contribute with something more relevant next time :)

best regards,
Felipe

2011/7/18 R. David Murray <report@bugs.python.org>

>
> R. David Murray <rdmurray@bitdance.com> added the comment:
>
> Thank you both for your work on this.  The patch I committed is a
> combination of my _addr_only, Filipe's tests, and Catalin's modifications to
> those tests.  quoteaddr, although in the __all__, is not documented and is
> really an implementation detail, as is the new _addr_only.  So I am only
> testing them indirectly through the documented parts of the API (I added a
> test for <> address, and one for an IDNA encoded address).
>
> Catalin, I think you are correct about the try/except/None stuff.  As far
> as I can tell it is left over from the old days before the email package and
> its philosophy of never throwing parsing errors.  Nowadays if parseaddr
> throws an error, it is a bug.  That's a refactoring not a bug fix, though,
> so I didn't backport it.
>
> ----------
> resolution:  -> fixed
> stage: test needed -> committed/rejected
> status: open -> closed
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue7484>
> _______________________________________
>
msg140648 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2011-07-19 02:21
Don't short change yourself.  This bug would still be open if it hadn't been for your work, regardless of how much of it wound up in the final patch :)
History
Date User Action Args
2011-07-19 02:21:24r.david.murraysetmessages: + msg140648
versions: + Python 3.3, - Python 2.6, Python 3.1
2011-07-19 02:17:09felipecruzsetfiles: + unnamed

messages: + msg140647
2011-07-19 02:09:54r.david.murraysetstatus: open -> closed
resolution: fixed
messages: + msg140645

stage: test needed -> resolved
2011-07-19 02:00:10python-devsetmessages: + msg140644
2011-07-19 01:43:51python-devsetnosy: + python-dev
messages: + msg140643
2011-07-17 12:57:16catalin.iacobsetfiles: + 88b5c7ab7a03.diff
2011-07-17 12:56:02catalin.iacobsethgrepos: + hgrepo43
messages: + msg140527
2011-07-13 14:40:57felipecruzsetmessages: + msg140260
2011-04-13 18:48:21felipecruzsetfiles: + issue7484-27_2.diff
2011-04-13 18:48:04felipecruzsetfiles: + issue7484-py3k_2.diff

messages: + msg133684
2011-04-13 01:59:47r.david.murraysetmessages: + msg133637
2011-04-13 01:23:17felipecruzsetfiles: + issue7484-27.diff
2011-04-13 01:22:50felipecruzsetfiles: + issue7484-py3k.diff
nosy: + felipecruz
messages: + msg133629

2011-04-12 09:12:01catalin.iacobsetnosy: + catalin.iacob
2009-12-25 17:41:43pablomouzosetfiles: + issue7484-py3k.diff
2009-12-25 17:41:07pablomouzosetfiles: + issue7484-trunk.diff

nosy: + pablomouzo
messages: + msg96884

keywords: + patch
2009-12-12 15:57:00r.david.murraysetpriority: normal

versions: + Python 2.6, Python 3.1, Python 2.7, Python 3.2, - Python 2.5
keywords: + easy
nosy: + r.david.murray

messages: + msg96298
stage: test needed
2009-12-12 06:55:52mpgcreate