From f86cb4370d4f5eca075bc47e669003e4791e905d Mon Sep 17 00:00:00 2001 From: stephanie rousset Date: Mon, 30 Sep 2024 11:40:01 +0200 Subject: [PATCH 01/12] fix: XSS vulnerability with img on initiative form and model --- config/application.rb | 2 ++ config/locales/en.yml | 2 ++ config/locales/fr.yml | 2 ++ .../initiatives/initiative_form_extends.rb | 19 +++++++++++++++++++ .../models/decidim/initiative_extends.rb | 18 ++++++++++++++++++ 5 files changed, 43 insertions(+) create mode 100644 lib/extends/forms/decidim/initiatives/initiative_form_extends.rb create mode 100644 lib/extends/models/decidim/initiative_extends.rb diff --git a/config/application.rb b/config/application.rb index 1fbb48f0e9..d754fd38ac 100644 --- a/config/application.rb +++ b/config/application.rb @@ -57,6 +57,8 @@ 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" + require "extends/models/decidim/initiative_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/config/locales/en.yml b/config/locales/en.yml index 8a50f0b69b..fc2460a52f 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -218,6 +218,8 @@ en: links: forgot_your_password: Forgot your password sign_in_with_france_connect: Sign in with france connect + errors: + no_img_tag_in_description: Images are not allowed faker: address: country_code: diff --git a/config/locales/fr.yml b/config/locales/fr.yml index ff9731cffb..b61d3b5ba4 100644 --- a/config/locales/fr.yml +++ b/config/locales/fr.yml @@ -220,6 +220,8 @@ fr: links: forgot_your_password: Mot de passe oublié ? sign_in_with_france_connect: FranceConnect + errors: + no_img_tag_in_description: Les images ne sont pas autorisées faker: address: country_code: 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..2d9e4c6fbc --- /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_img_tag_in_description + + private + + def no_img_tag_in_description + errors.add :description, I18n.t("errors.no_img_tag_in_description") if description =~ /(<img| Date: Mon, 30 Sep 2024 11:40:55 +0200 Subject: [PATCH 02/12] test: add tests for new validation --- spec/forms/initiative_form_spec.rb | 52 ++++++++++++++++++++++++++ spec/models/decidim/initiative_spec.rb | 37 ++++++++++++++++++ 2 files changed, 89 insertions(+) create mode 100644 spec/forms/initiative_form_spec.rb create mode 100644 spec/models/decidim/initiative_spec.rb diff --git a/spec/forms/initiative_form_spec.rb b/spec/forms/initiative_form_spec.rb new file mode 100644 index 0000000000..3a40658a80 --- /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 an img tag" do + let(:my_description) { '

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

' } + + it { is_expected.to be_invalid } + end + end + end +end diff --git a/spec/models/decidim/initiative_spec.rb b/spec/models/decidim/initiative_spec.rb new file mode 100644 index 0000000000..a976a65342 --- /dev/null +++ b/spec/models/decidim/initiative_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require "spec_helper" + +module Decidim + describe Initiative do + subject { initiative } + + let(:organization) { create(:organization) } + + context "when validating an initiative" do + context "and description contains an img tag" do + let(:validating_initiative) do + build(:initiative, + description: { en: 'description' }, + state: "validating") + end + + it "is not valid" do + expect(validating_initiative).not_to be_valid + end + end + + context "and description is valid" do + let(:validating_initiative) do + build(:initiative, + description: { en: "description" }, + state: "validating") + end + + it "is valid" do + expect(validating_initiative).to be_valid + end + end + end + end +end From 96a975b1a10f173f8b85355bdd5f058c3690aa34 Mon Sep 17 00:00:00 2001 From: stephanie rousset Date: Mon, 30 Sep 2024 12:16:24 +0200 Subject: [PATCH 03/12] docs: update overrides section --- OVERLOADS.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/OVERLOADS.md b/OVERLOADS.md index 95d0041245..951e389059 100644 --- a/OVERLOADS.md +++ b/OVERLOADS.md @@ -2,6 +2,11 @@ * `app/cells/decidim/version_cell.rb` This override the default `VersionCell` from `decidim-core`, by adding sanitization for `version_number` to prevent XSS attacks. +## Initiative XSS vulnerability +* `lib/extends/models/decidim/initiative_extends.rb` +* `lib/extends/forms/decidim/admin/initiative_form_extends.rb` +This adds a validation to prevent XSS vulnerability to img tag in description. + ## Proposal's draft (Decidim awesome overrides 0.26.7) * `app/views/decidim/proposals/collaborative_drafts/_edit_form_fields.html.erb` From baa23616f6d509aa3f25ed34a48f5aaedc6b2587 Mon Sep 17 00:00:00 2001 From: stephanie rousset Date: Mon, 30 Sep 2024 12:48:45 +0200 Subject: [PATCH 04/12] fix: interference from added extends with migration --- config/application.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/config/application.rb b/config/application.rb index d754fd38ac..b99f3a7e6d 100644 --- a/config/application.rb +++ b/config/application.rb @@ -57,8 +57,12 @@ 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" - require "extends/models/decidim/initiative_extends" + + if ActiveRecord::Base.connection.table_exists?(:decidim_initiatives) + require "extends/forms/decidim/initiatives/initiative_form_extends" + require "extends/models/decidim/initiative_extends" + end + 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 From 441a0cc4452e5199bc8c78e06527f52ecdaab73d Mon Sep 17 00:00:00 2001 From: stephanie rousset Date: Mon, 30 Sep 2024 12:59:27 +0200 Subject: [PATCH 05/12] style: update with rubocop --- config/application.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/config/application.rb b/config/application.rb index b99f3a7e6d..01077f3b63 100644 --- a/config/application.rb +++ b/config/application.rb @@ -63,7 +63,6 @@ class Application < Rails::Application require "extends/models/decidim/initiative_extends" end - 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 end From a02f61d0eb68fdc82ced4e82b1fd71412e87700b Mon Sep 17 00:00:00 2001 From: stephanie rousset Date: Mon, 30 Sep 2024 13:53:57 +0200 Subject: [PATCH 06/12] fix: ActiveRecord::NoDatabaseError --- config/application.rb | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/config/application.rb b/config/application.rb index 01077f3b63..efca29352b 100644 --- a/config/application.rb +++ b/config/application.rb @@ -58,7 +58,15 @@ class Application < Rails::Application require "extends/commands/decidim/admin/destroy_participatory_space_private_user_extends" require "extends/controllers/decidim/proposals/proposals_controller_extends" - if ActiveRecord::Base.connection.table_exists?(:decidim_initiatives) + def database_exists? + ActiveRecord::Base.connection + rescue ActiveRecord::NoDatabaseError + false + else + true + end + + if database_exists? && ActiveRecord::Base.connection.table_exists?(:decidim_initiatives) require "extends/forms/decidim/initiatives/initiative_form_extends" require "extends/models/decidim/initiative_extends" end From d97f3b43061309204235ff4e37c9f8f580b1f808 Mon Sep 17 00:00:00 2001 From: stephanie rousset Date: Mon, 30 Sep 2024 14:17:39 +0200 Subject: [PATCH 07/12] fix: trying to fix again interference --- config/application.rb | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/config/application.rb b/config/application.rb index efca29352b..faefdfaa5f 100644 --- a/config/application.rb +++ b/config/application.rb @@ -37,6 +37,13 @@ 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. + def database_exists? + ActiveRecord::Base.connection + rescue ActiveRecord::NoDatabaseError + false + else + true + end config.to_prepare do require "extends/helpers/decidim/forms/application_helper_extends" @@ -58,14 +65,6 @@ class Application < Rails::Application require "extends/commands/decidim/admin/destroy_participatory_space_private_user_extends" require "extends/controllers/decidim/proposals/proposals_controller_extends" - def database_exists? - ActiveRecord::Base.connection - rescue ActiveRecord::NoDatabaseError - false - else - true - end - if database_exists? && ActiveRecord::Base.connection.table_exists?(:decidim_initiatives) require "extends/forms/decidim/initiatives/initiative_form_extends" require "extends/models/decidim/initiative_extends" From ef3a9eb9bb59e6ee35be0bdbf0b2edfea2b441d0 Mon Sep 17 00:00:00 2001 From: stephanie rousset Date: Thu, 3 Oct 2024 17:32:27 +0200 Subject: [PATCH 08/12] fix: update initiative fomr extends and modify admin initiative controller --- .../admin/initiatives_controller.rb | 2 + config/application.rb | 14 +- config/locales/en.yml | 2 - config/locales/fr.yml | 2 - .../initiatives/initiative_form_extends.rb | 6 +- .../models/decidim/initiative_extends.rb | 18 - .../admin/initiatives_controller_spec.rb | 632 ++++++++++++++++++ spec/models/decidim/initiative_spec.rb | 37 - 8 files changed, 638 insertions(+), 75 deletions(-) delete mode 100644 lib/extends/models/decidim/initiative_extends.rb create mode 100644 spec/controllers/decidim/initiatives/admin/initiatives_controller_spec.rb delete mode 100644 spec/models/decidim/initiative_spec.rb diff --git a/app/controllers/decidim/initiatives/admin/initiatives_controller.rb b/app/controllers/decidim/initiatives/admin/initiatives_controller.rb index 06fbfba3e2..be4d7c34b6 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.class == String ? v.gsub(/on\w+=("|')/, "nothing") : v } render layout: "decidim/admin/initiative" end diff --git a/config/application.rb b/config/application.rb index faefdfaa5f..2ea5e51ef6 100644 --- a/config/application.rb +++ b/config/application.rb @@ -37,14 +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. - def database_exists? - ActiveRecord::Base.connection - rescue ActiveRecord::NoDatabaseError - false - else - true - end - config.to_prepare do require "extends/helpers/decidim/forms/application_helper_extends" require "extends/cells/decidim/forms/step_navigation_cell_extends" @@ -64,11 +56,7 @@ def database_exists? 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" - - if database_exists? && ActiveRecord::Base.connection.table_exists?(:decidim_initiatives) - require "extends/forms/decidim/initiatives/initiative_form_extends" - require "extends/models/decidim/initiative_extends" - end + 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/config/locales/en.yml b/config/locales/en.yml index fc2460a52f..8a50f0b69b 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -218,8 +218,6 @@ en: links: forgot_your_password: Forgot your password sign_in_with_france_connect: Sign in with france connect - errors: - no_img_tag_in_description: Images are not allowed faker: address: country_code: diff --git a/config/locales/fr.yml b/config/locales/fr.yml index b61d3b5ba4..ff9731cffb 100644 --- a/config/locales/fr.yml +++ b/config/locales/fr.yml @@ -220,8 +220,6 @@ fr: links: forgot_your_password: Mot de passe oublié ? sign_in_with_france_connect: FranceConnect - errors: - no_img_tag_in_description: Les images ne sont pas autorisées faker: address: country_code: diff --git a/lib/extends/forms/decidim/initiatives/initiative_form_extends.rb b/lib/extends/forms/decidim/initiatives/initiative_form_extends.rb index 2d9e4c6fbc..90b6a87669 100644 --- a/lib/extends/forms/decidim/initiatives/initiative_form_extends.rb +++ b/lib/extends/forms/decidim/initiatives/initiative_form_extends.rb @@ -6,12 +6,12 @@ module NoAdminInitiativeFormExtends extend ActiveSupport::Concern included do - validate :no_img_tag_in_description + validate :no_javascript_event_in_description private - def no_img_tag_in_description - errors.add :description, I18n.t("errors.no_img_tag_in_description") if description =~ /(<img|' }) } + + 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/models/decidim/initiative_spec.rb b/spec/models/decidim/initiative_spec.rb deleted file mode 100644 index a976a65342..0000000000 --- a/spec/models/decidim/initiative_spec.rb +++ /dev/null @@ -1,37 +0,0 @@ -# frozen_string_literal: true - -require "spec_helper" - -module Decidim - describe Initiative do - subject { initiative } - - let(:organization) { create(:organization) } - - context "when validating an initiative" do - context "and description contains an img tag" do - let(:validating_initiative) do - build(:initiative, - description: { en: 'description' }, - state: "validating") - end - - it "is not valid" do - expect(validating_initiative).not_to be_valid - end - end - - context "and description is valid" do - let(:validating_initiative) do - build(:initiative, - description: { en: "description" }, - state: "validating") - end - - it "is valid" do - expect(validating_initiative).to be_valid - end - end - end - end -end From 2e9edd0117a80d9c6a192153473341c95a618c10 Mon Sep 17 00:00:00 2001 From: stephanie rousset Date: Thu, 3 Oct 2024 17:33:16 +0200 Subject: [PATCH 09/12] refactor: update with rubocop --- .../decidim/initiatives/admin/initiatives_controller.rb | 2 +- .../decidim/initiatives/admin/initiatives_controller_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/decidim/initiatives/admin/initiatives_controller.rb b/app/controllers/decidim/initiatives/admin/initiatives_controller.rb index be4d7c34b6..2f4a2a6ec8 100644 --- a/app/controllers/decidim/initiatives/admin/initiatives_controller.rb +++ b/app/controllers/decidim/initiatives/admin/initiatives_controller.rb @@ -35,7 +35,7 @@ def edit ) @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.class == String ? v.gsub(/on\w+=("|')/, "nothing") : v } + @form.description.transform_values! { |v| v.instance_of?(String) ? v.gsub(/on\w+=("|')/, "nothing") : v } render layout: "decidim/admin/initiative" end diff --git a/spec/controllers/decidim/initiatives/admin/initiatives_controller_spec.rb b/spec/controllers/decidim/initiatives/admin/initiatives_controller_spec.rb index 994ea8f56b..f76c91dd4d 100644 --- a/spec/controllers/decidim/initiatives/admin/initiatives_controller_spec.rb +++ b/spec/controllers/decidim/initiatives/admin/initiatives_controller_spec.rb @@ -622,7 +622,7 @@ 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} + get :export, params: { format: :csv, collection_ids: collection_ids } expect(flash[:alert]).to be_nil expect(response).to have_http_status(:found) end From 32e069bd4c8090b92d18a9ec9e3a60687213c76f Mon Sep 17 00:00:00 2001 From: stephanie rousset Date: Thu, 3 Oct 2024 17:52:52 +0200 Subject: [PATCH 10/12] fix: validation in initiative_form extends and update test --- .../forms/decidim/initiatives/initiative_form_extends.rb | 2 +- spec/forms/initiative_form_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/extends/forms/decidim/initiatives/initiative_form_extends.rb b/lib/extends/forms/decidim/initiatives/initiative_form_extends.rb index 90b6a87669..9918c015a2 100644 --- a/lib/extends/forms/decidim/initiatives/initiative_form_extends.rb +++ b/lib/extends/forms/decidim/initiatives/initiative_form_extends.rb @@ -11,7 +11,7 @@ module NoAdminInitiativeFormExtends private def no_javascript_event_in_description - errors.add :description, :invalid if description =~ /on\w+=("|')/ + errors.add :description, :invalid if description =~ /on\w+=/ end end end diff --git a/spec/forms/initiative_form_spec.rb b/spec/forms/initiative_form_spec.rb index 3a40658a80..cc412c774e 100644 --- a/spec/forms/initiative_form_spec.rb +++ b/spec/forms/initiative_form_spec.rb @@ -42,7 +42,7 @@ module Initiatives it { is_expected.to be_valid } end - context "when description contains an img tag" do + context "when description contains a javascript event" do let(:my_description) { '

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

