Merge branch 'security-hide_merge_request_ids_on_emails' into 'master'

Prevent disclosure of merge request id via email

See merge request gitlab/gitlabhq!3313

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
       ""

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
 

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

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
+
+        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 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)}")
-        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
 

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