Merge branch 'security-bvl-exposure-in-commits-list' into 'master'

[master] Don't expose confidential information in commit message list

See merge request gitlab/gitlabhq!2626

3 files changed, 67 insertions, 55 deletions

diff --git a/app/views/projects/commits/_commit.html.haml b/app/views/projects/commits/_commit.html.haml
index c6789e32dbe..1a74b120c26 100644
--- a/app/views/projects/commits/_commit.html.haml
+++ b/app/views/projects/commits/_commit.html.haml
@@ -8,62 +8,50 @@
 - ref           = local_assigns.fetch(:ref) { merge_request&.source_branch }
 
 - link = commit_path(project, commit, merge_request: merge_request)
-- cache_key = [project.full_path,
-               ref,
-               commit.id,
-               Gitlab::CurrentSettings.current_application_settings,
-               @path.presence,
-               current_controller?(:commits),
-               merge_request&.iid,
-               view_details,
-               commit.status(ref),
-               I18n.locale].compact
-
-= cache(cache_key, expires_in: 1.day) do
-  %li.commit.flex-row.js-toggle-container{ id: "commit-#{commit.short_id}" }
-
-    .avatar-cell.d-none.d-sm-block
-      = author_avatar(commit, size: 36, has_tooltip: false)
-
-    .commit-detail.flex-list
-      .commit-content.qa-commit-content
-        - if view_details && merge_request
-          = link_to commit.title, project_commit_path(project, commit.id, merge_request_iid: merge_request.iid), class: "commit-row-message item-title"
-        - else
-          = link_to_markdown_field(commit, :title, link, class: "commit-row-message item-title")
-        %span.commit-row-message.d-block.d-sm-none
-          ·
-          = commit.short_id
-        - if commit.status(ref)
-          .d-block.d-sm-none
-            = render_commit_status(commit, ref: ref)
-        - if commit.description?
-          %button.text-expander.js-toggle-button
-            = sprite_icon('ellipsis_h', size: 12)
+%li.commit.flex-row.js-toggle-container{ id: "commit-#{commit.short_id}" }
+
+  .avatar-cell.d-none.d-sm-block
+    = author_avatar(commit, size: 36, has_tooltip: false)
+
+  .commit-detail.flex-list
+    .commit-content.qa-commit-content
+      - if view_details && merge_request
+        = link_to commit.title, project_commit_path(project, commit.id, merge_request_iid: merge_request.iid), class: "commit-row-message item-title"
+      - else
+        = link_to_markdown_field(commit, :title, link, class: "commit-row-message item-title")
+      %span.commit-row-message.d-block.d-sm-none
+        ·
+        = commit.short_id
+      - if commit.status(ref)
+        .d-block.d-sm-none
+          = render_commit_status(commit, ref: ref)
+      - if commit.description?
+        %button.text-expander.js-toggle-button
+          = sprite_icon('ellipsis_h', size: 12)
 
-        .committer
-          - commit_author_link = commit_author_link(commit, avatar: false, size: 24)
-          - commit_timeago = time_ago_with_tooltip(commit.authored_date, placement: 'bottom')
-          - commit_text =  _('%{commit_author_link} authored %{commit_timeago}') % { commit_author_link: commit_author_link, commit_timeago: commit_timeago }
-          #{ commit_text.html_safe }
+      .committer
+        - commit_author_link = commit_author_link(commit, avatar: false, size: 24)
+        - commit_timeago = time_ago_with_tooltip(commit.authored_date, placement: 'bottom')
+        - commit_text =  _('%{commit_author_link} authored %{commit_timeago}') % { commit_author_link: commit_author_link, commit_timeago: commit_timeago }
+        #{ commit_text.html_safe }
 
-        - if commit.description?
-          %pre.commit-row-description.js-toggle-content.append-bottom-8
-            = preserve(markdown_field(commit, :description))
+      - if commit.description?
+        %pre.commit-row-description.js-toggle-content.append-bottom-8
+          = preserve(markdown_field(commit, :description))
 
-      .commit-actions.flex-row.d-none.d-sm-flex
-        - if request.xhr?
-          = render partial: 'projects/commit/signature', object: commit.signature
-        - else
-          = render partial: 'projects/commit/ajax_signature', locals: { commit: commit }
+    .commit-actions.flex-row.d-none.d-sm-flex
+      - if request.xhr?
+        = render partial: 'projects/commit/signature', object: commit.signature
+      - else
+        = render partial: 'projects/commit/ajax_signature', locals: { commit: commit }
 
-        - if commit.status(ref)
-          = render_commit_status(commit, ref: ref)
+      - if commit.status(ref)
+        = render_commit_status(commit, ref: ref)
 
-        .js-commit-pipeline-status{ data: { endpoint: pipelines_project_commit_path(project, commit.id, ref: ref) } }
+      .js-commit-pipeline-status{ data: { endpoint: pipelines_project_commit_path(project, commit.id, ref: ref) } }
 
-        .commit-sha-group
-          .label.label-monospace
-            = commit.short_id
-          = clipboard_button(text: commit.id, title: _("Copy commit SHA to clipboard"), class: "btn btn-default", container: "body")
-          = link_to_browse_code(project, commit)
+      .commit-sha-group
+        .label.label-monospace
+          = commit.short_id
+        = clipboard_button(text: commit.id, title: _("Copy commit SHA to clipboard"), class: "btn btn-default", container: "body")
+        = link_to_browse_code(project, commit)
diff --git a/changelogs/unreleased/security-bvl-exposure-in-commits-list.yml b/changelogs/unreleased/security-bvl-exposure-in-commits-list.yml
new file mode 100644
index 00000000000..0361fb0c041
--- /dev/null
+++ b/changelogs/unreleased/security-bvl-exposure-in-commits-list.yml
@@ -0,0 +1,5 @@
+---
+title: Don't expose confidential information in commit message list
+merge_request:
+author:
+type: security
diff --git a/spec/features/projects/commits/user_browses_commits_spec.rb b/spec/features/projects/commits/user_browses_commits_spec.rb
index 534cfe1eb12..2159adf49fc 100644
--- a/spec/features/projects/commits/user_browses_commits_spec.rb
+++ b/spec/features/projects/commits/user_browses_commits_spec.rb
@@ -4,10 +4,9 @@ describe 'User browses commits' do
   include RepoHelpers
 
   let(:user) { create(:user) }
-  let(:project) { create(:project, :repository, namespace: user.namespace) }
+  let(:project) { create(:project, :public, :repository, namespace: user.namespace) }
 
   before do
-    project.add_maintainer(user)
     sign_in(user)
   end
 
@@ -127,6 +126,26 @@ describe 'User browses commits' do
         .and have_selector('entry summary', text: commit.description[0..10].delete("\r\n"))
     end
 
+    context 'when a commit links to a confidential issue' do
+      let(:confidential_issue) { create(:issue, confidential: true, title: 'Secret issue!', project: project) }
+
+      before do
+        project.repository.create_file(user, 'dummy-file', 'dummy content',
+                                       branch_name: 'feature',
+                                       message: "Linking #{confidential_issue.to_reference}")
+      end
+
+      context 'when the user cannot see confidential issues but was cached with a link', :use_clean_rails_memory_store_fragment_caching do
+        it 'does not render the confidential issue' do
+          visit project_commits_path(project, 'feature')
+          sign_in(create(:user))
+          visit project_commits_path(project, 'feature')
+
+          expect(page).not_to have_link(href: project_issue_path(project, confidential_issue))
+        end
+      end
+    end
+
     context 'master branch' do
       before do
         visit_commits_page