summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/validators/dynamic_path_validator.rb57
-rw-r--r--lib/constraints/group_url_constrainer.rb2
-rw-r--r--lib/constraints/project_url_constrainer.rb2
-rw-r--r--spec/models/namespace_spec.rb7
-rw-r--r--spec/models/project_spec.rb7
-rw-r--r--spec/validators/dynamic_path_validator_spec.rb140
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