Merge remote-tracking branch 'dev/12-5-stable' into 12-5-stable

CHANGELOG.md

@@ -2,6 +2,20 @@
 documentation](doc/development/changelog.md) for instructions on adding your own
 entry.
+## 12.5.1
+
+### Security (8 changes)
+
+- Check permissions before showing a forked project's source.
+- Encrypt application setting tokens.
+- Update Workhorse and Gitaly to fix a security issue.
+- Hide commit counts from guest users in Cycle Analytics.
+- Limit potential for DNS rebind SSRF in chat notifications.
+- Ensure are cleaned by ImportExport::AttributeCleaner.
+- Remove notes regarding Related Branches from Issue activity feeds for guest users.
+- Escape namespace in label references to prevent XSS.
+
+
 ## 12.5.0
 ### Security (15 changes)

GITALY_SERVER_VERSION

-1.72.0
+1.72.1

GITLAB_WORKHORSE_VERSION

-8.14.0
+8.14.1

VERSION

-12.5.0-ee
+12.5.1

app/assets/javascripts/cycle_analytics/cycle_analytics_bundle.js

@@ -58,10 +58,16 @@ export default () => {
         service: this.createCycleAnalyticsService(cycleAnalyticsEl.dataset.requestPath),
       };
     },
+    defaultNumberOfSummaryItems: 3,
     computed: {
       currentStage() {
         return this.store.currentActiveStage();
       },
+      summaryTableColumnClass() {
+        return this.state.summary.length === this.$options.defaultNumberOfSummaryItems
+          ? 'col-sm-3'
+          : 'col-sm-4';
+      },
     },
     created() {
       // Conditional check placed here to prevent this method from being called on the

app/controllers/admin/application_settings_controller.rb

@@ -86,6 +86,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
     params[:application_setting][:import_sources]&.delete("")
     params[:application_setting][:restricted_visibility_levels]&.delete("")
+    params[:application_setting].delete(:elasticsearch_aws_secret_access_key) if params[:application_setting][:elasticsearch_aws_secret_access_key].blank?
     # TODO Remove domain_blacklist_raw in APIv5 (See https://gitlab.com/gitlab-org/gitlab-foss/issues/67204)
     params.delete(:domain_blacklist_raw) if params[:domain_blacklist_file]
     params.delete(:domain_blacklist_raw) if params[:domain_blacklist]

app/helpers/projects_helper.rb

@@ -110,19 +110,26 @@ module ProjectsHelper
       { project_full_name: project.full_name }
   end
-  def remove_fork_project_message(project)
-    _("You are going to remove the fork relationship to source project %{forked_from_project}. Are you ABSOLUTELY sure?") %
-      { forked_from_project: fork_source_name(project) }
-  end
-  def fork_source_name(project)
-    if @project.fork_source
-      @project.fork_source.full_name
+  def remove_fork_project_description_message(project)
+    source = visible_fork_source(project)
+    if source
+      _('This will remove the fork relationship between this project and %{fork_source}.') %
+        { fork_source: link_to(source.full_name, project_path(source)) }
     else
-      @project.fork_network&.deleted_root_project_name
+      _('This will remove the fork relationship between this project and other projects in the fork network.')
     end
   end
+  def remove_fork_project_warning_message(project)
+    _("You are going to remove the fork relationship from %{project_full_name}. Are you ABSOLUTELY sure?") %
+      { project_full_name: project.full_name }
+  end
+
+  def visible_fork_source(project)
+    project.fork_source if project.fork_source && can?(current_user, :read_project, project.fork_source)
+  end
+
   def project_nav_tabs
     @nav_tabs ||= get_project_nav_tabs(@project, current_user)
   end

app/models/application_setting.rb

@@ -313,29 +313,25 @@ class ApplicationSetting < ApplicationRecord
                  algorithm: 'aes-256-cbc',
                  insecure_mode: true
-  attr_encrypted :external_auth_client_key,
-                 mode: :per_attribute_iv,
-                 key: Settings.attr_encrypted_db_key_base_truncated,
-                 algorithm: 'aes-256-gcm',
-                 encode: true
-
-  attr_encrypted :external_auth_client_key_pass,
-                 mode: :per_attribute_iv,
-                 key: Settings.attr_encrypted_db_key_base_truncated,
-                 algorithm: 'aes-256-gcm',
-                 encode: true
-
-  attr_encrypted :lets_encrypt_private_key,
-                 mode: :per_attribute_iv,
-                 key: Settings.attr_encrypted_db_key_base_truncated,
-                 algorithm: 'aes-256-gcm',
-                 encode: true
-  attr_encrypted :eks_secret_access_key,
-                 mode: :per_attribute_iv,
-                 key: Settings.attr_encrypted_db_key_base_truncated,
-                 algorithm: 'aes-256-gcm',
-                 encode: true
+  private_class_method def self.encryption_options_base_truncated_aes_256_gcm
+    {
+      mode: :per_attribute_iv,
+      key: Settings.attr_encrypted_db_key_base_truncated,
+      algorithm: 'aes-256-gcm',
+      encode: true
+    }
+  end
+  attr_encrypted :external_auth_client_key, encryption_options_base_truncated_aes_256_gcm
+  attr_encrypted :external_auth_client_key_pass, encryption_options_base_truncated_aes_256_gcm
+  attr_encrypted :lets_encrypt_private_key, encryption_options_base_truncated_aes_256_gcm
+  attr_encrypted :eks_secret_access_key, encryption_options_base_truncated_aes_256_gcm
+  attr_encrypted :akismet_api_key, encryption_options_base_truncated_aes_256_gcm
+  attr_encrypted :elasticsearch_aws_secret_access_key, encryption_options_base_truncated_aes_256_gcm
+  attr_encrypted :recaptcha_private_key, encryption_options_base_truncated_aes_256_gcm
+  attr_encrypted :recaptcha_site_key, encryption_options_base_truncated_aes_256_gcm
+  attr_encrypted :slack_app_secret, encryption_options_base_truncated_aes_256_gcm
+  attr_encrypted :slack_app_verification_token, encryption_options_base_truncated_aes_256_gcm
   before_validation :ensure_uuid!

app/models/note.rb

@@ -37,6 +37,10 @@ class Note < ApplicationRecord
   redact_field :note
+  TYPES_RESTRICTED_BY_ABILITY = {
+    branch: :download_code
+  }.freeze
+
   # Aliases to make application_helper#edited_time_ago_with_tooltip helper work properly with notes.
   # See https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/10392/diffs#note_28719102
   alias_attribute :last_edited_at, :updated_at
@@ -341,7 +345,7 @@ class Note < ApplicationRecord
   end
   def visible_for?(user)
-    !cross_reference_not_visible_for?(user)
+    !cross_reference_not_visible_for?(user) && system_note_viewable_by?(user)
   end
   def award_emoji?
@@ -493,6 +497,15 @@ class Note < ApplicationRecord
   private
+  def system_note_viewable_by?(user)
+    return true unless system_note_metadata
+
+    restriction = TYPES_RESTRICTED_BY_ABILITY[system_note_metadata.action.to_sym]
+    return Ability.allowed?(user, restriction, project) if restriction
+
+    true
+  end
+
   def keep_around_commit
     project.repository.keep_around(self.commit_id)
   end

app/models/project.rb

@@ -517,7 +517,11 @@ class Project < ApplicationRecord
   # This scope returns projects where user has access to both the project and the feature.
   def self.filter_by_feature_visibility(feature, user)
-    with_feature_available_for_user(feature, user).public_or_visible_to_user(user)
+    with_feature_available_for_user(feature, user)
+      .public_or_visible_to_user(
+        user,
+        ProjectFeature.required_minimum_access_level_for_private_project(feature)
+      )
   end
   scope :active, -> { joins(:issues, :notes, :merge_requests).order('issues.created_at, notes.created_at, merge_requests.created_at DESC') }

app/models/project_feature.rb

@@ -24,6 +24,7 @@ class ProjectFeature < ApplicationRecord
   FEATURES = %i(issues merge_requests wiki snippets builds repository pages).freeze
   PRIVATE_FEATURES_MIN_ACCESS_LEVEL = { merge_requests: Gitlab::Access::REPORTER }.freeze
+  PRIVATE_FEATURES_MIN_ACCESS_LEVEL_FOR_PRIVATE_PROJECT = { repository: Gitlab::Access::REPORTER }.freeze
   STRING_OPTIONS = HashWithIndifferentAccess.new({
     'disabled' => DISABLED,
     'private'  => PRIVATE,
@@ -51,6 +52,15 @@ class ProjectFeature < ApplicationRecord
       PRIVATE_FEATURES_MIN_ACCESS_LEVEL.fetch(feature, Gitlab::Access::GUEST)
     end
+    # Guest users can perform certain features on public and internal projects, but not private projects.
+    def required_minimum_access_level_for_private_project(feature)
+      feature = ensure_feature!(feature)
+
+      PRIVATE_FEATURES_MIN_ACCESS_LEVEL_FOR_PRIVATE_PROJECT.fetch(feature) do
+        required_minimum_access_level(feature)
+      end
+    end
+
     def access_level_from_str(level)
       STRING_OPTIONS.fetch(level)
     end

app/models/project_services/chat_notification_service.rb

@@ -113,12 +113,9 @@ class ChatNotificationService < Service
   private
+  # every notifier must implement this independently
   def notify(message, opts)
-    Slack::Notifier.new(webhook, opts).ping(
-      message.pretext,
-      attachments: message.attachments,
-      fallback: message.fallback
-    )
+    raise NotImplementedError
   end
   def custom_data(data)

app/models/project_services/mattermost_service.rb

 # frozen_string_literal: true
 class MattermostService < ChatNotificationService
+  include ::SlackService::Notifier
+
   def title
     'Mattermost notifications'
   end

app/models/project_services/slack_service.rb

@@ -30,4 +30,28 @@ class SlackService < ChatNotificationService
   def webhook_placeholder
     'https://hooks.slack.com/services/…'
   end
+
+  module Notifier
+    private
+
+    def notify(message, opts)
+      # See https://github.com/stevenosloan/slack-notifier#custom-http-client
+      notifier = Slack::Notifier.new(webhook, opts.merge(http_client: HTTPClient))
+
+      notifier.ping(
+        message.pretext,
+        attachments: message.attachments,
+        fallback: message.fallback
+      )
+    end
+
+    class HTTPClient
+      def self.post(uri, params = {})
+        params.delete(:http_options) # these are internal to the client and we do not want them
+        Gitlab::HTTP.post(uri, body: params)
+      end
+    end
+  end
+
+  include Notifier
 end

app/views/projects/_home_panel.html.haml

@@ -74,13 +74,12 @@
     - if @project.forked?
       %p
-        - if @project.fork_source
+        - source = visible_fork_source(@project)
+        - if source
           #{ s_('ForkedFromProjectPath|Forked from') }
-          = link_to project_path(@project.fork_source) do
-            = fork_source_name(@project)
+          = link_to source.full_name, project_path(source)
         - else
-          - deleted_message = s_('ForkedFromProjectPath|Forked from %{project_name} (deleted)')
-          = deleted_message % { project_name: fork_source_name(@project) }
+          = s_('ForkedFromProjectPath|Forked from an inaccessible project')
     = render_if_exists "projects/home_mirror"

app/views/projects/cycle_analytics/show.html.haml

@@ -13,10 +13,10 @@
       .content-block
         .container-fluid
           .row
-            .col-sm-3.col-12.column{ "v-for" => "item in state.summary" }
+            .col-12.column{ "v-for" => "item in state.summary", ":class" => "summaryTableColumnClass" }
               %h3.header {{ item.value }}
               %p.text {{ item.title }}
-            .col-sm-3.col-12.column
+            .col-12.column{ ":class" => "summaryTableColumnClass" }
               .dropdown.inline.js-ca-dropdown
                 %button.dropdown-menu-toggle{ "data-toggle" => "dropdown", :type => "button" }
                   %span.dropdown-label {{ n__('Last %d day', 'Last %d days', 30) }}

app/views/projects/edit.html.haml

@@ -126,17 +126,12 @@
     - if @project.forked? && can?(current_user, :remove_fork_project, @project)
       .sub-section
         %h4.danger-title= _('Remove fork relationship')
-        %p
-          = _('This will remove the fork relationship to source project')
-          = succeed "." do
-            - if @project.fork_source
-              = link_to(fork_source_name(@project), project_path(@project.fork_source))
-            - else
-              = fork_source_name(@project)
+        %p= remove_fork_project_description_message(@project)
+
         = form_for([@project.namespace.becomes(Namespace), @project], url: remove_fork_project_path(@project), method: :delete, remote: true, html: { class: 'transfer-project' }) do |f|
           %p
             %strong= _('Once removed, the fork relationship cannot be restored and you will no longer be able to send merge requests to the source.')
-          = button_to _('Remove fork relationship'), '#', class: "btn btn-remove js-confirm-danger", data: { "confirm-danger-message" => remove_fork_project_message(@project) }
+          = button_to _('Remove fork relationship'), '#', class: "btn btn-remove js-confirm-danger", data: { "confirm-danger-message" => remove_fork_project_warning_message(@project) }
     - if can?(current_user, :remove_project, @project)
       .sub-section

changelogs/unreleased/security-dos-issue-and-commit-comments-master.yml

+---
+title: Fix 500 error caused by invalid byte sequences in links
+merge_request:
+author:
+type: security

config/initializers/hangouts_chat_http_override.rb

+# frozen_string_literal: true
+
+module HangoutsChat
+  class Sender
+    class HTTP
+      module GitlabHTTPOverride
+        extend ::Gitlab::Utils::Override
+
+        attr_reader :uri
+
+        # see https://github.com/enzinia/hangouts-chat/blob/6a509f61a56e757f8f417578b393b94423831ff7/lib/hangouts_chat/http.rb
+        override :post
+        def post(payload)
+          httparty_response = Gitlab::HTTP.post(
+            uri,
+            body: payload.to_json,
+            headers: { 'Content-Type' => 'application/json' },
+            parse: nil # disables automatic response parsing
+          )
+          net_http_response = httparty_response.response
+          # The rest of the integration expects a Net::HTTP response
+          net_http_response
+        end
+      end
+
+      prepend GitlabHTTPOverride
+    end
+  end
+end

db/migrate/20191120084627_add_encrypted_fields_to_application_settings.rb

+# frozen_string_literal: true
+
+class AddEncryptedFieldsToApplicationSettings < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  PLAINTEXT_ATTRIBUTES = %w[
+    akismet_api_key
+    elasticsearch_aws_secret_access_key
+    recaptcha_private_key
+    recaptcha_site_key
+    slack_app_secret
+    slack_app_verification_token
+  ].freeze
+
+  def up
+    PLAINTEXT_ATTRIBUTES.each do |plaintext_attribute|
+      add_column :application_settings, "encrypted_#{plaintext_attribute}", :text
+      add_column :application_settings, "encrypted_#{plaintext_attribute}_iv", :string, limit: 255
+    end
+  end
+
+  def down
+    PLAINTEXT_ATTRIBUTES.each do |plaintext_attribute|
+      remove_column :application_settings, "encrypted_#{plaintext_attribute}"
+      remove_column :application_settings, "encrypted_#{plaintext_attribute}_iv"
+    end
+  end
+end