From 08b1bc3489e8d4e6d5786221bad090f16a1c021f Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Fri, 28 Apr 2017 18:09:01 +0200 Subject: Reject group-routes as names of child namespaces --- app/validators/dynamic_path_validator.rb | 89 ++++++++++++++++++++------ spec/models/group_spec.rb | 6 ++ spec/models/project_spec.rb | 7 ++ spec/models/user_spec.rb | 12 ++++ spec/validators/dynamic_path_validator_spec.rb | 76 ++++++++++++++-------- 5 files changed, 147 insertions(+), 43 deletions(-) diff --git a/app/validators/dynamic_path_validator.rb b/app/validators/dynamic_path_validator.rb index 96da92b5d76..c4ed5ac85eb 100644 --- a/app/validators/dynamic_path_validator.rb +++ b/app/validators/dynamic_path_validator.rb @@ -55,6 +55,7 @@ class DynamicPathValidator < ActiveModel::EachValidator snippets teams u + unicorn_test unsubscribes uploads users @@ -92,7 +93,52 @@ class DynamicPathValidator < ActiveModel::EachValidator wikis ]).freeze - STRICT_RESERVED = (TOP_LEVEL_ROUTES | WILDCARD_ROUTES).freeze + # These are all the paths that follow `/groups/*id/ or `/groups/*group_id` + # We need to reject these because we have a `/groups/*id` page that is the same + # as the `/*id`. + # + # If we would allow a subgroup to be created with the name `activity` then + # this group would not be accessible through `/groups/parent/activity` since + # this would map to the activity-page of it's parent. + GROUP_ROUTES = Set.new(%w[ + activity + avatar + edit + group_members + issues + labels + merge_requests + milestones + projects + subgroups + ]) + + CHILD_ROUTES = (WILDCARD_ROUTES | GROUP_ROUTES).freeze + + def self.without_reserved_wildcard_paths_regex + @full_path_without_wildcard_regex ||= regex_excluding_child_paths(WILDCARD_ROUTES) + end + + def self.without_reserved_child_paths_regex + @full_path_without_child_routes_regex ||= regex_excluding_child_paths(CHILD_ROUTES) + end + + # This is used to validate a full path. + # It doesn't match paths + # - Starting with one of the top level words + # - Containing one of the child level words in the middle of a path + def self.regex_excluding_child_paths(child_routes) + reserved_top_level_words = Regexp.union(TOP_LEVEL_ROUTES.to_a) + not_starting_in_reserved_word = %r{^(/?)(?!(#{reserved_top_level_words})(/|$))} + + reserved_child_level_words = Regexp.union(child_routes.to_a) + not_containing_reserved_child = %r{(?!(\S+)/(#{reserved_child_level_words})(/|$))} + + @full_path_regex = %r{ + #{not_starting_in_reserved_word} + #{not_containing_reserved_child} + #{Gitlab::Regex::FULL_NAMESPACE_REGEX_STR}}x + end def self.valid?(path) path_segments = path.split('/') @@ -102,41 +148,48 @@ class DynamicPathValidator < ActiveModel::EachValidator def self.reserved?(path) path = path.to_s.downcase - top_level, wildcard_part = path.split('/', 2) + _project_parts, namespace_parts = path.reverse.split('/', 2).map(&:reverse) - includes_reserved_top_level?(top_level) || includes_reserved_wildcard?(wildcard_part) + wildcard_reserved?(path) || any_reserved?(namespace_parts) end - def self.includes_reserved_wildcard?(path) - WILDCARD_ROUTES.any? do |reserved_word| - contains_path_part?(path, reserved_word) - end - end + def self.any_reserved?(path) + return false unless path - def self.includes_reserved_top_level?(path) - TOP_LEVEL_ROUTES.any? do |reserved_route| - contains_path_part?(path, reserved_route) - end + path !~ without_reserved_child_paths_regex end - def self.contains_path_part?(path, part) - path =~ %r{(/|\A)#{Regexp.quote(part)}(/|\z)} + def self.wildcard_reserved?(path) + return false unless path + + path !~ without_reserved_wildcard_paths_regex end def self.follow_format?(value) value =~ Gitlab::Regex.namespace_regex end - delegate :reserved?, :follow_format?, to: :class + delegate :reserved?, + :any_reserved?, + :follow_format?, to: :class + + def valid_full_path?(record, value) + full_path = record.respond_to?(:full_path) ? record.full_path : value + + case record + when Project || User + reserved?(full_path) + else + any_reserved?(full_path) + end + end def validate_each(record, attribute, value) unless follow_format?(value) record.errors.add(attribute, Gitlab::Regex.namespace_regex_message) end - full_path = record.respond_to?(:full_path) ? record.full_path : value - - if reserved?(full_path) + if valid_full_path?(record, value) record.errors.add(attribute, "#{value} is a reserved name") end end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 7b076d09585..a11805926cc 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -76,6 +76,12 @@ describe Group, models: true do expect(group).not_to be_valid end + + it 'rejects reserved group paths' do + group = build(:group, path: 'activity', parent: create(:group)) + + expect(group).not_to be_valid + end end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 04c90b651a0..2168099ee82 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -273,6 +273,13 @@ describe Project, models: true do expect(project).not_to be_valid end + + it 'allows a reserved group name' do + parent = create(:group) + project = build(:project, path: 'avatar', namespace: parent) + + expect(project).to be_valid + end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 0bcebc27598..1c2df4c9d97 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -97,6 +97,18 @@ describe User, models: true do expect(user.errors.values).to eq [['dashboard is a reserved name']] end + it 'allows child names' do + user = build(:user, username: 'avatar') + + expect(user).to be_valid + end + + it 'allows wildcard names' do + user = build(:user, username: 'blob') + + expect(user).to be_valid + end + it 'validates uniqueness' do expect(subject).to validate_uniqueness_of(:username).case_insensitive end diff --git a/spec/validators/dynamic_path_validator_spec.rb b/spec/validators/dynamic_path_validator_spec.rb index 5053a299bc3..f43b4892456 100644 --- a/spec/validators/dynamic_path_validator_spec.rb +++ b/spec/validators/dynamic_path_validator_spec.rb @@ -86,6 +86,16 @@ describe DynamicPathValidator do end.uniq end + STARTING_WITH_GROUP = %r{^/groups/\*(group_)?id/} + let(:group_routes) do + routes_without_format.select do |path| + path =~ STARTING_WITH_GROUP + end + end + + let(:paths_after_group_id) do + group_routes.map do |route| + route.gsub(STARTING_WITH_GROUP, '').split('/').first end.uniq end @@ -95,6 +105,12 @@ describe DynamicPathValidator do end end + describe 'GROUP_ROUTES' do + it "don't contain a second wildcard" do + expect(described_class::GROUP_ROUTES).to include(*paths_after_group_id) + end + end + describe 'WILDCARD_ROUTES' do it 'includes all paths that can be used after a namespace/project path' do aggregate_failures do @@ -105,59 +121,69 @@ describe DynamicPathValidator do end end - 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 + describe '.without_reserved_wildcard_paths_regex' do + subject { described_class.without_reserved_wildcard_paths_regex } - it 'recognizes a path ending in the path part' do - expect(described_class.contains_path_part?('some/path/user', 'user')). - to be_truthy + it 'rejects paths starting with a reserved top level' do + expect(subject).not_to match('dashboard/hello/world') + expect(subject).not_to match('dashboard') end - it 'skips partial path matches' do - expect(described_class.contains_path_part?('some/user1/path', 'user')). - to be_falsy + it 'rejects paths containing a wildcard reserved word' do + expect(subject).not_to match('hello/edit') + expect(subject).not_to match('hello/edit/in-the-middle') + expect(subject).not_to match('foo/bar1/refs/master/logs_tree') end - it 'can recognise combined paths' do - expect(described_class.contains_path_part?('some/user/admin/path', 'user/admin')). - to be_truthy + it 'matches valid paths' do + expect(subject).to match('parent/child/project-path') + expect(subject).to match('/parent/child/project-path') end + end - it 'only recognizes full paths' do - expect(described_class.contains_path_part?('frontend-fixtures', 's')). - to be_falsy - end + describe '.without_reserved_child_paths_regex' do + it 'rejects paths containing a child reserved word' do + subject = described_class.without_reserved_child_paths_regex - it 'handles regex chars gracefully' do - expect(described_class.contains_path_part?('frontend-fixtures', '-')). - to be_falsy + expect(subject).not_to match('hello/group_members') + expect(subject).not_to match('hello/activity/in-the-middle') + expect(subject).not_to match('foo/bar1/refs/master/logs_tree') end end describe ".valid?" do it 'is not case sensitive' do - expect(described_class.valid?("Users")).to be(false) + expect(described_class.valid?("Users")).to be_falsey end it "isn't valid when the top level is reserved" do test_path = 'u/should-be-a/reserved-word' - expect(described_class.valid?(test_path)).to be(false) + expect(described_class.valid?(test_path)).to be_falsey end it "isn't valid if any of the path segments is reserved" do test_path = 'the-wildcard/wikis/is-not-allowed' - expect(described_class.valid?(test_path)).to be(false) + expect(described_class.valid?(test_path)).to be_falsey end it "is valid if the path doesn't contain reserved words" do test_path = 'there-are/no-wildcards/in-this-path' - expect(described_class.valid?(test_path)).to be(true) + expect(described_class.valid?(test_path)).to be_truthy + end + + it 'allows allows a child path on the last spot' do + test_path = 'there/can-be-a/project-called/labels' + + expect(described_class.valid?(test_path)).to be_truthy + end + + it 'rejects a child path somewhere else' do + test_path = 'there/can-be-no/labels/group' + + expect(described_class.valid?(test_path)).to be_falsey end end end -- cgit v1.2.1