Opened 14 years ago

Closed 13 years ago

Last modified 12 years ago

#12534 closed (fixed)

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

Reported by: Jeremy Lainé 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: no UI/UX: no

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 Jannis Vajen 13 years ago.
Removes check for space in redirect_to
12534_with_test.diff (1.8 KB ) - added by Aymeric Augustin 13 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 by anonymous, 14 years ago

Cc: aymeric.augustin@… removed

comment:2 by Luke Plant, 14 years ago

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 by Russell Keith-Magee, 14 years ago

Resolution: duplicate
Status: newclosed

Duplicate of #11457.

comment:4 by Aymeric Augustin, 14 years ago

Resolution: duplicate
Status: closedreopened

#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 by Russell Keith-Magee, 14 years ago

Triage Stage: UnreviewedAccepted

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

comment:6 by anonymous, 13 years ago

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 by Jannis Leidel, 13 years ago

milestone: 1.3

comment:8 by John Anderson, 13 years ago

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.

by Jannis Vajen, 13 years ago

Attachment: 12534.patch added

Removes check for space in redirect_to

comment:9 by Jannis Vajen, 13 years ago

Has patch: set

comment:10 by Aymeric Augustin, 13 years ago

Triage Stage: AcceptedReady 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 by Aymeric Augustin, 13 years ago

Cc: aymeric.augustin@… added

comment:12 by Ramiro Morales, 13 years ago

Needs tests: set
Triage Stage: Ready for checkinAccepted

by Aymeric Augustin, 13 years ago

Attachment: 12534_with_test.diff added

comment:13 by Aymeric Augustin, 13 years ago

Needs tests: unset

Added test.

comment:14 by Jannis Leidel, 13 years ago

Resolution: fixed
Status: reopenedclosed

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 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

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