Code

Opened 2 years ago

Closed 5 weeks ago

#18314 closed Bug (fixed)

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

Reported by: anonymous Owned by: unaizalakain
Component: HTTP handling Version: master
Severity: Normal Keywords:
Cc: abhaga 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 yoyoma 2 years ago.
Patch to fix bug, updated to remove basestring.format() usage
18314.diff (5.5 KB) - added by SmileyChris 2 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 2 years ago by claudep

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Bug

comment:2 Changed 2 years ago by yoyoma

  • Owner changed from nobody to yoyoma

comment:3 Changed 2 years ago by yoyoma

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

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

comment:4 Changed 2 years ago by yoyoma

  • Resolution fixed deleted
  • Status changed from closed to reopened

Pardon my resolving of the ticket.

comment:5 Changed 2 years ago by yoyoma

  • Needs tests unset

comment:6 follow-up: Changed 2 years ago by abhaga

  • Cc abhaga 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 2 years ago by abhaga

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 2 years ago by yoyoma

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 2 years ago by yoyoma

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

Changed 2 years ago by SmileyChris

comment:9 follow-up: Changed 2 years ago by SmileyChris

  • Patch needs improvement unset

Here's a simpler way to do it.

comment:10 in reply to: ↑ 9 Changed 2 years ago by yoyoma

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 2 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 16 months ago by aaugustin

  • Status changed from reopened to new

comment:13 Changed 13 months 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 13 months ago by susan

  • Owner changed from yoyoma to susan
  • Status changed from new to assigned

comment:15 Changed 13 months ago by susan

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 9 months ago by timo

  • Easy pickings unset

comment:18 Changed 8 months ago by unaizalakain

  • Owner changed from susan to unaizalakain
  • Patch needs improvement unset
  • Version changed from 1.4 to master

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 8 months ago by unaizalakain (previous) (diff)

comment:19 Changed 5 weeks ago by Tim Graham <timograham@…>

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

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.