Commit 136db4dd authored by Luke Duncalfe's avatar Luke Duncalfe

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
parent e3eeb779
...@@ -13,6 +13,11 @@ class Projects::ApplicationController < ApplicationController ...@@ -13,6 +13,11 @@ class Projects::ApplicationController < ApplicationController
helper_method :repository, :can_collaborate_with_project?, :user_access 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 private
def project def project
......
# frozen_string_literal: true # frozen_string_literal: true
class Projects::TemplatesController < Projects::ApplicationController 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 def show
template = @template_type.find(params[:key], project) template = @template_type.find(params[:key], project)
...@@ -13,9 +15,20 @@ class Projects::TemplatesController < Projects::ApplicationController ...@@ -13,9 +15,20 @@ class Projects::TemplatesController < Projects::ApplicationController
private 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 def get_template_class
template_types = { issue: Gitlab::Template::IssueTemplate, merge_request: Gitlab::Template::MergeRequestTemplate }.with_indifferent_access template_types = { issue: Gitlab::Template::IssueTemplate, merge_request: Gitlab::Template::MergeRequestTemplate }.with_indifferent_access
@template_type = template_types[params[:template_type]] @template_type = template_types[params[:template_type]]
render json: [], status: :not_found unless @template_type
end end
end end
---
title: Prevent the detection of merge request templates by unauthorized users
merge_request:
author:
type: security
...@@ -41,7 +41,10 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do ...@@ -41,7 +41,10 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
# #
# Templates # 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' }
resource :avatar, only: [:show, :destroy] resource :avatar, only: [:show, :destroy]
resources :commit, only: [:show], constraints: { id: /\h{7,40}/ } do resources :commit, only: [:show], constraints: { id: /\h{7,40}/ } do
......
...@@ -3,49 +3,101 @@ ...@@ -3,49 +3,101 @@
require 'spec_helper' require 'spec_helper'
describe Projects::TemplatesController do describe Projects::TemplatesController do
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository, :private) }
let(:user) { create(:user) } let(:user) { create(:user) }
let(:user2) { create(:user) } let(:file_path_1) { '.gitlab/issue_templates/issue_template.md' }
let(:file_path_1) { '.gitlab/issue_templates/bug.md' } let(:file_path_2) { '.gitlab/merge_request_templates/merge_request_template.md' }
let(:body) { JSON.parse(response.body) } 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 describe '#show' do
project.add_developer(user) shared_examples 'renders issue templates as json' do
sign_in(user) it do
end get(:show, params: { namespace_id: project.namespace, template_type: 'issue', key: 'issue_template', project_id: project }, format: :json)
before do expect(response.status).to eq(200)
project.add_user(user, Gitlab::Access::MAINTAINER) expect(body['name']).to eq('issue_template')
project.repository.create_file(user, file_path_1, 'something valid', expect(body['content']).to eq('issue content')
message: 'test 3', branch_name: 'master') end
end end
describe '#show' do shared_examples 'renders merge request templates as json' do
it 'renders template name and content as json' do it do
get(:show, params: { namespace_id: project.namespace.to_param, template_type: "issue", key: "bug", project_id: project }, format: :json) 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(response.status).to eq(404)
expect(body["name"]).to eq("bug") end
expect(body["content"]).to eq("something valid")
end end
it 'renders 404 when unauthorized' do shared_examples 'renders 404 when params are invalid' do
sign_in(user2) it 'does not route when the template type is invalid' do
get(:show, params: { namespace_id: project.namespace.to_param, template_type: "issue", key: "bug", project_id: project }, format: :json) 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 end
it 'renders 404 when template type is not found' do context 'when the user is not a member of the project' do
sign_in(user) before do
get(:show, params: { namespace_id: project.namespace.to_param, template_type: "dont_exist", key: "bug", project_id: project }, format: :json) 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 end
it 'renders 404 without errors' do context 'when user is a member of the project' do
sign_in(user) before do
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 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 end
end end
...@@ -661,4 +661,24 @@ describe 'project routing' do ...@@ -661,4 +661,24 @@ describe 'project routing' do
end end
end 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 end
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