From c93ce836930a875452432ccc0c92733fb8adda29 Mon Sep 17 00:00:00 2001 From: manojmj Date: Thu, 27 Jun 2019 14:44:01 +0530 Subject: Do not allow localhost url redirection in GitHub Integration --- .../unreleased/security-github-ssrf-redirect.yml | 5 ++ config/initializers/octokit.rb | 1 + lib/gitlab/github_import/client.rb | 4 +- lib/gitlab/legacy_github_import/client.rb | 2 +- lib/gitlab/octokit/middleware.rb | 23 ++++++++ spec/lib/gitlab/octokit/middleware_spec.rb | 68 ++++++++++++++++++++++ 6 files changed, 100 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/security-github-ssrf-redirect.yml create mode 100644 config/initializers/octokit.rb create mode 100644 lib/gitlab/octokit/middleware.rb create mode 100644 spec/lib/gitlab/octokit/middleware_spec.rb diff --git a/changelogs/unreleased/security-github-ssrf-redirect.yml b/changelogs/unreleased/security-github-ssrf-redirect.yml new file mode 100644 index 00000000000..36a36de3eb0 --- /dev/null +++ b/changelogs/unreleased/security-github-ssrf-redirect.yml @@ -0,0 +1,5 @@ +--- +title: Do not allow localhost url redirection in GitHub Integration +merge_request: +author: +type: security diff --git a/config/initializers/octokit.rb b/config/initializers/octokit.rb new file mode 100644 index 00000000000..b3749258ec5 --- /dev/null +++ b/config/initializers/octokit.rb @@ -0,0 +1 @@ +Octokit.middleware.insert_after Octokit::Middleware::FollowRedirects, Gitlab::Octokit::Middleware diff --git a/lib/gitlab/github_import/client.rb b/lib/gitlab/github_import/client.rb index a61beafae0d..826b35d685c 100644 --- a/lib/gitlab/github_import/client.rb +++ b/lib/gitlab/github_import/client.rb @@ -40,7 +40,7 @@ module Gitlab # otherwise hitting the rate limit will result in a thread # being blocked in a `sleep()` call for up to an hour. def initialize(token, per_page: 100, parallel: true) - @octokit = Octokit::Client.new( + @octokit = ::Octokit::Client.new( access_token: token, per_page: per_page, api_endpoint: api_endpoint @@ -139,7 +139,7 @@ module Gitlab begin yield - rescue Octokit::TooManyRequests + rescue ::Octokit::TooManyRequests raise_or_wait_for_rate_limit # This retry will only happen when running in sequential mode as we'll diff --git a/lib/gitlab/legacy_github_import/client.rb b/lib/gitlab/legacy_github_import/client.rb index bbdd094e33b..b23efd64dee 100644 --- a/lib/gitlab/legacy_github_import/client.rb +++ b/lib/gitlab/legacy_github_import/client.rb @@ -101,7 +101,7 @@ module Gitlab # GitHub Rate Limit API returns 404 when the rate limit is # disabled. In this case we just want to return gracefully # instead of spitting out an error. - rescue Octokit::NotFound + rescue ::Octokit::NotFound nil end diff --git a/lib/gitlab/octokit/middleware.rb b/lib/gitlab/octokit/middleware.rb new file mode 100644 index 00000000000..2f762957d1b --- /dev/null +++ b/lib/gitlab/octokit/middleware.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Gitlab + module Octokit + class Middleware + def initialize(app) + @app = app + end + + def call(env) + Gitlab::UrlBlocker.validate!(env[:url], { allow_localhost: allow_local_requests?, allow_local_network: allow_local_requests? }) + + @app.call(env) + end + + private + + def allow_local_requests? + Gitlab::CurrentSettings.allow_local_requests_from_hooks_and_services? + end + end + end +end diff --git a/spec/lib/gitlab/octokit/middleware_spec.rb b/spec/lib/gitlab/octokit/middleware_spec.rb new file mode 100644 index 00000000000..7f2b523f5b7 --- /dev/null +++ b/spec/lib/gitlab/octokit/middleware_spec.rb @@ -0,0 +1,68 @@ +require 'spec_helper' + +describe Gitlab::Octokit::Middleware do + let(:app) { double(:app) } + let(:middleware) { described_class.new(app) } + + shared_examples 'Public URL' do + it 'does not raise an error' do + expect(app).to receive(:call).with(env) + + expect { middleware.call(env) }.not_to raise_error + end + end + + shared_examples 'Local URL' do + it 'raises an error' do + expect { middleware.call(env) }.to raise_error(Gitlab::UrlBlocker::BlockedUrlError) + end + end + + describe '#call' do + context 'when the URL is a public URL' do + let(:env) { { url: 'https://public-url.com' } } + + it_behaves_like 'Public URL' + end + + context 'when the URL is a localhost adresss' do + let(:env) { { url: 'http://127.0.0.1' } } + + context 'when localhost requests are not allowed' do + before do + stub_application_setting(allow_local_requests_from_hooks_and_services: false) + end + + it_behaves_like 'Local URL' + end + + context 'when localhost requests are allowed' do + before do + stub_application_setting(allow_local_requests_from_hooks_and_services: true) + end + + it_behaves_like 'Public URL' + end + end + + context 'when the URL is a local network address' do + let(:env) { { url: 'http://172.16.0.0' } } + + context 'when local network requests are not allowed' do + before do + stub_application_setting(allow_local_requests_from_hooks_and_services: false) + end + + it_behaves_like 'Local URL' + end + + context 'when local network requests are allowed' do + before do + stub_application_setting(allow_local_requests_from_hooks_and_services: true) + end + + it_behaves_like 'Public URL' + end + end + end +end -- cgit v1.2.3 From 019caa8de59f0ca701d4f099a4068605b17e3b93 Mon Sep 17 00:00:00 2001 From: drew cimino Date: Fri, 28 Jun 2019 10:40:34 -0400 Subject: Use MergeRequest#source_project as permissions reference for MergeRequest#all_pipelines MergeRequest#all_pipelines fetches Ci::Pipeline records from the source project, so we should specifically check that project for permissions. This was already happening for intra-project merge requests, but in the event that the target and source projects both have private builds, we should ensure that the project permissions are respected. --- .../merge_requests/application_controller.rb | 2 +- .../projects/merge_requests_controller.rb | 3 +- .../security-mr-pipeline-permissions.yml | 5 ++ .../projects/merge_requests_controller_spec.rb | 98 +++++++++++++++++++++- 4 files changed, 102 insertions(+), 6 deletions(-) create mode 100644 changelogs/unreleased/security-mr-pipeline-permissions.yml diff --git a/app/controllers/projects/merge_requests/application_controller.rb b/app/controllers/projects/merge_requests/application_controller.rb index dcc272aecff..006731c0e66 100644 --- a/app/controllers/projects/merge_requests/application_controller.rb +++ b/app/controllers/projects/merge_requests/application_controller.rb @@ -45,7 +45,7 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont def set_pipeline_variables @pipelines = - if can?(current_user, :read_pipeline, @project) + if can?(current_user, :read_pipeline, @merge_request.source_project) @merge_request.all_pipelines else Ci::Pipeline.none diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 7ee8e0ea8f8..7f87fc3792a 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -82,7 +82,8 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo end def pipelines - @pipelines = @merge_request.all_pipelines.page(params[:page]).per(30) + set_pipeline_variables + @pipelines = @pipelines.page(params[:page]).per(30) Gitlab::PollingInterval.set_header(response, interval: 10_000) diff --git a/changelogs/unreleased/security-mr-pipeline-permissions.yml b/changelogs/unreleased/security-mr-pipeline-permissions.yml new file mode 100644 index 00000000000..a317c93228c --- /dev/null +++ b/changelogs/unreleased/security-mr-pipeline-permissions.yml @@ -0,0 +1,5 @@ +--- +title: Use source project as permissions reference for MergeRequestsController#pipelines +merge_request: +author: +type: security diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 0eca663a683..64a89205e2f 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -623,10 +623,100 @@ describe Projects::MergeRequestsController do format: :json end - it 'responds with serialized pipelines' do - expect(json_response['pipelines']).not_to be_empty - expect(json_response['count']['all']).to eq 1 - expect(response).to include_pagination_headers + context 'with "enabled" builds on a public project' do + let(:project) { create(:project, :repository, :public) } + + context 'for a project owner' do + it 'responds with serialized pipelines' do + expect(json_response['pipelines']).to be_present + expect(json_response['count']['all']).to eq(1) + expect(response).to include_pagination_headers + end + end + + context 'for an unassociated user' do + let(:user) { create :user } + + it 'responds with no pipelines' do + expect(json_response['pipelines']).to be_present + expect(json_response['count']['all']).to eq(1) + expect(response).to include_pagination_headers + end + end + end + + context 'with private builds on a public project' do + let(:project) { create(:project, :repository, :public, :builds_private) } + + context 'for a project owner' do + it 'responds with serialized pipelines' do + expect(json_response['pipelines']).to be_present + expect(json_response['count']['all']).to eq(1) + expect(response).to include_pagination_headers + end + end + + context 'for an unassociated user' do + let(:user) { create :user } + + it 'responds with no pipelines' do + expect(json_response['pipelines']).to be_empty + expect(json_response['count']['all']).to eq(0) + expect(response).to include_pagination_headers + end + end + + context 'from a project fork' do + let(:fork_user) { create :user } + let(:forked_project) { fork_project(project, fork_user, repository: true) } # Forked project carries over :builds_private + let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: forked_project) } + + context 'with private builds' do + context 'for the target project member' do + it 'does not respond with serialized pipelines' do + expect(json_response['pipelines']).to be_empty + expect(json_response['count']['all']).to eq(0) + expect(response).to include_pagination_headers + end + end + + context 'for the source project member' do + let(:user) { fork_user } + + it 'responds with serialized pipelines' do + expect(json_response['pipelines']).to be_present + expect(json_response['count']['all']).to eq(1) + expect(response).to include_pagination_headers + end + end + end + + context 'with public builds' do + let(:forked_project) do + fork_project(project, fork_user, repository: true).tap do |new_project| + new_project.project_feature.update(builds_access_level: ProjectFeature::ENABLED) + end + end + + context 'for the target project member' do + it 'does not respond with serialized pipelines' do + expect(json_response['pipelines']).to be_present + expect(json_response['count']['all']).to eq(1) + expect(response).to include_pagination_headers + end + end + + context 'for the source project member' do + let(:user) { fork_user } + + it 'responds with serialized pipelines' do + expect(json_response['pipelines']).to be_present + expect(json_response['count']['all']).to eq(1) + expect(response).to include_pagination_headers + end + end + end + end end end -- cgit v1.2.3 From a7821dd910fd385a66cfe6c840c37c7b11263410 Mon Sep 17 00:00:00 2001 From: Fabio Pitino Date: Fri, 28 Jun 2019 17:27:07 +0100 Subject: Drop feature to take ownership of a trigger token Removing API and frontend interactions that allowed users to take ownership of a trigger token. Removed mentions from the documentation. --- app/controllers/projects/triggers_controller.rb | 12 +-------- app/views/projects/triggers/_trigger.html.haml | 3 --- ...urity-remove-take-trigger-ownership-feature.yml | 5 ++++ config/routes/project.rb | 6 +---- doc/api/pipeline_triggers.md | 29 ---------------------- doc/ci/triggers/README.md | 14 +---------- doc/user/project/new_ci_build_permissions_model.md | 3 +-- lib/api/triggers.rb | 21 ---------------- locale/gitlab.pot | 6 ----- spec/features/triggers_spec.rb | 23 ----------------- spec/requests/api/triggers_spec.rb | 28 --------------------- 11 files changed, 9 insertions(+), 141 deletions(-) create mode 100644 changelogs/unreleased/security-remove-take-trigger-ownership-feature.yml diff --git a/app/controllers/projects/triggers_controller.rb b/app/controllers/projects/triggers_controller.rb index 284e119ca06..7159d0243a3 100644 --- a/app/controllers/projects/triggers_controller.rb +++ b/app/controllers/projects/triggers_controller.rb @@ -4,7 +4,7 @@ class Projects::TriggersController < Projects::ApplicationController before_action :authorize_admin_build! before_action :authorize_manage_trigger!, except: [:index, :create] before_action :authorize_admin_trigger!, only: [:edit, :update] - before_action :trigger, only: [:take_ownership, :edit, :update, :destroy] + before_action :trigger, only: [:edit, :update, :destroy] layout 'project_settings' @@ -24,16 +24,6 @@ class Projects::TriggersController < Projects::ApplicationController redirect_to project_settings_ci_cd_path(@project, anchor: 'js-pipeline-triggers') end - def take_ownership - if trigger.update(owner: current_user) - flash[:notice] = _('Trigger was re-assigned.') - else - flash[:alert] = _('You could not take ownership of trigger.') - end - - redirect_to project_settings_ci_cd_path(@project, anchor: 'js-pipeline-triggers') - end - def edit end diff --git a/app/views/projects/triggers/_trigger.html.haml b/app/views/projects/triggers/_trigger.html.haml index 6f6f1e5e0c5..7367a96f8e4 100644 --- a/app/views/projects/triggers/_trigger.html.haml +++ b/app/views/projects/triggers/_trigger.html.haml @@ -30,10 +30,7 @@ Never %td.text-right.trigger-actions - - take_ownership_confirmation = "By taking ownership you will bind this trigger to your user account. With this the trigger will have access to all your projects as if it was you. Are you sure?" - revoke_trigger_confirmation = "By revoking a trigger you will break any processes making use of it. Are you sure?" - - if trigger.owner != current_user && can?(current_user, :manage_trigger, trigger) - = link_to 'Take ownership', take_ownership_project_trigger_path(@project, trigger), data: { confirm: take_ownership_confirmation }, method: :post, class: "btn btn-default btn-sm btn-trigger-take-ownership" - if can?(current_user, :admin_trigger, trigger) = link_to edit_project_trigger_path(@project, trigger), method: :get, title: "Edit", class: "btn btn-default btn-sm" do %i.fa.fa-pencil diff --git a/changelogs/unreleased/security-remove-take-trigger-ownership-feature.yml b/changelogs/unreleased/security-remove-take-trigger-ownership-feature.yml new file mode 100644 index 00000000000..201f66e1f18 --- /dev/null +++ b/changelogs/unreleased/security-remove-take-trigger-ownership-feature.yml @@ -0,0 +1,5 @@ +--- +title: Drop feature to take ownership of trigger token. +merge_request: +author: +type: security diff --git a/config/routes/project.rb b/config/routes/project.rb index c202463dadb..1f632765317 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -339,11 +339,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do resource :variables, only: [:show, :update] - resources :triggers, only: [:index, :create, :edit, :update, :destroy] do - member do - post :take_ownership - end - end + resources :triggers, only: [:index, :create, :edit, :update, :destroy] resource :mirror, only: [:show, :update] do member do diff --git a/doc/api/pipeline_triggers.md b/doc/api/pipeline_triggers.md index 736312df116..e207ff8e98a 100644 --- a/doc/api/pipeline_triggers.md +++ b/doc/api/pipeline_triggers.md @@ -120,35 +120,6 @@ curl --request PUT --header "PRIVATE-TOKEN: " --form descript } ``` -## Take ownership of a project trigger - -Update an owner of a project trigger. - -``` -POST /projects/:id/triggers/:trigger_id/take_ownership -``` - -| 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 | -| `trigger_id` | integer | yes | The trigger id | - -``` -curl --request POST --header "PRIVATE-TOKEN: " "https://gitlab.example.com/api/v4/projects/1/triggers/10/take_ownership" -``` - -```json -{ - "id": 10, - "description": "my trigger", - "created_at": "2016-01-07T09:53:58.235Z", - "last_used": null, - "token": "6d056f63e50fe6f8c5f8f4aa10edb7", - "updated_at": "2016-01-07T09:53:58.235Z", - "owner": null -} -``` - ## Remove a project trigger Remove a project's build trigger. diff --git a/doc/ci/triggers/README.md b/doc/ci/triggers/README.md index d1f9aa03b6b..2a382f18038 100644 --- a/doc/ci/triggers/README.md +++ b/doc/ci/triggers/README.md @@ -97,17 +97,6 @@ overview of the time the triggers were last used. ![Triggers page overview](img/triggers_page.png) -## Taking ownership of a trigger - -> **Note**: -GitLab 9.0 introduced a trigger ownership to solve permission problems. - -Each created trigger when run will impersonate their associated user including -their access to projects and their project permissions. - -You can take ownership of existing triggers by clicking *Take ownership*. -From now on the trigger will be run as you. - ## Revoking a trigger You can revoke a trigger any time by going at your project's @@ -282,8 +271,7 @@ Old triggers, created before GitLab 9.0 will be marked as legacy. Triggers with the legacy label do not have an associated user and only have access to the current project. They are considered deprecated and will be -removed with one of the future versions of GitLab. You are advised to -[take ownership](#taking-ownership-of-a-trigger) of any legacy triggers. +removed with one of the future versions of GitLab. [ee-2017]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/2017 [ee-2346]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/2346 diff --git a/doc/user/project/new_ci_build_permissions_model.md b/doc/user/project/new_ci_build_permissions_model.md index d35a8568394..5d7d9991091 100644 --- a/doc/user/project/new_ci_build_permissions_model.md +++ b/doc/user/project/new_ci_build_permissions_model.md @@ -92,8 +92,7 @@ to steal the tokens of other jobs. Since 9.0 [pipeline triggers][triggers] do support the new permission model. The new triggers do impersonate their associated user including their access -to projects and their project permissions. To migrate trigger to use new permission -model use **Take ownership**. +to projects and their project permissions. ## Before GitLab 8.12 diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index 0e829c5699b..eeecc390256 100644 --- a/lib/api/triggers.rb +++ b/lib/api/triggers.rb @@ -112,27 +112,6 @@ module API end end - desc 'Take ownership of trigger' do - success Entities::Trigger - end - params do - requires :trigger_id, type: Integer, desc: 'The trigger ID' - end - post ':id/triggers/:trigger_id/take_ownership' do - authenticate! - authorize! :admin_build, user_project - - trigger = user_project.triggers.find(params.delete(:trigger_id)) - break not_found!('Trigger') unless trigger - - if trigger.update(owner: current_user) - status :ok - present trigger, with: Entities::Trigger, current_user: current_user - else - render_validation_error!(trigger) - end - end - desc 'Delete a trigger' do success Entities::Trigger end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 9b6e8d8c8a4..47dca877c2f 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -11377,9 +11377,6 @@ msgstr "" msgid "Trigger was created successfully." msgstr "" -msgid "Trigger was re-assigned." -msgstr "" - msgid "Trigger was successfully updated." msgstr "" @@ -12321,9 +12318,6 @@ msgstr "" msgid "You could not create a new trigger." msgstr "" -msgid "You could not take ownership of trigger." -msgstr "" - msgid "You do not have any subscriptions yet" msgstr "" diff --git a/spec/features/triggers_spec.rb b/spec/features/triggers_spec.rb index 919859c145a..41b640bb53a 100644 --- a/spec/features/triggers_spec.rb +++ b/spec/features/triggers_spec.rb @@ -81,29 +81,6 @@ describe 'Triggers', :js do end end - describe 'trigger "Take ownership" workflow' do - before do - create(:ci_trigger, owner: user2, project: @project, description: trigger_title) - visit project_settings_ci_cd_path(@project) - end - - it 'button "Take ownership" has correct alert' do - expected_alert = 'By taking ownership you will bind this trigger to your user account. With this the trigger will have access to all your projects as if it was you. Are you sure?' - expect(page.find('a.btn-trigger-take-ownership')['data-confirm']).to eq expected_alert - end - - it 'take trigger ownership' do - # See if "Take ownership" on trigger works post trigger creation - page.accept_confirm do - first(:link, "Take ownership").send_keys(:return) - end - - expect(page.find('.flash-notice')).to have_content 'Trigger was re-assigned.' - expect(page.find('.triggers-list')).to have_content trigger_title - expect(page.find('.triggers-list .trigger-owner')).to have_content user.name - end - end - describe 'trigger "Revoke" workflow' do before do create(:ci_trigger, owner: user2, project: @project, description: trigger_title) diff --git a/spec/requests/api/triggers_spec.rb b/spec/requests/api/triggers_spec.rb index f0f01e97f1d..8ea3d16a41f 100644 --- a/spec/requests/api/triggers_spec.rb +++ b/spec/requests/api/triggers_spec.rb @@ -270,34 +270,6 @@ describe API::Triggers do end end - describe 'POST /projects/:id/triggers/:trigger_id/take_ownership' do - context 'authenticated user with valid permissions' do - it 'updates owner' do - post api("/projects/#{project.id}/triggers/#{trigger.id}/take_ownership", user) - - expect(response).to have_gitlab_http_status(200) - expect(json_response).to include('owner') - expect(trigger.reload.owner).to eq(user) - end - end - - context 'authenticated user with invalid permissions' do - it 'does not update owner' do - post api("/projects/#{project.id}/triggers/#{trigger.id}/take_ownership", user2) - - expect(response).to have_gitlab_http_status(403) - end - end - - context 'unauthenticated user' do - it 'does not update owner' do - post api("/projects/#{project.id}/triggers/#{trigger.id}/take_ownership") - - expect(response).to have_gitlab_http_status(401) - end - end - end - describe 'DELETE /projects/:id/triggers/:trigger_id' do context 'authenticated user with valid permissions' do it 'deletes trigger' do -- cgit v1.2.3 From 1c42f748f8cac88692d3dca5470f74331e48b8d1 Mon Sep 17 00:00:00 2001 From: Fabio Pitino Date: Fri, 21 Jun 2019 17:56:03 +0100 Subject: Don't display badges when builds are restricted Badges were leaked to unauthorized users even when Public Builds project setting is disabled. Added guard clause to the controller to check if user can read build. --- app/controllers/projects/badges_controller.rb | 3 +- ...ity-fix-badges-leaked-to-unauthorized-users.yml | 5 + .../controllers/projects/badges_controller_spec.rb | 124 ++++++++++++++++----- 3 files changed, 101 insertions(+), 31 deletions(-) create mode 100644 changelogs/unreleased/security-fix-badges-leaked-to-unauthorized-users.yml diff --git a/app/controllers/projects/badges_controller.rb b/app/controllers/projects/badges_controller.rb index 09a384e89ab..66b51b17790 100644 --- a/app/controllers/projects/badges_controller.rb +++ b/app/controllers/projects/badges_controller.rb @@ -3,7 +3,8 @@ class Projects::BadgesController < Projects::ApplicationController layout 'project_settings' before_action :authorize_admin_project!, only: [:index] - before_action :no_cache_headers, except: [:index] + before_action :no_cache_headers, only: [:pipeline, :coverage] + before_action :authorize_read_build!, only: [:pipeline, :coverage] def pipeline pipeline_status = Gitlab::Badge::Pipeline::Status diff --git a/changelogs/unreleased/security-fix-badges-leaked-to-unauthorized-users.yml b/changelogs/unreleased/security-fix-badges-leaked-to-unauthorized-users.yml new file mode 100644 index 00000000000..9526f3c559f --- /dev/null +++ b/changelogs/unreleased/security-fix-badges-leaked-to-unauthorized-users.yml @@ -0,0 +1,5 @@ +--- +title: Show badges if pipelines are public otherwise default to project permissions. +erge_request: +author: +type: security diff --git a/spec/controllers/projects/badges_controller_spec.rb b/spec/controllers/projects/badges_controller_spec.rb index 5ec8d8d41d7..4ae29ba7f54 100644 --- a/spec/controllers/projects/badges_controller_spec.rb +++ b/spec/controllers/projects/badges_controller_spec.rb @@ -7,51 +7,115 @@ describe Projects::BadgesController do let!(:pipeline) { create(:ci_empty_pipeline) } let(:user) { create(:user) } - before do - project.add_maintainer(user) - sign_in(user) - end + shared_examples 'a badge resource' do |badge_type| + context 'when pipelines are public' do + before do + project.update!(public_builds: true) + end + + context 'when project is public' do + before do + project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + end + + it "returns the #{badge_type} badge to unauthenticated users" do + get_badge(badge_type) + + expect(response).to have_gitlab_http_status(:ok) + end + end + + context 'when project is restricted' do + before do + project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL) + project.add_guest(user) + sign_in(user) + end + + it "returns the #{badge_type} badge to guest users" do + get_badge(badge_type) + + expect(response).to have_gitlab_http_status(:ok) + end + end + end - it 'requests the pipeline badge successfully' do - get_badge(:pipeline) + context 'format' do + before do + project.add_maintainer(user) + sign_in(user) + end - expect(response).to have_gitlab_http_status(:ok) - end + it 'renders the `flat` badge layout by default' do + get_badge(badge_type) - it 'requests the coverage badge successfully' do - get_badge(:coverage) + expect(response).to render_template('projects/badges/badge') + end - expect(response).to have_gitlab_http_status(:ok) - end + context 'when style param is set to `flat`' do + it 'renders the `flat` badge layout' do + get_badge(badge_type, 'flat') - it 'renders the `flat` badge layout by default' do - get_badge(:coverage) + expect(response).to render_template('projects/badges/badge') + end + end - expect(response).to render_template('projects/badges/badge') - end + context 'when style param is set to an invalid type' do + it 'renders the `flat` (default) badge layout' do + get_badge(badge_type, 'xxx') + + expect(response).to render_template('projects/badges/badge') + end + end - context 'when style param is set to `flat`' do - it 'renders the `flat` badge layout' do - get_badge(:coverage, 'flat') + context 'when style param is set to `flat-square`' do + it 'renders the `flat-square` badge layout' do + get_badge(badge_type, 'flat-square') - expect(response).to render_template('projects/badges/badge') + expect(response).to render_template('projects/badges/badge_flat-square') + end + end end - end - context 'when style param is set to an invalid type' do - it 'renders the `flat` (default) badge layout' do - get_badge(:coverage, 'xxx') + context 'when pipelines are not public' do + before do + project.update!(public_builds: false) + end - expect(response).to render_template('projects/badges/badge') + context 'when project is public' do + before do + project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + end + + it 'returns 404 to unauthenticated users' do + get_badge(badge_type) + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'when project is restricted to the user' do + before do + project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL) + project.add_guest(user) + sign_in(user) + end + + it 'defaults to project permissions' do + get_badge(:coverage) + + expect(response).to have_gitlab_http_status(:not_found) + end + end end end - context 'when style param is set to `flat-square`' do - it 'renders the `flat-square` badge layout' do - get_badge(:coverage, 'flat-square') + describe '#pipeline' do + it_behaves_like 'a badge resource', :pipeline + end - expect(response).to render_template('projects/badges/badge_flat-square') - end + describe '#coverage' do + it_behaves_like 'a badge resource', :coverage end def get_badge(badge, style = nil) -- cgit v1.2.3 From dfe906209e2238b82c84c9fb435498cae2f3d43e Mon Sep 17 00:00:00 2001 From: Adam Hegyi Date: Thu, 11 Jul 2019 06:48:50 +0200 Subject: Queries for Upload should be scoped by model --- app/controllers/concerns/uploads_actions.rb | 2 +- app/uploaders/records_uploads.rb | 2 +- .../unreleased/security-60551-fix-upload-scope.yml | 5 +++++ spec/controllers/groups/uploads_controller_spec.rb | 5 +++++ .../controllers/projects/uploads_controller_spec.rb | 5 +++++ .../controllers/uploads_actions_shared_examples.rb | 10 ++++++++++ spec/uploaders/records_uploads_spec.rb | 21 +++++++++++++++++++++ 7 files changed, 48 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/security-60551-fix-upload-scope.yml diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb index 59f6d3452a3..f5d35379e10 100644 --- a/app/controllers/concerns/uploads_actions.rb +++ b/app/controllers/concerns/uploads_actions.rb @@ -90,7 +90,7 @@ module UploadsActions return unless uploader = build_uploader upload_paths = uploader.upload_paths(params[:filename]) - upload = Upload.find_by(uploader: uploader_class.to_s, path: upload_paths) + upload = Upload.find_by(model: model, uploader: uploader_class.to_s, path: upload_paths) upload&.build_uploader end # rubocop: enable CodeReuse/ActiveRecord diff --git a/app/uploaders/records_uploads.rb b/app/uploaders/records_uploads.rb index 00b51f92b12..d7a859ebd5f 100644 --- a/app/uploaders/records_uploads.rb +++ b/app/uploaders/records_uploads.rb @@ -35,7 +35,7 @@ module RecordsUploads end def readd_upload - uploads.where(path: upload_path).delete_all + uploads.where(model: model, path: upload_path).delete_all upload.delete if upload self.upload = build_upload.tap(&:save!) diff --git a/changelogs/unreleased/security-60551-fix-upload-scope.yml b/changelogs/unreleased/security-60551-fix-upload-scope.yml new file mode 100644 index 00000000000..7d7096833a7 --- /dev/null +++ b/changelogs/unreleased/security-60551-fix-upload-scope.yml @@ -0,0 +1,5 @@ +--- +title: Queries for Upload should be scoped by model +merge_request: +author: +type: security diff --git a/spec/controllers/groups/uploads_controller_spec.rb b/spec/controllers/groups/uploads_controller_spec.rb index 0f99a957581..60342bf8e3d 100644 --- a/spec/controllers/groups/uploads_controller_spec.rb +++ b/spec/controllers/groups/uploads_controller_spec.rb @@ -10,6 +10,11 @@ describe Groups::UploadsController do { group_id: model } end + let(:other_model) { create(:group, :public) } + let(:other_params) do + { group_id: other_model } + end + it_behaves_like 'handle uploads' do let(:uploader_class) { NamespaceFileUploader } end diff --git a/spec/controllers/projects/uploads_controller_spec.rb b/spec/controllers/projects/uploads_controller_spec.rb index 776c1270977..661ed9840b1 100644 --- a/spec/controllers/projects/uploads_controller_spec.rb +++ b/spec/controllers/projects/uploads_controller_spec.rb @@ -10,6 +10,11 @@ describe Projects::UploadsController do { namespace_id: model.namespace.to_param, project_id: model } end + let(:other_model) { create(:project, :public) } + let(:other_params) do + { namespace_id: other_model.namespace.to_param, project_id: other_model } + end + it_behaves_like 'handle uploads' context 'when the URL the old style, without /-/system' do diff --git a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb index 59708173716..9036838e50a 100644 --- a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb +++ b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb @@ -74,6 +74,16 @@ shared_examples 'handle uploads' do UploadService.new(model, jpg, uploader_class).execute end + context 'when accessing a specific upload via different model' do + it 'responds with status 404' do + params.merge!(other_params) + + show_upload + + expect(response).to have_gitlab_http_status(404) + end + end + context "when the model is public" do before do model.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC) diff --git a/spec/uploaders/records_uploads_spec.rb b/spec/uploaders/records_uploads_spec.rb index 42352f9b9f8..6134137d2b7 100644 --- a/spec/uploaders/records_uploads_spec.rb +++ b/spec/uploaders/records_uploads_spec.rb @@ -85,6 +85,27 @@ describe RecordsUploads do expect { existing.reload }.to raise_error(ActiveRecord::RecordNotFound) expect(Upload.count).to eq(1) end + + it 'does not affect other uploads with different model but the same path' do + project = create(:project) + other_project = create(:project) + + uploader = RecordsUploadsExampleUploader.new(other_project) + + upload_for_project = Upload.create!( + path: File.join('uploads', 'rails_sample.jpg'), + size: 512.kilobytes, + model: project, + uploader: uploader.class.to_s + ) + + uploader.store!(upload_fixture('rails_sample.jpg')) + + upload_for_project_fresh = Upload.find(upload_for_project.id) + + expect(upload_for_project).to eq(upload_for_project_fresh) + expect(Upload.count).to eq(2) + end end describe '#destroy_upload callback' do -- cgit v1.2.3 From 43830eca33b6be5d59685be5c2f3270ed81bf751 Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Wed, 10 Jul 2019 17:04:02 -0300 Subject: Do not show moved issue ids for user not authorized Do not show moved issue id for users that cannot read issue --- app/serializers/issue_entity.rb | 7 ++++- .../unreleased/security-hide_moved_issue_id.yml | 5 ++++ spec/serializers/issue_entity_spec.rb | 33 ++++++++++++++++++++++ 3 files changed, 44 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/security-hide_moved_issue_id.yml diff --git a/app/serializers/issue_entity.rb b/app/serializers/issue_entity.rb index 36e601f45c5..82139855760 100644 --- a/app/serializers/issue_entity.rb +++ b/app/serializers/issue_entity.rb @@ -16,9 +16,14 @@ class IssueEntity < IssuableEntity expose :discussion_locked expose :assignees, using: API::Entities::UserBasic expose :due_date - expose :moved_to_id expose :project_id + expose :moved_to_id do |issue| + if issue.moved_to_id.present? && can?(request.current_user, :read_issue, issue.moved_to) + issue.moved_to_id + end + end + expose :web_url do |issue| project_issue_path(issue.project, issue) end diff --git a/changelogs/unreleased/security-hide_moved_issue_id.yml b/changelogs/unreleased/security-hide_moved_issue_id.yml new file mode 100644 index 00000000000..24353d797c9 --- /dev/null +++ b/changelogs/unreleased/security-hide_moved_issue_id.yml @@ -0,0 +1,5 @@ +--- +title: Do not show moved issue id for users that cannot read issue +merge_request: +author: +type: security diff --git a/spec/serializers/issue_entity_spec.rb b/spec/serializers/issue_entity_spec.rb index caa3e41402b..0e05b3c84f4 100644 --- a/spec/serializers/issue_entity_spec.rb +++ b/spec/serializers/issue_entity_spec.rb @@ -17,4 +17,37 @@ describe IssueEntity do it 'has time estimation attributes' do expect(subject).to include(:time_estimate, :total_time_spent, :human_time_estimate, :human_total_time_spent) end + + context 'when issue got moved' do + let(:public_project) { create(:project, :public) } + let(:member) { create(:user) } + let(:non_member) { create(:user) } + let(:issue) { create(:issue, project: public_project) } + + before do + project.add_developer(member) + public_project.add_developer(member) + Issues::MoveService.new(public_project, member).execute(issue, project) + end + + context 'when user cannot read target project' do + it 'does not return moved_to_id' do + request = double('request', current_user: non_member) + + response = described_class.new(issue, request: request).as_json + + expect(response[:moved_to_id]).to be_nil + end + end + + context 'when user can read target project' do + it 'returns moved moved_to_id' do + request = double('request', current_user: member) + + response = described_class.new(issue, request: request).as_json + + expect(response[:moved_to_id]).to eq(issue.moved_to_id) + end + end + end end -- cgit v1.2.3 From f5c1cd489834e824c83f2ae909cd0dd41fb95dab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20L=C3=B3pez?= Date: Tue, 2 Jul 2019 18:38:23 +0200 Subject: Fix Server Side Request Forgery mitigation bypass When we can't resolve the hostname or it is invalid, we shouldn't even perform the request. This fix also fixes the problem the SSRF rebinding attack. We can't stub feature flags outside example blocks. Nevertheless, there are some actions that calls the UrlBlocker, that are performed outside example blocks, ie: `set` instruction. That's why we have to use some signalign mechanism outside the scope of the specs. --- changelogs/unreleased/security-dns-ssrf-bypass.yml | 5 +++ lib/gitlab/url_blocker.rb | 13 ++++++- spec/lib/gitlab/url_blocker_spec.rb | 44 ++++++++++++++++------ spec/spec_helper.rb | 1 + 4 files changed, 49 insertions(+), 14 deletions(-) create mode 100644 changelogs/unreleased/security-dns-ssrf-bypass.yml diff --git a/changelogs/unreleased/security-dns-ssrf-bypass.yml b/changelogs/unreleased/security-dns-ssrf-bypass.yml new file mode 100644 index 00000000000..e48696ce5bd --- /dev/null +++ b/changelogs/unreleased/security-dns-ssrf-bypass.yml @@ -0,0 +1,5 @@ +--- +title: Fix Server Side Request Forgery mitigation bypass +merge_request: +author: +type: security diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb index f6b2e2acf16..cc937cbb3cf 100644 --- a/lib/gitlab/url_blocker.rb +++ b/lib/gitlab/url_blocker.rb @@ -85,9 +85,9 @@ module Gitlab # we'll be making the request to the IP address, instead of using the hostname. def enforce_uri_hostname(addrs_info, uri, hostname, dns_rebind_protection) address = addrs_info.first - ip_address = address&.ip_address + ip_address = address.ip_address - return [uri, nil] unless dns_rebind_protection && ip_address && ip_address != hostname + return [uri, nil] unless dns_rebind_protection && ip_address != hostname uri = uri.dup uri.hostname = ip_address @@ -111,6 +111,15 @@ module Gitlab addr.ipv6_v4mapped? ? addr.ipv6_to_ipv4 : addr end rescue SocketError + # In the test suite we use a lot of mocked urls that are either invalid or + # don't exist. In order to avoid modifying a ton of tests and factories + # we allow invalid urls unless the environment variable RSPEC_ALLOW_INVALID_URLS + # is not true + return if Rails.env.test? && ENV['RSPEC_ALLOW_INVALID_URLS'] == 'true' + + # If the addr can't be resolved or the url is invalid (i.e http://1.1.1.1.1) + # we block the url + raise BlockedUrlError, "Host cannot be resolved or invalid" end def validate_local_request(address_info:, allow_localhost:, allow_local_network:) diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb index f8b0cbfb6f6..0fab890978a 100644 --- a/spec/lib/gitlab/url_blocker_spec.rb +++ b/spec/lib/gitlab/url_blocker_spec.rb @@ -68,6 +68,16 @@ describe Gitlab::UrlBlocker do expect(uri).to eq(Addressable::URI.parse('https://example.org')) expect(hostname).to eq(nil) end + + context 'when it cannot be resolved' do + let(:import_url) { 'http://foobar.x' } + + it 'raises error' do + stub_env('RSPEC_ALLOW_INVALID_URLS', 'false') + + expect { described_class.validate!(import_url) }.to raise_error(described_class::BlockedUrlError) + end + end end context 'when the URL hostname is an IP address' do @@ -79,6 +89,16 @@ describe Gitlab::UrlBlocker do expect(uri).to eq(Addressable::URI.parse('https://93.184.216.34')) expect(hostname).to be(nil) end + + context 'when it is invalid' do + let(:import_url) { 'http://1.1.1.1.1' } + + it 'raises an error' do + stub_env('RSPEC_ALLOW_INVALID_URLS', 'false') + + expect { described_class.validate!(import_url) }.to raise_error(described_class::BlockedUrlError) + end + end end end end @@ -180,8 +200,6 @@ describe Gitlab::UrlBlocker do end it 'returns true for a non-alphanumeric hostname' do - stub_resolv - aggregate_failures do expect(described_class).to be_blocked_url('ssh://-oProxyCommand=whoami/a') @@ -300,10 +318,6 @@ describe Gitlab::UrlBlocker do end context 'when enforce_user is' do - before do - stub_resolv - end - context 'false (default)' do it 'does not block urls with a non-alphanumeric username' do expect(described_class).not_to be_blocked_url('ssh://-oProxyCommand=whoami@example.com/a') @@ -351,6 +365,18 @@ describe Gitlab::UrlBlocker do expect(described_class.blocked_url?('https://git‌lab.com/foo/foo.bar', ascii_only: true)).to be true end end + + it 'blocks urls with invalid ip address' do + stub_env('RSPEC_ALLOW_INVALID_URLS', 'false') + + expect(described_class).to be_blocked_url('http://8.8.8.8.8') + end + + it 'blocks urls whose hostname cannot be resolved' do + stub_env('RSPEC_ALLOW_INVALID_URLS', 'false') + + expect(described_class).to be_blocked_url('http://foobar.x') + end end describe '#validate_hostname' do @@ -382,10 +408,4 @@ describe Gitlab::UrlBlocker do end end end - - # Resolv does not support resolving UTF-8 domain names - # See https://bugs.ruby-lang.org/issues/4270 - def stub_resolv - allow(Resolv).to receive(:getaddresses).and_return([]) - end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 95e0d8858b9..089dbc09aa3 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -3,6 +3,7 @@ SimpleCovEnv.start! ENV["RAILS_ENV"] = 'test' ENV["IN_MEMORY_APPLICATION_SETTINGS"] = 'true' +ENV["RSPEC_ALLOW_INVALID_URLS"] = 'true' require File.expand_path('../config/environment', __dir__) require 'rspec/rails' -- cgit v1.2.3 From 6c27c0d394b70be4f2a2e0fa047f6844199c2661 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Fri, 12 Jul 2019 11:10:54 +0200 Subject: Filter params in MR build service Reusing the existing `IssuableBaseService#filter_params` which uses the policies to determine what params a user can set, and which values it can be set to. This also removed the need for the seperate call to `IssuableBaseService#ensure_milestone_available`. The `Issues::BuildService` does not suffer from this because it limits the params that are assignable to the `title`, `description` and `milestone_id`. --- app/services/merge_requests/build_service.rb | 28 ++++++++--- .../unreleased/security-bvl-filter-mr-params.yml | 5 ++ ...ess_private_project_info_through_new_mr_spec.rb | 56 ++++++++++++++++++++++ ...ccess_private_repository_through_new_mr_spec.rb | 37 -------------- spec/services/merge_requests/build_service_spec.rb | 37 +++++++++++++- 5 files changed, 119 insertions(+), 44 deletions(-) create mode 100644 changelogs/unreleased/security-bvl-filter-mr-params.yml create mode 100644 spec/features/merge_request/user_tries_to_access_private_project_info_through_new_mr_spec.rb delete mode 100644 spec/features/merge_request/user_tries_to_access_private_repository_through_new_mr_spec.rb diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index 109c964e577..b28f80939ae 100644 --- a/app/services/merge_requests/build_service.rb +++ b/app/services/merge_requests/build_service.rb @@ -11,15 +11,18 @@ module MergeRequests # https://gitlab.com/gitlab-org/gitlab-ce/issues/53658 merge_quick_actions_into_params!(merge_request, only: [:target_branch]) merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch) if params.has_key?(:force_remove_source_branch) - merge_request.assign_attributes(params) + # Assign the projects first so we can use policies for `filter_params` merge_request.author = current_user + merge_request.source_project = find_source_project + merge_request.target_project = find_target_project + + filter_params(merge_request) + merge_request.assign_attributes(params.to_h.compact) + merge_request.compare_commits = [] - merge_request.source_project = find_source_project - merge_request.target_project = find_target_project - merge_request.target_branch = find_target_branch - merge_request.can_be_created = projects_and_branches_valid? - ensure_milestone_available(merge_request) + merge_request.target_branch = find_target_branch + merge_request.can_be_created = projects_and_branches_valid? # compare branches only if branches are valid, otherwise # compare_branches may raise an error @@ -50,12 +53,14 @@ module MergeRequests to: :merge_request def find_source_project + source_project = project_from_params(:source_project) return source_project if source_project.present? && can?(current_user, :create_merge_request_from, source_project) project end def find_target_project + target_project = project_from_params(:target_project) return target_project if target_project.present? && can?(current_user, :create_merge_request_in, target_project) target_project = project.default_merge_request_target @@ -65,6 +70,17 @@ module MergeRequests project end + def project_from_params(param_name) + project_from_params = params.delete(param_name) + + id_param_name = :"#{param_name}_id" + if project_from_params.nil? && params[id_param_name] + project_from_params = Project.find_by_id(params.delete(id_param_name)) + end + + project_from_params + end + def find_target_branch target_branch || target_project.default_branch end diff --git a/changelogs/unreleased/security-bvl-filter-mr-params.yml b/changelogs/unreleased/security-bvl-filter-mr-params.yml new file mode 100644 index 00000000000..4433ec73b7c --- /dev/null +++ b/changelogs/unreleased/security-bvl-filter-mr-params.yml @@ -0,0 +1,5 @@ +--- +title: Filter merge request params on the new merge request page +merge_request: +author: +type: security diff --git a/spec/features/merge_request/user_tries_to_access_private_project_info_through_new_mr_spec.rb b/spec/features/merge_request/user_tries_to_access_private_project_info_through_new_mr_spec.rb new file mode 100644 index 00000000000..1ebe9e2e409 --- /dev/null +++ b/spec/features/merge_request/user_tries_to_access_private_project_info_through_new_mr_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe 'Merge Request > User tries to access private project information through the new mr page' do + let(:current_user) { create(:user) } + let(:private_project) do + create(:project, :public, :repository, + path: 'nothing-to-see-here', + name: 'nothing to see here', + repository_access_level: ProjectFeature::PRIVATE) + end + let(:owned_project) do + create(:project, :public, :repository, + namespace: current_user.namespace, + creator: current_user) + end + + context 'when the user enters the querystring info for the other project' do + let(:mr_path) do + project_new_merge_request_diffs_path( + owned_project, + merge_request: { + source_project_id: private_project.id, + source_branch: 'feature' + } + ) + end + + before do + sign_in current_user + visit mr_path + end + + it "does not mention the project the user can't see the repo of" do + expect(page).not_to have_content('nothing-to-see-here') + end + + context 'when the user enters label information from the private project in the querystring' do + let(:inaccessible_label) { create(:label, project: private_project) } + let(:mr_path) do + project_new_merge_request_path( + owned_project, + merge_request: { + label_ids: [inaccessible_label.id], + source_branch: 'feature' + } + ) + end + + it 'does not expose the label name' do + expect(page).not_to have_content(inaccessible_label.name) + end + end + end +end diff --git a/spec/features/merge_request/user_tries_to_access_private_repository_through_new_mr_spec.rb b/spec/features/merge_request/user_tries_to_access_private_repository_through_new_mr_spec.rb deleted file mode 100644 index 9318b5f1ebb..00000000000 --- a/spec/features/merge_request/user_tries_to_access_private_repository_through_new_mr_spec.rb +++ /dev/null @@ -1,37 +0,0 @@ -require 'spec_helper' - -describe 'Merge Request > Tries to access private repo of public project' do - let(:current_user) { create(:user) } - let(:private_project) do - create(:project, :public, :repository, - path: 'nothing-to-see-here', - name: 'nothing to see here', - repository_access_level: ProjectFeature::PRIVATE) - end - let(:owned_project) do - create(:project, :public, :repository, - namespace: current_user.namespace, - creator: current_user) - end - - context 'when the user enters the querystring info for the other project' do - let(:mr_path) do - project_new_merge_request_diffs_path( - owned_project, - merge_request: { - source_project_id: private_project.id, - source_branch: 'feature' - } - ) - end - - before do - sign_in current_user - visit mr_path - end - - it "does not mention the project the user can't see the repo of" do - expect(page).not_to have_content('nothing-to-see-here') - end - end -end diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index 5c3b209086c..f18239f6d39 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -1,5 +1,4 @@ # frozen_string_literal: true - require 'spec_helper' describe MergeRequests::BuildService do @@ -225,6 +224,11 @@ describe MergeRequests::BuildService do let(:label_ids) { [label2.id] } let(:milestone_id) { milestone2.id } + before do + # Guests are not able to assign labels or milestones to an issue + project.add_developer(user) + end + it 'assigns milestone_id and label_ids instead of issue labels and milestone' do expect(merge_request.milestone).to eq(milestone2) expect(merge_request.labels).to match_array([label2]) @@ -479,4 +483,35 @@ describe MergeRequests::BuildService do end end end + + context 'when assigning labels' do + let(:label_ids) { [create(:label, project: project).id] } + + context 'for members with less than developer access' do + it 'is not allowed' do + expect(merge_request.label_ids).to be_empty + end + end + + context 'for users allowed to assign labels' do + before do + project.add_developer(user) + end + + context 'for labels in the project' do + it 'is allowed for developers' do + expect(merge_request.label_ids).to contain_exactly(*label_ids) + end + end + + context 'for unrelated labels' do + let(:project_label) { create(:label, project: project) } + let(:label_ids) { [create(:label).id, project_label.id] } + + it 'only assigns related labels' do + expect(merge_request.label_ids).to contain_exactly(project_label.id) + end + end + end + end end -- cgit v1.2.3 From 85104fc81f7663fb77114cc6bf99ca095adf7701 Mon Sep 17 00:00:00 2001 From: GitLab Release Tools Bot Date: Thu, 25 Jul 2019 08:22:38 +0000 Subject: Update CHANGELOG.md for 12.1.2 [ci skip] --- CHANGELOG.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ed244b7ed2..0610228127d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,21 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 12.1.2 + +### Security (9 changes) + +- Restrict slash commands to users who can log in. +- Patch XSS issue in wiki links. +- Queries for Upload should be scoped by model. +- Filter merge request params on the new merge request page. +- Fix Server Side Request Forgery mitigation bypass. +- Show badges if pipelines are public otherwise default to project permissions. +- Do not allow localhost url redirection in GitHub Integration. +- Do not show moved issue id for users that cannot read issue. +- Drop feature to take ownership of trigger token. + + ## 12.1.1 - No changes. -- cgit v1.2.3 From acc694ead6f8a7428efab126aa6b8a29b132db43 Mon Sep 17 00:00:00 2001 From: Kerri Miller Date: Fri, 26 Jul 2019 13:41:11 +0000 Subject: Extract SanitizeNodeLink and apply to WikiLinkFilter The SanitizationFilter was running before the WikiFilter. Since WikiFilter can modify links, we could see links that _should_ be stopped by SanatizationFilter being rendered on the page. I (kerrizor) had previously addressed the bug in: https://gitlab.com/gitlab-org/gitlab-ee/commit/7bc971915bbeadb950bb0e1f13510bf3038229a4 However, an additional exploit was discovered after that was merged. Working through the issue, we couldn't simply shuffle the order of filters, due to some implicit assumptions about the order of filters, so instead we've extracted the logic that sanitizes a Nokogiri-generated Node object, and applied it to the WikiLinkFilter as well. On moving filters around: Once we start moving around filters, we get cascading failures; fix one, another one crops up. Many of the existing filters in the WikiPipeline chain seem to assume that other filters have already done their work, and thus operate on a "transform anything that's left" basis; WikiFilter, for instance, assumes any link it finds in the markdown should be prepended with the wiki_base_path.. but if it does that, it also turns `href="@user"` into `href="/path/to/wiki/@user"`, which the UserReferenceFilter doesn't see as a user reference it needs to transform into a user profile link. This is true for all the reference filters in the WikiPipeline. --- ...-60143-patch-additional-xss-vector-in-wikis.yml | 5 ++ lib/banzai/filter/autolink_filter.rb | 11 +-- lib/banzai/filter/base_sanitization_filter.rb | 32 +-------- lib/banzai/filter/wiki_link_filter.rb | 15 +++- lib/banzai/filter/wiki_link_filter/rewriter.rb | 8 --- lib/gitlab/utils/sanitize_node_link.rb | 61 +++++++++++++++++ spec/lib/banzai/filter/wiki_link_filter_spec.rb | 42 ------------ spec/lib/banzai/pipeline/wiki_pipeline_spec.rb | 79 ++++++++++++++++++++++ spec/lib/gitlab/utils/sanitize_node_link_spec.rb | 72 ++++++++++++++++++++ 9 files changed, 233 insertions(+), 92 deletions(-) create mode 100644 changelogs/unreleased/security-60143-patch-additional-xss-vector-in-wikis.yml create mode 100644 lib/gitlab/utils/sanitize_node_link.rb create mode 100644 spec/lib/gitlab/utils/sanitize_node_link_spec.rb diff --git a/changelogs/unreleased/security-60143-patch-additional-xss-vector-in-wikis.yml b/changelogs/unreleased/security-60143-patch-additional-xss-vector-in-wikis.yml new file mode 100644 index 00000000000..a8a26d5fc56 --- /dev/null +++ b/changelogs/unreleased/security-60143-patch-additional-xss-vector-in-wikis.yml @@ -0,0 +1,5 @@ +--- +title: Patch XSS issue in wiki links +merge_request: +author: +type: security diff --git a/lib/banzai/filter/autolink_filter.rb b/lib/banzai/filter/autolink_filter.rb index 56214043d87..5f2cbc24c60 100644 --- a/lib/banzai/filter/autolink_filter.rb +++ b/lib/banzai/filter/autolink_filter.rb @@ -18,6 +18,7 @@ module Banzai # class AutolinkFilter < HTML::Pipeline::Filter include ActionView::Helpers::TagHelper + include Gitlab::Utils::SanitizeNodeLink # Pattern to match text that should be autolinked. # @@ -72,19 +73,11 @@ module Banzai private - # Return true if any of the UNSAFE_PROTOCOLS strings are included in the URI scheme - def contains_unsafe?(scheme) - return false unless scheme - - scheme = scheme.strip.downcase - Banzai::Filter::SanitizationFilter::UNSAFE_PROTOCOLS.any? { |protocol| scheme.include?(protocol) } - end - def autolink_match(match) # start by stripping out dangerous links begin uri = Addressable::URI.parse(match) - return match if contains_unsafe?(uri.scheme) + return match unless safe_protocol?(uri.scheme) rescue Addressable::URI::InvalidURIError return match end diff --git a/lib/banzai/filter/base_sanitization_filter.rb b/lib/banzai/filter/base_sanitization_filter.rb index 420e92cb1e8..2dabca3552d 100644 --- a/lib/banzai/filter/base_sanitization_filter.rb +++ b/lib/banzai/filter/base_sanitization_filter.rb @@ -11,6 +11,7 @@ module Banzai # Extends HTML::Pipeline::SanitizationFilter with common rules. class BaseSanitizationFilter < HTML::Pipeline::SanitizationFilter include Gitlab::Utils::StrongMemoize + extend Gitlab::Utils::SanitizeNodeLink UNSAFE_PROTOCOLS = %w(data javascript vbscript).freeze @@ -40,7 +41,7 @@ module Banzai # Allow any protocol in `a` elements # and then remove links with unsafe protocols whitelist[:protocols].delete('a') - whitelist[:transformers].push(self.class.remove_unsafe_links) + whitelist[:transformers].push(self.class.method(:remove_unsafe_links)) # Remove `rel` attribute from `a` elements whitelist[:transformers].push(self.class.remove_rel) @@ -54,35 +55,6 @@ module Banzai end class << self - def remove_unsafe_links - lambda do |env| - node = env[:node] - - return unless node.name == 'a' - return unless node.has_attribute?('href') - - begin - node['href'] = node['href'].strip - uri = Addressable::URI.parse(node['href']) - - return unless uri.scheme - - # Remove all invalid scheme characters before checking against the - # list of unsafe protocols. - # - # See https://tools.ietf.org/html/rfc3986#section-3.1 - scheme = uri.scheme - .strip - .downcase - .gsub(/[^A-Za-z0-9\+\.\-]+/, '') - - node.remove_attribute('href') if UNSAFE_PROTOCOLS.include?(scheme) - rescue Addressable::URI::InvalidURIError - node.remove_attribute('href') - end - end - end - def remove_rel lambda do |env| if env[:node_name] == 'a' diff --git a/lib/banzai/filter/wiki_link_filter.rb b/lib/banzai/filter/wiki_link_filter.rb index 1728a442533..18947679b69 100644 --- a/lib/banzai/filter/wiki_link_filter.rb +++ b/lib/banzai/filter/wiki_link_filter.rb @@ -8,15 +8,19 @@ module Banzai # Context options: # :project_wiki class WikiLinkFilter < HTML::Pipeline::Filter + include Gitlab::Utils::SanitizeNodeLink + def call return doc unless project_wiki? - doc.search('a:not(.gfm)').each { |el| process_link_attr(el.attribute('href')) } - doc.search('video').each { |el| process_link_attr(el.attribute('src')) } + doc.search('a:not(.gfm)').each { |el| process_link(el.attribute('href'), el) } + + doc.search('video').each { |el| process_link(el.attribute('src'), el) } + doc.search('img').each do |el| attr = el.attribute('data-src') || el.attribute('src') - process_link_attr(attr) + process_link(attr, el) end doc @@ -24,6 +28,11 @@ module Banzai protected + def process_link(link_attr, node) + process_link_attr(link_attr) + remove_unsafe_links({ node: node }, remove_invalid_links: false) + end + def project_wiki? !context[:project_wiki].nil? end diff --git a/lib/banzai/filter/wiki_link_filter/rewriter.rb b/lib/banzai/filter/wiki_link_filter/rewriter.rb index 77b5053f38c..f4cc8beeb52 100644 --- a/lib/banzai/filter/wiki_link_filter/rewriter.rb +++ b/lib/banzai/filter/wiki_link_filter/rewriter.rb @@ -4,8 +4,6 @@ module Banzai module Filter class WikiLinkFilter < HTML::Pipeline::Filter class Rewriter - UNSAFE_SLUG_REGEXES = [/\Ajavascript:/i].freeze - def initialize(link_string, wiki:, slug:) @uri = Addressable::URI.parse(link_string) @wiki_base_path = wiki && wiki.wiki_base_path @@ -37,8 +35,6 @@ module Banzai # Of the form `./link`, `../link`, or similar def apply_hierarchical_link_rules! - return if slug_considered_unsafe? - @uri = Addressable::URI.join(@slug, @uri) if @uri.to_s[0] == '.' end @@ -58,10 +54,6 @@ module Banzai def repository_upload? @uri.relative? && @uri.path.starts_with?(Wikis::CreateAttachmentService::ATTACHMENT_PATH) end - - def slug_considered_unsafe? - UNSAFE_SLUG_REGEXES.any? { |r| r.match?(@slug) } - end end end end diff --git a/lib/gitlab/utils/sanitize_node_link.rb b/lib/gitlab/utils/sanitize_node_link.rb new file mode 100644 index 00000000000..620d71a7814 --- /dev/null +++ b/lib/gitlab/utils/sanitize_node_link.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require_dependency 'gitlab/utils' + +module Gitlab + module Utils + module SanitizeNodeLink + UNSAFE_PROTOCOLS = %w(data javascript vbscript).freeze + ATTRS_TO_SANITIZE = %w(href src data-src).freeze + + def remove_unsafe_links(env, remove_invalid_links: true) + node = env[:node] + + sanitize_node(node: node, remove_invalid_links: remove_invalid_links) + + # HTML entities such as have scannable attrs in + # children elements, which also need to be sanitized. + # + node.children.each do |child_node| + sanitize_node(node: child_node, remove_invalid_links: remove_invalid_links) + end + end + + # Remove all invalid scheme characters before checking against the + # list of unsafe protocols. + # + # See https://tools.ietf.org/html/rfc3986#section-3.1 + # + def safe_protocol?(scheme) + return false unless scheme + + scheme = scheme + .strip + .downcase + .gsub(/[^A-Za-z\+\.\-]+/, '') + + UNSAFE_PROTOCOLS.none?(scheme) + end + + private + + def sanitize_node(node:, remove_invalid_links: true) + ATTRS_TO_SANITIZE.each do |attr| + next unless node.has_attribute?(attr) + + begin + node[attr] = node[attr].strip + uri = Addressable::URI.parse(node[attr]) + + next unless uri.scheme + next if safe_protocol?(uri.scheme) + + node.remove_attribute(attr) + rescue Addressable::URI::InvalidURIError + node.remove_attribute(attr) if remove_invalid_links + end + end + end + end + end +end diff --git a/spec/lib/banzai/filter/wiki_link_filter_spec.rb b/spec/lib/banzai/filter/wiki_link_filter_spec.rb index cce1cd0b284..b9059b85fdc 100644 --- a/spec/lib/banzai/filter/wiki_link_filter_spec.rb +++ b/spec/lib/banzai/filter/wiki_link_filter_spec.rb @@ -70,47 +70,5 @@ describe Banzai::Filter::WikiLinkFilter do expect(filtered_link.attribute('href').value).to eq(invalid_link) end end - - context "when the slug is deemed unsafe or invalid" do - let(:link) { "alert(1);" } - - invalid_slugs = [ - "javascript:", - "JaVaScRiPt:", - "\u0001java\u0003script:", - "javascript :", - "javascript: ", - "javascript : ", - ":javascript:", - "javascript:", - "javascript:", - "javascript:", - "javascript:", - "java\0script:", - "  javascript:" - ] - - invalid_slugs.each do |slug| - context "with the slug #{slug}" do - it "doesn't rewrite a (.) relative link" do - filtered_link = filter( - "Link", - project_wiki: wiki, - page_slug: slug).children[0] - - expect(filtered_link.attribute('href').value).not_to include(slug) - end - - it "doesn't rewrite a (..) relative link" do - filtered_link = filter( - "Link", - project_wiki: wiki, - page_slug: slug).children[0] - - expect(filtered_link.attribute('href').value).not_to include(slug) - end - end - end - end end end diff --git a/spec/lib/banzai/pipeline/wiki_pipeline_spec.rb b/spec/lib/banzai/pipeline/wiki_pipeline_spec.rb index 64ca3ec345d..95c0c9f9a17 100644 --- a/spec/lib/banzai/pipeline/wiki_pipeline_spec.rb +++ b/spec/lib/banzai/pipeline/wiki_pipeline_spec.rb @@ -177,6 +177,85 @@ describe Banzai::Pipeline::WikiPipeline do end end end + + describe "checking slug validity when assembling links" do + context "with a valid slug" do + let(:valid_slug) { "http://example.com" } + + it "includes the slug in a (.) relative link" do + output = described_class.to_html( + "[Link](./alert(1);)", + project: project, + project_wiki: project_wiki, + page_slug: valid_slug + ) + + expect(output).to include(valid_slug) + end + + it "includeds the slug in a (..) relative link" do + output = described_class.to_html( + "[Link](../alert(1);)", + project: project, + project_wiki: project_wiki, + page_slug: valid_slug + ) + + expect(output).to include(valid_slug) + end + end + + context "when the slug is deemed unsafe or invalid" do + invalid_slugs = [ + "javascript:", + "JaVaScRiPt:", + "\u0001java\u0003script:", + "javascript :", + "javascript: ", + "javascript : ", + ":javascript:", + "javascript:", + "javascript:", + "javascript:", + "javascript:", + "java\0script:", + "  javascript:" + ] + + invalid_js_links = [ + "alert(1);", + "alert(document.location);" + ] + + invalid_slugs.each do |slug| + context "with the invalid slug #{slug}" do + invalid_js_links.each do |link| + it "doesn't include a prohibited slug in a (.) relative link '#{link}'" do + output = described_class.to_html( + "[Link](./#{link})", + project: project, + project_wiki: project_wiki, + page_slug: slug + ) + + expect(output).not_to include(slug) + end + + it "doesn't include a prohibited slug in a (..) relative link '#{link}'" do + output = described_class.to_html( + "[Link](../#{link})", + project: project, + project_wiki: project_wiki, + page_slug: slug + ) + + expect(output).not_to include(slug) + end + end + end + end + end + end end describe 'videos' do diff --git a/spec/lib/gitlab/utils/sanitize_node_link_spec.rb b/spec/lib/gitlab/utils/sanitize_node_link_spec.rb new file mode 100644 index 00000000000..064c2707d06 --- /dev/null +++ b/spec/lib/gitlab/utils/sanitize_node_link_spec.rb @@ -0,0 +1,72 @@ +require 'spec_helper' + +describe Gitlab::Utils::SanitizeNodeLink do + let(:klass) do + struct = Struct.new(:value) + struct.include(described_class) + + struct + end + + subject(:object) { klass.new(:value) } + + invalid_schemes = [ + "javascript:", + "JaVaScRiPt:", + "\u0001java\u0003script:", + "javascript :", + "javascript: ", + "javascript : ", + ":javascript:", + "javascript:", + "javascript:", + "  javascript:" + ] + + invalid_schemes.each do |scheme| + context "with the scheme: #{scheme}" do + describe "#remove_unsafe_links" do + tags = { + a: { + doc: HTML::Pipeline.parse("foo"), + attr: "href", + node_to_check: -> (doc) { doc.children.first } + }, + img: { + doc: HTML::Pipeline.parse(""), + attr: "src", + node_to_check: -> (doc) { doc.children.first } + }, + video: { + doc: HTML::Pipeline.parse(""), + attr: "src", + node_to_check: -> (doc) { doc.children.first.children.filter("source").first } + } + } + + tags.each do |tag, opts| + context "<#{tag}> tags" do + it "removes the unsafe link" do + node = opts[:node_to_check].call(opts[:doc]) + + expect { object.remove_unsafe_links({ node: node }, remove_invalid_links: true) } + .to change { node[opts[:attr]] } + + expect(node[opts[:attr]]).to be_blank + end + end + end + end + + describe "#safe_protocol?" do + let(:doc) { HTML::Pipeline.parse("foo") } + let(:node) { doc.children.first } + let(:uri) { Addressable::URI.parse(node['href'])} + + it "returns false" do + expect(object.safe_protocol?(scheme)).to be_falsy + end + end + end + end +end -- cgit v1.2.3 From a90b38641d43870a4bf36544ed1d966ae8d1fa13 Mon Sep 17 00:00:00 2001 From: GitLab Release Tools Bot Date: Fri, 26 Jul 2019 17:57:43 +0000 Subject: Update CHANGELOG.md for 12.1.2 [ci skip] --- CHANGELOG.md | 4 ++++ changelogs/unreleased/security-mr-pipeline-permissions.yml | 5 ----- 2 files changed, 4 insertions(+), 5 deletions(-) delete mode 100644 changelogs/unreleased/security-mr-pipeline-permissions.yml diff --git a/CHANGELOG.md b/CHANGELOG.md index 0610228127d..e2882bce1bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ entry. ## 12.1.2 +### Security (1 change) + +- Use source project as permissions reference for MergeRequestsController#pipelines. + ### Security (9 changes) - Restrict slash commands to users who can log in. diff --git a/changelogs/unreleased/security-mr-pipeline-permissions.yml b/changelogs/unreleased/security-mr-pipeline-permissions.yml deleted file mode 100644 index a317c93228c..00000000000 --- a/changelogs/unreleased/security-mr-pipeline-permissions.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Use source project as permissions reference for MergeRequestsController#pipelines -merge_request: -author: -type: security -- cgit v1.2.3 From cc7b15fe935d41aab85918eb7ae7c0ef81f8bfb0 Mon Sep 17 00:00:00 2001 From: GitLab Release Tools Bot Date: Mon, 29 Jul 2019 14:48:20 +0000 Subject: Update CHANGELOG.md for 11.11.7 [ci skip] --- CHANGELOG.md | 15 +++++++++++++++ ...ecurity-60143-patch-additional-xss-vector-in-wikis.yml | 5 ----- changelogs/unreleased/security-bvl-filter-mr-params.yml | 5 ----- changelogs/unreleased/security-dns-ssrf-bypass.yml | 5 ----- .../security-fix-badges-leaked-to-unauthorized-users.yml | 5 ----- changelogs/unreleased/security-github-ssrf-redirect.yml | 5 ----- changelogs/unreleased/security-hide_moved_issue_id.yml | 5 ----- .../security-remove-take-trigger-ownership-feature.yml | 5 ----- 8 files changed, 15 insertions(+), 35 deletions(-) delete mode 100644 changelogs/unreleased/security-60143-patch-additional-xss-vector-in-wikis.yml delete mode 100644 changelogs/unreleased/security-bvl-filter-mr-params.yml delete mode 100644 changelogs/unreleased/security-dns-ssrf-bypass.yml delete mode 100644 changelogs/unreleased/security-fix-badges-leaked-to-unauthorized-users.yml delete mode 100644 changelogs/unreleased/security-github-ssrf-redirect.yml delete mode 100644 changelogs/unreleased/security-hide_moved_issue_id.yml delete mode 100644 changelogs/unreleased/security-remove-take-trigger-ownership-feature.yml diff --git a/CHANGELOG.md b/CHANGELOG.md index e2882bce1bd..d93cc182c62 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -644,6 +644,21 @@ entry. - Moves snowplow to CE repo. +## 11.11.7 + +### Security (9 changes) + +- Restrict slash commands to users who can log in. +- Patch XSS issue in wiki links. +- Filter merge request params on the new merge request page. +- Fix Server Side Request Forgery mitigation bypass. +- Show badges if pipelines are public otherwise default to project permissions. +- Do not allow localhost url redirection in GitHub Integration. +- Do not show moved issue id for users that cannot read issue. +- Use source project as permissions reference for MergeRequestsController#pipelines. +- Drop feature to take ownership of trigger token. + + ## 11.11.4 (2019-06-26) ### Fixed (3 changes) diff --git a/changelogs/unreleased/security-60143-patch-additional-xss-vector-in-wikis.yml b/changelogs/unreleased/security-60143-patch-additional-xss-vector-in-wikis.yml deleted file mode 100644 index a8a26d5fc56..00000000000 --- a/changelogs/unreleased/security-60143-patch-additional-xss-vector-in-wikis.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Patch XSS issue in wiki links -merge_request: -author: -type: security diff --git a/changelogs/unreleased/security-bvl-filter-mr-params.yml b/changelogs/unreleased/security-bvl-filter-mr-params.yml deleted file mode 100644 index 4433ec73b7c..00000000000 --- a/changelogs/unreleased/security-bvl-filter-mr-params.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Filter merge request params on the new merge request page -merge_request: -author: -type: security diff --git a/changelogs/unreleased/security-dns-ssrf-bypass.yml b/changelogs/unreleased/security-dns-ssrf-bypass.yml deleted file mode 100644 index e48696ce5bd..00000000000 --- a/changelogs/unreleased/security-dns-ssrf-bypass.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Fix Server Side Request Forgery mitigation bypass -merge_request: -author: -type: security diff --git a/changelogs/unreleased/security-fix-badges-leaked-to-unauthorized-users.yml b/changelogs/unreleased/security-fix-badges-leaked-to-unauthorized-users.yml deleted file mode 100644 index 9526f3c559f..00000000000 --- a/changelogs/unreleased/security-fix-badges-leaked-to-unauthorized-users.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Show badges if pipelines are public otherwise default to project permissions. -erge_request: -author: -type: security diff --git a/changelogs/unreleased/security-github-ssrf-redirect.yml b/changelogs/unreleased/security-github-ssrf-redirect.yml deleted file mode 100644 index 36a36de3eb0..00000000000 --- a/changelogs/unreleased/security-github-ssrf-redirect.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Do not allow localhost url redirection in GitHub Integration -merge_request: -author: -type: security diff --git a/changelogs/unreleased/security-hide_moved_issue_id.yml b/changelogs/unreleased/security-hide_moved_issue_id.yml deleted file mode 100644 index 24353d797c9..00000000000 --- a/changelogs/unreleased/security-hide_moved_issue_id.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Do not show moved issue id for users that cannot read issue -merge_request: -author: -type: security diff --git a/changelogs/unreleased/security-remove-take-trigger-ownership-feature.yml b/changelogs/unreleased/security-remove-take-trigger-ownership-feature.yml deleted file mode 100644 index 201f66e1f18..00000000000 --- a/changelogs/unreleased/security-remove-take-trigger-ownership-feature.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Drop feature to take ownership of trigger token. -merge_request: -author: -type: security -- cgit v1.2.3