Merge branch '334043-fix-jira-xss-link' into 'master'

Fix XSS in Jira link

See merge request gitlab-org/security/gitlab!1604

app/helpers/external_link_helper.rb

 # frozen_string_literal: true
 
 module ExternalLinkHelper
+  include ActionView::Helpers::TextHelper
+
   def external_link(body, url, options = {})
-    link_to url, { target: '_blank', rel: 'noopener noreferrer' }.merge(options) do
+    link = link_to url, { target: '_blank', rel: 'noopener noreferrer' }.merge(options) do
       "#{body}#{sprite_icon('external-link', css_class: 'gl-ml-1')}".html_safe
     end
+    sanitize(link, tags: %w(a svg use), attributes: %w(target rel data-testid class href).concat(options.stringify_keys.keys))
   end
 end

app/helpers/icons_helper.rb

@@ -44,7 +44,7 @@ module IconsHelper
 
       content_tag(
         :svg,
-        content_tag(:use, '', { 'xlink:href' => "#{sprite_icon_path}##{icon_name}" } ),
+        content_tag(:use, '', { 'href' => "#{sprite_icon_path}##{icon_name}" } ),
         class: css_classes.empty? ? nil : css_classes.join(' '),
         data: { testid: "#{icon_name}-icon" }
       )

ee/spec/views/profiles/personal_access_tokens/_token_expiry_notification.html.haml_spec.rb

@@ -22,7 +22,7 @@ RSpec.describe 'profiles/personal_access_tokens/_token_expiry_notification.html.
 
       it 'contains the correct content', :aggregate_failures do
         expect(rendered).to have_selector '[data-feature-id="profile_personal_access_token_expiry"]'
-        expect(rendered).to match /<use xlink:href=".+?icons-.+?#error">/
+        expect(rendered).to match /<use href=".+?icons-.+?#error">/
         expect(rendered).to have_content '2 tokens have expired'
         expect(rendered).to have_content 'Until revoked, expired personal access tokens pose a security risk.'
       end

ee/spec/views/shared/credentials_inventory/_expiry_date.html.haml_spec.rb

@@ -46,7 +46,7 @@ RSpec.describe('shared/credentials_inventory/_expiry_date.html.haml') do
       end
 
       it 'has an icon' do
-        expect(rendered).to match(/<use xlink:href=".+?icons-.+?#warning">/)
+        expect(rendered).to match(/<use href=".+?icons-.+?#warning">/)
       end
     end
 
@@ -59,7 +59,7 @@ RSpec.describe('shared/credentials_inventory/_expiry_date.html.haml') do
       end
 
       it 'has an icon' do
-        expect(rendered).to match(/<use xlink:href=".+?icons-.+?#error">/)
+        expect(rendered).to match(/<use href=".+?icons-.+?#error">/)
       end
     end
   end

spec/helpers/external_link_helper_spec.rb

@@ -13,8 +13,14 @@ RSpec.describe ExternalLinkHelper do
 
   it 'allows options when creating external link with icon' do
     link = external_link('https://gitlab.com', 'https://gitlab.com', { "data-foo": "bar", class: "externalLink" }).to_s
-
     expect(link).to start_with('<a target="_blank" rel="noopener noreferrer" data-foo="bar" class="externalLink" href="https://gitlab.com">https://gitlab.com')
     expect(link).to include('data-testid="external-link-icon"')
   end
+
+  it 'sanitizes and returns external link with icon' do
+    link = external_link('sanitized link content', 'javascript:alert()').to_s
+    expect(link).not_to include('href="javascript:alert()"')
+    expect(link).to start_with('<a target="_blank" rel="noopener noreferrer">sanitized link content')
+    expect(link).to include('data-testid="external-link-icon"')
+  end
 end

spec/helpers/icons_helper_spec.rb

@@ -35,22 +35,22 @@ RSpec.describe IconsHelper do
 
     it 'returns svg icon html with DEFAULT_ICON_SIZE' do
       expect(sprite_icon(icon_name).to_s)
-        .to eq "<svg class=\"s#{IconsHelper::DEFAULT_ICON_SIZE}\" data-testid=\"#{icon_name}-icon\"><use xlink:href=\"#{icons_path}##{icon_name}\"></use></svg>"
+        .to eq "<svg class=\"s#{IconsHelper::DEFAULT_ICON_SIZE}\" data-testid=\"#{icon_name}-icon\"><use href=\"#{icons_path}##{icon_name}\"></use></svg>"
     end
 
     it 'returns svg icon html without size class' do
       expect(sprite_icon(icon_name, size: nil).to_s)
-        .to eq "<svg data-testid=\"#{icon_name}-icon\"><use xlink:href=\"#{icons_path}##{icon_name}\"></use></svg>"
+        .to eq "<svg data-testid=\"#{icon_name}-icon\"><use href=\"#{icons_path}##{icon_name}\"></use></svg>"
     end
 
     it 'returns svg icon html + size classes' do
       expect(sprite_icon(icon_name, size: 72).to_s)
-        .to eq "<svg class=\"s72\" data-testid=\"#{icon_name}-icon\"><use xlink:href=\"#{icons_path}##{icon_name}\"></use></svg>"
+        .to eq "<svg class=\"s72\" data-testid=\"#{icon_name}-icon\"><use href=\"#{icons_path}##{icon_name}\"></use></svg>"
     end
 
     it 'returns svg icon html + size classes + additional class' do
       expect(sprite_icon(icon_name, size: 72, css_class: 'icon-danger').to_s)
-        .to eq "<svg class=\"s72 icon-danger\" data-testid=\"#{icon_name}-icon\"><use xlink:href=\"#{icons_path}##{icon_name}\"></use></svg>"
+        .to eq "<svg class=\"s72 icon-danger\" data-testid=\"#{icon_name}-icon\"><use href=\"#{icons_path}##{icon_name}\"></use></svg>"
     end
 
     describe 'non existing icon' do