Opened 8 years ago

Closed 7 years ago

Last modified 4 years ago

#6621 closed Uncategorized (fixed)

build_absolute_uri in HttpRequest breaks with colon in URL path

Reported by: Greg Thornton <xdissent@…> Owned by: nobody
Component: HTTP handling Version: master
Severity: Normal Keywords: http url colon
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

It appears that HttpRequest from django.http has a bug that allows it to return a relative URL when the path component of the current URL (or the location argument) contains a colon. The show_url view below demonstrates the issue:

from django.http import HttpResponse
from urlparse import urljoin
import re

http_re = re.compile('^https?:')

def get_absolute_uri(request):
    location = request.get_full_path()
    if not http_re.match(location): 
        current_uri = '%s://%s%s' % (request.is_secure() and 'https' or 'http', request.get_host(), request.path)
        location = urljoin(current_uri, location)
    return location

def show_url(request):
    response = """
        Absolute URI: %s<br />
        Expected: %s<br />
    """ % (request.build_absolute_uri(), get_absolute_uri(request))
    return HttpResponse(response)

Output correct without colon (requested http://xdissent.com/show_url/testing123):

Absolute URI: http://xdissent.com/show_url/testing123
Expected: http://xdissent.com/show_url/testing123

but broken when a colon is found in the URL path (requested http://xdissent.com/show_url/testing:123):

Absolute URI: /show_url/testing:123
Expected: http://xdissent.com/show_url/testing:123

My function get_absolute_url is essentially copied from HttpRequest's build_absolute_uri, but uses a small regex to determine whether or not a path is relative whereas build_absolute_uri simply checks for the presence of a colon. Upon first glance, it appears that this is correct behaviour according to the URL spec because really any colons in the path portion of a URL should be URL encoded as %3A so build_absolute_uri wouldn't find a colon when used properly. However, Django automatically decodes encoded URL characters so even when using %3A for colons, build_absolute_uri still thinks the path is absolute when it is not. For example:

Still broken even when using encoded colons in the path (requested http://xdissent.com/show_url/testing%3A123):

Absolute URI: /show_url/testing:123
Expected: http://xdissent.com/show_url/testing:123

Attachments (2)

uri_fix.diff (899 bytes) - added by Greg Thornton <xdissent@…> 8 years ago.
patch for build_absolute_uri in HttpRequest to handle colons in URL paths
6621.diff (907 bytes) - added by programmerq 7 years ago.
Now applies cleanly to trunk

Download all attachments as: .zip

Change History (7)

Changed 8 years ago by Greg Thornton <xdissent@…>

patch for build_absolute_uri in HttpRequest to handle colons in URL paths

comment:1 Changed 8 years ago by guettli

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

According to the documentation get_full_path() does not contain the protocol (see below).

That's way I think the check for ':' (as regex or not) in build_absolute_uri is wrong.

The url http://foo/view_url/http%3A... is valid, too.

But this means, the feature, that location is an absolute url must be removed.
I think this is good, since build_abslute_uri() does nothing, if it guesses that
location is absolute.

Code should refuse to guess.

Using a regex guesses better, but only in 99.999%, since location='http://foo' can be
relative, too: http://foo/view_url/http%3A...

Documentation:

get_full_path():
Returns the path, plus an appended query string, if applicable.
Example: "/music/bands/the_beatles/?print=true"
build_absolute_uri(location):
...
If the location is already an absolute URI, it will not be altered.
Otherwise the absolute URI is built using the server variables available in this request.

comment:2 Changed 8 years ago by Andrew McNabb <amcnabb@…>

I've run into the same bug. I think this patch looks good, and I would love to see it merged.

Thanks.

comment:3 Changed 7 years ago by anonymous

  • Cc hv@… added

Changed 7 years ago by programmerq

Now applies cleanly to trunk

comment:4 Changed 7 years ago by programmerq

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

This was fixed in r8490

comment:5 Changed 4 years ago by guettli

  • Cc hv@… removed
  • Easy pickings unset
  • Severity set to Normal
  • Type set to Uncategorized
  • UI/UX unset
Note: See TracTickets for help on using tickets.
Back to Top