diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2022-01-10 23:36:29 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2022-01-10 23:36:29 +0300 |
commit | 1eef146c2d1de19d4e995d421e5787053e50db80 (patch) | |
tree | 2761efabea712248557826977a849e31e3fdb961 /app/models | |
parent | 661d663ab2b7c69977ba8a0db02ef4afc2427e39 (diff) |
Add latest changes from gitlab-org/security/gitlab@14-6-stable-ee
Diffstat (limited to 'app/models')
10 files changed, 58 insertions, 33 deletions
diff --git a/app/models/concerns/integrations/slack_mattermost_notifier.rb b/app/models/concerns/integrations/slack_mattermost_notifier.rb index cb6fafa8de0..be13701289a 100644 --- a/app/models/concerns/integrations/slack_mattermost_notifier.rb +++ b/app/models/concerns/integrations/slack_mattermost_notifier.rb @@ -6,6 +6,13 @@ module Integrations def notify(message, opts) # See https://gitlab.com/gitlab-org/slack-notifier/#custom-http-client + # + # TODO: By default both Markdown and HTML links are converted into Slack "mrkdwn" syntax, + # but it seems we only need to support Markdown and could disable HTML. + # + # See: + # - https://gitlab.com/gitlab-org/slack-notifier#middleware + # - https://gitlab.com/gitlab-org/gitlab/-/issues/347048 notifier = ::Slack::Messenger.new(webhook, opts.merge(http_client: HTTPClient)) notifier.ping( message.pretext, diff --git a/app/models/integrations/chat_message/alert_message.rb b/app/models/integrations/chat_message/alert_message.rb index ef0579124fe..e2c689f9435 100644 --- a/app/models/integrations/chat_message/alert_message.rb +++ b/app/models/integrations/chat_message/alert_message.rb @@ -23,7 +23,7 @@ module Integrations def attachments [{ - title: title, + title: strip_markup(title), title_link: alert_url, color: attachment_color, fields: attachment_fields @@ -31,7 +31,7 @@ module Integrations end def message - "Alert firing in #{project_name}" + "Alert firing in #{strip_markup(project_name)}" end private diff --git a/app/models/integrations/chat_message/base_message.rb b/app/models/integrations/chat_message/base_message.rb index afe3ffc45a0..ab213f4b43f 100644 --- a/app/models/integrations/chat_message/base_message.rb +++ b/app/models/integrations/chat_message/base_message.rb @@ -5,6 +5,10 @@ module Integrations class BaseMessage RELATIVE_LINK_REGEX = %r{!\[[^\]]*\]\((/uploads/[^\)]*)\)}.freeze + # Markup characters which are used for links in HTML, Markdown, + # and Slack "mrkdwn" syntax (`<http://example.com|Label>`). + UNSAFE_MARKUP_CHARACTERS = '<>[]|' + attr_reader :markdown attr_reader :user_full_name attr_reader :user_name @@ -65,12 +69,26 @@ module Integrations string.gsub(RELATIVE_LINK_REGEX, "#{project_url}\\1") end + # Remove unsafe markup from user input, which can be used to hijack links in our own markup, + # or insert new ones. + # + # This currently removes Markdown and Slack "mrkdwn" links (keeping the link label), + # and all HTML markup (keeping the text nodes). + # We can't just escape the markup characters, because each chat app handles this differently. + # + # See: + # - https://api.slack.com/reference/surfaces/formatting#escaping + # - https://gitlab.com/gitlab-org/slack-notifier#escaping + def strip_markup(string) + string&.delete(UNSAFE_MARKUP_CHARACTERS) + end + def attachment_color '#345' end def link(text, url) - "[#{text}](#{url})" + "[#{strip_markup(text)}](#{url})" end def pretty_duration(seconds) diff --git a/app/models/integrations/chat_message/deployment_message.rb b/app/models/integrations/chat_message/deployment_message.rb index c4f3bf9610d..b28edeecb4d 100644 --- a/app/models/integrations/chat_message/deployment_message.rb +++ b/app/models/integrations/chat_message/deployment_message.rb @@ -27,7 +27,7 @@ module Integrations def attachments [{ - text: "#{project_link} with job #{deployment_link} by #{user_link}\n#{commit_link}: #{commit_title}", + text: "#{project_link} with job #{deployment_link} by #{user_link}\n#{commit_link}: #{strip_markup(commit_title)}", color: color }] end @@ -40,9 +40,9 @@ module Integrations def message if running? - "Starting deploy to #{environment}" + "Starting deploy to #{strip_markup(environment)}" else - "Deploy to #{environment} #{humanized_status}" + "Deploy to #{strip_markup(environment)} #{humanized_status}" end end diff --git a/app/models/integrations/chat_message/issue_message.rb b/app/models/integrations/chat_message/issue_message.rb index 5fa6bd4090f..ca8ef670e67 100644 --- a/app/models/integrations/chat_message/issue_message.rb +++ b/app/models/integrations/chat_message/issue_message.rb @@ -32,7 +32,7 @@ module Integrations def activity { - title: "Issue #{state} by #{user_combined_name}", + title: "Issue #{state} by #{strip_markup(user_combined_name)}", subtitle: "in #{project_link}", text: issue_link, image: user_avatar @@ -42,7 +42,7 @@ module Integrations private def message - "[#{project_link}] Issue #{issue_link} #{state} by #{user_combined_name}" + "[#{project_link}] Issue #{issue_link} #{state} by #{strip_markup(user_combined_name)}" end def opened_issue? @@ -67,7 +67,7 @@ module Integrations end def issue_title - "#{Issue.reference_prefix}#{issue_iid} #{title}" + "#{Issue.reference_prefix}#{issue_iid} #{strip_markup(title)}" end end end diff --git a/app/models/integrations/chat_message/merge_message.rb b/app/models/integrations/chat_message/merge_message.rb index d2f48699f50..98da38de27c 100644 --- a/app/models/integrations/chat_message/merge_message.rb +++ b/app/models/integrations/chat_message/merge_message.rb @@ -29,7 +29,7 @@ module Integrations def activity { - title: "Merge request #{state_or_action_text} by #{user_combined_name}", + title: "Merge request #{state_or_action_text} by #{strip_markup(user_combined_name)}", subtitle: "in #{project_link}", text: merge_request_link, image: user_avatar @@ -39,7 +39,7 @@ module Integrations private def format_title(title) - '*' + title.lines.first.chomp + '*' + '*' + strip_markup(title.lines.first.chomp) + '*' end def message @@ -51,7 +51,7 @@ module Integrations end def merge_request_message - "#{user_combined_name} #{state_or_action_text} merge request #{merge_request_link} in #{project_link}" + "#{strip_markup(user_combined_name)} #{state_or_action_text} merge request #{merge_request_link} in #{project_link}" end def merge_request_link @@ -59,7 +59,7 @@ module Integrations end def merge_request_title - "#{MergeRequest.reference_prefix}#{merge_request_iid} #{title}" + "#{MergeRequest.reference_prefix}#{merge_request_iid} #{strip_markup(title)}" end def merge_request_url diff --git a/app/models/integrations/chat_message/note_message.rb b/app/models/integrations/chat_message/note_message.rb index 96675d2b27c..b2b2059536a 100644 --- a/app/models/integrations/chat_message/note_message.rb +++ b/app/models/integrations/chat_message/note_message.rb @@ -35,9 +35,9 @@ module Integrations def activity { - title: "#{user_combined_name} #{link('commented on ' + target, note_url)}", + title: "#{strip_markup(user_combined_name)} #{link('commented on ' + target, note_url)}", subtitle: "in #{project_link}", - text: formatted_title, + text: strip_markup(formatted_title), image: user_avatar } end @@ -45,7 +45,7 @@ module Integrations private def message - "#{user_combined_name} #{link('commented on ' + target, note_url)} in #{project_link}: *#{formatted_title}*" + "#{strip_markup(user_combined_name)} #{link('commented on ' + target, note_url)} in #{project_link}: *#{strip_markup(formatted_title)}*" end def format_title(title) diff --git a/app/models/integrations/chat_message/pipeline_message.rb b/app/models/integrations/chat_message/pipeline_message.rb index a3f68d34035..b3502905bf7 100644 --- a/app/models/integrations/chat_message/pipeline_message.rb +++ b/app/models/integrations/chat_message/pipeline_message.rb @@ -56,7 +56,7 @@ module Integrations [{ fallback: format(message), color: attachment_color, - author_name: user_combined_name, + author_name: strip_markup(user_combined_name), author_icon: user_avatar, author_link: author_url, title: s_("ChatMessage|Pipeline #%{pipeline_id} %{humanized_status} in %{duration}") % @@ -80,7 +80,7 @@ module Integrations pipeline_link: pipeline_link, ref_type: ref_type, ref_link: ref_link, - user_combined_name: user_combined_name, + user_combined_name: strip_markup(user_combined_name), humanized_status: humanized_status }, subtitle: s_("ChatMessage|in %{project_link}") % { project_link: project_link }, @@ -154,7 +154,7 @@ module Integrations pipeline_link: pipeline_link, ref_type: ref_type, ref_link: ref_link, - user_combined_name: user_combined_name, + user_combined_name: strip_markup(user_combined_name), humanized_status: humanized_status, duration: pretty_duration(duration) } @@ -189,7 +189,7 @@ module Integrations end def ref_link - "[#{ref}](#{ref_url})" + link(ref, ref_url) end def project_url @@ -197,7 +197,7 @@ module Integrations end def project_link - "[#{project.name}](#{project_url})" + link(project.name, project_url) end def pipeline_failed_jobs_url @@ -213,7 +213,7 @@ module Integrations end def pipeline_link - "[##{pipeline_id}](#{pipeline_url})" + link("##{pipeline_id}", pipeline_url) end def job_url(job) @@ -221,7 +221,7 @@ module Integrations end def job_link(job) - "[#{job[:name]}](#{job_url(job)})" + link(job[:name], job_url(job)) end def failed_jobs_links @@ -242,7 +242,7 @@ module Integrations def stage_link(stage) # All stages link to the pipeline page - "[#{stage}](#{pipeline_url})" + link(stage, pipeline_url) end def failed_stages_links @@ -254,7 +254,7 @@ module Integrations end def commit_link - "[#{commit.title}](#{commit_url})" + link(commit.title, commit_url) end def author_url diff --git a/app/models/integrations/chat_message/push_message.rb b/app/models/integrations/chat_message/push_message.rb index fabd214633b..60a3105d1c0 100644 --- a/app/models/integrations/chat_message/push_message.rb +++ b/app/models/integrations/chat_message/push_message.rb @@ -39,7 +39,7 @@ module Integrations def humanized_action(short: false) action, ref_link, target_link = compose_action_details - text = [user_combined_name, action, ref_type, ref_link] + text = [strip_markup(user_combined_name), action, ref_type, ref_link] text << target_link unless short text.join(' ') end @@ -67,7 +67,7 @@ module Integrations url = commit[:url] - "[#{id}](#{url}): #{title} - #{author}" + "#{link(id, url)}: #{strip_markup(title)} - #{strip_markup(author)}" end def new_branch? @@ -91,15 +91,15 @@ module Integrations end def ref_link - "[#{ref}](#{ref_url})" + link(ref, ref_url) end def project_link - "[#{project_name}](#{project_url})" + link(project_name, project_url) end def compare_link - "[Compare changes](#{compare_url})" + link('Compare changes', compare_url) end def compose_action_details diff --git a/app/models/integrations/chat_message/wiki_page_message.rb b/app/models/integrations/chat_message/wiki_page_message.rb index 00f0f911b0e..e7299696856 100644 --- a/app/models/integrations/chat_message/wiki_page_message.rb +++ b/app/models/integrations/chat_message/wiki_page_message.rb @@ -36,9 +36,9 @@ module Integrations def activity { - title: "#{user_combined_name} #{action} #{wiki_page_link}", + title: "#{strip_markup(user_combined_name)} #{action} #{wiki_page_link}", subtitle: "in #{project_link}", - text: title, + text: strip_markup(title), image: user_avatar } end @@ -46,7 +46,7 @@ module Integrations private def message - "#{user_combined_name} #{action} #{wiki_page_link} (#{diff_link}) in #{project_link}: *#{title}*" + "#{strip_markup(user_combined_name)} #{action} #{wiki_page_link} (#{diff_link}) in #{project_link}: *#{strip_markup(title)}*" end def description_message |