phatworx / devise_security_extension

An enterprise security extension for devise, trying to meet industrial standard security demands for web applications.
MIT License
747 stars 346 forks source link

:password_expirable not enforced for non-html-requests #209

Open odegroot opened 7 years ago

odegroot commented 7 years ago

I've found that devise_security_extension does not enforce password expiration for non-html-requests.

https://github.com/phatworx/devise_security_extension/blob/b2ee978af7d49f0fb0e7271c6ac074dfb4d39353/lib/devise_security_extension/controllers/helpers.rb#L32

In other words, if the request is an html-request, then go and check if a password change is needed. And if it's not an html-request, then skip the password change check.

Where request is an ActionDispatch::Request, and request.format is this one: http://api.rubyonrails.org/classes/ActionDispatch/Http/MimeNegotiation.html#method-i-format

This was a surprise to me: we have some sort of API endpoint, and one API client (curl/postman) was allowed through even when their password was expired, while another API client (powershell) was blocked. After much headscratching and debugging I found out that the trigger for being blocked or not was the presence or absence of an Accept: */* HTTP header. That was surprising to me, I did not expect password expiry to be linked to Accept-headers.

I was unable to determine if this is intended behavior for :password_expirable. This behavior is not mentioned in the README, nor in comments in code, not in the commit message that introduced this behavior. (2063e05aedf77ce2baa9e89a7961ba0dab7655e6)

So the issue here is: Is this intended behavior?

If it is not, then this is a bug and a security flaw, since it allows users to bypass :password_expirable under some circumstances.

If it is, then it would be nice to mention this in the documentation (README.md) (see also !137).