The dynamic path validator can block out partial paths

So we can block `objects` only when it is contained in `info/lfs` or `gitlab-lfs`

6 files changed, 120 insertions, 95 deletions

diff --git a/app/validators/dynamic_path_validator.rb b/app/validators/dynamic_path_validator.rb
index 3b93a0d0b28..6f04263d4d5 100644
--- a/app/validators/dynamic_path_validator.rb
+++ b/app/validators/dynamic_path_validator.rb
@@ -71,40 +71,42 @@ class DynamicPathValidator < ActiveModel::EachValidator
   #  without tree as reserved name routing can match 'group/project' as group name,
   #  'tree' as project name and 'deploy_keys' as route.
   #
-
   WILDCARD_ROUTES = Set.new(%w[tree commits wikis new edit create update logs_tree
                                preview blob blame raw files create_dir find_file
-                               artifacts graphs refs badges objects folders file])
+                               artifacts graphs refs badges info/lfs/objects
+                               gitlab-lfs/objects environments/folders])
 
   STRICT_RESERVED = (TOP_LEVEL_ROUTES | WILDCARD_ROUTES).freeze
 
-  def self.valid_full_path?(full_path)
-    path_segments = full_path.split('/')
-    root_segment = path_segments.shift
+  def self.valid?(path)
+    path_segments = path.split('/')
 
-    valid?(root_segment, type: :top_level) && valid_wildcard_segments?(path_segments)
+    !reserved?(path) && path_segments.all? { |value| follow_format?(value) }
   end
 
-  def self.valid_wildcard_segments?(segments)
-    segments.all? { |segment| valid?(segment, type: :wildcard) }
+  def self.reserved?(path)
+    path = path.to_s.downcase
+    top_level, wildcard_part = path.split('/', 2)
+
+    includes_reserved_top_level?(top_level) || includes_reserved_wildcard?(wildcard_part)
   end
 
-  def self.valid?(value, type: :strict)
-    !reserved?(value, type: type) && follow_format?(value)
+  def self.includes_reserved_wildcard?(path)
+    WILDCARD_ROUTES.any? do |reserved_word|
+      contains_path_part?(path, reserved_word)
+    end
   end
 
-  def self.reserved?(value, type: :strict)
-    value = value.to_s.downcase
-    case type
-    when :wildcard
-      WILDCARD_ROUTES.include?(value)
-    when :top_level
-      TOP_LEVEL_ROUTES.include?(value)
-    else
-      STRICT_RESERVED.include?(value)
+  def self.includes_reserved_top_level?(path)
+    TOP_LEVEL_ROUTES.any? do |reserved_route|
+      contains_path_part?(path, reserved_route)
     end
   end
 
+  def self.contains_path_part?(path, part)
+    path =~ /(.*\/|\A)#{Regexp.quote(part)}(\/.*|\z)/
+  end
+
   def self.follow_format?(value)
     value =~ Gitlab::Regex.namespace_regex
   end
@@ -116,21 +118,10 @@ class DynamicPathValidator < ActiveModel::EachValidator
       record.errors.add(attribute, Gitlab::Regex.namespace_regex_message)
     end
 
-    if reserved?(value, type: validation_type(record))
-      record.errors.add(attribute, "#{value} is a reserved name")
-    end
-  end
+    full_path = record.respond_to?(:full_path) ? record.full_path : value
 
-  def validation_type(record)
-    case record
-    when Namespace
-      record.has_parent? ? :wildcard : :top_level
-    when Project
-      :wildcard
-    when User
-      :top_level
-    else
-      :strict
+    if reserved?(full_path)
+      record.errors.add(attribute, "#{value} is a reserved name")
     end
   end
 end
diff --git a/lib/constraints/group_url_constrainer.rb b/lib/constraints/group_url_constrainer.rb
index 48a33690cb2..1501f64d537 100644
--- a/lib/constraints/group_url_constrainer.rb
+++ b/lib/constraints/group_url_constrainer.rb
@@ -2,7 +2,7 @@ class GroupUrlConstrainer
   def matches?(request)
     id = request.params[:id]
 
-    return false unless DynamicPathValidator.valid_full_path?(id)
+    return false unless DynamicPathValidator.valid?(id)
 
     Group.find_by_full_path(id).present?
   end
diff --git a/lib/constraints/project_url_constrainer.rb b/lib/constraints/project_url_constrainer.rb
index 72c457d0369..064e1bd78e3 100644
--- a/lib/constraints/project_url_constrainer.rb
+++ b/lib/constraints/project_url_constrainer.rb
@@ -4,7 +4,7 @@ class ProjectUrlConstrainer
     project_path = request.params[:project_id] || request.params[:id]
     full_path = namespace_path + '/' + project_path
 
