Commit 4ebc674a authored by Grzegorz Bizon's avatar Grzegorz Bizon

Merge branch 'add_project_ci_config_path_leading_slash_validation' into 'master'

Adds validation for Project#ci_config_path not to contain leading slash

Closes #34683

See merge request gitlab-org/gitlab-ce!15672
parents 3e78b5ae 70194cfc
...@@ -234,8 +234,8 @@ class Project < ActiveRecord::Base ...@@ -234,8 +234,8 @@ class Project < ActiveRecord::Base
validates :creator, presence: true, on: :create validates :creator, presence: true, on: :create
validates :description, length: { maximum: 2000 }, allow_blank: true validates :description, length: { maximum: 2000 }, allow_blank: true
validates :ci_config_path, validates :ci_config_path,
format: { without: /\.{2}/, format: { without: /(\.{2}|\A\/)/,
message: 'cannot include directory traversal.' }, message: 'cannot include leading slash or directory traversal.' },
length: { maximum: 255 }, length: { maximum: 255 },
allow_blank: true allow_blank: true
validates :name, validates :name,
...@@ -599,7 +599,7 @@ class Project < ActiveRecord::Base ...@@ -599,7 +599,7 @@ class Project < ActiveRecord::Base
def ci_config_path=(value) def ci_config_path=(value)
# Strip all leading slashes so that //foo -> foo # Strip all leading slashes so that //foo -> foo
super(value&.sub(%r{\A/+}, '')&.delete("\0")) super(value&.delete("\0"))
end end
def import_url=(value) def import_url=(value)
......
---
title: Prefer ci_config_path validation for leading slashes instead of sanitizing
the input
merge_request: 15672
author: Christiaan Van den Poel
type: other
...@@ -138,6 +138,7 @@ describe Project do ...@@ -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 validate_length_of(:ci_config_path).is_at_most(255) }
it { is_expected.to allow_value('').for(:ci_config_path) } 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.not_to allow_value('/test/foo').for(:ci_config_path) }
it { is_expected.to validate_presence_of(:creator) } it { is_expected.to validate_presence_of(:creator) }
...@@ -1548,8 +1549,8 @@ describe Project do ...@@ -1548,8 +1549,8 @@ describe Project do
expect(project.ci_config_path).to eq('foo/.gitlab_ci.yml') expect(project.ci_config_path).to eq('foo/.gitlab_ci.yml')
end end
it 'sets a string but removes all leading slashes and null characters' do it 'sets a string but removes all null characters' do
project.update!(ci_config_path: "///f\0oo/\0/.gitlab_ci.yml") project.update!(ci_config_path: "f\0oo/\0/.gitlab_ci.yml")
expect(project.ci_config_path).to eq('foo//.gitlab_ci.yml') expect(project.ci_config_path).to eq('foo//.gitlab_ci.yml')
end end
......
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