Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#21911 closed Bug (fixed)

Admin login can cause data loss

Reported by: Raymond Penners 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 ?

Change History (10)

comment:1 Changed 3 years ago by Russell Keith-Magee

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

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

comment:2 Changed 3 years ago by Raymond Penners

Cc: raymond.penners@… added

comment:3 Changed 3 years ago by Claude Paroz

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 years ago by Claude Paroz

Has patch: set
Needs documentation: set

comment:5 Changed 3 years ago by Claude Paroz

Needs documentation: unset
Version: 1.4master

I've updated the PR with release notes.

comment:6 Changed 3 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:7 Changed 3 years ago by Claude Paroz

Triage Stage: Ready for checkinAccepted

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 3 years ago by Claude Paroz (previous) (diff)

comment:8 Changed 3 years ago by Claude Paroz <claude@…>

In 5848bea9dc9458a9517d4c98993d742976771342:

Made staff_member_required redirect to login

Refs #21911.

comment:9 Changed 3 years ago by Claude Paroz <claude@…>

Resolution: fixed
Status: newclosed

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 3 years 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.

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