Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#21911 closed Bug (fixed)

Admin login can cause data loss

Reported by: Raymond Penners Owned by: nobody
Component: contrib.admin Version: dev
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 by Russell Keith-Magee, 10 years ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

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

comment:2 by Raymond Penners, 10 years ago

Cc: raymond.penners@… added

comment:3 by Claude Paroz, 10 years ago

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

Has patch: set
Needs documentation: set

comment:5 by Claude Paroz, 10 years ago

Needs documentation: unset
Version: 1.4master

I've updated the PR with release notes.

comment:6 by Tim Graham, 10 years ago

Triage Stage: AcceptedReady for checkin

comment:7 by Claude Paroz, 10 years ago

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

comment:8 by Claude Paroz <claude@…>, 10 years ago

In 5848bea9dc9458a9517d4c98993d742976771342:

Made staff_member_required redirect to login

Refs #21911.

comment:9 by Claude Paroz <claude@…>, 10 years ago

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 by Claude Paroz <claude@…>, 10 years ago

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