Add latest changes from gitlab-org/security/gitlab@13-12-stable-ee

16 files changed, 179 insertions, 65 deletions

diff --git a/app/assets/javascripts/lib/utils/url_utility.js b/app/assets/javascripts/lib/utils/url_utility.js
index 5b3aa3cf9dc..5ef39ded9cb 100644
--- a/app/assets/javascripts/lib/utils/url_utility.js
+++ b/app/assets/javascripts/lib/utils/url_utility.js
@@ -535,3 +535,27 @@ export function getURLOrigin(url) {
     return null;
   }
 }
+
+/**
+ * Returns `true` if the given `url` resolves to the same origin the page is served
+ * from; otherwise, returns `false`.
+ *
+ * The `url` may be absolute or relative.
+ *
+ * @param {string} url The URL to check.
+ * @returns {boolean}
+ */
+export function isSameOriginUrl(url) {
+  if (typeof url !== 'string') {
+    return false;
+  }
+
+  const { origin } = window.location;
+
+  try {
+    return new URL(url, origin).origin === origin;
+  } catch {
+    // Invalid URLs cannot have the same origin
+    return false;
+  }
+}
diff --git a/app/assets/javascripts/releases/components/app_edit_new.vue b/app/assets/javascripts/releases/components/app_edit_new.vue
index aecd0d6371e..3774f97a060 100644
--- a/app/assets/javascripts/releases/components/app_edit_new.vue
+++ b/app/assets/javascripts/releases/components/app_edit_new.vue
@@ -2,6 +2,7 @@
 import { GlButton, GlFormInput, GlFormGroup, GlSprintf } from '@gitlab/ui';
 import { mapState, mapActions, mapGetters } from 'vuex';
 import { getParameterByName } from '~/lib/utils/common_utils';
+import { isSameOriginUrl } from '~/lib/utils/url_utility';
 import { __ } from '~/locale';
 import MilestoneCombobox from '~/milestones/components/milestone_combobox.vue';
 import { BACK_URL_PARAM } from '~/releases/constants';
