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)

18852.patch (7.1 KB ) - added by Aymeric Augustin 12 years ago.
18852-1.diff (1.9 KB ) - added by Claude Paroz 12 years ago.
Sign() returns bytestring on python 2
18852-aggregate-python-3-port-changes.diff (4.7 KB ) - added by Aymeric Augustin 12 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 by Aymeric Augustin, 12 years ago

The regression was introduced in [92b2dec918].

This commit also unecessarily uses smart_bytes: we don't care about preserving lazy functions here.

Last edited 12 years ago by Aymeric Augustin (previous) (diff)

comment:2 by Aymeric Augustin, 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 Claude Paroz, 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 Aymeric Augustin, 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 Aymeric Augustin, 12 years ago

Has patch: set
Triage Stage: UnreviewedAccepted

Here's a patch that :

  • 1) uses byte strings in b64_encode and b64_decode for consistency with the functions provided by Python's base64 module
  • 2) uses native strings (str objects) as the output of sign 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 to force_unicode, which was replaced by force_text during the Python 3 migration, so the intent was clearly to return unicode there.

by Aymeric Augustin, 12 years ago

Attachment: 18852.patch added

by Claude Paroz, 12 years ago

Attachment: 18852-1.diff added

Sign() returns bytestring on python 2

comment:6 by Claude Paroz, 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 Aymeric Augustin, 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.

It is easier to read (and makes less changes) if you also take into account your previous commit. I'm attaching an aggregated diff that shows the net result.

Version 0, edited 12 years ago by Aymeric Augustin (next)

by Aymeric Augustin, 12 years ago

comment:8 by Aymeric Augustin <aymeric.augustin@…>, 12 years ago

Resolution: fixed
Status: newclosed

In [28ea4d4b07d98385e0bbfd54058e7eb42b246d26]:

Fixed #18852 -- Restored backwards compatibility

in django.core.signing. Specifically, kept the same return types
(str/unicode) under Python 2. Related to [92b2dec918].

Note: See TracTickets for help on using tickets.
Back to Top