Commit 2a6227a9 authored by Robert Speicher's avatar Robert Speicher

Merge branch 'dm-fix-routes' into 'master'

Fix ambiguous routing issues by teaching router about reserved words

See merge request !11570
parents 03bd3081 b0498c17
...@@ -205,7 +205,7 @@ class Project < ActiveRecord::Base ...@@ -205,7 +205,7 @@ class Project < ActiveRecord::Base
presence: true, presence: true,
dynamic_path: true, dynamic_path: true,
length: { maximum: 255 }, length: { maximum: 255 },
format: { with: Gitlab::Regex.project_path_regex, format: { with: Gitlab::Regex.project_path_format_regex,
message: Gitlab::Regex.project_path_regex_message }, message: Gitlab::Regex.project_path_regex_message },
uniqueness: { scope: :namespace_id } uniqueness: { scope: :namespace_id }
......
...@@ -6,199 +6,26 @@ ...@@ -6,199 +6,26 @@
# 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 DynamicPathValidator < ActiveModel::EachValidator class DynamicPathValidator < ActiveModel::EachValidator
# All routes that appear on the top level must be listed here. class << self
# This will make sure that groups cannot be created with these names def valid_namespace_path?(path)
# as these routes would be masked by the paths already in place. "#{path}/" =~ Gitlab::Regex.full_namespace_path_regex
# end
# Example:
# /api/api-project
#
# the path `api` shouldn't be allowed because it would be masked by `api/*`
#
TOP_LEVEL_ROUTES = %w[
-
.well-known
abuse_reports
admin
all
api
assets
autocomplete
ci
dashboard
explore
files
groups
health_check
help
hooks
import
invites
issues
jwt
koding
member
merge_requests
new
notes
notification_settings
oauth
profile
projects
public
repository
robots.txt
s
search
sent_notifications
services
snippets
teams
u
unicorn_test
unsubscribes
uploads
users
].freeze
# This list should contain all words following `/*namespace_id/:project_id` in
# routes that contain a second wildcard.
#
# Example:
# /*namespace_id/:project_id/badges/*ref/build
#
# If `badges` was allowed as a project/group name, we would not be able to access the
# `badges` route for those projects:
#
# Consider a namespace with path `foo/bar` and a project called `badges`.
# The route to the build badge would then be `/foo/bar/badges/badges/master/build.svg`
#
# When accessing this path the route would be matched to the `badges` path
# with the following params:
# - namespace_id: `foo`
# - project_id: `bar`
# - ref: `badges/master`
#
# Failing to find the project, this would result in a 404.
#
# By rejecting `badges` the router can _count_ on the fact that `badges` will
# be preceded by the `namespace/project`.
WILDCARD_ROUTES = %w[
badges
blame
blob
builds
commits
create
create_dir
edit
environments/folders
files
find_file
gitlab-lfs/objects
info/lfs/objects
new
preview
raw
refs
tree
update
wikis
].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 = %w[
activity
analytics
audit_events
avatar
edit
group_members
hooks
issues
labels
ldap
ldap_group_links
merge_requests
milestones
notification_setting
pipeline_quota
projects
subgroups
].freeze
CHILD_ROUTES = (WILDCARD_ROUTES | GROUP_ROUTES).freeze
def self.without_reserved_wildcard_paths_regex
@without_reserved_wildcard_paths_regex ||= regex_excluding_child_paths(WILDCARD_ROUTES)
end
def self.without_reserved_child_paths_regex
@without_reserved_child_paths_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)
not_starting_in_reserved_word = %r{\A/?(?!(#{reserved_top_level_words})(/|\z))}
reserved_child_level_words = Regexp.union(child_routes)
not_containing_reserved_child = %r{(?!\S+/(#{reserved_child_level_words})(/|\z))}
%r{#{not_starting_in_reserved_word}
#{not_containing_reserved_child}
#{Gitlab::Regex.full_namespace_regex}}x
end
def self.valid?(path)
path =~ Gitlab::Regex.full_namespace_regex && !full_path_reserved?(path)
end
def self.full_path_reserved?(path)
path = path.to_s.downcase
_project_part, namespace_parts = path.reverse.split('/', 2).map(&:reverse)
wildcard_reserved?(path) || child_reserved?(namespace_parts)
end
def self.child_reserved?(path)
return false unless path
path !~ without_reserved_child_paths_regex
end
def self.wildcard_reserved?(path)
return false unless path
path !~ without_reserved_wildcard_paths_regex def valid_project_path?(path)
"#{path}/" =~ Gitlab::Regex.full_project_path_regex
end
end end
delegate :full_path_reserved?, def path_valid_for_record?(record, value)
:child_reserved?,
to: :class
def path_reserved_for_record?(record, value)
full_path = record.respond_to?(:full_path) ? record.full_path : value full_path = record.respond_to?(:full_path) ? record.full_path : value
# For group paths the entire path cannot contain a reserved child word return true unless full_path
# The path doesn't contain the last `_project_part` so we need to validate
# if the entire path. case record
# Example: when Project
# A *group* with full path `parent/activity` is reserved. self.class.valid_project_path?(full_path)
# A *project* with full path `parent/activity` is allowed.
if record.is_a? Group
child_reserved?(full_path)
else else
full_path_reserved?(full_path) self.class.valid_namespace_path?(full_path)
end end
end end
...@@ -208,7 +35,7 @@ class DynamicPathValidator < ActiveModel::EachValidator ...@@ -208,7 +35,7 @@ class DynamicPathValidator < ActiveModel::EachValidator
return return
end end
if path_reserved_for_record?(record, value) unless path_valid_for_record?(record, value)
record.errors.add(attribute, "#{value} is a reserved name") record.errors.add(attribute, "#{value} is a reserved name")
end end
end end
......
...@@ -68,7 +68,9 @@ namespace :admin do ...@@ -68,7 +68,9 @@ namespace :admin do
resources :projects, only: [:index] resources :projects, only: [:index]
scope(path: 'projects/*namespace_id', as: :namespace) do scope(path: 'projects/*namespace_id',
as: :namespace,
constraints: { namespace_id: Gitlab::Regex.namespace_route_regex }) do
resources(:projects, resources(:projects,
path: '/', path: '/',
constraints: { id: Gitlab::Regex.project_route_regex }, constraints: { id: Gitlab::Regex.project_route_regex },
......
scope(path: '*namespace_id/:project_id', constraints: { format: nil }) do scope(path: '*namespace_id/:project_id',
format: nil,
constraints: { namespace_id: Gitlab::Regex.namespace_route_regex }) do
scope(constraints: { project_id: Gitlab::Regex.project_git_route_regex }, module: :projects) do scope(constraints: { project_id: Gitlab::Regex.project_git_route_regex }, module: :projects) do
# Git HTTP clients ('git clone' etc.) # Git HTTP clients ('git clone' etc.)
scope(controller: :git_http) do scope(controller: :git_http) do
......
...@@ -5,7 +5,22 @@ resources :projects, only: [:index, :new, :create] ...@@ -5,7 +5,22 @@ resources :projects, only: [:index, :new, :create]
draw :git_http draw :git_http
constraints(ProjectUrlConstrainer.new) do constraints(ProjectUrlConstrainer.new) do
scope(path: '*namespace_id', as: :namespace) do # If the route has a wildcard segment, the segment has a regex constraint,
# the segment is potentially followed by _another_ wildcard segment, and
# the `format` option is not set to false, we need to specify that
# regex constraint _outside_ of `constraints: {}`.
#
# Otherwise, Rails will overwrite the constraint with `/.+?/`,
# which breaks some of our wildcard routes like `/blob/*id`
# and `/tree/*id` that depend on the negative lookahead inside
# `Gitlab::Regex.namespace_route_regex`, which helps the router
# determine whether a certain path segment is part of `*namespace_id`,
# `:project_id`, or `*id`.
#
# See https://github.com/rails/rails/blob/v4.2.8/actionpack/lib/action_dispatch/routing/mapper.rb#L155
scope(path: '*namespace_id',
as: :namespace,
namespace_id: Gitlab::Regex.namespace_route_regex) do
scope(path: ':project_id', scope(path: ':project_id',
constraints: { project_id: Gitlab::Regex.project_route_regex }, constraints: { project_id: Gitlab::Regex.project_route_regex },
module: :projects, module: :projects,
......
...@@ -13,17 +13,17 @@ end ...@@ -13,17 +13,17 @@ end
constraints(UserUrlConstrainer.new) do constraints(UserUrlConstrainer.new) do
# Get all keys of user # Get all keys of user
get ':username.keys' => 'profiles/keys#get_keys', constraints: { username: Gitlab::Regex.namespace_route_regex } get ':username.keys' => 'profiles/keys#get_keys', constraints: { username: Gitlab::Regex.root_namespace_route_regex }
scope(path: ':username', scope(path: ':username',
as: :user, as: :user,
constraints: { username: Gitlab::Regex.namespace_route_regex }, constraints: { username: Gitlab::Regex.root_namespace_route_regex },
controller: :users) do controller: :users) do
get '/', action: :show get '/', action: :show
end end
end end
scope(constraints: { username: Gitlab::Regex.namespace_route_regex }) do scope(constraints: { username: Gitlab::Regex.root_namespace_route_regex }) do
scope(path: 'users/:username', scope(path: 'users/:username',
as: :user, as: :user,
controller: :users) do controller: :users) do
......
...@@ -71,9 +71,9 @@ structure. ...@@ -71,9 +71,9 @@ structure.
- You need to be an Owner of a group in order to be able to create - You need to be an Owner of a group in order to be able to create
a subgroup. For more information check the [permissions table][permissions]. a subgroup. For more information check the [permissions table][permissions].
- For a list of words that are not allowed to be used as group names see the - For a list of words that are not allowed to be used as group names see the
[`dynamic_path_validator.rb` file][reserved] under the `TOP_LEVEL_ROUTES`, `WILDCARD_ROUTES` and `GROUP_ROUTES` lists: [`regex.rb` file][reserved] under the `TOP_LEVEL_ROUTES`, `PROJECT_WILDCARD_ROUTES` and `GROUP_ROUTES` lists:
- `TOP_LEVEL_ROUTES`: are names that are reserved as usernames or top level groups - `TOP_LEVEL_ROUTES`: are names that are reserved as usernames or top level groups
- `WILDCARD_ROUTES`: are names that are reserved for child groups or projects. - `PROJECT_WILDCARD_ROUTES`: are names that are reserved for child groups or projects.
- `GROUP_ROUTES`: are names that are reserved for all groups or projects. - `GROUP_ROUTES`: are names that are reserved for all groups or projects.
To create a subgroup: To create a subgroup:
...@@ -163,4 +163,4 @@ Here's a list of what you can't do with subgroups: ...@@ -163,4 +163,4 @@ Here's a list of what you can't do with subgroups:
[ce-2772]: https://gitlab.com/gitlab-org/gitlab-ce/issues/2772 [ce-2772]: https://gitlab.com/gitlab-org/gitlab-ce/issues/2772
[permissions]: ../../permissions.md#group [permissions]: ../../permissions.md#group
[reserved]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/validators/dynamic_path_validator.rb [reserved]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/lib/gitlab/regex.rb
class GroupUrlConstrainer class GroupUrlConstrainer
def matches?(request) def matches?(request)
id = request.params[:id] id = request.params[:group_id] || request.params[:id]
return false unless DynamicPathValidator.valid?(id) return false unless DynamicPathValidator.valid_namespace_path?(id)
Group.find_by_full_path(id, follow_redirects: request.get?).present? Group.find_by_full_path(id, follow_redirects: request.get?).present?
end end
......
...@@ -4,7 +4,7 @@ class ProjectUrlConstrainer ...@@ -4,7 +4,7 @@ class ProjectUrlConstrainer
project_path = request.params[:project_id] || request.params[:id] project_path = request.params[:project_id] || request.params[:id]
full_path = namespace_path + '/' + project_path full_path = namespace_path + '/' + project_path
return false unless DynamicPathValidator.valid?(full_path) return false unless DynamicPathValidator.valid_project_path?(full_path)
Project.find_by_full_path(full_path, follow_redirects: request.get?).present? Project.find_by_full_path(full_path, follow_redirects: request.get?).present?
end end
......
...@@ -10,7 +10,7 @@ module Gitlab ...@@ -10,7 +10,7 @@ module Gitlab
# - Ending in `issues/id`/realtime_changes` for the `issue_title` route # - Ending in `issues/id`/realtime_changes` for the `issue_title` route
USED_IN_ROUTES = %w[noteable issue notes issues realtime_changes USED_IN_ROUTES = %w[noteable issue notes issues realtime_changes
commit pipelines merge_requests new].freeze commit pipelines merge_requests new].freeze
RESERVED_WORDS = DynamicPathValidator::WILDCARD_ROUTES - USED_IN_ROUTES RESERVED_WORDS = Gitlab::Regex::ILLEGAL_PROJECT_PATH_WORDS - USED_IN_ROUTES
RESERVED_WORDS_REGEX = Regexp.union(*RESERVED_WORDS) RESERVED_WORDS_REGEX = Regexp.union(*RESERVED_WORDS)
ROUTES = [ ROUTES = [
Gitlab::EtagCaching::Router::Route.new( Gitlab::EtagCaching::Router::Route.new(
......
...@@ -2,6 +2,136 @@ module Gitlab ...@@ -2,6 +2,136 @@ module Gitlab
module Regex module Regex
extend self extend self
# 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 = %w[
-
.well-known
abuse_reports
admin
all
api
assets
autocomplete
ci
dashboard
explore
files
groups
health_check
help
hooks
import
invites
issues
jwt
koding
member
merge_requests
new
notes
notification_settings
oauth
profile
projects
public
repository
robots.txt
s
search
sent_notifications
services
snippets
teams
u
unicorn_test
unsubscribes
uploads
users
].freeze
# This list should contain all words following `/*namespace_id/:project_id` in
# routes that contain a second wildcard.
#
# Example:
# /*namespace_id/:project_id/badges/*ref/build
#
# If `badges` was allowed as a project/group name, we would not be able to access the
# `badges` route for those projects:
#
# Consider a namespace with path `foo/bar` and a project called `badges`.
# The route to the build badge would then be `/foo/bar/badges/badges/master/build.svg`
#
# When accessing this path the route would be matched to the `badges` path
# with the following params:
# - namespace_id: `foo`
# - project_id: `bar`
# - ref: `badges/master`
#
# Failing to find the project, this would result in a 404.
#
# By rejecting `badges` the router can _count_ on the fact that `badges` will
# be preceded by the `namespace/project`.
PROJECT_WILDCARD_ROUTES = %w[
badges
blame
blob
builds
commits
create
create_dir
edit
environments/folders
files
find_file
gitlab-lfs/objects
info/lfs/objects
new
preview
raw
refs
tree
update
wikis
].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 its parent.
GROUP_ROUTES = %w[
activity
analytics
audit_events
avatar
edit
group_members
hooks
issues
labels
ldap
ldap_group_links
merge_requests
milestones
notification_setting
pipeline_quota
projects
subgroups
].freeze
ILLEGAL_PROJECT_PATH_WORDS = PROJECT_WILDCARD_ROUTES
ILLEGAL_GROUP_PATH_WORDS = (PROJECT_WILDCARD_ROUTES | GROUP_ROUTES).freeze
# The namespace regex is used in Javascript to validate usernames in the "Register" form. However, Javascript # The namespace regex is used in Javascript to validate usernames in the "Register" form. However, Javascript
# does not support the negative lookbehind assertion (?<!) that disallows usernames ending in `.git` and `.atom`. # does not support the negative lookbehind assertion (?<!) that disallows usernames ending in `.git` and `.atom`.
# Since this is a non-trivial problem to solve in Javascript (heavily complicate the regex, modify view code to # Since this is a non-trivial problem to solve in Javascript (heavily complicate the regex, modify view code to
...@@ -18,6 +148,29 @@ module Gitlab ...@@ -18,6 +148,29 @@ module Gitlab
# So `group/subgroup` will match this regex but not NAMESPACE_REGEX_STR # So `group/subgroup` will match this regex but not NAMESPACE_REGEX_STR
FULL_NAMESPACE_REGEX_STR = "(?:#{NAMESPACE_REGEX_STR}/)*#{NAMESPACE_REGEX_STR}".freeze FULL_NAMESPACE_REGEX_STR = "(?:#{NAMESPACE_REGEX_STR}/)*#{NAMESPACE_REGEX_STR}".freeze
def root_namespace_route_regex
@root_namespace_route_regex ||= begin
illegal_words = Regexp.new(Regexp.union(TOP_LEVEL_ROUTES).source, Regexp::IGNORECASE)
single_line_regexp %r{
(?!(#{illegal_words})/)
#{NAMESPACE_REGEX_STR}
}x
end
end
def root_namespace_path_regex
@root_namespace_path_regex ||= %r{\A#{root_namespace_route_regex}/\z}
end
def full_namespace_path_regex
@full_namespace_path_regex ||= %r{\A#{namespace_route_regex}/\z}
end
def full_project_path_regex
@full_project_path_regex ||= %r{\A#{namespace_route_regex}/#{project_route_regex}/\z}
end
def namespace_regex def namespace_regex
@namespace_regex ||= /\A#{NAMESPACE_REGEX_STR}\z/.freeze @namespace_regex ||= /\A#{NAMESPACE_REGEX_STR}\z/.freeze
end end
...@@ -27,7 +180,18 @@ module Gitlab ...@@ -27,7 +180,18 @@ module Gitlab
end end
def namespace_route_regex def namespace_route_regex
@namespace_route_regex ||= /#{NAMESPACE_REGEX_STR}/.freeze @namespace_route_regex ||= begin
illegal_words = Regexp.new(Regexp.union(ILLEGAL_GROUP_PATH_WORDS).source, Regexp::IGNORECASE)
single_line_regexp %r{
#{root_namespace_route_regex}
(?:
/
(?!#{illegal_words}/)
#{NAMESPACE_REGEX_STR}
)*
}x
end
end end
def namespace_regex_message def namespace_regex_message
...@@ -53,15 +217,26 @@ module Gitlab ...@@ -53,15 +217,26 @@ module Gitlab
end end
def project_path_regex def project_path_regex
@project_path_regex ||= /\A#{PROJECT_REGEX_STR}\z/.freeze @project_path_regex ||= %r{\A#{project_route_regex}/\z}
end end
def project_route_regex def project_route_regex
@project_route_regex ||= /#{PROJECT_REGEX_STR}/.freeze @project_route_regex ||= begin
illegal_words = Regexp.new(Regexp.union(ILLEGAL_PROJECT_PATH_WORDS).source, Regexp::IGNORECASE)
single_line_regexp %r{
(?!(#{illegal_words})/)
#{PROJECT_REGEX_STR}
}x
end
end end
def project_git_route_regex def project_git_route_regex
@project_route_git_regex ||= /#{PATH_REGEX_STR}\.git/.freeze @project_git_route_regex ||= /#{project_route_regex}\.git/.freeze
end
def project_path_format_regex
@project_path_format_regex ||= /\A#{PROJECT_REGEX_STR}\z/.freeze
end end
def project_path_regex_message def project_path_regex_message
...@@ -86,7 +261,7 @@ module Gitlab ...@@ -86,7 +261,7 @@ module Gitlab
# Valid git ref regex, see: # Valid git ref regex, see:
# https://www.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html # https://www.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html
@git_reference_regex ||= %r{ @git_reference_regex ||= single_line_regexp %r{
(?! (?!
(?# doesn't begins with) (?# doesn't begins with)
\/| (?# rule #6) \/| (?# rule #6)
...@@ -102,7 +277,7 @@ module Gitlab ...@@ -102,7 +277,7 @@ module Gitlab
(?# doesn't end with) (?# doesn't end with)
(?<!\.lock) (?# rule #1) (?<!\.lock) (?# rule #1)
(?<![\/.]) (?# rule #6-7) (?<![\/.]) (?# rule #6-7)
}x.freeze }x
end end
def container_registry_reference_regex def container_registry_reference_regex
...@@ -140,5 +315,13 @@ module Gitlab ...@@ -140,5 +315,13 @@ module Gitlab
"can contain only lowercase letters, digits, and '-'. " \ "can contain only lowercase letters, digits, and '-'. " \
"Must start with a letter, and cannot end with '-'" "Must start with a letter, and cannot end with '-'"
end end
private
def single_line_regexp(regex)
# Turns a multiline extended regexp into a single line one,
# beacuse `rake routes` breaks on multiline regexes.
Regexp.new(regex.source.gsub(/\(\?#.+?\)/, '').gsub(/\s*/, ''), regex.options ^ Regexp::EXTENDED).freeze
end
end end
end end
...@@ -174,7 +174,7 @@ describe Import::GitlabController do ...@@ -174,7 +174,7 @@ describe Import::GitlabController do
end end
end end
end end
context 'user has chosen an existing nested namespace for the project' do context 'user has chosen an existing nested namespace for the project' do
let(:parent_namespace) { create(:namespace, name: 'foo', owner: user) } let(:parent_namespace) { create(:namespace, name: 'foo', owner: user) }
let(:nested_namespace) { create(:namespace, name: 'bar', parent: parent_namespace, owner: user) } let(:nested_namespace) { create(:namespace, name: 'bar', parent: parent_namespace, owner: user) }
......
This diff is collapsed.
...@@ -37,7 +37,7 @@ describe Namespace, models: true do ...@@ -37,7 +37,7 @@ describe Namespace, models: true do
it 'rejects nested paths' do it 'rejects nested paths' do
parent = create(:group, :nested, path: 'environments') parent = create(:group, :nested, path: 'environments')
namespace = build(:project, path: 'folders', namespace: parent) namespace = build(:group, path: 'folders', parent: parent)
expect(namespace).not_to be_valid expect(namespace).not_to be_valid
end end
......
...@@ -462,6 +462,8 @@ describe 'project routing' do ...@@ -462,6 +462,8 @@ describe 'project routing' do
expect(get('/gitlab/gitlabhq/blob/master/app/models/compare.rb')).to route_to('projects/blob#show', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'master/app/models/compare.rb') expect(get('/gitlab/gitlabhq/blob/master/app/models/compare.rb')).to route_to('projects/blob#show', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'master/app/models/compare.rb')
expect(get('/gitlab/gitlabhq/blob/master/app/models/diff.js')).to route_to('projects/blob#show', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'master/app/models/diff.js') expect(get('/gitlab/gitlabhq/blob/master/app/models/diff.js')).to route_to('projects/blob#show', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'master/app/models/diff.js')
expect(get('/gitlab/gitlabhq/blob/master/files.scss')).to route_to('projects/blob#show', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'master/files.scss') expect(get('/gitlab/gitlabhq/blob/master/files.scss')).to route_to('projects/blob#show', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'master/files.scss')
expect(get('/gitlab/gitlabhq/blob/master/blob/index.js')).to route_to('projects/blob#show', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'master/blob/index.js')
expect(get('/gitlab/gitlabhq/blob/blob/master/blob/index.js')).to route_to('projects/blob#show', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'blob/master/blob/index.js')
end end
end end
...@@ -470,6 +472,8 @@ describe 'project routing' do ...@@ -470,6 +472,8 @@ describe 'project routing' do
it 'to #show' do it 'to #show' do
expect(get('/gitlab/gitlabhq/tree/master/app/models/project.rb')).to route_to('projects/tree#show', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'master/app/models/project.rb') expect(get('/gitlab/gitlabhq/tree/master/app/models/project.rb')).to route_to('projects/tree#show', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'master/app/models/project.rb')
expect(get('/gitlab/gitlabhq/tree/master/files.scss')).to route_to('projects/tree#show', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'master/files.scss') expect(get('/gitlab/gitlabhq/tree/master/files.scss')).to route_to('projects/tree#show', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'master/files.scss')
expect(get('/gitlab/gitlabhq/tree/master/tree/files')).to route_to('projects/tree#show', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'master/tree/files')
expect(get('/gitlab/gitlabhq/tree/tree/master/tree/files')).to route_to('projects/tree#show', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'tree/master/tree/files')
end end
end end
......
...@@ -3,246 +3,46 @@ require 'spec_helper' ...@@ -3,246 +3,46 @@ require 'spec_helper'
describe DynamicPathValidator do describe DynamicPathValidator do
let(:validator) { described_class.new(attributes: [:path]) } let(:validator) { described_class.new(attributes: [:path]) }
# Pass in a full path to remove the format segment: describe '#path_valid_for_record?' do
# `/ci/lint(.:format)` -> `/ci/lint` context 'for project' do
def without_format(path) it 'calls valid_project_path?' do
path.split('(', 2)[0] project = build(:project, path: 'activity')
end
# 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`
# -> 'builds/artifacts'
def path_before_wildcard(path)
path = path.gsub(STARTING_WITH_NAMESPACE, "")
path_segments = path.split('/').reject(&:empty?)
wildcard_index = path_segments.index { |segment| parameter?(segment) }
segments_before_wildcard = path_segments[0..wildcard_index - 1]
segments_before_wildcard.join('/')
end
def parameter?(segment)
segment =~ /[*:]/
end
# If the path is reserved. Then no conflicting paths can# be created for any
# route using this reserved word.
#
# Both `builds/artifacts` & `build` are covered by reserving the word
# `build`
def wildcards_include?(path)
described_class::WILDCARD_ROUTES.include?(path) ||
described_class::WILDCARD_ROUTES.include?(path.split('/').first)
end
def failure_message(missing_words, constant_name, migration_helper)
missing_words = Array(missing_words)
<<-MSG
Found new routes that could cause conflicts with existing namespaced routes
for groups or projects.
Add <#{missing_words.join(', ')}> to `DynamicPathValidator::#{constant_name} expect(described_class).to receive(:valid_project_path?).with(project.full_path).and_call_original
to make sure no projects or namespaces can be created with those paths.
To rename any existing records with those paths you can use the expect(validator.path_valid_for_record?(project, 'activity')).to be_truthy
`Gitlab::Database::RenameReservedpathsMigration::<VERSION>.#{migration_helper}`
migration helper.
Make sure to make a note of the renamed records in the release blog post.
MSG
end
let(:all_routes) do
Rails.application.routes.routes.routes.
map { |r| r.path.spec.to_s }
end
let(:routes_without_format) { all_routes.map { |path| without_format(path) } }
# Routes not starting with `/:` or `/*`
# 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 a path-part that includes a wildcard parameter `*`
# At the time of writing these routes match: http://rubular.com/r/Rv2pDE5Dvw
STARTING_WITH_NAMESPACE = %r{^/\*namespace_id/:(project_)?id}
NON_PARAM_PARTS = %r{[^:*][a-z\-_/]*}
ANY_OTHER_PATH_PART = %r{[a-z\-_/:]*}
WILDCARD_SEGMENT = %r{\*}
let(:namespaced_wildcard_routes) do
routes_without_format.select do |p|
p =~ %r{#{STARTING_WITH_NAMESPACE}/#{NON_PARAM_PARTS}/#{ANY_OTHER_PATH_PART}#{WILDCARD_SEGMENT}}
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_before_wildcard(route)
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
describe 'TOP_LEVEL_ROUTES' do
it 'includes all the top level namespaces' do
failure_block = lambda do
missing_words = top_level_words - described_class::TOP_LEVEL_ROUTES
failure_message(missing_words, 'TOP_LEVEL_ROUTES', 'rename_root_paths')
end end
expect(described_class::TOP_LEVEL_ROUTES)
.to include(*top_level_words), failure_block
end end
end
describe 'GROUP_ROUTES' do context 'for group' do
it "don't contain a second wildcard" do it 'calls valid_namespace_path?' do
failure_block = lambda do group = build(:group, :nested, path: 'activity')
missing_words = paths_after_group_id - described_class::GROUP_ROUTES
failure_message(missing_words, 'GROUP_ROUTES', 'rename_child_paths')
end
expect(described_class::GROUP_ROUTES) expect(described_class).to receive(:valid_namespace_path?).with(group.full_path).and_call_original
.to include(*paths_after_group_id), failure_block
end
end
describe 'WILDCARD_ROUTES' do expect(validator.path_valid_for_record?(group, 'activity')).to be_falsey
it 'includes all paths that can be used after a namespace/project path' do
aggregate_failures do
all_wildcard_paths.each do |path|
expect(wildcards_include?(path))
.to be(true), failure_message(path, 'WILDCARD_ROUTES', 'rename_wildcard_paths')
end
end end
end end
end
describe '.without_reserved_wildcard_paths_regex' do context 'for user' do
subject { described_class.without_reserved_wildcard_paths_regex } it 'calls valid_namespace_path?' do
user = build(:user, username: 'activity')
it 'rejects paths starting with a reserved top level' do expect(described_class).to receive(:valid_namespace_path?).with(user.full_path).and_call_original
expect(subject).not_to match('dashboard/hello/world')
expect(subject).not_to match('dashboard')
end
it 'matches valid paths with a toplevel word in a different place' do expect(validator.path_valid_for_record?(user, 'activity')).to be_truthy
expect(subject).to match('parent/dashboard/project-path') end
end
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 'matches valid paths' do
expect(subject).to match('parent/child/project-path')
end
end
describe '.regex_excluding_child_paths' do
let(:subject) { described_class.without_reserved_child_paths_regex }
it 'rejects paths containing a child reserved word' do
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
it 'allows a child path on the top level' do
expect(subject).to match('activity/foo')
expect(subject).to match('avatar')
end
end
describe ".valid?" do
it 'is not case sensitive' do
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_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_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_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
it 'rejects paths that are in an incorrect format' do context 'for user namespace' do
test_path = 'incorrect/format.git' it 'calls valid_namespace_path?' do
user = create(:user, username: 'activity')
expect(described_class.valid?(test_path)).to be_falsey namespace = user.namespace
end
end
describe '#path_reserved_for_record?' do expect(described_class).to receive(:valid_namespace_path?).with(namespace.full_path).and_call_original
it 'reserves a sub-group named activity' do
group = build(:group, :nested, path: 'activity')
expect(validator.path_reserved_for_record?(group, 'activity')).to be_truthy expect(validator.path_valid_for_record?(namespace, 'activity')).to be_truthy
end end
it "doesn't reserve a project called activity" do
project = build(:project, path: 'activity')
expect(validator.path_reserved_for_record?(project, 'activity')).to be_falsey
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