Commit 384a92b7 authored by Stan Hu's avatar Stan Hu

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
parent 7cb0dd98
...@@ -26,4 +26,10 @@ module RendersCommits ...@@ -26,4 +26,10 @@ module RendersCommits
commits commits
end end
def valid_ref?(ref_name)
return true unless ref_name.present?
Gitlab::GitRefValidator.validate(ref_name)
end
end end
...@@ -11,6 +11,7 @@ class Projects::CommitsController < Projects::ApplicationController ...@@ -11,6 +11,7 @@ class Projects::CommitsController < Projects::ApplicationController
before_action :require_non_empty_project before_action :require_non_empty_project
before_action :assign_ref_vars, except: :commits_root before_action :assign_ref_vars, except: :commits_root
before_action :authorize_download_code! before_action :authorize_download_code!
before_action :validate_ref!, except: :commits_root
before_action :set_commits, except: :commits_root before_action :set_commits, except: :commits_root
def commits_root def commits_root
...@@ -54,6 +55,10 @@ class Projects::CommitsController < Projects::ApplicationController ...@@ -54,6 +55,10 @@ class Projects::CommitsController < Projects::ApplicationController
private private
def validate_ref!
render_404 unless valid_ref?(@ref)
end
def set_commits def set_commits
render_404 unless @path.empty? || request.format == :atom || @repository.blob_at(@commit.id, @path) || @repository.tree(@commit.id, @path).entries.present? 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 @limit, @offset = (params[:limit] || 40).to_i, (params[:offset] || 0).to_i
......
...@@ -65,12 +65,6 @@ class Projects::CompareController < Projects::ApplicationController ...@@ -65,12 +65,6 @@ class Projects::CompareController < Projects::ApplicationController
private private
def valid_ref?(ref_name)
return true unless ref_name.present?
Gitlab::GitRefValidator.validate(ref_name)
end
def validate_refs! def validate_refs!
valid = [head_ref, start_ref].map { |ref| valid_ref?(ref) } valid = [head_ref, start_ref].map { |ref| valid_ref?(ref) }
......
...@@ -53,6 +53,12 @@ describe Projects::CommitsController do ...@@ -53,6 +53,12 @@ describe Projects::CommitsController do
it { is_expected.to respond_with(:not_found) } it { is_expected.to respond_with(:not_found) }
end 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 end
context "when the ref name ends in .atom" do context "when the ref name ends in .atom" do
...@@ -94,6 +100,30 @@ describe Projects::CommitsController do ...@@ -94,6 +100,30 @@ describe Projects::CommitsController do
end end
end 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 end
context 'token authentication' do context 'token authentication' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment