Internal allowed response disk path is ignored

README.md

@@ -122,14 +122,14 @@ Rails application:
 
 ## Updating VCR fixtures
 
-In order to generate new VCR fixtures you need to have a local GitLab instance 
-running and have next data: 
+In order to generate new VCR fixtures you need to have a local GitLab instance
+running and have next data:
 
 1. gitlab-org/gitlab-test project.
 2. SSH key with access to the project and ID 1 that belongs to Administrator.
 3. SSH key without access to the project and ID 2.
 
-You also need to modify `secret` variable at `spec/gitlab_net_spec.rb` so tests 
+You also need to modify `secret` variable at `spec/gitlab_net_spec.rb` so tests
 can connect to your local instance.
 
 ## Contributing

hooks/post-receive

@@ -6,13 +6,12 @@
 refs = $stdin.read
 key_id = ENV.delete('GL_ID')
 gl_repository = ENV['GL_REPOSITORY']
-repo_path = Dir.pwd
 
 require_relative '../lib/gitlab_custom_hook'
 require_relative '../lib/gitlab_post_receive'
 
-if GitlabPostReceive.new(gl_repository, repo_path, key_id, refs).exec &&
-    GitlabCustomHook.new(repo_path, key_id).post_receive(refs)
+if GitlabPostReceive.new(gl_repository, key_id, refs).exec &&
+    GitlabCustomHook.new(Dir.pwd, key_id).post_receive(refs)
   exit 0
 else
   exit 1

hooks/pre-receive

@@ -9,7 +9,7 @@ protocol = ENV.delete('GL_PROTOCOL')
 repo_path = Dir.pwd
 gl_repository = ENV['GL_REPOSITORY']
 
-def increase_reference_counter(gl_repository, repo_path)
+def increase_reference_counter(gl_repository)
   result = GitlabNet.new.pre_receive(gl_repository)
 
   result['reference_counter_increased']
@@ -23,9 +23,9 @@ require_relative '../lib/gitlab_net'
 # last so that it only runs if everything else succeeded. On post-receive on the
 # other hand, we run GitlabPostReceive first because the push is already done
 # and we don't want to skip it if the custom hook fails.
-if GitlabAccess.new(gl_repository, repo_path, key_id, refs, protocol).exec &&
+if GitlabAccess.new(gl_repository, key_id, refs, protocol).exec &&
     GitlabCustomHook.new(repo_path, key_id).pre_receive(refs) &&
-    increase_reference_counter(gl_repository, repo_path)
+    increase_reference_counter(gl_repository)
   exit 0
 else
   exit 1

lib/gitlab_access.rb

@@ -11,12 +11,11 @@ class GitlabAccess
 
   include NamesHelper
 
-  attr_reader :config, :gl_repository, :repo_path, :changes, :protocol
+  attr_reader :config, :gl_repository, :changes, :protocol
 
-  def initialize(gl_repository, repo_path, actor, changes, protocol)
+  def initialize(gl_repository, actor, changes, protocol)
     @config = GitlabConfig.new
     @gl_repository = gl_repository
-    @repo_path = repo_path.strip
     @actor = actor
     @changes = changes.lines
     @protocol = protocol
@@ -24,7 +23,7 @@ class GitlabAccess
 
   def exec
     status = GitlabMetrics.measure('check-access:git-receive-pack') do
-      api.check_access('git-receive-pack', @gl_repository, @repo_path, @actor, @changes, @protocol, env: ObjectDirsHelper.all_attributes.to_json)
+      api.check_access('git-receive-pack', @gl_repository, @actor, @changes, @protocol, env: ObjectDirsHelper.all_attributes.to_json)
     end
 
     raise AccessDeniedError, status.message unless status.allowed?

lib/gitlab_access_status.rb

@@ -3,12 +3,11 @@ require 'json'
 class GitAccessStatus
   attr_reader :message, :gl_repository, :gl_username, :repository_path, :gitaly
 
-  def initialize(status, message, gl_repository:, gl_username:, repository_path:, gitaly:)
+  def initialize(status, message, gl_repository:, gl_username:, gitaly:)
     @status = status
     @message = message
     @gl_repository = gl_repository
     @gl_username = gl_username
-    @repository_path = repository_path
     @gitaly = gitaly
   end
 
@@ -18,7 +17,6 @@ class GitAccessStatus
         values["message"],
         gl_repository: values["gl_repository"],
         gl_username: values["gl_username"],
-        repository_path: values["repository_path"],
         gitaly: values["gitaly"])
   end
 

lib/gitlab_net.rb

