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

23 files changed, 268 insertions, 20 deletions

diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_rebase.vue b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_rebase.vue
index 57be97855e3..b1fb377e47a 100644
--- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_rebase.vue
+++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_rebase.vue
@@ -1,5 +1,6 @@
 <script>
 import { GlLoadingIcon } from '@gitlab/ui';
+import { escape } from 'lodash';
 import simplePoll from '../../../lib/utils/simple_poll';
 import eventHub from '../../event_hub';
 import statusIcon from '../mr_widget_status_icon.vue';
@@ -44,11 +45,10 @@ export default {
     fastForwardMergeText() {
       return sprintf(
         __(
-          `Fast-forward merge is not possible. Rebase the source branch onto %{startTag}${this.mr.targetBranch}%{endTag} to allow this merge request to be merged.`,
+          'Fast-forward merge is not possible. Rebase the source branch onto %{targetBranch} to allow this merge request to be merged.',
         ),
         {
-          startTag: '<span class="label-branch">',
-          endTag: '</span>',
+          targetBranch: `<span class="label-branch">${escape(this.mr.targetBranch)}</span>`,
         },
         false,
       );
diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index 456b6430088..3ce449379b8 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -6,6 +6,9 @@ class ApplicationSetting < ApplicationRecord
   include TokenAuthenticatable
   include ChronicDurationAttribute
 
+  GRAFANA_URL_ERROR_MESSAGE = 'Please check your Grafana URL setting in ' \
+    'Admin Area > Settings > Metrics and profiling > Metrics - Grafana'
+
   add_authentication_token_field :runners_registration_token, encrypted: -> { Feature.enabled?(:application_settings_tokens_optional_encryption, default_enabled: true) ? :optional : :required }
   add_authentication_token_field :health_check_access_token
   add_authentication_token_field :static_objects_external_storage_auth_token
@@ -35,6 +38,14 @@ class ApplicationSetting < ApplicationRecord
 
   chronic_duration_attr_writer :archive_builds_in_human_readable, :archive_builds_in_seconds
 
+  validates :grafana_url,
+            system_hook_url: {
+              blocked_message: "is blocked: %{exception_message}. " + GRAFANA_URL_ERROR_MESSAGE
+            },
+            if: :grafana_url_absolute?
+
+  validate :validate_grafana_url
+
   validates :uuid, presence: true
 
   validates :outbound_local_requests_whitelist,
@@ -345,6 +356,19 @@ class ApplicationSetting < ApplicationRecord
   end
   after_commit :expire_performance_bar_allowed_user_ids_cache, if: -> { previous_changes.key?('performance_bar_allowed_group_id') }
 
+  def validate_grafana_url
+    unless parsed_grafana_url
+      self.errors.add(
+        :grafana_url,
+        "must be a valid relative or absolute URL. #{GRAFANA_URL_ERROR_MESSAGE}"
+      )
+    end
+  end
+
+  def grafana_url_absolute?
+    parsed_grafana_url&.absolute?
+  end
+
   def sourcegraph_url_is_com?
     !!(sourcegraph_url =~ /\Ahttps:\/\/(www\.)?sourcegraph\.com/)
   end
@@ -369,6 +393,12 @@ class ApplicationSetting < ApplicationRecord
   def recaptcha_or_login_protection_enabled
     recaptcha_enabled || login_recaptcha_protection_enabled
   end
+
+  private
+
+  def parsed_grafana_url
+    @parsed_grafana_url ||= Gitlab::Utils.parse_url(grafana_url)
+  end
 end
 
 ApplicationSetting.prepend_if_ee('EE::ApplicationSetting')
diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb
index bdff9e28df1..bc3be67bd32 100644
--- a/app/models/members/group_member.rb
+++ b/app/models/members/group_member.rb
@@ -66,6 +66,7 @@ class GroupMember < Member
 
   def after_accept_invite
     notification_service.accept_group_invite(self)
+    update_two_factor_requirement
 
     super
   end
diff --git a/app/validators/addressable_url_validator.rb b/app/validators/addressable_url_validator.rb
index 300bd01ed22..99f503c3f06 100644
--- a/app/validators/addressable_url_validator.rb
+++ b/app/validators/addressable_url_validator.rb
@@ -23,7 +23,8 @@
 # protect against Server-side Request Forgery (SSRF), or check for the right port.
 #
 # Configuration options:
-# * <tt>message</tt> - A custom error message (default is: "must be a valid URL").
+# * <tt>message</tt> - A custom error message, used when the URL is blank. (default is: "must be a valid URL").
+# * <tt>blocked_message</tt> - A custom error message, used when the URL is blocked. Default: +'is blocked: %{exception_message}'+.
 # * <tt>schemes</tt> - Array of URI schemes. Default: +['http', 'https']+
 # * <tt>allow_localhost</tt> - Allow urls pointing to +localhost+. Default: +true+
 # * <tt>allow_local_network</tt> - Allow urls pointing to private network addresses. Default: +true+
@@ -59,7 +60,8 @@ class AddressableUrlValidator < ActiveModel::EachValidator
   }.freeze
 
   DEFAULT_OPTIONS = BLOCKER_VALIDATE_OPTIONS.merge({
-    message: 'must be a valid URL'
+    message: 'must be a valid URL',
+    blocked_message: 'is blocked: %{exception_message}'
   }).freeze
 
   def initialize(options)
@@ -80,7 +82,7 @@ class AddressableUrlValidator < ActiveModel::EachValidator
 
     Gitlab::UrlBlocker.validate!(value, blocker_args)
   rescue Gitlab::UrlBlocker::BlockedUrlError => e
-    record.errors.add(attribute, "is blocked: #{e.message}")
+    record.errors.add(attribute, options.fetch(:blocked_message) % { exception_message: e.message })
   end
 
   private
diff --git a/app/views/admin/application_settings/_grafana.html.haml b/app/views/admin/application_settings/_grafana.html.haml
index b6e02bde895..700be7db54f 100644
--- a/app/views/admin/application_settings/_grafana.html.haml
+++ b/app/views/admin/application_settings/_grafana.html.haml
@@ -1,4 +1,4 @@
-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-grafana-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: metrics_and_profiling_admin_application_settings_path(anchor: 'js-grafana-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset
diff --git a/app/views/layouts/nav/sidebar/_admin.html.haml b/app/views/layouts/nav/sidebar/_admin.html.haml
index 71fef5df5bc..ed3be516393 100644
--- a/app/views/layouts/nav/sidebar/_admin.html.haml
+++ b/app/views/layouts/nav/sidebar/_admin.html.haml
@@ -83,7 +83,7 @@
                 = _('Requests Profiles')
           - if Gitlab::CurrentSettings.current_application_settings.grafana_enabled?
             = nav_link do
-              = link_to Gitlab::CurrentSettings.current_application_settings.grafana_url, target: '_blank', title: _('Metrics Dashboard') do
+              = link_to Gitlab::CurrentSettings.current_application_settings.grafana_url, target: '_blank', title: _('Metrics Dashboard'), rel: 'noopener noreferrer' do
                 %span
                   = _('Metrics Dashboard')
           = render_if_exists 'layouts/nav/ee/admin/new_monitoring_sidebar'
diff --git a/app/views/shared/issuable/form/_branch_chooser.html.haml b/app/views/shared/issuable/form/_branch_chooser.html.haml
index 29ac17c43b9..8d9e5ddf065 100644
--- a/app/views/shared/issuable/form/_branch_chooser.html.haml
+++ b/app/views/shared/issuable/form/_branch_chooser.html.haml
@@ -8,7 +8,9 @@
 
 .form-group.row.d-flex.gl-pl-3.gl-pr-3.branch-selector
   .align-self-center
-    %span= s_('From %{source_title} into').html_safe % { source_title: "<code>#{source_title}</code>".html_safe }
+    %span
+      = _('From <code>%{source_title}</code> into').html_safe % { source_title: source_title }
+
     - if issuable.new_record?
       %code= target_title
       &nbsp;
diff --git a/changelogs/unreleased/enfoce-group-member-2fa.yml b/changelogs/unreleased/enfoce-group-member-2fa.yml
new file mode 100644
index 00000000000..1e10f678eda
--- /dev/null
+++ b/changelogs/unreleased/enfoce-group-member-2fa.yml
@@ -0,0 +1,5 @@
+---
+title: Update user 2fa when accepting a group invite
+merge_request:
+author:
+type: security
diff --git a/changelogs/unreleased/security-49-xss-branch-names.yml b/changelogs/unreleased/security-49-xss-branch-names.yml
new file mode 100644
index 00000000000..d6ad72aa622
--- /dev/null
+++ b/changelogs/unreleased/security-49-xss-branch-names.yml
@@ -0,0 +1,5 @@
+---
+title: Fix for XSS in branch names
+merge_request:
+author:
+type: security
diff --git a/changelogs/unreleased/security-expire-confirmation-token.yml b/changelogs/unreleased/security-expire-confirmation-token.yml
new file mode 100644
index 00000000000..40d8063c409
--- /dev/null
+++ b/changelogs/unreleased/security-expire-confirmation-token.yml
@@ -0,0 +1,5 @@
+---
+title: Expire account confirmation token
+merge_request:
+author:
+type: security
diff --git a/changelogs/unreleased/security-grafana-stored-xss.yml b/changelogs/unreleased/security-grafana-stored-xss.yml
new file mode 100644
index 00000000000..5a98c6fd7ff
--- /dev/null
+++ b/changelogs/unreleased/security-grafana-stored-xss.yml
@@ -0,0 +1,5 @@
+---
+title: Prevent XSS in admin grafana URL setting
+merge_request:
+author:
+type: security
diff --git a/config/initializers/8_devise.rb b/config/initializers/8_devise.rb
index 8d4c5fa382c..df179f4294e 100644
--- a/config/initializers/8_devise.rb
+++ b/config/initializers/8_devise.rb
@@ -80,8 +80,16 @@ Devise.setup do |config|
   # When allow_unconfirmed_access_for is zero, the user won't be able to sign in without confirming.
   # You can use this to let your user access some features of your application
   # without confirming the account, but blocking it after a certain period
-  # (ie 2 days).
-  config.allow_unconfirmed_access_for = 30.days
+  # (e.g. 3 days).
+  config.allow_unconfirmed_access_for = 3.days
+
+  # A period that the user is allowed to confirm their account before their
+  # token becomes invalid. For example, if set to 1.day, the user can confirm
+  # their account within 1 days after the mail was sent, but on the second day
+  # their account can't be confirmed with the token any more.
+  # Default is nil, meaning there is no restriction on how long a user can take
+  # before confirming their account.
+  config.confirm_within = 1.day
 
   # Defines which key will be used when confirming an account
   # config.confirmation_keys = [ :email ]
diff --git a/db/migrate/20200214085940_clean_grafana_url.rb b/db/migrate/20200214085940_clean_grafana_url.rb
new file mode 100644
index 00000000000..81b7f70f09b
--- /dev/null
+++ b/db/migrate/20200214085940_clean_grafana_url.rb
@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+class CleanGrafanaUrl < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+
+  def up
+    execute(
+      <<-SQL
+      UPDATE
+        application_settings
+      SET
+        grafana_url = default
+      WHERE
+        position('javascript:' IN btrim(application_settings.grafana_url)) = 1
+      SQL
+    )
+  end
+
+  def down
+    # no-op
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 4c1107255a9..f9ee6e36728 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2020_02_04_113223) do
+ActiveRecord::Schema.define(version: 2020_02_14_085940) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_trgm"
diff --git a/doc/administration/monitoring/performance/grafana_configuration.md b/doc/administration/monitoring/performance/grafana_configuration.md
index ccba0a55479..ecede501f5e 100644
--- a/doc/administration/monitoring/performance/grafana_configuration.md
+++ b/doc/administration/monitoring/performance/grafana_configuration.md
@@ -113,8 +113,9 @@ If you have set up Grafana, you can enable a link to access it easily from the s
    and expand "Metrics - Grafana".
 1. Check the "Enable access to Grafana" checkbox.
 1. If Grafana is enabled through Omnibus GitLab and on the same server,
-   leave "Grafana URL" unchanged. In any other case, enter the full URL
-   path of the Grafana instance.
+   leave **Grafana URL** unchanged. It should be `/-/grafana`.
+
+   In any other case, enter the full URL of the Grafana instance.
 1. Click **Save changes**.
 1. The new link will be available in the admin area under **Monitoring > Metrics Dashboard**.
 
diff --git a/lib/gitlab/utils.rb b/lib/gitlab/utils.rb
index 7fbfc4c45c4..85cffa38216 100644
--- a/lib/gitlab/utils.rb
+++ b/lib/gitlab/utils.rb
@@ -130,5 +130,14 @@ module Gitlab
       IPAddr.new(str)
     rescue IPAddr::InvalidAddressError
     end
+
+    # Converts a string to an Addressable::URI object.
+    # If the string is not a valid URI, it returns nil.
+    # Param uri_string should be a String object.
+    # This method returns an Addressable::URI object or nil.
+    def parse_url(uri_string)
+      Addressable::URI.parse(uri_string)
+    rescue Addressable::URI::InvalidURIError, TypeError
+    end
   end
 end
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 40d5e7223d6..6dffcac4c89 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -7562,6 +7562,9 @@ msgstr ""
 msgid "Failure"
 msgstr ""
 
+msgid "Fast-forward merge is not possible. Rebase the source branch onto %{targetBranch} to allow this merge request to be merged."
+msgstr ""
+
 msgid "Fast-forward merge is not possible. Rebase the source branch onto the target branch or merge target branch into source branch to allow this merge request to be merged."
 msgstr ""
 
@@ -7987,7 +7990,7 @@ msgstr ""
 msgid "From %{providerTitle}"
 msgstr ""
 
-msgid "From %{source_title} into"
+msgid "From <code>%{source_title}</code> into"
 msgstr ""
 
 msgid "From Bitbucket"
diff --git a/spec/features/merge_request/user_creates_merge_request_spec.rb b/spec/features/merge_request/user_creates_merge_request_spec.rb
index 67f6d8ebe32..86ee9fa5aa5 100644
--- a/spec/features/merge_request/user_creates_merge_request_spec.rb
+++ b/spec/features/merge_request/user_creates_merge_request_spec.rb
@@ -5,9 +5,9 @@ require "spec_helper"
 describe "User creates a merge request", :js do
   include ProjectForksHelper
 
+  let_it_be(:project) { create(:project, :repository) }
+  let_it_be(:user) { create(:user) }
   let(:title) { "Some feature" }
-  let(:project) { create(:project, :repository) }
-  let(:user) { create(:user) }
 
   before do
     project.add_maintainer(user)
@@ -38,6 +38,26 @@ describe "User creates a merge request", :js do
     end
   end
 
+  context "XSS branch name exists" do
+    before do
+      project.repository.create_branch("<img/src='x'/onerror=alert('oops')>", "master")
+    end
+
+    it "doesn't execute the dodgy branch name" do
+      visit(project_new_merge_request_path(project))
+
+      find(".js-source-branch").click
+      click_link("<img/src='x'/onerror=alert('oops')>")
+
+      find(".js-target-branch").click
+      click_link("feature")
+
+      click_button("Compare branches")
+
+      expect { page.driver.browser.switch_to.alert }.to raise_error(Selenium::WebDriver::Error::NoSuchAlertError)
+    end
+  end
+
   context "to a forked project" do
     let(:forked_project) { fork_project(project, user, namespace: user.namespace, repository: true) }
 
diff --git a/spec/lib/gitlab/utils_spec.rb b/spec/lib/gitlab/utils_spec.rb
index 890918d4a7c..ff44608e8e8 100644
--- a/spec/lib/gitlab/utils_spec.rb
+++ b/spec/lib/gitlab/utils_spec.rb
@@ -252,4 +252,18 @@ describe Gitlab::Utils do
       expect(described_class.string_to_ip_object('1:0:0:0:0:0:0:0/124')).to eq(IPAddr.new('1:0:0:0:0:0:0:0/124'))
     end
   end
+
+  describe '.parse_url' do
+    it 'returns Addressable::URI object' do
+      expect(described_class.parse_url('http://gitlab.com')).to be_instance_of(Addressable::URI)
+    end
+
+    it 'returns nil when URI cannot be parsed' do
+      expect(described_class.parse_url('://gitlab.com')).to be nil
+    end
+
+    it 'returns nil with invalid parameter' do
+      expect(described_class.parse_url(1)).to be nil
+    end
+  end
 end
diff --git a/spec/migrations/clean_grafana_url_spec.rb b/spec/migrations/clean_grafana_url_spec.rb
new file mode 100644
index 00000000000..9f060fbaf7d
--- /dev/null
+++ b/spec/migrations/clean_grafana_url_spec.rb
@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'migrate', '20200214085940_clean_grafana_url.rb')
+
+describe CleanGrafanaUrl, :migration do
+  let(:application_settings_table) { table(:application_settings) }
+
+  [
+    'javascript:alert(window.opener.document.location)',
+    '  javascript:alert(window.opener.document.location)'
+  ].each do |grafana_url|
+    it "sets grafana_url back to its default value when grafana_url is '#{grafana_url}'" do
+      application_settings = application_settings_table.create!(grafana_url: grafana_url)
+
+      migrate!
+
+      expect(application_settings.reload.grafana_url).to eq('/-/grafana')
+    end
+  end
+
+  ['/-/grafana', '/some/relative/url', 'http://localhost:9000'].each do |grafana_url|
+    it "does not modify grafana_url when grafana_url is '#{grafana_url}'" do
+      application_settings = application_settings_table.create!(grafana_url: grafana_url)
+
+      migrate!
+
+      expect(application_settings.reload.grafana_url).to eq(grafana_url)
+    end
+  end
+
+  context 'when application_settings table has no rows' do
+    it 'does not fail' do
+      migrate!
+    end
+  end
+end
diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb
index a403aa296d4..ad2e84cfda2 100644
--- a/spec/models/application_setting_spec.rb
+++ b/spec/models/application_setting_spec.rb
@@ -19,6 +19,7 @@ describe ApplicationSetting do
     let(:http)  { 'http://example.com' }
     let(:https) { 'https://example.com' }
     let(:ftp)   { 'ftp://example.com' }
