change ActionController::RequestForgeryProtection to use Mime::Type#verify_request? [#73]

5 files changed, 84 insertions, 12 deletions

diff --git a/actionpack/lib/action_controller/mime_type.rb b/actionpack/lib/action_controller/mime_type.rb
index 8c02f20521..f43e2ba06d 100644
--- a/actionpack/lib/action_controller/mime_type.rb
+++ b/actionpack/lib/action_controller/mime_type.rb
@@ -17,6 +17,10 @@ module Mime
   #     end
   #   end
   class Type
+    @@html_types = Set.new [:html, :all]
+    @@unverifiable_types = Set.new [:text, :json, :csv, :xml, :rss, :atom, :yaml]
+    cattr_reader :html_types, :unverifiable_types
+
     # A simple helper class used in parsing the accept header
     class AcceptItem #:nodoc:
       attr_accessor :order, :name, :q
@@ -153,12 +157,21 @@ module Mime
         synonym.to_s == mime_type.to_s || synonym.to_sym == mime_type.to_sym 
       end
     end
-    
+
+    # Returns true if ActionPack should check requests using this Mime Type for possible request forgery.  See
+    # ActionController::RequestForgerProtection.
+    def verify_request?
+      !@@unverifiable_types.include?(to_sym)
+    end
+
+    def html?
+      @@html_types.include?(to_sym) || @string =~ /html/
+    end
+
     private
       def method_missing(method, *args)
         if method.to_s =~ /(\w+)\?$/
-          mime_type = $1.downcase.to_sym
-          mime_type == @symbol || (mime_type == :html && @symbol == :all)
+          $1.downcase.to_sym == to_sym
         else
           super
         end
diff --git a/actionpack/lib/action_controller/mime_types.rb b/actionpack/lib/action_controller/mime_types.rb
index 71706b4c41..01a266d3fe 100644
--- a/actionpack/lib/action_controller/mime_types.rb
+++ b/actionpack/lib/action_controller/mime_types.rb
@@ -17,4 +17,4 @@ Mime::Type.register "multipart/form-data", :multipart_form
 Mime::Type.register "application/x-www-form-urlencoded", :url_encoded_form
 
 # http://www.ietf.org/rfc/rfc4627.txt
-Mime::Type.register "application/json", :json, %w( text/x-json )
+Mime::Type.register "application/json", :json, %w( text/x-json )
\ No newline at end of file
diff --git a/actionpack/lib/action_controller/request_forgery_protection.rb b/actionpack/lib/action_controller/request_forgery_protection.rb
index 1b349baeaf..b52a2d4ff1 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.html?
+        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/test/controller/mime_type_test.rb b/actionpack/test/controller/mime_type_test.rb
index 03b0f8bad2..f16a3c68b4 100644
--- a/actionpack/test/controller/mime_type_test.rb
+++ b/actionpack/test/controller/mime_type_test.rb
@@ -52,16 +52,33 @@ class MimeTypeTest < Test::Unit::TestCase
   end
 
   def test_type_convenience_methods
-    types = [:html, :xml, :png, :pdf, :yaml, :url_encoded_form]
+    # Don't test Mime::ALL, since it Mime::ALL#html? == true
+    types = Mime::SET.to_a.map(&:to_sym).uniq - [:all]
+
+    # Remove custom Mime::Type instances set in other tests, like Mime::GIF and Mime::IPHONE
+    types.delete_if { |type| !Mime.const_defined?(type.to_s.upcase) }
+
     types.each do |type|
       mime = Mime.const_get(type.to_s.upcase)
-      assert mime.send("#{type}?"), "Mime::#{type.to_s.upcase} is not #{type}?"
-      (types - [type]).each { |t| assert !mime.send("#{t}?"), "Mime::#{t.to_s.upcase} is #{t}?" }
+      assert mime.send("#{type}?"), "#{mime.inspect} is not #{type}?"
+      (types - [type]).each { |other_type| assert !mime.send("#{other_type}?"), "#{mime.inspect} is #{other_type}?" }
     end
   end
-  
+
   def test_mime_all_is_html
     assert Mime::ALL.all?,  "Mime::ALL is not all?"
     assert Mime::ALL.html?, "Mime::ALL is not html?"
   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}"
+  end
 end
diff --git a/actionpack/test/controller/request_forgery_protection_test.rb b/actionpack/test/controller/request_forgery_protection_test.rb
index 295075fed4..833e8d8e00 100644
--- a/actionpack/test/controller/request_forgery_protection_test.rb
+++ b/actionpack/test/controller/request_forgery_protection_test.rb
@@ -107,19 +107,61 @@ module RequestForgeryProtectionTests
   end
 
   def test_should_not_allow_api_formatted_post_without_token
-    assert_raises(ActionController::InvalidAuthenticityToken) do 
+    assert_raises(ActionController::InvalidAuthenticityToken) do
       post :index, :format => 'xml'
     end
   end
 
-  def test_should_not_allow_put_without_token
+  def test_should_not_allow_api_formatted_put_without_token
     assert_raises(ActionController::InvalidAuthenticityToken) do
       put :index, :format => 'xml'
     end
   end
 
-  def test_should_not_allow_delete_without_token
+  def test_should_not_allow_api_formatted_delete_without_token
+    assert_raises(ActionController::InvalidAuthenticityToken) do
+      delete :index, :format => 'xml'
+    end
+  end
+
+  def test_should_not_allow_api_formatted_post_sent_as_url_encoded_form_without_token
+    assert_raises(ActionController::InvalidAuthenticityToken) do
+      @request.env['CONTENT_TYPE'] = Mime::URL_ENCODED_FORM.to_s
+      post :index, :format => 'xml'
+    end
+  end
+
+  def test_should_not_allow_api_formatted_put_sent_as_url_encoded_form_without_token
+    assert_raises(ActionController::InvalidAuthenticityToken) do
+      @request.env['CONTENT_TYPE'] = Mime::URL_ENCODED_FORM.to_s
+      put :index, :format => 'xml'
+    end
+  end
+
+  def test_should_not_allow_api_formatted_delete_sent_as_url_encoded_form_without_token
+    assert_raises(ActionController::InvalidAuthenticityToken) do
+      @request.env['CONTENT_TYPE'] = Mime::URL_ENCODED_FORM.to_s
+      delete :index, :format => 'xml'
+    end
+  end
+
+  def test_should_not_allow_api_formatted_post_sent_as_multipart_form_without_token
+    assert_raises(ActionController::InvalidAuthenticityToken) do
+      @request.env['CONTENT_TYPE'] = Mime::MULTIPART_FORM.to_s
+      post :index, :format => 'xml'
+    end
+  end
+
+  def test_should_not_allow_api_formatted_put_sent_as_multipart_form_without_token
+    assert_raises(ActionController::InvalidAuthenticityToken) do
+      @request.env['CONTENT_TYPE'] = Mime::MULTIPART_FORM.to_s
+      put :index, :format => 'xml'
+    end
+  end
+
+  def test_should_not_allow_api_formatted_delete_sent_as_multipart_form_without_token
     assert_raises(ActionController::InvalidAuthenticityToken) do
+      @request.env['CONTENT_TYPE'] = Mime::MULTIPART_FORM.to_s
       delete :index, :format => 'xml'
     end
   end