Add bit about protect_from_forgery with: :exception

presidentbeef / brakeman-site

Website for Brakeman

https://brakemanscanner.org

7 stars 18 forks source link

Add bit about protect_from_forgery with: :exception #17

Closed oreoshake closed 9 years ago

jvdp commented 8 years ago

@oreoshake Because of a pretty large number of reported exceptions (from different, real users) I was about to change from :with_exception to :reset_session for one of my apps when Brakeman notified me this would not be a good idea. I saw this "session memoization" remark and found this blog post with more details: https://blog.sourceclear.com/when-rails-protect_from_forgery-fails/

Isn't the real issue here the memoization of session attributes that may change throughout the request? Just throwing an exception leads to bad user experience when users end up reposting some form or end up with an invalid session somehow.

oreoshake commented 8 years ago

Isn't the real issue here the memoization of session attributes that may change throughout the request?

Yep. This was an entirely reactionary change to awful vulns we've seen in real life. We felt it was just too dangerous to do otherwise. New rails apps use this setting by default for the same reason.

What is happening that's causing the app to throw exceptions? That sounds like a bug in the application if it's happening for legitimate users.

jvdp commented 8 years ago

What is happening that's causing the app to throw exceptions? That sounds like a bug in the application if it's happening for legitimate users.

That's a good question. I'm 99% sure they are not 'legitimate' attacks so I think there's situations where you may end up posting a mismatched token. I will have to look into it more.

jvdp commented 8 years ago

@oreoshake I'm able to reproduce the issue as follows, using Safari on iOS:

visit the login page (standard devise)
submit the form empty / invalid; the result is a POST response with a flash message
close Safari and swipe it away in the task switcher
open Safari again and click "yes" when it asks to repost the form

The result is that Safari performs the same POST request with the old token but with a fresh session, causing an ActionController::InvalidAuthenticityToken exception. This seems to be possible on other browsers too (at least Chrome on Windows 7.)

The ideal outcome in this case would be to reject the POST but just go back to the login form, probably by catching the exception in this particular case. I think this establishes that legitimate users can in fact run in to this exception.