+    let(:javascript) { 'javascript:alert(window.opener.document.location)' }
 
     it { is_expected.to allow_value(nil).for(:home_page_url) }
     it { is_expected.to allow_value(http).for(:home_page_url) }
@@ -74,6 +75,53 @@ describe ApplicationSetting do
     it { is_expected.not_to allow_value('abc').for(:minimum_password_length) }
     it { is_expected.to allow_value(10).for(:minimum_password_length) }
 
+    context 'grafana_url validations' do
+      before do
+        subject.instance_variable_set(:@parsed_grafana_url, nil)
+      end
+
+      it { is_expected.to allow_value(http).for(:grafana_url) }
+      it { is_expected.to allow_value(https).for(:grafana_url) }
+      it { is_expected.not_to allow_value(ftp).for(:grafana_url) }
+      it { is_expected.not_to allow_value(javascript).for(:grafana_url) }
+      it { is_expected.to allow_value('/-/grafana').for(:grafana_url) }
+      it { is_expected.to allow_value('http://localhost:9000').for(:grafana_url) }
+
+      context 'when local URLs are not allowed in system hooks' do
+        before do
+          stub_application_setting(allow_local_requests_from_system_hooks: false)
+        end
+
+        it { is_expected.not_to allow_value('http://localhost:9000').for(:grafana_url) }
+      end
+
+      context 'with invalid grafana URL' do
+        it 'adds an error' do
+          subject.grafana_url = ' ' + http
+          expect(subject.save).to be false
+
+          expect(subject.errors[:grafana_url]).to eq([
+            'must be a valid relative or absolute URL. ' \
+            'Please check your Grafana URL setting in ' \
+            'Admin Area > Settings > Metrics and profiling > Metrics - Grafana'
+          ])
+        end
+      end
+
+      context 'with blocked grafana URL' do
+        it 'adds an error' do
+          subject.grafana_url = javascript
+          expect(subject.save).to be false
+
+          expect(subject.errors[:grafana_url]).to eq([
+            'is blocked: Only allowed schemes are http, https. Please check your ' \
+            'Grafana URL setting in ' \
+            'Admin Area > Settings > Metrics and profiling > Metrics - Grafana'
+          ])
+        end
+      end
+    end
+
     context 'when snowplow is enabled' do
       before do
         setting.snowplow_enabled = true
