[Django] #28679: urlencode does not decode bytes before passing them to Python's urlencode
Django
-------------------------------------+-------------------------------------
Reporter: François | Owner: nobody
Freitag |
Type: | Status: new
Uncategorized |
Component: Utilities | Version: 2.0
Severity: Normal | Keywords: urlencode bytes
Triage Stage: | Has patch: 1
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
{{{urlencode}}} should decode bytes objects before to pass them to
python's urlencode. Instead, it's currently calling str on the bytes,
passing the {{{bytes.__str__}}} representation of the bytes (b'...') to
urlencode.
{{{#!python
# A dictionary with bytes value
result = http.urlencode({'a': b'abc'}, doseq=True)
self.assertEqual(result, 'a=abc')
# Test result on 2.0a1
AssertionError: 'a=b%27abc%27' != 'a=abc'
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28679>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
| Freitag
Type: Uncategorized | Status: assigned
Component: Utilities | Version: 2.0
Severity: Normal | Resolution:
Keywords: urlencode bytes | Triage Stage:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => François Freitag
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/28679#comment:1>
Django
-------------------------------------+-------------------------------------
| Freitag
Type: Uncategorized | Status: assigned
Severity: Normal | Resolution:
Keywords: urlencode bytes | Triage Stage:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by François Freitag):
[https://github.com/django/django/pull/9197 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/28679#comment:2>
Django
-------------------------------------+-------------------------------------
| Freitag
Component: Utilities | Version: 2.0
Severity: Release blocker | Resolution:
Keywords: urlencode bytes | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* type: Uncategorized => Bug
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
Sure something needs to be fixed (sorry, I introduced the regression in
[fee42fd99ee470528858c2ccb3621135c30ec262]).
I just wonder if we should use `force_text` or rather let bytes be bytes,
as the Python `urlencode` seems to accept both strings or bytes.
--
Ticket URL: <https://code.djangoproject.com/ticket/28679#comment:3>
Django
-------------------------------------+-------------------------------------
| Freitag
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: urlencode bytes | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jon Dufresne):
What is the use case for passing bytes to `urlencode()`?
As URLs are strings, I'd expect their query parameters to also be strings
(or coercable to a string). At first glance, this feels like the bytes
data is being passed through the system as if it were a string instead of
being deocded/encoded at its endpoints.
--
Ticket URL: <https://code.djangoproject.com/ticket/28679#comment:4>
Django
-------------------------------------+-------------------------------------
| Freitag
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: urlencode bytes | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by François Freitag):
Python is
[https://github.com/python/cpython/blob/543386b7f077d210ea0722079d68beb6c066730a/Lib/urllib/parse.py#L867-L891|treating
bytes differently than other types], perhaps Django should maintain that
behaviour?
IMHO, decoding user-encoded text (assuming utf-8) and re-encoding is not
useful and may cause harm to users who decided to encode their data using
another encoding. I tend to agree with Claude on special-casing bytes.
--
Ticket URL: <https://code.djangoproject.com/ticket/28679#comment:5>
Django
-------------------------------------+-------------------------------------
| Freitag
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: urlencode bytes | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz):
Replying to [comment:4 Jon Dufresne]:
> What is the use case for passing bytes to `urlencode()`?
I had the same question initially. But then I saw that Python 3
`urlencode` was also mentioning `bytes` as possible input, so I guess
there are valid use cases for that.
--
Ticket URL: <https://code.djangoproject.com/ticket/28679#comment:6>
Django
-------------------------------------+-------------------------------------
| Freitag
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: urlencode bytes | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jon Dufresne):
> Python is bytes differently than other types, perhaps Django should
maintain that behaviour?
I see. Is this the only motivation or is there a use case? While I might
disagree with Python's choice here, I understand the argument of following
upstream and the wider community.
> IMHO, decoding user-encoded bytes (assuming utf-8) and re-encoding is
not useful and may cause harm to users who decided to encode their data
using another encoding. I tend to agree with Claude on special-casing
bytes.
This is precisely what
[https://github.com/django/django/blob/9718fa2e8abe430c3526a9278dd976443d4ae3c6/django/utils/encoding.py#L51-L70
force_text()] is doing. If decoding as utf-8 is causing harm, it hasn't
been avoided.
By using `force_text()` there is an implicit assumption that the bytes
data is utf-8 encoded. While that may be true for a large majority of the
cases, there is no reason to believe this is true for all cases. Having
the bytes data properly decoded as it is received would avoid this
assumption.
--
Ticket URL: <https://code.djangoproject.com/ticket/28679#comment:7>
Django
-------------------------------------+-------------------------------------
| Freitag
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: urlencode bytes | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by François Freitag):
My use case is a
[https://github.com/knaperek/djangosaml2/blob/b39eba56b7a0d89ad0b4f261c45f4d579143f64a/djangosaml2/tests/__init__.py#L391-L396|
library passing zipped xml, base64 encoded] over GET parameters. The
output of {{{base64.b64encode}}} is bytes, and I think Django should not
change that (even though it does not matter in my use case).
I updated the PR to simply pass bytes "as is" to Python's {{{urlencode}}}.
I agree that calling {{{force_text}}} on the value is not the best option.
Is this the only motivation or is there a use case?
I do not have a use case for Django not decoding bytes, but I find it
safer than decoding a content that does not need it. I prefer Django being
consistent with Python. The added value of Django's {{{urlencode}}} is the
understanding of QueryDict, I do not expect it to re-encode the bytes I've
given.
--
Ticket URL: <https://code.djangoproject.com/ticket/28679#comment:8>
Django
-------------------------------------+-------------------------------------
| Freitag
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: urlencode bytes | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jon Dufresne):
> I updated the PR to simply pass bytes "as is" to Python's `urlencode`.
This approach makes sense to me. Thanks for explaining.
--
Ticket URL: <https://code.djangoproject.com/ticket/28679#comment:9>
Django
-------------------------------------+-------------------------------------
| Freitag
Component: Utilities | Version: 2.0
Severity: Release blocker | Resolution: fixed
Keywords: urlencode bytes | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"41be85862d9067a809ccf3707d2a22dfef23d99a" 41be8586]:
{{{
#!CommitTicketReference repository=""
revision="41be85862d9067a809ccf3707d2a22dfef23d99a"
Fixed #28679 -- Fixed urlencode()'s handling of bytes.
Regression in fee42fd99ee470528858c2ccb3621135c30ec262.
Thanks Claude Paroz, Jon Dufresne, and Tim Graham for the guidance.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28679#comment:10>
Django
-------------------------------------+-------------------------------------
| Freitag
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: urlencode bytes | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"51998a2988e076f811ec27bab5de4c17a6ae6828" 51998a29]:
{{{
#!CommitTicketReference repository=""
revision="51998a2988e076f811ec27bab5de4c17a6ae6828"
[2.0.x] Fixed #28679 -- Fixed urlencode()'s handling of bytes.
Regression in fee42fd99ee470528858c2ccb3621135c30ec262.
Thanks Claude Paroz, Jon Dufresne, and Tim Graham for the guidance.
Backport of 41be85862d9067a809ccf3707d2a22dfef23d99a from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28679#comment:11>