Opened 12 years ago

Closed 10 years ago

#18314 closed Bug (fixed)

request.build_absolute_uri() functions incorrectly when the path starts with //

Reported by: anonymous Owned by: Unai Zalakain
Component: HTTP handling Version: dev
Severity: Normal Keywords:
Cc: Abhaya Agarwal Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

A request to:

http://www.example.com:8080//django-ate-my-baby/

leads to request.build_absolute_uri() returning:

http://django-ate-my-baby/

Attachments (2)

TICKET-18314.diff (5.5 KB ) - added by Michael Angeletti 12 years ago.
Patch to fix bug, updated to remove basestring.format() usage
18314.diff (5.5 KB ) - added by Chris Beaven 12 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 by Claude Paroz, 12 years ago

Needs tests: set
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

comment:2 by Michael Angeletti, 12 years ago

Owner: changed from nobody to Michael Angeletti

comment:3 by Michael Angeletti, 12 years ago

Has patch: set
Resolution: fixed
Status: newclosed

I've attached the fix, but if it needs improvements, let me know and I'll update the diff ASAP.

comment:4 by Michael Angeletti, 12 years ago

Resolution: fixed
Status: closedreopened

Pardon my resolving of the ticket.

comment:5 by Michael Angeletti, 12 years ago

Needs tests: unset

comment:6 by Abhaya Agarwal, 12 years ago

Cc: Abhaya Agarwal added
Patch needs improvement: set

Since the original function as well as your patch is using self.path to construct the base_uri, query parameters are not preserved anyway. So branching doesn't serve any purpose.
One simpler way is to strip out the additional slashes in the beginning of self.path in this function. It may be better to do that where the request is initialized.

comment:7 by Abhaya Agarwal, 12 years ago

Also str.format was introduced in Python 2.6 while Django 1.4 supports Python 2.5 also. So that should be reverted back to old style string interpolation.

in reply to:  6 comment:8 by Michael Angeletti, 12 years ago

Replying to abhaga:

Since the original function as well as your patch is using self.path to construct the base_uri, query parameters are not preserved anyway. So branching doesn't serve any purpose.
One simpler way is to strip out the additional slashes in the beginning of self.path in this function. It may be better to do that where the request is initialized.

That is incorrect. location, if provided or not is joined with the base_uri, and when not provided is built using request.get_full_path(). This happens in both my patch and the original.

>>> from urlparse import urljoin 
>>> urljoin('http://www.example.com/foo/', 'http://www.example.com/foo/?bar=baz')
'http://www.example.com/foo/?bar=baz'

Regarding the usage of basestring.format(), I'll provide an updated patch which uses string interpolation.

by Michael Angeletti, 12 years ago

Attachment: TICKET-18314.diff added

Patch to fix bug, updated to remove basestring.format() usage

by Chris Beaven, 12 years ago

Attachment: 18314.diff added

comment:9 by Chris Beaven, 12 years ago

Patch needs improvement: unset

Here's a simpler way to do it.

in reply to:  9 comment:10 by Michael Angeletti, 12 years ago

Replying to SmileyChris:

Here's a simpler way to do it.

I don't mean to sound too competitive, but I think my patch is as simple as possible, but no simpler :P It only has 2 very simple logical branches, and requires no hacking. I think prepending '//' to self.get_full_path() is a bit hacky, as it simply takes advantage urlsplit setting netloc to '' when a URL starts with // (explicit over implicit comes to mind here).

Also worth noting is:

        if not location: 

Not using is not None here could lead to an unintended result, if ever a bug were to cause this method to be called with an empty string provided as the location argument.

comment:11 by anonymous, 12 years ago

Any word on which patch will go to master? Both include passing tests for regression, etc., and both provide the exact same result.

comment:12 by Aymeric Augustin, 11 years ago

Status: reopenednew

comment:13 by Kamu, 11 years ago

Patch needs improvement: set

If I am not mistaken the patch requires updating. The test files being patched no longer exist at that location.

comment:14 by Susan Tan, 11 years ago

Owner: changed from Michael Angeletti to Susan Tan
Status: newassigned

comment:15 by Susan Tan, 11 years ago

The 1st test fails.But the other 3 pass. Can someone also test this patch? Hmm. Well, I'm going to have to fix up the patch.

comment:17 by Tim Graham, 10 years ago

Easy pickings: unset

comment:18 by Unai Zalakain, 10 years ago

Owner: changed from Susan Tan to Unai Zalakain
Patch needs improvement: unset
Version: 1.4master

Enhanced docs, all test passing plus correct handling of leading slashes by WSGIRequest.
Ups, I forgot to link to the PR: https://github.com/django/django/pull/1866

Last edited 10 years ago by Unai Zalakain (previous) (diff)

comment:19 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In 11284a63d48c84f1d60b5686d55cf8a9f8d64422:

Fixed #18314 -- Corrected request.build_absolute_uri() handling of paths starting with

HttpRequest.build_absolute_uri() now correctly handles paths starting with .
WSGIRequest now doesn't remove all the leading slashes either,
because http://test/server and http://test//server aren't the same thing
(RFC2396).

Thanks to SmileyChris for the initial patch.

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