Fix ambiguous routing issues by teaching router about reserved words

7 files changed, 61 insertions, 43 deletions

diff --git a/config/routes.rb b/config/routes.rb
index 2584981bb04..846054e6917 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -1,6 +1,5 @@
 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'
diff --git a/config/routes/admin.rb b/config/routes/admin.rb
index 48993420ed9..6e34dd3a2eb 100644
--- a/config/routes/admin.rb
+++ b/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
diff --git a/config/routes/git_http.rb b/config/routes/git_http.rb
index 42d874eeebc..a53c94326d4 100644
--- a/config/routes/git_http.rb
+++ b/config/routes/git_http.rb
@@ -1,5 +1,7 @@
-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.
     #
diff --git a/config/routes/group.rb b/config/routes/group.rb
index 7b29e0e807c..11cdff55ed8 100644
--- a/config/routes/group.rb
+++ b/config/routes/group.rb
@@ -1,9 +1,11 @@
+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
diff --git a/config/routes/project.rb b/config/routes/project.rb
index 01b94f9f2b8..a2bc63a4734 100644
--- a/config/routes/project.rb
+++ b/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
diff --git a/config/routes/repository.rb b/config/routes/repository.rb
index 5cf37a06e97..11911636fa7 100644
--- a/config/routes/repository.rb
+++ b/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
diff --git a/config/routes/user.rb b/config/routes/user.rb
index b064a15e802..df49a752f5e 100644
--- a/config/routes/user.rb
+++ b/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