Routing: respond with 405 Method Not Allowed status when the route path matches but the HTTP method does not. Closes #6953.

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

7 files changed, 91 insertions, 16 deletions

diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG
index 0258d02b54..356c88a42a 100644
--- a/actionpack/CHANGELOG
+++ b/actionpack/CHANGELOG
@@ -1,5 +1,7 @@
 *SVN*
 
+* Routing: respond with 405 Method Not Allowed status when the route path matches but the HTTP method does not.  #6953 [Josh Peek, defeated, Dan Kubb, Coda Hale]
+
 * Add support for assert_select_rjs with :show and :hide. #7780 [dchelimsky]
 
 * Make assert_select's failure messages clearer about what failed. #7779 [dchelimsky]
diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb
index dc5a1d18cd..8bff41d38d 100755
--- a/actionpack/lib/action_controller/base.rb
+++ b/actionpack/lib/action_controller/base.rb
@@ -22,6 +22,24 @@ module ActionController #:nodoc:
       @failures = failures
     end
   end
+  class MethodNotAllowed < ActionControllerError #:nodoc:
+    attr_reader :allowed_methods
+
+    def initialize(*allowed_methods)
+      super("Only #{allowed_methods.to_sentence} requests are allowed.")
+      @allowed_methods = allowed_methods
+    end
+
+    def allowed_methods_header
+      allowed_methods.map { |method_symbol| method_symbol.to_s.upcase } * ', '
+    end
+
+    def handle_response!(response)
+      response.headers['Allow'] ||= allowed_methods_header
+    end
+  end
+  class NotImplemented < MethodNotAllowed #:nodoc:
+  end
   class UnknownController < ActionControllerError #:nodoc:
   end
   class UnknownAction < ActionControllerError #:nodoc:
diff --git a/actionpack/lib/action_controller/rescue.rb b/actionpack/lib/action_controller/rescue.rb
index 225acd28f3..3ab10107bf 100644
--- a/actionpack/lib/action_controller/rescue.rb
+++ b/actionpack/lib/action_controller/rescue.rb
@@ -13,12 +13,14 @@ module ActionController #:nodoc:
 
     DEFAULT_RESCUE_RESPONSE = :internal_server_error
     DEFAULT_RESCUE_RESPONSES = {
-      'ActionController::RoutingError'    => :not_found,
-      'ActionController::UnknownAction'   => :not_found,
-      'ActiveRecord::RecordNotFound'      => :not_found,
-      'ActiveRecord::StaleObjectError'    => :conflict,
-      'ActiveRecord::RecordInvalid'       => :unprocessable_entity,
-      'ActiveRecord::RecordNotSaved'      => :unprocessable_entity
+      'ActionController::RoutingError'     => :not_found,
+      'ActionController::UnknownAction'    => :not_found,
+      'ActiveRecord::RecordNotFound'       => :not_found,
+      'ActiveRecord::StaleObjectError'     => :conflict,
+      'ActiveRecord::RecordInvalid'        => :unprocessable_entity,
+      'ActiveRecord::RecordNotSaved'       => :unprocessable_entity,
+      'ActionController::MethodNotAllowed' => :method_not_allowed,
+      'ActionController::NotImplemented'   => :not_implemented
     }
 
     DEFAULT_RESCUE_TEMPLATE = 'diagnostics'
@@ -56,6 +58,12 @@ module ActionController #:nodoc:
         log_error(exception) if logger
         erase_results if performed?
 
+        # Let the exception alter the response if it wants.
+        # For example, MethodNotAllowed sets the Allow header.
+        if exception.respond_to?(:handle_response!)
+          exception.handle_response!(response)
+        end
+
         if consider_all_requests_local || local_request?
           rescue_action_locally(exception)
         else
diff --git a/actionpack/lib/action_controller/routing.rb b/actionpack/lib/action_controller/routing.rb
index 00d5050b7e..4a1eda82bf 100644
--- a/actionpack/lib/action_controller/routing.rb
+++ b/actionpack/lib/action_controller/routing.rb
@@ -251,6 +251,8 @@ module ActionController
     # TODO: , (comma) should be an allowed path character.
     SEPARATORS = %w( / ; . , ? )
 
+    HTTP_METHODS = [:get, :head, :post, :put, :delete]
+
     # The root paths which may contain controller files
     mattr_accessor :controller_paths
     self.controller_paths = []
@@ -1345,7 +1347,16 @@ module ActionController
         routes.each do |route|
           result = route.recognize(path, environment) and return result
         end
-        raise RoutingError, "no route found to match #{path.inspect} with #{environment.inspect}"
+
+        allows = HTTP_METHODS.select { |verb| routes.find { |r| r.recognize(path, :method => verb) } }
+
+        if environment[:method] && !HTTP_METHODS.include?(environment[:method])
+          raise NotImplemented.new(*allows)
+        elsif !allows.empty?
+          raise MethodNotAllowed.new(*allows)
+        else
+          raise RoutingError, "No route matches #{path.inspect} with #{environment.inspect}"
+        end
       end
   
       def routes_by_controller
