Restrict nested group names to prevent ambiguous routesdz-nested-groups-restrictions

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

4 files changed, 34 insertions, 7 deletions

diff --git a/app/validators/namespace_validator.rb b/app/validators/namespace_validator.rb
index eb3ed31b65b..03921db6947 100644
--- a/app/validators/namespace_validator.rb
+++ b/app/validators/namespace_validator.rb
@@ -35,12 +35,21 @@ class NamespaceValidator < ActiveModel::EachValidator
     users
   ].freeze
 
+  WILDCARD_ROUTES = %w[tree commits wikis new edit create update logs_tree
+                       preview blob blame raw files create_dir find_file].freeze
+
+  STRICT_RESERVED = (RESERVED + WILDCARD_ROUTES).freeze
+
   def self.valid?(value)
     !reserved?(value) && follow_format?(value)
   end
 
-  def self.reserved?(value)
-    RESERVED.include?(value)
+  def self.reserved?(value, strict: false)
+    if strict
+      STRICT_RESERVED.include?(value)
+    else
+      RESERVED.include?(value)
+    end
   end
 
   def self.follow_format?(value)
@@ -54,7 +63,9 @@ class NamespaceValidator < ActiveModel::EachValidator
       record.errors.add(attribute, Gitlab::Regex.namespace_regex_message)
     end
 
-    if reserved?(value)
+    strict = record.is_a?(Group) && record.parent_id
+
+    if reserved?(value, strict: strict)
       record.errors.add(attribute, "#{value} is a reserved name")
     end
   end
diff --git a/app/validators/project_path_validator.rb b/app/validators/project_path_validator.rb
index 36279daa743..ee2ae65be7b 100644
--- a/app/validators/project_path_validator.rb
+++ b/app/validators/project_path_validator.rb
@@ -14,10 +14,8 @@ class ProjectPathValidator < ActiveModel::EachValidator
   #  without tree as reserved name routing can match 'group/project' as group name,
   #  'tree' as project name and 'deploy_keys' as route.
   #
-  RESERVED = (NamespaceValidator::RESERVED -
-              %w[dashboard help ci admin search notes services assets profile public] +
-              %w[tree commits wikis new edit create update logs_tree
-                 preview blob blame raw files create_dir find_file]).freeze
+  RESERVED = (NamespaceValidator::STRICT_RESERVED -
+              %w[dashboard help ci admin search notes services assets profile public]).freeze
 
   def self.valid?(value)
     !reserved?(value)
diff --git a/changelogs/unreleased/dz-nested-groups-restrictions.yml b/changelogs/unreleased/dz-nested-groups-restrictions.yml
new file mode 100644
index 00000000000..2ffb6032525
--- /dev/null
+++ b/changelogs/unreleased/dz-nested-groups-restrictions.yml
@@ -0,0 +1,4 @@
+---
+title: Restrict nested group names to prevent ambiguous routes
+merge_request: 9738
+author:
diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb
index 3f9c4289de9..7525a1b79ee 100644
--- a/spec/models/namespace_spec.rb
+++ b/spec/models/namespace_spec.rb
@@ -28,6 +28,20 @@ describe Namespace, models: true do
       expect(nested).not_to be_valid
       expect(nested.errors[:parent_id].first).to eq('has too deep level of nesting')
     end
+
+    describe 'reserved path validation' do
+      context 'nested group' do
+        let(:group) { build(:group, :nested, path: 'tree') }
+
+        it { expect(group).not_to be_valid }
+      end
+
+      context 'top-level group' do
+        let(:group) { build(:group, path: 'tree') }
+
+        it { expect(group).to be_valid }
+      end
+    end
   end
 
   describe "Respond to" do