From 70194cfc28009f419f74580cf199fe91aa1754ec Mon Sep 17 00:00:00 2001 From: Christiaan Van den Poel Date: Fri, 1 Dec 2017 11:15:15 +0000 Subject: Adds validation for Project#ci_config_path not to contain leading slash --- app/models/project.rb | 6 +++--- .../add_project_ci_config_path_leading_slash_validation.yml | 6 ++++++ spec/models/project_spec.rb | 5 +++-- 3 files changed, 12 insertions(+), 5 deletions(-) create mode 100644 changelogs/unreleased/add_project_ci_config_path_leading_slash_validation.yml diff --git a/app/models/project.rb b/app/models/project.rb index c6f7f56f311..eaf4f555d3b 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -234,8 +234,8 @@ class Project < ActiveRecord::Base validates :creator, presence: true, on: :create validates :description, length: { maximum: 2000 }, allow_blank: true validates :ci_config_path, - format: { without: /\.{2}/, - message: 'cannot include directory traversal.' }, + format: { without: /(\.{2}|\A\/)/, + message: 'cannot include leading slash or directory traversal.' }, length: { maximum: 255 }, allow_blank: true validates :name, @@ -599,7 +599,7 @@ class Project < ActiveRecord::Base def ci_config_path=(value) # Strip all leading slashes so that //foo -> foo - super(value&.sub(%r{\A/+}, '')&.delete("\0")) + super(value&.delete("\0")) end def import_url=(value) diff --git a/changelogs/unreleased/add_project_ci_config_path_leading_slash_validation.yml b/changelogs/unreleased/add_project_ci_config_path_leading_slash_validation.yml new file mode 100644 index 00000000000..1c96bd66ed4 --- /dev/null +++ b/changelogs/unreleased/add_project_ci_config_path_leading_slash_validation.yml @@ -0,0 +1,6 @@ +--- +title: Prefer ci_config_path validation for leading slashes instead of sanitizing + the input +merge_request: 15672 +author: Christiaan Van den Poel +type: other diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index a4abcc49a0d..521b7bd70ba 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -138,6 +138,7 @@ describe Project do it { is_expected.to validate_length_of(:ci_config_path).is_at_most(255) } it { is_expected.to allow_value('').for(:ci_config_path) } it { is_expected.not_to allow_value('test/../foo').for(:ci_config_path) } + it { is_expected.not_to allow_value('/test/foo').for(:ci_config_path) } it { is_expected.to validate_presence_of(:creator) } @@ -1548,8 +1549,8 @@ describe Project do expect(project.ci_config_path).to eq('foo/.gitlab_ci.yml') end - it 'sets a string but removes all leading slashes and null characters' do - project.update!(ci_config_path: "///f\0oo/\0/.gitlab_ci.yml") + it 'sets a string but removes all null characters' do + project.update!(ci_config_path: "f\0oo/\0/.gitlab_ci.yml") expect(project.ci_config_path).to eq('foo//.gitlab_ci.yml') end -- cgit v1.2.1