| 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): --- 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 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) *  |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2012-05-28 01:28 |
| Fix committed. |
|
|