Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Logout should only work as a POST request #124

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
6 participants
Contributor

cgmartin commented Aug 25, 2012

"...GET requests should always be idempotent. For example, your sign-out should only work as a POST request so that someone cannot make your users sign out by just including an img tag in their forum signature."
(Tips from an ex-Digg webdev @ http://duruk.net/some-web-development-tips/)

If concerned that "logout" should be a link instead of a form button, you can style a button to look like a link and the underlying functionality still works: http://stackoverflow.com/questions/4111743/html-anchor-to-send-post-data-on-click

Owner

DASPRiD commented Aug 25, 2012

I'd argue against this. What you actually just need is a one-time token for the sign out links.

Owner

EvanDotPro commented Aug 25, 2012

I'm just testing something.

Contributor

cgmartin commented Aug 25, 2012

As seen on IRC:

  • @EvanDotPro : strictly making it post doesn't completely solve it. you also need a csrf token.
  • jurians : in http sense you need a POST or alike because logout is not idempotent. You shouldn't be able to logout when you're not logged in. And in a lot of cases, you'll be redirected to login when you try to access /logout so that doesn't work then.
  • @DASPRiD : need a token either way (GET or POST methods)

I'll add a CSRF token to this PR, and attempt a second PR with the one-time-token with GET links if this one gets the thumbs down.

Thanks for the feedback

Contributor

cgmartin commented Aug 26, 2012

Added a logout form that includes a csrf check.

Also added a logout page that will be seen if the csrf check fails or if the user tries logging out with a GET request (similar to Github.com behavior). Screencap of what they would see: http://grab.by/fDZw

Feel free to close this PR if the consensus is to use a link with a one-time token as @DASPRiD suggests.

@cgmartin cgmartin and 1 other commented on an outdated diff Aug 26, 2012

src/ZfcUser/Form/LogoutFilter.php
@@ -0,0 +1,29 @@
+<?php
+
+namespace ZfcUser\Form;
+
+use Zend\Validator\Csrf as CsrfValidator;
+use ZfcBase\InputFilter\ProvidesEventsInputFilter;
+
+class LogoutFilter extends ProvidesEventsInputFilter
+{
+ public function __construct()
+ {
+ // Allow CSRF to timeout with session. Csrf element/validator uses 300 by default.
+ $sessionLifetime = ini_get("session.gc_maxlifetime");
+ $csrfValidator = new CsrfValidator(array('name' => 'csrf', 'timeout' => $sessionLifetime));
@cgmartin

cgmartin Aug 26, 2012

Contributor

Jurians brought up a good point on IRC... Shouldn't the standard zf2 CsrfValidator always use the session lifetime rather than using a default of 300 secs? Are there any security concerns that the 5 minute default addresses?

@EvanDotPro

EvanDotPro Sep 5, 2012

Owner

Probably. Keep in mind that Zend\Session has its own lifetime mechanism that sits on top of PHP's session gc, IIRC.

@cgmartin

cgmartin Sep 5, 2012

Contributor

Ah yes, good catch. I had made a recent change to the Csrf element in ZF2 so that this code in ZfcUser is now unnecessary. Previously you were required to set a timeout value. Now you can set the timeout to null to fall back to the parent Zend\Session lifetime. I'll clean this up. Thanks!

Owner

EvanDotPro commented Sep 5, 2012

I like where this PR is going -- redirection to the logout page with a submit/post button if the CSRF token fails seems like a pretty solid solution, IMO. GitHub seems to employ it successfully.

Contributor

cgmartin commented Sep 8, 2012

Cleaned up the Logout form's input filter using the new methods in the Csrf Element. (And rebased)

Contributor

thoaionline commented Dec 27, 2012

I don't think POST-based logout is a good idea. If you are over-cautious then a url with token should be sufficient. In most cases, a barebone /user/logout link is all people need.

Owner

EvanDotPro commented Feb 19, 2013

I've been talking about possibly creating a ZfcUserSecurity module which would add additional security features to ZfcUser -- perhaps we could look at a way to provide this in an external module like that to help keep the base simple? Though in all honesty, I don't see much wrong with a post logout... One 'gotcha' I could see is that it would be a little annoying if people are using Zend\Navigation for their logout link.

@maxnuf maxnuf referenced this pull request Feb 24, 2013

Closed

redirect to other domain #241

Contributor

Freeaqingme commented Mar 5, 2013

don't think POST-based logout is a good idea. If you are over-cautious then a url with token should be sufficient. In most cases, a barebone /user/logout link is all people need.

I'm not sure why you say that. There are various reasons for using post:

  • Browsers pre-fetch stuff as long as they're mere GET requests
  • The HTTP spec (RFC2616) says that a GET request may never change a state of any kind.

Also, CSRF-protection should be used by default. See also https://groups.google.com/forum/#!topic/django-developers/ax95u_f82D4/discussion

@EvanDotPro i oppose the idea of a security module for various reasons:

  • Developers have shown time after time not to bother with security. When it works from a functional PoV, they're done.
  • Assuming everybody want their application to be secure, everybody using zfcuser would also be using zfcuser_secure. If 100% of the users of module A also use module B, and module B only complements A, I'd say A and B should probably be merged.
  • Stretching things a little, if we provide a separate security module, should we also start saving passwords unhashed in zfcuser itself? ;)
Owner

EvanDotPro commented Mar 12, 2013

@Freeaqingme We'd obviously have to draw a line. We would of course always ship sane, secure defaults in ZfcUser -- I merely meant a security module to provide a lot of extra functionality that most would consider "bloat" such as login attempt logs, indexing those to be used for triggering things like a captcha for an account after X failed attempts, etc, etc. We used to have fields like last login IP/datetime, register IP/datetime, etc, and many complained about these being bloat especially since they weren't being used anywhere. A module like ZfcUserSecurity would provide a place to put things like this, and could also provide additional things you see around like an account activity log like Gmail provides, etc...

As for if the logout via post should be default or in a separate module, I don't have strong feelings one way or another. It's a pretty mild attack vector on its own, but it's real none-the-less, and could realistically be used as an intermediary step to another attack vector depending on the circumstances.

That said, if we want to include it in the base, I think we could provide a zfcUserLogoutLink() helper that would generate the logout link/button/js/whatever... could have options for a form+button, or link + js submit on a hidden form like GitHub. We'd then change the default logout action to only allow logout if it's a post and simply show a view with a logout form/button on a GET request, like GitHub. We could then document how they can simply add their own logout action that allows GET if they want/need that (i.e., lazy Zend\Navigation users... can't think of another use-case really).

Just thinking aloud, we could even provide a "jQuery" option to a logoutButton helper that would generate the inlineScript necessary to attach an onClick listener to an <a> using a attribute CSS selector... $( "a[href='/user/logout']" ) If they are already using jQuery, this could be an easy fix for the Zend\Navigation and similar use-cases where there's some underlying mechanism "forcing" certain markup for the logout link... Maybe that's just an edge case we shouldn't even worry about though. :)

Contributor

Freeaqingme commented Mar 12, 2013

@EvanDotPro your comment makes sense. Perhaps an Auditing addon would indeed make sense.

To add a little additional context (from irc):

<Freeaqingme> ocramius, but it doesn't really affect anyone, you can just style a (submit) button as link. And by creating a partial or view helper that renders the log out link + the containing form, it shouldn't be more hassle than it would otherwise be
<EvanDotPro> i believe github uses a link and onclick to trigger the form submit
<EvanDotPro> and if you just go to the url, it's just a page with a submit button on it
<EvanDotPro> example: https://github.com/logout
<Freeaqingme> EvanDotPro, that's another solution. But would introduce 1 extra step for users with accessibility problems
<Freeaqingme> nonetheless; you've got the logging out done securely ;)

@cgmartin how do you feel about adding this to your PR? Please let me know if you don't have time or something. It's time to get this baby merged ;)

Owner

EvanDotPro commented Mar 12, 2013

For the record, this and unit tests were the main things I was holding off for a 1.0 tag for.

Contributor

Freeaqingme commented Mar 13, 2013

@EvanDotPro I think that we'd also need to fix the redirecting as previously discussed on irc prior to tagging 1.0. That's the other potential attack vector I can see (IIRC).

Owner

EvanDotPro commented Mar 13, 2013

@Freeaqingme absolutely, I agree. We can create a 1.0 milestone and organize the outstanding issues under that.

Contributor

cgmartin commented Mar 13, 2013

@Freeaqingme I'd be happy to spend some more time on this one. Can you clarify the additions to be made? Is it just a matter of creating a view helper or partial for rendering the logout form? Thanks!

Owner

spiffyjr commented Jun 21, 2013

@EvanDotPro any additional input? If no, @Freeaqingme you can rebase and we'll get this merged.

See https://github.com/ZF-Commons/ZF-Commons/wiki/ZfcUser-1.0-to-2.0-PR-queue-handling for why it was closed (and reopen when ready).

@spiffyjr spiffyjr closed this Jun 21, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment