SECURITY: Validate backup chunk identifier

app/controllers/admin/backups_controller.rb

@@ -152,6 +152,8 @@ class Admin::BackupsController < Admin::AdminController
     chunk_number = params.fetch(:resumableChunkNumber)
     current_chunk_size = params.fetch(:resumableCurrentChunkSize).to_i
 
+    raise Discourse::InvalidParameters.new(:resumableIdentifier) unless valid_filename?(identifier)
+
     # path to chunk file
     chunk = BackupRestore::LocalBackupStore.chunk_path(identifier, filename, chunk_number)
     # check chunk upload status
@@ -163,13 +165,14 @@ class Admin::BackupsController < Admin::AdminController
   def upload_backup_chunk
     filename = params.fetch(:resumableFilename)
     total_size = params.fetch(:resumableTotalSize).to_i
+    identifier = params.fetch(:resumableIdentifier)
 
+    raise Discourse::InvalidParameters.new(:resumableIdentifier) unless valid_filename?(identifier)
     return render status: 415, plain: I18n.t("backup.backup_file_should_be_tar_gz") unless valid_extension?(filename)
     return render status: 415, plain: I18n.t("backup.not_enough_space_on_disk") unless has_enough_space_on_disk?(total_size)
     return render status: 415, plain: I18n.t("backup.invalid_filename") unless valid_filename?(filename)
 
     file = params.fetch(:file)
-    identifier = params.fetch(:resumableIdentifier)
     chunk_number = params.fetch(:resumableChunkNumber).to_i
     chunk_size = params.fetch(:resumableChunkSize).to_i
     current_chunk_size = params.fetch(:resumableCurrentChunkSize).to_i

spec/requests/admin/backups_controller_spec.rb

@@ -201,7 +201,9 @@ RSpec.describe Admin::BackupsController do
           described_class.any_instance.expects(:has_enough_space_on_disk?).returns(true)
 
           post "/admin/backups/upload", params: {
-            resumableFilename: invalid_filename, resumableTotalSize: 1
+            resumableFilename: invalid_filename,
+            resumableTotalSize: 1,
+            resumableIdentifier: 'test'
           }
 
           expect(response.status).to eq(415)
@@ -210,27 +212,59 @@ RSpec.describe Admin::BackupsController do
       end
     end
 
+    describe "when resumableIdentifier is invalid" do
+      it "should raise an error" do
+        filename = 'test_site-0123456789.tar.gz'
+        @paths = [backup_path(File.join('tmp', 'test', "#{filename}.part1"))]
+
+        post "/admin/backups/upload.json", params: {
+          resumableFilename: filename,
+          resumableTotalSize: 1,
+          resumableIdentifier: '../test',
+          resumableChunkNumber: '1',
+          resumableChunkSize: '1',
+          resumableCurrentChunkSize: '1',
+          file: fixture_file_upload(Tempfile.new)
+        }
+
+        expect(response.status).to eq(400)
+      end
+    end
+
     describe "when filename is valid" do
       it "should upload the file successfully" do
-        begin
+        described_class.any_instance.expects(:has_enough_space_on_disk?).returns(true)
-          described_class.any_instance.expects(:has_enough_space_on_disk?).returns(true)
 
-          filename = 'test_Site-0123456789.tar.gz'
+        filename = 'test_Site-0123456789.tar.gz'
-          @paths = [backup_path(File.join('tmp', 'test', "#{filename}.part1"))]
+        @paths = [backup_path(File.join('tmp', 'test', "#{filename}.part1"))]
 
-          post "/admin/backups/upload.json", params: {
+        post "/admin/backups/upload.json", params: {
-            resumableFilename: filename,
+          resumableFilename: filename,
-            resumableTotalSize: 1,
+          resumableTotalSize: 1,
-            resumableIdentifier: 'test',
+          resumableIdentifier: 'test',
-            resumableChunkNumber: '1',
+          resumableChunkNumber: '1',
-            resumableChunkSize: '1',
+          resumableChunkSize: '1',
-            resumableCurrentChunkSize: '1',
+          resumableCurrentChunkSize: '1',
-            file: fixture_file_upload(Tempfile.new)
+          file: fixture_file_upload(Tempfile.new)
-          }
+        }
 
-          expect(response.status).to eq(200)
+        expect(response.status).to eq(200)
-          expect(response.body).to eq("")
+        expect(response.body).to eq("")
-        end
+      end
+    end
+  end
+
+  describe "#check_backup_chunk" do
+    describe "when resumableIdentifier is invalid" do
+      it "should raise an error" do
+        get "/admin/backups/upload", params: {
+          resumableIdentifier: "../some_file",
+          resumableFilename: "test_site-0123456789.tar.gz",
+          resumableChunkNumber: '1',
+          resumableCurrentChunkSize: '1'
+        }
+
+        expect(response.status).to eq(400)
       end
     end
   end