@@ -65,7 +66,13 @@ export default {
       },
     },
     cancelPath() {
-      return getParameterByName(BACK_URL_PARAM) || this.releasesPagePath;
+      const backUrl = getParameterByName(BACK_URL_PARAM);
+
+      if (isSameOriginUrl(backUrl)) {
+        return backUrl;
+      }
+
+      return this.releasesPagePath;
     },
     saveButtonLabel() {
       return this.isExistingRelease ? __('Save changes') : __('Create release');
diff --git a/app/models/audit_event.rb b/app/models/audit_event.rb
index aff7eef4622..11036b76fc1 100644
--- a/app/models/audit_event.rb
+++ b/app/models/audit_event.rb
@@ -32,6 +32,9 @@ class AuditEvent < ApplicationRecord
   scope :by_author_id, -> (author_id) { where(author_id: author_id) }
 
   after_initialize :initialize_details
+
+  before_validation :sanitize_message
+
   # Note: The intention is to remove this once refactoring of AuditEvent
   # has proceeded further.
   #
@@ -83,6 +86,14 @@ class AuditEvent < ApplicationRecord
 
   private
 
+  def sanitize_message
+    message = details[:custom_message]
+
+    return unless message
+
+    self.details = details.merge(custom_message: Sanitize.clean(message))
+  end
+
   def default_author_value
     ::Gitlab::Audit::NullAuthor.for(author_id, (self[:author_name] || details[:author_name]))
   end
diff --git a/app/models/user.rb b/app/models/user.rb
index 323c1672dd5..91a7a2bc9c3 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -1255,12 +1255,23 @@ class User < ApplicationRecord
   end
 
   def sanitize_attrs
+    sanitize_links
+    sanitize_name
+  end
+
+  def sanitize_links
     %i[skype linkedin twitter].each do |attr|
       value = self[attr]
       self[attr] = Sanitize.clean(value) if value.present?
     end
   end
 
+  def sanitize_name
+    return unless self.name
+
+    self.name = self.name.gsub(%r{</?[^>]*>}, '')
+  end
+
   def set_notification_email
     if notification_email.blank? || all_emails.exclude?(notification_email)
       self.notification_email = email
diff --git a/app/services/feature_flags/base_service.rb b/app/services/feature_flags/base_service.rb
index f48f95e2550..d041703803b 100644
--- a/app/services/feature_flags/base_service.rb
+++ b/app/services/feature_flags/base_service.rb
@@ -49,9 +49,9 @@ module FeatureFlags
     end
 
     def created_scope_message(scope)
-      "Created rule <strong>#{scope.environment_scope}</strong> "\
-      "and set it as <strong>#{scope.active ? "active" : "inactive"}</strong> "\
-      "with strategies <strong>#{scope.strategies}</strong>."
+      "Created rule #{scope.environment_scope} "\
+      "and set it as #{scope.active ? "active" : "inactive"} "\
+      "with strategies #{scope.strategies}."
     end
 
     def feature_flag_by_name
diff --git a/app/services/feature_flags/create_service.rb b/app/services/feature_flags/create_service.rb
index de3a55d10fc..5c87af561d5 100644
--- a/app/services/feature_flags/create_service.rb
+++ b/app/services/feature_flags/create_service.rb
@@ -22,8 +22,7 @@ module FeatureFlags
     private
 
     def audit_message(feature_flag)
-      message_parts = ["Created feature flag <strong>#{feature_flag.name}</strong>",
-                       "with description <strong>\"#{feature_flag.description}\"</strong>."]
+      message_parts = ["Created feature flag #{feature_flag.name} with description \"#{feature_flag.description}\"."]
 
       message_parts += feature_flag.scopes.map do |scope|
         created_scope_message(scope)
diff --git a/app/services/feature_flags/destroy_service.rb b/app/services/feature_flags/destroy_service.rb
index c77e3e03ec3..b131a349fc7 100644
--- a/app/services/feature_flags/destroy_service.rb
+++ b/app/services/feature_flags/destroy_service.rb
@@ -23,7 +23,7 @@ module FeatureFlags
     end
 
     def audit_message(feature_flag)
-      "Deleted feature flag <strong>#{feature_flag.name}</strong>."
+      "Deleted feature flag #{feature_flag.name}."
     end
 
     def can_destroy?(feature_flag)
diff --git a/app/services/feature_flags/update_service.rb b/app/services/feature_flags/update_service.rb
index d956d4b3357..f5ab6f4005b 100644
--- a/app/services/feature_flags/update_service.rb
+++ b/app/services/feature_flags/update_service.rb
@@ -45,14 +45,14 @@ module FeatureFlags
 
       return if changes.empty?
 
-      "Updated feature flag <strong>#{feature_flag.name}</strong>. " + changes.join(" ")
+      "Updated feature flag #{feature_flag.name}. " + changes.join(" ")
     end
 
     def changed_attributes_messages(feature_flag)
       feature_flag.changes.slice(*AUDITABLE_ATTRIBUTES).map do |attribute_name, changes|
         "Updated #{attribute_name} "\
-        "from <strong>\"#{changes.first}\"</strong> to "\
-        "<strong>\"#{changes.second}\"</strong>."
+        "from \"#{changes.first}\" to "\
+        "\"#{changes.second}\"."
       end
     end
 
@@ -69,17 +69,17 @@ module FeatureFlags
     end
 
     def deleted_scope_message(scope)
-      "Deleted rule <strong>#{scope.environment_scope}</strong>."
+      "Deleted rule #{scope.environment_scope}."
     end
 
     def updated_scope_message(scope)
       changes = scope.changes.slice(*AUDITABLE_SCOPE_ATTRIBUTES_HUMAN_NAMES.keys)
       return if changes.empty?
 
-      message = "Updated rule <strong>#{scope.environment_scope}</strong> "
+      message = "Updated rule #{scope.environment_scope} "
       message += changes.map do |attribute_name, change|
         name = AUDITABLE_SCOPE_ATTRIBUTES_HUMAN_NAMES[attribute_name]
-        "#{name} from <strong>#{change.first}</strong> to <strong>#{change.second}</strong>"
+        "#{name} from #{change.first} to #{change.second}"
       end.join(' ')
 
       message + '.'
diff --git a/spec/features/snippets/notes_on_personal_snippets_spec.rb b/spec/features/snippets/notes_on_personal_snippets_spec.rb
index 47dad9bd88e..e03f71c5352 100644
--- a/spec/features/snippets/notes_on_personal_snippets_spec.rb
+++ b/spec/features/snippets/notes_on_personal_snippets_spec.rb
@@ -65,18 +65,6 @@ RSpec.describe 'Comments on personal snippets', :js do
         expect(page).to have_content(user_name)
       end
     end
-
-    context 'when the author name contains HTML' do
-      let(:user_name) { '<h1><a href="https://bad.link/malicious.exe" class="evil">Fake Content<img class="fake-icon" src="image.png"></a></h1>' }
-
-      it 'renders the name as plain text' do
-        visit snippet_path(snippet)
-
-        content = find("#note_#{snippet_notes[0].id} .note-header-author-name").text
-
-        expect(content).to eq user_name
-      end
-    end
   end
 
   context 'when submitting a note' do
diff --git a/spec/frontend/lib/utils/url_utility_spec.js b/spec/frontend/lib/utils/url_utility_spec.js
index e12cd8b0e37..3b2852d08e6 100644
--- a/spec/frontend/lib/utils/url_utility_spec.js
+++ b/spec/frontend/lib/utils/url_utility_spec.js
@@ -1,3 +1,4 @@
+import { TEST_HOST } from 'helpers/test_constants';
 import * as urlUtils from '~/lib/utils/url_utility';
 
 const shas = {
@@ -921,4 +922,37 @@ describe('URL utility', () => {
       expect(urlUtils.encodeSaferUrl(input)).toBe(input);
     });
   });
