diff --git a/app/controllers/avo/actions_controller.rb b/app/controllers/avo/actions_controller.rb index d01acf2ded..76bb1ef5dc 100644 --- a/app/controllers/avo/actions_controller.rb +++ b/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 diff --git a/app/controllers/avo/associations_controller.rb b/app/controllers/avo/associations_controller.rb index f95ed4171f..01d63c6a35 100644 --- a/app/controllers/avo/associations_controller.rb +++ b/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 diff --git a/app/views/avo/home/failed_to_load.html.erb b/app/views/avo/home/failed_to_load.html.erb index ffa9a8c2c9..2eaea2ee63 100644 --- a/app/views/avo/home/failed_to_load.html.erb +++ b/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 %>
diff --git a/lib/avo/resources/base.rb b/lib/avo/resources/base.rb index 14f6c922a9..882866f32e 100644 --- a/lib/avo/resources/base.rb +++ b/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 diff --git a/spec/dummy/app/avo/resources/fish.rb b/spec/dummy/app/avo/resources/fish.rb index d6b237e50f..c8a619ac1d 100644 --- a/spec/dummy/app/avo/resources/fish.rb +++ b/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] diff --git a/spec/features/avo/controller_action_spec.rb b/spec/features/avo/controller_action_spec.rb index 28cf4148d3..209daf1fdd 100644 --- a/spec/features/avo/controller_action_spec.rb +++ b/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? diff --git a/spec/features/avo/custom_fields_in_resource_tools_spec.rb b/spec/features/avo/custom_fields_in_resource_tools_spec.rb index 3d2226529f..d7586a24e2 100644 --- a/spec/features/avo/custom_fields_in_resource_tools_spec.rb +++ b/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 diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 6594b60af7..ac22e484f5 100644 --- a/spec/rails_helper.rb +++ b/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" diff --git a/spec/support/avo_helpers.rb b/spec/support/avo_helpers.rb index 17f4587786..fbe3d52f51 100644 --- a/spec/support/avo_helpers.rb +++ b/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 diff --git a/spec/system/avo/create_via_belongs_to_spec.rb b/spec/system/avo/create_via_belongs_to_spec.rb index e67efeeb05..63b9ebfd5a 100644 --- a/spec/system/avo/create_via_belongs_to_spec.rb +++ b/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,7 +137,7 @@ 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) @@ -145,7 +145,7 @@ 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) diff --git a/spec/system/avo/tags_spec.rb b/spec/system/avo/tags_spec.rb index 264815516c..5e5972a138 100644 --- a/spec/system/avo/tags_spec.rb +++ b/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