-    unless DynamicPathValidator.valid_full_path?(full_path)
+    unless DynamicPathValidator.valid?(full_path)
       return false
     end
 
diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb
index 6775fd2f1b8..8624616316c 100644
--- a/spec/models/namespace_spec.rb
+++ b/spec/models/namespace_spec.rb
@@ -34,6 +34,13 @@ describe Namespace, models: true do
         let(:group) { build(:group, :nested, path: 'tree') }
 
         it { expect(group).not_to be_valid }
+
+        it 'rejects nested paths' do
+          parent = create(:group, :nested, path: 'environments')
+          namespace = build(:project, path: 'folders', namespace: parent)
+
+          expect(namespace).not_to be_valid
+        end
       end
 
       context 'top-level group' do
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 726dade93f6..04c90b651a0 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -266,6 +266,13 @@ describe Project, models: true do
 
         expect(project).not_to be_valid
       end
+
+      it 'rejects nested paths' do
+        parent = create(:group, :nested, path: 'environments')
+        project = build(:project, path: 'folders', namespace: parent)
+
+        expect(project).not_to be_valid
+      end
     end
   end
 
diff --git a/spec/validators/dynamic_path_validator_spec.rb b/spec/validators/dynamic_path_validator_spec.rb
index b931a4a2dc3..71098b17dfd 100644
--- a/spec/validators/dynamic_path_validator_spec.rb
+++ b/spec/validators/dynamic_path_validator_spec.rb
@@ -12,23 +12,30 @@ describe DynamicPathValidator do
   # Pass in a full path and get the last segment before a wildcard
   # That's not a parameter
   # `/*namespace_id/:project_id/builds/artifacts/*ref_name_and_path`
-  #    -> 'artifacts'
-  def segment_before_last_wildcard(path)
-    path_segments = path.split('/').reject { |segment| segment.empty?  }
-    last_wildcard_index = path_segments.rindex { |part| part.starts_with?('*') }
-
-    index_of_non_param_segment = last_wildcard_index - 1
-    part_before_wildcard = path_segments[index_of_non_param_segment]
-    while parameter?(part_before_wildcard)
-      index_of_non_param_segment -= 1
-      part_before_wildcard = path_segments[index_of_non_param_segment]
+  #    -> 'builds/artifacts'
+  def path_between_wildcards(path)
+    path = path.gsub(STARTING_WITH_NAMESPACE, "")
+    path_segments = path.split('/').reject(&:empty?)
+    wildcard_index = path_segments.index { |segment| segment.starts_with?('*') }
+
+    segments_before_wildcard = path_segments[0..wildcard_index - 1]
+
+    param_index = segments_before_wildcard.index { |segment| segment.starts_with?(':') }
+    if param_index
+      segments_before_wildcard = segments_before_wildcard[param_index + 1..-1]
     end
 
-    part_before_wildcard
+    segments_before_wildcard.join('/')
   end
 
-  def parameter?(path_segment)
-    path_segment.starts_with?(':') || path_segment.starts_with?('*')
+  # If the path is reserved. Then no conflicting paths can# be created for any
+  # route using this reserved word.
+  #
+  # Both `builds/artifacts` & `artifacts/file` are covered by reserving the word
+  # `artifacts`
+  def wildcards_include?(path)
+    described_class::WILDCARD_ROUTES.include?(path) ||
+      path.split('/').any? { |segment| described_class::WILDCARD_ROUTES.include?(segment) }
   end
 
   let(:all_routes) do
@@ -42,14 +49,20 @@ describe DynamicPathValidator do
   # all routes not starting with a param
   let(:routes_not_starting_in_wildcard) { routes_without_format.select { |p| p !~ %r{^/[:*]} } }
 
+  let(:top_level_words) do
+    routes_not_starting_in_wildcard.map do |route|
+      route.split('/')[1]
+    end.compact.uniq
+  end
+
   # All routes that start with a namespaced path, that have 1 or more
   # path-segments before having another wildcard parameter.
   # - Starting with paths:
   #   - `/*namespace_id/:project_id/`
   #   - `/*namespace_id/:id/`
-  # - Followed by one or more path-parts not starting with `:` or `/`
+  # - Followed by one or more path-parts not starting with `:` or `*`
   # - Followed by a path-part that includes a wildcard parameter `*`
-  # At the time of writing these routes match: http://rubular.com/r/QDxulzZlxZ
+  # At the time of writing these routes match: http://rubular.com/r/Rv2pDE5Dvw
   STARTING_WITH_NAMESPACE = /^\/\*namespace_id\/:(project_)?id/
   NON_PARAM_PARTS = /[^:*][a-z\-_\/]*/
   ANY_OTHER_PATH_PART = /[a-z\-_\/:]*/
@@ -60,82 +73,89 @@ describe DynamicPathValidator do
     end
   end
 
