Validate refs used in controllers don't have spacessh-validate-ref-name-in-commit

This avoids an unnecessary call to Gitaly and reduces gRPC errors.

* Closes https://gitlab.com/gitlab-org/gitaly/issues/1425
* Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/58572

4 files changed, 58 insertions, 0 deletions

diff --git a/changelogs/unreleased/sh-validate-ref-name-in-commit.yml b/changelogs/unreleased/sh-validate-ref-name-in-commit.yml
new file mode 100644
index 00000000000..399529556bc
--- /dev/null
+++ b/changelogs/unreleased/sh-validate-ref-name-in-commit.yml
@@ -0,0 +1,5 @@
+---
+title: Validate refs used in controllers don't have spaces
+merge_request: 24037
+author:
+type: other
diff --git a/lib/extracts_path.rb b/lib/extracts_path.rb
index b2c8d46ede1..44a9c7ea536 100644
--- a/lib/extracts_path.rb
+++ b/lib/extracts_path.rb
@@ -113,6 +113,9 @@ module ExtractsPath
     @id = get_id
     @ref, @path = extract_ref(@id)
     @repo = @project.repository
+    @ref.strip!
+
+    raise InvalidPathError if @ref.match?(/\s/)
 
     @commit = @repo.commit(@ref)
 
diff --git a/spec/controllers/projects/tree_controller_spec.rb b/spec/controllers/projects/tree_controller_spec.rb
index 78201498eaa..6d9a0be6f2e 100644
--- a/spec/controllers/projects/tree_controller_spec.rb
+++ b/spec/controllers/projects/tree_controller_spec.rb
@@ -72,6 +72,26 @@ describe Projects::TreeController do
     end
   end
 
+  describe 'GET show with whitespace in ref' do
+    render_views
+
+    let(:id) { "this ref/api/responses" }
+
+    it 'does not call make a Gitaly request' do
+      allow(::Gitlab::GitalyClient).to receive(:call).and_call_original
+      expect(::Gitlab::GitalyClient).not_to receive(:call).with(anything, :commit_service, :find_commit, anything, anything)
+
+      get(:show,
+          params: {
+            namespace_id: project.namespace.to_param,
+            project_id: project,
+            id: id
+          })
+
+      expect(response).to have_gitlab_http_status(:not_found)
+    end
+  end
+
   describe 'GET show with blob path' do
     render_views
 
diff --git a/spec/lib/extracts_path_spec.rb b/spec/lib/extracts_path_spec.rb
index e0691aba600..21ba72953fb 100644
--- a/spec/lib/extracts_path_spec.rb
+++ b/spec/lib/extracts_path_spec.rb
@@ -44,6 +44,36 @@ describe ExtractsPath do
       end
     end
 
+    context 'ref contains trailing space' do
+      let(:ref) { 'master ' }
+
+      it 'strips surrounding space' do
+        assign_ref_vars
+
+        expect(@ref).to eq('master')
+      end
+    end
+
+    context 'ref contains leading space' do
+      let(:ref) { ' master ' }
+
+      it 'strips surrounding space' do
+        assign_ref_vars
+
+        expect(@ref).to eq('master')
+      end
+    end
+
+    context 'ref contains space in the middle' do
+      let(:ref) { 'master plan ' }
+
+      it 'returns 404' do
+        expect(self).to receive(:render_404)
+
+        assign_ref_vars
+      end
+    end
+
     context 'path contains space' do
       let(:params) { { path: 'with space', ref: '38008cb17ce1466d8fec2dfa6f6ab8dcfe5cf49e' } }