CSRF protection from cross-origin <script> tags #13345

jeremy · 2013-12-17T04:59:23Z

Extend protect_from_forgery to cover GET requests with JavaScript responses, protecting apps from cross-origin <script> embedding that may leak sensitive data.

TODO

Changelog
Update Security guide
Test actions that use Accept rather than an explicit format
Test actions that had neither .format nor Accept

jeremy · 2013-12-17T05:05:55Z

actionpack/lib/action_controller/metal/request_forgery_protection.rb

+      end
+
+      def non_xhr_javascript_response?
+        content_type =~ %r(\Atext/javascript) && !request.xhr?


This content type comparison is gross but I couldn't find a better alternative.

Can content type be some other like application/javascript?

I think so.

text/ecmascript and application/ecmascript are also valid values and I think there are more supported content types for javascript execution.

Whitelisting is not an option right?

unless I'm mistaken, doesn't the Mime types already take care of this?

request.format == :js && !request.xhr?

@pothibo request.format is the requested format, not the response Content-Type.

For example, in #test_should_only_allow_same_origin_js_get_with_xhr_header:

> [content_type, request.format, rendered_format] => ["text/javascript", #<Mime::Type:0x007ff4d96d15e8 @synonyms=["application/xhtml+xml"], @symbol=:html, @string="text/html">, #<Mime::NullType:0x007ff4d8438530>]

@rafaelfranca You'll only get those Content-Type if you manually specify them with render ..., type: 'application/ecmascript'. Mime::JS will always be text/javascript.

We may want a whitelist to extend verification to other Content-Type (including JSON for those supporting ancient browsers) in the future, but I don't think it's necessary to start.

acapilleri · 2013-12-17T06:15:26Z

👍 Thanks!

carlosantoniodasilva · 2013-12-17T10:34:01Z

actionpack/lib/action_controller/metal/request_forgery_protection.rb

@@ -89,6 +98,8 @@ def protect_from_forgery(options = {})
        self.forgery_protection_strategy = protection_method_class(options[:with] || :null_session)
        self.request_forgery_protection_token ||= :authenticity_token
        prepend_before_action :verify_authenticity_token, options
+        prepend_before_action :mark_for_same_origin_verification, options
+        append_after_action :verify_same_origin_request


If people can skip verify_authenticity_token in a controller that inherits protect_from_forgery, then mark_for_same_origin_verification would still be triggered and thus verify_same_origin_request too, not? Would they need to skip that too, independently?

Yes. I'll move the marker to verify_authenticity_token to fix that.

I used a separate, private method because we encourage people to change the builtin method:

# The actual before_action that is used. Modify this to change how you handle unverified requests. def verify_authenticity_token

So an overriding method would miss the marking and skip same-origin verification.

I'm removing that comment. Shouldn't be overriding that method.

rafaelfranca · 2013-12-17T15:15:19Z

Tests are broken 😢

rafaelfranca · 2013-12-17T15:16:04Z

actionpack/CHANGELOG.md

@@ -1,3 +1,8 @@
+*   Extend protect_from_forgery to GET requests with JavaScript responses,


Extend protect_from_forgery to GET requests with JavaScript responses,

@homakov

Thanks to @homakov for sounding the alarm about JSONP-style data leaking

jeremy · 2013-12-17T20:24:33Z

@rafaelfranca fixed the other broken tests, they hit the same-origin verification 😁

rafaelfranca · 2013-12-17T20:26:49Z

carlosantoniodasilva · 2013-12-17T20:28:36Z

👍

CSRF protection from cross-origin <script> tags

homakov · 2013-12-19T00:45:32Z

@jeremy probably instead of exception it should respond with "please send x requested with header to verify request origin"

jeremy · 2013-12-19T01:27:45Z

@homakov thought about that.. responding with 403 Forbidden so it's clear it's a client error not an application error

App devs can use rescue_from to do that today. But then they'd never get exception emails / notifications and they remain ignorant of the issue. We do that with CSRF also, and it feels strange for an app to ignore unauthorized requests. We just don't have a good convention or API for alerting devs about attacks that were thwarted. Could do that with an AS notification channel: publish security events, let apps/plugins consume them and report and alert.

In any case, this is starting as small as possible. I hope we'll see PRs that do a default rescue_from for this exception, or add separate response strategies like CSRF token verification handling uses.

jeremy · 2014-04-15T22:49:12Z

@TechFounder Sounds like you're making a GET request that has a JavaScript response, but you aren't passing the X-Requested-With request header. You'll need to track down where and why.

harmdewit · 2014-04-22T08:57:42Z

In our production server i'm getting ActionController::InvalidCrossOriginRequest:Security warnings in my app for requests with format=*/*. The strange thing is that most requests are correctly called with format=js and some with the strange format=html and even fewer with format=*/*. Only the requests with format=*/* are raising the error and return status 500. I'm trying to reproduce requests with format=*/* but without success.

Am i seeing these inconsistencies because of browser differences? I'm using jquery-rails, is it incorrectly setting the dataType?

masonforest · 2015-01-22T16:33:39Z

@harmdewit did you ever figure this out? I'm having the same issue.

harmdewit · 2015-01-22T21:08:55Z

I'm not sure what the issue was after this long time but putting this into our ApplicationController seemed to fix it

+  def non_xhr_javascript_response?
+    unless request.get?
+      content_type =~ %r(\Atext/javascript) && !request.xhr?
+    end
+  end

masonforest · 2015-01-22T21:21:23Z

Thanks @harmdewit! 😃

masonforest · 2015-01-22T22:44:16Z

I'm not sure what is requesting format=/ either.

Another solution I came up with is to reorder the respond_to formats:

So I changed this:

respond_to do |format|
     format.js do
       ...
     end
     format.html
end

to this:

respond_to do |format|
     format.html
     format.js do
       ...
     end
end

That way format=/ will return html.

rails/rails#13345 introduced CSRF protection to gets with JavaScript responses.

jeremy reviewed Dec 17, 2013
View reviewed changes

carlosantoniodasilva reviewed Dec 17, 2013
View reviewed changes

rafaelfranca reviewed Dec 17, 2013
View reviewed changes

CSRF protection from cross-origin <script> tags

1650bb3

Thanks to @homakov for sounding the alarm about JSONP-style data leaking

jeremy added a commit that referenced this pull request Dec 17, 2013

Merge pull request #13345 from jeremy/get-csrf

39ca25f

CSRF protection from cross-origin <script> tags

jeremy merged commit 39ca25f into rails:master Dec 17, 2013

jeremy deleted the get-csrf branch December 17, 2013 20:30

jeremy mentioned this pull request Dec 18, 2013

Ajax request doesn't validate CSRF token if the request is GET #11509

Closed

eval mentioned this pull request Jan 19, 2014

Introduce render :plain and render :html, make render :body as an alias to render :text #12374

Closed

4 tasks

JonRowe mentioned this pull request Mar 12, 2014

format: :js causes fails on Rails 4.1.0.rc1 rspec/rspec-rails#950

Closed

oliverguenther added a commit to oliverguenther/openproject that referenced this pull request Aug 10, 2015

Fix ActionController::InvalidCrossOriginRequest in specs

5e52803

rails/rails#13345 introduced CSRF protection to gets with JavaScript responses.

myabc pushed a commit to myabc/openproject that referenced this pull request Aug 19, 2015

Fix ActionController::InvalidCrossOriginRequest in specs

471fc04

rails/rails#13345 introduced CSRF protection to gets with JavaScript responses.

cantin mentioned this pull request Aug 22, 2017

Fix theme javascripts action publify/publify#823

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSRF protection from cross-origin <script> tags #13345

CSRF protection from cross-origin <script> tags #13345

jeremy commented Dec 17, 2013

jeremy Dec 17, 2013

carlosantoniodasilva Dec 17, 2013

rafaelfranca Dec 17, 2013

rafaelfranca Dec 17, 2013

pier-oliviert Dec 17, 2013

jeremy Dec 17, 2013

jeremy Dec 17, 2013

rafaelfranca Dec 17, 2013

acapilleri commented Dec 17, 2013

carlosantoniodasilva Dec 17, 2013

jeremy Dec 17, 2013

rafaelfranca commented Dec 17, 2013

rafaelfranca Dec 17, 2013

jeremy commented Dec 17, 2013

rafaelfranca commented Dec 17, 2013

carlosantoniodasilva commented Dec 17, 2013

homakov commented Dec 19, 2013

jeremy commented Dec 19, 2013

jeremy commented Apr 15, 2014

harmdewit commented Apr 22, 2014

masonforest commented Jan 22, 2015

harmdewit commented Jan 22, 2015

masonforest commented Jan 22, 2015

masonforest commented Jan 22, 2015

		@@ -1,3 +1,8 @@
		* Extend protect_from_forgery to GET requests with JavaScript responses,

CSRF protection from cross-origin <script> tags #13345

CSRF protection from cross-origin <script> tags #13345

Conversation

jeremy commented Dec 17, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acapilleri commented Dec 17, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelfranca commented Dec 17, 2013

Choose a reason for hiding this comment

jeremy commented Dec 17, 2013

rafaelfranca commented Dec 17, 2013

carlosantoniodasilva commented Dec 17, 2013

homakov commented Dec 19, 2013

jeremy commented Dec 19, 2013

jeremy commented Apr 15, 2014

harmdewit commented Apr 22, 2014

masonforest commented Jan 22, 2015

harmdewit commented Jan 22, 2015

masonforest commented Jan 22, 2015

masonforest commented Jan 22, 2015