@@ -15,14 +15,13 @@ class GitlabNet # rubocop:disable Metrics/ClassLength
   CHECK_TIMEOUT = 5
   READ_TIMEOUT = 300
 
-  def check_access(cmd, gl_repository, repo, actor, changes, protocol, env: {})
+  def check_access(cmd, gl_repository, actor, changes, protocol, env: {})
     changes = changes.join("\n") unless changes.is_a?(String)
 
     params = {
       action: cmd,
       changes: changes,
       gl_repository: gl_repository,
-      project: sanitize_path(repo),
       protocol: protocol,
       env: env
     }
@@ -43,7 +42,6 @@ class GitlabNet # rubocop:disable Metrics/ClassLength
                           'API is not accessible',
                           gl_repository: nil,
                           gl_username: nil,
-                          repository_path: nil,
                           gitaly: nil)
     end
   end
@@ -72,11 +70,10 @@ class GitlabNet # rubocop:disable Metrics/ClassLength
     JSON.parse(resp.body) rescue {}
   end
 
-  def merge_request_urls(gl_repository, repo_path, changes)
+  def merge_request_urls(gl_repository, changes)
     changes = changes.join("\n") unless changes.is_a?(String)
     changes = changes.encode('UTF-8', 'ASCII', invalid: :replace, replace: '')
-    url = "#{host}/merge_request_urls?project=#{URI.escape(repo_path)}&changes=#{URI.escape(changes)}"
-    url += "&gl_repository=#{URI.escape(gl_repository)}" if gl_repository
+    url = "#{host}/merge_request_urls?changes=#{URI.escape(changes)}&gl_repository=#{URI.escape(gl_repository)}"
     resp = get(url)
 
     if resp.code == '200'

lib/gitlab_post_receive.rb

@@ -8,12 +8,11 @@ require 'securerandom'
 class GitlabPostReceive
   include NamesHelper
 
-  attr_reader :config, :gl_repository, :repo_path, :changes, :jid
+  attr_reader :config, :gl_repository, :changes, :jid
 
-  def initialize(gl_repository, repo_path, actor, changes)
+  def initialize(gl_repository, actor, changes)
     @config = GitlabConfig.new
     @gl_repository = gl_repository
-    @repo_path = repo_path.strip
     @actor = actor
     @changes = changes
     @jid = SecureRandom.hex(12)

lib/gitlab_shell.rb

@@ -19,7 +19,6 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
   GL_PROTOCOL = 'ssh'.freeze
 
   attr_accessor :key_id, :gl_repository, :repo_name, :command, :git_access, :username
-  attr_reader :repo_path
 
   def initialize(key_id)
     @key_id = key_id
@@ -105,7 +104,6 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
 
     raise AccessDeniedError, status.message unless status.allowed?
 
-    self.repo_path = status.repository_path
     @gl_repository = status.gl_repository
     @gitaly = status.gitaly
     @username = status.gl_username
@@ -123,7 +121,7 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
     end
 
     executable = @command
-    args = [repo_path]
+    args = []
 
     if GITALY_MIGRATED_COMMANDS.key?(executable) && @gitaly
       executable = GITALY_MIGRATED_COMMANDS[executable]
@@ -264,11 +262,4 @@ class GitlabShell # rubocop:disable Metrics/ClassLength
       return false
     end
   end
-
-  def repo_path=(repo_path)
-    raise ArgumentError, "Repository path not provided. Please make sure you're using GitLab v8.10 or later." unless repo_path
-    raise InvalidRepositoryPathError if File.absolute_path(repo_path) != repo_path
-
-    @repo_path = repo_path
-  end
 end

spec/gitlab_access_spec.rb

@@ -4,30 +4,23 @@ require 'gitlab_access'
 describe GitlabAccess do
   let(:repository_path) { "/home/git/repositories" }
   let(:repo_name)   { 'dzaporozhets/gitlab-ci' }
-  let(:repo_path)  { File.join(repository_path, repo_name) + ".git" }
   let(:api) do
     double(GitlabNet).tap do |api|
       api.stub(check_access: GitAccessStatus.new(true,
                                                  'ok',
                                                  gl_repository: 'project-1',
                                                  gl_username: 'testuser',
-                                                 repository_path: '/home/git/repositories',
                                                  gitaly: nil))
     end
   end
   subject do
-    GitlabAccess.new(nil, repo_path, 'key-123', 'wow', 'ssh').tap do |access|
+    GitlabAccess.new(nil, 'key-123', 'wow', 'ssh').tap do |access|
       access.stub(exec_cmd: :exec_called)
       access.stub(api: api)
     end
   end
 
