Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix unnecessary escaping of path chars in reverse() (#22223) #2408

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants

The path segments concatenated by reverse() only need to get characters
quoted that are reserved as per section 3.3 of RFC-3986.

Unreserved characters that Django currently quotes as part of the work done
on #13260 are: ":@&=+$,"

Quoting unreserved characters leads to issues in situations where client and
server require unambigious URL representation, like creating a signature base
string in OAuth 1. Here the client produces a digest of a URL with quoted
chars, while the server ends up with a digest of the unquoted chars, leading
to different signatures (see
https://groups.google.com/forum/#!msg/django-developers/ZLGk7T4mJuw/4RqfgbZ-jOQJ).

To address this change still applies urlquote, but excludes the unreserved
path characters.

@erikvanzijst erikvanzijst Fix unnecessary escaping of path chars in reverse().
The path segments concatenated by reverse() only need to get characters
quoted that are reserved as per section 3.3 of RFC-3986.

Unreserved characters that Django currently quotes as part of the work done
on #13260 are: ":@&=+$,"

Quoting unreserved characters leads to issues in situations where client and
server require unambigious URL representation, like creating a signature base
string in OAuth 1. Here the client produces a digest of a URL with quoted
chars, while the server ends up with a digest of the unquoted chars, leading
to different signatures (see
https://groups.google.com/forum/#!msg/django-developers/ZLGk7T4mJuw/4RqfgbZ-jOQJ).

To address this change still applies urlquote, but excludes the unreserved
path characters.
a2f1705

Hey guys (@apollo13?), can I get some eyes on this pull request? It addresses https://code.djangoproject.com/ticket/22223

@jorgecarleitao jorgecarleitao commented on the diff Jun 10, 2014

django/core/urlresolvers.py
@@ -418,7 +418,8 @@ def _reverse_with_prefix(self, lookup_view, _prefix, *args, **kwargs):
# arguments in order to return a properly encoded URL.
candidate_pat = prefix_norm.replace('%', '%%') + result
if re.search('^%s%s' % (prefix_norm, pattern), candidate_pat % candidate_subs, re.UNICODE):
- candidate_subs = dict((k, urlquote(v)) for (k, v) in candidate_subs.items())
+ candidate_subs = dict((k, urlquote(v, safe='/:@&=+$,')) for
@jorgecarleitao

jorgecarleitao Jun 10, 2014

Contributor

Since the ticket trac had some discussion about RFCs, putting '/:@&=+$,' as a constant up in the top, with a comment to the RFC that specifies it could help. Other possibility is to make the constant name as explicit as possible, e.g. RFC_2396_COMPLIANT = '/:@&=+$,'.

@jorgecarleitao

jorgecarleitao Jun 10, 2014

Contributor

Also, see ticket trac, it seems 2396 is obsolete.

Owner

timgraham commented Jun 30, 2014

Superseded by #2859. Would be great if you could review it, thanks!

@timgraham timgraham closed this Jun 30, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment