Commit 74fcccaa authored by Bob Van Landuyt's avatar Bob Van Landuyt
Browse files

Streamline the path validation in groups & projects

`Project` uses `ProjectPathValidator` which is now a
`NamespaceValidator` that skips the format validation.

That way we're sure we are using the same collection of reserved
paths.

I updated the path constraints to reflect the changes: We now allow
some values that are only used on a top level namespace as a name for
a nested group/project.
parent 4f3171caedf5
...@@ -199,10 +199,11 @@ def update_forks_visibility_level ...@@ -199,10 +199,11 @@ def update_forks_visibility_level
project_path: true, project_path: true,
length: { maximum: 255 }, length: { maximum: 255 },
format: { with: Gitlab::Regex.project_path_regex, 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 :namespace, presence: true
validates :name, uniqueness: { scope: :namespace_id } validates :name, uniqueness: { scope: :namespace_id }
validates :path, uniqueness: { scope: :namespace_id }
validates :import_url, addressable_url: true, if: :external_import? validates :import_url, addressable_url: true, if: :external_import?
validates :import_url, importable_url: true, if: [:external_import?, :import_url_changed?] validates :import_url, importable_url: true, if: [:external_import?, :import_url_changed?]
validates :star_count, numericality: { greater_than_or_equal_to: 0 } validates :star_count, numericality: { greater_than_or_equal_to: 0 }
......
...@@ -5,7 +5,16 @@ ...@@ -5,7 +5,16 @@
# Values are checked for formatting and exclusion from a list of reserved path # Values are checked for formatting and exclusion from a list of reserved path
# names. # names.
class NamespaceValidator < ActiveModel::EachValidator 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 .well-known
admin admin
all all
...@@ -49,35 +58,56 @@ class NamespaceValidator < ActiveModel::EachValidator ...@@ -49,35 +58,56 @@ class NamespaceValidator < ActiveModel::EachValidator
jwt jwt
oauth oauth
sent_notifications sent_notifications
].freeze ]).freeze
WILDCARD_ROUTES = %w[tree commits wikis new edit create update logs_tree # All project routes with wildcard argument must be listed here.
preview blob blame raw files create_dir find_file # Otherwise it can lead to routing issues when route considered as project name.
artifacts graphs refs badges info git-upload-pack #
git-receive-pack gitlab-lfs autocomplete_sources # Example:
templates avatar commit pages compare network snippets # /group/project/tree/deploy_keys
services mattermost deploy_keys forks import merge_requests #
branches merged_branches tags protected_branches variables # without tree as reserved name routing can match 'group/project' as group name,
triggers pipelines environments cycle_analytics builds # 'tree' as project name and 'deploy_keys' as route.
hooks container_registry milestones labels issues #
project_members group_links notes noteable boards todos WILDCARD_ROUTES = Set.new(%w[tree commits wikis new edit create update logs_tree
uploads runners runner_projects settings repository preview blob blame raw files create_dir find_file
transfer remove_fork archive unarchive housekeeping artifacts graphs refs badges info git-upload-pack
toggle_star preview_markdown export remove_export git-receive-pack gitlab-lfs autocomplete_sources
generate_new_export download_export activity templates avatar commit pages compare network snippets
new_issue_address registry].freeze 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) def self.valid_full_path?(full_path)
!reserved?(value) && follow_format?(value) 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 end
def self.reserved?(value, strict: false) def self.valid?(value, type: :strict)
if strict !reserved?(value, type: type) && follow_format?(value)
STRICT_RESERVED.include?(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 else
RESERVED.include?(value) STRICT_RESERVED.include?(value)
end end
end end
...@@ -92,10 +122,19 @@ def validate_each(record, attribute, value) ...@@ -92,10 +122,19 @@ def validate_each(record, attribute, value)
record.errors.add(attribute, Gitlab::Regex.namespace_regex_message) record.errors.add(attribute, Gitlab::Regex.namespace_regex_message)
end end
strict = record.is_a?(Group) && record.parent_id if reserved?(value, type: validation_type(record))
if reserved?(value, strict: strict)
record.errors.add(attribute, "#{value} is a reserved name") record.errors.add(attribute, "#{value} is a reserved name")
end end
end end
def validation_type(record)
case record
when Group
record.parent_id ? :wildcard : :top_level
when Project
:wildcard
else
:strict
end
end
end end
...@@ -4,25 +4,17 @@ ...@@ -4,25 +4,17 @@
# #
# Values are checked for formatting and exclusion from a list of reserved path # Values are checked for formatting and exclusion from a list of reserved path
# names. # names.
class ProjectPathValidator < ActiveModel::EachValidator #
# All project routes with wildcard argument must be listed here. # This is basically the same as the `NamespaceValidator` but it skips the validation
# Otherwise it can lead to routing issues when route considered as project name. # of the format with `Gitlab::Regex.namespace_regex`. The format of projects
# # is validated in the class itself.
# Example: class ProjectPathValidator < NamespaceValidator
# /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
def self.valid?(value) def self.valid?(value)
!reserved?(value) !reserved?(value)
end end
def self.reserved?(value) def self.reserved?(value, type: :wildcard)
RESERVED.include?(value) super(value, type: :wildcard)
end end
delegate :reserved?, to: :class delegate :reserved?, to: :class
......
...@@ -2,16 +2,8 @@ class GroupUrlConstrainer ...@@ -2,16 +2,8 @@ class GroupUrlConstrainer
def matches?(request) def matches?(request)
id = request.params[:id] id = request.params[:id]
return false unless valid?(id) return false unless NamespaceValidator.valid_full_path?(id)
Group.find_by_full_path(id).present? Group.find_by_full_path(id).present?
end end
private
def valid?(id)
id.split('/').all? do |namespace|
NamespaceValidator.valid?(namespace)
end
end
end end
...@@ -2,31 +2,39 @@ module Gitlab ...@@ -2,31 +2,39 @@ module Gitlab
module EtagCaching module EtagCaching
class Router class Router
Route = Struct.new(:regexp, :name) Route = Struct.new(:regexp, :name)
# We enable an ETag for every request matching the regex.
RESERVED_WORDS = NamespaceValidator::WILDCARD_ROUTES.map { |word| "/#{word}/" }.join('|') # 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 = [ ROUTES = [
Gitlab::EtagCaching::Router::Route.new( Gitlab::EtagCaching::Router::Route.new(
%r(^(?!.*(#{RESERVED_WORDS})).*/noteable/issue/\d+/notes\z), %r(^(?!.*(#{RESERVED_WORDS_REGEX})).*/noteable/issue/\d+/notes\z),
'issue_notes' 'issue_notes'
), ),
Gitlab::EtagCaching::Router::Route.new( Gitlab::EtagCaching::Router::Route.new(
%r(^(?!.*(#{RESERVED_WORDS})).*/issues/\d+/rendered_title\z), %r(^(?!.*(#{RESERVED_WORDS_REGEX})).*/issues/\d+/rendered_title\z),
'issue_title' 'issue_title'
), ),
Gitlab::EtagCaching::Router::Route.new( Gitlab::EtagCaching::Router::Route.new(
%r(^(?!.*(#{RESERVED_WORDS})).*/commit/\S+/pipelines\.json\z), %r(^(?!.*(#{RESERVED_WORDS_REGEX})).*/commit/\S+/pipelines\.json\z),
'commit_pipelines' 'commit_pipelines'
), ),
Gitlab::EtagCaching::Router::Route.new( 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' 'new_merge_request_pipelines'
), ),
Gitlab::EtagCaching::Router::Route.new( 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' 'merge_request_pipelines'
), ),
Gitlab::EtagCaching::Router::Route.new( Gitlab::EtagCaching::Router::Route.new(
%r(^(?!.*(#{RESERVED_WORDS})).*/pipelines\.json\z), %r(^(?!.*(#{RESERVED_WORDS_REGEX})).*/pipelines\.json\z),
'project_pipelines' 'project_pipelines'
) )
].freeze ].freeze
......
...@@ -17,6 +17,13 @@ ...@@ -17,6 +17,13 @@
it { expect(subject.matches?(request)).to be_truthy } it { expect(subject.matches?(request)).to be_truthy }
end 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 context 'invalid request' do
let(:request) { build_request('foo') } let(:request) { build_request('foo') }
......
...@@ -57,6 +57,26 @@ ...@@ -57,6 +57,26 @@
it { is_expected.not_to validate_presence_of :owner } 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_presence_of :two_factor_grace_period }
it { is_expected.to validate_numericality_of(:two_factor_grace_period).is_greater_than_or_equal_to(0) } 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 end
describe '.visible_to_user' do describe '.visible_to_user' do
......
...@@ -253,6 +253,20 @@ ...@@ -253,6 +253,20 @@
expect(new_project.errors.full_messages.first).to eq('The project is still being deleted. Please try again later.') expect(new_project.errors.full_messages.first).to eq('The project is still being deleted. Please try again later.')
end end
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 end
describe 'default_scope' do describe 'default_scope' do
......
require 'spec_helper' require 'spec_helper'
describe NamespaceValidator do describe NamespaceValidator do
let(:validator) { described_class.new(attributes: [:path]) }
describe 'RESERVED' do describe 'RESERVED' do
it 'includes all the top level namespaces' do it 'includes all the top level namespaces' do
all_top_level_routes = Rails.application.routes.routes.routes. all_top_level_routes = Rails.application.routes.routes.routes.
...@@ -26,4 +27,36 @@ ...@@ -26,4 +27,36 @@
expect(described_class::WILDCARD_ROUTES).to include(*all_wildcard_paths) expect(described_class::WILDCARD_ROUTES).to include(*all_wildcard_paths)
end end
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 end
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment