Bugfixing (#5354)

* Sanitize redirect_url

* Only allow relative files at design app

* Add rel=noopener

* Limit allowed file extensions to be uploaded and strip image metadata

* Validate nickname format

decidim-core/app/cells/decidim/fingerprint/show.erb

@@ -14,7 +14,7 @@
     <code class="fingerprint-source"><%= decidim_html_escape model.fingerprint.source %></code>
   </p>
 
-  <p><%= t "decidim.fingerprint.replicate_help", online_calculator_link: link_to(t("decidim.fingerprint.online_calculator_name"), "http://www.md5calc.com/sha256", target: "_blank") %>
+  <p><%= t "decidim.fingerprint.replicate_help", online_calculator_link: link_to(t("decidim.fingerprint.online_calculator_name"), "http://www.md5calc.com/sha256", target: "_blank", rel: "noopener") %>
 
   <button class="close-button" data-close type="button">
     <span aria-hidden="true">&times;</span>

decidim-core/app/controllers/concerns/decidim/devise_controllers.rb

@@ -17,6 +17,7 @@ module DeviseControllers
       include Decidim::LocaleSwitcher
       include ImpersonateUsers
       include NeedsPermission
+      include Decidim::SafeRedirect
 
       helper Decidim::TranslationsHelper
       helper Decidim::MetaTagsHelper
@@ -43,9 +44,9 @@ def permission_scope
       end
 
       def store_current_location
-        return if params[:redirect_url].blank? || !request.format.html?
+        return if redirect_url.blank? || !request.format.html?
 
-        store_location_for(:user, params[:redirect_url])
+        store_location_for(:user, redirect_url)
       end
     end
   end

decidim-core/app/controllers/concerns/decidim/safe_redirect.rb

@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+require "active_support/concern"
+
+module Decidim
+  # This concern groups methods and helpers related to redirecting the user from URL params.
+  module SafeRedirect
+    extend ActiveSupport::Concern
+
+    included do
+      helper_method :redirect_url
+
+      # Sanitizes the redirect url only allowing relative paths or absolute URLs
+      # that match the current organization.
+      def redirect_url
+        return if params[:redirect_url].blank?
+        return params[:redirect_url] unless params[:redirect_url].start_with?("http")
+        return if URI.parse(params[:redirect_url]).host != current_organization.host
+
+        params[:redirect_url]
+      end
+    end
+  end
+end

decidim-core/app/controllers/decidim/application_controller.rb

@@ -12,6 +12,7 @@ class ApplicationController < ::DecidimController
     include HttpCachingDisabler
     include ActionAuthorization
     include ForceAuthentication
+    include SafeRedirect
 
     helper Decidim::MetaTagsHelper
     helper Decidim::DecidimFormHelper
@@ -53,13 +54,13 @@ class ApplicationController < ::DecidimController
     def store_current_location
       return if skip_store_location?
 
-      value = params[:redirect_url] || request.url
+      value = redirect_url || request.url
       store_location_for(:user, value)
     end
 
     def skip_store_location?
       # Skip if Devise already handles the redirection
-      return true if devise_controller? && params[:redirect_url].blank?
+      return true if devise_controller? && redirect_url.blank?
       # Skip for all non-HTML requests"
       return true unless request.format.html?
       # Skip if a signed in user requests the TOS page without having agreed to

decidim-core/app/forms/decidim/account_form.rb

@@ -18,7 +18,7 @@ class AccountForm < Form
 
     validates :name, presence: true
     validates :email, presence: true, 'valid_email_2/email': { disposable: true }
-    validates :nickname, presence: true
+    validates :nickname, presence: true, format: /\A[\w\-]+\z/
 
     validates :nickname, length: { maximum: Decidim::User.nickname_max_length, allow_blank: true }
     validates :password, confirmation: true

decidim-core/app/forms/decidim/registration_form.rb

@@ -14,7 +14,7 @@ class RegistrationForm < Form
     attribute :tos_agreement, Boolean
 
     validates :name, presence: true
-    validates :nickname, presence: true, length: { maximum: Decidim::User.nickname_max_length }
+    validates :nickname, presence: true, format: /\A[\w\-]+\z/, length: { maximum: Decidim::User.nickname_max_length }
     validates :email, presence: true, 'valid_email_2/email': { disposable: true }
     validates :password, confirmation: true
     validates :password, password: { name: :name, email: :email, username: :nickname }

decidim-core/app/uploaders/decidim/attachment_uploader.rb

@@ -7,6 +7,7 @@ class AttachmentUploader < ApplicationUploader
 
     process :set_content_type_and_size_in_model
     process :validate_dimensions
+    process :strip
 
     version :thumbnail, if: :image? do
       process resize_to_fit: [nil, 237]
@@ -18,6 +19,20 @@ class AttachmentUploader < ApplicationUploader
 
     protected
 
+    def extension_white_list
+      %w(jpg jpeg gif png bmp pdf doc docx xls xlsx ppt ppx rtf txt odt ott odf otg ods ots)
+    end
+
+    # Strips out all embedded information from the image
+    def strip
+      return unless image?(self)
+
+      manipulate! do |img|
+        img.strip
+        img
+      end
+    end
+
     # CarrierWave automatically calls this method and validates the content
     # type fo the temp file to match against any of these options.
     def content_type_whitelist

decidim-core/app/uploaders/decidim/avatar_uploader.rb

@@ -3,8 +3,6 @@
 module Decidim
   # This class deals with uploading avatars to a User.
   class AvatarUploader < ImageUploader
-    include CarrierWave::MiniMagick
-
     process :validate_dimensions
 
     version :profile do

decidim-core/app/uploaders/decidim/homepage_image_uploader.rb

@@ -3,8 +3,6 @@
 module Decidim
   # This class deals with uploading hero images to organizations.
   class HomepageImageUploader < ImageUploader
-    include CarrierWave::MiniMagick
-
     version :big do
       process resize_to_fill: [1920, 666]
     end

decidim-core/app/uploaders/decidim/image_uploader.rb

@@ -5,7 +5,7 @@ module Decidim
   class ImageUploader < ApplicationUploader
     include CarrierWave::MiniMagick
 
-    process :validate_size, :validate_dimensions
+    process :validate_size, :validate_dimensions, :strip
     process quality: Decidim.image_uploader_quality
 
     # CarrierWave automatically calls this method and validates the content
@@ -16,6 +16,20 @@ def content_type_whitelist
       ]
     end
 
