Opened 9 years ago

Closed 8 years ago

Last modified 7 years ago

#987 closed defect (fixed)

HttpResponseRedirect uses/allows relative URIs for the HTTP Location header, which is forbidden by HTTP

Reported by: henryk@… Owned by: nobody
Component: Core (Other) Version:
Severity: minor Keywords:
Cc: henryk@…, tbarta@…, tyson@…, dev@…, forest@…, ludvig.ericson@…, sam@…, absoludity@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Moin,

Using the HttpResponseRedirect class (for example as in part 4 of the Django tutorial) may create a Location: header in the HTTP response with a relative URI, which is forbidden by RFC 2616, section 14.30 (an absolute URI is required). Call me a perfectionist, but I think that should be fixed.

The best solution would probably be to automatically create an absolute URI if the URI that is used to create the Redirect is a relative one, so that the apps don't have to deal with it (and stay decoupled from the project). Alternatively update the tutorial and docs with information on how to create an absolute URI.

-- 
Henryk Plötz
Grüße aus Berlin

Attachments (7)

absolute.diff (816 bytes) - added by Simon G. <dev@…> 8 years ago.
tests.py (905 bytes) - added by Simon G. <dev@…> 8 years ago.
absolute2.diff (1.0 KB) - added by Simon G. <dev@…> 8 years ago.
redirect_absoluteuri.patch (2.0 KB) - added by SmileyChris 8 years ago.
redirect_absoluteuri.2.patch (8.1 KB) - added by SmileyChris 8 years ago.
redirect_absoluteuri.3.patch (7.2 KB) - added by SmileyChris 8 years ago.
redirect_absoluteuri.4.patch (7.1 KB) - added by SmileyChris 8 years ago.
a rogue import snuck in there somehow

Download all attachments as: .zip

Change History (35)

comment:1 Changed 9 years ago by ian@…

while relative redirects might not be in the HTTP spec, most browsers work with them.

which hostname do you propose to use for the absolute URI?
django is not really in a position in most cases to know what the right one is, unless your application can figure it out, and in some cases can not be determined when django gets the request.

comment:2 Changed 9 years ago by hugo

The host to use could just be sites.get_current().domain - that's what it's there for, to get the current running site (based on the SITE_ID setting).

comment:3 Changed 9 years ago by adrian

Just one small problem with using sites.get_current().domain -- it assumes the developer has edited the sites table. For something as core as redirects, I'm not sure we would want to assume that, because it could confuse people.

comment:4 Changed 9 years ago by henryk@…

  • Cc henryk@… added

Moin,

one easy way to get developers to edit the site table is to put information and recommendation on it into the tutorial. I for one don't know anything about it yet and didn't see any obvious documentation on how to use it. Another possibility: the URI scheme (http or https) and host name should be deducible from the WSGI environment.

So a good plan is: Leave the site unconfigured in the default configuration and take the values that the browser provided (and maybe even log a warning once in a while). As soon as a site and base URI are correctly configured these settings should override the browser provided values, because the configured values presumably give the canonical URI. That's also basically the way Apache handles the issue.

Related issue: There should be a dead easy and clearly documented way to retrieve the absolute URI of the current site or even app, and a relative URI to the root of the current site/app.

-- 
Henryk Plötz
Grüße aus Berlin

comment:5 Changed 8 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Design decision needed

comment:6 Changed 8 years ago by tbarta@…

  • Cc tbarta@… added

adding self to cc list

comment:7 Changed 8 years ago by tyson@…

  • Cc tyson@… added

I agree with Henryk, there should be a clearly defined method to retrieve the abosulute URI for the the current site/app, using the same logic that the Apache webserver uses to resolve the server name.

HttpResponseRedirect should parse redirect_to and determine if it is absolute or relative. If relative, use os.path.join and os.path.normpath or similar in conjunction with the method specified by Henryk to create the absolute URI.

FYI, Safari is a picky browser that (correctly) barfs on relative URIs.

comment:8 Changed 8 years ago by Simon G. <dev@…>

  • Cc dev@… added
  • Has patch set

I've made a patch which checks if the sites framework has been changed from its default setting, if yes, then HttpResponseRedirect will return an absolute URL. If NOT, then we'll keep the relative redirect.

Changed 8 years ago by Simon G. <dev@…>

Changed 8 years ago by Simon G. <dev@…>

comment:9 Changed 8 years ago by Simon G. <dev@…>

Note that I'm not sure what the best thing to do when there's multiple sites installed - I've never used them.

Changed 8 years ago by Simon G. <dev@…>

comment:10 Changed 8 years ago by Simon G. <dev@…>

Here's a patch that takes into account the SITE_ID setting in the settings.py, but this may be getting too dependant on settings etc.

comment:11 Changed 8 years ago by forest@…

  • Cc forest@… added

Added self to Cc.

Changed 8 years ago by SmileyChris

comment:12 Changed 8 years ago by SmileyChris

Forget about using the optional sites. If we use a piece of response middleware we can get the scheme and host from the request.

Doesn't this make more sense?

comment:13 Changed 8 years ago by mtredinnick

Getting this right is not an optional feature, so I think it should be in the core message handling path, not something you might accidentally remove if you remove the middleware. It's not compulsory to have CommonMiddleware installed.

comment:14 Changed 8 years ago by SmileyChris

Fair enough, but it is still a much better fit than relying on sites. I'm thinking perhaps (and had actually originally implemented before I joined it with ComonMiddleware) we could have a compulsary_middleware property of the BaseHandler which contained some processors that were always enabled and activated before any ones from settings.MIDDLEWARE_CLASSES - I think the streaming upload was going to need a "compulsary" middleware too.

Changed 8 years ago by SmileyChris

