Commit c73d0ff7 authored by Mayra Cabrera's avatar Mayra Cabrera

Fixes invalid external locations

- Ensures every location, whether local or remote is a valid yml file
- Dry up files by creating a base class
- Adjust specs

Closes #4874
Closes #4873
parent 5ceb0c6d
module Gitlab
module Ci
module External
module File
class Base
YAML_WHITELIST_EXTENSION = /(yml|yaml)$/i.freeze
def initialize(location, opts = {})
@location = location
end
def valid?
location.match(YAML_WHITELIST_EXTENSION) && content
end
def content
raise NotImplementedError, 'content must be implemented and return a string or nil'
end
def error_message
raise NotImplementedError, 'error_message must be implemented and return a string'
end
end
end
end
end
end
...@@ -2,27 +2,28 @@ module Gitlab ...@@ -2,27 +2,28 @@ module Gitlab
module Ci module Ci
module External module External
module File module File
class Local class Local < Base
attr_reader :location, :project, :sha attr_reader :location, :project, :sha
def initialize(location, opts = {}) def initialize(location, opts = {})
@location = location super
@project = opts.fetch(:project) @project = opts.fetch(:project)
@sha = opts.fetch(:sha) @sha = opts.fetch(:sha)
end end
def valid? def content
local_file_content @content ||= fetch_local_content
end end
def content def error_message
local_file_content "Local file '#{location}' is not valid."
end end
private private
def local_file_content def fetch_local_content
@local_file_content ||= project.repository.blob_data_at(sha, location) project.repository.blob_data_at(sha, location)
end end
end end
end end
......
...@@ -2,29 +2,25 @@ module Gitlab ...@@ -2,29 +2,25 @@ module Gitlab
module Ci module Ci
module External module External
module File module File
class Remote class Remote < Base
include Gitlab::Utils::StrongMemoize include Gitlab::Utils::StrongMemoize
attr_reader :location attr_reader :location
def initialize(location, opts = {})
@location = location
end
def valid?
::Gitlab::UrlSanitizer.valid?(location) && content
end
def content def content
return @content if defined?(@content) return @content if defined?(@content)
@content = strong_memoize(:content) do @content = strong_memoize(:content) do
begin begin
HTTParty.get(location) HTTParty.get(location)
rescue HTTParty::Error, Timeout::Error rescue HTTParty::Error, Timeout::Error, SocketError
false nil
end end
end end
end end
def error_message
"Remote file '#{location}' is not valid."
end
end end
end end
end end
......
...@@ -17,10 +17,8 @@ module Gitlab ...@@ -17,10 +17,8 @@ module Gitlab
attr_reader :locations, :project, :sha attr_reader :locations, :project, :sha
def build_external_file(location) def build_external_file(location)
remote_file = Gitlab::Ci::External::File::Remote.new(location) if ::Gitlab::UrlSanitizer.valid?(location)
Gitlab::Ci::External::File::Remote.new(location)
if remote_file.valid?
remote_file
else else
options = { project: project, sha: sha } options = { project: project, sha: sha }
Gitlab::Ci::External::File::Local.new(location, options) Gitlab::Ci::External::File::Local.new(location, options)
......
...@@ -28,7 +28,7 @@ module Gitlab ...@@ -28,7 +28,7 @@ module Gitlab
def validate_external_file(external_file) def validate_external_file(external_file)
unless external_file.valid? unless external_file.valid?
raise FileError, "External file: '#{external_file.location}' should be a valid local or remote file" raise FileError, external_file.error_message
end end
end end
......
...@@ -46,7 +46,7 @@ describe EE::Gitlab::Ci::Config do ...@@ -46,7 +46,7 @@ describe EE::Gitlab::Ci::Config do
context "when gitlab_ci_yml has valid 'include' defined" do context "when gitlab_ci_yml has valid 'include' defined" do
before do before do
allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:local_file_content).and_return(local_file_content) allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:fetch_local_content).and_return(local_file_content)
WebMock.stub_request(:get, remote_location).to_return(body: remote_file_content) WebMock.stub_request(:get, remote_location).to_return(body: remote_file_content)
end end
...@@ -85,7 +85,7 @@ describe EE::Gitlab::Ci::Config do ...@@ -85,7 +85,7 @@ describe EE::Gitlab::Ci::Config do
it 'raises error YamlProcessor validationError' do it 'raises error YamlProcessor validationError' do
expect { config }.to raise_error( expect { config }.to raise_error(
::Gitlab::Ci::YamlProcessor::ValidationError, ::Gitlab::Ci::YamlProcessor::ValidationError,
"External file: 'invalid' should be a valid local or remote file" "Local file 'invalid' is not valid."
) )
end end
end end
......
...@@ -4,12 +4,12 @@ describe Gitlab::Ci::External::File::Local do ...@@ -4,12 +4,12 @@ describe Gitlab::Ci::External::File::Local do
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
let(:local_file) { described_class.new(location, { project: project, sha: '12345' }) } let(:local_file) { described_class.new(location, { project: project, sha: '12345' }) }
describe "#valid?" do describe '#valid?' do
context 'when is a valid local path' do context 'when is a valid local path' do
let(:location) { '/vendor/gitlab-ci-yml/existent-file.yml' } let(:location) { '/vendor/gitlab-ci-yml/existent-file.yml' }
before do before do
allow_any_instance_of(described_class).to receive(:local_file_content).and_return("image: 'ruby2:2'") allow_any_instance_of(described_class).to receive(:fetch_local_content).and_return("image: 'ruby2:2'")
end end
it 'should return true' do it 'should return true' do
...@@ -25,29 +25,52 @@ describe Gitlab::Ci::External::File::Local do ...@@ -25,29 +25,52 @@ describe Gitlab::Ci::External::File::Local do
end end
end end
describe "#content" do context 'when is not a yaml file' do
let(:location) { '/config/application.rb' }
it 'should return false' do
expect(local_file.valid?).to be_falsy
end
end
end
describe '#content' do
context 'with a a valid file' do
let(:local_file_content) do let(:local_file_content) do
<<~HEREDOC <<~HEREDOC
before_script: before_script:
- apt-get update -qq && apt-get install -y -qq sqlite3 libsqlite3-dev nodejs - apt-get update -qq && apt-get install -y -qq sqlite3 libsqlite3-dev nodejs
- ruby -v - ruby -v
- which ruby - which ruby
- gem install bundler --no-ri --no-rdoc - gem install bundler --no-ri --no-rdoc
- bundle install --jobs $(nproc) "${FLAGS[@]}" - bundle install --jobs $(nproc) "${FLAGS[@]}"
HEREDOC HEREDOC
end end
let(:location) { '/vendor/gitlab-ci-yml/existent-file.yml' }
before do
allow_any_instance_of(described_class).to receive(:fetch_local_content).and_return(local_file_content)
end
context 'with a local file' do it 'should return the content of the file' do
let(:location) { '/vendor/gitlab-ci-yml/non-existent-file.yml' } expect(local_file.content).to eq(local_file_content)
end
end
before do context 'with an invalid file' do
allow_any_instance_of(described_class).to receive(:local_file_content).and_return(local_file_content) let(:location) { '/vendor/gitlab-ci-yml/non-existent-file.yml' }
end
it 'should return the content of the file' do it 'should be nil' do
expect(local_file.content).to eq(local_file_content) expect(local_file.content).to be_nil
end
end end
end end
end end
describe '#error_message' do
let(:location) { '/vendor/gitlab-ci-yml/non-existent-file.yml' }
it 'should return an error message' do
expect(local_file.error_message).to eq("Local file '#{location}' is not valid.")
end
end
end end
...@@ -25,7 +25,7 @@ describe Gitlab::Ci::External::File::Remote do ...@@ -25,7 +25,7 @@ describe Gitlab::Ci::External::File::Remote do
end end
end end
context 'when is not a valid remote url' do context 'with an irregular url' do
let(:location) { 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' } let(:location) { 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
it 'should return false' do it 'should return false' do
...@@ -42,6 +42,14 @@ describe Gitlab::Ci::External::File::Remote do ...@@ -42,6 +42,14 @@ describe Gitlab::Ci::External::File::Remote do
expect(remote_file.valid?).to be_falsy expect(remote_file.valid?).to be_falsy
end end
end end
context 'when is not a yaml file' do
let(:location) { 'https://asdasdasdaj48ggerexample.com' }
it 'should be falsy' do
expect(remote_file.valid?).to be_falsy
end
end
end end
describe "#content" do describe "#content" do
...@@ -64,5 +72,25 @@ describe Gitlab::Ci::External::File::Remote do ...@@ -64,5 +72,25 @@ describe Gitlab::Ci::External::File::Remote do
expect(remote_file.content).to be_falsy expect(remote_file.content).to be_falsy
end end
end end
context 'with an invalid remote url' do
let(:location) { 'https://asdasdasdaj48ggerexample.com' }
before do
WebMock.stub_request(:get, location).to_raise(SocketError.new('Some HTTP error'))
end
it 'should be nil' do
expect(remote_file.content).to be_nil
end
end
end
describe "#error_message" do
let(:location) { 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
it 'should return an error message' do
expect(remote_file.error_message).to eq("Remote file '#{location}' is not valid.")
end
end end
end end
...@@ -19,18 +19,23 @@ describe Gitlab::Ci::External::Processor do ...@@ -19,18 +19,23 @@ describe Gitlab::Ci::External::Processor do
it 'should raise an error' do it 'should raise an error' do
expect { processor.perform }.to raise_error( expect { processor.perform }.to raise_error(
described_class::FileError, described_class::FileError,
"External file: '/vendor/gitlab-ci-yml/non-existent-file.yml' should be a valid local or remote file" "Local file '/vendor/gitlab-ci-yml/non-existent-file.yml' is not valid."
) )
end end
end end
context 'when an invalid remote file is defined' do context 'when an invalid remote file is defined' do
let(:values) { { include: 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml', image: 'ruby:2.2' } } let(:remote_file) { 'http://doesntexist.com/.gitlab-ci-1.yml' }
let(:values) { { include: remote_file, image: 'ruby:2.2' } }
before do
WebMock.stub_request(:get, remote_file).to_raise(SocketError.new('Some HTTP error'))
end
it 'should raise an error' do it 'should raise an error' do
expect { processor.perform }.to raise_error( expect { processor.perform }.to raise_error(
described_class::FileError, described_class::FileError,
"External file: 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' should be a valid local or remote file" "Remote file '#{remote_file}' is not valid."
) )
end end
end end
...@@ -85,7 +90,7 @@ describe Gitlab::Ci::External::Processor do ...@@ -85,7 +90,7 @@ describe Gitlab::Ci::External::Processor do
end end
before do before do
allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:local_file_content).and_return(local_file_content) allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:fetch_local_content).and_return(local_file_content)
end end
it 'should append the file to the values' do it 'should append the file to the values' do
...@@ -124,7 +129,7 @@ describe Gitlab::Ci::External::Processor do ...@@ -124,7 +129,7 @@ describe Gitlab::Ci::External::Processor do
before do before do
local_file_content = File.read("#{Rails.root}/spec/ee/fixtures/gitlab/ci/external_files/.gitlab-ci-template-1.yml") local_file_content = File.read("#{Rails.root}/spec/ee/fixtures/gitlab/ci/external_files/.gitlab-ci-template-1.yml")
allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:local_file_content).and_return(local_file_content) allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:fetch_local_content).and_return(local_file_content)
WebMock.stub_request(:get, remote_file).to_return(body: remote_file_content) WebMock.stub_request(:get, remote_file).to_return(body: remote_file_content)
end end
...@@ -143,7 +148,7 @@ describe Gitlab::Ci::External::Processor do ...@@ -143,7 +148,7 @@ describe Gitlab::Ci::External::Processor do
let(:local_file_content) { 'invalid content file ////' } let(:local_file_content) { 'invalid content file ////' }
before do before do
allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:local_file_content).and_return(local_file_content) allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:fetch_local_content).and_return(local_file_content)
end end
it 'should raise an error' do it 'should raise an error' 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