Validate branch-names and references in WebUI, API

Add specs for GitRefValidator

11 files changed, 184 insertions, 25 deletions

diff --git a/app/controllers/projects/branches_controller.rb b/app/controllers/projects/branches_controller.rb
index 3c8e7ec73f6..6845fc5e6e6 100644
--- a/app/controllers/projects/branches_controller.rb
+++ b/app/controllers/projects/branches_controller.rb
@@ -17,9 +17,17 @@ class Projects::BranchesController < Projects::ApplicationController
   end
 
   def create
-    @branch = CreateBranchService.new.execute(project, params[:branch_name], params[:ref], current_user)
-
-    redirect_to project_tree_path(@project, @branch.name)
+    result = CreateBranchService.new.execute(project,
+                                             params[:branch_name],
+                                             params[:ref],
+                                             current_user)
+    if result[:status] == :success
+      @branch = result[:branch]
+      redirect_to project_tree_path(@project, @branch.name)
+    else
+      @error = result[:message]
+      render action: 'new'
+    end
   end
 
   def destroy
diff --git a/app/services/create_branch_service.rb b/app/services/create_branch_service.rb
index 98beeee8354..79b8239602e 100644
--- a/app/services/create_branch_service.rb
+++ b/app/services/create_branch_service.rb
@@ -1,13 +1,38 @@
 class CreateBranchService
   def execute(project, branch_name, ref, current_user)
+    valid_branch = Gitlab::GitRefValidator.validate(branch_name)
+    if valid_branch == false
+      return error('Branch name invalid')
+    end
+
     repository = project.repository
+    existing_branch = repository.find_branch(branch_name)
+    if existing_branch
+      return error('Branch already exists')
+    end
+
     repository.add_branch(branch_name, ref)
     new_branch = repository.find_branch(branch_name)
 
     if new_branch
       Event.create_ref_event(project, current_user, new_branch, 'add')
+      return success(new_branch)
+    else
+      return error('Invalid reference name')
     end
+  end
+
+  def error(message)
+    {
+      message: message,
+      status: :error
+    }
+  end
 
-    new_branch
+  def success(branch)
+    {
+      branch: branch,
+      status: :success
+    }
   end
 end
diff --git a/app/services/delete_branch_service.rb b/app/services/delete_branch_service.rb
index ce2d8093dff..a94dabcdfc0 100644
--- a/app/services/delete_branch_service.rb
+++ b/app/services/delete_branch_service.rb
@@ -5,21 +5,21 @@ class DeleteBranchService
 
     # No such branch
     unless branch
-      return error('No such branch')
+      return error('No such branch', 404)
     end
 
     if branch_name == repository.root_ref
-      return error('Cannot remove HEAD branch')
+      return error('Cannot remove HEAD branch', 405)
     end
 
     # Dont allow remove of protected branch
     if project.protected_branch?(branch_name)
-      return error('Protected branch cant be removed')
+      return error('Protected branch cant be removed', 405)
     end
 
     # Dont allow user to remove branch if he is not allowed to push
     unless current_user.can?(:push_code, project)
-      return error('You dont have push access to repo')
+      return error('You dont have push access to repo', 405)
     end
 
     if repository.rm_branch(branch_name)
@@ -30,9 +30,10 @@ class DeleteBranchService
     end
   end
 
-  def error(message)
+  def error(message, return_code = 400)
     {
       message: message,
+      return_code: return_code,
       state: :error
     }
   end
diff --git a/app/views/projects/branches/new.html.haml b/app/views/projects/branches/new.html.haml
index 5da2ede2937..3f202f7ea6b 100644
--- a/app/views/projects/branches/new.html.haml
+++ b/app/views/projects/branches/new.html.haml
@@ -1,3 +1,7 @@
+- if @error
+  .alert.alert-danger
+    %button{ type: "button", class: "close", "data-dismiss" => "alert"} &times;
+    = @error
 %h3.page-title
   %i.icon-code-fork
   New branch
@@ -5,11 +9,11 @@
   .form-group
     = label_tag :branch_name, 'Name for new branch', class: 'control-label'
     .col-sm-10
-      = text_field_tag :branch_name, nil, placeholder: 'enter new branch name', required: true, tabindex: 1, class: 'form-control'
+      = text_field_tag :branch_name, params[:branch_name], placeholder: 'enter new branch name', required: true, tabindex: 1, class: 'form-control'
   .form-group
     = label_tag :ref, 'Create from', class: 'control-label'
     .col-sm-10
-      = text_field_tag :ref, nil, placeholder: 'existing branch name, tag or commit SHA', required: true, tabindex: 2, class: 'form-control'
+      = text_field_tag :ref, params[:ref], placeholder: 'existing branch name, tag or commit SHA', required: true, tabindex: 2, class: 'form-control'
   .form-actions
     = submit_tag 'Create branch', class: 'btn btn-create', tabindex: 3
     = link_to 'Cancel', project_branches_path(@project), class: 'btn btn-cancel'
