#1370 Security enhancement

app/controllers/dmsf_controller.rb

@@ -130,14 +130,19 @@ class DmsfController < ApplicationController
   end
 
   def download_email_entries
-    # IE has got a tendency to cache files
+    file_path = helpers.email_entry_tmp_file_path(params[:entry])
-    expires_in 0.years, 'must-revalidate' => true
+    if File.exist?(file_path)
-    send_file(
+      # IE has got a tendency to cache files
-      params[:path],
+      expires_in 0.years, 'must-revalidate' => true
-      filename: 'Documents.zip',
+      send_file(
-      type: 'application/zip',
+        file_path,
-      disposition: 'attachment'
+        filename: 'Documents.zip',
-    )
+        type: 'application/zip',
+        disposition: 'attachment'
+      )
+    else
+      render_404
+    end
   rescue StandardError => e
     flash[:error] = e.message
   end
@@ -204,12 +209,14 @@ class DmsfController < ApplicationController
   end
 
   def entries_email
+    file_path = helpers.email_entry_tmp_file_path(params[:email][:zipped_content])
+    params[:email][:zipped_content] = file_path
     if params[:email][:to].strip.blank?
       flash[:error] = l(:error_email_to_must_be_entered)
     else
       DmsfMailer.deliver_send_documents @project, params[:email].permit!, User.current
-      if File.exist?(params[:email][:zipped_content])
+      if File.exist?(file_path)
-        File.delete(params[:email][:zipped_content])
+        File.delete(file_path)
       else
         flash[:error] = l(:header_minimum_filesize)
       end
