Automatic merge of gitlab-org/gitlab master

config/feature_flags/development/loose_index_scan_for_distinct_values.yml

----
-name: loose_index_scan_for_distinct_values
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/55985
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/324210
-milestone: '13.10'
-type: development
-group: group::optimize
-default_enabled: false

config/initializers/wikicloth_disable_lua_patch.rb

+# frozen_string_literal: true
+
+require 'wikicloth'
+require 'wikicloth/extensions/lua'
+
+# Adds patch to disable lua support to eliminate vulnerability to injection attack.
+#
+# The maintainers are not releasing new versions, so we need to patch it here.
+#
+# If they ever do release a version which contains a fix for this, then we can remove this file.
+#
+# See: https://gitlab.com/gitlab-org/gitlab/-/issues/345892#note_751107320
+
+# Guard to ensure we remember to delete this patch if they ever release a new version of wikicloth
+# which disables Lua by default or otherwise eliminates all vulnerabilities mentioned in
+# https://gitlab.com/gitlab-org/gitlab/-/issues/345892, including the possibility of an HTML/JS
+# injection attack as mentioned in https://gitlab.com/gitlab-org/gitlab/-/issues/345892#note_751981608
+unless Gem::Version.new(WikiCloth::VERSION) == Gem::Version.new('0.8.1')
+  raise 'New version of WikiCloth detected, please either update the version for this check, ' \
+    'or remove this patch if no longer needed'
+end
+
+module WikiCloth
+  class LuaExtension < Extension
+    protected
+
+    def init_lua
+      @options[:disable_lua] = true
+    end
+  end
+end

ee/app/assets/javascripts/vulnerabilities/components/vulnerability_details.vue

@@ -6,8 +6,10 @@ import convertReportType from 'ee/vue_shared/security_reports/store/utils/conver
 import { SUPPORTING_MESSAGE_TYPES } from 'ee/vulnerabilities/constants';
 import { s__, __ } from '~/locale';
 import CodeBlock from '~/vue_shared/components/code_block.vue';
+import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
 import DetailItem from './detail_item.vue';
 import VulnerabilityDetailSection from './vulnerability_detail_section.vue';
