Commit 0f84d384 authored by Sean McGivern's avatar Sean McGivern

Merge branch...

Merge branch 'ee-29925-gitlab-shell-hooks-can-no-longer-send-absolute-paths-to-gitlab-ce' into 'master'

29925 gitlab shell hooks can no longer send absolute paths to gitlab ce

See merge request !1832
parents e564b256 3fe84f0f
......@@ -59,7 +59,7 @@ class Projects::GitHttpController < Projects::GitHttpClientController
def render_ok
set_workhorse_internal_api_content_type
render json: Gitlab::Workhorse.git_http_ok(repository, user, action_name)
render json: Gitlab::Workhorse.git_http_ok(repository, wiki?, user, action_name)
end
def render_http_not_allowed
......
......@@ -3,26 +3,26 @@ class PostReceive
include DedicatedSidekiqQueue
extend Gitlab::CurrentSettings
def perform(repo_path, identifier, changes)
repo_relative_path = Gitlab::RepoPath.strip_storage_path(repo_path)
def perform(project_identifier, identifier, changes)
project, is_wiki = parse_project_identifier(project_identifier)
if project.nil?
log("Triggered hook for non-existing project with identifier \"#{project_identifier}\"")
return false
end
changes = Base64.decode64(changes) unless changes.include?(' ')
# Use Sidekiq.logger so arguments can be correlated with execution
# time and thread ID's.
Sidekiq.logger.info "changes: #{changes.inspect}" if ENV['SIDEKIQ_LOG_ARGUMENTS']
post_received = Gitlab::GitPostReceive.new(repo_relative_path, identifier, changes)
if post_received.project.nil?
log("Triggered hook for non-existing project with full path \"#{repo_relative_path}\"")
return false
end
post_received = Gitlab::GitPostReceive.new(project, identifier, changes)
if post_received.wiki?
if is_wiki
update_wiki_es_indexes(post_received)
# Triggers repository update on secondary nodes when Geo is enabled
Gitlab::Geo.notify_wiki_update(post_received.project) if Gitlab::Geo.enabled?
elsif post_received.regular_project?
else
# TODO: gitlab-org/gitlab-ce#26325. Remove this.
if Gitlab::Geo.enabled?
hook_data = {
......@@ -36,9 +36,6 @@ class PostReceive
end
process_project_changes(post_received)
else
log("Triggered hook for unidentifiable repository type with full path \"#{repo_relative_path}\"")
false
end
end
......@@ -69,6 +66,21 @@ class PostReceive
private
# To maintain backwards compatibility, we accept both gl_repository or
# repository paths as project identifiers. Our plan is to migrate to
# gl_repository only with the following plan:
# 9.2: Handle both possible values. Keep Gitlab-Shell sending only repo paths
# 9.3 (or patch release): Make GitLab Shell pass gl_repository if present
# 9.4 (or patch release): Make GitLab Shell always pass gl_repository
# 9.5 (or patch release): Handle only gl_repository as project identifier on this method
def parse_project_identifier(project_identifier)
if project_identifier.start_with?('/')
Gitlab::RepoPath.parse(project_identifier)
else
Gitlab::GlRepository.parse(project_identifier)
end
end
def log(message)
Gitlab::GitLogger.error("POST-RECEIVE: #{message}")
end
......
---
title: Generate and handle a gl_repository param to pass around components
merge_request: 10992
author:
module API
module Helpers
module InternalHelpers
# Project paths may be any of the following:
# * /repository/storage/path/namespace/project
# * /namespace/project
# * namespace/project
#
# In addition, they may have a '.git' extension and multiple namespaces
#
# Transform all these cases to 'namespace/project'
def clean_project_path(project_path, storages = Gitlab.config.repositories.storages.values)
project_path = project_path.sub(/\.git\z/, '')
storages.each do |storage|
storage_path = File.expand_path(storage['path'])
if project_path.start_with?(storage_path)
project_path = project_path.sub(storage_path, '')
break
end
end
project_path.sub(/\A\//, '')
end
def project_path
@project_path ||= clean_project_path(params[:project])
end
def wiki?
@wiki ||= project_path.end_with?('.wiki') &&
!Project.find_by_full_path(project_path)
set_project unless defined?(@wiki)
@wiki
end
def project
@project ||= begin
# Check for *.wiki repositories.
# Strip out the .wiki from the pathname before finding the
# project. This applies the correct project permissions to
# the wiki repository as well.
project_path.chomp!('.wiki') if wiki?
Project.find_by_full_path(project_path)
end
set_project unless defined?(@project)
@project
end
def ssh_authentication_abilities
......@@ -66,6 +32,16 @@ module API
::Users::ActivityService.new(actor, 'Git SSH').execute if commands.include?(params[:action])
end
private
def set_project
if params[:gl_repository]
@project, @wiki = Gitlab::GlRepository.parse(params[:gl_repository])
else
@project, @wiki = Gitlab::RepoPath.parse(params[:project])
end
end
end
end
end
......@@ -42,6 +42,10 @@ module API
if access_status.status
log_user_activity(actor)
# Project id to pass between components that don't share/don't have
# access to the same filesystem mounts
response[:gl_repository] = Gitlab::GlRepository.gl_repository(project, wiki?)
# Return the repository full path so that gitlab-shell has it when
# handling ssh commands
response[:repository_path] =
......@@ -146,11 +150,9 @@ module API
return unless Gitlab::GitalyClient.enabled?
relative_path = Gitlab::RepoPath.strip_storage_path(params[:repo_path])
project = Project.find_by_full_path(relative_path.sub(/\.(git|wiki)\z/, ''))
begin
Gitlab::GitalyClient::Notifications.new(project.repository).post_receive
repository = wiki? ? project.wiki.repository : project.repository
Gitlab::GitalyClient::Notifications.new(repository.raw_repository).post_receive
rescue GRPC::Unavailable => e
render_api_error!(e, 500)
end
......
module Gitlab
class GitPostReceive
include Gitlab::Identifier
attr_reader :repo_path, :identifier, :changes, :project
attr_reader :project, :identifier, :changes
def initialize(repo_path, identifier, changes)
repo_path.gsub!(/\.git\z/, '')
repo_path.gsub!(/\A\//, '')
@repo_path = repo_path
def initialize(project, identifier, changes)
@project = project
@identifier = identifier
@changes = deserialize_changes(changes)
retrieve_project_and_type
end
def wiki?
@type == :wiki
end
def regular_project?
@type == :project
end
def identify(revision)
......@@ -28,16 +15,6 @@ module Gitlab
private
def retrieve_project_and_type
@type = :project
@project = Project.find_by_full_path(@repo_path)
if @repo_path.end_with?('.wiki') && !@project
@type = :wiki
@project = Project.find_by_full_path(@repo_path.gsub(/\.wiki\z/, ''))
end
end
def deserialize_changes(changes)
changes = utf8_encode_changes(changes)
changes.lines
......
module Gitlab
module GlRepository
def self.gl_repository(project, is_wiki)
"#{is_wiki ? 'wiki' : 'project'}-#{project.id}"
end
def self.parse(gl_repository)
match_data = /\A(project|wiki)-([1-9][0-9]*)\z/.match(gl_repository)
unless match_data
raise ArgumentError, "Invalid GL Repository \"#{gl_repository}\""
end
type, id = match_data.captures
project = Project.find_by(id: id)
wiki = type == 'wiki'
[project, wiki]
end
end
end
......@@ -2,18 +2,29 @@ module Gitlab
module RepoPath
NotFoundError = Class.new(StandardError)
def self.strip_storage_path(repo_path)
result = nil
def self.parse(repo_path)
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'))
wiki = true
else
wiki = false
end
Gitlab.config.repositories.storages.values.each do |params|
storage_path = params['path']
if repo_path.start_with?(storage_path)
result = repo_path.sub(storage_path, '')
break
[project, wiki]
end
def self.strip_storage_path(repo_path, fail_on_not_found: true)
result = repo_path
storage = Gitlab.config.repositories.storages.values.find do |params|
repo_path.start_with?(params['path'])
end
if result.nil?
if storage
result = result.sub(storage['path'], '')
elsif fail_on_not_found
raise NotFoundError.new("No known storage path matches #{repo_path.inspect}")
end
......
......@@ -16,15 +16,17 @@ module Gitlab
SECRET_LENGTH = 32
class << self
def git_http_ok(repository, user, action)
def git_http_ok(repository, is_wiki, user, action)
project = repository.project
repo_path = repository.path_to_repo
params = {
GL_ID: Gitlab::GlId.gl_id(user),
GL_REPOSITORY: Gitlab::GlRepository.gl_repository(project, is_wiki),
RepoPath: repo_path,
}
if Gitlab.config.gitaly.enabled
address = Gitlab::GitalyClient.get_address(repository.project.repository_storage)
address = Gitlab::GitalyClient.get_address(project.repository_storage)
params[:Repository] = repository.gitaly_repository.to_h
feature_enabled = case action.to_s
......
require 'spec_helper'
describe ::Gitlab::GlRepository do
describe '.parse' do
set(:project) { create(:project) }
it 'parses a project gl_repository' do
expect(described_class.parse("project-#{project.id}")).to eq([project, false])
end
it 'parses a wiki gl_repository' do
expect(described_class.parse("wiki-#{project.id}")).to eq([project, true])
end
it 'throws an argument error on an invalid gl_repository' do
expect { described_class.parse("badformat-#{project.id}") }.to raise_error(ArgumentError)
end
end
end
require 'spec_helper'
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
it 'parses a full wiki path' do
expect(described_class.parse(project.wiki.repository.path)).to eq([project, true])
end
it 'parses a relative repository path' do
expect(described_class.parse(project.full_path + '.git')).to eq([project, false])
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 path starting with /' do
expect(described_class.parse('/' + project.full_path + '.git')).to eq([project, false])
end
end
describe '.strip_storage_path' do
before do
allow(Gitlab.config.repositories).to receive(:storages).and_return({
......
......@@ -181,10 +181,23 @@ describe Gitlab::Workhorse, lib: true do
let(:user) { create(:user) }
let(:repo_path) { repository.path_to_repo }
let(:action) { 'info_refs' }
let(:params) do
{ GL_ID: "user-#{user.id}", GL_REPOSITORY: "project-#{project.id}", RepoPath: repo_path }
end
subject { described_class.git_http_ok(repository, false, user, action) }
it { expect(subject).to include(params) }
subject { described_class.git_http_ok(repository, user, action) }
context 'when is_wiki' do
let(:params) do
{ GL_ID: "user-#{user.id}", GL_REPOSITORY: "wiki-#{project.id}", RepoPath: repo_path }
end
subject { described_class.git_http_ok(repository, true, user, action) }
it { expect(subject).to include({ GL_ID: "user-#{user.id}", RepoPath: repo_path }) }
it { expect(subject).to include(params) }
end
context 'when Gitaly is enabled' do
let(:gitaly_params) do
......
require 'spec_helper'
describe ::API::Helpers::InternalHelpers do
include described_class
describe '.clean_project_path' do
project = 'namespace/project'
namespaced = File.join('namespace2', project)
{
File.join(Dir.pwd, project) => project,
File.join(Dir.pwd, namespaced) => namespaced,
project => project,
namespaced => namespaced,
project + '.git' => project,
namespaced + '.git' => namespaced,
"/" + project => project,
"/" + namespaced => namespaced,
}.each do |project_path, expected|
context project_path do
# Relative and absolute storage paths, with and without trailing /
['.', './', Dir.pwd, Dir.pwd + '/'].each do |storage_path|
context "storage path is #{storage_path}" do
subject { clean_project_path(project_path, [{ 'path' => storage_path }]) }
it { is_expected.to eq(expected) }
end
end
end
end
end
end
......@@ -228,6 +228,7 @@ describe API::Internal do
expect(response).to have_http_status(200)
expect(json_response["status"]).to be_truthy
expect(json_response["repository_path"]).to eq(project.wiki.repository.path_to_repo)
expect(json_response["gl_repository"]).to eq("wiki-#{project.id}")
expect(user).not_to have_an_activity_record
end
end
......@@ -239,6 +240,7 @@ describe API::Internal do
expect(response).to have_http_status(200)
expect(json_response["status"]).to be_truthy
expect(json_response["repository_path"]).to eq(project.wiki.repository.path_to_repo)
expect(json_response["gl_repository"]).to eq("wiki-#{project.id}")
expect(user).to have_an_activity_record
end
end
......@@ -250,6 +252,7 @@ describe API::Internal do
expect(response).to have_http_status(200)
expect(json_response["status"]).to be_truthy
expect(json_response["repository_path"]).to eq(project.repository.path_to_repo)
expect(json_response["gl_repository"]).to eq("project-#{project.id}")
expect(user).to have_an_activity_record
end
end
......@@ -261,6 +264,7 @@ describe API::Internal do
expect(response).to have_http_status(200)
expect(json_response["status"]).to be_truthy
expect(json_response["repository_path"]).to eq(project.repository.path_to_repo)
expect(json_response["gl_repository"]).to eq("project-#{project.id}")
expect(user).not_to have_an_activity_record
end
......@@ -271,6 +275,7 @@ describe API::Internal do
expect(response).to have_http_status(200)
expect(json_response["status"]).to be_truthy
expect(json_response["repository_path"]).to eq(project.repository.path_to_repo)
expect(json_response["gl_repository"]).to eq("project-#{project.id}")
end
end
......@@ -281,6 +286,7 @@ describe API::Internal do
expect(response).to have_http_status(200)
expect(json_response["status"]).to be_truthy
expect(json_response["repository_path"]).to eq(project.repository.path_to_repo)
expect(json_response["gl_repository"]).to eq("project-#{project.id}")
end
end
end
......@@ -492,18 +498,39 @@ describe API::Internal do
expect(json_response).to eq([])
end
context 'with a gl_repository parameter' do
let(:gl_repository) { "project-#{project.id}" }
it 'returns link to create new merge request' do
get api("/internal/merge_request_urls?gl_repository=#{gl_repository}&changes=#{changes}"), secret_token: secret_token
expect(json_response).to match [{
"branch_name" => "new_branch",
"url" => "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch",
"new_merge_request" => true
}]
end
end
end
describe 'POST /notify_post_receive' do
let(:valid_params) do
{ repo_path: project.repository.path, secret_token: secret_token }
{ project: project.repository.path, secret_token: secret_token }
end
let(:valid_wiki_params) do
{ project: project.wiki.repository.path, secret_token: secret_token }
end
before do
allow(Gitlab.config.gitaly).to receive(:enabled).and_return(true)
end
it "calls the Gitaly client if it's enabled" do
it "calls the Gitaly client with the project's repository" do
expect(Gitlab::GitalyClient::Notifications).
to receive(:new).with(gitlab_git_repository_with(path: project.repository.path)).
and_call_original
expect_any_instance_of(Gitlab::GitalyClient::Notifications).
to receive(:post_receive)
......@@ -512,6 +539,18 @@ describe API::Internal do
expect(response).to have_http_status(200)
end
it "calls the Gitaly client with the wiki's repository if it's a wiki" do
expect(Gitlab::GitalyClient::Notifications).
to receive(:new).with(gitlab_git_repository_with(path: project.wiki.repository.path)).
and_call_original
expect_any_instance_of(Gitlab::GitalyClient::Notifications).
to receive(:post_receive)
post api("/internal/notify_post_receive"), valid_wiki_params
expect(response).to have_http_status(200)
end
it "returns 500 if the gitaly call fails" do
expect_any_instance_of(Gitlab::GitalyClient::Notifications).
to receive(:post_receive).and_raise(GRPC::Unavailable)
......@@ -520,6 +559,40 @@ describe API::Internal do
expect(response).to have_http_status(500)
end
context 'with a gl_repository parameter' do
let(:valid_params) do
{ gl_repository: "project-#{project.id}", secret_token: secret_token }
end
let(:valid_wiki_params) do
{ gl_repository: "wiki-#{project.id}", secret_token: secret_token }
end
it "calls the Gitaly client with the project's repository" do
expect(Gitlab::GitalyClient::Notifications).
to receive(:new).with(gitlab_git_repository_with(path: project.repository.path)).
and_call_original
expect_any_instance_of(Gitlab::GitalyClient::Notifications).
to receive(:post_receive)
post api("/internal/notify_post_receive"), valid_params
expect(response).to have_http_status(200)
end
it "calls the Gitaly client with the wiki's repository if it's a wiki" do
expect(Gitlab::GitalyClient::Notifications).
to receive(:new).with(gitlab_git_repository_with(path: project.wiki.repository.path)).
and_call_original
expect_any_instance_of(Gitlab::GitalyClient::Notifications).
to receive(:post_receive)
post api("/internal/notify_post_receive"), valid_wiki_params
expect(response).to have_http_status(200)
end
end
end
def project_with_repo_path(path)
......
RSpec::Matchers.define :gitlab_git_repository_with do |values|
match do |actual|
actual.is_a?(Gitlab::Git::Repository) &&
values.all? { |k, v| actual.send(k) == v }
end
end
......@@ -5,6 +5,7 @@ describe PostReceive do
let(:wrongly_encoded_changes) { changes.encode("ISO-8859-1").force_encoding("UTF-8") }
let(:base64_changes) { Base64.encode64(wrongly_encoded_changes) }
let(:project) { create(:project, :repository) }
let(:project_identifier) { "project-#{project.id}" }
let(:key) { create(:key, user: project.owner) }
let(:key_id) { key.shell_id }
......@@ -14,6 +15,26 @@ describe PostReceive do
end
end
context 'with a non-existing project' do
let(:project_identifier) { "project-123456789" }
let(:error_message) do
"Triggered hook for non-existing project with identifier \"#{project_identifier}\""
end
it "returns false and logs an error" do
expect(Gitlab::GitLogger).to receive(:error).with("POST-RECEIVE: #{error_message}")
expect(described_class.new.perform(project_identifier, key_id, base64_changes)).to be(false)
end
end
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
described_class.new.perform(pwd(project), key_id, base64_changes)
end
end
describe "#process_project_changes" do
before do
allow_any_instance_of(Gitlab::GitPostReceive).to receive(:identify).and_return(project.owner)
......@@ -25,7 +46,7 @@ describe PostReceive do
it "calls GitTagPushService" do
expect_any_instance_of(GitPushService).to receive(:execute).and_return(true)
expect_any_instance_of(GitTagPushService).not_to receive(:execute)
described_class.new.perform(pwd(project), key_id, base64_changes)
described_class.new.perform(project_identifier, key_id, base64_changes)
end
end
......@@ -35,7 +56,7 @@ describe PostReceive do
it "calls GitTagPushService" do
expect_any_instance_of(GitPushService).not_to receive(:execute)
expect_any_instance_of(GitTagPushService).to receive(:execute).and_return(true)
described_class.new.perform(pwd(project), key_id, base64_changes)
described_class.new.perform(project_identifier, key_id, base64_changes)
end
end
......@@ -45,12 +66,12 @@ describe PostReceive do
it "does not call any of the services" do
expect_any_instance_of(GitPushService).not_to receive(:execute)
expect_any_instance_of(GitTagPushService).not_to receive(:execute)
described_class.new.perform(pwd(project), key_id, base64_changes)
described_class.new.perform(project_identifier, key_id, base64_changes)
end
end
context "gitlab-ci.yml" do
subject { described_class.new.perform(pwd(project), key_id, base64_changes) }
subject { described_class.new.perform(project_identifier, key_id, base64_changes) }
context "creates a Ci::Pipeline for every change" do
before do
......@@ -74,8 +95,8 @@ describe PostReceive do
context "webhook" do
it "fetches the correct project" do
expect(Project).to receive(:find_by_full_path).with(project.path_with_namespace).and_return(project)
described_class.new.perform(pwd(project), key_id, base64_changes)
expect(Project).to receive(:find_by).with(id: project.id.to_s)
described_class.new.perform(project_identifier, key_id, base64_changes)
end
it "triggers wiki index update" do
......@@ -96,22 +117,22 @@ describe PostReceive do
expect(project).not_to receive(:execute_hooks)
expect(described_class.new.perform(pwd(project), key_id, base64_changes)).to be_falsey
expect(described_class.new.perform(project_identifier, key_id, base64_changes)).to be_falsey
end
it "asks the project to trigger all hooks" do
allow(Project).to receive(:find_by_full_path).and_return(project)
allow(Project).to receive(:find_by).and_return(project)
expect(project).to receive(:execute_hooks).twice
expect(project).to receive(:execute_services).twice
described_class.new.perform(pwd(project), key_id, base64_changes)
described_class.new.perform(project_identifier, key_id, base64_changes)
end
it "enqueues a UpdateMergeRequestsWorker job" do
allow(Project).to receive(:find_by_full_path).and_return(project)
allow(Project).to receive(:find_by).and_return(project)
expect(UpdateMergeRequestsWorker).to receive(:perform_async).with(project.id, project.owner.id, any_args)
described_class.new.perform(pwd(project), key_id, base64_changes)
described_class.new.perform(project_identifier, key_id, base64_changes)
end
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment