Opened 4 years ago

Closed 2 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: master
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 4 years ago.
Patch to fix bug, updated to remove basestring.format() usage
18314.diff (5.5 KB) - added by Chris Beaven 4 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 4 years ago by Claude Paroz

Needs documentation: unset
Needs tests: set
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

comment:2 Changed 4 years ago by Michael Angeletti

Owner: changed from nobody to Michael Angeletti

comment:3 Changed 4 years ago by Michael Angeletti

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 Changed 4 years ago by Michael Angeletti

Resolution: fixed
Status: closedreopened

Pardon my resolving of the ticket.

comment:5 Changed 4 years ago by Michael Angeletti

Needs tests: unset

comment:6 Changed 4 years ago by Abhaya Agarwal

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 Changed 4 years ago by Abhaya Agarwal

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.

comment:8 in reply to:  6 Changed 4 years ago by Michael Angeletti

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.

Changed 4 years ago by Michael Angeletti

Attachment: TICKET-18314.diff added

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

Changed 4 years ago by Chris Beaven

Attachment: 18314.diff added

comment:9 Changed 4 years ago by Chris Beaven

Patch needs improvement: unset

Here's a simpler way to do it.

comment:10 in reply to:  9 Changed 4 years ago by Michael Angeletti

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 Changed 4 years ago by anonymous

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 Changed 4 years ago by Aymeric Augustin

Status: reopenednew

comment:13 Changed 3 years ago by Kamu

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 Changed 3 years ago by Susan Tan

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

comment:15 Changed 3 years ago by Susan Tan

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 Changed 3 years ago by Tim Graham

Easy pickings: unset

comment:18 Changed 3 years ago by Unai Zalakain

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.

Version 0, edited 3 years ago by Unai Zalakain (next)

comment:19 Changed 2 years ago by Tim Graham <timograham@…>

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