From 20ca6beb30b9771c27e7b8fc0528007cf02290ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 2 Oct 2019 02:24:25 +0200 Subject: [PATCH 01/24] Remove unneeded `html_safe` and `raw` calls There's no HTML in these texts, or it has already been escaped by Rails `link_to` helper method. --- app/controllers/direct_uploads_controller.rb | 2 +- .../_select_investment.html.erb | 2 +- ...new_actions_notification_on_published.html.erb | 2 +- app/views/kaminari/_first_page.html.erb | 2 +- app/views/kaminari/_last_page.html.erb | 2 +- app/views/kaminari/_next_page.html.erb | 2 +- app/views/kaminari/_prev_page.html.erb | 2 +- app/views/layouts/_footer.html.erb | 2 +- app/views/layouts/_notification_item.html.erb | 4 ++-- spec/features/xss_spec.rb | 15 +++++++++++++++ 10 files changed, 25 insertions(+), 10 deletions(-) create mode 100644 spec/features/xss_spec.rb diff --git a/app/controllers/direct_uploads_controller.rb b/app/controllers/direct_uploads_controller.rb index d1d52eb4c..425c43200 100644 --- a/app/controllers/direct_uploads_controller.rb +++ b/app/controllers/direct_uploads_controller.rb @@ -17,7 +17,7 @@ class DirectUploadsController < ApplicationController render json: { cached_attachment: @direct_upload.relation.cached_attachment, filename: @direct_upload.relation.attachment.original_filename, - destroy_link: render_destroy_upload_link(@direct_upload).html_safe, + destroy_link: render_destroy_upload_link(@direct_upload), attachment_url: @direct_upload.relation.attachment.url } else @direct_upload.destroy_attachment diff --git a/app/views/admin/budget_investments/_select_investment.html.erb b/app/views/admin/budget_investments/_select_investment.html.erb index 793e05c7c..b293894b2 100644 --- a/app/views/admin/budget_investments/_select_investment.html.erb +++ b/app/views/admin/budget_investments/_select_investment.html.erb @@ -31,7 +31,7 @@ <% valuators = [investment.assigned_valuation_groups, investment.assigned_valuators].compact %> <% no_valuators_assigned = t("admin.budget_investments.index.no_valuators_assigned") %> - <%= raw valuators.present? ? valuators.join(", ") : no_valuators_assigned %> + <%= valuators.present? ? valuators.join(", ") : no_valuators_assigned %> diff --git a/app/views/dashboard/mailer/new_actions_notification_on_published.html.erb b/app/views/dashboard/mailer/new_actions_notification_on_published.html.erb index bae02cd64..e566ebd03 100644 --- a/app/views/dashboard/mailer/new_actions_notification_on_published.html.erb +++ b/app/views/dashboard/mailer/new_actions_notification_on_published.html.erb @@ -36,7 +36,7 @@ <% end %> diff --git a/app/views/kaminari/_first_page.html.erb b/app/views/kaminari/_first_page.html.erb index e8afb0431..a5335a30b 100644 --- a/app/views/kaminari/_first_page.html.erb +++ b/app/views/kaminari/_first_page.html.erb @@ -1,3 +1,3 @@
  • - <%= link_to t("views.pagination.first").html_safe, kaminari_path(url), :remote => remote %> + <%= link_to t("views.pagination.first"), kaminari_path(url), :remote => remote %>
    • diff --git a/app/views/kaminari/_last_page.html.erb b/app/views/kaminari/_last_page.html.erb index 5a49bd7e2..697b3bd15 100644 --- a/app/views/kaminari/_last_page.html.erb +++ b/app/views/kaminari/_last_page.html.erb @@ -1,3 +1,3 @@
  • - <%= link_to t("views.pagination.last").html_safe, kaminari_path(url), :remote => remote %> + <%= link_to t("views.pagination.last"), kaminari_path(url), :remote => remote %>
    • diff --git a/app/views/kaminari/_next_page.html.erb b/app/views/kaminari/_next_page.html.erb index 11c700900..366367031 100644 --- a/app/views/kaminari/_next_page.html.erb +++ b/app/views/kaminari/_next_page.html.erb @@ -1,3 +1,3 @@
  • - <%= link_to t("views.pagination.next").html_safe, kaminari_path(url), :rel => "next", :remote => remote %> + <%= link_to t("views.pagination.next"), kaminari_path(url), :rel => "next", :remote => remote %>
    • diff --git a/app/views/kaminari/_prev_page.html.erb b/app/views/kaminari/_prev_page.html.erb index aba1d9369..d0147ff5c 100644 --- a/app/views/kaminari/_prev_page.html.erb +++ b/app/views/kaminari/_prev_page.html.erb @@ -1,3 +1,3 @@
  • - <%= link_to t("views.pagination.previous").html_safe, kaminari_path(url), :rel => "prev", :remote => remote %> + <%= link_to t("views.pagination.previous"), kaminari_path(url), :rel => "prev", :remote => remote %>
    • diff --git a/app/views/layouts/_footer.html.erb b/app/views/layouts/_footer.html.erb index 1337be64b..ebd7dc990 100644 --- a/app/views/layouts/_footer.html.erb +++ b/app/views/layouts/_footer.html.erb @@ -2,7 +2,7 @@

    - <%= link_to t("layouts.header.open_gov", open: "#{t("layouts.header.open")}").html_safe %> + <%= link_to t("layouts.header.open_gov", open: t("layouts.header.open")), root_path %>

    diff --git a/app/views/layouts/_notification_item.html.erb b/app/views/layouts/_notification_item.html.erb index 7a21a3c38..c766f862a 100644 --- a/app/views/layouts/_notification_item.html.erb +++ b/app/views/layouts/_notification_item.html.erb @@ -10,11 +10,11 @@ <%= t("layouts.header.notification_item.new_notifications", - count: current_user.notifications_count).html_safe %> + count: current_user.notifications_count) %> <% else %>

    - <%= t("budgets.ballots.show.voted_info_html") %> + <%= t("budgets.ballots.show.voted_info") %>

    <%= t("budgets.ballots.show.voted_info_2") %>

    diff --git a/app/views/debates/new.html.erb b/app/views/debates/new.html.erb index cf3cbc7b9..4e9368c2d 100644 --- a/app/views/debates/new.html.erb +++ b/app/views/debates/new.html.erb @@ -9,7 +9,7 @@ info_link: link_to(t("debates.new.info_link"), new_proposal_path)).html_safe %> <% if feature?(:help_page) %> - <%= link_to help_path, title: t("shared.target_blank_html"), target: "_blank" do %> + <%= link_to help_path, title: t("shared.target_blank"), target: "_blank" do %> <%= t("debates.new.more_info") %> <% end %> <% end %> diff --git a/app/views/layouts/_footer.html.erb b/app/views/layouts/_footer.html.erb index ebd7dc990..160134d8e 100644 --- a/app/views/layouts/_footer.html.erb +++ b/app/views/layouts/_footer.html.erb @@ -38,7 +38,7 @@ <% if setting["twitter_handle"] %>
  • <%= link_to "https://twitter.com/#{setting["twitter_handle"]}", target: "_blank", - title: t("shared.go_to_page") + t("social.twitter", org: setting["org_name"]) + t("shared.target_blank_html") do %> + title: t("shared.go_to_page") + t("social.twitter", org: setting["org_name"]) + t("shared.target_blank") do %> <%= t("social.twitter", org: setting["org_name"]) %> <% end %> @@ -47,7 +47,7 @@ <% if setting["facebook_handle"] %>
  • <%= link_to "https://www.facebook.com/#{setting["facebook_handle"]}/", target: "_blank", - title: t("shared.go_to_page") + t("social.facebook", org: setting["org_name"]) + t("shared.target_blank_html") do %> + title: t("shared.go_to_page") + t("social.facebook", org: setting["org_name"]) + t("shared.target_blank") do %> <%= t("social.facebook", org: setting["org_name"]) %> <% end %> @@ -56,7 +56,7 @@ <% if setting["youtube_handle"] %>
  • <%= link_to "https://www.youtube.com/#{setting["youtube_handle"]}", target: "_blank", - title: t("shared.go_to_page") + t("social.youtube", org: setting["org_name"]) + t("shared.target_blank_html") do %> + title: t("shared.go_to_page") + t("social.youtube", org: setting["org_name"]) + t("shared.target_blank") do %> <%= t("social.youtube", org: setting["org_name"]) %> <% end %> @@ -65,7 +65,7 @@ <% if setting["telegram_handle"] %>
  • <%= link_to "https://www.telegram.me/#{setting["telegram_handle"]}", target: "_blank", - title: t("shared.go_to_page") + t("social.telegram", org: setting["org_name"]) + t("shared.target_blank_html") do %> + title: t("shared.go_to_page") + t("social.telegram", org: setting["org_name"]) + t("shared.target_blank") do %> <%= t("social.telegram", org: setting["org_name"]) %> <% end %> @@ -74,7 +74,7 @@ <% if setting["instagram_handle"] %>
  • <%= link_to "https://www.instagram.com/#{setting["instagram_handle"]}", target: "_blank", - title: t("shared.go_to_page") + t("social.instagram", org: setting["org_name"]) + t("shared.target_blank_html") do %> + title: t("shared.go_to_page") + t("social.instagram", org: setting["org_name"]) + t("shared.target_blank") do %> <%= t("social.instagram", org: setting["org_name"]) %> <% end %> diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index e7a5480ec..e9662d726 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -32,9 +32,9 @@

    <%= t("layouts.application.ie", chrome: link_to( - t("layouts.application.chrome"), "https://www.google.com/chrome/browser/desktop/", title: t("shared.target_blank_html"), target: "_blank"), + t("layouts.application.chrome"), "https://www.google.com/chrome/browser/desktop/", title: t("shared.target_blank"), target: "_blank"), firefox: link_to( - t("layouts.application.firefox"), "https://www.mozilla.org/firefox", title: t("shared.target_blank_html"), target: "_blank") + t("layouts.application.firefox"), "https://www.mozilla.org/firefox", title: t("shared.target_blank"), target: "_blank") ).html_safe %>

    • diff --git a/app/views/organizations/registrations/new.html.erb b/app/views/organizations/registrations/new.html.erb index 705f27fde..f9100fec6 100644 --- a/app/views/organizations/registrations/new.html.erb +++ b/app/views/organizations/registrations/new.html.erb @@ -32,7 +32,7 @@ label: t("devise_views.users.registrations.new.terms", terms: link_to(t("devise_views.users.registrations.new.terms_link"), "/conditions", - title: t("shared.target_blank_html"), + title: t("shared.target_blank"), target: "_blank") ).html_safe %> diff --git a/app/views/organizations/registrations/success.html.erb b/app/views/organizations/registrations/success.html.erb index 550f06342..d526133bc 100644 --- a/app/views/organizations/registrations/success.html.erb +++ b/app/views/organizations/registrations/success.html.erb @@ -2,7 +2,7 @@

    <%= t("devise_views.organizations.registrations.success.thank_you_html") %>

    <%= t("devise_views.organizations.registrations.success.instructions_1_html") %>

    <%= t("devise_views.organizations.registrations.success.instructions_2_html") %>

    -

    <%= t("devise_views.organizations.registrations.success.instructions_3_html") %>

    +

    <%= t("devise_views.organizations.registrations.success.instructions_3") %>

    <%= link_to t("devise_views.organizations.registrations.success.back_to_index"), root_path, class: "button margin-top expanded" %> diff --git a/app/views/pages/help/_budgets.html.erb b/app/views/pages/help/_budgets.html.erb index 116bb0deb..0371a038c 100644 --- a/app/views/pages/help/_budgets.html.erb +++ b/app/views/pages/help/_budgets.html.erb @@ -10,7 +10,7 @@

    <%= image_tag "help/budgets_#{I18n.locale}.png", alt: t("pages.help.budgets.image_alt") %> -
    <%= t("pages.help.budgets.figcaption_html") %>
    +
    <%= t("pages.help.budgets.figcaption") %>
    diff --git a/app/views/pages/help/_proposals.html.erb b/app/views/pages/help/_proposals.html.erb index 6cc932242..ececd3b85 100644 --- a/app/views/pages/help/_proposals.html.erb +++ b/app/views/pages/help/_proposals.html.erb @@ -10,7 +10,7 @@
    <%= image_tag "help/proposals_#{I18n.locale}.png", alt: t("pages.help.proposals.image_alt") %> -
    <%= t("pages.help.proposals.figcaption_html") %>
    +
    <%= t("pages.help.proposals.figcaption") %>
    diff --git a/app/views/proposals/new.html.erb b/app/views/proposals/new.html.erb index f7fc3b335..8c1da0eb9 100644 --- a/app/views/proposals/new.html.erb +++ b/app/views/proposals/new.html.erb @@ -5,7 +5,7 @@

    <%= t("proposals.new.start_new") %>

    - <%= link_to help_path(anchor: "proposals"), title: t("shared.target_blank_html"), target: "_blank" do %> + <%= link_to help_path(anchor: "proposals"), title: t("shared.target_blank"), target: "_blank" do %> <%= t("proposals.new.more_info") %> <% end %>
    diff --git a/app/views/users/registrations/new.html.erb b/app/views/users/registrations/new.html.erb index c97b1befb..de50cc265 100644 --- a/app/views/users/registrations/new.html.erb +++ b/app/views/users/registrations/new.html.erb @@ -40,7 +40,7 @@ title: t("devise_views.users.registrations.new.terms_title"), label: t("devise_views.users.registrations.new.terms", terms: link_to(t("devise_views.users.registrations.new.terms_link"), "/conditions", - title: t("shared.target_blank_html"), + title: t("shared.target_blank"), target: "_blank") ).html_safe %> diff --git a/app/views/users/registrations/success.html.erb b/app/views/users/registrations/success.html.erb index 900e29557..e622aeabe 100644 --- a/app/views/users/registrations/success.html.erb +++ b/app/views/users/registrations/success.html.erb @@ -1,7 +1,7 @@

    <%= t("devise_views.users.registrations.success.title") %>

    <%= t("devise_views.users.registrations.success.thank_you_html") %>

    <%= t("devise_views.users.registrations.success.instructions_1_html") %>

    -

    <%= t("devise_views.users.registrations.success.instructions_2_html") %>

    +

    <%= t("devise_views.users.registrations.success.instructions_2") %>

    <%= link_to t("devise_views.users.registrations.success.back_to_index"), root_path, class: "button margin-top expanded" %> diff --git a/app/views/verification/residence/new.html.erb b/app/views/verification/residence/new.html.erb index cb77a4f46..6933f94f9 100644 --- a/app/views/verification/residence/new.html.erb +++ b/app/views/verification/residence/new.html.erb @@ -78,7 +78,7 @@ title: t("verification.residence.new.accept_terms_text_title"), label: t("verification.residence.new.accept_terms_text", terms_url: link_to(t("verification.residence.new.terms"), "/census_terms", - title: t("shared.target_blank_html"), + title: t("shared.target_blank"), target: "_blank") ).html_safe %> diff --git a/config/locales/en/budgets.yml b/config/locales/en/budgets.yml index 92fd1b8bd..7871f5a61 100644 --- a/config/locales/en/budgets.yml +++ b/config/locales/en/budgets.yml @@ -10,7 +10,7 @@ en: voted_html: one: "You have voted one investment." other: "You have voted %{count} investments." - voted_info_html: "Your ballot is confirmed!" + voted_info: "Your ballot is confirmed!" voted_info_2: "But you can change your vote at any time until this phase is closed." zero: You have not voted any investment project. reasons_for_not_balloting: diff --git a/config/locales/en/devise_views.yml b/config/locales/en/devise_views.yml index 41ceae937..2840069d7 100644 --- a/config/locales/en/devise_views.yml +++ b/config/locales/en/devise_views.yml @@ -52,7 +52,7 @@ en: back_to_index: I understand; go back to main page instructions_1_html: "We will contact you soon to verify that you do in fact represent this collective." instructions_2_html: While your email is reviewed, we have sent you a link to confirm your account. - instructions_3_html: Once confirmed, you may begin to participate as an unverified collective. + instructions_3: Once confirmed, you may begin to participate as an unverified collective. thank_you_html: Thank you for registering your collective on the website. It is now pending verification. title: Registration of organisation / collective passwords: @@ -124,6 +124,6 @@ en: success: back_to_index: I understand; go back to main page instructions_1_html: Please check your email - we have sent you a link to confirm your account. - instructions_2_html: Once confirmed, you may begin participation. + instructions_2: Once confirmed, you may begin participation. thank_you_html: Thank you for registering for the website. You must now confirm your email address. title: Confirm your email address diff --git a/config/locales/en/general.yml b/config/locales/en/general.yml index fcdba9cca..105cbf783 100644 --- a/config/locales/en/general.yml +++ b/config/locales/en/general.yml @@ -773,7 +773,7 @@ en: districts: "Districts" districts_list: "Districts list" categories: "Categories" - target_blank_html: " (link opens in new window)" + target_blank: " (link opens in new window)" you_are_in: "You are in" unflag: Unflag unfollow_entity: "Unfollow %{entity}" diff --git a/config/locales/en/pages.yml b/config/locales/en/pages.yml index 4cc92db43..abe473675 100644 --- a/config/locales/en/pages.yml +++ b/config/locales/en/pages.yml @@ -27,13 +27,13 @@ en: description: "In the %{link} section you can make proposals for the City Council to carry them out. The proposals require support, and if they reach sufficient support, they are put to a public vote. The proposals approved in these citizens' votes are accepted by the City Council and carried out." link: "citizen proposals" image_alt: "Button to support a proposal" - figcaption_html: 'Button to "Support" a proposal.' + figcaption: 'Button to "Support" a proposal.' budgets: title: "Participatory Budgeting" description: "The %{link} section helps people make a direct decision on what part of the municipal budget is spent on." link: "participative budgets" image_alt: "Different phases of a participatory budget" - figcaption_html: '"Support" and "Voting" phases of participatory budgets.' + figcaption: '"Support" and "Voting" phases of participatory budgets.' polls: title: "Polls" description: "The %{link} section is activated each time a proposal reaches 1% support and goes to the vote or when the City Council proposes an issue for people to decide on." diff --git a/config/locales/es/budgets.yml b/config/locales/es/budgets.yml index 16ed2b4c0..a73292b76 100644 --- a/config/locales/es/budgets.yml +++ b/config/locales/es/budgets.yml @@ -10,7 +10,7 @@ es: voted_html: one: "Has votado un proyecto." other: "Has votado %{count} proyectos." - voted_info_html: "¡Tus votos están confirmados!" + voted_info: "¡Tus votos están confirmados!" voted_info_2: "Pero puedes cambiarlos en cualquier momento hasta el cierre de esta fase." zero: Todavía no has votado ningún proyecto de gasto. reasons_for_not_balloting: diff --git a/config/locales/es/devise_views.yml b/config/locales/es/devise_views.yml index 9e2581d9a..5134b29e2 100644 --- a/config/locales/es/devise_views.yml +++ b/config/locales/es/devise_views.yml @@ -52,7 +52,7 @@ es: back_to_index: Entendido, volver a la página principal instructions_1_html: "En breve nos pondremos en contacto contigo para verificar que realmente representas a este colectivo." instructions_2_html: Mientras revisa tu correo electrónico, te hemos enviado un enlace para confirmar tu cuenta. - instructions_3_html: Una vez confirmado, podrás empezar a participar como colectivo no verificado. + instructions_3: Una vez confirmado, podrás empezar a participar como colectivo no verificado. thank_you_html: Gracias por registrar tu colectivo en la web. Ahora está pendiente de verificación. title: Registro de organización / colectivo passwords: @@ -124,6 +124,6 @@ es: success: back_to_index: Entendido, volver a la página principal instructions_1_html: Por favor revisa tu correo electrónico - te hemos enviado un enlace para confirmar tu cuenta. - instructions_2_html: Una vez confirmado, podrás empezar a participar. + instructions_2: Una vez confirmado, podrás empezar a participar. thank_you_html: Gracias por registrarte en la web. Ahora debes confirmar tu correo. title: Revisa tu correo diff --git a/config/locales/es/general.yml b/config/locales/es/general.yml index 680e12920..eb25dd7fd 100644 --- a/config/locales/es/general.yml +++ b/config/locales/es/general.yml @@ -770,7 +770,7 @@ es: districts: "Distritos" districts_list: "Listado de distritos" categories: "Categorías" - target_blank_html: " (se abre en ventana nueva)" + target_blank: " (se abre en ventana nueva)" you_are_in: "Estás en" unflag: Deshacer denuncia unfollow_entity: "Dejar de seguir %{entity}" diff --git a/config/locales/es/pages.yml b/config/locales/es/pages.yml index 33835a1e3..333366f65 100644 --- a/config/locales/es/pages.yml +++ b/config/locales/es/pages.yml @@ -27,13 +27,13 @@ es: description: "En la sección de %{link} puedes plantear propuestas para que el Ayuntamiento las lleve a cabo. Las propuestas recaban apoyos, y si alcanzan los apoyos suficientes se someten a votación ciudadana. Las propuestas aprobadas en estas votaciones ciudadanas son asumidas por el Ayuntamiento y se llevan a cabo." link: "propuestas ciudadanas" image_alt: "Botón para apoyar una propuesta" - figcaption_html: 'Botón para "Apoyar" una propuesta.' + figcaption: 'Botón para "Apoyar" una propuesta.' budgets: title: "Presupuestos participativos" description: "La sección de %{link} sirve para que la gente decida de manera directa a qué se destina una parte del presupuesto municipal." link: "presupuestos participativos" image_alt: "Diferentes fases de un presupuesto participativo" - figcaption_html: 'Fase de "Apoyos" y fase de "Votación" de los presupuestos participativos.' + figcaption: 'Fase de "Apoyos" y fase de "Votación" de los presupuestos participativos.' polls: title: "Votaciones" description: "La sección de %{link} se activa cada vez que una propuesta alcanza el 1% de apoyos y pasa a votación o cuando el Ayuntamiento propone un tema para que la gente decida sobre él." From eb16b9df48093cb04d230d7bbcaf1c5a161143b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 2 Oct 2019 16:28:24 +0200 Subject: [PATCH 04/24] Remove unneded html_safe in investment description The description is already marked as HTML safe because we sanitize it before storing it in the database. --- app/views/budgets/investments/_investment_detail.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/budgets/investments/_investment_detail.erb b/app/views/budgets/investments/_investment_detail.erb index 6c49abe38..de1a7be07 100644 --- a/app/views/budgets/investments/_investment_detail.erb +++ b/app/views/budgets/investments/_investment_detail.erb @@ -22,7 +22,7 @@ <%= t("budgets.investments.show.code_html", code: investment.id) %>

    -<%= safe_html_with_links investment.description.html_safe %> +<%= safe_html_with_links investment.description %> <% if feature?(:map) && map_location_available?(@investment.map_location) %>
    From db1ccb18c7bd48fdd5ab63d6ad0259974a0a557a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 1 Oct 2019 18:50:17 +0200 Subject: [PATCH 05/24] Use `safe_join` instead of `html_safe` The name `html_safe` is very confusing, and many developers (including me a few years ago) think what that method does is convert the HTML contents to safe content. It's actually quite the opposite: it marks the string as safe, so the HTML inside it isn't stripped out by Rails. In some cases we were marking strings as safe because we wanted to add some HTML. However, it meant the whole string was considered safe, and not just the contents which were under our control. In particular, some translations added by admins to the database or through crowding were marked as safe, when it wasn't necessarily the case. Although AFAIK crowdin checks for potential cross-site scripting attacks, it's a good practice to sanitize parts of a string potentially out of our control before marking the string as HTML safe. --- app/helpers/budget_investments_helper.rb | 2 +- app/helpers/documents_helper.rb | 11 ++++++----- app/helpers/translatable_form_helper.rb | 4 ++-- spec/features/xss_spec.rb | 18 ++++++++++++++++++ 4 files changed, 27 insertions(+), 8 deletions(-) diff --git a/app/helpers/budget_investments_helper.rb b/app/helpers/budget_investments_helper.rb index bcfdf0f49..1e6d8405e 100644 --- a/app/helpers/budget_investments_helper.rb +++ b/app/helpers/budget_investments_helper.rb @@ -10,7 +10,7 @@ module BudgetInvestmentsHelper translation = t("admin.budget_investments.index.list.#{column}") link_to( - "#{translation} ".html_safe, + safe_join([translation, content_tag(:span, "", class: "icon-sortable #{icon}")]), admin_budget_budget_investments_path(sort_by: column, direction: direction) ) end diff --git a/app/helpers/documents_helper.rb b/app/helpers/documents_helper.rb index a096f7346..1386796bb 100644 --- a/app/helpers/documents_helper.rb +++ b/app/helpers/documents_helper.rb @@ -50,10 +50,11 @@ module DocumentsHelper end def document_item_link(document) - link_to "#{document.title} (#{document.humanized_content_type} | \ - #{number_to_human_size(document.attachment_file_size)})".html_safe, - document.attachment.url, - target: "_blank", - title: t("shared.target_blank") + info_text = "#{document.humanized_content_type} | #{number_to_human_size(document.attachment_file_size)}" + + link_to safe_join([document.title, content_tag(:small, "(#{info_text})")], " "), + document.attachment.url, + target: "_blank", + title: t("shared.target_blank") end end diff --git a/app/helpers/translatable_form_helper.rb b/app/helpers/translatable_form_helper.rb index 92eff3d33..78ce9566d 100644 --- a/app/helpers/translatable_form_helper.rb +++ b/app/helpers/translatable_form_helper.rb @@ -26,9 +26,9 @@ module TranslatableFormHelper visible_locales.map do |locale| @translations[locale] = translation_for(locale) end - visible_locales.map do |locale| + safe_join(visible_locales.map do |locale| Globalize.with_locale(locale) { fields_for_locale(locale, &block) } - end.join.html_safe + end) end private diff --git a/spec/features/xss_spec.rb b/spec/features/xss_spec.rb index c99fc6c76..9519c4766 100644 --- a/spec/features/xss_spec.rb +++ b/spec/features/xss_spec.rb @@ -12,4 +12,22 @@ describe "Cross-Site Scripting protection", :js do expect(page.text).not_to be_empty end + + scenario "document title" do + process = create(:legislation_process) + create(:document, documentable: process, title: attack_code) + + visit legislation_process_path(process) + + expect(page.text).not_to be_empty + end + + scenario "hacked translations" do + I18nContent.create(key: "admin.budget_investments.index.list.title", value: attack_code) + + login_as(create(:administrator).user) + visit admin_budget_budget_investments_path(create(:budget_investment).budget) + + expect(page.text).not_to be_empty + end end From 6b12da76541f48cc7c85342d20867aca162ea5ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 5 Oct 2019 18:53:36 +0200 Subject: [PATCH 06/24] Fix ERB being used in an HTML comment This was causing erb-lint to issue a warning. --- app/views/layouts/application.html.erb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index e9662d726..7fd5aee79 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -21,8 +21,8 @@
    <%= render "layouts/header", with_subnavigation: true %> - + <% end %> <%= render "layouts/flash" %> From 60ae224115700772fac2e2c42a9f18c8796ded96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 1 Oct 2019 18:50:31 +0200 Subject: [PATCH 07/24] Use `tag` to mark a `
    ` as safe HTML Using `html_safe` on the whole text meant the translations were also considered HTML safe, but they are not supposed to have HTML. --- app/helpers/signature_sheets_helper.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/helpers/signature_sheets_helper.rb b/app/helpers/signature_sheets_helper.rb index d40689837..20ef2e709 100644 --- a/app/helpers/signature_sheets_helper.rb +++ b/app/helpers/signature_sheets_helper.rb @@ -24,10 +24,10 @@ module SignatureSheetsHelper text_help += t("admin.signature_sheets.new.text_help.postal_code_note") end - text_help += "
    " + text_help += tag(:br) text_help += t("admin.signature_sheets.new.text_help.required_fields_structure_note") - return text_help.html_safe + return text_help end def example_text_help From 368f42f1a2951e401a2c07953c161032e8aa8d1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 5 Oct 2019 23:46:33 +0200 Subject: [PATCH 08/24] Revert `loofah` update We need to update other gems as well if we update this one. Dependabot updated it automatically when updating `foundation_rails_helper`, but it doesn't seem to be necessary. --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index f86ab3ced..788ea82b6 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -313,7 +313,7 @@ GEM actionmailer (>= 3.2) letter_opener (~> 1.0) railties (>= 3.2) - loofah (2.3.0) + loofah (2.2.3) crass (~> 1.0.2) nokogiri (>= 1.5.9) mail (2.7.1) From 0f485308b73193afd3d9a10f2fca3f39d04f5cc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 2 Oct 2019 13:42:48 +0200 Subject: [PATCH 09/24] Sanitize CKEditor content before displaying it It's possible to create a newsletter or a proposed action with