From 1ba642ef864c74759ef93b60ee96c9f1a8c7cdfa Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 5 Dec 2018 14:31:43 +0100 Subject: [PATCH 01/42] Fix path disclosure on Project Import --- app/services/projects/import_error_filter.rb | 14 +++++++ app/services/projects/import_service.rb | 12 +++++- .../security-import-path-logging.yml | 5 +++ lib/gitlab/import_export/shared.rb | 39 +++++++++++++------ spec/lib/gitlab/import_export/shared_spec.rb | 31 +++++++++++++++ .../import_export/version_checker_spec.rb | 2 +- .../projects/import_error_filter_spec.rb | 17 ++++++++ spec/services/projects/import_service_spec.rb | 4 +- 8 files changed, 107 insertions(+), 17 deletions(-) create mode 100644 app/services/projects/import_error_filter.rb create mode 100644 changelogs/unreleased/security-import-path-logging.yml create mode 100644 spec/lib/gitlab/import_export/shared_spec.rb create mode 100644 spec/services/projects/import_error_filter_spec.rb diff --git a/app/services/projects/import_error_filter.rb b/app/services/projects/import_error_filter.rb new file mode 100644 index 00000000000..a0fc5149bb4 --- /dev/null +++ b/app/services/projects/import_error_filter.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +module Projects + # Used by project imports, it removes any potential paths + # included in an error message that could be stored in the DB + class ImportErrorFilter + ERROR_MESSAGE_FILTER = /[^\s]*#{File::SEPARATOR}[^\s]*(?=(\s|\z))/ + FILTER_MESSAGE = '[FILTERED]' + + def self.filter_message(message) + message.gsub(ERROR_MESSAGE_FILTER, FILTER_MESSAGE) + end + end +end diff --git a/app/services/projects/import_service.rb b/app/services/projects/import_service.rb index 0c426faa22d..afd32c0d968 100644 --- a/app/services/projects/import_service.rb +++ b/app/services/projects/import_service.rb @@ -24,8 +24,16 @@ module Projects import_data success - rescue => e + rescue Gitlab::UrlBlocker::BlockedUrlError => e + Gitlab::Sentry.track_acceptable_exception(e, extra: { project_path: project.full_path, importer: project.import_type }) + error("Error importing repository #{project.safe_import_url} into #{project.full_path} - #{e.message}") + rescue => e + message = Projects::ImportErrorFilter.filter_message(e.message) + + Gitlab::Sentry.track_acceptable_exception(e, extra: { project_path: project.full_path, importer: project.import_type }) + + error("Error importing repository #{project.safe_import_url} into #{project.full_path} - #{message}") end private @@ -35,7 +43,7 @@ module Projects begin Gitlab::UrlBlocker.validate!(project.import_url, ports: Project::VALID_IMPORT_PORTS) rescue Gitlab::UrlBlocker::BlockedUrlError => e - raise Error, "Blocked import URL: #{e.message}" + raise e, "Blocked import URL: #{e.message}" end end diff --git a/changelogs/unreleased/security-import-path-logging.yml b/changelogs/unreleased/security-import-path-logging.yml new file mode 100644 index 00000000000..2ba2d88d82a --- /dev/null +++ b/changelogs/unreleased/security-import-path-logging.yml @@ -0,0 +1,5 @@ +--- +title: Fix path disclosure on project import error +merge_request: +author: +type: security diff --git a/lib/gitlab/import_export/shared.rb b/lib/gitlab/import_export/shared.rb index c13e6c1d83b..947caaaefee 100644 --- a/lib/gitlab/import_export/shared.rb +++ b/lib/gitlab/import_export/shared.rb @@ -8,6 +8,7 @@ module Gitlab def initialize(project) @project = project @errors = [] + @logger = Gitlab::Import::Logger.build end def active_export_count @@ -23,19 +24,16 @@ module Gitlab end def error(error) - error_out(error.message, caller[0].dup) - add_error_message(error.message) + log_error(message: error.message, caller: caller[0].dup) + log_debug(backtrace: error.backtrace&.join("\n")) - # Debug: - if error.backtrace - Rails.logger.error("Import/Export backtrace: #{error.backtrace.join("\n")}") - else - Rails.logger.error("No backtrace found") - end + Gitlab::Sentry.track_acceptable_exception(error, extra: log_base_data) + + add_error_message(error.message) end - def add_error_message(error_message) - @errors << error_message + def add_error_message(message) + @errors << filtered_error_message(message) end def after_export_in_progress? @@ -52,8 +50,25 @@ module Gitlab @project.disk_path end - def error_out(message, caller) - Rails.logger.error("Import/Export error raised on #{caller}: #{message}") + def log_error(details) + @logger.error(log_base_data.merge(details)) + end + + def log_debug(details) + @logger.debug(log_base_data.merge(details)) + end + + def log_base_data + { + importer: 'Import/Export', + import_jid: @project&.import_state&.import_jid, + project_id: @project&.id, + project_path: @project&.full_path + } + end + + def filtered_error_message(message) + Projects::ImportErrorFilter.filter_message(message) end def after_export_lock_file diff --git a/spec/lib/gitlab/import_export/shared_spec.rb b/spec/lib/gitlab/import_export/shared_spec.rb new file mode 100644 index 00000000000..f2d750c6595 --- /dev/null +++ b/spec/lib/gitlab/import_export/shared_spec.rb @@ -0,0 +1,31 @@ +require 'spec_helper' +require 'fileutils' + +describe Gitlab::ImportExport::Shared do + let(:project) { build(:project) } + subject { project.import_export_shared } + + describe '#error' do + let(:error) { StandardError.new('Error importing into /my/folder Permission denied @ unlink_internal - /var/opt/gitlab/gitlab-rails/shared/a/b/c/uploads/file') } + + it 'filters any full paths' do + subject.error(error) + + expect(subject.errors).to eq(['Error importing into [FILTERED] Permission denied @ unlink_internal - [FILTERED]']) + end + + it 'calls the error logger with the full message' do + expect(subject).to receive(:log_error).with(hash_including(message: error.message)) + + subject.error(error) + end + + it 'calls the debug logger with a backtrace' do + error.set_backtrace('backtrace') + + expect(subject).to receive(:log_debug).with(hash_including(backtrace: 'backtrace')) + + subject.error(error) + end + end +end diff --git a/spec/lib/gitlab/import_export/version_checker_spec.rb b/spec/lib/gitlab/import_export/version_checker_spec.rb index 49d857d9483..76f8253ec9b 100644 --- a/spec/lib/gitlab/import_export/version_checker_spec.rb +++ b/spec/lib/gitlab/import_export/version_checker_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' include ImportExport::CommonUtil describe Gitlab::ImportExport::VersionChecker do - let(:shared) { Gitlab::ImportExport::Shared.new(nil) } + let!(:shared) { Gitlab::ImportExport::Shared.new(nil) } describe 'bundle a project Git repo' do let(:version) { Gitlab::ImportExport.version } diff --git a/spec/services/projects/import_error_filter_spec.rb b/spec/services/projects/import_error_filter_spec.rb new file mode 100644 index 00000000000..312b658de89 --- /dev/null +++ b/spec/services/projects/import_error_filter_spec.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Projects::ImportErrorFilter do + it 'filters any full paths' do + message = 'Error importing into /my/folder Permission denied @ unlink_internal - /var/opt/gitlab/gitlab-rails/shared/a/b/c/uploads/file' + + expect(described_class.filter_message(message)).to eq('Error importing into [FILTERED] Permission denied @ unlink_internal - [FILTERED]') + end + + it 'filters any relative paths ignoring single slash ones' do + message = 'Error importing into my/project Permission denied @ unlink_internal - ../file/ and folder/../file' + + expect(described_class.filter_message(message)).to eq('Error importing into [FILTERED] Permission denied @ unlink_internal - [FILTERED] and [FILTERED]') + end +end diff --git a/spec/services/projects/import_service_spec.rb b/spec/services/projects/import_service_spec.rb index 06f865dc848..a005defb3ee 100644 --- a/spec/services/projects/import_service_spec.rb +++ b/spec/services/projects/import_service_spec.rb @@ -136,12 +136,12 @@ describe Projects::ImportService do end it 'fails if repository import fails' do - expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).and_raise(Gitlab::Shell::Error.new('Failed to import the repository')) + expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).and_raise(Gitlab::Shell::Error.new('Failed to import the repository /a/b/c')) result = subject.execute expect(result[:status]).to eq :error - expect(result[:message]).to eq "Error importing repository #{project.safe_import_url} into #{project.full_path} - Failed to import the repository" + expect(result[:message]).to eq "Error importing repository #{project.safe_import_url} into #{project.full_path} - Failed to import the repository [FILTERED]" end context 'when repository import scheduled' do From 7084d71e781d9893fe4c24e45af434e2ca511fdd Mon Sep 17 00:00:00 2001 From: James Lopez Date: Fri, 21 Dec 2018 15:26:33 +0100 Subject: [PATCH 02/42] Fix contributed projects finder shown private info --- app/finders/contributed_projects_finder.rb | 7 ++++ .../security-contributed-projects.yml | 5 +++ spec/controllers/users_controller_spec.rb | 32 +++++++++++++++++++ .../contributed_projects_finder_spec.rb | 12 +++++++ 4 files changed, 56 insertions(+) create mode 100644 changelogs/unreleased/security-contributed-projects.yml diff --git a/app/finders/contributed_projects_finder.rb b/app/finders/contributed_projects_finder.rb index c1ef9dfefa7..f8c7f0c3167 100644 --- a/app/finders/contributed_projects_finder.rb +++ b/app/finders/contributed_projects_finder.rb @@ -14,6 +14,9 @@ class ContributedProjectsFinder < UnionFinder # Returns an ActiveRecord::Relation. # rubocop: disable CodeReuse/ActiveRecord def execute(current_user = nil) + # Do not show contributed projects if the user profile is private. + return Project.none unless can_read_profile?(current_user) + segments = all_projects(current_user) find_union(segments, Project).includes(:namespace).order_id_desc @@ -22,6 +25,10 @@ class ContributedProjectsFinder < UnionFinder private + def can_read_profile?(current_user) + Ability.allowed?(current_user, :read_user_profile, @user) + end + def all_projects(current_user) projects = [] diff --git a/changelogs/unreleased/security-contributed-projects.yml b/changelogs/unreleased/security-contributed-projects.yml new file mode 100644 index 00000000000..f745a2255ca --- /dev/null +++ b/changelogs/unreleased/security-contributed-projects.yml @@ -0,0 +1,5 @@ +--- +title: Fix contributed projects info still visible when user enable private profile +merge_request: +author: +type: security diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 27edf226ca3..af61026098b 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -206,6 +206,38 @@ describe UsersController do end end + describe 'GET #contributed' do + let(:project) { create(:project, :public) } + let(:current_user) { create(:user) } + + before do + sign_in(current_user) + + project.add_developer(public_user) + project.add_developer(private_user) + end + + context 'with public profile' do + it 'renders contributed projects' do + create(:push_event, project: project, author: public_user) + + get :contributed, params: { username: public_user.username } + + expect(assigns[:contributed_projects]).not_to be_empty + end + end + + context 'with private profile' do + it 'does not render contributed projects' do + create(:push_event, project: project, author: private_user) + + get :contributed, params: { username: private_user.username } + + expect(assigns[:contributed_projects]).to be_empty + end + end + end + describe 'GET #snippets' do before do sign_in(user) diff --git a/spec/finders/contributed_projects_finder_spec.rb b/spec/finders/contributed_projects_finder_spec.rb index 81fb4e3561c..ee84fd067d4 100644 --- a/spec/finders/contributed_projects_finder_spec.rb +++ b/spec/finders/contributed_projects_finder_spec.rb @@ -31,4 +31,16 @@ describe ContributedProjectsFinder do it { is_expected.to match_array([private_project, internal_project, public_project]) } end + + context 'user with private profile' do + it 'does not return contributed projects' do + private_user = create(:user, private_profile: true) + public_project.add_maintainer(private_user) + create(:push_event, project: public_project, author: private_user) + + projects = described_class.new(private_user).execute(current_user) + + expect(projects).to be_empty + end + end end From 81fee3617ab9005aa07d31308630e17330b8fd8b Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Wed, 9 Jan 2019 17:24:05 -0200 Subject: [PATCH 03/42] Don't process MR refs for guests in the notes --- app/policies/project_policy.rb | 2 +- .../security-do-not-process-mr-ref-for-guests.yml | 5 +++++ spec/policies/project_policy_spec.rb | 12 +++++++++++- 3 files changed, 17 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/security-do-not-process-mr-ref-for-guests.yml diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index d70417e710e..e801900e981 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -392,7 +392,7 @@ class ProjectPolicy < BasePolicy end.enable :read_issue_iid rule do - (can?(:read_project_for_iids) & merge_requests_visible_to_user) | can?(:read_merge_request) + (~guest & can?(:read_project_for_iids) & merge_requests_visible_to_user) | can?(:read_merge_request) end.enable :read_merge_request_iid rule { ~can_have_multiple_clusters & has_clusters }.prevent :add_cluster diff --git a/changelogs/unreleased/security-do-not-process-mr-ref-for-guests.yml b/changelogs/unreleased/security-do-not-process-mr-ref-for-guests.yml new file mode 100644 index 00000000000..0281dde11e6 --- /dev/null +++ b/changelogs/unreleased/security-do-not-process-mr-ref-for-guests.yml @@ -0,0 +1,5 @@ +--- +title: Don't process MR refs for guests in the notes +merge_request: 2771 +author: +type: security diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index 9cb20854f6e..3f87d5eef4b 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -12,7 +12,7 @@ describe ProjectPolicy do let(:base_guest_permissions) do %i[ read_project read_board read_list read_wiki read_issue - read_project_for_iids read_issue_iid read_merge_request_iid read_label + read_project_for_iids read_issue_iid read_label read_milestone read_project_snippet read_project_member read_note create_project create_issue create_note upload_file create_merge_request_in award_emoji read_release @@ -152,6 +152,16 @@ describe ProjectPolicy do end end + context 'for a guest in a private project' do + let(:project) { create(:project, :private) } + subject { described_class.new(guest, project) } + + it 'disallows the guest from reading the merge request and merge request iid' do + expect_disallowed(:read_merge_request) + expect_disallowed(:read_merge_request_iid) + end + end + context 'builds feature' do subject { described_class.new(owner, project) } From 25da6e56d7d1a02cc411d044816ec4afcc9f1fb2 Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Wed, 9 Jan 2019 16:55:29 +0800 Subject: [PATCH 04/42] Fix slow project reference pattern regex --- app/models/project.rb | 1 + changelogs/unreleased/security-fix-regex-dos.yml | 5 +++++ lib/gitlab/path_regex.rb | 3 ++- spec/lib/banzai/filter/project_reference_filter_spec.rb | 6 ++++++ 4 files changed, 14 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/security-fix-regex-dos.yml diff --git a/app/models/project.rb b/app/models/project.rb index cab173503ce..c105a41b080 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -530,6 +530,7 @@ class Project < ActiveRecord::Base def reference_pattern %r{ + (?#{Gitlab::PathRegex::FULL_NAMESPACE_FORMAT_REGEX})\/)? (?#{Gitlab::PathRegex::PROJECT_PATH_FORMAT_REGEX}) }x diff --git a/changelogs/unreleased/security-fix-regex-dos.yml b/changelogs/unreleased/security-fix-regex-dos.yml new file mode 100644 index 00000000000..b08566d2f15 --- /dev/null +++ b/changelogs/unreleased/security-fix-regex-dos.yml @@ -0,0 +1,5 @@ +--- +title: Fix slow regex in project reference pattern +merge_request: +author: +type: security diff --git a/lib/gitlab/path_regex.rb b/lib/gitlab/path_regex.rb index fa68dead80b..3c888be0710 100644 --- a/lib/gitlab/path_regex.rb +++ b/lib/gitlab/path_regex.rb @@ -125,7 +125,8 @@ module Gitlab # allow non-regex validations, etc), `NAMESPACE_FORMAT_REGEX_JS` serves as a Javascript-compatible version of # `NAMESPACE_FORMAT_REGEX`, with the negative lookbehind assertion removed. This means that the client-side validation # will pass for usernames ending in `.atom` and `.git`, but will be caught by the server-side validation. - PATH_REGEX_STR = '[a-zA-Z0-9_\.][a-zA-Z0-9_\-\.]*'.freeze + PATH_START_CHAR = '[a-zA-Z0-9_\.]'.freeze + PATH_REGEX_STR = PATH_START_CHAR + '[a-zA-Z0-9_\-\.]*'.freeze NAMESPACE_FORMAT_REGEX_JS = PATH_REGEX_STR + '[a-zA-Z0-9_\-]|[a-zA-Z0-9_]'.freeze NO_SUFFIX_REGEX = /(? character' do doc = reference_filter("Hey #{reference}foo") expect(doc.css('a').first.attr('href')).to eq urls.project_url(subject) From b5a06d2934585fd7e1206d58b0384eb22585fa6a Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Mon, 14 Jan 2019 13:33:40 +0800 Subject: [PATCH 05/42] Use common error for unauthenticated users Removes special error message when creating new issues --- app/controllers/projects/issues_controller.rb | 10 +--------- .../security-fix-new-issues-login-message.yml | 5 +++++ spec/controllers/projects/issues_controller_spec.rb | 2 +- 3 files changed, 7 insertions(+), 10 deletions(-) create mode 100644 changelogs/unreleased/security-fix-new-issues-login-message.yml diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 21688e54481..4d6bcb1de6a 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -19,7 +19,7 @@ class Projects::IssuesController < Projects::ApplicationController prepend_before_action(only: [:index]) { authenticate_sessionless_user!(:rss) } prepend_before_action(only: [:calendar]) { authenticate_sessionless_user!(:ics) } - prepend_before_action :authenticate_new_issue!, only: [:new] + prepend_before_action :authenticate_user!, only: [:new] prepend_before_action :store_uri, only: [:new, :show] before_action :whitelist_query_limiting, only: [:create, :create_merge_request, :move, :bulk_update] @@ -247,14 +247,6 @@ class Projects::IssuesController < Projects::ApplicationController ] + [{ label_ids: [], assignee_ids: [] }] end - def authenticate_new_issue! - return if current_user - - notice = "Please sign in to create the new issue." - - redirect_to new_user_session_path, notice: notice - end - def store_uri if request.get? && !request.xhr? store_location_for :user, request.fullpath diff --git a/changelogs/unreleased/security-fix-new-issues-login-message.yml b/changelogs/unreleased/security-fix-new-issues-login-message.yml new file mode 100644 index 00000000000..9dabf2438c9 --- /dev/null +++ b/changelogs/unreleased/security-fix-new-issues-login-message.yml @@ -0,0 +1,5 @@ +--- +title: Use common error for unauthenticated users when creating issues +merge_request: +author: +type: security diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index df21dc7bc85..c250c6297e2 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -131,7 +131,7 @@ describe Projects::IssuesController do it 'redirects to signin if not logged in' do get :new, params: { namespace_id: project.namespace, project_id: project } - expect(flash[:notice]).to eq 'Please sign in to create the new issue.' + expect(flash[:alert]).to eq 'You need to sign in or sign up before continuing.' expect(response).to redirect_to(new_user_session_path) end From 71729b38c54bafb440c3a588bc88d8e6346654ce Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Tue, 15 Jan 2019 15:17:21 +1300 Subject: [PATCH 06/42] Prefer build() rather than create() --- spec/lib/gitlab/data_builder/push_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/lib/gitlab/data_builder/push_spec.rb b/spec/lib/gitlab/data_builder/push_spec.rb index befdc18d1aa..d70438ba27c 100644 --- a/spec/lib/gitlab/data_builder/push_spec.rb +++ b/spec/lib/gitlab/data_builder/push_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe Gitlab::DataBuilder::Push do let(:project) { create(:project, :repository) } - let(:user) { create(:user) } + let(:user) { build(:user) } describe '.build_sample' do let(:data) { described_class.build_sample(project, user) } From a2338de00c8723a1e14068cef198b20f1e20ab82 Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Tue, 15 Jan 2019 16:21:28 +0800 Subject: [PATCH 07/42] Prevent award_emoji to notes not visible to user When the parent noteable is not visible to the user (e.g. confidential) we prevent the user from adding emoji reactions to notes --- app/policies/note_policy.rb | 1 + .../security-2776-fix-add-reaction-permissions.yml | 5 +++++ spec/policies/note_policy_spec.rb | 2 ++ 3 files changed, 8 insertions(+) create mode 100644 changelogs/unreleased/security-2776-fix-add-reaction-permissions.yml diff --git a/app/policies/note_policy.rb b/app/policies/note_policy.rb index f22843b6463..8d23e3abed3 100644 --- a/app/policies/note_policy.rb +++ b/app/policies/note_policy.rb @@ -18,6 +18,7 @@ class NotePolicy < BasePolicy prevent :read_note prevent :admin_note prevent :resolve_note + prevent :award_emoji end rule { is_author }.policy do diff --git a/changelogs/unreleased/security-2776-fix-add-reaction-permissions.yml b/changelogs/unreleased/security-2776-fix-add-reaction-permissions.yml new file mode 100644 index 00000000000..3ad92578c44 --- /dev/null +++ b/changelogs/unreleased/security-2776-fix-add-reaction-permissions.yml @@ -0,0 +1,5 @@ +--- +title: Prevent awarding emojis to notes whose parent is not visible to user +merge_request: +author: +type: security diff --git a/spec/policies/note_policy_spec.rb b/spec/policies/note_policy_spec.rb index 7e25c53e77c..0e848c74659 100644 --- a/spec/policies/note_policy_spec.rb +++ b/spec/policies/note_policy_spec.rb @@ -28,6 +28,7 @@ describe NotePolicy, mdoels: true do expect(policy).to be_disallowed(:admin_note) expect(policy).to be_disallowed(:resolve_note) expect(policy).to be_disallowed(:read_note) + expect(policy).to be_disallowed(:award_emoji) end end @@ -40,6 +41,7 @@ describe NotePolicy, mdoels: true do expect(policy).to be_allowed(:admin_note) expect(policy).to be_allowed(:resolve_note) expect(policy).to be_allowed(:read_note) + expect(policy).to be_allowed(:award_emoji) end end end From 4d7f93519ca82cca0ed3a4ed2165b41fe254368f Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 19 Dec 2018 14:15:58 +0100 Subject: [PATCH 08/42] Do not expose trigger token when user should not see it --- .../projects/triggers_controller.rb | 7 ++--- app/models/ci/trigger.rb | 1 + app/presenters/ci/trigger_presenter.rb | 19 ++++++++++++ .../projects/triggers/_trigger.html.haml | 2 +- lib/api/entities.rb | 5 +++- lib/api/helpers/presentable.rb | 29 +++++++++++++++++++ lib/api/triggers.rb | 4 +-- spec/requests/api/triggers_spec.rb | 14 +++++---- 8 files changed, 68 insertions(+), 13 deletions(-) create mode 100644 app/presenters/ci/trigger_presenter.rb create mode 100644 lib/api/helpers/presentable.rb diff --git a/app/controllers/projects/triggers_controller.rb b/app/controllers/projects/triggers_controller.rb index f5fdfb8accc..c7b4ebb2b24 100644 --- a/app/controllers/projects/triggers_controller.rb +++ b/app/controllers/projects/triggers_controller.rb @@ -66,12 +66,11 @@ class Projects::TriggersController < Projects::ApplicationController end def trigger - @trigger ||= project.triggers.find(params[:id]) || render_404 + @trigger ||= project.triggers.find(params[:id]) + .present(current_user: current_user) end def trigger_params - params.require(:trigger).permit( - :description - ) + params.require(:trigger).permit(:description) end end diff --git a/app/models/ci/trigger.rb b/app/models/ci/trigger.rb index 55db42162ca..3a9cdfcc35e 100644 --- a/app/models/ci/trigger.rb +++ b/app/models/ci/trigger.rb @@ -4,6 +4,7 @@ module Ci class Trigger < ActiveRecord::Base extend Gitlab::Ci::Model include IgnorableColumn + include Presentable ignore_column :deleted_at diff --git a/app/presenters/ci/trigger_presenter.rb b/app/presenters/ci/trigger_presenter.rb new file mode 100644 index 00000000000..605c8f328a4 --- /dev/null +++ b/app/presenters/ci/trigger_presenter.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module Ci + class TriggerPresenter < Gitlab::View::Presenter::Delegated + presents :trigger + + def has_token_exposed? + can?(current_user, :admin_trigger, trigger) + end + + def token + if has_token_exposed? + trigger.token + else + trigger.short_token + end + end + end +end diff --git a/app/views/projects/triggers/_trigger.html.haml b/app/views/projects/triggers/_trigger.html.haml index 7e4618e1a88..6f6f1e5e0c5 100644 --- a/app/views/projects/triggers/_trigger.html.haml +++ b/app/views/projects/triggers/_trigger.html.haml @@ -1,6 +1,6 @@ %tr %td - - if can?(current_user, :admin_trigger, trigger) + - if trigger.has_token_exposed? %span= trigger.token = clipboard_button(text: trigger.token, title: "Copy trigger token to clipboard") - else diff --git a/lib/api/entities.rb b/lib/api/entities.rb index a2a3c0a16d7..829d6fb13d4 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -1216,8 +1216,11 @@ module API end class Trigger < Grape::Entity + include ::API::Helpers::Presentable + expose :id - expose :token, :description + expose :token + expose :description expose :created_at, :updated_at, :last_used expose :owner, using: Entities::UserBasic end diff --git a/lib/api/helpers/presentable.rb b/lib/api/helpers/presentable.rb new file mode 100644 index 00000000000..973c2132efe --- /dev/null +++ b/lib/api/helpers/presentable.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module API + module Helpers + ## + # This module makes it possible to use `app/presenters` with + # Grape Entities. It instantiates model presenter and passes + # options defined in the API endpoint to the presenter itself. + # + # present object, with: Entities::Something, + # current_user: current_user, + # another_option: 'my options' + # + # Example above will make `current_user` and `another_option` + # values available in the subclass of `Gitlab::View::Presenter` + # thorough a separate method in the presenter. + # + # The model class needs to have `::Presentable` module mixed in + # if you want to use `API::Helpers::Presentable`. + # + module Presentable + extend ActiveSupport::Concern + + def initialize(object, options = {}) + super(object.present(options), options) + end + end + end +end diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index 3ce1529f259..7b8691171f0 100644 --- a/lib/api/triggers.rb +++ b/lib/api/triggers.rb @@ -51,7 +51,7 @@ module API triggers = user_project.triggers.includes(:trigger_requests) - present paginate(triggers), with: Entities::Trigger + present paginate(triggers), with: Entities::Trigger, current_user: current_user end # rubocop: enable CodeReuse/ActiveRecord @@ -68,7 +68,7 @@ module API trigger = user_project.triggers.find(params.delete(:trigger_id)) break not_found!('Trigger') unless trigger - present trigger, with: Entities::Trigger + present trigger, with: Entities::Trigger, current_user: current_user end desc 'Create a trigger' do diff --git a/spec/requests/api/triggers_spec.rb b/spec/requests/api/triggers_spec.rb index 15dc901d06e..f0f01e97f1d 100644 --- a/spec/requests/api/triggers_spec.rb +++ b/spec/requests/api/triggers_spec.rb @@ -1,8 +1,9 @@ require 'spec_helper' describe API::Triggers do - let(:user) { create(:user) } - let(:user2) { create(:user) } + set(:user) { create(:user) } + set(:user2) { create(:user) } + let!(:trigger_token) { 'secure_token' } let!(:trigger_token_2) { 'secure_token_2' } let!(:project) { create(:project, :repository, creator: user) } @@ -132,14 +133,17 @@ describe API::Triggers do end describe 'GET /projects/:id/triggers' do - context 'authenticated user with valid permissions' do - it 'returns list of triggers' do + context 'authenticated user who can access triggers' do + it 'returns a list of triggers with tokens exposed correctly' do get api("/projects/#{project.id}/triggers", user) expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers + expect(json_response).to be_a(Array) - expect(json_response[0]).to have_key('token') + expect(json_response.size).to eq 2 + expect(json_response.dig(0, 'token')).to eq trigger_token + expect(json_response.dig(1, 'token')).to eq trigger_token_2[0..3] end end From 69486116331bb262687ade4dfd4ab0619fad4063 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 19 Dec 2018 15:51:02 +0100 Subject: [PATCH 09/42] Present all pipeline triggers using trigger presenter --- app/controllers/projects/settings/ci_cd_controller.rb | 2 ++ app/models/ci/trigger.rb | 2 +- lib/api/triggers.rb | 6 +++--- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/app/controllers/projects/settings/ci_cd_controller.rb b/app/controllers/projects/settings/ci_cd_controller.rb index 75e590f3f33..f2f63e986bb 100644 --- a/app/controllers/projects/settings/ci_cd_controller.rb +++ b/app/controllers/projects/settings/ci_cd_controller.rb @@ -99,7 +99,9 @@ module Projects def define_triggers_variables @triggers = @project.triggers + .present(current_user: current_user) @trigger = ::Ci::Trigger.new + .present(current_user: current_user) end def define_badges_variables diff --git a/app/models/ci/trigger.rb b/app/models/ci/trigger.rb index 3a9cdfcc35e..637148c4ce4 100644 --- a/app/models/ci/trigger.rb +++ b/app/models/ci/trigger.rb @@ -30,7 +30,7 @@ module Ci end def short_token - token[0...4] + token[0...4] if token.present? end def legacy? diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index 7b8691171f0..b67f056f491 100644 --- a/lib/api/triggers.rb +++ b/lib/api/triggers.rb @@ -85,7 +85,7 @@ module API declared_params(include_missing: false).merge(owner: current_user)) if trigger.valid? - present trigger, with: Entities::Trigger + present trigger, with: Entities::Trigger, current_user: current_user else render_validation_error!(trigger) end @@ -106,7 +106,7 @@ module API break not_found!('Trigger') unless trigger if trigger.update(declared_params(include_missing: false)) - present trigger, with: Entities::Trigger + present trigger, with: Entities::Trigger, current_user: current_user else render_validation_error!(trigger) end @@ -127,7 +127,7 @@ module API if trigger.update(owner: current_user) status :ok - present trigger, with: Entities::Trigger + present trigger, with: Entities::Trigger, current_user: current_user else render_validation_error!(trigger) end From ba8a1ec92c783dae6f050e80c491bd1a42e1023d Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 20 Dec 2018 14:37:37 +0100 Subject: [PATCH 10/42] Add some specs for trigger presenter --- spec/presenters/ci/trigger_presenter_spec.rb | 51 ++++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 spec/presenters/ci/trigger_presenter_spec.rb diff --git a/spec/presenters/ci/trigger_presenter_spec.rb b/spec/presenters/ci/trigger_presenter_spec.rb new file mode 100644 index 00000000000..a589759f379 --- /dev/null +++ b/spec/presenters/ci/trigger_presenter_spec.rb @@ -0,0 +1,51 @@ +require 'spec_helper' + +describe Ci::TriggerPresenter do + set(:user) { create(:user) } + set(:project) { create(:project) } + + set(:trigger) do + create(:ci_trigger, token: '123456789abcd', project: project) + end + + let(:subject) do + described_class.new(trigger, current_user: user) + end + + before do + project.add_maintainer(user) + end + + context 'when user is not a trigger owner' do + describe '#token' do + it 'exposes only short token' do + expect(subject.token).not_to eq trigger.token + expect(subject.token).to eq '1234' + end + end + + describe '#has_token_exposed?' do + it 'does not have token exposed' do + expect(subject).not_to have_token_exposed + end + end + end + + context 'when user is a trigger owner and builds admin' do + before do + trigger.update(owner: user) + end + + describe '#token' do + it 'exposes full token' do + expect(subject.token).to eq trigger.token + end + end + + describe '#has_token_exposed?' do + it 'has token exposed' do + expect(subject).to have_token_exposed + end + end + end +end From 0001066c5c9adc33abfd6fe5aa7422cb7479862d Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 3 Jan 2019 10:54:05 +0100 Subject: [PATCH 11/42] Fix subject in trigger presenter tests --- spec/presenters/ci/trigger_presenter_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/presenters/ci/trigger_presenter_spec.rb b/spec/presenters/ci/trigger_presenter_spec.rb index a589759f379..231b539c188 100644 --- a/spec/presenters/ci/trigger_presenter_spec.rb +++ b/spec/presenters/ci/trigger_presenter_spec.rb @@ -8,7 +8,7 @@ describe Ci::TriggerPresenter do create(:ci_trigger, token: '123456789abcd', project: project) end - let(:subject) do + subject do described_class.new(trigger, current_user: user) end From 63e5a314b4f149fad39d40f898e13f705340cc22 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 3 Jan 2019 11:32:58 +0100 Subject: [PATCH 12/42] Add changelog for trigger token exposure fix --- .../unreleased/security-pipeline-trigger-tokens-exposure.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/security-pipeline-trigger-tokens-exposure.yml diff --git a/changelogs/unreleased/security-pipeline-trigger-tokens-exposure.yml b/changelogs/unreleased/security-pipeline-trigger-tokens-exposure.yml new file mode 100644 index 00000000000..97d743eead1 --- /dev/null +++ b/changelogs/unreleased/security-pipeline-trigger-tokens-exposure.yml @@ -0,0 +1,5 @@ +--- +title: Expose CI/CD trigger token only to the trigger owner +merge_request: +author: +type: security From c3cfffbc6fcb0bdcbc45e7670a041b2a38f1b6f1 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Tue, 15 Jan 2019 15:11:04 +1300 Subject: [PATCH 13/42] Fix private user email being visible in tag webhooks Fixes #54721 --- .../unreleased/security-fix-user-email-tag-push-leak.yml | 5 +++++ lib/gitlab/data_builder/push.rb | 2 +- spec/lib/gitlab/data_builder/push_spec.rb | 4 ++-- 3 files changed, 8 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/security-fix-user-email-tag-push-leak.yml diff --git a/changelogs/unreleased/security-fix-user-email-tag-push-leak.yml b/changelogs/unreleased/security-fix-user-email-tag-push-leak.yml new file mode 100644 index 00000000000..915ea7b5216 --- /dev/null +++ b/changelogs/unreleased/security-fix-user-email-tag-push-leak.yml @@ -0,0 +1,5 @@ +--- +title: Fix private user email being visible in push (and tag push) webhooks +merge_request: +author: +type: security diff --git a/lib/gitlab/data_builder/push.rb b/lib/gitlab/data_builder/push.rb index 862127110b9..ea08b5f7eae 100644 --- a/lib/gitlab/data_builder/push.rb +++ b/lib/gitlab/data_builder/push.rb @@ -93,7 +93,7 @@ module Gitlab user_id: user.id, user_name: user.name, user_username: user.username, - user_email: user.email, + user_email: user.public_email, user_avatar: user.avatar_url(only_path: false), project_id: project.id, project: project.hook_attrs, diff --git a/spec/lib/gitlab/data_builder/push_spec.rb b/spec/lib/gitlab/data_builder/push_spec.rb index d70438ba27c..0c4decc6518 100644 --- a/spec/lib/gitlab/data_builder/push_spec.rb +++ b/spec/lib/gitlab/data_builder/push_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe Gitlab::DataBuilder::Push do let(:project) { create(:project, :repository) } - let(:user) { build(:user) } + let(:user) { build(:user, public_email: 'public-email@example.com') } describe '.build_sample' do let(:data) { described_class.build_sample(project, user) } @@ -36,7 +36,7 @@ describe Gitlab::DataBuilder::Push do it { expect(data[:user_id]).to eq(user.id) } it { expect(data[:user_name]).to eq(user.name) } it { expect(data[:user_username]).to eq(user.username) } - it { expect(data[:user_email]).to eq(user.email) } + it { expect(data[:user_email]).to eq(user.public_email) } it { expect(data[:user_avatar]).to eq(user.avatar_url) } it { expect(data[:project_id]).to eq(project.id) } it { expect(data[:project]).to be_a(Hash) } From e5c0ee815fcd7b073e52882e0fd10a6dcb5369bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20L=C3=B3pez?= Date: Wed, 9 Jan 2019 16:05:52 +0100 Subject: [PATCH 14/42] Fixed bug when external wiki is enabled When the external wiki is enabled, the internal wiki link is replaced by the external wiki url. But the internal wiki is still accessible. In this change the external wiki will have its own tab in the sidebar and only if the services are disabled the tab (and access rights) will not be displayed. --- app/helpers/external_wiki_helper.rb | 12 ----- app/helpers/projects_helper.rb | 13 +++-- app/policies/project_policy.rb | 2 +- .../layouts/nav/sidebar/_project.html.haml | 21 ++++++-- .../projects/blob/viewers/_readme.html.haml | 2 +- app/views/projects/wikis/pages.html.haml | 2 +- app/views/projects/wikis/show.html.haml | 2 +- ...cess-rights-with-external-wiki-enabled.yml | 5 ++ locale/gitlab.pot | 3 ++ spec/helpers/projects_helper_spec.rb | 20 +++++++ spec/models/ability_spec.rb | 1 - .../external_wiki_service_spec.rb | 21 -------- spec/policies/project_policy_spec.rb | 24 ++++++--- .../nav/sidebar/_project.html.haml_spec.rb | 54 +++++++++++++++++++ 14 files changed, 131 insertions(+), 51 deletions(-) delete mode 100644 app/helpers/external_wiki_helper.rb create mode 100644 changelogs/unreleased/security-fix-wiki-access-rights-with-external-wiki-enabled.yml diff --git a/app/helpers/external_wiki_helper.rb b/app/helpers/external_wiki_helper.rb deleted file mode 100644 index e36d63b2946..00000000000 --- a/app/helpers/external_wiki_helper.rb +++ /dev/null @@ -1,12 +0,0 @@ -# frozen_string_literal: true - -module ExternalWikiHelper - def get_project_wiki_path(project) - external_wiki_service = project.external_wiki - if external_wiki_service - external_wiki_service.properties['external_wiki_url'] - else - project_wiki_path(project, :home) - end - end -end diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index e67c327f7f8..085debcaf0e 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -313,19 +313,24 @@ module ProjectsHelper nav_tabs << :operations end - if project.external_issue_tracker - nav_tabs << :external_issue_tracker - end - tab_ability_map.each do |tab, ability| if can?(current_user, ability, project) nav_tabs << tab end end + nav_tabs << external_nav_tabs(project) + nav_tabs.flatten end + def external_nav_tabs(project) + [].tap do |tabs| + tabs << :external_issue_tracker if project.external_issue_tracker + tabs << :external_wiki if project.has_external_wiki? + end + end + def tab_ability_map { environments: :read_environment, diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 3146f26bed5..c336d5907d9 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -311,7 +311,7 @@ class ProjectPolicy < BasePolicy prevent(*create_read_update_admin_destroy(:project_snippet)) end - rule { wiki_disabled & ~has_external_wiki }.policy do + rule { wiki_disabled }.policy do prevent(*create_read_update_admin_destroy(:wiki)) prevent(:download_wiki_code) end diff --git a/app/views/layouts/nav/sidebar/_project.html.haml b/app/views/layouts/nav/sidebar/_project.html.haml index e516c76400a..2321eebf1f3 100644 --- a/app/views/layouts/nav/sidebar/_project.html.haml +++ b/app/views/layouts/nav/sidebar/_project.html.haml @@ -275,19 +275,34 @@ %strong.fly-out-top-item-name = _('Registry') - - if project_nav_tab? :wiki + - if project_nav_tab?(:wiki) + - wiki_url = project_wiki_path(@project, :home) = nav_link(controller: :wikis) do - = link_to get_project_wiki_path(@project), class: 'shortcuts-wiki' do + = link_to wiki_url, class: 'shortcuts-wiki' do .nav-icon-container = sprite_icon('book') %span.nav-item-name = _('Wiki') %ul.sidebar-sub-level-items.is-fly-out-only = nav_link(controller: :wikis, html_options: { class: "fly-out-top-item" } ) do - = link_to get_project_wiki_path(@project) do + = link_to wiki_url do %strong.fly-out-top-item-name = _('Wiki') + - if project_nav_tab?(:external_wiki) + - external_wiki_url = @project.external_wiki.external_wiki_url + = nav_link do + = link_to external_wiki_url, class: 'shortcuts-external_wiki' do + .nav-icon-container + = sprite_icon('issue-external') + %span.nav-item-name + = _('External Wiki') + %ul.sidebar-sub-level-items.is-fly-out-only + = nav_link(html_options: { class: "fly-out-top-item" } ) do + = link_to external_wiki_url do + %strong.fly-out-top-item-name + = _('External Wiki') + - if project_nav_tab? :snippets = nav_link(controller: :snippets) do = link_to project_snippets_path(@project), class: 'shortcuts-snippets' do diff --git a/app/views/projects/blob/viewers/_readme.html.haml b/app/views/projects/blob/viewers/_readme.html.haml index d8492abc638..c2329a7aa66 100644 --- a/app/views/projects/blob/viewers/_readme.html.haml +++ b/app/views/projects/blob/viewers/_readme.html.haml @@ -1,4 +1,4 @@ = icon('info-circle fw') = succeed '.' do To learn more about this project, read - = link_to "the wiki", get_project_wiki_path(viewer.project) + = link_to "the wiki", project_wiki_path(viewer.project, :home) diff --git a/app/views/projects/wikis/pages.html.haml b/app/views/projects/wikis/pages.html.haml index aeef64fd7eb..94267b6e0cf 100644 --- a/app/views/projects/wikis/pages.html.haml +++ b/app/views/projects/wikis/pages.html.haml @@ -1,5 +1,5 @@ - @no_container = true -- add_to_breadcrumbs "Wiki", get_project_wiki_path(@project) +- add_to_breadcrumbs "Wiki", project_wiki_path(@project, :home) - breadcrumb_title s_("Wiki|Pages") - page_title s_("Wiki|Pages"), _("Wiki") diff --git a/app/views/projects/wikis/show.html.haml b/app/views/projects/wikis/show.html.haml index 4d5fd55364c..8b348bb4e4f 100644 --- a/app/views/projects/wikis/show.html.haml +++ b/app/views/projects/wikis/show.html.haml @@ -2,7 +2,7 @@ - breadcrumb_title @page.human_title - wiki_breadcrumb_dropdown_links(@page.slug) - page_title @page.human_title, _("Wiki") -- add_to_breadcrumbs _("Wiki"), get_project_wiki_path(@project) +- add_to_breadcrumbs _("Wiki"), project_wiki_path(@project, :home) .wiki-page-header.has-sidebar-toggle %button.btn.btn-default.sidebar-toggle.js-sidebar-wiki-toggle{ role: "button", type: "button" } diff --git a/changelogs/unreleased/security-fix-wiki-access-rights-with-external-wiki-enabled.yml b/changelogs/unreleased/security-fix-wiki-access-rights-with-external-wiki-enabled.yml new file mode 100644 index 00000000000..d5f20b87a90 --- /dev/null +++ b/changelogs/unreleased/security-fix-wiki-access-rights-with-external-wiki-enabled.yml @@ -0,0 +1,5 @@ +--- +title: Fix wiki access rights when external wiki is enabled +merge_request: +author: +type: security diff --git a/locale/gitlab.pot b/locale/gitlab.pot index f25f9cc531a..3d376085da0 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -3013,6 +3013,9 @@ msgstr "" msgid "External URL" msgstr "" +msgid "External Wiki" +msgstr "" + msgid "Facebook" msgstr "" diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb index 88b5d87f087..c2dd666f9df 100644 --- a/spec/helpers/projects_helper_spec.rb +++ b/spec/helpers/projects_helper_spec.rb @@ -358,6 +358,26 @@ describe ProjectsHelper do is_expected.not_to include(:pipelines) end end + + context 'when project has external wiki' do + before do + allow(project).to receive(:has_external_wiki?).and_return(true) + end + + it 'includes external wiki tab' do + is_expected.to include(:external_wiki) + end + end + + context 'when project does not have external wiki' do + before do + allow(project).to receive(:has_external_wiki?).and_return(false) + end + + it 'does not include external wiki tab' do + is_expected.not_to include(:external_wiki) + end + end end describe '#show_projects' do diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index 199f49d0bf2..eee80e9bad7 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -298,7 +298,6 @@ describe Ability do context 'wiki named abilities' do it 'disables wiki abilities if the project has no wiki' do - expect(project).to receive(:has_external_wiki?).and_return(false) expect(subject).not_to be_allowed(:read_wiki) expect(subject).not_to be_allowed(:create_wiki) expect(subject).not_to be_allowed(:update_wiki) diff --git a/spec/models/project_services/external_wiki_service_spec.rb b/spec/models/project_services/external_wiki_service_spec.rb index 25e6ce7e804..62fd97b038b 100644 --- a/spec/models/project_services/external_wiki_service_spec.rb +++ b/spec/models/project_services/external_wiki_service_spec.rb @@ -1,7 +1,6 @@ require 'spec_helper' describe ExternalWikiService do - include ExternalWikiHelper describe "Associations" do it { is_expected.to belong_to :project } it { is_expected.to have_one :service_hook } @@ -25,24 +24,4 @@ describe ExternalWikiService do it { is_expected.not_to validate_presence_of(:external_wiki_url) } end end - - describe 'External wiki' do - let(:project) { create(:project) } - - context 'when it is active' do - before do - properties = { 'external_wiki_url' => 'https://gitlab.com' } - @service = project.create_external_wiki_service(active: true, properties: properties) - end - - after do - @service.destroy! - end - - it 'replaces the wiki url' do - wiki_path = get_project_wiki_path(project) - expect(wiki_path).to match('https://gitlab.com') - end - end - end end diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index 9cb20854f6e..54d52b91d61 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -102,15 +102,27 @@ describe ProjectPolicy do expect(Ability).not_to be_allowed(user, :read_issue, project) end - context 'when the feature is disabled' do + context 'wiki feature' do + let(:permissions) { %i(read_wiki create_wiki update_wiki admin_wiki download_wiki_code) } + subject { described_class.new(owner, project) } - before do - project.project_feature.update_attribute(:wiki_access_level, ProjectFeature::DISABLED) - end + context 'when the feature is disabled' do + before do + project.project_feature.update_attribute(:wiki_access_level, ProjectFeature::DISABLED) + end - it 'does not include the wiki permissions' do - expect_disallowed :read_wiki, :create_wiki, :update_wiki, :admin_wiki, :download_wiki_code + it 'does not include the wiki permissions' do + expect_disallowed(*permissions) + end + + context 'when there is an external wiki' do + it 'does not include the wiki permissions' do + allow(project).to receive(:has_external_wiki?).and_return(true) + + expect_disallowed(*permissions) + end + end end end diff --git a/spec/views/layouts/nav/sidebar/_project.html.haml_spec.rb b/spec/views/layouts/nav/sidebar/_project.html.haml_spec.rb index ec20c346234..9d7f1d47598 100644 --- a/spec/views/layouts/nav/sidebar/_project.html.haml_spec.rb +++ b/spec/views/layouts/nav/sidebar/_project.html.haml_spec.rb @@ -75,4 +75,58 @@ describe 'layouts/nav/sidebar/_project' do end end end + + describe 'wiki entry tab' do + let(:can_read_wiki) { true } + + before do + allow(view).to receive(:can?).with(nil, :read_wiki, project).and_return(can_read_wiki) + end + + describe 'when wiki is enabled' do + it 'shows the wiki tab with the wiki internal link' do + render + + expect(rendered).to have_link('Wiki', href: project_wiki_path(project, :home)) + end + end + + describe 'when wiki is disabled' do + let(:can_read_wiki) { false } + + it 'does not show the wiki tab' do + render + + expect(rendered).not_to have_link('Wiki', href: project_wiki_path(project, :home)) + end + end + end + + describe 'external wiki entry tab' do + let(:properties) { { 'external_wiki_url' => 'https://gitlab.com' } } + let(:service_status) { true } + + before do + project.create_external_wiki_service(active: service_status, properties: properties) + project.reload + end + + context 'when it is active' do + it 'shows the external wiki tab with the external wiki service link' do + render + + expect(rendered).to have_link('External Wiki', href: properties['external_wiki_url']) + end + end + + context 'when it is disabled' do + let(:service_status) { false } + + it 'does not show the external wiki tab' do + render + + expect(rendered).not_to have_link('External Wiki', href: project_wiki_path(project, :home)) + end + end + end end From 9ec860ea05d5c74387cbff4593ca76072a38ad5f Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Tue, 11 Dec 2018 14:52:22 +0000 Subject: [PATCH 15/42] Group Guests are no longer able to see merge requests Group guests will only be displayed merge requests to projects they have a access level to, higher than Reporter. Visible projects will still display the merge requests to Guests --- app/models/project.rb | 22 ++++--- app/models/project_feature.rb | 19 +++++- app/models/user.rb | 8 ++- ...-guests-can-see-list-of-merge-requests.yml | 6 ++ spec/finders/merge_requests_finder_spec.rb | 32 +++++++--- spec/models/project_spec.rb | 60 +++++++++++++++++++ spec/models/user_spec.rb | 27 +++++++++ 7 files changed, 154 insertions(+), 20 deletions(-) create mode 100644 changelogs/unreleased/security-guests-can-see-list-of-merge-requests.yml diff --git a/app/models/project.rb b/app/models/project.rb index 15465d9b356..ccf4ab5171b 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -377,8 +377,10 @@ class Project < ActiveRecord::Base # "enabled" here means "not disabled". It includes private features! scope :with_feature_enabled, ->(feature) { - access_level_attribute = ProjectFeature.access_level_attribute(feature) - with_project_feature.where(project_features: { access_level_attribute => [nil, ProjectFeature::PRIVATE, ProjectFeature::ENABLED, ProjectFeature::PUBLIC] }) + access_level_attribute = ProjectFeature.arel_table[ProjectFeature.access_level_attribute(feature)] + enabled_feature = access_level_attribute.gt(ProjectFeature::DISABLED).or(access_level_attribute.eq(nil)) + + with_project_feature.where(enabled_feature) } # Picks a feature where the level is exactly that given. @@ -465,7 +467,8 @@ class Project < ActiveRecord::Base # logged in users to more efficiently get private projects with the given # feature. def self.with_feature_available_for_user(feature, user) - visible = [nil, ProjectFeature::ENABLED, ProjectFeature::PUBLIC] + visible = [ProjectFeature::ENABLED, ProjectFeature::PUBLIC] + min_access_level = ProjectFeature.required_minimum_access_level(feature) if user&.admin? with_feature_enabled(feature) @@ -473,10 +476,15 @@ class Project < ActiveRecord::Base column = ProjectFeature.quoted_access_level_column(feature) with_project_feature - .where("#{column} IN (?) OR (#{column} = ? AND EXISTS (?))", - visible, - ProjectFeature::PRIVATE, - user.authorizations_for_projects) + .where( + "(projects.visibility_level > :private AND (#{column} IS NULL OR #{column} >= (:public_visible) OR (#{column} = :private_visible AND EXISTS(:authorizations))))"\ + " OR (projects.visibility_level = :private AND (#{column} IS NULL OR #{column} >= :private_visible) AND EXISTS(:authorizations))", + { + private: Gitlab::VisibilityLevel::PRIVATE, + public_visible: ProjectFeature::ENABLED, + private_visible: ProjectFeature::PRIVATE, + authorizations: user.authorizations_for_projects(min_access_level: min_access_level) + }) else with_feature_access_level(feature, visible) end diff --git a/app/models/project_feature.rb b/app/models/project_feature.rb index 39f2b8fe0de..f700090a493 100644 --- a/app/models/project_feature.rb +++ b/app/models/project_feature.rb @@ -23,11 +23,11 @@ class ProjectFeature < ActiveRecord::Base PUBLIC = 30 FEATURES = %i(issues merge_requests wiki snippets builds repository pages).freeze + PRIVATE_FEATURES_MIN_ACCESS_LEVEL = { merge_requests: Gitlab::Access::REPORTER }.freeze class << self def access_level_attribute(feature) - feature = feature.model_name.plural.to_sym if feature.respond_to?(:model_name) - raise ArgumentError, "invalid project feature: #{feature}" unless FEATURES.include?(feature) + feature = ensure_feature!(feature) "#{feature}_access_level".to_sym end @@ -38,6 +38,21 @@ class ProjectFeature < ActiveRecord::Base "#{table}.#{attribute}" end + + def required_minimum_access_level(feature) + feature = ensure_feature!(feature) + + PRIVATE_FEATURES_MIN_ACCESS_LEVEL.fetch(feature, Gitlab::Access::GUEST) + end + + private + + def ensure_feature!(feature) + feature = feature.model_name.plural.to_sym if feature.respond_to?(:model_name) + raise ArgumentError, "invalid project feature: #{feature}" unless FEATURES.include?(feature) + + feature + end end # Default scopes force us to unscope here since a service may need to check diff --git a/app/models/user.rb b/app/models/user.rb index 26fd2d903a1..262b1835e98 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -754,8 +754,12 @@ class User < ActiveRecord::Base # # Example use: # `Project.where('EXISTS(?)', user.authorizations_for_projects)` - def authorizations_for_projects - project_authorizations.select(1).where('project_authorizations.project_id = projects.id') + def authorizations_for_projects(min_access_level: nil) + authorizations = project_authorizations.select(1).where('project_authorizations.project_id = projects.id') + + return authorizations unless min_access_level.present? + + authorizations.where('project_authorizations.access_level >= ?', min_access_level) end # Returns the projects this user has reporter (or greater) access to, limited diff --git a/changelogs/unreleased/security-guests-can-see-list-of-merge-requests.yml b/changelogs/unreleased/security-guests-can-see-list-of-merge-requests.yml new file mode 100644 index 00000000000..f5b74011829 --- /dev/null +++ b/changelogs/unreleased/security-guests-can-see-list-of-merge-requests.yml @@ -0,0 +1,6 @@ +--- +title: Group guests are no longer able to see merge requests they don't have access + to at group level +merge_request: +author: +type: security diff --git a/spec/finders/merge_requests_finder_spec.rb b/spec/finders/merge_requests_finder_spec.rb index ff4c6b8dd42..107da08a0a9 100644 --- a/spec/finders/merge_requests_finder_spec.rb +++ b/spec/finders/merge_requests_finder_spec.rb @@ -68,20 +68,34 @@ describe MergeRequestsFinder do expect(merge_requests.size).to eq(2) end - it 'filters by group' do - params = { group_id: group.id } + context 'filtering by group' do + it 'includes all merge requests when user has access' do + params = { group_id: group.id } - merge_requests = described_class.new(user, params).execute + merge_requests = described_class.new(user, params).execute - expect(merge_requests.size).to eq(3) - end + expect(merge_requests.size).to eq(3) + end - it 'filters by group including subgroups', :nested_groups do - params = { group_id: group.id, include_subgroups: true } + it 'excludes merge requests from projects the user does not have access to' do + private_project = create_project_without_n_plus_1(:private, group: group) + private_mr = create(:merge_request, :simple, author: user, source_project: private_project, target_project: private_project) + params = { group_id: group.id } - merge_requests = described_class.new(user, params).execute + private_project.add_guest(user) + merge_requests = described_class.new(user, params).execute - expect(merge_requests.size).to eq(6) + expect(merge_requests.size).to eq(3) + expect(merge_requests).not_to include(private_mr) + end + + it 'filters by group including subgroups', :nested_groups do + params = { group_id: group.id, include_subgroups: true } + + merge_requests = described_class.new(user, params).execute + + expect(merge_requests.size).to eq(6) + end end it 'filters by non_archived' do diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 7a8dc59039e..585dfe46189 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -3074,6 +3074,66 @@ describe Project do end end + describe '.with_feature_available_for_user' do + let!(:user) { create(:user) } + let!(:feature) { MergeRequest } + let!(:project) { create(:project, :public, :merge_requests_enabled) } + + subject { described_class.with_feature_available_for_user(feature, user) } + + context 'when user has access to project' do + subject { described_class.with_feature_available_for_user(feature, user) } + + before do + project.add_guest(user) + end + + context 'when public project' do + context 'when feature is public' do + it 'returns project' do + is_expected.to include(project) + end + end + + context 'when feature is private' do + let!(:project) { create(:project, :public, :merge_requests_private) } + + it 'returns project when user has access to the feature' do + project.add_maintainer(user) + + is_expected.to include(project) + end + + it 'does not return project when user does not have the minimum access level required' do + is_expected.not_to include(project) + end + end + end + + context 'when private project' do + let!(:project) { create(:project) } + + it 'returns project when user has access to the feature' do + project.add_maintainer(user) + + is_expected.to include(project) + end + + it 'does not return project when user does not have the minimum access level required' do + is_expected.not_to include(project) + end + end + end + + context 'when user does not have access to project' do + let!(:project) { create(:project) } + + it 'does not return project when user cant access project' do + is_expected.not_to include(project) + end + end + end + describe '#pages_available?' do let(:project) { create(:project, group: group) } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 33842e74b92..78477ab0a5a 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1997,6 +1997,33 @@ describe User do expect(subject).to include(accessible) expect(subject).not_to include(other) end + + context 'with min_access_level' do + let!(:user) { create(:user) } + let!(:project) { create(:project, :private, namespace: user.namespace) } + + before do + project.add_developer(user) + end + + subject { Project.where("EXISTS (?)", user.authorizations_for_projects(min_access_level: min_access_level)) } + + context 'when developer access' do + let(:min_access_level) { Gitlab::Access::DEVELOPER } + + it 'includes projects a user has access to' do + expect(subject).to include(project) + end + end + + context 'when owner access' do + let(:min_access_level) { Gitlab::Access::OWNER } + + it 'does not include projects with higher access level' do + expect(subject).not_to include(project) + end + end + end end describe '#authorized_projects', :delete do From 79b256ed6dedf3e5f0c670f1dac47b113039cbff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20L=C3=B3pez?= Date: Fri, 14 Dec 2018 17:51:37 +0100 Subject: [PATCH 16/42] Added validations to prevent LFS object forgery --- app/models/lfs_download_object.rb | 22 +++ app/services/projects/import_service.rb | 8 +- .../lfs_download_link_list_service.rb | 13 +- .../lfs_pointers/lfs_download_service.rb | 113 ++++++++---- ...ty-fix-lfs-import-project-ssrf-forgery.yml | 5 + .../importer/lfs_object_importer.rb | 8 +- .../representation/lfs_object.rb | 4 +- .../importer/lfs_object_importer_spec.rb | 22 ++- .../importer/lfs_objects_importer_spec.rb | 14 +- spec/models/lfs_download_object_spec.rb | 68 ++++++++ spec/services/projects/import_service_spec.rb | 9 +- .../lfs_download_link_list_service_spec.rb | 18 +- .../lfs_pointers/lfs_download_service_spec.rb | 164 ++++++++++++++---- 13 files changed, 362 insertions(+), 106 deletions(-) create mode 100644 app/models/lfs_download_object.rb create mode 100644 changelogs/unreleased/security-fix-lfs-import-project-ssrf-forgery.yml create mode 100644 spec/models/lfs_download_object_spec.rb diff --git a/app/models/lfs_download_object.rb b/app/models/lfs_download_object.rb new file mode 100644 index 00000000000..6383f95d546 --- /dev/null +++ b/app/models/lfs_download_object.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class LfsDownloadObject + include ActiveModel::Validations + + attr_accessor :oid, :size, :link + delegate :sanitized_url, :credentials, to: :sanitized_uri + + validates :oid, format: { with: /\A\h{64}\z/ } + validates :size, numericality: { greater_than_or_equal_to: 0 } + validates :link, public_url: { protocols: %w(http https) } + + def initialize(oid:, size:, link:) + @oid = oid + @size = size + @link = link + end + + def sanitized_uri + @sanitized_uri ||= Gitlab::UrlSanitizer.new(link) + end +end diff --git a/app/services/projects/import_service.rb b/app/services/projects/import_service.rb index 0c426faa22d..3353aec6aec 100644 --- a/app/services/projects/import_service.rb +++ b/app/services/projects/import_service.rb @@ -86,11 +86,11 @@ module Projects return unless project.lfs_enabled? - oids_to_download = Projects::LfsPointers::LfsImportService.new(project).execute - download_service = Projects::LfsPointers::LfsDownloadService.new(project) + lfs_objects_to_download = Projects::LfsPointers::LfsImportService.new(project).execute - oids_to_download.each do |oid, link| - download_service.execute(oid, link) + lfs_objects_to_download.each do |lfs_download_object| + Projects::LfsPointers::LfsDownloadService.new(project, lfs_download_object) + .execute end rescue => e # Right now, to avoid aborting the importing process, we silently fail diff --git a/app/services/projects/lfs_pointers/lfs_download_link_list_service.rb b/app/services/projects/lfs_pointers/lfs_download_link_list_service.rb index a837ea82e38..7998976b00a 100644 --- a/app/services/projects/lfs_pointers/lfs_download_link_list_service.rb +++ b/app/services/projects/lfs_pointers/lfs_download_link_list_service.rb @@ -41,16 +41,17 @@ module Projects end def parse_response_links(objects_response) - objects_response.each_with_object({}) do |entry, link_list| + objects_response.each_with_object([]) do |entry, link_list| begin - oid = entry['oid'] link = entry.dig('actions', DOWNLOAD_ACTION, 'href') raise DownloadLinkNotFound unless link - link_list[oid] = add_credentials(link) - rescue DownloadLinkNotFound, URI::InvalidURIError - Rails.logger.error("Link for Lfs Object with oid #{oid} not found or invalid.") + link_list << LfsDownloadObject.new(oid: entry['oid'], + size: entry['size'], + link: add_credentials(link)) + rescue DownloadLinkNotFound, Addressable::URI::InvalidURIError + log_error("Link for Lfs Object with oid #{entry['oid']} not found or invalid.") end end end @@ -70,7 +71,7 @@ module Projects end def add_credentials(link) - uri = URI.parse(link) + uri = Addressable::URI.parse(link) if should_add_credentials?(uri) uri.user = remote_uri.user diff --git a/app/services/projects/lfs_pointers/lfs_download_service.rb b/app/services/projects/lfs_pointers/lfs_download_service.rb index b5128443435..398f00a598d 100644 --- a/app/services/projects/lfs_pointers/lfs_download_service.rb +++ b/app/services/projects/lfs_pointers/lfs_download_service.rb @@ -4,68 +4,93 @@ module Projects module LfsPointers class LfsDownloadService < BaseService - VALID_PROTOCOLS = %w[http https].freeze + SizeError = Class.new(StandardError) + OidError = Class.new(StandardError) + + attr_reader :lfs_download_object + delegate :oid, :size, :credentials, :sanitized_url, to: :lfs_download_object, prefix: :lfs + + def initialize(project, lfs_download_object) + super(project) + + @lfs_download_object = lfs_download_object + end # rubocop: disable CodeReuse/ActiveRecord - def execute(oid, url) - return unless project&.lfs_enabled? && oid.present? && url.present? + def execute + return unless project&.lfs_enabled? && lfs_download_object + return error("LFS file with oid #{lfs_oid} has invalid attributes") unless lfs_download_object.valid? + return if LfsObject.exists?(oid: lfs_oid) - return if LfsObject.exists?(oid: oid) - - sanitized_uri = sanitize_url!(url) - - with_tmp_file(oid) do |file| - download_and_save_file(file, sanitized_uri) - lfs_object = LfsObject.new(oid: oid, size: file.size, file: file) - - project.all_lfs_objects << lfs_object + wrap_download_errors do + download_lfs_file! end - rescue Gitlab::UrlBlocker::BlockedUrlError => e - Rails.logger.error("LFS file with oid #{oid} couldn't be downloaded: #{e.message}") - rescue StandardError => e - Rails.logger.error("LFS file with oid #{oid} couldn't be downloaded from #{sanitized_uri.sanitized_url}: #{e.message}") end # rubocop: enable CodeReuse/ActiveRecord private - def sanitize_url!(url) - Gitlab::UrlSanitizer.new(url).tap do |sanitized_uri| - # Just validate that HTTP/HTTPS protocols are used. The - # subsequent Gitlab::HTTP.get call will do network checks - # based on the settings. - Gitlab::UrlBlocker.validate!(sanitized_uri.sanitized_url, - protocols: VALID_PROTOCOLS) + def wrap_download_errors(&block) + yield + rescue SizeError, OidError, StandardError => e + error("LFS file with oid #{lfs_oid} could't be downloaded from #{lfs_sanitized_url}: #{e.message}") + end + + def download_lfs_file! + with_tmp_file do |tmp_file| + download_and_save_file!(tmp_file) + project.all_lfs_objects << LfsObject.new(oid: lfs_oid, + size: lfs_size, + file: tmp_file) + + success end end - def download_and_save_file(file, sanitized_uri) - response = Gitlab::HTTP.get(sanitized_uri.sanitized_url, headers(sanitized_uri)) do |fragment| + def download_and_save_file!(file) + digester = Digest::SHA256.new + response = Gitlab::HTTP.get(lfs_sanitized_url, download_headers) do |fragment| + digester << fragment file.write(fragment) + + raise_size_error! if file.size > lfs_size end raise StandardError, "Received error code #{response.code}" unless response.success? + + raise_size_error! if file.size != lfs_size + raise_oid_error! if digester.hexdigest != lfs_oid end - def headers(sanitized_uri) - query_options.tap do |headers| - credentials = sanitized_uri.credentials - - if credentials[:user].present? || credentials[:password].present? + def download_headers + { stream_body: true }.tap do |headers| + if lfs_credentials[:user].present? || lfs_credentials[:password].present? # Using authentication headers in the request - headers[:http_basic_authentication] = [credentials[:user], credentials[:password]] + headers[:basic_auth] = { username: lfs_credentials[:user], password: lfs_credentials[:password] } end end end - def query_options - { stream_body: true } - end - - def with_tmp_file(oid) + def with_tmp_file create_tmp_storage_dir - File.open(File.join(tmp_storage_dir, oid), 'wb') { |file| yield file } + File.open(tmp_filename, 'wb') do |file| + begin + yield file + rescue StandardError => e + # If the lfs file is successfully downloaded it will be removed + # when it is added to the project's lfs files. + # Nevertheless if any excetion raises the file would remain + # in the file system. Here we ensure to remove it + File.unlink(file) if File.exist?(file) + + raise e + end + end + end + + def tmp_filename + File.join(tmp_storage_dir, lfs_oid) end def create_tmp_storage_dir @@ -79,6 +104,20 @@ module Projects def storage_dir @storage_dir ||= Gitlab.config.lfs.storage_path end + + def raise_size_error! + raise SizeError, 'Size mistmatch' + end + + def raise_oid_error! + raise OidError, 'Oid mismatch' + end + + def error(message, http_status = nil) + log_error(message) + + super + end end end end diff --git a/changelogs/unreleased/security-fix-lfs-import-project-ssrf-forgery.yml b/changelogs/unreleased/security-fix-lfs-import-project-ssrf-forgery.yml new file mode 100644 index 00000000000..b6315ec29d8 --- /dev/null +++ b/changelogs/unreleased/security-fix-lfs-import-project-ssrf-forgery.yml @@ -0,0 +1,5 @@ +--- +title: Add more LFS validations to prevent forgery +merge_request: +author: +type: security diff --git a/lib/gitlab/github_import/importer/lfs_object_importer.rb b/lib/gitlab/github_import/importer/lfs_object_importer.rb index a88c17aaf82..195383fd3e9 100644 --- a/lib/gitlab/github_import/importer/lfs_object_importer.rb +++ b/lib/gitlab/github_import/importer/lfs_object_importer.rb @@ -13,10 +13,12 @@ module Gitlab @project = project end + def lfs_download_object + LfsDownloadObject.new(oid: lfs_object.oid, size: lfs_object.size, link: lfs_object.link) + end + def execute - Projects::LfsPointers::LfsDownloadService - .new(project) - .execute(lfs_object.oid, lfs_object.download_link) + Projects::LfsPointers::LfsDownloadService.new(project, lfs_download_object).execute end end end diff --git a/lib/gitlab/github_import/representation/lfs_object.rb b/lib/gitlab/github_import/representation/lfs_object.rb index debe0fa0baf..a4606173f49 100644 --- a/lib/gitlab/github_import/representation/lfs_object.rb +++ b/lib/gitlab/github_import/representation/lfs_object.rb @@ -9,11 +9,11 @@ module Gitlab attr_reader :attributes - expose_attribute :oid, :download_link + expose_attribute :oid, :link, :size # Builds a lfs_object def self.from_api_response(lfs_object) - new({ oid: lfs_object[0], download_link: lfs_object[1] }) + new({ oid: lfs_object.oid, link: lfs_object.link, size: lfs_object.size }) end # Builds a new lfs_object using a Hash that was built from a JSON payload. diff --git a/spec/lib/gitlab/github_import/importer/lfs_object_importer_spec.rb b/spec/lib/gitlab/github_import/importer/lfs_object_importer_spec.rb index 4857f2afbe2..8fd328d9c1e 100644 --- a/spec/lib/gitlab/github_import/importer/lfs_object_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/lfs_object_importer_spec.rb @@ -2,20 +2,26 @@ require 'spec_helper' describe Gitlab::GithubImport::Importer::LfsObjectImporter do let(:project) { create(:project) } - let(:download_link) { "http://www.gitlab.com/lfs_objects/oid" } - - let(:github_lfs_object) do - Gitlab::GithubImport::Representation::LfsObject.new( - oid: 'oid', download_link: download_link - ) + let(:lfs_attributes) do + { + oid: 'oid', + size: 1, + link: 'http://www.gitlab.com/lfs_objects/oid' + } end + let(:lfs_download_object) { LfsDownloadObject.new(lfs_attributes) } + let(:github_lfs_object) { Gitlab::GithubImport::Representation::LfsObject.new(lfs_attributes) } + let(:importer) { described_class.new(github_lfs_object, project, nil) } describe '#execute' do it 'calls the LfsDownloadService with the lfs object attributes' do - expect_any_instance_of(Projects::LfsPointers::LfsDownloadService) - .to receive(:execute).with('oid', download_link) + allow(importer).to receive(:lfs_download_object).and_return(lfs_download_object) + + service = double + expect(Projects::LfsPointers::LfsDownloadService).to receive(:new).with(project, lfs_download_object).and_return(service) + expect(service).to receive(:execute) importer.execute end diff --git a/spec/lib/gitlab/github_import/importer/lfs_objects_importer_spec.rb b/spec/lib/gitlab/github_import/importer/lfs_objects_importer_spec.rb index 5f5c6b803c0..50442552eee 100644 --- a/spec/lib/gitlab/github_import/importer/lfs_objects_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/lfs_objects_importer_spec.rb @@ -5,7 +5,15 @@ describe Gitlab::GithubImport::Importer::LfsObjectsImporter do let(:client) { double(:client) } let(:download_link) { "http://www.gitlab.com/lfs_objects/oid" } - let(:github_lfs_object) { ['oid', download_link] } + let(:lfs_attributes) do + { + oid: 'oid', + size: 1, + link: 'http://www.gitlab.com/lfs_objects/oid' + } + end + + let(:lfs_download_object) { LfsDownloadObject.new(lfs_attributes) } describe '#parallel?' do it 'returns true when running in parallel mode' do @@ -48,7 +56,7 @@ describe Gitlab::GithubImport::Importer::LfsObjectsImporter do allow(importer) .to receive(:each_object_to_import) - .and_yield(['oid', download_link]) + .and_yield(lfs_download_object) expect(Gitlab::GithubImport::Importer::LfsObjectImporter) .to receive(:new) @@ -71,7 +79,7 @@ describe Gitlab::GithubImport::Importer::LfsObjectsImporter do allow(importer) .to receive(:each_object_to_import) - .and_yield(github_lfs_object) + .and_yield(lfs_download_object) expect(Gitlab::GithubImport::ImportLfsObjectWorker) .to receive(:perform_async) diff --git a/spec/models/lfs_download_object_spec.rb b/spec/models/lfs_download_object_spec.rb new file mode 100644 index 00000000000..88838b127d2 --- /dev/null +++ b/spec/models/lfs_download_object_spec.rb @@ -0,0 +1,68 @@ +require 'rails_helper' + +describe LfsDownloadObject do + let(:oid) { 'cd293be6cea034bd45a0352775a219ef5dc7825ce55d1f7dae9762d80ce64411' } + let(:link) { 'http://www.example.com' } + let(:size) { 1 } + + subject { described_class.new(oid: oid, size: size, link: link) } + + describe 'validations' do + it { is_expected.to validate_numericality_of(:size).is_greater_than_or_equal_to(0) } + + context 'oid attribute' do + it 'must be 64 characters long' do + aggregate_failures do + expect(described_class.new(oid: 'a' * 63, size: size, link: link)).to be_invalid + expect(described_class.new(oid: 'a' * 65, size: size, link: link)).to be_invalid + expect(described_class.new(oid: 'a' * 64, size: size, link: link)).to be_valid + end + end + + it 'must contain only hexadecimal characters' do + aggregate_failures do + expect(subject).to be_valid + expect(described_class.new(oid: 'g' * 64, size: size, link: link)).to be_invalid + end + end + end + + context 'link attribute' do + it 'only http and https protocols are valid' do + aggregate_failures do + expect(described_class.new(oid: oid, size: size, link: 'http://www.example.com')).to be_valid + expect(described_class.new(oid: oid, size: size, link: 'https://www.example.com')).to be_valid + expect(described_class.new(oid: oid, size: size, link: 'ftp://www.example.com')).to be_invalid + expect(described_class.new(oid: oid, size: size, link: 'ssh://www.example.com')).to be_invalid + expect(described_class.new(oid: oid, size: size, link: 'git://www.example.com')).to be_invalid + end + end + + it 'cannot be empty' do + expect(described_class.new(oid: oid, size: size, link: '')).not_to be_valid + end + + context 'when localhost or local network addresses' do + subject { described_class.new(oid: oid, size: size, link: 'http://192.168.1.1') } + + before do + allow(ApplicationSetting) + .to receive(:current) + .and_return(ApplicationSetting.build_from_defaults(allow_local_requests_from_hooks_and_services: setting)) + end + + context 'are allowed' do + let(:setting) { true } + + it { expect(subject).to be_valid } + end + + context 'are not allowed' do + let(:setting) { false } + + it { expect(subject).to be_invalid } + end + end + end + end +end diff --git a/spec/services/projects/import_service_spec.rb b/spec/services/projects/import_service_spec.rb index 06f865dc848..c525de6cd25 100644 --- a/spec/services/projects/import_service_spec.rb +++ b/spec/services/projects/import_service_spec.rb @@ -152,8 +152,11 @@ describe Projects::ImportService do it 'downloads lfs objects if lfs_enabled is enabled for project' do allow(project).to receive(:lfs_enabled?).and_return(true) + + service = double expect_any_instance_of(Projects::LfsPointers::LfsImportService).to receive(:execute).and_return(oid_download_links) - expect_any_instance_of(Projects::LfsPointers::LfsDownloadService).to receive(:execute).twice + expect(Projects::LfsPointers::LfsDownloadService).to receive(:new).and_return(service).twice + expect(service).to receive(:execute).twice subject.execute end @@ -211,8 +214,10 @@ describe Projects::ImportService do it 'does not have a custom repository importer downloads lfs objects' do allow(Gitlab::GithubImport::ParallelImporter).to receive(:imports_repository?).and_return(false) + service = double expect_any_instance_of(Projects::LfsPointers::LfsImportService).to receive(:execute).and_return(oid_download_links) - expect_any_instance_of(Projects::LfsPointers::LfsDownloadService).to receive(:execute) + expect(Projects::LfsPointers::LfsDownloadService).to receive(:new).and_return(service).twice + expect(service).to receive(:execute).twice subject.execute end diff --git a/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb index d7a2829d5f8..f222c52199f 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb @@ -37,8 +37,8 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do describe '#execute' do it 'retrieves each download link of every non existent lfs object' do - subject.execute(new_oids).each do |oid, link| - expect(link).to eq "#{import_url}/gitlab-lfs/objects/#{oid}" + subject.execute(new_oids).each do |lfs_download_object| + expect(lfs_download_object.link).to eq "#{import_url}/gitlab-lfs/objects/#{lfs_download_object.oid}" end end @@ -50,8 +50,8 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do it 'adds credentials to the download_link' do result = subject.execute(new_oids) - result.each do |oid, link| - expect(link.starts_with?('http://user:password@')).to be_truthy + result.each do |lfs_download_object| + expect(lfs_download_object.link.starts_with?('http://user:password@')).to be_truthy end end end @@ -60,8 +60,8 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do it 'does not add any credentials' do result = subject.execute(new_oids) - result.each do |oid, link| - expect(link.starts_with?('http://user:password@')).to be_falsey + result.each do |lfs_download_object| + expect(lfs_download_object.link.starts_with?('http://user:password@')).to be_falsey end end end @@ -74,8 +74,8 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do it 'downloads without any credentials' do result = subject.execute(new_oids) - result.each do |oid, link| - expect(link.starts_with?('http://user:password@')).to be_falsey + result.each do |lfs_download_object| + expect(lfs_download_object.link.starts_with?('http://user:password@')).to be_falsey end end end @@ -92,7 +92,7 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do describe '#parse_response_links' do it 'does not add oid entry if href not found' do - expect(Rails.logger).to receive(:error).with("Link for Lfs Object with oid whatever not found or invalid.") + expect(subject).to receive(:log_error).with("Link for Lfs Object with oid whatever not found or invalid.") result = subject.send(:parse_response_links, invalid_object_response) diff --git a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb index 95c9b6e63b8..d952d1d1ffd 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb @@ -2,68 +2,156 @@ require 'spec_helper' describe Projects::LfsPointers::LfsDownloadService do let(:project) { create(:project) } - let(:oid) { '9e548e25631dd9ce6b43afd6359ab76da2819d6a5b474e66118c7819e1d8b3e8' } - let(:download_link) { "http://gitlab.com/#{oid}" } let(:lfs_content) { SecureRandom.random_bytes(10) } + let(:oid) { Digest::SHA256.hexdigest(lfs_content) } + let(:download_link) { "http://gitlab.com/#{oid}" } + let(:size) { lfs_content.size } + let(:lfs_object) { LfsDownloadObject.new(oid: oid, size: size, link: download_link) } + let(:local_request_setting) { false } - subject { described_class.new(project) } + subject { described_class.new(project, lfs_object) } before do - allow(project).to receive(:lfs_enabled?).and_return(true) - WebMock.stub_request(:get, download_link).to_return(body: lfs_content) + ApplicationSetting.create_from_defaults - allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(false) + stub_application_setting(allow_local_requests_from_hooks_and_services: local_request_setting) + allow(project).to receive(:lfs_enabled?).and_return(true) + end + + shared_examples 'lfs temporal file is removed' do + it do + subject.execute + + expect(File.exist?(subject.send(:tmp_filename))).to be false + end + end + + shared_examples 'no lfs object is created' do + it do + expect { subject.execute }.not_to change { LfsObject.count } + end + + it 'returns error result' do + expect(subject.execute[:status]).to eq :error + end + + it 'an error is logged' do + expect(subject).to receive(:log_error) + + subject.execute + end + + it_behaves_like 'lfs temporal file is removed' + end + + shared_examples 'lfs object is created' do + it do + expect(subject).to receive(:download_and_save_file!).and_call_original + + expect { subject.execute }.to change { LfsObject.count }.by(1) + end + + it 'returns success result' do + expect(subject.execute[:status]).to eq :success + end + + it_behaves_like 'lfs temporal file is removed' end describe '#execute' do context 'when file download succeeds' do - it 'a new lfs object is created' do - expect { subject.execute(oid, download_link) }.to change { LfsObject.count }.from(0).to(1) + before do + WebMock.stub_request(:get, download_link).to_return(body: lfs_content) end + it_behaves_like 'lfs object is created' + it 'has the same oid' do - subject.execute(oid, download_link) + subject.execute expect(LfsObject.first.oid).to eq oid end + it 'has the same size' do + subject.execute + + expect(LfsObject.first.size).to eq size + end + it 'stores the content' do - subject.execute(oid, download_link) + subject.execute expect(File.binread(LfsObject.first.file.file.file)).to eq lfs_content end end context 'when file download fails' do - it 'no lfs object is created' do - expect { subject.execute(oid, download_link) }.to change { LfsObject.count } + before do + allow(Gitlab::HTTP).to receive(:get).and_return(code: 500, 'success?' => false) + end + + it_behaves_like 'no lfs object is created' + + it 'raise StandardError exception' do + expect(subject).to receive(:download_and_save_file!).and_raise(StandardError) + + subject.execute + end + end + + context 'when downloaded lfs file has a different size' do + let(:size) { 1 } + + before do + WebMock.stub_request(:get, download_link).to_return(body: lfs_content) + end + + it_behaves_like 'no lfs object is created' + + it 'raise SizeError exception' do + expect(subject).to receive(:download_and_save_file!).and_raise(described_class::SizeError) + + subject.execute + end + end + + context 'when downloaded lfs file has a different oid' do + before do + WebMock.stub_request(:get, download_link).to_return(body: lfs_content) + allow_any_instance_of(Digest::SHA256).to receive(:hexdigest).and_return('foobar') + end + + it_behaves_like 'no lfs object is created' + + it 'raise OidError exception' do + expect(subject).to receive(:download_and_save_file!).and_raise(described_class::OidError) + + subject.execute end end context 'when credentials present' do let(:download_link_with_credentials) { "http://user:password@gitlab.com/#{oid}" } + let(:lfs_object) { LfsDownloadObject.new(oid: oid, size: size, link: download_link_with_credentials) } before do WebMock.stub_request(:get, download_link).with(headers: { 'Authorization' => 'Basic dXNlcjpwYXNzd29yZA==' }).to_return(body: lfs_content) end it 'the request adds authorization headers' do - subject.execute(oid, download_link_with_credentials) + subject end end context 'when localhost requests are allowed' do let(:download_link) { 'http://192.168.2.120' } + let(:local_request_setting) { true } before do - allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(true) + WebMock.stub_request(:get, download_link).to_return(body: lfs_content) end - it 'downloads the file' do - expect(subject).to receive(:download_and_save_file).and_call_original - - expect { subject.execute(oid, download_link) }.to change { LfsObject.count }.by(1) - end + it_behaves_like 'lfs object is created' end context 'when a bad URL is used' do @@ -71,7 +159,9 @@ describe Projects::LfsPointers::LfsDownloadService do with_them do it 'does not download the file' do - expect { subject.execute(oid, download_link) }.not_to change { LfsObject.count } + expect(subject).not_to receive(:download_lfs_file!) + + expect { subject.execute }.not_to change { LfsObject.count } end end end @@ -85,15 +175,11 @@ describe Projects::LfsPointers::LfsDownloadService do WebMock.stub_request(:get, download_link).to_return(status: 301, headers: { 'Location' => redirect_link }) end - it 'does not follow the redirection' do - expect(Rails.logger).to receive(:error).with(/LFS file with oid #{oid} couldn't be downloaded/) - - expect { subject.execute(oid, download_link) }.not_to change { LfsObject.count } - end + it_behaves_like 'no lfs object is created' end end - context 'that is valid' do + context 'that is not blocked' do let(:redirect_link) { "http://example.com/"} before do @@ -101,21 +187,35 @@ describe Projects::LfsPointers::LfsDownloadService do WebMock.stub_request(:get, redirect_link).to_return(body: lfs_content) end - it 'follows the redirection' do - expect { subject.execute(oid, download_link) }.to change { LfsObject.count }.from(0).to(1) - end + it_behaves_like 'lfs object is created' + end + end + + context 'when the lfs object attributes are invalid' do + let(:oid) { 'foobar' } + + before do + expect(lfs_object).to be_invalid + end + + it_behaves_like 'no lfs object is created' + + it 'does not download the file' do + expect(subject).not_to receive(:download_lfs_file!) + + subject.execute end end context 'when an lfs object with the same oid already exists' do before do - create(:lfs_object, oid: 'oid') + create(:lfs_object, oid: oid) end it 'does not download the file' do - expect(subject).not_to receive(:download_and_save_file) + expect(subject).not_to receive(:download_lfs_file!) - subject.execute('oid', download_link) + subject.execute end end end From 7d823035657dd00568a4154dcc11779914058891 Mon Sep 17 00:00:00 2001 From: Brett Walker Date: Mon, 14 Jan 2019 16:57:54 -0600 Subject: [PATCH 17/42] Show tooltip for malicious looking links Such as those with IDN homographs or embedded right-to-left (RTLO) characters. Autolinked hrefs should be escaped --- .../security-2769-idn-homograph-attack.yml | 5 ++ lib/banzai/filter/autolink_filter.rb | 13 ++- lib/banzai/filter/external_link_filter.rb | 85 +++++++++++++++++-- lib/banzai/pipeline/email_pipeline.rb | 3 +- .../lib/banzai/filter/autolink_filter_spec.rb | 16 ++++ .../filter/external_link_filter_spec.rb | 65 ++++++++++++++ .../banzai/pipeline/email_pipeline_spec.rb | 14 +++ .../lib/banzai/pipeline/full_pipeline_spec.rb | 38 +++++++++ 8 files changed, 227 insertions(+), 12 deletions(-) create mode 100644 changelogs/unreleased/security-2769-idn-homograph-attack.yml diff --git a/changelogs/unreleased/security-2769-idn-homograph-attack.yml b/changelogs/unreleased/security-2769-idn-homograph-attack.yml new file mode 100644 index 00000000000..a014b522c96 --- /dev/null +++ b/changelogs/unreleased/security-2769-idn-homograph-attack.yml @@ -0,0 +1,5 @@ +--- +title: Make potentially malicious links more visible in the UI and scrub RTLO chars from links +merge_request: 2770 +author: +type: security diff --git a/lib/banzai/filter/autolink_filter.rb b/lib/banzai/filter/autolink_filter.rb index deda4b1872e..f3061bad4ff 100644 --- a/lib/banzai/filter/autolink_filter.rb +++ b/lib/banzai/filter/autolink_filter.rb @@ -8,6 +8,10 @@ module Banzai # # Based on HTML::Pipeline::AutolinkFilter # + # Note that our CommonMark parser, `commonmarker` (using the autolink extension) + # handles standard autolinking, like http/https. We detect additional + # schemes (smb, rdar, etc). + # # Context options: # :autolink - Boolean, skips all processing done by this filter when false # :link_attr - Hash of attributes for the generated links @@ -107,10 +111,13 @@ module Banzai end end - # match has come from node.to_html above, so we know it's encoded - # correctly. + # Since this came from a Text node, make sure the new href is encoded. + # `commonmarker` percent encodes the domains of links it handles, so + # do the same (instead of using `normalized_encode`). + href_safe = Addressable::URI.encode(match).html_safe + html_safe_match = match.html_safe - options = link_options.merge(href: html_safe_match) + options = link_options.merge(href: href_safe) content_tag(:a, html_safe_match, options) + dropped end diff --git a/lib/banzai/filter/external_link_filter.rb b/lib/banzai/filter/external_link_filter.rb index 4f60b6f84c6..61ee3eac216 100644 --- a/lib/banzai/filter/external_link_filter.rb +++ b/lib/banzai/filter/external_link_filter.rb @@ -4,17 +4,29 @@ module Banzai module Filter # HTML Filter to modify the attributes of external links class ExternalLinkFilter < HTML::Pipeline::Filter - SCHEMES = ['http', 'https', nil].freeze + SCHEMES = ['http', 'https', nil].freeze + RTLO = "\u202E".freeze + ENCODED_RTLO = '%E2%80%AE'.freeze def call links.each do |node| - uri = uri(node['href'].to_s) + # URI.parse does stricter checking on the url than Addressable, + # such as on `mailto:` links. Since we've been using it, do an + # initial parse for validity and then use Addressable + # for IDN support, etc + uri = uri_strict(node['href'].to_s) + if uri + node.set_attribute('href', uri.to_s) + addressable_uri = addressable_uri(node['href']) + else + addressable_uri = nil + end - node.set_attribute('href', uri.to_s) if uri - - if SCHEMES.include?(uri&.scheme) && !internal_url?(uri) - node.set_attribute('rel', 'nofollow noreferrer noopener') - node.set_attribute('target', '_blank') + unless internal_url?(addressable_uri) + punycode_autolink_node!(addressable_uri, node) + sanitize_link_text!(node) + add_malicious_tooltip!(addressable_uri, node) + add_nofollow!(addressable_uri, node) end end @@ -23,12 +35,18 @@ module Banzai private - def uri(href) + def uri_strict(href) URI.parse(href) rescue URI::Error nil end + def addressable_uri(href) + Addressable::URI.parse(href) + rescue Addressable::URI::InvalidURIError + nil + end + def links query = 'descendant-or-self::a[@href and not(@href = "")]' doc.xpath(query) @@ -45,6 +63,57 @@ module Banzai def internal_url @internal_url ||= URI.parse(Gitlab.config.gitlab.url) end + + # Only replace an autolink with an IDN with it's punycode + # version if we need emailable links. Otherwise let it + # be shown normally and the tooltips will show the + # punycode version. + def punycode_autolink_node!(uri, node) + return unless uri + return unless context[:emailable_links] + + unencoded_uri_str = Addressable::URI.unencode(node['href']) + + if unencoded_uri_str == node.content && idn?(uri) + node.content = uri.normalize + end + end + + # escape any right-to-left (RTLO) characters in link text + def sanitize_link_text!(node) + node.inner_html = node.inner_html.gsub(RTLO, ENCODED_RTLO) + end + + # If the domain is an international domain name (IDN), + # let's expose with a tooltip in case it's intended + # to be malicious. This is particularly useful for links + # where the link text is not the same as the actual link. + # We will continue to show the unicode version of the domain + # in autolinked link text, which could contain emojis, etc. + # + # Also show the tooltip if the url contains the RTLO character, + # as this is an indicator of a malicious link + def add_malicious_tooltip!(uri, node) + if idn?(uri) || has_encoded_rtlo?(uri) + node.add_class('has-tooltip') + node.set_attribute('title', uri.normalize) + end + end + + def add_nofollow!(uri, node) + if SCHEMES.include?(uri&.scheme) + node.set_attribute('rel', 'nofollow noreferrer noopener') + node.set_attribute('target', '_blank') + end + end + + def idn?(uri) + uri&.normalized_host&.start_with?('xn--') + end + + def has_encoded_rtlo?(uri) + uri&.to_s&.include?(ENCODED_RTLO) + end end end end diff --git a/lib/banzai/pipeline/email_pipeline.rb b/lib/banzai/pipeline/email_pipeline.rb index 2c08581ce0d..fc51063c06c 100644 --- a/lib/banzai/pipeline/email_pipeline.rb +++ b/lib/banzai/pipeline/email_pipeline.rb @@ -11,7 +11,8 @@ module Banzai def self.transform_context(context) super(context).merge( - only_path: false + only_path: false, + emailable_links: true ) end end diff --git a/spec/lib/banzai/filter/autolink_filter_spec.rb b/spec/lib/banzai/filter/autolink_filter_spec.rb index 7a457403b51..6217381c491 100644 --- a/spec/lib/banzai/filter/autolink_filter_spec.rb +++ b/spec/lib/banzai/filter/autolink_filter_spec.rb @@ -188,6 +188,22 @@ describe Banzai::Filter::AutolinkFilter do expect(doc.at_css('a')['class']).to eq 'custom' end + it 'escapes RTLO and other characters' do + # rendered text looks like "http://example.com/evilexe.mp3" + evil_link = "#{link}evil\u202E3pm.exe" + doc = filter("#{evil_link}") + + expect(doc.at_css('a')['href']).to eq "http://about.gitlab.com/evil%E2%80%AE3pm.exe" + end + + it 'encodes international domains' do + link = "http://one😄two.com" + expected = "http://one%F0%9F%98%84two.com" + doc = filter(link) + + expect(doc.at_css('a')['href']).to eq expected + end + described_class::IGNORE_PARENTS.each do |elem| it "ignores valid links contained inside '#{elem}' element" do exp = act = "<#{elem}>See #{link}" diff --git a/spec/lib/banzai/filter/external_link_filter_spec.rb b/spec/lib/banzai/filter/external_link_filter_spec.rb index e6dae8d5382..2acbe05f082 100644 --- a/spec/lib/banzai/filter/external_link_filter_spec.rb +++ b/spec/lib/banzai/filter/external_link_filter_spec.rb @@ -62,6 +62,13 @@ describe Banzai::Filter::ExternalLinkFilter do expect(doc.to_html).to eq(expected) end + + it 'adds rel and target to improperly formatted autolinks' do + doc = filter %q(

mailto://jblogs@example.com

) + expected = %q(

mailto://jblogs@example.com

) + + expect(doc.to_html).to eq(expected) + end end context 'for links with a username' do @@ -112,4 +119,62 @@ describe Banzai::Filter::ExternalLinkFilter do it_behaves_like 'an external link with rel attribute' end + + context 'links with RTLO character' do + # In rendered text this looks like "http://example.com/evilexe.mp3" + let(:doc) { filter %Q(http://example.com/evil\u202E3pm.exe) } + + it_behaves_like 'an external link with rel attribute' + + it 'escapes RTLO in link text' do + expected = %q(http://example.com/evil%E2%80%AE3pm.exe) + + expect(doc.to_html).to include(expected) + end + + it 'does not mangle the link text' do + doc = filter %Q(Oneand\u202Eexe.mp3) + + expect(doc.to_html).to include('Oneand%E2%80%AEexe.mp3') + end + end + + context 'for generated autolinks' do + context 'with an IDN character' do + let(:doc) { filter(%q(http://exa😄mple.com)) } + let(:doc_email) { filter(%q(http://exa😄mple.com), emailable_links: true) } + + it_behaves_like 'an external link with rel attribute' + + it 'does not change the link text' do + expect(doc.to_html).to include('http://exa😄mple.com') + end + + it 'uses punycode for emails' do + expect(doc_email.to_html).to include('http://xn--example-6p25f.com/') + end + end + end + + context 'for links that look malicious' do + context 'with an IDN character' do + let(:doc) { filter %q(http://exa😄mple.com) } + + it 'adds a toolip with punycode' do + expect(doc.to_html).to include('http://exa😄mple.com') + expect(doc.to_html).to include('class="has-tooltip"') + expect(doc.to_html).to include('title="http://xn--example-6p25f.com/"') + end + end + + context 'with RTLO character' do + let(:doc) { filter %q(Evil Test) } + + it 'adds a toolip with punycode' do + expect(doc.to_html).to include('Evil Test') + expect(doc.to_html).to include('class="has-tooltip"') + expect(doc.to_html).to include('title="http://example.com/evil%E2%80%AE3pm.exe"') + end + end + end end diff --git a/spec/lib/banzai/pipeline/email_pipeline_spec.rb b/spec/lib/banzai/pipeline/email_pipeline_spec.rb index 6a11ca2f9d5..b99161109eb 100644 --- a/spec/lib/banzai/pipeline/email_pipeline_spec.rb +++ b/spec/lib/banzai/pipeline/email_pipeline_spec.rb @@ -10,5 +10,19 @@ describe Banzai::Pipeline::EmailPipeline do expect(described_class.filters).not_to be_empty expect(described_class.filters).not_to include(Banzai::Filter::ImageLazyLoadFilter) end + + it 'shows punycode for autolinks' do + examples = %W[ + http://one😄two.com + http://\u0261itlab.com + ] + + examples.each do |markdown| + result = described_class.call(markdown, project: nil)[:output] + link = result.css('a').first + + expect(link.content).to include('http://xn--') + end + end end end diff --git a/spec/lib/banzai/pipeline/full_pipeline_spec.rb b/spec/lib/banzai/pipeline/full_pipeline_spec.rb index 3634655c6a5..162856be4c5 100644 --- a/spec/lib/banzai/pipeline/full_pipeline_spec.rb +++ b/spec/lib/banzai/pipeline/full_pipeline_spec.rb @@ -57,4 +57,42 @@ describe Banzai::Pipeline::FullPipeline do expect(html.lines.map(&:strip).join("\n")).to eq filtered_footnote end end + + describe 'links are detected as malicious' do + it 'has tooltips for malicious links' do + examples = %W[ + http://example.com/evil\u202E3pm.exe + [evilexe.mp3](http://example.com/evil\u202E3pm.exe) + rdar://localhost.com/\u202E3pm.exe + http://one😄two.com + [Evil-Test](http://one😄two.com) + http://\u0261itlab.com + [Evil-GitLab-link](http://\u0261itlab.com) + ![Evil-GitLab-link](http://\u0261itlab.com.png) + ] + + examples.each do |markdown| + result = described_class.call(markdown, project: nil)[:output] + link = result.css('a').first + + expect(link[:class]).to include('has-tooltip') + end + end + + it 'has no tooltips for safe links' do + examples = %w[ + http://example.com + [Safe-Test](http://example.com) + https://commons.wikimedia.org/wiki/File:اسكرام_2_-_تمنراست.jpg + [Wikipedia-link](https://commons.wikimedia.org/wiki/File:اسكرام_2_-_تمنراست.jpg) + ] + + examples.each do |markdown| + result = described_class.call(markdown, project: nil)[:output] + link = result.css('a').first + + expect(link[:class]).to be_nil + end + end + end end From ac2d8af55c7f33ecb6a5d0bad008ad5f8c4706b9 Mon Sep 17 00:00:00 2001 From: Brett Walker Date: Mon, 21 Jan 2019 13:52:05 -0600 Subject: [PATCH 18/42] Bump the CACHE_COMMONMARK_VERSION Since we needed to bump the version to 13 in the backports, and we know that an MR on master also bumped it to 13, bump to 14 to ensure that when a customer upgrades to the most recent release, the markdown gets recalcuated as necessary. --- app/models/concerns/cache_markdown_field.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/concerns/cache_markdown_field.rb b/app/models/concerns/cache_markdown_field.rb index 73a27326f6c..002f3e17891 100644 --- a/app/models/concerns/cache_markdown_field.rb +++ b/app/models/concerns/cache_markdown_field.rb @@ -15,7 +15,7 @@ module CacheMarkdownField # Increment this number every time the renderer changes its output CACHE_REDCARPET_VERSION = 3 CACHE_COMMONMARK_VERSION_START = 10 - CACHE_COMMONMARK_VERSION = 13 + CACHE_COMMONMARK_VERSION = 14 # changes to these attributes cause the cache to be invalidates INVALIDATED_BY = %w[author project].freeze From 0ebe4191630ed86eea20465327a8caa1c737a4bc Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Tue, 15 Jan 2019 19:51:37 +0530 Subject: [PATCH 19/42] Add `sanitize_name` helper to sanitize URLs in user full name --- app/helpers/emails_helper.rb | 8 ++++++++ spec/helpers/emails_helper_spec.rb | 14 ++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/app/helpers/emails_helper.rb b/app/helpers/emails_helper.rb index fa5d3ae474a..dedc58f482b 100644 --- a/app/helpers/emails_helper.rb +++ b/app/helpers/emails_helper.rb @@ -36,6 +36,14 @@ module EmailsHelper nil end + def sanitize_name(name) + if name =~ URI::DEFAULT_PARSER.regexp[:URI_REF] + name.tr('.', '_') + else + name + end + end + def password_reset_token_valid_time valid_hours = Devise.reset_password_within / 60 / 60 if valid_hours >= 24 diff --git a/spec/helpers/emails_helper_spec.rb b/spec/helpers/emails_helper_spec.rb index 3820cf5cb9d..23d7e41803e 100644 --- a/spec/helpers/emails_helper_spec.rb +++ b/spec/helpers/emails_helper_spec.rb @@ -1,6 +1,20 @@ require 'spec_helper' describe EmailsHelper do + describe 'sanitize_name' do + context 'when name contains a valid URL string' do + it 'returns name with `.` replaced with `_` to prevent mail clients from auto-linking URLs' do + expect(sanitize_name('https://about.gitlab.com')).to eq('https://about_gitlab_com') + expect(sanitize_name('www.gitlab.com')).to eq('www_gitlab_com') + expect(sanitize_name('//about.gitlab.com/handbook/security/#best-practices')).to eq('//about_gitlab_com/handbook/security/#best-practices') + end + + it 'returns name as it is when it does not contain a URL' do + expect(sanitize_name('Foo Bar')).to eq('Foo Bar') + end + end + end + describe 'password_reset_token_valid_time' do def validate_time_string(time_limit, expected_string) Devise.reset_password_within = time_limit From 2ec7bc0798ce0ef0f55c429a2819b29588043548 Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Wed, 16 Jan 2019 02:53:24 +0800 Subject: [PATCH 20/42] Prevent comments by email when issue is locked This changes the permission check so it uses the policy on Noteable instead of Project. This prevents bypassing of rules defined in Noteable for locked discussions and confidential issues. Also rechecks permissions when reply_to_discussion_id is provided since the discussion_id may be from a different noteable. --- app/policies/issue_policy.rb | 1 + app/policies/personal_snippet_policy.rb | 5 ++- app/policies/project_snippet_policy.rb | 2 + app/services/notes/build_service.rb | 15 +------- ...79-fix-email-comment-permissions-check.yml | 5 +++ lib/gitlab/email/handler/reply_processing.rb | 2 +- .../email/handler/create_note_handler_spec.rb | 37 +++++++++++++++++++ spec/models/sent_notification_spec.rb | 2 +- spec/policies/personal_snippet_policy_spec.rb | 29 +++++++++------ spec/policies/project_snippet_policy_spec.rb | 20 +++++----- spec/services/notes/build_service_spec.rb | 9 +++++ spec/services/notes/create_service_spec.rb | 4 ++ 12 files changed, 94 insertions(+), 37 deletions(-) create mode 100644 changelogs/unreleased/security-2779-fix-email-comment-permissions-check.yml diff --git a/app/policies/issue_policy.rb b/app/policies/issue_policy.rb index a0706eaa46c..dd8c5d49cf4 100644 --- a/app/policies/issue_policy.rb +++ b/app/policies/issue_policy.rb @@ -18,6 +18,7 @@ class IssuePolicy < IssuablePolicy prevent :read_issue_iid prevent :update_issue prevent :admin_issue + prevent :create_note end rule { locked }.policy do diff --git a/app/policies/personal_snippet_policy.rb b/app/policies/personal_snippet_policy.rb index 777f933cdcd..16e67a06d56 100644 --- a/app/policies/personal_snippet_policy.rb +++ b/app/policies/personal_snippet_policy.rb @@ -28,5 +28,8 @@ class PersonalSnippetPolicy < BasePolicy rule { anonymous }.prevent :comment_personal_snippet - rule { can?(:comment_personal_snippet) }.enable :award_emoji + rule { can?(:comment_personal_snippet) }.policy do + enable :create_note + enable :award_emoji + end end diff --git a/app/policies/project_snippet_policy.rb b/app/policies/project_snippet_policy.rb index 7dafa33bb99..e5e005cee6d 100644 --- a/app/policies/project_snippet_policy.rb +++ b/app/policies/project_snippet_policy.rb @@ -43,4 +43,6 @@ class ProjectSnippetPolicy < BasePolicy enable :update_project_snippet enable :admin_project_snippet end + + rule { ~can?(:read_project_snippet) }.prevent :create_note end diff --git a/app/services/notes/build_service.rb b/app/services/notes/build_service.rb index 7b92fe6fe14..bae98ede561 100644 --- a/app/services/notes/build_service.rb +++ b/app/services/notes/build_service.rb @@ -9,7 +9,7 @@ module Notes if in_reply_to_discussion_id.present? discussion = find_discussion(in_reply_to_discussion_id) - unless discussion + unless discussion && can?(current_user, :create_note, discussion.noteable) note = Note.new note.errors.add(:base, 'Discussion to reply to cannot be found') return note @@ -34,19 +34,8 @@ module Notes if project project.notes.find_discussion(discussion_id) else - discussion = Note.find_discussion(discussion_id) - noteable = discussion.noteable - - return nil unless noteable_without_project?(noteable) - - discussion + Note.find_discussion(discussion_id) end end - - def noteable_without_project?(noteable) - return true if noteable.is_a?(PersonalSnippet) && can?(current_user, :comment_personal_snippet, noteable) - - false - end end end diff --git a/changelogs/unreleased/security-2779-fix-email-comment-permissions-check.yml b/changelogs/unreleased/security-2779-fix-email-comment-permissions-check.yml new file mode 100644 index 00000000000..2f76064d8a4 --- /dev/null +++ b/changelogs/unreleased/security-2779-fix-email-comment-permissions-check.yml @@ -0,0 +1,5 @@ +--- +title: Prevent unauthorized replies when discussion is locked or confidential +merge_request: +author: +type: security diff --git a/lib/gitlab/email/handler/reply_processing.rb b/lib/gitlab/email/handler/reply_processing.rb index ba9730d2685..d8f4be8ada1 100644 --- a/lib/gitlab/email/handler/reply_processing.rb +++ b/lib/gitlab/email/handler/reply_processing.rb @@ -56,7 +56,7 @@ module Gitlab raise ProjectNotFound unless author.can?(:read_project, project) end - raise UserNotAuthorizedError unless author.can?(permission, project || noteable) + raise UserNotAuthorizedError unless author.can?(permission, try(:noteable) || project) end def verify_record!(record:, invalid_exception:, record_name:) diff --git a/spec/lib/gitlab/email/handler/create_note_handler_spec.rb b/spec/lib/gitlab/email/handler/create_note_handler_spec.rb index b1f48c15c21..e5420ea6bea 100644 --- a/spec/lib/gitlab/email/handler/create_note_handler_spec.rb +++ b/spec/lib/gitlab/email/handler/create_note_handler_spec.rb @@ -118,6 +118,43 @@ describe Gitlab::Email::Handler::CreateNoteHandler do end end + shared_examples "checks permissions on noteable" do + context "when user has access" do + before do + project.add_reporter(user) + end + + it "creates a comment" do + expect { receiver.execute }.to change { noteable.notes.count }.by(1) + end + end + + context "when user does not have access" do + it "raises UserNotAuthorizedError" do + expect { receiver.execute }.to raise_error(Gitlab::Email::UserNotAuthorizedError) + end + end + end + + context "when discussion is locked" do + before do + noteable.update_attribute(:discussion_locked, true) + end + + it_behaves_like "checks permissions on noteable" + end + + context "when issue is confidential" do + let(:issue) { create(:issue, project: project) } + let(:note) { create(:note, noteable: issue, project: project) } + + before do + issue.update_attribute(:confidential, true) + end + + it_behaves_like "checks permissions on noteable" + end + context "when everything is fine" do before do setup_attachment diff --git a/spec/models/sent_notification_spec.rb b/spec/models/sent_notification_spec.rb index 5ec04b99957..677613b7980 100644 --- a/spec/models/sent_notification_spec.rb +++ b/spec/models/sent_notification_spec.rb @@ -48,7 +48,7 @@ describe SentNotification do let(:note) { create(:diff_note_on_merge_request) } it 'creates a new SentNotification' do - expect { described_class.record_note(note, user.id) }.to change { described_class.count }.by(1) + expect { described_class.record_note(note, note.author.id) }.to change { described_class.count }.by(1) end end diff --git a/spec/policies/personal_snippet_policy_spec.rb b/spec/policies/personal_snippet_policy_spec.rb index 3809692b373..d7036cc4171 100644 --- a/spec/policies/personal_snippet_policy_spec.rb +++ b/spec/policies/personal_snippet_policy_spec.rb @@ -14,6 +14,13 @@ describe PersonalSnippetPolicy do ] end + let(:comment_permissions) do + [ + :comment_personal_snippet, + :create_note + ] + end + def permissions(user) described_class.new(user, snippet) end @@ -26,7 +33,7 @@ describe PersonalSnippetPolicy do it do is_expected.to be_allowed(:read_personal_snippet) - is_expected.to be_disallowed(:comment_personal_snippet) + is_expected.to be_disallowed(*comment_permissions) is_expected.to be_disallowed(:award_emoji) is_expected.to be_disallowed(*author_permissions) end @@ -37,7 +44,7 @@ describe PersonalSnippetPolicy do it do is_expected.to be_allowed(:read_personal_snippet) - is_expected.to be_allowed(:comment_personal_snippet) + is_expected.to be_allowed(*comment_permissions) is_expected.to be_allowed(:award_emoji) is_expected.to be_disallowed(*author_permissions) end @@ -48,7 +55,7 @@ describe PersonalSnippetPolicy do it do is_expected.to be_allowed(:read_personal_snippet) - is_expected.to be_allowed(:comment_personal_snippet) + is_expected.to be_allowed(*comment_permissions) is_expected.to be_allowed(:award_emoji) is_expected.to be_allowed(*author_permissions) end @@ -63,7 +70,7 @@ describe PersonalSnippetPolicy do it do is_expected.to be_disallowed(:read_personal_snippet) - is_expected.to be_disallowed(:comment_personal_snippet) + is_expected.to be_disallowed(*comment_permissions) is_expected.to be_disallowed(:award_emoji) is_expected.to be_disallowed(*author_permissions) end @@ -74,7 +81,7 @@ describe PersonalSnippetPolicy do it do is_expected.to be_allowed(:read_personal_snippet) - is_expected.to be_allowed(:comment_personal_snippet) + is_expected.to be_allowed(*comment_permissions) is_expected.to be_allowed(:award_emoji) is_expected.to be_disallowed(*author_permissions) end @@ -85,7 +92,7 @@ describe PersonalSnippetPolicy do it do is_expected.to be_disallowed(:read_personal_snippet) - is_expected.to be_disallowed(:comment_personal_snippet) + is_expected.to be_disallowed(*comment_permissions) is_expected.to be_disallowed(:award_emoji) is_expected.to be_disallowed(*author_permissions) end @@ -96,7 +103,7 @@ describe PersonalSnippetPolicy do it do is_expected.to be_allowed(:read_personal_snippet) - is_expected.to be_allowed(:comment_personal_snippet) + is_expected.to be_allowed(*comment_permissions) is_expected.to be_allowed(:award_emoji) is_expected.to be_allowed(*author_permissions) end @@ -111,7 +118,7 @@ describe PersonalSnippetPolicy do it do is_expected.to be_disallowed(:read_personal_snippet) - is_expected.to be_disallowed(:comment_personal_snippet) + is_expected.to be_disallowed(*comment_permissions) is_expected.to be_disallowed(:award_emoji) is_expected.to be_disallowed(*author_permissions) end @@ -122,7 +129,7 @@ describe PersonalSnippetPolicy do it do is_expected.to be_disallowed(:read_personal_snippet) - is_expected.to be_disallowed(:comment_personal_snippet) + is_expected.to be_disallowed(*comment_permissions) is_expected.to be_disallowed(:award_emoji) is_expected.to be_disallowed(*author_permissions) end @@ -133,7 +140,7 @@ describe PersonalSnippetPolicy do it do is_expected.to be_disallowed(:read_personal_snippet) - is_expected.to be_disallowed(:comment_personal_snippet) + is_expected.to be_disallowed(*comment_permissions) is_expected.to be_disallowed(:award_emoji) is_expected.to be_disallowed(*author_permissions) end @@ -144,7 +151,7 @@ describe PersonalSnippetPolicy do it do is_expected.to be_allowed(:read_personal_snippet) - is_expected.to be_allowed(:comment_personal_snippet) + is_expected.to be_allowed(*comment_permissions) is_expected.to be_allowed(:award_emoji) is_expected.to be_allowed(*author_permissions) end diff --git a/spec/policies/project_snippet_policy_spec.rb b/spec/policies/project_snippet_policy_spec.rb index 4d32e06b553..d6329e84579 100644 --- a/spec/policies/project_snippet_policy_spec.rb +++ b/spec/policies/project_snippet_policy_spec.rb @@ -41,7 +41,7 @@ describe ProjectSnippetPolicy do subject { abilities(regular_user, :public) } it do - expect_allowed(:read_project_snippet) + expect_allowed(:read_project_snippet, :create_note) expect_disallowed(*author_permissions) end end @@ -50,7 +50,7 @@ describe ProjectSnippetPolicy do subject { abilities(external_user, :public) } it do - expect_allowed(:read_project_snippet) + expect_allowed(:read_project_snippet, :create_note) expect_disallowed(*author_permissions) end end @@ -70,7 +70,7 @@ describe ProjectSnippetPolicy do subject { abilities(regular_user, :internal) } it do - expect_allowed(:read_project_snippet) + expect_allowed(:read_project_snippet, :create_note) expect_disallowed(*author_permissions) end end @@ -79,7 +79,7 @@ describe ProjectSnippetPolicy do subject { abilities(external_user, :internal) } it do - expect_disallowed(:read_project_snippet) + expect_disallowed(:read_project_snippet, :create_note) expect_disallowed(*author_permissions) end end @@ -92,7 +92,7 @@ describe ProjectSnippetPolicy do end it do - expect_allowed(:read_project_snippet) + expect_allowed(:read_project_snippet, :create_note) expect_disallowed(*author_permissions) end end @@ -112,7 +112,7 @@ describe ProjectSnippetPolicy do subject { abilities(regular_user, :private) } it do - expect_disallowed(:read_project_snippet) + expect_disallowed(:read_project_snippet, :create_note) expect_disallowed(*author_permissions) end end @@ -123,7 +123,7 @@ describe ProjectSnippetPolicy do subject { described_class.new(regular_user, snippet) } it do - expect_allowed(:read_project_snippet) + expect_allowed(:read_project_snippet, :create_note) expect_allowed(*author_permissions) end end @@ -136,7 +136,7 @@ describe ProjectSnippetPolicy do end it do - expect_allowed(:read_project_snippet) + expect_allowed(:read_project_snippet, :create_note) expect_disallowed(*author_permissions) end end @@ -149,7 +149,7 @@ describe ProjectSnippetPolicy do end it do - expect_allowed(:read_project_snippet) + expect_allowed(:read_project_snippet, :create_note) expect_disallowed(*author_permissions) end end @@ -158,7 +158,7 @@ describe ProjectSnippetPolicy do subject { abilities(create(:admin), :private) } it do - expect_allowed(:read_project_snippet) + expect_allowed(:read_project_snippet, :create_note) expect_allowed(*author_permissions) end end diff --git a/spec/services/notes/build_service_spec.rb b/spec/services/notes/build_service_spec.rb index ff85c261cd4..9aaccb4bffe 100644 --- a/spec/services/notes/build_service_spec.rb +++ b/spec/services/notes/build_service_spec.rb @@ -45,6 +45,15 @@ describe Notes::BuildService do end end + context 'when user has no access to discussion' do + it 'sets an error' do + another_user = create(:user) + new_note = described_class.new(project, another_user, note: 'Test', in_reply_to_discussion_id: note.discussion_id).execute + + expect(new_note.errors[:base]).to include('Discussion to reply to cannot be found') + end + end + context 'personal snippet note' do def reply(note, user = nil) user ||= create(:user) diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index 80b015d4cd0..1b9ba42cfd6 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -127,6 +127,10 @@ describe Notes::CreateService do create(:diff_note_on_merge_request, noteable: merge_request, project: project_with_repo) end + before do + project_with_repo.add_maintainer(user) + end + context 'when eligible to have a note diff file' do let(:new_opts) do opts.merge(in_reply_to_discussion_id: nil, From 3c30c341a090b592a3ea5eddb95bd8ad7e7038ce Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Tue, 15 Jan 2019 19:52:26 +0530 Subject: [PATCH 21/42] Use `sanitize_name` to sanitize URL in user full name --- .../mailer/_confirmation_instructions_secondary.html.haml | 2 +- app/views/notify/_note_email.text.erb | 4 ++-- app/views/notify/autodevops_disabled_email.text.erb | 2 +- app/views/notify/closed_issue_email.html.haml | 2 +- app/views/notify/closed_issue_email.text.haml | 2 +- app/views/notify/closed_merge_request_email.html.haml | 2 +- app/views/notify/closed_merge_request_email.text.haml | 6 +++--- app/views/notify/issue_status_changed_email.html.haml | 2 +- app/views/notify/issue_status_changed_email.text.erb | 2 +- app/views/notify/member_access_requested_email.text.erb | 2 +- app/views/notify/member_invite_accepted_email.text.erb | 2 +- app/views/notify/member_invited_email.text.erb | 2 +- app/views/notify/merge_request_status_email.html.haml | 2 +- app/views/notify/merge_request_status_email.text.haml | 6 +++--- .../notify/merge_request_unmergeable_email.text.haml | 4 ++-- app/views/notify/merged_merge_request_email.text.haml | 4 ++-- app/views/notify/new_gpg_key_email.html.haml | 2 +- app/views/notify/new_gpg_key_email.text.erb | 2 +- app/views/notify/new_issue_email.text.erb | 2 +- app/views/notify/new_mention_in_issue_email.text.erb | 4 ++-- .../notify/new_mention_in_merge_request_email.text.erb | 4 ++-- app/views/notify/new_merge_request_email.html.haml | 2 +- app/views/notify/new_ssh_key_email.html.haml | 2 +- app/views/notify/new_ssh_key_email.text.erb | 2 +- app/views/notify/new_user_email.html.haml | 2 +- app/views/notify/new_user_email.text.erb | 2 +- app/views/notify/pipeline_failed_email.text.erb | 6 +++--- app/views/notify/pipeline_success_email.text.erb | 6 +++--- app/views/notify/push_to_merge_request_email.html.haml | 2 +- app/views/notify/push_to_merge_request_email.text.haml | 2 +- app/views/notify/reassigned_issue_email.html.haml | 2 +- app/views/notify/reassigned_issue_email.text.erb | 2 +- app/views/notify/reassigned_merge_request_email.html.haml | 4 ++-- app/views/notify/reassigned_merge_request_email.text.erb | 4 ++-- app/views/notify/resolved_all_discussions_email.html.haml | 2 +- app/views/notify/resolved_all_discussions_email.text.erb | 2 +- spec/mailers/notify_spec.rb | 8 +++++--- 37 files changed, 56 insertions(+), 54 deletions(-) diff --git a/app/views/devise/mailer/_confirmation_instructions_secondary.html.haml b/app/views/devise/mailer/_confirmation_instructions_secondary.html.haml index 3d0a1f622a5..ccc3e734276 100644 --- a/app/views/devise/mailer/_confirmation_instructions_secondary.html.haml +++ b/app/views/devise/mailer/_confirmation_instructions_secondary.html.haml @@ -1,5 +1,5 @@ #content - = email_default_heading("#{@resource.user.name}, you've added an additional email!") + = email_default_heading("#{sanitize_name(@resource.user.name)}, you've added an additional email!") %p Click the link below to confirm your email address (#{@resource.email}) #cta = link_to 'Confirm your email address', confirmation_url(@resource, confirmation_token: @token) diff --git a/app/views/notify/_note_email.text.erb b/app/views/notify/_note_email.text.erb index 50209c46ed1..5a67214059c 100644 --- a/app/views/notify/_note_email.text.erb +++ b/app/views/notify/_note_email.text.erb @@ -3,7 +3,7 @@ <% discussion = note.discussion if note.part_of_discussion? -%> <% if discussion && !discussion.individual_note? -%> -<%= note.author_name -%> +<%= sanitize_name(note.author_name) -%> <% if discussion.new_discussion? -%> <%= " started a new discussion" -%> <% else -%> @@ -16,7 +16,7 @@ <% elsif Gitlab::CurrentSettings.email_author_in_body -%> -<%= "#{note.author_name} commented:" -%> +<%= "#{sanitize_name(note.author_name)} commented:" -%> <% end -%> diff --git a/app/views/notify/autodevops_disabled_email.text.erb b/app/views/notify/autodevops_disabled_email.text.erb index 695780c3145..bf863952478 100644 --- a/app/views/notify/autodevops_disabled_email.text.erb +++ b/app/views/notify/autodevops_disabled_email.text.erb @@ -3,7 +3,7 @@ Auto DevOps pipeline was disabled for <%= @project.name %> The Auto DevOps pipeline failed for pipeline <%= @pipeline.iid %> (<%= pipeline_url(@pipeline) %>) and has been disabled for <%= @project.name %>. In order to use the Auto DevOps pipeline with your project, please review the currently supported languagues (https://docs.gitlab.com/ee/topics/autodevops/#currently-supported-languages), adjust your project accordingly, and turn on the Auto DevOps pipeline within your CI/CD project settings (<%= project_settings_ci_cd_url(@project) %>). <% if @pipeline.user -%> - Pipeline #<%= @pipeline.id %> ( <%= pipeline_url(@pipeline) %> ) triggered by <%= @pipeline.user.name %> ( <%= user_url(@pipeline.user) %> ) + Pipeline #<%= @pipeline.id %> ( <%= pipeline_url(@pipeline) %> ) triggered by <%= sanitize_name(@pipeline.user.name) %> ( <%= user_url(@pipeline.user) %> ) <% else -%> Pipeline #<%= @pipeline.id %> ( <%= pipeline_url(@pipeline) %> ) triggered by API <% end -%> diff --git a/app/views/notify/closed_issue_email.html.haml b/app/views/notify/closed_issue_email.html.haml index b7284dd819b..eb148d72da1 100644 --- a/app/views/notify/closed_issue_email.html.haml +++ b/app/views/notify/closed_issue_email.html.haml @@ -1,2 +1,2 @@ %p - Issue was closed by #{@updated_by.name} + Issue was closed by #{sanitize_name(@updated_by.name)} diff --git a/app/views/notify/closed_issue_email.text.haml b/app/views/notify/closed_issue_email.text.haml index b35d4b7502d..b1f0a3f37ec 100644 --- a/app/views/notify/closed_issue_email.text.haml +++ b/app/views/notify/closed_issue_email.text.haml @@ -1,3 +1,3 @@ -Issue was closed by #{@updated_by.name} +Issue was closed by #{sanitize_name(@updated_by.name)} Issue ##{@issue.iid}: #{project_issue_url(@issue.project, @issue)} diff --git a/app/views/notify/closed_merge_request_email.html.haml b/app/views/notify/closed_merge_request_email.html.haml index 44e018304e1..2aa753e0d55 100644 --- a/app/views/notify/closed_merge_request_email.html.haml +++ b/app/views/notify/closed_merge_request_email.html.haml @@ -1,2 +1,2 @@ %p - Merge Request #{@merge_request.to_reference} was closed by #{@updated_by.name} + Merge Request #{@merge_request.to_reference} was closed by #{sanitize_name(@updated_by.name)} diff --git a/app/views/notify/closed_merge_request_email.text.haml b/app/views/notify/closed_merge_request_email.text.haml index c4e06cb3cb1..1094d584a1c 100644 --- a/app/views/notify/closed_merge_request_email.text.haml +++ b/app/views/notify/closed_merge_request_email.text.haml @@ -1,8 +1,8 @@ -Merge Request #{@merge_request.to_reference} was closed by #{@updated_by.name} +Merge Request #{@merge_request.to_reference} was closed by #{sanitize_name(@updated_by.name)} Merge Request url: #{project_merge_request_url(@merge_request.target_project, @merge_request)} = merge_path_description(@merge_request, 'to') -Author: #{@merge_request.author_name} -Assignee: #{@merge_request.assignee_name} +Author: #{sanitize_name(@merge_request.author_name)} +Assignee: #{sanitize_name(@merge_request.assignee_name)} diff --git a/app/views/notify/issue_status_changed_email.html.haml b/app/views/notify/issue_status_changed_email.html.haml index b6051b11cea..66e73a9b03f 100644 --- a/app/views/notify/issue_status_changed_email.html.haml +++ b/app/views/notify/issue_status_changed_email.html.haml @@ -1,2 +1,2 @@ %p - Issue was #{@issue_status} by #{@updated_by.name} + Issue was #{@issue_status} by #{sanitize_name(@updated_by.name)} diff --git a/app/views/notify/issue_status_changed_email.text.erb b/app/views/notify/issue_status_changed_email.text.erb index 4200881f7e8..f38b09e9820 100644 --- a/app/views/notify/issue_status_changed_email.text.erb +++ b/app/views/notify/issue_status_changed_email.text.erb @@ -1,4 +1,4 @@ -Issue was <%= @issue_status %> by <%= @updated_by.name %> +Issue was <%= @issue_status %> by <%= sanitize_name(@updated_by.name) %> Issue <%= @issue.iid %>: <%= url_for(project_issue_url(@issue.project, @issue)) %> diff --git a/app/views/notify/member_access_requested_email.text.erb b/app/views/notify/member_access_requested_email.text.erb index 9c5ee0eaf26..ddb4a7b3d2c 100644 --- a/app/views/notify/member_access_requested_email.text.erb +++ b/app/views/notify/member_access_requested_email.text.erb @@ -1,3 +1,3 @@ -<%= member.user.name %> (<%= user_url(member.user) %>) requested <%= member.human_access %> access to the <%= member_source.human_name %> <%= member_source.model_name.singular %>. +<%= sanitize_name(member.user.name) %> (<%= user_url(member.user) %>) requested <%= member.human_access %> access to the <%= member_source.human_name %> <%= member_source.model_name.singular %>. <%= polymorphic_url([member_source, :members]) %> diff --git a/app/views/notify/member_invite_accepted_email.text.erb b/app/views/notify/member_invite_accepted_email.text.erb index cef87101427..c824533eac2 100644 --- a/app/views/notify/member_invite_accepted_email.text.erb +++ b/app/views/notify/member_invite_accepted_email.text.erb @@ -1,3 +1,3 @@ -<%= member.invite_email %>, now known as <%= member.user.name %>, has accepted your invitation to join the <%= member_source.human_name %> <%= member_source.model_name.singular %>. +<%= member.invite_email %>, now known as <%= sanitize_name(member.user.name) %>, has accepted your invitation to join the <%= member_source.human_name %> <%= member_source.model_name.singular %>. <%= member_source.web_url %> diff --git a/app/views/notify/member_invited_email.text.erb b/app/views/notify/member_invited_email.text.erb index 0a6393355be..d944c3b4a50 100644 --- a/app/views/notify/member_invited_email.text.erb +++ b/app/views/notify/member_invited_email.text.erb @@ -1,4 +1,4 @@ -You have been invited <%= "by #{member.created_by.name} " if member.created_by %>to join the <%= member_source.human_name %> <%= member_source.model_name.singular %> as <%= member.human_access %>. +You have been invited <%= "by #{sanitize_name(member.created_by.name)} " if member.created_by %>to join the <%= member_source.human_name %> <%= member_source.model_name.singular %> as <%= member.human_access %>. Accept invitation: <%= invite_url(@token) %> Decline invitation: <%= decline_invite_url(@token) %> diff --git a/app/views/notify/merge_request_status_email.html.haml b/app/views/notify/merge_request_status_email.html.haml index b487e26b122..ffb416abf72 100644 --- a/app/views/notify/merge_request_status_email.html.haml +++ b/app/views/notify/merge_request_status_email.html.haml @@ -1,2 +1,2 @@ %p - Merge Request #{@merge_request.to_reference} was #{@mr_status} by #{@updated_by.name} + Merge Request #{@merge_request.to_reference} was #{@mr_status} by #{sanitize_name(@updated_by.name)} diff --git a/app/views/notify/merge_request_status_email.text.haml b/app/views/notify/merge_request_status_email.text.haml index ae2a2933865..b9b9e0c3ad7 100644 --- a/app/views/notify/merge_request_status_email.text.haml +++ b/app/views/notify/merge_request_status_email.text.haml @@ -1,8 +1,8 @@ -Merge Request #{@merge_request.to_reference} was #{@mr_status} by #{@updated_by.name} +Merge Request #{@merge_request.to_reference} was #{@mr_status} by #{sanitize_name(@updated_by.name)} Merge Request url: #{project_merge_request_url(@merge_request.target_project, @merge_request)} = merge_path_description(@merge_request, 'to') -Author: #{@merge_request.author_name} -Assignee: #{@merge_request.assignee_name} +Author: #{sanitize_name(@merge_request.author_name)} +Assignee: #{sanitize_name(@merge_request.assignee_name)} diff --git a/app/views/notify/merge_request_unmergeable_email.text.haml b/app/views/notify/merge_request_unmergeable_email.text.haml index dcdd6db69d6..0c7bf1bb044 100644 --- a/app/views/notify/merge_request_unmergeable_email.text.haml +++ b/app/views/notify/merge_request_unmergeable_email.text.haml @@ -4,5 +4,5 @@ Merge Request url: #{project_merge_request_url(@merge_request.target_project, @m = merge_path_description(@merge_request, 'to') -Author: #{@merge_request.author_name} -Assignee: #{@merge_request.assignee_name} +Author: #{sanitize_name(@merge_request.author_name)} +Assignee: #{sanitize_name(@merge_request.assignee_name)} diff --git a/app/views/notify/merged_merge_request_email.text.haml b/app/views/notify/merged_merge_request_email.text.haml index 661c23bcbe2..045a43cbc84 100644 --- a/app/views/notify/merged_merge_request_email.text.haml +++ b/app/views/notify/merged_merge_request_email.text.haml @@ -4,5 +4,5 @@ Merge Request url: #{project_merge_request_url(@merge_request.target_project, @m = merge_path_description(@merge_request, 'to') -Author: #{@merge_request.author_name} -Assignee: #{@merge_request.assignee_name} +Author: #{sanitize_name(@merge_request.author_name)} +Assignee: #{sanitize_name(@merge_request.assignee_name)} diff --git a/app/views/notify/new_gpg_key_email.html.haml b/app/views/notify/new_gpg_key_email.html.haml index 4b9350c4e88..b857705e01f 100644 --- a/app/views/notify/new_gpg_key_email.html.haml +++ b/app/views/notify/new_gpg_key_email.html.haml @@ -1,5 +1,5 @@ %p - Hi #{@user.name}! + Hi #{sanitize_name(@user.name)}! %p A new GPG key was added to your account: %p diff --git a/app/views/notify/new_gpg_key_email.text.erb b/app/views/notify/new_gpg_key_email.text.erb index 80b5a1fd7ff..92ea851eee4 100644 --- a/app/views/notify/new_gpg_key_email.text.erb +++ b/app/views/notify/new_gpg_key_email.text.erb @@ -1,4 +1,4 @@ -Hi <%= @user.name %>! +Hi <%= sanitize_name(@user.name) %>! A new GPG key was added to your account: diff --git a/app/views/notify/new_issue_email.text.erb b/app/views/notify/new_issue_email.text.erb index 3c716f77296..58a2bcbe5eb 100644 --- a/app/views/notify/new_issue_email.text.erb +++ b/app/views/notify/new_issue_email.text.erb @@ -1,7 +1,7 @@ New Issue was created. Issue <%= @issue.iid %>: <%= url_for(project_issue_url(@issue.project, @issue)) %> -Author: <%= @issue.author_name %> +Author: <%= sanitize_name(@issue.author_name) %> Assignee: <%= @issue.assignee_list %> <%= @issue.description %> diff --git a/app/views/notify/new_mention_in_issue_email.text.erb b/app/views/notify/new_mention_in_issue_email.text.erb index 23213106c5b..173091e4a80 100644 --- a/app/views/notify/new_mention_in_issue_email.text.erb +++ b/app/views/notify/new_mention_in_issue_email.text.erb @@ -1,7 +1,7 @@ You have been mentioned in an issue. Issue <%= @issue.iid %>: <%= url_for(project_issue_url(@issue.project, @issue)) %> -Author: <%= @issue.author_name %> -Assignee: <%= @issue.assignee_list %> +Author: <%= sanitize_name(@issue.author_name) %> +Assignee: <%= sanitize_name(@issue.assignee_list) %> <%= @issue.description %> diff --git a/app/views/notify/new_mention_in_merge_request_email.text.erb b/app/views/notify/new_mention_in_merge_request_email.text.erb index 6fcebb22fc4..96a4f3f9eac 100644 --- a/app/views/notify/new_mention_in_merge_request_email.text.erb +++ b/app/views/notify/new_mention_in_merge_request_email.text.erb @@ -3,7 +3,7 @@ You have been mentioned in Merge Request <%= @merge_request.to_reference %> <%= url_for(project_merge_request_url(@merge_request.target_project, @merge_request)) %> <%= merge_path_description(@merge_request, 'to') %> -Author: <%= @merge_request.author_name %> -Assignee: <%= @merge_request.assignee_name %> +Author: <%= sanitize_name(@merge_request.author_name) %> +Assignee: <%= sanitize_name(@merge_request.assignee_name) %> <%= @merge_request.description %> diff --git a/app/views/notify/new_merge_request_email.html.haml b/app/views/notify/new_merge_request_email.html.haml index 5acd45b74a7..db23447dd39 100644 --- a/app/views/notify/new_merge_request_email.html.haml +++ b/app/views/notify/new_merge_request_email.html.haml @@ -7,7 +7,7 @@ - if @merge_request.assignee_id.present? %p - Assignee: #{@merge_request.assignee_name} + Assignee: #{sanitize_name(@merge_request.assignee_name)} = render_if_exists 'notify/merge_request_approvers', presenter: @mr_presenter diff --git a/app/views/notify/new_ssh_key_email.html.haml b/app/views/notify/new_ssh_key_email.html.haml index 63b0cbbd205..d031842be95 100644 --- a/app/views/notify/new_ssh_key_email.html.haml +++ b/app/views/notify/new_ssh_key_email.html.haml @@ -1,5 +1,5 @@ %p - Hi #{@user.name}! + Hi #{sanitize_name(@user.name)}! %p A new public key was added to your account: %p diff --git a/app/views/notify/new_ssh_key_email.text.erb b/app/views/notify/new_ssh_key_email.text.erb index 05b551c89a0..690357d69ed 100644 --- a/app/views/notify/new_ssh_key_email.text.erb +++ b/app/views/notify/new_ssh_key_email.text.erb @@ -1,4 +1,4 @@ -Hi <%= @user.name %>! +Hi <%= sanitize_name(@user.name) %>! A new public key was added to your account: diff --git a/app/views/notify/new_user_email.html.haml b/app/views/notify/new_user_email.html.haml index db4424a01f9..dfbb5c75bd3 100644 --- a/app/views/notify/new_user_email.html.haml +++ b/app/views/notify/new_user_email.html.haml @@ -1,5 +1,5 @@ %p - Hi #{@user['name']}! + Hi #{sanitize_name(@user['name'])}! %p - if Gitlab::CurrentSettings.allow_signup? Your account has been created successfully. diff --git a/app/views/notify/new_user_email.text.erb b/app/views/notify/new_user_email.text.erb index dd9b71e3b84..f3f20f3bfba 100644 --- a/app/views/notify/new_user_email.text.erb +++ b/app/views/notify/new_user_email.text.erb @@ -1,4 +1,4 @@ -Hi <%= @user.name %>! +Hi <%= sanitize_name(@user.name) %>! The Administrator created an account for you. Now you are a member of the company GitLab application. diff --git a/app/views/notify/pipeline_failed_email.text.erb b/app/views/notify/pipeline_failed_email.text.erb index 294238eee51..722eedf90be 100644 --- a/app/views/notify/pipeline_failed_email.text.erb +++ b/app/views/notify/pipeline_failed_email.text.erb @@ -10,20 +10,20 @@ Commit: <%= @pipeline.short_sha %> ( <%= commit_url(@pipeline) %> ) Commit Message: <%= @pipeline.git_commit_message.truncate(50) %> <% commit = @pipeline.commit -%> <% if commit.author -%> -Commit Author: <%= commit.author.name %> ( <%= user_url(commit.author) %> ) +Commit Author: <%= sanitize_name(commit.author.name) %> ( <%= user_url(commit.author) %> ) <% else -%> Commit Author: <%= commit.author_name %> <% end -%> <% if commit.different_committer? -%> <% if commit.committer -%> -Committed by: <%= commit.committer.name %> ( <%= user_url(commit.committer) %> ) +Committed by: <%= sanitize_name(commit.committer.name) %> ( <%= user_url(commit.committer) %> ) <% else -%> Committed by: <%= commit.committer_name %> <% end -%> <% end -%> <% if @pipeline.user -%> -Pipeline #<%= @pipeline.id %> ( <%= pipeline_url(@pipeline) %> ) triggered by <%= @pipeline.user.name %> ( <%= user_url(@pipeline.user) %> ) +Pipeline #<%= @pipeline.id %> ( <%= pipeline_url(@pipeline) %> ) triggered by <%= sanitize_name(@pipeline.user.name) %> ( <%= user_url(@pipeline.user) %> ) <% else -%> Pipeline #<%= @pipeline.id %> ( <%= pipeline_url(@pipeline) %> ) triggered by API <% end -%> diff --git a/app/views/notify/pipeline_success_email.text.erb b/app/views/notify/pipeline_success_email.text.erb index 39622cf7f02..9aadf380f79 100644 --- a/app/views/notify/pipeline_success_email.text.erb +++ b/app/views/notify/pipeline_success_email.text.erb @@ -10,13 +10,13 @@ Commit: <%= @pipeline.short_sha %> ( <%= commit_url(@pipeline) %> ) Commit Message: <%= @pipeline.git_commit_message.truncate(50) %> <% commit = @pipeline.commit -%> <% if commit.author -%> -Commit Author: <%= commit.author.name %> ( <%= user_url(commit.author) %> ) +Commit Author: <%= sanitize_name(commit.author.name) %> ( <%= user_url(commit.author) %> ) <% else -%> Commit Author: <%= commit.author_name %> <% end -%> <% if commit.different_committer? -%> <% if commit.committer -%> -Committed by: <%= commit.committer.name %> ( <%= user_url(commit.committer) %> ) +Committed by: <%= sanitize_name(commit.committer.name) %> ( <%= user_url(commit.committer) %> ) <% else -%> Committed by: <%= commit.committer_name %> <% end -%> @@ -25,7 +25,7 @@ Committed by: <%= commit.committer_name %> <% job_count = @pipeline.total_size -%> <% stage_count = @pipeline.stages_count -%> <% if @pipeline.user -%> -Pipeline #<%= @pipeline.id %> ( <%= pipeline_url(@pipeline) %> ) triggered by <%= @pipeline.user.name %> ( <%= user_url(@pipeline.user) %> ) +Pipeline #<%= @pipeline.id %> ( <%= pipeline_url(@pipeline) %> ) triggered by <%= sanitize_name(@pipeline.user.name) %> ( <%= user_url(@pipeline.user) %> ) <% else -%> Pipeline #<%= @pipeline.id %> ( <%= pipeline_url(@pipeline) %> ) triggered by API <% end -%> diff --git a/app/views/notify/push_to_merge_request_email.html.haml b/app/views/notify/push_to_merge_request_email.html.haml index 67744ec1cee..97258833cfc 100644 --- a/app/views/notify/push_to_merge_request_email.html.haml +++ b/app/views/notify/push_to_merge_request_email.html.haml @@ -1,5 +1,5 @@ %h3 - = @updated_by_user.name + = sanitize_name(@updated_by_user.name) pushed new commits to merge request = link_to(@merge_request.to_reference, project_merge_request_url(@merge_request.target_project, @merge_request)) diff --git a/app/views/notify/push_to_merge_request_email.text.haml b/app/views/notify/push_to_merge_request_email.text.haml index 95759d127e2..10c8e158846 100644 --- a/app/views/notify/push_to_merge_request_email.text.haml +++ b/app/views/notify/push_to_merge_request_email.text.haml @@ -1,4 +1,4 @@ -#{@updated_by_user.name} pushed new commits to merge request #{@merge_request.to_reference} +#{sanitize_name(@updated_by_user.name)} pushed new commits to merge request #{@merge_request.to_reference} \ #{url_for(project_merge_request_url(@merge_request.target_project, @merge_request))} \ diff --git a/app/views/notify/reassigned_issue_email.html.haml b/app/views/notify/reassigned_issue_email.html.haml index ee2f40e1683..6d25488a7e2 100644 --- a/app/views/notify/reassigned_issue_email.html.haml +++ b/app/views/notify/reassigned_issue_email.html.haml @@ -2,7 +2,7 @@ Assignee changed - if @previous_assignees.any? from - %strong= @previous_assignees.map(&:name).to_sentence + %strong= sanitize_name(@previous_assignees.map(&:name).to_sentence) to - if @issue.assignees.any? %strong= @issue.assignee_list diff --git a/app/views/notify/reassigned_issue_email.text.erb b/app/views/notify/reassigned_issue_email.text.erb index 6c357f1074a..7bf2e8e6ce3 100644 --- a/app/views/notify/reassigned_issue_email.text.erb +++ b/app/views/notify/reassigned_issue_email.text.erb @@ -2,5 +2,5 @@ Reassigned Issue <%= @issue.iid %> <%= url_for([@issue.project.namespace.becomes(Namespace), @issue.project, @issue, { only_path: false }]) %> -Assignee changed <%= "from #{@previous_assignees.map(&:name).to_sentence}" if @previous_assignees.any? -%> +Assignee changed <%= "from #{sanitize_name(@previous_assignees.map(&:name).to_sentence)}" if @previous_assignees.any? -%> to <%= "#{@issue.assignees.any? ? @issue.assignee_list : 'Unassigned'}" %> diff --git a/app/views/notify/reassigned_merge_request_email.html.haml b/app/views/notify/reassigned_merge_request_email.html.haml index 24c2b08810b..e4f19bc3200 100644 --- a/app/views/notify/reassigned_merge_request_email.html.haml +++ b/app/views/notify/reassigned_merge_request_email.html.haml @@ -2,9 +2,9 @@ Assignee changed - if @previous_assignee from - %strong= @previous_assignee.name + %strong= sanitize_name(@previous_assignee.name) to - if @merge_request.assignee_id - %strong= @merge_request.assignee_name + %strong= sanitize_name(@merge_request.assignee_name) - else %strong Unassigned diff --git a/app/views/notify/reassigned_merge_request_email.text.erb b/app/views/notify/reassigned_merge_request_email.text.erb index 998a40fefde..96c770b5219 100644 --- a/app/views/notify/reassigned_merge_request_email.text.erb +++ b/app/views/notify/reassigned_merge_request_email.text.erb @@ -2,5 +2,5 @@ Reassigned Merge Request <%= @merge_request.iid %> <%= url_for([@merge_request.project.namespace.becomes(Namespace), @merge_request.project, @merge_request, { only_path: false }]) %> -Assignee changed <%= "from #{@previous_assignee.name}" if @previous_assignee -%> - to <%= "#{@merge_request.assignee_id ? @merge_request.assignee_name : 'Unassigned'}" %> +Assignee changed <%= "from #{sanitize_name(@previous_assignee.name)}" if @previous_assignee -%> + to <%= "#{@merge_request.assignee_id ? sanitize_name(@merge_request.assignee_name) : 'Unassigned'}" %> diff --git a/app/views/notify/resolved_all_discussions_email.html.haml b/app/views/notify/resolved_all_discussions_email.html.haml index 522421b7cc3..502b8f21e35 100644 --- a/app/views/notify/resolved_all_discussions_email.html.haml +++ b/app/views/notify/resolved_all_discussions_email.html.haml @@ -1,2 +1,2 @@ %p - All discussions on Merge Request #{@merge_request.to_reference} were resolved by #{@resolved_by.name} + All discussions on Merge Request #{@merge_request.to_reference} were resolved by #{sanitize_name(@resolved_by.name)} diff --git a/app/views/notify/resolved_all_discussions_email.text.erb b/app/views/notify/resolved_all_discussions_email.text.erb index 2881f3e699e..c4b36bfe1a8 100644 --- a/app/views/notify/resolved_all_discussions_email.text.erb +++ b/app/views/notify/resolved_all_discussions_email.text.erb @@ -1,3 +1,3 @@ -All discussions on Merge Request <%= @merge_request.to_reference %> were resolved by <%= @resolved_by.name %> +All discussions on Merge Request <%= @merge_request.to_reference %> were resolved by <%= sanitize_name(@resolved_by.name) %> <%= url_for(project_merge_request_url(@merge_request.target_project, @merge_request)) %> diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index f2d99872401..ec3972ac8db 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -9,8 +9,10 @@ describe Notify do include_context 'gitlab email notification' + let(:current_user_sanitized) { 'www_example_com' } + set(:user) { create(:user) } - set(:current_user) { create(:user, email: "current@email.com") } + set(:current_user) { create(:user, email: "current@email.com", name: 'www.example.com') } set(:assignee) { create(:user, email: 'assignee@example.com', name: 'John Doe') } set(:merge_request) do @@ -182,7 +184,7 @@ describe Notify do aggregate_failures do is_expected.to have_referable_subject(issue, reply: true) is_expected.to have_body_text(status) - is_expected.to have_body_text(current_user.name) + is_expected.to have_body_text(current_user_sanitized) is_expected.to have_body_text(project_issue_path project, issue) end end @@ -361,7 +363,7 @@ describe Notify do aggregate_failures do is_expected.to have_referable_subject(merge_request, reply: true) is_expected.to have_body_text(status) - is_expected.to have_body_text(current_user.name) + is_expected.to have_body_text(current_user_sanitized) is_expected.to have_body_text(project_merge_request_path(project, merge_request)) end end From 7b8b2ad69c600e3eaa84509d93dc8e0722f72762 Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Tue, 22 Jan 2019 18:59:42 +0530 Subject: [PATCH 22/42] Add changelog entry --- .../unreleased/security-22076-sanitize-url-in-names.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelogs/unreleased/security-22076-sanitize-url-in-names.yml diff --git a/changelogs/unreleased/security-22076-sanitize-url-in-names.yml b/changelogs/unreleased/security-22076-sanitize-url-in-names.yml new file mode 100644 index 00000000000..4e0ad4dd4c4 --- /dev/null +++ b/changelogs/unreleased/security-22076-sanitize-url-in-names.yml @@ -0,0 +1,6 @@ +--- +title: Sanitize user full name to clean up any URL to prevent mail clients from auto-linking + URLs +merge_request: 2793 +author: +type: security From 1a8100cff59d983191b43dacdddbdab65ea23c6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Wed, 2 Jan 2019 20:01:11 +0100 Subject: [PATCH 23/42] Extract GitLab Pages using RubyZip RubyZip allows us to perform strong validation of expanded paths where we do extract file. We introduce the following additional checks to extract routines: 1. None of path components can be symlinked, 2. We drop privileges support for directories, 3. Symlink source needs to point within the target directory, like `public/`, 4. The symlink source needs to exist ahead of time. --- Gemfile | 1 + Gemfile.lock | 1 + app/services/projects/update_pages_service.rb | 41 ++-- .../unreleased/extract-pages-with-rubyzip.yml | 5 + lib/safe_zip/entry.rb | 97 +++++++++ lib/safe_zip/extract.rb | 73 +++++++ lib/safe_zip/extract_params.rb | 36 ++++ spec/fixtures/pages_non_writeable.zip | Bin 0 -> 727 bytes .../invalid-symlink-does-not-exist.zip | Bin 0 -> 1183 bytes .../safe_zip/invalid-symlinks-outside.zip | Bin 0 -> 1309 bytes .../fixtures/safe_zip/valid-non-writeable.zip | Bin 0 -> 727 bytes spec/fixtures/safe_zip/valid-simple.zip | Bin 0 -> 1144 bytes .../safe_zip/valid-symlinks-first.zip | Bin 0 -> 528 bytes spec/lib/safe_zip/entry_spec.rb | 196 ++++++++++++++++++ spec/lib/safe_zip/extract_params_spec.rb | 54 +++++ spec/lib/safe_zip/extract_spec.rb | 80 +++++++ .../projects/update_pages_service_spec.rb | 35 +++- 17 files changed, 594 insertions(+), 25 deletions(-) create mode 100644 changelogs/unreleased/extract-pages-with-rubyzip.yml create mode 100644 lib/safe_zip/entry.rb create mode 100644 lib/safe_zip/extract.rb create mode 100644 lib/safe_zip/extract_params.rb create mode 100644 spec/fixtures/pages_non_writeable.zip create mode 100644 spec/fixtures/safe_zip/invalid-symlink-does-not-exist.zip create mode 100644 spec/fixtures/safe_zip/invalid-symlinks-outside.zip create mode 100644 spec/fixtures/safe_zip/valid-non-writeable.zip create mode 100644 spec/fixtures/safe_zip/valid-simple.zip create mode 100644 spec/fixtures/safe_zip/valid-symlinks-first.zip create mode 100644 spec/lib/safe_zip/entry_spec.rb create mode 100644 spec/lib/safe_zip/extract_params_spec.rb create mode 100644 spec/lib/safe_zip/extract_spec.rb diff --git a/Gemfile b/Gemfile index b3eeb3ec0ec..e632a083acf 100644 --- a/Gemfile +++ b/Gemfile @@ -57,6 +57,7 @@ gem 'u2f', '~> 0.2.1' # GitLab Pages gem 'validates_hostname', '~> 1.0.6' +gem 'rubyzip', '~> 1.2.2', require: false # Browser detection gem 'browser', '~> 2.5' diff --git a/Gemfile.lock b/Gemfile.lock index 419a6831924..d8982614d8b 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1137,6 +1137,7 @@ DEPENDENCIES ruby-prof (~> 0.17.0) ruby-progressbar ruby_parser (~> 3.8) + rubyzip (~> 1.2.2) rugged (~> 0.27) sanitize (~> 4.6) sass (~> 3.5) diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index eb2478be3cf..5caeb4cfa5f 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -7,7 +7,11 @@ module Projects BLOCK_SIZE = 32.kilobytes MAX_SIZE = 1.terabyte - SITE_PATH = 'public/'.freeze + PUBLIC_DIR = 'public'.freeze + + # this has to be invalid group name, + # as it shares the namespace with groups + TMP_EXTRACT_PATH = '@pages.tmp'.freeze attr_reader :build @@ -27,12 +31,11 @@ module Projects raise InvalidStateError, 'pages are outdated' unless latest? # Create temporary directory in which we will extract the artifacts - FileUtils.mkdir_p(tmp_path) - Dir.mktmpdir(nil, tmp_path) do |archive_path| + make_secure_tmp_dir(tmp_path) do |archive_path| extract_archive!(archive_path) # Check if we did extract public directory - archive_public_path = File.join(archive_path, 'public') + archive_public_path = File.join(archive_path, PUBLIC_DIR) raise InvalidStateError, 'pages miss the public folder' unless Dir.exist?(archive_public_path) raise InvalidStateError, 'pages are outdated' unless latest? @@ -85,22 +88,18 @@ module Projects raise InvalidStateError, 'missing artifacts metadata' unless build.artifacts_metadata? # Calculate page size after extract - public_entry = build.artifacts_metadata_entry(SITE_PATH, recursive: true) + public_entry = build.artifacts_metadata_entry(PUBLIC_DIR + '/', recursive: true) if public_entry.total_size > max_size raise InvalidStateError, "artifacts for pages are too large: #{public_entry.total_size}" end - # Requires UnZip at least 6.00 Info-ZIP. - # -qq be (very) quiet - # -n never overwrite existing files - # We add * to end of SITE_PATH, because we want to extract SITE_PATH and all subdirectories - site_path = File.join(SITE_PATH, '*') build.artifacts_file.use_file do |artifacts_path| - unless system(*%W(unzip -n #{artifacts_path} #{site_path} -d #{temp_path})) - raise FailedToExtractError, 'pages failed to extract' - end + SafeZip::Extract.new(artifacts_path) + .extract(directories: [PUBLIC_DIR], to: temp_path) end + rescue SafeZip::Extract::Error => e + raise FailedToExtractError, e.message end def deploy_page!(archive_public_path) @@ -139,7 +138,7 @@ module Projects end def tmp_path - @tmp_path ||= File.join(::Settings.pages.path, 'tmp') + @tmp_path ||= File.join(::Settings.pages.path, TMP_EXTRACT_PATH) end def pages_path @@ -147,11 +146,11 @@ module Projects end def public_path - @public_path ||= File.join(pages_path, 'public') + @public_path ||= File.join(pages_path, PUBLIC_DIR) end def previous_public_path - @previous_public_path ||= File.join(pages_path, "public.#{SecureRandom.hex}") + @previous_public_path ||= File.join(pages_path, "#{PUBLIC_DIR}.#{SecureRandom.hex}") end def ref @@ -188,5 +187,15 @@ module Projects def pages_deployments_failed_total_counter @pages_deployments_failed_total_counter ||= Gitlab::Metrics.counter(:pages_deployments_failed_total, "Counter of GitLab Pages deployments which failed") end + + def make_secure_tmp_dir(tmp_path) + FileUtils.mkdir_p(tmp_path) + path = Dir.mktmpdir(nil, tmp_path) + begin + yield(path) + ensure + FileUtils.remove_entry_secure(path) + end + end end end diff --git a/changelogs/unreleased/extract-pages-with-rubyzip.yml b/changelogs/unreleased/extract-pages-with-rubyzip.yml new file mode 100644 index 00000000000..8352e79d3e5 --- /dev/null +++ b/changelogs/unreleased/extract-pages-with-rubyzip.yml @@ -0,0 +1,5 @@ +--- +title: Extract GitLab Pages using RubyZip +merge_request: +author: +type: security diff --git a/lib/safe_zip/entry.rb b/lib/safe_zip/entry.rb new file mode 100644 index 00000000000..664e2f52f91 --- /dev/null +++ b/lib/safe_zip/entry.rb @@ -0,0 +1,97 @@ +# frozen_string_literal: true + +module SafeZip + class Entry + attr_reader :zip_archive, :zip_entry + attr_reader :path, :params + + def initialize(zip_archive, zip_entry, params) + @zip_archive = zip_archive + @zip_entry = zip_entry + @params = params + @path = ::File.expand_path(zip_entry.name, params.extract_path) + end + + def path_dir + ::File.dirname(path) + end + + def real_path_dir + ::File.realpath(path_dir) + end + + def exist? + ::File.exist?(path) + end + + def extract + # do not extract if file is not part of target directory + return false unless matching_target_directory + + # do not overwrite existing file + raise SafeZip::Extract::AlreadyExistsError, "File already exists #{zip_entry.name}" if exist? + + create_path_dir + + if zip_entry.file? + extract_file + elsif zip_entry.directory? + extract_dir + elsif zip_entry.symlink? + extract_symlink + else + raise SafeZip::Extract::UnsupportedEntryError, "File #{zip_entry.name} cannot be extracted" + end + rescue SafeZip::Extract::Error + raise + rescue => e + raise SafeZip::Extract::ExtractError, e.message + end + + private + + def extract_file + zip_archive.extract(zip_entry, path) + end + + def extract_dir + FileUtils.mkdir(path) + end + + def extract_symlink + source_path = read_symlink + real_source_path = expand_symlink(source_path) + + # ensure that source path of symlink is within target directories + unless real_source_path.start_with?(matching_target_directory) + raise SafeZip::Extract::PermissionDeniedError, "Symlink cannot be created targeting: #{source_path}" + end + + ::File.symlink(source_path, path) + end + + def create_path_dir + # Create all directories, but ignore permissions + FileUtils.mkdir_p(path_dir) + + # disallow to make path dirs to point to another directories + unless path_dir == real_path_dir + raise SafeZip::Extract::PermissionDeniedError, "Directory of #{zip_entry.name} points to another directory" + end + end + + def matching_target_directory + params.matching_target_directory(path) + end + + def read_symlink + zip_archive.read(zip_entry) + end + + def expand_symlink(source_path) + ::File.realpath(source_path, path_dir) + rescue + raise SafeZip::Extract::SymlinkSourceDoesNotExistError, "Symlink source #{source_path} does not exist" + end + end +end diff --git a/lib/safe_zip/extract.rb b/lib/safe_zip/extract.rb new file mode 100644 index 00000000000..3bd4935ef34 --- /dev/null +++ b/lib/safe_zip/extract.rb @@ -0,0 +1,73 @@ +# frozen_string_literal: true + +module SafeZip + class Extract + Error = Class.new(StandardError) + PermissionDeniedError = Class.new(Error) + SymlinkSourceDoesNotExistError = Class.new(Error) + UnsupportedEntryError = Class.new(Error) + AlreadyExistsError = Class.new(Error) + NoMatchingError = Class.new(Error) + ExtractError = Class.new(Error) + + attr_reader :archive_path + + def initialize(archive_file) + @archive_path = archive_file + end + + def extract(opts = {}) + params = SafeZip::ExtractParams.new(**opts) + + if Feature.enabled?(:safezip_use_rubyzip, default_enabled: true) + extract_with_ruby_zip(params) + else + legacy_unsafe_extract_with_system_zip(params) + end + end + + private + + def extract_with_ruby_zip(params) + Zip::File.open(archive_path) do |zip_archive| + # Extract all files in the following order: + # 1. Directories first, + # 2. Files next, + # 3. Symlinks last (or anything else) + extracted = extract_all_entries(zip_archive, params, + zip_archive.lazy.select(&:directory?)) + + extracted += extract_all_entries(zip_archive, params, + zip_archive.lazy.select(&:file?)) + + extracted += extract_all_entries(zip_archive, params, + zip_archive.lazy.reject(&:directory?).reject(&:file?)) + + raise NoMatchingError, 'No entries extracted' unless extracted > 0 + end + end + + def extract_all_entries(zip_archive, params, entries) + entries.count do |zip_entry| + SafeZip::Entry.new(zip_archive, zip_entry, params) + .extract + end + end + + def legacy_unsafe_extract_with_system_zip(params) + # Requires UnZip at least 6.00 Info-ZIP. + # -n never overwrite existing files + args = %W(unzip -n -qq #{archive_path}) + + # We add * to end of directory, because we want to extract directory and all subdirectories + args += params.directories_wildcard + + # Target directory where we extract + args += %W(-d #{params.extract_path}) + + unless system(*args) + raise Error, 'archive failed to extract' + end + end + end +end diff --git a/lib/safe_zip/extract_params.rb b/lib/safe_zip/extract_params.rb new file mode 100644 index 00000000000..bd3b788bac9 --- /dev/null +++ b/lib/safe_zip/extract_params.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module SafeZip + class ExtractParams + include Gitlab::Utils::StrongMemoize + + attr_reader :directories, :extract_path + + def initialize(directories:, to:) + @directories = directories + @extract_path = ::File.realpath(to) + end + + def matching_target_directory(path) + target_directories.find do |directory| + path.start_with?(directory) + end + end + + def target_directories + strong_memoize(:target_directories) do + directories.map do |directory| + ::File.join(::File.expand_path(directory, extract_path), '') + end + end + end + + def directories_wildcard + strong_memoize(:directories_wildcard) do + directories.map do |directory| + ::File.join(directory, '*') + end + end + end + end +end diff --git a/spec/fixtures/pages_non_writeable.zip b/spec/fixtures/pages_non_writeable.zip new file mode 100644 index 0000000000000000000000000000000000000000..69f175d8504884c71c70d445a66dcfc967f1cfab GIT binary patch literal 727 zcmWIWW@Zs#;9y{2h*)jp2c+PDok4=3pfo8bGg-g1f}4Sn1tbF|LPJ;?7-zUR#RPb> zgLJ|{0Nk*tOHd8tLl~A=T%1}`jMdCr7P!siU=XT9HB%hJ%*@=x^i;iqymV|14*BXB z!^pt!|3A#ttWbBuT#adiAi{>syp+@my^NCF99(99@;H0)WH`rp{ZpO~nSkL0vFP#B zsT+iV7V!XafHxzP2s7@`hPX$NA=eSaA{h1v!?1-m#4rJd|Bk*0!w7~w%uGaBz(NTW z+Ay%KaUq781j8QTdx+u4Ap;6%7}(ax42&kI?+FKcfHx}}NGl5vh5|L_05Jmt0B<;V A>Hq)$ literal 0 HcmV?d00001 diff --git a/spec/fixtures/safe_zip/invalid-symlink-does-not-exist.zip b/spec/fixtures/safe_zip/invalid-symlink-does-not-exist.zip new file mode 100644 index 0000000000000000000000000000000000000000..b9ae1548713ab160866691dc880ddb4f6f29d0e2 GIT binary patch literal 1183 zcmWIWW@h1H00Gx!rhZ@slwfC&VJIj~%E?UD4-MgDU@la0ivi)%3T_5QmKV$n3}7Mv zZh%mmk>BH|Q#bGc4FF+5gaMg(DXA5D86~+nV5365I>tQz;)-sRM`}(^zCw6@QBDdM z!q}pzMkwy$Ll~P_T%1}`3~`=^fD>WoA^T4p!<@|A#Pn3Xg1mINBbmQqI1;zRB(NK3 zq?c6yF%jg%PJAYs1Eb)}BN-M!aOk0iAjoY9qoS}!2G|G1`d|vI7~xuw*?qMaE7^c% zgD@|`wGdrkPvbBb63_wOj7)OOxKfJ*G;jqN{yKt~@U+7UNjn(n24NUx%7GZh2sCC} zBgizUVaVwSXe=ll;V~97=DnhA;|JZ6HN3e*S(;tVh|K~8-D6o)z$ zY9>;eBHh6fNQN(IGz10*7Q>;5g0RyuQy0SN{}(nROh+W2&=8=ppfrIeL@*N;va$Ek Zj4j4(Br7OcGOz<-EE5BR4iGai0079~@bUlv literal 0 HcmV?d00001 diff --git a/spec/fixtures/safe_zip/invalid-symlinks-outside.zip b/spec/fixtures/safe_zip/invalid-symlinks-outside.zip new file mode 100644 index 0000000000000000000000000000000000000000..c184a1dafe285eefe69b04fc001138c0d1559ab1 GIT binary patch literal 1309 zcmajfze@sP7zgl2Py2CJEJLfaZ`Sn8 z&h{)Qdw01aolZ9ak=Ty{;9N8xcsLW<`AHhNZ!r!6Q&x zhG{m}cAIUpta6resn4tH2_)&vbGB8otb_8nbQ#0eGwfqr{KTLL`L6{oB52qa8~6zm z(h*XQc2bg4tCuQ9y49#S842r;13xg>`O*8~6h@;H7}b$1hq*_Bj*yE*=jq;rDQTZp zv(wfMHXbeDMP`Npai1DKREob^r^Rm*csZXLrS z$UGjC=3>Fj$WjN#uyX6Xi~xh)C1<@3PEIP<&Le5;UrzcZL^HI2e0tGLfoIb68~701 z>0L8W$jP`|M~)AACmi?6iDXDIgWHlz>5=pd`bhc}MJ(h$Ld1p($}m)L_+-?_gw=<# Qar(i*48E`Ni&Z=T|1hl>_W%F@ literal 0 HcmV?d00001 diff --git a/spec/fixtures/safe_zip/valid-non-writeable.zip b/spec/fixtures/safe_zip/valid-non-writeable.zip new file mode 100644 index 0000000000000000000000000000000000000000..69f175d8504884c71c70d445a66dcfc967f1cfab GIT binary patch literal 727 zcmWIWW@Zs#;9y{2h*)jp2c+PDok4=3pfo8bGg-g1f}4Sn1tbF|LPJ;?7-zUR#RPb> zgLJ|{0Nk*tOHd8tLl~A=T%1}`jMdCr7P!siU=XT9HB%hJ%*@=x^i;iqymV|14*BXB z!^pt!|3A#ttWbBuT#adiAi{>syp+@my^NCF99(99@;H0)WH`rp{ZpO~nSkL0vFP#B zsT+iV7V!XafHxzP2s7@`hPX$NA=eSaA{h1v!?1-m#4rJd|Bk*0!w7~w%uGaBz(NTW z+Ay%KaUq781j8QTdx+u4Ap;6%7}(ax42&kI?+FKcfHx}}NGl5vh5|L_05Jmt0B<;V A>Hq)$ literal 0 HcmV?d00001 diff --git a/spec/fixtures/safe_zip/valid-simple.zip b/spec/fixtures/safe_zip/valid-simple.zip new file mode 100644 index 0000000000000000000000000000000000000000..a56b8b41dcc86e7c671a35513bd59666c824f758 GIT binary patch literal 1144 zcmWIWW@h1H0D&vrhJIiMlwfC&VJIj~%E?UD4-MgDU|zr0AqIp?E4UdLSza(RFo1~w zxB;ht20VT`bpsF301y^L7?7Ell3JmcQIeYjHflD|m_?XId8Fp#cn=g@hw!-uWB_{TLk&O+eUMRDJcb(j zj7)OOxYC6L(6b;Q!0^`*M8i`FDRuWrX84r85sUAGz10*EF3^- z5}1+*2Q6j_f*A>lr#hgK@bm;U3`_P4@J4tZGc`dBgT&K*gkfm$gwWHZ%e?;W%$1k#KZ^$R!AVAxfIz@OphZQ8jLU$H5h;f rV+jIegE761FnCMjDultv0Rc1?6c8BhWn}|-j~NJA85tP9f;bESdn|NT literal 0 HcmV?d00001 diff --git a/spec/lib/safe_zip/entry_spec.rb b/spec/lib/safe_zip/entry_spec.rb new file mode 100644 index 00000000000..115e28c5994 --- /dev/null +++ b/spec/lib/safe_zip/entry_spec.rb @@ -0,0 +1,196 @@ +require 'spec_helper' + +describe SafeZip::Entry do + let(:target_path) { Dir.mktmpdir('safe-zip') } + let(:directories) { %w(public folder/with/subfolder) } + let(:params) { SafeZip::ExtractParams.new(directories: directories, to: target_path) } + + let(:entry) { described_class.new(zip_archive, zip_entry, params) } + let(:entry_name) { 'public/folder/index.html' } + let(:entry_path_dir) { File.join(target_path, File.dirname(entry_name)) } + let(:entry_path) { File.join(target_path, entry_name) } + let(:zip_archive) { double } + + let(:zip_entry) do + double( + name: entry_name, + file?: false, + directory?: false, + symlink?: false) + end + + after do + FileUtils.remove_entry_secure(target_path) + end + + context '#path_dir' do + subject { entry.path_dir } + + it { is_expected.to eq(target_path + '/public/folder') } + end + + context '#exist?' do + subject { entry.exist? } + + context 'when entry does not exist' do + it { is_expected.not_to be_truthy } + end + + context 'when entry does exist' do + before do + create_entry + end + + it { is_expected.to be_truthy } + end + end + + describe '#extract' do + subject { entry.extract } + + context 'when entry does not match the filtered directories' do + using RSpec::Parameterized::TableSyntax + + where(:entry_name) do + [ + 'assets/folder/index.html', + 'public/../folder/index.html', + 'public/../../../../../index.html', + '../../../../../public/index.html', + '/etc/passwd' + ] + end + + with_them do + it 'does not extract file' do + is_expected.to be_falsey + end + end + end + + context 'when entry does exist' do + before do + create_entry + end + + it 'raises an exception' do + expect { subject }.to raise_error(SafeZip::Extract::AlreadyExistsError) + end + end + + context 'when entry type is unknown' do + it 'raises an exception' do + expect { subject }.to raise_error(SafeZip::Extract::UnsupportedEntryError) + end + end + + context 'when entry is valid' do + shared_examples 'secured symlinks' do + context 'when we try to extract entry into symlinked folder' do + before do + FileUtils.mkdir_p(File.join(target_path, "source")) + File.symlink("source", File.join(target_path, "public")) + end + + it 'raises an exception' do + expect { subject }.to raise_error(SafeZip::Extract::PermissionDeniedError) + end + end + end + + context 'and is file' do + before do + allow(zip_entry).to receive(:file?) { true } + end + + it 'does extract file' do + expect(zip_archive).to receive(:extract) + .with(zip_entry, entry_path) + .and_return(true) + + is_expected.to be_truthy + end + + it_behaves_like 'secured symlinks' + end + + context 'and is directory' do + let(:entry_name) { 'public/folder/assets' } + + before do + allow(zip_entry).to receive(:directory?) { true } + end + + it 'does create directory' do + is_expected.to be_truthy + + expect(File.exist?(entry_path)).to eq(true) + end + + it_behaves_like 'secured symlinks' + end + + context 'and is symlink' do + let(:entry_name) { 'public/folder/assets' } + + before do + allow(zip_entry).to receive(:symlink?) { true } + allow(zip_archive).to receive(:read).with(zip_entry) { entry_symlink } + end + + shared_examples 'a valid symlink' do + it 'does create symlink' do + is_expected.to be_truthy + + expect(File.exist?(entry_path)).to eq(true) + end + end + + context 'when source is within target' do + let(:entry_symlink) { '../images' } + + context 'but does not exist' do + it 'raises an exception' do + expect { subject }.to raise_error(SafeZip::Extract::SymlinkSourceDoesNotExistError) + end + end + + context 'and does exist' do + before do + FileUtils.mkdir_p(File.join(target_path, 'public', 'images')) + end + + it_behaves_like 'a valid symlink' + end + end + + context 'when source points outside of target' do + let(:entry_symlink) { '../../images' } + + before do + FileUtils.mkdir(File.join(target_path, 'images')) + end + + it 'raises an exception' do + expect { subject }.to raise_error(SafeZip::Extract::PermissionDeniedError) + end + end + + context 'when source points to /etc/passwd' do + let(:entry_symlink) { '/etc/passwd' } + + it 'raises an exception' do + expect { subject }.to raise_error(SafeZip::Extract::PermissionDeniedError) + end + end + end + end + end + + private + + def create_entry + FileUtils.mkdir_p(entry_path_dir) + FileUtils.touch(entry_path) + end +end diff --git a/spec/lib/safe_zip/extract_params_spec.rb b/spec/lib/safe_zip/extract_params_spec.rb new file mode 100644 index 00000000000..85e22cfa495 --- /dev/null +++ b/spec/lib/safe_zip/extract_params_spec.rb @@ -0,0 +1,54 @@ +require 'spec_helper' + +describe SafeZip::ExtractParams do + let(:target_path) { Dir.mktmpdir("safe-zip") } + let(:params) { described_class.new(directories: directories, to: target_path) } + let(:directories) { %w(public folder/with/subfolder) } + + after do + FileUtils.remove_entry_secure(target_path) + end + + describe '#extract_path' do + subject { params.extract_path } + + it { is_expected.to eq(target_path) } + end + + describe '#matching_target_directory' do + using RSpec::Parameterized::TableSyntax + + subject { params.matching_target_directory(target_path + path) } + + where(:path, :result) do + '/public/index.html' | '/public/' + '/non/existing/path' | nil + '/public' | nil + '/folder/with/index.html' | nil + end + + with_them do + it { is_expected.to eq(result ? target_path + result : nil) } + end + end + + describe '#target_directories' do + subject { params.target_directories } + + it 'starts with target_path' do + is_expected.to all(start_with(target_path + '/')) + end + + it 'ends with / for all paths' do + is_expected.to all(end_with('/')) + end + end + + describe '#directories_wildcard' do + subject { params.directories_wildcard } + + it 'adds * for all paths' do + is_expected.to all(end_with('/*')) + end + end +end diff --git a/spec/lib/safe_zip/extract_spec.rb b/spec/lib/safe_zip/extract_spec.rb new file mode 100644 index 00000000000..dc7e25c0cf6 --- /dev/null +++ b/spec/lib/safe_zip/extract_spec.rb @@ -0,0 +1,80 @@ +require 'spec_helper' + +describe SafeZip::Extract do + let(:target_path) { Dir.mktmpdir('safe-zip') } + let(:directories) { %w(public) } + let(:object) { described_class.new(archive) } + let(:archive) { Rails.root.join('spec', 'fixtures', 'safe_zip', archive_name) } + + after do + FileUtils.remove_entry_secure(target_path) + end + + context '#extract' do + subject { object.extract(directories: directories, to: target_path) } + + shared_examples 'extracts archive' do |param| + before do + stub_feature_flags(safezip_use_rubyzip: param) + end + + it 'does extract archive' do + subject + + expect(File.exist?(File.join(target_path, 'public', 'index.html'))).to eq(true) + expect(File.exist?(File.join(target_path, 'source'))).to eq(false) + end + end + + shared_examples 'fails to extract archive' do |param| + before do + stub_feature_flags(safezip_use_rubyzip: param) + end + + it 'does not extract archive' do + expect { subject }.to raise_error(SafeZip::Extract::Error) + end + end + + %w(valid-simple.zip valid-symlinks-first.zip valid-non-writeable.zip).each do |name| + context "when using #{name} archive" do + let(:archive_name) { name } + + context 'for RubyZip' do + it_behaves_like 'extracts archive', true + end + + context 'for UnZip' do + it_behaves_like 'extracts archive', false + end + end + end + + %w(invalid-symlink-does-not-exist.zip invalid-symlinks-outside.zip).each do |name| + context "when using #{name} archive" do + let(:archive_name) { name } + + context 'for RubyZip' do + it_behaves_like 'fails to extract archive', true + end + + context 'for UnZip (UNSAFE)' do + it_behaves_like 'extracts archive', false + end + end + end + + context 'when no matching directories are found' do + let(:archive_name) { 'valid-simple.zip' } + let(:directories) { %w(non/existing) } + + context 'for RubyZip' do + it_behaves_like 'fails to extract archive', true + end + + context 'for UnZip' do + it_behaves_like 'fails to extract archive', false + end + end + end +end diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index 36b619ba9be..8b70845befe 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -5,24 +5,27 @@ describe Projects::UpdatePagesService do set(:pipeline) { create(:ci_pipeline, project: project, sha: project.commit('HEAD').sha) } set(:build) { create(:ci_build, pipeline: pipeline, ref: 'HEAD') } let(:invalid_file) { fixture_file_upload('spec/fixtures/dk.png') } - let(:extension) { 'zip' } - let(:file) { fixture_file_upload("spec/fixtures/pages.#{extension}") } - let(:empty_file) { fixture_file_upload("spec/fixtures/pages_empty.#{extension}") } - let(:metadata) do - filename = "spec/fixtures/pages.#{extension}.meta" - fixture_file_upload(filename) if File.exist?(filename) - end + let(:file) { fixture_file_upload("spec/fixtures/pages.zip") } + let(:empty_file) { fixture_file_upload("spec/fixtures/pages_empty.zip") } + let(:metadata_filename) { "spec/fixtures/pages.zip.meta" } + let(:metadata) { fixture_file_upload(metadata_filename) if File.exist?(metadata_filename) } subject { described_class.new(project, build) } before do + stub_feature_flags(safezip_use_rubyzip: true) + project.remove_pages end - context 'legacy artifacts' do - let(:extension) { 'zip' } + context '::TMP_EXTRACT_PATH' do + subject { described_class::TMP_EXTRACT_PATH } + it { is_expected.not_to match(Gitlab::PathRegex.namespace_format_regex) } + end + + context 'legacy artifacts' do before do build.update(legacy_artifacts_file: file) build.update(legacy_artifacts_metadata: metadata) @@ -132,6 +135,20 @@ describe Projects::UpdatePagesService do end end + context 'when using pages with non-writeable public' do + let(:file) { fixture_file_upload("spec/fixtures/pages_non_writeable.zip") } + + context 'when using RubyZip' do + before do + stub_feature_flags(safezip_use_rubyzip: true) + end + + it 'succeeds to extract' do + expect(execute).to eq(:success) + end + end + end + context 'when timeout happens by DNS error' do before do allow_any_instance_of(described_class) From 87c5abfc0fb7dccf7f94c0b56ef93501506952c5 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Tue, 8 Jan 2019 17:57:58 +0000 Subject: [PATCH 24/42] Verify that LFS upload requests are genuine LFS uploads are handled in concert by workhorse and rails. In normal use, workhorse: * Authorizes the request with rails (upload_authorize) * Handles the upload of the file to a tempfile - disk or object storage * Validates the file size and contents * Hands off to rails to complete the upload (upload_finalize) In `upload_finalize`, the LFS object is linked to the project. As LFS objects are deduplicated across all projects, it may already exist. If not, the temporary file is copied to the correct place, and will be used by all future LFS objects with the same OID. Workhorse uses the Content-Type of the request to decide to follow this routine, as the URLs are ambiguous. If the Content-Type is anything but "application/octet-stream", the request is proxied directly to rails, on the assumption that this is a normal file edit request. If it's an actual LFS request with a different content-type, however, it is routed to the Rails `upload_finalize` action, which treats it as an LFS upload just as it would a workhorse-modified request. The outcome is that users can upload LFS objects that don't match the declared size or OID. They can also create links to LFS objects they don't really own, allowing them to read the contents of files if they know just the size or OID. We can close this hole by requiring requests to `upload_finalize` to be sourced from Workhorse. The mechanism to do this already exists. --- GITLAB_WORKHORSE_VERSION | 2 +- .../projects/lfs_storage_controller.rb | 2 +- ...767-verify-lfs-finalize-from-workhorse.yml | 5 ++++ spec/requests/lfs_http_spec.rb | 23 +++++++++++++++---- 4 files changed, 25 insertions(+), 7 deletions(-) create mode 100644 changelogs/unreleased/security-2767-verify-lfs-finalize-from-workhorse.yml diff --git a/GITLAB_WORKHORSE_VERSION b/GITLAB_WORKHORSE_VERSION index da156181014..0e79152459e 100644 --- a/GITLAB_WORKHORSE_VERSION +++ b/GITLAB_WORKHORSE_VERSION @@ -1 +1 @@ -8.1.0 \ No newline at end of file +8.1.1 diff --git a/app/controllers/projects/lfs_storage_controller.rb b/app/controllers/projects/lfs_storage_controller.rb index babeee48ef3..013e01b82aa 100644 --- a/app/controllers/projects/lfs_storage_controller.rb +++ b/app/controllers/projects/lfs_storage_controller.rb @@ -5,7 +5,7 @@ class Projects::LfsStorageController < Projects::GitHttpClientController include WorkhorseRequest include SendFileUpload - skip_before_action :verify_workhorse_api!, only: [:download, :upload_finalize] + skip_before_action :verify_workhorse_api!, only: :download def download lfs_object = LfsObject.find_by_oid(oid) diff --git a/changelogs/unreleased/security-2767-verify-lfs-finalize-from-workhorse.yml b/changelogs/unreleased/security-2767-verify-lfs-finalize-from-workhorse.yml new file mode 100644 index 00000000000..e79e3263df7 --- /dev/null +++ b/changelogs/unreleased/security-2767-verify-lfs-finalize-from-workhorse.yml @@ -0,0 +1,5 @@ +--- +title: Verify that LFS upload requests are genuine +merge_request: +author: +type: security diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb index f1514e90eb2..1781759c54b 100644 --- a/spec/requests/lfs_http_spec.rb +++ b/spec/requests/lfs_http_spec.rb @@ -1086,6 +1086,12 @@ describe 'Git LFS API and storage' do end end + context 'and request to finalize the upload is not sent by gitlab-workhorse' do + it 'fails with a JWT decode error' do + expect { put_finalize(lfs_tmp_file, verified: false) }.to raise_error(JWT::DecodeError) + end + end + context 'and workhorse requests upload finalize for a new lfs object' do before do lfs_object.destroy @@ -1347,9 +1353,13 @@ describe 'Git LFS API and storage' do context 'when pushing the same lfs object to the second project' do before do + finalize_headers = headers + .merge('X-Gitlab-Lfs-Tmp' => lfs_tmp_file) + .merge(workhorse_internal_api_request_header) + put "#{second_project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}", params: {}, - headers: headers.merge('X-Gitlab-Lfs-Tmp' => lfs_tmp_file).compact + headers: finalize_headers end it 'responds with status 200' do @@ -1370,7 +1380,7 @@ describe 'Git LFS API and storage' do put "#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}/authorize", params: {}, headers: authorize_headers end - def put_finalize(lfs_tmp = lfs_tmp_file, with_tempfile: false, args: {}) + def put_finalize(lfs_tmp = lfs_tmp_file, with_tempfile: false, verified: true, args: {}) upload_path = LfsObjectUploader.workhorse_local_upload_path file_path = upload_path + '/' + lfs_tmp if lfs_tmp @@ -1384,11 +1394,14 @@ describe 'Git LFS API and storage' do 'file.name' => File.basename(file_path) } - put_finalize_with_args(args.merge(extra_args).compact) + put_finalize_with_args(args.merge(extra_args).compact, verified: verified) end - def put_finalize_with_args(args) - put "#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}", params: args, headers: headers + def put_finalize_with_args(args, verified:) + finalize_headers = headers + finalize_headers.merge!(workhorse_internal_api_request_header) if verified + + put "#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}", params: args, headers: finalize_headers end def lfs_tmp_file From 6d57b2fdb803547ac2bb65911373c311ca94a677 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Tue, 22 Jan 2019 09:38:08 -0800 Subject: [PATCH 25/42] Alias GitHub and BitBucket OAuth2 callback URLs To prevent an OAuth2 covert redirect vulnerability, this commit adds and uses an alias for the GitHub and BitBucket OAuth2 callback URLs to the following paths: GitHub: /users/auth/-/import/github Bitbucket: /users/auth/-/import/bitbucket This allows admins to put a more restrictive callback URL in the OAuth2 configuration settings. Instead of https://example.com, admins can now use: https://example.com/users/auth It's possible but not trivial to change Devise and OmniAuth to use a different prefix for callback URLs instead of /users/auth. For now, aliasing the import URLs under the /users/auth namespace should suffice. Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/56663 --- app/controllers/import/bitbucket_controller.rb | 4 ++-- app/controllers/import/github_controller.rb | 2 +- .../sh-fix-import-redirect-vulnerability.yml | 5 +++++ config/routes/import.rb | 9 +++++++++ doc/integration/bitbucket.md | 6 +++++- doc/integration/github.md | 6 +++++- spec/controllers/import/bitbucket_controller_spec.rb | 11 +++++++++-- spec/controllers/import/github_controller_spec.rb | 8 +++++++- 8 files changed, 43 insertions(+), 8 deletions(-) create mode 100644 changelogs/unreleased/sh-fix-import-redirect-vulnerability.yml diff --git a/app/controllers/import/bitbucket_controller.rb b/app/controllers/import/bitbucket_controller.rb index 1b30b4dda36..2b1395f364f 100644 --- a/app/controllers/import/bitbucket_controller.rb +++ b/app/controllers/import/bitbucket_controller.rb @@ -8,7 +8,7 @@ class Import::BitbucketController < Import::BaseController rescue_from Bitbucket::Error::Unauthorized, with: :bitbucket_unauthorized def callback - response = client.auth_code.get_token(params[:code], redirect_uri: callback_import_bitbucket_url) + response = client.auth_code.get_token(params[:code], redirect_uri: users_import_bitbucket_callback_url) session[:bitbucket_token] = response.token session[:bitbucket_expires_at] = response.expires_at @@ -89,7 +89,7 @@ class Import::BitbucketController < Import::BaseController end def go_to_bitbucket_for_permissions - redirect_to client.auth_code.authorize_url(redirect_uri: callback_import_bitbucket_url) + redirect_to client.auth_code.authorize_url(redirect_uri: users_import_bitbucket_callback_url) end def bitbucket_unauthorized diff --git a/app/controllers/import/github_controller.rb b/app/controllers/import/github_controller.rb index 34c7dbdc2fe..3fbc0817e95 100644 --- a/app/controllers/import/github_controller.rb +++ b/app/controllers/import/github_controller.rb @@ -83,7 +83,7 @@ class Import::GithubController < Import::BaseController end def callback_import_url - public_send("callback_import_#{provider}_url", extra_import_params) # rubocop:disable GitlabSecurity/PublicSend + public_send("users_import_#{provider}_callback_url", extra_import_params) # rubocop:disable GitlabSecurity/PublicSend end def provider_unauthorized diff --git a/changelogs/unreleased/sh-fix-import-redirect-vulnerability.yml b/changelogs/unreleased/sh-fix-import-redirect-vulnerability.yml new file mode 100644 index 00000000000..addf327b69d --- /dev/null +++ b/changelogs/unreleased/sh-fix-import-redirect-vulnerability.yml @@ -0,0 +1,5 @@ +--- +title: Alias GitHub and BitBucket OAuth2 callback URLs +merge_request: +author: +type: security diff --git a/config/routes/import.rb b/config/routes/import.rb index 3998d977c81..69df82611f2 100644 --- a/config/routes/import.rb +++ b/config/routes/import.rb @@ -1,3 +1,12 @@ +# Alias import callbacks under the /users/auth endpoint so that +# the OAuth2 callback URL can be restricted under http://example.com/users/auth +# instead of http://example.com. +Devise.omniauth_providers.each do |provider| + next if provider == 'ldapmain' + + get "/users/auth/-/import/#{provider}/callback", to: "import/#{provider}#callback", as: "users_import_#{provider}_callback" +end + namespace :import do resource :github, only: [:create, :new], controller: :github do post :personal_access_token diff --git a/doc/integration/bitbucket.md b/doc/integration/bitbucket.md index a69db1d1a6e..68ec8c4b5c2 100644 --- a/doc/integration/bitbucket.md +++ b/doc/integration/bitbucket.md @@ -43,9 +43,13 @@ you to use. | :--- | :---------- | | **Name** | This can be anything. Consider something like `'s GitLab` or `'s GitLab` or something else descriptive. | | **Application description** | Fill this in if you wish. | - | **Callback URL** | The URL to your GitLab installation, e.g., `https://gitlab.example.com`. | + | **Callback URL** | The URL to your GitLab installation, e.g., `https://gitlab.example.com/users/auth`. | | **URL** | The URL to your GitLab installation, e.g., `https://gitlab.example.com`. | + NOTE: Be sure to append `/users/auth` to the end of the callback URL + to prevent a [OAuth2 convert + redirect](http://tetraph.com/covert_redirect/) vulnerability. + NOTE: Starting in GitLab 8.15, you MUST specify a callback URL, or you will see an "Invalid redirect_uri" message. For more details, see [the Bitbucket documentation](https://confluence.atlassian.com/bitbucket/oauth-faq-338365710.html). diff --git a/doc/integration/github.md b/doc/integration/github.md index b8156b2b593..eca9aa16499 100644 --- a/doc/integration/github.md +++ b/doc/integration/github.md @@ -21,9 +21,13 @@ To get the credentials (a pair of Client ID and Client Secret), you must registe - Application name: This can be anything. Consider something like `'s GitLab` or `'s GitLab` or something else descriptive. - Homepage URL: the URL to your GitLab installation. e.g., `https://gitlab.company.com` - Application description: Fill this in if you wish. - - Authorization callback URL: `http(s)://${YOUR_DOMAIN}`. Please make sure the port is included if your GitLab instance is not configured on default port. + - Authorization callback URL: `http(s)://${YOUR_DOMAIN}/users/auth`. Please make sure the port is included if your GitLab instance is not configured on default port. ![Register OAuth App](img/github_register_app.png) + NOTE: Be sure to append `/users/auth` to the end of the callback URL + to prevent a [OAuth2 convert + redirect](http://tetraph.com/covert_redirect/) vulnerability. + 1. Select **Register application**. 1. You should now see a pair of **Client ID** and **Client Secret** near the top right of the page (see screenshot). diff --git a/spec/controllers/import/bitbucket_controller_spec.rb b/spec/controllers/import/bitbucket_controller_spec.rb index 51793f2c048..0bc09c86939 100644 --- a/spec/controllers/import/bitbucket_controller_spec.rb +++ b/spec/controllers/import/bitbucket_controller_spec.rb @@ -8,6 +8,7 @@ describe Import::BitbucketController do let(:secret) { "sekrettt" } let(:refresh_token) { SecureRandom.hex(15) } let(:access_params) { { token: token, expires_at: nil, expires_in: nil, refresh_token: nil } } + let(:code) { SecureRandom.hex(8) } def assign_session_tokens session[:bitbucket_token] = token @@ -32,10 +33,16 @@ describe Import::BitbucketController do expires_in: expires_in, refresh_token: refresh_token) allow_any_instance_of(OAuth2::Client) - .to receive(:get_token).and_return(access_token) + .to receive(:get_token) + .with(hash_including( + 'grant_type' => 'authorization_code', + 'code' => code, + redirect_uri: users_import_bitbucket_callback_url), + {}) + .and_return(access_token) stub_omniauth_provider('bitbucket') - get :callback + get :callback, params: { code: code } expect(session[:bitbucket_token]).to eq(token) expect(session[:bitbucket_refresh_token]).to eq(refresh_token) diff --git a/spec/controllers/import/github_controller_spec.rb b/spec/controllers/import/github_controller_spec.rb index 780e49f7b93..bca5f3f6589 100644 --- a/spec/controllers/import/github_controller_spec.rb +++ b/spec/controllers/import/github_controller_spec.rb @@ -12,9 +12,15 @@ describe Import::GithubController do it "redirects to GitHub for an access token if logged in with GitHub" do allow(controller).to receive(:logged_in_with_provider?).and_return(true) - expect(controller).to receive(:go_to_provider_for_permissions) + expect(controller).to receive(:go_to_provider_for_permissions).and_call_original + allow_any_instance_of(Gitlab::LegacyGithubImport::Client) + .to receive(:authorize_url) + .with(users_import_github_callback_url) + .and_call_original get :new + + expect(response).to have_http_status(302) end it "prompts for an access token if GitHub not configured" do From e7aaada259b5dc38ce4b63273ff2b935ad04d539 Mon Sep 17 00:00:00 2001 From: Dennis Tang Date: Tue, 22 Jan 2019 22:10:48 +0000 Subject: [PATCH 26/42] Use sanitized user status message for user popover --- .../vue_shared/components/user_popover/user_popover.vue | 8 ++++---- .../security-55320-stored-xss-in-user-status.yml | 5 +++++ .../components/user_popover/user_popover_spec.js | 6 +++--- 3 files changed, 12 insertions(+), 7 deletions(-) create mode 100644 changelogs/unreleased/security-55320-stored-xss-in-user-status.yml diff --git a/app/assets/javascripts/vue_shared/components/user_popover/user_popover.vue b/app/assets/javascripts/vue_shared/components/user_popover/user_popover.vue index d24fe1b547e..f9773622001 100644 --- a/app/assets/javascripts/vue_shared/components/user_popover/user_popover.vue +++ b/app/assets/javascripts/vue_shared/components/user_popover/user_popover.vue @@ -28,10 +28,10 @@ export default { }, computed: { statusHtml() { - if (this.user.status.emoji && this.user.status.message) { - return `${glEmojiTag(this.user.status.emoji)} ${this.user.status.message}`; - } else if (this.user.status.message) { - return this.user.status.message; + if (this.user.status.emoji && this.user.status.message_html) { + return `${glEmojiTag(this.user.status.emoji)} ${this.user.status.message_html}`; + } else if (this.user.status.message_html) { + return this.user.status.message_html; } return ''; }, diff --git a/changelogs/unreleased/security-55320-stored-xss-in-user-status.yml b/changelogs/unreleased/security-55320-stored-xss-in-user-status.yml new file mode 100644 index 00000000000..8ea9ae0ccdf --- /dev/null +++ b/changelogs/unreleased/security-55320-stored-xss-in-user-status.yml @@ -0,0 +1,5 @@ +--- +title: Use sanitized user status message for user popover +merge_request: +author: +type: security diff --git a/spec/javascripts/vue_shared/components/user_popover/user_popover_spec.js b/spec/javascripts/vue_shared/components/user_popover/user_popover_spec.js index de3e0c149de..e8b41e8eeff 100644 --- a/spec/javascripts/vue_shared/components/user_popover/user_popover_spec.js +++ b/spec/javascripts/vue_shared/components/user_popover/user_popover_spec.js @@ -122,7 +122,7 @@ describe('User Popover Component', () => { describe('status data', () => { it('should show only message', () => { const testProps = Object.assign({}, DEFAULT_PROPS); - testProps.user.status = { message: 'Hello World' }; + testProps.user.status = { message_html: 'Hello World' }; vm = mountComponent(UserPopover, { ...DEFAULT_PROPS, @@ -134,12 +134,12 @@ describe('User Popover Component', () => { it('should show message and emoji', () => { const testProps = Object.assign({}, DEFAULT_PROPS); - testProps.user.status = { emoji: 'basketball_player', message: 'Hello World' }; + testProps.user.status = { emoji: 'basketball_player', message_html: 'Hello World' }; vm = mountComponent(UserPopover, { ...DEFAULT_PROPS, target: document.querySelector('.js-user-link'), - status: { emoji: 'basketball_player', message: 'Hello World' }, + status: { emoji: 'basketball_player', message_html: 'Hello World' }, }); expect(vm.$el.textContent).toContain('Hello World'); From 022f60e80144058bcb4b42b0b3b9e4da130983d0 Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Mon, 14 Jan 2019 22:53:37 +0100 Subject: [PATCH 27/42] Sent notification only to authorized users When moving a project, it's possible that some users who had access to the project in old path can not access the project in the new path. Because `project_authorizations` records are updated asynchronously, when we send the notification about moved project the list of project team members contains old project members, we want to notify all these members except the old users who can not access the new location. --- app/models/member.rb | 2 ++ app/models/project_team.rb | 12 ++++++++ app/services/notification_service.rb | 3 +- .../security-project-move-users.yml | 5 ++++ spec/models/project_team_spec.rb | 15 ++++++++++ spec/services/notification_service_spec.rb | 29 +++++++++++++++---- 6 files changed, 59 insertions(+), 7 deletions(-) create mode 100644 changelogs/unreleased/security-project-move-users.yml diff --git a/app/models/member.rb b/app/models/member.rb index 9fc95ea00c3..82d207b79de 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -84,6 +84,8 @@ class Member < ActiveRecord::Base scope :order_recent_sign_in, -> { left_join_users.reorder(Gitlab::Database.nulls_last_order('users.last_sign_in_at', 'DESC')) } scope :order_oldest_sign_in, -> { left_join_users.reorder(Gitlab::Database.nulls_last_order('users.last_sign_in_at', 'ASC')) } + scope :on_project_and_ancestors, ->(project) { where(source: [project] + project.ancestors) } + before_validation :generate_invite_token, on: :create, if: -> (member) { member.invite_email.present? } after_create :send_invite, if: :invite?, unless: :importing? diff --git a/app/models/project_team.rb b/app/models/project_team.rb index 33bc6a561f9..aeba2843e5d 100644 --- a/app/models/project_team.rb +++ b/app/models/project_team.rb @@ -74,6 +74,14 @@ class ProjectTeam end alias_method :users, :members + # `members` method uses project_authorizations table which + # is updated asynchronously, on project move it still contains + # old members who may not have access to the new location, + # so we filter out only members of project or project's group + def members_in_project_and_ancestors + members.where(id: member_user_ids) + end + def guests @guests ||= fetch_members(Gitlab::Access::GUEST) end @@ -191,4 +199,8 @@ class ProjectTeam def group project.group end + + def member_user_ids + Member.on_project_and_ancestors(project).select(:user_id) + end end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index e1cf327209b..1a65561dd70 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -373,7 +373,8 @@ class NotificationService end def project_was_moved(project, old_path_with_namespace) - recipients = notifiable_users(project.team.members, :mention, project: project) + recipients = project.private? ? project.team.members_in_project_and_ancestors : project.team.members + recipients = notifiable_users(recipients, :mention, project: project) recipients.each do |recipient| mailer.project_was_moved_email( diff --git a/changelogs/unreleased/security-project-move-users.yml b/changelogs/unreleased/security-project-move-users.yml new file mode 100644 index 00000000000..744df68651f --- /dev/null +++ b/changelogs/unreleased/security-project-move-users.yml @@ -0,0 +1,5 @@ +--- +title: Notify only users who can access the project on project move. +merge_request: +author: +type: security diff --git a/spec/models/project_team_spec.rb b/spec/models/project_team_spec.rb index c4af17f4726..3537dead5d1 100644 --- a/spec/models/project_team_spec.rb +++ b/spec/models/project_team_spec.rb @@ -178,6 +178,21 @@ describe ProjectTeam do end end + describe '#members_in_project_and_ancestors' do + context 'group project' do + it 'filters out users who are not members of the project' do + group = create(:group) + project = create(:project, group: group) + group_member = create(:group_member, group: group) + old_user = create(:user) + + ProjectAuthorization.create!(project: project, user: old_user, access_level: Gitlab::Access::GUEST) + + expect(project.team.members_in_project_and_ancestors).to contain_exactly(group_member.user) + end + end + end + describe "#human_max_access" do it 'returns Maintainer role' do user = create(:user) diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index d20e712d365..6a5a6989607 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -1646,6 +1646,23 @@ describe NotificationService, :mailer do should_not_email(@u_guest_custom) should_not_email(@u_disabled) end + + context 'users not having access to the new location' do + it 'does not send email' do + old_user = create(:user) + ProjectAuthorization.create!(project: project, user: old_user, access_level: Gitlab::Access::GUEST) + + build_group(project) + reset_delivered_emails! + + notification.project_was_moved(project, "gitlab/gitlab") + + should_email(@g_watcher) + should_email(@g_global_watcher) + should_email(project.creator) + should_not_email(old_user) + end + end end context 'user with notifications disabled' do @@ -2232,8 +2249,8 @@ describe NotificationService, :mailer do # Users in the project's group but not part of project's team # with different notification settings - def build_group(project) - group = create_nested_group + def build_group(project, visibility: :public) + group = create_nested_group(visibility) project.update(namespace_id: group.id) # Group member: global=disabled, group=watch @@ -2249,10 +2266,10 @@ describe NotificationService, :mailer do # Creates a nested group only if supported # to avoid errors on MySQL - def create_nested_group + def create_nested_group(visibility) if Group.supports_nested_objects? - parent_group = create(:group, :public) - child_group = create(:group, :public, parent: parent_group) + parent_group = create(:group, visibility) + child_group = create(:group, visibility, parent: parent_group) # Parent group member: global=disabled, parent_group=watch, child_group=global @pg_watcher ||= create_user_with_notification(:watch, 'parent_group_watcher', parent_group) @@ -2272,7 +2289,7 @@ describe NotificationService, :mailer do child_group else - create(:group, :public) + create(:group, visibility) end end From 04d773d82632d08753fc6c10a193ef6178bc54ec Mon Sep 17 00:00:00 2001 From: Steve Azzopardi Date: Tue, 22 Jan 2019 14:37:49 +0100 Subject: [PATCH 28/42] Stop showing ci for guest users When a user is a guest user, and the "Public Pipeline" is set to false inside of "Settings > CI/CD > General" the commit status in the project dashboard should not be shown. --- app/views/shared/projects/_project.html.haml | 2 +- ...ity-commit-status-shown-for-guest-user.yml | 5 +++++ spec/features/dashboard/projects_spec.rb | 21 +++++++++++++++++++ 3 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/security-commit-status-shown-for-guest-user.yml diff --git a/app/views/shared/projects/_project.html.haml b/app/views/shared/projects/_project.html.haml index fea7e17be3d..e1564d57426 100644 --- a/app/views/shared/projects/_project.html.haml +++ b/app/views/shared/projects/_project.html.haml @@ -84,7 +84,7 @@ title: _('Issues'), data: { container: 'body', placement: 'top' } do = sprite_icon('issues', size: 14, css_class: 'append-right-4') = number_with_delimiter(project.open_issues_count) - - if pipeline_status && can?(current_user, :read_cross_project) && project.pipeline_status.has_status? + - if pipeline_status && can?(current_user, :read_cross_project) && project.pipeline_status.has_status? && can?(current_user, :read_build, project) %span.icon-wrapper.pipeline-status = render_project_pipeline_status(project.pipeline_status, tooltip_placement: 'top') .updated-note diff --git a/changelogs/unreleased/security-commit-status-shown-for-guest-user.yml b/changelogs/unreleased/security-commit-status-shown-for-guest-user.yml new file mode 100644 index 00000000000..a80170091d0 --- /dev/null +++ b/changelogs/unreleased/security-commit-status-shown-for-guest-user.yml @@ -0,0 +1,5 @@ +--- +title: Fix showing ci status for guest users when public pipline are not set +merge_request: +author: +type: security diff --git a/spec/features/dashboard/projects_spec.rb b/spec/features/dashboard/projects_spec.rb index edca8f9df08..6c4b04ab76b 100644 --- a/spec/features/dashboard/projects_spec.rb +++ b/spec/features/dashboard/projects_spec.rb @@ -147,6 +147,27 @@ describe 'Dashboard Projects' do expect(page).to have_link('Commit: passed') end end + + context 'guest user of project and project has private pipelines' do + let(:guest_user) { create(:user) } + + before do + project.update(public_builds: false) + project.add_guest(guest_user) + sign_in(guest_user) + end + + it 'shows that the last pipeline passed' do + visit dashboard_projects_path + + page.within('.controls') do + expect(page).not_to have_xpath("//a[@href='#{pipelines_project_commit_path(project, project.commit, ref: pipeline.ref)}']") + expect(page).not_to have_css('.ci-status-link') + expect(page).not_to have_css('.ci-status-icon-success') + expect(page).not_to have_link('Commit: passed') + end + end + end end context 'last push widget', :use_clean_rails_memory_store_caching do From 19f9666abf847efd4a5cb50d38faee77f91ec233 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 19 Dec 2018 12:49:59 +0100 Subject: [PATCH 29/42] Fix tree restorer visibility level --- .../security-import-project-visibility.yml | 5 ++ ..._update_project_import_visibility_level.rb | 60 +++++++++++++ .../import_export/project_tree_restorer.rb | 9 +- .../project_tree_restorer_spec.rb | 61 ++++++++++++- ...te_project_import_visibility_level_spec.rb | 86 +++++++++++++++++++ 5 files changed, 219 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/security-import-project-visibility.yml create mode 100644 db/post_migrate/20181219130552_update_project_import_visibility_level.rb create mode 100644 spec/migrations/update_project_import_visibility_level_spec.rb diff --git a/changelogs/unreleased/security-import-project-visibility.yml b/changelogs/unreleased/security-import-project-visibility.yml new file mode 100644 index 00000000000..04ae172a9a1 --- /dev/null +++ b/changelogs/unreleased/security-import-project-visibility.yml @@ -0,0 +1,5 @@ +--- +title: Restrict project import visibility based on its group +merge_request: +author: +type: security diff --git a/db/post_migrate/20181219130552_update_project_import_visibility_level.rb b/db/post_migrate/20181219130552_update_project_import_visibility_level.rb new file mode 100644 index 00000000000..6209de88b31 --- /dev/null +++ b/db/post_migrate/20181219130552_update_project_import_visibility_level.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +class UpdateProjectImportVisibilityLevel < ActiveRecord::Migration[5.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + BATCH_SIZE = 100 + + PRIVATE = 0 + INTERNAL = 10 + + disable_ddl_transaction! + + class Namespace < ActiveRecord::Base + self.table_name = 'namespaces' + end + + class Project < ActiveRecord::Base + include EachBatch + + belongs_to :namespace + + IMPORT_TYPE = 'gitlab_project' + + scope :with_group_visibility, ->(visibility) do + joins(:namespace) + .where(namespaces: { type: 'Group', visibility_level: visibility }) + .where(import_type: IMPORT_TYPE) + .where('projects.visibility_level > namespaces.visibility_level') + end + + self.table_name = 'projects' + end + + def up + # Update project's visibility to be the same as the group + # if it is more restrictive than `PUBLIC`. + update_projects_visibility(PRIVATE) + update_projects_visibility(INTERNAL) + end + + def down + # no-op: unrecoverable data migration + end + + private + + def update_projects_visibility(visibility) + say_with_time("Updating project visibility to #{visibility} on #{Project::IMPORT_TYPE} imports.") do + Project.with_group_visibility(visibility).select(:id).each_batch(of: BATCH_SIZE) do |batch, _index| + batch_sql = Gitlab::Database.mysql? ? batch.pluck(:id).join(', ') : batch.select(:id).to_sql + + say("Updating #{batch.size} items.", true) + + execute("UPDATE projects SET visibility_level = '#{visibility}' WHERE id IN (#{batch_sql})") + end + end + end +end diff --git a/lib/gitlab/import_export/project_tree_restorer.rb b/lib/gitlab/import_export/project_tree_restorer.rb index a56ec65b9f1..51001750a6c 100644 --- a/lib/gitlab/import_export/project_tree_restorer.rb +++ b/lib/gitlab/import_export/project_tree_restorer.rb @@ -107,7 +107,7 @@ module Gitlab def project_params @project_params ||= begin - attrs = json_params.merge(override_params) + attrs = json_params.merge(override_params).merge(visibility_level) # Cleaning all imported and overridden params Gitlab::ImportExport::AttributeCleaner.clean(relation_hash: attrs, @@ -127,6 +127,13 @@ module Gitlab end end + def visibility_level + level = override_params['visibility_level'] || json_params['visibility_level'] || @project.visibility_level + level = @project.group.visibility_level if @project.group && level > @project.group.visibility_level + + { 'visibility_level' => level } + end + # Given a relation hash containing one or more models and its relationships, # loops through each model and each object from a model type and # and assigns its correspondent attributes hash from +tree_hash+ diff --git a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb index 242c16c4bdc..9b0da882390 100644 --- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb @@ -273,6 +273,11 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do it 'has group milestone' do expect(project.group.milestones.size).to eq(results.fetch(:milestones, 0)) end + + it 'has the correct visibility level' do + # INTERNAL in the `project.json`, group's is PRIVATE + expect(project.visibility_level).to eq(Gitlab::VisibilityLevel::PRIVATE) + end end context 'Light JSON' do @@ -347,7 +352,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do :issues_disabled, name: 'project', path: 'project', - group: create(:group)) + group: create(:group, visibility_level: Gitlab::VisibilityLevel::PRIVATE)) end before do @@ -434,4 +439,58 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do end end end + + describe '#restored_project' do + let(:project) { create(:project) } + let(:shared) { project.import_export_shared } + let(:tree_hash) { { 'visibility_level' => visibility } } + let(:restorer) { described_class.new(user: nil, shared: shared, project: project) } + + before do + restorer.instance_variable_set(:@tree_hash, tree_hash) + end + + context 'no group visibility' do + let(:visibility) { Gitlab::VisibilityLevel::PRIVATE } + + it 'uses the project visibility' do + expect(restorer.restored_project.visibility_level).to eq(visibility) + end + end + + context 'with group visibility' do + before do + group = create(:group, visibility_level: group_visibility) + + project.update(group: group) + end + + context 'private group visibility' do + let(:group_visibility) { Gitlab::VisibilityLevel::PRIVATE } + let(:visibility) { Gitlab::VisibilityLevel::PUBLIC } + + it 'uses the group visibility' do + expect(restorer.restored_project.visibility_level).to eq(group_visibility) + end + end + + context 'public group visibility' do + let(:group_visibility) { Gitlab::VisibilityLevel::PUBLIC } + let(:visibility) { Gitlab::VisibilityLevel::PRIVATE } + + it 'uses the project visibility' do + expect(restorer.restored_project.visibility_level).to eq(visibility) + end + end + + context 'internal group visibility' do + let(:group_visibility) { Gitlab::VisibilityLevel::INTERNAL } + let(:visibility) { Gitlab::VisibilityLevel::PUBLIC } + + it 'uses the group visibility' do + expect(restorer.restored_project.visibility_level).to eq(group_visibility) + end + end + end + end end diff --git a/spec/migrations/update_project_import_visibility_level_spec.rb b/spec/migrations/update_project_import_visibility_level_spec.rb new file mode 100644 index 00000000000..9ea9b956f67 --- /dev/null +++ b/spec/migrations/update_project_import_visibility_level_spec.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20181219130552_update_project_import_visibility_level.rb') + +describe UpdateProjectImportVisibilityLevel, :migration do + let(:namespaces) { table(:namespaces) } + let(:projects) { table(:projects) } + let(:project) { projects.find_by_name(name) } + + before do + stub_const("#{described_class}::BATCH_SIZE", 1) + end + + context 'private visibility level' do + let(:name) { 'private-public' } + + it 'updates the project visibility' do + create_namespace(name, Gitlab::VisibilityLevel::PRIVATE) + create_project(name, Gitlab::VisibilityLevel::PUBLIC) + + expect { migrate! }.to change { project.reload.visibility_level }.to(Gitlab::VisibilityLevel::PRIVATE) + end + end + + context 'internal visibility level' do + let(:name) { 'internal-public' } + + it 'updates the project visibility' do + create_namespace(name, Gitlab::VisibilityLevel::INTERNAL) + create_project(name, Gitlab::VisibilityLevel::PUBLIC) + + expect { migrate! }.to change { project.reload.visibility_level }.to(Gitlab::VisibilityLevel::INTERNAL) + end + end + + context 'public visibility level' do + let(:name) { 'public-public' } + + it 'does not update the project visibility' do + create_namespace(name, Gitlab::VisibilityLevel::PUBLIC) + create_project(name, Gitlab::VisibilityLevel::PUBLIC) + + expect { migrate! }.not_to change { project.reload.visibility_level } + end + end + + context 'private project visibility level' do + let(:name) { 'public-private' } + + it 'does not update the project visibility' do + create_namespace(name, Gitlab::VisibilityLevel::PUBLIC) + create_project(name, Gitlab::VisibilityLevel::PRIVATE) + + expect { migrate! }.not_to change { project.reload.visibility_level } + end + end + + context 'no namespace' do + let(:name) { 'no-namespace' } + + it 'does not update the project visibility' do + create_namespace(name, Gitlab::VisibilityLevel::PRIVATE, type: nil) + create_project(name, Gitlab::VisibilityLevel::PUBLIC) + + expect { migrate! }.not_to change { project.reload.visibility_level } + end + end + + def create_namespace(name, visibility, options = {}) + namespaces.create({ + name: name, + path: name, + type: 'Group', + visibility_level: visibility + }.merge(options)) + end + + def create_project(name, visibility) + projects.create!(namespace_id: namespaces.find_by_name(name).id, + name: name, + path: name, + import_type: 'gitlab_project', + visibility_level: visibility) + end +end From 446a1da00e9cee53c91b06763f5b3992c21a7c9f Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Tue, 22 Jan 2019 12:49:14 +0000 Subject: [PATCH 30/42] Disable git v2 protocol temporarily --- GITALY_SERVER_VERSION | 2 +- .../unreleased/security-2780-disable-git-v2-protocol.yml | 5 +++++ doc/administration/git_protocol.md | 7 +++++++ 3 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/security-2780-disable-git-v2-protocol.yml diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index cd99d386a8d..63e799cf451 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -1.14.0 \ No newline at end of file +1.14.1 diff --git a/changelogs/unreleased/security-2780-disable-git-v2-protocol.yml b/changelogs/unreleased/security-2780-disable-git-v2-protocol.yml new file mode 100644 index 00000000000..30a08a98e83 --- /dev/null +++ b/changelogs/unreleased/security-2780-disable-git-v2-protocol.yml @@ -0,0 +1,5 @@ +--- +title: Disable git v2 protocol temporarily +merge_request: +author: +type: security diff --git a/doc/administration/git_protocol.md b/doc/administration/git_protocol.md index 341a00009e5..11b2adeeeb8 100644 --- a/doc/administration/git_protocol.md +++ b/doc/administration/git_protocol.md @@ -5,6 +5,13 @@ description: "Set and configure Git protocol v2" # Configuring Git Protocol v2 > [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/issues/46555) in GitLab 11.4. +> [Temporarily disabled](https://gitlab.com/gitlab-org/gitlab-ce/issues/55769) in GitLab 11.5.8, 11.6.6, 11.7.1, and 11.8+ + +NOTE: **Note:** +Git protocol v2 support has been [temporarily disabled](https://gitlab.com/gitlab-org/gitlab-ce/issues/55769), +as a feature used to hide certain internal references does not function when it +is enabled, and this has a security impact. Once this problem has been resolved, +protocol v2 support will be re-enabled. Git protocol v2 improves the v1 wire protocol in several ways and is enabled by default in GitLab for HTTP requests. In order to enable SSH, From 2dfa614c83ce1a4bfb818d1f040924b4d6e7e0c6 Mon Sep 17 00:00:00 2001 From: Constance Okoghenun Date: Thu, 24 Jan 2019 14:41:42 +0000 Subject: [PATCH 31/42] [master] Resolve "[Security] Stored XSS via KaTeX" --- .../security-stored-xss-via-katex.yml | 5 +++++ spec/features/markdown/math_spec.rb | 18 +++++++++++++++++- 2 files changed, 22 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/security-stored-xss-via-katex.yml diff --git a/changelogs/unreleased/security-stored-xss-via-katex.yml b/changelogs/unreleased/security-stored-xss-via-katex.yml new file mode 100644 index 00000000000..a71ae1123f2 --- /dev/null +++ b/changelogs/unreleased/security-stored-xss-via-katex.yml @@ -0,0 +1,5 @@ +--- +title: Fixed XSS content in KaTex links +merge_request: +author: +type: security diff --git a/spec/features/markdown/math_spec.rb b/spec/features/markdown/math_spec.rb index 678ce80b382..53abb5e3722 100644 --- a/spec/features/markdown/math_spec.rb +++ b/spec/features/markdown/math_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' describe 'Math rendering', :js do + let!(:project) { create(:project, :public) } + it 'renders inline and display math correctly' do description = <<~MATH This math is inline $`a^2+b^2=c^2`$. @@ -11,7 +13,6 @@ describe 'Math rendering', :js do ``` MATH - project = create(:project, :public) issue = create(:issue, project: project, description: description) visit project_issue_path(project, issue) @@ -19,4 +20,19 @@ describe 'Math rendering', :js do expect(page).to have_selector('.katex .mord.mathdefault', text: 'b') expect(page).to have_selector('.katex-display .mord.mathdefault', text: 'b') end + + it 'only renders non XSS links' do + description = <<~MATH + This link is valid $`\\href{javascript:alert('xss');}{xss}`$. + + This link is valid $`\\href{https://gitlab.com}{Gitlab}`$. + MATH + + issue = create(:issue, project: project, description: description) + + visit project_issue_path(project, issue) + + expect(page).to have_selector('.katex-error', text: "\href{javascript:alert('xss');}{xss}") + expect(page).to have_selector('.katex-html a', text: 'Gitlab') + end end From 0034119e5094474dc7d283b2e278dff9a37dc232 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Fri, 16 Nov 2018 16:09:32 +0100 Subject: [PATCH 32/42] Add subresources removal to member destroy service --- .../concerns/membership_actions.rb | 4 +- app/helpers/members_helper.rb | 15 ++++- app/models/member.rb | 1 + app/models/members/group_member.rb | 2 + app/models/members/project_member.rb | 4 ++ app/services/members/destroy_service.rb | 28 ++++++++- .../fix-security-group-user-removal.yml | 5 ++ .../groups/group_members_controller_spec.rb | 2 +- spec/helpers/members_helper_spec.rb | 4 +- spec/services/members/destroy_service_spec.rb | 60 +++++++++++++++++-- 10 files changed, 114 insertions(+), 11 deletions(-) create mode 100644 changelogs/unreleased/fix-security-group-user-removal.yml diff --git a/app/controllers/concerns/membership_actions.rb b/app/controllers/concerns/membership_actions.rb index ca713192c9e..6402e01ddc0 100644 --- a/app/controllers/concerns/membership_actions.rb +++ b/app/controllers/concerns/membership_actions.rb @@ -35,7 +35,9 @@ module MembershipActions respond_to do |format| format.html do - message = "User was successfully removed from #{source_type}." + source = source_type == 'group' ? 'group and any subresources' : source_type + + message = "User was successfully removed from #{source}." redirect_to members_page_url, notice: message end diff --git a/app/helpers/members_helper.rb b/app/helpers/members_helper.rb index ab4a1ccc0d1..11d5591d509 100644 --- a/app/helpers/members_helper.rb +++ b/app/helpers/members_helper.rb @@ -18,12 +18,13 @@ module MembersHelper "remove #{member.user.name} from" end - "#{text} #{action} the #{member.source.human_name} #{member.real_source_type.humanize(capitalize: false)}?" + "#{text} #{action} the #{member.source.human_name} #{source_text(member)}?" end def remove_member_title(member) action = member.request? ? 'Deny access request' : 'Remove user' - "#{action} from #{member.real_source_type.humanize(capitalize: false)}" + + "#{action} from #{source_text(member)}" end def leave_confirmation_message(member_source) @@ -35,4 +36,14 @@ module MembersHelper options = params.slice(:search, :sort).merge(options).permit! "#{request.path}?#{options.to_param}" end + + private + + def source_text(member) + type = member.real_source_type.humanize(capitalize: false) + + return type if member.request? || member.invite? || type != 'group' + + 'group and any subresources' + end end diff --git a/app/models/member.rb b/app/models/member.rb index 82d207b79de..18667b2e986 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -78,6 +78,7 @@ class Member < ActiveRecord::Base scope :owners, -> { active.where(access_level: OWNER) } scope :owners_and_maintainers, -> { active.where(access_level: [OWNER, MAINTAINER]) } scope :owners_and_masters, -> { owners_and_maintainers } # @deprecated + scope :with_user, -> (user) { where(user: user) } scope :order_name_asc, -> { left_join_users.reorder(Gitlab::Database.nulls_last_order('users.name', 'ASC')) } scope :order_name_desc, -> { left_join_users.reorder(Gitlab::Database.nulls_last_order('users.name', 'DESC')) } diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb index fc49ee7ac8c..2c9e1ba1d80 100644 --- a/app/models/members/group_member.rb +++ b/app/models/members/group_member.rb @@ -12,6 +12,8 @@ class GroupMember < Member validates :source_type, format: { with: /\ANamespace\z/ } default_scope { where(source_type: SOURCE_TYPE) } + scope :in_groups, ->(groups) { where(source_id: groups.select(:id)) } + after_create :update_two_factor_requirement, unless: :invite? after_destroy :update_two_factor_requirement, unless: :invite? diff --git a/app/models/members/project_member.rb b/app/models/members/project_member.rb index 016c18ce6c8..5372c6084f4 100644 --- a/app/models/members/project_member.rb +++ b/app/models/members/project_member.rb @@ -12,6 +12,10 @@ class ProjectMember < Member default_scope { where(source_type: SOURCE_TYPE) } scope :in_project, ->(project) { where(source_id: project.id) } + scope :in_namespaces, ->(groups) do + joins('INNER JOIN projects ON projects.id = members.source_id') + .where('projects.namespace_id in (?)', groups.select(:id)) + end class << self # Add users to projects with passed access option diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb index ae0c644e6c0..f9717a9426b 100644 --- a/app/services/members/destroy_service.rb +++ b/app/services/members/destroy_service.rb @@ -2,9 +2,11 @@ module Members class DestroyService < Members::BaseService - def execute(member, skip_authorization: false) + def execute(member, skip_authorization: false, skip_subresources: false) raise Gitlab::Access::AccessDeniedError unless skip_authorization || can_destroy_member?(member) + @skip_auth = skip_authorization + return member if member.is_a?(GroupMember) && member.source.last_owner?(member.user) member.destroy @@ -15,6 +17,7 @@ module Members notification_service.decline_access_request(member) end + delete_subresources(member) unless skip_subresources enqueue_delete_todos(member) after_execute(member: member) @@ -24,6 +27,29 @@ module Members private + def delete_subresources(member) + return unless member.is_a?(GroupMember) && member.user && member.group + + delete_project_members(member) + delete_subgroup_members(member) if Group.supports_nested_objects? + end + + def delete_project_members(member) + groups = member.group.self_and_descendants + + ProjectMember.in_namespaces(groups).with_user(member.user).each do |project_member| + self.class.new(current_user).execute(project_member, skip_authorization: @skip_auth) + end + end + + def delete_subgroup_members(member) + groups = member.group.descendants + + GroupMember.in_groups(groups).with_user(member.user).each do |group_member| + self.class.new(current_user).execute(group_member, skip_authorization: @skip_auth, skip_subresources: true) + end + end + def can_destroy_member?(member) can?(current_user, destroy_member_permission(member), member) end diff --git a/changelogs/unreleased/fix-security-group-user-removal.yml b/changelogs/unreleased/fix-security-group-user-removal.yml new file mode 100644 index 00000000000..09d09a96f84 --- /dev/null +++ b/changelogs/unreleased/fix-security-group-user-removal.yml @@ -0,0 +1,5 @@ +--- +title: Add subresources removal to member destroy service +merge_request: +author: +type: security diff --git a/spec/controllers/groups/group_members_controller_spec.rb b/spec/controllers/groups/group_members_controller_spec.rb index ed38dadfd6b..3a801fabafc 100644 --- a/spec/controllers/groups/group_members_controller_spec.rb +++ b/spec/controllers/groups/group_members_controller_spec.rb @@ -126,7 +126,7 @@ describe Groups::GroupMembersController do it '[HTML] removes user from members' do delete :destroy, params: { group_id: group, id: member } - expect(response).to set_flash.to 'User was successfully removed from group.' + expect(response).to set_flash.to 'User was successfully removed from group and any subresources.' expect(response).to redirect_to(group_group_members_path(group)) expect(group.members).not_to include member end diff --git a/spec/helpers/members_helper_spec.rb b/spec/helpers/members_helper_spec.rb index 4590904c93d..908e8960f37 100644 --- a/spec/helpers/members_helper_spec.rb +++ b/spec/helpers/members_helper_spec.rb @@ -16,7 +16,7 @@ describe MembersHelper do it { expect(remove_member_message(project_member_invite)).to eq "Are you sure you want to revoke the invitation for #{project_member_invite.invite_email} to join the #{project.full_name} project?" } it { expect(remove_member_message(project_member_request)).to eq "Are you sure you want to deny #{requester.name}'s request to join the #{project.full_name} project?" } it { expect(remove_member_message(project_member_request, user: requester)).to eq "Are you sure you want to withdraw your access request for the #{project.full_name} project?" } - it { expect(remove_member_message(group_member)).to eq "Are you sure you want to remove #{group_member.user.name} from the #{group.name} group?" } + it { expect(remove_member_message(group_member)).to eq "Are you sure you want to remove #{group_member.user.name} from the #{group.name} group and any subresources?" } it { expect(remove_member_message(group_member_invite)).to eq "Are you sure you want to revoke the invitation for #{group_member_invite.invite_email} to join the #{group.name} group?" } it { expect(remove_member_message(group_member_request)).to eq "Are you sure you want to deny #{requester.name}'s request to join the #{group.name} group?" } it { expect(remove_member_message(group_member_request, user: requester)).to eq "Are you sure you want to withdraw your access request for the #{group.name} group?" } @@ -33,7 +33,7 @@ describe MembersHelper do it { expect(remove_member_title(project_member)).to eq 'Remove user from project' } it { expect(remove_member_title(project_member_request)).to eq 'Deny access request from project' } - it { expect(remove_member_title(group_member)).to eq 'Remove user from group' } + it { expect(remove_member_title(group_member)).to eq 'Remove user from group and any subresources' } it { expect(remove_member_title(group_member_request)).to eq 'Deny access request from group' } end diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb index 5aa7165e135..e872a537761 100644 --- a/spec/services/members/destroy_service_spec.rb +++ b/spec/services/members/destroy_service_spec.rb @@ -69,14 +69,14 @@ describe Members::DestroyService do it 'calls Member#after_decline_request' do expect_any_instance_of(NotificationService).to receive(:decline_access_request).with(member) - described_class.new(current_user).execute(member) + described_class.new(current_user).execute(member, opts) end context 'when current user is the member' do it 'does not call Member#after_decline_request' do expect_any_instance_of(NotificationService).not_to receive(:decline_access_request).with(member) - described_class.new(member_user).execute(member) + described_class.new(member_user).execute(member, opts) end end end @@ -159,7 +159,7 @@ describe Members::DestroyService do end it_behaves_like 'a service destroying a member' do - let(:opts) { { skip_authorization: true } } + let(:opts) { { skip_authorization: true, skip_subresources: true } } let(:member) { group_project.requesters.find_by(user_id: member_user.id) } end @@ -168,12 +168,14 @@ describe Members::DestroyService do end it_behaves_like 'a service destroying a member' do - let(:opts) { { skip_authorization: true } } + let(:opts) { { skip_authorization: true, skip_subresources: true } } let(:member) { group.requesters.find_by(user_id: member_user.id) } end end context 'when current user can destroy the given access requester' do + let(:opts) { { skip_subresources: true } } + before do group_project.add_maintainer(current_user) group.add_owner(current_user) @@ -229,4 +231,54 @@ describe Members::DestroyService do end end end + + context 'subresources' do + let(:user) { create(:user) } + let(:member_user) { create(:user) } + let(:opts) { {} } + + let(:group) { create(:group, :public) } + let(:subgroup) { create(:group, parent: group) } + let(:subsubgroup) { create(:group, parent: subgroup) } + let(:subsubproject) { create(:project, group: subsubgroup) } + + let(:group_project) { create(:project, :public, group: group) } + let(:control_project) { create(:project, group: subsubgroup) } + + before do + create(:group_member, :developer, group: subsubgroup, user: member_user) + + subsubproject.add_developer(member_user) + control_project.add_maintainer(user) + group.add_owner(user) + + group_member = create(:group_member, :developer, group: group, user: member_user) + + described_class.new(user).execute(group_member, opts) + end + + it 'removes the project membership' do + expect(group_project.members.map(&:user)).not_to include(member_user) + end + + it 'removes the group membership' do + expect(group.members.map(&:user)).not_to include(member_user) + end + + it 'removes the subgroup membership', :postgresql do + expect(subgroup.members.map(&:user)).not_to include(member_user) + end + + it 'removes the subsubgroup membership', :postgresql do + expect(subsubgroup.members.map(&:user)).not_to include(member_user) + end + + it 'removes the subsubproject membership', :postgresql do + expect(subsubproject.members.map(&:user)).not_to include(member_user) + end + + it 'does not remove the user from the control project' do + expect(control_project.members.map(&:user)).to include(user) + end + end end From 6bd8e4cb867712326e658d11d4c530a350a5eefb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20L=C3=B3pez?= Date: Fri, 25 Jan 2019 12:11:36 +0000 Subject: [PATCH 33/42] [master] Check access rights when creating/updating ProtectedRefs --- .../protected_branches/api_service.rb | 8 ------- spec/lib/gitlab/git_access_spec.rb | 23 +++++++------------ 2 files changed, 8 insertions(+), 23 deletions(-) diff --git a/app/services/protected_branches/api_service.rb b/app/services/protected_branches/api_service.rb index 4340d3e8260..9b85e13107b 100644 --- a/app/services/protected_branches/api_service.rb +++ b/app/services/protected_branches/api_service.rb @@ -6,8 +6,6 @@ module ProtectedBranches @push_params = AccessLevelParams.new(:push, params) @merge_params = AccessLevelParams.new(:merge, params) - verify_params! - protected_branch_params = { name: params[:name], push_access_levels_attributes: @push_params.access_levels, @@ -16,11 +14,5 @@ module ProtectedBranches ::ProtectedBranches::CreateService.new(@project, @current_user, protected_branch_params).execute end - - private - - def verify_params! - # EE-only - end end end diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 3e34dd592f2..634c370d211 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -776,10 +776,13 @@ describe Gitlab::GitAccess do it "has the correct permissions for #{role}s" do if role == :admin user.update_attribute(:admin, true) + project.add_guest(user) else project.add_role(user, role) end + protected_branch.save + aggregate_failures do matrix.each do |action, allowed| check = -> { push_changes(changes[action]) } @@ -861,25 +864,19 @@ describe Gitlab::GitAccess do [%w(feature exact), ['feat*', 'wildcard']].each do |protected_branch_name, protected_branch_type| context do - before do - create(:protected_branch, name: protected_branch_name, project: project) - end + let(:protected_branch) { create(:protected_branch, :maintainers_can_push, name: protected_branch_name, project: project) } run_permission_checks(permissions_matrix) end context "when developers are allowed to push into the #{protected_branch_type} protected branch" do - before do - create(:protected_branch, :developers_can_push, name: protected_branch_name, project: project) - end + let(:protected_branch) { create(:protected_branch, :developers_can_push, name: protected_branch_name, project: project) } run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true })) end context "developers are allowed to merge into the #{protected_branch_type} protected branch" do - before do - create(:protected_branch, :developers_can_merge, name: protected_branch_name, project: project) - end + let(:protected_branch) { create(:protected_branch, :developers_can_merge, name: protected_branch_name, project: project) } context "when a merge request exists for the given source/target branch" do context "when the merge request is in progress" do @@ -906,17 +903,13 @@ describe Gitlab::GitAccess do end context "when developers are allowed to push and merge into the #{protected_branch_type} protected branch" do - before do - create(:protected_branch, :developers_can_merge, :developers_can_push, name: protected_branch_name, project: project) - end + let(:protected_branch) { create(:protected_branch, :developers_can_merge, :developers_can_push, name: protected_branch_name, project: project) } run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true })) end context "when no one is allowed to push to the #{protected_branch_name} protected branch" do - before do - create(:protected_branch, :no_one_can_push, name: protected_branch_name, project: project) - end + let(:protected_branch) { build(:protected_branch, :no_one_can_push, name: protected_branch_name, project: project) } run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false }, maintainer: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false }, From a0383ab43ec2c885aae6602ffa47ffde79c76786 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Mon, 28 Jan 2019 12:12:30 +0000 Subject: [PATCH 34/42] [master] Pipelines section is available to unauthorized users --- .../merge_requests/application_controller.rb | 9 ++-- .../projects/pipelines_controller.rb | 1 + app/helpers/projects_helper.rb | 3 +- app/models/commit.rb | 5 +- app/models/project.rb | 8 +++ app/policies/ci/pipeline_policy.rb | 9 ++++ app/policies/project_policy.rb | 20 +++++-- app/presenters/commit_presenter.rb | 13 +++++ app/presenters/merge_request_presenter.rb | 4 ++ .../merge_request_widget_entity.rb | 2 +- app/views/projects/commit/_ci_menu.html.haml | 4 +- .../projects/commit/_commit_box.html.haml | 4 +- app/views/projects/commit/show.html.haml | 5 +- app/views/projects/commits/_commit.html.haml | 5 +- .../projects/issues/_merge_requests.html.haml | 3 +- .../issues/_related_branches.html.haml | 2 +- .../merge_requests/_merge_request.html.haml | 2 +- app/views/projects/pipelines/_info.html.haml | 33 ++++++------ changelogs/unreleased/test-permissions.yml | 5 ++ lib/api/pipelines.rb | 6 +-- .../pipeline_schedules_controller_spec.rb | 11 +++- .../projects/pipelines_controller_spec.rb | 47 ++++++++++------ .../security/project/internal_access_spec.rb | 6 +-- .../security/project/private_access_spec.rb | 2 +- .../security/project/public_access_spec.rb | 10 ++-- spec/helpers/projects_helper_spec.rb | 16 +++++- .../project_tree_restorer_spec.rb | 4 +- spec/models/commit_spec.rb | 1 + spec/models/project_spec.rb | 24 +++++++++ spec/policies/ci/pipeline_policy_spec.rb | 8 +++ spec/policies/project_policy_spec.rb | 44 ++++++++++----- spec/presenters/commit_presenter_spec.rb | 54 +++++++++++++++++++ .../merge_request_widget_entity_spec.rb | 39 ++++++++++---- .../commit/_commit_box.html.haml_spec.rb | 6 ++- .../_related_branches.html.haml_spec.rb | 4 ++ 35 files changed, 324 insertions(+), 95 deletions(-) create mode 100644 app/presenters/commit_presenter.rb create mode 100644 changelogs/unreleased/test-permissions.yml create mode 100644 spec/presenters/commit_presenter_spec.rb diff --git a/app/controllers/projects/merge_requests/application_controller.rb b/app/controllers/projects/merge_requests/application_controller.rb index 368ee89ff5c..54ff7ded8e5 100644 --- a/app/controllers/projects/merge_requests/application_controller.rb +++ b/app/controllers/projects/merge_requests/application_controller.rb @@ -39,8 +39,11 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont end def set_pipeline_variables - @pipelines = @merge_request.all_pipelines - @pipeline = @merge_request.head_pipeline - @statuses_count = @pipeline.present? ? @pipeline.statuses.relevant.count : 0 + @pipelines = + if can?(current_user, :read_pipeline, @project) + @merge_request.all_pipelines + else + Ci::Pipeline.none + end end end diff --git a/app/controllers/projects/pipelines_controller.rb b/app/controllers/projects/pipelines_controller.rb index 67827b1d3bb..df43d9994a1 100644 --- a/app/controllers/projects/pipelines_controller.rb +++ b/app/controllers/projects/pipelines_controller.rb @@ -4,6 +4,7 @@ class Projects::PipelinesController < Projects::ApplicationController before_action :whitelist_query_limiting, only: [:create, :retry] before_action :pipeline, except: [:index, :new, :create, :charts] before_action :authorize_read_pipeline! + before_action :authorize_read_build!, only: [:index] before_action :authorize_create_pipeline!, only: [:new, :create] before_action :authorize_update_pipeline!, only: [:retry, :cancel] diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index b0d3d509f5d..85248a16f50 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -305,7 +305,8 @@ module ProjectsHelper nav_tabs << :container_registry end - if project.builds_enabled? && can?(current_user, :read_pipeline, project) + # Pipelines feature is tied to presence of builds + if can?(current_user, :read_build, project) nav_tabs << :pipelines end diff --git a/app/models/commit.rb b/app/models/commit.rb index 01f4c58daa1..982e13e2845 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -11,6 +11,7 @@ class Commit include Mentionable include Referable include StaticModel + include Presentable include ::Gitlab::Utils::StrongMemoize attr_mentionable :safe_message, pipeline: :single_line @@ -304,7 +305,9 @@ class Commit end def last_pipeline - @last_pipeline ||= pipelines.last + strong_memoize(:last_pipeline) do + pipelines.last + end end def status(ref = nil) diff --git a/app/models/project.rb b/app/models/project.rb index 1023b40a608..cecfee82334 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -578,6 +578,14 @@ class Project < ActiveRecord::Base end end + def all_pipelines + if builds_enabled? + super + else + super.external + end + end + # returns all ancestor-groups upto but excluding the given namespace # when no namespace is given, all ancestors upto the top are returned def ancestors_upto(top = nil, hierarchy_order: nil) diff --git a/app/policies/ci/pipeline_policy.rb b/app/policies/ci/pipeline_policy.rb index e42d78f47c5..2c90b8a73cd 100644 --- a/app/policies/ci/pipeline_policy.rb +++ b/app/policies/ci/pipeline_policy.rb @@ -10,6 +10,15 @@ module Ci @subject.project.branch_allows_collaboration?(@user, @subject.ref) end + condition(:external_pipeline, scope: :subject, score: 0) do + @subject.external? + end + + # Disallow users without permissions from accessing internal pipelines + rule { ~can?(:read_build) & ~external_pipeline }.policy do + prevent :read_pipeline + end + rule { protected_ref }.prevent :update_pipeline rule { can?(:public_access) & branch_allows_collaboration }.policy do diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 95ae85ed60c..cadbc5ae009 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -108,6 +108,10 @@ class ProjectPolicy < BasePolicy condition(:has_clusters, scope: :subject) { clusterable_has_clusters? } condition(:can_have_multiple_clusters) { multiple_clusters_available? } + condition(:internal_builds_disabled) do + !@subject.builds_enabled? + end + features = %w[ merge_requests issues @@ -196,7 +200,6 @@ class ProjectPolicy < BasePolicy enable :read_build enable :read_container_image enable :read_pipeline - enable :read_pipeline_schedule enable :read_environment enable :read_deployment enable :read_merge_request @@ -235,6 +238,7 @@ class ProjectPolicy < BasePolicy enable :update_build enable :create_pipeline enable :update_pipeline + enable :read_pipeline_schedule enable :create_pipeline_schedule enable :create_merge_request_from enable :create_wiki @@ -320,7 +324,6 @@ class ProjectPolicy < BasePolicy end rule { builds_disabled | repository_disabled }.policy do - prevent(*create_update_admin_destroy(:pipeline)) prevent(*create_read_update_admin_destroy(:build)) prevent(*create_read_update_admin_destroy(:pipeline_schedule)) prevent(*create_read_update_admin_destroy(:environment)) @@ -328,11 +331,22 @@ class ProjectPolicy < BasePolicy prevent(*create_read_update_admin_destroy(:deployment)) end + # There's two separate cases when builds_disabled is true: + # 1. When internal CI is disabled - builds_disabled && internal_builds_disabled + # - We do not prevent the user from accessing Pipelines to allow him to access external CI + # 2. When the user is not allowed to access CI - builds_disabled && ~internal_builds_disabled + # - We prevent the user from accessing Pipelines + rule { (builds_disabled & ~internal_builds_disabled) | repository_disabled }.policy do + prevent(*create_read_update_admin_destroy(:pipeline)) + prevent(*create_read_update_admin_destroy(:commit_status)) + end + rule { repository_disabled }.policy do prevent :push_code prevent :download_code prevent :fork_project prevent :read_commit_status + prevent :read_pipeline prevent(*create_read_update_admin_destroy(:release)) end @@ -359,7 +373,6 @@ class ProjectPolicy < BasePolicy enable :read_merge_request enable :read_note enable :read_pipeline - enable :read_pipeline_schedule enable :read_commit_status enable :read_container_image enable :download_code @@ -378,7 +391,6 @@ class ProjectPolicy < BasePolicy rule { public_builds & can?(:guest_access) }.policy do enable :read_pipeline - enable :read_pipeline_schedule end # These rules are included to allow maintainers of projects to push to certain diff --git a/app/presenters/commit_presenter.rb b/app/presenters/commit_presenter.rb new file mode 100644 index 00000000000..05adbe1d4f5 --- /dev/null +++ b/app/presenters/commit_presenter.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class CommitPresenter < Gitlab::View::Presenter::Simple + presents :commit + + def status_for(ref) + can?(current_user, :read_commit_status, commit.project) && commit.status(ref) + end + + def any_pipelines? + can?(current_user, :read_pipeline, commit.project) && commit.pipelines.any? + end +end diff --git a/app/presenters/merge_request_presenter.rb b/app/presenters/merge_request_presenter.rb index 44b6ca299ae..c59e73f824c 100644 --- a/app/presenters/merge_request_presenter.rb +++ b/app/presenters/merge_request_presenter.rb @@ -170,6 +170,10 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated source_branch_exists? && merge_request.can_remove_source_branch?(current_user) end + def can_read_pipeline? + pipeline && can?(current_user, :read_pipeline, pipeline) + end + def mergeable_discussions_state # This avoids calling MergeRequest#mergeable_discussions_state without # considering the state of the MR first. If a MR isn't mergeable, we can diff --git a/app/serializers/merge_request_widget_entity.rb b/app/serializers/merge_request_widget_entity.rb index 9361c9f987b..f42abf06e1e 100644 --- a/app/serializers/merge_request_widget_entity.rb +++ b/app/serializers/merge_request_widget_entity.rb @@ -57,7 +57,7 @@ class MergeRequestWidgetEntity < IssuableEntity end expose :merge_commit_message - expose :actual_head_pipeline, with: PipelineDetailsEntity, as: :pipeline + expose :actual_head_pipeline, with: PipelineDetailsEntity, as: :pipeline, if: -> (mr, _) { presenter(mr).can_read_pipeline? } expose :merge_pipeline, with: PipelineDetailsEntity, if: ->(mr, _) { mr.merged? && can?(request.current_user, :read_pipeline, mr.target_project)} # Booleans diff --git a/app/views/projects/commit/_ci_menu.html.haml b/app/views/projects/commit/_ci_menu.html.haml index f6666921a25..8b6e3e42ea1 100644 --- a/app/views/projects/commit/_ci_menu.html.haml +++ b/app/views/projects/commit/_ci_menu.html.haml @@ -1,9 +1,11 @@ +- any_pipelines = @commit.present(current_user: current_user).any_pipelines? + %ul.nav-links.no-top.no-bottom.commit-ci-menu.nav.nav-tabs = nav_link(path: 'commit#show') do = link_to project_commit_path(@project, @commit.id) do Changes %span.badge.badge-pill= @diffs.size - - if can?(current_user, :read_pipeline, @project) + - if any_pipelines = nav_link(path: 'commit#pipelines') do = link_to pipelines_project_commit_path(@project, @commit.id) do Pipelines diff --git a/app/views/projects/commit/_commit_box.html.haml b/app/views/projects/commit/_commit_box.html.haml index 2a919a767c0..3971ca473e0 100644 --- a/app/views/projects/commit/_commit_box.html.haml +++ b/app/views/projects/commit/_commit_box.html.haml @@ -74,8 +74,8 @@ %span.commit-info.merge-requests{ 'data-project-commit-path' => merge_requests_project_commit_path(@project, @commit.id, format: :json) } = icon('spinner spin') - - if @commit.last_pipeline - - last_pipeline = @commit.last_pipeline + - last_pipeline = @commit.last_pipeline + - if can?(current_user, :read_pipeline, last_pipeline) .well-segment.pipeline-info .status-icon-container = link_to project_pipeline_path(@project, last_pipeline.id), class: "ci-status-icon-#{last_pipeline.status}" do diff --git a/app/views/projects/commit/show.html.haml b/app/views/projects/commit/show.html.haml index 79e32949db9..06f0cd9675e 100644 --- a/app/views/projects/commit/show.html.haml +++ b/app/views/projects/commit/show.html.haml @@ -9,10 +9,7 @@ .container-fluid{ class: [limited_container_width, container_class] } = render "commit_box" - - if @commit.status - = render "ci_menu" - - else - .block-connector + = render "ci_menu" = render "projects/diffs/diffs", diffs: @diffs, environment: @environment, is_commit: true .limited-width-notes diff --git a/app/views/projects/commits/_commit.html.haml b/app/views/projects/commits/_commit.html.haml index 1a74b120c26..0d3c6e7027c 100644 --- a/app/views/projects/commits/_commit.html.haml +++ b/app/views/projects/commits/_commit.html.haml @@ -6,6 +6,7 @@ - merge_request = local_assigns.fetch(:merge_request, nil) - project = local_assigns.fetch(:project) { merge_request&.project } - ref = local_assigns.fetch(:ref) { merge_request&.source_branch } +- commit_status = commit.present(current_user: current_user).status_for(ref) - link = commit_path(project, commit, merge_request: merge_request) %li.commit.flex-row.js-toggle-container{ id: "commit-#{commit.short_id}" } @@ -22,7 +23,7 @@ %span.commit-row-message.d-block.d-sm-none · = commit.short_id - - if commit.status(ref) + - if commit_status .d-block.d-sm-none = render_commit_status(commit, ref: ref) - if commit.description? @@ -45,7 +46,7 @@ - else = render partial: 'projects/commit/ajax_signature', locals: { commit: commit } - - if commit.status(ref) + - if commit_status = render_commit_status(commit, ref: ref) .js-commit-pipeline-status{ data: { endpoint: pipelines_project_commit_path(project, commit.id, ref: ref) } } diff --git a/app/views/projects/issues/_merge_requests.html.haml b/app/views/projects/issues/_merge_requests.html.haml index eb46bf5c6a3..6f652391be3 100644 --- a/app/views/projects/issues/_merge_requests.html.haml +++ b/app/views/projects/issues/_merge_requests.html.haml @@ -12,6 +12,7 @@ %ul.content-list.related-items-list - has_any_head_pipeline = @merge_requests.any?(&:head_pipeline_id) - @merge_requests.each do |merge_request| + - merge_request = merge_request.present(current_user: current_user) %li.list-item.py-0.px-0 .item-body.issuable-info-container.py-lg-3.px-lg-3.pl-md-3 .item-contents @@ -25,7 +26,7 @@ = merge_request.target_project.full_path = merge_request.to_reference %span.mr-ci-status.flex-md-grow-1.justify-content-end.d-flex.ml-md-2 - - if merge_request.head_pipeline + - if merge_request.can_read_pipeline? = render_pipeline_status(merge_request.head_pipeline, tooltip_placement: 'bottom') - elsif has_any_head_pipeline = icon('blank fw') diff --git a/app/views/projects/issues/_related_branches.html.haml b/app/views/projects/issues/_related_branches.html.haml index 1df38db9fd4..ffdd96870ef 100644 --- a/app/views/projects/issues/_related_branches.html.haml +++ b/app/views/projects/issues/_related_branches.html.haml @@ -6,7 +6,7 @@ %li - target = @project.repository.find_branch(branch).dereferenced_target - pipeline = @project.pipeline_for(branch, target.sha) if target - - if pipeline + - if can?(current_user, :read_pipeline, pipeline) %span.related-branch-ci-status = render_pipeline_status(pipeline) %span.related-branch-info diff --git a/app/views/projects/merge_requests/_merge_request.html.haml b/app/views/projects/merge_requests/_merge_request.html.haml index faa070d0389..d7994909366 100644 --- a/app/views/projects/merge_requests/_merge_request.html.haml +++ b/app/views/projects/merge_requests/_merge_request.html.haml @@ -46,7 +46,7 @@ %li.issuable-status.d-none.d-sm-inline-block = icon('ban') CLOSED - - if merge_request.head_pipeline + - if can?(current_user, :read_pipeline, merge_request.head_pipeline) %li.issuable-pipeline-status.d-none.d-sm-inline-block = render_pipeline_status(merge_request.head_pipeline) - if merge_request.open? && merge_request.broken? diff --git a/app/views/projects/pipelines/_info.html.haml b/app/views/projects/pipelines/_info.html.haml index 0f0114d513c..69a47faabed 100644 --- a/app/views/projects/pipelines/_info.html.haml +++ b/app/views/projects/pipelines/_info.html.haml @@ -6,23 +6,22 @@ = preserve(markdown(commit.description, pipeline: :single_line)) .info-well - - if commit.status - .well-segment.pipeline-info - .icon-container - = icon('clock-o') - = pluralize @pipeline.total_size, "job" - - if @pipeline.ref - from - - if @pipeline.ref_exists? - = link_to @pipeline.ref, project_ref_path(@project, @pipeline.ref), class: "ref-name" - - else - %span.ref-name - = @pipeline.ref - - if @pipeline.duration - in - = time_interval_in_words(@pipeline.duration) - - if @pipeline.queued_duration - = "(queued for #{time_interval_in_words(@pipeline.queued_duration)})" + .well-segment.pipeline-info + .icon-container + = icon('clock-o') + = pluralize @pipeline.total_size, "job" + - if @pipeline.ref + from + - if @pipeline.ref_exists? + = link_to @pipeline.ref, project_ref_path(@project, @pipeline.ref), class: "ref-name" + - else + %span.ref-name + = @pipeline.ref + - if @pipeline.duration + in + = time_interval_in_words(@pipeline.duration) + - if @pipeline.queued_duration + = "(queued for #{time_interval_in_words(@pipeline.queued_duration)})" .well-segment .icon-container diff --git a/changelogs/unreleased/test-permissions.yml b/changelogs/unreleased/test-permissions.yml new file mode 100644 index 00000000000..cfb69fdcb1e --- /dev/null +++ b/changelogs/unreleased/test-permissions.yml @@ -0,0 +1,5 @@ +--- +title: Disallows unauthorized users from accessing the pipelines section. +merge_request: +author: +type: security diff --git a/lib/api/pipelines.rb b/lib/api/pipelines.rb index 7a7b23d2bbb..0317d69edde 100644 --- a/lib/api/pipelines.rb +++ b/lib/api/pipelines.rb @@ -76,7 +76,7 @@ module API requires :pipeline_id, type: Integer, desc: 'The pipeline ID' end get ':id/pipelines/:pipeline_id' do - authorize! :read_pipeline, user_project + authorize! :read_pipeline, pipeline present pipeline, with: Entities::Pipeline end @@ -104,7 +104,7 @@ module API requires :pipeline_id, type: Integer, desc: 'The pipeline ID' end post ':id/pipelines/:pipeline_id/retry' do - authorize! :update_pipeline, user_project + authorize! :update_pipeline, pipeline pipeline.retry_failed(current_user) @@ -119,7 +119,7 @@ module API requires :pipeline_id, type: Integer, desc: 'The pipeline ID' end post ':id/pipelines/:pipeline_id/cancel' do - authorize! :update_pipeline, user_project + authorize! :update_pipeline, pipeline pipeline.cancel_running diff --git a/spec/controllers/projects/pipeline_schedules_controller_spec.rb b/spec/controllers/projects/pipeline_schedules_controller_spec.rb index 80506249ea9..fa732437fc1 100644 --- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb +++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb @@ -3,9 +3,14 @@ require 'spec_helper' describe Projects::PipelineSchedulesController do include AccessMatchersForController + set(:user) { create(:user) } set(:project) { create(:project, :public, :repository) } set(:pipeline_schedule) { create(:ci_pipeline_schedule, project: project) } + before do + project.add_developer(user) + end + describe 'GET #index' do render_views @@ -14,6 +19,10 @@ describe Projects::PipelineSchedulesController do create(:ci_pipeline_schedule, :inactive, project: project) end + before do + sign_in(user) + end + it 'renders the index view' do visit_pipelines_schedules @@ -21,7 +30,7 @@ describe Projects::PipelineSchedulesController do expect(response).to render_template(:index) end - it 'avoids N + 1 queries' do + it 'avoids N + 1 queries', :request_store do control_count = ActiveRecord::QueryRecorder.new { visit_pipelines_schedules }.count create_list(:ci_pipeline_schedule, 2, project: project) diff --git a/spec/controllers/projects/pipelines_controller_spec.rb b/spec/controllers/projects/pipelines_controller_spec.rb index 0bb3ef76a3b..740b28f0f46 100644 --- a/spec/controllers/projects/pipelines_controller_spec.rb +++ b/spec/controllers/projects/pipelines_controller_spec.rb @@ -5,7 +5,7 @@ describe Projects::PipelinesController do set(:user) { create(:user) } let(:project) { create(:project, :public, :repository) } - let(:feature) { ProjectFeature::DISABLED } + let(:feature) { ProjectFeature::ENABLED } before do stub_not_protect_default_branch @@ -186,6 +186,27 @@ describe Projects::PipelinesController do end end + context 'when builds are disabled' do + let(:feature) { ProjectFeature::DISABLED } + + it 'users can not see internal pipelines' do + get_pipeline_json + + expect(response).to have_gitlab_http_status(:not_found) + end + + context 'when pipeline is external' do + let(:pipeline) { create(:ci_pipeline, source: :external, project: project) } + + it 'users can see the external pipeline' do + get_pipeline_json + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['id']).to be(pipeline.id) + end + end + end + def get_pipeline_json get :show, params: { namespace_id: project.namespace, project_id: project, id: pipeline }, format: :json end @@ -326,16 +347,14 @@ describe Projects::PipelinesController do format: :json end - context 'when builds are enabled' do - let(:feature) { ProjectFeature::ENABLED } - - it 'retries a pipeline without returning any content' do - expect(response).to have_gitlab_http_status(:no_content) - expect(build.reload).to be_retried - end + it 'retries a pipeline without returning any content' do + expect(response).to have_gitlab_http_status(:no_content) + expect(build.reload).to be_retried end context 'when builds are disabled' do + let(:feature) { ProjectFeature::DISABLED } + it 'fails to retry pipeline' do expect(response).to have_gitlab_http_status(:not_found) end @@ -355,16 +374,14 @@ describe Projects::PipelinesController do format: :json end - context 'when builds are enabled' do - let(:feature) { ProjectFeature::ENABLED } - - it 'cancels a pipeline without returning any content' do - expect(response).to have_gitlab_http_status(:no_content) - expect(pipeline.reload).to be_canceled - end + it 'cancels a pipeline without returning any content' do + expect(response).to have_gitlab_http_status(:no_content) + expect(pipeline.reload).to be_canceled end context 'when builds are disabled' do + let(:feature) { ProjectFeature::DISABLED } + it 'fails to retry pipeline' do expect(response).to have_gitlab_http_status(:not_found) end diff --git a/spec/features/security/project/internal_access_spec.rb b/spec/features/security/project/internal_access_spec.rb index 001e6c10eb2..9ee87563ecf 100644 --- a/spec/features/security/project/internal_access_spec.rb +++ b/spec/features/security/project/internal_access_spec.rb @@ -452,9 +452,9 @@ describe "Internal Project Access" do it { is_expected.to be_allowed_for(:owner).of(project) } it { is_expected.to be_allowed_for(:maintainer).of(project) } it { is_expected.to be_allowed_for(:developer).of(project) } - it { is_expected.to be_allowed_for(:reporter).of(project) } - it { is_expected.to be_allowed_for(:guest).of(project) } - it { is_expected.to be_allowed_for(:user) } + it { is_expected.to be_denied_for(:reporter).of(project) } + it { is_expected.to be_denied_for(:guest).of(project) } + it { is_expected.to be_denied_for(:user) } it { is_expected.to be_denied_for(:external) } it { is_expected.to be_denied_for(:visitor) } end diff --git a/spec/features/security/project/private_access_spec.rb b/spec/features/security/project/private_access_spec.rb index c6618355eea..12613c39307 100644 --- a/spec/features/security/project/private_access_spec.rb +++ b/spec/features/security/project/private_access_spec.rb @@ -485,7 +485,7 @@ describe "Private Project Access" do it { is_expected.to be_allowed_for(:owner).of(project) } it { is_expected.to be_allowed_for(:maintainer).of(project) } it { is_expected.to be_allowed_for(:developer).of(project) } - it { is_expected.to be_allowed_for(:reporter).of(project) } + it { is_expected.to be_denied_for(:reporter).of(project) } it { is_expected.to be_denied_for(:guest).of(project) } it { is_expected.to be_denied_for(:user) } it { is_expected.to be_denied_for(:external) } diff --git a/spec/features/security/project/public_access_spec.rb b/spec/features/security/project/public_access_spec.rb index 3717dc13f1e..57cc0db1f38 100644 --- a/spec/features/security/project/public_access_spec.rb +++ b/spec/features/security/project/public_access_spec.rb @@ -272,11 +272,11 @@ describe "Public Project Access" do it { is_expected.to be_allowed_for(:owner).of(project) } it { is_expected.to be_allowed_for(:maintainer).of(project) } it { is_expected.to be_allowed_for(:developer).of(project) } - it { is_expected.to be_allowed_for(:reporter).of(project) } - it { is_expected.to be_allowed_for(:guest).of(project) } - it { is_expected.to be_allowed_for(:user) } - it { is_expected.to be_allowed_for(:external) } - it { is_expected.to be_allowed_for(:visitor) } + it { is_expected.to be_denied_for(:reporter).of(project) } + it { is_expected.to be_denied_for(:guest).of(project) } + it { is_expected.to be_denied_for(:user) } + it { is_expected.to be_denied_for(:external) } + it { is_expected.to be_denied_for(:visitor) } end describe "GET /:project_path/environments" do diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb index c2dd666f9df..10f61731206 100644 --- a/spec/helpers/projects_helper_spec.rb +++ b/spec/helpers/projects_helper_spec.rb @@ -354,8 +354,20 @@ describe ProjectsHelper do allow(project).to receive(:builds_enabled?).and_return(false) end - it "do not include pipelines tab" do - is_expected.not_to include(:pipelines) + context 'when user has access to builds' do + it "does include pipelines tab" do + is_expected.to include(:pipelines) + end + end + + context 'when user does not have access to builds' do + before do + allow(helper).to receive(:can?) { false } + end + + it "does not include pipelines tab" do + is_expected.not_to include(:pipelines) + end end end diff --git a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb index 9b0da882390..6084dc96410 100644 --- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb @@ -12,7 +12,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do ] RSpec::Mocks.with_temporary_scope do - @project = create(:project, :builds_disabled, :issues_disabled, name: 'project', path: 'project') + @project = create(:project, :builds_enabled, :issues_disabled, name: 'project', path: 'project') @shared = @project.import_export_shared allow(@shared).to receive(:export_path).and_return('spec/lib/gitlab/import_export/') @@ -40,7 +40,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do project = Project.find_by_path('project') expect(project.project_feature.issues_access_level).to eq(ProjectFeature::DISABLED) - expect(project.project_feature.builds_access_level).to eq(ProjectFeature::DISABLED) + expect(project.project_feature.builds_access_level).to eq(ProjectFeature::ENABLED) expect(project.project_feature.snippets_access_level).to eq(ProjectFeature::ENABLED) expect(project.project_feature.wiki_access_level).to eq(ProjectFeature::ENABLED) expect(project.project_feature.merge_requests_access_level).to eq(ProjectFeature::ENABLED) diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index a2d2d77746d..baad8352185 100644 --- a/spec/models/commit_spec.rb +++ b/spec/models/commit_spec.rb @@ -11,6 +11,7 @@ describe Commit do it { is_expected.to include_module(Participable) } it { is_expected.to include_module(Referable) } it { is_expected.to include_module(StaticModel) } + it { is_expected.to include_module(Presentable) } end describe '.lazy' do diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 585dfe46189..8a45f9856bc 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -405,6 +405,30 @@ describe Project do end end + describe '#all_pipelines' do + let(:project) { create(:project) } + + before do + create(:ci_pipeline, project: project, ref: 'master', source: :web) + create(:ci_pipeline, project: project, ref: 'master', source: :external) + end + + it 'has all pipelines' do + expect(project.all_pipelines.size).to eq(2) + end + + context 'when builds are disabled' do + before do + project.project_feature.update_attribute(:builds_access_level, ProjectFeature::DISABLED) + end + + it 'should return .external pipelines' do + expect(project.all_pipelines).to all(have_attributes(source: 'external')) + expect(project.all_pipelines.size).to eq(1) + end + end + end + describe 'project token' do it 'sets an random token if none provided' do project = FactoryBot.create(:project, runners_token: '') diff --git a/spec/policies/ci/pipeline_policy_spec.rb b/spec/policies/ci/pipeline_policy_spec.rb index 8022f61e67d..844d96017de 100644 --- a/spec/policies/ci/pipeline_policy_spec.rb +++ b/spec/policies/ci/pipeline_policy_spec.rb @@ -75,6 +75,14 @@ describe Ci::PipelinePolicy, :models do end end + context 'when user does not have access to internal CI' do + let(:project) { create(:project, :builds_disabled, :public) } + + it 'disallows the user from reading the pipeline' do + expect(policy).to be_disallowed :read_pipeline + end + end + describe 'destroy_pipeline' do let(:project) { create(:project, :public) } diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index 6c854bab5a5..49226a01846 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -175,21 +175,41 @@ describe ProjectPolicy do end context 'builds feature' do - subject { described_class.new(owner, project) } + context 'when builds are disabled' do + subject { described_class.new(owner, project) } - it 'disallows all permissions when the feature is disabled' do - project.project_feature.update(builds_access_level: ProjectFeature::DISABLED) + before do + project.project_feature.update(builds_access_level: ProjectFeature::DISABLED) + end - builds_permissions = [ - :create_pipeline, :update_pipeline, :admin_pipeline, :destroy_pipeline, - :create_build, :read_build, :update_build, :admin_build, :destroy_build, - :create_pipeline_schedule, :read_pipeline_schedule, :update_pipeline_schedule, :admin_pipeline_schedule, :destroy_pipeline_schedule, - :create_environment, :read_environment, :update_environment, :admin_environment, :destroy_environment, - :create_cluster, :read_cluster, :update_cluster, :admin_cluster, - :create_deployment, :read_deployment, :update_deployment, :admin_deployment, :destroy_deployment - ] + it 'disallows all permissions except pipeline when the feature is disabled' do + builds_permissions = [ + :create_build, :read_build, :update_build, :admin_build, :destroy_build, + :create_pipeline_schedule, :read_pipeline_schedule, :update_pipeline_schedule, :admin_pipeline_schedule, :destroy_pipeline_schedule, + :create_environment, :read_environment, :update_environment, :admin_environment, :destroy_environment, + :create_cluster, :read_cluster, :update_cluster, :admin_cluster, :destroy_cluster, + :create_deployment, :read_deployment, :update_deployment, :admin_deployment, :destroy_deployment + ] - expect_disallowed(*builds_permissions) + expect_disallowed(*builds_permissions) + end + end + + context 'when builds are disabled only for some users' do + subject { described_class.new(guest, project) } + + before do + project.project_feature.update(builds_access_level: ProjectFeature::PRIVATE) + end + + it 'disallows pipeline and commit_status permissions' do + builds_permissions = [ + :create_pipeline, :update_pipeline, :admin_pipeline, :destroy_pipeline, + :create_commit_status, :update_commit_status, :admin_commit_status, :destroy_commit_status + ] + + expect_disallowed(*builds_permissions) + end end end diff --git a/spec/presenters/commit_presenter_spec.rb b/spec/presenters/commit_presenter_spec.rb new file mode 100644 index 00000000000..4a0d3a28c32 --- /dev/null +++ b/spec/presenters/commit_presenter_spec.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe CommitPresenter do + let(:project) { create(:project, :repository) } + let(:commit) { project.commit } + let(:user) { create(:user) } + let(:presenter) { described_class.new(commit, current_user: user) } + + describe '#status_for' do + subject { presenter.status_for('ref') } + + context 'when user can read_commit_status' do + before do + allow(presenter).to receive(:can?).with(user, :read_commit_status, project).and_return(true) + end + + it 'returns commit status for ref' do + expect(commit).to receive(:status).with('ref').and_return('test') + + expect(subject).to eq('test') + end + end + + context 'when user can not read_commit_status' do + it 'is false' do + is_expected.to eq(false) + end + end + end + + describe '#any_pipelines?' do + subject { presenter.any_pipelines? } + + context 'when user can read pipeline' do + before do + allow(presenter).to receive(:can?).with(user, :read_pipeline, project).and_return(true) + end + + it 'returns if there are any pipelines for commit' do + expect(commit).to receive_message_chain(:pipelines, :any?).and_return(true) + + expect(subject).to eq(true) + end + end + + context 'when user can not read pipeline' do + it 'is false' do + is_expected.to eq(false) + end + end + end +end diff --git a/spec/serializers/merge_request_widget_entity_spec.rb b/spec/serializers/merge_request_widget_entity_spec.rb index 561421d5ac8..376698a16df 100644 --- a/spec/serializers/merge_request_widget_entity_spec.rb +++ b/spec/serializers/merge_request_widget_entity_spec.rb @@ -31,23 +31,40 @@ describe MergeRequestWidgetEntity do describe 'pipeline' do let(:pipeline) { create(:ci_empty_pipeline, project: project, ref: resource.source_branch, sha: resource.source_branch_sha, head_pipeline_of: resource) } - context 'when is up to date' do - let(:req) { double('request', current_user: user, project: project) } + before do + allow_any_instance_of(MergeRequestPresenter).to receive(:can?).and_call_original + allow_any_instance_of(MergeRequestPresenter).to receive(:can?).with(user, :read_pipeline, anything).and_return(result) + end - it 'returns pipeline' do - pipeline_payload = PipelineDetailsEntity - .represent(pipeline, request: req) - .as_json + context 'when user has access to pipelines' do + let(:result) { true } - expect(subject[:pipeline]).to eq(pipeline_payload) + context 'when is up to date' do + let(:req) { double('request', current_user: user, project: project) } + + it 'returns pipeline' do + pipeline_payload = PipelineDetailsEntity + .represent(pipeline, request: req) + .as_json + + expect(subject[:pipeline]).to eq(pipeline_payload) + end + end + + context 'when is not up to date' do + it 'returns nil' do + pipeline.update(sha: "not up to date") + + expect(subject[:pipeline]).to eq(nil) + end end end - context 'when is not up to date' do - it 'returns nil' do - pipeline.update(sha: "not up to date") + context 'when user does not have access to pipelines' do + let(:result) { false } - expect(subject[:pipeline]).to be_nil + it 'does not have pipeline' do + expect(subject[:pipeline]).to eq(nil) end end end diff --git a/spec/views/projects/commit/_commit_box.html.haml_spec.rb b/spec/views/projects/commit/_commit_box.html.haml_spec.rb index 2fdd28a3be4..1086546c10d 100644 --- a/spec/views/projects/commit/_commit_box.html.haml_spec.rb +++ b/spec/views/projects/commit/_commit_box.html.haml_spec.rb @@ -9,6 +9,7 @@ describe 'projects/commit/_commit_box.html.haml' do assign(:commit, project.commit) allow(view).to receive(:current_user).and_return(user) allow(view).to receive(:can_collaborate_with_project?).and_return(false) + project.add_developer(user) end it 'shows the commit SHA' do @@ -48,7 +49,6 @@ describe 'projects/commit/_commit_box.html.haml' do context 'viewing a commit' do context 'as a developer' do before do - project.add_developer(user) allow(view).to receive(:can_collaborate_with_project?).and_return(true) end @@ -60,6 +60,10 @@ describe 'projects/commit/_commit_box.html.haml' do end context 'as a non-developer' do + before do + project.add_guest(user) + end + it 'does not have a link to create a new tag' do render diff --git a/spec/views/projects/issues/_related_branches.html.haml_spec.rb b/spec/views/projects/issues/_related_branches.html.haml_spec.rb index 8c845251765..5cff7694029 100644 --- a/spec/views/projects/issues/_related_branches.html.haml_spec.rb +++ b/spec/views/projects/issues/_related_branches.html.haml_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' describe 'projects/issues/_related_branches' do include Devise::Test::ControllerHelpers + let(:user) { create(:user) } let(:project) { create(:project, :repository) } let(:branch) { project.repository.find_branch('feature') } let!(:pipeline) { create(:ci_pipeline, project: project, sha: branch.dereferenced_target.id, ref: 'feature') } @@ -11,6 +12,9 @@ describe 'projects/issues/_related_branches' do assign(:project, project) assign(:related_branches, ['feature']) + project.add_developer(user) + allow(view).to receive(:current_user).and_return(user) + render end From b5e10cd3ac4e15e7421ebc9acc5d4f9ca9e8e3ea Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Fri, 25 Jan 2019 17:53:56 +0000 Subject: [PATCH 35/42] Merge branch '56860-fix-spec-race-condition-upside-the-head' into 'master' Fix a JS race in a spec Closes #56860 See merge request gitlab-org/gitlab-ce!24684 --- .../projects/settings/user_changes_default_branch_spec.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/spec/features/projects/settings/user_changes_default_branch_spec.rb b/spec/features/projects/settings/user_changes_default_branch_spec.rb index fcf05e04a5c..7dc18601f50 100644 --- a/spec/features/projects/settings/user_changes_default_branch_spec.rb +++ b/spec/features/projects/settings/user_changes_default_branch_spec.rb @@ -15,6 +15,9 @@ describe 'Projects > Settings > User changes default branch' do let(:project) { create(:project, :repository, namespace: user.namespace) } it 'allows to change the default branch', :js do + # Otherwise, running JS may overwrite our change to project_default_branch + wait_for_requests + select2('fix', from: '#project_default_branch') page.within '#default-branch-settings' do From 13b9cf4f650a23f319a6f366491b50f68856314b Mon Sep 17 00:00:00 2001 From: GitLab Release Tools Bot Date: Mon, 28 Jan 2019 21:19:19 +0000 Subject: [PATCH 36/42] Update CHANGELOG.md for 11.7.1 [ci skip] --- CHANGELOG.md | 30 +++++++++++++++++++ .../unreleased/extract-pages-with-rubyzip.yml | 5 ---- .../fix-security-group-user-removal.yml | 5 ---- ...767-verify-lfs-finalize-from-workhorse.yml | 5 ---- .../security-2769-idn-homograph-attack.yml | 5 ---- ...rity-2776-fix-add-reaction-permissions.yml | 5 ---- ...79-fix-email-comment-permissions-check.yml | 5 ---- .../security-2780-disable-git-v2-protocol.yml | 5 ---- ...ity-commit-status-shown-for-guest-user.yml | 5 ---- .../security-contributed-projects.yml | 5 ---- ...urity-do-not-process-mr-ref-for-guests.yml | 5 ---- ...ty-fix-lfs-import-project-ssrf-forgery.yml | 5 ---- .../security-fix-new-issues-login-message.yml | 5 ---- .../unreleased/security-fix-regex-dos.yml | 5 ---- .../security-fix-user-email-tag-push-leak.yml | 5 ---- ...cess-rights-with-external-wiki-enabled.yml | 5 ---- ...-guests-can-see-list-of-merge-requests.yml | 6 ---- .../security-import-path-logging.yml | 5 ---- .../security-import-project-visibility.yml | 5 ---- ...urity-pipeline-trigger-tokens-exposure.yml | 5 ---- .../security-project-move-users.yml | 5 ---- 21 files changed, 30 insertions(+), 101 deletions(-) delete mode 100644 changelogs/unreleased/extract-pages-with-rubyzip.yml delete mode 100644 changelogs/unreleased/fix-security-group-user-removal.yml delete mode 100644 changelogs/unreleased/security-2767-verify-lfs-finalize-from-workhorse.yml delete mode 100644 changelogs/unreleased/security-2769-idn-homograph-attack.yml delete mode 100644 changelogs/unreleased/security-2776-fix-add-reaction-permissions.yml delete mode 100644 changelogs/unreleased/security-2779-fix-email-comment-permissions-check.yml delete mode 100644 changelogs/unreleased/security-2780-disable-git-v2-protocol.yml delete mode 100644 changelogs/unreleased/security-commit-status-shown-for-guest-user.yml delete mode 100644 changelogs/unreleased/security-contributed-projects.yml delete mode 100644 changelogs/unreleased/security-do-not-process-mr-ref-for-guests.yml delete mode 100644 changelogs/unreleased/security-fix-lfs-import-project-ssrf-forgery.yml delete mode 100644 changelogs/unreleased/security-fix-new-issues-login-message.yml delete mode 100644 changelogs/unreleased/security-fix-regex-dos.yml delete mode 100644 changelogs/unreleased/security-fix-user-email-tag-push-leak.yml delete mode 100644 changelogs/unreleased/security-fix-wiki-access-rights-with-external-wiki-enabled.yml delete mode 100644 changelogs/unreleased/security-guests-can-see-list-of-merge-requests.yml delete mode 100644 changelogs/unreleased/security-import-path-logging.yml delete mode 100644 changelogs/unreleased/security-import-project-visibility.yml delete mode 100644 changelogs/unreleased/security-pipeline-trigger-tokens-exposure.yml delete mode 100644 changelogs/unreleased/security-project-move-users.yml diff --git a/CHANGELOG.md b/CHANGELOG.md index c1deab58d38..e84aa126c63 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,36 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 11.7.1 (2019-01-28) + +### Security (24 changes) + +- Make potentially malicious links more visible in the UI and scrub RTLO chars from links. !2770 +- Don't process MR refs for guests in the notes. !2771 +- Sanitize user full name to clean up any URL to prevent mail clients from auto-linking URLs. !2828 +- Fixed XSS content in KaTex links. +- Disallows unauthorized users from accessing the pipelines section. +- Verify that LFS upload requests are genuine. +- Extract GitLab Pages using RubyZip. +- Prevent awarding emojis to notes whose parent is not visible to user. +- Prevent unauthorized replies when discussion is locked or confidential. +- Disable git v2 protocol temporarily. +- Fix showing ci status for guest users when public pipline are not set. +- Fix contributed projects info still visible when user enable private profile. +- Add subresources removal to member destroy service. +- Add more LFS validations to prevent forgery. +- Use common error for unauthenticated users when creating issues. +- Fix slow regex in project reference pattern. +- Fix private user email being visible in push (and tag push) webhooks. +- Fix wiki access rights when external wiki is enabled. +- Group guests are no longer able to see merge requests they don't have access to at group level. +- Fix path disclosure on project import error. +- Restrict project import visibility based on its group. +- Expose CI/CD trigger token only to the trigger owner. +- Notify only users who can access the project on project move. +- Alias GitHub and BitBucket OAuth2 callback URLs. + + ## 11.7.0 (2019-01-22) ### Security (14 changes, 1 of them is from the community) diff --git a/changelogs/unreleased/extract-pages-with-rubyzip.yml b/changelogs/unreleased/extract-pages-with-rubyzip.yml deleted file mode 100644 index 8352e79d3e5..00000000000 --- a/changelogs/unreleased/extract-pages-with-rubyzip.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Extract GitLab Pages using RubyZip -merge_request: -author: -type: security diff --git a/changelogs/unreleased/fix-security-group-user-removal.yml b/changelogs/unreleased/fix-security-group-user-removal.yml deleted file mode 100644 index 09d09a96f84..00000000000 --- a/changelogs/unreleased/fix-security-group-user-removal.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Add subresources removal to member destroy service -merge_request: -author: -type: security diff --git a/changelogs/unreleased/security-2767-verify-lfs-finalize-from-workhorse.yml b/changelogs/unreleased/security-2767-verify-lfs-finalize-from-workhorse.yml deleted file mode 100644 index e79e3263df7..00000000000 --- a/changelogs/unreleased/security-2767-verify-lfs-finalize-from-workhorse.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Verify that LFS upload requests are genuine -merge_request: -author: -type: security diff --git a/changelogs/unreleased/security-2769-idn-homograph-attack.yml b/changelogs/unreleased/security-2769-idn-homograph-attack.yml deleted file mode 100644 index a014b522c96..00000000000 --- a/changelogs/unreleased/security-2769-idn-homograph-attack.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Make potentially malicious links more visible in the UI and scrub RTLO chars from links -merge_request: 2770 -author: -type: security diff --git a/changelogs/unreleased/security-2776-fix-add-reaction-permissions.yml b/changelogs/unreleased/security-2776-fix-add-reaction-permissions.yml deleted file mode 100644 index 3ad92578c44..00000000000 --- a/changelogs/unreleased/security-2776-fix-add-reaction-permissions.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Prevent awarding emojis to notes whose parent is not visible to user -merge_request: -author: -type: security diff --git a/changelogs/unreleased/security-2779-fix-email-comment-permissions-check.yml b/changelogs/unreleased/security-2779-fix-email-comment-permissions-check.yml deleted file mode 100644 index 2f76064d8a4..00000000000 --- a/changelogs/unreleased/security-2779-fix-email-comment-permissions-check.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Prevent unauthorized replies when discussion is locked or confidential -merge_request: -author: -type: security diff --git a/changelogs/unreleased/security-2780-disable-git-v2-protocol.yml b/changelogs/unreleased/security-2780-disable-git-v2-protocol.yml deleted file mode 100644 index 30a08a98e83..00000000000 --- a/changelogs/unreleased/security-2780-disable-git-v2-protocol.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Disable git v2 protocol temporarily -merge_request: -author: -type: security diff --git a/changelogs/unreleased/security-commit-status-shown-for-guest-user.yml b/changelogs/unreleased/security-commit-status-shown-for-guest-user.yml deleted file mode 100644 index a80170091d0..00000000000 --- a/changelogs/unreleased/security-commit-status-shown-for-guest-user.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Fix showing ci status for guest users when public pipline are not set -merge_request: -author: -type: security diff --git a/changelogs/unreleased/security-contributed-projects.yml b/changelogs/unreleased/security-contributed-projects.yml deleted file mode 100644 index f745a2255ca..00000000000 --- a/changelogs/unreleased/security-contributed-projects.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Fix contributed projects info still visible when user enable private profile -merge_request: -author: -type: security diff --git a/changelogs/unreleased/security-do-not-process-mr-ref-for-guests.yml b/changelogs/unreleased/security-do-not-process-mr-ref-for-guests.yml deleted file mode 100644 index 0281dde11e6..00000000000 --- a/changelogs/unreleased/security-do-not-process-mr-ref-for-guests.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Don't process MR refs for guests in the notes -merge_request: 2771 -author: -type: security diff --git a/changelogs/unreleased/security-fix-lfs-import-project-ssrf-forgery.yml b/changelogs/unreleased/security-fix-lfs-import-project-ssrf-forgery.yml deleted file mode 100644 index b6315ec29d8..00000000000 --- a/changelogs/unreleased/security-fix-lfs-import-project-ssrf-forgery.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Add more LFS validations to prevent forgery -merge_request: -author: -type: security diff --git a/changelogs/unreleased/security-fix-new-issues-login-message.yml b/changelogs/unreleased/security-fix-new-issues-login-message.yml deleted file mode 100644 index 9dabf2438c9..00000000000 --- a/changelogs/unreleased/security-fix-new-issues-login-message.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Use common error for unauthenticated users when creating issues -merge_request: -author: -type: security diff --git a/changelogs/unreleased/security-fix-regex-dos.yml b/changelogs/unreleased/security-fix-regex-dos.yml deleted file mode 100644 index b08566d2f15..00000000000 --- a/changelogs/unreleased/security-fix-regex-dos.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Fix slow regex in project reference pattern -merge_request: -author: -type: security diff --git a/changelogs/unreleased/security-fix-user-email-tag-push-leak.yml b/changelogs/unreleased/security-fix-user-email-tag-push-leak.yml deleted file mode 100644 index 915ea7b5216..00000000000 --- a/changelogs/unreleased/security-fix-user-email-tag-push-leak.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Fix private user email being visible in push (and tag push) webhooks -merge_request: -author: -type: security diff --git a/changelogs/unreleased/security-fix-wiki-access-rights-with-external-wiki-enabled.yml b/changelogs/unreleased/security-fix-wiki-access-rights-with-external-wiki-enabled.yml deleted file mode 100644 index d5f20b87a90..00000000000 --- a/changelogs/unreleased/security-fix-wiki-access-rights-with-external-wiki-enabled.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Fix wiki access rights when external wiki is enabled -merge_request: -author: -type: security diff --git a/changelogs/unreleased/security-guests-can-see-list-of-merge-requests.yml b/changelogs/unreleased/security-guests-can-see-list-of-merge-requests.yml deleted file mode 100644 index f5b74011829..00000000000 --- a/changelogs/unreleased/security-guests-can-see-list-of-merge-requests.yml +++ /dev/null @@ -1,6 +0,0 @@ ---- -title: Group guests are no longer able to see merge requests they don't have access - to at group level -merge_request: -author: -type: security diff --git a/changelogs/unreleased/security-import-path-logging.yml b/changelogs/unreleased/security-import-path-logging.yml deleted file mode 100644 index 2ba2d88d82a..00000000000 --- a/changelogs/unreleased/security-import-path-logging.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Fix path disclosure on project import error -merge_request: -author: -type: security diff --git a/changelogs/unreleased/security-import-project-visibility.yml b/changelogs/unreleased/security-import-project-visibility.yml deleted file mode 100644 index 04ae172a9a1..00000000000 --- a/changelogs/unreleased/security-import-project-visibility.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Restrict project import visibility based on its group -merge_request: -author: -type: security diff --git a/changelogs/unreleased/security-pipeline-trigger-tokens-exposure.yml b/changelogs/unreleased/security-pipeline-trigger-tokens-exposure.yml deleted file mode 100644 index 97d743eead1..00000000000 --- a/changelogs/unreleased/security-pipeline-trigger-tokens-exposure.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Expose CI/CD trigger token only to the trigger owner -merge_request: -author: -type: security diff --git a/changelogs/unreleased/security-project-move-users.yml b/changelogs/unreleased/security-project-move-users.yml deleted file mode 100644 index 744df68651f..00000000000 --- a/changelogs/unreleased/security-project-move-users.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Notify only users who can access the project on project move. -merge_request: -author: -type: security From 20d6be4d8c39d6045547c0e7a533258479eebd8d Mon Sep 17 00:00:00 2001 From: GitLab Release Tools Bot Date: Mon, 28 Jan 2019 21:26:14 +0000 Subject: [PATCH 37/42] Update CHANGELOG.md for 11.5.8 [ci skip] --- CHANGELOG.md | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e84aa126c63..98c4c8f9233 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -558,6 +558,33 @@ entry. - Enable Rubocop on lib/gitlab. (gfyoung) +## 11.5.8 (2019-01-28) + +### Security (21 changes) + +- Make potentially malicious links more visible in the UI and scrub RTLO chars from links. !2770 +- Don't process MR refs for guests in the notes. !2771 +- Fixed XSS content in KaTex links. +- Verify that LFS upload requests are genuine. +- Extract GitLab Pages using RubyZip. +- Prevent awarding emojis to notes whose parent is not visible to user. +- Prevent unauthorized replies when discussion is locked or confidential. +- Disable git v2 protocol temporarily. +- Fix showing ci status for guest users when public pipline are not set. +- Fix contributed projects info still visible when user enable private profile. +- Disallows unauthorized users from accessing the pipelines section. +- Add more LFS validations to prevent forgery. +- Use common error for unauthenticated users when creating issues. +- Fix slow regex in project reference pattern. +- Fix private user email being visible in push (and tag push) webhooks. +- Fix wiki access rights when external wiki is enabled. +- Fix path disclosure on project import error. +- Restrict project import visibility based on its group. +- Expose CI/CD trigger token only to the trigger owner. +- Notify only users who can access the project on project move. +- Alias GitHub and BitBucket OAuth2 callback URLs. + + ## 11.5.5 (2018-12-20) ### Security (1 change) From 6fa5fd8515e0f2d5a6341134560021f353d84362 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Tue, 29 Jan 2019 07:49:59 -0800 Subject: [PATCH 38/42] Fix uninitialized constant with GitLab Pages deploy pages:deploy step was failing with the following error: ``` unitialized constant SafeZip::Extract::Zip ``` Since license_finder already pulls in rubyzip, we can make it a required gem. We also use the scope operator to make the reference to Zip::File explicit. --- Gemfile | 2 +- changelogs/unreleased/sh-fix-pages-zip-constant.yml | 5 +++++ lib/safe_zip/extract.rb | 2 +- 3 files changed, 7 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/sh-fix-pages-zip-constant.yml diff --git a/Gemfile b/Gemfile index e632a083acf..f30b7fb3041 100644 --- a/Gemfile +++ b/Gemfile @@ -57,7 +57,7 @@ gem 'u2f', '~> 0.2.1' # GitLab Pages gem 'validates_hostname', '~> 1.0.6' -gem 'rubyzip', '~> 1.2.2', require: false +gem 'rubyzip', '~> 1.2.2' # Browser detection gem 'browser', '~> 2.5' diff --git a/changelogs/unreleased/sh-fix-pages-zip-constant.yml b/changelogs/unreleased/sh-fix-pages-zip-constant.yml new file mode 100644 index 00000000000..fcd8aa45825 --- /dev/null +++ b/changelogs/unreleased/sh-fix-pages-zip-constant.yml @@ -0,0 +1,5 @@ +--- +title: Fix uninitialized constant with GitLab Pages +merge_request: +author: +type: fixed diff --git a/lib/safe_zip/extract.rb b/lib/safe_zip/extract.rb index 3bd4935ef34..679c021c730 100644 --- a/lib/safe_zip/extract.rb +++ b/lib/safe_zip/extract.rb @@ -29,7 +29,7 @@ module SafeZip private def extract_with_ruby_zip(params) - Zip::File.open(archive_path) do |zip_archive| + ::Zip::File.open(archive_path) do |zip_archive| # Extract all files in the following order: # 1. Directories first, # 2. Files next, From aca9ce3eb61b4af37ee47daaf03c5e1e7a06a15d Mon Sep 17 00:00:00 2001 From: GitLab Release Tools Bot Date: Tue, 29 Jan 2019 23:35:05 +0000 Subject: [PATCH 39/42] Update CHANGELOG.md for 11.7.2 [ci skip] --- CHANGELOG.md | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 98c4c8f9233..594a632f0da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,40 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 11.7.2 (2019-01-29) + +### Security (24 changes) + +- Make potentially malicious links more visible in the UI and scrub RTLO chars from links. !2770 +- Don't process MR refs for guests in the notes. !2771 +- Sanitize user full name to clean up any URL to prevent mail clients from auto-linking URLs. !2828 +- Fixed XSS content in KaTex links. +- Disallows unauthorized users from accessing the pipelines section. +- Verify that LFS upload requests are genuine. +- Extract GitLab Pages using RubyZip. +- Prevent awarding emojis to notes whose parent is not visible to user. +- Prevent unauthorized replies when discussion is locked or confidential. +- Disable git v2 protocol temporarily. +- Fix showing ci status for guest users when public pipline are not set. +- Fix contributed projects info still visible when user enable private profile. +- Add subresources removal to member destroy service. +- Add more LFS validations to prevent forgery. +- Use common error for unauthenticated users when creating issues. +- Fix slow regex in project reference pattern. +- Fix private user email being visible in push (and tag push) webhooks. +- Fix wiki access rights when external wiki is enabled. +- Group guests are no longer able to see merge requests they don't have access to at group level. +- Fix path disclosure on project import error. +- Restrict project import visibility based on its group. +- Expose CI/CD trigger token only to the trigger owner. +- Notify only users who can access the project on project move. +- Alias GitHub and BitBucket OAuth2 callback URLs. + +### Fixed (1 change) + +- Fix uninitialized constant with GitLab Pages. + + ## 11.7.1 (2019-01-28) ### Security (24 changes) From ec22c9d2e5f1bbec25db1d87ac2b9bf8c12e8d42 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Wed, 30 Jan 2019 14:16:32 +0000 Subject: [PATCH 40/42] Fix requiring the rubyzip Gem In commit 6fa5fd8515e0f2d5a6341134560021f353d84362 the `require: false` was removed to ensure the Gem was loaded at run time. Unfortunately, the `require` necessary for the rubyzip Gem is "zip" and not "rubyzip". As a result, Bundler would not require the Gem. This meant that we would still run into constant errors when referring to `Zip::File`. --- Gemfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile b/Gemfile index f30b7fb3041..970e3dd871e 100644 --- a/Gemfile +++ b/Gemfile @@ -57,7 +57,7 @@ gem 'u2f', '~> 0.2.1' # GitLab Pages gem 'validates_hostname', '~> 1.0.6' -gem 'rubyzip', '~> 1.2.2' +gem 'rubyzip', '~> 1.2.2', require: 'zip' # Browser detection gem 'browser', '~> 2.5' From 6afa5770f630536d5caf88375c88a4c9df69b8b6 Mon Sep 17 00:00:00 2001 From: GitLab Release Tools Bot Date: Wed, 30 Jan 2019 18:11:05 +0000 Subject: [PATCH 41/42] Update CHANGELOG.md for 11.6.8 [ci skip] --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 594a632f0da..37bff7e50a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -252,6 +252,10 @@ entry. - Update url placeholder for the sentry configuration page. !24338 +## 11.6.8 (2019-01-30) + +- No changes. + ## 11.6.5 (2019-01-17) ### Fixed (5 changes) From 02f36f4f2856130101bb21c4584ee0ff89469079 Mon Sep 17 00:00:00 2001 From: GitLab Release Tools Bot Date: Mon, 4 Feb 2019 20:09:36 +0000 Subject: [PATCH 42/42] Update CHANGELOG.md for 11.6.9 [ci skip] --- CHANGELOG.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 37bff7e50a3..cf76a2c67df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -252,6 +252,13 @@ entry. - Update url placeholder for the sentry configuration page. !24338 +## 11.6.9 (2019-02-04) + +### Security (1 change) + +- Use sanitized user status message for user popover. + + ## 11.6.8 (2019-01-30) - No changes.