Merge branch '833-validate-urls-on-pull-from-remote-repository' into 'master'

Resolves error when using a bad formatted URL on "Pull from remote" & "Push to remote" sections

Closes #833

See merge request gitlab-org/gitlab-ee!3389

app/models/project.rb

@@ -635,6 +635,8 @@ class Project < ActiveRecord::Base
     else
       super
     end
+  rescue
+    super
   end
 
   def valid_import_url?

changelogs/unreleased-ee/833-validate-urls-on-pull-from-remote-repository.yml

+---
+title: Fix error when entering an invalid url to push to or pull from a remote repository
+merge_request: 3389
+author:
+type: fixed

ee/app/controllers/projects/mirrors_controller.rb

@@ -46,7 +46,6 @@ class Projects::MirrorsController < Projects::ApplicationController
 
     respond_to do |format|
       format.html { redirect_to_repository_settings(@project) }
-
       format.json do
         if @project.errors.present?
           render json: @project.errors, status: :unprocessable_entity

ee/app/models/remote_mirror.rb

@@ -16,6 +16,7 @@ class RemoteMirror < ActiveRecord::Base
   validates :url, presence: true, url: { protocols: %w(ssh git http https), allow_blank: true }
 
   validate  :url_availability, if: -> (mirror) { mirror.url_changed? || mirror.enabled? }
+  validates :url, addressable_url: true, if: :url_changed?
 
   after_save :set_override_remote_mirror_available, unless: -> { Gitlab::CurrentSettings.current_application_settings.remote_mirror_available }
   after_save :refresh_remote, if: :mirror_url_changed?
@@ -109,6 +110,7 @@ class RemoteMirror < ActiveRecord::Base
   end
 
   def url=(value)
+    return super(value) unless Gitlab::UrlSanitizer.valid?(value)
     mirror_url = Gitlab::UrlSanitizer.new(value)
     self.credentials = mirror_url.credentials
 
@@ -119,6 +121,8 @@ class RemoteMirror < ActiveRecord::Base
     if super
       Gitlab::UrlSanitizer.new(super, credentials: credentials).full_url
     end
+  rescue
+    super
   end
 
   def safe_url

spec/controllers/projects/mirrors_controller_spec.rb

@@ -178,6 +178,7 @@ describe Projects::MirrorsController do
 
   describe '#update' do
     let(:project) { create(:project, :repository, :mirror, :remote_mirror) }
+    let(:attributes) { { project: { mirror_user_id: project.owner.id, mirror_trigger_builds: 0 }, namespace_id: project.namespace.to_param, project_id: project.to_param } }
 
     before do
       sign_in(project.owner)
@@ -246,19 +247,55 @@ describe Projects::MirrorsController do
       end
     end
 
-    context 'HTML' do
+    context 'with a valid URL for a pull' do
       it 'processes a successful update' do
-        do_put(project, import_url: 'https://updated.example.com')
+        do_put(project, username_only_import_url: "https://updated.example.com")
 
         expect(response).to redirect_to(project_settings_repository_path(project))
         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, username_only_import_url: "ftp://invalid.invalid'")
+
+        expect(response).to redirect_to(project_settings_repository_path(project))
+        expect(flash[:alert]).to match(/must be a valid URL/)
+      end
+    end
+
+    context 'With valid URL for a push' do
+      before do
+        @remote_mirror_attributes = { "0" => { "enabled" => "0", url: 'https://updated.example.com' } }
+      end
+
+      it 'processes a successful update' do
+        do_put(project, remote_mirrors_attributes: @remote_mirror_attributes)
+
+        expect(response).to redirect_to(project_settings_repository_path(project))
+        expect(flash[:notice]).to match(/successfully updated/)
+      end
+
+      it 'should create a RemoteMirror object' do
+        expect { do_put(project, remote_mirrors_attributes: @remote_mirror_attributes) }.to change(RemoteMirror, :count).by(1)
+      end
+    end
+
+    context 'With invalid URL for a push' do
+      before do
+        @remote_mirror_attributes = { "0" => { "enabled" => "0", url: 'ftp://invalid.invalid' } }
+      end
 
       it 'processes an unsuccessful update' do
-        do_put(project, import_url: 'ftp://invalid.invalid')
+        do_put(project, remote_mirrors_attributes: @remote_mirror_attributes)
 
         expect(response).to redirect_to(project_settings_repository_path(project))
-        expect(flash[:alert]).to match(/valid URL/)
+        expect(flash[:alert]).to match(/must be a valid URL/)
+      end
+
+      it 'should not create a RemoteMirror object' do
+        expect { do_put(project, remote_mirrors_attributes: @remote_mirror_attributes) }.not_to change(RemoteMirror, :count)
       end
     end
   end

spec/factories/remote_mirrors.rb

+require_relative '../support/test_env'
+
+FactoryGirl.define do
+  factory :remote_mirror, class: 'RemoteMirror' do
+    association :project, :repository
+    url "http://foo:bar@test.com"
+  end
+end

spec/models/remote_mirror_spec.rb

 require 'rails_helper'
 
 describe RemoteMirror do
+  describe 'URL validation' do
+    context 'with a valid URL' do
+      it 'should be valid' do
+        remote_mirror = build(:remote_mirror)
+        expect(remote_mirror).to be_valid
+      end
+    end
+
+    context 'with an invalid URL' do
+      it 'should not be valid' do
+        remote_mirror = build(:remote_mirror, url: 'ftp://invalid.invalid')
+        expect(remote_mirror).not_to be_valid
+        expect(remote_mirror.errors[:url].size).to eq(2)
+      end
+    end
+  end
+
   describe 'encrypting credentials' do
     context 'when setting URL for a first time' do
       it 'stores the URL without credentials' do