+
+  describe('isSameOriginUrl', () => {
+    // eslint-disable-next-line no-script-url
+    const javascriptUrl = 'javascript:alert(1)';
+
+    beforeEach(() => {
+      setWindowLocation({ origin: TEST_HOST });
+    });
+
+    it.each`
+      url                                | expected
+      ${TEST_HOST}                       | ${true}
+      ${`${TEST_HOST}/a/path`}           | ${true}
+      ${'//test.host/no-protocol'}       | ${true}
+      ${'/a/root/relative/path'}         | ${true}
+      ${'a/relative/path'}               | ${true}
+      ${'#hash'}                         | ${true}
+      ${'?param=foo'}                    | ${true}
+      ${''}                              | ${true}
+      ${'../../../'}                     | ${true}
+      ${`${TEST_HOST}:8080/wrong-port`}  | ${false}
+      ${'ws://test.host/wrong-protocol'} | ${false}
+      ${'http://phishing.test'}          | ${false}
+      ${'//phishing.test'}               | ${false}
+      ${'//invalid:url'}                 | ${false}
+      ${javascriptUrl}                   | ${false}
+      ${'data:,Hello%2C%20World%21'}     | ${false}
+      ${null}                            | ${false}
+      ${undefined}                       | ${false}
+    `('returns $expected given $url', ({ url, expected }) => {
+      expect(urlUtils.isSameOriginUrl(url)).toBe(expected);
+    });
+  });
 });
diff --git a/spec/frontend/releases/components/app_edit_new_spec.js b/spec/frontend/releases/components/app_edit_new_spec.js
index 65ed6d6166f..748b48dacaa 100644
--- a/spec/frontend/releases/components/app_edit_new_spec.js
+++ b/spec/frontend/releases/components/app_edit_new_spec.js
@@ -4,6 +4,7 @@ import MockAdapter from 'axios-mock-adapter';
 import { merge } from 'lodash';
 import Vuex from 'vuex';
 import { getJSONFixture } from 'helpers/fixtures';
