diff --git a/CHANGELOG.md b/CHANGELOG.md index 9234c0281..97821d253 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ - Add messages for Arabic language - Add `methods_ln.js` files with regexps for DE, NL, and PT languages - Modify admin layout view to load the `methods_ln.js` file with a `javascript_include_tag` if the file exists +- Fix uploads to AWS S3 folders + - Also, introduced the path traversal validation to the add_folder method, which was found unsafe ## [2.8.2](https://github.com/owen2345/camaleon-cms/tree/2.8.2) (2024-08-25) diff --git a/app/controllers/camaleon_cms/admin/media_controller.rb b/app/controllers/camaleon_cms/admin/media_controller.rb index 12e8acfef..100d75ed3 100644 --- a/app/controllers/camaleon_cms/admin/media_controller.rb +++ b/app/controllers/camaleon_cms/admin/media_controller.rb @@ -57,7 +57,8 @@ def actions case params[:media_action] when 'new_folder' params[:folder] = slugify_folder(params[:folder]) - return render partial: 'render_file_item', locals: { files: [cama_uploader.add_folder(params[:folder])] } + r = cama_uploader.add_folder(params[:folder]) + return render partial: 'render_file_item', locals: { files: [r] } if r[:error].blank? when 'del_folder' r = cama_uploader.delete_folder(params[:folder]) when 'del_file' diff --git a/app/helpers/camaleon_cms/uploader_helper.rb b/app/helpers/camaleon_cms/uploader_helper.rb index f8b830a88..f22e279c1 100644 --- a/app/helpers/camaleon_cms/uploader_helper.rb +++ b/app/helpers/camaleon_cms/uploader_helper.rb @@ -76,7 +76,7 @@ def upload_file(uploaded_io, settings = {}) res = { error: nil } # guard against path traversal - return { error: 'Invalid file path' } unless cama_uploader.class.valid_folder_path?(settings[:folder]) + return { error: 'Invalid file path' } unless cama_uploader.valid_folder_path?(settings[:folder]) # formats validations return { error: "#{ct('file_format_error')} (#{settings[:formats]})" } unless cama_uploader.class.validate_file_format( diff --git a/app/uploaders/camaleon_cms_aws_uploader.rb b/app/uploaders/camaleon_cms_aws_uploader.rb index bffe161b0..fa3d397ab 100644 --- a/app/uploaders/camaleon_cms_aws_uploader.rb +++ b/app/uploaders/camaleon_cms_aws_uploader.rb @@ -105,6 +105,8 @@ def add_file(uploaded_io_or_file_path, key, args = {}) # add new folder to AWS with :key def add_folder(key) + return { error: 'Invalid folder path' } unless valid_folder_path?(key) + key = "#{@aws_settings['inner_folder']}/#{key}" if @aws_settings['inner_folder'].present? key = key.cama_fix_media_key s3_file = bucket.object(key.slice(1..-1) << '/') diff --git a/app/uploaders/camaleon_cms_local_uploader.rb b/app/uploaders/camaleon_cms_local_uploader.rb index 0e61e6f24..da862445c 100644 --- a/app/uploaders/camaleon_cms_local_uploader.rb +++ b/app/uploaders/camaleon_cms_local_uploader.rb @@ -25,7 +25,7 @@ def browser_files(prefix = '/', _objects = {}) end def fetch_file(file_name) - return { error: 'Invalid file path' } if file_name.include?('..') + return { error: 'Invalid file path' } unless valid_folder_path?(file_name) return file_name if file_exists?(file_name) @@ -96,6 +96,8 @@ def add_file(uploaded_io_or_file_path, key, args = {}) # create a new folder into local directory def add_folder(key) + return { error: 'Invalid folder path' } unless valid_folder_path?(key) + d = File.join(@root_folder, key).to_s is_new_folder = false unless Dir.exist?(d) diff --git a/app/uploaders/camaleon_cms_uploader.rb b/app/uploaders/camaleon_cms_uploader.rb index 2703989d9..ebd85707a 100644 --- a/app/uploaders/camaleon_cms_uploader.rb +++ b/app/uploaders/camaleon_cms_uploader.rb @@ -125,10 +125,8 @@ def self.validate_file_format(key, valid_formats = '*') valid_formats.include?(File.extname(key).sub('.', '').split('?').first.try(:downcase)) end - def self.valid_folder_path?(path) - return true if path == '/' - - return false if path.include?('..') || File.absolute_path?(path) || path.include?('://') + def valid_folder_path?(path) + return false if path.include?('..') || path.include?('://') true end diff --git a/spec/dummy/db/test.sqlite3-shm b/spec/dummy/db/test.sqlite3-shm deleted file mode 100644 index fe9ac2845..000000000 Binary files a/spec/dummy/db/test.sqlite3-shm and /dev/null differ diff --git a/spec/dummy/db/test.sqlite3-wal b/spec/dummy/db/test.sqlite3-wal deleted file mode 100644 index e69de29bb..000000000 diff --git a/spec/helpers/uploader_helper_spec.rb b/spec/helpers/uploader_helper_spec.rb index 206d0630b..cab35fda6 100644 --- a/spec/helpers/uploader_helper_spec.rb +++ b/spec/helpers/uploader_helper_spec.rb @@ -56,16 +56,26 @@ end describe 'file upload with invalid path' do - it 'upload a local file with invalid path of a path traversal try' do + it "doesn't upload a local file with invalid path of a path traversal try" do expect(upload_file(File.open(@path), { folder: '../../config/initializers' }).keys.include?(:error)).to be(true) end - it 'upload a local file with invalid URI-like path' do + it "doesn't upload a local file with invalid URI-like path" do expect(upload_file(File.open(@path), { folder: 'file:///config/initializers' }).keys.include?(:error)).to be(true) end + end + + context 'with an absolute path' do + let(:file) { File.join(current_site.upload_directory, '/tmp/config/initializers/rails_tmp.png') } + + after { File.delete(file) } + + it 'uploads a local file with an absolute path into the upload directory, not into the volume root' do + expect(File).not_to exist(file) + + upload_file(File.open(@path), { folder: '/tmp/config/initializers' }) - it 'upload a local file with an absolute path' do - expect(upload_file(File.open(@path), { folder: '/tmp/config/initializers' }).keys.include?(:error)).to be(true) + expect(File).to exist(file) end end @@ -78,7 +88,7 @@ end end - it 'upload a local file with invalid format' do + it "doesn't upload a local file with invalid format" do expect(upload_file(File.open(@path), { formats: 'audio' }).keys.include?(:error)).to be(true) end diff --git a/spec/requests/admin/media_controller/download_private_file_spec.rb b/spec/requests/admin/media_controller/download_private_file_spec.rb new file mode 100644 index 000000000..c15b6c1a0 --- /dev/null +++ b/spec/requests/admin/media_controller/download_private_file_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'Download private file requests', type: :request do + init_site + + let(:current_site) { Cama::Site.first.decorate } + + before do + allow_any_instance_of(CamaleonCms::Admin::MediaController).to receive(:cama_authenticate) + allow_any_instance_of(CamaleonCms::Admin::MediaController).to receive(:current_site).and_return(current_site) + end + + context 'when the file path is valid and file exists' do + before do + allow_any_instance_of(CamaleonCmsLocalUploader).to receive(:fetch_file).and_return('some_file') + + allow_any_instance_of(CamaleonCms::Admin::MediaController).to receive(:send_file) + allow_any_instance_of(CamaleonCms::Admin::MediaController).to receive(:default_render) + end + + it 'allows the file to be downloaded' do + expect_any_instance_of(CamaleonCms::Admin::MediaController).to receive(:send_file).with('some_file', disposition: 'inline') + + get '/admin/media/download_private_file', params: { file: 'some_file' } + end + end + + context 'when file path is invalid' do + it 'returns invalid file path error' do + get '/admin/media/download_private_file', params: { file: './../../../../../etc/passwd' } + + expect(response.body).to include('Invalid file path') + end + end + + context 'when the file is not found' do + it 'returns file not found error' do + get '/admin/media/download_private_file', params: { file: 'passwd' } + + expect(response.body).to include('File not found') + end + end +end diff --git a/spec/requests/admin/media_controller/new_folder_spec.rb b/spec/requests/admin/media_controller/new_folder_spec.rb new file mode 100644 index 000000000..faa96d6c6 --- /dev/null +++ b/spec/requests/admin/media_controller/new_folder_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'New folder request', type: :request do + init_site + + let(:current_site) { Cama::Site.first.decorate } + + before do + allow_any_instance_of(CamaleonCms::AdminController).to receive(:cama_authenticate) + allow_any_instance_of(CamaleonCms::AdminController).to receive(:current_site).and_return(current_site) + allow_any_instance_of(CamaleonCms::Admin::MediaController).to receive(:authorize!) + end + + context 'when the folder path is valid' do + it 'creates the new folder' do + post '/admin/media/actions', params: { folder: '/test2', media_action: 'new_folder' } + + expect(Dir).to exist(File.join(current_site.upload_directory, '/test2')) + end + end + + context 'when the folder path is invalid' do + it 'returns invalid file path error' do + post '/admin/media/actions', params: { folder: '/../test3', media_action: 'new_folder' } + + expect(Dir).not_to exist(File.join(current_site.upload_directory, '/../test3')) + + expect(response.body).to include('Invalid folder path') + end + end +end diff --git a/spec/requests/download_private_file_spec.rb b/spec/requests/download_private_file_spec.rb deleted file mode 100644 index 338189130..000000000 --- a/spec/requests/download_private_file_spec.rb +++ /dev/null @@ -1,47 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -RSpec.describe 'Media requests', type: :request do - init_site - - describe 'Download private file' do - let(:current_site) { Cama::Site.first.decorate } - - before do - allow_any_instance_of(CamaleonCms::Admin::MediaController).to receive(:cama_authenticate) - allow_any_instance_of(CamaleonCms::Admin::MediaController).to receive(:current_site).and_return(current_site) - end - - context 'when the file path is valid and file exists' do - before do - allow_any_instance_of(CamaleonCmsLocalUploader).to receive(:fetch_file).and_return('some_file') - - allow_any_instance_of(CamaleonCms::Admin::MediaController).to receive(:send_file) - allow_any_instance_of(CamaleonCms::Admin::MediaController).to receive(:default_render) - end - - it 'allows the file to be downloaded' do - expect_any_instance_of(CamaleonCms::Admin::MediaController).to receive(:send_file).with('some_file', disposition: 'inline') - - get '/admin/media/download_private_file', params: { file: 'some_file' } - end - end - - context 'when file path is invalid' do - it 'returns invalid file path error' do - get '/admin/media/download_private_file', params: { file: './../../../../../etc/passwd' } - - expect(response.body).to include('Invalid file path') - end - end - - context 'when the file is not found' do - it 'returns file not found error' do - get '/admin/media/download_private_file', params: { file: 'passwd' } - - expect(response.body).to include('File not found') - end - end - end -end diff --git a/spec/uploaders/local_uploader_spec.rb b/spec/uploaders/local_uploader_spec.rb index a5338359a..08914f7d3 100644 --- a/spec/uploaders/local_uploader_spec.rb +++ b/spec/uploaders/local_uploader_spec.rb @@ -8,15 +8,23 @@ let(:current_site) { Cama::Site.first.decorate } let(:uploader) { described_class.new(current_site: current_site) } - describe '#delete_folder' do - it 'returns an error' do - expect(uploader.delete_folder('../tmp')).to eql(error: 'Invalid folder path') + context 'with an invalid path containing path traversal characters' do + describe '#add_folder' do + it 'returns an error' do + expect(uploader.add_folder('../tmp')).to eql(error: 'Invalid folder path') + end + end + + describe '#delete_folder' do + it 'returns an error' do + expect(uploader.delete_folder('../tmp')).to eql(error: 'Invalid folder path') + end end - end - describe '#delete_file' do - it 'returns an error' do - expect(uploader.delete_file('../test.rb')).to eql(error: 'Invalid file path') + describe '#delete_file' do + it 'returns an error' do + expect(uploader.delete_file('../test.rb')).to eql(error: 'Invalid file path') + end end end end