Repository navigation
gh-125346: Fix decoding with non-standard Base64 alphabet - #141128
serhiy-storchaka merged 11 commits into
Conversation
Sorry, something went wrong.
The "+" and "/" characters are no longer recognized as the part of the Base64 alphabet in base64.urlsafe_b64decode() and base64.b64decode() the altchars argument that does not contain them.
|
@serhiy-storchaka Thanks for this, can you link this PR to either the original issue or a new issue for tracking purposes. |
Sorry, something went wrong.
sethmlarson
left a comment
There was a problem hiding this comment.
I'm really concerned about the subtle breakages that this change could cause, especially because the default behavior is to throw away characters that aren't in the current alphabet. If the default behavior was to raise an error I would feel better about this change.
Makes me wonder if we should target validate=True with this behavior change (because IMO, the silent dropping of invalid characters is in itself a concerning behavior) and then long-term move to having validate be enabled by default?
Sorry, something went wrong.
serhiy-storchaka
left a comment
There was a problem hiding this comment.
This worries me too. We can keep the old behavior but emit a warning if characters + or / occur in Base64 data with the alternative alphabet.
But urlsafe_b64decode() does not have the validate parameter.
Sorry, something went wrong.
I wonder if for urlsafe_b64decode() it is okay to error out on bad characters as the function name is more clear that this is for a specific base64 alphabet? |
Sorry, something went wrong.
|
But is not passing altchars to b64decode() also makes it clear that it uses a different alphabet? |
Sorry, something went wrong.
|
@serhiy-storchaka Yes maybe you're right that using altchars is enough that users should be expecting the alphabet to completely shift. I did some poking around with libraries that use altchars with b64decode() and found comments that confirm your thinking. Maybe that's a vote for moving to always erroring in case a non-alphabet value is supplied? |
Sorry, something went wrong.
|
If this is only for 3.15, then making validate=True by default will make the code more reliable by default. |
Sorry, something went wrong.
…ternative alphabet is used (pythonGH-141128) Emit a warning in base64.urlsafe_b64decode() and base64.b64decode() when the "+" or "/" characters occur in the Base64 data with alternative alphabet if they are not the part of the alternative alphabet. It is a DeprecationWarning in the strict mode (will be error) and a FutureWarning in non-strict mode (will be ignored).
|
Sorry to comment on a merged pull request but I'd rather ask the question here before opening an issue: The altchars len validation moved from a simple assert to a ValueError. While I think it makes sense it's not being documented as being changed. Should it be ? Line 61 in 77c06f3 Does either of this concerns make sense ? If so I'll open a new issue. |
Sorry, something went wrong.
|
feel free to open a new PR referencing the same issue for little cleanup followups like that. We don't need to document ValueError or document that one as that is a normal error response to an API not being used quite right. |
Sorry, something went wrong.
Some minor changes to be able to compile mypy with mypyc on Python 3.15.0a8. - Update `typing-extensions` to `>=4.14.0` for Python `>=3.15` due to the removal of `typing.no_type_check_decorator` python/cpython#133601 https://docs.python.org/3/library/typing.html#typing.no_type_check_decorator https://github.com/python/typing_extensions/blob/4.14.0/CHANGELOG.md - `TypeAliasType.__qualname__` was added in python/cpython#139817 - Some new FutureWarnings for b64_decode python/cpython#125346 python/cpython#141128
Some minor changes to be able to compile mypy with mypyc on Python 3.15.0a8. - Update `typing-extensions` to `>=4.14.0` for Python `>=3.15` due to the removal of `typing.no_type_check_decorator` python/cpython#133601 https://docs.python.org/3/library/typing.html#typing.no_type_check_decorator https://github.com/python/typing_extensions/blob/4.14.0/CHANGELOG.md - `TypeAliasType.__qualname__` was added in python/cpython#139817 - Some new FutureWarnings for b64_decode python/cpython#125346 python/cpython#141128
The "+" and "/" characters are no longer recognized as the part of the Base64 alphabet in base64.urlsafe_b64decode() and base64.b64decode() the altchars argument that does not contain them.