-  before do
-    GitlabConfig.any_instance.stub(repos_path: repository_path)
-  end
-
   describe :initialize do
-    it { subject.repo_path.should == repo_path }
     it { subject.changes.should == ['wow'] }
     it { subject.protocol.should == 'ssh' }
   end
@@ -48,7 +41,6 @@ describe GitlabAccess do
                   'denied',
                   gl_repository: nil,
                   gl_username: nil,
-                  repository_path: nil,
                   gitaly: nil
                 ))
       end

spec/gitlab_net_spec.rb

@@ -100,29 +100,23 @@ describe GitlabNet, vcr: true do
     let(:encoded_changes) { "123456%20789012%20refs/heads/test%0A654321%20210987%20refs/tags/tag" }
 
     it "sends the given arguments as encoded URL parameters" do
-      gitlab_net.should_receive(:get).with("#{host}/merge_request_urls?project=#{project}&changes=#{encoded_changes}&gl_repository=#{gl_repository}")
+      gitlab_net.should_receive(:get).with("#{host}/merge_request_urls?changes=#{encoded_changes}&gl_repository=#{gl_repository}")
 
-      gitlab_net.merge_request_urls(gl_repository, project, changes)
-    end
-
-    it "omits the gl_repository parameter if it's nil" do
-      gitlab_net.should_receive(:get).with("#{host}/merge_request_urls?project=#{project}&changes=#{encoded_changes}")
-
-      gitlab_net.merge_request_urls(nil, project, changes)
+      gitlab_net.merge_request_urls(gl_repository, changes)
     end
 
     it "returns an empty array when the result cannot be parsed as JSON" do
       response = double(:response, code: '200', body: '')
       allow(gitlab_net).to receive(:get).and_return(response)
 
-      expect(gitlab_net.merge_request_urls(gl_repository, project, changes)).to eq([])
+      expect(gitlab_net.merge_request_urls(gl_repository, changes)).to eq([])
     end
 
     it "returns an empty array when the result's status is not 200" do
       response = double(:response, code: '500', body: '[{}]')
       allow(gitlab_net).to receive(:get).and_return(response)
 
-      expect(gitlab_net.merge_request_urls(gl_repository, project, changes)).to eq([])
+      expect(gitlab_net.merge_request_urls(gl_repository, changes)).to eq([])
     end
   end
 
@@ -266,7 +260,7 @@ describe GitlabNet, vcr: true do
     context 'ssh key with access nil, to project' do
       it 'should allow pull access for host' do
         VCR.use_cassette("allowed-pull") do
-          access = gitlab_net.check_access('git-receive-pack', nil, project, key, changes, 'ssh')
+          access = gitlab_net.check_access('git-receive-pack', nil, key, changes, 'ssh')
           access.allowed?.should be_true
         end
       end
@@ -274,13 +268,13 @@ describe GitlabNet, vcr: true do
       it 'adds the secret_token to the request' do
         VCR.use_cassette("allowed-pull") do
           Net::HTTP::Post.any_instance.should_receive(:set_form_data).with(hash_including(secret_token: secret))
-          gitlab_net.check_access('git-receive-pack', nil, project, key, changes, 'ssh')
+          gitlab_net.check_access('git-receive-pack', nil, key, changes, 'ssh')
         end
       end
 
       it 'should allow push access for host' do
         VCR.use_cassette("allowed-push") do
-          access = gitlab_net.check_access('git-upload-pack', nil, project, key, changes, 'ssh')
+          access = gitlab_net.check_access('git-upload-pack', nil, key, changes, 'ssh')
           access.allowed?.should be_true
         end
       end
@@ -289,7 +283,7 @@ describe GitlabNet, vcr: true do
     context 'ssh access has been disabled' do
       it 'should deny pull access for host' do
         VCR.use_cassette('ssh-pull-disabled') do
-          access = gitlab_net.check_access('git-upload-pack', nil, project, key, changes, 'ssh')
+          access = gitlab_net.check_access('git-upload-pack', nil, key, changes, 'ssh')
           access.allowed?.should be_false
           access.message.should eq 'Git access over SSH is not allowed'
         end
@@ -297,7 +291,7 @@ describe GitlabNet, vcr: true do
 
       it 'should deny push access for host' do
         VCR.use_cassette('ssh-push-disabled') do
-          access = gitlab_net.check_access('git-receive-pack', nil, project, key, changes, 'ssh')
+          access = gitlab_net.check_access('git-receive-pack', nil, key, changes, 'ssh')
           access.allowed?.should be_false
           access.message.should eq 'Git access over SSH is not allowed'
         end
