From 5a15640d0aa8f6374843f6612f848cbd40d3cfd9 Mon Sep 17 00:00:00 2001
From: texpert <branzeanu.aurel@gmail.com>
Date: Sun, 15 Sep 2024 14:03:50 +0300
Subject: [PATCH 1/3] Fix uploads to AWS S3 folder

---
 app/helpers/camaleon_cms/uploader_helper.rb  |  2 +-
 app/uploaders/camaleon_cms_local_uploader.rb |  7 ++++++
 app/uploaders/camaleon_cms_uploader.rb       |  4 ++--
 spec/helpers/uploader_helper_spec.rb         | 23 ++++++++++++++++----
 4 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/app/helpers/camaleon_cms/uploader_helper.rb b/app/helpers/camaleon_cms/uploader_helper.rb
index f8b830a8..f22e279c 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_local_uploader.rb b/app/uploaders/camaleon_cms_local_uploader.rb
index 0e61e6f2..c2e811b6 100644
--- a/app/uploaders/camaleon_cms_local_uploader.rb
+++ b/app/uploaders/camaleon_cms_local_uploader.rb
@@ -130,4 +130,11 @@ def delete_file(key)
   def parse_key(file_path)
     file_path.sub(@root_folder, '').cama_fix_media_key
   end
+
+  def valid_folder_path?(path)
+    return false unless super
+    return false if File.absolute_path?(path)
+
+    true
+  end
 end
diff --git a/app/uploaders/camaleon_cms_uploader.rb b/app/uploaders/camaleon_cms_uploader.rb
index 2703989d..a8e7c910 100644
--- a/app/uploaders/camaleon_cms_uploader.rb
+++ b/app/uploaders/camaleon_cms_uploader.rb
@@ -125,10 +125,10 @@ 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)
+  def valid_folder_path?(path)
     return true if path == '/'
 
-    return false if path.include?('..') || File.absolute_path?(path) || path.include?('://')
+    return false if path.include?('..') || path.include?('://')
 
     true
   end
diff --git a/spec/helpers/uploader_helper_spec.rb b/spec/helpers/uploader_helper_spec.rb
index 206d0630..c5d4fab6 100644
--- a/spec/helpers/uploader_helper_spec.rb
+++ b/spec/helpers/uploader_helper_spec.rb
@@ -56,16 +56,31 @@
   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
 
-    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)
+    context 'with local server' do
+      before { current_site.set_option('filesystem_type', 'local') }
+
+      it "doesn't 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)
+      end
+    end
+
+    context 'with AWS server' do
+      before do
+        current_site.set_option('filesystem_type', 's3')
+        allow_any_instance_of(CamaleonCmsAwsUploader).to receive(:add_file).and_return({})
+      end
+
+      it 'uploads a local file with an absolute path' do
+        expect(upload_file(File.open(@path), { folder: '/tmp/config/initializers' }).keys.include?(:error)).to be(false)
+      end
     end
   end
 

From 055cacaadbef82e5977cc585cbab1439e93ceb30 Mon Sep 17 00:00:00 2001
From: texpert <branzeanu.aurel@gmail.com>
Date: Sun, 15 Sep 2024 18:54:44 +0300
Subject: [PATCH 2/3] Introduce path travesal checks on add_folder, and remove
 check for an absolute path

---
 .../camaleon_cms/admin/media_controller.rb    |   3 +-
 app/uploaders/camaleon_cms_aws_uploader.rb    |   2 +
 app/uploaders/camaleon_cms_local_uploader.rb  |  11 ++--
 app/uploaders/camaleon_cms_uploader.rb        |   2 -
 spec/dummy/db/test.sqlite3-shm                | Bin 32768 -> 0 bytes
 spec/dummy/db/test.sqlite3-wal                |   0
 spec/helpers/uploader_helper_spec.rb          |  25 ++++------
 .../download_private_file_spec.rb             |  45 +++++++++++++++++
 .../admin/media_controller/new_folder_spec.rb |  33 ++++++++++++
 spec/requests/download_private_file_spec.rb   |  47 ------------------
 spec/uploaders/local_uploader_spec.rb         |  22 +++++---
 11 files changed, 110 insertions(+), 80 deletions(-)
 delete mode 100644 spec/dummy/db/test.sqlite3-shm
 delete mode 100644 spec/dummy/db/test.sqlite3-wal
 create mode 100644 spec/requests/admin/media_controller/download_private_file_spec.rb
 create mode 100644 spec/requests/admin/media_controller/new_folder_spec.rb
 delete mode 100644 spec/requests/download_private_file_spec.rb

