diff options
author | GitLab Release Tools Bot <robert+release-tools@gitlab.com> | 2019-06-26 21:40:58 +0000 |
---|---|---|
committer | GitLab Release Tools Bot <robert+release-tools@gitlab.com> | 2019-06-26 21:40:58 +0000 |
commit | 87c6c8dabc402c4692e426d48d58febd4994be7f (patch) | |
tree | 1f8e30cf458abeeec39d7327c4f44066b6daf826 | |
parent | 3807840db3c78d21fd3b38d62ad30e568970936e (diff) | |
parent | cf8fc36815e9522290ce1f082cd107bd1d5470b2 (diff) | |
download | gitlab-ce-87c6c8dabc402c4692e426d48d58febd4994be7f.tar.gz |
Merge branch 'security-prevent-detection-of-merge-request-template-name-12-0' into '12-0-stable'
Guests can know whether merge request template name exists or not
See merge request gitlab/gitlabhq!3161
-rw-r--r-- | app/controllers/projects/application_controller.rb | 5 | ||||
-rw-r--r-- | app/controllers/projects/templates_controller.rb | 17 | ||||
-rw-r--r-- | changelogs/unreleased/security-prevent-detection-of-merge-request-template-name.yml | 5 | ||||
-rw-r--r-- | config/routes/project.rb | 5 | ||||
-rw-r--r-- | spec/controllers/projects/templates_controller_spec.rb | 110 | ||||
-rw-r--r-- | spec/routing/project_routing_spec.rb | 20 |
6 files changed, 130 insertions, 32 deletions
diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb index 80e4f54bbf4..910bb819df6 100644 --- a/app/controllers/projects/application_controller.rb +++ b/app/controllers/projects/application_controller.rb @@ -12,6 +12,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 a1e769f6ca3..2f97a6f8d33 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -168,7 +168,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: %r{issue|merge_request}, format: 'json' } resources :commit, only: [:show], constraints: { id: /\h{7,40}/ } do member do diff --git a/spec/controllers/projects/templates_controller_spec.rb b/spec/controllers/projects/templates_controller_spec.rb index bebf17728c0..9e7d34b10c0 100644 --- a/spec/controllers/projects/templates_controller_spec.rb +++ b/spec/controllers/projects/templates_controller_spec.rb @@ -3,49 +3,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 83775b1040e..6dde40d1cb6 100644 --- a/spec/routing/project_routing_spec.rb +++ b/spec/routing/project_routing_spec.rb @@ -693,4 +693,24 @@ describe 'project routing' do it_behaves_like 'redirecting a legacy project path', "/gitlab/gitlabhq/settings/repository", "/gitlab/gitlabhq/-/settings/repository" 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 |