From 384a92b7362ad75801add04292f6ef7938207fc4 Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Sun, 9 Dec 2018 21:23:15 -0800
Subject: [PATCH] Check for valid refs in CommitController before doing
 anything

Before a 404 would be rendered only after a request to Gitaly would
return with an InvalidArgument error. Now we check that the ref have a
valid format before sending it to Gitaly. In both cases, a 404 is
returned to the user, but this change prevents Gitaly from generating
error noise in production.

Closes https://gitlab.com/gitlab-org/gitaly/issues/1425
---
 app/controllers/concerns/renders_commits.rb   |  6 ++++
 .../projects/commits_controller.rb            |  5 ++++
 .../projects/compare_controller.rb            |  6 ----
 .../projects/commits_controller_spec.rb       | 30 +++++++++++++++++++
 4 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/app/controllers/concerns/renders_commits.rb b/app/controllers/concerns/renders_commits.rb
index f48e0586211..ed9b898a2a3 100644
--- a/app/controllers/concerns/renders_commits.rb
+++ b/app/controllers/concerns/renders_commits.rb
@@ -26,4 +26,10 @@ module RendersCommits
 
     commits
   end
+
+  def valid_ref?(ref_name)
+    return true unless ref_name.present?
+
+    Gitlab::GitRefValidator.validate(ref_name)
+  end
 end
diff --git a/app/controllers/projects/commits_controller.rb b/app/controllers/projects/commits_controller.rb
index e40a1a1d744..2510a31c9b3 100644
--- a/app/controllers/projects/commits_controller.rb
+++ b/app/controllers/projects/commits_controller.rb
@@ -11,6 +11,7 @@ class Projects::CommitsController < Projects::ApplicationController
   before_action :require_non_empty_project
   before_action :assign_ref_vars, except: :commits_root
   before_action :authorize_download_code!
+  before_action :validate_ref!, except: :commits_root
   before_action :set_commits, except: :commits_root
 
   def commits_root
@@ -54,6 +55,10 @@ class Projects::CommitsController < Projects::ApplicationController
 
   private
 
+  def validate_ref!
+    render_404 unless valid_ref?(@ref)
+  end
+
   def set_commits
     render_404 unless @path.empty? || request.format == :atom || @repository.blob_at(@commit.id, @path) || @repository.tree(@commit.id, @path).entries.present?
     @limit, @offset = (params[:limit] || 40).to_i, (params[:offset] || 0).to_i
diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb
index 2917925947f..5586c2fc631 100644
--- a/app/controllers/projects/compare_controller.rb
+++ b/app/controllers/projects/compare_controller.rb
@@ -65,12 +65,6 @@ class Projects::CompareController < Projects::ApplicationController
 
   private
 
-  def valid_ref?(ref_name)
-    return true unless ref_name.present?
-
-    Gitlab::GitRefValidator.validate(ref_name)
-  end
-
   def validate_refs!
     valid = [head_ref, start_ref].map { |ref| valid_ref?(ref) }
 
diff --git a/spec/controllers/projects/commits_controller_spec.rb b/spec/controllers/projects/commits_controller_spec.rb
index 5c72dab698c..80513650636 100644
--- a/spec/controllers/projects/commits_controller_spec.rb
+++ b/spec/controllers/projects/commits_controller_spec.rb
@@ -53,6 +53,12 @@ describe Projects::CommitsController do
 
           it { is_expected.to respond_with(:not_found) }
         end
+
+        context "branch with invalid format, valid file" do
+          let(:id) { 'branch with space/README.md' }
+
+          it { is_expected.to respond_with(:not_found) }
+        end
       end
 
       context "when the ref name ends in .atom" do
@@ -94,6 +100,30 @@ describe Projects::CommitsController do
         end
       end
     end
+
+    describe "GET /commits/:id/signatures" do
+      render_views
+
+      before do
+        get(:signatures,
+            namespace_id: project.namespace,
+            project_id: project,
+            id: id,
+            format: :json)
+      end
+
+      context "valid branch" do
+        let(:id) { 'master' }
+
+        it { is_expected.to respond_with(:success) }
+      end
+
+      context "invalid branch format" do
+        let(:id) { 'some branch' }
+
+        it { is_expected.to respond_with(:not_found) }
+      end
+    end
   end
 
   context 'token authentication' do