[Fix #51463] Raise an error when invalid :on or :except options are given to #resource or #resources

actionpack/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Raise an `ArgumentError` when invalid `:on` or `:except` options are passed into `#resouce` and `#resources`.
+
+    *Joshua Young*
+
 *   Request Forgery takes relative paths into account.
 
     *Stefan Wienert*

actionpack/lib/action_dispatch/routing/mapper.rb

@@ -1168,13 +1168,27 @@ module Resources
         CANONICAL_ACTIONS = %w(index create new show update destroy)
 
         class Resource # :nodoc:
+          class << self
+            def default_actions(api_only)
+              if api_only
+                [:index, :create, :show, :update, :destroy]
+              else
+                [:index, :create, :new, :show, :update, :destroy, :edit]
+              end
+            end
+          end
+
           attr_reader :controller, :path, :param
 
           def initialize(entities, api_only, shallow, options = {})
             if options[:param].to_s.include?(":")
               raise ArgumentError, ":param option can't contain colons"
             end
 
+            if (invalid_actions = (options.values_at(:only, :except).flatten.compact - default_actions)).any?
+              raise ArgumentError, "expected :only and :except to be one or more of #{default_actions}, got #{invalid_actions}"
+            end
+
             @name       = entities.to_s
             @path       = (options[:path] || @name).to_s
             @controller = (options[:controller] || @name).to_s
@@ -1188,11 +1202,7 @@ def initialize(entities, api_only, shallow, options = {})
           end
 
           def default_actions
-            if @api_only
-              [:index, :create, :show, :update, :destroy]
-            else
-              [:index, :create, :new, :show, :update, :destroy, :edit]
-            end
+            self.class.default_actions(@api_only)
           end
 
           def actions
@@ -1263,6 +1273,16 @@ def singleton?; false; end
         end
 
         class SingletonResource < Resource # :nodoc:
+          class << self
+            def default_actions(api_only)
+              if api_only
+                [:show, :create, :update, :destroy]
+              else
+                [:show, :create, :update, :destroy, :new, :edit]
+              end
+            end
+          end
+
           def initialize(entities, api_only, shallow, options)
             super
             @as         = nil
@@ -1271,11 +1291,7 @@ def initialize(entities, api_only, shallow, options)
           end
 
           def default_actions
-            if @api_only
-              [:show, :create, :update, :destroy]
-            else
-              [:show, :create, :update, :destroy, :new, :edit]
-            end
+            self.class.default_actions(@api_only)
           end
 
           def plural
@@ -1336,7 +1352,7 @@ def resource(*resources, &block)
           end
 
           with_scope_level(:resource) do
-            options = apply_action_options options
+            options = apply_action_options :resource, options
             resource_scope(SingletonResource.new(resources.pop, api_only?, @scope[:shallow], options)) do
               yield if block_given?
 
@@ -1506,7 +1522,7 @@ def resources(*resources, &block)
           end
 
           with_scope_level(:resources) do
-            options = apply_action_options options
+            options = apply_action_options :resources, options
             resource_scope(Resource.new(resources.pop, api_only?, @scope[:shallow], options)) do
               yield if block_given?
 
@@ -1776,17 +1792,32 @@ def apply_common_behavior_for(method, resources, options, &block)
             false
           end
 
-          def apply_action_options(options)
+          def apply_action_options(method, options)
             return options if action_options? options
-            options.merge scope_action_options
+            options.merge scope_action_options(method)
           end
 
           def action_options?(options)
             options[:only] || options[:except]
           end
 
-          def scope_action_options
-            @scope[:action_options] || {}
+          def scope_action_options(method)
+            return {} unless @scope[:action_options]
+
+            actions = applicable_actions_for(method)
+            @scope[:action_options].dup.tap do |options|
+              (options[:only] = Array(options[:only]) & actions) if options[:only]
+              (options[:except] = Array(options[:except]) & actions) if options[:except]
+            end
+          end
+
+          def applicable_actions_for(method)
+            case method
+            when :resource
+              SingletonResource.default_actions(api_only?)
+            when :resources
+              Resource.default_actions(api_only?)
+            end
           end
 
           def resource_scope?

actionpack/test/controller/resources_test.rb

@@ -1109,6 +1109,51 @@ def test_singleton_resource_name_is_not_singularized
     end
   end
 
+  def test_invalid_only_option_for_resources
+    expected_message = "expected :only and :except to be one or more of [:index, :create, :new, :show, :update, :destroy, :edit], got [:foo, :bar]"
+    assert_raise(ArgumentError, match: expected_message) do
+      with_routing do |set|
+        set.draw do
+          resources :products, only: [:foo, :bar]
+        end
+      end
+    end
+  end
+
+  def test_invalid_only_option_for_singleton_resource
+    expected_message = "expected :only and :except to be one or more of [:show, :create, :update, :destroy, :new, :edit], got [:foo, :bar]"
+    assert_raise(ArgumentError, match: expected_message) do
+      with_routing do |set|
+        set.draw do
+          resource :products, only: [:foo, :bar]
+        end
+      end
+    end
+  end
+
+  def test_invalid_except_option_for_resources
+    expected_message = "expected :only and :except to be one or more of [:index, :create, :new, :show, :update, :destroy, :edit], got [:foo]"
+
+    assert_raise(ArgumentError, match: expected_message) do
+      with_routing do |set|
+        set.draw do
+          resources :products, except: :foo
+        end
+      end
+    end
+  end
+
+  def test_invalid_except_option_for_singleton_resource
+    expected_message = "expected :only and :except to be one or more of [:show, :create, :update, :destroy, :new, :edit], got [:foo]"
+    assert_raise(ArgumentError, match: expected_message) do
+      with_routing do |set|
+        set.draw do
+          resource :products, except: :foo
+        end
+      end
+    end
+  end
+
   private
     def with_restful_routing(*args)
       options = args.extract_options!