reverse() escapes unreserved characters
|Reported by:||erik.van.zijst@…||Owned by:||nobody|
|Cc:||denilsonsa@…, apollo13, jorgecarleitao@…||Triage Stage:||Ready for checkin|
|Has patch:||yes||Needs documentation:||no|
|Needs tests:||no||Patch needs improvement:||no|
Django's django.core.urlresolvers.reverse() seems to have changed its behavior in 1.6. It now runs the arguments through urlquote(), without specifying the safe characters for path components. As a result:
In : reverse('test', args=['foo:bar']) Out: '/foo:bar'
but on 1.6.2:
In : reverse('test', args=['foo:bar']) Out: '/foo%3Abar'
It would seem to me that this is a regression, as ":@-._~!$&'()*+,;=" are all allowed unescaped in path segments AFAIK.
I'm bringing this up because it breaks certain OAuth 1 clients against Bitbucket.
In some places we redirect to URLs whose path segment contains a ":". Prior to us upgrading to 1.6 the response's location header preserved that colon, but now it gets escaped, changing the URL (e.g. https://api.bitbucket.org/2.0/repositories/david/django-storages/pullrequests/51/diff redirecting to https://api.bitbucket.org/2.0/repositories/david/django-storages/diff/regadas/django-storages%3A069fd1d01fbf..f153a70ba254)
In OAuth 1, requests are signed, including the request URL, but the RFC-5849 does not mandate any pre-processing of the URL. For several OAuth clients (including requests-oauthlib and python-oauth2) that means they compute the signature over a string that contains "%3A" instead of ":".
On the server however, the request path automatically gets unquoted before it hits the middlewares and views. As our OAuth layer is a middleware that reconstructs the signature, it ends up computing over ":", yielding a different signature than the client, breaking authentication.
This might be addressable by changing these OAuth clients to perform unquoting on the path segment, but a better solution would seem to make urlresolvers.py:RegexURLResolver respect the reserved characters for path segments and not escape what does not need to be escaped.
I'll follow up with a pull request, unless there are strong feelings, or unwanted consequences of that approach.
Change History (13)
comment:1 Changed 13 months ago by erik.van.zijst@…
- Needs documentation unset
- Needs tests unset
- Patch needs improvement unset
comment:4 Changed 13 months ago by claudep
- Has patch set
- Patch needs improvement set
- Triage Stage changed from Unreviewed to Accepted
comment:10 Changed 10 months ago by jorgecarleitao
- Cc jorgecarleitao@… added