Opened 12 years ago
Closed 12 years ago
#18852 closed Bug (fixed)
Backwards-incompatible change in django.core.signing
Reported by: | Aymeric Augustin | Owned by: | nobody |
---|---|---|---|
Component: | Core (Other) | Version: | dev |
Severity: | Release blocker | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
(django-dev)myk@myk django % git checkout stable/1.4.x Switched to branch 'stable/1.4.x' (django-dev)myk@myk django % python Python 2.7.1 (r271:86832, Jun 16 2011, 16:59:05) [GCC 4.2.1 (Based on Apple Inc. build 5658) (LLVM build 2335.15.00)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> from django.core import signing >>> signing.b64_encode('foo') 'Zm9v' >>> ^D (django-dev)myk@myk django % git checkout master Switched to branch 'master' (django-dev)myk@myk django % python Python 2.7.1 (r271:86832, Jun 16 2011, 16:59:05) [GCC 4.2.1 (Based on Apple Inc. build 5658) (LLVM build 2335.15.00)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> from django.core import signing >>> signing.b64_encode('foo') u'Zm9v' >>> ^D
A string is returned under 1.4.x, a unicode in master.
Attachments (3)
Change History (11)
comment:2 by , 12 years ago
I used a private API in the bug report, however, the same problem happens in the public APIs:
>>> from django.core.signing import Signer >>> signer = Signer() >>> value = signer.sign('My string') >>> value u'My string:ChWrz0sf2O07KCdM9iP-xb50tuY'
Notice that a unicode value is returned here, where the docs show a bytestring.
comment:3 by , 12 years ago
I'm not sure it's a regression from 1.4. For example, in 1.4, replace 'q;wjmbk;wkmb' by 'q;wjmbk;wkmb\xc3\xa0' (utf-8 encoded bytestring) in line 41 (test_sign_unsign) of tests/regressiontests/signing/tests.py. This will fail as the returned string is also unicode.
I would be more enclined to fix the docs, unless you could elaborate about real issues it could bring.
comment:4 by , 12 years ago
I noticed the issue by accident, because it breaks https://github.com/aaugustin/django-sesame.
The issues brought by a backwards-incompatible changes in public APIs are obvious. We have an API stability policy.
comment:5 by , 12 years ago
Has patch: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
Here's a patch that :
- 1) uses byte strings in
b64_encode
andb64_decode
for consistency with the functions provided by Python'sbase64
module - 2) uses native strings (
str
objects) as the output ofsign
for backwards compatibility with previous versions of Django on Python 2, and because I believe it's the right thing to do under Python 3, - 3) uses unicode strings as the output of
unsign
for backwards compatibility — there was an explicit call toforce_unicode
, which was replaced byforce_text
during the Python 3 migration, so the intent was clearly to return unicode there.
by , 12 years ago
Attachment: | 18852.patch added |
---|
comment:6 by , 12 years ago
Here is a minimal patch to fix the type of the returned value of sign() on Python 2. I would not opposed your approach, but I generally try to avoid manipulating bytestrings, which seems to me a little more Python-3 friendly. Would my patch fix your issue in django-sesame?
comment:7 by , 12 years ago
I agree about avoiding bytestrings when possible. At the lower level, crypto operates on bytes. The question is, where we draw the frontier?
My proposal moves it back "upwards" a bit — above the b64_decode/encode
functions.
The diff is easier to read (with less changes) if you also take into account your previous commit. I'm attaching an aggregated diff that shows the net result.
by , 12 years ago
Attachment: | 18852-aggregate-python-3-port-changes.diff added |
---|
comment:8 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
The regression was introduced in [92b2dec918].
This commit also unecessarily uses
smart_bytes
: we don't care about preserving lazy functions here.