diff options
| -rw-r--r-- | changelogs/unreleased/security-confidential-titles.yml | 5 | ||||
| -rw-r--r-- | lib/gitlab/tree_summary.rb | 51 | ||||
| -rw-r--r-- | spec/controllers/projects/refs_controller_spec.rb | 12 | ||||
| -rw-r--r-- | spec/lib/gitlab/tree_summary_spec.rb | 75 |
4 files changed, 110 insertions, 33 deletions
diff --git a/changelogs/unreleased/security-confidential-titles.yml b/changelogs/unreleased/security-confidential-titles.yml new file mode 100644 index 00000000000..506cbc095c4 --- /dev/null +++ b/changelogs/unreleased/security-confidential-titles.yml @@ -0,0 +1,5 @@ +--- +title: Prevent exposure of confidential issue titles in file browser +merge_request: +author: +type: security diff --git a/lib/gitlab/tree_summary.rb b/lib/gitlab/tree_summary.rb index 9b67599668a..bc7b8bd2b94 100644 --- a/lib/gitlab/tree_summary.rb +++ b/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 diff --git a/spec/controllers/projects/refs_controller_spec.rb b/spec/controllers/projects/refs_controller_spec.rb index d10351feb9e..b625ce35d61 100644 --- a/spec/controllers/projects/refs_controller_spec.rb +++ b/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 diff --git a/spec/lib/gitlab/tree_summary_spec.rb b/spec/lib/gitlab/tree_summary_spec.rb index 303a4a80581..d2c5844b0fa 100644 --- a/spec/lib/gitlab/tree_summary_spec.rb +++ b/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' } |