' } it { is_expected.to be_invalid } From 775bcb5c54420e6d8ec72acb3166d656dd53dd1a Mon Sep 17 00:00:00 2001 From: stephanie rousset Date: Thu, 3 Oct 2024 17:53:47 +0200 Subject: [PATCH 11/12] docs: update overrides section in overloads.md --- OVERLOADS.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/OVERLOADS.md b/OVERLOADS.md index 951e389059..f5fc373654 100644 --- a/OVERLOADS.md +++ b/OVERLOADS.md @@ -3,9 +3,8 @@ This override the default `VersionCell` from `decidim-core`, by adding sanitization for `version_number` to prevent XSS attacks. ## Initiative XSS vulnerability -* `lib/extends/models/decidim/initiative_extends.rb` -* `lib/extends/forms/decidim/admin/initiative_form_extends.rb` -This adds a validation to prevent XSS vulnerability to img tag in description. +* `lib/extends/forms/decidim/initiatives/initiative_form_extends.rb` +This adds a validation to prevent XSS vulnerability in description. ## Proposal's draft (Decidim awesome overrides 0.26.7) * `app/views/decidim/proposals/collaborative_drafts/_edit_form_fields.html.erb` From 8668d3d953367e46abfae6234aeb859fd8741c51 Mon Sep 17 00:00:00 2001 From: Quentin Champenois <26109239+Quentinchampenois@users.noreply.github.com> Date: Mon, 7 Oct 2024 10:43:57 +0200 Subject: [PATCH 12/12] fix: Update OVERLOADS.md --- OVERLOADS.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/OVERLOADS.md b/OVERLOADS.md index f5fc373654..5d341953aa 100644 --- a/OVERLOADS.md +++ b/OVERLOADS.md @@ -2,9 +2,9 @@ * `app/cells/decidim/version_cell.rb` This override the default `VersionCell` from `decidim-core`, by adding sanitization for `version_number` to prevent XSS attacks. -## Initiative XSS vulnerability +## Initiative form * `lib/extends/forms/decidim/initiatives/initiative_form_extends.rb` -This adds a validation to prevent XSS vulnerability in description. +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`