Merge branch...

Merge branch 'philipcunningham-dont-validate-url-when-using-runner-for-validation-270751' into 'master'

Validate URL addressable for DAST runner context

See merge request gitlab-org/gitlab!66588

ee/app/models/dast_site.rb

@@ -5,7 +5,10 @@ class DastSite < ApplicationRecord
   belongs_to :dast_site_validation
   has_many :dast_site_profiles
 
-  validates :url, length: { maximum: 255 }, uniqueness: { scope: :project_id }, public_url: true
+  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 :project_id, presence: true
   validate :dast_site_validation_project_id_fk
 
@@ -18,4 +21,8 @@ class DastSite < ApplicationRecord
       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/spec/models/dast_site_spec.rb

@@ -3,7 +3,9 @@
 require 'spec_helper'
 
 RSpec.describe DastSite, type: :model do
-  subject { create(:dast_site) }
+  let_it_be(:project) { create(:project) }
+
+  subject { create(:dast_site, project: project) }
 
   describe 'associations' do
     it { is_expected.to belong_to(:project) }
@@ -32,11 +34,26 @@ RSpec.describe DastSite, type: :model do
     end
 
     context 'when the url is not public' do
-      subject { build(:dast_site, url: 'http://127.0.0.1') }
+      let_it_be(:message) { 'Url is blocked: Requests to localhost are not allowed' }
 
-      it 'is not valid' do
-        expect(subject.valid?).to be_falsey
-        expect(subject.errors.full_messages).to include('Url is blocked: Requests to localhost are not allowed')
+      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
       end
     end
   end

ee/spec/requests/api/graphql/mutations/dast_site_profiles/update_spec.rb

@@ -37,6 +37,7 @@ RSpec.describe 'Creating a DAST Site Profile' do
   end
 
   it_behaves_like 'an on-demand scan mutation when user cannot run an on-demand scan'
+
   it_behaves_like 'an on-demand scan mutation when user can run an on-demand scan' do
     it 'updates the dast_site_profile' do
       subject
@@ -49,10 +50,17 @@ RSpec.describe 'Creating a DAST Site Profile' do
       end
     end
 
-    context 'when there is an issue updating the dast_site_profile' do
-      let(:new_target_url) { 'http://localhost:3000' }
+    context 'when there is a validation error' do
+      before do
+        allow(dast_site_profile).to receive(:valid?).and_return(false)
+        allow(dast_site_profile).to receive_message_chain(:errors, :full_messages).and_return(['There was a validation error'])
+
+        allow_next_instance_of(DastSiteProfilesFinder) do |instance|
+          allow(instance).to receive_message_chain(:execute, :first!).and_return(dast_site_profile)
+        end
+      end
 
-      it_behaves_like 'a mutation that returns errors in the response', errors: ['Url is blocked: Requests to localhost are not allowed']
+      it_behaves_like 'a mutation that returns errors in the response', errors: ['There was a validation error']
     end
 
     context 'when the dast_site_profile does not exist' do

ee/spec/services/app_sec/dast/site_profiles/create_service_spec.rb

@@ -36,7 +36,6 @@ RSpec.describe AppSec::Dast::SiteProfiles::CreateService do
 
     let(:status) { subject.status }
     let(:message) { subject.message }
-    let(:errors) { subject.errors }
     let(:payload) { subject.payload }
 
     context 'when a user does not have access to the project' do
@@ -111,18 +110,6 @@ RSpec.describe AppSec::Dast::SiteProfiles::CreateService do
         end
       end
 
-      context 'when the target url is localhost' do
-        let(:target_url) { 'http://localhost:3000/hello-world' }
-
-        it 'returns an error status' do
-          expect(status).to eq(:error)
-        end
-
-        it 'populates errors' do
-          expect(errors).to include('Url is blocked: Requests to localhost are not allowed')
-        end
-      end
-
       context 'when excluded_urls is nil' do
         let(:excluded_urls) { nil }
 

ee/spec/services/app_sec/dast/site_profiles/update_service_spec.rb

@@ -113,18 +113,6 @@ RSpec.describe AppSec::Dast::SiteProfiles::UpdateService do
         end
       end
 
-      context 'when the target url is localhost' do
-        let(:new_target_url) { 'http://localhost:3000/hello-world' }
-
-        it 'returns an error status' do
-          expect(status).to eq(:error)
-        end
-
-        it 'populates errors' do
-          expect(errors).to include('Url is blocked: Requests to localhost are not allowed')
-        end
-      end
-
       context 'when the target url is nil' do
         let(:params) { default_params.merge(target_url: nil) }
 

ee/spec/services/dast_sites/find_or_create_service_spec.rb

@@ -49,12 +49,12 @@ RSpec.describe DastSites::FindOrCreateService do
         end
       end
 
-      context 'when the target url is localhost' do
-        let(:url) { 'http://localhost:3000/hello-world' }
+      context 'when the record is invalid' do
+        let(:url) { 'i-am-not-a-url' }
 
         it 'raises an exception' do
           expect { subject }.to raise_error(ActiveRecord::RecordInvalid) do |err|
-            expect(err.record.errors.full_messages).to include('Url is blocked: Requests to localhost are not allowed')
+            expect(err.record.errors.full_messages).to include('Url is blocked: Only allowed schemes are http, https')
           end
         end
       end