Opened 3 years ago

Closed 3 years ago

#19992 closed New feature (duplicate)

Put protection against unsafe redirects into `HttpResponseRedirectBase`

Reported by: coolRR Owned by: nobody
Component: HTTP handling Version: master
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


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:

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

  • Keywords security added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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

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


comment:4 Changed 3 years ago by aaugustin

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

This appears to be a duplicate of #13751.

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