summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorGitLab Bot <gitlab-bot@gitlab.com>2020-12-04 16:51:40 +0000
committerGitLab Bot <gitlab-bot@gitlab.com>2020-12-04 16:51:40 +0000
commitaefe6486cf0d193067112b90145083d73b96bfef (patch)
tree02dbf7d022069b183f34b63e99eb359d7e001ddb
parent66ebf02c05dc69a65731d61baf28ef3335db2bbf (diff)
downloadgitlab-ce-aefe6486cf0d193067112b90145083d73b96bfef.tar.gz
Add latest changes from gitlab-org/security/gitlab@13-6-stable-ee
-rw-r--r--app/assets/javascripts/behaviors/markdown/render_mermaid.js32
-rw-r--r--app/controllers/explore/projects_controller.rb4
-rw-r--r--app/controllers/search_controller.rb1
-rw-r--r--app/finders/projects_finder.rb4
-rw-r--r--app/graphql/types/user_type.rb6
-rw-r--r--app/presenters/user_presenter.rb14
-rw-r--r--app/views/explore/projects/_projects.html.haml6
-rw-r--r--changelogs/unreleased/security-296-private_profile_exposure.yml5
-rw-r--r--changelogs/unreleased/security-mermaid-rc-13-6.yml5
-rw-r--r--changelogs/unreleased/security-prevent-short-searches-in-explore-projects.yml5
-rw-r--r--changelogs/unreleased/security-search-term-logged.yml5
-rw-r--r--config/application.rb1
-rw-r--r--spec/controllers/search_controller_spec.rb2
-rw-r--r--spec/features/explore/user_explores_projects_spec.rb11
-rw-r--r--spec/features/markdown/mermaid_spec.rb65
-rw-r--r--spec/finders/projects_finder_spec.rb23
-rw-r--r--spec/requests/api/graphql/user_query_spec.rb46
17 files changed, 220 insertions, 15 deletions
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 } }' }