Merge branch 'security-dblessing_oauth_authorization_html_escape-17-4' into '17-4-stable-ee'

Escape OAuth application name on authorize page

See merge request https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/4517



Merged-by: GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com>
Approved-by: Andrew Evans <aevans@gitlab.com>
Co-authored-by: Drew Blessing <drew@gitlab.com>

app/views/doorkeeper/authorizations/new.html.haml

 .gl-ml-auto.gl-mr-auto{ class: 'sm:gl-w-1/2' }
   .gl-items-center
     .gl-text-size-h1
-      = html_escape(_('%{client_name} is requesting access to your account on %{title}.')) % { title: brand_title.html_safe, client_name: "<strong>#{html_escape(@pre_auth.client.name)}</strong>".html_safe }
+      = safe_format(_('%{strong_start}%{client_name}%{strong_end} is requesting access to your account on %{title}.'),
+        { client_name: @pre_auth.client.name, title: brand_title },
+        tag_pair(tag.strong, :strong_start, :strong_end))
     .gl-flex.gl-items-center.gl-gap-2.gl-py-5
       = render Pajamas::AvatarComponent.new(current_user, size: 24, avatar_options: { data: { testid: 'user_avatar_content' }, title: current_user.username })
       .gl-pl-1
@@ -12,7 +14,9 @@
   - if current_user.admin?
     = render Pajamas::AlertComponent.new(variant: :warning, dismissible: false, alert_options: { class: 'gl-mb-5'}) do |c|
       - c.with_body do
-        = html_escape(_('You are an administrator, which means authorizing access to %{client_name} will allow it to interact with GitLab as an administrator as well.')) % { client_name: "<strong>#{html_escape(@pre_auth.client.name)}</strong>".html_safe }
+        = safe_format(_('You are an administrator, which means authorizing access to %{strong_start}%{client_name}%{strong_end} will allow it to interact with GitLab as an administrator as well.'),
+          { client_name: @pre_auth.client.name },
+          tag_pair(tag.strong, :strong_start, :strong_end))
   - if @pre_auth.scopes
     - @pre_auth.scopes.each do |scope|
       %strong= t scope, scope: [:doorkeeper, :scopes]
@@ -26,12 +30,17 @@
       - else
         %p.gl-text-orange-500
           = sprite_icon('warning-solid')
-          = html_escape(_('Make sure you trust %{client_name} before authorizing.')) % { client_name: "<strong>#{html_escape(@pre_auth.client.name)}</strong>".html_safe }
+          = safe_format(_('Make sure you trust %{strong_start}%{client_name}%{strong_end} before authorizing.'),
+            { client_name: @pre_auth.client.name },
+            tag_pair(tag.strong, :strong_start, :strong_end))
       %p
-        = html_escape(_('%{owner} %{created_date} ago.')) % { owner: auth_app_owner_text(@pre_auth.client.application.owner), created_date: time_ago_in_words(@pre_auth.client.application.created_at.to_date) }
+        = safe_format(_('%{owner} %{created_date} ago.'),
+          { owner: auth_app_owner_text(@pre_auth.client.application.owner),
+            created_date: time_ago_in_words(@pre_auth.client.application.created_at.to_date) })
         - domain = URI.parse(@pre_auth.redirect_uri).host.gsub(/^www\./, '')
         - if @pre_auth.redirect_uri.start_with?('http://', 'https://') && domain != 'localhost'
-          = html_escape(_('You will be redirected to %{domain} after authorizing.')) % { domain: "<strong>#{domain}</strong>".html_safe }
+          = safe_format(_('You will be redirected to %{strong_start}%{domain}%{strong_end} after authorizing.'),
+            { domain: domain }, tag_pair(tag.strong, :strong_start, :strong_end))
   %div
     = form_tag oauth_authorization_path, method: :post, class: 'gl-inline-block gl-pr-3' do
       = hidden_field_tag :client_id, @pre_auth.client.uid
@@ -44,8 +53,8 @@
       = hidden_field_tag :code_challenge_method, @pre_auth.code_challenge_method
       = render Pajamas::ButtonComponent.new(type: :submit,
         variant: :confirm,
-        button_options: {testid: 'authorization-button'}) do
-        = html_escape(_('Authorize %{client_name}')) % { client_name: @pre_auth.client.name.html_safe }
+        button_options: { data: { testid: 'authorization-button' }}) do
+        = safe_format(_('Authorize %{client_name}'), { client_name: @pre_auth.client.name })
     = form_tag oauth_authorization_path, method: :delete, class: 'gl-inline-block' do
       = hidden_field_tag :client_id, @pre_auth.client.uid
       = hidden_field_tag :redirect_uri, @pre_auth.redirect_uri

locale/gitlab.pot

@@ -641,9 +641,6 @@ msgstr[1] ""
 msgid "%{chartTitle} no data series"
 msgstr ""