@@ -307,7 +301,7 @@ describe GitlabNet, vcr: true do
     context 'http access has been disabled' do
       it 'should deny pull access for host' do
         VCR.use_cassette('http-pull-disabled') do
-          access = gitlab_net.check_access('git-upload-pack', nil, project, key, changes, 'http')
+          access = gitlab_net.check_access('git-upload-pack', nil, key, changes, 'http')
           access.allowed?.should be_false
           access.message.should eq 'Pulling over HTTP is not allowed.'
         end
@@ -315,7 +309,7 @@ describe GitlabNet, vcr: true do
 
       it 'should deny push access for host' do
         VCR.use_cassette("http-push-disabled") do
-          access = gitlab_net.check_access('git-receive-pack', nil, project, key, changes, 'http')
+          access = gitlab_net.check_access('git-receive-pack', nil, key, changes, 'http')
           access.allowed?.should be_false
           access.message.should eq 'Pushing over HTTP is not allowed.'
         end
@@ -325,21 +319,21 @@ describe GitlabNet, vcr: true do
     context 'ssh key without access to project' do
       it 'should deny pull access for host' do
         VCR.use_cassette("ssh-pull-project-denied") do
-          access = gitlab_net.check_access('git-receive-pack', nil, project, key2, changes, 'ssh')
+          access = gitlab_net.check_access('git-receive-pack', nil, key2, changes, 'ssh')
           access.allowed?.should be_false
         end
       end
 
       it 'should deny push access for host' do
         VCR.use_cassette("ssh-push-project-denied") do
-          access = gitlab_net.check_access('git-upload-pack', nil, project, key2, changes, 'ssh')
+          access = gitlab_net.check_access('git-upload-pack', nil, key2, changes, 'ssh')
           access.allowed?.should be_false
         end
       end
 
       it 'should deny push access for host (with user)' do
         VCR.use_cassette("ssh-push-project-denied-with-user") do
-          access = gitlab_net.check_access('git-upload-pack', nil, project, 'user-2', changes, 'ssh')
+          access = gitlab_net.check_access('git-upload-pack', nil, 'user-2', changes, 'ssh')
           access.allowed?.should be_false
         end
       end
@@ -348,7 +342,7 @@ describe GitlabNet, vcr: true do
     it "raises an exception if the connection fails" do
       Net::HTTP.any_instance.stub(:request).and_raise(StandardError)
       expect {
-        gitlab_net.check_access('git-upload-pack', nil, project, 'user-1', changes, 'ssh')
+        gitlab_net.check_access('git-upload-pack', nil, 'user-1', changes, 'ssh')
       }.to raise_error(GitlabNet::ApiUnreachableError)
     end
   end

spec/gitlab_post_receive_spec.rb

@@ -3,15 +3,13 @@ require 'spec_helper'
 require 'gitlab_post_receive'
 
 describe GitlabPostReceive do
-  let(:repository_path) { "/home/git/repositories" }
   let(:repo_name) { 'dzaporozhets/gitlab-ci' }
   let(:actor) { 'key-123' }
   let(:changes) { "123456 789012 refs/heads/tést\n654321 210987 refs/tags/tag" }
   let(:wrongly_encoded_changes) { changes.encode("ISO-8859-1").force_encoding("UTF-8") }
   let(:base64_changes) { Base64.encode64(wrongly_encoded_changes) }
-  let(:repo_path) { File.join(repository_path, repo_name) + ".git" }
   let(:gl_repository) { "project-1" }
-  let(:gitlab_post_receive) { GitlabPostReceive.new(gl_repository, repo_path, actor, wrongly_encoded_changes) }
+  let(:gitlab_post_receive) { GitlabPostReceive.new(gl_repository, actor, wrongly_encoded_changes) }
   let(:broadcast_message) { "test " * 10 + "message " * 10 }
   let(:enqueued_at) { Time.new(2016, 6, 23, 6, 59) }
   let(:new_merge_request_urls) do
@@ -31,7 +29,6 @@ describe GitlabPostReceive do
 
   before do
     $logger = double('logger').as_null_object # Global vars are bad
-    GitlabConfig.any_instance.stub(repos_path: repository_path)
   end
 
   describe "#exec" do

spec/gitlab_shell_spec.rb

@@ -19,15 +19,15 @@ describe GitlabShell do
     end
   end
 
