Merge branch 'master' of dev.gitlab.org:gitlab/gitlabhq

CHANGELOG.md

@@ -279,6 +279,15 @@ entry.
 - Creates Vue component for artifacts block on job page.
 
 
+## 11.2.5 (2018-10-05)
+
+### Security (3 changes)
+
+- Filter user sensitive data from discussions JSON. !2538
+- Properly filter private references from system notes.
+- Markdown API no longer displays confidential title references unless authorized.
+
+
 ## 11.2.4 (2018-09-26)
 
 ### Security (6 changes)
@@ -558,6 +567,15 @@ entry.
 - Moves help_popover component to a common location.
 
 
+## 11.1.8 (2018-10-05)
+
+### Security (3 changes)
+
+- Filter user sensitive data from discussions JSON. !2539
+- Properly filter private references from system notes.
+- Markdown API no longer displays confidential title references unless authorized.
+
+
 ## 11.1.7 (2018-09-26)
 
 ### Security (6 changes)

app/models/note.rb

@@ -38,10 +38,12 @@ class Note < ActiveRecord::Base
   alias_attribute :last_edited_at, :updated_at
   alias_attribute :last_edited_by, :updated_by
 
-  # Attribute containing rendered and redacted Markdown as generated by
-  # Banzai::ObjectRenderer.
+  # Number of user visible references as generated by Banzai::ObjectRenderer
   attr_accessor :redacted_note_html
 
+  # Total of all references as generated by Banzai::ObjectRenderer
+  attr_accessor :total_reference_count
+
   # An Array containing the number of visible references as generated by
   # Banzai::ObjectRenderer
   attr_accessor :user_visible_reference_count
@@ -288,15 +290,7 @@ class Note < ActiveRecord::Base
   end
 
   def cross_reference_not_visible_for?(user)
-    cross_reference? && !has_referenced_mentionables?(user)
-  end
-
-  def has_referenced_mentionables?(user)
-    if user_visible_reference_count.present?
-      user_visible_reference_count > 0
-    else
-      referenced_mentionables(user).any?
-    end
+    cross_reference? && !all_referenced_mentionables_allowed?(user)
   end
 
   def award_emoji?
@@ -466,9 +460,18 @@ class Note < ActiveRecord::Base
     self.discussion_id ||= discussion_class.discussion_id(self)
   end
 
+  def all_referenced_mentionables_allowed?(user)
+    if user_visible_reference_count.present? && total_reference_count.present?
+      # if they are not equal, then there are private/confidential references as well
+      user_visible_reference_count > 0 && user_visible_reference_count == total_reference_count
+    else
+      referenced_mentionables(user).any?
+    end
+  end
+
   def force_cross_reference_regex_check?
     return unless system?
 
-    SystemNoteMetadata::TYPES_WITH_CROSS_REFERENCES.include?(system_note_metadata&.action)
+    system_note_metadata&.cross_reference_types&.include?(system_note_metadata&.action)
   end
 end

app/models/project_services/hipchat_service.rb

@@ -149,7 +149,7 @@ class HipchatService < Service
 
     context.merge!(options)
 
-    html = Banzai.post_process(Banzai.render(text, context), context)
+    html = Banzai.render_and_post_process(text, context)
     sanitized_html = sanitize(html, tags: HIPCHAT_ALLOWED_TAGS, attributes: %w[href title alt])
 
     sanitized_html.truncate(200, separator: ' ', omission: '...')

app/models/system_note_metadata.rb

