Action caching is limited to GET requests returning 200 OK status. Closes #3335.

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@6970 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

3 files changed, 38 insertions, 7 deletions

diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG
index 0209cdf065..aa82d379f9 100644
--- a/actionpack/CHANGELOG
+++ b/actionpack/CHANGELOG
@@ -1,5 +1,7 @@
 *SVN*
 
+* Action caching is limited to GET requests returning 200 OK status.  #3335 [tom@craz8.com, halfbyte, Dan Kubb, Josh Peek]
+
 * Improve Text Helper test coverage.  #7274 [Rob Sanheim, Josh Peek]
 
 * Improve Action View test coverage.  #7241, #7243, #7244 [Rich Collins]
diff --git a/actionpack/lib/action_controller/caching.rb b/actionpack/lib/action_controller/caching.rb
index f77cf207f5..f9f3c37c1c 100644
--- a/actionpack/lib/action_controller/caching.rb
+++ b/actionpack/lib/action_controller/caching.rb
@@ -240,20 +240,22 @@ module ActionController #:nodoc:
         end
 
         def after(controller)
-          return if !@actions.include?(controller.action_name.intern) || controller.rendered_action_cache
+          return if !@actions.include?(controller.action_name.intern) || controller.rendered_action_cache || !caching_allowed(controller)
           controller.write_fragment(controller.action_cache_path.path, controller.response.body)
         end
-        
+
         private
-          
           def set_content_type!(controller, extension)
             controller.response.content_type = Mime::EXTENSION_LOOKUP[extension].to_s if extension
           end
-          
+
           def path_options_for(controller, options)
             ((path_options = options[:cache_path]).respond_to?(:call) ? path_options.call(controller) : path_options) || {}
           end
-          
+
+          def caching_allowed(controller)
+            controller.request.get? && controller.response.headers['Status'].to_i == 200
+          end
       end
       
       class ActionCachePath
diff --git a/actionpack/test/controller/caching_test.rb b/actionpack/test/controller/caching_test.rb
index 0dc73472cf..29fa0074f7 100644
--- a/actionpack/test/controller/caching_test.rb
+++ b/actionpack/test/controller/caching_test.rb
@@ -121,7 +121,7 @@ end
 
 
 class ActionCachingTestController < ActionController::Base
-  caches_action :index
+  caches_action :index, :redirected, :forbidden
   caches_action :show, :cache_path => 'http://test.host/custom/show'
   caches_action :edit, :cache_path => Proc.new { |c| c.params[:id] ? "http://test.host/#{c.params[:id]};edit" : "http://test.host/edit" }
 
@@ -129,7 +129,16 @@ class ActionCachingTestController < ActionController::Base
     @cache_this = Time.now.to_f.to_s
     render :text => @cache_this
   end
-  
+
+  def redirected
+    redirect_to :action => 'index'
+  end
+
+  def forbidden
+    render :text => "Forbidden"
+    headers["Status"] = "403 Forbidden"
+  end
+
   alias_method :show, :index
   alias_method :edit, :index
 
@@ -245,6 +254,24 @@ class ActionCacheTest < Test::Unit::TestCase
     assert_equal david_cache, @response.body
   end
 
+  def test_redirect_is_not_cached
+    get :redirected
+    assert_response :redirect
+    reset!
+
+    get :redirected
+    assert_response :redirect
+  end
+
+  def test_forbidden_is_not_cached
+    get :forbidden
+    assert_response :forbidden
+    reset!
+
+    get :forbidden
+    assert_response :forbidden
+  end
+
   def test_xml_version_of_resource_is_treated_as_different_cache
     @mock_controller.mock_url_for = 'http://example.org/posts/'
     @mock_controller.mock_path    = '/posts/index.xml'