+    # Strips out all embedded information from the image
+    def strip
+      manipulate! do |img|
+        img.strip
+        img
+      end
+    end
+
+    # Add a white list of extensions which are allowed to be uploaded.
+    # For images you might use something like this:
+    def extension_white_list
+      %w(jpg jpeg gif png bmp ico)
+    end
+
     # A simple check to avoid DoS with maliciously crafted images, or just to
     # avoid reckless users that upload gigapixels images.
     #

decidim-core/app/uploaders/decidim/oauth_application_logo_uploader.rb

@@ -3,7 +3,6 @@
 module Decidim
   # This class deals with uploading hero images to ParticipatoryProcesses.
   class OAuthApplicationLogoUploader < ImageUploader
-    include CarrierWave::MiniMagick
     process resize_to_fit: [75, 75]
   end
 end

decidim-core/app/uploaders/decidim/official_image_footer_uploader.rb

@@ -3,7 +3,6 @@
 module Decidim
   # This class deals with uploading hero images to ParticipatoryProcesses.
   class OfficialImageFooterUploader < ImageUploader
-    include CarrierWave::MiniMagick
     process resize_to_fit: [600, 180]
   end
 end

decidim-core/app/uploaders/decidim/official_image_header_uploader.rb

@@ -3,7 +3,6 @@
 module Decidim
   # This class deals with uploading hero images to ParticipatoryProcesses.
   class OfficialImageHeaderUploader < ImageUploader
-    include CarrierWave::MiniMagick
     process resize_to_fit: [160, 160]
   end
 end

decidim-core/app/views/decidim/application/_document.html.erb

@@ -12,7 +12,7 @@
     </div>
   </div>
   <div class="card--list__data">
-    <%= link_to document.url, target: "_blank", class: "card--list__data__icon" do %>
+    <%= link_to document.url, target: "_blank", rel: "noopener", class: "card--list__data__icon" do %>
         <%= icon "cloud-download" %>
     <% end %>
   </div>

decidim-core/app/views/decidim/application/_photos.html.erb

@@ -5,7 +5,7 @@
       <% photos.in_groups_of(3, false).each do |group| %>
         <% group.each_with_index do |photo, index| %>
           <div class="columns small-6 medium-4 <%= (index == 2 || photo == group.last ? "end" : "") %>">