@@ -9,6 +9,7 @@ class SystemNoteMetadata < ActiveRecord::Base
   TYPES_WITH_CROSS_REFERENCES = %w[
     commit cross_reference
     close duplicate
+    moved
   ].freeze
 
   ICON_TYPES = %w[
@@ -26,4 +27,8 @@ class SystemNoteMetadata < ActiveRecord::Base
   def icon_types
     ICON_TYPES
   end
+
+  def cross_reference_types
+    TYPES_WITH_CROSS_REFERENCES
+  end
 end

app/serializers/discussion_entity.rb

@@ -27,7 +27,7 @@ class DiscussionEntity < Grape::Entity
 
   expose :resolved?, as: :resolved
   expose :resolved_by_push?, as: :resolved_by_push
-  expose :resolved_by
+  expose :resolved_by, using: NoteUserEntity
   expose :resolved_at
   expose :resolve_path, if: -> (d, _) { d.resolvable? } do |discussion|
     resolve_project_merge_request_discussion_path(discussion.project, discussion.noteable, discussion.id)

changelogs/unreleased/security-bw-confidential-titles-through-markdown-api.yml

@@ -0,0 +1,5 @@
+---
+title: Markdown API no longer displays confidential title references unless authorized
+merge_request:
+author:
+type: security

changelogs/unreleased/security-fix-leaking-private-project-namespace.yml

@@ -0,0 +1,5 @@
+---
+title: Properly filter private references from system notes
+merge_request:
+author:
+type: security

changelogs/unreleased/security-osw-user-info-leak-discussions.yml

@@ -0,0 +1,5 @@
+---
+title: Filter user sensitive data from discussions JSON
+merge_request: 2536
+author:
+type: security

lib/api/markdown.rb

@@ -12,7 +12,8 @@ module API
         detail "This feature was introduced in GitLab 11.0."
       end
       post do
-        context = { only_path: false }
+        context = { only_path: false, current_user: current_user }
+        context[:pipeline] = params[:gfm] ? :full : :plain_markdown
 
         if params[:project]
           project = Project.find_by_full_path(params[:project])
@@ -24,9 +25,7 @@ module API
           context[:skip_project_check] = true
         end
 
-        context[:pipeline] = params[:gfm] ? :full : :plain_markdown
-
-        { html: Banzai.render(params[:text], context) }
+        { html: Banzai.render_and_post_process(params[:text], context) }
       end
     end
   end

lib/banzai.rb

@@ -1,4 +1,11 @@
 module Banzai
+  # if you need to render markdown, then you probably need to post_process as well,
+  # such as removing references that the current user doesn't have
+  # permission to make
+  def self.render_and_post_process(text, context = {})
+    post_process(render(text, context), context)
+  end
+
   def self.render(text, context = {})
     Renderer.render(text, context)
   end

lib/banzai/object_renderer.rb

@@ -38,6 +38,7 @@ module Banzai
         redacted_data = redacted[index]
         object.__send__("redacted_#{attribute}_html=", redacted_data[:document].to_html(save_options).html_safe) # rubocop:disable GitlabSecurity/PublicSend
         object.user_visible_reference_count = redacted_data[:visible_reference_count] if object.respond_to?(:user_visible_reference_count)
+        object.total_reference_count = redacted_data[:total_reference_count] if object.respond_to?(:total_reference_count)
       end
     end
 

lib/banzai/redactor.rb

@@ -37,7 +37,13 @@ module Banzai
 
       all_document_nodes.each do |entry|
         nodes_for_document = entry[:nodes]
-        doc_data = { document: entry[:document], visible_reference_count: nodes_for_document.count }
+
+        doc_data = {
+          document:                entry[:document],
+          total_reference_count:   nodes_for_document.count,
+          visible_reference_count: nodes_for_document.count
+        }
+
         metadata << doc_data
 
         nodes_for_document.each do |node|

spec/fixtures/api/schemas/entities/note_user_entity.json

@@ -0,0 +1,21 @@
+{
+  "type": "object",
+  "required": [
+    "id",
+    "state",
+    "avatar_url",
+    "path",
+    "name",
+    "username"
+  ],
+  "properties": {
+    "id": { "type": "integer" },
+    "state": { "type": "string" },
+    "avatar_url": { "type": "string" },
+    "path": { "type": "string" },
+    "name": { "type": "string" },
+    "username": { "type": "string" },
+    "status_tooltip_html": { "$ref": "../types/nullable_string.json" }
+  },
+  "additionalProperties": false
+}

spec/models/note_spec.rb

@@ -231,33 +231,60 @@ describe Note do
     let(:ext_proj)  { create(:project, :public) }
     let(:ext_issue) { create(:issue, project: ext_proj) }
 
-    let(:note) do
-      create :note,
-        noteable: ext_issue, project: ext_proj,
-        note: "mentioned in issue #{private_issue.to_reference(ext_proj)}",
-        system: true
+    shared_examples "checks references" do
+      it "returns true" do
+        expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_truthy
+      end
+
+      it "returns false" do
+        expect(note.cross_reference_not_visible_for?(private_user)).to be_falsy
+      end
+
+      it "returns false if user visible reference count set" do
+        note.user_visible_reference_count = 1
+        note.total_reference_count = 1
+
+        expect(note).not_to receive(:reference_mentionables)
+        expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_falsy
+      end
+
+      it "returns true if ref count is 0" do
+        note.user_visible_reference_count = 0
+
+        expect(note).not_to receive(:reference_mentionables)
+        expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_truthy
+      end
     end
 
-    it "returns true" do
-      expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_truthy
+    context "when there is one reference in note" do
+      let(:note) do
+        create :note,
+          noteable: ext_issue, project: ext_proj,
+          note: "mentioned in issue #{private_issue.to_reference(ext_proj)}",
+          system: true
+      end
+
+      it_behaves_like "checks references"
     end
 
-    it "returns false" do
-      expect(note.cross_reference_not_visible_for?(private_user)).to be_falsy
-    end
+    context "when there are two references in note" do
+      let(:note) do
+        create :note,
+          noteable: ext_issue, project: ext_proj,
+          note: "mentioned in issue #{private_issue.to_reference(ext_proj)} and " \
+                "public issue #{ext_issue.to_reference(ext_proj)}",
+          system: true
+      end
 
-    it "returns false if user visible reference count set" do
-      note.user_visible_reference_count = 1
+      it_behaves_like "checks references"
 
-      expect(note).not_to receive(:reference_mentionables)
-      expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_falsy
-    end
+      it "returns true if user visible reference count set and there is a private reference" do
+        note.user_visible_reference_count = 1
+        note.total_reference_count = 2
 
-    it "returns true if ref count is 0" do
-      note.user_visible_reference_count = 0
-
-      expect(note).not_to receive(:reference_mentionables)
-      expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_truthy
+        expect(note).not_to receive(:reference_mentionables)
+        expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_truthy
+      end
     end
   end
 
@@ -269,7 +296,7 @@ describe Note do
     end
 
     context 'when the note might contain cross references' do
-      SystemNoteMetadata::TYPES_WITH_CROSS_REFERENCES.each do |type|
+      SystemNoteMetadata.new.cross_reference_types.each do |type|
         let(:note) { create(:note, :system) }
         let!(:metadata) { create(:system_note_metadata, note: note, action: type) }
 

spec/requests/api/markdown_spec.rb

@@ -106,6 +106,52 @@ describe API::Markdown do
                                         .and include("#1</a>")
           end
         end
+
+        context 'with a public project and confidential issue' do
+          let(:public_project)     { create(:project, :public) }
+          let(:confidential_issue) { create(:issue, :confidential, project: public_project, title: 'Confidential title') }
+
+          let(:text)   { ":tada: Hello world! :100: #{confidential_issue.to_reference}" }
+          let(:params) { { text: text, gfm: true, project: public_project.full_path } }
+
+          shared_examples 'user without proper access' do
+            it 'does not render the title or link' do
+              expect(response).to have_http_status(201)
+              expect(json_response["html"]).not_to include('Confidential title')
+              expect(json_response["html"]).not_to include('<a href=')
+              expect(json_response["html"]).to include('Hello world!')
+                                          .and include('data-name="tada"')
+                                          .and include('data-name="100"')
+                                          .and include('#1</p>')
+            end
+          end
+
+          context 'when not logged in' do
+            let(:user) { }
+
+            it_behaves_like 'user without proper access'
+          end
+
+          context 'when logged in as user without access' do
+            let(:user) { create(:user) }
+
+            it_behaves_like 'user without proper access'
+          end
+
+          context 'when logged in as author' do
+            let(:user) { confidential_issue.author }
+
+            it 'renders the title or link' do
+              expect(response).to have_http_status(201)
+              expect(json_response["html"]).to include('Confidential title')
+              expect(json_response["html"]).to include('Hello world!')
+                                          .and include('data-name="tada"')
+                                          .and include('data-name="100"')
+                                          .and include("<a href=\"#{IssuesHelper.url_for_issue(confidential_issue.iid, public_project)}\"")
+                                          .and include("#1</a>")
+            end
+          end
+        end
       end
     end
   end

spec/serializers/discussion_entity_spec.rb

@@ -36,6 +36,13 @@ describe DiscussionEntity do
     )
   end
 
+  it 'resolved_by matches note_user_entity schema' do
+    Notes::ResolveService.new(note.project, user).execute(note)
+
+    expect(subject[:resolved_by].with_indifferent_access)
+      .to match_schema('entities/note_user_entity')
+  end
+
   context 'when is LegacyDiffDiscussion' do
     let(:project) { create(:project) }
     let(:merge_request) { create(:merge_request, source_project: project) }