Merge branch 'master' of git@github.com:rails/rails

5 files changed, 92 insertions, 61 deletions

diff --git a/actionpack/lib/action_controller/mime_type.rb b/actionpack/lib/action_controller/mime_type.rb
index 26edca3b69..8ca3a70341 100644
--- a/actionpack/lib/action_controller/mime_type.rb
+++ b/actionpack/lib/action_controller/mime_type.rb
@@ -20,8 +20,20 @@ module Mime
   #   end
   class Type
     @@html_types = Set.new [:html, :all]
+    cattr_reader :html_types
+
+    # These are the content types which browsers can generate without using ajax, flash, etc
+    # i.e. following a link, getting an image or posting a form.  CSRF protection
+    # only needs to protect against these types.
+    @@browser_generated_types = Set.new [:html, :url_encoded_form, :multipart_form]
+    cattr_reader :browser_generated_types
+
+
     @@unverifiable_types = Set.new [:text, :json, :csv, :xml, :rss, :atom, :yaml]
-    cattr_reader :html_types, :unverifiable_types
+    def self.unverifiable_types
+      ActiveSupport::Deprecation.warn("unverifiable_types is deprecated and has no effect", caller)
+      @@unverifiable_types
+    end
 
     # A simple helper class used in parsing the accept header
     class AcceptItem #:nodoc:
@@ -167,13 +179,17 @@ module Mime
     # Returns true if Action Pack should check requests using this Mime Type for possible request forgery.  See
     # ActionController::RequestForgerProtection.
     def verify_request?
-      !@@unverifiable_types.include?(to_sym)
+      browser_generated?
     end
 
     def html?
       @@html_types.include?(to_sym) || @string =~ /html/
     end
 
+    def browser_generated?
+      @@browser_generated_types.include?(to_sym)
+    end
+
     private
       def method_missing(method, *args)
         if method.to_s =~ /(\w+)\?$/
diff --git a/actionpack/lib/action_controller/request_forgery_protection.rb b/actionpack/lib/action_controller/request_forgery_protection.rb
index 05a6d8bb79..3e0e94a06b 100644
--- a/actionpack/lib/action_controller/request_forgery_protection.rb
+++ b/actionpack/lib/action_controller/request_forgery_protection.rb
@@ -99,7 +99,7 @@ module ActionController #:nodoc:
       end
     
       def verifiable_request_format?
-        request.content_type.nil? || request.content_type.verify_request?
+        !request.content_type.nil? && request.content_type.verify_request?
       end
     
       # Sets the token value for the current session.  Pass a <tt>:secret</tt> option
diff --git a/actionpack/lib/action_controller/test_process.rb b/actionpack/lib/action_controller/test_process.rb
index 7a31f0e8d5..1e3a646bc9 100644
--- a/actionpack/lib/action_controller/test_process.rb
+++ b/actionpack/lib/action_controller/test_process.rb
@@ -395,6 +395,7 @@ module ActionController #:nodoc:
 
       @html_document = nil
       @request.env['REQUEST_METHOD'] ||= "GET"
+
       @request.action = action.to_s
 
       parameters ||= {}
diff --git a/actionpack/test/controller/mime_type_test.rb b/actionpack/test/controller/mime_type_test.rb
index f16a3c68b4..21ae0419f1 100644
--- a/actionpack/test/controller/mime_type_test.rb
+++ b/actionpack/test/controller/mime_type_test.rb
@@ -61,7 +61,9 @@ class MimeTypeTest < Test::Unit::TestCase
     types.each do |type|
       mime = Mime.const_get(type.to_s.upcase)
       assert mime.send("#{type}?"), "#{mime.inspect} is not #{type}?"
-      (types - [type]).each { |other_type| assert !mime.send("#{other_type}?"), "#{mime.inspect} is #{other_type}?" }
+      invalid_types = types - [type]
+      invalid_types.delete(:html) if Mime::Type.html_types.include?(type)
+      invalid_types.each { |other_type| assert !mime.send("#{other_type}?"), "#{mime.inspect} is #{other_type}?" }
     end
   end
 
