Opened 11 years ago

Closed 11 years ago

#19992 closed New feature (duplicate)

Put protection against unsafe redirects into `HttpResponseRedirectBase`

Reported by: Ram Rachum Owned by: nobody
Component: HTTP handling Version: dev
Severity: Normal Keywords: security
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Here's something I have in my app that I think other users in Django might find beneficial.

In many circumstances you want to redirect a user inside your site to a dynamic URL. You usually have protection in that case against redirecting out of your site, like in here:

https://github.com/django/django/blob/d9330d5be2ee60b208dcab2616eb164ea2e6bf36/django/contrib/auth/decorators.py#L30-L36

I think it's annoying to have that protection in various places in your code instead of having it directly in HttpResponseRedirectBase. I suggest that such protection will be automatically enabled in HttpResponseRedirectBase, and when you want to be able to redirect to an external site, you'll have to do some extra action to make it clear that you know the risks. (For backwards compatibility with existing apps, we can make this behavior off by default, and to allow enabling it on a per-app basis.)

What do you think?

Change History (4)

comment:1 by Jacob, 11 years ago

Keywords: security added
Triage Stage: UnreviewedAccepted

I think this is a good idea.

However, we do need to think a bit about whether it should be enabled by default. If we do, it'll break existing code, but if we don't then we're violating "safe by default." My preference would be something like redirect(..., safe=False) with safe defaulting to True, but that may be too backwards-incompatible.

comment:2 by Ram Rachum, 11 years ago

I agree with the idea of a safe flag to functions that do a redirect. (Which I guess is redirect and the constructor for the redirect response, possibly several more?)

I would think though whether it's correct to call it safe, because it might just mean "local", and calling it safe might give an illusion of safety. But I don't feel strongly about the name.

Now, the thing is, since we'll have safe=True by default, existing apps will break. So I think that this functionality needs to be turned on and off on an app-by-app basis. I suggest it being off by default, but that you could turn it on for each app individually, so you could turn it on for your apps without breaking the third-party apps that you're using.

What do you think?

comment:3 by anonymous, 11 years ago

Nudge.

comment:4 by Aymeric Augustin, 11 years ago

Resolution: duplicate
Status: newclosed

This appears to be a duplicate of #13751.

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