diff --git a/app/controllers/dmsf_controller.rb b/app/controllers/dmsf_controller.rb index e65e6cc0..c43e8a9b 100644 --- a/app/controllers/dmsf_controller.rb +++ b/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 - expires_in 0.years, 'must-revalidate' => true - send_file( - params[:path], - filename: 'Documents.zip', - type: 'application/zip', - disposition: 'attachment' - ) + file_path = helpers.email_entry_tmp_file_path(params[:entry]) + if File.exist?(file_path) + # IE has got a tendency to cache files + expires_in 0.years, 'must-revalidate' => true + send_file( + file_path, + 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]) - File.delete(params[:email][:zipped_content]) + if File.exist?(file_path) + 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}", diff --git a/app/helpers/dmsf_helper.rb b/app/helpers/dmsf_helper.rb index 603911e7..b9414bcc 100644 --- a/app/helpers/dmsf_helper.rb +++ b/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 diff --git a/app/views/dmsf/email_entries.html.erb b/app/views/dmsf/email_entries.html.erb index 7c35561c..2214068c 100644 --- a/app/views/dmsf/email_entries.html.erb +++ b/app/views/dmsf/email_entries.html.erb @@ -53,8 +53,7 @@
<%= label_tag '', l(:label_email_documents) %> - <%= link_to 'Documents.zip', download_email_entries_path(id: @project, folder_id: @folder, - path: @email_params[:zipped_content]) %> + <%= link_to 'Documents.zip', download_email_entries_path(id: @project, folder_id: @folder, entry: @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'))") diff --git a/config/routes.rb b/config/routes.rb index 2b1870f1..fe57fba7 100644 --- a/config/routes.rb +++ b/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', - action: 'download_email_entries', - as: 'download_email_entries' + get '/projects/:id/dmsf/entries/:entry/download_email_entries', controller: 'dmsf', + action: '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' diff --git a/lib/redmine_dmsf/dmsf_zip.rb b/lib/redmine_dmsf/dmsf_zip.rb index f0b4a3ca..d122dc11 100644 --- a/lib/redmine_dmsf/dmsf_zip.rb +++ b/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 = [] diff --git a/test/functional/dmsf_controller_test.rb b/test/functional/dmsf_controller_test.rb index fdb67f9b..32fe40b1 100644 --- a/test/functional/dmsf_controller_test.rb +++ b/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