+import { TEST_HOST } from 'helpers/test_constants';
 import * as commonUtils from '~/lib/utils/common_utils';
 import ReleaseEditNewApp from '~/releases/components/app_edit_new.vue';
 import AssetLinksForm from '~/releases/components/asset_links_form.vue';
@@ -11,6 +12,7 @@ import { BACK_URL_PARAM } from '~/releases/constants';
 
 const originalRelease = getJSONFixture('api/releases/release.json');
 const originalMilestones = originalRelease.milestones;
+const releasesPagePath = 'path/to/releases/page';
 
 describe('Release edit/new component', () => {
   let wrapper;
@@ -24,7 +26,7 @@ describe('Release edit/new component', () => {
     state = {
       release,
       markdownDocsPath: 'path/to/markdown/docs',
-      releasesPagePath: 'path/to/releases/page',
+      releasesPagePath,
       projectId: '8',
       groupId: '42',
       groupMilestonesAvailable: true,
@@ -75,6 +77,8 @@ describe('Release edit/new component', () => {
   };
 
   beforeEach(() => {
+    global.jsdom.reconfigure({ url: TEST_HOST });
+
     mock = new MockAdapter(axios);
     gon.api_version = 'v4';
 
@@ -146,22 +150,33 @@ describe('Release edit/new component', () => {
     });
   });
 
-  describe(`when the URL contains a "${BACK_URL_PARAM}" parameter`, () => {
-    const backUrl = 'https://example.gitlab.com/back/url';
-
-    beforeEach(async () => {
-      commonUtils.getParameterByName = jest
-        .fn()
-        .mockImplementation((paramToGet) => ({ [BACK_URL_PARAM]: backUrl }[paramToGet]));
+  // eslint-disable-next-line no-script-url
+  const xssBackUrl = 'javascript:alert(1)';
+  describe.each`
+    backUrl                            | expectedHref
+    ${`${TEST_HOST}/back/url`}         | ${`${TEST_HOST}/back/url`}
+    ${`/back/url?page=2`}              | ${`/back/url?page=2`}
+    ${`back/url?page=3`}               | ${`back/url?page=3`}
+    ${'http://phishing.test/back/url'} | ${releasesPagePath}
+    ${'//phishing.test/back/url'}      | ${releasesPagePath}
+    ${xssBackUrl}                      | ${releasesPagePath}
+  `(
+    `when the URL contains a "${BACK_URL_PARAM}=$backUrl" parameter`,
+    ({ backUrl, expectedHref }) => {
+      beforeEach(async () => {
+        global.jsdom.reconfigure({
+          url: `${TEST_HOST}?${BACK_URL_PARAM}=${encodeURIComponent(backUrl)}`,
+        });
 
-      await factory();
-    });
+        await factory();
+      });
 
-    it('renders a "Cancel" button with an href pointing to the main Releases page', () => {
-      const cancelButton = wrapper.find('.js-cancel-button');
-      expect(cancelButton.attributes().href).toBe(backUrl);
-    });
-  });
+      it(`renders a "Cancel" button with an href pointing to ${expectedHref}`, () => {
+        const cancelButton = wrapper.find('.js-cancel-button');
+        expect(cancelButton.attributes().href).toBe(expectedHref);
+      });
+    },
+  );
 
   describe('when creating a new release', () => {
     beforeEach(async () => {
diff --git a/spec/models/audit_event_spec.rb b/spec/models/audit_event_spec.rb
index 5c87c2e68db..bc603bc5ab6 100644
--- a/spec/models/audit_event_spec.rb
+++ b/spec/models/audit_event_spec.rb
@@ -3,9 +3,6 @@
 require 'spec_helper'
 
 RSpec.describe AuditEvent do
-  let_it_be(:audit_event) { create(:project_audit_event) }
-  subject { audit_event }
-
   describe 'validations' do
     include_examples 'validates IP address' do
       let(:attribute) { :ip_address }
@@ -13,6 +10,15 @@ RSpec.describe AuditEvent do
     end
   end
 
+  it 'sanitizes custom_message in the details hash' do
+    audit_event = create(:project_audit_event, details: { target_id: 678, custom_message: '<strong>Arnold</strong>' })
+
+    expect(audit_event.details).to include(
+      target_id: 678,
+      custom_message: 'Arnold'
+    )
+  end
+
   describe '#as_json' do
     context 'ip_address' do
       subject { build(:group_audit_event, ip_address: '192.168.1.1').as_json }
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 05610057363..2cd8d52a7c1 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -2877,7 +2877,7 @@ RSpec.describe User do
   end
 
   describe '#sanitize_attrs' do
-    let(:user) { build(:user, name: 'test & user', skype: 'test&user') }
+    let(:user) { build(:user, name: 'test <& user', skype: 'test&user') }
 
     it 'encodes HTML entities in the Skype attribute' do
       expect { user.sanitize_attrs }.to change { user.skype }.to('test&amp;user')
@@ -2886,6 +2886,25 @@ RSpec.describe User do
     it 'does not encode HTML entities in the name attribute' do
       expect { user.sanitize_attrs }.not_to change { user.name }
     end
+
+    it 'sanitizes attr from html tags' do
+      user = create(:user, name: '<a href="//example.com">Test<a>', twitter: '<a href="//evil.com">https://twitter.com<a>')
+
+      expect(user.name).to eq('Test')
+      expect(user.twitter).to eq('https://twitter.com')
+    end
+
+    it 'sanitizes attr from js scripts' do
+      user = create(:user, name: '<script>alert("Test")</script>')
+
+      expect(user.name).to eq("alert(\"Test\")")
+    end
+
+    it 'sanitizes attr from iframe scripts' do
+      user = create(:user, name: 'User"><iframe src=javascript:alert()><iframe>')
+
+      expect(user.name).to eq('User">')
+    end
   end
 
   describe '#starred?' do
diff --git a/spec/services/feature_flags/create_service_spec.rb b/spec/services/feature_flags/create_service_spec.rb
index 2e0c162ebc1..4eb2b25fb64 100644
--- a/spec/services/feature_flags/create_service_spec.rb
+++ b/spec/services/feature_flags/create_service_spec.rb
@@ -68,12 +68,12 @@ RSpec.describe FeatureFlags::CreateService do
       end
 
       it 'creates audit event' do
-        expected_message = 'Created feature flag <strong>feature_flag</strong> '\
-                           'with description <strong>"description"</strong>. '\
-                           'Created rule <strong>*</strong> and set it as <strong>active</strong> '\
-                           'with strategies <strong>[{"name"=>"default", "parameters"=>{}}]</strong>. '\
-                           'Created rule <strong>production</strong> and set it as <strong>inactive</strong> '\
-                           'with strategies <strong>[{"name"=>"default", "parameters"=>{}}]</strong>.'
+        expected_message = 'Created feature flag feature_flag '\
+                           'with description "description". '\
+                           'Created rule * and set it as active '\
+                           'with strategies [{"name"=&gt;"default", "parameters"=&gt;{}}]. '\
+                           'Created rule production and set it as inactive '\
+                           'with strategies [{"name"=&gt;"default", "parameters"=&gt;{}}].'
 
         expect { subject }.to change { AuditEvent.count }.by(1)
         expect(AuditEvent.last.details[:custom_message]).to eq(expected_message)
diff --git a/spec/services/feature_flags/destroy_service_spec.rb b/spec/services/feature_flags/destroy_service_spec.rb
index ee30474873c..d3796ef6b4d 100644
--- a/spec/services/feature_flags/destroy_service_spec.rb
+++ b/spec/services/feature_flags/destroy_service_spec.rb
@@ -33,7 +33,7 @@ RSpec.describe FeatureFlags::DestroyService do
 
     it 'creates audit log' do
       expect { subject }.to change { AuditEvent.count }.by(1)
-      expect(audit_event_message).to eq("Deleted feature flag <strong>#{feature_flag.name}</strong>.")
+      expect(audit_event_message).to eq("Deleted feature flag #{feature_flag.name}.")
     end
 
     context 'when user is reporter' do
diff --git a/spec/services/feature_flags/update_service_spec.rb b/spec/services/feature_flags/update_service_spec.rb
index 1a127a0d472..a9e9ea78d42 100644
--- a/spec/services/feature_flags/update_service_spec.rb
+++ b/spec/services/feature_flags/update_service_spec.rb
@@ -38,9 +38,9 @@ RSpec.describe FeatureFlags::UpdateService do
 
       expect { subject }.to change { AuditEvent.count }.by(1)
       expect(audit_event_message).to(
-        eq("Updated feature flag <strong>new_name</strong>. "\
-           "Updated name from <strong>\"#{name_was}\"</strong> "\
-           "to <strong>\"new_name\"</strong>.")
+        eq("Updated feature flag new_name. "\
+           "Updated name from \"#{name_was}\" "\
+           "to \"new_name\".")
       )
     end
 
@@ -94,8 +94,8 @@ RSpec.describe FeatureFlags::UpdateService do
       it 'creates audit event with changed description' do
         expect { subject }.to change { AuditEvent.count }.by(1)
         expect(audit_event_message).to(
-          include("Updated description from <strong>\"\"</strong>"\
-                  " to <strong>\"new description\"</strong>.")
+          include("Updated description from \"\""\
+                  " to \"new description\".")
         )
       end
     end
@@ -110,7 +110,7 @@ RSpec.describe FeatureFlags::UpdateService do
       it 'creates audit event about changing active state' do
         expect { subject }.to change { AuditEvent.count }.by(1)
         expect(audit_event_message).to(
-          include('Updated active from <strong>"true"</strong> to <strong>"false"</strong>.')
+          include('Updated active from "true" to "false".')
         )
       end
 
@@ -132,8 +132,8 @@ RSpec.describe FeatureFlags::UpdateService do
       it 'creates audit event about changing active state' do
         expect { subject }.to change { AuditEvent.count }.by(1)
         expect(audit_event_message).to(
-          include("Updated rule <strong>*</strong> active state "\
-                  "from <strong>true</strong> to <strong>false</strong>.")
+          include("Updated rule * active state "\
+                  "from true to false.")
         )
       end
     end
@@ -149,8 +149,8 @@ RSpec.describe FeatureFlags::UpdateService do
       it 'creates audit event with changed name' do
         expect { subject }.to change { AuditEvent.count }.by(1)
         expect(audit_event_message).to(
-          include("Updated rule <strong>staging</strong> environment scope "\
-                  "from <strong>review</strong> to <strong>staging</strong>.")
+          include("Updated rule staging environment scope "\
+                  "from review to staging.")
         )
       end
 
@@ -185,7 +185,7 @@ RSpec.describe FeatureFlags::UpdateService do
 
       it 'creates audit event with deleted scope' do
         expect { subject }.to change { AuditEvent.count }.by(1)
-        expect(audit_event_message).to include("Deleted rule <strong>review</strong>.")
+        expect(audit_event_message).to include("Deleted rule review.")
       end
 
       context 'when scope can not be deleted' do
@@ -210,8 +210,8 @@ RSpec.describe FeatureFlags::UpdateService do
       end
 
       it 'creates audit event with new scope' do
-        expected = 'Created rule <strong>review</strong> and set it as <strong>active</strong> '\
-                   'with strategies <strong>[{"name"=>"default", "parameters"=>{}}]</strong>.'
+        expected = 'Created rule review and set it as active '\
+                   'with strategies [{"name"=&gt;"default", "parameters"=&gt;{}}].'
 
         subject
 
@@ -260,7 +260,7 @@ RSpec.describe FeatureFlags::UpdateService do
       end
 
       it 'creates an audit event' do
-        expected = %r{Updated rule <strong>sandbox</strong> strategies from <strong>.*</strong> to <strong>.*</strong>.}
+        expected = %r{Updated rule sandbox strategies from .* to .*.}
 
         expect { subject }.to change { AuditEvent.count }.by(1)
         expect(audit_event_message).to match(expected)