diff options
| -rw-r--r-- | app/models/project.rb | 5 | ||||
| -rw-r--r-- | app/validators/namespace_validator.rb | 93 | ||||
| -rw-r--r-- | app/validators/project_path_validator.rb | 22 | ||||
| -rw-r--r-- | lib/constraints/group_url_constrainer.rb | 10 | ||||
| -rw-r--r-- | lib/gitlab/etag_caching/router.rb | 24 | ||||
| -rw-r--r-- | spec/lib/constraints/group_url_constrainer_spec.rb | 7 | ||||
| -rw-r--r-- | spec/models/group_spec.rb | 20 | ||||
| -rw-r--r-- | spec/models/project_spec.rb | 14 | ||||
| -rw-r--r-- | spec/validators/namespace_validator_spec.rb | 33 |
9 files changed, 167 insertions, 61 deletions
diff --git a/app/models/project.rb b/app/models/project.rb index c7dc562c238..205b080c353 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -199,10 +199,11 @@ class Project < ActiveRecord::Base project_path: true, length: { maximum: 255 }, format: { with: Gitlab::Regex.project_path_regex, - message: Gitlab::Regex.project_path_regex_message } + message: Gitlab::Regex.project_path_regex_message }, + uniqueness: { scope: :namespace_id } + validates :namespace, presence: true validates :name, uniqueness: { scope: :namespace_id } - validates :path, uniqueness: { scope: :namespace_id } validates :import_url, addressable_url: true, if: :external_import? validates :import_url, importable_url: true, if: [:external_import?, :import_url_changed?] validates :star_count, numericality: { greater_than_or_equal_to: 0 } diff --git a/app/validators/namespace_validator.rb b/app/validators/namespace_validator.rb index 2aef4204e31..5a8b482d3db 100644 --- a/app/validators/namespace_validator.rb +++ b/app/validators/namespace_validator.rb @@ -5,7 +5,16 @@ # Values are checked for formatting and exclusion from a list of reserved path # names. class NamespaceValidator < ActiveModel::EachValidator - RESERVED = %w[ + # All routes that appear on the top level must be listed here. + # This will make sure that groups cannot be created with these names + # as these routes would be masked by the paths already in place. + # + # Example: + # /api/api-project + # + # the path `api` shouldn't be allowed because it would be masked by `api/*` + # + TOP_LEVEL_ROUTES = Set.new(%w[ .well-known admin all @@ -49,35 +58,56 @@ class NamespaceValidator < ActiveModel::EachValidator jwt oauth sent_notifications - ].freeze + ]).freeze - WILDCARD_ROUTES = %w[tree commits wikis new edit create update logs_tree - preview blob blame raw files create_dir find_file - artifacts graphs refs badges info git-upload-pack - git-receive-pack gitlab-lfs autocomplete_sources - templates avatar commit pages compare network snippets - services mattermost deploy_keys forks import merge_requests - branches merged_branches tags protected_branches variables - triggers pipelines environments cycle_analytics builds - hooks container_registry milestones labels issues - project_members group_links notes noteable boards todos - uploads runners runner_projects settings repository - transfer remove_fork archive unarchive housekeeping - toggle_star preview_markdown export remove_export - generate_new_export download_export activity - new_issue_address registry].freeze + # All project routes with wildcard argument must be listed here. + # Otherwise it can lead to routing issues when route considered as project name. + # + # Example: + # /group/project/tree/deploy_keys + # + # 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 info git-upload-pack + git-receive-pack gitlab-lfs autocomplete_sources + templates avatar commit pages compare network snippets + services mattermost deploy_keys forks import merge_requests + branches merged_branches tags protected_branches variables + triggers pipelines environments cycle_analytics builds + hooks container_registry milestones labels issues + project_members group_links notes noteable boards todos + uploads runners runner_projects settings repository + transfer remove_fork archive unarchive housekeeping + toggle_star preview_markdown export remove_export + generate_new_export download_export activity + new_issue_address registry]) - STRICT_RESERVED = (RESERVED + WILDCARD_ROUTES).freeze + STRICT_RESERVED = (TOP_LEVEL_ROUTES | WILDCARD_ROUTES) - def self.valid?(value) - !reserved?(value) && follow_format?(value) + def self.valid_full_path?(full_path) + pieces = full_path.split('/') + first_part = pieces.first + pieces.all? do |namespace| + type = first_part == namespace ? :top_level : :wildcard + valid?(namespace, type: type) + end end - def self.reserved?(value, strict: false) - if strict - STRICT_RESERVED.include?(value) + def self.valid?(value, type: :strict) + !reserved?(value, type: type) && follow_format?(value) + end + + def self.reserved?(value, type: :strict) + case type + when :wildcard + WILDCARD_ROUTES.include?(value) + when :top_level + TOP_LEVEL_ROUTES.include?(value) else - RESERVED.include?(value) + STRICT_RESERVED.include?(value) end end @@ -92,10 +122,19 @@ class NamespaceValidator < ActiveModel::EachValidator record.errors.add(attribute, Gitlab::Regex.namespace_regex_message) end - strict = record.is_a?(Group) && record.parent_id - - if reserved?(value, strict: strict) + if reserved?(value, type: validation_type(record)) record.errors.add(attribute, "#{value} is a reserved name") end end + + def validation_type(record) + case record + when Group + record.parent_id ? :wildcard : :top_level + when Project + :wildcard + else + :strict + end + end end diff --git a/app/validators/project_path_validator.rb b/app/validators/project_path_validator.rb index ee2ae65be7b..d41bdaeab84 100644 --- a/app/validators/project_path_validator.rb +++ b/app/validators/project_path_validator.rb @@ -4,25 +4,17 @@ # # Values are checked for formatting and exclusion from a list of reserved path # names. -class ProjectPathValidator < ActiveModel::EachValidator - # All project routes with wildcard argument must be listed here. - # Otherwise it can lead to routing issues when route considered as project name. - # - # Example: - # /group/project/tree/deploy_keys - # - # without tree as reserved name routing can match 'group/project' as group name, - # 'tree' as project name and 'deploy_keys' as route. - # - RESERVED = (NamespaceValidator::STRICT_RESERVED - - %w[dashboard help ci admin search notes services assets profile public]).freeze - +# +# This is basically the same as the `NamespaceValidator` but it skips the validation +# of the format with `Gitlab::Regex.namespace_regex`. The format of projects +# is validated in the class itself. +class ProjectPathValidator < NamespaceValidator def self.valid?(value) !reserved?(value) end - def self.reserved?(value) - RESERVED.include?(value) + def self.reserved?(value, type: :wildcard) + super(value, type: :wildcard) end delegate :reserved?, to: :class diff --git a/lib/constraints/group_url_constrainer.rb b/lib/constraints/group_url_constrainer.rb index bae4db1ca4d..559f510944a 100644 --- a/lib/constraints/group_url_constrainer.rb +++ b/lib/constraints/group_url_constrainer.rb @@ -2,16 +2,8 @@ class GroupUrlConstrainer def matches?(request) id = request.params[:id] - return false unless valid?(id) + return false unless NamespaceValidator.valid_full_path?(id) Group.find_by_full_path(id).present? end - - private - - def valid?(id) - id.split('/').all? do |namespace| - NamespaceValidator.valid?(namespace) - end - end end diff --git a/lib/gitlab/etag_caching/router.rb b/lib/gitlab/etag_caching/router.rb index f6e4f279c06..67aa237f2f1 100644 --- a/lib/gitlab/etag_caching/router.rb +++ b/lib/gitlab/etag_caching/router.rb @@ -2,31 +2,39 @@ module Gitlab module EtagCaching class Router Route = Struct.new(:regexp, :name) - - RESERVED_WORDS = NamespaceValidator::WILDCARD_ROUTES.map { |word| "/#{word}/" }.join('|') + # We enable an ETag for every request matching the regex. + # To match a regex the path needs to match the following: + # - Don't contain a reserved word (expect for the words used in the + # regex itself) + # - Ending in `noteable/issue/<id>/notes` for the `issue_notes` route + # - Ending in `issues/id`/rendered_title` for the `issue_title` route + USED_IN_ROUTES = %w[noteable issue notes issues renderred_title + commit pipelines merge_requests new].freeze + RESERVED_WORDS = NamespaceValidator::WILDCARD_ROUTES - USED_IN_ROUTES + RESERVED_WORDS_REGEX = Regexp.union(*RESERVED_WORDS) ROUTES = [ Gitlab::EtagCaching::Router::Route.new( - %r(^(?!.*(#{RESERVED_WORDS})).*/noteable/issue/\d+/notes\z), + %r(^(?!.*(#{RESERVED_WORDS_REGEX})).*/noteable/issue/\d+/notes\z), 'issue_notes' ), Gitlab::EtagCaching::Router::Route.new( - %r(^(?!.*(#{RESERVED_WORDS})).*/issues/\d+/rendered_title\z), + %r(^(?!.*(#{RESERVED_WORDS_REGEX})).*/issues/\d+/rendered_title\z), 'issue_title' ), Gitlab::EtagCaching::Router::Route.new( - %r(^(?!.*(#{RESERVED_WORDS})).*/commit/\S+/pipelines\.json\z), + %r(^(?!.*(#{RESERVED_WORDS_REGEX})).*/commit/\S+/pipelines\.json\z), 'commit_pipelines' ), Gitlab::EtagCaching::Router::Route.new( - %r(^(?!.*(#{RESERVED_WORDS})).*/merge_requests/new\.json\z), + %r(^(?!.*(#{RESERVED_WORDS_REGEX})).*/merge_requests/new\.json\z), 'new_merge_request_pipelines' ), Gitlab::EtagCaching::Router::Route.new( - %r(^(?!.*(#{RESERVED_WORDS})).*/merge_requests/\d+/pipelines\.json\z), + %r(^(?!.*(#{RESERVED_WORDS_REGEX})).*/merge_requests/\d+/pipelines\.json\z), 'merge_request_pipelines' ), Gitlab::EtagCaching::Router::Route.new( - %r(^(?!.*(#{RESERVED_WORDS})).*/pipelines\.json\z), + %r(^(?!.*(#{RESERVED_WORDS_REGEX})).*/pipelines\.json\z), 'project_pipelines' ) ].freeze diff --git a/spec/lib/constraints/group_url_constrainer_spec.rb b/spec/lib/constraints/group_url_constrainer_spec.rb index 96dacdc5cd2..f95adf3a84b 100644 --- a/spec/lib/constraints/group_url_constrainer_spec.rb +++ b/spec/lib/constraints/group_url_constrainer_spec.rb @@ -17,6 +17,13 @@ describe GroupUrlConstrainer, lib: true do it { expect(subject.matches?(request)).to be_truthy } end + context 'valid request for nested group with reserved top level name' do + let!(:nested_group) { create(:group, path: 'api', parent: group) } + let!(:request) { build_request('gitlab/api') } + + it { expect(subject.matches?(request)).to be_truthy } + end + context 'invalid request' do let(:request) { build_request('foo') } diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 8ffde6f7fbb..7b076d09585 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -57,6 +57,26 @@ describe Group, models: true do it { is_expected.not_to validate_presence_of :owner } it { is_expected.to validate_presence_of :two_factor_grace_period } it { is_expected.to validate_numericality_of(:two_factor_grace_period).is_greater_than_or_equal_to(0) } + + describe 'path validation' do + it 'rejects paths reserved on the root namespace when the group has no parent' do + group = build(:group, path: 'api') + + expect(group).not_to be_valid + end + + it 'allows root paths when the group has a parent' do + group = build(:group, path: 'api', parent: create(:group)) + + expect(group).to be_valid + end + + it 'rejects any wildcard paths when not a top level group' do + group = build(:group, path: 'tree', parent: create(:group)) + + expect(group).not_to be_valid + end + end end describe '.visible_to_user' do diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 92d420337f9..726dade93f6 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -253,6 +253,20 @@ describe Project, models: true do expect(new_project.errors.full_messages.first).to eq('The project is still being deleted. Please try again later.') end end + + describe 'path validation' do + it 'allows paths reserved on the root namespace' do + project = build(:project, path: 'api') + + expect(project).to be_valid + end + + it 'rejects paths reserved on another level' do + project = build(:project, path: 'tree') + + expect(project).not_to be_valid + end + end end describe 'default_scope' do diff --git a/spec/validators/namespace_validator_spec.rb b/spec/validators/namespace_validator_spec.rb index e21b8ef5abd..4b5dd59e048 100644 --- a/spec/validators/namespace_validator_spec.rb +++ b/spec/validators/namespace_validator_spec.rb @@ -1,6 +1,7 @@ require 'spec_helper' describe NamespaceValidator do + let(:validator) { described_class.new(attributes: [:path]) } describe 'RESERVED' do it 'includes all the top level namespaces' do all_top_level_routes = Rails.application.routes.routes.routes. @@ -26,4 +27,36 @@ describe NamespaceValidator do expect(described_class::WILDCARD_ROUTES).to include(*all_wildcard_paths) 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) + end + + it 'uses wildcard validation for groups with a parent' do + group = build(:group, parent: create(:group)) + + type = validator.validation_type(group) + + expect(type).to eq(:wildcard) + end + + it 'uses wildcard validation for a project' do + project = build(:project) + + type = validator.validation_type(project) + + expect(type).to eq(:wildcard) + end + + it 'uses strict validation for everything else' do + type = validator.validation_type(double) + + expect(type).to eq(:strict) + end + end end |