From aefe6486cf0d193067112b90145083d73b96bfef Mon Sep 17 00:00:00 2001
From: GitLab Bot <gitlab-bot@gitlab.com>
Date: Fri, 4 Dec 2020 16:51:40 +0000
Subject: Add latest changes from gitlab-org/security/gitlab@13-6-stable-ee

---
 .../behaviors/markdown/render_mermaid.js           | 32 +++++++++--
 app/controllers/explore/projects_controller.rb     |  4 +-
 app/controllers/search_controller.rb               |  1 -
 app/finders/projects_finder.rb                     |  4 ++
 app/graphql/types/user_type.rb                     |  6 +-
 app/presenters/user_presenter.rb                   | 14 +++++
 app/views/explore/projects/_projects.html.haml     |  6 +-
 .../security-296-private_profile_exposure.yml      |  5 ++
 changelogs/unreleased/security-mermaid-rc-13-6.yml |  5 ++
 ...-prevent-short-searches-in-explore-projects.yml |  5 ++
 .../unreleased/security-search-term-logged.yml     |  5 ++
 config/application.rb                              |  1 +
 spec/controllers/search_controller_spec.rb         |  2 +-
 .../explore/user_explores_projects_spec.rb         | 11 ++++
 spec/features/markdown/mermaid_spec.rb             | 65 ++++++++++++++++++++++
 spec/finders/projects_finder_spec.rb               | 23 ++++++++
 spec/requests/api/graphql/user_query_spec.rb       | 46 ++++++++++++++-
 17 files changed, 220 insertions(+), 15 deletions(-)
 create mode 100644 changelogs/unreleased/security-296-private_profile_exposure.yml
 create mode 100644 changelogs/unreleased/security-mermaid-rc-13-6.yml
 create mode 100644 changelogs/unreleased/security-prevent-short-searches-in-explore-projects.yml
 create mode 100644 changelogs/unreleased/security-search-term-logged.yml

diff --git a/app/assets/javascripts/behaviors/markdown/render_mermaid.js b/app/assets/javascripts/behaviors/markdown/render_mermaid.js
index 233c5f84340..602f156dbf0 100644
--- a/app/assets/javascripts/behaviors/markdown/render_mermaid.js
+++ b/app/assets/javascripts/behaviors/markdown/render_mermaid.js
@@ -18,7 +18,13 @@ import { __, sprintf } from '~/locale';
 //
 
 // This is an arbitrary number; Can be iterated upon when suitable.
