Merge branch 'philipcunningham-meta-tag-runner-feature-flag-cleanup-338648' into 'master'

Remove DAST site validation feature flags

See merge request gitlab-org/gitlab!68330

config/feature_flags/development/dast_meta_tag_validation.yml

----
-name: dast_meta_tag_validation
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/67945
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/337711
-milestone: '14.2'
-type: development
-group: group::dynamic analysis
-default_enabled: true

config/feature_flags/development/dast_runner_site_validation.yml

----
-name: dast_runner_site_validation
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/61649
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/331082
-milestone: '14.0'
-type: development
-group: group::dynamic analysis
-default_enabled: true

doc/user/application_security/dast/index.md

@@ -1049,11 +1049,7 @@ When an API site type is selected, a [host override](#host-override) is used to
 #### Site profile validation
 > - Site profile validation [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/233020) in GitLab 13.8.
-> - Meta tag validation [enabled on GitLab.com](https://gitlab.com/groups/gitlab-org/-/epics/6460) in GitLab 14.2 and is ready for production use.
-> - Meta tag validation [enabled with `dast_meta_tag_validation flag` flag](https://gitlab.com/gitlab-org/gitlab/-/issues/337711) for self-managed GitLab in GitLab 14.2 and is ready for production use.
-
-FLAG:
-On self-managed GitLab, by default this feature is available. To hide the feature, ask an administrator to [disable the `dast_meta_tag_validation` flag](../../../administration/feature_flags.md). On GitLab.com, this feature is available but can be configured by GitLab.com administrators only.
+> - Meta tag validation [introduced](https://gitlab.com/groups/gitlab-org/-/epics/6460) in GitLab 14.2.
 Site profile validation reduces the risk of running an active scan against the wrong website. A site
 must be validated before an active scan can run against it. The site validation methods are as

ee/app/assets/javascripts/security_configuration/dast_site_validation/components/dast_site_validation_modal.vue

@@ -18,7 +18,6 @@ import download from '~/lib/utils/downloader';
 import { cleanLeadingSeparator, joinPaths, stripPathTail } from '~/lib/utils/url_utility';
 import { __, s__ } from '~/locale';
 import ModalCopyButton from '~/vue_shared/components/modal_copy_button.vue';
-import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
 import {
   DAST_SITE_VALIDATION_MODAL_ID,
   DAST_SITE_VALIDATION_HTTP_HEADER_KEY,
@@ -47,7 +46,6 @@ export default {
     GlTruncate,
     GlLink,
   },
-  mixins: [glFeatureFlagMixin()],
   props: {
     fullPath: {
       type: String,
@@ -91,11 +89,7 @@ export default {
       };
     },
     validationMethodOptions() {
-      const options = Object.values(DAST_SITE_VALIDATION_METHODS);
-      if (!this.glFeatures.dastMetaTagValidation) {
-        return options.filter(({ value }) => value !== DAST_SITE_VALIDATION_METHOD_META_TAG);
-      }
-      return options;
+      return Object.values(DAST_SITE_VALIDATION_METHODS);
     },
     urlObject() {
       try {

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

@@ -8,7 +8,6 @@ class DastProfilesController < Projects::ApplicationController
       before_action do
         authorize_read_on_demand_scans!
         push_frontend_feature_flag(:dast_failed_site_validations, @project, default_enabled: :yaml)
-        push_frontend_feature_flag(:dast_meta_tag_validation, @project, default_enabled: :yaml)
       end
       feature_category :dynamic_application_security_testing

ee/app/models/dast_site.rb

@@ -6,8 +6,7 @@ class DastSite < ApplicationRecord
   has_many :dast_site_profiles
   validates :url, length: { maximum: 255 }, uniqueness: { scope: :project_id }
-  validates :url, addressable_url: true, if: :runner_validation_enabled?
-  validates :url, public_url: true, unless: :runner_validation_enabled?
+  validates :url, addressable_url: true
   validates :project_id, presence: true
   validate :dast_site_validation_project_id_fk
@@ -21,8 +20,4 @@ def dast_site_validation_project_id_fk
       errors.add(:project_id, 'does not match dast_site_validation.project')
     end
   end
-
-  def runner_validation_enabled?
-    ::Feature.enabled?(:dast_runner_site_validation, project, default_enabled: :yaml)
-  end
 end

ee/app/models/dast_site_validation.rb

@@ -9,8 +9,6 @@ class DastSiteValidation < ApplicationRecord
   validates :dast_site_token_id, presence: true
   validates :validation_strategy, presence: true
-  validate :meta_tag_validation_must_happen_on_runner, if: :meta_tag?
-
   scope :by_project_id, -> (project_id) do
     joins(:dast_site_token).where(dast_site_tokens: { project_id: project_id })
   end
@@ -81,11 +79,4 @@ def self.get_normalized_url_base(url)
   def set_normalized_url_base
     self.url_base = self.class.get_normalized_url_base(dast_site_token.url)
   end
-
-  def meta_tag_validation_must_happen_on_runner
-    return if ::Feature.enabled?(:dast_runner_site_validation, project, default_enabled: :yaml) &&
-              ::Feature.enabled?(:dast_meta_tag_validation, project, default_enabled: :yaml)
-
-    errors.add(:base, 'Meta tag validation is not enabled')
-  end
 end

ee/app/services/app_sec/dast/site_validations/runner_service.rb

@@ -22,8 +22,7 @@ def execute
         private
         def allowed?
-          can?(current_user, :create_on_demand_dast_scan, project) &&
-            ::Feature.enabled?(:dast_runner_site_validation, project, default_enabled: :yaml)
+          can?(current_user, :create_on_demand_dast_scan, project)
         end
         def dast_site_validation

ee/app/services/dast_site_validations/create_service.rb

@@ -12,7 +12,7 @@ def execute
       associate_dast_site!(dast_site_validation)
-      perform_async_validation(dast_site_validation)
+      perform_runner_validation(dast_site_validation)
     rescue ActiveRecord::RecordInvalid => err
       ServiceResponse.error(message: err.record.errors.full_messages)
     rescue KeyError => err
@@ -66,29 +66,7 @@ def create_validation!
       )
     end
-    def perform_async_validation(dast_site_validation)
-      if Feature.enabled?(:dast_runner_site_validation, dast_site_validation.project, default_enabled: :yaml)
-        runner_validation(dast_site_validation)
-      else
-        worker_validation(dast_site_validation)
-      end
-    end
-
-    def worker_validation(dast_site_validation)
-      jid = DastSiteValidationWorker.perform_async(dast_site_validation.id)
-
-      unless jid.present?
-        log_error(message: 'Unable to validate dast_site_validation', dast_site_validation_id: dast_site_validation.id)
-
-        dast_site_validation.fail_op
-
-        return ServiceResponse.error(message: 'Validation failed')
-      end
-
-      ServiceResponse.success(payload: dast_site_validation)
-    end
-
-    def runner_validation(dast_site_validation)
+    def perform_runner_validation(dast_site_validation)
       AppSec::Dast::SiteValidations::RunnerService.new(
         project: dast_site_validation.project,
         current_user: current_user,

ee/app/services/dast_site_validations/validate_service.rb

-# frozen_string_literal: true
-
-module DastSiteValidations
-  class ValidateService < BaseContainerService
-    PermissionsError = Class.new(StandardError)
-    TokenNotFound = Class.new(StandardError)
-
-    def execute!
-      raise PermissionsError, 'Insufficient permissions' unless allowed?
-
-      return if dast_site_validation.passed?
-
-      if dast_site_validation.pending?
-        dast_site_validation.start
-      else
-        dast_site_validation.retry
-      end
-
-      response = make_http_request!
-
-      validate!(response)
-    end
-
-    private
-
-    def allowed?
-      container.feature_available?(:security_on_demand_scans)
-    end
-
-    def dast_site_validation
-      @dast_site_validation ||= params.fetch(:dast_site_validation)
-    end
-
-    def make_http_request!
-      Gitlab::HTTP.get(dast_site_validation.validation_url, use_read_total_timeout: true)
-    end
-
-    def token_found?(response)
-      token = dast_site_validation.dast_site_token.token
-
-      case dast_site_validation.validation_strategy
-      when 'text_file'
-        response.content_type == 'text/plain' && response.body.rstrip == token
-      when 'header'
-        response.headers[DastSiteValidation::HEADER] == token
-      else
-        false
-      end
-    end
-
-    def validate!(response)
-      raise TokenNotFound, 'Could not find token' unless token_found?(response)
-
-      dast_site_validation.pass
-    end
-  end
-end

ee/app/workers/dast_site_validation_worker.rb

@@ -14,18 +14,8 @@ class DastSiteValidationWorker
   feature_category :dynamic_application_security_testing
   tags :exclude_from_kubernetes
-  sidekiq_retries_exhausted do |job|
-    dast_site_validation = DastSiteValidation.find(job['args'][0])
-    dast_site_validation.fail_op
-  end
-
-  def perform(dast_site_validation_id)
-    dast_site_validation = DastSiteValidation.find(dast_site_validation_id)
-    project = dast_site_validation.project
-
-    DastSiteValidations::ValidateService.new(
-      container: project,
-      params: { dast_site_validation: dast_site_validation }
-    ).execute!
+  def perform(_dast_site_validation_id)
+    # Scheduled for removal in %15.0
+    # Please see https://gitlab.com/gitlab-org/gitlab/-/issues/339088
   end
 end

ee/lib/api/internal/app_sec/dast/site_validations.rb

@@ -15,9 +15,7 @@ class SiteValidations < ::API::Base
           namespace :internal do
             namespace :dast do
               resource :site_validations do
-                desc 'Transitions a DAST site validation to a new state.' do
-                  detail 'This feature is gated by the :dast_runner_site_validation feature flag.'
-                end
+                desc 'Transitions a DAST site validation to a new state.'
                 route_setting :authentication, job_token_allowed: true
                 params do
                   requires :event, type: Symbol, values: %i[start fail_op retry pass], desc: 'The transition event.'
@@ -25,10 +23,6 @@ class SiteValidations < ::API::Base
                 post ':id/transition' do
                   validation = DastSiteValidation.find(params[:id])
-                  unless Feature.enabled?(:dast_runner_site_validation, validation.project, default_enabled: :yaml)
-                    render_api_error!('404 Feature flag disabled: :dast_runner_site_validation', 404)
-                  end
-
                   authorize!(:create_on_demand_dast_scan, validation)
                   bad_request!('Project mismatch') unless current_authenticated_job.project == validation.project

ee/spec/frontend/security_configuration/dast_profiles/dast_site_validation/components/dast_site_validation_modal_spec.js

@@ -57,9 +57,6 @@ describe('DastSiteValidationModal', () => {
             static: true,
             visible: true,
           },
-          provide: {
-            glFeatures: { dastMetaTagValidation: true },
-          },
         },
         mountOptions,
         {
@@ -324,22 +321,6 @@ describe('DastSiteValidationModal', () => {
     });
   });
-  describe('with the dastMetaTagValidation feature flag disabled', () => {
-    beforeEach(() => {
-      createFullComponent({
-        provide: {
-          glFeatures: {
-            dastMetaTagValidation: false,
-          },
-        },
-      });
-    });
-
-    it('does not render the meta tag validation method', () => {
-      expect(findRadioInputForValidationMethod('meta tag')).toBe(null);
-    });
-  });
-
   describe.each(validationMethods)('"%s" validation submission', (validationMethod) => {
     beforeEach(async () => {
       createFullComponent();

ee/spec/graphql/mutations/dast_site_validations/create_spec.rb

@@ -29,42 +29,28 @@
       )
     end
-    shared_examples 'a validation mutation' do
-      context 'when on demand scan feature is enabled' do
-        context 'when the project does not exist' do
-          let(:full_path) { SecureRandom.hex }
-          it 'raises an exception' do
-            expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
-          end
+    context 'when on demand scan feature is enabled' do
+      context 'when the project does not exist' do
+        let(:full_path) { SecureRandom.hex }
+        it 'raises an exception' do
+          expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
         end
-        context 'when the user can run a dast scan' do
-          before do
-            project.add_developer(user)
-          end
-
-          it 'returns the dast_site_validation id' do
-            expect(subject[:id]).to eq(dast_site_validation.to_global_id)
-          end
-          it 'returns the dast_site_validation status' do
-            expect(subject[:status]).to eq(dast_site_validation.state)
-          end
+      end
+      context 'when the user can run a dast scan' do
+        before do
+          project.add_developer(user)
+        end
+        it 'returns the dast_site_validation id' do
+          expect(subject[:id]).to eq(dast_site_validation.to_global_id)
         end
-      end
-    end
-    context 'worker validation' do
-      before do
-        stub_feature_flags(dast_runner_site_validation: false)
+        it 'returns the dast_site_validation status' do
+          expect(subject[:status]).to eq(dast_site_validation.state)
+        end
       end
-
-      it_behaves_like 'a validation mutation'
-    end
-
-    context 'runner validation' do
-      it_behaves_like 'a validation mutation'
     end
   end
 end

ee/spec/models/dast_site_spec.rb

@@ -38,22 +38,9 @@
       subject { build(:dast_site, project: project, url: 'http://127.0.0.1') }
-      context 'worker validation' do
-        before do
-          stub_feature_flags(dast_runner_site_validation: false)
-        end
-
-        it 'is not valid', :aggregate_failures do
-          expect(subject).not_to be_valid
-          expect(subject.errors.full_messages).to include(message)
-        end
-      end
-
-      context 'runner validation' do
-        it 'is is valid', :aggregate_failures do
-          expect(subject).to be_valid
-          expect(subject.errors.full_messages).not_to include(message)
-        end
+      it 'is is valid', :aggregate_failures do
+        expect(subject).to be_valid
+        expect(subject.errors.full_messages).not_to include(message)
       end
     end
   end

ee/spec/models/dast_site_validation_spec.rb

@@ -17,37 +17,6 @@
   describe 'validations' do
     it { is_expected.to be_valid }
     it { is_expected.to validate_presence_of(:dast_site_token_id) }
-
-    context 'when strategy is meta_tag' do
-      subject { build(:dast_site_validation, dast_site_token: dast_site_token, validation_strategy: :meta_tag) }
-
-      shared_examples 'meta tag validation is disabled' do
-        it 'is not valid', :aggregate_failures do
-          expect(subject).not_to be_valid
-          expect(subject.errors.full_messages).to include('Meta tag validation is not enabled')
-        end
-      end
-
-      context 'when dast_meta_tag_validation and dast_runner_site_validation are enabled' do
-        it { is_expected.to be_valid }
-      end
-
-      context 'when dast_meta_tag_validation is disabled' do
-        before do
-          stub_feature_flags(dast_meta_tag_validation: false)
-        end
-
-        it_behaves_like 'meta tag validation is disabled'
-      end
-
-      context 'when dast_runner_site_validation is disabled' do
-        before do
-          stub_feature_flags(dast_runner_site_validation: false)
-        end
-
-        it_behaves_like 'meta tag validation is disabled'
-      end
-    end
   end
   describe 'before_create' do

ee/spec/requests/api/internal/app_sec/dast/site_validations_spec.rb

@@ -101,19 +101,6 @@
           end
         end
-        context 'when the feature flag is disabled' do
-          before do
-            stub_feature_flags(dast_runner_site_validation: false)
-          end
-
-          it 'returns 404 and a contextual error message', :aggregate_failures do
-            subject
-
-            expect(response).to have_gitlab_http_status(:not_found)
-            expect(json_response).to eq('message' => '404 Feature flag disabled: :dast_runner_site_validation')
-          end
-        end
-
         context 'when user has access to the site validation' do
           context 'when the state transition is unknown' do
             let(:event_param) { :unknown_transition }

ee/spec/services/app_sec/dast/site_validations/runner_service_spec.rb

@@ -75,14 +75,6 @@
           expect { subject }.to change { dast_site_validation.state }.from('pending').to('failed')
         end
       end
-
-      context 'when feature flag is disabled' do
-        before do
-          stub_feature_flags(dast_runner_site_validation: false)
-        end
-
-        it_behaves_like 'a failure'
-      end
     end
   end
 end

ee/spec/services/dast_site_validations/create_service_spec.rb

@@ -87,80 +87,23 @@
   end
   describe 'execute', :clean_gitlab_redis_shared_state do
-    context 'worker validation' do
-      before do
-        stub_feature_flags(dast_runner_site_validation: false)
-      end
-
-      it_behaves_like 'the licensed feature is not available'
-
-      it_behaves_like 'the licensed feature is available' do
-        it 'attempts to validate' do
-          expect(DastSiteValidationWorker).to receive(:perform_async)
-
-          subject
-        end
-
-        context 'when worker does not return a job id' do
-          before do
-            allow(DastSiteValidationWorker).to receive(:perform_async).and_return(nil)
-          end
-
-          let(:dast_site_validation) do
-            DastSiteValidation.find_by!(dast_site_token: dast_site_token, url_path: params[:url_path])
-          end
-
-          it 'communicates failure' do
-            aggregate_failures do
-              expect(subject.status).to eq(:error)
-              expect(subject.message).to eq('Validation failed')
-            end
-          end
-
-          it 'sets dast_site_validation.state to failed' do
-            subject
-
-            expect(dast_site_validation.state).to eq('failed')
-          end
-          it 'logs an error' do
-            allow(Gitlab::AppLogger).to receive(:error)
-            subject
-            expect(Gitlab::AppLogger).to have_received(:error).with(message: 'Unable to validate dast_site_validation', dast_site_validation_id: dast_site_validation.id)
-          end
-        end
-
-        it_behaves_like 'a dast_site_validation already exists' do
-          it 'does not attempt to re-validate' do
-            expect(DastSiteValidationWorker).not_to receive(:perform_async)
-
-            subject
-          end
-        end
+    it_behaves_like 'the licensed feature is not available'
+    it_behaves_like 'the licensed feature is available' do
+      it 'attempts to validate' do
+        expected_args = { project: project, current_user: developer, params: { dast_site_validation: instance_of(DastSiteValidation) } }
+        expect(AppSec::Dast::SiteValidations::RunnerService).to receive(:new).with(expected_args).and_call_original
+        subject
       end
-    end
-    context 'runner validation' do
-      it_behaves_like 'the licensed feature is not available'
-
-      it_behaves_like 'the licensed feature is available' do
-        it 'attempts to validate' do
-          expected_args = { project: project, current_user: developer, params: { dast_site_validation: instance_of(DastSiteValidation) } }
-
-          expect(AppSec::Dast::SiteValidations::RunnerService).to receive(:new).with(expected_args).and_call_original
+      it_behaves_like 'a dast_site_validation already exists' do
+        it 'does not attempt to re-validate' do
+          expect(AppSec::Dast::SiteValidations::RunnerService).not_to receive(:new)
           subject
         end
-
-        it_behaves_like 'a dast_site_validation already exists' do
-          it 'does not attempt to re-validate' do
-            expect(AppSec::Dast::SiteValidations::RunnerService).not_to receive(:new)
-
-            subject
-          end
-        end
       end
     end
   end

ee/spec/services/dast_site_validations/validate_service_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe DastSiteValidations::ValidateService do
-  let(:dast_site_validation) { create(:dast_site_validation) }
-  let(:token) { dast_site_validation.dast_site_token.token }
-  let(:headers) { { 'Content-Type' => 'text/plain; charset=utf-8' } }
-
-  subject do
-    described_class.new(
-      container: dast_site_validation.project,
-      params: { dast_site_validation: dast_site_validation }
-    ).execute!
-  end
-
-  describe 'execute!' do
-    context 'when on demand scan licensed feature is not available' do
-      it 'communicates failure' do
-        stub_licensed_features(security_on_demand_scans: false)
-
-        expect { subject }.to raise_error(DastSiteValidations::ValidateService::PermissionsError)
-      end
-    end
-
-    context 'when the feature is available' do
-      before do
-        stub_licensed_features(security_on_demand_scans: true)
-        stub_request(:get, dast_site_validation.validation_url).to_return(body: token, headers: headers)
-      end
-
-      it 'validates the url before making an http request' do
-        uri = URI(dast_site_validation.validation_url)
-        opt = hash_including(allow_local_network: false, allow_localhost: false, dns_rebind_protection: true)
-
-        aggregate_failures do
-          expect(Gitlab::UrlBlocker).to receive(:validate!).with(uri, opt).and_call_original
-          expect(Gitlab::HTTP).to receive(:get).with(dast_site_validation.validation_url, use_read_total_timeout: true).and_call_original
-        end
-
-        subject
-      end
-
-      context 'when validation has already been attempted' do
-        let_it_be(:dast_site_validation) { create(:dast_site_validation, state: :failed) }
-
-        it 'marks the validation as a retry' do
-          freeze_time do
-            subject
-
-            expect(dast_site_validation.reload.validation_last_retried_at).to eq(Time.now.utc)
-          end
-        end
-      end
-
-      shared_examples 'a validation' do
-        context 'when the token is found' do
-          it 'calls dast_site_validation#start' do
-            expect(dast_site_validation).to receive(:start)
-
-            subject
-          end
-
-          it 'calls dast_site_validation#pass' do
-            expect(dast_site_validation).to receive(:pass)
-
-            subject
-          end
-
-          it 'marks the validation successful' do
-            subject
-
-            expect(dast_site_validation.reload.state).to eq('passed')
-          end
-
-          context 'when validation has already started' do
-            before do
-              dast_site_validation.update_column(:state, :inprogress)
-            end
-
-            it 'does not call dast_site_validation#pass' do
-              expect(dast_site_validation).not_to receive(:start)
-
-              subject
-            end
-          end
-
-          context 'when validation is already complete' do
-            before do
-              dast_site_validation.update_column(:state, :passed)
-            end
-
-            it 'does not re-validate' do
-              expect(Gitlab::HTTP).not_to receive(:get)
-
-              subject
-            end
-          end
-        end
-
-        context 'when the token is not found' do
-          let(:token) do
-            '<div>' + dast_site_validation.dast_site_token.token + '</div>'
-          end
-
-          it 'raises an exception' do
-            expect { subject }.to raise_error(DastSiteValidations::ValidateService::TokenNotFound)
-          end
-        end
-      end
-
-      context 'when validation_strategy=text_file' do
-        let(:dast_site_validation) { create(:dast_site_validation, validation_strategy: :text_file) }
-
-        before do
-          stub_request(:get, dast_site_validation.validation_url).to_return(body: token, headers: headers)
-        end
-
-        it_behaves_like 'a validation'
-
-        context 'when the http response body has trailing newlines after the token' do
-          let(:token) { dast_site_validation.dast_site_token.token + "\n\n" }
-
-          it 'does not raise an exception' do
-            expect { subject }.not_to raise_error(DastSiteValidations::ValidateService::TokenNotFound)
-          end
-        end
-
-        context 'when content type is incorrect' do
-          let(:headers) { { 'Content-Type' => 'text/html; charset=UTF-8' } }
-
-          it 'raises an exception' do
-            expect { subject }.to raise_error(DastSiteValidations::ValidateService::TokenNotFound)
-          end
-        end
-      end
-
-      context 'when validation_strategy=header' do
-        let(:dast_site_validation) { create(:dast_site_validation, validation_strategy: :header) }
-
-        before do
-          headers = { DastSiteValidation::HEADER => token }
-
-          stub_request(:get, dast_site_validation.validation_url).to_return(headers: headers)
-        end
-
-        it_behaves_like 'a validation'
-      end
-    end
-  end
-end