-            <%= link_to photo.big_url, target: "_blank" do %>
+            <%= link_to photo.big_url, target: "_blank", rel: "noopener" do %>
               <%= image_tag photo.thumbnail_url, class:"thumbnail", alt: strip_tags(translated_attribute(photo.title)) %>
             <% end %>
           </div>

decidim-core/app/views/layouts/decidim/_social_media_links.html.erb

@@ -2,35 +2,35 @@
   <ul class="footer-social">
     <% if current_organization.twitter_handler.present? %>
       <li>
-        <a class="footer-social__icon" target="_blank" title="Twitter" href="https://twitter.com/<%= current_organization.twitter_handler %>">
+        <a class="footer-social__icon" target="_blank" rel="noopener" title="Twitter" href="https://twitter.com/<%= current_organization.twitter_handler %>">
           <%= icon "twitter", role: "img", aria_label: "Twitter" %>
         </a>
       </li>
     <% end %>
     <% if current_organization.facebook_handler.present? %>
       <li>
-        <a class="footer-social__icon" target="_blank" title="Facebook" href="https://www.facebook.com/<%= current_organization.facebook_handler %>">
+        <a class="footer-social__icon" target="_blank" rel="noopener" title="Facebook" href="https://www.facebook.com/<%= current_organization.facebook_handler %>">
           <%= icon "facebook", role: "img", aria_label: "Facebook" %>
         </a>
       </li>
     <% end %>
     <% if current_organization.instagram_handler.present? %>
       <li>
-        <a class="footer-social__icon" target="_blank" title="Instagram" href="https://www.instagram.com/<%= current_organization.instagram_handler %>">
+        <a class="footer-social__icon" target="_blank" rel="noopener" title="Instagram" href="https://www.instagram.com/<%= current_organization.instagram_handler %>">
           <%= icon "instagram", role: "img", aria_label: "Instagram" %>
         </a>
       </li>
     <% end %>
     <% if current_organization.youtube_handler.present? %>
       <li>
-        <a class="footer-social__icon" target="_blank" title="YouTube" href="https://www.youtube.com/<%= current_organization.youtube_handler %>">
+        <a class="footer-social__icon" target="_blank" rel="noopener" title="YouTube" href="https://www.youtube.com/<%= current_organization.youtube_handler %>">
           <%= icon "youtube", role: "img", aria_label: "YouTube" %>
         </a>
       </li>
     <% end %>
     <% if current_organization.github_handler.present? %>
       <li>
-        <a class="footer-social__icon" target="_blank" title="GitHub" href="https://www.github.com/<%= current_organization.github_handler %>">
+        <a class="footer-social__icon" target="_blank" rel="noopener" title="GitHub" href="https://www.github.com/<%= current_organization.github_handler %>">
           <%= icon "github", role: "img", aria_label: "GitHub" %>
         </a>
       </li>

decidim-core/app/views/layouts/decidim/_wrapper.html.erb

@@ -127,7 +127,7 @@ end
           <div class="medium-3 large-4 column">
             <a rel="license" class="cc-badge"
                href="http://creativecommons.org/licenses/by-sa/4.0/"
-               target="_blank">
+               target="_blank" rel="noopener">
               <%= image_tag("decidim/cc-badge.png", alt: "Creative Commons License" ) %>
             </a>
             <%= t("layouts.decidim.footer.made_with_open_source").html_safe %>
@@ -136,7 +136,7 @@ end
             <div class="decidim-logo">
               <a rel="decidim"
                  href="https://decidim.org/"
-                 target="_blank">
+                 target="_blank" rel="noopener">
                 <%= image_tag("decidim/decidim-logo.svg", alt: "Decidim Logo" ) %>
               </a>
             </div>

decidim-core/spec/controllers/application_controller_spec.rb

@@ -26,6 +26,26 @@ def tos
       end
     end
 
+    describe "redirect_url" do
+      it "allows relative paths" do
+        get :show, params: { redirect_url: "/my/account" }
+
+        expect(controller.helpers.redirect_url).to eq("/my/account")
+      end
+
+      it "allows absolute URLs within the organization" do
+        get :show, params: { redirect_url: "http://#{organization.host}/my/account" }
+
+        expect(controller.helpers.redirect_url).to eq("http://#{organization.host}/my/account")
+      end
+
+      it "doesn't allow other URLs" do
+        get :show, params: { redirect_url: "http://www.example.org" }
+
+        expect(controller.helpers.redirect_url).to eq(nil)
+      end
+    end
+
     describe "#show" do
       context "when authenticated" do
         before do

decidim-core/spec/forms/account_form_spec.rb

@@ -98,6 +98,14 @@ module Decidim
           expect(subject).to be_valid
         end
       end
+
+      context "with invalid characters" do
+        let(:nickname) { "foo@bar" }
+
+        it "is invalid" do
+          expect(subject).not_to be_valid
+        end
+      end
     end
 
     describe "password" do

decidim-meetings/app/views/decidim/meetings/meetings/_meeting_minutes.html.erb

@@ -18,11 +18,11 @@
           <div class="card--list__item">
             <div class="card--list__text">
               <div>
-                <%= link_to meeting.minutes.video_url, meeting.minutes.video_url, target: "_blank" %>
+                <%= link_to meeting.minutes.video_url, meeting.minutes.video_url, target: "_blank", rel: "noopener" %>
               </div>
             </div>
             <div class="card--list__data">
-              <%= link_to meeting.minutes.video_url, target: "_blank", class: "card--list__data__icon" do %>
+              <%= link_to meeting.minutes.video_url, target: "_blank", class: "card--list__data__icon", rel: "noopener" do %>
                 <%= icon "external-link" %>
               <% end %>
             </div>
@@ -32,11 +32,11 @@
           <div class="card--list__item">
             <div class="card--list__text">
               <div>
-                <%= link_to meeting.minutes.audio_url, meeting.minutes.audio_url, target: "_blank" %>
+                <%= link_to meeting.minutes.audio_url, meeting.minutes.audio_url, target: "_blank", rel: "noopener" %>
               </div>
             </div>
             <div class="card--list__data">
-              <%= link_to meeting.minutes.audio_url, target: "_blank", class: "card--list__data__icon" do %>
+              <%= link_to meeting.minutes.audio_url, target: "_blank", class: "card--list__data__icon", rel: "noopener" do %>
                 <%= icon "external-link" %>
               <% end %>
             </div>

decidim-verifications/app/controllers/decidim/verifications/authorizations_controller.rb

@@ -36,7 +36,7 @@ def create
         AuthorizeUser.call(handler) do
           on(:ok) do
             flash[:notice] = t("authorizations.create.success", scope: "decidim.verifications")
-            redirect_to params[:redirect_url] || authorizations_path
+            redirect_to redirect_url || authorizations_path
           end
 
           on(:invalid) do
@@ -88,9 +88,9 @@ def pending_authorizations
       end
 
       def store_current_location
-        return if params[:redirect_url].blank? || !request.format.html?
+        return if redirect_url.blank? || !request.format.html?
 
-        store_location_for(:user, params[:redirect_url])
+        store_location_for(:user, redirect_url)
       end
     end
   end

decidim-verifications/app/controllers/decidim/verifications/sms/authorizations_controller.rb

@@ -21,7 +21,7 @@ def create
             on(:ok) do
               flash[:notice] = t("authorizations.create.success", scope: "decidim.verifications.sms")
               authorization_method = Decidim::Verifications::Adapter.from_element(authorization.name)
-              redirect_to authorization_method.resume_authorization_path(redirect_url: params[:redirect_url])
+              redirect_to authorization_method.resume_authorization_path(redirect_url: redirect_url)
             end
             on(:invalid) do
               flash.now[:alert] = t("authorizations.create.error", scope: "decidim.verifications.sms")
@@ -45,8 +45,8 @@ def update
             on(:ok) do
               flash[:notice] = t("authorizations.update.success", scope: "decidim.verifications.sms")
 
-              if params[:redirect_url]
-                redirect_to params[:redirect_url]
+              if redirect_url
+                redirect_to redirect_url
               else
                 redirect_to decidim_verifications.authorizations_path
               end

decidim-verifications/app/views/decidim/verifications/authorizations/new.html.erb

@@ -10,7 +10,7 @@
       <div class="columns large-6 medium-centered">
         <div class="card">
           <div class="card__content">
-            <%= authorization_form_for(handler, url: authorizations_path(redirect_url: params[:redirect_url])) do |form| %>
+            <%= authorization_form_for(handler, url: authorizations_path(redirect_url: redirect_url)) do |form| %>
               <% if lookup_context.exists?(handler.to_partial_path, [], true) %>
                 <%= render partial: handler.to_partial_path, locals: { handler: handler, form: form } %>
               <% else %>