Merge branch 'file-path-validator' into 'master'

Add file path validator to custom validations module

Closes #33768

See merge request gitlab-org/gitlab!24223

changelogs/unreleased/file-path-validator.yml

+---
+title: Add custom validator for validating file path
+merge_request: 24223
+author: Rajendra Kadam
+type: added

lib/api/files.rb

@@ -61,7 +61,7 @@ module API
       end
 
       params :simple_file_params do
-        requires :file_path, type: String, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
+        requires :file_path, type: String, file_path: true, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
         requires :branch, type: String, desc: 'Name of the branch to commit into. To create a new branch, also provide `start_branch`.', allow_blank: false
         requires :commit_message, type: String, allow_blank: false, desc: 'Commit message'
         optional :start_branch, type: String, desc: 'Name of the branch to start the new commit from'
@@ -85,7 +85,7 @@ module API
 
       desc 'Get blame file metadata from repository'
       params do
-        requires :file_path, type: String, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
+        requires :file_path, type: String, file_path: true, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
         requires :ref, type: String, desc: 'The name of branch, tag or commit', allow_blank: false
       end
       head ":id/repository/files/:file_path/blame", requirements: FILE_ENDPOINT_REQUIREMENTS do
@@ -96,7 +96,7 @@ module API
 
       desc 'Get blame file from the repository'
       params do
-        requires :file_path, type: String, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
+        requires :file_path, type: String, file_path: true, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
         requires :ref, type: String, desc: 'The name of branch, tag or commit', allow_blank: false
       end
       get ":id/repository/files/:file_path/blame", requirements: FILE_ENDPOINT_REQUIREMENTS do
@@ -110,7 +110,7 @@ module API
 
       desc 'Get raw file metadata from repository'
       params do
-        requires :file_path, type: String, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
+        requires :file_path, type: String, file_path: true, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
         requires :ref, type: String, desc: 'The name of branch, tag or commit', allow_blank: false
       end
       head ":id/repository/files/:file_path/raw", requirements: FILE_ENDPOINT_REQUIREMENTS do
@@ -121,7 +121,7 @@ module API
 
       desc 'Get raw file contents from the repository'
       params do
-        requires :file_path, type: String, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
+        requires :file_path, type: String, file_path: true, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
         requires :ref, type: String, desc: 'The name of branch, tag commit', allow_blank: false
       end
       get ":id/repository/files/:file_path/raw", requirements: FILE_ENDPOINT_REQUIREMENTS do
@@ -135,7 +135,7 @@ module API
 
       desc 'Get file metadata from repository'
       params do
-        requires :file_path, type: String, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
+        requires :file_path, type: String, file_path: true, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
         requires :ref, type: String, desc: 'The name of branch, tag or commit', allow_blank: false
       end
       head ":id/repository/files/:file_path", requirements: FILE_ENDPOINT_REQUIREMENTS do
@@ -146,7 +146,7 @@ module API
 
       desc 'Get a file from the repository'
       params do
-        requires :file_path, type: String, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
+        requires :file_path, type: String, file_path: true, desc: 'The url encoded path to the file. Ex. lib%2Fclass%2Erb'
         requires :ref, type: String, desc: 'The name of branch, tag or commit', allow_blank: false
       end
       get ":id/repository/files/:file_path", requirements: FILE_ENDPOINT_REQUIREMENTS do

lib/api/helpers/custom_validators.rb

@@ -3,6 +3,17 @@
 module API
   module Helpers
     module CustomValidators
+      class FilePath < Grape::Validations::Base
+        def validate_param!(attr_name, params)
+          path = params[attr_name]
+
+          Gitlab::Utils.check_path_traversal!(path)
+        rescue StandardError
+          raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)],
+                                               message: "should be a valid file path"
+        end
+      end
+
       class Absence < Grape::Validations::Base
         def validate_param!(attr_name, params)
           return if params.respond_to?(:key?) && !params.key?(attr_name)
@@ -38,6 +49,7 @@ module API
   end
 end
 
+Grape::Validations.register_validator(:file_path, ::API::Helpers::CustomValidators::FilePath)
 Grape::Validations.register_validator(:absence, ::API::Helpers::CustomValidators::Absence)
 Grape::Validations.register_validator(:integer_none_any, ::API::Helpers::CustomValidators::IntegerNoneAny)
 Grape::Validations.register_validator(:array_none_any, ::API::Helpers::CustomValidators::ArrayNoneAny)

lib/gitlab/utils.rb

@@ -5,10 +5,20 @@ module Gitlab
     extend self
 
     # Ensure that the relative path will not traverse outside the base directory