comment:15 Changed 8 years ago by SmileyChris

Ok, here's a take on the same thing without using the CommonMiddleware. Comes with tests this time.

This patch does two things, but since the second relies on the first I'm naughtily submitting them together.

  1. New function request.build_absolute_uri(location)
  1. New BaseCommonMiddleware attached to the BaseHandler

comment:16 Changed 8 years ago by SmileyChris

Actually, the patch calls it BaseCompulsaryMiddleware, but I like the name BaseMandatoryMiddleware more now I think about it.

See also: #4986, which will make get_host a bit more functional too.

comment:17 follow-up: Changed 8 years ago by Ludvig Ericson <ludvig.ericson@…>

  • Cc ludvig.ericson@… added

Sorry if I'm being blatantly ignorant, but kind sires, can we not simply try to find the HTTP Host header, which is in fact mandatory in HTTP 1.1, and most HTTP 1.0 clients send it anyway? If it doesn't exist, then the client leaves us no choice but to violate the HTTP RFCs. Not entirely sure how this would be done with Django, but the concept is there.

On a slightly related note, I don't get why you need absolute URLs, what bothers me most is the fact that you have to specify the protocol (scheme if you will) - makes it even harder as we have to specify HTTP vs. HTTPS.

+add self to Cc

comment:18 in reply to: ↑ 17 Changed 8 years ago by SmileyChris

Replying to Ludvig Ericson <ludvig.ericson@gmail.com>:

Sorry if I'm being blatantly ignorant, but kind sires, can we not simply try to find the HTTP Host header, which is in fact mandatory in HTTP 1.1, and most HTTP 1.0 clients send it anyway?

Did you read my patch? That's pretty much what it does.

If it doesn't exist, then the client leaves us no choice but to violate the HTTP RFCs. Not entirely sure how this would be done with Django, but the concept is there.

Regarding getting it, get_host is how it is done in Django. And #4986 will make it a bit smarter still to fall back if the HTTP Host isn't provided.

On a slightly related note, I don't get why you need absolute URLs, what bothers me most is the fact that you have to specify the protocol (scheme if you will) - makes it even harder as we have to specify HTTP vs. HTTPS.

We're just trying to follow the HTTP spec, and this is the wrong place to contest that ;). If you check out my patch, you'll see that it's all handled pretty well (and automatically).

comment:19 Changed 8 years ago by Thomas Güttler <hv@…>

  • Cc hv@… added

Please fix this. Redirects must be absolute.

I get very strange results with Apache and SCGI:
Redirects after POST: The URL in the browser (firefox 1.5.04) does
not change, but the content of the post result gets displayed.

(See also http://barryp.org/blog/feeds/atom/scgi/)

comment:20 follow-up: Changed 8 years ago by ubernostrum

Chris is there a reason why you wrote this as a middleware instead of just putting the functionality directly into Django's HTTP handler? It's kind of confusing to have a middleware which nonetheless forces itself to be always enabled...

comment:21 Changed 8 years ago by Thomas Güttler <hv@…>

  • Triage Stage changed from Design decision needed to Ready for checkin

The patch redirect_absoluteuri.patch works for me. The SCGI redirect problems are gone.
I looked at the source and I think it could be commited.

redirect_absoluteuri.2.patch builds an own middleware. I think that is not necessary.
Absolute redirects should be enabled by default.

The patches from Simon G. don't work, since they only work for http, but not for https.

comment:22 Changed 8 years ago by anonymous

  • Cc sam@… added

comment:23 in reply to: ↑ 20 Changed 8 years ago by SmileyChris

Replying to ubernostrum:

Chris is there a reason why you wrote this as a middleware instead of just putting the functionality directly into Django's HTTP handler? It's kind of confusing to have a middleware which nonetheless forces itself to be always enabled...

  1. Logic abstraction
  1. Ability to easily extend/override in a HTTP Handler subclass (probably YAGNI, I know)

It's only confusing if you're looking at source - I wouldn't bother putting it in documentation.

comment:24 Changed 8 years ago by SmileyChris

Thomas, the redirect shouldn't be optional. So the problem with just attaching it to the common middleware is that it can be disabled.

2.patch attaches the new middleware directly to Django's base HTTP handler - you don't manually attach it in your settings.

It also adds the cool new function request.build_absolute_uri(location) ;)

Changed 8 years ago by SmileyChris

Changed 8 years ago by SmileyChris

a rogue import snuck in there somehow

comment:25 Changed 8 years ago by mtredinnick

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

(In [6164]) Fixed #987 -- Convert relative URI portions into absolute URIs in HTTP Location headers. Based on a patch from SmileyChris.

comment:26 Changed 8 years ago by absoludity

  • Cc absoludity@… added

Am I right in saying that since this change, all unit tests using something along the lines of:

self.assertRedirects(response, '/accounts/login/')

will currently need to include the actual site's url (or a settings variable) like:

self.assertRedirects(response, settings.HOME_URL + '/accounts/login/') (certainly seems to be the case for me!)?

If so, I'm guessing assertRedirects needs to be updated so that it only checks the path of the responseLocation? (although this won't always be true either, for eg where people are using middleware subdomain mapping to do things like http://me.example.com/inbox/ -> http://example.com/u/me/inbox/ etc.).

Or is it better to leave as is so that the unit-tests are coupled to site? Thoughts?

Thanks for any feedback!
-Michael

comment:27 Changed 8 years ago by absoludity

Sorry, my mistake... I'd forgotten that I'm explicitly setting the HTTP_HOST header in my base test-case class so that I can do the middleware subdomain mapping mentioned above.

-Michael

comment:28 Changed 7 years ago by guettli

  • Cc hv@… removed
Note: See TracTickets for help on using tickets.
Back to Top