From cf8fc36815e9522290ce1f082cd107bd1d5470b2 Mon Sep 17 00:00:00 2001
From: Luke Duncalfe <lduncalfe@eml.cc>
Date: Thu, 23 May 2019 16:33:11 +1200
Subject: 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
---
 app/controllers/projects/application_controller.rb |  5 +++++
 app/controllers/projects/templates_controller.rb   | 17 +++++++++++++++--
 2 files changed, 20 insertions(+), 2 deletions(-)

(limited to 'app')

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
-- 
cgit v1.2.1