From 60d1dcb83ac97e3d0dfd9cdf0daa970671ba3d68 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 19 Jan 2017 17:11:48 +0000 Subject: [PATCH 1/6] Merge branch 'fix-users-deleting-public-deployment-keys' into 'security' Fix users being able to delete instance public deployment keys See merge request !2049 --- .../fix-users-deleting-public-deployment-keys.yml | 4 ++++ lib/api/deploy_keys.rb | 10 +++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/fix-users-deleting-public-deployment-keys.yml diff --git a/changelogs/unreleased/fix-users-deleting-public-deployment-keys.yml b/changelogs/unreleased/fix-users-deleting-public-deployment-keys.yml new file mode 100644 index 000000000000..c9edd1de86c8 --- /dev/null +++ b/changelogs/unreleased/fix-users-deleting-public-deployment-keys.yml @@ -0,0 +1,4 @@ +--- +title: Prevent users from deleting system deploy keys via the project deploy key API +merge_request: +author: diff --git a/lib/api/deploy_keys.rb b/lib/api/deploy_keys.rb index 853607308412..f6cb17bafd81 100644 --- a/lib/api/deploy_keys.rb +++ b/lib/api/deploy_keys.rb @@ -105,15 +105,19 @@ class DeployKeys < Grape::API present key.deploy_key, with: Entities::SSHKey end - desc 'Delete existing deploy key of currently authenticated user' do + desc 'Delete deploy key for a project' do success Key end params do requires :key_id, type: Integer, desc: 'The ID of the deploy key' end delete ":id/#{path}/:key_id" do - key = user_project.deploy_keys.find(params[:key_id]) - key.destroy + key = user_project.deploy_keys_projects.find_by(deploy_key_id: params[:key_id]) + if key + key.destroy + else + not_found!('Deploy Key') + end end end end -- GitLab From d7755ede246988e3186a46b2c9fbd1b70660b529 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 4 Jan 2017 19:13:29 +0000 Subject: [PATCH 2/6] Merge branch 'fix/rename-group-export-vuln' into 'security' Fix export files not removed when a user takes over a namespace See merge request !2051 --- app/models/namespace.rb | 20 ++++++ .../namespace_export_file_spec.rb | 62 +++++++++++++++++++ spec/models/namespace_spec.rb | 11 +++- 3 files changed, 91 insertions(+), 2 deletions(-) create mode 100644 spec/features/projects/import_export/namespace_export_file_spec.rb diff --git a/app/models/namespace.rb b/app/models/namespace.rb index d41833de66f2..dd33975731f1 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -130,6 +130,8 @@ def move_dir Gitlab::UploadsTransfer.new.rename_namespace(path_was, path) + remove_exports! + # If repositories moved successfully we need to # send update instructions to users. # However we cannot allow rollback since we moved namespace dir @@ -214,6 +216,8 @@ def rm_dir GitlabShellWorker.perform_in(5.minutes, :rm_namespace, repository_storage_path, new_path) end end + + remove_exports! end def refresh_access_of_projects_invited_groups @@ -226,4 +230,20 @@ def refresh_access_of_projects_invited_groups def full_path_changed? path_changed? || parent_id_changed? end + + def remove_exports! + Gitlab::Popen.popen(%W(find #{export_path} -not -path #{export_path} -delete)) + end + + def export_path + File.join(Gitlab::ImportExport.storage_path, full_path_was) + end + + def full_path_was + if parent + parent.full_path + '/' + path_was + else + path_was + end + end end diff --git a/spec/features/projects/import_export/namespace_export_file_spec.rb b/spec/features/projects/import_export/namespace_export_file_spec.rb new file mode 100644 index 000000000000..d0bafc6168cf --- /dev/null +++ b/spec/features/projects/import_export/namespace_export_file_spec.rb @@ -0,0 +1,62 @@ +require 'spec_helper' + +feature 'Import/Export - Namespace export file cleanup', feature: true, js: true do + let(:export_path) { "#{Dir::tmpdir}/import_file_spec" } + let(:config_hash) { YAML.load_file(Gitlab::ImportExport.config_file).deep_stringify_keys } + + let(:project) { create(:empty_project) } + + background do + allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path) + end + + after do + FileUtils.rm_rf(export_path, secure: true) + end + + context 'admin user' do + before do + login_as(:admin) + end + + context 'moving the namespace' do + scenario 'removes the export file' do + setup_export_project + + old_export_path = project.export_path.dup + + expect(File).to exist(old_export_path) + + project.namespace.update(path: 'new_path') + + expect(File).not_to exist(old_export_path) + end + end + + context 'deleting the namespace' do + scenario 'removes the export file' do + setup_export_project + + old_export_path = project.export_path.dup + + expect(File).to exist(old_export_path) + + project.namespace.destroy + + expect(File).not_to exist(old_export_path) + end + end + + def setup_export_project + visit edit_namespace_project_path(project.namespace, project) + + expect(page).to have_content('Export project') + + click_link 'Export project' + + visit edit_namespace_project_path(project.namespace, project) + + expect(page).to have_content('Download export') + end + end +end diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 600538ff5f43..f8e03fa114a8 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -117,6 +117,7 @@ new_path = @namespace.path + "_new" allow(@namespace).to receive(:path_was).and_return(@namespace.path) allow(@namespace).to receive(:path).and_return(new_path) + expect(@namespace).to receive(:remove_exports!) expect(@namespace.move_dir).to be_truthy end @@ -139,11 +140,17 @@ let!(:project) { create(:project, namespace: namespace) } let!(:path) { File.join(Gitlab.config.repositories.storages.default, namespace.path) } - before { namespace.destroy } - it "removes its dirs when deleted" do + namespace.destroy + expect(File.exist?(path)).to be(false) end + + it 'removes the exports folder' do + expect(namespace).to receive(:remove_exports!) + + namespace.destroy + end end describe '.find_by_path_or_name' do -- GitLab From 3a5df1d8fc518900d8e33a6be8a2243e399c754a Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 3 Jan 2017 18:03:13 +0000 Subject: [PATCH 3/6] Merge branch 'fix-api-mr-permissions' into 'security' Ensure that only privileged users can access merge requests in the API See merge request !2053 --- .../unreleased/fix-api-mr-permissions.yml | 4 +++ lib/api/helpers.rb | 6 ++++ lib/api/merge_request_diffs.rb | 8 ++--- lib/api/merge_requests.rb | 25 +++++++--------- lib/api/subscriptions.rb | 4 +-- lib/api/todos.rb | 2 +- spec/requests/api/merge_requests_spec.rb | 29 +++++++++++++++++++ spec/requests/api/todos_spec.rb | 15 +++++++++- 8 files changed, 68 insertions(+), 25 deletions(-) create mode 100644 changelogs/unreleased/fix-api-mr-permissions.yml diff --git a/changelogs/unreleased/fix-api-mr-permissions.yml b/changelogs/unreleased/fix-api-mr-permissions.yml new file mode 100644 index 000000000000..33b677b1f291 --- /dev/null +++ b/changelogs/unreleased/fix-api-mr-permissions.yml @@ -0,0 +1,4 @@ +--- +title: Don't allow project guests to subscribe to merge requests through the API +merge_request: +author: Robert Schilling diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 49c5f0652ab5..a1d7b323f4f1 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -90,6 +90,12 @@ def find_project_merge_request(id) MergeRequestsFinder.new(current_user, project_id: user_project.id).find(id) end + def find_merge_request_with_access(id, access_level = :read_merge_request) + merge_request = user_project.merge_requests.find(id) + authorize! access_level, merge_request + merge_request + end + def authenticate! unauthorized! unless current_user end diff --git a/lib/api/merge_request_diffs.rb b/lib/api/merge_request_diffs.rb index 07435d78468e..bc3d69f6904b 100644 --- a/lib/api/merge_request_diffs.rb +++ b/lib/api/merge_request_diffs.rb @@ -15,10 +15,8 @@ class MergeRequestDiffs < Grape::API end get ":id/merge_requests/:merge_request_id/versions" do - merge_request = user_project.merge_requests. - find(params[:merge_request_id]) + merge_request = find_merge_request_with_access(params[:merge_request_id]) - authorize! :read_merge_request, merge_request present merge_request.merge_request_diffs, with: Entities::MergeRequestDiff end @@ -34,10 +32,8 @@ class MergeRequestDiffs < Grape::API end get ":id/merge_requests/:merge_request_id/versions/:version_id" do - merge_request = user_project.merge_requests. - find(params[:merge_request_id]) + merge_request = find_merge_request_with_access(params[:merge_request_id]) - authorize! :read_merge_request, merge_request present merge_request.merge_request_diffs.find(params[:version_id]), with: Entities::MergeRequestDiffFull end end diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index e77af4b7a0d3..7ffb38e62daa 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -118,8 +118,8 @@ def handle_merge_request_errors!(errors) success Entities::MergeRequest end get path do - merge_request = find_project_merge_request(params[:merge_request_id]) - authorize! :read_merge_request, merge_request + merge_request = find_merge_request_with_access(params[:merge_request_id]) + present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project end @@ -127,8 +127,8 @@ def handle_merge_request_errors!(errors) success Entities::RepoCommit end get "#{path}/commits" do - merge_request = find_project_merge_request(params[:merge_request_id]) - authorize! :read_merge_request, merge_request + merge_request = find_merge_request_with_access(params[:merge_request_id]) + present merge_request.commits, with: Entities::RepoCommit end @@ -136,8 +136,8 @@ def handle_merge_request_errors!(errors) success Entities::MergeRequestChanges end get "#{path}/changes" do - merge_request = find_project_merge_request(params[:merge_request_id]) - authorize! :read_merge_request, merge_request + merge_request = find_merge_request_with_access(params[:merge_request_id]) + present merge_request, with: Entities::MergeRequestChanges, current_user: current_user end @@ -155,8 +155,7 @@ def handle_merge_request_errors!(errors) :remove_source_branch end put path do - merge_request = find_project_merge_request(params.delete(:merge_request_id)) - authorize! :update_merge_request, merge_request + merge_request = find_merge_request_with_access(params.delete(:merge_request_id), :update_merge_request) mr_params = declared_params(include_missing: false) mr_params[:force_remove_source_branch] = mr_params.delete(:remove_source_branch) if mr_params[:remove_source_branch].present? @@ -235,10 +234,7 @@ def handle_merge_request_errors!(errors) use :pagination end get "#{path}/comments" do - merge_request = find_project_merge_request(params[:merge_request_id]) - - authorize! :read_merge_request, merge_request - + merge_request = find_merge_request_with_access(params[:merge_request_id]) present paginate(merge_request.notes.fresh), with: Entities::MRNote end @@ -250,8 +246,7 @@ def handle_merge_request_errors!(errors) requires :note, type: String, desc: 'The text of the comment' end post "#{path}/comments" do - merge_request = find_project_merge_request(params[:merge_request_id]) - authorize! :create_note, merge_request + merge_request = find_merge_request_with_access(params[:merge_request_id], :create_note) opts = { note: params[:note], @@ -275,7 +270,7 @@ def handle_merge_request_errors!(errors) use :pagination end get "#{path}/closes_issues" do - merge_request = find_project_merge_request(params[:merge_request_id]) + merge_request = find_merge_request_with_access(params[:merge_request_id]) issues = ::Kaminari.paginate_array(merge_request.closes_issues(current_user)) present paginate(issues), with: issue_entity(user_project), current_user: current_user end diff --git a/lib/api/subscriptions.rb b/lib/api/subscriptions.rb index 10749b34004f..e11d7537cc9c 100644 --- a/lib/api/subscriptions.rb +++ b/lib/api/subscriptions.rb @@ -3,8 +3,8 @@ class Subscriptions < Grape::API before { authenticate! } subscribable_types = { - 'merge_request' => proc { |id| user_project.merge_requests.find(id) }, - 'merge_requests' => proc { |id| user_project.merge_requests.find(id) }, + 'merge_request' => proc { |id| find_merge_request_with_access(id, :update_merge_request) }, + 'merge_requests' => proc { |id| find_merge_request_with_access(id, :update_merge_request) }, 'issues' => proc { |id| find_project_issue(id) }, 'labels' => proc { |id| find_project_label(id) }, } diff --git a/lib/api/todos.rb b/lib/api/todos.rb index ed8f48aa1e3b..9bd077263a7e 100644 --- a/lib/api/todos.rb +++ b/lib/api/todos.rb @@ -5,7 +5,7 @@ class Todos < Grape::API before { authenticate! } ISSUABLE_TYPES = { - 'merge_requests' => ->(id) { user_project.merge_requests.find(id) }, + 'merge_requests' => ->(id) { find_merge_request_with_access(id) }, 'issues' => ->(id) { find_project_issue(id) } } diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 6f20ac492694..71a7994e544c 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -627,6 +627,17 @@ expect(json_response.first['title']).to eq(issue.title) expect(json_response.first['id']).to eq(issue.id) end + + it 'returns 403 if the user has no access to the merge request' do + project = create(:empty_project, :private) + merge_request = create(:merge_request, :simple, source_project: project) + guest = create(:user) + project.team << [guest, :guest] + + get api("/projects/#{project.id}/merge_requests/#{merge_request.id}/closes_issues", guest) + + expect(response).to have_http_status(403) + end end describe 'POST :id/merge_requests/:merge_request_id/subscription' do @@ -648,6 +659,15 @@ expect(response).to have_http_status(404) end + + it 'returns 403 if user has no access to read code' do + guest = create(:user) + project.team << [guest, :guest] + + post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/subscription", guest) + + expect(response).to have_http_status(403) + end end describe 'DELETE :id/merge_requests/:merge_request_id/subscription' do @@ -669,6 +689,15 @@ expect(response).to have_http_status(404) end + + it 'returns 403 if user has no access to read code' do + guest = create(:user) + project.team << [guest, :guest] + + delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}/subscription", guest) + + expect(response).to have_http_status(403) + end end describe 'Time tracking' do diff --git a/spec/requests/api/todos_spec.rb b/spec/requests/api/todos_spec.rb index 6fe695626caa..56dc017ce54b 100644 --- a/spec/requests/api/todos_spec.rb +++ b/spec/requests/api/todos_spec.rb @@ -183,12 +183,25 @@ expect(response.status).to eq(404) end + + it 'returns an error if the issuable is not accessible' do + guest = create(:user) + project_1.team << [guest, :guest] + + post api("/projects/#{project_1.id}/#{issuable_type}/#{issuable.id}/todo", guest) + + if issuable_type == 'merge_requests' + expect(response).to have_http_status(403) + else + expect(response).to have_http_status(404) + end + end end describe 'POST :id/issuable_type/:issueable_id/todo' do context 'for an issue' do it_behaves_like 'an issuable', 'issues' do - let(:issuable) { create(:issue, author: author_1, project: project_1) } + let(:issuable) { create(:issue, :confidential, author: author_1, project: project_1) } end end -- GitLab From a1f959430b752aca21f798d37d338e11afaa6110 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 19 Jan 2017 21:00:47 +0000 Subject: [PATCH 4/6] Merge branch 'fix-guest-access-posting-to-notes' into 'security' Prevent users from creating notes on resources they can't access See https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2054 --- .../fix-guest-access-posting-to-notes.yml | 4 +++ lib/api/notes.rb | 26 ++++++++++++------- spec/requests/api/notes_spec.rb | 12 +++++++++ 3 files changed, 32 insertions(+), 10 deletions(-) create mode 100644 changelogs/unreleased/fix-guest-access-posting-to-notes.yml diff --git a/changelogs/unreleased/fix-guest-access-posting-to-notes.yml b/changelogs/unreleased/fix-guest-access-posting-to-notes.yml new file mode 100644 index 000000000000..81377c0c6f08 --- /dev/null +++ b/changelogs/unreleased/fix-guest-access-posting-to-notes.yml @@ -0,0 +1,4 @@ +--- +title: Prevent users from creating notes on resources they can't access +merge_request: +author: diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 284e4cf549ad..4d2a8f482670 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -70,21 +70,27 @@ class Notes < Grape::API end post ":id/#{noteables_str}/:noteable_id/notes" do opts = { - note: params[:body], - noteable_type: noteables_str.classify, - noteable_id: params[:noteable_id] + note: params[:body], + noteable_type: noteables_str.classify, + noteable_id: params[:noteable_id] } - if params[:created_at] && (current_user.is_admin? || user_project.owner == current_user) - opts[:created_at] = params[:created_at] - end + noteable = user_project.send(noteables_str.to_sym).find(params[:noteable_id]) + + if can?(current_user, noteable_read_ability_name(noteable), noteable) + if params[:created_at] && (current_user.is_admin? || user_project.owner == current_user) + opts[:created_at] = params[:created_at] + end - note = ::Notes::CreateService.new(user_project, current_user, opts).execute + note = ::Notes::CreateService.new(user_project, current_user, opts).execute - if note.valid? - present note, with: Entities::const_get(note.class.name) + if note.valid? + present note, with: Entities::const_get(note.class.name) + else + not_found!("Note #{note.errors.messages}") + end else - not_found!("Note #{note.errors.messages}") + not_found!("Note") end end diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index 0f8d054b31e9..0353ebea9e59 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -264,6 +264,18 @@ end end + context 'when user does not have access to read the noteable' do + it 'responds with 404' do + project = create(:empty_project, :private) { |p| p.add_guest(user) } + issue = create(:issue, :confidential, project: project) + + post api("/projects/#{project.id}/issues/#{issue.id}/notes", user), + body: 'Foo' + + expect(response).to have_http_status(404) + end + end + context 'when user does not have access to create noteable' do let(:private_issue) { create(:issue, project: create(:empty_project, :private)) } -- GitLab From ec3226bd60f545c8ffb231011c45afa0bfc66e86 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 19 Jan 2017 17:49:16 +0000 Subject: [PATCH 5/6] Merge branch 'upgrade-omniauth' into 'security' Upgrade OmniAuth Ruby gem to 1.3.2 Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/26813 See merge request !2056 --- Gemfile | 2 +- Gemfile.lock | 4 ++-- changelogs/unreleased/upgrade-omniauth.yml | 4 ++++ 3 files changed, 7 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/upgrade-omniauth.yml diff --git a/Gemfile b/Gemfile index 83ba5d31b92f..4e9cf91c4295 100644 --- a/Gemfile +++ b/Gemfile @@ -21,7 +21,7 @@ gem 'rugged', '~> 0.24.0' # Authentication libraries gem 'devise', '~> 4.2' gem 'doorkeeper', '~> 4.2.0' -gem 'omniauth', '~> 1.3.1' +gem 'omniauth', '~> 1.3.2' gem 'omniauth-auth0', '~> 1.4.1' gem 'omniauth-azure-oauth2', '~> 0.0.6' gem 'omniauth-cas3', '~> 1.1.2' diff --git a/Gemfile.lock b/Gemfile.lock index 104e6444803e..c91159828386 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -449,7 +449,7 @@ GEM octokit (4.6.2) sawyer (~> 0.8.0, >= 0.5.3) oj (2.17.4) - omniauth (1.3.1) + omniauth (1.3.2) hashie (>= 1.2, < 4) rack (>= 1.0, < 3) omniauth-auth0 (1.4.1) @@ -925,7 +925,7 @@ DEPENDENCIES oauth2 (~> 1.2.0) octokit (~> 4.6.2) oj (~> 2.17.4) - omniauth (~> 1.3.1) + omniauth (~> 1.3.2) omniauth-auth0 (~> 1.4.1) omniauth-authentiq (~> 0.2.0) omniauth-azure-oauth2 (~> 0.0.6) diff --git a/changelogs/unreleased/upgrade-omniauth.yml b/changelogs/unreleased/upgrade-omniauth.yml new file mode 100644 index 000000000000..7e0334566dca --- /dev/null +++ b/changelogs/unreleased/upgrade-omniauth.yml @@ -0,0 +1,4 @@ +--- +title: Upgrade omniauth gem to 1.3.2 +merge_request: +author: -- GitLab From 76deb55f5602ffb137d6e1daf17212e36efb52bf Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Fri, 20 Jan 2017 13:28:27 -0500 Subject: [PATCH 6/6] Add a changelog entry for #26242 --- changelogs/unreleased/8-15-stable.yml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelogs/unreleased/8-15-stable.yml diff --git a/changelogs/unreleased/8-15-stable.yml b/changelogs/unreleased/8-15-stable.yml new file mode 100644 index 000000000000..75502e139e7b --- /dev/null +++ b/changelogs/unreleased/8-15-stable.yml @@ -0,0 +1,4 @@ +--- +title: Ensure export files are removed after a namespace is deleted +merge_request: +author: -- GitLab