Fix ambiguous routing issues by teaching router about reserved words

app/controllers/projects/refs_controller.rb

@@ -74,6 +74,6 @@ class Projects::RefsController < Projects::ApplicationController
   private
   def validate_ref_id
-    return not_found! if params[:id].present? && params[:id] !~ Gitlab::Regex.git_reference_regex
+    return not_found! if params[:id].present? && params[:id] !~ Gitlab::PathRegex.git_reference_regex
   end
 end

app/models/project.rb

@@ -205,8 +205,8 @@ class Project < ActiveRecord::Base
     presence: true,
     dynamic_path: true,
     length: { maximum: 255 },
-    format: { with: Gitlab::Regex.project_path_regex,
-              message: Gitlab::Regex.project_path_regex_message },
+    format: { with: Gitlab::PathRegex.project_path_format_regex,
+              message: Gitlab::PathRegex.project_path_format_message },
     uniqueness: { scope: :namespace_id }
   validates :namespace, presence: true
@@ -380,11 +380,9 @@ class Project < ActiveRecord::Base
     end
     def reference_pattern
-      name_pattern = Gitlab::Regex::FULL_NAMESPACE_REGEX_STR
-
       %r{
-        ((?<namespace>#{name_pattern})\/)?
-        (?<project>#{name_pattern})
+        ((?<namespace>#{Gitlab::PathRegex::FULL_NAMESPACE_FORMAT_REGEX})\/)?
+        (?<project>#{Gitlab::PathRegex::PROJECT_PATH_FORMAT_REGEX})
       }x
     end

app/models/user.rb

@@ -367,7 +367,7 @@ class User < ActiveRecord::Base
     def reference_pattern
       %r{
         #{Regexp.escape(reference_prefix)}
-        (?<user>#{Gitlab::Regex::FULL_NAMESPACE_REGEX_STR})
+        (?<user>#{Gitlab::PathRegex::FULL_NAMESPACE_FORMAT_REGEX})
       }x
     end

app/validators/dynamic_path_validator.rb

@@ -3,212 +3,45 @@
 # Custom validator for GitLab path values.
 # These paths are assigned to `Namespace` (& `Group` as a subclass) & `Project`
 #
-# Values are checked for formatting and exclusion from a list of reserved path
+# Values are checked for formatting and exclusion from a list of illegal path
 # names.
 class DynamicPathValidator < ActiveModel::EachValidator
-  # 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`.
-  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
+  class << self
+    def valid_user_path?(path)
+      "#{path}/" =~ Gitlab::PathRegex.root_namespace_path_regex
+    end
+    def valid_group_path?(path)
+      "#{path}/" =~ Gitlab::PathRegex.full_namespace_path_regex
+    end
+    def valid_project_path?(path)
+      "#{path}/" =~ Gitlab::PathRegex.full_project_path_regex
+    end
   end
-  delegate :full_path_reserved?,
-           :child_reserved?,
-           to: :class
-
-  def path_reserved_for_record?(record, value)
+  def path_valid_for_record?(record, value)
     full_path = record.respond_to?(:full_path) ? record.full_path : value
-    # For group paths the entire path cannot contain a reserved child word
-    # The path doesn't contain the last `_project_part` so we need to validate
-    # if the entire path.
-    # Example:
-    #   A *group* with full path `parent/activity` is reserved.
-    #   A *project* with full path `parent/activity` is allowed.
-    if record.is_a? Group
-      child_reserved?(full_path)
-    else
-      full_path_reserved?(full_path)
+    return true unless full_path
+
+    case record
+    when Project
+      self.class.valid_project_path?(full_path)
+    when Group
+      self.class.valid_group_path?(full_path)
+    else # User or non-Group Namespace
+      self.class.valid_user_path?(full_path)
     end
   end
   def validate_each(record, attribute, value)
-    unless value =~ Gitlab::Regex.namespace_regex
-      record.errors.add(attribute, Gitlab::Regex.namespace_regex_message)
+    unless value =~ Gitlab::PathRegex.namespace_format_regex
+      record.errors.add(attribute, Gitlab::PathRegex.namespace_format_message)
       return
     end
-    if path_reserved_for_record?(record, value)
+    unless path_valid_for_record?(record, value)
       record.errors.add(attribute, "#{value} is a reserved name")
     end
   end

app/views/devise/shared/_signup_box.html.haml

@@ -8,7 +8,7 @@
         = f.text_field :name, class: "form-control top", required: true, title: "This field is required."
       .username.form-group
         = f.label :username
-        = f.text_field :username, class: "form-control middle", pattern: Gitlab::Regex::NAMESPACE_REGEX_STR_JS, required: true, title: 'Please create a username with only alphanumeric characters.'
+        = f.text_field :username, class: "form-control middle", pattern: Gitlab::PathRegex::NAMESPACE_FORMAT_REGEX_JS, required: true, title: 'Please create a username with only alphanumeric characters.'
         %p.validation-error.hide Username is already taken.
         %p.validation-success.hide Username is available.
         %p.validation-pending.hide Checking username availability...

app/views/shared/_group_form.html.haml

@@ -15,7 +15,7 @@
           %strong= parent.full_path + '/'
       = f.text_field :path, placeholder: 'open-source', class: 'form-control',
         autofocus: local_assigns[:autofocus] || false, required: true,
-        pattern: Gitlab::Regex::NAMESPACE_REGEX_STR_JS,
+        pattern: Gitlab::PathRegex::NAMESPACE_FORMAT_REGEX_JS,
         title: 'Please choose a group path with no special characters.',
         "data-bind-in" => "#{'create_chat_team' if Gitlab.config.mattermost.enabled}"
       - if parent

config/routes.rb

 require 'sidekiq/web'
 require 'sidekiq/cron/web'
-require 'constraints/group_url_constrainer'
 Rails.application.routes.draw do
   concern :access_requestable do
@@ -85,20 +84,6 @@ Rails.application.routes.draw do
   root to: "root#index"
-  # Since group show page is wildcard routing
-  # we want all other routing to be checked before matching this one
-  constraints(GroupUrlConstrainer.new) do
-    scope(path: '*id',
-          as: :group,
-          constraints: { id: Gitlab::Regex.namespace_route_regex, format: /(html|json|atom)/ },
-          controller: :groups) do
-      get '/', action: :show
-      patch '/', action: :update
-      put '/', action: :update
-      delete '/', action: :destroy
-    end
-  end
-
   draw :test if Rails.env.test?
   get '*unmatched_route', to: 'application#route_not_found'

config/routes/admin.rb

@@ -36,7 +36,7 @@ namespace :admin do
   scope(path: 'groups/*id',
         controller: :groups,
-        constraints: { id: Gitlab::Regex.namespace_route_regex, format: /(html|json|atom)/ }) do
+        constraints: { id: Gitlab::PathRegex.full_namespace_route_regex, format: /(html|json|atom)/ }) do
     scope(as: :group) do
       put :members_update
@@ -68,10 +68,12 @@ namespace :admin do
   resources :projects, only: [:index]
-  scope(path: 'projects/*namespace_id', as: :namespace) do
+  scope(path: 'projects/*namespace_id',
+        as: :namespace,
+        constraints: { namespace_id: Gitlab::PathRegex.full_namespace_route_regex }) do
     resources(:projects,
               path: '/',
-              constraints: { id: Gitlab::Regex.project_route_regex },
+              constraints: { id: Gitlab::PathRegex.project_route_regex },
               only: [:show]) do
       member do

config/routes/git_http.rb

-scope(path: '*namespace_id/:project_id', constraints: { format: nil }) do
-  scope(constraints: { project_id: Gitlab::Regex.project_git_route_regex }, module: :projects) do
+scope(path: '*namespace_id/:project_id',
+      format: nil,
+      constraints: { namespace_id: Gitlab::PathRegex.full_namespace_route_regex }) do
+  scope(constraints: { project_id: Gitlab::PathRegex.project_git_route_regex }, module: :projects) do
     # Git HTTP clients ('git clone' etc.)
     scope(controller: :git_http) do
       get '/info/refs', action: :info_refs
@@ -26,7 +28,7 @@ scope(path: '*namespace_id/:project_id', constraints: { format: nil }) do
   end
   # Redirect /group/project/info/refs to /group/project.git/info/refs
-  scope(constraints: { project_id: Gitlab::Regex.project_route_regex }) do
+  scope(constraints: { project_id: Gitlab::PathRegex.project_route_regex }) do
     # Allow /info/refs, /info/refs?service=git-upload-pack, and
     # /info/refs?service=git-receive-pack, but nothing else.
     #

config/routes/group.rb

+require 'constraints/group_url_constrainer'
+
 resources :groups, only: [:index, :new, :create]
 scope(path: 'groups/*group_id',
       module: :groups,
       as: :group,
-      constraints: { group_id: Gitlab::Regex.namespace_route_regex }) do
+      constraints: { group_id: Gitlab::PathRegex.full_namespace_route_regex }) do
   resources :group_members, only: [:index, :create, :update, :destroy], concerns: :access_requestable do
     post :resend_invite, on: :member
     delete :leave, on: :collection
@@ -25,7 +27,7 @@ end
 scope(path: 'groups/*id',
       controller: :groups,
-      constraints: { id: Gitlab::Regex.namespace_route_regex, format: /(html|json|atom)/ }) do
+      constraints: { id: Gitlab::PathRegex.full_namespace_route_regex, format: /(html|json|atom)/ }) do
   get :edit, as: :edit_group
   get :issues, as: :issues_group
   get :merge_requests, as: :merge_requests_group
@@ -34,3 +36,15 @@ scope(path: 'groups/*id',
   get :subgroups, as: :subgroups_group
   get '/', action: :show, as: :group_canonical
 end
+
+constraints(GroupUrlConstrainer.new) do
+  scope(path: '*id',
+        as: :group,
+        constraints: { id: Gitlab::PathRegex.full_namespace_route_regex, format: /(html|json|atom)/ },
+        controller: :groups) do
+    get '/', action: :show
+    patch '/', action: :update
+    put '/', action: :update
+    delete '/', action: :destroy
+  end
+end

config/routes/project.rb

@@ -5,9 +5,24 @@ resources :projects, only: [:index, :new, :create]
 draw :git_http
 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::PathRegex.full_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::PathRegex.full_namespace_route_regex) do
     scope(path: ':project_id',
-          constraints: { project_id: Gitlab::Regex.project_route_regex },
+          constraints: { project_id: Gitlab::PathRegex.project_route_regex },
           module: :projects,
           as: :project) do
@@ -314,7 +329,7 @@ constraints(ProjectUrlConstrainer.new) do
       resources :runner_projects, only: [:create, :destroy]
       resources :badges, only: [:index] do
         collection do
-          scope '*ref', constraints: { ref: Gitlab::Regex.git_reference_regex } do
+          scope '*ref', constraints: { ref: Gitlab::PathRegex.git_reference_regex } do
             constraints format: /svg/ do
               get :build
               get :coverage
@@ -337,7 +352,7 @@ constraints(ProjectUrlConstrainer.new) do
     resources(:projects,
               path: '/',
-              constraints: { id: Gitlab::Regex.project_route_regex },
+              constraints: { id: Gitlab::PathRegex.project_route_regex },
               only: [:edit, :show, :update, :destroy]) do
       member do
         put :transfer

config/routes/repository.rb

@@ -2,7 +2,7 @@
 resource :repository, only: [:create] do
   member do
-    get 'archive', constraints: { format: Gitlab::Regex.archive_formats_regex }
+    get 'archive', constraints: { format: Gitlab::PathRegex.archive_formats_regex }
   end
 end
@@ -24,7 +24,7 @@ scope format: false do
     member do
       # tree viewer logs
-      get 'logs_tree', constraints: { id: Gitlab::Regex.git_reference_regex }
+      get 'logs_tree', constraints: { id: Gitlab::PathRegex.git_reference_regex }
       # Directories with leading dots erroneously get rejected if git
       # ref regex used in constraints. Regex verification now done in controller.
       get 'logs_tree/*path', action: :logs_tree, as: :logs_file, format: false, constraints: {
@@ -34,7 +34,7 @@ scope format: false do
     end
   end
-  scope constraints: { id: Gitlab::Regex.git_reference_regex } do
+  scope constraints: { id: Gitlab::PathRegex.git_reference_regex } do
     resources :network, only: [:show]
     resources :graphs, only: [:show] do

config/routes/user.rb

@@ -11,19 +11,7 @@ devise_scope :user do
   get '/users/almost_there' => 'confirmations#almost_there'
 end
-constraints(UserUrlConstrainer.new) do
-  # Get all keys of user
-  get ':username.keys' => 'profiles/keys#get_keys', constraints: { username: Gitlab::Regex.namespace_route_regex }
-
-  scope(path: ':username',
-        as: :user,
-        constraints: { username: Gitlab::Regex.namespace_route_regex },
-        controller: :users) do
-    get '/', action: :show
-  end
-end
-
-scope(constraints: { username: Gitlab::Regex.namespace_route_regex }) do
+scope(constraints: { username: Gitlab::PathRegex.root_namespace_route_regex }) do
   scope(path: 'users/:username',
         as: :user,
         controller: :users) do
@@ -46,3 +34,15 @@ scope(constraints: { username: Gitlab::Regex.namespace_route_regex }) do
   get '/u/:username/snippets', to: redirect('/users/%{username}/snippets')
   get '/u/:username/contributed', to: redirect('/users/%{username}/contributed')
 end
+
+constraints(UserUrlConstrainer.new) do
+  # Get all keys of user
+  get ':username.keys' => 'profiles/keys#get_keys', constraints: { username: Gitlab::PathRegex.root_namespace_route_regex }
+
+  scope(path: ':username',
+        as: :user,
+        constraints: { username: Gitlab::PathRegex.root_namespace_route_regex },
+        controller: :users) do
+    get '/', action: :show
+  end
+end

doc/user/group/subgroups/index.md

@@ -71,9 +71,9 @@ structure.
 - 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].
 - 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:
+  [`dynamic_path_validator.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
-  - `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.
 To create a subgroup:

lib/api/repositories.rb

@@ -85,7 +85,7 @@ module API
         optional :sha, type: String, desc: 'The commit sha of the archive to be downloaded'
         optional :format, type: String, desc: 'The archive format'
       end
-      get ':id/repository/archive', requirements: { format: Gitlab::Regex.archive_formats_regex } do
+      get ':id/repository/archive', requirements: { format: Gitlab::PathRegex.archive_formats_regex } do
         begin
           send_git_archive user_project.repository, ref: params[:sha], format: params[:format]
         rescue

lib/api/v3/repositories.rb

@@ -72,7 +72,7 @@ module API
           optional :sha, type: String, desc: 'The commit sha of the archive to be downloaded'
           optional :format, type: String, desc: 'The archive format'
         end
-        get ':id/repository/archive', requirements: { format: Gitlab::Regex.archive_formats_regex } do
+        get ':id/repository/archive', requirements: { format: Gitlab::PathRegex.archive_formats_regex } do
           begin
             send_git_archive user_project.repository, ref: params[:sha], format: params[:format]
           rescue

lib/constraints/group_url_constrainer.rb

 class GroupUrlConstrainer
   def matches?(request)
-    id = request.params[:id]
-    return false unless DynamicPathValidator.valid?(id)
-    Group.find_by_full_path(id, follow_redirects: request.get?).present?
+    full_path = request.params[:group_id] || request.params[:id]
+    return false unless DynamicPathValidator.valid_group_path?(full_path)
+    Group.find_by_full_path(full_path, follow_redirects: request.get?).present?
   end
 end

lib/constraints/project_url_constrainer.rb

@@ -2,9 +2,9 @@ class ProjectUrlConstrainer
   def matches?(request)
     namespace_path = request.params[:namespace_id]
     project_path = request.params[:project_id] || request.params[:id]
-    full_path = namespace_path + '/' + project_path
-    return false unless DynamicPathValidator.valid?(full_path)
+    full_path = [namespace_path, project_path].join('/')
+    return false unless DynamicPathValidator.valid_project_path?(full_path)
     Project.find_by_full_path(full_path, follow_redirects: request.get?).present?
   end

lib/constraints/user_url_constrainer.rb

 class UserUrlConstrainer
   def matches?(request)
-    User.find_by_full_path(request.params[:username], follow_redirects: request.get?).present?
+    full_path = request.params[:username]
+
+    return false unless DynamicPathValidator.valid_user_path?(full_path)
+
+    User.find_by_full_path(full_path, follow_redirects: request.get?).present?
   end
 end

lib/gitlab/etag_caching/router.rb

@@ -10,7 +10,7 @@ module Gitlab
       #   - Ending in `issues/id`/realtime_changes` for the `issue_title` route
       USED_IN_ROUTES = %w[noteable issue notes issues realtime_changes
                           commit pipelines merge_requests new].freeze
-      RESERVED_WORDS = DynamicPathValidator::WILDCARD_ROUTES - USED_IN_ROUTES
+      RESERVED_WORDS = Gitlab::PathRegex::ILLEGAL_PROJECT_PATH_WORDS - USED_IN_ROUTES
       RESERVED_WORDS_REGEX = Regexp.union(*RESERVED_WORDS)
       ROUTES = [
         Gitlab::EtagCaching::Router::Route.new(