classification
Title: decoding functions in the base64 module could accept unicode strings
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Anthony.Kong, berker.peksag, catalin.iacob, eric.araujo, gvanrossum, petri.lehtinen, pitrou, poq, python-dev, r.david.murray
Priority: normal Keywords: easy, patch

Created on 2011-12-20 13:04 by pitrou, last changed 2012-02-25 15:41 by r.david.murray. This issue is now closed.

Files
File name Uploaded Description Edit
issue13641_v1.diff berker.peksag, 2011-12-31 23:50 review
issue13641_v2_with_tests.diff berker.peksag, 2012-01-04 23:21 review
issue13641_v3_with_tests.diff berker.peksag, 2012-01-16 21:11 review
issue13641-alternative-v1.patch catalin.iacob, 2012-02-16 20:45 review
issue13641-alternative-v2.patch catalin.iacob, 2012-02-19 18:17 review
Messages (20)
msg149912 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-12-20 13:04
Similarly to #13637 for the binascii module, the decoding functions in the base64 module could accept ASCII-only unicode strings.
msg150445 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-01-01 22:15
Thanks for the patch, Berker. It seems a bit too simple, though. You should add some tests in Lib/test/test_base64.py and run them (using "./python -m test -v test_base64"), this will allow you to see if your changes are correct.
msg150633 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2012-01-04 23:21
Hi Antoine,

I added some tests for b64decode function.

Also, I wrote some tests for b32decode and b16decode functions and failed. I think my patch is not working for b32decode and b16decode functions. I'll dig into code and try to find a way.

Thanks!
msg151646 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-01-19 17:54
Thanks for the updated patch!
Two comments:
- I see no tests for map01 and altchars being passed as an str, is this supported by the patch or am I reading it wrong?
- apparently b16decode is not tackled, is it deliberate?

Thanks again.
msg153499 - (view) Author: Catalin Iacob (catalin.iacob) * Date: 2012-02-16 20:45
Attached alternative patch with a different approach: on input, strings are encoded as bytes and the rest of the code proceeds as before.

All existing tests for bytes now test for strings as well and there is a new test for strings with non ASCII characters.

Berker's patch was more intrusive and forgot to allow strings in _translate, leading to failures if altchars or map01 were used.
msg153500 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-02-16 21:14
Um.  I'm inclined to think that #13637 was a mistake.

Functions that accept bytes and return bytes and also accept string and return string seem uncontroversial.  However, accepting bytes or string and returning bytes is not an obviously good idea, and IMO at least merits some discussion.  In fact, I thought it *had* been discussed, specifically in the context of the b2a/a2b functions, and been rejected.
msg153501 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-02-16 21:17
> However, accepting bytes or string and returning bytes is not an
> obviously good idea, and IMO at least merits some discussion.

Why? "a" in "a2b" means ASCII, and unicode is as valid a container for ASCII text as bytes is.
msg153504 - (view) Author: (poq) Date: 2012-02-16 21:40
FWIW, I was surprised by the return type of b64encode when I first used it in Python 3. It seems to me that b64encode turns binary data into text and thus intuitively should take bytes and return str.

Similarly it seems intuitive to me for b64decode to take str as input and return bytes, as it turns text back into binary data.
msg153505 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-02-16 22:14
OK, I skimmed the thread I was remembering, and while it was discussing str->str and bytes->bytes primarily, the only pronouncement I could find was that functions should not accept a *mix* of bytes and string.  So I guess I withdraw my objection, although it still makes me a bit uncomfortable.
msg153604 - (view) Author: Catalin Iacob (catalin.iacob) * Date: 2012-02-17 21:52
My current patch allows mixing of bytes and str for the data to be decoded and the altchars or map01 parameter. Given David's observation in msg153505 I'll update the patch to require that both the data and altchars/map01 have the same type.
msg153713 - (view) Author: Catalin Iacob (catalin.iacob) * Date: 2012-02-19 18:17
Attached v2 of patch where mixing str and binary data for altchars or map01 raises TypeError.

