Merge branch 'security-prevent-detection-of-merge-request-template-name' into 'master'

Guests can know whether merge request template name exists or not

See merge request gitlab/gitlabhq!3117

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

app/controllers/projects/templates_controller.rb

 # 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

changelogs/unreleased/security-prevent-detection-of-merge-request-template-name.yml

+---
+title: Prevent the detection of merge request templates by unauthorized users
+merge_request:
+author:
+type: security

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

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

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