From bb430504468235ca473daa0e1080910be6be4f6c Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 6 Jan 2018 06:18:13 +0000 Subject: Merge branch 'security-10-3-do-not-expose-passwords-or-tokens-in-service-integrations-api' into 'security-10-3' Filter out sensitive fields from the project services API See merge request gitlab/gitlabhq!2281 (cherry picked from commit 476f2576444632f2a9a61b4cead9c1077f2c81d7) 2bcbbda0 Filter out sensitive fields from the project services API --- app/models/service.rb | 5 ++++ changelogs/unreleased/api-no-service-pw-output.yml | 5 ++++ lib/api/entities.rb | 5 +--- lib/api/services.rb | 2 +- lib/api/v3/entities.rb | 5 +--- lib/api/v3/services.rb | 2 +- spec/models/service_spec.rb | 34 ++++++++++++++++++++++ spec/requests/api/services_spec.rb | 4 +-- spec/support/services_shared_context.rb | 8 ++--- 9 files changed, 52 insertions(+), 18 deletions(-) create mode 100644 changelogs/unreleased/api-no-service-pw-output.yml diff --git a/app/models/service.rb b/app/models/service.rb index 3c4f1885dd0..e5b8168cdd3 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -117,6 +117,11 @@ class Service < ActiveRecord::Base nil end + def api_field_names + fields.map { |field| field[:name] } + .reject { |field_name| field_name =~ /(password|token|key)/ } + end + def global_fields fields end diff --git a/changelogs/unreleased/api-no-service-pw-output.yml b/changelogs/unreleased/api-no-service-pw-output.yml new file mode 100644 index 00000000000..f0d0adaad1c --- /dev/null +++ b/changelogs/unreleased/api-no-service-pw-output.yml @@ -0,0 +1,5 @@ +--- +title: Filter out sensitive fields from the project services API +merge_request: +author: Robert Schilling +type: security diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 928706dfda7..2031da8e89d 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -713,10 +713,7 @@ module API expose :job_events # Expose serialized properties expose :properties do |service, options| - field_names = service.fields - .select { |field| options[:include_passwords] || field[:type] != 'password' } - .map { |field| field[:name] } - service.properties.slice(*field_names) + service.properties.slice(*service.api_field_names) end end diff --git a/lib/api/services.rb b/lib/api/services.rb index bbcc851d07a..10e77953664 100644 --- a/lib/api/services.rb +++ b/lib/api/services.rb @@ -669,7 +669,7 @@ module API service_params = declared_params(include_missing: false).merge(active: true) if service.update_attributes(service_params) - present service, with: Entities::ProjectService, include_passwords: current_user.admin? + present service, with: Entities::ProjectService else render_api_error!('400 Bad Request', 400) end diff --git a/lib/api/v3/entities.rb b/lib/api/v3/entities.rb index c17b6f45ed8..b74f12410de 100644 --- a/lib/api/v3/entities.rb +++ b/lib/api/v3/entities.rb @@ -257,10 +257,7 @@ module API expose :job_events, as: :build_events # Expose serialized properties expose :properties do |service, options| - field_names = service.fields - .select { |field| options[:include_passwords] || field[:type] != 'password' } - .map { |field| field[:name] } - service.properties.slice(*field_names) + service.properties.slice(*service.api_field_names) end end diff --git a/lib/api/v3/services.rb b/lib/api/v3/services.rb index 44ed94d2869..20ca1021c71 100644 --- a/lib/api/v3/services.rb +++ b/lib/api/v3/services.rb @@ -622,7 +622,7 @@ module API end get ":id/services/:service_slug" do service = user_project.find_or_initialize_service(params[:service_slug].underscore) - present service, with: Entities::ProjectService, include_passwords: current_user.admin? + present service, with: Entities::ProjectService end end diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb index 0f2f906c667..49031b6323e 100644 --- a/spec/models/service_spec.rb +++ b/spec/models/service_spec.rb @@ -254,4 +254,38 @@ describe Service do end end end + + describe '#api_field_names' do + let(:fake_service) do + Class.new(Service) do + def fields + [ + { name: 'token' }, + { name: 'api_token' }, + { name: 'key' }, + { name: 'api_key' }, + { name: 'password' }, + { name: 'password_field' }, + { name: 'safe_field' } + ] + end + end + end + + let(:service) do + fake_service.new(properties: [ + { token: 'token-value' }, + { api_token: 'api_token-value' }, + { key: 'key-value' }, + { api_key: 'api_key-value' }, + { password: 'password-value' }, + { password_field: 'password_field-value' }, + { safe_field: 'safe_field-value' } + ]) + end + + it 'filters out sensitive fields' do + expect(service.api_field_names).to eq(['safe_field']) + end + end end diff --git a/spec/requests/api/services_spec.rb b/spec/requests/api/services_spec.rb index ba697e2b305..f4c7886e8d4 100644 --- a/spec/requests/api/services_spec.rb +++ b/spec/requests/api/services_spec.rb @@ -81,14 +81,14 @@ describe API::Services do get api("/projects/#{project.id}/services/#{dashed_service}", admin) expect(response).to have_gitlab_http_status(200) - expect(json_response['properties'].keys.map(&:to_sym)).to match_array(service_attrs_list.map) + expect(json_response['properties'].keys).to match_array(service_instance.api_field_names) end it "returns properties of service #{service} other than passwords when authenticated as project owner" do get api("/projects/#{project.id}/services/#{dashed_service}", user) expect(response).to have_gitlab_http_status(200) - expect(json_response['properties'].keys.map(&:to_sym)).to match_array(service_attrs_list_without_passwords) + expect(json_response['properties'].keys).to match_array(service_instance.api_field_names) end it "returns error when authenticated but not a project owner" do diff --git a/spec/support/services_shared_context.rb b/spec/support/services_shared_context.rb index 7457484a932..b17da18d4d5 100644 --- a/spec/support/services_shared_context.rb +++ b/spec/support/services_shared_context.rb @@ -3,13 +3,9 @@ Service.available_services_names.each do |service| let(:dashed_service) { service.dasherize } let(:service_method) { "#{service}_service".to_sym } let(:service_klass) { "#{service}_service".classify.constantize } - let(:service_fields) { service_klass.new.fields } + let(:service_instance) { service_klass.new } + let(:service_fields) { service_instance.fields } let(:service_attrs_list) { service_fields.inject([]) {|arr, hash| arr << hash[:name].to_sym } } - let(:service_attrs_list_without_passwords) do - service_fields - .select { |field| field[:type] != 'password' } - .map { |field| field[:name].to_sym} - end let(:service_attrs) do service_attrs_list.inject({}) do |hash, k| if k =~ /^(token*|.*_token|.*_key)/ -- cgit v1.2.1 From 64a70d38d15674a7eef9762ae0e30ac5a79e4f95 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 21 Dec 2017 18:34:34 +0000 Subject: Merge branch 'ac/41346-xss-ci-job-output' into 'security-10-3' [10.3] Fix XSS vulnerability in Pipeline job trace See merge request gitlab/gitlabhq!2258 (cherry picked from commit 44caa80ed9a2514a74a5eeab10ff51849d64851b) 5f86f3ff Fix XSS vulnerability in Pipeline job trace --- lib/gitlab/regex.rb | 2 +- spec/lib/gitlab/ci/ansi2html_spec.rb | 55 +++++++++++++++++++++++++++++++++--- 2 files changed, 52 insertions(+), 5 deletions(-) diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb index 2c7b8af83f2..5065be33f64 100644 --- a/lib/gitlab/regex.rb +++ b/lib/gitlab/regex.rb @@ -67,7 +67,7 @@ module Gitlab end def build_trace_section_regex - @build_trace_section_regexp ||= /section_((?:start)|(?:end)):(\d+):([^\r]+)\r\033\[0K/.freeze + @build_trace_section_regexp ||= /section_((?:start)|(?:end)):(\d+):([a-zA-Z0-9_.-]+)\r\033\[0K/.freeze end end end diff --git a/spec/lib/gitlab/ci/ansi2html_spec.rb b/spec/lib/gitlab/ci/ansi2html_spec.rb index 33540eab5d6..d8df1312c47 100644 --- a/spec/lib/gitlab/ci/ansi2html_spec.rb +++ b/spec/lib/gitlab/ci/ansi2html_spec.rb @@ -213,11 +213,58 @@ describe Gitlab::Ci::Ansi2html do "#{section_end[0...-5]}" end - it "prints light red" do - text = "#{section_start}\e[91mHello\e[0m\n#{section_end}" - html = %{#{section_start_html}Hello
#{section_end_html}} + shared_examples 'forbidden char in section_name' do + it 'ignores sections' do + text = "#{section_start}Some text#{section_end}" + html = text.gsub("\033[0K", '').gsub('<', '<') - expect(convert_html(text)).to eq(html) + expect(convert_html(text)).to eq(html) + end + end + + shared_examples 'a legit section' do + let(:text) { "#{section_start}Some text#{section_end}" } + + it 'prints light red' do + text = "#{section_start}\e[91mHello\e[0m\n#{section_end}" + html = %{#{section_start_html}Hello
#{section_end_html}} + + expect(convert_html(text)).to eq(html) + end + + it 'begins with a section_start html marker' do + expect(convert_html(text)).to start_with(section_start_html) + end + + it 'ends with a section_end html marker' do + expect(convert_html(text)).to end_with(section_end_html) + end + end + + it_behaves_like 'a legit section' + + context 'section name includes $' do + let(:section_name) { 'my_$ection'} + + it_behaves_like 'forbidden char in section_name' + end + + context 'section name includes <' do + let(:section_name) { ''} + + it_behaves_like 'forbidden char in section_name' + end + + context 'section name contains .-_' do + let(:section_name) { 'a.Legit-SeCtIoN_namE' } + + it_behaves_like 'a legit section' + end + + it 'do not allow XSS injections' do + text = "#{section_start}section_end:1:2#{section_end}" + + expect(convert_html(text)).not_to include('' + end + end end context 'editing issue labels', :js do -- cgit v1.2.1 From c8de335e5ea956ac3490e90f4e7582745f8aa7da Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Fri, 5 Jan 2018 17:53:31 +0000 Subject: Merge branch 'milestones-finder-order-fix' into 'security-10-3' Remove order param from the MilestoneFinder See merge request gitlab/gitlabhq!2259 (cherry picked from commit 14408042e78f2ebc2644f956621b461dbfa3d36d) 155881e7 Remove order param from the MilestoneFinder --- app/controllers/groups/milestones_controller.rb | 6 ++-- app/controllers/projects/milestones_controller.rb | 14 ++++---- app/finders/milestones_finder.rb | 8 ++--- app/models/global_milestone.rb | 8 ++--- .../unreleased/milestones-finder-order-fix.yml | 5 +++ spec/finders/milestones_finder_spec.rb | 41 +++++++--------------- .../services/projects/autocomplete_service_spec.rb | 15 ++++---- 7 files changed, 43 insertions(+), 54 deletions(-) create mode 100644 changelogs/unreleased/milestones-finder-order-fix.yml diff --git a/app/controllers/groups/milestones_controller.rb b/app/controllers/groups/milestones_controller.rb index f013d21275e..acf6aaf57f4 100644 --- a/app/controllers/groups/milestones_controller.rb +++ b/app/controllers/groups/milestones_controller.rb @@ -75,8 +75,6 @@ class Groups::MilestonesController < Groups::ApplicationController end def milestones - search_params = params.merge(group_ids: group.id) - milestones = MilestonesFinder.new(search_params).execute legacy_milestones = GroupMilestone.build_collection(group, group_projects, params) @@ -94,4 +92,8 @@ class Groups::MilestonesController < Groups::ApplicationController render_404 unless @milestone end + + def search_params + params.permit(:state).merge(group_ids: group.id) + end end diff --git a/app/controllers/projects/milestones_controller.rb b/app/controllers/projects/milestones_controller.rb index 980bbf699b6..0f70efbce40 100644 --- a/app/controllers/projects/milestones_controller.rb +++ b/app/controllers/projects/milestones_controller.rb @@ -92,12 +92,6 @@ class Projects::MilestonesController < Projects::ApplicationController def milestones @milestones ||= begin - if @project.group && can?(current_user, :read_group, @project.group) - group = @project.group - end - - search_params = params.merge(project_ids: @project.id, group_ids: group&.id) - MilestonesFinder.new(search_params).execute end end @@ -113,4 +107,12 @@ class Projects::MilestonesController < Projects::ApplicationController def milestone_params params.require(:milestone).permit(:title, :description, :start_date, :due_date, :state_event) end + + def search_params + if @project.group && can?(current_user, :read_group, @project.group) + group = @project.group + end + + params.permit(:state).merge(project_ids: @project.id, group_ids: group&.id) + end end diff --git a/app/finders/milestones_finder.rb b/app/finders/milestones_finder.rb index 0a5a0ea2f35..b4605fca193 100644 --- a/app/finders/milestones_finder.rb +++ b/app/finders/milestones_finder.rb @@ -46,11 +46,7 @@ class MilestonesFinder end def order(items) - if params.has_key?(:order) - items.reorder(params[:order]) - else - order_statement = Gitlab::Database.nulls_last_order('due_date', 'ASC') - items.reorder(order_statement) - end + order_statement = Gitlab::Database.nulls_last_order('due_date', 'ASC') + items.reorder(order_statement).order('title ASC') end end diff --git a/app/models/global_milestone.rb b/app/models/global_milestone.rb index c0864769314..dc2f6817190 100644 --- a/app/models/global_milestone.rb +++ b/app/models/global_milestone.rb @@ -44,10 +44,10 @@ class GlobalMilestone def self.group_milestones_states_count(group) return STATE_COUNT_HASH unless group - params = { group_ids: [group.id], state: 'all', order: nil } + params = { group_ids: [group.id], state: 'all' } relation = MilestonesFinder.new(params).execute - grouped_by_state = relation.group(:state).count + grouped_by_state = relation.reorder(nil).group(:state).count { opened: grouped_by_state['active'] || 0, @@ -60,10 +60,10 @@ class GlobalMilestone def self.legacy_group_milestone_states_count(projects) return STATE_COUNT_HASH unless projects - params = { project_ids: projects.map(&:id), state: 'all', order: nil } + params = { project_ids: projects.map(&:id), state: 'all' } relation = MilestonesFinder.new(params).execute - project_milestones_by_state_and_title = relation.group(:state, :title).count + project_milestones_by_state_and_title = relation.reorder(nil).group(:state, :title).count opened = count_by_state(project_milestones_by_state_and_title, 'active') closed = count_by_state(project_milestones_by_state_and_title, 'closed') diff --git a/changelogs/unreleased/milestones-finder-order-fix.yml b/changelogs/unreleased/milestones-finder-order-fix.yml new file mode 100644 index 00000000000..983c301a82d --- /dev/null +++ b/changelogs/unreleased/milestones-finder-order-fix.yml @@ -0,0 +1,5 @@ +--- +title: Prevent a SQL injection in the MilestonesFinder +merge_request: +author: +type: security diff --git a/spec/finders/milestones_finder_spec.rb b/spec/finders/milestones_finder_spec.rb index 8ae08656e01..0b3cf7ece5f 100644 --- a/spec/finders/milestones_finder_spec.rb +++ b/spec/finders/milestones_finder_spec.rb @@ -21,10 +21,19 @@ describe MilestonesFinder do expect(result).to contain_exactly(milestone_1, milestone_2) end - it 'returns milestones for groups and projects' do - result = described_class.new(project_ids: [project_1.id, project_2.id], group_ids: group.id, state: 'all').execute + context 'milestones for groups and project' do + let(:result) do + described_class.new(project_ids: [project_1.id, project_2.id], group_ids: group.id, state: 'all').execute + end + + it 'returns milestones for groups and projects' do + expect(result).to contain_exactly(milestone_1, milestone_2, milestone_3, milestone_4) + end - expect(result).to contain_exactly(milestone_1, milestone_2, milestone_3, milestone_4) + it 'orders milestones by due date' do + expect(result.first).to eq(milestone_1) + expect(result.second).to eq(milestone_3) + end end context 'with filters' do @@ -61,30 +70,4 @@ describe MilestonesFinder do expect(result.to_a).to contain_exactly(milestone_1) end end - - context 'with order' do - let(:params) do - { - project_ids: [project_1.id, project_2.id], - group_ids: group.id, - state: 'all' - } - end - - it "default orders by due date" do - result = described_class.new(params).execute - - expect(result.first).to eq(milestone_1) - expect(result.second).to eq(milestone_3) - end - - it "orders by parameter" do - result = described_class.new(params.merge(order: 'id DESC')).execute - - expect(result.first).to eq(milestone_4) - expect(result.second).to eq(milestone_3) - expect(result.third).to eq(milestone_2) - expect(result.fourth).to eq(milestone_1) - end - end end diff --git a/spec/services/projects/autocomplete_service_spec.rb b/spec/services/projects/autocomplete_service_spec.rb index 426593be428..b70938b1dbc 100644 --- a/spec/services/projects/autocomplete_service_spec.rb +++ b/spec/services/projects/autocomplete_service_spec.rb @@ -93,26 +93,27 @@ describe Projects::AutocompleteService do let(:user) { create(:user) } let(:group) { create(:group) } let(:project) { create(:project, group: group) } - let!(:group_milestone) { create(:milestone, group: group) } - let!(:project_milestone) { create(:milestone, project: project) } + let!(:group_milestone1) { create(:milestone, group: group, due_date: '2017-01-01', title: 'Second Title') } + let!(:group_milestone2) { create(:milestone, group: group, due_date: '2017-01-01', title: 'First Title') } + let!(:project_milestone) { create(:milestone, project: project, due_date: '2016-01-01') } let(:milestone_titles) { described_class.new(project, user).milestones.map(&:title) } - it 'includes project and group milestones' do - expect(milestone_titles).to eq([group_milestone.title, project_milestone.title]) + it 'includes project and group milestones and sorts them correctly' do + expect(milestone_titles).to eq([project_milestone.title, group_milestone2.title, group_milestone1.title]) end it 'does not include closed milestones' do - group_milestone.close + group_milestone1.close - expect(milestone_titles).to eq([project_milestone.title]) + expect(milestone_titles).to eq([project_milestone.title, group_milestone2.title]) end it 'does not include milestones from other projects in the group' do other_project = create(:project, group: group) project_milestone.update!(project: other_project) - expect(milestone_titles).to eq([group_milestone.title]) + expect(milestone_titles).to eq([group_milestone2.title, group_milestone1.title]) end end end -- cgit v1.2.1 From 11cf18697aa33c40440432e9e697596c19c1875c Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Thu, 4 Jan 2018 05:42:52 +0000 Subject: Merge branch 'sh-validate-path-project-import-10-3' into 'security-10-3' Validate project path in Gitlab import - 10.3 port See merge request gitlab/gitlabhq!2268 (cherry picked from commit 94c82376d66fc80d46dd2d5eeb5bade408ec6a7e) 2b94a7c2 Validate project path in Gitlab import --- .../projects/gitlab_projects_import_service.rb | 2 +- .../import/gitlab_projects_controller_spec.rb | 38 ++++++++++++++++++++++ .../projects/import_export/import_file_spec.rb | 2 +- .../gitlab_projects_import_service_spec.rb | 31 ++++++++++++++++++ 4 files changed, 71 insertions(+), 2 deletions(-) create mode 100644 spec/controllers/import/gitlab_projects_controller_spec.rb create mode 100644 spec/services/projects/gitlab_projects_import_service_spec.rb diff --git a/app/services/projects/gitlab_projects_import_service.rb b/app/services/projects/gitlab_projects_import_service.rb index 4ca6414b73b..a3d7f5cbed5 100644 --- a/app/services/projects/gitlab_projects_import_service.rb +++ b/app/services/projects/gitlab_projects_import_service.rb @@ -26,7 +26,7 @@ module Projects end def tmp_filename - "#{SecureRandom.hex}_#{params[:path]}" + SecureRandom.hex end def file diff --git a/spec/controllers/import/gitlab_projects_controller_spec.rb b/spec/controllers/import/gitlab_projects_controller_spec.rb new file mode 100644 index 00000000000..8759d3c0b97 --- /dev/null +++ b/spec/controllers/import/gitlab_projects_controller_spec.rb @@ -0,0 +1,38 @@ +require 'spec_helper' + +describe Import::GitlabProjectsController do + set(:namespace) { create(:namespace) } + set(:user) { namespace.owner } + let(:file) { fixture_file_upload(Rails.root + 'spec/fixtures/doc_sample.txt', 'text/plain') } + + before do + sign_in(user) + end + + describe 'POST create' do + context 'with an invalid path' do + it 'redirects with an error' do + post :create, namespace_id: namespace.id, path: '/test', file: file + + expect(flash[:alert]).to start_with('Project could not be imported') + expect(response).to have_gitlab_http_status(302) + end + + it 'redirects with an error when a relative path is used' do + post :create, namespace_id: namespace.id, path: '../test', file: file + + expect(flash[:alert]).to start_with('Project could not be imported') + expect(response).to have_gitlab_http_status(302) + end + end + + context 'with a valid path' do + it 'redirects to the new project path' do + post :create, namespace_id: namespace.id, path: 'test', file: file + + expect(flash[:notice]).to include('is being imported') + expect(response).to have_gitlab_http_status(302) + end + end + end +end diff --git a/spec/features/projects/import_export/import_file_spec.rb b/spec/features/projects/import_export/import_file_spec.rb index af125e1b9d3..e8bb9c6a86c 100644 --- a/spec/features/projects/import_export/import_file_spec.rb +++ b/spec/features/projects/import_export/import_file_spec.rb @@ -32,7 +32,7 @@ feature 'Import/Export - project import integration test', :js do expect(page).to have_content('Import an exported GitLab project') expect(URI.parse(current_url).query).to eq("namespace_id=#{namespace.id}&path=#{project_path}") - expect(Gitlab::ImportExport).to receive(:import_upload_path).with(filename: /\A\h{32}_test-project-path\h*\z/).and_call_original + expect(Gitlab::ImportExport).to receive(:import_upload_path).with(filename: /\A\h{32}\z/).and_call_original attach_file('file', file) click_on 'Import project' diff --git a/spec/services/projects/gitlab_projects_import_service_spec.rb b/spec/services/projects/gitlab_projects_import_service_spec.rb new file mode 100644 index 00000000000..bb0e274c93e --- /dev/null +++ b/spec/services/projects/gitlab_projects_import_service_spec.rb @@ -0,0 +1,31 @@ +require 'spec_helper' + +describe Projects::GitlabProjectsImportService do + set(:namespace) { build(:namespace) } + let(:file) { fixture_file_upload(Rails.root + 'spec/fixtures/doc_sample.txt', 'text/plain') } + subject { described_class.new(namespace.owner, { namespace_id: namespace.id, path: path, file: file }) } + + describe '#execute' do + context 'with an invalid path' do + let(:path) { '/invalid-path/' } + + it 'returns an invalid project' do + project = subject.execute + + expect(project).not_to be_persisted + expect(project).not_to be_valid + end + end + + context 'with a valid path' do + let(:path) { 'test-path' } + + it 'creates a project' do + project = subject.execute + + expect(project).to be_persisted + expect(project).to be_valid + end + end + end +end -- cgit v1.2.1 From 7c4f7c283d8ae69ddfdc5feefbe31aab12906ffe Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 3 Jan 2018 18:00:36 +0000 Subject: Merge branch 'ac/fix-path-traversal' into 'security-10-3' [10.3] Fix path traversal in gitlab-ci.yml cache:key See merge request gitlab/gitlabhq!2270 (cherry picked from commit c32d0c6807dfd41d7838a35742e6d0986871b389) df29094a Fix path traversal in gitlab-ci.yml cache:key --- changelogs/unreleased/security-10-3.yml | 5 +++ doc/ci/yaml/README.md | 2 +- lib/gitlab/ci/config/entry/validators.rb | 16 +++++++- spec/lib/gitlab/ci/config/entry/key_spec.rb | 62 +++++++++++++++++++++++++++++ 4 files changed, 83 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/security-10-3.yml diff --git a/changelogs/unreleased/security-10-3.yml b/changelogs/unreleased/security-10-3.yml new file mode 100644 index 00000000000..5c718d976cd --- /dev/null +++ b/changelogs/unreleased/security-10-3.yml @@ -0,0 +1,5 @@ +--- +title: Fix path traversal in gitlab-ci.yml cache:key +merge_request: +author: +type: security diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md index 32464cbb259..a3cbb843908 100644 --- a/doc/ci/yaml/README.md +++ b/doc/ci/yaml/README.md @@ -258,7 +258,7 @@ The `cache:key` variable can use any of the [predefined variables](../variables/ The default key is **default** across the project, therefore everything is shared between each pipelines and jobs by default, starting from GitLab 9.0. ->**Note:** The `cache:key` variable cannot contain the `/` character. +>**Note:** The `cache:key` variable cannot contain the `/` character, or the equivalent URI encoded `%2F`; a value made only of dots (`.`, `%2E`) is also forbidden. --- diff --git a/lib/gitlab/ci/config/entry/validators.rb b/lib/gitlab/ci/config/entry/validators.rb index eb606b57667..55658900628 100644 --- a/lib/gitlab/ci/config/entry/validators.rb +++ b/lib/gitlab/ci/config/entry/validators.rb @@ -64,10 +64,24 @@ module Gitlab include LegacyValidationHelpers def validate_each(record, attribute, value) - unless validate_string(value) + if validate_string(value) + validate_path(record, attribute, value) + else record.errors.add(attribute, 'should be a string or symbol') end end + + private + + def validate_path(record, attribute, value) + path = CGI.unescape(value.to_s) + + if path.include?('/') + record.errors.add(attribute, 'cannot contain the "/" character') + elsif path == '.' || path == '..' + record.errors.add(attribute, 'cannot be "." or ".."') + end + end end class RegexpValidator < ActiveModel::EachValidator diff --git a/spec/lib/gitlab/ci/config/entry/key_spec.rb b/spec/lib/gitlab/ci/config/entry/key_spec.rb index 5d4de60bc8a..846f5f44470 100644 --- a/spec/lib/gitlab/ci/config/entry/key_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/key_spec.rb @@ -4,6 +4,26 @@ describe Gitlab::Ci::Config::Entry::Key do let(:entry) { described_class.new(config) } describe 'validations' do + shared_examples 'key with slash' do + it 'is invalid' do + expect(entry).not_to be_valid + end + + it 'reports errors with config value' do + expect(entry.errors).to include 'key config cannot contain the "/" character' + end + end + + shared_examples 'key with only dots' do + it 'is invalid' do + expect(entry).not_to be_valid + end + + it 'reports errors with config value' do + expect(entry.errors).to include 'key config cannot be "." or ".."' + end + end + context 'when entry config value is correct' do let(:config) { 'test' } @@ -30,6 +50,48 @@ describe Gitlab::Ci::Config::Entry::Key do end end end + + context 'when entry value contains slash' do + let(:config) { 'key/with/some/slashes' } + + it_behaves_like 'key with slash' + end + + context 'when entry value contains URI encoded slash (%2F)' do + let(:config) { 'key%2Fwith%2Fsome%2Fslashes' } + + it_behaves_like 'key with slash' + end + + context 'when entry value is a dot' do + let(:config) { '.' } + + it_behaves_like 'key with only dots' + end + + context 'when entry value is two dots' do + let(:config) { '..' } + + it_behaves_like 'key with only dots' + end + + context 'when entry value is a URI encoded dot (%2E)' do + let(:config) { '%2e' } + + it_behaves_like 'key with only dots' + end + + context 'when entry value is two URI encoded dots (%2E)' do + let(:config) { '%2E%2e' } + + it_behaves_like 'key with only dots' + end + + context 'when entry value is one dot and one URI encoded dot' do + let(:config) { '.%2e' } + + it_behaves_like 'key with only dots' + end end describe '.default' do -- cgit v1.2.1 From 523050b6383256072364937bd61054aebca2978b Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 5 Jan 2018 17:55:37 +0000 Subject: Merge branch '41567-projectfix' into 'security-10-3' check project access on MR create See merge request gitlab/gitlabhq!2273 (cherry picked from commit 1fe2325d6ef2bced4c5e97b57691c894f38b2834) 43e85f49 check project access on MR create --- app/services/merge_requests/create_service.rb | 28 +++++++--- changelogs/unreleased/projectfix.yml | 6 +++ spec/features/cycle_analytics_spec.rb | 1 + .../microsoft_teams_service_spec.rb | 4 ++ spec/requests/api/merge_requests_spec.rb | 26 ++++++--- spec/requests/api/v3/merge_requests_spec.rb | 26 ++++++--- .../services/merge_requests/create_service_spec.rb | 61 ++++++++++++++++++++++ ...ack_mattermost_notifications_shared_examples.rb | 1 + 8 files changed, 133 insertions(+), 20 deletions(-) create mode 100644 changelogs/unreleased/projectfix.yml diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb index 49cf534dc0d..634bf3bd690 100644 --- a/app/services/merge_requests/create_service.rb +++ b/app/services/merge_requests/create_service.rb @@ -1,15 +1,11 @@ module MergeRequests class CreateService < MergeRequests::BaseService def execute - # @project is used to determine whether the user can set the merge request's - # assignee, milestone and labels. Whether they can depends on their - # permissions on the target project. - source_project = @project - @project = Project.find(params[:target_project_id]) if params[:target_project_id] + set_projects! merge_request = MergeRequest.new merge_request.target_project = @project - merge_request.source_project = source_project + merge_request.source_project = @source_project merge_request.source_branch = params[:source_branch] merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch) @@ -58,5 +54,25 @@ module MergeRequests pipelines.order(id: :desc).first end + + def set_projects! + # @project is used to determine whether the user can set the merge request's + # assignee, milestone and labels. Whether they can depends on their + # permissions on the target project. + @source_project = @project + @project = Project.find(params[:target_project_id]) if params[:target_project_id] + + # make sure that source/target project ids are not in + # params so it can't be overridden later when updating attributes + # from params when applying quick actions + params.delete(:source_project_id) + params.delete(:target_project_id) + + unless can?(current_user, :read_project, @source_project) && + can?(current_user, :read_project, @project) + + raise Gitlab::Access::AccessDeniedError + end + end end end diff --git a/changelogs/unreleased/projectfix.yml b/changelogs/unreleased/projectfix.yml new file mode 100644 index 00000000000..4d8ebe6194a --- /dev/null +++ b/changelogs/unreleased/projectfix.yml @@ -0,0 +1,6 @@ +--- +title: Check user authorization for source and target projects when creating a merge + request. +merge_request: +author: +type: security diff --git a/spec/features/cycle_analytics_spec.rb b/spec/features/cycle_analytics_spec.rb index 177cd50dd72..2dac3502209 100644 --- a/spec/features/cycle_analytics_spec.rb +++ b/spec/features/cycle_analytics_spec.rb @@ -113,6 +113,7 @@ feature 'Cycle Analytics', :js do context "as a guest" do before do + project.add_developer(user) project.add_guest(guest) allow_any_instance_of(Gitlab::ReferenceExtractor).to receive(:issues).and_return([issue]) diff --git a/spec/models/project_services/microsoft_teams_service_spec.rb b/spec/models/project_services/microsoft_teams_service_spec.rb index 6a5d0decfec..733086e258f 100644 --- a/spec/models/project_services/microsoft_teams_service_spec.rb +++ b/spec/models/project_services/microsoft_teams_service_spec.rb @@ -92,6 +92,10 @@ describe MicrosoftTeamsService do service.hook_data(merge_request, 'open') end + before do + project.add_developer(user) + end + it "calls Microsoft Teams API" do chat_service.execute(merge_sample_data) diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 91616da6d9a..fe2aeb2e2ec 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -685,16 +685,28 @@ describe API::MergeRequests do expect(response).to have_gitlab_http_status(400) end - context 'when target_branch is specified' do + context 'when target_branch and target_project_id is specified' do + let(:params) do + { title: 'Test merge_request', + target_branch: 'master', + source_branch: 'markdown', + author: user2, + target_project_id: unrelated_project.id } + end + it 'returns 422 if targeting a different fork' do - post api("/projects/#{forked_project.id}/merge_requests", user2), - title: 'Test merge_request', - target_branch: 'master', - source_branch: 'markdown', - author: user2, - target_project_id: unrelated_project.id + unrelated_project.add_developer(user2) + + post api("/projects/#{forked_project.id}/merge_requests", user2), params + expect(response).to have_gitlab_http_status(422) end + + it 'returns 403 if targeting a different fork which user can not access' do + post api("/projects/#{forked_project.id}/merge_requests", user2), params + + expect(response).to have_gitlab_http_status(403) + end end it "returns 201 when target_branch is specified and for the same project" do diff --git a/spec/requests/api/v3/merge_requests_spec.rb b/spec/requests/api/v3/merge_requests_spec.rb index 2e2b9449429..0520eedd18b 100644 --- a/spec/requests/api/v3/merge_requests_spec.rb +++ b/spec/requests/api/v3/merge_requests_spec.rb @@ -371,16 +371,28 @@ describe API::MergeRequests do expect(response).to have_gitlab_http_status(400) end - context 'when target_branch is specified' do + context 'when target_branch and target_project_id is specified' do + let(:params) do + { title: 'Test merge_request', + target_branch: 'master', + source_branch: 'markdown', + author: user2, + target_project_id: unrelated_project.id } + end + it 'returns 422 if targeting a different fork' do - post v3_api("/projects/#{forked_project.id}/merge_requests", user2), - title: 'Test merge_request', - target_branch: 'master', - source_branch: 'markdown', - author: user2, - target_project_id: unrelated_project.id + unrelated_project.add_developer(user2) + + post v3_api("/projects/#{forked_project.id}/merge_requests", user2), params + expect(response).to have_gitlab_http_status(422) end + + it 'returns 403 if targeting a different fork which user can not access' do + post v3_api("/projects/#{forked_project.id}/merge_requests", user2), params + + expect(response).to have_gitlab_http_status(403) + end end it "returns 201 when target_branch is specified and for the same project" do diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index a047f891ab2..8f35c8c42f6 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -263,5 +263,66 @@ describe MergeRequests::CreateService do expect(issue_ids).to match_array([first_issue.id, second_issue.id]) end end + + context 'when source and target projects are different' do + let(:target_project) { create(:project) } + + let(:opts) do + { + title: 'Awesome merge_request', + source_branch: 'feature', + target_branch: 'master', + target_project_id: target_project.id + } + end + + context 'when user can not access source project' do + before do + target_project.add_developer(assignee) + target_project.add_master(user) + end + + it 'raises an error' do + expect { described_class.new(project, user, opts).execute } + .to raise_error Gitlab::Access::AccessDeniedError + end + end + + context 'when user can not access target project' do + before do + target_project.add_developer(assignee) + target_project.add_master(user) + end + + it 'raises an error' do + expect { described_class.new(project, user, opts).execute } + .to raise_error Gitlab::Access::AccessDeniedError + end + end + end + + context 'when user sets source project id' do + let(:another_project) { create(:project) } + + let(:opts) do + { + title: 'Awesome merge_request', + source_branch: 'feature', + target_branch: 'master', + source_project_id: another_project.id + } + end + + before do + project.add_developer(assignee) + project.add_master(user) + end + + it 'ignores source_project_id' do + merge_request = described_class.new(project, user, opts).execute + + expect(merge_request.source_project_id).to eq(project.id) + end + end end end diff --git a/spec/support/slack_mattermost_notifications_shared_examples.rb b/spec/support/slack_mattermost_notifications_shared_examples.rb index 17f3a861ba8..e827a8da0b7 100644 --- a/spec/support/slack_mattermost_notifications_shared_examples.rb +++ b/spec/support/slack_mattermost_notifications_shared_examples.rb @@ -57,6 +57,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do @issue = issue_service.execute @issues_sample_data = issue_service.hook_data(@issue, 'open') + project.add_developer(user) opts = { title: 'Awesome merge_request', description: 'please fix', -- cgit v1.2.1 From 65c5cdb6b050151e5acb205684c9ebbca4eb7358 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 5 Jan 2018 15:23:44 +0000 Subject: Merge branch 'sh-migrate-can-push-to-deploy-keys-projects-10-3' into 'security-10-3' [10.3] Migrate `can_push` column from `keys` to `deploy_keys_project` See merge request gitlab/gitlabhq!2276 (cherry picked from commit f6ca52d31bac350a23938e0aebf717c767b4710c) 1f2bd3c0 Backport to 10.3 --- .../javascripts/deploy_keys/components/key.vue | 27 ++++++--- app/controllers/admin/deploy_keys_controller.rb | 4 +- app/controllers/projects/deploy_keys_controller.rb | 9 ++- app/models/deploy_key.rb | 20 ++++++- app/models/deploy_keys_project.rb | 10 +++- .../projects/settings/deploy_keys_presenter.rb | 2 +- app/serializers/deploy_key_entity.rb | 9 +-- app/serializers/deploy_keys_project_entity.rb | 4 ++ app/views/admin/deploy_keys/index.html.haml | 8 +-- app/views/projects/deploy_keys/_form.html.haml | 18 +++--- app/views/shared/deploy_keys/_form.html.haml | 19 ++++--- ...sh-migrate-can-push-to-deploy-keys-projects.yml | 5 ++ ...1145425_add_can_push_to_deploy_keys_projects.rb | 15 +++++ ..._populate_can_push_from_deploy_keys_projects.rb | 44 +++++++++++++++ ..._populate_can_push_from_deploy_keys_projects.rb | 43 +++++++++++++++ .../20171215121259_remove_can_push_from_keys.rb | 17 ++++++ db/schema.rb | 4 +- doc/api/deploy_keys.md | 48 ++++++++++++---- lib/api/deploy_keys.rb | 64 +++++++++++++++------- lib/api/entities.rb | 7 ++- lib/api/v3/deploy_keys.rb | 46 +++++++++++----- spec/factories/deploy_keys_projects.rb | 4 ++ spec/factories/keys.rb | 4 -- spec/features/admin/admin_deploy_keys_spec.rb | 14 +++-- .../projects/settings/repository_settings_spec.rb | 6 +- .../javascripts/deploy_keys/components/key_spec.js | 20 ++++--- spec/lib/gitlab/auth_spec.rb | 6 +- spec/lib/gitlab/git_access_spec.rb | 14 ++--- spec/models/deploy_keys_project_spec.rb | 2 +- spec/requests/api/deploy_keys_spec.rb | 12 +--- spec/requests/api/v3/deploy_keys_spec.rb | 2 +- spec/requests/lfs_http_spec.rb | 4 +- spec/serializers/deploy_key_entity_spec.rb | 15 +++-- 33 files changed, 384 insertions(+), 142 deletions(-) create mode 100644 app/serializers/deploy_keys_project_entity.rb create mode 100644 changelogs/unreleased/sh-migrate-can-push-to-deploy-keys-projects.yml create mode 100644 db/migrate/20171211145425_add_can_push_to_deploy_keys_projects.rb create mode 100644 db/migrate/20171215113714_populate_can_push_from_deploy_keys_projects.rb create mode 100644 db/post_migrate/20171215121205_post_populate_can_push_from_deploy_keys_projects.rb create mode 100644 db/post_migrate/20171215121259_remove_can_push_from_keys.rb diff --git a/app/assets/javascripts/deploy_keys/components/key.vue b/app/assets/javascripts/deploy_keys/components/key.vue index b41d464475f..7df2440a7c4 100644 --- a/app/assets/javascripts/deploy_keys/components/key.vue +++ b/app/assets/javascripts/deploy_keys/components/key.vue @@ -1,5 +1,6 @@ @@ -51,20 +58,22 @@
{{ deployKey.fingerprint }}
-
- Write access allowed -
- {{ project.full_name }} + {{ deployKeysProject.project.full_name }} +
diff --git a/app/controllers/admin/deploy_keys_controller.rb b/app/controllers/admin/deploy_keys_controller.rb index a7ab481519d..b0c4c31cffc 100644 --- a/app/controllers/admin/deploy_keys_controller.rb +++ b/app/controllers/admin/deploy_keys_controller.rb @@ -50,10 +50,10 @@ class Admin::DeployKeysController < Admin::ApplicationController end def create_params - params.require(:deploy_key).permit(:key, :title, :can_push) + params.require(:deploy_key).permit(:key, :title) end def update_params - params.require(:deploy_key).permit(:title, :can_push) + params.require(:deploy_key).permit(:title) end end diff --git a/app/controllers/projects/deploy_keys_controller.rb b/app/controllers/projects/deploy_keys_controller.rb index cf8829ba95b..103079888b2 100644 --- a/app/controllers/projects/deploy_keys_controller.rb +++ b/app/controllers/projects/deploy_keys_controller.rb @@ -24,7 +24,7 @@ class Projects::DeployKeysController < Projects::ApplicationController def create @key = DeployKeys::CreateService.new(current_user, create_params).execute - unless @key.valid? && @project.deploy_keys << @key + unless @key.valid? flash[:alert] = @key.errors.full_messages.join(', ').html_safe end redirect_to_repository_settings(@project) @@ -70,11 +70,14 @@ class Projects::DeployKeysController < Projects::ApplicationController end def create_params - params.require(:deploy_key).permit(:key, :title, :can_push) + create_params = params.require(:deploy_key) + .permit(:key, :title, deploy_keys_projects_attributes: [:can_push]) + create_params.dig(:deploy_keys_projects_attributes, '0')&.merge!(project_id: @project.id) + create_params end def update_params - params.require(:deploy_key).permit(:title, :can_push) + params.require(:deploy_key).permit(:title, deploy_keys_projects_attributes: [:id, :can_push]) end def authorize_update_deploy_key! diff --git a/app/models/deploy_key.rb b/app/models/deploy_key.rb index eae5eee4fee..c2e0a5fa126 100644 --- a/app/models/deploy_key.rb +++ b/app/models/deploy_key.rb @@ -1,10 +1,16 @@ class DeployKey < Key - has_many :deploy_keys_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + include IgnorableColumn + + has_many :deploy_keys_projects, inverse_of: :deploy_key, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :projects, through: :deploy_keys_projects scope :in_projects, ->(projects) { joins(:deploy_keys_projects).where('deploy_keys_projects.project_id in (?)', projects) } scope :are_public, -> { where(public: true) } + ignore_column :can_push + + accepts_nested_attributes_for :deploy_keys_projects + def private? !public? end @@ -22,10 +28,18 @@ class DeployKey < Key end def has_access_to?(project) - projects.include?(project) + deploy_keys_project_for(project).present? end def can_push_to?(project) - can_push? && has_access_to?(project) + !!deploy_keys_project_for(project)&.can_push? + end + + def deploy_keys_project_for(project) + deploy_keys_projects.find_by(project: project) + end + + def projects_with_write_access + Project.preload(:route).where(id: deploy_keys_projects.with_write_access.select(:project_id)) end end diff --git a/app/models/deploy_keys_project.rb b/app/models/deploy_keys_project.rb index b37b9bfbdac..6eef12c4373 100644 --- a/app/models/deploy_keys_project.rb +++ b/app/models/deploy_keys_project.rb @@ -1,8 +1,14 @@ class DeployKeysProject < ActiveRecord::Base belongs_to :project - belongs_to :deploy_key + belongs_to :deploy_key, inverse_of: :deploy_keys_projects - validates :deploy_key_id, presence: true + scope :without_project_deleted, -> { joins(:project).where(projects: { pending_delete: false }) } + scope :in_project, ->(project) { where(project: project) } + scope :with_write_access, -> { where(can_push: true) } + + accepts_nested_attributes_for :deploy_key + + validates :deploy_key, presence: true validates :deploy_key_id, uniqueness: { scope: [:project_id], message: "already exists in project" } validates :project_id, presence: true diff --git a/app/presenters/projects/settings/deploy_keys_presenter.rb b/app/presenters/projects/settings/deploy_keys_presenter.rb index 229311eb6ee..c226586fba5 100644 --- a/app/presenters/projects/settings/deploy_keys_presenter.rb +++ b/app/presenters/projects/settings/deploy_keys_presenter.rb @@ -7,7 +7,7 @@ module Projects delegate :size, to: :available_public_keys, prefix: true def new_key - @key ||= DeployKey.new + @key ||= DeployKey.new.tap { |dk| dk.deploy_keys_projects.build } end def enabled_keys diff --git a/app/serializers/deploy_key_entity.rb b/app/serializers/deploy_key_entity.rb index c75431a79ae..2678f99510c 100644 --- a/app/serializers/deploy_key_entity.rb +++ b/app/serializers/deploy_key_entity.rb @@ -3,19 +3,20 @@ class DeployKeyEntity < Grape::Entity expose :user_id expose :title expose :fingerprint - expose :can_push expose :destroyed_when_orphaned?, as: :destroyed_when_orphaned expose :almost_orphaned?, as: :almost_orphaned expose :created_at expose :updated_at - expose :projects, using: ProjectEntity do |deploy_key| - deploy_key.projects.without_deleted.select { |project| options[:user].can?(:read_project, project) } + expose :deploy_keys_projects, using: DeployKeysProjectEntity do |deploy_key| + deploy_key.deploy_keys_projects + .without_project_deleted + .select { |deploy_key_project| Ability.allowed?(options[:user], :read_project, deploy_key_project.project) } end expose :can_edit private def can_edit - options[:user].can?(:update_deploy_key, object) + Ability.allowed?(options[:user], :update_deploy_key, object) end end diff --git a/app/serializers/deploy_keys_project_entity.rb b/app/serializers/deploy_keys_project_entity.rb new file mode 100644 index 00000000000..568ef5ab75e --- /dev/null +++ b/app/serializers/deploy_keys_project_entity.rb @@ -0,0 +1,4 @@ +class DeployKeysProjectEntity < Grape::Entity + expose :can_push + expose :project, using: ProjectEntity +end diff --git a/app/views/admin/deploy_keys/index.html.haml b/app/views/admin/deploy_keys/index.html.haml index 92370034baa..1420163fd5a 100644 --- a/app/views/admin/deploy_keys/index.html.haml +++ b/app/views/admin/deploy_keys/index.html.haml @@ -12,7 +12,7 @@ %tr %th.col-sm-2 Title %th.col-sm-4 Fingerprint - %th.col-sm-2 Write access allowed + %th.col-sm-2 Projects with write access %th.col-sm-2 Added at %th.col-sm-2 %tbody @@ -23,10 +23,8 @@ %td %code.key-fingerprint= deploy_key.fingerprint %td - - if deploy_key.can_push? - Yes - - else - No + - deploy_key.projects_with_write_access.each do |project| + = link_to project.full_name, admin_project_path(project), class: 'label deploy-project-label' %td %span.cgray added #{time_ago_with_tooltip(deploy_key.created_at)} diff --git a/app/views/projects/deploy_keys/_form.html.haml b/app/views/projects/deploy_keys/_form.html.haml index edaa3a1119e..c363180d0db 100644 --- a/app/views/projects/deploy_keys/_form.html.haml +++ b/app/views/projects/deploy_keys/_form.html.haml @@ -10,13 +10,15 @@ %p.light.append-bottom-0 Paste a machine public key here. Read more about how to generate it = link_to "here", help_page_path("ssh/README") - .form-group - .checkbox - = f.label :can_push do - = f.check_box :can_push - %strong Write access allowed - .form-group - %p.light.append-bottom-0 - Allow this key to push to repository as well? (Default only allows pull access.) + + = f.fields_for :deploy_keys_projects do |deploy_keys_project_form| + .form-group + .checkbox + = deploy_keys_project_form.label :can_push do + = deploy_keys_project_form.check_box :can_push + %strong Write access allowed + .form-group + %p.light.append-bottom-0 + Allow this key to push to repository as well? (Default only allows pull access.) = f.submit "Add key", class: "btn-create btn" diff --git a/app/views/shared/deploy_keys/_form.html.haml b/app/views/shared/deploy_keys/_form.html.haml index e6075c3ae3a..87c2965bb21 100644 --- a/app/views/shared/deploy_keys/_form.html.haml +++ b/app/views/shared/deploy_keys/_form.html.haml @@ -1,5 +1,6 @@ - form = local_assigns.fetch(:form) - deploy_key = local_assigns.fetch(:deploy_key) +- deploy_keys_project = deploy_key.deploy_keys_project_for(@project) = form_errors(deploy_key) @@ -20,11 +21,13 @@ .col-sm-10 = form.text_field :fingerprint, class: 'form-control', readonly: 'readonly' -.form-group - .control-label - .col-sm-10 - = form.label :can_push do - = form.check_box :can_push - %strong Write access allowed - %p.light.append-bottom-0 - Allow this key to push to repository as well? (Default only allows pull access.) +- if deploy_keys_project.present? + = form.fields_for :deploy_keys_projects, deploy_keys_project do |deploy_keys_project_form| + .form-group + .control-label + .col-sm-10 + = deploy_keys_project_form.label :can_push do + = deploy_keys_project_form.check_box :can_push + %strong Write access allowed + %p.light.append-bottom-0 + Allow this key to push to repository as well? (Default only allows pull access.) diff --git a/changelogs/unreleased/sh-migrate-can-push-to-deploy-keys-projects.yml b/changelogs/unreleased/sh-migrate-can-push-to-deploy-keys-projects.yml new file mode 100644 index 00000000000..cb53a94e465 --- /dev/null +++ b/changelogs/unreleased/sh-migrate-can-push-to-deploy-keys-projects.yml @@ -0,0 +1,5 @@ +--- +title: Fix writable shared deploy keys +merge_request: +author: +type: security diff --git a/db/migrate/20171211145425_add_can_push_to_deploy_keys_projects.rb b/db/migrate/20171211145425_add_can_push_to_deploy_keys_projects.rb new file mode 100644 index 00000000000..5dc723db9f9 --- /dev/null +++ b/db/migrate/20171211145425_add_can_push_to_deploy_keys_projects.rb @@ -0,0 +1,15 @@ +class AddCanPushToDeployKeysProjects < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + disable_ddl_transaction! + + def up + add_column_with_default :deploy_keys_projects, :can_push, :boolean, default: false, allow_null: false + end + + def down + remove_column :deploy_keys_projects, :can_push + end +end diff --git a/db/migrate/20171215113714_populate_can_push_from_deploy_keys_projects.rb b/db/migrate/20171215113714_populate_can_push_from_deploy_keys_projects.rb new file mode 100644 index 00000000000..ec0427a8e5a --- /dev/null +++ b/db/migrate/20171215113714_populate_can_push_from_deploy_keys_projects.rb @@ -0,0 +1,44 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class PopulateCanPushFromDeployKeysProjects < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + disable_ddl_transaction! + + class DeploysKeyProject < ActiveRecord::Base + include EachBatch + + self.table_name = 'deploy_keys_projects' + end + + def up + DeploysKeyProject.each_batch(of: 10_000) do |batch| + start_id, end_id = batch.pluck('MIN(id), MAX(id)').first + + execute <<-EOF + UPDATE deploy_keys_projects + SET can_push = keys.can_push + FROM keys + WHERE deploy_key_id = keys.id + AND deploy_keys_projects.id BETWEEN #{start_id} AND #{end_id} + EOF + end + end + + def down + DeploysKeyProject.each_batch(of: 10_000) do |batch| + start_id, end_id = batch.pluck('MIN(id), MAX(id)').first + + execute <<-EOF + UPDATE keys + SET can_push = deploy_keys_projects.can_push + FROM deploy_keys_projects + WHERE deploy_keys_projects.deploy_key_id = keys.id + AND deploy_keys_projects.id BETWEEN #{start_id} AND #{end_id} + EOF + end + end +end diff --git a/db/post_migrate/20171215121205_post_populate_can_push_from_deploy_keys_projects.rb b/db/post_migrate/20171215121205_post_populate_can_push_from_deploy_keys_projects.rb new file mode 100644 index 00000000000..05d236f8e96 --- /dev/null +++ b/db/post_migrate/20171215121205_post_populate_can_push_from_deploy_keys_projects.rb @@ -0,0 +1,43 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class PostPopulateCanPushFromDeployKeysProjects < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + disable_ddl_transaction! + + class DeploysKeyProject < ActiveRecord::Base + include EachBatch + + self.table_name = 'deploy_keys_projects' + end + + def up + DeploysKeyProject.each_batch(of: 10_000) do |batch| + start_id, end_id = batch.pluck('MIN(id), MAX(id)').first + + execute <<-EOF + UPDATE deploy_keys_projects + SET can_push = keys.can_push + FROM keys + WHERE deploy_key_id = keys.id + AND deploy_keys_projects.id BETWEEN #{start_id} AND #{end_id} + EOF + end + end + + def down + DeploysKeyProject.each_batch(of: 10_000) do |batch| + start_id, end_id = batch.pluck('MIN(id), MAX(id)').first + + execute <<-EOF + UPDATE keys + SET can_push = deploy_keys_projects.can_push + FROM deploy_keys_projects + WHERE deploy_keys_projects.deploy_key_id = keys.id + AND deploy_keys_projects.id BETWEEN #{start_id} AND #{end_id} + EOF + end + end +end diff --git a/db/post_migrate/20171215121259_remove_can_push_from_keys.rb b/db/post_migrate/20171215121259_remove_can_push_from_keys.rb new file mode 100644 index 00000000000..0599811d986 --- /dev/null +++ b/db/post_migrate/20171215121259_remove_can_push_from_keys.rb @@ -0,0 +1,17 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class RemoveCanPushFromKeys < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + disable_ddl_transaction! + + def up + remove_column :keys, :can_push + end + + def down + add_column_with_default :keys, :can_push, :boolean, default: false, allow_null: false + end +end diff --git a/db/schema.rb b/db/schema.rb index c0a141885ad..12e6ce36a7c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20171206221519) do +ActiveRecord::Schema.define(version: 20171215121259) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -618,6 +618,7 @@ ActiveRecord::Schema.define(version: 20171206221519) do t.integer "project_id", null: false t.datetime "created_at" t.datetime "updated_at" + t.boolean "can_push", default: false, null: false end add_index "deploy_keys_projects", ["project_id"], name: "index_deploy_keys_projects_on_project_id", using: :btree @@ -892,7 +893,6 @@ ActiveRecord::Schema.define(version: 20171206221519) do t.string "type" t.string "fingerprint" t.boolean "public", default: false, null: false - t.boolean "can_push", default: false, null: false t.datetime "last_used_at" end diff --git a/doc/api/deploy_keys.md b/doc/api/deploy_keys.md index 273d5a56b6f..698fa22a438 100644 --- a/doc/api/deploy_keys.md +++ b/doc/api/deploy_keys.md @@ -19,15 +19,13 @@ Example response: { "id": 1, "title": "Public key", - "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=", - "can_push": false, + "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=", "created_at": "2013-10-02T10:12:29Z" }, { "id": 3, "title": "Another Public key", - "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=", - "can_push": true, + "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=", "created_at": "2013-10-02T11:12:29Z" } ] @@ -57,15 +55,15 @@ Example response: "id": 1, "title": "Public key", "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=", - "can_push": false, - "created_at": "2013-10-02T10:12:29Z" + "created_at": "2013-10-02T10:12:29Z", + "can_push": false }, { "id": 3, "title": "Another Public key", "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=", - "can_push": false, - "created_at": "2013-10-02T11:12:29Z" + "created_at": "2013-10-02T11:12:29Z", + "can_push": false } ] ``` @@ -96,8 +94,8 @@ Example response: "id": 1, "title": "Public key", "key": "ssh-rsa AAAAB3NzaC1yc2EAAAABJQAAAIEAiPWx6WM4lhHNedGfBpPJNPpZ7yKu+dnn1SJejgt4596k6YjzGGphH2TUxwKzxcKDKKezwkpfnxPkSMkuEspGRt/aZZ9wa++Oi7Qkr8prgHc4soW6NUlfDzpvZK2H5E7eQaSeP3SAwGmQKUFHCddNaP0L+hM7zhFNzjFvpaMgJw0=", - "can_push": false, - "created_at": "2013-10-02T10:12:29Z" + "created_at": "2013-10-02T10:12:29Z", + "can_push": false } ``` @@ -135,6 +133,36 @@ Example response: } ``` +## Update deploy key + +Updates a deploy key for a project. + +``` +PUT /projects/:id/deploy_keys/:key_id +``` + +| Attribute | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | +| `title` | string | no | New deploy key's title | +| `can_push` | boolean | no | Can deploy key push to the project's repository | + +```bash +curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" --header "Content-Type: application/json" --data '{"title": "New deploy key", "can_push": true}' "https://gitlab.example.com/api/v4/projects/5/deploy_keys/11" +``` + +Example response: + +```json +{ + "id": 11, + "title": "New deploy key", + "key": "ssh-rsa AAAA...", + "created_at": "2015-08-29T12:44:31.550Z", + "can_push": true +} +``` + ## Delete deploy key Removes a deploy key from the project. If the deploy key is used only for this project, it will be deleted from the system. diff --git a/lib/api/deploy_keys.rb b/lib/api/deploy_keys.rb index 281269b1190..a92f17937b8 100644 --- a/lib/api/deploy_keys.rb +++ b/lib/api/deploy_keys.rb @@ -4,6 +4,16 @@ module API before { authenticate! } + helpers do + def add_deploy_keys_project(project, attrs = {}) + project.deploy_keys_projects.create(attrs) + end + + def find_by_deploy_key(project, key_id) + project.deploy_keys_projects.find_by!(deploy_key: key_id) + end + end + desc 'Return all deploy keys' params do use :pagination @@ -21,28 +31,31 @@ module API before { authorize_admin_project } desc "Get a specific project's deploy keys" do - success Entities::SSHKey + success Entities::DeployKeysProject end params do use :pagination end get ":id/deploy_keys" do - present paginate(user_project.deploy_keys), with: Entities::SSHKey + keys = user_project.deploy_keys_projects.preload(:deploy_key) + + present paginate(keys), with: Entities::DeployKeysProject end desc 'Get single deploy key' do - success Entities::SSHKey + success Entities::DeployKeysProject end params do requires :key_id, type: Integer, desc: 'The ID of the deploy key' end get ":id/deploy_keys/:key_id" do - key = user_project.deploy_keys.find params[:key_id] - present key, with: Entities::SSHKey + key = find_by_deploy_key(user_project, params[:key_id]) + + present key, with: Entities::DeployKeysProject end desc 'Add new deploy key to currently authenticated user' do - success Entities::SSHKey + success Entities::DeployKeysProject end params do requires :key, type: String, desc: 'The new deploy key' @@ -53,24 +66,31 @@ module API params[:key].strip! # Check for an existing key joined to this project - key = user_project.deploy_keys.find_by(key: params[:key]) + key = user_project.deploy_keys_projects + .joins(:deploy_key) + .find_by(keys: { key: params[:key] }) + if key - present key, with: Entities::SSHKey + present key, with: Entities::DeployKeysProject break end # Check for available deploy keys in other projects key = current_user.accessible_deploy_keys.find_by(key: params[:key]) if key - user_project.deploy_keys << key - present key, with: Entities::SSHKey + added_key = add_deploy_keys_project(user_project, deploy_key: key, can_push: !!params[:can_push]) + + present added_key, with: Entities::DeployKeysProject break end # Create a new deploy key - key = DeployKey.new(declared_params(include_missing: false)) - if key.valid? && user_project.deploy_keys << key - present key, with: Entities::SSHKey + key_attributes = { can_push: !!params[:can_push], + deploy_key_attributes: declared_params.except(:can_push) } + key = add_deploy_keys_project(user_project, key_attributes) + + if key.valid? + present key, with: Entities::DeployKeysProject else render_validation_error!(key) end @@ -86,14 +106,20 @@ module API at_least_one_of :title, :can_push end put ":id/deploy_keys/:key_id" do - key = DeployKey.find(params.delete(:key_id)) + deploy_keys_project = find_by_deploy_key(user_project, params[:key_id]) - authorize!(:update_deploy_key, key) + authorize!(:update_deploy_key, deploy_keys_project.deploy_key) - if key.update_attributes(declared_params(include_missing: false)) - present key, with: Entities::SSHKey + can_push = params[:can_push].nil? ? deploy_keys_project.can_push : params[:can_push] + title = params[:title] || deploy_keys_project.deploy_key.title + + result = deploy_keys_project.update_attributes(can_push: can_push, + deploy_key_attributes: { id: params[:key_id], + title: title }) + if result + present deploy_keys_project, with: Entities::DeployKeysProject else - render_validation_error!(key) + render_validation_error!(deploy_keys_project) end end @@ -122,7 +148,7 @@ module API requires :key_id, type: Integer, desc: 'The ID of the deploy key' end delete ":id/deploy_keys/:key_id" do - key = user_project.deploy_keys_projects.find_by(deploy_key_id: params[:key_id]) + key = user_project.deploy_keys.find(params[:key_id]) not_found!('Deploy Key') unless key destroy_conditionally!(key) diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 2031da8e89d..71b3c3b1d40 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -554,13 +554,18 @@ module API end class SSHKey < Grape::Entity - expose :id, :title, :key, :created_at, :can_push + expose :id, :title, :key, :created_at end class SSHKeyWithUser < SSHKey expose :user, using: Entities::UserPublic end + class DeployKeysProject < Grape::Entity + expose :deploy_key, merge: true, using: Entities::SSHKey + expose :can_push + end + class GPGKey < Grape::Entity expose :id, :key, :created_at end diff --git a/lib/api/v3/deploy_keys.rb b/lib/api/v3/deploy_keys.rb index b90e2061da3..47e54ca85a5 100644 --- a/lib/api/v3/deploy_keys.rb +++ b/lib/api/v3/deploy_keys.rb @@ -3,6 +3,16 @@ module API class DeployKeys < Grape::API before { authenticate! } + helpers do + def add_deploy_keys_project(project, attrs = {}) + project.deploy_keys_projects.create(attrs) + end + + def find_by_deploy_key(project, key_id) + project.deploy_keys_projects.find_by!(deploy_key: key_id) + end + end + get "deploy_keys" do authenticated_as_admin! @@ -18,25 +28,28 @@ module API %w(keys deploy_keys).each do |path| desc "Get a specific project's deploy keys" do - success ::API::Entities::SSHKey + success ::API::Entities::DeployKeysProject end get ":id/#{path}" do - present user_project.deploy_keys, with: ::API::Entities::SSHKey + keys = user_project.deploy_keys_projects.preload(:deploy_key) + + present keys, with: ::API::Entities::DeployKeysProject end desc 'Get single deploy key' do - success ::API::Entities::SSHKey + success ::API::Entities::DeployKeysProject end params do requires :key_id, type: Integer, desc: 'The ID of the deploy key' end get ":id/#{path}/:key_id" do - key = user_project.deploy_keys.find params[:key_id] - present key, with: ::API::Entities::SSHKey + key = find_by_deploy_key(user_project, params[:key_id]) + + present key, with: ::API::Entities::DeployKeysProject end desc 'Add new deploy key to currently authenticated user' do - success ::API::Entities::SSHKey + success ::API::Entities::DeployKeysProject end params do requires :key, type: String, desc: 'The new deploy key' @@ -47,24 +60,31 @@ module API params[:key].strip! # Check for an existing key joined to this project - key = user_project.deploy_keys.find_by(key: params[:key]) + key = user_project.deploy_keys_projects + .joins(:deploy_key) + .find_by(keys: { key: params[:key] }) + if key - present key, with: ::API::Entities::SSHKey + present key, with: ::API::Entities::DeployKeysProject break end # Check for available deploy keys in other projects key = current_user.accessible_deploy_keys.find_by(key: params[:key]) if key - user_project.deploy_keys << key - present key, with: ::API::Entities::SSHKey + added_key = add_deploy_keys_project(user_project, deploy_key: key, can_push: !!params[:can_push]) + + present added_key, with: ::API::Entities::DeployKeysProject break end # Create a new deploy key - key = DeployKey.new(declared_params(include_missing: false)) - if key.valid? && user_project.deploy_keys << key - present key, with: ::API::Entities::SSHKey + key_attributes = { can_push: !!params[:can_push], + deploy_key_attributes: declared_params.except(:can_push) } + key = add_deploy_keys_project(user_project, key_attributes) + + if key.valid? + present key, with: ::API::Entities::DeployKeysProject else render_validation_error!(key) end diff --git a/spec/factories/deploy_keys_projects.rb b/spec/factories/deploy_keys_projects.rb index 27cece487bd..c7202714f21 100644 --- a/spec/factories/deploy_keys_projects.rb +++ b/spec/factories/deploy_keys_projects.rb @@ -2,5 +2,9 @@ FactoryGirl.define do factory :deploy_keys_project do deploy_key project + + trait :write_access do + can_push true + end end end diff --git a/spec/factories/keys.rb b/spec/factories/keys.rb index 3f7c794b14a..06055e5b201 100644 --- a/spec/factories/keys.rb +++ b/spec/factories/keys.rb @@ -15,10 +15,6 @@ FactoryGirl.define do factory :another_deploy_key, class: 'DeployKey' end - factory :write_access_key, class: 'DeployKey' do - can_push true - end - factory :rsa_key_2048 do key do 'ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDFf6RYK3qu/RKF/3ndJmL5xgMLp3O9' \ diff --git a/spec/features/admin/admin_deploy_keys_spec.rb b/spec/features/admin/admin_deploy_keys_spec.rb index 241c7cbc34e..cb96830cb7c 100644 --- a/spec/features/admin/admin_deploy_keys_spec.rb +++ b/spec/features/admin/admin_deploy_keys_spec.rb @@ -17,6 +17,16 @@ RSpec.describe 'admin deploy keys' do end end + it 'shows all the projects the deploy key has write access' do + write_key = create(:deploy_keys_project, :write_access, deploy_key: deploy_key) + + visit admin_deploy_keys_path + + page.within(find('.deploy-keys-list', match: :first)) do + expect(page).to have_content(write_key.project.full_name) + end + end + describe 'create a new deploy key' do let(:new_ssh_key) { attributes_for(:key)[:key] } @@ -28,14 +38,12 @@ RSpec.describe 'admin deploy keys' do it 'creates a new deploy key' do fill_in 'deploy_key_title', with: 'laptop' fill_in 'deploy_key_key', with: new_ssh_key - check 'deploy_key_can_push' click_button 'Create' expect(current_path).to eq admin_deploy_keys_path page.within(find('.deploy-keys-list', match: :first)) do expect(page).to have_content('laptop') - expect(page).to have_content('Yes') end end end @@ -48,14 +56,12 @@ RSpec.describe 'admin deploy keys' do it 'updates an existing deploy key' do fill_in 'deploy_key_title', with: 'new-title' - check 'deploy_key_can_push' click_button 'Save changes' expect(current_path).to eq admin_deploy_keys_path page.within(find('.deploy-keys-list', match: :first)) do expect(page).to have_content('new-title') - expect(page).to have_content('Yes') end end end diff --git a/spec/features/projects/settings/repository_settings_spec.rb b/spec/features/projects/settings/repository_settings_spec.rb index e2a5619c22b..7827b24f95b 100644 --- a/spec/features/projects/settings/repository_settings_spec.rb +++ b/spec/features/projects/settings/repository_settings_spec.rb @@ -43,7 +43,7 @@ feature 'Repository settings' do fill_in 'deploy_key_title', with: 'new_deploy_key' fill_in 'deploy_key_key', with: new_ssh_key - check 'deploy_key_can_push' + check 'deploy_key_deploy_keys_projects_attributes_0_can_push' click_button 'Add key' expect(page).to have_content('new_deploy_key') @@ -57,7 +57,7 @@ feature 'Repository settings' do find('li', text: private_deploy_key.title).click_link('Edit') fill_in 'deploy_key_title', with: 'updated_deploy_key' - check 'deploy_key_can_push' + check 'deploy_key_deploy_keys_projects_attributes_0_can_push' click_button 'Save changes' expect(page).to have_content('updated_deploy_key') @@ -74,11 +74,9 @@ feature 'Repository settings' do find('li', text: private_deploy_key.title).click_link('Edit') fill_in 'deploy_key_title', with: 'updated_deploy_key' - check 'deploy_key_can_push' click_button 'Save changes' expect(page).to have_content('updated_deploy_key') - expect(page).to have_content('Write access allowed') end scenario 'remove an existing deploy key' do diff --git a/spec/javascripts/deploy_keys/components/key_spec.js b/spec/javascripts/deploy_keys/components/key_spec.js index 5b64cbb2dfc..c253648007f 100644 --- a/spec/javascripts/deploy_keys/components/key_spec.js +++ b/spec/javascripts/deploy_keys/components/key_spec.js @@ -52,18 +52,24 @@ describe('Deploy keys key', () => { ).toBe('Remove'); }); - it('shows write access text when key has write access', (done) => { - vm.deployKey.can_push = true; + it('shows write access title when key has write access', (done) => { + vm.deployKey.deploy_keys_projects[0].can_push = true; Vue.nextTick(() => { expect( - vm.$el.querySelector('.write-access-allowed'), - ).not.toBeNull(); - - expect( - vm.$el.querySelector('.write-access-allowed').textContent.trim(), + vm.$el.querySelector('.deploy-project-label').getAttribute('data-original-title'), ).toBe('Write access allowed'); + done(); + }); + }); + + it('does not show write access title when key has write access', (done) => { + vm.deployKey.deploy_keys_projects[0].can_push = false; + Vue.nextTick(() => { + expect( + vm.$el.querySelector('.deploy-project-label').getAttribute('data-original-title'), + ).toBe('Read access only'); done(); }); }); diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index a6fbec295b5..cc202ce8bca 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/spec/lib/gitlab/auth_spec.rb @@ -136,8 +136,8 @@ describe Gitlab::Auth do it 'grants deploy key write permissions' do project = create(:project) - key = create(:deploy_key, can_push: true) - create(:deploy_keys_project, deploy_key: key, project: project) + key = create(:deploy_key) + create(:deploy_keys_project, :write_access, deploy_key: key, project: project) token = Gitlab::LfsToken.new(key).token expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: "lfs+deploy-key-#{key.id}") @@ -146,7 +146,7 @@ describe Gitlab::Auth do it 'does not grant deploy key write permissions' do project = create(:project) - key = create(:deploy_key, can_push: true) + key = create(:deploy_key) token = Gitlab::LfsToken.new(key).token expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: "lfs+deploy-key-#{key.id}") diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 2db560c2cec..6a74fd61f35 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -51,12 +51,12 @@ describe Gitlab::GitAccess do context 'when the project exists' do context 'when actor exists' do context 'when actor is a DeployKey' do - let(:deploy_key) { create(:deploy_key, user: user, can_push: true) } + let(:deploy_key) { create(:deploy_key, user: user) } let(:actor) { deploy_key } context 'when the DeployKey has access to the project' do before do - deploy_key.projects << project + deploy_key.deploy_keys_projects.create(project: project, can_push: true) end it 'allows push and pull access' do @@ -696,15 +696,13 @@ describe Gitlab::GitAccess do end describe 'deploy key permissions' do - let(:key) { create(:deploy_key, user: user, can_push: can_push) } + let(:key) { create(:deploy_key, user: user) } let(:actor) { key } context 'when deploy_key can push' do - let(:can_push) { true } - context 'when project is authorized' do before do - key.projects << project + key.deploy_keys_projects.create(project: project, can_push: true) end it { expect { push_access_check }.not_to raise_error } @@ -732,11 +730,9 @@ describe Gitlab::GitAccess do end context 'when deploy_key cannot push' do - let(:can_push) { false } - context 'when project is authorized' do before do - key.projects << project + key.deploy_keys_projects.create(project: project, can_push: false) end it { expect { push_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:deploy_key_upload]) } diff --git a/spec/models/deploy_keys_project_spec.rb b/spec/models/deploy_keys_project_spec.rb index 0345fefb254..fca3090ff4a 100644 --- a/spec/models/deploy_keys_project_spec.rb +++ b/spec/models/deploy_keys_project_spec.rb @@ -8,7 +8,7 @@ describe DeployKeysProject do describe "Validation" do it { is_expected.to validate_presence_of(:project_id) } - it { is_expected.to validate_presence_of(:deploy_key_id) } + it { is_expected.to validate_presence_of(:deploy_key) } end describe "Destroying" do diff --git a/spec/requests/api/deploy_keys_spec.rb b/spec/requests/api/deploy_keys_spec.rb index 1f1e6ea17e4..0772b3f2e64 100644 --- a/spec/requests/api/deploy_keys_spec.rb +++ b/spec/requests/api/deploy_keys_spec.rb @@ -110,7 +110,7 @@ describe API::DeployKeys do end it 'accepts can_push parameter' do - key_attrs = attributes_for :write_access_key + key_attrs = attributes_for(:another_key).merge(can_push: true) post api("/projects/#{project.id}/deploy_keys", admin), key_attrs @@ -160,16 +160,6 @@ describe API::DeployKeys do expect(json_response['title']).to eq('new title') expect(json_response['can_push']).to eq(true) end - - it 'updates a private ssh key from projects user has access with correct attributes' do - create(:deploy_keys_project, project: project2, deploy_key: private_deploy_key) - - put api("/projects/#{project.id}/deploy_keys/#{private_deploy_key.id}", admin), { title: 'new title', can_push: true } - - expect(json_response['id']).to eq(private_deploy_key.id) - expect(json_response['title']).to eq('new title') - expect(json_response['can_push']).to eq(true) - end end describe 'DELETE /projects/:id/deploy_keys/:key_id' do diff --git a/spec/requests/api/v3/deploy_keys_spec.rb b/spec/requests/api/v3/deploy_keys_spec.rb index 785bc1eb4ba..501af587ad4 100644 --- a/spec/requests/api/v3/deploy_keys_spec.rb +++ b/spec/requests/api/v3/deploy_keys_spec.rb @@ -107,7 +107,7 @@ describe API::V3::DeployKeys do end it 'accepts can_push parameter' do - key_attrs = attributes_for :write_access_key + key_attrs = attributes_for(:another_key).merge(can_push: true) post v3_api("/projects/#{project.id}/#{path}", admin), key_attrs diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb index c597623bc4d..35b34f62f1f 100644 --- a/spec/requests/lfs_http_spec.rb +++ b/spec/requests/lfs_http_spec.rb @@ -781,11 +781,11 @@ describe 'Git LFS API and storage' do end context 'when deploy key has project push access' do - let(:key) { create(:deploy_key, can_push: true) } + let(:key) { create(:deploy_key) } let(:authorization) { authorize_deploy_key } let(:update_user_permissions) do - project.deploy_keys << key + project.deploy_keys_projects.create(deploy_key: key, can_push: true) end it_behaves_like 'pushes new LFS objects' diff --git a/spec/serializers/deploy_key_entity_spec.rb b/spec/serializers/deploy_key_entity_spec.rb index d3aefa2c9eb..2bd8162d1b7 100644 --- a/spec/serializers/deploy_key_entity_spec.rb +++ b/spec/serializers/deploy_key_entity_spec.rb @@ -21,18 +21,21 @@ describe DeployKeyEntity do user_id: deploy_key.user_id, title: deploy_key.title, fingerprint: deploy_key.fingerprint, - can_push: deploy_key.can_push, destroyed_when_orphaned: true, almost_orphaned: false, created_at: deploy_key.created_at, updated_at: deploy_key.updated_at, can_edit: false, - projects: [ + deploy_keys_projects: [ { - id: project.id, - name: project.name, - full_path: project_path(project), - full_name: project.full_name + can_push: false, + project: + { + id: project.id, + name: project.name, + full_path: project_path(project), + full_name: project.full_name + } } ] } -- cgit v1.2.1 From a95e10137c3fab082839c4a4f118b4aaffa97288 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Fri, 5 Jan 2018 21:36:18 +0000 Subject: Merge branch '41293-fix-command-injection-vulnerability-on-system_hook_push-queue-through-web-hook' into 'security-10-3' Don't allow line breaks on HTTP headers See merge request gitlab/gitlabhq!2277 (cherry picked from commit 7fc0a6fc096768a5604d6dd24d7d952e53300c82) 073b8f9c Don't allow line breaks on HTTP headers --- app/models/hooks/web_hook.rb | 1 + app/services/web_hook_service.rb | 2 +- lib/gitlab/utils.rb | 4 ++++ spec/lib/gitlab/utils_spec.rb | 16 ++++++++++++++++ spec/models/hooks/web_hook_spec.rb | 6 ++++++ 5 files changed, 28 insertions(+), 1 deletion(-) diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb index 5a70e114f56..27729deeac9 100644 --- a/app/models/hooks/web_hook.rb +++ b/app/models/hooks/web_hook.rb @@ -4,6 +4,7 @@ class WebHook < ActiveRecord::Base has_many :web_hook_logs, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent validates :url, presence: true, url: true + validates :token, format: { without: /\n/ } def execute(data, hook_name) WebHookService.new(self, data, hook_name).execute diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index 6ebc7c89500..36e589d5aa8 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -113,7 +113,7 @@ class WebHookService 'Content-Type' => 'application/json', 'X-Gitlab-Event' => hook_name.singularize.titleize }.tap do |hash| - hash['X-Gitlab-Token'] = hook.token if hook.token.present? + hash['X-Gitlab-Token'] = Gitlab::Utils.remove_line_breaks(hook.token) if hook.token.present? end end end diff --git a/lib/gitlab/utils.rb b/lib/gitlab/utils.rb index b3baaf036d8..fa22f0e37b2 100644 --- a/lib/gitlab/utils.rb +++ b/lib/gitlab/utils.rb @@ -27,6 +27,10 @@ module Gitlab .gsub(/(\A-+|-+\z)/, '') end + def remove_line_breaks(str) + str.gsub(/\r?\n/, '') + end + def to_boolean(value) return value if [true, false].include?(value) return true if value =~ /^(true|t|yes|y|1|on)$/i diff --git a/spec/lib/gitlab/utils_spec.rb b/spec/lib/gitlab/utils_spec.rb index e872a5290c5..bda239b7871 100644 --- a/spec/lib/gitlab/utils_spec.rb +++ b/spec/lib/gitlab/utils_spec.rb @@ -17,6 +17,22 @@ describe Gitlab::Utils do end end + describe '.remove_line_breaks' do + using RSpec::Parameterized::TableSyntax + + where(:original, :expected) do + "foo\nbar\nbaz" | "foobarbaz" + "foo\r\nbar\r\nbaz" | "foobarbaz" + "foobar" | "foobar" + end + + with_them do + it "replace line breaks with an empty string" do + expect(described_class.remove_line_breaks(original)).to eq(expected) + end + end + end + describe '.to_boolean' do it 'accepts booleans' do expect(to_boolean(true)).to be(true) diff --git a/spec/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb index 388120160ab..ea6d6e53ef5 100644 --- a/spec/models/hooks/web_hook_spec.rb +++ b/spec/models/hooks/web_hook_spec.rb @@ -29,6 +29,12 @@ describe WebHook do expect(hook.url).to eq('https://example.com') end end + + describe 'token' do + it { is_expected.to allow_value("foobar").for(:token) } + + it { is_expected.not_to allow_values("foo\nbar", "foo\r\nbar").for(:token) } + end end describe 'execute' do -- cgit v1.2.1 From 056d35cad0e09a59fdf44cb6bd7063f73a970f01 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Mon, 8 Jan 2018 15:42:41 +0000 Subject: Merge branch 'fix/import-rce-10-3' into 'security-10-3' [10.3] Fix RCE via project import mechanism See merge request gitlab/gitlabhq!2294 (cherry picked from commit dcfec507d6f9ee119d65a832393e7c593af1d3b2) 86d75812 Fix RCE via project import mechanism --- changelogs/unreleased/fix-import-rce.yml | 5 ++ lib/gitlab/import_export/file_importer.rb | 6 ++- lib/gitlab/import_export/saver.rb | 2 +- lib/gitlab/import_export/shared.rb | 14 +++++- .../lib/gitlab/import_export/file_importer_spec.rb | 57 +++++++++++++++++----- 5 files changed, 68 insertions(+), 16 deletions(-) create mode 100644 changelogs/unreleased/fix-import-rce.yml diff --git a/changelogs/unreleased/fix-import-rce.yml b/changelogs/unreleased/fix-import-rce.yml new file mode 100644 index 00000000000..c47e45fac31 --- /dev/null +++ b/changelogs/unreleased/fix-import-rce.yml @@ -0,0 +1,5 @@ +--- +title: Fix RCE via project import mechanism +merge_request: +author: +type: security diff --git a/lib/gitlab/import_export/file_importer.rb b/lib/gitlab/import_export/file_importer.rb index 989342389bc..5c971564a73 100644 --- a/lib/gitlab/import_export/file_importer.rb +++ b/lib/gitlab/import_export/file_importer.rb @@ -17,12 +17,16 @@ module Gitlab def import mkdir_p(@shared.export_path) + remove_symlinks! + wait_for_archived_file do decompress_archive end rescue => e @shared.error(e) false + ensure + remove_symlinks! end private @@ -43,7 +47,7 @@ module Gitlab raise Projects::ImportService::Error.new("Unable to decompress #{@archive_file} into #{@shared.export_path}") unless result - remove_symlinks! + result end def remove_symlinks! diff --git a/lib/gitlab/import_export/saver.rb b/lib/gitlab/import_export/saver.rb index 6130c124dd1..2daeba90a51 100644 --- a/lib/gitlab/import_export/saver.rb +++ b/lib/gitlab/import_export/saver.rb @@ -37,7 +37,7 @@ module Gitlab end def archive_file - @archive_file ||= File.join(@shared.export_path, '..', Gitlab::ImportExport.export_filename(project: @project)) + @archive_file ||= File.join(@shared.archive_path, Gitlab::ImportExport.export_filename(project: @project)) end end end diff --git a/lib/gitlab/import_export/shared.rb b/lib/gitlab/import_export/shared.rb index 9fd0b709ef2..d03cbc880fd 100644 --- a/lib/gitlab/import_export/shared.rb +++ b/lib/gitlab/import_export/shared.rb @@ -9,7 +9,11 @@ module Gitlab end def export_path - @export_path ||= Gitlab::ImportExport.export_path(relative_path: opts[:relative_path]) + @export_path ||= Gitlab::ImportExport.export_path(relative_path: relative_path) + end + + def archive_path + @archive_path ||= Gitlab::ImportExport.export_path(relative_path: relative_archive_path) end def error(error) @@ -21,6 +25,14 @@ module Gitlab private + def relative_path + File.join(opts[:relative_path], SecureRandom.hex) + end + + def relative_archive_path + File.join(opts[:relative_path], '..') + end + def error_out(message, caller) Rails.logger.error("Import/Export error raised on #{caller}: #{message}") end diff --git a/spec/lib/gitlab/import_export/file_importer_spec.rb b/spec/lib/gitlab/import_export/file_importer_spec.rb index 162b776e107..5cdc5138fda 100644 --- a/spec/lib/gitlab/import_export/file_importer_spec.rb +++ b/spec/lib/gitlab/import_export/file_importer_spec.rb @@ -12,30 +12,61 @@ describe Gitlab::ImportExport::FileImporter do stub_const('Gitlab::ImportExport::FileImporter::MAX_RETRIES', 0) allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path) allow_any_instance_of(Gitlab::ImportExport::CommandLineUtil).to receive(:untar_zxf).and_return(true) - + allow(SecureRandom).to receive(:hex).and_return('abcd') setup_files - - described_class.import(archive_file: '', shared: shared) end after do FileUtils.rm_rf(export_path) end - it 'removes symlinks in root folder' do - expect(File.exist?(symlink_file)).to be false - end + context 'normal run' do + before do + described_class.import(archive_file: '', shared: shared) + end - it 'removes hidden symlinks in root folder' do - expect(File.exist?(hidden_symlink_file)).to be false - end + it 'removes symlinks in root folder' do + expect(File.exist?(symlink_file)).to be false + end + + it 'removes hidden symlinks in root folder' do + expect(File.exist?(hidden_symlink_file)).to be false + end + + it 'removes symlinks in subfolders' do + expect(File.exist?(subfolder_symlink_file)).to be false + end - it 'removes symlinks in subfolders' do - expect(File.exist?(subfolder_symlink_file)).to be false + it 'does not remove a valid file' do + expect(File.exist?(valid_file)).to be true + end + + it 'creates the file in the right subfolder' do + expect(shared.export_path).to include('test/abcd') + end end - it 'does not remove a valid file' do - expect(File.exist?(valid_file)).to be true + context 'error' do + before do + allow_any_instance_of(described_class).to receive(:wait_for_archived_file).and_raise(StandardError) + described_class.import(archive_file: '', shared: shared) + end + + it 'removes symlinks in root folder' do + expect(File.exist?(symlink_file)).to be false + end + + it 'removes hidden symlinks in root folder' do + expect(File.exist?(hidden_symlink_file)).to be false + end + + it 'removes symlinks in subfolders' do + expect(File.exist?(subfolder_symlink_file)).to be false + end + + it 'does not remove a valid file' do + expect(File.exist?(valid_file)).to be true + end end def setup_files -- cgit v1.2.1 From 9c53c64e1e54ac8a18366a9f785b8a239c0e4357 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 9 Jan 2018 08:39:22 +0000 Subject: Merge branch 'fl-ipythin-10-3' into 'security-10-3' Port of [10.2] Sanitizes IPython notebook output See merge request gitlab/gitlabhq!2285 (cherry picked from commit 1c46e031c70706450a8e0ae730f4c323b72f9e4c) aac035fe Port of [10.2] Sanitizes IPython notebook output --- app/assets/javascripts/notebook/cells/markdown.vue | 8 ++- .../javascripts/notebook/cells/output/html.vue | 15 ++++- package.json | 1 + spec/javascripts/notebook/cells/markdown_spec.js | 12 ++++ .../notebook/cells/output/html_sanitize_tests.js | 66 ++++++++++++++++++++++ .../javascripts/notebook/cells/output/html_spec.js | 29 ++++++++++ yarn.lock | 49 +++++++++++++++- 7 files changed, 176 insertions(+), 4 deletions(-) create mode 100644 spec/javascripts/notebook/cells/output/html_sanitize_tests.js create mode 100644 spec/javascripts/notebook/cells/output/html_spec.js diff --git a/app/assets/javascripts/notebook/cells/markdown.vue b/app/assets/javascripts/notebook/cells/markdown.vue index 82c51a1068c..721753af595 100644 --- a/app/assets/javascripts/notebook/cells/markdown.vue +++ b/app/assets/javascripts/notebook/cells/markdown.vue @@ -1,6 +1,7 @@ diff --git a/package.json b/package.json index c0a4db122bd..d53e50da88f 100644 --- a/package.json +++ b/package.json @@ -59,6 +59,7 @@ "raven-js": "^3.14.0", "raw-loader": "^0.5.1", "react-dev-utils": "^0.5.2", + "sanitize-html": "^1.16.1", "select2": "3.5.2-browserify", "sql.js": "^0.4.0", "svg4everybody": "2.1.9", diff --git a/spec/javascripts/notebook/cells/markdown_spec.js b/spec/javascripts/notebook/cells/markdown_spec.js index a88e9ed3d99..db2a16b0b68 100644 --- a/spec/javascripts/notebook/cells/markdown_spec.js +++ b/spec/javascripts/notebook/cells/markdown_spec.js @@ -42,6 +42,18 @@ describe('Markdown component', () => { expect(vm.$el.querySelector('.markdown h1')).not.toBeNull(); }); + it('sanitizes output', (done) => { + Object.assign(cell, { + source: ['[XSS](data:text/html;base64,PHNjcmlwdD5hbGVydChkb2N1bWVudC5kb21haW4pPC9zY3JpcHQ+Cg==)\n'], + }); + + Vue.nextTick(() => { + expect(vm.$el.querySelector('a').getAttribute('href')).toBeNull(); + + done(); + }); + }); + describe('katex', () => { beforeEach(() => { json = getJSONFixture('blob/notebook/math.json'); diff --git a/spec/javascripts/notebook/cells/output/html_sanitize_tests.js b/spec/javascripts/notebook/cells/output/html_sanitize_tests.js new file mode 100644 index 00000000000..d587573fc9e --- /dev/null +++ b/spec/javascripts/notebook/cells/output/html_sanitize_tests.js @@ -0,0 +1,66 @@ +export default { + 'protocol-based JS injection: simple, no spaces': { + input: 'foo', + output: 'foo', + }, + 'protocol-based JS injection: simple, spaces before': { + input: 'foo', + output: 'foo', + }, + 'protocol-based JS injection: simple, spaces after': { + input: 'foo', + output: 'foo', + }, + 'protocol-based JS injection: simple, spaces before and after': { + input: 'foo', + output: 'foo', + }, + 'protocol-based JS injection: preceding colon': { + input: 'foo', + output: 'foo', + }, + 'protocol-based JS injection: UTF-8 encoding': { + input: 'foo', + output: 'foo', + }, + 'protocol-based JS injection: long UTF-8 encoding': { + input: 'foo', + output: 'foo', + }, + 'protocol-based JS injection: long UTF-8 encoding without semicolons': { + input: 'foo', + output: 'foo', + }, + 'protocol-based JS injection: hex encoding': { + input: 'foo', + output: 'foo', + }, + 'protocol-based JS injection: long hex encoding': { + input: 'foo', + output: 'foo', + }, + 'protocol-based JS injection: hex encoding without semicolons': { + input: 'foo', + output: 'foo', + }, + 'protocol-based JS injection: null char': { + input: 'foo', + output: 'foo', + }, + 'protocol-based JS injection: invalid URL char': { + input: '', // eslint-disable-line no-useless-escape + output: '', + }, + 'protocol-based JS injection: Unicode': { + input: 'foo', + output: 'foo', + }, + 'protocol-based JS injection: spaces and entities': { + input: 'foo', + output: 'foo', + }, + 'img on error': { + input: '', + output: '', + }, +}; diff --git a/spec/javascripts/notebook/cells/output/html_spec.js b/spec/javascripts/notebook/cells/output/html_spec.js new file mode 100644 index 00000000000..9c5385f2922 --- /dev/null +++ b/spec/javascripts/notebook/cells/output/html_spec.js @@ -0,0 +1,29 @@ +import Vue from 'vue'; +import htmlOutput from '~/notebook/cells/output/html.vue'; +import sanitizeTests from './html_sanitize_tests'; + +describe('html output cell', () => { + function createComponent(rawCode) { + const Component = Vue.extend(htmlOutput); + + return new Component({ + propsData: { + rawCode, + }, + }).$mount(); + } + + describe('sanitizes output', () => { + Object.keys(sanitizeTests).forEach((key) => { + it(key, () => { + const test = sanitizeTests[key]; + const vm = createComponent(test.input); + const outputEl = [...vm.$el.querySelectorAll('div')].pop(); + + expect(outputEl.innerHTML).toEqual(test.output); + + vm.$destroy(); + }); + }); + }); +}); diff --git a/yarn.lock b/yarn.lock index 69e8f8a0647..cf0eaae2f4a 100644 --- a/yarn.lock +++ b/yarn.lock @@ -242,7 +242,7 @@ array-union@^1.0.1: dependencies: array-uniq "^1.0.1" -array-uniq@^1.0.1: +array-uniq@^1.0.1, array-uniq@^1.0.2: version "1.0.3" resolved "https://registry.yarnpkg.com/array-uniq/-/array-uniq-1.0.3.tgz#af6ac877a25cc7f74e058894753858dfdb24fdb6" @@ -3084,7 +3084,7 @@ html-entities@1.2.0, html-entities@^1.2.0: version "1.2.0" resolved "https://registry.yarnpkg.com/html-entities/-/html-entities-1.2.0.tgz#41948caf85ce82fed36e4e6a0ed371a6664379e2" -htmlparser2@^3.8.2: +htmlparser2@^3.8.2, htmlparser2@^3.9.0: version "3.9.2" resolved "https://registry.yarnpkg.com/htmlparser2/-/htmlparser2-3.9.2.tgz#1bdf87acca0f3f9e53fa4fcceb0f4b4cbb00b338" dependencies: @@ -3940,6 +3940,10 @@ lodash.capitalize@^4.0.0: version "4.2.1" resolved "https://registry.yarnpkg.com/lodash.capitalize/-/lodash.capitalize-4.2.1.tgz#f826c9b4e2a8511d84e3aca29db05e1a4f3b72a9" +lodash.clonedeep@^4.5.0: + version "4.5.0" + resolved "https://registry.yarnpkg.com/lodash.clonedeep/-/lodash.clonedeep-4.5.0.tgz#e23f3f9c4f8fbdde872529c1071857a086e5ccef" + lodash.cond@^4.3.0: version "4.5.2" resolved "https://registry.yarnpkg.com/lodash.cond/-/lodash.cond-4.5.2.tgz#f471a1da486be60f6ab955d17115523dd1d255d5" @@ -3955,6 +3959,10 @@ lodash.defaults@^3.1.2: lodash.assign "^3.0.0" lodash.restparam "^3.0.0" +lodash.escaperegexp@^4.1.2: + version "4.1.2" + resolved "https://registry.yarnpkg.com/lodash.escaperegexp/-/lodash.escaperegexp-4.1.2.tgz#64762c48618082518ac3df4ccf5d5886dae20347" + lodash.get@^3.7.0: version "3.7.0" resolved "https://registry.yarnpkg.com/lodash.get/-/lodash.get-3.7.0.tgz#3ce68ae2c91683b281cc5394128303cbf75e691f" @@ -3989,6 +3997,10 @@ lodash.memoize@^4.1.2: version "4.1.2" resolved "https://registry.yarnpkg.com/lodash.memoize/-/lodash.memoize-4.1.2.tgz#bcc6c49a42a2840ed997f323eada5ecd182e0bfe" +lodash.mergewith@^4.6.0: + version "4.6.0" + resolved "https://registry.yarnpkg.com/lodash.mergewith/-/lodash.mergewith-4.6.0.tgz#150cf0a16791f5903b8891eab154609274bdea55" + lodash.restparam@^3.0.0: version "3.6.1" resolved "https://registry.yarnpkg.com/lodash.restparam/-/lodash.restparam-3.6.1.tgz#936a4e309ef330a7645ed4145986c85ae5b20805" @@ -5041,6 +5053,14 @@ postcss@^5.0.10, postcss@^5.0.11, postcss@^5.0.12, postcss@^5.0.13, postcss@^5.0 source-map "^0.5.6" supports-color "^3.2.3" +postcss@^6.0.14: + version "6.0.15" + resolved "https://registry.yarnpkg.com/postcss/-/postcss-6.0.15.tgz#f460cd6269fede0d1bf6defff0b934a9845d974d" + dependencies: + chalk "^2.3.0" + source-map "^0.6.1" + supports-color "^5.1.0" + postcss@^6.0.8: version "6.0.14" resolved "https://registry.yarnpkg.com/postcss/-/postcss-6.0.14.tgz#5534c72114739e75d0afcf017db853099f562885" @@ -5547,6 +5567,18 @@ safe-buffer@^5.0.1, safe-buffer@~5.0.1: version "5.0.1" resolved "https://registry.yarnpkg.com/safe-buffer/-/safe-buffer-5.0.1.tgz#d263ca54696cd8a306b5ca6551e92de57918fbe7" +sanitize-html@^1.16.1: + version "1.16.3" + resolved "https://registry.yarnpkg.com/sanitize-html/-/sanitize-html-1.16.3.tgz#96c1b44a36ff7312e1c22a14b05274370ac8bd56" + dependencies: + htmlparser2 "^3.9.0" + lodash.clonedeep "^4.5.0" + lodash.escaperegexp "^4.1.2" + lodash.mergewith "^4.6.0" + postcss "^6.0.14" + srcset "^1.0.0" + xtend "^4.0.0" + sax@~1.2.1: version "1.2.2" resolved "https://registry.yarnpkg.com/sax/-/sax-1.2.2.tgz#fd8631a23bc7826bef5d871bdb87378c95647828" @@ -5860,6 +5892,13 @@ sql.js@^0.4.0: version "0.4.0" resolved "https://registry.yarnpkg.com/sql.js/-/sql.js-0.4.0.tgz#23be9635520eb0ff43a741e7e830397266e88445" +srcset@^1.0.0: + version "1.0.0" + resolved "https://registry.yarnpkg.com/srcset/-/srcset-1.0.0.tgz#a5669de12b42f3b1d5e83ed03c71046fc48f41ef" + dependencies: + array-uniq "^1.0.2" + number-is-nan "^1.0.0" + sshpk@^1.7.0: version "1.13.1" resolved "https://registry.yarnpkg.com/sshpk/-/sshpk-1.13.1.tgz#512df6da6287144316dc4c18fe1cf1d940739be3" @@ -6004,6 +6043,12 @@ supports-color@^4.2.1: dependencies: has-flag "^2.0.0" +supports-color@^5.1.0: + version "5.1.0" + resolved "https://registry.yarnpkg.com/supports-color/-/supports-color-5.1.0.tgz#058a021d1b619f7ddf3980d712ea3590ce7de3d5" + dependencies: + has-flag "^2.0.0" + svg4everybody@2.1.9: version "2.1.9" resolved "https://registry.yarnpkg.com/svg4everybody/-/svg4everybody-2.1.9.tgz#5bd9f6defc133859a044646d4743fabc28db7e2d" -- cgit v1.2.1 From d2536bf5a683098f4077cf644e8344cc7ea8e13a Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 9 Jan 2018 16:47:31 +0000 Subject: Merge branch 'jej/fix-disabled-oauth-access-10-3' into 'security-10-3' [10.3] Prevent login with disabled OAuth providers See merge request gitlab/gitlabhq!2296 (cherry picked from commit 4936650427ffc88e6ee927aedbb2c724d24b094c) a0f9d222 Prevents login with disabled OAuth providers --- app/controllers/omniauth_callbacks_controller.rb | 9 +++ .../unreleased/jej-fix-disabled-oauth-access.yml | 5 ++ lib/gitlab/o_auth.rb | 6 ++ lib/gitlab/o_auth/user.rb | 11 ++-- .../omniauth_callbacks_controller_spec.rb | 75 ++++++++++++++++++++++ spec/features/oauth_login_spec.rb | 3 +- spec/support/devise_helpers.rb | 15 +++-- spec/support/login_helpers.rb | 7 ++ 8 files changed, 118 insertions(+), 13 deletions(-) create mode 100644 changelogs/unreleased/jej-fix-disabled-oauth-access.yml create mode 100644 lib/gitlab/o_auth.rb create mode 100644 spec/controllers/omniauth_callbacks_controller_spec.rb diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index e3c18cba1dd..54d8566cc77 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -110,6 +110,8 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController continue_login_process end + rescue Gitlab::OAuth::SigninDisabledForProviderError + handle_disabled_provider rescue Gitlab::OAuth::SignupDisabledError handle_signup_error end @@ -165,6 +167,13 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController redirect_to new_user_session_path end + def handle_disabled_provider + label = Gitlab::OAuth::Provider.label_for(oauth['provider']) + flash[:alert] = "Signing in using #{label} has been disabled" + + redirect_to new_user_session_path + end + def log_audit_event(user, options = {}) AuditEventService.new(user, user, options) .for_authentication.security_event diff --git a/changelogs/unreleased/jej-fix-disabled-oauth-access.yml b/changelogs/unreleased/jej-fix-disabled-oauth-access.yml new file mode 100644 index 00000000000..3a92c432dc6 --- /dev/null +++ b/changelogs/unreleased/jej-fix-disabled-oauth-access.yml @@ -0,0 +1,5 @@ +--- +title: Prevent OAuth login POST requests when a provider has been disabled +merge_request: +author: +type: security diff --git a/lib/gitlab/o_auth.rb b/lib/gitlab/o_auth.rb new file mode 100644 index 00000000000..5ad8d83bd6e --- /dev/null +++ b/lib/gitlab/o_auth.rb @@ -0,0 +1,6 @@ +module Gitlab + module OAuth + SignupDisabledError = Class.new(StandardError) + SigninDisabledForProviderError = Class.new(StandardError) + end +end diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb index d33f33d192f..fff9360ea27 100644 --- a/lib/gitlab/o_auth/user.rb +++ b/lib/gitlab/o_auth/user.rb @@ -5,8 +5,6 @@ # module Gitlab module OAuth - SignupDisabledError = Class.new(StandardError) - class User attr_accessor :auth_hash, :gl_user @@ -29,7 +27,8 @@ module Gitlab end def save(provider = 'OAuth') - unauthorized_to_create unless gl_user + raise SigninDisabledForProviderError if oauth_provider_disabled? + raise SignupDisabledError unless gl_user block_after_save = needs_blocking? @@ -226,8 +225,10 @@ module Gitlab Gitlab::AppLogger end - def unauthorized_to_create - raise SignupDisabledError + def oauth_provider_disabled? + Gitlab::CurrentSettings.current_application_settings + .disabled_oauth_sign_in_sources + .include?(auth_hash.provider) end end end diff --git a/spec/controllers/omniauth_callbacks_controller_spec.rb b/spec/controllers/omniauth_callbacks_controller_spec.rb new file mode 100644 index 00000000000..c639ad32ec6 --- /dev/null +++ b/spec/controllers/omniauth_callbacks_controller_spec.rb @@ -0,0 +1,75 @@ +require 'spec_helper' + +describe OmniauthCallbacksController do + include LoginHelpers + + let(:user) { create(:omniauth_user, extern_uid: 'my-uid', provider: provider) } + let(:provider) { :github } + + before do + mock_auth_hash(provider.to_s, 'my-uid', user.email) + stub_omniauth_provider(provider, context: request) + end + + it 'allows sign in' do + post provider + + expect(request.env['warden']).to be_authenticated + end + + shared_context 'sign_up' do + let(:user) { double(email: 'new@example.com') } + + before do + stub_omniauth_setting(block_auto_created_users: false) + end + end + + context 'sign up' do + include_context 'sign_up' + + it 'is allowed' do + post provider + + expect(request.env['warden']).to be_authenticated + end + end + + context 'when OAuth is disabled' do + before do + stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') + settings = Gitlab::CurrentSettings.current_application_settings + settings.update(disabled_oauth_sign_in_sources: [provider.to_s]) + end + + it 'prevents login via POST' do + post provider + + expect(request.env['warden']).not_to be_authenticated + end + + it 'shows warning when attempting login' do + post provider + + expect(response).to redirect_to new_user_session_path + expect(flash[:alert]).to eq('Signing in using GitHub has been disabled') + end + + it 'allows linking the disabled provider' do + user.identities.destroy_all + sign_in(user) + + expect { post provider }.to change { user.reload.identities.count }.by(1) + end + + context 'sign up' do + include_context 'sign_up' + + it 'is prevented' do + post provider + + expect(request.env['warden']).not_to be_authenticated + end + end + end +end diff --git a/spec/features/oauth_login_spec.rb b/spec/features/oauth_login_spec.rb index 49d8e52f861..a5e325ee2e3 100644 --- a/spec/features/oauth_login_spec.rb +++ b/spec/features/oauth_login_spec.rb @@ -10,8 +10,7 @@ feature 'OAuth Login', :js, :allow_forgery_protection do def stub_omniauth_config(provider) OmniAuth.config.add_mock(provider, OmniAuth::AuthHash.new(provider: provider.to_s, uid: "12345")) - set_devise_mapping(context: Rails.application) - Rails.application.env_config['omniauth.auth'] = OmniAuth.config.mock_auth[provider] + stub_omniauth_provider(provider) end providers = [:github, :twitter, :bitbucket, :gitlab, :google_oauth2, diff --git a/spec/support/devise_helpers.rb b/spec/support/devise_helpers.rb index 890a2d9d287..66874e10f38 100644 --- a/spec/support/devise_helpers.rb +++ b/spec/support/devise_helpers.rb @@ -2,13 +2,16 @@ module DeviseHelpers # explicitly tells Devise which mapping to use # this is needed when we are testing a Devise controller bypassing the router def set_devise_mapping(context:) - env = - if context.respond_to?(:env_config) - context.env_config - elsif context.respond_to?(:env) - context.env - end + env = env_from_context(context) env['devise.mapping'] = Devise.mappings[:user] if env end + + def env_from_context(context) + if context.respond_to?(:env_config) + context.env_config + elsif context.respond_to?(:env) + context.env + end + end end diff --git a/spec/support/login_helpers.rb b/spec/support/login_helpers.rb index 50702a0ac88..b52b6a28c54 100644 --- a/spec/support/login_helpers.rb +++ b/spec/support/login_helpers.rb @@ -125,6 +125,13 @@ module LoginHelpers }) end + def stub_omniauth_provider(provider, context: Rails.application) + env = env_from_context(context) + + set_devise_mapping(context: context) + env['omniauth.auth'] = OmniAuth.config.mock_auth[provider] + end + def stub_omniauth_saml_config(messages) set_devise_mapping(context: Rails.application) Rails.application.routes.disable_clear_and_finalize = true -- cgit v1.2.1