From 829c9c65f9b730b3ecad7d3ba222e3dcd6489b85 Mon Sep 17 00:00:00 2001 From: Brett Walker Date: Wed, 19 Sep 2018 14:58:43 -0500 Subject: [PATCH 1/6] post_process markdown redered by API --- .../project_services/hipchat_service.rb | 2 +- ...nfidential-titles-through-markdown-api.yml | 5 ++ lib/api/markdown.rb | 7 ++- lib/banzai.rb | 7 +++ spec/requests/api/markdown_spec.rb | 46 +++++++++++++++++++ 5 files changed, 62 insertions(+), 5 deletions(-) create mode 100644 changelogs/unreleased/security-bw-confidential-titles-through-markdown-api.yml diff --git a/app/models/project_services/hipchat_service.rb b/app/models/project_services/hipchat_service.rb index 66012f0da99..a69b7b4c4b6 100644 --- a/app/models/project_services/hipchat_service.rb +++ b/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: '...') diff --git a/changelogs/unreleased/security-bw-confidential-titles-through-markdown-api.yml b/changelogs/unreleased/security-bw-confidential-titles-through-markdown-api.yml new file mode 100644 index 00000000000..e0231b7962f --- /dev/null +++ b/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 diff --git a/lib/api/markdown.rb b/lib/api/markdown.rb index 5d55224c1a7..09a8c34c5c0 100644 --- a/lib/api/markdown.rb +++ b/lib/api/markdown.rb @@ -10,7 +10,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]) @@ -22,9 +23,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 diff --git a/lib/banzai.rb b/lib/banzai.rb index 5df98f66f3b..788f29a6c08 100644 --- a/lib/banzai.rb +++ b/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 diff --git a/spec/requests/api/markdown_spec.rb b/spec/requests/api/markdown_spec.rb index a55796cf343..e369c1435f0 100644 --- a/spec/requests/api/markdown_spec.rb +++ b/spec/requests/api/markdown_spec.rb @@ -106,6 +106,52 @@ describe API::Markdown do .and include("#1") 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('') + 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("") + end + end + end end end end From b18345fac49e3b0ef35a22f3d784aef9f6bf9209 Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Mon, 1 Oct 2018 10:53:08 -0300 Subject: [PATCH 2/6] Filter user sensitive data from discussions JSON --- app/serializers/discussion_entity.rb | 2 +- .../schemas/entities/note_user_entity.json | 21 +++++++++++++++++++ spec/serializers/discussion_entity_spec.rb | 7 +++++++ 3 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 spec/fixtures/api/schemas/entities/note_user_entity.json diff --git a/app/serializers/discussion_entity.rb b/app/serializers/discussion_entity.rb index ebe76c9fcda..b6786a0d597 100644 --- a/app/serializers/discussion_entity.rb +++ b/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) diff --git a/spec/fixtures/api/schemas/entities/note_user_entity.json b/spec/fixtures/api/schemas/entities/note_user_entity.json new file mode 100644 index 00000000000..9b838054563 --- /dev/null +++ b/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 +} diff --git a/spec/serializers/discussion_entity_spec.rb b/spec/serializers/discussion_entity_spec.rb index 378540a35b6..0590304e832 100644 --- a/spec/serializers/discussion_entity_spec.rb +++ b/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) } From 96faeb330860d8f6c509947e9f683c337ccdb6f8 Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Mon, 1 Oct 2018 14:33:01 -0300 Subject: [PATCH 3/6] Add changelog --- .../unreleased/security-osw-user-info-leak-discussions.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/security-osw-user-info-leak-discussions.yml diff --git a/changelogs/unreleased/security-osw-user-info-leak-discussions.yml b/changelogs/unreleased/security-osw-user-info-leak-discussions.yml new file mode 100644 index 00000000000..5acbb80fc3d --- /dev/null +++ b/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 From 9ba554c8a053c5c9ad52a4e38956c4b9a6f140f7 Mon Sep 17 00:00:00 2001 From: Brett Walker Date: Thu, 27 Sep 2018 13:58:23 -0500 Subject: [PATCH 4/6] Filter system notes with public and private cross references --- app/models/note.rb | 27 ++++---- app/models/system_note_metadata.rb | 5 ++ ...-fix-leaking-private-project-namespace.yml | 5 ++ lib/banzai/object_renderer.rb | 1 + lib/banzai/redactor.rb | 8 ++- spec/models/note_spec.rb | 69 +++++++++++++------ 6 files changed, 81 insertions(+), 34 deletions(-) create mode 100644 changelogs/unreleased/security-fix-leaking-private-project-namespace.yml diff --git a/app/models/note.rb b/app/models/note.rb index bea02d69b65..1b595ef60b4 100644 --- a/app/models/note.rb +++ b/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 diff --git a/app/models/system_note_metadata.rb b/app/models/system_note_metadata.rb index 6fadbcefa53..d555ebe5322 100644 --- a/app/models/system_note_metadata.rb +++ b/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 diff --git a/changelogs/unreleased/security-fix-leaking-private-project-namespace.yml b/changelogs/unreleased/security-fix-leaking-private-project-namespace.yml new file mode 100644 index 00000000000..589d16c0c35 --- /dev/null +++ b/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 diff --git a/lib/banzai/object_renderer.rb b/lib/banzai/object_renderer.rb index a176f1e261b..7137c1da57d 100644 --- a/lib/banzai/object_renderer.rb +++ b/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 diff --git a/lib/banzai/redactor.rb b/lib/banzai/redactor.rb index 28928d6f376..e77bee78496 100644 --- a/lib/banzai/redactor.rb +++ b/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| diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 947be44c903..1783dd3206b 100644 --- a/spec/models/note_spec.rb +++ b/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) } From 5159d0c7e28c59b0c63f43c9b52fbacfa3a1021e Mon Sep 17 00:00:00 2001 From: GitLab Release Tools Bot Date: Fri, 5 Oct 2018 08:47:56 +0000 Subject: [PATCH 5/6] Update CHANGELOG.md for 11.1.8 [ci skip] --- CHANGELOG.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a02d1bc38b7..33638fdb6a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -558,6 +558,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) From aa53ae8aac79420bdc52c2d1c006bab097ac1719 Mon Sep 17 00:00:00 2001 From: GitLab Release Tools Bot Date: Fri, 5 Oct 2018 09:37:42 +0000 Subject: [PATCH 6/6] Update CHANGELOG.md for 11.2.5 [ci skip] --- CHANGELOG.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 33638fdb6a2..dcc2c01931d 100644 --- a/CHANGELOG.md +++ b/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)