From 9756ff1544623f2a3cb1ebbceb8059df1623516a Mon Sep 17 00:00:00 2001 From: gloria_odipo Date: Wed, 3 Dec 2025 19:00:21 +0300 Subject: [PATCH] Validate URL during pull mirroring via UI Unify API and UI behavior when pull mirroring by validating URL Changelog: changed --- .../beta/validate_pull_mirror_url.yml | 10 +++ .../pull_mirrors/update_service.rb | 22 +++++ .../ee/projects/mirrors_controller_spec.rb | 86 ++++++++++++++++--- ee/spec/features/projects/mirror_spec.rb | 6 ++ .../ee/repository_mirrors_settings_spec.rb | 3 + ee/spec/requests/api/project_mirror_spec.rb | 6 ++ .../projects/mirrors_controller_spec.rb | 3 + .../pull_mirrors/update_service_spec.rb | 70 +++++++++++++++ 8 files changed, 192 insertions(+), 14 deletions(-) create mode 100644 config/feature_flags/beta/validate_pull_mirror_url.yml diff --git a/config/feature_flags/beta/validate_pull_mirror_url.yml b/config/feature_flags/beta/validate_pull_mirror_url.yml new file mode 100644 index 00000000000000..2ff7f0597f968a --- /dev/null +++ b/config/feature_flags/beta/validate_pull_mirror_url.yml @@ -0,0 +1,10 @@ +--- +name: validate_pull_mirror_url +description: Validate pull mirror URLs during configuration via UI +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/work_items/419644 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/215056 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/583793 +milestone: '18.7' +group: group::source code +type: beta +default_enabled: false diff --git a/ee/app/services/repositories/pull_mirrors/update_service.rb b/ee/app/services/repositories/pull_mirrors/update_service.rb index d440ad56c26d09..c4b2070397a1d7 100644 --- a/ee/app/services/repositories/pull_mirrors/update_service.rb +++ b/ee/app/services/repositories/pull_mirrors/update_service.rb @@ -22,6 +22,12 @@ def update_mirror update_project_import_relations + is_valid = project.valid? + + validate_import_url if is_valid + + return false if project.errors.any? + project.save # It's possible that import state is not created, when user doesn't set an import_url @@ -56,6 +62,22 @@ def allowed_attributes def allowed? Ability.allowed?(current_user, :admin_remote_mirror, project) end + + def validate_import_url + return unless Feature.enabled?(:validate_pull_mirror_url, project) + + import_url = allowed_attributes[:import_url] || allowed_attributes[:username_only_import_url] + return if import_url.blank? + return if mirror_disabled? + + result = Import::ValidateRemoteGitEndpointService.new( + url: import_url, + user: allowed_attributes.dig(:import_data_attributes, :user), + password: allowed_attributes.dig(:import_data_attributes, :password) + ).execute + + project.errors.add(:import_url, result.message) if result.error? + end end end end diff --git a/ee/spec/controllers/ee/projects/mirrors_controller_spec.rb b/ee/spec/controllers/ee/projects/mirrors_controller_spec.rb index e3adb936f29356..331bf55dbfe906 100644 --- a/ee/spec/controllers/ee/projects/mirrors_controller_spec.rb +++ b/ee/spec/controllers/ee/projects/mirrors_controller_spec.rb @@ -5,6 +5,12 @@ RSpec.describe Projects::MirrorsController, feature_category: :source_code_management do include ReactiveCachingHelpers + before do + allow_next_instance_of(Import::ValidateRemoteGitEndpointService) do |service| + allow(service).to receive(:execute).and_return(ServiceResponse.success) + end + end + describe 'setting up a remote mirror' do let_it_be(:project) { create(:project, :repository) } let_it_be(:first_owner) { project.first_owner } @@ -223,30 +229,82 @@ end end - context 'with a valid URL for a pull' do - it 'processes a successful update' do - do_put(project, mirror: true, username_only_import_url: "https://updated.example.com") + context 'when URL validation is enabled' do + before do + stub_feature_flags(validate_pull_mirror_url: true) + end - expect(response).to redirect_to(project_settings_repository_path(project, anchor: 'js-push-remote-settings')) - expect(flash[:notice]).to match(/successfully updated/) + context 'with a valid URL for a pull' do + it 'processes a successful update' do + do_put(project, mirror: true, username_only_import_url: "https://updated.example.com") + + expect(response).to redirect_to(project_settings_repository_path(project, anchor: 'js-push-remote-settings')) + expect(flash[:notice]).to match(/successfully updated/) + end + end + + context 'with a invalid URL for a pull' do + it 'processes an unsuccessful update' do + do_put(project, mirror: true, username_only_import_url: "ftp://invalid.invalid'") + + expect(response).to redirect_to(project_settings_repository_path(project, anchor: 'js-push-remote-settings')) + expect(flash[:alert]).to match(/is blocked/) + end + end + + context 'with an invalid port for a pull' do + it 'processes an unsuccessful update' do + do_put(project, mirror: true, username_only_import_url: 'https://updated.example.com:wrong_port') + + expect(response).to redirect_to(project_settings_repository_path(project, anchor: 'js-push-remote-settings')) + expect(flash[:alert]).to match(/is blocked/) + end + end + + context 'when URL is valid but not accessible' do + it 'returns an error' do + allow_next_instance_of(Import::ValidateRemoteGitEndpointService) do |service| + allow(service).to receive(:execute).and_return( + ServiceResponse.error(message: 'Unable to access repository with the URL and credentials provided') + ) + end + + do_put(project, mirror: true, username_only_import_url: 'https://updated.example.com') + + expect(response).to redirect_to(project_settings_repository_path(project, anchor: 'js-push-remote-settings')) + expect(flash[:alert]).to match(/Unable to access repository/) + end end end - context 'with a invalid URL for a pull' do - it 'processes an unsuccessful update' do - do_put(project, mirror: true, username_only_import_url: "ftp://invalid.invalid'") + context 'when URL validation is disabled' do + before do + stub_feature_flags(validate_pull_mirror_url: false) + end - expect(response).to redirect_to(project_settings_repository_path(project, anchor: 'js-push-remote-settings')) - expect(flash[:alert]).to match(/is blocked/) + context 'with an invalid URL for a pull' do + it 'allows the update without validation' do + do_put(project, mirror: true, username_only_import_url: "http://not-existing-url-1/git") + + expect(response).to redirect_to(project_settings_repository_path(project, anchor: 'js-push-remote-settings')) + expect(flash[:notice]).to match(/successfully updated/) + end end end - context 'with an invalid port for a pull' do - it 'processes an unsuccessful update' do - do_put(project, mirror: true, username_only_import_url: 'https://updated.example.com:wrong_port') + context 'when disabling an existing pull mirror' do + let_it_be(:project) { create(:project, :repository, :mirror, import_url: 'https://example.com') } + + it 'does not validate URL when disabling mirror' do + expect_next_instance_of(Projects::UpdateService) do |service| + expect(service).to receive(:execute).and_call_original + end + + do_put(project, mirror: false) expect(response).to redirect_to(project_settings_repository_path(project, anchor: 'js-push-remote-settings')) - expect(flash[:alert]).to match(/is blocked/) + expect(flash[:notice]).to match(/successfully disabled/) + expect(project.reload.mirror).to be_falsey end end diff --git a/ee/spec/features/projects/mirror_spec.rb b/ee/spec/features/projects/mirror_spec.rb index 45a52bd8872a87..42abca8bd8d3ad 100644 --- a/ee/spec/features/projects/mirror_spec.rb +++ b/ee/spec/features/projects/mirror_spec.rb @@ -10,6 +10,12 @@ let(:import_state) { create(:import_state, :mirror, :finished, project: project) } let(:user) { create(:user) } + before do + allow_next_instance_of(Import::ValidateRemoteGitEndpointService) do |service| + allow(service).to receive(:execute).and_return(ServiceResponse.success) + end + end + describe 'On a project' do before do project.add_maintainer(user) diff --git a/ee/spec/features/projects/settings/ee/repository_mirrors_settings_spec.rb b/ee/spec/features/projects/settings/ee/repository_mirrors_settings_spec.rb index b9b088e615b2cc..a6cd0a69ceb1ae 100644 --- a/ee/spec/features/projects/settings/ee/repository_mirrors_settings_spec.rb +++ b/ee/spec/features/projects/settings/ee/repository_mirrors_settings_spec.rb @@ -11,6 +11,9 @@ before do project.add_maintainer(user) sign_in(user) + allow_next_instance_of(Import::ValidateRemoteGitEndpointService) do |service| + allow(service).to receive(:execute).and_return(ServiceResponse.success) + end end context 'unlicensed' do diff --git a/ee/spec/requests/api/project_mirror_spec.rb b/ee/spec/requests/api/project_mirror_spec.rb index 5342806f330f7a..f29cf8d97c1785 100644 --- a/ee/spec/requests/api/project_mirror_spec.rb +++ b/ee/spec/requests/api/project_mirror_spec.rb @@ -455,6 +455,12 @@ def project_member(role, user) let(:mirror_url) { 'http://example.com' } shared_examples 'pull mirror configuration' do + before do + allow_next_instance_of(Import::ValidateRemoteGitEndpointService) do |service| + allow(service).to receive(:execute).and_return(ServiceResponse.success) + end + end + context 'when user is missing' do let(:current_user) { nil } diff --git a/ee/spec/requests/projects/mirrors_controller_spec.rb b/ee/spec/requests/projects/mirrors_controller_spec.rb index dae8eb19458481..1953369d25ff31 100644 --- a/ee/spec/requests/projects/mirrors_controller_spec.rb +++ b/ee/spec/requests/projects/mirrors_controller_spec.rb @@ -18,6 +18,9 @@ before do project.add_maintainer(user) login_as(user) + allow_next_instance_of(Import::ValidateRemoteGitEndpointService) do |service| + allow(service).to receive(:execute).and_return(ServiceResponse.success) + end end it 'updates a pull mirror configuration' do diff --git a/ee/spec/services/repositories/pull_mirrors/update_service_spec.rb b/ee/spec/services/repositories/pull_mirrors/update_service_spec.rb index fda2a96e0fa42a..cf78957966e3cf 100644 --- a/ee/spec/services/repositories/pull_mirrors/update_service_spec.rb +++ b/ee/spec/services/repositories/pull_mirrors/update_service_spec.rb @@ -33,6 +33,12 @@ let(:updated_project) { execute.payload[:project] } + before do + allow_next_instance_of(Import::ValidateRemoteGitEndpointService) do |service| + allow(service).to receive(:execute).and_return(ServiceResponse.success) + end + end + it 'updates a pull mirror' do is_expected.to be_success @@ -146,5 +152,69 @@ expect(execute.message.full_messages).to include(/Import URL is blocked/) end end + + context 'when URL validation is enabled' do + before do + stub_feature_flags(validate_pull_mirror_url: true) + end + + context 'when import URL is not accessible' do + let(:url) { 'http://not-existing-url-1/git' } + + before do + allow_next_instance_of(Import::ValidateRemoteGitEndpointService) do |service| + allow(service).to receive(:execute).and_return( + ServiceResponse.error(message: 'Unable to access repository with the URL and credentials provided') + ) + end + end + + it 'returns an error' do + is_expected.to be_error + expect(execute.message.full_messages).to( + include(/Unable to access repository with the URL and credentials provided/) + ) + end + end + end + + context 'when URL validation is disabled' do + before do + stub_feature_flags(validate_pull_mirror_url: false) + end + + context 'with an inaccessible URL' do + it 'allows the update without validation' do + service_with_inaccessible_url = described_class.new(project, user, params.merge(import_url: 'http://not-existing-url-1/git')) + result = service_with_inaccessible_url.execute + + expect(result).to be_success + end + end + end + + context 'when import_url is blank' do + let(:params) { { import_url: '' } } + + before do + create(:import_state, project: project) + end + + it 'skips validation' do + expect(Import::ValidateRemoteGitEndpointService).not_to receive(:new) + + is_expected.to be_success + end + end + + context 'when mirror is disabled and url is provided' do + let(:enabled) { false } + + it 'skips validation' do + expect(Import::ValidateRemoteGitEndpointService).not_to receive(:new) + + is_expected.to be_success + end + end end end -- GitLab