diff --git a/doc/api/branches.md b/doc/api/branches.md
index 31469b6fe97..74386615545 100644
--- a/doc/api/branches.md
+++ b/doc/api/branches.md
@@ -196,6 +196,8 @@ Parameters:
 }
 ```
 
+It return 200 if succeed or 400 if failed with error message explaining reason.
+
 ## Delete repository branch
 
 ```
@@ -207,4 +209,5 @@ Parameters:
 - `id` (required) - The ID of a project
 - `branch` (required) - The name of the branch
 
-It return 200 if succeed or 405 if failed with error message explaining reason.
+It return 200 if succeed, 404 if the branch to be deleted does not exist
+or 400 for other reasons. In case of an error, an explaining message is provided.
diff --git a/features/project/commits/branches.feature b/features/project/commits/branches.feature
index d657bd4951f..6725a697c21 100644
--- a/features/project/commits/branches.feature
+++ b/features/project/commits/branches.feature
@@ -23,3 +23,21 @@ Feature: Project Browse branches
     Given I visit project branches page
     And I click branch 'improve/awesome' delete link
     Then I should not see branch 'improve/awesome'
+
+  Scenario: I create a branch with invalid name
+    Given I visit project branches page
+    And I click new branch link
+    When I submit new branch form with invalid name
+    Then I should see new an error that branch is invalid
+
+  Scenario: I create a branch with invalid reference
+    Given I visit project branches page
+    And I click new branch link
+    When I submit new branch form with invalid reference
+    Then I should see new an error that ref is invalid
+
+  Scenario: I create a branch that already exists
+    Given I visit project branches page
+    And I click new branch link
+    When I submit new branch form with branch that already exists
+    Then I should see new an error that branch already exists
diff --git a/features/steps/project/browse_branches.rb b/features/steps/project/browse_branches.rb
index c00a95a62fd..cfc88bdad22 100644
--- a/features/steps/project/browse_branches.rb
+++ b/features/steps/project/browse_branches.rb
@@ -38,10 +38,38 @@ class ProjectBrowseBranches < Spinach::FeatureSteps
     click_button 'Create branch'
   end
 
+  step 'I submit new branch form with invalid name' do
+    fill_in 'branch_name', with: '1.0 stable'
+    fill_in 'ref', with: 'master'
+    click_button 'Create branch'
+  end
+
+  step 'I submit new branch form with invalid reference' do
+    fill_in 'branch_name', with: 'foo'
+    fill_in 'ref', with: 'foo'
+    click_button 'Create branch'
+  end
+
+  step 'I submit new branch form with branch that already exists' do
+    fill_in 'branch_name', with: 'master'
+    fill_in 'ref', with: 'master'
+    click_button 'Create branch'
+  end
+
   step 'I should see new branch created' do
-    within '.tree-ref-holder' do
-      page.should have_content 'deploy_keys'
-    end
+    page.should have_content 'deploy_keys'
+  end
+
+  step 'I should see new an error that branch is invalid' do
+    page.should have_content 'Branch name invalid'
+  end
+
+  step 'I should see new an error that ref is invalid' do
+    page.should have_content 'Invalid reference name'
+  end
+
+  step 'I should see new an error that branch already exists' do
+    page.should have_content 'Branch already exists'
   end
 
   step "I click branch 'improve/awesome' delete link" do
diff --git a/lib/api/branches.rb b/lib/api/branches.rb
index b32a4aa7bc2..4db5f61dd28 100644
--- a/lib/api/branches.rb
+++ b/lib/api/branches.rb
@@ -80,9 +80,17 @@ module API
       #   POST /projects/:id/repository/branches
       post ":id/repository/branches" do
         authorize_push_project
-        @branch = CreateBranchService.new.execute(user_project, params[:branch_name], params[:ref], current_user)
-
-        present @branch, with: Entities::RepoObject, project: user_project
+        result = CreateBranchService.new.execute(user_project,
+                                                 params[:branch_name],
+                                                 params[:ref],
+                                                 current_user)
+        if result[:status] == :success
+          present result[:branch],
+                  with: Entities::RepoObject,
+                  project: user_project
+        else
+          render_api_error!(result[:message], 400)
+        end
       end
 
       # Delete branch
@@ -99,7 +107,7 @@ module API
         if result[:state] == :success
           true
         else
-          render_api_error!(result[:message], 405)
+          render_api_error!(result[:message], result[:return_code])
         end
       end
     end