-    def check_path_traversal!(path)
-      raise StandardError.new("Invalid path") if path.start_with?("..#{File::SEPARATOR}") ||
+    # We url decode the path to avoid passing invalid paths forward in url encoded format.
+    # We are ok to pass some double encoded paths to File.open since they won't resolve.
+    # Also see https://gitlab.com/gitlab-org/gitlab/-/merge_requests/24223#note_284122580
+    # It also checks for ALT_SEPARATOR aka '\' (forward slash)
+    def check_path_traversal!(path, allowed_absolute: false)
+      path = CGI.unescape(path)
+
+      if path.start_with?("..#{File::SEPARATOR}", "..#{File::ALT_SEPARATOR}") ||
           path.include?("#{File::SEPARATOR}..#{File::SEPARATOR}") ||
-          path.end_with?("#{File::SEPARATOR}..")
+          path.end_with?("#{File::SEPARATOR}..") ||
+          (!allowed_absolute && Pathname.new(path).absolute?)
+
+        raise StandardError.new("Invalid path")
+      end
 
       path
     end

spec/lib/api/helpers/custom_validators_spec.rb

@@ -24,7 +24,38 @@ describe API::Helpers::CustomValidators do
 
     context 'invalid parameters' do
       it 'raises a validation error' do
-        expect_validation_error({ 'test' => 'some_value' })
+        expect_validation_error('test' => 'some_value')
+      end
+    end
+  end
+
+  describe API::Helpers::CustomValidators::FilePath do
+    subject do
+      described_class.new(['test'], {}, false, scope.new)
+    end
+
+    context 'valid file path' do
+      it 'does not raise a validation error' do
+        expect_no_validation_error('test' => './foo')
+        expect_no_validation_error('test' => './bar.rb')
+        expect_no_validation_error('test' => 'foo%2Fbar%2Fnew%2Ffile.rb')
+        expect_no_validation_error('test' => 'foo%2Fbar%2Fnew')
+        expect_no_validation_error('test' => 'foo%252Fbar%252Fnew%252Ffile.rb')
+      end
+    end
+
+    context 'invalid file path' do
+      it 'raise a validation error' do
+        expect_validation_error('test' => '../foo')
+        expect_validation_error('test' => '../')
+        expect_validation_error('test' => 'foo/../../bar')
+        expect_validation_error('test' => 'foo/../')
+        expect_validation_error('test' => 'foo/..')
+        expect_validation_error('test' => '../')
+        expect_validation_error('test' => '..\\')
+        expect_validation_error('test' => '..\/')
+        expect_validation_error('test' => '%2e%2e%2f')
+        expect_validation_error('test' => '/etc/passwd')
       end
     end
   end
@@ -36,12 +67,12 @@ describe API::Helpers::CustomValidators do
 
     context 'valid parameters' do
       it 'does not raise a validation error' do
-        expect_no_validation_error({ 'test' => 2 })
-        expect_no_validation_error({ 'test' => 100 })
-        expect_no_validation_error({ 'test' => 'None' })
-        expect_no_validation_error({ 'test' => 'Any' })
-        expect_no_validation_error({ 'test' => 'none' })
-        expect_no_validation_error({ 'test' => 'any' })
+        expect_no_validation_error('test' => 2)
+        expect_no_validation_error('test' => 100)
+        expect_no_validation_error('test' => 'None')
+        expect_no_validation_error('test' => 'Any')
+        expect_no_validation_error('test' => 'none')
+        expect_no_validation_error('test' => 'any')
       end
     end
 
@@ -59,18 +90,18 @@ describe API::Helpers::CustomValidators do
 
     context 'valid parameters' do
       it 'does not raise a validation error' do
-        expect_no_validation_error({ 'test' => [] })
-        expect_no_validation_error({ 'test' => [1, 2, 3] })
-        expect_no_validation_error({ 'test' => 'None' })
-        expect_no_validation_error({ 'test' => 'Any' })
-        expect_no_validation_error({ 'test' => 'none' })
-        expect_no_validation_error({ 'test' => 'any' })
+        expect_no_validation_error('test' => [])
+        expect_no_validation_error('test' => [1, 2, 3])
+        expect_no_validation_error('test' => 'None')
+        expect_no_validation_error('test' => 'Any')
+        expect_no_validation_error('test' => 'none')
+        expect_no_validation_error('test' => 'any')
       end
     end
 
     context 'invalid parameters' do
       it 'raises a validation error' do
-        expect_validation_error({ 'test' => 'some_other_string' })
+        expect_validation_error('test' => 'some_other_string')
       end
     end
   end

spec/lib/gitlab/utils_spec.rb

@@ -31,6 +31,14 @@ describe Gitlab::Utils do
     it 'does nothing for a safe string' do
       expect(check_path_traversal!('./foo')).to eq('./foo')
     end