@@ -523,7 +530,7 @@ class DmsfController < ApplicationController
     end
 
     @email_params = {
-      zipped_content: zipped_content,
+      zipped_content: helpers.tmp_entry_identifier(zipped_content),
       folders: selected_folders,
       files: selected_files,
       subject: "#{@project.name} #{l(:label_dmsf_file_plural).downcase}",

app/helpers/dmsf_helper.rb

@@ -79,4 +79,24 @@ module DmsfHelper
     url << ''
     url.join '/'
   end
+
+  # Downloads zipped files securely by sanitizing the params[:entry]. Characters considered unsafe
+  # are replaced with a underscore. The file_path is joined with File.join instead of Rails.root.join to eliminate
+  # the risk of overriding the absolute path (Rails.root/tmp) with file_name when given as absoulte path too. This
+  # makes the path double secure.
+  def email_entry_tmp_file_path(entry)
+    sanitized_entry = DmsfHelper.sanitize_filename(entry)
+    file_name = "#{RedmineDmsf::DmsfZip::FILE_PREFIX}#{sanitized_entry}.zip"
+    # rubocop:disable Rails/FilePath
+    File.join(Rails.root.to_s, 'tmp', file_name)
+    # rubocop:enable Rails/FilePath
+  end
+
+  # Extracts the variable part of the temp file name to be used as identifier in the
+  # download email entries route.
+  def tmp_entry_identifier(zipped_content)
+    path = Pathname.new(zipped_content)
+    zipped_file = path.basename(path.extname).to_s
+    zipped_file.delete_prefix(RedmineDmsf::DmsfZip::FILE_PREFIX)
+  end
 end

app/views/dmsf/email_entries.html.erb

@@ -53,8 +53,7 @@
     <p>
       <%= label_tag '', l(:label_email_documents) %>
       <span>
-        <%= link_to 'Documents.zip', download_email_entries_path(id: @project, folder_id: @folder,
+        <%= link_to 'Documents.zip', download_email_entries_path(id: @project, folder_id: @folder, entry: @email_params[:zipped_content]) %>
-                                                                 path: @email_params[:zipped_content]) %>
         <%= l(:label_or) %>
         <%= check_box_tag('email[links_only]', 1, RedmineDmsf.dmsf_documents_email_links_only?,
               onchange: "$('#public_url').toggle($('#email_links_only').prop('checked'))")

config/routes.rb

@@ -37,9 +37,9 @@ if Redmine::Plugin.installed? 'redmine_dmsf'
     post '/projects/:id/dmsf/entries', controller: 'dmsf', action: 'entries_operation', as: 'entries_operations_dmsf'
     post '/projects/:id/dmsf/entries/delete', controller: 'dmsf', action: 'delete_entries', as: 'delete_entries'
     post '/projects/:id/dmsf/entries/email', to: 'dmsf#entries_email', as: 'email_entries'
-    get '/projects/:id/dmsf/entries/download_email_entries', controller: 'dmsf',
+    get '/projects/:id/dmsf/entries/:entry/download_email_entries', controller: 'dmsf',
-                                                             action: 'download_email_entries',
+                                                                    action: 'download_email_entries',
-                                                             as: 'download_email_entries'
+                                                                    as: 'download_email_entries'
     get '/projects/:id/entries/copymove', to: 'dmsf#copymove', as: 'copymove_entries'
     get '/projects/:id/dmsf/lock', controller: 'dmsf', action: 'lock', as: 'lock_dmsf'
     get '/projects/:id/dmsf/unlock', controller: 'dmsf', action: 'unlock', as: 'unlock_dmsf'

lib/redmine_dmsf/dmsf_zip.rb

@@ -22,12 +22,14 @@ require 'zip'
 
 module RedmineDmsf
   module DmsfZip
+    FILE_PREFIX = 'dmsf_zip_'
+
     # ZIP
     class Zip
       attr_reader :dmsf_files
 
       def initialize
-        @temp_file = Tempfile.new(%w[dmsf_zip_ .zip], Rails.root.join('tmp'))
+        @temp_file = Tempfile.new([FILE_PREFIX, '.zip'], Rails.root.join('tmp'))
         @zip_file = ::Zip::OutputStream.open(@temp_file)
         @files = []
         @dmsf_files = []

test/functional/dmsf_controller_test.rb

@@ -24,6 +24,7 @@ require File.expand_path('../../test_helper', __FILE__)
 class DmsfControllerTest < RedmineDmsf::Test::TestCase
   include Redmine::I18n
   include Rails.application.routes.url_helpers
+  include DmsfHelper
 
   fixtures :custom_fields, :custom_values, :dmsf_links, :dmsf_folder_permissions, :dmsf_locks,
            :dmsf_folders, :dmsf_files, :dmsf_file_revisions
@@ -378,6 +379,29 @@ class DmsfControllerTest < RedmineDmsf::Test::TestCase
     zip_file.unlink
   end
 
+  def test_download_email_entries
+    post '/login', params: { username: 'jsmith', password: 'jsmith' }
+    zip_file = Tempfile.new([RedmineDmsf::DmsfZip::FILE_PREFIX, '.zip'], Rails.root.join('tmp'))
+    entry = tmp_entry_identifier(zip_file.path)
+    get "/projects/#{@project1.identifier}/dmsf/entries/#{entry}/download_email_entries"
+    assert_response :success
+  end
+
+  def test_download_email_entries_not_found
+    post '/login', params: { username: 'jsmith', password: 'jsmith' }
+    get "/projects/#{@project1.identifier}/dmsf/entries/notfound/download_email_entries"
+    assert_response :not_found
+  end
+
+  def test_download_email_entries_forbidden
+    @role_manager.remove_permission! :view_dmsf_files
+    post '/login', params: { username: 'jsmith', password: 'jsmith' }
+    zip_file = Tempfile.new([RedmineDmsf::DmsfZip::FILE_PREFIX, '.zip'], Rails.root.join('tmp'))
+    entry = tmp_entry_identifier(zip_file.path)
+    get "/projects/#{@project1.identifier}/dmsf/entries/#{entry}/download_email_entries"
+    assert_response :forbidden
+  end
+
   def test_add_email_forbidden
     post '/login', params: { username: 'jsmith', password: 'jsmith' }
     @role_manager.remove_permission! :view_dmsf_files