Merge pull request #3851 from consul/security

Reduce false positives count in security reports

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());
         }
       });
     }

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

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

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

app/helpers/banners_helper.rb

@@ -18,4 +18,11 @@ module BannersHelper
   def banner_font_color_or_default
     @banner.font_color.presence || banner_default_font_color
   end
+
+  def banner_target_link(banner)
+    link_to banner.target_url do
+      content_tag(:h2, banner.title, style: "color:#{banner.font_color}") +
+        content_tag(:h3, banner.description, style: "color:#{banner.font_color}")
+    end
+  end
 end

app/models/budget/investment.rb

@@ -126,7 +126,7 @@ class Budget
     end
 
     def self.sort_by_title
-      with_translation.sort_by(&:title)
+      all.sort_by(&:title)
     end
 
     def self.filter_params(params)

app/models/concerns/globalizable.rb

@@ -29,8 +29,6 @@ module Globalizable
       translation_class.send :acts_as_paranoid, column: :hidden_at
     end
 
-    scope :with_translation, -> { joins("LEFT OUTER JOIN #{translations_table_name} ON #{table_name}.id = #{translations_table_name}.#{reflections["translations"].foreign_key} AND #{translations_table_name}.locale='#{I18n.locale}'") }
-
     private
 
       def required_attribute?(attribute)

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

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
 

app/views/admin/banners/_form.html.erb

@@ -84,15 +84,9 @@
     <div class="actions small-12 medium-3 column">
       <%= f.submit(class: "button expanded", value: t("admin.banners.edit.form.submit_button")) %>
     </div>
+  </div>
 
-    <div id="js-banner-background" class="banner clear"
-                                   style="background-color:<%= @banner.background_color %>">
-      <%= link_to @banner.target_url do %>
-        <h2 id="js-banner-title" style="color:<%= @banner.font_color %>"><%= @banner.title %></h2>
-        <h3 id="js-banner-description" style="color:<%= @banner.font_color %>">
-          <%= @banner.description %>
-        </h3>
-      <% end %>
-    </div>
+  <div class="row">
+    <%= render "shared/banner", banner: @banner %>
   </div>
 <% end %>

app/views/admin/banners/index.html.erb

@@ -35,12 +35,7 @@
       <tr>
         <td colspan="3">
           <span class="help-text"><%= t("admin.banners.index.preview") %></span>
-          <div class="banner" style="background-color:<%= banner.background_color %>">
-            <%= link_to banner.target_url do %>
-              <h2 style="color:<%= banner.font_color %>"><%= banner.title %></h2>
-              <h3 style="color:<%= banner.font_color %>"><%= banner.description %></h3>
-            <% end %>
-          </div>
+          <%= render "shared/banner", banner: banner %>
         </td>
       </tr>
     </tbody>

app/views/shared/_banner.html.erb

@@ -1,7 +1,4 @@
-<% banner = @banners.sample %>
+<% banner ||= @banners.sample %>
 <div class="banner" style="background-color:<%= banner.background_color %>;">
-  <%= link_to banner.target_url do %>
-    <h2 style="color:<%= banner.font_color %>;"><%= banner.title %></h2>
-    <h3 style="color:<%= banner.font_color %>;"><%= banner.description %></h3>
-  <% end %>
+  <%= sanitize banner_target_link(banner), attributes: %w[href style] %>
 </div>

spec/features/admin/banners_spec.rb

@@ -164,7 +164,7 @@ describe "Admin banners magement" do
 
     page.find("body").click
 
-    within("div#js-banner-background") do
+    within(".banner") do
       expect(page).to have_selector("h2", text: "Modified title")
       expect(page).to have_selector("h3", text: "Edited text")
     end

spec/features/xss_spec.rb

@@ -25,6 +25,16 @@ describe "Cross-Site Scripting protection", :js do
     expect(page.text).not_to be_empty
   end
 
+  scenario "banner URL" do
+    banner = create(:banner, title: "Banned!", target_url: "javascript:document.body.remove()")
+
+    login_as(create(:administrator).user)
+    visit edit_admin_banner_path(banner)
+    find(:css, "a", text: "Banned!").click
+
+    expect(page.text).not_to be_empty
+  end
+
   scenario "document title" do
     process = create(:legislation_process)
     create(:document, documentable: process, title: attack_code)