classification
Title: silent error in email.message.Message.get_payload
Type: enhancement Stage: resolved
Components: email, Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: r.david.murray Nosy List: BreamoreBoy, ajaksu2, barry, python-dev, r.david.murray, rndblnch
Priority: normal Keywords: patch

Created on 2007-03-02 17:04 by rndblnch, last changed 2012-05-28 01:28 by r.david.murray. This issue is now closed.

Files
File name Uploaded Description Edit
unsilent_get_payload_decoding_errors.patch rndblnch, 2009-04-06 10:38 rough patch
noisy_get_payload.diff ajaksu2, 2009-05-12 05:03 Add a test case for get_payload with silent=False review
base64_payload_defects.patch r.david.murray, 2012-05-27 20:53 review
Messages (19)
msg31413 - (view) Author: Renaud Blanch (rndblnch) Date: 2007-03-02 17:04
I rencently had trouble finding a bug in an email processing script because of an error that pass silently in the email module.
The get_payload() method in the email.message module always return something when the decode argument is set to True.
This behaviour is well documented, but is their a reason to catch the decoding errors ?
Why not let them pop-up to the calling code ?

renaud
msg84660 - (view) Author: Daniel Diniz (ajaksu2) Date: 2009-03-30 21:42
Renaud: providing a test script would help us here.
msg85337 - (view) Author: Renaud Blanch (rndblnch) Date: 2009-04-03 21:58
Daniel: i can't remember the exact scenario (i filled this bug 2 years
ago !)
after having a look back at email.message.Message.get_payload, i
remember the problem: the decoding errors are silented by the method and
you have no way to know if the decoding has been successful or not.
to find my malformed email i had to patch the module like that
(basically just removing the try/except blocks):

<patch>
--- message.py.orig	2009-04-03 23:46:47.000000000 +0200
+++ message.py	2009-04-03 23:48:27.000000000 +0200
@@ -192,19 +192,11 @@
             if cte == 'quoted-printable':
                 return utils._qdecode(payload)
             elif cte == 'base64':
-                try:
-                    return utils._bdecode(payload)
-                except binascii.Error:
-                    # Incorrect padding
-                    return payload
+                return utils._bdecode(payload)
             elif cte in ('x-uuencode', 'uuencode', 'uue', 'x-uue'):
                 sfp = StringIO()
-                try:
-                    uu.decode(StringIO(payload+'\n'), sfp, quiet=True)
-                    payload = sfp.getvalue()
-                except uu.Error:
-                    # Some decoding problem
-                    return payload
+                uu.decode(StringIO(payload+'\n'), sfp, quiet=True)
+                return sfp.getvalue()
         # Everything else, including encodings with 8bit or 7bit are
returned
         # unchanged.
         return payload
</patch>

once again, the behaviour is documented, so it's not really a bug.
but it caused me a lot of trouble (and it does not conforms very well to
"Errors should never pass silently.":)

but i guess applying such a patch could potentially break number of
client code so... is it worth the change?
msg85605 - (view) Author: Daniel Diniz (ajaksu2) Date: 2009-04-06 01:40
Hmm, ISTM that a change that breaks existing code that relies on
documented behavior has a negligible chance of being accepted.

However, I agree that the current behavior isn't developer-friendly. I
think adding an alternative behavior to get_payload (add a new
parameter?), allowing callers to log errors and/or displaying warnings
could have a better chance of being accepted, while still providing the
control you want.

Barry?
msg85632 - (view) Author: Renaud Blanch (rndblnch) Date: 2009-04-06 10:38
good idea, why not something like sketched in the attached patch?
it does not break any existing code, while providing a way for new users
to have a chance to get the decoding errors.
of course, the doc should be updated accordingly, and tests should be added.
msg86270 - (view) Author: Daniel Diniz (ajaksu2) Date: 2009-04-22 05:07
Looks good to me, adding tests and docs could be a nice Bug Day task.
msg87606 - (view) Author: Daniel Diniz (ajaksu2) Date: 2009-05-12 05:02
Renaud,
Here's your patch with a test case against trunk.
msg87684 - (view) Author: Renaud Blanch (rndblnch) Date: 2009-05-13 13:42
looks very good to me.
thanks daniel for your work
renaud
msg110522 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2010-07-16 22:49
To me a bug is a bug is a bug, but I'm sure there are different opinions so feel free to swap this back to feature request if you think that fits.  I'll try and have a crack at this tomorrow.
msg110526 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-07-16 23:29
No, it's not a bug if it is working as documented.  (Well, it might be a design bug, but fixes to those are called "feature requests" :)

We're planning on addressing this is email6, by the way.