+import VulnerabilityTraining from './vulnerability_training.vue';
 
 export default {
   name: 'VulnerabilityDetails',
@@ -18,10 +20,12 @@ export default {
     DetailItem,
     GlSprintf,
     VulnerabilityDetailSection,
+    VulnerabilityTraining,
   },
   directives: {
     SafeHtml: GlSafeHtmlDirective,
   },
+  mixins: [glFeatureFlagsMixin()],
   props: {
     vulnerability: {
       type: Object,
@@ -148,6 +152,9 @@ export default {
     hasResponses() {
       return Boolean(this.hasResponse || this.hasRecordedResponse);
     },
+    hasTraining() {
+      return this.glFeatures.secureVulnerabilityTraining && this.vulnerability.identifiers?.length;
+    },
   },
   methods: {
     getHeadersAsCodeBlockLines(headers) {
@@ -373,5 +380,7 @@ export default {
         </li>
       </ul>
     </template>
+
+    <vulnerability-training v-if="hasTraining" :identifiers="vulnerability.identifiers" />
   </div>
 </template>

ee/app/assets/javascripts/vulnerabilities/components/vulnerability_training.vue

+<script>
+import { s__ } from '~/locale';
+import { SUPPORTED_REFERENCE_SCHEMA } from '../constants';
+
+export const i18n = {
+  trainingTitle: s__('Vulnerability|Training'),
+  trainingDescription: s__(
+    'Vulnerability|Learn more about this vulnerability and the best way to resolve it.',
+  ),
+  trainingUnavailable: s__('Vulnerability|Training not available for this vulnerability.'),
+};
+
+export default {
+  i18n,
+  props: {
+    identifiers: {
+      type: Array,
+      required: true,
+    },
+  },
+  computed: {
+    isSupportedReferenceSchema() {
+      return this.referenceSchemas.some(
+        (referenceSchema) => referenceSchema?.toLowerCase() === SUPPORTED_REFERENCE_SCHEMA.cwe,
+      );
+    },
+    referenceSchemas() {
+      return this.identifiers.map((identifier) => identifier?.externalType);
+    },
+  },
+};
+</script>
+
+<template>
+  <div>
+    <h3>{{ $options.i18n.trainingTitle }}</h3>
+    <p class="gl-text-gray-600!" data-testid="description">
+      {{ $options.i18n.trainingDescription }}
+    </p>
+    <p v-if="!isSupportedReferenceSchema" data-testid="unavailable-message">
+      {{ $options.i18n.trainingUnavailable }}
+    </p>
+  </div>
+</template>

ee/app/assets/javascripts/vulnerabilities/constants.js

@@ -85,3 +85,7 @@ export const SUPPORTING_MESSAGE_TYPES = {
   // eslint-disable-next-line @gitlab/require-i18n-strings
   RECORDED: 'Recorded',
 };
+
+export const SUPPORTED_REFERENCE_SCHEMA = {
+  cwe: 'cwe',
+};

ee/app/controllers/projects/security/vulnerabilities_controller.rb

@@ -10,6 +10,7 @@ module Projects
 
       before_action do
         push_frontend_feature_flag(:create_vulnerability_jira_issue_via_graphql, @project, default_enabled: :yaml)
+        push_frontend_feature_flag(:secure_vulnerability_training, @project, default_enabled: :yaml)
       end
 
       before_action :vulnerability, except: [:index, :new]

ee/spec/frontend/vulnerabilities/vulnerability_details_spec.js

@@ -4,6 +4,7 @@ import { mount } from '@vue/test-utils';
 import SeverityBadge from 'ee/vue_shared/security_reports/components/severity_badge.vue';
 import VulnerabilityDetails from 'ee/vulnerabilities/components/vulnerability_details.vue';
 import { SUPPORTING_MESSAGE_TYPES } from 'ee/vulnerabilities/constants';
+import VulnerabilityTraining from 'ee/vulnerabilities/components/vulnerability_training.vue';
 
 describe('Vulnerability Details', () => {
   let wrapper;
@@ -16,16 +17,24 @@ describe('Vulnerability Details', () => {
     descriptionHtml: 'vulnerability description <code>sample</code>',
   };
 
-  const createWrapper = (vulnerabilityOverrides) => {
+  const createWrapper = (vulnerabilityOverrides, { secureVulnerabilityTraining = true } = {}) => {
     const propsData = {
       vulnerability: { ...vulnerability, ...vulnerabilityOverrides },
     };
-    wrapper = mount(VulnerabilityDetails, { propsData });
+    wrapper = mount(VulnerabilityDetails, {
+      propsData,
+      provide: {
+        glFeatures: {
+          secureVulnerabilityTraining,
+        },
+      },
+    });
   };
 
   const getById = (id) => wrapper.find(`[data-testid="${id}"]`);
   const getAllById = (id) => wrapper.findAll(`[data-testid="${id}"]`);
   const getText = (id) => getById(id).text();
+  const findVulnerabilityTraining = () => wrapper.findComponent(VulnerabilityTraining);
 
   afterEach(() => {
     wrapper.destroy();
@@ -397,4 +406,26 @@ describe('Vulnerability Details', () => {
       expect(getSectionData('recorded-response')).toEqual(expectedData);
     });
   });
+
+  describe('vulnerability training', () => {
+    it('renders the component', () => {
+      const identifiers = [{ externalType: 'cwe' }, { externalType: 'cve' }];
+      createWrapper({ identifiers });
+      expect(wrapper.findComponent(VulnerabilityTraining).props()).toMatchObject({
+        identifiers,
+      });
+    });
+
+    it('does not render the component when there are no identifiers', () => {
+      createWrapper();
+      expect(findVulnerabilityTraining().exists()).toBe(false);
+    });
+  });
+
+  describe('when secureVulnerabilityTraining feature flag is disabled', () => {
+    it('does not render the VulnerabilityTraining component', () => {
+      createWrapper(undefined, { secureVulnerabilityTraining: false });
+      expect(findVulnerabilityTraining().exists()).toBe(false);
+    });
+  });
 });

ee/spec/frontend/vulnerabilities/vulnerability_training_spec.js

+import VulnerabilityTraining, {
+  i18n,
+} from 'ee/vulnerabilities/components/vulnerability_training.vue';
+import { shallowMountExtended } from 'helpers/vue_test_utils_helper';
+import { SUPPORTED_REFERENCE_SCHEMA } from 'ee/vulnerabilities/constants';
+
+const defaultProps = {
+  identifiers: [{ externalType: SUPPORTED_REFERENCE_SCHEMA.cwe }, { externalType: 'cve' }],
+};
+
+describe('VulnerabilityTraining component', () => {
+  let wrapper;
+
+  const createComponent = (props = {}) => {
+    wrapper = shallowMountExtended(VulnerabilityTraining, {
+      propsData: {
+        ...defaultProps,
+        ...props,
+      },
+    });
+  };
+
+  afterEach(() => {
+    wrapper.destroy();
+  });
+
+  const findTitle = () => wrapper.findByRole('heading', i18n.trainingTitle);
+  const findDescription = () => wrapper.findByTestId('description');
+  const findUnavailableMessage = () => wrapper.findByTestId('unavailable-message');
+
+  describe('basic structure', () => {
+    beforeEach(() => {
+      createComponent();
+    });
+
+    it('displays the title', () => {
+      expect(findTitle().text()).toBe(i18n.trainingTitle);
+    });
+
+    it('displays the description', () => {
+      expect(findDescription().text()).toBe(i18n.trainingDescription);
+    });
+  });
+
+  describe('training availability message', () => {
+    it('displays the message', () => {
+      createComponent({ identifiers: [{ externalType: 'not supported identifier' }] });
+      expect(findUnavailableMessage().text()).toBe(i18n.trainingUnavailable);
+    });
+
+    it.each`
+      identifiers                                                         | exists
+      ${[{ externalType: 'cve' }]}                                        | ${true}
+      ${[{ externalType: SUPPORTED_REFERENCE_SCHEMA.cwe.toUpperCase() }]} | ${false}
+      ${[{ externalType: SUPPORTED_REFERENCE_SCHEMA.cwe.toLowerCase() }]} | ${false}
+    `('sets it to "$exists" for "$identifiers"', ({ identifiers, exists }) => {
+      createComponent({ identifiers });
+      expect(findUnavailableMessage().exists()).toBe(exists);
+    });
+  });
+});

lib/gitlab/database/batch_counter.rb

@@ -52,12 +52,7 @@ module Gitlab
             batch_end = [batch_start + batch_size, finish].min
             batch_relation = build_relation_batch(batch_start, batch_end, mode)
 
-            op_args = @operation_args
-            if @operation == :count && @operation_args.blank? && use_loose_index_scan_for_distinct_values?(mode)
-              op_args = [Gitlab::Database::LooseIndexScanDistinctCount::COLUMN_ALIAS]
-            end
-
-            results = merge_results(results, batch_relation.send(@operation, *op_args)) # rubocop:disable GitlabSecurity/PublicSend
+            results = merge_results(results, batch_relation.send(@operation, *@operation_args)) # rubocop:disable GitlabSecurity/PublicSend
             batch_start = batch_end
           rescue ActiveRecord::QueryCanceled => error
             # retry with a safe batch size & warmer cache
@@ -67,18 +62,6 @@ module Gitlab
               log_canceled_batch_fetch(batch_start, mode, batch_relation.to_sql, error)
               return FALLBACK
             end
-          rescue Gitlab::Database::LooseIndexScanDistinctCount::ColumnConfigurationError => error
-            Gitlab::AppJsonLogger
-              .error(
-                event: 'batch_count',
-                relation: @relation.table_name,
-                operation: @operation,
-                operation_args: @operation_args,
-                mode: mode,
-                message: "LooseIndexScanDistinctCount column error: #{error.message}"
-              )
-
-            return FALLBACK
           end
 
           sleep(SLEEP_TIME_IN_SECONDS)
@@ -104,11 +87,7 @@ module Gitlab
       private
 
       def build_relation_batch(start, finish, mode)
-        if use_loose_index_scan_for_distinct_values?(mode)
-          Gitlab::Database::LooseIndexScanDistinctCount.new(@relation, @column).build_query(from: start, to: finish)
-        else
-          @relation.select(@column).public_send(mode).where(between_condition(start, finish)) # rubocop:disable GitlabSecurity/PublicSend
-        end
+        @relation.select(@column).public_send(mode).where(between_condition(start, finish)) # rubocop:disable GitlabSecurity/PublicSend
       end
 
       def batch_size_for_mode_and_operation(mode, operation)
@@ -151,10 +130,6 @@ module Gitlab
           )
       end
 
-      def use_loose_index_scan_for_distinct_values?(mode)
-        Feature.enabled?(:loose_index_scan_for_distinct_values) && not_group_by_query? && mode == :distinct
-      end
-
       def not_group_by_query?
         !@relation.is_a?(ActiveRecord::Relation) || @relation.group_values.blank?
       end

lib/gitlab/database/loose_index_scan_distinct_count.rb

-# frozen_string_literal: true
-
-module Gitlab
-  module Database
-    # This class builds efficient batched distinct query by using loose index scan.
-    # Consider the following example:
-    # > Issue.distinct(:project_id).where(project_id: (1...100)).count
-    #
-    # Note: there is an index on project_id
-    #
-    # This query will read each element in the index matching the project_id filter.
-    # If for a project_id has 100_000 issues, all 100_000 elements will be read.
-    #
-    # A loose index scan will only read one entry from the index for each project_id to reduce the number of disk reads.
-    #
-    # Usage:
-    #
-    # Gitlab::Database::LooseIndexScanDisctinctCount.new(Issue, :project_id).count(from: 1, to: 100)
-    #
-    # The query will return the number of distinct projects_ids between 1 and 100
-    #
-    # Getting the Arel query:
-    #
-    # Gitlab::Database::LooseIndexScanDisctinctCount.new(Issue, :project_id).build_query(from: 1, to: 100)
-    class LooseIndexScanDistinctCount
-      COLUMN_ALIAS = 'distinct_count_column'
-
-      ColumnConfigurationError = Class.new(StandardError)
-
-      def initialize(scope, column)
-        if scope.is_a?(ActiveRecord::Relation)
-          @scope = scope
-          @model = scope.model
-        else
-          @scope = scope.where({})
-          @model = scope
-        end
-
-        @column = transform_column(column)
-      end
-
-      def count(from:, to:)
-        build_query(from: from, to: to).count(COLUMN_ALIAS)
-      end
-
-      def build_query(from:, to:) # rubocop:disable Metrics/AbcSize
-        cte = Gitlab::SQL::RecursiveCTE.new(:counter_cte, union_args: { remove_order: false })
-        table = model.arel_table
-
-        cte << @scope
-          .dup
-          .select(column.as(COLUMN_ALIAS))
-          .where(column.gteq(from))
-          .where(column.lt(to))
-          .order(column)
-          .limit(1)
-
-        inner_query = @scope
-          .dup
-          .where(column.gt(cte.table[COLUMN_ALIAS]))
-          .where(column.lt(to))
-          .select(column.as(COLUMN_ALIAS))
-          .order(column)
-          .limit(1)
-
-        cte << cte.table
-          .project(Arel::Nodes::Grouping.new(Arel.sql(inner_query.to_sql)).as(COLUMN_ALIAS))
-          .where(cte.table[COLUMN_ALIAS].lt(to))
-
-        model
-          .with
-          .recursive(cte.to_arel)
-          .from(cte.alias_to(table))
-          .unscope(where: :source_type)
-          .unscope(where: model.inheritance_column) # Remove STI query, not needed here
-      end
-
-      private
-
-      attr_reader :column, :model
-
-      # Transforms the column so it can be used in Arel expressions
-      #
-      # 'table.column' => 'table.column'
-      # 'column' => 'table_name.column'
-      # :column => 'table_name.column'
-      # Arel::Attributes::Attribute => name of the column
-      def transform_column(column)
-        if column.is_a?(String) || column.is_a?(Symbol)
-          column_as_string = column.to_s
-          column_as_string = "#{model.table_name}.#{column_as_string}" unless column_as_string.include?('.')
-
-          Arel.sql(column_as_string)
-        elsif column.is_a?(Arel::Attributes::Attribute)
-          column
-        else
-          raise ColumnConfigurationError, "Cannot transform the column: #{column.inspect}, please provide the column name as string"
-        end
-      end
-    end
-  end
-end

locale/gitlab.pot

@@ -39578,6 +39578,9 @@ msgstr ""
 msgid "Vulnerability|Information related how the vulnerability was discovered and its impact to the system."
 msgstr ""
 
+msgid "Vulnerability|Learn more about this vulnerability and the best way to resolve it."
+msgstr ""
+
 msgid "Vulnerability|Links"
 msgstr ""
 
@@ -39626,6 +39629,12 @@ msgstr ""
 msgid "Vulnerability|Tool"
 msgstr ""
 
+msgid "Vulnerability|Training"
+msgstr ""
+
+msgid "Vulnerability|Training not available for this vulnerability."
+msgstr ""
+
 msgid "Vulnerability|Unmodified Response"
 msgstr ""
 

spec/lib/gitlab/database/batch_count_spec.rb

@@ -270,8 +270,6 @@ RSpec.describe Gitlab::Database::BatchCount do
     end
 
     it "defaults the batch size to #{Gitlab::Database::BatchCounter::DEFAULT_DISTINCT_BATCH_SIZE}" do
-      stub_feature_flags(loose_index_scan_for_distinct_values: false)
-
       min_id = model.minimum(:id)
       relation = instance_double(ActiveRecord::Relation)
       allow(model).to receive_message_chain(:select, public_send: relation)
@@ -317,85 +315,13 @@ RSpec.describe Gitlab::Database::BatchCount do
       end
     end
 
-    context 'when the loose_index_scan_for_distinct_values feature flag is off' do
-      it_behaves_like 'when batch fetch query is canceled' do
-        let(:mode) { :distinct }
-        let(:operation) { :count }
-        let(:operation_args) { nil }
-        let(:column) { nil }
-
-        subject { described_class.method(:batch_distinct_count) }
-
-        before do
-          stub_feature_flags(loose_index_scan_for_distinct_values: false)
-        end
-      end
-    end
-
-    context 'when the loose_index_scan_for_distinct_values feature flag is on' do
+    it_behaves_like 'when batch fetch query is canceled' do
       let(:mode) { :distinct }
       let(:operation) { :count }
       let(:operation_args) { nil }
       let(:column) { nil }
 
-      let(:batch_size) { 10_000 }
-
       subject { described_class.method(:batch_distinct_count) }
-
-      before do
-        stub_feature_flags(loose_index_scan_for_distinct_values: true)
-      end
-
-      it 'reduces batch size by half and retry fetch' do
-        too_big_batch_relation_mock = instance_double(ActiveRecord::Relation)
-
-        count_method = double(send: 1)
-
-        allow(too_big_batch_relation_mock).to receive(:send).and_raise(ActiveRecord::QueryCanceled)
-        allow(Gitlab::Database::LooseIndexScanDistinctCount).to receive_message_chain(:new, :build_query).with(from: 0, to: batch_size).and_return(too_big_batch_relation_mock)
-        allow(Gitlab::Database::LooseIndexScanDistinctCount).to receive_message_chain(:new, :build_query).with(from: 0, to: batch_size / 2).and_return(count_method)
-        allow(Gitlab::Database::LooseIndexScanDistinctCount).to receive_message_chain(:new, :build_query).with(from: batch_size / 2, to: batch_size).and_return(count_method)
-
-        subject.call(model, column, batch_size: batch_size, start: 0, finish: batch_size - 1)
-      end
-
-      context 'when all retries fail' do
-        let(:batch_count_query) { 'SELECT COUNT(id) FROM relation WHERE id BETWEEN 0 and 1' }
-
-        before do
-          relation = instance_double(ActiveRecord::Relation)
-          allow(Gitlab::Database::LooseIndexScanDistinctCount).to receive_message_chain(:new, :build_query).and_return(relation)
-          allow(relation).to receive(:send).and_raise(ActiveRecord::QueryCanceled.new('query timed out'))
-          allow(relation).to receive(:to_sql).and_return(batch_count_query)
-        end
-
-        it 'logs failing query' do
-          expect(Gitlab::AppJsonLogger).to receive(:error).with(
-            event: 'batch_count',
-            relation: model.table_name,
-            operation: operation,
-            operation_args: operation_args,
-            start: 0,
-            mode: mode,
-            query: batch_count_query,
-            message: 'Query has been canceled with message: query timed out'
-          )
-          expect(subject.call(model, column, batch_size: batch_size, start: 0)).to eq(-1)
-        end
-      end
-
-      context 'when LooseIndexScanDistinctCount raises error' do
-        let(:column) { :creator_id }
-        let(:error_class) { Gitlab::Database::LooseIndexScanDistinctCount::ColumnConfigurationError }
-
-        it 'rescues ColumnConfigurationError' do
-          allow(Gitlab::Database::LooseIndexScanDistinctCount).to receive(:new).and_raise(error_class.new('error message'))
-
-          expect(Gitlab::AppJsonLogger).to receive(:error).with(a_hash_including(message: 'LooseIndexScanDistinctCount column error: error message'))
-
-          expect(subject.call(Project, column, batch_size: 10_000, start: 0)).to eq(-1)
-        end
-      end
     end
   end
 

spec/lib/gitlab/database/loose_index_scan_distinct_count_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Gitlab::Database::LooseIndexScanDistinctCount do
-  context 'counting distinct users' do
-    let_it_be(:user) { create(:user) }
-    let_it_be(:other_user) { create(:user) }
-
-    let(:column) { :creator_id }
-
-    before_all do
-      create_list(:project, 3, creator: user)
-      create_list(:project, 1, creator: other_user)
-    end
-
-    subject(:count) { described_class.new(Project, :creator_id).count(from: Project.minimum(:creator_id), to: Project.maximum(:creator_id) + 1) }
-
-    it { is_expected.to eq(2) }
-
-    context 'when STI model is queried' do
-      it 'does not raise error' do
-        expect { described_class.new(Group, :owner_id).count(from: 0, to: 1) }.not_to raise_error
-      end
-    end
-
-    context 'when model with default_scope is queried' do
-      it 'does not raise error' do
-        expect { described_class.new(GroupMember, :id).count(from: 0, to: 1) }.not_to raise_error
-      end
-    end
-
-    context 'when the fully qualified column is given' do
-      let(:column) { 'projects.creator_id' }
-
-      it { is_expected.to eq(2) }
-    end
-
-    context 'when AR attribute is given' do
-      let(:column) { Project.arel_table[:creator_id] }
-
-      it { is_expected.to eq(2) }
-    end
-
-    context 'when invalid value is given for the column' do
-      let(:column) { Class.new }
-
-      it { expect { described_class.new(Group, column) }.to raise_error(Gitlab::Database::LooseIndexScanDistinctCount::ColumnConfigurationError) }
-    end
-
-    context 'when null values are present' do
-      before do
-        create_list(:project, 2).each { |p| p.update_column(:creator_id, nil) }
-      end
-
-      it { is_expected.to eq(2) }
-    end
-  end
-
-  context 'counting STI models' do
-    let!(:groups) { create_list(:group, 3) }
-    let!(:namespaces) { create_list(:namespace, 2) }
-
-    let(:max_id) { Namespace.maximum(:id) + 1 }
-
-    it 'counts groups' do
-      count = described_class.new(Group, :id).count(from: 0, to: max_id)
-      expect(count).to eq(3)
-    end
-  end
-end