Opened 3 years ago

Closed 3 years ago

#18852 closed Bug (fixed)

Backwards-incompatible change in django.core.signing

Reported by: aaugustin Owned by: nobody
Component: Core (Other) Version: master
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 aaugustin 3 years ago.
18852-1.diff (1.9 KB) - added by claudep 3 years ago.
Sign() returns bytestring on python 2
18852-aggregate-python-3-port-changes.diff (4.7 KB) - added by aaugustin 3 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 3 years ago by aaugustin

The regression was introduced in [92b2dec918].

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

Last edited 3 years ago by aaugustin (previous) (diff)

comment:2 Changed 3 years ago by aaugustin

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 Changed 3 years ago by claudep

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 Changed 3 years ago by aaugustin

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 Changed 3 years ago by aaugustin

  • Has patch set
  • Triage Stage changed from Unreviewed to Accepted

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.

Changed 3 years ago by aaugustin

Changed 3 years ago by claudep

Sign() returns bytestring on python 2

comment:6 Changed 3 years ago by claudep

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 Changed 3 years ago by aaugustin

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.

Last edited 3 years ago by aaugustin (previous) (diff)

Changed 3 years ago by aaugustin

comment:8 Changed 3 years ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from new to closed

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