Merge pull request #1099 from texpert/fix-uploads-to-aws-s3-folder

Fix uploads to AWS S3 folder

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)
 

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'

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(

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) << '/')

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)

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

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
 

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

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

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