I'm OK with trying to get this into 3.2, but I'll be giving higher priority to bugs.
msg110566 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2010-07-17 15:20
I've patched py3k test_email.py without patching message.py and the test passes on Windows Vista.  Either the patch is wrong, I've had finger problems or I'm misunderstanding the intent, can someone please advise.
msg111494 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2010-07-24 17:48
I've double checked this and still all tests pass before patching message.py.  Could somebody else please pick this one up, thanks.
msg123138 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-12-02 22:36
I've taken another look at this, and the email module is pretty consistent about just passing through data if it can't interpret it according to standards.  I think it would lead to a cluttered API if we add support for being strict and raising errors piecemeal.  So I'm deferring this to 3.3 and the email6 work, where we plan to have a comprehensive 'strict' mode.
msg161722 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-05-27 20:37
OK, here is patch based on the new policy support in 3.3.  I have some concern that the behavior change it introduces might cause some issues, but since it seems like a reasonable change and is happening at a feature release boundary, I think it should be OK.

The behavior change is that now by default get_payload will always produce *some* kind of decoding of a base64 part, and will register defects if it finds padding errors or characters outside the base64 character set.
msg161726 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-05-27 20:44
Oh, and to be clear on how this addresses the bug report: if you set 'raise_on_defect' to true when you call the parser, then you'll get an exception when you call msg.get_payload(decode=True).
msg161728 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-05-27 20:53
Hmm.  Thinking about it, though, that might not work if there are other errors in the message, many of which are more benign.  Probably the raise_on_defect control is a little too coarse.

I've been thinking that we need a way to set the policy on an already existing message, which would make this work more usefully.  But that's a different issue.
msg161745 - (view) Author: Roundup Robot (python-dev) Date: 2012-05-28 01:24
New changeset 17341b51af4f by R David Murray in branch 'default':
#1672568: email now registers defects for base64 payload format errors.
http://hg.python.org/cpython/rev/17341b51af4f
msg161746 - (view) Author: Roundup Robot (python-dev) Date: 2012-05-28 01:28
New changeset d68e30be755e by R David Murray in branch 'default':
News item for #1672568.
http://hg.python.org/cpython/rev/d68e30be755e
msg161747 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-05-28 01:28
Fix committed.
History
Date User Action Args
2012-05-28 01:28:30r.david.murraysetstatus: open -> closed
resolution: fixed
messages: + msg161747

stage: patch review -> resolved
2012-05-28 01:28:07python-devsetmessages: + msg161746
2012-05-28 01:24:24python-devsetnosy: + python-dev
messages: + msg161745
2012-05-27 20:53:47r.david.murraysetfiles: + base64_payload_defects.patch

messages: + msg161728
2012-05-27 20:50:27r.david.murraysetfiles: - base64_payload_defects.patch
2012-05-27 20:44:27r.david.murraysetmessages: + msg161726
2012-05-27 20:37:55r.david.murraysetfiles: + base64_payload_defects.patch
keywords: + patch
messages: + msg161722
2012-05-16 01:04:56r.david.murraysetcomponents: + email
2010-12-02 22:36:03r.david.murraysetkeywords: - patch, easy

messages: + msg123138
versions: + Python 3.3, - Python 3.2
2010-07-24 17:48:44BreamoreBoysetmessages: + msg111494
2010-07-17 15:20:59BreamoreBoysetmessages: + msg110566
2010-07-16 23:29:51r.david.murraysettype: behavior -> enhancement
messages: + msg110526
versions: - Python 3.1, Python 2.7
2010-07-16 22:49:21BreamoreBoysetversions: + Python 3.2
nosy: + BreamoreBoy

messages: + msg110522

type: enhancement -> behavior
2010-05-05 13:41:06barrysetassignee: barry -> r.david.murray

nosy: + r.david.murray
2009-05-13 13:42:19rndblnchsetmessages: + msg87684
2009-05-12 05:03:18ajaksu2setfiles: + noisy_get_payload.diff

messages: + msg87606
stage: test needed -> patch review
2009-04-22 05:07:38ajaksu2setkeywords: + easy

messages: + msg86270
2009-04-06 10:38:29rndblnchsetfiles: + unsilent_get_payload_decoding_errors.patch

messages: + msg85632
2009-04-06 01:40:22ajaksu2setkeywords: + patch
type: behavior -> enhancement
messages: + msg85605

versions: + Python 3.1, Python 2.7, - Python 2.6, Python 3.0
2009-04-03 21:58:34rndblnchsetmessages: + msg85337
2009-03-30 21:42:18ajaksu2setversions: + Python 2.6, Python 3.0
nosy: + ajaksu2

messages: + msg84660

type: behavior
stage: test needed
2007-03-02 17:04:08rndblnchcreate