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

3 files changed, 12 insertions, 5 deletions

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