Code

Opened 6 years ago

Closed 19 months ago

#7989 closed Uncategorized (duplicate)

Logout view should require POST request

Reported by: jcassee Owned by:
Component: contrib.auth Version: master
Severity: Normal Keywords: authentication
Cc: joost@… Triage Stage: Design decision needed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This ticket assumes that the mantra "GET and HEAD methods SHOULD NOT have the significance of taking an action other than retrieval" is to be taken seriously and applied consistently. See for example ticket #3651.

The documentation on authentication suggests a logout view that takes a GET request ('How to log a user out'). Additionally, the django.contrib.auth.views.logout generic view accepts a GET request. This seems to go against the above principle, as it changes the internal state of the application.

Please consider a change for logout similar to the patch from ticket #3651. I am willing to do it myself if this ticket is accepted.

Attachments (0)

Change History (11)

comment:1 Changed 6 years ago by julianb

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

You may be right, but does it really change the state of the application if some session information gets deleted?

comment:2 Changed 6 years ago by jcassee

Maybe we are on a sliding scale, but it is undeniably true that the GET request of the logout view will (in most applications) affect the content retrieved by subsequent GET requests.

comment:3 Changed 6 years ago by frasern

I definitely think that this would be the right thing to do as the logout action does indeed change the state.

In the event of a GET request, the fix implemented for #3651 does not change the user's preferred language, effectively leaving the system state unchanged. For obvious reasons, I think it would be dangerous to take no action on a GET request to logout. We therefore need to decide what should happen if the logout view is request using GET.

A possible solution would be to take the user to a temporary page that immediately performs a POST using Javascript; for example,

<h1>Confirm logout</h1>
<p>Please confirm you wish to logout of this site.</p>
<form action="/account/logout/" method="post" id="logout_form">
  <input type="submit" value="Logout now" />
</form>
<script type="text/javascript">
  document.getElementById('logout_form').submit();
</script>

Django would need to provide a default implementation of this template, but this should be customisable.

This approach would probably stop any issues with link-prefetchers and make the logout action "safe", but it doesn't feel right to redirect from a GET to a POST.

comment:4 Changed 6 years ago by jcassee

I like the idea by frasern, except that I would not add the javascript that automatically submits the form. I think I will write a patch that implements this idea and see what we end up with.

comment:5 Changed 6 years ago by russellm

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

From the HTTP 1.1 spec:

9.1.1 Safe Methods

Implementors should be aware that the software represents the user in their interactions over the Internet, and should be careful to allow the user to be aware of any actions they might take which may have an unexpected significance to themselves or others.

In particular, the convention has been established that the GET and HEAD methods SHOULD NOT have the significance of taking an action other than retrieval. These methods ought to be considered "safe".

Then, RFC 2119 defines SHOULD NOT:

SHOULD NOT This phrase, or the phrase "NOT RECOMMENDED" mean that

there may exist valid reasons in particular circumstances when the
particular behavior is acceptable or even useful, but the full
implications should be understood and the case carefully weighed
before implementing any behavior described with this label.

In this case, I'm happy to invoke the 'having weighed the full implications' cause. logout() is idempotent, and in practice that is all that is required in this case. I'm not convinced it's a problem that warrants the nasty javascript hack (or the requirement to move to a form for logouts) that is required to fix it.

comment:6 Changed 5 years ago by SamBull

  • Resolution wontfix deleted
  • Status changed from closed to reopened

While I agree that the JavaScript hack is ugly, I disagree with the wontfix ruling here. I think allowing GET-based logout on large social sites is problematic. It's trivial to wrap the logout view in another view that only allows POST, but such a view often has no sensible home in a django project.

I don't see a good reason why the best practice isn't followed here. Requiring POST for these things is a potential nuisance, but it's the right thing to do. Requiring POST for language changes can be a nuisance as well. In the past I've been able to create GET-like behaviour for language selection by replacing the POST form with a link that triggers a hidden form submit, using jquery. It provides a nicer user experience when js is enabled but it gracefully degrades to a "logout" submit button otherwise. I'd be happy to provide a code sample here for how this could be applied to logout.

I think backwards incompatibility concerns can be addressed with either an additional, optional parameter to the logout view or with an additional setting, called either "require_post" or "REQUIRE_POST_FOR_LOGOUT", respectively. The value would default to True. Developers would be free to change this to False so their GET-based logouts would still work.

I apologize for reopening this ticket, but I feel strongly that state changing behaviour shouldn't be attached to GET requests, and that things get cruddier when that's allowed. If there's any interest in changing this behaviour, now that we are post-1.0, I would be happy to write a patch based on whichever method is preferred (no backwards compatibility, adding a param to logout, or adding a setting to settings)

comment:7 Changed 5 years ago by Pyth

-1. I find it unlikely that most developers are concerned about logout attacks for the nature of their application. For the majority, this will add clutter and complicate things. I appreciate that some people will want this functionality (I know I do), and it is trival to add on a per-project basis.

As an alternative to the POST approach (with its accompanying annoyance of forms or JavaScript) you might create a per-session token to prevent blind attacks. A small substring algorithmically derived from the session identifier might be sufficient, considering what's at stake here. This way you could use /logout/a5b8/ to log out. At any rate, I think this should be up to the developer to add, while it would be acceptable for the documentation to raise awareness of this fact.

comment:8 Changed 5 years ago by Pyth

-0 is more like it.

comment:9 Changed 5 years ago by kmtracey

  • Resolution set to wontfix
  • Status changed from reopened to closed

Please read: http://docs.djangoproject.com/en/dev/internals/contributing/

where it says please do not reopen tickets that have been closed wontfix by a core developer. The right way to get a wontfix decision reconsidered is to raise the issue on the developers list, where there will be a wider audience to participate and perhaps change the mind(s) of the individual(s) who decided to wontfix the ticket, or perhaps change your mind. Or not, if there is not enough interest or no minds are amenable to changing. At any rate on that list you can be somewhat sure anyone who might be at all interested in the issue will at least see the conversation, which cannot be said for updates made to individual tickets in the tracker.

comment:10 Changed 19 months ago by aaugustin

  • Easy pickings unset
  • Resolution wontfix deleted
  • Severity set to Normal
  • Status changed from closed to reopened
  • Type set to Uncategorized
  • UI/UX unset

comment:11 Changed 19 months ago by aaugustin

  • Resolution set to duplicate
  • Status changed from reopened to closed

Reported again as #15619, with a longer discussion.

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.