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

20 files changed, 133 insertions, 47 deletions

diff --git a/app/assets/javascripts/error_tracking/components/error_tracking_list.vue b/app/assets/javascripts/error_tracking/components/error_tracking_list.vue
index a07428dafea..de4b11699fc 100644
--- a/app/assets/javascripts/error_tracking/components/error_tracking_list.vue
+++ b/app/assets/javascripts/error_tracking/components/error_tracking_list.vue
@@ -22,12 +22,16 @@ import AccessorUtils from '~/lib/utils/accessor';
 import { __ } from '~/locale';
 import Tracking from '~/tracking';
 import TimeAgo from '~/vue_shared/components/time_ago_tooltip.vue';
+import { sanitizeUrl } from '~/lib/utils/url_utility';
 import { trackErrorListViewsOptions, trackErrorStatusUpdateOptions } from '../utils';
 import { I18N_ERROR_TRACKING_LIST } from '../constants';
 import ErrorTrackingActions from './error_tracking_actions.vue';
 
 export const tableDataClass = 'table-col d-flex d-md-table-cell align-items-center';
 
+const isValidErrorId = (errorId) => {
+  return /^[0-9]+$/.test(errorId);
+};
 export default {
   FIRST_PAGE: 1,
   PREV_PAGE: 1,
@@ -202,6 +206,9 @@ export default {
       this.searchByQuery(text);
     },
     getDetailsLink(errorId) {
+      if (!isValidErrorId(errorId)) {
+        return 'about:blank';
+      }
       return `error_tracking/${errorId}/details`;
     },
     goToNextPage() {
@@ -222,7 +229,10 @@ export default {
       return filter === this.statusFilter;
     },
     getIssueUpdatePath(errorId) {
-      return `/${this.projectPath}/-/error_tracking/${errorId}.json`;
+      if (!isValidErrorId(errorId)) {
+        return 'about:blank';
+      }
+      return sanitizeUrl(`/${this.projectPath}/-/error_tracking/${errorId}.json`);
     },
     filterErrors(status, label) {
       this.filterValue = label;
diff --git a/app/assets/javascripts/vue_merge_request_widget/components/extensions/child_content.vue b/app/assets/javascripts/vue_merge_request_widget/components/extensions/child_content.vue
index 52c9f047b76..a10e5efa0e7 100644
--- a/app/assets/javascripts/vue_merge_request_widget/components/extensions/child_content.vue
+++ b/app/assets/javascripts/vue_merge_request_widget/components/extensions/child_content.vue
@@ -1,5 +1,6 @@
 <script>
 import { GlBadge, GlLink, GlSafeHtmlDirective, GlModalDirective } from '@gitlab/ui';
+import { isArray } from 'lodash';
 import Actions from '../action_buttons.vue';
 import StatusIcon from './status_icon.vue';
 import { generateText } from './utils';
@@ -35,6 +36,20 @@ export default {
       required: true,
     },
   },
+  computed: {
+    subtext() {
+      const { subtext } = this.data;
+      if (subtext) {
+        if (isArray(subtext)) {
+          return subtext.map((t) => generateText(t)).join('<br />');
+        }
+
+        return generateText(subtext);
+      }
+
+      return null;
+    },
+  },
   methods: {
     isArray(arr) {
       return Array.isArray(arr);
@@ -93,11 +108,7 @@ export default {
             @clickedAction="onClickedAction"
           />
         </div>
-        <p
-          v-if="data.subtext"
-          v-safe-html="generateText(data.subtext)"
-          class="gl-m-0 gl-font-sm"
-        ></p>
+        <p v-if="subtext" v-safe-html="subtext" class="gl-m-0 gl-font-sm"></p>
       </div>
     </div>
     <template v-if="data.children && level === 2">
diff --git a/app/assets/javascripts/vue_merge_request_widget/components/extensions/utils.js b/app/assets/javascripts/vue_merge_request_widget/components/extensions/utils.js
index cba12507eba..757178ee336 100644
--- a/app/assets/javascripts/vue_merge_request_widget/components/extensions/utils.js
+++ b/app/assets/javascripts/vue_merge_request_widget/components/extensions/utils.js
@@ -35,6 +35,9 @@ const textStyleTags = {
   [getStartTag('small')]: '<span class="gl-font-sm gl-text-gray-700">',
 };
 
+const escapeText = (text) =>
+  document.createElement('div').appendChild(document.createTextNode(text)).parentNode.innerHTML;
+
 const createText = (text) => {
   return text
     .replace(
@@ -61,7 +64,7 @@ const createText = (text) => {
 
 export const generateText = (text) => {
   if (typeof text === 'string') {
-    return createText(text);
+    return createText(escapeText(text));
   } else if (
     typeof text === 'object' &&
     typeof text.text === 'string' &&
@@ -69,8 +72,8 @@ export const generateText = (text) => {
   ) {
     return createText(
       `${
-        text.prependText ? `${text.prependText} ` : ''
-      }<a class="gl-text-decoration-underline" href="${text.href}">${text.text}</a>`,
+        text.prependText ? `${escapeText(text.prependText)} ` : ''
+      }<a class="gl-text-decoration-underline" href="${text.href}">${escapeText(text.text)}</a>`,
     );
   }
 
diff --git a/app/assets/javascripts/vue_merge_request_widget/extensions/code_quality/index.js b/app/assets/javascripts/vue_merge_request_widget/extensions/code_quality/index.js
index 2477429af5b..68347ac269e 100644
--- a/app/assets/javascripts/vue_merge_request_widget/extensions/code_quality/index.js
+++ b/app/assets/javascripts/vue_merge_request_widget/extensions/code_quality/index.js
@@ -19,25 +19,23 @@ export default {
       if (errorSummary.errored >= 1 && errorSummary.resolved >= 1) {
         const improvements = sprintf(
           n__(
-            '%{strongOpen}%{errors}%{strongClose} point',
-            '%{strongOpen}%{errors}%{strongClose} points',
+            '%{strong_start}%{errors}%{strong_end} point',
+            '%{strong_start}%{errors}%{strong_end} points',
             resolvedErrors.length,
           ),
           {
             errors: resolvedErrors.length,
-            strongOpen: '<strong>',
-            strongClose: '</strong>',
           },
           false,
         );
 
         const degradations = sprintf(
           n__(
-            '%{strongOpen}%{errors}%{strongClose} point',
-            '%{strongOpen}%{errors}%{strongClose} points',
+            '%{strong_start}%{errors}%{strong_end} point',
+            '%{strong_start}%{errors}%{strong_end} points',
             newErrors.length,
           ),
-          { errors: newErrors.length, strongOpen: '<strong>', strongClose: '</strong>' },
+          { errors: newErrors.length },
           false,
         );
         return sprintf(
@@ -96,14 +94,11 @@ export default {
       this.collapsedData.resolvedErrors.map((e) => {
         return fullData.push({
           text: `${capitalizeFirstCharacter(e.severity)} - ${e.description}`,
-          subtext: sprintf(
-            s__(`ciReport|in %{open_link}${e.file_path}:${e.line}%{close_link}`),
-            {
-              open_link: `<a class="gl-text-decoration-underline" href="${e.urlPath}">`,
-              close_link: '</a>',
-            },
-            false,
-          ),
+          subtext: {
+            prependText: s__(`ciReport|in`),
+            text: `${e.file_path}:${e.line}`,
+            href: e.urlPath,
+          },
           icon: {
             name: SEVERITY_ICONS_EXTENSION[e.severity],
           },
diff --git a/app/assets/javascripts/vue_merge_request_widget/extensions/terraform/index.js b/app/assets/javascripts/vue_merge_request_widget/extensions/terraform/index.js
index 6611aedcb07..626a99f7d64 100644
--- a/app/assets/javascripts/vue_merge_request_widget/extensions/terraform/index.js
+++ b/app/assets/javascripts/vue_merge_request_widget/extensions/terraform/index.js
@@ -63,13 +63,16 @@ export default {
       if (valid.length) {
         title = validText;
         if (invalid.length) {
-          subtitle = sprintf(`<br>%{small_start}${invalidText}%{small_end}`);
+          subtitle = invalidText;
         }
       } else {
         title = invalidText;
       }
 
-      return `${title}${subtitle}`;
+      return {
+        subject: title,
+        meta: subtitle,
+      };
     },
     fetchCollapsedData() {
       return axios
@@ -152,9 +155,8 @@ export default {
       }
 
       return {
-        text: `${title}
-        <br>
-        ${subtitle}`,
+        text: title,
+        supportingText: subtitle,
         icon: { name: iconName },
         actions,
       };
diff --git a/app/assets/javascripts/vue_merge_request_widget/extensions/test_report/utils.js b/app/assets/javascripts/vue_merge_request_widget/extensions/test_report/utils.js
index 6896f8831e8..37f9964d23a 100644
--- a/app/assets/javascripts/vue_merge_request_widget/extensions/test_report/utils.js
+++ b/app/assets/javascripts/vue_merge_request_widget/extensions/test_report/utils.js
@@ -60,7 +60,7 @@ export const reportSubTextBuilder = ({ suite_errors: suiteErrors, summary }) =>
     if (suiteErrors?.base) {
       errors.push(`${i18n.baseReportParsingError} ${suiteErrors.base}`);
     }
-    return errors.join('<br />');
+    return errors;
   }
   return recentFailuresTextBuilder(summary);
 };
diff --git a/app/models/concerns/integrations/has_web_hook.rb b/app/models/concerns/integrations/has_web_hook.rb
index e6ca6cc7938..5fd71f3d72f 100644
--- a/app/models/concerns/integrations/has_web_hook.rb
+++ b/app/models/concerns/integrations/has_web_hook.rb
@@ -14,6 +14,11 @@ module Integrations
       raise NotImplementedError
     end
 
+    # Return the url variables to be used for the webhook.
+    def url_variables
+      raise NotImplementedError
+    end
+
     # Return whether the webhook should use SSL verification.
     def hook_ssl_verification
       if respond_to?(:enable_ssl_verification)
@@ -26,7 +31,11 @@ module Integrations
     # Create or update the webhook, raising an exception if it cannot be saved.
     def update_web_hook!
       hook = service_hook || build_service_hook
-      hook.url = hook_url if hook.url != hook_url # avoid reencryption
+
+      # Avoid reencryption
+      hook.url = hook_url if hook.url != hook_url
+      hook.url_variables = url_variables if hook.url_variables != url_variables
+
       hook.enable_ssl_verification = hook_ssl_verification
       hook.save! if hook.changed?
       hook
diff --git a/app/models/concerns/safe_url.rb b/app/models/concerns/safe_url.rb
index 7dce05bddba..d6378e6ac6f 100644
--- a/app/models/concerns/safe_url.rb
+++ b/app/models/concerns/safe_url.rb
@@ -3,13 +3,16 @@
 module SafeUrl
   extend ActiveSupport::Concern
 
+  # Return the URL with obfuscated userinfo
+  # and keeping it intact
   def safe_url(allowed_usernames: [])
     return if url.nil?
 
-    uri = URI.parse(url)
+    escaped = Addressable::URI.escape(url)
+    uri = URI.parse(escaped)
     uri.password = '*****' if uri.password
     uri.user = '*****' if uri.user && allowed_usernames.exclude?(uri.user)
-    uri.to_s
-  rescue URI::Error
+    Addressable::URI.unescape(uri.to_s)
+  rescue URI::Error, TypeError
   end
 end
diff --git a/app/models/hooks/web_hook_log.rb b/app/models/hooks/web_hook_log.rb
index 3fc3f193f19..c32957fbef9 100644
--- a/app/models/hooks/web_hook_log.rb
+++ b/app/models/hooks/web_hook_log.rb
@@ -22,7 +22,7 @@ class WebHookLog < ApplicationRecord
   validates :web_hook, presence: true
 
   before_save :obfuscate_basic_auth
-  before_save :redact_author_email
+  before_save :redact_user_emails
 
   def self.recent
     where(created_at: 2.days.ago.beginning_of_day..Time.zone.now)
@@ -54,9 +54,9 @@ class WebHookLog < ApplicationRecord
     self.url = safe_url
   end
 
-  def redact_author_email
-    return unless self.request_data.dig('commit', 'author', 'email').present?
-
-    self.request_data['commit']['author']['email'] = _('[REDACTED]')
+  def redact_user_emails
+    self.request_data.deep_transform_values! do |value|
+      value =~ URI::MailTo::EMAIL_REGEXP ? _('[REDACTED]') : value
+    end
   end
 end
diff --git a/app/models/integrations/buildkite.rb b/app/models/integrations/buildkite.rb
index 7a48e71b934..f2d2aca3ffe 100644
--- a/app/models/integrations/buildkite.rb
+++ b/app/models/integrations/buildkite.rb
@@ -50,7 +50,11 @@ module Integrations
 
     override :hook_url
     def hook_url
-      "#{buildkite_endpoint('webhook')}/deliver/#{webhook_token}"
+      "#{buildkite_endpoint('webhook')}/deliver/{webhook_token}"
+    end
+
+    def url_variables
+      { 'webhook_token' => webhook_token }
     end
 
     def execute(data)
diff --git a/app/models/integrations/datadog.rb b/app/models/integrations/datadog.rb
index 4479725a33b..c9407aa738e 100644
--- a/app/models/integrations/datadog.rb
+++ b/app/models/integrations/datadog.rb
@@ -154,13 +154,17 @@ module Integrations
       url = api_url.presence || sprintf(URL_TEMPLATE, datadog_domain: datadog_domain)
       url = URI.parse(url)
       query = {
-        "dd-api-key" => api_key,
+        "dd-api-key" => 'THIS_VALUE_WILL_BE_REPLACED',
         service: datadog_service.presence,
         env: datadog_env.presence,
         tags: datadog_tags_query_param.presence
       }.compact
       url.query = query.to_query
-      url.to_s
+      url.to_s.gsub('THIS_VALUE_WILL_BE_REPLACED', '{api_key}')
+    end
+
+    def url_variables
+      { 'api_key' => api_key }
     end
 
     def execute(data)
diff --git a/app/models/integrations/drone_ci.rb b/app/models/integrations/drone_ci.rb
index de69afeba6a..d1a64aa96d4 100644
--- a/app/models/integrations/drone_ci.rb
+++ b/app/models/integrations/drone_ci.rb
@@ -106,7 +106,11 @@ module Integrations
 
     override :hook_url
     def hook_url
-      [drone_url, "/hook", "?owner=#{project.namespace.full_path}", "&name=#{project.path}", "&access_token=#{token}"].join
+      [drone_url, "/hook", "?owner=#{project.namespace.full_path}", "&name=#{project.path}", "&access_token={token}"].join
+    end
+
+    def url_variables
+      { 'token' => token }
     end
 
     override :update_web_hook!
diff --git a/app/models/integrations/jenkins.rb b/app/models/integrations/jenkins.rb
index c68b5fd2a96..74a6449f4f9 100644
--- a/app/models/integrations/jenkins.rb
+++ b/app/models/integrations/jenkins.rb
@@ -69,6 +69,10 @@ module Integrations
       url.to_s
     end
 
+    def url_variables
+      {}
+    end
+
     def self.supported_events
       %w(push merge_request tag_push)
     end
diff --git a/app/models/integrations/packagist.rb b/app/models/integrations/packagist.rb
index f91404dab23..7177c82a167 100644
--- a/app/models/integrations/packagist.rb
+++ b/app/models/integrations/packagist.rb
@@ -66,7 +66,11 @@ module Integrations
     override :hook_url
     def hook_url
       base_url = server.presence || 'https://packagist.org'
-      "#{base_url}/api/update-package?username=#{username}&apiToken=#{token}"
+      "#{base_url}/api/update-package?username={username}&apiToken={token}"
+    end
+
+    def url_variables
+      { 'username' => username, 'token' => token }
     end
   end
 end
diff --git a/app/models/user.rb b/app/models/user.rb
index 8825c18ea48..3f07e1b1ec0 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -2159,6 +2159,10 @@ class User < ApplicationRecord
     (Date.current - created_at.to_date).to_i
   end
 
+  def webhook_email
+    public_email.presence || _('[REDACTED]')
+  end
+
   protected
 
   # override, from Devise::Validatable
diff --git a/app/policies/issuable_policy.rb b/app/policies/issuable_policy.rb
index e5913bab726..e864ce8752a 100644
--- a/app/policies/issuable_policy.rb
+++ b/app/policies/issuable_policy.rb
@@ -22,6 +22,12 @@ class IssuablePolicy < BasePolicy
     enable :reopen_issue
   end
 
+  # This rule replicates permissions in NotePolicy#can_read_confidential and it's used in
+  # TodoPolicy for performance reasons
+  rule { can?(:reporter_access) | assignee_or_author | admin }.policy do
+    enable :read_confidential_notes
+  end
+
   rule { can?(:read_merge_request) & assignee_or_author }.policy do
     enable :update_merge_request
     enable :reopen_merge_request
diff --git a/app/policies/note_policy.rb b/app/policies/note_policy.rb
index e85f18f2d37..1bffcc5aea2 100644
--- a/app/policies/note_policy.rb
+++ b/app/policies/note_policy.rb
@@ -20,6 +20,7 @@ class NotePolicy < BasePolicy
 
   condition(:confidential, scope: :subject) { @subject.confidential? }
 
+  # If this condition changes IssuablePolicy#read_confidential_notes should be updated too
   condition(:can_read_confidential) do
     access_level >= Gitlab::Access::REPORTER || @subject.noteable_assignee_or_author?(@user) || admin?
   end
diff --git a/app/policies/todo_policy.rb b/app/policies/todo_policy.rb
index 6237fbc50fa..5c24964f24a 100644
--- a/app/policies/todo_policy.rb
+++ b/app/policies/todo_policy.rb
@@ -5,10 +5,25 @@ class TodoPolicy < BasePolicy
   condition(:own_todo) do
     @user && @subject.user_id == @user.id
   end
+
+  desc "User can read the todo's target"
   condition(:can_read_target) do
     @user && @subject.target&.readable_by?(@user)
   end
 
+  desc "Todo has confidential note"
+  condition(:has_confidential_note, scope: :subject) { @subject&.note&.confidential? }
+
+  desc "User can read the todo's confidential note"
+  condition(:can_read_todo_confidential_note) do
+    @user && @user.can?(:read_confidential_notes, @subject.target)
+  end
+
   rule { own_todo & can_read_target }.enable :read_todo
-  rule { own_todo & can_read_target }.enable :update_todo
+  rule { can?(:read_todo) }.enable :update_todo
+
+  rule { has_confidential_note & ~can_read_todo_confidential_note }.policy do
+    prevent :read_todo
+    prevent :update_todo
+  end
 end
diff --git a/app/services/base_project_service.rb b/app/services/base_project_service.rb
index 1bf4a235a79..8cb6b632a9e 100644
--- a/app/services/base_project_service.rb
+++ b/app/services/base_project_service.rb
@@ -7,7 +7,9 @@ class BaseProjectService < ::BaseContainerService
   attr_accessor :project
 
   def initialize(project:, current_user: nil, params: {})
-    super(container: project, current_user: current_user, params: params)
+    # we need to exclude project params since they may come from external requests. project should always
+    # be passed as part of the service's initializer
+    super(container: project, current_user: current_user, params: params.except(:project, :project_id))
 
     @project = project
   end
diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb
index bf5be708060..7250ce5c0b0 100644
--- a/app/uploaders/file_uploader.rb
+++ b/app/uploaders/file_uploader.rb
@@ -14,7 +14,12 @@ class FileUploader < GitlabUploader
   include ObjectStorage::Concern
   prepend ObjectStorage::Extension::RecordsUploads
 
-  MARKDOWN_PATTERN = %r{\!?\[.*?\]\(/uploads/(?<secret>[0-9a-f]{32})/(?<file>.*?)\)}.freeze
+  # This pattern is vulnerable to malicious inputs, so use Gitlab::UntrustedRegexp
+  # to place bounds on execution time
+  MARKDOWN_PATTERN = Gitlab::UntrustedRegexp.new(
+    '!?\[.*?\]\(/uploads/(?P<secret>[0-9a-f]{32})/(?P<file>.*?)\)'
+  )
+
   DYNAMIC_PATH_PATTERN = %r{.*(?<secret>\b(\h{10}|\h{32}))\/(?<identifier>.*)}.freeze
   VALID_SECRET_PATTERN = %r{\A\h{10,32}\z}.freeze