-  let(:gitaly_check_access) { GitAccessStatus.new(
-    true,
-    'ok',
-    gl_repository: gl_repository,
-    gl_username: gl_username,
-    repository_path: repo_path,
-    gitaly: { 'repository' => { 'relative_path' => repo_name, 'storage_name' => 'default'} , 'address' => 'unix:gitaly.socket' }
-  )
-  }
+  let(:gitaly_check_access) do
+    GitAccessStatus.new(
+      true,
+      'ok',
+      gl_repository: gl_repository,
+      gl_username: gl_username,
+      gitaly: { 'repository' => { 'relative_path' => repo_name, 'storage_name' => 'default'} , 'address' => 'unix:gitaly.socket' }
+    )
+  end
 
   let(:api) do
     double(GitlabNet).tap do |api|
@@ -37,7 +37,6 @@ describe GitlabShell do
                 'ok',
                 gl_repository: gl_repository,
                 gl_username: gl_username,
-                repository_path: repo_path,
                 gitaly: nil))
       api.stub(two_factor_recovery_codes: {
         'success' => true,
@@ -51,7 +50,6 @@ describe GitlabShell do
   let(:tmp_repos_path) { File.join(ROOT_PATH, 'tmp', 'repositories') }
 
   let(:repo_name) { 'gitlab-ci.git' }
-  let(:repo_path) { File.join(tmp_repos_path, repo_name) }
   let(:gl_repository) { 'project-1' }
   let(:gl_username) { 'testuser' }
 
@@ -171,14 +169,6 @@ describe GitlabShell do
       end
     end
 
-    context 'git-upload-pack' do
-      it_behaves_like 'upload-pack', 'git-upload-pack'
-    end
-
-    context 'git upload-pack' do
-      it_behaves_like 'upload-pack', 'git upload-pack'
-    end
-
     context 'gitaly-upload-pack' do
       let(:ssh_cmd) { "git-upload-pack gitlab-ci.git" }
       before {
@@ -206,25 +196,6 @@ describe GitlabShell do
       end
     end
 
-    context 'git-receive-pack' do
-      let(:ssh_cmd) { "git-receive-pack gitlab-ci.git" }
-      after { subject.exec(ssh_cmd) }
-
-      it "should process the command" do
-        subject.should_receive(:process_cmd).with(%W(git-receive-pack gitlab-ci.git))
-      end
-
-      it "should execute the command" do
-        subject.should_receive(:exec_cmd).with('git-receive-pack', repo_path)
-      end
-
-      it "should log the command execution" do
-        message = "executing git command"
-        user_string = "user with key #{key_id}"
-        $logger.should_receive(:info).with(message, command: "git-receive-pack #{repo_path}", user: user_string)
-      end
-    end
-
     context 'gitaly-receive-pack' do
       let(:ssh_cmd) { "git-receive-pack gitlab-ci.git" }
       before {
@@ -254,7 +225,7 @@ describe GitlabShell do
 
     shared_examples_for 'upload-archive' do |command|
       let(:ssh_cmd) { "#{command} gitlab-ci.git" }
-      let(:exec_cmd_params) { ['git-upload-archive', repo_path] }
+      let(:exec_cmd_params) { ['git-upload-archive'] }
       let(:exec_cmd_log_params) { exec_cmd_params }
 
       after { subject.exec(ssh_cmd) }
@@ -279,14 +250,6 @@ describe GitlabShell do
       end
     end
 
-    context 'git-upload-archive' do
-      it_behaves_like 'upload-archive', 'git-upload-archive'
-    end
-
-    context 'git upload-archive' do
-      it_behaves_like 'upload-archive', 'git upload-archive'
-    end
-
     context 'gitaly-upload-archive' do
       before do
         api.stub(check_access: gitaly_check_access)
@@ -406,32 +369,13 @@ describe GitlabShell do
                   'denied',
                   gl_repository: nil,
                   gl_username: nil,
-                  repository_path: nil,
                   gitaly: nil))
+
         message = 'Access denied'
         user_string = "user with key #{key_id}"
         $logger.should_receive(:warn).with(message, command: 'git-upload-pack gitlab-ci.git', user: user_string)
       end
     end
-
-    describe 'set the repository path' do
-      context 'with a correct path' do
-        before { subject.exec(ssh_cmd) }
-
-        its(:repo_path) { should == repo_path }
-      end
-
-      context "with a path that doesn't match an absolute path" do
-        before do
-          File.stub(:absolute_path) { 'y/gitlab-ci.git' }
-        end
-
-        it "refuses to assign the path" do
-          $stderr.should_receive(:puts).with("GitLab: Invalid repository path")
-          expect(subject.exec(ssh_cmd)).to be_false
-        end
-      end
-    end
   end
 
   describe :exec_cmd do