Merge branch 'security-confidential-titles' into 'master'

Prevent exposure of confidential issue titles in file browser

See merge request gitlab-org/security/gitlab!1171

changelogs/unreleased/security-confidential-titles.yml

+---
+title: Prevent exposure of confidential issue titles in file browser
+merge_request:
+author:
+type: security

lib/gitlab/tree_summary.rb

@@ -40,21 +40,17 @@ module Gitlab
     #     - An Array of the unique ::Commit objects in the first value
     def summarize
       summary = contents
-        .map { |content| build_entry(content) }
         .tap { |summary| fill_last_commits!(summary) }
 
       [summary, commits]
     end
 
     def fetch_logs
-      cache_key = ['projects', project.id, 'logs', commit.id, path, offset]
-      Rails.cache.fetch(cache_key, expires_in: CACHE_EXPIRE_IN) do
-        logs, _ = summarize
+      logs, _ = summarize
 
-        new_offset = next_offset if more?
+      new_offset = next_offset if more?
 
-        [logs.as_json, new_offset]
-      end
+      [logs.as_json, new_offset]
     end
 
     # Does the tree contain more entries after the given offset + limit?
@@ -71,7 +67,7 @@ module Gitlab
     private
 
     def contents
-      all_contents[offset, limit]
+      all_contents[offset, limit] || []
     end
 
     def commits
@@ -82,22 +78,17 @@ module Gitlab
       project.repository
     end
 
-    def entry_path(entry)
-      File.join(*[path, entry[:file_name]].compact).force_encoding(Encoding::ASCII_8BIT)
+    # Ensure the path is in "path/" format
+    def ensured_path
+      File.join(*[path, ""]) if path
     end
 
-    def build_entry(entry)
-      { file_name: entry.name, type: entry.type }
+    def entry_path(entry)
+      File.join(*[path, entry[:file_name]].compact).force_encoding(Encoding::ASCII_8BIT)
     end
 
     def fill_last_commits!(entries)
-      # Ensure the path is in "path/" format
-      ensured_path =
-        if path
-          File.join(*[path, ""])
-        end
-
-      commits_hsh = repository.list_last_commits_for_tree(commit.id, ensured_path, offset: offset, limit: limit, literal_pathspec: true)
+      commits_hsh = fetch_last_cached_commits_list
       prerender_commit_full_titles!(commits_hsh.values)
 
       entries.each do |entry|
@@ -112,6 +103,18 @@ module Gitlab
       end
     end
 
+    def fetch_last_cached_commits_list
+      cache_key = ['projects', project.id, 'last_commits_list', commit.id, ensured_path, offset, limit]
+
+      commits = Rails.cache.fetch(cache_key, expires_in: CACHE_EXPIRE_IN) do
+        repository
+          .list_last_commits_for_tree(commit.id, ensured_path, offset: offset, limit: limit, literal_pathspec: true)
+          .transform_values!(&:to_hash)
+      end
+
+      commits.transform_values! { |value| Commit.from_hash(value, project) }
+    end
+
     def cache_commit(commit)
       return unless commit.present?
 
@@ -123,12 +126,18 @@ module Gitlab
     end
 
     def all_contents
-      strong_memoize(:all_contents) do
+      strong_memoize(:all_contents) { cached_contents }
+    end
+
+    def cached_contents
+      cache_key = ['projects', project.id, 'content', commit.id, path]
+
+      Rails.cache.fetch(cache_key, expires_in: CACHE_EXPIRE_IN) do
         [
           *tree.trees,
           *tree.blobs,
           *tree.submodules
-        ]
+        ].map { |entry| { file_name: entry.name, type: entry.type } }
       end
     end
 

spec/controllers/projects/refs_controller_spec.rb

@@ -56,18 +56,6 @@ RSpec.describe Projects::RefsController do
         expect(response).to be_successful
         expect(json_response).to be_kind_of(Array)
       end
-
-      it 'caches tree summary data', :use_clean_rails_memory_store_caching do
-        expect_next_instance_of(::Gitlab::TreeSummary) do |instance|
-          expect(instance).to receive_messages(summarize: ['logs'], next_offset: 50, more?: true)
-        end
-
-        xhr_get(:json, offset: 25)
-
-        cache_key = "projects/#{project.id}/logs/#{project.commit.id}/#{path}/25"
-        expect(Rails.cache.fetch(cache_key)).to eq(['logs', 50])
-        expect(response.headers['More-Logs-Offset']).to eq("50")
-      end
     end
   end
 end

spec/lib/gitlab/tree_summary_spec.rb

@@ -3,6 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::TreeSummary do
+  include RepoHelpers
   using RSpec::Parameterized::TableSyntax
 
   let(:project) { create(:project, :empty_repo) }
@@ -44,6 +45,40 @@ RSpec.describe Gitlab::TreeSummary do
         expect(commits).to match_array(entries.map { |entry| entry[:commit] })
       end
     end
+
+    context 'when offset is over the limit' do
+      let(:offset) { 100 }
+
+      it 'returns an empty array' do
+        expect(summarized).to eq([[], []])
+      end
+    end
+
+    context 'with caching', :use_clean_rails_memory_store_caching do
+      subject { Rails.cache.fetch(key) }
+
+      before do
+        summarized
+      end
+
+      context 'Repository tree cache' do
+        let(:key) { ['projects', project.id, 'content', commit.id, path] }
+
+        it 'creates a cache for repository content' do
+          is_expected.to eq([{ file_name: 'a.txt', type: :blob }])
+        end
+      end
+
+      context 'Commits list cache' do
+        let(:offset) { 0 }
+        let(:limit) { 25 }
+        let(:key) { ['projects', project.id, 'last_commits_list', commit.id, path, offset, limit] }
+
+        it 'creates a cache for commits list' do
+          is_expected.to eq('a.txt' => commit.to_hash)
+        end
+      end
+    end
   end
 
   describe '#summarize (entries)' do
@@ -167,6 +202,46 @@ RSpec.describe Gitlab::TreeSummary do
     end
   end
 
+  describe 'References in commit messages' do
+    let_it_be(:project) { create(:project, :empty_repo) }
+    let_it_be(:issue) { create(:issue, project: project) }
+    let(:entries) { summary.summarize.first }
+    let(:entry) { entries.find { |entry| entry[:file_name] == 'issue.txt' } }
+
+    before_all do
+      create_file_in_repo(project, 'master', 'master', 'issue.txt', '', commit_message: "Issue ##{issue.iid}")
+    end
+
+    where(:project_visibility, :user_role, :issue_confidential, :expected_result) do
+      'private'  | :guest    | false | true
+      'private'  | :guest    | true  | false
+      'private'  | :reporter | false | true
+      'private'  | :reporter | true  | true
+
+      'internal' | :guest    | false | true
+      'internal' | :guest    | true  | false
+      'internal' | :reporter | false | true
+      'internal' | :reporter | true  | true
+
+      'public'   | :guest    | false | true
+      'public'   | :guest    | true  | false
+      'public'   | :reporter | false | true
+      'public'   | :reporter | true  | true
+    end
+
+    with_them do
+      subject { entry[:commit_title_html].include?("title=\"#{issue.title}\"") }
+
+      before do
+        project.add_role(user, user_role)
+        project.update!(visibility_level: Gitlab::VisibilityLevel.level_value(project_visibility))
+        issue.update!(confidential: issue_confidential)
+      end
+
+      it { is_expected.to eq(expected_result) }
+    end
+  end
+
   describe '#more?' do
     let(:path) { 'tmp/more' }