-
Notifications
You must be signed in to change notification settings - Fork 21.6k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Don't check authenticity tokens for any AJAX requests
- Loading branch information
Showing
3 changed files
with
10 additions
and
6 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yehuda, can you explain this commit? Is this associated with a Lighthouse ticket somehow?
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this removed?
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Um, wasn’t half the point of Auth Tokens security against rogue AJAX requests? What is the point of this commit?
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this seems odd.
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jameskilton rogue Ajax requests cannot produce CSRF attacks, so this was just an annoyance to people hand-writing JS.
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jameskilton: If you can make a rogue AJAX request, you have bigger problems than CSRF attacks ;)
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At last, no dumb tokens to generate by hand =)
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@labria at last… a positive comment on this change! W00H00!
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice indeed. :)
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same original policy protects ajax requests. you can’t make browsers submit forms or make requests with custom headers.
This should be safe until browsers drop the same origin policy, which is (one hopes) forever.
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why thank you kind sir. No more ugly inline JS only for that one pesky line of auth token.
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also a big win for cacheability, because we can remove user-specific authenticity tokens from ajaxified links and forms that don’t need to support a non-ajax fallthrough.
Very nice!
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent! This was a pain when implementing unobtrusive JS…
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t understand this commit. What’s stopping an attacker who is compiling a browser herself and making different-origin a reality? Now the security of Rails apps is tied to ‘hopes’ of third parties doing the right thing?
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tilsammans: I believe CSRF protection is only useful to prevent users from doing cross-site requests without knowing it (i.e. submitting a form whose
is on another site where he’s authenticated).If one compiles its own browser making different-origin a reality as you suggested, he will be the only one in danger. Am I missing something here ?
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about the preformatted tag above… having previews or comment edition would be such a nice feature ;)
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really great addition, will save me a lot unnecessary hacking :)
@tilsammans
As far as I know protected_from_forgery, is for securing the frontend users, from other users/sites/scripts, who tried to pretend to be the frontend user. I mean that a person who is compiling a browser himself, for example, could only take his session data, witch is his. This is the same as that I could change the html with FireBug and fire some forms, run ajax queries, and other things from every site.
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what happens when someone posts a CSRF request with the header ‘X-Requested-With’ set to ‘XMLHttpRequest’ ?
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mildodurden: You can’t do that with browsers as they exist currently. If you can prove otherwise, please let us know and we can revert this.
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tilsammans: If the attacker is executing custom browsers on the user’s computer, there’s nothing we can do about it. They could also be running keyloggers, screenshotters, botnets etc.
This is for protecting against CSRF attacks, not a silver bullet for all possible attacks ever inventable.
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For further details on CSRF, check out this great Stanford paper:
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance to see this change on 2.3 branch? :)
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for a backport! :)
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for a backport!
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for a backport
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could backport it in a backwards compatible way. Check it if it’s included, otherwise ignore it.
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for for 2.3
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just saw binary42’s response. There reason to backport it, is that new code is still being written for Rails and this is a stumbling block when you want to make requests from Ajax, desktop client, or Flash. I don’t see this as a policy change, so much as a bug fix. (But I’m new to Rails, maybe I’m missing something)
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about local XSS attacks? With unescaped output, something akin to the MySpace worm is enabled by this change.
Granted, Rails coders always remember to escape output! :) But was the intent to remove a layer of defensive programming?
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1 for 2.3 (revised vote)
@binary42 after thinking about this more this morning, I realized that changing the behavior in a dot release is probably unwise. If there is an existing app depending on this behavior with an open cross-domain.xml and using cookie-based authentication, then a malicious flash app hosted on another site could make requests on behalf of a user without them being aware of it.
In terms of the “5 minute” fix… I’m still struggling to find the right few lines of code to write. I thought it work would to add the following to my controller:
but I still get the exception. I’m sure I’m missing something basic. Is the code change I need to make documented somewhere?
Thanks!
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ultrasaurus for a quick 2.3 patch, check out this gist: http://gist.github.com/98660 . You can put this an initializer and it should work
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulrosania: If your users are able to execute JavaScript through a XSS, this authenticity token won't be able to prevent CSRFs at all. It would be a snap to fetch the authenticity token from a page which has a form on it...
@everyone else who wants to see this in 2.3: http://gist.github.com/99635 <- a simple initializer like the one of gbuesing, but not using class eval.
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulrosania: In fact the twitter worm did exactly what NoKarma is describing. XSS is the stack-smashing of web attacks, if you're attacked you're done. Nothing can save you
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ultrasaurus:
skip_before_filter :verify_authenticity_token, :only => [:aircrafts_by_manufacturer]
?
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vrybas: instead of explicitly skipping the filter you should do:
protect_from_forgery :except=>[:aircrafts_by_manufacturer]
I'm also reasonably sure that you could do something along the lines of
protect_from_forgery :if=>:some_method_which_returns_true_when_ajax_or_whatever
256b0ee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it pretty easy to add authenticity_tokens to all ajax requests? ie these 7 lines of code? http://gist.github.com/149110