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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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'): |
There was a problem 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"); |
There was a problem 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.
There was a problem 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"
There was a problem 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 :)
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@bitdancer, were you referring to bpo-27397?
There was a problem 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 |
|
Thanks @taleinat for the PR |
…alid length (pythonGH-7416) (cherry picked from commit 1b85c71) Co-authored-by: Tal Einat <taleinat+github@gmail.com>
|
GH-7602 is a backport of this pull request to the 3.7 branch. |
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.
Give "Invalid length" rather than "Invalid padding" in this case.
Also added some specific test cases for invalid padding.
https://bugs.python.org/issue33770