diff --git a/actionpack/test/controller/rescue_test.rb b/actionpack/test/controller/rescue_test.rb
index 885ac0b8c0..100fee8a6e 100644
--- a/actionpack/test/controller/rescue_test.rb
+++ b/actionpack/test/controller/rescue_test.rb
@@ -7,6 +7,14 @@ class RescueController < ActionController::Base
     render :text => 'already rendered'
     raise "don't panic!"
   end
+
+  def method_not_allowed
+    raise ActionController::MethodNotAllowed.new(:get, :head, :put)
+  end
+  
+  def not_implemented
+    raise ActionController::NotImplemented.new(:get, :put)
+  end
 end
 
 
@@ -144,6 +152,8 @@ class RescueTest < Test::Unit::TestCase
     assert_equal :conflict, responses['ActiveRecord::StaleObjectError']
     assert_equal :unprocessable_entity, responses['ActiveRecord::RecordInvalid']
     assert_equal :unprocessable_entity, responses['ActiveRecord::RecordNotSaved']
+    assert_equal :method_not_allowed, responses['ActionController::MethodNotAllowed']
+    assert_equal :not_implemented, responses['ActionController::NotImplemented']
   end
 
   def test_rescue_templates
@@ -176,6 +186,22 @@ class RescueTest < Test::Unit::TestCase
       assert_nil @controller.send(:clean_backtrace, Exception.new)
     end
   end
+  
+  def test_not_implemented
+    with_all_requests_local false do
+      head :not_implemented
+    end
+    assert_response :not_implemented
+    assert_equal "GET, PUT", @response.headers['Allow']
+  end
+
+  def test_method_not_allowed
+    with_all_requests_local false do
+      get :method_not_allowed
+    end
+    assert_response :method_not_allowed
+    assert_equal "GET, HEAD, PUT", @response.headers['Allow']
+  end
 
   protected
     def with_all_requests_local(local = true)
diff --git a/actionpack/test/controller/resources_test.rb b/actionpack/test/controller/resources_test.rb
index 60d94120af..8149c02a9d 100644
--- a/actionpack/test/controller/resources_test.rb
+++ b/actionpack/test/controller/resources_test.rb
@@ -188,7 +188,7 @@ class ResourcesTest < Test::Unit::TestCase
     with_restful_routing :messages do
       assert_restful_routes_for :messages do |options|
         assert_recognizes(options.merge(:action => "new"), :path => "/messages/new", :method => :get)
-        assert_raises(ActionController::RoutingError) do
+        assert_raises(ActionController::MethodNotAllowed) do
           ActionController::Routing::Routes.recognize_path("/messages/new", :method => :post)
         end
       end
@@ -384,11 +384,11 @@ class ResourcesTest < Test::Unit::TestCase
       options = { :controller => controller_name.to_s }
       collection_path = "/#{controller_name}"
 
-      assert_raises(ActionController::RoutingError) do
+      assert_raises(ActionController::MethodNotAllowed) do
         assert_recognizes(options.merge(:action => 'update'), :path => collection_path, :method => :put)
       end
 
-      assert_raises(ActionController::RoutingError) do
+      assert_raises(ActionController::MethodNotAllowed) do
         assert_recognizes(options.merge(:action => 'destroy'), :path => collection_path, :method => :delete)
       end
     end
diff --git a/actionpack/test/controller/routing_test.rb b/actionpack/test/controller/routing_test.rb
index 2abccd7402..7efb6b8a6e 100644
--- a/actionpack/test/controller/routing_test.rb
+++ b/actionpack/test/controller/routing_test.rb
@@ -1590,8 +1590,13 @@ class RouteSetTest < Test::Unit::TestCase
     assert_nothing_raised { set.recognize(request) }
     assert_equal("update", request.path_parameters[:action])
 
-    request.method = :update
-    assert_raises(ActionController::RoutingError) { set.recognize(request) }
+    begin
+      request.method = :bacon
+      set.recognize(request)
+      flunk 'Should have raised NotImplemented'
+    rescue ActionController::NotImplemented => e
+      assert_equal [:get, :post, :put, :delete], e.allowed_methods
+    end
 
     request.path = "/people/5"
     request.method = :get
@@ -1608,10 +1613,15 @@ class RouteSetTest < Test::Unit::TestCase
     assert_nothing_raised { set.recognize(request) }
     assert_equal("destroy", request.path_parameters[:action])
     assert_equal("5", request.path_parameters[:id])
-    
-    request.method = :post
-    assert_raises(ActionController::RoutingError) { set.recognize(request) }
-    
+
+    begin
+      request.method = :post
+      set.recognize(request)
+      flunk 'Should have raised MethodNotAllowed'
+    rescue ActionController::MethodNotAllowed => e
+      assert_equal [:get, :put, :delete], e.allowed_methods
+    end
+
   ensure
     Object.send(:remove_const, :PeopleController)
   end