Merge branch '17848-fix-possible-command-injections' into 'master'

Only use basename from $BACKUP variable

See merge request gitlab-org/gitlab!16464

config/brakeman.ignore

-{
-  "ignored_warnings": [
-    {
-      "warning_type": "Cross-Site Request Forgery",
-      "warning_code": 7,
-      "fingerprint": "dc562678129557cdb8b187217da304044547a3605f05fe678093dcb4b4d8bbe4",
-      "message": "'protect_from_forgery' should be called in Oauth::GeoAuthController",
-      "file": "app/controllers/oauth/geo_auth_controller.rb",
-      "line": 1,
-      "link": "http://brakemanscanner.org/docs/warning_types/cross-site_request_forgery/",
-      "code": null,
-      "render_path": null,
-      "location": {
-        "type": "controller",
-        "controller": "Oauth::GeoAuthController"
-      },
-      "user_input": null,
-      "confidence": "High",
-      "note": ""
-    }
-  ],
-  "updated": "2017-01-20 02:06:54 +0000",
-  "brakeman_version": "3.4.1"
-}

doc/development/shell_commands.md

@@ -87,7 +87,7 @@ $ cat -- -l
 hello
 ```
 
-In the GitLab codebase, we avoid the option/argument ambiguity by _always_ using `--`.
+In the GitLab codebase, we avoid the option/argument ambiguity by _always_ using `--` for commands that support it.
 
 ```ruby
 # Wrong

lib/backup/manager.rb

@@ -127,7 +127,7 @@ module Backup
         end
 
         tar_file = if ENV['BACKUP'].present?
-                     "#{ENV['BACKUP']}#{FILE_NAME_SUFFIX}"
+                     File.basename(ENV['BACKUP']) + FILE_NAME_SUFFIX
                    else
                      backup_file_list.first
                    end
@@ -235,8 +235,8 @@ module Backup
     end
 
     def tar_file
-      @tar_file ||= if ENV['BACKUP']
-                      ENV['BACKUP'] + "#{FILE_NAME_SUFFIX}"
+      @tar_file ||= if ENV['BACKUP'].present?
+                      File.basename(ENV['BACKUP']) + FILE_NAME_SUFFIX
                     else
                       "#{backup_information[:backup_created_at].strftime('%s_%Y_%m_%d_')}#{backup_information[:gitlab_version]}#{FILE_NAME_SUFFIX}"
                     end

spec/lib/backup/manager_spec.rb

@@ -21,6 +21,49 @@ describe Backup::Manager do
     $progress = @old_progress # rubocop:disable Style/GlobalVars
   end
 
+  describe '#pack' do
+    let(:backup_contents) { ['backup_contents'] }
+    let(:tar_system_options) { { out: [tar_file, 'w', Gitlab.config.backup.archive_permissions] } }
+    let(:tar_cmdline) { ['tar', '-cf', '-', *backup_contents, tar_system_options] }
+
+    let(:backup_information) do
+      {
+        backup_created_at: Time.zone.parse('2019-01-01'),
+        gitlab_version: '12.3'
+      }
+    end
+
+    before do
+      allow(ActiveRecord::Base.connection).to receive(:reconnect!)
+      allow(Kernel).to receive(:system).and_return(true)
+
+      allow(subject).to receive(:backup_contents).and_return(backup_contents)
+      allow(subject).to receive(:backup_information).and_return(backup_information)
+      allow(subject).to receive(:upload)
+    end
+
+    context 'when BACKUP is not set' do
+      let(:tar_file) { '1546300800_2019_01_01_12.3_gitlab_backup.tar' }
+
+      it 'uses the default tar file name' do
+        subject.pack
+
+        expect(Kernel).to have_received(:system).with(*tar_cmdline)
+      end
+    end
+
+    context 'when BACKUP is set' do
+      let(:tar_file) { 'custom_gitlab_backup.tar' }
+
+      it 'uses the given value as tar file name' do
+        stub_env('BACKUP', '/ignored/path/custom')
+        subject.pack
+
+        expect(Kernel).to have_received(:system).with(*tar_cmdline)
+      end
+    end
+  end
+
   describe '#remove_old' do
     let(:files) do
       [
@@ -238,7 +281,7 @@ describe Backup::Manager do
         allow(Kernel).to receive(:system).and_return(true)
         allow(YAML).to receive(:load_file).and_return(gitlab_version: Gitlab::VERSION)
 
-        stub_env('BACKUP', '1451606400_2016_01_01_1.2.3')
+        stub_env('BACKUP', '/ignored/path/1451606400_2016_01_01_1.2.3')
       end
 
       it 'unpacks the file' do