Opened 5 years ago

Closed 5 years ago

#24836 closed Bug (fixed)

force_text doesn't force SimpleLazyObject into a string

Reported by: Piotr Maliński Owned by: nobody
Component: Utilities Version: 1.8
Severity: Release blocker Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

With Django 1.8 https://github.com/django/django/commit/8099d33b6553c9ee7de779ae9d191a1bf22adbda csrf_token now is a SimpleLazyObject. In my view for Facebook authentication JavaScript made an ajax request to authenticate user and get data back. To get new csrf_token I used:

force_text(csrf(request).get('csrf_token'))

But now json can't encode SimpleLazyObject. Should force_text force it to string or maybe using vanilla json isn't recommended even more for anything-Django data?

Change History (13)

comment:1 Changed 5 years ago by Tim Graham

Resolution: worksforme
Status: newclosed

I added this test to tests/csrf_tests/tests.py, but couldn't reproduce a problem:

from django.test import SimpleTestCase
from django.utils.encoding import force_text

class TestContextProcessor(SimpleTestCase):

    def test_force_text_on_token(self):
        request = HttpRequest()
        request.META['CSRF_COOKIE'] = 'test-token'
        self.assertEqual(force_text(csrf(request).get('csrf_token')), 'test-token')

Could you please reopen if you can provide a snippet that reproduces a crash?

comment:2 Changed 5 years ago by Piotr Maliński

Resolution: worksforme
Status: closednew

It does compare to string but it's not a string:

>>> from django.utils.functional import SimpleLazyObject
>>> from django.utils.encoding import force_text
>>> force_text(SimpleLazyObject(lambda: 'aaa'))
<SimpleLazyObject: 'aaa'>

so:

>>> force_text(SimpleLazyObject(lambda: 'aaa')) == 'aaa'
True
>>> import json
>>> json.dumps(force_text(SimpleLazyObject(lambda: 'aaa')))
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/usr/lib/python3.4/json/__init__.py", line 230, in dumps
    return _default_encoder.encode(obj)
  File "/usr/lib/python3.4/json/encoder.py", line 186, in encode
    return encode_basestring_ascii(o)
TypeError: first argument must be a string, not SimpleLazyObject

comment:3 Changed 5 years ago by Tim Graham

Component: UtilitiesCSRF
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

Asked on the original pull request if the author has an idea of how to fix it. If not, we could revert the breaking patch and add a test for this case (unless the performance benefits are deemed to be beneficial enough such that breaking backwards compatibility is okay).

comment:4 Changed 5 years ago by Piotr Maliński

It may also be a point for "force_text" - how much should it force to be a "text" instead of an object:

Similar to smart_text, except that lazy instances are resolved to strings, rather than kept as lazy objects.

comment:5 Changed 5 years ago by Carl Meyer

Yes, it seems to me that this is just a bug in force_text; it doesn't do what it says on the tin.

comment:6 Changed 5 years ago by Tim Graham

I'm not quite sure what the fix would look like, but I think force_text() might need special knowledge of SimpleLazyObject if we were to fix it there, which doesn't seem like a good separation of concerns. The docstring for SimpleLazyObject says, "Designed for compound objects of unknown type. For builtins or objects of known type, use django.utils.functional.lazy."

Currently force_text() does a six.text_type() cast and type(force_text(obj) -> SimpleLazyObject if obj is a SimpleLazyObject.

comment:7 Changed 5 years ago by Carl Meyer

I think the bug is in the interaction between the fact that SimpleLazyObject overrides isinstance checks in order to pretend to be its wrapped type, and the performance shortcut at the top of force_text:

if isinstance(s, six.text_type):
    return s

So in the specific situation of a SimpleLazyObject wrapping an object of six.text_type, force_text does nothing at all, it doesn't cast to six.test_type. (If it actually did cast, we wouldn't have this problem; SimpleLazyObject does not survive a type cast).

IMO the right fix would be to make the shortcut check stricter:

if issubclass(type(s), six.text_type):
    return s

Then force_text would actually behave as its docstring claims, and resolve all lazy instances (including SimpleLazyObject) to strings.

We could make the check even stricter and require an exact class match -- if type(s) is six.text_type -- but then force_text would begin reducing string subclasses to the base string type too. I don't have a clear intuition that it ought to do that, so I think I'd favor the issubclass version.

Last edited 5 years ago by Carl Meyer (previous) (diff)

comment:8 Changed 5 years ago by Marten Kenbeek

Specifically, if type(s) is six.text_type would cast a SafeText instance to a plain string, so I'd say that issubclass is the way to go in that case.

The problems with json.dumps and the like come from that fact that deep down, on a C level, a SimpleLazyObject is simply not a string. I don't believe that is something we can fix. Subclasses of six.text_type don't have that problem.

comment:9 Changed 5 years ago by Tim Graham

Component: CSRFUtilities
Has patch: set
Patch needs improvement: set

PR, but have some test failures on Python 2 that need to be investigated.

comment:10 Changed 5 years ago by Tim Graham

Patch needs improvement: unset

Test failures resolved.

comment:11 Changed 5 years ago by Claude Paroz

Triage Stage: AcceptedReady for checkin

I'm a bit scared by introducing such a low-level change in a stable release. Another scenario would be to revert Alex change in 1.8 and introduce this in master only.

comment:12 Changed 5 years ago by Tim Graham <timograham@…>

In b16f84f1:

[1.8.x] Refs #24836 -- Reverted "Simplified the lazy CSRF token implementation in csrf context processor."

This reverts commit 8099d33b6553c9ee7de779ae9d191a1bf22adbda as it caused
a regression that cannot be solved without changing force_text() which has
a small risk of introducing regressions. This change will remain in master
along with an update to force_text().

comment:13 Changed 5 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 70be31bb:

Fixed #24836 -- Made force_text() resolve lazy objects.

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