Merge pull request #4968 from pdf/issue_4831

Expand refs constraints to include valid characters

config/routes.rb

@@ -222,14 +222,14 @@ Gitlab::Application.routes.draw do
         end
       end
 
-      resources :branches, only: [:index, :new, :create, :destroy], constraints: { id: /[a-zA-Z.\/0-9_\-#%+]+/ } do
+      resources :branches, only: [:index, :new, :create, :destroy], constraints: { id: Gitlab::Regex.git_reference_regex } do
         collection do
-          get :recent
+          get :recent, constraints: { id: Gitlab::Regex.git_reference_regex }
         end
       end
 
-      resources :tags, only: [:index, :new, :create, :destroy], constraints: { id: /[a-zA-Z.\/0-9_\-#%+]+/ }
-      resources :protected_branches, only: [:index, :create, :destroy], constraints: { id: /[a-zA-Z.\/0-9_\-#%+]+/ }
+      resources :tags, only: [:index, :new, :create, :destroy], constraints: { id: Gitlab::Regex.git_reference_regex }
+      resources :protected_branches, only: [:index, :create, :destroy], constraints: { id: Gitlab::Regex.git_reference_regex }
 
       resources :refs, only: [] do
         collection do
@@ -238,11 +238,11 @@ Gitlab::Application.routes.draw do
 
         member do
           # tree viewer logs
-          get "logs_tree", constraints: { id: /[a-zA-Z.\/0-9_\-#%+]+/ }
+          get "logs_tree", constraints: { id: Gitlab::Regex.git_reference_regex }
           get "logs_tree/:path" => "refs#logs_tree",
             as: :logs_file,
             constraints: {
-              id:   /[a-zA-Z.0-9\/_\-#%+]+/,
+              id:   Gitlab::Regex.git_reference_regex,
               path: /.*/
             }
         end

lib/gitlab/regex.rb

@@ -18,6 +18,29 @@ module Gitlab
       default_regex
     end
 
+    def git_reference_regex
+      # Valid git ref regex, see:
+      # https://www.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html
+
+      %r{
+        (?!
+           # doesn't begins with
+           \/|                    # (rule #6)
+           # doesn't contain
+           .*(?:
+              [\/.]\.|            # (rule #1,3)
+              \/\/|               # (rule #6)
+              @\{|                # (rule #8)
+              \\                  # (rule #9)
+           )
+        )
+        [^\000-\040\177~^:?*\[]+  # (rule #4-5)
+         # doesn't end with
+        (?<!\.lock)               # (rule #1)
+        (?<![\/.])                # (rule #6-7)
+      }x
+    end
+
     protected
 
     def default_regex

spec/routing/project_routing_spec.rb

@@ -138,12 +138,24 @@ end
 describe Projects::BranchesController, "routing" do
   it "to #branches" do
     get("/gitlab/gitlabhq/branches").should route_to('projects/branches#index', project_id: 'gitlab/gitlabhq')
+    delete("/gitlab/gitlabhq/branches/feature%2345").should route_to('projects/branches#destroy', project_id: 'gitlab/gitlabhq', id: 'feature#45')
+    delete("/gitlab/gitlabhq/branches/feature%2B45").should route_to('projects/branches#destroy', project_id: 'gitlab/gitlabhq', id: 'feature+45')
+    delete("/gitlab/gitlabhq/branches/feature@45").should route_to('projects/branches#destroy', project_id: 'gitlab/gitlabhq', id: 'feature@45')
+    delete("/gitlab/gitlabhq/branches/feature%2345/foo/bar/baz").should route_to('projects/branches#destroy', project_id: 'gitlab/gitlabhq', id: 'feature#45/foo/bar/baz')
+    delete("/gitlab/gitlabhq/branches/feature%2B45/foo/bar/baz").should route_to('projects/branches#destroy', project_id: 'gitlab/gitlabhq', id: 'feature+45/foo/bar/baz')
+    delete("/gitlab/gitlabhq/branches/feature@45/foo/bar/baz").should route_to('projects/branches#destroy', project_id: 'gitlab/gitlabhq', id: 'feature@45/foo/bar/baz')
   end
 end
 
 describe Projects::TagsController, "routing" do
   it "to #tags" do
     get("/gitlab/gitlabhq/tags").should route_to('projects/tags#index', project_id: 'gitlab/gitlabhq')
+    delete("/gitlab/gitlabhq/tags/feature%2345").should route_to('projects/tags#destroy', project_id: 'gitlab/gitlabhq', id: 'feature#45')
+    delete("/gitlab/gitlabhq/tags/feature%2B45").should route_to('projects/tags#destroy', project_id: 'gitlab/gitlabhq', id: 'feature+45')
+    delete("/gitlab/gitlabhq/tags/feature@45").should route_to('projects/tags#destroy', project_id: 'gitlab/gitlabhq', id: 'feature@45')
+    delete("/gitlab/gitlabhq/tags/feature%2345/foo/bar/baz").should route_to('projects/tags#destroy', project_id: 'gitlab/gitlabhq', id: 'feature#45/foo/bar/baz')
+    delete("/gitlab/gitlabhq/tags/feature%2B45/foo/bar/baz").should route_to('projects/tags#destroy', project_id: 'gitlab/gitlabhq', id: 'feature+45/foo/bar/baz')
+    delete("/gitlab/gitlabhq/tags/feature@45/foo/bar/baz").should route_to('projects/tags#destroy', project_id: 'gitlab/gitlabhq', id: 'feature@45/foo/bar/baz')
   end
 end
 
@@ -183,9 +195,11 @@ describe Projects::RefsController, "routing" do
     get("/gitlab/gitlabhq/refs/stable/logs_tree").should             route_to('projects/refs#logs_tree', project_id: 'gitlab/gitlabhq', id: 'stable')
     get("/gitlab/gitlabhq/refs/feature%2345/logs_tree").should             route_to('projects/refs#logs_tree', project_id: 'gitlab/gitlabhq', id: 'feature#45')
     get("/gitlab/gitlabhq/refs/feature%2B45/logs_tree").should             route_to('projects/refs#logs_tree', project_id: 'gitlab/gitlabhq', id: 'feature+45')
+    get("/gitlab/gitlabhq/refs/feature@45/logs_tree").should             route_to('projects/refs#logs_tree', project_id: 'gitlab/gitlabhq', id: 'feature@45')
     get("/gitlab/gitlabhq/refs/stable/logs_tree/foo/bar/baz").should route_to('projects/refs#logs_tree', project_id: 'gitlab/gitlabhq', id: 'stable', path: 'foo/bar/baz')
     get("/gitlab/gitlabhq/refs/feature%2345/logs_tree/foo/bar/baz").should route_to('projects/refs#logs_tree', project_id: 'gitlab/gitlabhq', id: 'feature#45', path: 'foo/bar/baz')
     get("/gitlab/gitlabhq/refs/feature%2B45/logs_tree/foo/bar/baz").should route_to('projects/refs#logs_tree', project_id: 'gitlab/gitlabhq', id: 'feature+45', path: 'foo/bar/baz')
+    get("/gitlab/gitlabhq/refs/feature@45/logs_tree/foo/bar/baz").should route_to('projects/refs#logs_tree', project_id: 'gitlab/gitlabhq', id: 'feature@45', path: 'foo/bar/baz')
     get("/gitlab/gitlabhq/refs/stable/logs_tree/files.scss").should route_to('projects/refs#logs_tree', project_id: 'gitlab/gitlabhq', id: 'stable', path: 'files.scss')
   end
 end