Code

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#12534 closed (fixed)

django.contrib.auth.views.login refuses to redirect to urls with spaces

Reported by: sharky Owned by: nobody
Component: contrib.auth Version: 1.1
Severity: Keywords:
Cc: aymeric.augustin@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

While logged out, I am trying to access a page which is protected by the "login_required" decorator at:

http://example.com/foo%20bar/

I get redirected to:

http://example.com/accounts/login?next=/foo%20bar/

Once I enter my credentials, instead of getting redirected to the expected page, I get sent to the default URL as defined by settings.LOGIN_REDIRECT_URL.

This bug is due to the code at line 24 of django/contrib/auth/views.py:

# Light security check -- make sure redirect_to isn't garbage.
if not redirect_to or '' in redirect_to or ' ' in redirect_to:

redirect_to = settings.LOGIN_REDIRECT_URL

Could someone please explain how checking for spaces or double slashes is a "security check"? From my point of view, it's a bug, django refuses to redirect me to an URL which is perfectly valid!

Many thanks in advance!

Attachments (2)

12534.patch (915 bytes) - added by jnns 3 years ago.
Removes check for space in redirect_to
12534_with_test.diff (1.8 KB) - added by aaugustin 3 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 4 years ago by anonymous

  • Cc aymeric.augustin@… removed
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 4 years ago by lukeplant

I'm guessing the double slash check is to stop redirecting to an external site, which would be a phishing vulnerability. I don't know about the space thing, it does seem like it should be allowed to redirect to a URL with a space in it, as you are allowed to have spaces in the path element of the URL.

If you allow a space, it seems to work fine (tested in Firefox and Opera). Some browsers, such as Firefox, actually display a space in the address bar, rather than %20.

The only concern I have is with the redirect. Django sets "Location" to "/foo bar/" if you allow the space. With the development server, this somehow gets translated to "Location: /foo%20bar/" in the header that is sent to the browser, I haven't tested with modpython.

comment:3 Changed 4 years ago by russellm

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

Duplicate of #11457.

comment:4 Changed 4 years ago by aaugustin

  • Resolution duplicate deleted
  • Status changed from closed to reopened

#11457 handles URLs containing , but it does not address the main issue of this ticket, which is URLs containing spaces.

The comment explaining the restriction on spaces is unclear:

# Light security check -- make sure redirect_to isn't garbage.

and no better reason was found, see:
http://code.djangoproject.com/ticket/11457#comment:3
http://code.djangoproject.com/ticket/11457#comment:4

Could the restriction be either explained or removed?

To remove it, line 34 of django/contrib/auth/views.py must be changed from:

    if not redirect_to or ' ' in redirect_to:

to

    if not redirect_to:

comment:5 Changed 4 years ago by russellm

  • Triage Stage changed from Unreviewed to Accepted

Ok - #11457 *was* a duplicate; but it got committed without addressing this issue.

comment:6 Changed 3 years ago by anonymous

The url below (where the redirect_to field has a query string embedded) has a space in one of the query string parameters. This breaks the login view since it rejects the redirect_to.

http://127.0.0.1:8000/login/?next=/%3Fanswer%3Dtest%20answer%26puzzle_id%3D461

comment:7 Changed 3 years ago by jezdez

  • milestone set to 1.3

comment:8 Changed 3 years ago by sontek

I looked in the history and it has been that way as long as Django has been public, I don't think there is any true security reason for having that check.

Changed 3 years ago by jnns

Removes check for space in redirect_to

comment:9 Changed 3 years ago by jnns

  • Has patch set

comment:10 Changed 3 years ago by aaugustin

  • Triage Stage changed from Accepted to Ready for checkin

RFC 2616 (HTTP) says:

For definitive information on URL syntax and semantics,
see "Uniform Resource Identifiers (URI): Generic Syntax and Semantics," RFC 2396

RFC 2396 allows using the SPACE character in URLs, encoded as %20 (it is already decoded when the view is called). It even uses it as an example in §2.4.1 :

For example, "%20" is the escaped encoding for the US-ASCII space character."

So there is no reason to forbid the SPACE character.

Regarding lukeplant's comment above, HttpResponseRedirect and friends re-encode the URL in the Location header like this:

self['Location'] = iri_to_uri(redirect_to)

So we are safe.

The patch applies cleanly (with -p1 - it's a git patch) and does not break any tests. Looks good.

comment:11 Changed 3 years ago by aaugustin

  • Cc aymeric.augustin@… added

comment:12 Changed 3 years ago by ramiro

  • Needs tests set
  • Triage Stage changed from Ready for checkin to Accepted

Changed 3 years ago by aaugustin

comment:13 Changed 3 years ago by aaugustin

  • Needs tests unset

Added test.

comment:14 Changed 3 years ago by jezdez

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

In [15702]:

Fixed #12534 -- Loosened the the security check for "next" redirects after logins slightly to allow paths that contain spaces. Thanks for the patch, jnns and aaugustin.

comment:15 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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.