+
+    it 'does nothing if an absolute path is allowed' do
+      expect(check_path_traversal!('/etc/folder/path', allowed_absolute: true)). to eq('/etc/folder/path')
+    end
+
+    it 'raises exception if an absolute path is not allowed' do
+      expect { check_path_traversal!('/etc/folder/path') }.to raise_error(/Invalid path/)
+    end
   end
 
   describe '.slugify' do

spec/requests/api/files_spec.rb

@@ -7,6 +7,8 @@ describe API::Files do
   let!(:project) { create(:project, :repository, namespace: user.namespace ) }
   let(:guest) { create(:user) { |u| project.add_guest(u) } }
   let(:file_path) { "files%2Fruby%2Fpopen%2Erb" }
+  let(:rouge_file_path) { "%2e%2e%2f" }
+  let(:invalid_file_message) { 'file_path should be a valid file path' }
   let(:params) do
     {
       ref: 'master'
@@ -55,6 +57,12 @@ describe API::Files do
 
   describe "HEAD /projects/:id/repository/files/:file_path" do
     shared_examples_for 'repository files' do
+      it 'returns 400 when file path is invalid' do
+        head api(route(rouge_file_path), current_user), params: params
+
+        expect(response).to have_gitlab_http_status(:bad_request)
+      end
+
       it 'returns file attributes in headers' do
         head api(route(file_path), current_user), params: params
 
@@ -145,6 +153,13 @@ describe API::Files do
 
   describe "GET /projects/:id/repository/files/:file_path" do
     shared_examples_for 'repository files' do
+      it 'returns 400 for invalid file path' do
+        get api(route(rouge_file_path), current_user), params: params
+
+        expect(response).to have_gitlab_http_status(:bad_request)
+        expect(json_response['error']).to eq(invalid_file_message)
+      end
+
       it 'returns file attributes as json' do
         get api(route(file_path), current_user), params: params
 
@@ -302,6 +317,13 @@ describe API::Files do
           .to eq('c440cd09bae50c4632cc58638ad33c6aa375b6109d811e76a9cc3a613c1e8887')
       end
 
+      it 'returns 400 when file path is invalid' do
+        get api(route(rouge_file_path) + '/blame', current_user), params: params
+
+        expect(response).to have_gitlab_http_status(:bad_request)
+        expect(json_response['error']).to eq(invalid_file_message)
+      end
+
       it 'returns blame file attributes as json' do
         get api(route(file_path) + '/blame', current_user), params: params
 
@@ -418,6 +440,13 @@ describe API::Files do
 
   describe "GET /projects/:id/repository/files/:file_path/raw" do
     shared_examples_for 'repository raw files' do
+      it 'returns 400 when file path is invalid' do
+        get api(route(rouge_file_path) + "/raw", current_user), params: params
+
+        expect(response).to have_gitlab_http_status(:bad_request)
+        expect(json_response['error']).to eq(invalid_file_message)
+      end
+
       it 'returns raw file info' do
         url = route(file_path) + "/raw"
         expect(Gitlab::Workhorse).to receive(:send_git_blob)
@@ -535,6 +564,13 @@ describe API::Files do
       }
     end
 
+    it 'returns 400 when file path is invalid' do
+      post api(route(rouge_file_path), user), params: params
+
+      expect(response).to have_gitlab_http_status(:bad_request)
+      expect(json_response['error']).to eq(invalid_file_message)
+    end
+
     it "creates a new file in project repo" do
       post api(route(file_path), user), params: params
 
@@ -662,6 +698,17 @@ describe API::Files do
       expect(response).to have_gitlab_http_status(:ok)
     end
 
+    it "returns 400 when file path is invalid" do
+      last_commit = Gitlab::Git::Commit
+                        .last_for_path(project.repository, 'master', URI.unescape(file_path))
+      params_with_correct_id = params.merge(last_commit_id: last_commit.id)
+
+      put api(route(rouge_file_path), user), params: params_with_correct_id
+
+      expect(response).to have_gitlab_http_status(:bad_request)
+      expect(json_response['error']).to eq(invalid_file_message)
+    end
+
     it "returns a 400 bad request if no params given" do
       put api(route(file_path), user)
 
@@ -690,6 +737,13 @@ describe API::Files do
       }
     end
 
+    it 'returns 400 when file path is invalid' do
+      delete api(route(rouge_file_path), user), params: params
+
+      expect(response).to have_gitlab_http_status(:bad_request)
+      expect(json_response['error']).to eq(invalid_file_message)
+    end
+
     it "deletes existing file in project repo" do
       delete api(route(file_path), user), params: params