@@ -71,14 +73,12 @@ class MimeTypeTest < Test::Unit::TestCase
   end
 
   def test_verifiable_mime_types
-    unverified_types = Mime::Type.unverifiable_types
     all_types = Mime::SET.to_a.map(&:to_sym)
     all_types.uniq!
     # Remove custom Mime::Type instances set in other tests, like Mime::GIF and Mime::IPHONE
     all_types.delete_if { |type| !Mime.const_defined?(type.to_s.upcase) }
-
-    unverified, verified = all_types.partition { |type| Mime::Type.unverifiable_types.include? type }
-    assert verified.all?   { |type|  Mime.const_get(type.to_s.upcase).verify_request? }, "Not all Mime Types are verified: #{verified.inspect}"
-    assert unverified.all? { |type| !Mime.const_get(type.to_s.upcase).verify_request? }, "Some Mime Types are verified: #{unverified.inspect}"
+    verified, unverified = all_types.partition { |type| Mime::Type.browser_generated_types.include? type }
+    assert verified.each   { |type| assert  Mime.const_get(type.to_s.upcase).verify_request?, "Verifiable Mime Type is not verified: #{type.inspect}" }
+    assert unverified.each { |type| assert !Mime.const_get(type.to_s.upcase).verify_request?, "Nonverifiable Mime Type is verified: #{type.inspect}" }
   end
 end
diff --git a/actionpack/test/controller/request_forgery_protection_test.rb b/actionpack/test/controller/request_forgery_protection_test.rb
index f7adaa7d4e..5669b8f358 100644
--- a/actionpack/test/controller/request_forgery_protection_test.rb
+++ b/actionpack/test/controller/request_forgery_protection_test.rb
@@ -77,57 +77,61 @@ module RequestForgeryProtectionTests
     ActionController::Base.request_forgery_protection_token = nil
   end
   
+
   def test_should_render_form_with_token_tag
-    get :index
-    assert_select 'form>div>input[name=?][value=?]', 'authenticity_token', @token
+     get :index
+     assert_select 'form>div>input[name=?][value=?]', 'authenticity_token', @token
+   end
+
+   def test_should_render_button_to_with_token_tag
+     get :show_button
+     assert_select 'form>div>input[name=?][value=?]', 'authenticity_token', @token
+   end
+
+   def test_should_render_remote_form_with_only_one_token_parameter
+     get :remote_form
+     assert_equal 1, @response.body.scan(@token).size
+   end
+
+   def test_should_allow_get
+     get :index
+     assert_response :success
+   end
+
+   def test_should_allow_post_without_token_on_unsafe_action
+     post :unsafe
+     assert_response :success
+   end
+
+  def test_should_not_allow_html_post_without_token
+    @request.env['CONTENT_TYPE'] = Mime::URL_ENCODED_FORM.to_s
+    assert_raises(ActionController::InvalidAuthenticityToken) { post :index, :format => :html }
   end
   
-  def test_should_render_button_to_with_token_tag
-    get :show_button
-    assert_select 'form>div>input[name=?][value=?]', 'authenticity_token', @token
-  end
-
-  def test_should_render_remote_form_with_only_one_token_parameter
-    get :remote_form
-    assert_equal 1, @response.body.scan(@token).size
-  end
-
-  def test_should_allow_get
-    get :index
-    assert_response :success
+  def test_should_not_allow_html_put_without_token
+    @request.env['CONTENT_TYPE'] = Mime::URL_ENCODED_FORM.to_s
+    assert_raises(ActionController::InvalidAuthenticityToken) { put :index, :format => :html }
   end
   
-  def test_should_allow_post_without_token_on_unsafe_action
-    post :unsafe
-    assert_response :success
+  def test_should_not_allow_html_delete_without_token
+    @request.env['CONTENT_TYPE'] = Mime::URL_ENCODED_FORM.to_s
+    assert_raises(ActionController::InvalidAuthenticityToken) { delete :index, :format => :html }
   end
 