diff --git a/spec/models/members/group_member_spec.rb b/spec/models/members/group_member_spec.rb
index ad7dfac87af..9b5cce1aebf 100644
--- a/spec/models/members/group_member_spec.rb
+++ b/spec/models/members/group_member_spec.rb
@@ -65,10 +65,10 @@ describe GroupMember do
   end
 
   describe '#update_two_factor_requirement' do
-    let(:user) { build :user }
-    let(:group_member) { build :group_member, user: user }
-
     it 'is called after creation and deletion' do
+      user = build :user
+      group_member = build :group_member, user: user
+
       expect(user).to receive(:update_two_factor_requirement)
 
       group_member.save
@@ -79,6 +79,21 @@ describe GroupMember do
     end
   end
 
+  describe '#after_accept_invite' do
+    it 'calls #update_two_factor_requirement' do
+      email = 'foo@email.com'
+      user = build(:user, email: email)
+      group = create(:group, require_two_factor_authentication: true)
+      group_member = create(:group_member, group: group, invite_token: '1234', invite_email: email)
+
+      expect(user).to receive(:require_two_factor_authentication_from_group).and_call_original
+
+      group_member.accept_invite!(user)
+
+      expect(user.require_two_factor_authentication_from_group).to be_truthy
+    end
+  end
+
   context 'access levels' do
     context 'with parent group' do
       it_behaves_like 'inherited access level as a member of entity' do
