Merge remote-tracking branch 'dev/14-4-stable' into 14-4-stable

21 files changed, 128 insertions, 23 deletions

diff --git a/app/assets/javascripts/lib/dompurify.js b/app/assets/javascripts/lib/dompurify.js
index d421d66981e..47ede8cb1bb 100644
--- a/app/assets/javascripts/lib/dompurify.js
+++ b/app/assets/javascripts/lib/dompurify.js
@@ -1,5 +1,5 @@
 import { sanitize as dompurifySanitize, addHook } from 'dompurify';
-import { getBaseURL, relativePathToAbsolute } from '~/lib/utils/url_utility';
+import { getNormalizedURL, getBaseURL, relativePathToAbsolute } from '~/lib/utils/url_utility';
 
 const defaultConfig = {
   // Safely allow SVG <use> tags
@@ -11,12 +11,14 @@ const defaultConfig = {
 
 // Only icons urls from `gon` are allowed
 const getAllowedIconUrls = (gon = window.gon) =>
-  [gon.sprite_file_icons, gon.sprite_icons].filter(Boolean);
+  [gon.sprite_file_icons, gon.sprite_icons]
+    .filter(Boolean)
+    .map((path) => relativePathToAbsolute(path, getBaseURL()));
 
-const isUrlAllowed = (url) => getAllowedIconUrls().some((allowedUrl) => url.startsWith(allowedUrl));
+const isUrlAllowed = (url) =>
+  getAllowedIconUrls().some((allowedUrl) => getNormalizedURL(url).startsWith(allowedUrl));
 
-const isHrefSafe = (url) =>
-  isUrlAllowed(url) || isUrlAllowed(relativePathToAbsolute(url, getBaseURL())) || url.match(/^#/);
+const isHrefSafe = (url) => url.match(/^#/) || isUrlAllowed(url);
 
 const removeUnsafeHref = (node, attr) => {
   if (!node.hasAttribute(attr)) {
@@ -36,13 +38,14 @@ const removeUnsafeHref = (node, attr) => {
  *   <use href="/assets/icons-xxx.svg#icon_name"></use>
  * </svg>
  *
+ * It validates both href & xlink:href attributes.
+ * Note that `xlink:href` is deprecated, but still in use
+ * https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/xlink:href
+ *
  * @param {Object} node - Node to sanitize
  */
 const sanitizeSvgIcon = (node) => {
   removeUnsafeHref(node, 'href');
-
-  // Note: `xlink:href` is deprecated, but still in use
-  // https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/xlink:href
   removeUnsafeHref(node, 'xlink:href');
 };
 
diff --git a/app/assets/javascripts/lib/utils/url_utility.js b/app/assets/javascripts/lib/utils/url_utility.js
index 1c22d21a313..c70d23d06ec 100644
--- a/app/assets/javascripts/lib/utils/url_utility.js
+++ b/app/assets/javascripts/lib/utils/url_utility.js
@@ -399,6 +399,24 @@ export function isSafeURL(url) {
   }
 }
 
+/**
+ * Returns a normalized url
+ *
+ * https://gitlab.com/foo/../baz => https://gitlab.com/baz
+ *
+ * @param {String} url - URL to be transformed
+ * @param {String?} baseUrl - current base URL
+ * @returns {String}
+ */
+export const getNormalizedURL = (url, baseUrl) => {
+  const base = baseUrl || getBaseURL();
+  try {
+    return new URL(url, base).href;
+  } catch (e) {
+    return '';
+  }
+};
+
 export function getWebSocketProtocol() {
   return window.location.protocol.replace('http', 'ws');
 }
diff --git a/app/assets/javascripts/pages/projects/shared/permissions/components/settings_panel.vue b/app/assets/javascripts/pages/projects/shared/permissions/components/settings_panel.vue
index 261f7af7ef1..c53d367ed71 100644
--- a/app/assets/javascripts/pages/projects/shared/permissions/components/settings_panel.vue
+++ b/app/assets/javascripts/pages/projects/shared/permissions/components/settings_panel.vue
@@ -37,6 +37,10 @@ export default {
     securityAndComplianceLabel: s__('ProjectSettings|Security & Compliance'),
     snippetsLabel: s__('ProjectSettings|Snippets'),
     wikiLabel: s__('ProjectSettings|Wiki'),
+    pucWarningLabel: s__('ProjectSettings|Warn about Potentially Unwanted Characters'),
+    pucWarningHelpText: s__(
+      'ProjectSettings|Highlight the usage of hidden unicode characters. These have innocent uses for right-to-left languages, but can also be used in potential exploits.',
+    ),
   },
 
   components: {
@@ -178,6 +182,7 @@ export default {
       securityAndComplianceAccessLevel: featureAccessLevel.PROJECT_MEMBERS,
       operationsAccessLevel: featureAccessLevel.EVERYONE,
       containerRegistryAccessLevel: featureAccessLevel.EVERYONE,
+      warnAboutPotentiallyUnwantedCharacters: true,
       lfsEnabled: true,
       requestAccessEnabled: true,
       highlightChangesClass: false,
@@ -752,5 +757,19 @@ export default {
         }}</template>
       </gl-form-checkbox>
     </project-setting-row>
+    <project-setting-row class="gl-mb-5">
+      <input
+        :value="warnAboutPotentiallyUnwantedCharacters"
+        type="hidden"
+        name="project[project_setting_attributes][warn_about_potentially_unwanted_characters]"
+      />
+      <gl-form-checkbox
+        v-model="warnAboutPotentiallyUnwantedCharacters"
+        name="project[project_setting_attributes][warn_about_potentially_unwanted_characters]"
+      >
+        {{ $options.i18n.pucWarningLabel }}
+        <template #help>{{ $options.i18n.pucWarningHelpText }}</template>
+      </gl-form-checkbox>
+    </project-setting-row>
   </div>
 </template>
diff --git a/app/assets/javascripts/snippets/components/show.vue b/app/assets/javascripts/snippets/components/show.vue
index 46629a569ec..35d88d5ec8e 100644
--- a/app/assets/javascripts/snippets/components/show.vue
+++ b/app/assets/javascripts/snippets/components/show.vue
@@ -66,7 +66,13 @@ export default {
           data-qa-selector="clone_button"
         />
       </div>
-      <snippet-blob v-for="blob in blobs" :key="blob.path" :snippet="snippet" :blob="blob" />
+      <snippet-blob
+        v-for="blob in blobs"
+        :key="blob.path"
+        :snippet="snippet"
+        :blob="blob"
+        class="project-highlight-puc"
+      />
     </template>
   </div>
 </template>
diff --git a/app/assets/stylesheets/framework/highlight.scss b/app/assets/stylesheets/framework/highlight.scss
index b4a1d9f9977..122c605e603 100644
--- a/app/assets/stylesheets/framework/highlight.scss
+++ b/app/assets/stylesheets/framework/highlight.scss
@@ -85,3 +85,9 @@
 td.line-numbers {
   line-height: 1;
 }
+
+.project-highlight-puc .unicode-bidi::before {
+  content: '�';
+  cursor: pointer;
+  text-decoration: underline wavy $red-500;
+}
diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index 26da0436dd8..0760f97d7c1 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -409,6 +409,7 @@ class ProjectsController < Projects::ApplicationController
       show_default_award_emojis
       squash_option
       mr_default_target_self
+      warn_about_potentially_unwanted_characters
     ]
   end
 
diff --git a/app/graphql/mutations/issues/set_severity.rb b/app/graphql/mutations/issues/set_severity.rb
index 778563ba053..872a0e7b33d 100644
--- a/app/graphql/mutations/issues/set_severity.rb
+++ b/app/graphql/mutations/issues/set_severity.rb
@@ -8,6 +8,8 @@ module Mutations
       argument :severity, Types::IssuableSeverityEnum, required: true,
                description: 'Set the incident severity level.'
 
+      authorize :admin_issue
+
       def resolve(project_path:, iid:, severity:)
         issue = authorized_find!(project_path: project_path, iid: iid)
         project = issue.project
diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb
index 03e7fb5ffc4..e3b63d122d2 100644
--- a/app/helpers/projects_helper.rb
+++ b/app/helpers/projects_helper.rb
@@ -376,6 +376,12 @@ module ProjectsHelper
     }
   end
 
+  def project_classes(project)
+    return "project-highlight-puc" if project.warn_about_potentially_unwanted_characters?
+
+    ""
+  end
+
   private
 
   def tab_ability_map
@@ -532,6 +538,7 @@ module ProjectsHelper
       metricsDashboardAccessLevel: feature.metrics_dashboard_access_level,
       operationsAccessLevel: feature.operations_access_level,
       showDefaultAwardEmojis: project.show_default_award_emojis?,
+      warnAboutPotentiallyUnwantedCharacters: project.warn_about_potentially_unwanted_characters?,
       securityAndComplianceAccessLevel: project.security_and_compliance_access_level,
       containerRegistryAccessLevel: feature.container_registry_access_level
     }
diff --git a/app/models/concerns/resolvable_discussion.rb b/app/models/concerns/resolvable_discussion.rb
index 3e1e5faee54..60e1dde17b9 100644
--- a/app/models/concerns/resolvable_discussion.rb
+++ b/app/models/concerns/resolvable_discussion.rb
@@ -81,8 +81,7 @@ module ResolvableDiscussion
     return false unless current_user
     return false unless resolvable?
 
-    current_user == self.noteable.try(:author) ||
-      current_user.can?(:resolve_note, self.project)
+    current_user.can?(:resolve_note, self.noteable)
   end
 
   def resolve!(current_user)
diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index e6406293c66..07f9bb99952 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -34,6 +34,8 @@ class Namespace < ApplicationRecord
   SHARED_RUNNERS_SETTINGS = [SR_DISABLED_AND_UNOVERRIDABLE, SR_DISABLED_WITH_OVERRIDE, SR_ENABLED].freeze
   URL_MAX_LENGTH = 255
 
+  PATH_TRAILING_VIOLATIONS = %w[.git .atom .].freeze
+
   cache_markdown_field :description, pipeline: :description
 
   has_many :projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
@@ -200,9 +202,14 @@ class Namespace < ApplicationRecord
       # Remove everything that's not in the list of allowed characters.
       path.gsub!(/[^a-zA-Z0-9_\-\.]/,    "")
       # Remove trailing violations ('.atom', '.git', or '.')
-      path.gsub!(/(\.atom|\.git|\.)*\z/, "")
+      loop do
+        orig = path
+        PATH_TRAILING_VIOLATIONS.each { |ext| path = path.chomp(ext) }
+        break if orig == path
+      end
+
       # Remove leading violations ('-')
-      path.gsub!(/\A\-+/,                "")
+      path.gsub!(/\A\-+/, "")
 
       # Users with the great usernames of "." or ".." would end up with a blank username.
       # Work around that by setting their username to "blank", followed by a counter.
diff --git a/app/models/project.rb b/app/models/project.rb
index 6eb19b4462c..2ceba10e86e 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -423,8 +423,8 @@ class Project < ApplicationRecord
     :container_registry_access_level, :container_registry_enabled?,
     to: :project_feature, allow_nil: true
   alias_method :container_registry_enabled, :container_registry_enabled?
-  delegate :show_default_award_emojis, :show_default_award_emojis=,
-    :show_default_award_emojis?,
+  delegate :show_default_award_emojis, :show_default_award_emojis=, :show_default_award_emojis?,
+    :warn_about_potentially_unwanted_characters, :warn_about_potentially_unwanted_characters=, :warn_about_potentially_unwanted_characters?,
     to: :project_setting, allow_nil: true
   delegate :scheduled?, :started?, :in_progress?, :failed?, :finished?,
     prefix: :import, to: :import_state, allow_nil: true
@@ -2714,8 +2714,23 @@ class Project < ApplicationRecord
     self.errors.add(:base, _("Could not change HEAD: branch '%{branch}' does not exist") % { branch: branch })
   end
 
+  def visible_group_links(for_user:)
+    user = for_user
+    links = project_group_links_with_preload
+    user.max_member_access_for_group_ids(links.map(&:group_id)) if user && links.any?
+
+    DeclarativePolicy.user_scope do
+      links.select { Ability.allowed?(user, :read_group, _1.group) }
+    end
+  end
+
   private
 
+  # overridden in EE
+  def project_group_links_with_preload
+    project_group_links
+  end
+
   def save_topics
     return if @topic_list.nil?
 
diff --git a/app/models/project_group_link.rb b/app/models/project_group_link.rb
index d704f4c2c87..8394ebe1df4 100644
--- a/app/models/project_group_link.rb
+++ b/app/models/project_group_link.rb
@@ -15,6 +15,7 @@ class ProjectGroupLink < ApplicationRecord
   validate :different_group
 
   scope :non_guests, -> { where('group_access > ?', Gitlab::Access::GUEST) }
+  scope :in_group, -> (group_ids) { where(group_id: group_ids) }
 
   alias_method :shared_with_group, :group
 
diff --git a/app/models/user.rb b/app/models/user.rb
index 25a2588a6a7..0e19e6e4a79 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -1434,7 +1434,7 @@ class User < ApplicationRecord
       name: name,
       username: username,
       avatar_url: avatar_url(only_path: false),
-      email: email
+      email: public_email.presence || _('[REDACTED]')
     }
   end
 
diff --git a/app/policies/issuable_policy.rb b/app/policies/issuable_policy.rb
index 61263e47d7c..39ce26526e6 100644
--- a/app/policies/issuable_policy.rb
+++ b/app/policies/issuable_policy.rb
@@ -11,6 +11,8 @@ class IssuablePolicy < BasePolicy
     @user && @subject.assignee_or_author?(@user)
   end
 
+  condition(:is_author) { @subject&.author == @user }
+
   rule { can?(:guest_access) & assignee_or_author }.policy do
     enable :read_issue
     enable :update_issue
@@ -20,6 +22,10 @@ class IssuablePolicy < BasePolicy
     enable :reopen_merge_request
   end
 
+  rule { is_author }.policy do
+    enable :resolve_note
+  end
+
   rule { locked & ~is_project_member }.policy do
     prevent :create_note
     prevent :admin_note
diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb
index 59aa47beff9..87573c9ad13 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -221,6 +221,7 @@ class ProjectPolicy < BasePolicy
     enable :set_note_created_at
     enable :set_emails_disabled
     enable :set_show_default_award_emojis
+    enable :set_warn_about_potentially_unwanted_characters
   end
 
   rule { can?(:guest_access) }.policy do
diff --git a/app/services/concerns/update_visibility_level.rb b/app/services/concerns/update_visibility_level.rb
index b7a161f5089..4cd14a2fb53 100644
--- a/app/services/concerns/update_visibility_level.rb
+++ b/app/services/concerns/update_visibility_level.rb
@@ -1,13 +1,17 @@
 # frozen_string_literal: true
 
 module UpdateVisibilityLevel
+  # check that user is allowed to set specified visibility_level
   def valid_visibility_level_change?(target, new_visibility)
-    # check that user is allowed to set specified visibility_level
-    if new_visibility && new_visibility.to_i != target.visibility_level
+    return true unless new_visibility
+
+    new_visibility_level = Gitlab::VisibilityLevel.level_value(new_visibility)
+
+    if new_visibility_level != target.visibility_level_value
       unless can?(current_user, :change_visibility_level, target) &&
-          Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility)
+          Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility_level)
 
-        deny_visibility_level(target, new_visibility)
+        deny_visibility_level(target, new_visibility_level)
         return false
       end
     end
diff --git a/app/services/groups/transfer_service.rb b/app/services/groups/transfer_service.rb
index 774f81b0a5e..334083a859f 100644
--- a/app/services/groups/transfer_service.rb
+++ b/app/services/groups/transfer_service.rb
@@ -182,6 +182,14 @@ module Groups
 
       # schedule refreshing projects for all the members of the group
       @group.refresh_members_authorized_projects
+
+      # When a group is transferred, it also affects who gets access to the projects shared to
+      # the subgroups within its hierarchy, so we also schedule jobs that refresh authorizations for all such shared projects.
+      project_group_shares_within_the_hierarchy = ProjectGroupLink.in_group(group.self_and_descendants.select(:id))
+
+      project_group_shares_within_the_hierarchy.find_each do |project_group_link|
+        AuthorizedProjectUpdate::ProjectRecalculateWorker.perform_async(project_group_link.project_id)
+      end
     end
 
     def raise_transfer_error(message)
diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb
index 1ad43b051be..2d6334251ad 100644
--- a/app/services/groups/update_service.rb
+++ b/app/services/groups/update_service.rb
@@ -15,7 +15,7 @@ module Groups
         return false
       end
 
-      return false unless valid_visibility_level_change?(group, params[:visibility_level])
+      return false unless valid_visibility_level_change?(group, group.visibility_attribute_value(params))
 
       return false unless valid_share_with_group_lock_change?
 
@@ -77,7 +77,7 @@ module Groups
     end
 
     def after_update
-      if group.previous_changes.include?(:visibility_level) && group.private?
+      if group.previous_changes.include?(group.visibility_level_field) && group.private?
         # don't enqueue immediately to prevent todos removal in case of a mistake
         TodosDestroyer::GroupPrivateWorker.perform_in(Todo::WAIT_FOR_DELETE, group.id)
       end
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 59e521853de..2daf098b94a 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -142,6 +142,7 @@ class IssuableBaseService < ::BaseProjectService
   def filter_severity(issuable)
     severity = params.delete(:severity)
     return unless severity && issuable.supports_severity?
+    return unless can_admin_issuable?(issuable)
 
     severity = IssuableSeverity::DEFAULT unless IssuableSeverity.severities.key?(severity)
     return if severity == issuable.severity
diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb
index a32e80af4b1..b34ecf06e52 100644
--- a/app/services/projects/update_service.rb
+++ b/app/services/projects/update_service.rb
@@ -49,7 +49,7 @@ module Projects
     private
 
     def validate!
-      unless valid_visibility_level_change?(project, params[:visibility_level])
+      unless valid_visibility_level_change?(project, project.visibility_attribute_value(params))
         raise ValidationError, s_('UpdateProject|New visibility level not allowed!')
       end
 
diff --git a/app/views/layouts/project.html.haml b/app/views/layouts/project.html.haml
index 2df502d2899..a54e0351d2f 100644
--- a/app/views/layouts/project.html.haml
+++ b/app/views/layouts/project.html.haml
@@ -6,6 +6,7 @@
 - display_subscription_banner!
 - display_namespace_storage_limit_alert!
 - @left_sidebar = true
+- @content_class = [@content_class, project_classes(@project)].compact.join(" ")
 
 - content_for :project_javascripts do
   - project = @target_project || @project