Merge branch 'feat/255185' into 'master'

Area(utils) add: check_allowed_absolute_path_and_path_traversal

See merge request gitlab-org/gitlab!81321

doc/development/secure_coding_guidelines.md

@@ -460,8 +460,7 @@ parameter when using `check_allowed_absolute_path!()`.
 To use a combination of both checks, follow the example below:
 
 ```ruby
-path = Gitlab::Utils.check_path_traversal!(path)
-Gitlab::Utils.check_allowed_absolute_path!(path, path_allowlist)
+Gitlab::Utils.check_allowed_absolute_path_and_path_traversal!(path, path_allowlist)
 ```
 
 In the REST API, we have the [`FilePath`](https://gitlab.com/gitlab-org/security/gitlab/-/blob/master/lib/api/validations/validators/file_path.rb)

lib/api/validations/validators/file_path.rb

@@ -8,8 +8,7 @@ module API
           options = @option.is_a?(Hash) ? @option : {}
           path_allowlist = options.fetch(:allowlist, [])
           path = params[attr_name]
-          path = Gitlab::Utils.check_path_traversal!(path)
-          Gitlab::Utils.check_allowed_absolute_path!(path, path_allowlist)
+          Gitlab::Utils.check_allowed_absolute_path_and_path_traversal!(path, path_allowlist)
         rescue StandardError
           raise Grape::Exceptions::Validation.new(
             params: [@scope.full_name(attr_name)],

lib/gitlab/utils.rb

@@ -37,6 +37,13 @@ module Gitlab
       raise StandardError, "path #{path} is not allowed"
     end
 
+    def check_allowed_absolute_path_and_path_traversal!(path, path_allowlist)
+      traversal_path = check_path_traversal!(path)
+      raise StandardError, "path is not a string!" unless traversal_path.is_a?(String)
+
+      check_allowed_absolute_path!(traversal_path, path_allowlist)
+    end
+
     def decode_path(encoded_path)
       decoded = CGI.unescape(encoded_path)
       if decoded != CGI.unescape(decoded)

spec/lib/gitlab/utils_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe Gitlab::Utils do
 
   delegate :to_boolean, :boolean_to_yes_no, :slugify, :random_string, :which,
            :ensure_array_from_string, :to_exclusive_sentence, :bytes_to_megabytes,
-           :append_path, :check_path_traversal!, :allowlisted?, :check_allowed_absolute_path!, :decode_path, :ms_to_round_sec, to: :described_class
+           :append_path, :check_path_traversal!, :allowlisted?, :check_allowed_absolute_path!, :decode_path, :ms_to_round_sec, :check_allowed_absolute_path_and_path_traversal!, to: :described_class
 
   describe '.check_path_traversal!' do
     it 'detects path traversal in string without any separators' do
@@ -58,6 +58,65 @@ RSpec.describe Gitlab::Utils do
     end
   end
 
+  describe '.check_allowed_absolute_path_and_path_traversal!' do
+    let(:allowed_paths) { %w[/home/foo ./foo .test/foo ..test/foo dir/..foo.rb dir/.foo.rb] }
+
+    it 'detects path traversal in string without any separators' do
+      expect { check_allowed_absolute_path_and_path_traversal!('.', allowed_paths) }.to raise_error(/Invalid path/)
+      expect { check_allowed_absolute_path_and_path_traversal!('..', allowed_paths) }.to raise_error(/Invalid path/)
+    end
+
+    it 'detects path traversal at the start of the string' do
+      expect { check_allowed_absolute_path_and_path_traversal!('../foo', allowed_paths) }.to raise_error(/Invalid path/)
+      expect { check_allowed_absolute_path_and_path_traversal!('..\\foo', allowed_paths) }.to raise_error(/Invalid path/)
+    end
+
+    it 'detects path traversal at the start of the string, even to just the subdirectory' do
+      expect { check_allowed_absolute_path_and_path_traversal!('../', allowed_paths) }.to raise_error(/Invalid path/)
+      expect { check_allowed_absolute_path_and_path_traversal!('..\\', allowed_paths) }.to raise_error(/Invalid path/)
+      expect { check_allowed_absolute_path_and_path_traversal!('/../', allowed_paths) }.to raise_error(/Invalid path/)
+      expect { check_allowed_absolute_path_and_path_traversal!('\\..\\', allowed_paths) }.to raise_error(/Invalid path/)
+    end
+
+    it 'detects path traversal in the middle of the string' do
+      expect { check_allowed_absolute_path_and_path_traversal!('foo/../../bar', allowed_paths) }.to raise_error(/Invalid path/)
+      expect { check_allowed_absolute_path_and_path_traversal!('foo\\..\\..\\bar', allowed_paths) }.to raise_error(/Invalid path/)
+      expect { check_allowed_absolute_path_and_path_traversal!('foo/..\\bar', allowed_paths) }.to raise_error(/Invalid path/)
+      expect { check_allowed_absolute_path_and_path_traversal!('foo\\../bar', allowed_paths) }.to raise_error(/Invalid path/)
+      expect { check_allowed_absolute_path_and_path_traversal!('foo/..\\..\\..\\..\\../bar', allowed_paths) }.to raise_error(/Invalid path/)
+    end
+
+    it 'detects path traversal at the end of the string when slash-terminates' do
+      expect { check_allowed_absolute_path_and_path_traversal!('foo/../', allowed_paths) }.to raise_error(/Invalid path/)
+      expect { check_allowed_absolute_path_and_path_traversal!('foo\\..\\', allowed_paths) }.to raise_error(/Invalid path/)
+    end
+
+    it 'detects path traversal at the end of the string' do
+      expect { check_allowed_absolute_path_and_path_traversal!('foo/..', allowed_paths) }.to raise_error(/Invalid path/)
+      expect { check_allowed_absolute_path_and_path_traversal!('foo\\..', allowed_paths) }.to raise_error(/Invalid path/)
+    end
+
+    it 'does not return errors for a safe string' do
+      expect(check_allowed_absolute_path_and_path_traversal!('./foo', allowed_paths)).to be_nil
+      expect(check_allowed_absolute_path_and_path_traversal!('.test/foo', allowed_paths)).to be_nil
+      expect(check_allowed_absolute_path_and_path_traversal!('..test/foo', allowed_paths)).to be_nil
+      expect(check_allowed_absolute_path_and_path_traversal!('dir/..foo.rb', allowed_paths)).to be_nil
+      expect(check_allowed_absolute_path_and_path_traversal!('dir/.foo.rb', allowed_paths)).to be_nil
+    end
+
+    it 'raises error for a non-string' do
+      expect {check_allowed_absolute_path_and_path_traversal!(nil, allowed_paths)}.to raise_error(StandardError)
+    end
+
+    it 'raises an exception if an absolute path is not allowed' do
+      expect { check_allowed_absolute_path!('/etc/passwd', allowed_paths) }.to raise_error(StandardError)
+    end
+
+    it 'does nothing for an allowed absolute path' do
+      expect(check_allowed_absolute_path!('/home/foo', allowed_paths)).to be_nil
+    end
+  end
+
   describe '.allowlisted?' do
     let(:allowed_paths) { ['/home/foo', '/foo/bar', '/etc/passwd']}