Merge branch 'branch-invalid-name' into 'master'

Add JS validation for invalid characters  in branch name

Fixes #3293 

Demo:

![out-1080p](/uploads/ba21c359b6b8b440c40cacf772ec0df7/out-1080p.gif)


See merge request !2122

8 files changed, 251 insertions, 9 deletions

diff --git a/app/assets/javascripts/new_branch_form.js.coffee b/app/assets/javascripts/new_branch_form.js.coffee
new file mode 100644
index 00000000000..4b350854f78
--- /dev/null
+++ b/app/assets/javascripts/new_branch_form.js.coffee
@@ -0,0 +1,78 @@
+class @NewBranchForm
+  constructor: (form, availableRefs) ->
+    @branchNameError = form.find('.js-branch-name-error')
+    @name = form.find('.js-branch-name')
+    @ref  = form.find('#ref')
+
+    @setupAvailableRefs(availableRefs)
+    @setupRestrictions()
+    @addBinding()
+    @init()
+
+  addBinding: ->
+    @name.on 'blur', @validate
+
+  init: ->
+    @name.trigger 'blur' if @name.val().length > 0
+
+  setupAvailableRefs: (availableRefs) ->
+    @ref.autocomplete
+      source: availableRefs,
+      minLength: 1
+
+  setupRestrictions: ->
+    startsWith = {
+      pattern: /^(\/|\.)/g,
+      prefix: "can't start with",
+      conjunction: "or"
+    }
+
+    endsWith = {
+      pattern: /(\/|\.|\.lock)$/g,
+      prefix: "can't end in",
+      conjunction: "or"
+    }
+
+    invalid = {
+      pattern: /(\s|~|\^|:|\?|\*|\[|\\|\.\.|@\{|\/{2,}){1}/g
+      prefix: "can't contain",
+      conjunction: ", "
+    }
+
+    single = {
+      pattern: /^@+$/g
+      prefix: "can't be",
+      conjunction: "or"
+    }
+
+    @restrictions = [startsWith, invalid, endsWith, single]
+
+  validate: =>
+    @branchNameError.empty()
+
+    unique = (values, value) ->
+      values.push(value) unless value in values
+      values
+
+    formatter = (values, restriction) ->
+      formatted = values.map (value) ->
+        switch
+          when /\s/.test value then 'spaces'
+          when /\/{2,}/g.test value then 'consecutive slashes'
+          else "'#{value}'"
+
+      "#{restriction.prefix} #{formatted.join(restriction.conjunction)}"
+
+    validator = (errors, restriction) =>
+      matched = @name.val().match(restriction.pattern)
+
+      if matched
+        errors.concat formatter(matched.reduce(unique, []), restriction)
+      else
+        errors
+
+    errors = @restrictions.reduce validator, []
+
+    if errors.length > 0
+      errorMessage = $("<span/>").text(errors.join(', '))
+      @branchNameError.append(errorMessage)
diff --git a/app/services/create_branch_service.rb b/app/services/create_branch_service.rb
index de18f3bc556..a6844985c4e 100644
--- a/app/services/create_branch_service.rb
+++ b/app/services/create_branch_service.rb
@@ -4,7 +4,7 @@ class CreateBranchService < BaseService
   def execute(branch_name, ref)
     valid_branch = Gitlab::GitRefValidator.validate(branch_name)
     if valid_branch == false
-      return error('Branch name invalid')
+      return error('Branch name is invalid')
     end
 
     repository = project.repository
diff --git a/app/views/projects/branches/new.html.haml b/app/views/projects/branches/new.html.haml
index 31943a2407a..c659af6338c 100644
--- a/app/views/projects/branches/new.html.haml
+++ b/app/views/projects/branches/new.html.haml
@@ -9,11 +9,12 @@
   New Branch
 %hr
 
-= form_tag namespace_project_branches_path, method: :post, id: "new-branch-form", class: "form-horizontal js-requires-input" do
+= form_tag namespace_project_branches_path, method: :post, id: "new-branch-form", class: "form-horizontal js-create-branch-form js-requires-input" do
   .form-group
     = label_tag :branch_name, nil, class: 'control-label'
     .col-sm-10
-      = text_field_tag :branch_name, params[:branch_name], required: true, tabindex: 1, autofocus: true, class: 'form-control'
+      = text_field_tag :branch_name, params[:branch_name], required: true, tabindex: 1, autofocus: true, class: 'form-control js-branch-name'
+      .help-block.text-danger.js-branch-name-error
   .form-group
     = label_tag :ref, 'Create from', class: 'control-label'
     .col-sm-10
@@ -26,7 +27,4 @@
 :javascript
   var availableRefs = #{@project.repository.ref_names.to_json};
 
-  $("#ref").autocomplete({
-    source: availableRefs,
-    minLength: 1
-  });
+  new NewBranchForm($('.js-create-branch-form'), availableRefs)
diff --git a/features/project/commits/branches.feature b/features/project/commits/branches.feature
index 5103ca12947..2c17d32154a 100644
--- a/features/project/commits/branches.feature
+++ b/features/project/commits/branches.feature
@@ -25,6 +25,7 @@ Feature: Project Commits Branches
     And I click branch 'improve/awesome' delete link
     Then I should not see branch 'improve/awesome'
 
+  @javascript
   Scenario: I create a branch with invalid name
     Given I visit project branches page
     And I click new branch link
diff --git a/features/steps/project/commits/branches.rb b/features/steps/project/commits/branches.rb
index 338f5e8d3ee..0a42931147d 100644
--- a/features/steps/project/commits/branches.rb
+++ b/features/steps/project/commits/branches.rb
@@ -61,7 +61,8 @@ class Spinach::Features::ProjectCommitsBranches < Spinach::FeatureSteps
   end
 
   step 'I should see new an error that branch is invalid' do
-    expect(page).to have_content 'Branch name invalid'
+    expect(page).to have_content 'Branch name is invalid'
+    expect(page).to have_content "can't contain spaces"
   end
 
   step 'I should see new an error that ref is invalid' do
diff --git a/spec/javascripts/fixtures/new_branch.html.haml b/spec/javascripts/fixtures/new_branch.html.haml
new file mode 100644
index 00000000000..f06629e5ecc
--- /dev/null
+++ b/spec/javascripts/fixtures/new_branch.html.haml
@@ -0,0 +1,4 @@
+%form.js-create-branch-form
+  %input.js-branch-name
+  .js-branch-name-error
+  %input{id: "ref"}
diff --git a/spec/javascripts/new_branch_spec.js.coffee b/spec/javascripts/new_branch_spec.js.coffee
new file mode 100644
index 00000000000..8889ce2e9b3
--- /dev/null
+++ b/spec/javascripts/new_branch_spec.js.coffee
@@ -0,0 +1,160 @@
+#= require jquery.ui.all
+#= require new_branch_form
+
+describe 'Branch', ->
+  describe 'create a new branch', ->
+    fixture.preload('new_branch.html')
+
+    fillNameWith = (value) ->
+      $('.js-branch-name').val(value).trigger('blur')
+
+    expectToHaveError = (error) ->
+      expect($('.js-branch-name-error span').text()).toEqual(error)
+
+    beforeEach ->
+      fixture.load('new_branch.html')
+      $('form').on 'submit', (e) -> e.preventDefault()
+
+      @form = new NewBranchForm($('.js-create-branch-form'), [])
+
+    it "can't start with a dot", ->
+      fillNameWith '.foo'
+      expectToHaveError "can't start with '.'"
+
+    it "can't start with a slash", ->
+      fillNameWith '/foo'
+      expectToHaveError "can't start with '/'"
+
+    it "can't have two consecutive dots", ->
+      fillNameWith 'foo..bar'
+      expectToHaveError "can't contain '..'"
+
+    it "can't have spaces anywhere", ->
+      fillNameWith ' foo'
+      expectToHaveError "can't contain spaces"
+      fillNameWith 'foo bar'
+      expectToHaveError "can't contain spaces"
+      fillNameWith 'foo '
+      expectToHaveError "can't contain spaces"
+
+    it "can't have ~ anywhere", ->
+      fillNameWith '~foo'
+      expectToHaveError "can't contain '~'"
+      fillNameWith 'foo~bar'
+      expectToHaveError "can't contain '~'"
+      fillNameWith 'foo~'
+      expectToHaveError "can't contain '~'"
+
+    it "can't have tilde anwhere", ->
+      fillNameWith '~foo'
+      expectToHaveError "can't contain '~'"
+      fillNameWith 'foo~bar'
+      expectToHaveError "can't contain '~'"
+      fillNameWith 'foo~'
+      expectToHaveError "can't contain '~'"
+
+    it "can't have caret anywhere", ->
+      fillNameWith '^foo'
+      expectToHaveError "can't contain '^'"
+      fillNameWith 'foo^bar'
+      expectToHaveError "can't contain '^'"
+      fillNameWith 'foo^'
+      expectToHaveError "can't contain '^'"
+
+    it "can't have : anywhere", ->
+      fillNameWith ':foo'
+      expectToHaveError "can't contain ':'"
+      fillNameWith 'foo:bar'
+      expectToHaveError "can't contain ':'"
+      fillNameWith ':foo'
+      expectToHaveError "can't contain ':'"
+
+    it "can't have question mark anywhere", ->
+      fillNameWith '?foo'
+      expectToHaveError "can't contain '?'"
+      fillNameWith 'foo?bar'
+      expectToHaveError "can't contain '?'"
+      fillNameWith 'foo?'
+      expectToHaveError "can't contain '?'"
+
+    it "can't have asterisk anywhere", ->
+      fillNameWith '*foo'
+      expectToHaveError "can't contain '*'"
+      fillNameWith 'foo*bar'
+      expectToHaveError "can't contain '*'"
+      fillNameWith 'foo*'
+      expectToHaveError "can't contain '*'"
+
+    it "can't have open bracket anywhere", ->
+      fillNameWith '[foo'
+      expectToHaveError "can't contain '['"
+      fillNameWith 'foo[bar'
+      expectToHaveError "can't contain '['"
+      fillNameWith 'foo['
+      expectToHaveError "can't contain '['"
+
+    it "can't have a backslash anywhere", ->
+      fillNameWith '\\foo'
+      expectToHaveError "can't contain '\\'"
+      fillNameWith 'foo\\bar'
+      expectToHaveError "can't contain '\\'"
+      fillNameWith 'foo\\'
+      expectToHaveError "can't contain '\\'"
+
+    it "can't contain a sequence @{ anywhere", ->
+      fillNameWith '@{foo'
+      expectToHaveError "can't contain '@{'"
+      fillNameWith 'foo@{bar'
+      expectToHaveError "can't contain '@{'"
+      fillNameWith 'foo@{'
+      expectToHaveError "can't contain '@{'"
+
+    it "can't have consecutive slashes", ->
+      fillNameWith 'foo//bar'
+      expectToHaveError "can't contain consecutive slashes"
+
+    it "can't end with a slash", ->
+      fillNameWith 'foo/'
+      expectToHaveError "can't end in '/'"
+
+    it "can't end with a dot", ->
+      fillNameWith 'foo.'
+      expectToHaveError "can't end in '.'"
+
+    it "can't end with .lock", ->
+      fillNameWith 'foo.lock'
+      expectToHaveError "can't end in '.lock'"
+
+    it "can't be the single character @", ->
+      fillNameWith '@'
+      expectToHaveError "can't be '@'"
+
+    it "concatenates all error messages", ->
+      fillNameWith '/foo bar?~.'
+      expectToHaveError "can't start with '/', can't contain spaces, '?', '~', can't end in '.'"
+
+    it "doesn't duplicate error messages", ->
+      fillNameWith '?foo?bar?zoo?'
+      expectToHaveError "can't contain '?'"
+
+    it "removes the error message when is a valid name", ->
+      fillNameWith 'foo?bar'
+      expect($('.js-branch-name-error span').length).toEqual(1)
+      fillNameWith 'foobar'
+      expect($('.js-branch-name-error span').length).toEqual(0)
+
+    it "can have dashes anywhere", ->
+      fillNameWith '-foo-bar-zoo-'
+      expect($('.js-branch-name-error span').length).toEqual(0)
+
+    it "can have underscores anywhere", ->
+      fillNameWith '_foo_bar_zoo_'
+      expect($('.js-branch-name-error span').length).toEqual(0)
+
+    it "can have numbers anywhere", ->
+      fillNameWith '1foo2bar3zoo4'
+      expect($('.js-branch-name-error span').length).toEqual(0)
+
+    it "can be only letters", ->
+      fillNameWith 'foo'
+      expect($('.js-branch-name-error span').length).toEqual(0)
diff --git a/spec/requests/api/branches_spec.rb b/spec/requests/api/branches_spec.rb
index 5c1b58535cc..36461e84c3a 100644
--- a/spec/requests/api/branches_spec.rb
+++ b/spec/requests/api/branches_spec.rb
@@ -118,7 +118,7 @@ describe API::API, api: true  do
            branch_name: 'new design',
            ref: branch_sha
       expect(response.status).to eq(400)
-      expect(json_response['message']).to eq('Branch name invalid')
+      expect(json_response['message']).to eq('Branch name is invalid')
     end
 
     it 'should return 400 if branch already exists' do