Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#27363 closed Bug (fixed)

SessionMiddleware can return redirect(request.path) which might be an unsafe thing to do

Reported by: Raphael Michel Owned by: Andrew Nester
Component: contrib.sessions Version: 1.10
Severity: Normal Keywords:
Cc: nicole@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hello everyone,

a friend of mine just spent hours to debug why a view randomly responded to a POST request with a redirect even though no redirects appeared in
the code at all. While the root cause was a caching issue in the session backend, the redirect was caused by the redirect call in SessionMiddleware that has been introduced in https://github.com/django/django/commit/3389c5ea229884a1943873fe7e7ffc2800cefc22#diff-7e0a8bc945a2cf57bf4e4b4c9804c340R58 in an attempt to fix the issue reported in #21608.

I believe this is a regression in 1.10 as it causes highly unexpected behaviour by issuing a redirect to the same view, a view that might have been called via POST and not even support handling GET requests.

Also, the patch is only appropriate under the assumption that sessions are only used for authentication. The code comment says "The user is now logged out; redirecting to same page will result in a redirect to the login page if required". This is problematic whenever sessions are used for something other than authentication and a subsequent GET request might do something completely different than redirecting to the login page.

The underlying problem that causes #21608 is to be seen as a race condition between two requests, where one request deletes the session and then another request tries to save it. I believe, if this race condition occurs, we either need to fail silently (just don't save) or fail loudly (raise e.g. an error somewhere between 400 and 500, like we raise a 403 if an old CSRF token is used after a logout which is a different but similar race).

I'm personally in favour of failing loudly, as the view might expect the changed session to be saved - which it then isn't.

Change History (6)

comment:1 by Nicole Klünder, 8 years ago

Cc: nicole@… added

comment:2 by Tim Graham, 8 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Andrew Nester, 8 years ago

Owner: changed from nobody to Andrew Nester
Status: newassigned

Agree that it's much better to return 400 HTTP status in case of deleted session.
Proper pull request has been added to implement this

Version 0, edited 8 years ago by Andrew Nester (next)

comment:4 by Raphael Michel, 8 years ago

Has patch: set

comment:5 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: assignedclosed

In 1ce04bc:

Fixed #27363 -- Replaced unsafe redirect in SessionMiddleware with SuspiciousOperation.

comment:6 by Tim Graham <timograham@…>, 8 years ago

In acacf54f:

[1.10.x] Fixed #27363 -- Replaced unsafe redirect in SessionMiddleware with SuspiciousOperation.

Backport of 1ce04bcce0076360623ae164afd3541a5c031af2 from master

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