-msgid "%{client_name} is requesting access to your account on %{title}."
-msgstr ""
-
 msgid "%{codeStart}$%{codeEnd} will be treated as the start of a reference to another variable."
 msgstr ""
@@ -1379,6 +1376,9 @@ msgid_plural "%{strong_start}%{branch_count}%{strong_end} Branches"
 msgstr[0] ""
 msgstr[1] ""
+msgid "%{strong_start}%{client_name}%{strong_end} is requesting access to your account on %{title}."
+msgstr ""
+
 msgid "%{strong_start}%{commit_count}%{strong_end} Commit"
 msgid_plural "%{strong_start}%{commit_count}%{strong_end} Commits"
 msgstr[0] ""
@@ -32536,7 +32536,7 @@ msgstr ""
 msgid "Make sure you save it - you won't be able to access it again."
 msgstr ""
-msgid "Make sure you trust %{client_name} before authorizing."
+msgid "Make sure you trust %{strong_start}%{client_name}%{strong_end} before authorizing."
 msgstr ""
 msgid "Makes this %{type} confidential."
@@ -62528,7 +62528,7 @@ msgstr ""
 msgid "You are already impersonating another user"
 msgstr ""
-msgid "You are an administrator, which means authorizing access to %{client_name} will allow it to interact with GitLab as an administrator as well."
+msgid "You are an administrator, which means authorizing access to %{strong_start}%{client_name}%{strong_end} will allow it to interact with GitLab as an administrator as well."
 msgstr ""
 msgid "You are attempting to delete a file that has been previously updated."
@@ -63128,7 +63128,7 @@ msgstr ""
 msgid "You tried to fork %{link_to_the_project} but it failed for the following reason:"
 msgstr ""
-msgid "You will be redirected to %{domain} after authorizing."
+msgid "You will be redirected to %{strong_start}%{domain}%{strong_end} after authorizing."
 msgstr ""
 msgid "You will be the author of all events in the activity feed that are the result of an update, like new branches being created or new commits being pushed to existing branches."

spec/features/oauth_provider_authorize_spec.rb

@@ -3,21 +3,81 @@
 require 'spec_helper'
 RSpec.describe 'OAuth Provider', feature_category: :system_access do
+  def visit_oauth_authorization_path
+    visit oauth_authorization_path(
+      client_id: application.uid,
+      redirect_uri: application.redirect_uri.split.first,
+      response_type: 'code',
+      state: 'my_state',
+      scope: 'read_user'
+    )
+  end
+
+  before do
+    sign_in(user)
+  end
+
   describe 'Standard OAuth Authorization' do
     let(:application) { create(:oauth_application, scopes: 'read_user') }
     before do
-      sign_in(user)
-
-      visit oauth_authorization_path(
-        client_id: application.uid,
-        redirect_uri: application.redirect_uri.split.first,
-        response_type: 'code',
-        state: 'my_state',
-        scope: 'read_user'
-      )
+      visit_oauth_authorization_path
     end
     it_behaves_like 'Secure OAuth Authorizations'
   end
+
+  context 'when the OAuth application has HTML in the name' do
+    let(:client_name) { '<img src=x onerror=alert(1)>' }
+    let(:application) { create(:oauth_application, name: client_name, scopes: 'read_user') }
+    let(:user) { create(:admin) }
+
+    before do
+      visit_oauth_authorization_path
+    end
+
+    it 'sanitizes the HTML in the authorize button' do
+      within_testid('authorization-button') do
+        expect(page).to have_content(format(_('Authorize %{client_name}'), client_name: client_name))
+      end
+    end
+
+    # rubocop:disable Layout/LineLength -- It is a string
+    it 'sanitizes the HTML in the warning text' do
+      expect(page).to have_content(
+        format(
+          _('You are an administrator, which means authorizing access to %{client_name} will allow it to interact with GitLab as an administrator as well.'),
+          client_name: client_name
+        )
+      )
+    end
+    # rubocop:enable Layout/LineLength
+
+    it 'sanitizes the trust text HTML' do
+      expect(page).to have_content(
+        format(
+          _('Make sure you trust %{client_name} before authorizing.'),
+          client_name: client_name
+        ))
+    end
+  end
+
+  context 'when brand title has HTML' do
+    let(:application) { create(:oauth_application, scopes: 'read_user') }
+    let(:user) { create(:user) }
+    let!(:appearance) { create(:appearance, title: '<img src=x onerror=alert(1)>') }
+
+    before do
+      visit_oauth_authorization_path
+    end
+
+    it 'sanitizes the HTML' do
+      expect(page).to have_content(
+        format(_('%{client_name} is requesting access to your account on %{title}.'),
+          title: '<img src=x onerror=alert(1)>',
+          client_name: application.name
+        )
+      )
+    end
+  end
 end