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`

1 files changed, 80 insertions, 60 deletions

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