From dd45a03c04b9cc91cd761bf2e94644ee92f2a8f6 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 29 Mar 2023 23:53:47 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@15-10-stable-ee --- .../javascripts/pages/projects/blob/show/index.js | 7 +-- app/assets/javascripts/repository/index.js | 7 +-- .../repository/utils/ref_switcher_utils.js | 27 ++++++---- app/controllers/concerns/confirm_email_warning.rb | 14 +++-- app/controllers/projects/blob_controller.rb | 10 ++++ app/controllers/projects/tree_controller.rb | 9 ++++ app/controllers/projects_controller.rb | 10 +++- app/models/hooks/web_hook.rb | 18 +++++-- app/policies/project_policy.rb | 1 - app/views/projects/blob/_breadcrumb.html.haml | 2 +- app/views/projects/tree/_tree_header.html.haml | 2 +- .../output_example_snapshots/snapshot_spec.html | 16 +++--- lib/api/repositories.rb | 4 ++ lib/extracts_ref.rb | 14 ++++- lib/gitlab/unicode.rb | 6 +++ lib/rouge/formatters/html_gitlab.rb | 9 +++- locale/gitlab.pot | 3 ++ spec/controllers/admin/hooks_controller_spec.rb | 3 +- .../concerns/confirm_email_warning_spec.rb | 34 +++++++++++- spec/controllers/projects/blob_controller_spec.rb | 33 ++++++++++-- .../projects/clusters_controller_spec.rb | 23 +++++++- .../environments/prometheus_api_controller_spec.rb | 23 +++++++- spec/controllers/projects/tree_controller_spec.rb | 34 ++++++++++-- spec/controllers/projects_controller_spec.rb | 63 ++++++++++++++++++++++ spec/factories/project_hooks.rb | 2 +- spec/features/admin/users/user_spec.rb | 30 +++++++++++ .../repository/utils/ref_switcher_utils_spec.js | 29 ++++++++-- spec/helpers/hooks_helper_spec.rb | 2 +- spec/lib/rouge/formatters/html_gitlab_spec.rb | 21 +++++++- spec/models/hooks/web_hook_spec.rb | 28 ++++++++++ spec/policies/project_policy_spec.rb | 46 ++++++++++++++-- spec/requests/api/repositories_spec.rb | 16 ++++++ spec/services/web_hook_service_spec.rb | 4 +- 33 files changed, 490 insertions(+), 60 deletions(-) diff --git a/app/assets/javascripts/pages/projects/blob/show/index.js b/app/assets/javascripts/pages/projects/blob/show/index.js index a0f391c912b..02fcc6ea940 100644 --- a/app/assets/javascripts/pages/projects/blob/show/index.js +++ b/app/assets/javascripts/pages/projects/blob/show/index.js @@ -15,7 +15,7 @@ import '~/sourcegraph/load'; import createStore from '~/code_navigation/store'; import { generateRefDestinationPath } from '~/repository/utils/ref_switcher_utils'; import RefSelector from '~/ref/components/ref_selector.vue'; -import { visitUrl } from '~/lib/utils/url_utility'; +import { joinPaths, visitUrl } from '~/lib/utils/url_utility'; Vue.use(Vuex); Vue.use(VueApollo); @@ -34,7 +34,7 @@ const initRefSwitcher = () => { if (!refSwitcherEl) return false; - const { projectId, projectRootPath, ref } = refSwitcherEl.dataset; + const { projectId, projectRootPath, ref, refType } = refSwitcherEl.dataset; return new Vue({ el: refSwitcherEl, @@ -42,7 +42,8 @@ const initRefSwitcher = () => { return createElement(RefSelector, { props: { projectId, - value: ref, + value: refType ? joinPaths('refs', refType, ref) : ref, + useSymbolicRefNames: true, }, on: { input(selectedRef) { diff --git a/app/assets/javascripts/repository/index.js b/app/assets/javascripts/repository/index.js index 6cedc606a37..0db9dcb43df 100644 --- a/app/assets/javascripts/repository/index.js +++ b/app/assets/javascripts/repository/index.js @@ -2,7 +2,7 @@ import { GlButton } from '@gitlab/ui'; import Vue from 'vue'; import Vuex from 'vuex'; import { parseBoolean } from '~/lib/utils/common_utils'; -import { escapeFileUrl, visitUrl } from '~/lib/utils/url_utility'; +import { joinPaths, escapeFileUrl, visitUrl } from '~/lib/utils/url_utility'; import { __ } from '~/locale'; import initWebIdeLink from '~/pages/projects/shared/web_ide_link'; import PerformancePlugin from '~/performance/vue_performance_plugin'; @@ -128,7 +128,7 @@ export default function setupVueRepositoryList() { if (!refSwitcherEl) return false; - const { projectId, projectRootPath } = refSwitcherEl.dataset; + const { projectId, projectRootPath, refType } = refSwitcherEl.dataset; return new Vue({ el: refSwitcherEl, @@ -136,7 +136,8 @@ export default function setupVueRepositoryList() { return createElement(RefSelector, { props: { projectId, - value: ref, + value: refType ? joinPaths('refs', refType, ref) : ref, + useSymbolicRefNames: true, }, on: { input(selectedRef) { diff --git a/app/assets/javascripts/repository/utils/ref_switcher_utils.js b/app/assets/javascripts/repository/utils/ref_switcher_utils.js index c62f7f709c4..bcad4a2c822 100644 --- a/app/assets/javascripts/repository/utils/ref_switcher_utils.js +++ b/app/assets/javascripts/repository/utils/ref_switcher_utils.js @@ -16,22 +16,29 @@ const getNamespaceTargetRegex = (ref) => new RegExp(`(/-/(blob|tree))/${ref}/(.* * @param {string} selectedRef - The selected ref from the ref dropdown. */ export function generateRefDestinationPath(projectRootPath, ref, selectedRef) { - const currentPath = window.location.pathname; - const encodedHash = '%23'; + const url = new URL(window.location.href); + const currentPath = url.pathname; + let refType = null; let namespace = '/-/tree'; let target; + let actualRef = selectedRef; + + const matches = selectedRef.match(/^refs\/(heads|tags)\/(.+)/); + if (matches) { + [, refType, actualRef] = matches; + } + if (refType) { + url.searchParams.set('ref_type', refType); + } else { + url.searchParams.delete('ref_type'); + } + const NAMESPACE_TARGET_REGEX = getNamespaceTargetRegex(ref); const match = NAMESPACE_TARGET_REGEX.exec(currentPath); if (match) { [, namespace, , target] = match; } + url.pathname = joinPaths(projectRootPath, namespace, actualRef, target); - const destinationPath = joinPaths( - projectRootPath, - namespace, - encodeURI(selectedRef).replace(/#/g, encodedHash), - target, - ); - - return `${destinationPath}${window.location.hash}`; + return url.toString(); } diff --git a/app/controllers/concerns/confirm_email_warning.rb b/app/controllers/concerns/confirm_email_warning.rb index 8b7371cbc17..2efea461a35 100644 --- a/app/controllers/concerns/confirm_email_warning.rb +++ b/app/controllers/concerns/confirm_email_warning.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true module ConfirmEmailWarning + include Gitlab::Utils::StrongMemoize extend ActiveSupport::Concern included do @@ -17,11 +18,9 @@ module ConfirmEmailWarning return unless current_user return if current_user.confirmed? - email = current_user.unconfirmed_email || current_user.email - flash.now[:warning] = format( confirm_warning_message, - email: email, + email: email_to_display, resend_link: view_context.link_to(_('Resend it'), user_confirmation_path(user: { email: email }), method: :post), update_link: view_context.link_to(_('Update it'), profile_path) ).html_safe @@ -29,7 +28,16 @@ module ConfirmEmailWarning private + def email + current_user.unconfirmed_email || current_user.email + end + strong_memoize_attr :email + def confirm_warning_message _("Please check your email (%{email}) to verify that you own this address and unlock the power of CI/CD. Didn't receive it? %{resend_link}. Wrong email address? %{update_link}.") end + + def email_to_display + html_escape(email) + end end diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb index 3413aeb6f8a..2d0c4a0a6c1 100644 --- a/app/controllers/projects/blob_controller.rb +++ b/app/controllers/projects/blob_controller.rb @@ -31,6 +31,7 @@ class Projects::BlobController < Projects::ApplicationController before_action :authorize_edit_tree!, only: [:new, :create, :update, :destroy] before_action :commit, except: [:new, :create] + before_action :check_for_ambiguous_ref, only: [:show] before_action :blob, except: [:new, :create] before_action :require_branch_head, only: [:edit, :update] before_action :editor_variables, except: [:show, :preview, :diff] @@ -157,6 +158,15 @@ class Projects::BlobController < Projects::ApplicationController end end + def check_for_ambiguous_ref + @ref_type = ref_type + + if @ref_type == ExtractsRef::BRANCH_REF_TYPE && ambiguous_ref?(@project, @ref) + branch = @project.repository.find_branch(@ref) + redirect_to project_blob_path(@project, File.join(branch.target, @path)) + end + end + def commit @commit ||= @repository.commit(@ref) diff --git a/app/controllers/projects/tree_controller.rb b/app/controllers/projects/tree_controller.rb index ba18a2e0dce..a8b54933487 100644 --- a/app/controllers/projects/tree_controller.rb +++ b/app/controllers/projects/tree_controller.rb @@ -28,6 +28,15 @@ class Projects::TreeController < Projects::ApplicationController def show return render_404 unless @commit + @ref_type = ref_type + if @ref_type == BRANCH_REF_TYPE && ambiguous_ref?(@project, @ref) + branch = @project.repository.find_branch(@ref) + if branch + redirect_to project_tree_path(@project, branch.target) + return + end + end + if tree.entries.empty? if @repository.blob_at(@commit.id, @path) redirect_to project_blob_path(@project, File.join(@ref, @path)) diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index f18055f80b7..c12caecdc23 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -173,11 +173,19 @@ class ProjectsController < Projects::ApplicationController flash.now[:alert] = _("Project '%{project_name}' queued for deletion.") % { project_name: @project.name } end + if ambiguous_ref?(@project, @ref) + branch = @project.repository.find_branch(@ref) + + # The files view would render a ref other than the default branch + # This redirect can be removed once the view is fixed + redirect_to(project_tree_path(@project, branch.target), alert: _("The default branch of this project clashes with another ref")) + return + end + respond_to do |format| format.html do @notification_setting = current_user.notification_settings_for(@project) if current_user @project = @project.present(current_user: current_user) - render_landing_page end diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb index 7e55ffe2e5e..25ccdc2b4f1 100644 --- a/app/models/hooks/web_hook.rb +++ b/app/models/hooks/web_hook.rb @@ -36,7 +36,7 @@ class WebHook < ApplicationRecord after_initialize :initialize_url_variables before_validation :reset_token - before_validation :reset_url_variables, unless: ->(hook) { hook.is_a?(ServiceHook) } + before_validation :reset_url_variables, unless: ->(hook) { hook.is_a?(ServiceHook) }, on: :update before_validation :set_branch_filter_nil, if: :branch_filter_strategy_all_branches? validates :push_events_branch_filter, untrusted_regexp: true, if: :branch_filter_strategy_regex? validates :push_events_branch_filter, "web_hooks/wildcard_branch_filter": true, if: :branch_filter_strategy_wildcard? @@ -105,7 +105,7 @@ class WebHook < ApplicationRecord # See app/validators/json_schemas/web_hooks_url_variables.json VARIABLE_REFERENCE_RE = /\{([A-Za-z]+[0-9]*(?:[._-][A-Za-z0-9]+)*)\}/.freeze - def interpolated_url + def interpolated_url(url = self.url, url_variables = self.url_variables) return url unless url.include?('{') vars = url_variables @@ -131,7 +131,19 @@ class WebHook < ApplicationRecord end def reset_url_variables - self.url_variables = {} if url_changed? && !encrypted_url_variables_changed? + interpolated_url_was = interpolated_url(decrypt_url_was, url_variables_were) + + return if url_variables_were.empty? || interpolated_url_was == interpolated_url + + self.url_variables = {} if url_changed? && url_variables_were.to_a.intersection(url_variables.to_a).any? + end + + def decrypt_url_was + self.class.decrypt_url(encrypted_url_was, iv: Base64.decode64(encrypted_url_iv_was)) + end + + def url_variables_were + self.class.decrypt_url_variables(encrypted_url_variables_was, iv: encrypted_url_variables_iv_was) end def initialize_url_variables diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index a955de77309..e2daa8b88a7 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -422,7 +422,6 @@ class ProjectPolicy < BasePolicy end rule { can?(:metrics_dashboard) }.policy do - enable :read_prometheus enable :read_deployment end diff --git a/app/views/projects/blob/_breadcrumb.html.haml b/app/views/projects/blob/_breadcrumb.html.haml index e77367a7b42..79b13dc861a 100644 --- a/app/views/projects/blob/_breadcrumb.html.haml +++ b/app/views/projects/blob/_breadcrumb.html.haml @@ -2,7 +2,7 @@ .nav-block .tree-ref-container .tree-ref-holder - #js-tree-ref-switcher{ data: { project_id: @project.id, project_root_path: project_path(@project), ref: current_ref } } + #js-tree-ref-switcher{ data: { project_id: @project.id, project_root_path: project_path(@project), ref: current_ref, ref_type: @ref_type.to_s } } %ul.breadcrumb.repo-breadcrumb %li.breadcrumb-item diff --git a/app/views/projects/tree/_tree_header.html.haml b/app/views/projects/tree/_tree_header.html.haml index 6cd3c584f2a..d494d9cc36d 100644 --- a/app/views/projects/tree/_tree_header.html.haml +++ b/app/views/projects/tree/_tree_header.html.haml @@ -2,7 +2,7 @@ .tree-ref-container.gl-display-flex.gl-flex-wrap.gl-gap-2.mb-2.mb-md-0 .tree-ref-holder.gl-max-w-26 - #js-tree-ref-switcher{ data: { project_id: @project.id, project_root_path: project_path(@project) } } + #js-tree-ref-switcher{ data: { project_id: @project.id, ref_type: @ref_type.to_s, project_root_path: project_path(@project) } } #js-repo-breadcrumb{ data: breadcrumb_data_attributes } diff --git a/glfm_specification/output_example_snapshots/snapshot_spec.html b/glfm_specification/output_example_snapshots/snapshot_spec.html index 96131037648..e1ef404ed03 100644 --- a/glfm_specification/output_example_snapshots/snapshot_spec.html +++ b/glfm_specification/output_example_snapshots/snapshot_spec.html @@ -7028,7 +7028,7 @@ references and their corresponding code points.

-
<p>  &amp; © Æ Ď
+
<p>  &amp; © Æ Ď
 ¾ ℋ ⅆ
 ∲ ≧̸</p>
@@ -7344,11 +7344,11 @@ stripped in this way:

-
` b `
+
` b `
-
<p><code> b </code></p>
+
<p><code> b </code></p>
@@ -7356,12 +7356,12 @@ stripped in this way:

-
` `
+
` `
 `  `
-
<p><code> </code>
+
<p><code> </code>
 <code>  </code></p>
@@ -7832,11 +7832,11 @@ not part of a [left-flanking delimiter run]:

-
* a *
+
* a *
-
<p>* a *</p>
+
<p>* a *</p>
@@ -9790,7 +9790,7 @@ Other [Unicode whitespace] like non-breaking space doesn't work.

-
[link](/url "title")
+
[link](/url "title")
diff --git a/lib/api/repositories.rb b/lib/api/repositories.rb index 70535496b12..6f8d34ea387 100644 --- a/lib/api/repositories.rb +++ b/lib/api/repositories.rb @@ -203,6 +203,10 @@ module API render_api_error!("Target project id:#{params[:from_project_id]} is not a fork of project id:#{params[:id]}", 400) end + unless can?(current_user, :read_code, target_project) + forbidden!("You don't have access to this fork's parent project") + end + cache_key = compare_cache_key(current_user, user_project, target_project, declared_params) cache_action(cache_key, expires_in: 1.minute) do diff --git a/lib/extracts_ref.rb b/lib/extracts_ref.rb index dba1aad639c..49c9772f760 100644 --- a/lib/extracts_ref.rb +++ b/lib/extracts_ref.rb @@ -5,7 +5,8 @@ # Can be extended for different types of repository object, e.g. Project or Snippet module ExtractsRef InvalidPathError = Class.new(StandardError) - + BRANCH_REF_TYPE = 'heads' + TAG_REF_TYPE = 'tags' # Given a string containing both a Git tree-ish, such as a branch or tag, and # a filesystem path joined by forward slashes, attempts to separate the two. # @@ -91,7 +92,7 @@ module ExtractsRef def ref_type return unless params[:ref_type].present? - params[:ref_type] == 'tags' ? 'tags' : 'heads' + params[:ref_type] == TAG_REF_TYPE ? TAG_REF_TYPE : BRANCH_REF_TYPE end private @@ -154,4 +155,13 @@ module ExtractsRef def repository_container raise NotImplementedError end + + def ambiguous_ref?(project, ref) + return true if project.repository.ambiguous_ref?(ref) + + return false unless ref&.starts_with?('refs/') + + unprefixed_ref = ref.sub(%r{^refs/(heads|tags)/}, '') + project.repository.commit(unprefixed_ref).present? + end end diff --git a/lib/gitlab/unicode.rb b/lib/gitlab/unicode.rb index b49c5647dab..f291ea1b4ee 100644 --- a/lib/gitlab/unicode.rb +++ b/lib/gitlab/unicode.rb @@ -9,6 +9,12 @@ module Gitlab # https://idiosyncratic-ruby.com/41-proper-unicoding.html BIDI_REGEXP = /\p{Bidi Control}/.freeze + # Regular expression for identifying space characters + # + # In web browsers space characters can be confused with simple + # spaces which may be misleading + SPACE_REGEXP = /\p{Space_Separator}/.freeze + class << self # Warning message used to highlight bidi characters in the GUI def bidi_warning diff --git a/lib/rouge/formatters/html_gitlab.rb b/lib/rouge/formatters/html_gitlab.rb index 436739bed12..a7e95a96b8b 100644 --- a/lib/rouge/formatters/html_gitlab.rb +++ b/lib/rouge/formatters/html_gitlab.rb @@ -25,7 +25,10 @@ module Rouge yield %() line.each do |token, value| - yield highlight_unicode_control_characters(span(token, value.chomp! || value)) + value = value.chomp! || value + value = replace_space_characters(value) + + yield highlight_unicode_control_characters(span(token, value)) end yield ellipsis if @ellipsis_indexes.include?(@line_number - 1) && @ellipsis_svg.present? @@ -42,6 +45,10 @@ module Rouge %(#{@ellipsis_svg}) end + def replace_space_characters(text) + text.gsub(Gitlab::Unicode::SPACE_REGEXP, ' ') + end + def highlight_unicode_control_characters(text) text.gsub(Gitlab::Unicode::BIDI_REGEXP) do |char| %(#{char}) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 97b4b0e614e..6da296d5381 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -43539,6 +43539,9 @@ msgstr "" msgid "The default branch for this project has been changed. Please update your bookmarks." msgstr "" +msgid "The default branch of this project clashes with another ref" +msgstr "" + msgid "The dependency list details information about the components used within your project." msgstr "" diff --git a/spec/controllers/admin/hooks_controller_spec.rb b/spec/controllers/admin/hooks_controller_spec.rb index 4101bd7f658..4e68ffdda2a 100644 --- a/spec/controllers/admin/hooks_controller_spec.rb +++ b/spec/controllers/admin/hooks_controller_spec.rb @@ -59,6 +59,7 @@ RSpec.describe Admin::HooksController do enable_ssl_verification: false, url_variables: [ { key: 'token', value: 'some secret value' }, + { key: 'baz', value: 'qux' }, { key: 'foo', value: nil } ] } @@ -71,7 +72,7 @@ RSpec.describe Admin::HooksController do expect(flash[:notice]).to include('was updated') expect(hook).to have_attributes(hook_params.except(:url_variables)) expect(hook).to have_attributes( - url_variables: { 'token' => 'some secret value', 'baz' => 'woo' } + url_variables: { 'token' => 'some secret value', 'baz' => 'qux' } ) end end diff --git a/spec/controllers/concerns/confirm_email_warning_spec.rb b/spec/controllers/concerns/confirm_email_warning_spec.rb index fca99d37000..7cfbd86cdcb 100644 --- a/spec/controllers/concerns/confirm_email_warning_spec.rb +++ b/spec/controllers/concerns/confirm_email_warning_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ConfirmEmailWarning do +RSpec.describe ConfirmEmailWarning, feature_category: :system_access do before do stub_application_setting_enum('email_confirmation_setting', 'soft') end @@ -82,6 +82,38 @@ RSpec.describe ConfirmEmailWarning do it { is_expected.to set_confirm_warning_for(user.email) } end end + + context 'when user is being impersonated' do + let(:impersonator) { create(:admin) } + + before do + allow(controller).to receive(:session).and_return({ impersonator_id: impersonator.id }) + + get :index + end + + it { is_expected.to set_confirm_warning_for(user.email) } + + context 'when impersonated user email has html in their email' do + let(:user) { create(:user, confirmed_at: nil, unconfirmed_email: "malicious@test.com
") } + + it { is_expected.to set_confirm_warning_for("malicious@test.com<form><input/title='<script>alert(document.domain)</script>'>") } + end + end + + context 'when user is not being impersonated' do + before do + get :index + end + + it { is_expected.to set_confirm_warning_for(user.email) } + + context 'when user email has html in their email' do + let(:user) { create(:user, confirmed_at: nil, unconfirmed_email: "malicious@test.com") } + + it { is_expected.to set_confirm_warning_for("malicious@test.com<form><input/title='<script>alert(document.domain)</script>'>") } + end + end end end end diff --git a/spec/controllers/projects/blob_controller_spec.rb b/spec/controllers/projects/blob_controller_spec.rb index c091badd09d..2c05521d997 100644 --- a/spec/controllers/projects/blob_controller_spec.rb +++ b/spec/controllers/projects/blob_controller_spec.rb @@ -2,15 +2,16 @@ require 'spec_helper' -RSpec.describe Projects::BlobController do +RSpec.describe Projects::BlobController, feature_category: :source_code_management do include ProjectForksHelper let(:project) { create(:project, :public, :repository, previous_default_branch: previous_default_branch) } let(:previous_default_branch) { nil } describe "GET show" do - def request - get(:show, params: { namespace_id: project.namespace, project_id: project, id: id }) + let(:params) { { namespace_id: project.namespace, project_id: project, id: id } } + let(:request) do + get(:show, params: params) end render_views @@ -18,10 +19,34 @@ RSpec.describe Projects::BlobController do context 'with file path' do before do expect(::Gitlab::GitalyClient).to receive(:allow_ref_name_caching).and_call_original - + project.repository.add_tag(project.creator, 'ambiguous_ref', RepoHelpers.sample_commit.id) + project.repository.add_branch(project.creator, 'ambiguous_ref', RepoHelpers.another_sample_commit.id) request end + context 'when the ref is ambiguous' do + let(:ref) { 'ambiguous_ref' } + let(:path) { 'README.md' } + let(:id) { "#{ref}/#{path}" } + let(:params) { { namespace_id: project.namespace, project_id: project, id: id, ref_type: ref_type } } + + context 'and explicitly requesting a branch' do + let(:ref_type) { 'heads' } + + it 'redirects to blob#show with sha for the branch' do + expect(response).to redirect_to(project_blob_path(project, "#{RepoHelpers.another_sample_commit.id}/#{path}")) + end + end + + context 'and explicitly requesting a tag' do + let(:ref_type) { 'tags' } + + it 'responds with success' do + expect(response).to be_ok + end + end + end + context "valid branch, valid file" do let(:id) { 'master/README.md' } diff --git a/spec/controllers/projects/clusters_controller_spec.rb b/spec/controllers/projects/clusters_controller_spec.rb index 38f72c769f3..d16e5eea2e9 100644 --- a/spec/controllers/projects/clusters_controller_spec.rb +++ b/spec/controllers/projects/clusters_controller_spec.rb @@ -7,7 +7,7 @@ RSpec.describe Projects::ClustersController, feature_category: :kubernetes_manag include GoogleApi::CloudPlatformHelpers include KubernetesHelpers - let_it_be(:project) { create(:project) } + let_it_be_with_reload(:project) { create(:project) } let(:user) { create(:user) } @@ -140,6 +140,27 @@ RSpec.describe Projects::ClustersController, feature_category: :kubernetes_manag expect(response).to redirect_to(new_user_session_path) end end + + context 'with a public project' do + before do + project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + project.project_feature.update!(metrics_dashboard_access_level: ProjectFeature::ENABLED) + end + + context 'with guest user' do + let(:prometheus_body) { nil } + + before do + project.add_guest(user) + end + + it 'returns 404' do + get :prometheus_proxy, params: prometheus_proxy_params + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end end end diff --git a/spec/controllers/projects/environments/prometheus_api_controller_spec.rb b/spec/controllers/projects/environments/prometheus_api_controller_spec.rb index 68d50cf19f0..6b0c164e432 100644 --- a/spec/controllers/projects/environments/prometheus_api_controller_spec.rb +++ b/spec/controllers/projects/environments/prometheus_api_controller_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe Projects::Environments::PrometheusApiController do let_it_be(:user) { create(:user) } - let_it_be(:project) { create(:project) } + let_it_be_with_reload(:project) { create(:project) } let_it_be(:proxyable) { create(:environment, project: project) } before do @@ -70,6 +70,27 @@ RSpec.describe Projects::Environments::PrometheusApiController do expect(response).to redirect_to(new_user_session_path) end end + + context 'with a public project' do + before do + project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + project.project_feature.update!(metrics_dashboard_access_level: ProjectFeature::ENABLED) + end + + context 'with guest user' do + let(:prometheus_body) { nil } + + before do + project.add_guest(user) + end + + it 'returns 404' do + get :prometheus_proxy, params: prometheus_proxy_params + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end end end end diff --git a/spec/controllers/projects/tree_controller_spec.rb b/spec/controllers/projects/tree_controller_spec.rb index 2b3adc719c1..61998d516e8 100644 --- a/spec/controllers/projects/tree_controller_spec.rb +++ b/spec/controllers/projects/tree_controller_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::TreeController do +RSpec.describe Projects::TreeController, feature_category: :source_code_management do let(:project) { create(:project, :repository, previous_default_branch: previous_default_branch) } let(:previous_default_branch) { nil } let(:user) { create(:user) } @@ -15,15 +15,41 @@ RSpec.describe Projects::TreeController do end describe "GET show" do + let(:params) do + { + namespace_id: project.namespace.to_param, project_id: project, id: id + } + end + # Make sure any errors accessing the tree in our views bubble up to this spec render_views before do expect(::Gitlab::GitalyClient).to receive(:allow_ref_name_caching).and_call_original + project.repository.add_tag(project.creator, 'ambiguous_ref', RepoHelpers.sample_commit.id) + project.repository.add_branch(project.creator, 'ambiguous_ref', RepoHelpers.another_sample_commit.id) + get :show, params: params + end - get :show, params: { - namespace_id: project.namespace.to_param, project_id: project, id: id - } + context 'when the ref is ambiguous' do + let(:id) { 'ambiguous_ref' } + let(:params) { { namespace_id: project.namespace, project_id: project, id: id, ref_type: ref_type } } + + context 'and explicitly requesting a branch' do + let(:ref_type) { 'heads' } + + it 'redirects to blob#show with sha for the branch' do + expect(response).to redirect_to(project_tree_path(project, RepoHelpers.another_sample_commit.id)) + end + end + + context 'and explicitly requesting a tag' do + let(:ref_type) { 'tags' } + + it 'responds with success' do + expect(response).to be_ok + end + end end context "valid branch, no path" do diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index cd6d3990309..5ece9f09e5f 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -163,6 +163,69 @@ RSpec.describe ProjectsController, feature_category: :projects do expect(assigns(:notification_setting).level).to eq("watch") end end + + context 'when there is a tag with the same name as the default branch' do + let_it_be(:tagged_project) { create(:project, :public, :custom_repo, files: ['somefile']) } + let(:tree_with_default_branch) do + branch = tagged_project.repository.find_branch(tagged_project.default_branch) + project_tree_path(tagged_project, branch.target) + end + + before do + tagged_project.repository.create_file( + tagged_project.creator, + 'file_for_tag', + 'content for file', + message: "Automatically created file", + branch_name: 'branch-to-tag' + ) + + tagged_project.repository.add_tag( + tagged_project.creator, + tagged_project.default_branch, # tag name + 'branch-to-tag' # target + ) + end + + it 'redirects to tree view for the default branch' do + get :show, params: { namespace_id: tagged_project.namespace, id: tagged_project } + expect(response).to redirect_to(tree_with_default_branch) + end + end + + context 'when the default branch name can resolve to another ref' do + let!(:project_with_default_branch) do + create(:project, :public, :custom_repo, files: ['somefile']).tap do |p| + p.repository.create_branch("refs/heads/refs/heads/#{other_ref}", 'master') + p.change_head("refs/heads/#{other_ref}") + end.reload + end + + let(:other_ref) { 'branch-name' } + + context 'but there is no other ref' do + it 'responds with ok' do + get :show, params: { namespace_id: project_with_default_branch.namespace, id: project_with_default_branch } + expect(response).to be_ok + end + end + + context 'and that other ref exists' do + let(:tree_with_default_branch) do + branch = project_with_default_branch.repository.find_branch(project_with_default_branch.default_branch) + project_tree_path(project_with_default_branch, branch.target) + end + + before do + project_with_default_branch.repository.create_branch(other_ref, 'master') + end + + it 'redirects to tree view for the default branch' do + get :show, params: { namespace_id: project_with_default_branch.namespace, id: project_with_default_branch } + expect(response).to redirect_to(tree_with_default_branch) + end + end + end end describe "when project repository is disabled" do diff --git a/spec/factories/project_hooks.rb b/spec/factories/project_hooks.rb index a7f562df92d..3e70b897df6 100644 --- a/spec/factories/project_hooks.rb +++ b/spec/factories/project_hooks.rb @@ -7,7 +7,7 @@ FactoryBot.define do project trait :url_variables do - url_variables { { 'abc' => 'supers3cret' } } + url_variables { { 'abc' => 'supers3cret', 'def' => 'foobar' } } end trait :token do diff --git a/spec/features/admin/users/user_spec.rb b/spec/features/admin/users/user_spec.rb index 1552d4e6187..66129617220 100644 --- a/spec/features/admin/users/user_spec.rb +++ b/spec/features/admin/users/user_spec.rb @@ -271,6 +271,36 @@ RSpec.describe 'Admin::Users::User', feature_category: :user_management do icon = first('[data-testid="incognito-icon"]') expect(icon).not_to be nil end + + context 'when viewing the confirm email warning', :js do + let_it_be(:another_user) { create(:user, :unconfirmed) } + + let(:warning_alert) { page.find(:css, '[data-testid="alert-warning"]') } + let(:expected_styling) { { 'pointer-events' => 'none', 'cursor' => 'default' } } + + context 'with an email that does not contain HTML' do + before do + subject + end + + it 'displays the warning alert including the email' do + expect(warning_alert.text).to include("Please check your email (#{another_user.email}) to verify") + end + end + + context 'with an email that contains HTML' do + let(:malicious_email) { "malicious@test.com" } + let(:another_user) { create(:user, confirmed_at: nil, unconfirmed_email: malicious_email) } + + before do + subject + end + + it 'displays the impersonation alert, excludes email, and disables links' do + expect(warning_alert.text).to include("check your email (#{another_user.unconfirmed_email}) to verify") + end + end + end end context 'ending impersonation' do diff --git a/spec/frontend/repository/utils/ref_switcher_utils_spec.js b/spec/frontend/repository/utils/ref_switcher_utils_spec.js index 7f708f13eaa..220dbf17398 100644 --- a/spec/frontend/repository/utils/ref_switcher_utils_spec.js +++ b/spec/frontend/repository/utils/ref_switcher_utils_spec.js @@ -1,5 +1,6 @@ import { generateRefDestinationPath } from '~/repository/utils/ref_switcher_utils'; import setWindowLocation from 'helpers/set_window_location_helper'; +import { TEST_HOST } from 'spec/test_constants'; import { refWithSpecialCharMock, encodedRefWithSpecialCharMock } from '../mock_data'; const projectRootPath = 'root/Project1'; @@ -16,16 +17,38 @@ describe('generateRefDestinationPath', () => { ${`${projectRootPath}/-/blob/${currentRef}/dir1/test.js`} | ${`${projectRootPath}/-/blob/${selectedRef}/dir1/test.js`} ${`${projectRootPath}/-/blob/${currentRef}/dir1/dir2/test.js`} | ${`${projectRootPath}/-/blob/${selectedRef}/dir1/dir2/test.js`} ${`${projectRootPath}/-/blob/${currentRef}/dir1/dir2/test.js#L123`} | ${`${projectRootPath}/-/blob/${selectedRef}/dir1/dir2/test.js#L123`} - `('generates the correct destination path for $currentPath', ({ currentPath, result }) => { + `('generates the correct destination path for $currentPath', ({ currentPath, result }) => { setWindowLocation(currentPath); - expect(generateRefDestinationPath(projectRootPath, currentRef, selectedRef)).toBe(result); + expect(generateRefDestinationPath(projectRootPath, currentRef, selectedRef)).toBe( + `${TEST_HOST}/${result}`, + ); + }); + + describe('when using symbolic ref names', () => { + it.each` + currentPath | nextRef | result + ${`${projectRootPath}/-/blob/${currentRef}/dir1/dir2/test.js#L123`} | ${'someHash'} | ${`${projectRootPath}/-/blob/someHash/dir1/dir2/test.js#L123`} + ${`${projectRootPath}/-/blob/${currentRef}/dir1/dir2/test.js#L123`} | ${'refs/heads/prefixedByUseSymbolicRefNames'} | ${`${projectRootPath}/-/blob/prefixedByUseSymbolicRefNames/dir1/dir2/test.js?ref_type=heads#L123`} + ${`${projectRootPath}/-/blob/${currentRef}/dir1/dir2/test.js#L123`} | ${'refs/tags/prefixedByUseSymbolicRefNames'} | ${`${projectRootPath}/-/blob/prefixedByUseSymbolicRefNames/dir1/dir2/test.js?ref_type=tags#L123`} + ${`${projectRootPath}/-/tree/${currentRef}/dir1/dir2/test.js#L123`} | ${'refs/heads/prefixedByUseSymbolicRefNames'} | ${`${projectRootPath}/-/tree/prefixedByUseSymbolicRefNames/dir1/dir2/test.js?ref_type=heads#L123`} + ${`${projectRootPath}/-/tree/${currentRef}/dir1/dir2/test.js#L123`} | ${'refs/tags/prefixedByUseSymbolicRefNames'} | ${`${projectRootPath}/-/tree/prefixedByUseSymbolicRefNames/dir1/dir2/test.js?ref_type=tags#L123`} + ${`${projectRootPath}/-/tree/${currentRef}/dir1/dir2/test.js#L123`} | ${'refs/heads/refs/heads/branchNameContainsPrefix'} | ${`${projectRootPath}/-/tree/refs/heads/branchNameContainsPrefix/dir1/dir2/test.js?ref_type=heads#L123`} + `( + 'generates the correct destination path for $currentPath with ref type when it can be extracted', + ({ currentPath, result, nextRef }) => { + setWindowLocation(currentPath); + expect(generateRefDestinationPath(projectRootPath, currentRef, nextRef)).toBe( + `${TEST_HOST}/${result}`, + ); + }, + ); }); it('encodes the selected ref', () => { const result = `${projectRootPath}/-/tree/${encodedRefWithSpecialCharMock}`; expect(generateRefDestinationPath(projectRootPath, currentRef, refWithSpecialCharMock)).toBe( - result, + `${TEST_HOST}/${result}`, ); }); }); diff --git a/spec/helpers/hooks_helper_spec.rb b/spec/helpers/hooks_helper_spec.rb index a6cfbfe86ca..d8fa64e099a 100644 --- a/spec/helpers/hooks_helper_spec.rb +++ b/spec/helpers/hooks_helper_spec.rb @@ -26,7 +26,7 @@ RSpec.describe HooksHelper do it 'returns proper data' do expect(subject).to match( url: project_hook.url, - url_variables: Gitlab::Json.dump([{ key: 'abc' }]) + url_variables: Gitlab::Json.dump([{ key: 'abc' }, { key: 'def' }]) ) end end diff --git a/spec/lib/rouge/formatters/html_gitlab_spec.rb b/spec/lib/rouge/formatters/html_gitlab_spec.rb index 79bfdb262c0..6fc1b395fc8 100644 --- a/spec/lib/rouge/formatters/html_gitlab_spec.rb +++ b/spec/lib/rouge/formatters/html_gitlab_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Rouge::Formatters::HTMLGitlab do +RSpec.describe Rouge::Formatters::HTMLGitlab, feature_category: :source_code_management do describe '#format' do subject { described_class.format(tokens, **options) } @@ -67,5 +67,24 @@ RSpec.describe Rouge::Formatters::HTMLGitlab do is_expected.to include(%{}).exactly(4).times end end + + context 'when space characters and zero-width spaces are used' do + let(:lang) { 'ruby' } + let(:tokens) { lexer.lex(code, continue: false) } + + let(:code) do + <<~JS + def\u00a0\u1680\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200a\u202f\u205f\u3000hello + JS + end + + it 'replaces the space characters with spaces' do + is_expected.to eq( + "" \ + "def hello" \ + "" + ) + end + end end end diff --git a/spec/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb index 8fd76d2a835..48dfaff74d8 100644 --- a/spec/models/hooks/web_hook_spec.rb +++ b/spec/models/hooks/web_hook_spec.rb @@ -242,6 +242,22 @@ RSpec.describe WebHook, feature_category: :integrations do expect(hook.url_variables).to eq({}) end + it 'resets url variables if url is changed and url variables are appended' do + hook.url = 'http://suspicious.example.com/{abc}/{foo}' + hook.url_variables = hook.url_variables.merge('foo' => 'bar') + + expect(hook).not_to be_valid + expect(hook.url_variables).to eq({}) + end + + it 'resets url variables if url is changed and url variables are removed' do + hook.url = 'http://suspicious.example.com/{abc}' + hook.url_variables = hook.url_variables.except("def") + + expect(hook).not_to be_valid + expect(hook.url_variables).to eq({}) + end + it 'does not reset url variables if both url and url variables are changed' do hook.url = 'http://example.com/{one}/{two}' hook.url_variables = { 'one' => 'foo', 'two' => 'bar' } @@ -249,6 +265,18 @@ RSpec.describe WebHook, feature_category: :integrations do expect(hook).to be_valid expect(hook.url_variables).to eq({ 'one' => 'foo', 'two' => 'bar' }) end + + context 'without url variables' do + subject(:hook) { build_stubbed(:project_hook, project: project, url: 'http://example.com') } + + it 'does not reset url variables' do + hook.url = 'http://example.com/{one}/{two}' + hook.url_variables = { 'one' => 'foo', 'two' => 'bar' } + + expect(hook).to be_valid + expect(hook.url_variables).to eq({ 'one' => 'foo', 'two' => 'bar' }) + end + end end it "only consider these branch filter strategies are valid" do diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index 383f56b6ef5..50f425f4efe 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -727,6 +727,39 @@ RSpec.describe ProjectPolicy, feature_category: :system_access do end end + describe 'read_prometheus', feature_category: :metrics do + using RSpec::Parameterized::TableSyntax + + before do + project.project_feature.update!(metrics_dashboard_access_level: ProjectFeature::ENABLED) + end + + let(:policy) { :read_prometheus } + + where(:project_visibility, :role, :allowed) do + :public | :anonymous | false + :public | :guest | false + :public | :reporter | true + :internal | :anonymous | false + :internal | :guest | false + :internal | :reporter | true + :private | :anonymous | false + :private | :guest | false + :private | :reporter | true + end + + with_them do + let(:current_user) { public_send(role) } + let(:project) { public_send("#{project_visibility}_project") } + + if params[:allowed] + it { is_expected.to be_allowed(policy) } + else + it { is_expected.not_to be_allowed(policy) } + end + end + end + describe 'update_max_artifacts_size' do context 'when no user' do let(:current_user) { anonymous } @@ -1002,7 +1035,7 @@ RSpec.describe ProjectPolicy, feature_category: :system_access do let(:current_user) { guest } it { is_expected.to be_allowed(:metrics_dashboard) } - it { is_expected.to be_allowed(:read_prometheus) } + it { is_expected.to be_disallowed(:read_prometheus) } it { is_expected.to be_allowed(:read_deployment) } it { is_expected.to be_allowed(:read_metrics_user_starred_dashboard) } it { is_expected.to be_allowed(:create_metrics_user_starred_dashboard) } @@ -1012,7 +1045,7 @@ RSpec.describe ProjectPolicy, feature_category: :system_access do let(:current_user) { anonymous } it { is_expected.to be_allowed(:metrics_dashboard) } - it { is_expected.to be_allowed(:read_prometheus) } + it { is_expected.to be_disallowed(:read_prometheus) } it { is_expected.to be_allowed(:read_deployment) } it { is_expected.to be_disallowed(:read_metrics_user_starred_dashboard) } it { is_expected.to be_disallowed(:create_metrics_user_starred_dashboard) } @@ -1038,12 +1071,14 @@ RSpec.describe ProjectPolicy, feature_category: :system_access do let(:current_user) { guest } it { is_expected.to be_disallowed(:metrics_dashboard) } + it { is_expected.to be_disallowed(:read_prometheus) } end context 'with anonymous' do let(:current_user) { anonymous } it { is_expected.to be_disallowed(:metrics_dashboard) } + it { is_expected.to be_disallowed(:read_prometheus) } end end @@ -1066,7 +1101,7 @@ RSpec.describe ProjectPolicy, feature_category: :system_access do let(:current_user) { guest } it { is_expected.to be_allowed(:metrics_dashboard) } - it { is_expected.to be_allowed(:read_prometheus) } + it { is_expected.to be_disallowed(:read_prometheus) } it { is_expected.to be_allowed(:read_deployment) } it { is_expected.to be_allowed(:read_metrics_user_starred_dashboard) } it { is_expected.to be_allowed(:create_metrics_user_starred_dashboard) } @@ -1076,6 +1111,7 @@ RSpec.describe ProjectPolicy, feature_category: :system_access do let(:current_user) { anonymous } it { is_expected.to be_disallowed(:metrics_dashboard) } + it { is_expected.to be_disallowed(:read_prometheus) } end end end @@ -1098,12 +1134,14 @@ RSpec.describe ProjectPolicy, feature_category: :system_access do let(:current_user) { guest } it { is_expected.to be_disallowed(:metrics_dashboard) } + it { is_expected.to be_disallowed(:read_prometheus) } end context 'with anonymous' do let(:current_user) { anonymous } it { is_expected.to be_disallowed(:metrics_dashboard) } + it { is_expected.to be_disallowed(:read_prometheus) } end end @@ -1122,12 +1160,14 @@ RSpec.describe ProjectPolicy, feature_category: :system_access do let(:current_user) { guest } it { is_expected.to be_disallowed(:metrics_dashboard) } + it { is_expected.to be_disallowed(:read_prometheus) } end context 'with anonymous' do let(:current_user) { anonymous } it { is_expected.to be_disallowed(:metrics_dashboard) } + it { is_expected.to be_disallowed(:read_prometheus) } end end end diff --git a/spec/requests/api/repositories_spec.rb b/spec/requests/api/repositories_spec.rb index b146dda5030..8853eff0b3e 100644 --- a/spec/requests/api/repositories_spec.rb +++ b/spec/requests/api/repositories_spec.rb @@ -572,6 +572,22 @@ RSpec.describe API::Repositories, feature_category: :source_code_management do context 'when authenticated', 'as a developer' do it_behaves_like 'repository compare' do let(:current_user) { user } + + context 'when user does not have read access to the parent project' do + let_it_be(:group) { create(:group) } + let(:forked_project) { fork_project(project, current_user, repository: true, namespace: group) } + + before do + forked_project.add_guest(current_user) + end + + it 'returns 403 error' do + get api(route, current_user), params: { from: 'improve/awesome', to: 'feature', from_project_id: forked_project.id } + + expect(response).to have_gitlab_http_status(:forbidden) + expect(json_response['message']).to eq("403 Forbidden - You don't have access to this fork's parent project") + end + end end end diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 5736bf885be..b4250fcf04d 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -130,8 +130,8 @@ RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state, context 'there is userinfo' do before do project_hook.update!( - url: 'http://{one}:{two}@example.com', - url_variables: { 'one' => 'a', 'two' => 'b' } + url: 'http://{foo}:{bar}@example.com', + url_variables: { 'foo' => 'a', 'bar' => 'b' } ) stub_full_request('http://example.com', method: :post) end -- cgit v1.2.1