From fb93142488cfb79bac45f184b7945018550bf326 Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Thu, 8 Aug 2019 16:29:45 -0300 Subject: Prevent disclosure of merge request id via email Do not disclosure merge request id via email for unauthorized users when closing issues. --- app/helpers/emails_helper.rb | 4 ++ app/mailers/emails/issues.rb | 2 + .../security-hide_merge_request_ids_on_emails.yml | 5 ++ spec/helpers/emails_helper_spec.rb | 56 +++++++++++++++++----- spec/services/issues/close_service_spec.rb | 40 +++++++++++++--- 5 files changed, 89 insertions(+), 18 deletions(-) create mode 100644 changelogs/unreleased/security-hide_merge_request_ids_on_emails.yml diff --git a/app/helpers/emails_helper.rb b/app/helpers/emails_helper.rb index 36122d3a22a..23596769738 100644 --- a/app/helpers/emails_helper.rb +++ b/app/helpers/emails_helper.rb @@ -90,6 +90,8 @@ module EmailsHelper when MergeRequest merge_request = MergeRequest.find(closed_via[:id]).present + return "" unless Ability.allowed?(@recipient, :read_merge_request, merge_request) + case format when :html merge_request_link = link_to(merge_request.to_reference, merge_request.web_url) @@ -102,6 +104,8 @@ module EmailsHelper # Technically speaking this should be Commit but per # https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/15610#note_163812339 # we can't deserialize Commit without custom serializer for ActiveJob + return "" unless Ability.allowed?(@recipient, :download_code, @project) + _("via %{closed_via}") % { closed_via: closed_via } else "" diff --git a/app/mailers/emails/issues.rb b/app/mailers/emails/issues.rb index f3a3203f7ad..0bba2a8bf24 100644 --- a/app/mailers/emails/issues.rb +++ b/app/mailers/emails/issues.rb @@ -34,6 +34,8 @@ module Emails setup_issue_mail(issue_id, recipient_id, closed_via: closed_via) @updated_by = User.find(updated_by_user_id) + @recipient = User.find(recipient_id) + mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id, reason)) end diff --git a/changelogs/unreleased/security-hide_merge_request_ids_on_emails.yml b/changelogs/unreleased/security-hide_merge_request_ids_on_emails.yml new file mode 100644 index 00000000000..cd8c9590a70 --- /dev/null +++ b/changelogs/unreleased/security-hide_merge_request_ids_on_emails.yml @@ -0,0 +1,5 @@ +--- +title: Prevent disclosure of merge request ID via email +merge_request: +author: +type: security diff --git a/spec/helpers/emails_helper_spec.rb b/spec/helpers/emails_helper_spec.rb index d25f0c6de4a..a14ae2cde4b 100644 --- a/spec/helpers/emails_helper_spec.rb +++ b/spec/helpers/emails_helper_spec.rb @@ -6,30 +6,62 @@ describe EmailsHelper do let(:merge_request) { create(:merge_request) } let(:merge_request_presenter) { merge_request.present } - context "and format is text" do - it "returns plain text" do - expect(closure_reason_text(merge_request, format: :text)).to eq("via merge request #{merge_request.to_reference} (#{merge_request_presenter.web_url})") + context 'when user can read merge request' do + let(:user) { create(:user) } + + before do + merge_request.project.add_developer(user) + self.instance_variable_set(:@recipient, user) + self.instance_variable_set(:@project, merge_request.project) + end + + context "and format is text" do + it "returns plain text" do + expect(helper.closure_reason_text(merge_request, format: :text)).to eq("via merge request #{merge_request.to_reference} (#{merge_request_presenter.web_url})") + end end - end - context "and format is HTML" do - it "returns HTML" do - expect(closure_reason_text(merge_request, format: :html)).to eq("via merge request #{link_to(merge_request.to_reference, merge_request_presenter.web_url)}") + context "and format is HTML" do + it "returns HTML" do + expect(helper.closure_reason_text(merge_request, format: :html)).to eq("via merge request #{link_to(merge_request.to_reference, merge_request_presenter.web_url)}") + end + end + + context "and format is unknown" do + it "returns plain text" do + expect(helper.closure_reason_text(merge_request, format: :text)).to eq("via merge request #{merge_request.to_reference} (#{merge_request_presenter.web_url})") + end end end - context "and format is unknown" do - it "returns plain text" do - expect(closure_reason_text(merge_request, format: :text)).to eq("via merge request #{merge_request.to_reference} (#{merge_request_presenter.web_url})") + context 'when user cannot read merge request' do + it "does not have link to merge request" do + expect(helper.closure_reason_text(merge_request)).to be_empty end end end context 'when given a String' do + let(:user) { create(:user) } + let(:project) { create(:project) } let(:closed_via) { "5a0eb6fd7e0f133044378c662fcbbc0d0c16dbfa" } - it "returns plain text" do - expect(closure_reason_text(closed_via)).to eq("via #{closed_via}") + context 'when user can read commits' do + before do + project.add_developer(user) + self.instance_variable_set(:@recipient, user) + self.instance_variable_set(:@project, project) + end + + it "returns plain text" do + expect(closure_reason_text(closed_via)).to eq("via #{closed_via}") + end + end + + context 'when user cannot read commits' do + it "returns plain text" do + expect(closure_reason_text(closed_via)).to be_empty + end end end diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index 6874a8a0929..642a49d57d5 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -60,35 +60,63 @@ describe Issues::CloseService do describe '#close_issue' do context "closed by a merge request" do - before do + it 'mentions closure via a merge request' do perform_enqueued_jobs do described_class.new(project, user).close_issue(issue, closed_via: closing_merge_request) end - end - it 'mentions closure via a merge request' do email = ActionMailer::Base.deliveries.last expect(email.to.first).to eq(user2.email) expect(email.subject).to include(issue.title) expect(email.body.parts.map(&:body)).to all(include(closing_merge_request.to_reference)) end + + context 'when user cannot read merge request' do + it 'does not mention merge request' do + project.project_feature.update_attribute(:repository_access_level, ProjectFeature::DISABLED) + perform_enqueued_jobs do + described_class.new(project, user).close_issue(issue, closed_via: closing_merge_request) + end + + email = ActionMailer::Base.deliveries.last + body_text = email.body.parts.map(&:body).join(" ") + + expect(email.to.first).to eq(user2.email) + expect(email.subject).to include(issue.title) + expect(body_text).not_to include(closing_merge_request.to_reference) + end + end end context "closed by a commit" do - before do + it 'mentions closure via a commit' do perform_enqueued_jobs do described_class.new(project, user).close_issue(issue, closed_via: closing_commit) end - end - it 'mentions closure via a commit' do email = ActionMailer::Base.deliveries.last expect(email.to.first).to eq(user2.email) expect(email.subject).to include(issue.title) expect(email.body.parts.map(&:body)).to all(include(closing_commit.id)) end + + context 'when user cannot read the commit' do + it 'does not mention the commit id' do + project.project_feature.update_attribute(:repository_access_level, ProjectFeature::DISABLED) + perform_enqueued_jobs do + described_class.new(project, user).close_issue(issue, closed_via: closing_commit) + end + + email = ActionMailer::Base.deliveries.last + body_text = email.body.parts.map(&:body).join(" ") + + expect(email.to.first).to eq(user2.email) + expect(email.subject).to include(issue.title) + expect(body_text).not_to include(closing_commit.id) + end + end end context "valid params" do -- cgit v1.2.1