From 0e7c3b4cc06dc8125f69b5ab45a7ca4f02fce8db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 10 Nov 2019 19:05:34 +0100 Subject: [PATCH 1/7] Remove redundant method to set order It was being incorrectly detected as used in a dangerous send. We can get rid of the warning by taking advantage of the `has_orders` method and getting rid of this code. --- app/controllers/communities_controller.rb | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/app/controllers/communities_controller.rb b/app/controllers/communities_controller.rb index 39a95f6b9..42a901f5a 100644 --- a/app/controllers/communities_controller.rb +++ b/app/controllers/communities_controller.rb @@ -1,8 +1,6 @@ class CommunitiesController < ApplicationController - TOPIC_ORDERS = %w[newest most_commented oldest].freeze - before_action :set_order, :set_community, :load_topics, :load_participants - - has_orders TOPIC_ORDERS + has_orders %w[newest most_commented oldest] + before_action :set_community, :load_topics, :load_participants skip_authorization_check @@ -14,26 +12,18 @@ class CommunitiesController < ApplicationController private - def set_order - @order = valid_order? ? params[:order] : "newest" - end - def set_community @community = Community.find(params[:id]) end def load_topics - @topics = @community.topics.send("sort_by_#{@order}").page(params[:page]) + @topics = @community.topics.send("sort_by_#{@current_order}").page(params[:page]) end def load_participants @participants = @community.participants end - def valid_order? - params[:order].present? && TOPIC_ORDERS.include?(params[:order]) - end - def communitable_exists? @community.proposal.present? || @community.investment.present? end From 58157beb01569493ce299f549c8adb5a24a313cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 10 Nov 2019 19:19:42 +0100 Subject: [PATCH 2/7] Add CSRF protection to management controllers --- app/controllers/management/base_controller.rb | 1 + app/controllers/management/sessions_controller.rb | 1 + 2 files changed, 2 insertions(+) diff --git a/app/controllers/management/base_controller.rb b/app/controllers/management/base_controller.rb index a5a6ed593..6d4070e09 100644 --- a/app/controllers/management/base_controller.rb +++ b/app/controllers/management/base_controller.rb @@ -2,6 +2,7 @@ class Management::BaseController < ActionController::Base include GlobalizeFallbacks layout "management" default_form_builder ConsulFormBuilder + protect_from_forgery with: :exception before_action :verify_manager before_action :set_locale diff --git a/app/controllers/management/sessions_controller.rb b/app/controllers/management/sessions_controller.rb index 84d9d1265..d2fdfe3eb 100644 --- a/app/controllers/management/sessions_controller.rb +++ b/app/controllers/management/sessions_controller.rb @@ -4,6 +4,7 @@ class Management::SessionsController < ActionController::Base include GlobalizeFallbacks include AccessDeniedHandler default_form_builder ConsulFormBuilder + protect_from_forgery with: :exception def create destroy_session From d746401862c6fb2276f813993efab61064fc79b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 10 Nov 2019 19:35:54 +0100 Subject: [PATCH 3/7] Avoid a format validation security warning This was actually a false positive, since our new regular expression does the exact same thing. However, false positives generate noise and make it harder to deal with real issues, so I'm changing it anyway. We could add a more advanced regular expression, like `URI::MailTo::EMAIL_REGEXP`. However, this expression marks emails with non-English characters as invalid, when in practice it's possible to have an email address with non-English characters. --- app/models/newsletter.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/newsletter.rb b/app/models/newsletter.rb index be36dd2af..7e8955931 100644 --- a/app/models/newsletter.rb +++ b/app/models/newsletter.rb @@ -3,7 +3,7 @@ class Newsletter < ApplicationRecord validates :subject, presence: true validates :segment_recipient, presence: true - validates :from, presence: true, format: { with: /@/ } + validates :from, presence: true, format: { with: /\A.+@.+\Z/ } validates :body, presence: true validate :validate_segment_recipient From 55d339572c41bb5ae17429ac90ff4a8e875e9258 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 10 Nov 2019 20:36:22 +0100 Subject: [PATCH 4/7] Simplify setting tsvector values We make the code easier to read and at the same time we remove a SQL injection false positive regarding the use of `WHERE id = #{id}`. We still get a warning about SQL injection regarding the `tsv =` part. It's a false positive, since the value of that parameter does not depend on user input. --- app/models/concerns/search_cache.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/models/concerns/search_cache.rb b/app/models/concerns/search_cache.rb index 3ec8bc33f..77dbabb73 100644 --- a/app/models/concerns/search_cache.rb +++ b/app/models/concerns/search_cache.rb @@ -6,8 +6,7 @@ module SearchCache end def calculate_tsvector - ActiveRecord::Base.connection.execute(" - UPDATE #{self.class.table_name} SET tsv = (#{searchable_values_sql}) WHERE id = #{id}") + self.class.where(id: id).update_all("tsv = (#{searchable_values_sql})") end private From 6cb3047da2700a886156f5680afdb0e719a9c2c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 10 Nov 2019 23:10:23 +0100 Subject: [PATCH 5/7] Reuse partial to render a banner --- app/assets/javascripts/banners.js | 10 +++++----- app/views/admin/banners/_form.html.erb | 12 +++--------- app/views/admin/banners/index.html.erb | 7 +------ app/views/shared/_banner.html.erb | 2 +- spec/features/admin/banners_spec.rb | 2 +- 5 files changed, 11 insertions(+), 22 deletions(-) diff --git a/app/assets/javascripts/banners.js b/app/assets/javascripts/banners.js index 9ae732ee4..583015a89 100644 --- a/app/assets/javascripts/banners.js +++ b/app/assets/javascripts/banners.js @@ -4,23 +4,23 @@ initialize: function() { $("[data-js-banner-title]").on({ change: function() { - $("#js-banner-title").text($(this).val()); + $(".banner h2").text($(this).val()); } }); $("[data-js-banner-description]").on({ change: function() { - $("#js-banner-description").text($(this).val()); + $(".banner h3").text($(this).val()); } }); $("[name='banner[background_color]']").on({ change: function() { - $("#js-banner-background").css("background-color", $(this).val()); + $(".banner").css("background-color", $(this).val()); } }); $("[name='banner[font_color]']").on({ change: function() { - $("#js-banner-title").css("color", $(this).val()); - $("#js-banner-description").css("color", $(this).val()); + $(".banner h2").css("color", $(this).val()); + $(".banner h3").css("color", $(this).val()); } }); } diff --git a/app/views/admin/banners/_form.html.erb b/app/views/admin/banners/_form.html.erb index a3589c90e..bf8fc89b7 100644 --- a/app/views/admin/banners/_form.html.erb +++ b/app/views/admin/banners/_form.html.erb @@ -84,15 +84,9 @@
<%= f.submit(class: "button expanded", value: t("admin.banners.edit.form.submit_button")) %>
+ - +
+ <%= render "shared/banner", banner: @banner %>
<% end %> diff --git a/app/views/admin/banners/index.html.erb b/app/views/admin/banners/index.html.erb index a466c6b7f..4b3e97230 100644 --- a/app/views/admin/banners/index.html.erb +++ b/app/views/admin/banners/index.html.erb @@ -35,12 +35,7 @@ <%= t("admin.banners.index.preview") %> - + <%= render "shared/banner", banner: banner %> diff --git a/app/views/shared/_banner.html.erb b/app/views/shared/_banner.html.erb index 3a632b20b..48f315a94 100644 --- a/app/views/shared/_banner.html.erb +++ b/app/views/shared/_banner.html.erb @@ -1,4 +1,4 @@ -<% banner = @banners.sample %> +<% banner ||= @banners.sample %>