-const MAX_CHAR_LIMIT = 5000;
+const MAX_CHAR_LIMIT = 2000;
+// Max # of mermaid blocks that can be rendered in a page.
+const MAX_MERMAID_BLOCK_LIMIT = 50;
+// Keep a map of mermaid blocks we've already rendered.
+const elsProcessingMap = new WeakMap();
+let renderedMermaidBlocks = 0;
+
 let mermaidModule = {};
 
 function importMermaidModule() {
@@ -110,13 +116,22 @@ function renderMermaids($els) {
       let renderedChars = 0;
 
       $els.each((i, el) => {
+        // Skipping all the elements which we've already queued in requestIdleCallback
+        if (elsProcessingMap.has(el)) {
+          return;
+        }
+
         const { source } = fixElementSource(el);
         /**
-         * Restrict the rendering to a certain amount of character to
-         * prevent mermaidjs from hanging up the entire thread and
-         * causing a DoS.
+         * Restrict the rendering to a certain amount of character
+         * and mermaid blocks to prevent mermaidjs from hanging
+         * up the entire thread and causing a DoS.
          */
-        if ((source && source.length > MAX_CHAR_LIMIT) || renderedChars > MAX_CHAR_LIMIT) {
+        if (
+          (source && source.length > MAX_CHAR_LIMIT) ||
+          renderedChars > MAX_CHAR_LIMIT ||
+          renderedMermaidBlocks >= MAX_MERMAID_BLOCK_LIMIT
+        ) {
           const html = `
           <div class="alert gl-alert gl-alert-warning alert-dismissible lazy-render-mermaid-container js-lazy-render-mermaid-container fade show" role="alert">
             <div>
@@ -146,8 +161,13 @@ function renderMermaids($els) {
         }
 
         renderedChars += source.length;
+        renderedMermaidBlocks += 1;
+
+        const requestId = window.requestIdleCallback(() => {
+          renderMermaidEl(el);
+        });
 
-        renderMermaidEl(el);
+        elsProcessingMap.set(el, requestId);
       });
     })
     .catch(err => {
diff --git a/app/controllers/explore/projects_controller.rb b/app/controllers/explore/projects_controller.rb
index 42795e418a4..7a485eebfe3 100644
--- a/app/controllers/explore/projects_controller.rb
+++ b/app/controllers/explore/projects_controller.rb
@@ -8,6 +8,8 @@ class Explore::ProjectsController < Explore::ApplicationController
   include SortingHelper
   include SortingPreference
 
+  MIN_SEARCH_LENGTH = 3
+
   before_action :set_non_archived_param
   before_action :set_sorting
 
@@ -72,7 +74,7 @@ class Explore::ProjectsController < Explore::ApplicationController
   def load_projects
     load_project_counts
 
-    projects = ProjectsFinder.new(current_user: current_user, params: params).execute
+    projects = ProjectsFinder.new(current_user: current_user, params: params.merge(minimum_search_length: MIN_SEARCH_LENGTH)).execute
 
     projects = preload_associations(projects)
     projects = projects.page(params[:page]).without_count
diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb
index 4b21edc98d5..c92b3457640 100644
--- a/app/controllers/search_controller.rb
+++ b/app/controllers/search_controller.rb
@@ -144,7 +144,6 @@ class SearchController < ApplicationController
     payload[:metadata] ||= {}
     payload[:metadata]['meta.search.group_id'] = params[:group_id]
     payload[:metadata]['meta.search.project_id'] = params[:project_id]
-    payload[:metadata]['meta.search.search'] = params[:search]
     payload[:metadata]['meta.search.scope'] = params[:scope]
     payload[:metadata]['meta.search.filters.confidential'] = params[:confidential]
     payload[:metadata]['meta.search.filters.state'] = params[:state]
diff --git a/app/finders/projects_finder.rb b/app/finders/projects_finder.rb
index 14b84d0bfa6..05dc69ebff6 100644
--- a/app/finders/projects_finder.rb
+++ b/app/finders/projects_finder.rb
@@ -18,6 +18,7 @@
 #     personal: boolean
 #     search: string
 #     search_namespaces: boolean
+#     minimum_search_length: int
 #     non_archived: boolean
 #     archived: 'only' or boolean
 #     min_access_level: integer
@@ -182,6 +183,9 @@ class ProjectsFinder < UnionFinder
 
   def by_search(items)
     params[:search] ||= params[:name]
+
+    return items.none if params[:search].present? && params[:minimum_search_length].present? && params[:search].length < params[:minimum_search_length].to_i
+
     items.optionally_search(params[:search], include_namespace: params[:search_namespaces].present?)
   end
 
diff --git a/app/graphql/types/user_type.rb b/app/graphql/types/user_type.rb
index 2bb2284f8b0..783a0d8425a 100644
--- a/app/graphql/types/user_type.rb
+++ b/app/graphql/types/user_type.rb
@@ -30,8 +30,7 @@ module Types
           resolver: Resolvers::TodoResolver,
           description: 'Todos of the user'
     field :group_memberships, Types::GroupMemberType.connection_type, null: true,
-          description: 'Group memberships of the user',
-          method: :group_members
+          description: 'Group memberships of the user'
     field :group_count, GraphQL::INT_TYPE, null: true,
           resolver: Resolvers::Users::GroupCountResolver,
           description: 'Group count for the user',
@@ -39,8 +38,7 @@ module Types
     field :status, Types::UserStatusType, null: true,
            description: 'User status'
     field :project_memberships, Types::ProjectMemberType.connection_type, null: true,
-          description: 'Project memberships of the user',
-          method: :project_members
+          description: 'Project memberships of the user'
     field :starred_projects, Types::ProjectType.connection_type, null: true,
           description: 'Projects starred by the user',
           resolver: Resolvers::UserStarredProjectsResolver
diff --git a/app/presenters/user_presenter.rb b/app/presenters/user_presenter.rb
index f201b36346f..0028e6d9ef0 100644
--- a/app/presenters/user_presenter.rb
+++ b/app/presenters/user_presenter.rb
@@ -2,4 +2,18 @@
 
 class UserPresenter < Gitlab::View::Presenter::Delegated
   presents :user
+
+  def group_memberships
+    should_be_private? ? GroupMember.none : user.group_members
+  end
+
+  def project_memberships
+    should_be_private? ? ProjectMember.none : user.project_members
+  end
+
+  private
+
+  def should_be_private?
+    !can?(current_user, :read_user_profile, user)
+  end
 end
diff --git a/app/views/explore/projects/_projects.html.haml b/app/views/explore/projects/_projects.html.haml
index 4275f76c046..b2154f71082 100644
--- a/app/views/explore/projects/_projects.html.haml
+++ b/app/views/explore/projects/_projects.html.haml
@@ -1 +1,5 @@
-= render 'shared/projects/list', projects: projects, user: current_user, explore_page: true, pipeline_status: Feature.enabled?(:dashboard_pipeline_status, default_enabled: true)
+- if params[:name].present? && params[:name].size < Explore::ProjectsController::MIN_SEARCH_LENGTH
+  .nothing-here-block
+    %h5= _('Enter at least three characters to search')
+- else
+  = render 'shared/projects/list', projects: projects, user: current_user, explore_page: true, pipeline_status: Feature.enabled?(:dashboard_pipeline_status, default_enabled: true)
diff --git a/changelogs/unreleased/security-296-private_profile_exposure.yml b/changelogs/unreleased/security-296-private_profile_exposure.yml
new file mode 100644
index 00000000000..05d98788aed
--- /dev/null
+++ b/changelogs/unreleased/security-296-private_profile_exposure.yml
@@ -0,0 +1,5 @@
+---
+title: Ensure group and project memberships are not leaked via API for users with private profiles
+merge_request:
+author:
+type: security
diff --git a/changelogs/unreleased/security-mermaid-rc-13-6.yml b/changelogs/unreleased/security-mermaid-rc-13-6.yml
new file mode 100644
index 00000000000..10c620de108
--- /dev/null
+++ b/changelogs/unreleased/security-mermaid-rc-13-6.yml
@@ -0,0 +1,5 @@
+---
+title: Fix mermaid resource consumption in GFM fields
+merge_request:
+author:
+type: security
diff --git a/changelogs/unreleased/security-prevent-short-searches-in-explore-projects.yml b/changelogs/unreleased/security-prevent-short-searches-in-explore-projects.yml
new file mode 100644
index 00000000000..672ccc09a33
--- /dev/null
+++ b/changelogs/unreleased/security-prevent-short-searches-in-explore-projects.yml
@@ -0,0 +1,5 @@
+---
+title: Require at least 3 characters when searching for project in the Explore page
+merge_request:
+author:
+type: security
diff --git a/changelogs/unreleased/security-search-term-logged.yml b/changelogs/unreleased/security-search-term-logged.yml
new file mode 100644
index 00000000000..c3e9d1862bd
--- /dev/null
+++ b/changelogs/unreleased/security-search-term-logged.yml
@@ -0,0 +1,5 @@
+---
+title: Filter search parameter to prevent data leaks
+merge_request:
+author:
+type: security
diff --git a/config/application.rb b/config/application.rb
index e8aebec086b..3981ba348ae 100644
--- a/config/application.rb
+++ b/config/application.rb
@@ -137,6 +137,7 @@ module Gitlab
       encrypted_key
       import_url
       elasticsearch_url
+      search
       otp_attempt
       sentry_dsn
       trace
diff --git a/spec/controllers/search_controller_spec.rb b/spec/controllers/search_controller_spec.rb
index afebc6982c1..bbd39fd4c83 100644
--- a/spec/controllers/search_controller_spec.rb
+++ b/spec/controllers/search_controller_spec.rb
@@ -272,7 +272,7 @@ RSpec.describe SearchController do
 
       expect(last_payload[:metadata]['meta.search.group_id']).to eq('123')
       expect(last_payload[:metadata]['meta.search.project_id']).to eq('456')
-      expect(last_payload[:metadata]['meta.search.search']).to eq('hello world')
+      expect(last_payload[:metadata]).not_to have_key('meta.search.search')
       expect(last_payload[:metadata]['meta.search.scope']).to eq('issues')
       expect(last_payload[:metadata]['meta.search.force_search_results']).to eq('true')
       expect(last_payload[:metadata]['meta.search.filters.confidential']).to eq('true')
diff --git a/spec/features/explore/user_explores_projects_spec.rb b/spec/features/explore/user_explores_projects_spec.rb
index bf4d6c946e1..c082ff1fb0c 100644
--- a/spec/features/explore/user_explores_projects_spec.rb
+++ b/spec/features/explore/user_explores_projects_spec.rb
@@ -47,6 +47,14 @@ RSpec.describe 'User explores projects' do
         end
       end
 
+      shared_examples 'minimum search length' do
+        it 'shows a prompt to enter a longer search term', :js do
+          fill_in 'name', with: 'z'
+
+          expect(page).to have_content('Enter at least three characters to search')
+        end
+      end
+
       context 'when viewing public projects' do
         before do
           visit(explore_projects_path)
@@ -54,6 +62,7 @@ RSpec.describe 'User explores projects' do
 
         include_examples 'shows public and internal projects'
         include_examples 'empty search results'
+        include_examples 'minimum search length'
       end
 
       context 'when viewing most starred projects' do
@@ -63,6 +72,7 @@ RSpec.describe 'User explores projects' do
 
         include_examples 'shows public and internal projects'
         include_examples 'empty search results'
+        include_examples 'minimum search length'
       end
 
       context 'when viewing trending projects' do
@@ -76,6 +86,7 @@ RSpec.describe 'User explores projects' do
 
         include_examples 'shows public projects'
         include_examples 'empty search results'
+        include_examples 'minimum search length'
       end
     end
   end
diff --git a/spec/features/markdown/mermaid_spec.rb b/spec/features/markdown/mermaid_spec.rb
index bdb549326fa..58314a49c4b 100644
--- a/spec/features/markdown/mermaid_spec.rb
+++ b/spec/features/markdown/mermaid_spec.rb
@@ -19,6 +19,9 @@ RSpec.describe 'Mermaid rendering', :js do
 
     visit project_issue_path(project, issue)
 
+    wait_for_requests
+    wait_for_mermaid
+
     %w[A B C D].each do |label|
       expect(page).to have_selector('svg text', text: label)
     end
@@ -39,6 +42,7 @@ RSpec.describe 'Mermaid rendering', :js do
     visit project_issue_path(project, issue)
 
     wait_for_requests
+    wait_for_mermaid
 
     expected = '<text style=""><tspan xml:space="preserve" dy="1em" x="1">Line 1</tspan><tspan xml:space="preserve" dy="1em" x="1">Line 2</tspan></text>'
     expect(page.html.scan(expected).count).to be(4)
@@ -65,6 +69,9 @@ RSpec.describe 'Mermaid rendering', :js do
 
     visit project_issue_path(project, issue)
 
+    wait_for_requests
+    wait_for_mermaid
+
     page.within('.description') do
       expect(page).to have_selector('svg')
       expect(page).to have_selector('pre.mermaid')
@@ -92,6 +99,9 @@ RSpec.describe 'Mermaid rendering', :js do
 
     visit project_issue_path(project, issue)
 
+    wait_for_requests
+    wait_for_mermaid
+
     page.within('.description') do
       page.find('summary').click
       svg = page.find('svg.mermaid')
@@ -118,6 +128,9 @@ RSpec.describe 'Mermaid rendering', :js do
 
     visit project_issue_path(project, issue)
 
+    wait_for_requests
+    wait_for_mermaid
+
     expect(page).to have_css('svg.mermaid[style*="max-width"][width="100%"]')
   end
 
@@ -147,6 +160,7 @@ RSpec.describe 'Mermaid rendering', :js do
     end
 
     wait_for_requests
+    wait_for_mermaid
 
     find('.js-lazy-render-mermaid').click
 
@@ -156,4 +170,55 @@ RSpec.describe 'Mermaid rendering', :js do
       expect(page).not_to have_selector('.js-lazy-render-mermaid-container')
     end
   end
+
+  it 'does not render more than 50 mermaid blocks', :js, quarantine: { issue: 'https://gitlab.com/gitlab-org/gitlab/-/issues/234081' } do
+    graph_edges = "A-->B;B-->A;"
+
+    description = <<~MERMAID
+    ```mermaid
+    graph LR
+    #{graph_edges}
+    ```
+    MERMAID
+
+    description *= 51
+
+    project = create(:project, :public)
+
+    issue = create(:issue, project: project, description: description)
+
+    visit project_issue_path(project, issue)
+
+    wait_for_requests
+    wait_for_mermaid
+
+    page.within('.description') do
+      expect(page).to have_selector('svg')
+
+      expect(page).to have_selector('.lazy-alert-shown')
+
+      expect(page).to have_selector('.js-lazy-render-mermaid-container')
+    end
+  end
+end
+
+def wait_for_mermaid
+  run_idle_callback = <<~RUN_IDLE_CALLBACK
+  window.requestIdleCallback(() => {
+    window.__CAPYBARA_IDLE_CALLBACK_EXEC__ = 1;
+  })
+  RUN_IDLE_CALLBACK
+
+  page.evaluate_script(run_idle_callback)
+
+  Timeout.timeout(Capybara.default_max_wait_time) do
+    loop until finished_rendering?
+  end
+end
+
+def finished_rendering?
+  check_idle_callback = <<~CHECK_IDLE_CALLBACK
+    window.__CAPYBARA_IDLE_CALLBACK_EXEC__
+  CHECK_IDLE_CALLBACK
+  page.evaluate_script(check_idle_callback) == 1
 end
diff --git a/spec/finders/projects_finder_spec.rb b/spec/finders/projects_finder_spec.rb
index 2d712bd44ce..57977fb69b4 100644
--- a/spec/finders/projects_finder_spec.rb
+++ b/spec/finders/projects_finder_spec.rb
@@ -161,6 +161,29 @@ RSpec.describe ProjectsFinder, :do_not_mock_admin_mode do
         it { is_expected.to eq([public_project]) }
       end
 
+      describe 'filter by search with minimum search length' do
+        context 'when search term is shorter than minimum length' do
+          let(:params) { { search: 'C', minimum_search_length: 3 } }
+
+          it { is_expected.to be_empty }
+        end
+
+        context 'when search term is longer than minimum length' do
+          let(:project) { create(:project, :public, group: group, name: 'test_project') }
+          let(:params) { { search: 'test', minimum_search_length: 3 } }
+
+          it { is_expected.to eq([project]) }
+        end
+
+        context 'when minimum length is invalid' do
+          let(:params) { { search: 'C', minimum_search_length: 'x' } }
+
+          it 'ignores the minimum length param' do
+            is_expected.to eq([public_project])
+          end
+        end
+      end
+
       describe 'filter by group name' do
         let(:params) { { name: group.name, search_namespaces: true } }
 
diff --git a/spec/requests/api/graphql/user_query_spec.rb b/spec/requests/api/graphql/user_query_spec.rb
index ef313504388..8c45a67cb0f 100644
--- a/spec/requests/api/graphql/user_query_spec.rb
+++ b/spec/requests/api/graphql/user_query_spec.rb
@@ -250,7 +250,7 @@ RSpec.describe 'getting user information' do
 
     context 'the user is private' do
       before do
-        user.update(private_profile: true)
+        user.update!(private_profile: true)
         post_graphql(query, current_user: current_user)
       end
 
@@ -260,6 +260,50 @@ RSpec.describe 'getting user information' do
         it_behaves_like 'a working graphql query'
       end
 
+      context 'we request the groupMemberships' do
+        let_it_be(:membership_a) { create(:group_member, user: user) }
+        let(:group_memberships) { graphql_data_at(:user, :group_memberships, :nodes) }
+        let(:user_fields) { 'groupMemberships { nodes { id } }' }
+
+        it_behaves_like 'a working graphql query'
+
+        it 'cannot be found' do
+          expect(group_memberships).to be_empty
+        end
+
+        context 'the current user is the user' do
+          let(:current_user) { user }
+
+          it 'can be found' do
+            expect(group_memberships).to include(
+              a_hash_including('id' => global_id_of(membership_a))
+            )
+          end
+        end
+      end
+
+      context 'we request the projectMemberships' do
+        let_it_be(:membership_a) { create(:project_member, user: user) }
+        let(:project_memberships) { graphql_data_at(:user, :project_memberships, :nodes) }
+        let(:user_fields) { 'projectMemberships { nodes { id } }' }
+
+        it_behaves_like 'a working graphql query'
+
+        it 'cannot be found' do
+          expect(project_memberships).to be_empty
+        end
+
+        context 'the current user is the user' do
+          let(:current_user) { user }
+
+          it 'can be found' do
+            expect(project_memberships).to include(
+              a_hash_including('id' => global_id_of(membership_a))
+            )
+          end
+        end
+      end
+
       context 'we request the authoredMergeRequests' do
         let(:user_fields) { 'authoredMergeRequests { nodes { id } }' }
 
-- 
cgit v1.2.3