Code

Opened 3 months ago

Closed 2 months ago

Last modified 2 months ago

#21911 closed Bug (fixed)

Admin login can cause data loss

Reported by: pennersr Owned by: nobody
Component: contrib.admin Version: master
Severity: Release blocker Keywords:
Cc: raymond.penners@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Prerequisites: plain Django project with admin enabled, no CSRF middleware

To reproduce:

  1. Open up a browser, make sure you are not logged in
  2. Open up two tabs, both visiting: /admin/sites/site/add/
  3. You should be presented with a login form in both tabs.
  4. Login in tab 1).
  5. Switch to tab 2), also login.

Expected result:

  1. You end up at /admin/sites/site/add/ via a GET request

Actual result:

  1. the login as part of login at 2) fires its POST data at the /admin/sites/site/add/ view,

(proven by the fact that you will see validation errors)

Now, while in this case the actual result may seem rather harmless, we have had an incident where somebody unknowingly destroyed precious data by doing this. This may happen if the URL belongs to an update URL, and, the form happens to be considered valid (e.g. if no fields are required you will essentially blank out the model you are updating).

Why doesn't the staff_member_required decorator use redirects to redirect to separate login view, with next=/admin/sites/add ?

Attachments (0)

Change History (10)

comment:1 Changed 3 months ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted

This is concerning; data loss is never an acceptable outcome. Marking release blocker on that basis.

comment:2 Changed 3 months ago by pennersr

  • Cc raymond.penners@… added

comment:3 Changed 3 months ago by claudep

Currently, the staff_member_required decorator allows to post login data to any admin view to log in a user. I am not a fan of this functionality, but changing this would have to be documented as a backwards incompatibility.
This is mainly tested in SecureViewTests.

comment:4 Changed 3 months ago by claudep

  • Has patch set
  • Needs documentation set

comment:5 Changed 2 months ago by claudep

  • Needs documentation unset
  • Version changed from 1.4 to master

I've updated the PR with release notes.

comment:6 Changed 2 months ago by timo

  • Triage Stage changed from Accepted to Ready for checkin

comment:7 Changed 2 months ago by claudep

  • Triage Stage changed from Ready for checkin to Accepted

I've just pushed a third commit to the pull request, as I think that the this_is_the_login_form field from the admin authentication form is now of no use.
May i have a quick review for that last point? (tests do pass with SQLite/Python 2.7)
https://github.com/claudep/django/commit/7b164cd5121608cac146ecc9d432d516a3b220d5

Last edited 2 months ago by claudep (previous) (diff)

comment:8 Changed 2 months ago by Claude Paroz <claude@…>

In 5848bea9dc9458a9517d4c98993d742976771342:

Made staff_member_required redirect to login

Refs #21911.

comment:9 Changed 2 months ago by Claude Paroz <claude@…>

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

In be0ad62994a340ad54a0b328771931932a45a899:

Fixed #21911 -- Made admin views redirect to login when needed

Historically, the Django admin used to pass through the request
from an unauthorized access to the login view directly. Now we
are using a proper redirection, which is also preventing
inadvertantly changing data when POSTing login data to an admin
view when user is already authorized.
Thanks Marc Tamlyn and Tim Graham for the reviews.

comment:10 Changed 2 months ago by Claude Paroz <claude@…>

In 343dfff13370dac679b640196979a33951127290:

Removed the this_is_the_login_form hack

Refs #21911. Now that we have a more traditional login form, we
don't need any more a special field telling us we are dealing with
the login form.

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.