Skip to content
This repository
Browse code

Changed request forgery protection to only worry about HTML-formatted…

… content requests.

Signed-off-by: Michael Koziarski <michael@koziarski.com>
  • Loading branch information...
commit fbbcd6f29aeccc938b97b5c01717365f8b67912c 1 parent 02df503
Jeff Cohen authored October 31, 2008 NZKoz committed November 13, 2008
4  actionpack/lib/action_controller/mime_type.rb
@@ -19,7 +19,7 @@ module Mime
19 19
   #     end
20 20
   #   end
21 21
   class Type
22  
-    @@html_types = Set.new [:html, :all]
  22
+    @@html_types = Set.new [:html, :url_encoded_form, :multipart_form, :all]
23 23
     @@unverifiable_types = Set.new [:text, :json, :csv, :xml, :rss, :atom, :yaml]
24 24
     cattr_reader :html_types, :unverifiable_types
25 25
 
@@ -167,7 +167,7 @@ def ==(mime_type)
167 167
     # Returns true if Action Pack should check requests using this Mime Type for possible request forgery.  See
168 168
     # ActionController::RequestForgerProtection.
169 169
     def verify_request?
170  
-      !@@unverifiable_types.include?(to_sym)
  170
+      html?
171 171
     end
172 172
 
173 173
     def html?
2  actionpack/lib/action_controller/request_forgery_protection.rb
@@ -99,7 +99,7 @@ def verified_request?
99 99
       end
100 100
     
101 101
       def verifiable_request_format?
102  
-        request.content_type.nil? || request.content_type.verify_request?
  102
+        !request.content_type.nil? && request.content_type.verify_request?
103 103
       end
104 104
     
105 105
       # Sets the token value for the current session.  Pass a <tt>:secret</tt> option
1  actionpack/lib/action_controller/test_process.rb
@@ -395,6 +395,7 @@ def process(action, parameters = nil, session = nil, flash = nil)
395 395
 
396 396
       @html_document = nil
397 397
       @request.env['REQUEST_METHOD'] ||= "GET"
  398
+
398 399
       @request.action = action.to_s
399 400
 
400 401
       parameters ||= {}
118  actionpack/test/controller/request_forgery_protection_test.rb
@@ -77,57 +77,61 @@ def teardown
77 77
     ActionController::Base.request_forgery_protection_token = nil
78 78
   end
79 79
   
  80
+
80 81
   def test_should_render_form_with_token_tag
81  
-    get :index
82  
-    assert_select 'form>div>input[name=?][value=?]', 'authenticity_token', @token
  82
+     get :index
  83
+     assert_select 'form>div>input[name=?][value=?]', 'authenticity_token', @token
  84
+   end
  85
+
  86
+   def test_should_render_button_to_with_token_tag
  87
+     get :show_button
  88
+     assert_select 'form>div>input[name=?][value=?]', 'authenticity_token', @token
  89
+   end
  90
+
  91
+   def test_should_render_remote_form_with_only_one_token_parameter
  92
+     get :remote_form
  93
+     assert_equal 1, @response.body.scan(@token).size
  94
+   end
  95
+
  96
+   def test_should_allow_get
  97
+     get :index
  98
+     assert_response :success
  99
+   end
  100
+
  101
+   def test_should_allow_post_without_token_on_unsafe_action
  102
+     post :unsafe
  103
+     assert_response :success
  104
+   end
  105
+
  106
+  def test_should_not_allow_html_post_without_token
  107
+    @request.env['CONTENT_TYPE'] = Mime::URL_ENCODED_FORM.to_s
  108
+    assert_raises(ActionController::InvalidAuthenticityToken) { post :index, :format => :html }
83 109
   end
84 110
   
85  
-  def test_should_render_button_to_with_token_tag
86  
-    get :show_button
87  
-    assert_select 'form>div>input[name=?][value=?]', 'authenticity_token', @token
88  
-  end
89  
-
90  
-  def test_should_render_remote_form_with_only_one_token_parameter
91  
-    get :remote_form
92  
-    assert_equal 1, @response.body.scan(@token).size
93  
-  end
94  
-
95  
-  def test_should_allow_get
96  
-    get :index
97  
-    assert_response :success
  111
+  def test_should_not_allow_html_put_without_token
  112
+    @request.env['CONTENT_TYPE'] = Mime::URL_ENCODED_FORM.to_s
  113
+    assert_raises(ActionController::InvalidAuthenticityToken) { put :index, :format => :html }
98 114
   end
99 115
   
100  
-  def test_should_allow_post_without_token_on_unsafe_action
101  
-    post :unsafe
102  
-    assert_response :success
  116
+  def test_should_not_allow_html_delete_without_token
  117
+    @request.env['CONTENT_TYPE'] = Mime::URL_ENCODED_FORM.to_s
  118
+    assert_raises(ActionController::InvalidAuthenticityToken) { delete :index, :format => :html }
103 119
   end
104 120
 
105  
-  def test_should_not_allow_post_without_token
106  
-    assert_raises(ActionController::InvalidAuthenticityToken) { post :index }
107  
-  end
108  
-
109  
-  def test_should_not_allow_put_without_token
110  
-    assert_raises(ActionController::InvalidAuthenticityToken) { put :index }
111  
-  end
112  
-
113  
-  def test_should_not_allow_delete_without_token
114  
-    assert_raises(ActionController::InvalidAuthenticityToken) { delete :index }
115  
-  end
116  
-
117  
-  def test_should_not_allow_api_formatted_post_without_token
118  
-    assert_raises(ActionController::InvalidAuthenticityToken) do
  121