I also added a note for each of the changed functions that it also accepts strings (but didn't also update the docstrings).

When writing the docs, the new functionality seemed hard to describe; maybe that means this issue only complicates things and is not worth it, or maybe it just means I don't have experience at writing docs.

But, regardless of having worked at a patch, I have to admit that I'm also not 100% sure this issue is a good idea. I *do* think that either both this issue and #13637 should be accepted or both rejected.
msg153714 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-02-19 18:21
I think trying to prevent mixed argument types is completely overkill. There's no ambiguity since they all have to be ASCII anyway.
So I would prefer to commit issue13641-alternative-v1.patch
msg153757 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-02-20 02:13
OK' I'm back to being 100% on the side of rejecting both of these changes.  ASCII is not unocode, it is bytes.  You can decode it to unicode but it is not unicode.  Those transformations operate bytes to bytes, not bytes to unicode.

We made the bytes unicode separation to avoid the problem where you have a working program that unexpectedly gets non ASCII input and blows up with a unicode error.  IMO these patches are reintroducing that problem.  The programer should have to explicitly encode to ASCII if they are inadvisedly workimg with it in a string as part of a wire protocol (why else would they be using these transforms).
msg153759 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-02-20 02:21
> OK' I'm back to being 100% on the side of rejecting both of these
> changes.  ASCII is not unocode, it is bytes.  You can decode it to
> unicode but it is not unicode.  Those transformations operate bytes to
> bytes, not bytes to unicode.

ASCII is just a subset of the unicode character set.

> We made the bytes unicode separation to avoid the problem where you
> have a working program that unexpectedly gets non ASCII input and
> blows up with a unicode error.

How is blowing up with a unicode error worse than blowing up with a
ValueError? Both indicate wrong input. At worse the code could catch
UnicodeError and re-raise it as ValueError, but I don't see the point.

> The programer should have to explicitly encode to ASCII if they are
> inadvisedly workimg with it in a string as part of a wire protocol
> (why else would they be using these transforms).

Inadvisedly? There are many situations where you can have base64 data in
some unicode strings.
msg153793 - (view) Author: Roundup Robot (python-dev) Date: 2012-02-20 18:33
New changeset c760bd844222 by Antoine Pitrou in branch 'default':
Issue #13641: Decoding functions in the base64 module now accept ASCII-only unicode strings.
http://hg.python.org/cpython/rev/c760bd844222
msg153794 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-02-20 18:34
I've committed issue13641-alternative-v1.patch. I really think practicality beats purity here and, furthermore, there's no associated danger (non-ASCII data is rejected both as bytes and str).
msg153827 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-02-21 01:06
Non-ascii binary data should not be being rejected unless validate
is true.  So what are you going to do with non-ascii-range unicode in
that case?  Ignore it as well?  That can't be right.

I believe this should be discussed on python-dev.
msg153828 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-02-21 01:10
I disagree with this commit.  Reopening pending discussion on python-dev.
msg153830 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-02-21 01:23
> Non-ascii binary data should not be being rejected unless validate
> is true.  So what are you going to do with non-ascii-range unicode in
> that case?  Ignore it as well?  That can't be right.

It's not ignored, it raises ValueError. Since the common case it to feed
valid (not invalid) baseXX data to these functions, that's a very benign
limitation.
msg153847 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2012-02-21 04:18
Aside: I, too, at first thought this would be a bad idea because it brings back the Python 2 issue of accepting some but not all Unicode strings. But then I realized that by their nature these functions only accepts a very specific set of characters -- so the restriction to (a subset of) ASCII is intrinsic to the functionality, and there is no possibility of confusion. If anything, accepting bytes is more likely to be confusing (they could be EBCDIC! :-). So no objection here. And a slight preference for ValueError.
History
Date User Action Args
2012-02-25 15:41:16r.david.murraysetstatus: open -> closed
2012-02-21 04:18:15gvanrossumsetnosy: + gvanrossum
messages: + msg153847
2012-02-21 01:23:45pitrousetmessages: + msg153830
2012-02-21 01:10:03r.david.murraysetstatus: closed -> open

messages: + msg153828
2012-02-21 01:06:41r.david.murraysetmessages: + msg153827
2012-02-20 18:34:54pitrousetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2012-02-20 18:34:40pitrousetmessages: + msg153794
2012-02-20 18:33:48python-devsetnosy: + python-dev
messages: + msg153793
2012-02-20 02:21:45pitrousetmessages: + msg153759
2012-02-20 02:13:31r.david.murraysetmessages: + msg153757
2012-02-19 18:21:54pitrousetmessages: + msg153714
2012-02-19 18:17:17catalin.iacobsetfiles: + issue13641-alternative-v2.patch

messages: + msg153713
2012-02-17 21:52:48catalin.iacobsetmessages: + msg153604
2012-02-16 22:14:10r.david.murraysetmessages: + msg153505
2012-02-16 21:40:53poqsetnosy: + poq
messages: + msg153504
2012-02-16 21:17:49pitrousetmessages: + msg153501
2012-02-16 21:14:32r.david.murraysetnosy: + r.david.murray
messages: + msg153500
2012-02-16 20:45:57catalin.iacobsetfiles: + issue13641-alternative-v1.patch
nosy: + catalin.iacob
messages: + msg153499

2012-01-19 17:54:21pitrousetmessages: + msg151646
stage: patch review
2012-01-16 21:11:29berker.peksagsetfiles: + issue13641_v3_with_tests.diff
2012-01-04 23:21:55berker.peksagsetfiles: + issue13641_v2_with_tests.diff

messages: + msg150633
2012-01-01 22:15:40pitrousetmessages: + msg150445
2011-12-31 23:50:05berker.peksagsetfiles: + issue13641_v1.diff
nosy: + berker.peksag
keywords: + patch
2011-12-30 20:50:55eric.araujosetnosy: + eric.araujo
2011-12-28 10:59:10Anthony.Kongsetnosy: + Anthony.Kong
2011-12-28 07:22:14petri.lehtinensetnosy: + petri.lehtinen
2011-12-20 13:04:47pitroucreate