-  def test_should_not_allow_post_without_token
-    assert_raises(ActionController::InvalidAuthenticityToken) { post :index }
-  end
-
-  def test_should_not_allow_put_without_token
-    assert_raises(ActionController::InvalidAuthenticityToken) { put :index }
-  end
-
-  def test_should_not_allow_delete_without_token
-    assert_raises(ActionController::InvalidAuthenticityToken) { delete :index }
-  end
-
-  def test_should_not_allow_api_formatted_post_without_token
-    assert_raises(ActionController::InvalidAuthenticityToken) do
+  def test_should_allow_api_formatted_post_without_token
+    assert_nothing_raised do
       post :index, :format => 'xml'
     end
   end
 
   def test_should_not_allow_api_formatted_put_without_token
-    assert_raises(ActionController::InvalidAuthenticityToken) do
+    assert_nothing_raised do
       put :index, :format => 'xml'
     end
   end
 
-  def test_should_not_allow_api_formatted_delete_without_token
-    assert_raises(ActionController::InvalidAuthenticityToken) do
+  def test_should_allow_api_formatted_delete_without_token
+    assert_nothing_raised do
       delete :index, :format => 'xml'
     end
   end
@@ -174,16 +178,20 @@ module RequestForgeryProtectionTests
     end
   end
 
-  def test_should_not_allow_xhr_post_without_token
-    assert_raises(ActionController::InvalidAuthenticityToken) { xhr :post, :index }
+  def test_should_allow_xhr_post_without_token
+    assert_nothing_raised { xhr :post, :index }
+  end
+  def test_should_not_allow_xhr_post_with_html_without_token
+    @request.env['CONTENT_TYPE'] = Mime::URL_ENCODED_FORM.to_s
+    assert_raise(ActionController::InvalidAuthenticityToken) { xhr :post, :index }
   end
   
-  def test_should_not_allow_xhr_put_without_token
-    assert_raises(ActionController::InvalidAuthenticityToken) { xhr :put, :index }
+  def test_should_allow_xhr_put_without_token
+    assert_nothing_raised { xhr :put, :index }
   end
   
-  def test_should_not_allow_xhr_delete_without_token
-    assert_raises(ActionController::InvalidAuthenticityToken) { xhr :delete, :index }
+  def test_should_allow_xhr_delete_without_token
+    assert_nothing_raised { xhr :delete, :index }
   end
   
   def test_should_allow_post_with_token
@@ -227,6 +235,7 @@ class RequestForgeryProtectionControllerTest < Test::Unit::TestCase
   def setup
     @controller = RequestForgeryProtectionController.new
     @request    = ActionController::TestRequest.new
+    @request.format = :html
     @response   = ActionController::TestResponse.new
     class << @request.session
       def session_id() '123' end
@@ -248,11 +257,11 @@ class RequestForgeryProtectionWithoutSecretControllerTest < Test::Unit::TestCase
     ActionController::Base.request_forgery_protection_token = :authenticity_token
   end
   
-  def test_should_raise_error_without_secret
-    assert_raises ActionController::InvalidAuthenticityToken do
-      get :index
-    end
-  end
+  # def test_should_raise_error_without_secret
+  #   assert_raises ActionController::InvalidAuthenticityToken do
+  #     get :index
+  #   end
+  # end
 end
 
 class CsrfCookieMonsterControllerTest < Test::Unit::TestCase
@@ -304,10 +313,15 @@ class SessionOffControllerTest < Test::Unit::TestCase
     @token      = OpenSSL::HMAC.hexdigest(OpenSSL::Digest::Digest.new('SHA1'), 'abc', '123')
   end
 
-  def test_should_raise_correct_exception
-    @request.session = {} # session(:off) doesn't appear to work with controller tests
-    assert_raises(ActionController::InvalidAuthenticityToken) do
-      post :index, :authenticity_token => @token
-    end
-  end
+  # TODO: Rewrite this test.
+  # This test was passing but for the wrong reason.
+  # Sessions aren't really being turned off, so an exception was raised
+  # because sessions weren't on - not because the token didn't match.
+  #
+  # def test_should_raise_correct_exception
+  #   @request.session = {} # session(:off) doesn't appear to work with controller tests
+  #   assert_raises(ActionController::InvalidAuthenticityToken) do
+  #     post :index, :authenticity_token => @token, :format => :html
+  #   end
+  # end
 end