+  # This will return all paths that are used in a namespaced route
+  # before another wildcard path:
+  #
+  # /*namespace_id/:project_id/builds/artifacts/*ref_name_and_path
+  # /*namespace_id/:project_id/info/lfs/objects/*oid
+  # /*namespace_id/:project_id/commits/*id
+  # /*namespace_id/:project_id/builds/:build_id/artifacts/file/*path
+  # -> ['builds/artifacts', 'info/lfs/objects', 'commits', 'artifacts/file']
+  let(:all_wildcard_paths) do
+    namespaced_wildcard_routes.map do |route|
+      path_between_wildcards(route)
+    end.uniq
+  end
+
   describe 'TOP_LEVEL_ROUTES' do
     it 'includes all the top level namespaces' do
-      top_level_words =  routes_not_starting_in_wildcard.
-                           map { |p| p.split('/')[1] }. # Get the first part of the path
-                           compact.
-                           uniq
-
       expect(described_class::TOP_LEVEL_ROUTES).to include(*top_level_words)
     end
   end
 
   describe 'WILDCARD_ROUTES' do
     it 'includes all paths that can be used after a namespace/project path' do
-      all_wildcard_paths = namespaced_wildcard_routes.map do |path|
-        segment_before_last_wildcard(path)
-      end.uniq
-
-      expect(described_class::WILDCARD_ROUTES).to include(*all_wildcard_paths)
+      aggregate_failures do
+        all_wildcard_paths.each do |path|
+          expect(wildcards_include?(path)).to be(true), "Expected #{path} to be rejected"
+        end
+      end
     end
   end
 
-  describe ".valid?" do
-    it 'is not case sensitive' do
-      expect(described_class.valid?("Users", type: :top_level)).to be(false)
+  describe '.contains_path_part?' do
+    it 'recognizes a path part' do
+      expect(described_class.contains_path_part?('user/some/path', 'user')).
+        to be_truthy
     end
-  end
-
-  describe '.valid_full_path' do
-    it "isn't valid when the top level is reserved" do
-      test_path = 'u/should-be-a/reserved-word'
 
-      expect(described_class.valid_full_path?(test_path)).to be(false)
+    it 'recognizes a path ending in the path part' do
+      expect(described_class.contains_path_part?('some/path/user', 'user')).
+        to be_truthy
     end
 
-    it "isn't valid if any of the path segments is reserved" do
-      test_path = 'the-wildcard/wikis/is-a-reserved-path'
+    it 'skips partial path matchies' do
+      expect(described_class.contains_path_part?('some/user1/path', 'user')).
+        to be_falsy
+    end
 
-      expect(described_class.valid_full_path?(test_path)).to be(false)
+    it 'can recognise combined paths' do
+      expect(described_class.contains_path_part?('some/user/admin/path', 'user/admin')).
+        to be_truthy
     end
 
-    it "is valid if the path doen't contain reserved words" do
-      test_path = 'there-are/no-wildcards/in-this-path'
+    it 'only recognizes full paths' do
+      expect(described_class.contains_path_part?('frontend-fixtures', 's')).
+        to be_falsy
+    end
 
-      expect(described_class.valid_full_path?(test_path)).to be(true)
+    it 'handles regex chars gracefully' do
+      expect(described_class.contains_path_part?('frontend-fixtures', '-')).
+        to be_falsy
     end
   end
 
-  describe '#validation_type' do
-    it 'uses top level validation for groups without parent' do
-      group = build(:group)
-
-      type = validator.validation_type(group)
-
-      expect(type).to eq(:top_level)
+  describe ".valid?" do
+    it 'is not case sensitive' do
+      expect(described_class.valid?("Users")).to be(false)
     end
 
-    it 'uses wildcard validation for groups with a parent' do
-      group = build(:group, parent: create(:group))
-
-      type = validator.validation_type(group)
+    it "isn't valid when the top level is reserved" do
+      test_path = 'u/should-be-a/reserved-word'
 
-      expect(type).to eq(:wildcard)
+      expect(described_class.valid?(test_path)).to be(false)
     end
 
-    it 'uses wildcard validation for a project' do
-      project = build(:project)
-
-      type = validator.validation_type(project)
+    it "isn't valid if any of the path segments is reserved" do
+      test_path = 'the-wildcard/wikis/is-not-allowed'
 
-      expect(type).to eq(:wildcard)
+      expect(described_class.valid?(test_path)).to be(false)
     end
 
-    it 'uses strict validation for everything else' do
-      type = validator.validation_type(double)
+    it "is valid if the path doesn't contain reserved words" do
+      test_path = 'there-are/no-wildcards/in-this-path'
 
-      expect(type).to eq(:strict)
+      expect(described_class.valid?(test_path)).to be(true)
     end
   end
 end