diff options
22 files changed, 165 insertions, 23 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index db641aa9d36..8b104d77bb4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,6 @@ entry. ## 12.5.5 -- No changes. ### Security (1 change) - Upgrade Akismet gem to v3.0.0. !21786 @@ -1 +1 @@ -12.5.5 +12.5.5-ee diff --git a/app/controllers/profiles/notifications_controller.rb b/app/controllers/profiles/notifications_controller.rb index 5f44e55f3ef..d295b64082c 100644 --- a/app/controllers/profiles/notifications_controller.rb +++ b/app/controllers/profiles/notifications_controller.rb @@ -11,6 +11,7 @@ class Profiles::NotificationsController < Profiles::ApplicationController exclude_group_ids: @group_notifications.select(:source_id) ).execute.map { |group| current_user.notification_settings_for(group, inherit: true) } @project_notifications = current_user.notification_settings.for_projects.order(:id) + .select { |notification| current_user.can?(:read_project, notification.source) } @global_notification_setting = current_user.global_notification_setting end # rubocop: enable CodeReuse/ActiveRecord diff --git a/app/helpers/notifications_helper.rb b/app/helpers/notifications_helper.rb index 8855e0cdd70..9a64fe98f86 100644 --- a/app/helpers/notifications_helper.rb +++ b/app/helpers/notifications_helper.rb @@ -116,4 +116,8 @@ module NotificationsHelper def show_unsubscribe_title?(noteable) can?(current_user, "read_#{noteable.to_ability_name}".to_sym, noteable) end + + def can_read_project?(project) + can?(current_user, :read_project, project) + end end diff --git a/app/models/user.rb b/app/models/user.rb index d0e758b0055..1439a981c8d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1307,7 +1307,7 @@ class User < ApplicationRecord .select('ci_runners.*') group_runners = Ci::RunnerNamespace - .where(namespace_id: owned_or_maintainers_groups.select(:id)) + .where(namespace_id: owned_groups.select(:id)) .joins(:runner) .select('ci_runners.*') diff --git a/app/views/sent_notifications/unsubscribe.html.haml b/app/views/sent_notifications/unsubscribe.html.haml index 22fcfcda297..1eecbe3bc0e 100644 --- a/app/views/sent_notifications/unsubscribe.html.haml +++ b/app/views/sent_notifications/unsubscribe.html.haml @@ -1,13 +1,16 @@ - noteable = @sent_notification.noteable - noteable_type = @sent_notification.noteable_type.titleize.downcase - noteable_text = show_unsubscribe_title?(noteable) ? %(#{noteable.title} (#{noteable.to_reference})) : %(#{noteable.to_reference}) -- page_title _("Unsubscribe"), noteable_text, noteable_type.pluralize, @sent_notification.project.full_name +- show_project_path = can_read_project?(@sent_notification.project) +- project_path = show_project_path ? @sent_notification.project.full_name : _("GitLab / Unsubscribe") +- noteable_url = show_project_path ? url_for([@sent_notification.project.namespace.becomes(Namespace), @sent_notification.project, noteable]) : breadcrumb_title_link +- page_title _('Unsubscribe'), noteable_text, noteable_type.pluralize, project_path %h3.page-title = _("Unsubscribe from %{type}") % { type: noteable_type } %p - - link_to_noteable_text = link_to(noteable_text, url_for([@sent_notification.project.namespace.becomes(Namespace), @sent_notification.project, noteable])) + - link_to_noteable_text = link_to(noteable_text, noteable_url) = _("Are you sure you want to unsubscribe from the %{type}: %{link_to_noteable_text}?").html_safe % { type: noteable_type, link_to_noteable_text: link_to_noteable_text } %p diff --git a/changelogs/unreleased/security-11-graphql-timeout-12-5.yml b/changelogs/unreleased/security-11-graphql-timeout-12-5.yml new file mode 100644 index 00000000000..1d06aaced9d --- /dev/null +++ b/changelogs/unreleased/security-11-graphql-timeout-12-5.yml @@ -0,0 +1,5 @@ +--- +title: 'GraphQL: Add timeout to all queries' +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-12-5-mc-api-runner-owner-permissions.yml b/changelogs/unreleased/security-12-5-mc-api-runner-owner-permissions.yml new file mode 100644 index 00000000000..2f23dbf7b9f --- /dev/null +++ b/changelogs/unreleased/security-12-5-mc-api-runner-owner-permissions.yml @@ -0,0 +1,5 @@ +--- +title: Return only runners from groups where user is owner for user CI owned runners. +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-29983-private-project-name-exposed.yml b/changelogs/unreleased/security-29983-private-project-name-exposed.yml new file mode 100644 index 00000000000..2cae417ec1d --- /dev/null +++ b/changelogs/unreleased/security-29983-private-project-name-exposed.yml @@ -0,0 +1,5 @@ +--- +title: Filter out notification settings for projects that a user does not have at least read access +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-34072-project-name-disclosed.yml b/changelogs/unreleased/security-34072-project-name-disclosed.yml new file mode 100644 index 00000000000..f14c7728273 --- /dev/null +++ b/changelogs/unreleased/security-34072-project-name-disclosed.yml @@ -0,0 +1,5 @@ +--- +title: Hide project name and path when unsusbcribing from an issue or merge request +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-fix-invalid-byte-sequence-upload-links-master.yml b/changelogs/unreleased/security-fix-invalid-byte-sequence-upload-links-master.yml new file mode 100644 index 00000000000..afe48b448b0 --- /dev/null +++ b/changelogs/unreleased/security-fix-invalid-byte-sequence-upload-links-master.yml @@ -0,0 +1,5 @@ +--- +title: Fix 500 error caused by invalid byte sequences in uploads links +merge_request: +author: +type: security diff --git a/config/initializers/graphql.rb b/config/initializers/graphql.rb index f1bc289f1f0..2b21c9d9729 100644 --- a/config/initializers/graphql.rb +++ b/config/initializers/graphql.rb @@ -5,3 +5,7 @@ GraphQL::Field.accepts_definitions(authorize: GraphQL::Define.assign_metadata_ke GraphQL::Schema::Object.accepts_definition(:authorize) GraphQL::Schema::Field.accepts_definition(:authorize) + +GitlabSchema.middleware << GraphQL::Schema::TimeoutMiddleware.new(max_seconds: ENV.fetch('GITLAB_RAILS_GRAPHQL_TIMEOUT', 30).to_i) do |timeout_error, query| + Gitlab::GraphqlLogger.error(message: timeout_error.to_s, query: query.query_string, query_variables: query.provided_variables) +end diff --git a/lib/banzai/filter/relative_link_filter.rb b/lib/banzai/filter/relative_link_filter.rb index 583b0081319..4f257189f8e 100644 --- a/lib/banzai/filter/relative_link_filter.rb +++ b/lib/banzai/filter/relative_link_filter.rb @@ -116,7 +116,7 @@ module Banzai end def process_link_to_upload_attr(html_attr) - path_parts = [Addressable::URI.unescape(html_attr.value)] + path_parts = [unescape_and_scrub_uri(html_attr.value)] if project path_parts.unshift(relative_url_root, project.full_path) @@ -172,7 +172,7 @@ module Banzai end def cleaned_file_path(uri) - Addressable::URI.unescape(uri.path).scrub.delete("\0").chomp("/") + unescape_and_scrub_uri(uri.path).delete("\0").chomp("/") end def relative_file_path(uri) @@ -184,7 +184,7 @@ module Banzai def request_path return unless context[:requested_path] - Addressable::URI.unescape(context[:requested_path]).chomp("/") + unescape_and_scrub_uri(context[:requested_path]).chomp("/") end # Convert a relative path into its correct location based on the currently @@ -266,6 +266,12 @@ module Banzai def repository @repository ||= project&.repository end + + private + + def unescape_and_scrub_uri(uri) + Addressable::URI.unescape(uri).scrub + end end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index dc40d58a58c..d2310393e23 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -8193,6 +8193,9 @@ msgstr "" msgid "GitHub import" msgstr "" +msgid "GitLab / Unsubscribe" +msgstr "" + msgid "GitLab CI Linter has been moved" msgstr "" diff --git a/spec/controllers/profiles/notifications_controller_spec.rb b/spec/controllers/profiles/notifications_controller_spec.rb index dbc408bcdd9..ede68744ac6 100644 --- a/spec/controllers/profiles/notifications_controller_spec.rb +++ b/spec/controllers/profiles/notifications_controller_spec.rb @@ -52,6 +52,35 @@ describe Profiles::NotificationsController do end.to exceed_query_limit(control) end end + + context 'with project notifications' do + let!(:notification_setting) { create(:notification_setting, source: project, user: user, level: :watch) } + + before do + sign_in(user) + get :show + end + + context 'when project is public' do + let(:project) { create(:project, :public) } + + it 'shows notification setting for project' do + expect(assigns(:project_notifications).map(&:source_id)).to include(project.id) + end + end + + context 'when project is public' do + let(:project) { create(:project, :private) } + + it 'shows notification setting for project' do + # notification settings for given project were created before project was set to private + expect(user.notification_settings.for_projects.map(&:source_id)).to include(project.id) + + # check that notification settings for project where user does not have access are filtered + expect(assigns(:project_notifications)).to be_empty + end + end + end end describe 'POST update' do diff --git a/spec/controllers/sent_notifications_controller_spec.rb b/spec/controllers/sent_notifications_controller_spec.rb index 0e634d8ba99..4dd4f49dcf1 100644 --- a/spec/controllers/sent_notifications_controller_spec.rb +++ b/spec/controllers/sent_notifications_controller_spec.rb @@ -56,7 +56,7 @@ describe SentNotificationsController do get(:unsubscribe, params: { id: sent_notification.reply_key }) end - shared_examples 'unsubscribing as anonymous' do + shared_examples 'unsubscribing as anonymous' do |project_visibility| it 'does not unsubscribe the user' do expect(noteable.subscribed?(user, target_project)).to be_truthy end @@ -69,6 +69,18 @@ describe SentNotificationsController do expect(response.status).to eq(200) expect(response).to render_template :unsubscribe end + + if project_visibility == :private + it 'does not show project name or path' do + expect(response.body).not_to include(noteable.project.name) + expect(response.body).not_to include(noteable.project.full_name) + end + else + it 'shows project name or path' do + expect(response.body).to include(noteable.project.name) + expect(response.body).to include(noteable.project.full_name) + end + end end context 'when project is public' do @@ -79,7 +91,7 @@ describe SentNotificationsController do expect(response.body).to include(issue.title) end - it_behaves_like 'unsubscribing as anonymous' + it_behaves_like 'unsubscribing as anonymous', :public end context 'when unsubscribing from confidential issue' do @@ -90,7 +102,7 @@ describe SentNotificationsController do expect(response.body).to include(confidential_issue.to_reference) end - it_behaves_like 'unsubscribing as anonymous' + it_behaves_like 'unsubscribing as anonymous', :public end context 'when unsubscribing from merge request' do @@ -100,7 +112,12 @@ describe SentNotificationsController do expect(response.body).to include(merge_request.title) end - it_behaves_like 'unsubscribing as anonymous' + it 'shows project name or path' do + expect(response.body).to include(issue.project.name) + expect(response.body).to include(issue.project.full_name) + end + + it_behaves_like 'unsubscribing as anonymous', :public end end @@ -110,11 +127,11 @@ describe SentNotificationsController do context 'when unsubscribing from issue' do let(:noteable) { issue } - it 'shows issue title' do + it 'does not show issue title' do expect(response.body).not_to include(issue.title) end - it_behaves_like 'unsubscribing as anonymous' + it_behaves_like 'unsubscribing as anonymous', :private end context 'when unsubscribing from confidential issue' do @@ -125,17 +142,17 @@ describe SentNotificationsController do expect(response.body).to include(confidential_issue.to_reference) end - it_behaves_like 'unsubscribing as anonymous' + it_behaves_like 'unsubscribing as anonymous', :private end context 'when unsubscribing from merge request' do let(:noteable) { merge_request } - it 'shows merge request title' do + it 'dos not show merge request title' do expect(response.body).not_to include(merge_request.title) end - it_behaves_like 'unsubscribing as anonymous' + it_behaves_like 'unsubscribing as anonymous', :private end end end diff --git a/spec/lib/banzai/filter/relative_link_filter_spec.rb b/spec/lib/banzai/filter/relative_link_filter_spec.rb index 371c7a2347c..fdd6b0c8ae4 100644 --- a/spec/lib/banzai/filter/relative_link_filter_spec.rb +++ b/spec/lib/banzai/filter/relative_link_filter_spec.rb @@ -124,6 +124,15 @@ describe Banzai::Filter::RelativeLinkFilter do expect { filter(act) }.not_to raise_error end + it 'does not raise an exception on URIs containing invalid utf-8 byte sequences in uploads' do + act = link("/uploads/%FF") + expect { filter(act) }.not_to raise_error + end + + it 'does not raise an exception on URIs containing invalid utf-8 byte sequences in context requested path' do + expect { filter(link("files/test.md"), requested_path: '%FF') }.not_to raise_error + end + it 'does not raise an exception with a garbled path' do act = link("open(/var/tmp/):%20/location%0Afrom:%20/test") expect { filter(act) }.not_to raise_error diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index ee7edb1516c..6e4db13fcbe 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -2533,8 +2533,8 @@ describe User, :do_not_mock_admin_mode do add_user(:maintainer) end - it 'loads' do - expect(user.ci_owned_runners).to contain_exactly(runner) + it 'does not load' do + expect(user.ci_owned_runners).to be_empty end end @@ -2549,6 +2549,20 @@ describe User, :do_not_mock_admin_mode do end end + shared_examples :group_member do + context 'when the user is owner' do + before do + add_user(:owner) + end + + it 'loads' do + expect(user.ci_owned_runners).to contain_exactly(runner) + end + end + + it_behaves_like :member + end + context 'with groups projects runners' do let(:group) { create(:group) } let!(:project) { create(:project, group: group) } @@ -2557,7 +2571,7 @@ describe User, :do_not_mock_admin_mode do group.add_user(user, access) end - it_behaves_like :member + it_behaves_like :group_member end context 'with groups runners' do @@ -2568,14 +2582,14 @@ describe User, :do_not_mock_admin_mode do group.add_user(user, access) end - it_behaves_like :member + it_behaves_like :group_member end context 'with other projects runners' do let!(:project) { create(:project) } def add_user(access) - project.add_role(user, access) + project.add_user(user, access) end it_behaves_like :member @@ -2593,7 +2607,7 @@ describe User, :do_not_mock_admin_mode do subgroup.add_user(another_user, :owner) end - it_behaves_like :member + it_behaves_like :group_member end end diff --git a/spec/requests/api/graphql/gitlab_schema_spec.rb b/spec/requests/api/graphql/gitlab_schema_spec.rb index 2aeb75a10b4..2cb8436662b 100644 --- a/spec/requests/api/graphql/gitlab_schema_spec.rb +++ b/spec/requests/api/graphql/gitlab_schema_spec.rb @@ -8,6 +8,18 @@ describe 'GitlabSchema configurations' do set(:project) { create(:project) } shared_examples 'imposing query limits' do + describe 'timeouts' do + context 'when timeout is reached' do + it 'shows an error' do + Timecop.scale(50000000) do # ludicrously large number because the timeout has to happen before the query even begins + subject + + expect_graphql_errors_to_include /Timeout/ + end + end + end + end + describe '#max_complexity' do context 'when complexity is too high' do it 'shows an error' do diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index 8daba204d50..7bad30d107d 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -6,6 +6,7 @@ describe API::Runners do let(:admin) { create(:user, :admin) } let(:user) { create(:user) } let(:user2) { create(:user) } + let(:group_maintainer) { create(:user) } let(:project) { create(:project, creator_id: user.id) } let(:project2) { create(:project, creator_id: user.id) } @@ -20,6 +21,7 @@ describe API::Runners do before do # Set project access for users + create(:group_member, :maintainer, user: group_maintainer, group: group) create(:project_member, :maintainer, user: user, project: project) create(:project_member, :maintainer, user: user, project: project2) create(:project_member, :reporter, user: user2, project: project) @@ -525,6 +527,20 @@ describe API::Runners do end.to change { Ci::Runner.project_type.count }.by(-1) end + it 'does not delete group runner with maintainer access' do + delete api("/runners/#{group_runner.id}", group_maintainer) + + expect(response).to have_http_status(403) + end + + it 'deletes group runner with owner access' do + expect do + delete api("/runners/#{group_runner.id}", user) + + expect(response).to have_http_status(204) + end.to change { Ci::Runner.group_type.count }.by(-1) + end + it_behaves_like '412 response' do let(:request) { api("/runners/#{project_runner.id}", user) } end diff --git a/vendor/gitignore/C++.gitignore b/vendor/gitignore/C++.gitignore index 259148fa18f..259148fa18f 100755..100644 --- a/vendor/gitignore/C++.gitignore +++ b/vendor/gitignore/C++.gitignore diff --git a/vendor/gitignore/Java.gitignore b/vendor/gitignore/Java.gitignore index a1c2a238a96..a1c2a238a96 100755..100644 --- a/vendor/gitignore/Java.gitignore +++ b/vendor/gitignore/Java.gitignore |