Merge branch 'fix-branch-errors-with-encoded-slashes' into 'master'

Fix errors deleting, creating, and viewing graphs using branches with encoded slashes

* Closes #1804
* Closes #1359

See merge request !1084

5 files changed, 37 insertions, 4 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 943e4e8c586..df8e9a48474 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,6 +1,8 @@
 Please view this file on the master branch, on stable branches it's out of date.
 
 v 7.14.0 (unreleased)
+  - Fix "Network" and "Graphs" pages for branches with encoded slashes (Stan Hu)
+  - Fix errors deleting and creating branches with encoded slashes (Stan Hu)
   - Fix multi-line syntax highlighting (Stan Hu)
   - Fix network graph when branch name has single quotes (Stan Hu)
   - Add "Confirm user" button in user admin page (Stan Hu)
diff --git a/app/controllers/projects/branches_controller.rb b/app/controllers/projects/branches_controller.rb
index 117ae3aaa3d..3ac0a75fa70 100644
--- a/app/controllers/projects/branches_controller.rb
+++ b/app/controllers/projects/branches_controller.rb
@@ -17,7 +17,9 @@ class Projects::BranchesController < Projects::ApplicationController
 
   def create
     branch_name = sanitize(strip_tags(params[:branch_name]))
+    branch_name = Addressable::URI.unescape(branch_name)
     ref = sanitize(strip_tags(params[:ref]))
+    ref = Addressable::URI.unescape(ref)
     result = CreateBranchService.new(project, current_user).
         execute(branch_name, ref)
 
@@ -32,9 +34,8 @@ class Projects::BranchesController < Projects::ApplicationController
   end
 
   def destroy
-    status = DeleteBranchService.new(project, current_user).execute(params[:id])
-    @branch_name = params[:id]
-
+    @branch_name = Addressable::URI.unescape(params[:id])
+    status = DeleteBranchService.new(project, current_user).execute(@branch_name)
     respond_to do |format|
       format.html do
         redirect_to namespace_project_branches_path(@project.namespace,
diff --git a/lib/extracts_path.rb b/lib/extracts_path.rb
index 3f420553d42..322aed5e27c 100644
--- a/lib/extracts_path.rb
+++ b/lib/extracts_path.rb
@@ -94,7 +94,7 @@ module ExtractsPath
     @options = params.select {|key, value| allowed_options.include?(key) && !value.blank? }
     @options = HashWithIndifferentAccess.new(@options)
 
-    @id = get_id
+    @id = Addressable::URI.unescape(get_id)
     @ref, @path = extract_ref(@id)
     @repo = @project.repository
     if @options[:extended_sha1].blank?
diff --git a/spec/controllers/branches_controller_spec.rb b/spec/controllers/branches_controller_spec.rb
index bd4c946b64b..8e06d4bdc77 100644
--- a/spec/controllers/branches_controller_spec.rb
+++ b/spec/controllers/branches_controller_spec.rb
@@ -54,6 +54,13 @@ describe Projects::BranchesController do
       let(:ref) { "<script>alert('ref');</script>" }
       it { is_expected.to render_template('new') }
     end
+
+    context "valid branch name with encoded slashes" do
+      let(:branch) { "feature%2Ftest" }
+      let(:ref) { "<script>alert('ref');</script>" }
+      it { is_expected.to render_template('new') }
+      it { project.repository.branch_names.include?('feature/test')}
+    end
   end
 
   describe "POST destroy" do
@@ -74,6 +81,19 @@ describe Projects::BranchesController do
       it { expect(subject).to render_template('destroy') }
     end
 
+    context "valid branch name with unencoded slashes" do
+      let(:branch) { "improve/awesome" }
+
+      it { expect(response.status).to eq(200) }
+      it { expect(subject).to render_template('destroy') }
+    end
+
+    context "valid branch name with encoded slashes" do
+      let(:branch) { "improve%2Fawesome" }
+
+      it { expect(response.status).to eq(200) }
+      it { expect(subject).to render_template('destroy') }
+    end
     context "invalid branch name, valid ref" do
       let(:branch) { "no-branch" }
 
diff --git a/spec/lib/extracts_path_spec.rb b/spec/lib/extracts_path_spec.rb
index 4439775f612..9c115bbfc6a 100644
--- a/spec/lib/extracts_path_spec.rb
+++ b/spec/lib/extracts_path_spec.rb
@@ -29,6 +29,16 @@ describe ExtractsPath do
       assign_ref_vars
       expect(@logs_path).to eq("/#{@project.path_with_namespace}/refs/#{ref}/logs_tree/files/ruby/popen.rb")
     end
+
+    context 'escaped sequences in ref' do
+      let(:ref) { "improve%2Fawesome" }
+
+      it "id should have no escape sequences" do
+        assign_ref_vars
+        expect(@ref).to eq('improve/awesome')
+        expect(@logs_path).to eq("/#{@project.path_with_namespace}/refs/#{ref}/logs_tree/files/ruby/popen.rb")
+      end
+    end
   end
 
   describe '#extract_ref' do