diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb
index 928f17e6a8e6e040e09c8bc6b67e40e9d60236e5..7d0e2b3e2efadfc5e236221abd0ff7eb622e1ade 100644
--- a/app/controllers/projects/git_http_client_controller.rb
+++ b/app/controllers/projects/git_http_client_controller.rb
@@ -4,7 +4,7 @@ class Projects::GitHttpClientController < Projects::ApplicationController
include ActionController::HttpAuthentication::Basic
include KerberosSpnegoHelper
- attr_reader :authentication_result
+ attr_reader :authentication_result, :redirected_path
delegate :actor, :authentication_abilities, to: :authentication_result, allow_nil: true
@@ -14,7 +14,6 @@ class Projects::GitHttpClientController < Projects::ApplicationController
skip_before_action :verify_authenticity_token
skip_before_action :repository
before_action :authenticate_user
- before_action :ensure_project_found!
private
@@ -68,38 +67,14 @@ class Projects::GitHttpClientController < Projects::ApplicationController
headers['Www-Authenticate'] = challenges.join("\n") if challenges.any?
end
- def ensure_project_found!
- render_not_found if project.blank?
- end
-
def project
- return @project if defined?(@project)
-
- project_id, _ = project_id_with_suffix
- @project =
- if project_id.blank?
- nil
- else
- Project.find_by_full_path("#{params[:namespace_id]}/#{project_id}")
- end
- end
+ parse_repo_path unless defined?(@project)
- # This method returns two values so that we can parse
- # params[:project_id] (untrusted input!) in exactly one place.
- def project_id_with_suffix
- id = params[:project_id] || ''
-
- %w[.wiki.git .git].each do |suffix|
- if id.end_with?(suffix)
- # Be careful to only remove the suffix from the end of 'id'.
- # Accidentally removing it from the middle is how security
- # vulnerabilities happen!
- return [id.slice(0, id.length - suffix.length), suffix]
- end
- end
+ @project
+ end
- # Something is wrong with params[:project_id]; do not pass it on.
- [nil, nil]
+ def parse_repo_path
+ @project, @wiki, @redirected_path = Gitlab::RepoPath.parse("#{params[:namespace_id]}/#{params[:project_id]}")
end
def render_missing_personal_token
@@ -114,14 +89,9 @@ class Projects::GitHttpClientController < Projects::ApplicationController
end
def wiki?
- return @wiki if defined?(@wiki)
-
- _, suffix = project_id_with_suffix
- @wiki = suffix == '.wiki.git'
- end
+ parse_repo_path unless defined?(@wiki)
- def render_not_found
- render plain: 'Not Found', status: :not_found
+ @wiki
end
def handle_basic_authentication(login, password)
diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb
index b6b62da7b60ab20321050e2482cbf15955a10ca8..71ae60cb8cdacbb7578560a09124f2e62997da73 100644
--- a/app/controllers/projects/git_http_controller.rb
+++ b/app/controllers/projects/git_http_controller.rb
@@ -56,7 +56,7 @@ class Projects::GitHttpController < Projects::GitHttpClientController
end
def access
- @access ||= access_klass.new(access_actor, project, 'http', authentication_abilities: authentication_abilities)
+ @access ||= access_klass.new(access_actor, project, 'http', authentication_abilities: authentication_abilities, redirected_path: redirected_path)
end
def access_actor
diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb
index d3732d67622933e2d60aa18792b6558af6c01a20..5e9cf5e68b11baec287fe9cc1284c10485b733c5 100644
--- a/lib/api/helpers/internal_helpers.rb
+++ b/lib/api/helpers/internal_helpers.rb
@@ -10,6 +10,10 @@ module API
set_project unless defined?(@project)
@project
end
+
+ def redirected_path
+ @redirected_path
+ end
def ssh_authentication_abilities
[
@@ -38,8 +42,9 @@ module API
def set_project
if params[:gl_repository]
@project, @wiki = Gitlab::GlRepository.parse(params[:gl_repository])
+ @redirected_path = nil
else
- @project, @wiki = Gitlab::RepoPath.parse(params[:project])
+ @project, @wiki, @redirected_path = Gitlab::RepoPath.parse(params[:project])
end
end
diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index ecd6d672cf7fca2fd4ff97035abf9c0872d35b66..9ec418edea4984dee0e6f588a68335b37240ccd2 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -34,7 +34,7 @@ module API
access_checker_klass = wiki? ? Gitlab::GitAccessWiki : Gitlab::GitAccess
access_checker = access_checker_klass
- .new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities)
+ .new(actor, project, protocol, authentication_abilities: ssh_authentication_abilities, redirected_path: redirected_path)
begin
access_checker.check(params[:action], params[:changes])
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index 0a19d24eb2005c5a7fc216c0732c7f0b20213c36..0b62911958dfb0e4642149864af85917761c81a0 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -22,12 +22,13 @@ module Gitlab
PUSH_COMMANDS = %w{ git-receive-pack }.freeze
ALL_COMMANDS = DOWNLOAD_COMMANDS + PUSH_COMMANDS
- attr_reader :actor, :project, :protocol, :authentication_abilities
+ attr_reader :actor, :project, :protocol, :authentication_abilities, :redirected_path
- def initialize(actor, project, protocol, authentication_abilities:)
+ def initialize(actor, project, protocol, authentication_abilities:, redirected_path: nil)
@actor = actor
@project = project
@protocol = protocol
+ @redirected_path = redirected_path
@authentication_abilities = authentication_abilities
end
@@ -35,6 +36,7 @@ module Gitlab
check_protocol!
check_active_user!
check_project_accessibility!
+ check_project_moved!
check_command_disabled!(cmd)
check_command_existence!(cmd)
check_repository_existence!
@@ -87,6 +89,21 @@ module Gitlab
end
end
+ def check_project_moved!
+ if redirected_path
+ url = protocol == 'ssh' ? project.ssh_url_to_repo : project.http_url_to_repo
+ message = <<-MESSAGE.strip_heredoc
+ Project '#{redirected_path}' was moved to '#{project.full_path}'.
+
+ Please update your Git remote and try again:
+
+ git remote set-url origin #{url}
+ MESSAGE
+
+ raise NotFoundError, message
+ end
+ end
+
def check_command_disabled!(cmd)
if upload_pack?(cmd)
check_upload_pack_disabled!
diff --git a/lib/gitlab/repo_path.rb b/lib/gitlab/repo_path.rb
index 878e03f61d70d41653a7772aa729cc62f7d76043..3591fa9145ef7cdb20ba774b042a5552685438c3 100644
--- a/lib/gitlab/repo_path.rb
+++ b/lib/gitlab/repo_path.rb
@@ -3,16 +3,18 @@ module Gitlab
NotFoundError = Class.new(StandardError)
def self.parse(repo_path)
+ wiki = false
project_path = strip_storage_path(repo_path.sub(/\.git\z/, ''), fail_on_not_found: false)
- project = Project.find_by_full_path(project_path)
- if project_path.end_with?('.wiki') && !project
- project = Project.find_by_full_path(project_path.chomp('.wiki'))
+ project, was_redirected = find_project(project_path)
+
+ if project_path.end_with?('.wiki') && project.nil?
+ project, was_redirected = find_project(project_path.chomp('.wiki'))
wiki = true
- else
- wiki = false
end
- [project, wiki]
+ redirected_path = project_path if was_redirected
+
+ [project, wiki, redirected_path]
end
def self.strip_storage_path(repo_path, fail_on_not_found: true)
@@ -30,5 +32,12 @@ module Gitlab
result.sub(/\A\/*/, '')
end
+
+ def self.find_project(project_path)
+ project = Project.find_by_full_path(project_path, follow_redirects: true)
+ was_redirected = project && project.full_path.casecmp(project_path) != 0
+
+ [project, was_redirected]
+ end
end
end
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index 3dcc20c48e8a1ad0297a95a45378111fc13d0326..9a86cfa66e4831c2116e4112c81230225fa12e3d 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -3,11 +3,12 @@ require 'spec_helper'
describe Gitlab::GitAccess, lib: true do
let(:pull_access_check) { access.check('git-upload-pack', '_any') }
let(:push_access_check) { access.check('git-receive-pack', '_any') }
- let(:access) { Gitlab::GitAccess.new(actor, project, protocol, authentication_abilities: authentication_abilities) }
+ let(:access) { Gitlab::GitAccess.new(actor, project, protocol, authentication_abilities: authentication_abilities, redirected_path: redirected_path) }
let(:project) { create(:project, :repository) }
let(:user) { create(:user) }
let(:actor) { user }
let(:protocol) { 'ssh' }
+ let(:redirected_path) { nil }
let(:authentication_abilities) do
[
:read_project,
@@ -162,6 +163,46 @@ describe Gitlab::GitAccess, lib: true do
end
end
+ describe '#check_project_moved!' do
+ before do
+ project.team << [user, :master]
+ end
+
+ context 'when a redirect was not followed to find the project' do
+ context 'pull code' do
+ it { expect { pull_access_check }.not_to raise_error }
+ end
+
+ context 'push code' do
+ it { expect { push_access_check }.not_to raise_error }
+ end
+ end
+
+ context 'when a redirect was followed to find the project' do
+ let(:redirected_path) { 'some/other-path' }
+
+ context 'pull code' do
+ it { expect { pull_access_check }.to raise_not_found(/Project '#{redirected_path}' was moved to '#{project.full_path}'/) }
+ it { expect { pull_access_check }.to raise_not_found(/git remote set-url origin #{project.ssh_url_to_repo}/) }
+
+ context 'http protocol' do
+ let(:protocol) { 'http' }
+ it { expect { pull_access_check }.to raise_not_found(/git remote set-url origin #{project.http_url_to_repo}/) }
+ end
+ end
+
+ context 'push code' do
+ it { expect { push_access_check }.to raise_not_found(/Project '#{redirected_path}' was moved to '#{project.full_path}'/) }
+ it { expect { push_access_check }.to raise_not_found(/git remote set-url origin #{project.ssh_url_to_repo}/) }
+
+ context 'http protocol' do
+ let(:protocol) { 'http' }
+ it { expect { push_access_check }.to raise_not_found(/git remote set-url origin #{project.http_url_to_repo}/) }
+ end
+ end
+ end
+ end
+
describe '#check_command_disabled!' do
before do
project.team << [user, :master]
diff --git a/spec/lib/gitlab/git_access_wiki_spec.rb b/spec/lib/gitlab/git_access_wiki_spec.rb
index a1eb95750ba314b9c777a08803906dd449a5634a..797ec8cb23ebfa8771ffd0d38235c6af3edfd149 100644
--- a/spec/lib/gitlab/git_access_wiki_spec.rb
+++ b/spec/lib/gitlab/git_access_wiki_spec.rb
@@ -1,9 +1,10 @@
require 'spec_helper'
describe Gitlab::GitAccessWiki, lib: true do
- let(:access) { Gitlab::GitAccessWiki.new(user, project, 'web', authentication_abilities: authentication_abilities) }
+ let(:access) { Gitlab::GitAccessWiki.new(user, project, 'web', authentication_abilities: authentication_abilities, redirected_path: redirected_path) }
let(:project) { create(:project, :repository) }
let(:user) { create(:user) }
+ let(:redirected_path) { nil }
let(:authentication_abilities) do
[
:read_project,
diff --git a/spec/lib/gitlab/repo_path_spec.rb b/spec/lib/gitlab/repo_path_spec.rb
index f90253971075d031ece9d211ab73e122eb6fa69b..efea4f429bfa358924d11430d5e4dda186b8cf24 100644
--- a/spec/lib/gitlab/repo_path_spec.rb
+++ b/spec/lib/gitlab/repo_path_spec.rb
@@ -4,24 +4,44 @@ describe ::Gitlab::RepoPath do
describe '.parse' do
set(:project) { create(:project) }
- it 'parses a full repository path' do
- expect(described_class.parse(project.repository.path)).to eq([project, false])
- end
+ context 'a repository storage path' do
+ it 'parses a full repository path' do
+ expect(described_class.parse(project.repository.path)).to eq([project, false, nil])
+ end
- it 'parses a full wiki path' do
- expect(described_class.parse(project.wiki.repository.path)).to eq([project, true])
+ it 'parses a full wiki path' do
+ expect(described_class.parse(project.wiki.repository.path)).to eq([project, true, nil])
+ end
end
- it 'parses a relative repository path' do
- expect(described_class.parse(project.full_path + '.git')).to eq([project, false])
- end
+ context 'a relative path' do
+ it 'parses a relative repository path' do
+ expect(described_class.parse(project.full_path + '.git')).to eq([project, false, nil])
+ end
- it 'parses a relative wiki path' do
- expect(described_class.parse(project.full_path + '.wiki.git')).to eq([project, true])
- end
+ it 'parses a relative wiki path' do
+ expect(described_class.parse(project.full_path + '.wiki.git')).to eq([project, true, nil])
+ end
+
+ it 'parses a relative path starting with /' do
+ expect(described_class.parse('/' + project.full_path + '.git')).to eq([project, false, nil])
+ end
+
+ context 'of a redirected project' do
+ let(:redirect) { project.route.create_redirect('foo/bar') }
+
+ it 'parses a relative repository path' do
+ expect(described_class.parse(redirect.path + '.git')).to eq([project, false, 'foo/bar'])
+ end
+
+ it 'parses a relative wiki path' do
+ expect(described_class.parse(redirect.path + '.wiki.git')).to eq([project, true, 'foo/bar.wiki'])
+ end
- it 'parses a relative path starting with /' do
- expect(described_class.parse('/' + project.full_path + '.git')).to eq([project, false])
+ it 'parses a relative path starting with /' do
+ expect(described_class.parse('/' + redirect.path + '.git')).to eq([project, false, 'foo/bar'])
+ end
+ end
end
end
@@ -43,4 +63,33 @@ describe ::Gitlab::RepoPath do
)
end
end
+
+ describe '.find_project' do
+ let(:project) { create(:empty_project) }
+ let(:redirect) { project.route.create_redirect('foo/bar/baz') }
+
+ context 'when finding a project by its canonical path' do
+ context 'when the cases match' do
+ it 'returns the project and false' do
+ expect(described_class.find_project(project.full_path)).to eq([project, false])
+ end
+ end
+
+ context 'when the cases do not match' do
+ # This is slightly different than web behavior because on the web it is
+ # easy and safe to redirect someone to the correctly-cased URL. For git
+ # requests, we should accept wrongly-cased URLs because it is a pain to
+ # block people's git operations and force them to update remote URLs.
+ it 'returns the project and false' do
+ expect(described_class.find_project(project.full_path.upcase)).to eq([project, false])
+ end
+ end
+ end
+
+ context 'when finding a project via a redirect' do
+ it 'returns the project and true' do
+ expect(described_class.find_project(redirect.path)).to eq([project, true])
+ end
+ end
+ end
end
diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb
index 86e15d896df7850ce98adedc0158a954c766b09a..6deaea956e03a1fd73ce230f871e93b7db9fb8a6 100644
--- a/spec/requests/api/internal_spec.rb
+++ b/spec/requests/api/internal_spec.rb
@@ -321,8 +321,6 @@ describe API::Internal do
end
context "archived project" do
- let(:personal_project) { create(:empty_project, namespace: user.namespace) }
-
before do
project.team << [user, :developer]
project.archive!
@@ -445,6 +443,42 @@ describe API::Internal do
expect(json_response['status']).to be_truthy
end
end
+
+ context 'the project path was changed' do
+ let!(:old_path_to_repo) { project.repository.path_to_repo }
+ let!(:old_full_path) { project.full_path }
+ let(:project_moved_message) do
+ <<-MSG.strip_heredoc
+ Project '#{old_full_path}' was moved to '#{project.full_path}'.
+
+ Please update your Git remote and try again:
+
+ git remote set-url origin #{project.ssh_url_to_repo}
+ MSG
+ end
+
+ before do
+ project.team << [user, :developer]
+ project.path = 'new_path'
+ project.save!
+ end
+
+ it 'rejects the push' do
+ push_with_path(key, old_path_to_repo)
+
+ expect(response).to have_http_status(200)
+ expect(json_response['status']).to be_falsey
+ expect(json_response['message']).to eq(project_moved_message)
+ end
+
+ it 'rejects the SSH pull' do
+ pull_with_path(key, old_path_to_repo)
+
+ expect(response).to have_http_status(200)
+ expect(json_response['status']).to be_falsey
+ expect(json_response['message']).to eq(project_moved_message)
+ end
+ end
end
describe 'GET /internal/merge_request_urls' do
@@ -587,6 +621,17 @@ describe API::Internal do
)
end
+ def pull_with_path(key, path_to_repo, protocol = 'ssh')
+ post(
+ api("/internal/allowed"),
+ key_id: key.id,
+ project: path_to_repo,
+ action: 'git-upload-pack',
+ secret_token: secret_token,
+ protocol: protocol
+ )
+ end
+
def push(key, project, protocol = 'ssh', env: nil)
post(
api("/internal/allowed"),
@@ -600,6 +645,19 @@ describe API::Internal do
)
end
+ def push_with_path(key, path_to_repo, protocol = 'ssh', env: nil)
+ post(
+ api("/internal/allowed"),
+ changes: 'd14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/master',
+ key_id: key.id,
+ project: path_to_repo,
+ action: 'git-receive-pack',
+ secret_token: secret_token,
+ protocol: protocol,
+ env: env
+ )
+ end
+
def archive(key, project)
post(
api("/internal/allowed"),
diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index dce78faefc9468487e3135100c28a4192d7c8632..b08148eca3cd00f17a8eb2c1539f6072cfcaeb2d 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -316,6 +316,26 @@ describe 'Git HTTP requests', lib: true do
it_behaves_like 'pushes require Basic HTTP Authentication'
end
end
+
+ context 'and the user requests a redirected path' do
+ let!(:redirect) { project.route.create_redirect('foo/bar') }
+ let(:path) { "#{redirect.path}.git" }
+ let(:project_moved_message) do
+ <<-MSG.strip_heredoc
+ Project '#{redirect.path}' was moved to '#{project.full_path}'.
+
+ Please update your Git remote and try again:
+
+ git remote set-url origin #{project.http_url_to_repo}
+ MSG
+ end
+
+ it 'downloads get status 404 with "project was moved" message' do
+ clone_get(path, {})
+ expect(response).to have_http_status(:not_found)
+ expect(response.body).to match(project_moved_message)
+ end
+ end
end
context "when the project is private" do
@@ -505,6 +525,33 @@ describe 'Git HTTP requests', lib: true do
Rack::Attack::Allow2Ban.reset(ip, options)
end
end
+
+ context 'and the user requests a redirected path' do
+ let!(:redirect) { project.route.create_redirect('foo/bar') }
+ let(:path) { "#{redirect.path}.git" }
+ let(:project_moved_message) do
+ <<-MSG.strip_heredoc
+ Project '#{redirect.path}' was moved to '#{project.full_path}'.
+
+ Please update your Git remote and try again:
+
+ git remote set-url origin #{project.http_url_to_repo}
+ MSG
+ end
+
+ it 'downloads get status 404 with "project was moved" message' do
+ clone_get(path, env)
+ expect(response).to have_http_status(:not_found)
+ expect(response.body).to match(project_moved_message)
+ end
+
+ it 'uploads get status 404 with "project was moved" message' do
+ upload(path, env) do |response|
+ expect(response).to have_http_status(:not_found)
+ expect(response.body).to match(project_moved_message)
+ end
+ end
+ end
end
context "when the user doesn't have access to the project" do
@@ -680,7 +727,7 @@ describe 'Git HTTP requests', lib: true do
end
context "POST git-receive-pack" do
- it "failes to find a route" do
+ it "fails to find a route" do
expect { push_post(project.path_with_namespace) }.to raise_error(ActionController::RoutingError)
end
end
diff --git a/spec/workers/post_receive_spec.rb b/spec/workers/post_receive_spec.rb
index 3c93da63f2e04eadb17944531fc100b3cc465cfe..6c4df7350ea6a4e826a71be0394e59f4e4d48e78 100644
--- a/spec/workers/post_receive_spec.rb
+++ b/spec/workers/post_receive_spec.rb
@@ -32,7 +32,7 @@ describe PostReceive do
context "with an absolute path as the project identifier" do
it "searches the project by full path" do
- expect(Project).to receive(:find_by_full_path).with(project.full_path).and_call_original
+ expect(Project).to receive(:find_by_full_path).with(project.full_path, follow_redirects: true).and_call_original
described_class.new.perform(pwd(project), key_id, base64_changes)
end