Merge pull request rails#51464 from joshuay03/raise-error-on-invalid-resources-on-or-except-args

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

Author: byroot

actionpack/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Raise an `ArgumentError` when invalid `:on` or `:except` options are passed into `#resource` and `#resources`.
+
+    *Joshua Young*
+
 ## Rails 8.0.0.beta1 (September 26, 2024) ##
 
 *   Fix non-GET requests not updating cookies in `ActionController::TestCase`.

actionpack/lib/action_dispatch/routing/mapper.rb

@@ -1162,13 +1162,28 @@ 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
 
+            valid_actions = self.class.default_actions(false) # ignore api_only for this validation
+            if invalid_actions = invalid_only_except_options(options, valid_actions).presence
+              raise ArgumentError, ":only and :except must include only #{valid_actions}, but also included #{invalid_actions}"
+            end
+
             @name       = entities.to_s
             @path       = (options[:path] || @name).to_s
             @controller = (options[:controller] || @name).to_s
@@ -1182,11 +1197,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
@@ -1254,9 +1265,24 @@ def shallow?
           end
 
           def singleton?; false; end
+
+          private
+            def invalid_only_except_options(options, valid_actions)
+              options.values_at(:only, :except).flatten.compact.uniq - valid_actions
+            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
@@ -1265,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
@@ -1330,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?
 
@@ -1500,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?
 
@@ -1769,17 +1791,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/lib/action_dispatch/testing/assertions/routing.rb

@@ -118,9 +118,9 @@ def setup # :nodoc:
       #       assert_equal "/users", users_path
       #     end
       #
-      def with_routing(&block)
+      def with_routing(config = nil, &block)
         old_routes, old_controller = @routes, @controller
-        create_routes(&block)
+        create_routes(config, &block)
       ensure
         reset_routes(old_routes, old_controller)
       end
@@ -267,8 +267,8 @@ def method_missing(selector, ...)
       end
 
       private
-        def create_routes
-          @routes = ActionDispatch::Routing::RouteSet.new
+        def create_routes(config = nil)
+          @routes = ActionDispatch::Routing::RouteSet.new(config || ActionDispatch::Routing::RouteSet::DEFAULT_CONFIG)
           if @controller
             @controller = @controller.clone
             _routes = @routes

actionpack/test/controller/resources_test.rb

@@ -1108,6 +1108,51 @@ def test_singleton_resource_name_is_not_singularized
     end
   end
 
+  def test_invalid_only_option_for_resources
+    expected_message = ":only and :except must include only [:index, :create, :new, :show, :update, :destroy, :edit], but also included [: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 = ":only and :except must include only [:show, :create, :update, :destroy, :new, :edit], but also included [: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 = ":only and :except must include only [:index, :create, :new, :show, :update, :destroy, :edit], but also included [: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 = ":only and :except must include only [:show, :create, :update, :destroy, :new, :edit], but also included [: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!