Commit 4fa16ea6 authored by Mayra Cabrera's avatar Mayra Cabrera

Fixes error when a bad formatted URL is set on 'Push to'/'Pull from' remote sections

- Adds rescue clase to import_url method
- Ensure RemoteMirror model also validates the url

Closes #833
parent 028ec529
...@@ -635,6 +635,8 @@ class Project < ActiveRecord::Base ...@@ -635,6 +635,8 @@ class Project < ActiveRecord::Base
else else
super super
end end
rescue
super
end end
def valid_import_url? def valid_import_url?
......
---
title: Fix error when entering an invalid url to push to or pull from a remote repository
merge_request: 3389
author:
type: fixed
...@@ -46,7 +46,6 @@ class Projects::MirrorsController < Projects::ApplicationController ...@@ -46,7 +46,6 @@ class Projects::MirrorsController < Projects::ApplicationController
respond_to do |format| respond_to do |format|
format.html { redirect_to_repository_settings(@project) } format.html { redirect_to_repository_settings(@project) }
format.json do format.json do
if @project.errors.present? if @project.errors.present?
render json: @project.errors, status: :unprocessable_entity render json: @project.errors, status: :unprocessable_entity
......
...@@ -16,6 +16,7 @@ class RemoteMirror < ActiveRecord::Base ...@@ -16,6 +16,7 @@ class RemoteMirror < ActiveRecord::Base
validates :url, presence: true, url: { protocols: %w(ssh git http https), allow_blank: true } validates :url, presence: true, url: { protocols: %w(ssh git http https), allow_blank: true }
validate :url_availability, if: -> (mirror) { mirror.url_changed? || mirror.enabled? } 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 :set_override_remote_mirror_available, unless: -> { Gitlab::CurrentSettings.current_application_settings.remote_mirror_available }
after_save :refresh_remote, if: :mirror_url_changed? after_save :refresh_remote, if: :mirror_url_changed?
...@@ -109,6 +110,7 @@ class RemoteMirror < ActiveRecord::Base ...@@ -109,6 +110,7 @@ class RemoteMirror < ActiveRecord::Base
end end
def url=(value) def url=(value)
return super(value) unless Gitlab::UrlSanitizer.valid?(value)
mirror_url = Gitlab::UrlSanitizer.new(value) mirror_url = Gitlab::UrlSanitizer.new(value)
self.credentials = mirror_url.credentials self.credentials = mirror_url.credentials
...@@ -119,6 +121,8 @@ class RemoteMirror < ActiveRecord::Base ...@@ -119,6 +121,8 @@ class RemoteMirror < ActiveRecord::Base
if super if super
Gitlab::UrlSanitizer.new(super, credentials: credentials).full_url Gitlab::UrlSanitizer.new(super, credentials: credentials).full_url
end end
rescue
super
end end
def safe_url def safe_url
......
...@@ -178,6 +178,7 @@ describe Projects::MirrorsController do ...@@ -178,6 +178,7 @@ describe Projects::MirrorsController do
describe '#update' do describe '#update' do
let(:project) { create(:project, :repository, :mirror, :remote_mirror) } 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 before do
sign_in(project.owner) sign_in(project.owner)
...@@ -246,19 +247,55 @@ describe Projects::MirrorsController do ...@@ -246,19 +247,55 @@ describe Projects::MirrorsController do
end end
end end
context 'HTML' do context 'with a valid URL for a pull' do
it 'processes a successful update' 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(response).to redirect_to(project_settings_repository_path(project))
expect(flash[:notice]).to match(/successfully updated/) expect(flash[:notice]).to match(/successfully updated/)
end 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 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(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 end
end end
......
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
require 'rails_helper' require 'rails_helper'
describe RemoteMirror do 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 describe 'encrypting credentials' do
context 'when setting URL for a first time' do context 'when setting URL for a first time' do
it 'stores the URL without credentials' do it 'stores the URL without credentials' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment