diff options
author | Robert Speicher <robert@gitlab.com> | 2017-05-24 18:08:05 +0300 |
---|---|---|
committer | Timothy Andrew <mail@timothyandrew.net> | 2017-05-25 08:04:04 +0300 |
commit | 218eae87754711b9e89a98301ae04ca293fdf934 (patch) | |
tree | 97c4e9216e9390e468e2ee152facc00a31e99e41 /lib | |
parent | a17b740a0b3e6c1570b1d2147074c2acbf2f2c1c (diff) |
Merge branch 'dm-fix-routes' into 'master'
Fix ambiguous routing issues by teaching router about reserved words
See merge request !11570
Diffstat (limited to 'lib')
-rw-r--r-- | lib/constraints/group_url_constrainer.rb | 4 | ||||
-rw-r--r-- | lib/constraints/project_url_constrainer.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/etag_caching/router.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/regex.rb | 195 |
4 files changed, 193 insertions, 10 deletions
diff --git a/lib/constraints/group_url_constrainer.rb b/lib/constraints/group_url_constrainer.rb index 5f379756c11..0ea2f97352d 100644 --- a/lib/constraints/group_url_constrainer.rb +++ b/lib/constraints/group_url_constrainer.rb @@ -1,8 +1,8 @@ class GroupUrlConstrainer 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? end diff --git a/lib/constraints/project_url_constrainer.rb b/lib/constraints/project_url_constrainer.rb index 6f542f63f98..4444a1abee3 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 - 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? end diff --git a/lib/gitlab/etag_caching/router.rb b/lib/gitlab/etag_caching/router.rb index 12f1ef35ae2..d74e31af5c6 100644 --- a/lib/gitlab/etag_caching/router.rb +++ b/lib/gitlab/etag_caching/router.rb @@ -10,7 +10,7 @@ module Gitlab # - Ending in `issues/id`/rendered_title` for the `issue_title` route USED_IN_ROUTES = %w[noteable issue notes issues rendered_title 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) ROUTES = [ Gitlab::EtagCaching::Router::Route.new( diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb index b7fef5dd068..f609850f8fa 100644 --- a/lib/gitlab/regex.rb +++ b/lib/gitlab/regex.rb @@ -2,6 +2,136 @@ module Gitlab module Regex 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 # 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 @@ -18,6 +148,29 @@ module Gitlab # So `group/subgroup` will match this regex but not NAMESPACE_REGEX_STR 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 @namespace_regex ||= /\A#{NAMESPACE_REGEX_STR}\z/.freeze end @@ -27,7 +180,18 @@ module Gitlab end 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 def namespace_regex_message @@ -53,15 +217,26 @@ module Gitlab end def project_path_regex - @project_path_regex ||= /\A#{PROJECT_REGEX_STR}\z/.freeze + @project_path_regex ||= %r{\A#{project_route_regex}/\z} end 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 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 def project_path_regex_message @@ -86,7 +261,7 @@ module Gitlab # Valid git ref regex, see: # 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) \/| (?# rule #6) @@ -102,7 +277,7 @@ module Gitlab (?# doesn't end with) (?<!\.lock) (?# rule #1) (?<![\/.]) (?# rule #6-7) - }x.freeze + }x end def container_registry_reference_regex @@ -140,5 +315,13 @@ module Gitlab "can contain only lowercase letters, digits, and '-'. " \ "Must start with a letter, and cannot end with '-'" 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 |