diff --git a/app/controllers/camaleon_cms/admin/media_controller.rb b/app/controllers/camaleon_cms/admin/media_controller.rb
index 12e8acfe..100d75ed 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/uploaders/camaleon_cms_aws_uploader.rb b/app/uploaders/camaleon_cms_aws_uploader.rb
index bffe161b..fa3d397a 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 c2e811b6..da862445 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)
@@ -130,11 +132,4 @@ def delete_file(key)
   def parse_key(file_path)
     file_path.sub(@root_folder, '').cama_fix_media_key
   end
-
-  def valid_folder_path?(path)
-    return false unless super
-    return false if File.absolute_path?(path)
-
-    true
-  end
 end
diff --git a/app/uploaders/camaleon_cms_uploader.rb b/app/uploaders/camaleon_cms_uploader.rb
index a8e7c910..ebd85707 100644
--- a/app/uploaders/camaleon_cms_uploader.rb
+++ b/app/uploaders/camaleon_cms_uploader.rb
@@ -126,8 +126,6 @@ def self.validate_file_format(key, valid_formats = '*')
   end
 
   def valid_folder_path?(path)
-    return true if path == '/'
-
     return false if path.include?('..') || path.include?('://')
 
     true
diff --git a/spec/dummy/db/test.sqlite3-shm b/spec/dummy/db/test.sqlite3-shm
deleted file mode 100644
index fe9ac2845eca6fe6da8a63cd096d9cf9e24ece10..0000000000000000000000000000000000000000
GIT binary patch
literal 0
HcmV?d00001

literal 32768
zcmeIuAr62r3<XeTSayS60=Z=O-0|kP2&fu@!6Du^ziFC^7w55MN)9hQYV3Wk79a2P
z)%8g3H30$y2oNAZfB*pk1PBlyK!5-N0t5&UAV7cs0RjXF5FkK+009C72oNAZfB*pk
z1PBlyK!5-N0t5&UAV7cs0RjXF5FkK+009C72oNAZfB*pk1PBlyK!5-N0t5&UAV7cs
w0RjXF5FkK+009C72oNAZfB*pk1PBlyK!5-N0t5&UAV7cs0RjXF5cq?@4dk5`Qvd(}

diff --git a/spec/dummy/db/test.sqlite3-wal b/spec/dummy/db/test.sqlite3-wal
deleted file mode 100644
index e69de29b..00000000
diff --git a/spec/helpers/uploader_helper_spec.rb b/spec/helpers/uploader_helper_spec.rb
index c5d4fab6..cab35fda 100644
--- a/spec/helpers/uploader_helper_spec.rb
+++ b/spec/helpers/uploader_helper_spec.rb
@@ -63,24 +63,19 @@
     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 local server' do
-      before { current_site.set_option('filesystem_type', 'local') }
+  context 'with an absolute path' do
+    let(:file) { File.join(current_site.upload_directory, '/tmp/config/initializers/rails_tmp.png') }
 
-      it "doesn't 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)
-      end
-    end
+    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)
 
-    context 'with AWS server' do
-      before do
-        current_site.set_option('filesystem_type', 's3')
-        allow_any_instance_of(CamaleonCmsAwsUploader).to receive(:add_file).and_return({})
-      end
+      upload_file(File.open(@path), { folder: '/tmp/config/initializers' })
 
-      it 'uploads a local file with an absolute path' do
-        expect(upload_file(File.open(@path), { folder: '/tmp/config/initializers' }).keys.include?(:error)).to be(false)
-      end
+      expect(File).to exist(file)
     end
   end
 
@@ -93,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 00000000..c15b6c1a
--- /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 00000000..faa96d6c
--- /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 33818913..00000000
--- 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 a5338359..08914f7d 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

From b235c086ab7b9c7f036c7b93b340cd26a5dcf7a4 Mon Sep 17 00:00:00 2001
From: texpert <branzeanu.aurel@gmail.com>
Date: Mon, 16 Sep 2024 20:52:25 +0300
Subject: [PATCH 3/3] Add changelog item

[skip ci]
---
 CHANGELOG.md | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 9234c028..97821d25 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)