diff --git a/spec/validators/addressable_url_validator_spec.rb b/spec/validators/addressable_url_validator_spec.rb
index e8a44f7a12a..46b1bebb074 100644
--- a/spec/validators/addressable_url_validator_spec.rb
+++ b/spec/validators/addressable_url_validator_spec.rb
@@ -5,6 +5,9 @@ require 'spec_helper'
 describe AddressableUrlValidator do
   let!(:badge) { build(:badge, link_url: 'http://www.example.com') }
 
+  let(:validator) { described_class.new(validator_options.reverse_merge(attributes: [:link_url])) }
+  let(:validator_options) { {} }
+
   subject { validator.validate(badge) }
 
   include_examples 'url validator examples', described_class::DEFAULT_OPTIONS[:schemes]
@@ -114,6 +117,19 @@ describe AddressableUrlValidator do
     end
   end
 
+  context 'when blocked_message is set' do
+    let(:message) { 'is not allowed due to: %{exception_message}' }
+    let(:validator_options) { { blocked_message: message } }
+
+    it 'blocks url with provided error message' do
+      badge.link_url = 'javascript:alert(window.opener.document.location)'
+
+      subject
+
+      expect(badge.errors.first[1]).to eq 'is not allowed due to: Only allowed schemes are http, https'
+    end
+  end
+
   context 'when allow_nil is set to true' do
     let(:validator) { described_class.new(attributes: [:link_url], allow_nil: true) }