fix: false positive on unpermitted parameters (#3097)

* fix: false positive on unpermitted parameters

* alternative approach

* wip

* revert dedicated method

* shorter comment

* wip

* fix associations

* enable raise when unnpermitted parameters on tests

* Update spec/rails_helper.rb

* permit id on action params

* wip

* lint

* lint

* more linting

* wip

* lint

* fix test

* add age as extra param on default

* lint

* rm unnecessary permit

* enforce permitted_params naming

* escape params src

app/controllers/avo/actions_controller.rb

@@ -64,7 +64,7 @@ def handle
     private
 
     def action_params
-      params.permit(:authenticity_token, :resource_name, :action_id, :button, fields: {})
+      @action_params ||= params.permit(:id, :authenticity_token, :resource_name, :action_id, :button, fields: {})
     end
 
     def set_action

app/controllers/avo/associations_controller.rb

@@ -132,7 +132,7 @@ def set_reflection_field
     end
 
     def attachment_id
-      params[:related_id] || params.require(:fields).permit(:related_id)[:related_id]
+      params[:related_id] || params.dig(:fields, :related_id)
     end
 
     def reflection_class
@@ -200,7 +200,7 @@ def through_reflection?
     end
 
     def additional_params
-      @additional_params ||= params[:fields].permit(@attach_fields&.map(&:id))
+      @additional_params ||= params[:fields].slice(*@attach_fields&.map(&:id))
     end
 
     def set_attach_fields
@@ -214,15 +214,12 @@ def set_attach_fields
 
     def new_join_record
       @resource.fill_record(
-        @reflection.through_reflection.klass.new,
-        additional_params.merge(
-          {
-            source_foreign_key => @attachment_record.id,
-            through_foreign_key => @record.id
-          }
+        @reflection.through_reflection.klass.new(
+          source_foreign_key => @attachment_record.id,
+          through_foreign_key => @record.id
         ),
+        additional_params,
         fields: @attach_fields,
-        extra_params: [source_foreign_key, through_foreign_key]
       )
     end
   end

app/views/avo/home/failed_to_load.html.erb

@@ -2,7 +2,7 @@
   <%
   classes = 'absolute inset-auto left-1/2 top-1/2 transform -translate-x-1/2 -translate-y-1/2'
   label = t 'avo.failed_to_load'
-  src_url = params[:src].present? && !params[:src].starts_with?('http://') ? params.permit(:src).fetch(:src) : nil
+  src_url = params[:src].present? && !params[:src].starts_with?('http://') ? CGI.escapeHTML(params[:src]) : nil
 %>
   <div class="relative flex-1 py-4">
     <div class="relative block text-gray-300 h-64 w-full">

lib/avo/resources/base.rb

@@ -445,9 +445,9 @@ def fields_by_database_id
           .to_h
       end
 
-      def fill_record(record, params, extra_params: [], fields: nil)
+      def fill_record(record, permitted_params, extra_params: [], fields: nil)
         # Write the field values
-        params.each do |key, value|
+        permitted_params.each do |key, value|
           field = if fields.present?
             fields.find { |f| f.id == key.to_sym }
           else
@@ -456,13 +456,17 @@ def fill_record(record, params, extra_params: [], fields: nil)
 
           next unless field.present?
 
-          record = field.fill_field record, key, value, params
+          record = field.fill_field record, key, value, permitted_params
         end
 
         # Write the user configured extra params to the record
         if extra_params.present?
+          # Pick only the extra params
+          # params at this point are already permited, only need the keys to access them
+          extra_attributes = permitted_params.slice(*flatten_keys(extra_params))
+
           # Let Rails fill in the rest of the params
-          record.assign_attributes params.permit(extra_params)
+          record.assign_attributes extra_attributes
         end
 
         record
@@ -613,6 +617,22 @@ def entity_loader(entity)
       def record_param
         @record_param ||= @record.persisted? ? @record.to_param : nil
       end
+
+      private
+
+      def flatten_keys(array)
+        # [:fish_type, information: [:name, :history], reviews_attributes: [:body, :user_id]]
+        # becomes
+        # [:fish_type, :information, :reviews_attributes]
+        array.flat_map do |item|
+          case item
+          when Hash
+            item.keys
+          else
+            item
+          end
+        end
+      end
     end
   end
 end

spec/dummy/app/avo/resources/fish.rb

@@ -3,7 +3,7 @@ class Avo::Resources::Fish < Avo::BaseResource
   self.search = {
     query: -> { query.ransack(id_eq: params[:q], name_cont: params[:q], m: "or").result(distinct: false) }
   }
-  self.extra_params = [:fish_type, :something_else, properties: [], information: [:name, :history], reviews_attributes: [:body, :user_id]]
+  self.extra_params = [:fish_type, :something_else, properties: [], information: [:name, :history, :age], reviews_attributes: [:body, :user_id]]
   self.view_types = -> do
     if current_user.is_admin?
       [:table, :grid]

spec/features/avo/controller_action_spec.rb

@@ -29,14 +29,14 @@
     end
 
     it "assigns the @widget" do
-      post :create, params: {user: {name: "Adrian"}}
+      post :create, params: {user: {first_name: "Adrian"}}
 
       assert assigns(:view).new?
       assert assigns(:view).form?
     end
 
     it "assigns the @widget" do
-      put :update, params: {id: user.id, user: {name: "Adrian"}}
+      put :update, params: {id: user.id, user: {first_name: "Adrian"}}
 
       assert assigns(:view).edit?
       assert assigns(:view).form?

spec/features/avo/custom_fields_in_resource_tools_spec.rb

@@ -4,7 +4,7 @@
   let(:fish) { create :fish, name: :Salmon }
 
   describe "fish information" do
-    it "sends the params to the model" do
+    before(:example) do
       visit avo.edit_resources_fish_path fish
 
       expect(page).to have_text "There should be an image of this fish below 🐠"
@@ -20,10 +20,55 @@
       find('input[name="fish[information][age]"]').set("Fishy age")
 
       expect(properties_fields.count).to be 2
+    end
+
+    it "raise unnpermited params" do
+      # Remove age from extra params
+      with_temporary_class_option(
+        Avo::Resources::Fish,
+        :extra_params,
+        [
+          :fish_type,
+          :something_else,
+          properties: [],
+          information: [:name, :history],
+          reviews_attributes: [:body, :user_id]
+        ]
+      ) do
+        expect { save }.to raise_error("found unpermitted parameter: :age")
+      end
+    end
+
+    it "sends the params to the model ignoring unpermitted age" do
       expect_any_instance_of(Fish).to receive("fish_type=").with("Fishy type")
       expect_any_instance_of(Fish).to receive("properties=").with(["Fishy property 1", "Fishy property 2"])
+      # Verify that age is not included on the information
       expect_any_instance_of(Fish).to receive("information=").with({name: "Fishy name", history: "Fishy history"})
 
+      # Remove age from extra params
+      with_temporary_class_option(
+        Avo::Resources::Fish,
+        :extra_params,
+        [
+          :fish_type,
+          :something_else,
+          properties: [],
+          information: [:name, :history],
+          reviews_attributes: [:body, :user_id]
+        ]
+      ) do
+        # Don't raise unpermitted parameters error
+        with_temporary_class_option(ActionController::Parameters, :action_on_unpermitted_parameters, :log) do
+          save
+        end
+      end
+    end
+
+    it "sends all the params to the model" do
+      expect_any_instance_of(Fish).to receive("fish_type=").with("Fishy type")
+      expect_any_instance_of(Fish).to receive("properties=").with(["Fishy property 1", "Fishy property 2"])
+      expect_any_instance_of(Fish).to receive("information=").with({name: "Fishy name", history: "Fishy history", age: "Fishy age"})
+
       save
     end
   end

spec/rails_helper.rb

@@ -49,6 +49,9 @@
 
 # ActiveRecord::Migrator.migrate(File.join(Rails.root, 'db/migrate'))
 
+# Ensure that there are no unpermitted_parameters logs
+ActionController::Parameters.action_on_unpermitted_parameters = :raise
+
 require "support/download_helpers"
 require "support/request_helpers"
 

spec/support/avo_helpers.rb

@@ -8,5 +8,14 @@ def find_component(name)
     def expect_missing_component(name)
       expect(page).not_to have_css "[data-component-name=\"#{name.to_s.underscore}\"]"
     end
+
+    def with_temporary_class_option(klass, option_name, option_value, &block)
+      previous_value = klass.send(option_name)
+      klass.send(:"#{option_name}=", option_value)
+
+      block.call
+
+      klass.send(:"#{option_name}=", previous_value)
+    end
   end
 end

spec/system/avo/create_via_belongs_to_spec.rb

@@ -21,14 +21,14 @@
             fill_in 'user_last_name', with: 'LastName'
             fill_in 'user_password', with: 'password'
             fill_in 'user_password_confirmation', with: 'password'
-            click_on 'Save'
+            click_on "Save"
             sleep 0.2
           end
         end.to change(User, :count).by(1)
 
         expect(page).to have_select('fish_user_id', selected: User.last.name)
 
-        click_on 'Save'
+        click_on "Save"
         sleep 0.2
 
         expect(fish.reload.user).to eq User.last
@@ -47,14 +47,14 @@
         expect do
           within('.modal-container') do
             fill_in 'post_name', with: 'Test post'
-            click_on 'Save'
+            click_on "Save"
             sleep 0.2
           end
         end.to change(Post, :count).by(1)
 
         expect(page).to have_select('comment_commentable_id', selected: Post.last.name)
 
-        click_on 'Save'
+        click_on "Save"
         sleep 0.2
 
         expect(comment.reload.commentable).to eq Post.last
@@ -75,7 +75,7 @@
           fill_in 'user_last_name', with: 'LastName'
           fill_in 'user_password', with: 'password'
           fill_in 'user_password_confirmation', with: 'password'
-          click_on 'Save'
+          click_on "Save"
           sleep 0.2
         end
       end.to change(User, :count).by(1)
@@ -86,7 +86,7 @@
       expect(page).to have_select('fish_user_id', selected: User.last.name)
 
       expect do
-        click_on 'Save'
+        click_on "Save"
         sleep 0.2
       end.to change(Fish, :count).by(1)
 
@@ -106,7 +106,7 @@
       expect do
         within('.modal-container') do
           fill_in 'post_name', with: 'Test post'
-          click_on 'Save'
+          click_on "Save"
           sleep 0.2
         end
       end.to change(Post, :count).by(1)
@@ -115,7 +115,7 @@
 
       expect do
         fill_in 'comment_body', with: 'Test Comment'
-        click_on 'Save'
+        click_on "Save"
         sleep 0.2
       end.to change(Comment, :count).by(1)
 
@@ -137,15 +137,15 @@
       expect do
         within('.modal-container') do
           fill_in 'course_name', with: 'Test course'
-          click_on 'Save'
+          click_on "Save"
           sleep 0.2
         end
       end.to change(Course, :count).by(1)
 
       expect(page).to have_select('course_link_course_id', selected: Course.last.name)
 
       expect do
-        click_on 'Save'
+        click_on "Save"
         sleep 0.2
       end.to change(Course::Link, :count).by(1)
 

spec/system/avo/tags_spec.rb

@@ -143,8 +143,12 @@
       wait_for_tags_to_load(field_value_slot)
       type(:down, :return)
 
-      sleep 0.3
-      click_on "Run"
+      # TODO: fix "found unpermitted parameter: :user_id-dummy"
+      # tags field passes a dummy param generated from a fake input which is always unpermitted
+      with_temporary_class_option(ActionController::Parameters, :action_on_unpermitted_parameters, :log) do
+        sleep 0.3
+        click_on "Run"
+      end
     end
   end