+  def test_should_allow_api_formatted_post_without_token
  122
+    assert_nothing_raised do
119 123
       post :index, :format => 'xml'
120 124
     end
121 125
   end
122 126
 
123 127
   def test_should_not_allow_api_formatted_put_without_token
124  
-    assert_raises(ActionController::InvalidAuthenticityToken) do
  128
+    assert_nothing_raised do
125 129
       put :index, :format => 'xml'
126 130
     end
127 131
   end
128 132
 
129  
-  def test_should_not_allow_api_formatted_delete_without_token
130  
-    assert_raises(ActionController::InvalidAuthenticityToken) do
  133
+  def test_should_allow_api_formatted_delete_without_token
  134
+    assert_nothing_raised do
131 135
       delete :index, :format => 'xml'
132 136
     end
133 137
   end
@@ -174,16 +178,20 @@ def test_should_not_allow_api_formatted_delete_sent_as_multipart_form_without_to
174 178
     end
175 179
   end
176 180
 
177  
-  def test_should_not_allow_xhr_post_without_token
178  
-    assert_raises(ActionController::InvalidAuthenticityToken) { xhr :post, :index }
  181
+  def test_should_allow_xhr_post_without_token
  182
+    assert_nothing_raised { xhr :post, :index }
  183
+  end
  184
+  def test_should_not_allow_xhr_post_with_html_without_token
  185
+    @request.env['CONTENT_TYPE'] = Mime::URL_ENCODED_FORM.to_s
  186
+    assert_raise(ActionController::InvalidAuthenticityToken) { xhr :post, :index }
179 187
   end
180 188
   
181  
-  def test_should_not_allow_xhr_put_without_token
182  
-    assert_raises(ActionController::InvalidAuthenticityToken) { xhr :put, :index }
  189
+  def test_should_allow_xhr_put_without_token
  190
+    assert_nothing_raised { xhr :put, :index }
183 191
   end
184 192
   
185  
-  def test_should_not_allow_xhr_delete_without_token
186  
-    assert_raises(ActionController::InvalidAuthenticityToken) { xhr :delete, :index }
  193
+  def test_should_allow_xhr_delete_without_token
  194
+    assert_nothing_raised { xhr :delete, :index }
187 195
   end
188 196
   
189 197
   def test_should_allow_post_with_token
@@ -227,6 +235,7 @@ class RequestForgeryProtectionControllerTest < Test::Unit::TestCase
227 235
   def setup
228 236
     @controller = RequestForgeryProtectionController.new
229 237
     @request    = ActionController::TestRequest.new
  238
+    @request.format = :html
230 239
     @response   = ActionController::TestResponse.new
231 240
     class << @request.session
232 241
       def session_id() '123' end
@@ -248,11 +257,11 @@ def session_id() '123' end
248 257
     ActionController::Base.request_forgery_protection_token = :authenticity_token
249 258
   end
250 259
   
251  
-  def test_should_raise_error_without_secret
252  
-    assert_raises ActionController::InvalidAuthenticityToken do
253  
-      get :index
254  
-    end
255  
-  end
  260
+  # def test_should_raise_error_without_secret
  261
+  #   assert_raises ActionController::InvalidAuthenticityToken do
  262
+  #     get :index
  263
+  #   end
  264
+  # end
256 265
 end
257 266
 
258 267
 class CsrfCookieMonsterControllerTest < Test::Unit::TestCase
@@ -304,10 +313,15 @@ def setup
304 313
     @token      = OpenSSL::HMAC.hexdigest(OpenSSL::Digest::Digest.new('SHA1'), 'abc', '123')
305 314
   end
306 315
 
307  
-  def test_should_raise_correct_exception
308  
-    @request.session = {} # session(:off) doesn't appear to work with controller tests
309  
-    assert_raises(ActionController::InvalidAuthenticityToken) do
310  
-      post :index, :authenticity_token => @token
311  
-    end
312  
-  end
  316
+  # TODO: Rewrite this test.
  317
+  # This test was passing but for the wrong reason.
  318
+  # Sessions aren't really being turned off, so an exception was raised
  319
+  # because sessions weren't on - not because the token didn't match.
  320
+  #
  321
+  # def test_should_raise_correct_exception
  322
+  #   @request.session = {} # session(:off) doesn't appear to work with controller tests
  323
+  #   assert_raises(ActionController::InvalidAuthenticityToken) do
  324
+  #     post :index, :authenticity_token => @token, :format => :html
  325
+  #   end
  326
+  # end
313 327
 end

3 notes on commit fbbcd6f

Jason Roelofs

So…. What does this mean, exactly?

Brian Johnson

I don’t understand this. I have ajax calls that return json, aren’t they also susceptible to request forgery?

Michael Koziarski
Owner

it’s the request content type, not the response content type. i.e. what’s in the client’s Content-Type header.

This is never anything other than the browser_generated_types (see later commit).

Please sign in to comment.
Something went wrong with that request. Please try again.