summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorLuke Duncalfe <lduncalfe@eml.cc>2019-05-23 16:33:11 +1200
committerLuke Duncalfe <lduncalfe@eml.cc>2019-06-13 15:35:04 +1200
commit5351ebf83b4769bdd876aed0898b4202ebff6e91 (patch)
tree6fccffc0797786d780dc009b8712874ad624eb34
parent3c240b7aea7fee1c4267d0ceb717ba0234e5e788 (diff)
downloadgitlab-ce-5351ebf83b4769bdd876aed0898b4202ebff6e91.tar.gz
Authorize access before serving project template
Previously, if a user was a guest member of a private project, they could access the merge request template as we were not checking permission-levels of the user. When a issue template is asked for, the user must have :read_issue for the project; or :read_merge_request when a merge request template is asked for. We also now rescue_from FileNotFoundError and handle as 404. This is because RepoTemplateFinder can raise a FileNotFoundError exception, which Rails previously handled as a 500. Handling these in a way that is consistent with ActiveRecord::RecordNotFound exceptions, within controllers that inherit from Projects::ApplicationController at least, and returning a 404. https://gitlab.com/gitlab-org/gitlab-ce/issues/54943
-rw-r--r--app/controllers/projects/application_controller.rb5
-rw-r--r--app/controllers/projects/templates_controller.rb17
-rw-r--r--changelogs/unreleased/security-prevent-detection-of-merge-request-template-name.yml5
-rw-r--r--config/routes/project.rb5
-rw-r--r--spec/controllers/projects/templates_controller_spec.rb110
-rw-r--r--spec/routing/project_routing_spec.rb20
6 files changed, 130 insertions, 32 deletions
diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb
index 781eac7f080..9ffde6afdb7 100644
--- a/app/controllers/projects/application_controller.rb
+++ b/app/controllers/projects/application_controller.rb
@@ -13,6 +13,11 @@ class Projects::ApplicationController < ApplicationController
helper_method :repository, :can_collaborate_with_project?, :user_access
+ rescue_from Gitlab::Template::Finders::RepoTemplateFinder::FileNotFoundError do |exception|
+ log_exception(exception)
+ render_404
+ end
+
private
def project
diff --git a/app/controllers/projects/templates_controller.rb b/app/controllers/projects/templates_controller.rb
index 7ceea4e5b96..f987033a26c 100644
--- a/app/controllers/projects/templates_controller.rb
+++ b/app/controllers/projects/templates_controller.rb
@@ -1,7 +1,9 @@
# frozen_string_literal: true
class Projects::TemplatesController < Projects::ApplicationController
- before_action :authenticate_user!, :get_template_class
+ before_action :authenticate_user!
+ before_action :authorize_can_read_issuable!
+ before_action :get_template_class
def show
template = @template_type.find(params[:key], project)
@@ -13,9 +15,20 @@ class Projects::TemplatesController < Projects::ApplicationController
private
+ # User must have:
+ # - `read_merge_request` to see merge request templates, or
+ # - `read_issue` to see issue templates
+ #
+ # Note params[:template_type] has a route constraint to limit it to
+ # `merge_request` or `issue`
+ def authorize_can_read_issuable!
+ action = [:read_, params[:template_type]].join
+
+ authorize_action!(action)
+ end
+
def get_template_class
template_types = { issue: Gitlab::Template::IssueTemplate, merge_request: Gitlab::Template::MergeRequestTemplate }.with_indifferent_access
@template_type = template_types[params[:template_type]]
- render json: [], status: :not_found unless @template_type
end
end
diff --git a/changelogs/unreleased/security-prevent-detection-of-merge-request-template-name.yml b/changelogs/unreleased/security-prevent-detection-of-merge-request-template-name.yml
new file mode 100644
index 00000000000..d7bb884cb4b
--- /dev/null
+++ b/changelogs/unreleased/security-prevent-detection-of-merge-request-template-name.yml
@@ -0,0 +1,5 @@
+---
+title: Prevent the detection of merge request templates by unauthorized users
+merge_request:
+author:
+type: security
diff --git a/config/routes/project.rb b/config/routes/project.rb
index 93d168fc595..bde482f0b6b 100644
--- a/config/routes/project.rb
+++ b/config/routes/project.rb
@@ -41,7 +41,10 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
#
# Templates
#
- get '/templates/:template_type/:key' => 'templates#show', as: :template, constraints: { key: %r{[^/]+} }
+ get '/templates/:template_type/:key' => 'templates#show',
+ as: :template,
+ defaults: { format: 'json' },
+ constraints: { key: %r{[^/]+}, template_type: /issue|merge_request/, format: 'json' }
resource :avatar, only: [:show, :destroy]
resources :commit, only: [:show], constraints: { id: /\h{7,40}/ } do
diff --git a/spec/controllers/projects/templates_controller_spec.rb b/spec/controllers/projects/templates_controller_spec.rb
index 01e53669627..a7352b252eb 100644
--- a/spec/controllers/projects/templates_controller_spec.rb
+++ b/spec/controllers/projects/templates_controller_spec.rb
@@ -1,49 +1,101 @@
require 'spec_helper'
describe Projects::TemplatesController do
- let(:project) { create(:project, :repository) }
+ let(:project) { create(:project, :repository, :private) }
let(:user) { create(:user) }
- let(:user2) { create(:user) }
- let(:file_path_1) { '.gitlab/issue_templates/bug.md' }
+ let(:file_path_1) { '.gitlab/issue_templates/issue_template.md' }
+ let(:file_path_2) { '.gitlab/merge_request_templates/merge_request_template.md' }
let(:body) { JSON.parse(response.body) }
+ let!(:file_1) { project.repository.create_file(user, file_path_1, 'issue content', message: 'message', branch_name: 'master') }
+ let!(:file_2) { project.repository.create_file(user, file_path_2, 'merge request content', message: 'message', branch_name: 'master') }
- before do
- project.add_developer(user)
- sign_in(user)
- end
+ describe '#show' do
+ shared_examples 'renders issue templates as json' do
+ it do
+ get(:show, params: { namespace_id: project.namespace, template_type: 'issue', key: 'issue_template', project_id: project }, format: :json)
- before do
- project.add_user(user, Gitlab::Access::MAINTAINER)
- project.repository.create_file(user, file_path_1, 'something valid',
- message: 'test 3', branch_name: 'master')
- end
+ expect(response.status).to eq(200)
+ expect(body['name']).to eq('issue_template')
+ expect(body['content']).to eq('issue content')
+ end
+ end
- describe '#show' do
- it 'renders template name and content as json' do
- get(:show, params: { namespace_id: project.namespace.to_param, template_type: "issue", key: "bug", project_id: project }, format: :json)
+ shared_examples 'renders merge request templates as json' do
+ it do
+ get(:show, params: { namespace_id: project.namespace, template_type: 'merge_request', key: 'merge_request_template', project_id: project }, format: :json)
+
+ expect(response.status).to eq(200)
+ expect(body['name']).to eq('merge_request_template')
+ expect(body['content']).to eq('merge request content')
+ end
+ end
+
+ shared_examples 'renders 404 when requesting an issue template' do
+ it do
+ get(:show, params: { namespace_id: project.namespace, template_type: 'issue', key: 'issue_template', project_id: project }, format: :json)
+
+ expect(response.status).to eq(404)
+ end
+ end
+
+ shared_examples 'renders 404 when requesting a merge request template' do
+ it do
+ get(:show, params: { namespace_id: project.namespace, template_type: 'merge_request', key: 'merge_request_template', project_id: project }, format: :json)
- expect(response.status).to eq(200)
- expect(body["name"]).to eq("bug")
- expect(body["content"]).to eq("something valid")
+ expect(response.status).to eq(404)
+ end
end
- it 'renders 404 when unauthorized' do
- sign_in(user2)
- get(:show, params: { namespace_id: project.namespace.to_param, template_type: "issue", key: "bug", project_id: project }, format: :json)
+ shared_examples 'renders 404 when params are invalid' do
+ it 'does not route when the template type is invalid' do
+ expect do
+ get(:show, params: { namespace_id: project.namespace, template_type: 'invalid_type', key: 'issue_template', project_id: project }, format: :json)
+ end.to raise_error(ActionController::UrlGenerationError)
+ end
+
+ it 'renders 404 when the format type is invalid' do
+ get(:show, params: { namespace_id: project.namespace, template_type: 'issue', key: 'issue_template', project_id: project }, format: :html)
+
+ expect(response.status).to eq(404)
+ end
+
+ it 'renders 404 when the key is unknown' do
+ get(:show, params: { namespace_id: project.namespace, template_type: 'issue', key: 'unknown_template', project_id: project }, format: :json)
- expect(response.status).to eq(404)
+ expect(response.status).to eq(404)
+ end
end
- it 'renders 404 when template type is not found' do
- sign_in(user)
- get(:show, params: { namespace_id: project.namespace.to_param, template_type: "dont_exist", key: "bug", project_id: project }, format: :json)
+ context 'when the user is not a member of the project' do
+ before do
+ sign_in(user)
+ end
- expect(response.status).to eq(404)
+ include_examples 'renders 404 when requesting an issue template'
+ include_examples 'renders 404 when requesting a merge request template'
+ include_examples 'renders 404 when params are invalid'
end
- it 'renders 404 without errors' do
- sign_in(user)
- expect { get(:show, params: { namespace_id: project.namespace.to_param, template_type: "dont_exist", key: "bug", project_id: project }, format: :json) }.not_to raise_error
+ context 'when user is a member of the project' do
+ before do
+ project.add_developer(user)
+ sign_in(user)
+ end
+
+ include_examples 'renders issue templates as json'
+ include_examples 'renders merge request templates as json'
+ include_examples 'renders 404 when params are invalid'
+ end
+
+ context 'when user is a guest of the project' do
+ before do
+ project.add_guest(user)
+ sign_in(user)
+ end
+
+ include_examples 'renders issue templates as json'
+ include_examples 'renders 404 when requesting a merge request template'
+ include_examples 'renders 404 when params are invalid'
end
end
end
diff --git a/spec/routing/project_routing_spec.rb b/spec/routing/project_routing_spec.rb
index a0d01fc8263..e4c21bc3077 100644
--- a/spec/routing/project_routing_spec.rb
+++ b/spec/routing/project_routing_spec.rb
@@ -661,4 +661,24 @@ describe 'project routing' do
end
end
end
+
+ describe Projects::TemplatesController, 'routing' do
+ describe '#show' do
+ def show_with_template_type(template_type)
+ "/gitlab/gitlabhq/templates/#{template_type}/template_name"
+ end
+
+ it 'routes when :template_type is `merge_request`' do
+ expect(get(show_with_template_type('merge_request'))).to route_to('projects/templates#show', namespace_id: 'gitlab', project_id: 'gitlabhq', template_type: 'merge_request', key: 'template_name', format: 'json')
+ end
+
+ it 'routes when :template_type is `issue`' do
+ expect(get(show_with_template_type('issue'))).to route_to('projects/templates#show', namespace_id: 'gitlab', project_id: 'gitlabhq', template_type: 'issue', key: 'template_name', format: 'json')
+ end
+
+ it 'routes to application#route_not_found when :template_type is unknown' do
+ expect(get(show_with_template_type('invalid'))).to route_to('application#route_not_found', unmatched_route: 'gitlab/gitlabhq/templates/invalid/template_name')
+ end
+ end
+ end
end