bpo-33770: improve base64 exception message for encoded inputs of invalid length by taleinat · Pull Request #7416 · python/cpython (original) (raw)
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation10 Commits7 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Give "Invalid length" rather than "Invalid padding" in this case.
Also added some specific test cases for invalid padding.
| # Test base64 with invalid padding |
|---|
| def assertInvalidPadding(data): |
| with self.assertRaises(binascii.Error, |
| expected_regexp=r'(?i)Invalid padding'): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember other case of passing this argument as keyword. I'm not sure the name of this parameter is the part of the public API.
And seems you meant assertRaisesRegex.
Thanks for the review @serhiy-storchaka! To my horror, both the tests and the implementation were actually broken. Now fixed, ready for re-review.
| ** This is an invalid length, as there is no possible input that |
|---|
| ** could encoded into such a base64 string. |
| */ |
| PyErr_SetString(Error, "Invalid length"); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to change this, I'd suggest adding some verbiage to the error message saying that padding ==6 (or whatever) is invalid in base64, and probably mentioning the value of leftbits, if that's useful to the caller. But just having two different error messages for different cases with no additional context would confuse me more than it would help me.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like this? "Invalid base64-encoded string: length cannot be 1 more than a multiple of 4"
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that it is important, IMO, that the messages be different. In the 'incorrect padding' case one can add padding to get a decode (which may be wrong, but most likely will be right). In the invalid length case, one cannot do that. The email package cares about this, as it tries its best to do the decode even if the padding is incorrect. I'm pretty sure there is an open issue against the email package for which this is the answer, though I don't have time to go look for it right now :)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if this difference is important, perhaps this should be a separate exception class/sub-class?
also fixed error message missing space
@taleinat: Please replace # with GH- in the commit message next time. Thanks!
Thanks @taleinat for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request
…alid length (pythonGH-7416)
(cherry picked from commit 1b85c71)
Co-authored-by: Tal Einat taleinat+github@gmail.com
ned-deily pushed a commit that referenced this pull request
…alid length (GH-7416) (GH-7602)
(cherry picked from commit 1b85c71)
Co-authored-by: Tal Einat taleinat+github@gmail.com
jugmac00 pushed a commit to jugmac00/launchpad that referenced this pull request
python/cpython#7416 changed the exception message raised for base64 inputs whose length has a remainder of 1 when divided by 4. In these tests, we want to make sure that the underlying exception message is passed through to allow debugging, but we don't much care about the exact details, so change the input data to something that raises an "Incorrect padding" exception on Python 3.8 as well as on earlier versions.