diff --git a/lib/gitlab/git_ref_validator.rb b/lib/gitlab/git_ref_validator.rb
new file mode 100644
index 00000000000..13cb08948bb
--- /dev/null
+++ b/lib/gitlab/git_ref_validator.rb
@@ -0,0 +1,11 @@
+module Gitlab
+  module GitRefValidator
+    extend self
+    # Validates a given name against the git reference specification
+    #
+    # Returns true for a valid reference name, false otherwise
+    def validate(ref_name)
+      system *%W(git check-ref-format refs/#{ref_name})
+    end
+  end
+end
diff --git a/spec/lib/git_ref_validator_spec.rb b/spec/lib/git_ref_validator_spec.rb
new file mode 100644
index 00000000000..b2469c18395
--- /dev/null
+++ b/spec/lib/git_ref_validator_spec.rb
@@ -0,0 +1,20 @@
+require 'spec_helper'
+
+describe Gitlab::GitRefValidator do
+  it { Gitlab::GitRefValidator.validate('feature/new').should be_true }
+  it { Gitlab::GitRefValidator.validate('implement_@all').should be_true }
+  it { Gitlab::GitRefValidator.validate('my_new_feature').should be_true }
+  it { Gitlab::GitRefValidator.validate('#1').should be_true }
+  it { Gitlab::GitRefValidator.validate('feature/~new/').should be_false }
+  it { Gitlab::GitRefValidator.validate('feature/^new/').should be_false }
+  it { Gitlab::GitRefValidator.validate('feature/:new/').should be_false }
+  it { Gitlab::GitRefValidator.validate('feature/?new/').should be_false }
+  it { Gitlab::GitRefValidator.validate('feature/*new/').should be_false }
+  it { Gitlab::GitRefValidator.validate('feature/[new/').should be_false }
+  it { Gitlab::GitRefValidator.validate('feature/new/').should be_false }
+  it { Gitlab::GitRefValidator.validate('feature/new.').should be_false }
+  it { Gitlab::GitRefValidator.validate('feature\@{').should be_false }
+  it { Gitlab::GitRefValidator.validate('feature\new').should be_false }
+  it { Gitlab::GitRefValidator.validate('feature//new').should be_false }
+  it { Gitlab::GitRefValidator.validate('feature new').should be_false }
+end
diff --git a/spec/requests/api/branches_spec.rb b/spec/requests/api/branches_spec.rb
index f3d7ca2ed21..e7f91c5e46e 100644
--- a/spec/requests/api/branches_spec.rb
+++ b/spec/requests/api/branches_spec.rb
@@ -94,22 +94,50 @@ describe API::API, api: true  do
   describe "POST /projects/:id/repository/branches" do
     it "should create a new branch" do
       post api("/projects/#{project.id}/repository/branches", user),
-        branch_name: branch_name,
-        ref: branch_sha
+           branch_name: 'feature1',
+           ref: branch_sha
 
       response.status.should == 201
 
-      json_response['name'].should == branch_name
+      json_response['name'].should == 'feature1'
       json_response['commit']['id'].should == branch_sha
     end
 
     it "should deny for user without push access" do
       post api("/projects/#{project.id}/repository/branches", user2),
-        branch_name: branch_name,
-        ref: branch_sha
-
+           branch_name: branch_name,
+           ref: branch_sha
       response.status.should == 403
     end
+
+    it 'should return 400 if branch name is invalid' do
+      post api("/projects/#{project.id}/repository/branches", user),
+           branch_name: 'new design',
+           ref: branch_sha
+      response.status.should == 400
+      json_response['message'].should == 'Branch name invalid'
+    end
+
+    it 'should return 400 if branch already exists' do
+      post api("/projects/#{project.id}/repository/branches", user),
+           branch_name: 'new_design1',
+           ref: branch_sha
+      response.status.should == 201
+
+      post api("/projects/#{project.id}/repository/branches", user),
+           branch_name: 'new_design1',
+           ref: branch_sha
+      response.status.should == 400
+      json_response['message'].should == 'Branch already exists'
+    end
+
+    it 'should return 400 if ref name is invalid' do
+      post api("/projects/#{project.id}/repository/branches", user),
+           branch_name: 'new_design3',
+           ref: 'foo'
+      response.status.should == 400
+      json_response['message'].should == 'Invalid reference name'
+    end
   end
 
   describe "DELETE /projects/:id/repository/branches/:branch" do
@@ -120,6 +148,11 @@ describe API::API, api: true  do
       response.status.should == 200
     end
 
+    it 'should return 404 if branch not exists' do
+      delete api("/projects/#{project.id}/repository/branches/foobar", user)
+      response.status.should == 404
+    end
+
     it "should remove protected branch" do
       project.protected_branches.create(name: branch_name)
       delete api("/projects/#{project.id}/repository/branches/#{branch_name}", user)