[Django] #27363: SessionMiddleware can return redirect(request.path) which might be an unsafe thing to do
Django
unsafe thing to do
----------------------------------+--------------------
Reporter: Raphael Michel | Owner: nobody
Type: Bug | Status: new
Component: contrib.sessions | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27363>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
unsafe thing to do
Reporter: Raphael Michel | Owner: nobody
Type: Bug | Status: new
Component: contrib.sessions | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Nicole Klünder):
* cc: nicole@… (added)
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/27363#comment:1>
Django
unsafe thing to do
Reporter: Raphael Michel | Owner: nobody
Type: Bug | Status: new
Component: contrib.sessions | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/27363#comment:2>
Django
unsafe thing to do
Reporter: Raphael Michel | Owner: Andrew
| Nester
Type: Bug | Status: assigned
Component: contrib.sessions | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Andrew Nester):
* status: new => assigned
* owner: nobody => Andrew Nester
Comment:
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
--
Ticket URL: <https://code.djangoproject.com/ticket/27363#comment:3>
Django
unsafe thing to do
Reporter: Raphael Michel | Owner: Andrew
| Nester
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Raphael Michel):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27363#comment:4>
Django
unsafe thing to do
Reporter: Raphael Michel | Owner: Andrew
| Nester
Component: contrib.sessions | Version: 1.10
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"1ce04bcce0076360623ae164afd3541a5c031af2" 1ce04bc]:
{{{
#!CommitTicketReference repository=""
revision="1ce04bcce0076360623ae164afd3541a5c031af2"
Fixed #27363 -- Replaced unsafe redirect in SessionMiddleware with
SuspiciousOperation.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27363#comment:5>
Django
unsafe thing to do
Reporter: Raphael Michel | Owner: Andrew
| Nester
Type: Bug | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"acacf54fa1371b6254bd32af2cc3ca4fb3b4832c" acacf54f]:
{{{
#!CommitTicketReference repository=""
revision="acacf54fa1371b6254bd32af2cc3ca4fb3b4832c"
[1.10.x] Fixed #27363 -- Replaced unsafe redirect in SessionMiddleware
with SuspiciousOperation.
Backport of 1ce04bcce0076360623ae164afd3541a5c031af2 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27363#comment:6>