From 7f820f10808e7040f0480135653ac6de63229ff5 Mon Sep 17 00:00:00 2001 From: stephanierousset <61418966+Stef-Rousset@users.noreply.github.com> Date: Wed, 9 Oct 2024 16:37:29 +0200 Subject: [PATCH] Fix form initiatives (#600) * fix: XSS vulnerability with img on initiative form and model * test: add tests for new validation * docs: update overrides section * fix: interference from added extends with migration * style: update with rubocop * fix: ActiveRecord::NoDatabaseError * fix: trying to fix again interference * fix: update initiative fomr extends and modify admin initiative controller * refactor: update with rubocop * fix: validation in initiative_form extends and update test * docs: update overrides section in overloads.md * fix: Update OVERLOADS.md --------- Co-authored-by: Quentin Champenois <26109239+Quentinchampenois@users.noreply.github.com> --- OVERLOADS.md | 4 + .../admin/initiatives_controller.rb | 2 + config/application.rb | 2 +- .../initiatives/initiative_form_extends.rb | 19 + .../admin/initiatives_controller_spec.rb | 632 ++++++++++++++++++ spec/forms/initiative_form_spec.rb | 52 ++ 6 files changed, 710 insertions(+), 1 deletion(-) create mode 100644 lib/extends/forms/decidim/initiatives/initiative_form_extends.rb create mode 100644 spec/controllers/decidim/initiatives/admin/initiatives_controller_spec.rb create mode 100644 spec/forms/initiative_form_spec.rb diff --git a/OVERLOADS.md b/OVERLOADS.md index 78fb20434c..7e9b14d431 100644 --- a/OVERLOADS.md +++ b/OVERLOADS.md @@ -11,6 +11,10 @@ This override the default `AssembliesHelpler` from `decidim-assemblies`, by addi * `app/controllers/decidim/participatory_processes/participatory_processes_controller.rb` This override the default `ParticipatoryProcessesController` from `decidim-participatory_processes`, by adding custom sort for participatory_processes +## Initiative form +* `lib/extends/forms/decidim/initiatives/initiative_form_extends.rb` +This adds a validation to form's description. + ## Proposal's draft (Decidim awesome overrides 0.26.7) * `app/views/decidim/proposals/collaborative_drafts/_edit_form_fields.html.erb` diff --git a/app/controllers/decidim/initiatives/admin/initiatives_controller.rb b/app/controllers/decidim/initiatives/admin/initiatives_controller.rb index 06fbfba3e2..2f4a2a6ec8 100644 --- a/app/controllers/decidim/initiatives/admin/initiatives_controller.rb +++ b/app/controllers/decidim/initiatives/admin/initiatives_controller.rb @@ -34,6 +34,8 @@ def edit initiative: current_initiative ) @form.attachment = form_attachment_model + # "sanitize" the translated description, if the value is a hash (for machine_translation key) we don't modify it + @form.description.transform_values! { |v| v.instance_of?(String) ? v.gsub(/on\w+=("|')/, "nothing") : v } render layout: "decidim/admin/initiative" end diff --git a/config/application.rb b/config/application.rb index 1fbb48f0e9..2ea5e51ef6 100644 --- a/config/application.rb +++ b/config/application.rb @@ -37,7 +37,6 @@ class Application < Rails::Application # Application configuration can go into files in config/initializers # -- all .rb files in that directory are automatically loaded after loading # the framework and any gems in your application. - config.to_prepare do require "extends/helpers/decidim/forms/application_helper_extends" require "extends/cells/decidim/forms/step_navigation_cell_extends" @@ -57,6 +56,7 @@ class Application < Rails::Application require "extends/controllers/decidim/newsletters_controller_extends" require "extends/commands/decidim/admin/destroy_participatory_space_private_user_extends" require "extends/controllers/decidim/proposals/proposals_controller_extends" + require "extends/forms/decidim/initiatives/initiative_form_extends" Decidim::GraphiQL::Rails.config.tap do |config| config.initial_query = "{\n deployment {\n version\n branch\n remote\n upToDate\n currentCommit\n latestCommit\n locallyModified\n }\n}".html_safe diff --git a/lib/extends/forms/decidim/initiatives/initiative_form_extends.rb b/lib/extends/forms/decidim/initiatives/initiative_form_extends.rb new file mode 100644 index 0000000000..9918c015a2 --- /dev/null +++ b/lib/extends/forms/decidim/initiatives/initiative_form_extends.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +require "active_support/concern" + +module NoAdminInitiativeFormExtends + extend ActiveSupport::Concern + + included do + validate :no_javascript_event_in_description + + private + + def no_javascript_event_in_description + errors.add :description, :invalid if description =~ /on\w+=/ + end + end +end + +Decidim::Initiatives::InitiativeForm.include(NoAdminInitiativeFormExtends) diff --git a/spec/controllers/decidim/initiatives/admin/initiatives_controller_spec.rb b/spec/controllers/decidim/initiatives/admin/initiatives_controller_spec.rb new file mode 100644 index 0000000000..f76c91dd4d --- /dev/null +++ b/spec/controllers/decidim/initiatives/admin/initiatives_controller_spec.rb @@ -0,0 +1,632 @@ +# frozen_string_literal: true + +require "spec_helper" + +describe Decidim::Initiatives::Admin::InitiativesController do + routes { Decidim::Initiatives::AdminEngine.routes } + render_views + + let(:user) { create(:user, :confirmed, :admin_terms_accepted, organization: organization) } + let(:admin_user) { create(:user, :admin, :confirmed, organization: organization) } + let(:organization) { create(:organization) } + let!(:initiative) { create(:initiative, organization: organization) } + let!(:created_initiative) { create(:initiative, :created, organization: organization) } + + before do + request.env["decidim.current_organization"] = organization + initiative.author.update(admin_terms_accepted_at: Time.current) + initiative.committee_members.approved.first.user.update(admin_terms_accepted_at: Time.current) + created_initiative.author.update(admin_terms_accepted_at: Time.current) + end + + context "when index" do + context "and Users without initiatives" do + before do + sign_in user, scope: :user + end + + it "initiative list is not allowed" do + get :index + expect(flash[:alert]).not_to be_empty + expect(response).to have_http_status(:found) + end + end + + context "and anonymous users do" do + it "initiative list is not allowed" do + get :index + expect(flash[:alert]).not_to be_empty + expect(response).to have_http_status(:found) + end + end + + context "and admin users" do + before do + sign_in admin_user, scope: :user + end + + it "initiative list is allowed" do + get :index + expect(flash[:alert]).to be_nil + expect(response).to have_http_status(:ok) + end + end + + context "and initiative author" do + before do + sign_in initiative.author, scope: :user + end + + it "initiative list is allowed" do + get :index + expect(flash[:alert]).to be_nil + expect(response).to have_http_status(:ok) + end + end + + describe "and promotal committee members" do + before do + sign_in initiative.committee_members.approved.first.user, scope: :user + end + + it "initiative list is allowed" do + get :index + expect(flash[:alert]).to be_nil + expect(response).to have_http_status(:ok) + end + end + end + + context "when edit" do + context "and Users without initiatives" do + before do + sign_in user, scope: :user + end + + it "are not allowed" do + get :edit, params: { slug: initiative.to_param } + expect(flash[:alert]).not_to be_empty + expect(response).to have_http_status(:found) + end + end + + context "and anonymous users" do + it "are not allowed" do + get :edit, params: { slug: initiative.to_param } + expect(flash[:alert]).not_to be_empty + expect(response).to have_http_status(:found) + end + end + + context "and admin users" do + before do + sign_in admin_user, scope: :user + end + + it "are allowed" do + get :edit, params: { slug: initiative.to_param } + expect(flash[:alert]).to be_nil + expect(response).to have_http_status(:ok) + end + + context "and initiative description contains a javascript event" do + let!(:my_initiative) { create(:initiative, organization: organization, description: { en: 'img src="invalid.jpg" onerror="alert();">' }) } + + it "modifies description value" do + get :edit, params: { slug: my_initiative.to_param } + expect(response.body).to include("nothingalert()") + end + end + end + + context "and initiative author" do + before do + sign_in initiative.author, scope: :user + end + + it "are allowed" do + get :edit, params: { slug: initiative.to_param } + expect(flash[:alert]).to be_nil + expect(response).to have_http_status(:ok) + end + end + + context "and promotal committee members" do + before do + sign_in initiative.committee_members.approved.first.user, scope: :user + end + + it "are allowed" do + get :edit, params: { slug: initiative.to_param } + expect(flash[:alert]).to be_nil + expect(response).to have_http_status(:ok) + end + end + end + + context "when update" do + let(:valid_attributes) do + attrs = attributes_for(:initiative, organization: organization) + attrs[:signature_end_date] = I18n.l(attrs[:signature_end_date], format: :decidim_short) + attrs[:signature_start_date] = I18n.l(attrs[:signature_start_date], format: :decidim_short) + attrs + end + + context "and Users without initiatives" do + before do + sign_in user, scope: :user + end + + it "are not allowed" do + put :update, + params: { + slug: initiative.to_param, + initiative: valid_attributes + } + expect(flash[:alert]).not_to be_empty + expect(response).to have_http_status(:found) + end + end + + context "and anonymous users do" do + it "are not allowed" do + put :update, + params: { + slug: initiative.to_param, + initiative: valid_attributes + } + expect(flash[:alert]).not_to be_empty + expect(response).to have_http_status(:found) + end + end + + context "and admin users" do + before do + sign_in admin_user, scope: :user + end + + it "are allowed" do + put :update, + params: { + slug: initiative.to_param, + initiative: valid_attributes + } + expect(flash[:alert]).to be_nil + expect(response).to have_http_status(:found) + end + end + + context "and initiative author" do + context "and initiative published" do + before do + sign_in initiative.author, scope: :user + end + + it "are not allowed" do + put :update, + params: { + slug: initiative.to_param, + initiative: valid_attributes + } + expect(flash[:alert]).not_to be_nil + expect(response).to have_http_status(:found) + end + end + + context "and initiative created" do + let(:initiative) { create(:initiative, :created, organization: organization) } + + before do + sign_in initiative.author, scope: :user + end + + it "are allowed" do + put :update, + params: { + slug: initiative.to_param, + initiative: valid_attributes + } + expect(flash[:alert]).to be_nil + expect(response).to have_http_status(:found) + end + end + end + + context "and promotal committee members" do + context "and initiative published" do + before do + sign_in initiative.committee_members.approved.first.user, scope: :user + end + + it "are not allowed" do + put :update, + params: { + slug: initiative.to_param, + initiative: valid_attributes + } + expect(flash[:alert]).not_to be_nil + expect(response).to have_http_status(:found) + end + end + + context "and initiative created" do + let(:initiative) { create(:initiative, :created, organization: organization) } + + before do + sign_in initiative.committee_members.approved.first.user, scope: :user + end + + it "are allowed" do + put :update, + params: { + slug: initiative.to_param, + initiative: valid_attributes + } + expect(flash[:alert]).to be_nil + expect(response).to have_http_status(:found) + end + end + end + end + + context "when GET send_to_technical_validation" do + context "and Initiative in created state" do + context "and has not enough committee members" do + before do + created_initiative.author.confirm + sign_in created_initiative.author, scope: :user + end + + it "does not pass to technical validation phase" do + created_initiative.type.update(minimum_committee_members: 4) + get :send_to_technical_validation, params: { slug: created_initiative.to_param } + + created_initiative.reload + expect(created_initiative).not_to be_validating + end + + it "does pass to technical validation phase" do + created_initiative.type.update(minimum_committee_members: 3) + get :send_to_technical_validation, params: { slug: created_initiative.to_param } + + created_initiative.reload + expect(created_initiative).to be_validating + end + end + + context "and User is not the owner of the initiative" do + let(:other_user) { create(:user, organization: organization) } + + before do + sign_in other_user, scope: :user + end + + it "Raises an error" do + get :send_to_technical_validation, params: { slug: created_initiative.to_param } + expect(flash[:alert]).not_to be_empty + expect(response).to have_http_status(:found) + end + end + + context "and User is the owner of the initiative. It is in created state" do + before do + created_initiative.author.confirm + sign_in created_initiative.author, scope: :user + end + + it "Passes to technical validation phase" do + get :send_to_technical_validation, params: { slug: created_initiative.to_param } + + created_initiative.reload + expect(created_initiative).to be_validating + end + end + end + + context "and Initiative in discarded state" do + let!(:discarded_initiative) { create(:initiative, :discarded, organization: organization) } + + before do + discarded_initiative.author.update(admin_terms_accepted_at: Time.current) + sign_in discarded_initiative.author, scope: :user + end + + it "Passes to technical validation phase" do + get :send_to_technical_validation, params: { slug: discarded_initiative.to_param } + + discarded_initiative.reload + expect(discarded_initiative).to be_validating + end + end + + context "and Initiative not in created or discarded state (published)" do + before do + sign_in initiative.author, scope: :user + end + + it "Raises an error" do + get :send_to_technical_validation, params: { slug: initiative.to_param } + expect(flash[:alert]).not_to be_empty + expect(response).to have_http_status(:found) + end + end + end + + context "when POST publish" do + let!(:initiative) { create(:initiative, :validating, organization: organization) } + + context "and Initiative owner" do + before do + sign_in initiative.author, scope: :user + end + + it "Raises an error" do + post :publish, params: { slug: initiative.to_param } + expect(flash[:alert]).not_to be_empty + expect(response).to have_http_status(:found) + end + end + + context "and Administrator" do + let!(:admin) { create(:user, :confirmed, :admin, organization: organization) } + + before do + sign_in admin, scope: :user + end + + it "initiative gets published" do + post :publish, params: { slug: initiative.to_param } + expect(response).to have_http_status(:found) + + initiative.reload + expect(initiative).to be_published + expect(initiative.published_at).not_to be_nil + expect(initiative.signature_start_date).not_to be_nil + expect(initiative.signature_end_date).not_to be_nil + end + end + end + + context "when DELETE unpublish" do + context "and Initiative owner" do + before do + sign_in initiative.author, scope: :user + end + + it "Raises an error" do + delete :unpublish, params: { slug: initiative.to_param } + expect(flash[:alert]).not_to be_empty + expect(response).to have_http_status(:found) + end + end + + context "and Administrator" do + let(:admin) { create(:user, :confirmed, :admin, organization: organization) } + + before do + sign_in admin, scope: :user + end + + it "initiative gets unpublished" do + delete :unpublish, params: { slug: initiative.to_param } + expect(response).to have_http_status(:found) + + initiative.reload + expect(initiative).not_to be_published + expect(initiative).to be_discarded + expect(initiative.published_at).to be_nil + end + end + end + + context "when DELETE discard" do + let(:initiative) { create(:initiative, :validating, organization: organization) } + + context "and Initiative owner" do + before do + sign_in initiative.author, scope: :user + end + + it "Raises an error" do + delete :discard, params: { slug: initiative.to_param } + expect(flash[:alert]).not_to be_empty + expect(response).to have_http_status(:found) + end + end + + context "and Administrator" do + let(:admin) { create(:user, :confirmed, :admin, organization: organization) } + + before do + sign_in admin, scope: :user + end + + it "initiative gets discarded" do + delete :discard, params: { slug: initiative.to_param } + expect(response).to have_http_status(:found) + + initiative.reload + expect(initiative).to be_discarded + expect(initiative.published_at).to be_nil + end + end + end + + context "when POST accept" do + let!(:initiative) { create(:initiative, :acceptable, signature_type: "any", organization: organization) } + + context "and Initiative owner" do + before do + sign_in initiative.author, scope: :user + end + + it "Raises an error" do + post :accept, params: { slug: initiative.to_param } + expect(flash[:alert]).not_to be_empty + expect(response).to have_http_status(:found) + end + end + + context "when Administrator" do + let!(:admin) { create(:user, :confirmed, :admin, organization: organization) } + + before do + sign_in admin, scope: :user + end + + it "initiative gets published" do + post :accept, params: { slug: initiative.to_param } + expect(response).to have_http_status(:found) + + initiative.reload + expect(initiative).to be_accepted + end + end + end + + context "when DELETE reject" do + let!(:initiative) { create(:initiative, :rejectable, signature_type: "any", organization: organization) } + + context "and Initiative owner" do + before do + sign_in initiative.author, scope: :user + end + + it "Raises an error" do + delete :reject, params: { slug: initiative.to_param } + expect(flash[:alert]).not_to be_empty + expect(response).to have_http_status(:found) + end + end + + context "when Administrator" do + let!(:admin) { create(:user, :confirmed, :admin, organization: organization) } + + before do + sign_in admin, scope: :user + end + + it "initiative gets rejected" do + delete :reject, params: { slug: initiative.to_param } + expect(response).to have_http_status(:found) + expect(flash[:alert]).to be_nil + + initiative.reload + expect(initiative).to be_rejected + end + end + end + + context "when GET export_votes" do + let(:initiative) { create(:initiative, organization: organization, signature_type: "any") } + + context "and author" do + before do + sign_in initiative.author, scope: :user + end + + it "is not allowed" do + get :export_votes, params: { slug: initiative.to_param, format: :csv } + expect(flash[:alert]).not_to be_empty + expect(response).to have_http_status(:found) + end + end + + context "and promotal committee" do + before do + sign_in initiative.committee_members.approved.first.user, scope: :user + end + + it "is not allowed" do + get :export_votes, params: { slug: initiative.to_param, format: :csv } + expect(flash[:alert]).not_to be_empty + expect(response).to have_http_status(:found) + end + end + + context "and admin user" do + let!(:vote) { create(:initiative_user_vote, initiative: initiative) } + + before do + sign_in admin_user, scope: :user + end + + it "is allowed" do + get :export_votes, params: { slug: initiative.to_param, format: :csv } + expect(flash[:alert]).to be_nil + expect(response).to have_http_status(:ok) + end + end + end + + context "when GET export_pdf_signatures" do + let(:initiative) { create(:initiative, :with_user_extra_fields_collection, organization: organization) } + + context "and author" do + before do + sign_in initiative.author, scope: :user + end + + it "is not allowed" do + get :export_pdf_signatures, params: { slug: initiative.to_param, format: :pdf } + expect(flash[:alert]).not_to be_empty + expect(response).to have_http_status(:found) + end + end + + context "and admin" do + before do + sign_in admin_user, scope: :user + end + + it "is allowed" do + get :export_pdf_signatures, params: { slug: initiative.to_param, format: :pdf } + expect(flash[:alert]).to be_nil + expect(response).to have_http_status(:ok) + end + end + end + + context "when GET export" do + context "and user" do + before do + sign_in user, scope: :user + end + + it "is not allowed" do + expect(Decidim::Initiatives::ExportInitiativesJob).not_to receive(:perform_later).with(user, "CSV", nil) + + get :export, params: { format: :csv } + expect(flash[:alert]).not_to be_empty + expect(response).to have_http_status(:found) + end + end + + context "and admin" do + before do + sign_in admin_user, scope: :user + end + + it "is allowed" do + expect(Decidim::Initiatives::ExportInitiativesJob).to receive(:perform_later).with(admin_user, organization, "csv", nil) + + get :export, params: { format: :csv } + expect(flash[:alert]).to be_nil + expect(response).to have_http_status(:found) + end + + context "when a collection of ids is passed as a parameter" do + let!(:initiatives) { create_list(:initiative, 3, organization: organization) } + let(:collection_ids) { initiatives.map(&:id) } + + it "enqueues the job" do + expect(Decidim::Initiatives::ExportInitiativesJob).to receive(:perform_later).with(admin_user, organization, "csv", collection_ids) + + get :export, params: { format: :csv, collection_ids: collection_ids } + expect(flash[:alert]).to be_nil + expect(response).to have_http_status(:found) + end + end + end + end +end diff --git a/spec/forms/initiative_form_spec.rb b/spec/forms/initiative_form_spec.rb new file mode 100644 index 0000000000..cc412c774e --- /dev/null +++ b/spec/forms/initiative_form_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require "spec_helper" + +module Decidim + module Initiatives + describe InitiativeForm do + subject { described_class.from_params(attributes).with_context(context) } + + let(:organization) { create(:organization) } + let(:initiatives_type) { create(:initiatives_type, organization: organization) } + let(:scope) { create(:initiatives_type_scope, type: initiatives_type) } + let(:attachment_params) { nil } + + let(:title) { ::Faker::Lorem.sentence(word_count: 5) } + let(:my_description) { "" } + let(:attributes) do + { + title: title, + description: my_description, + type_id: initiatives_type.id, + scope_id: scope&.scope&.id, + signature_type: "offline", + attachment: attachment_params + }.merge(custom_signature_end_date).merge(area) + end + let(:custom_signature_end_date) { {} } + let(:area) { {} } + let(:context) do + { + current_organization: organization, + current_component: nil, + initiative_type: initiatives_type + } + end + let(:state) { "validating" } + let(:initiative) { create(:initiative, organization: organization, state: state, scoped_type: scope) } + + context "when everything is OK" do + let(:my_description) { ::Faker::Lorem.sentence(word_count: 25) } + + it { is_expected.to be_valid } + end + + context "when description contains a javascript event" do + let(:my_description) { '

description<img src=\"invalid.jpg\" onerror=\"alert();\">

' } + + it { is_expected.to be_invalid } + end + end + end +end