Merge branch 'security-65756-ex-admin-attacker-can-comment-in-internalsecurity-65756-ex-admin-attacker-can-comment-in-internal-12-4' into '12-4-stable'

Improper access control allows the attacker to comment in internal commit after they are no longer admin

See merge request gitlab/gitlabhq!3497

3 files changed, 42 insertions, 12 deletions

diff --git a/app/policies/commit_policy.rb b/app/policies/commit_policy.rb
index 4d4f0ba9267..4b358c45ec2 100644
--- a/app/policies/commit_policy.rb
+++ b/app/policies/commit_policy.rb
@@ -4,4 +4,5 @@ class CommitPolicy < BasePolicy
   delegate { @subject.project }
 
   rule { can?(:download_code) }.enable :read_commit
+  rule { ~can?(:read_commit) }.prevent :create_note
 end
diff --git a/changelogs/unreleased/security-65756-ex-admin-attacker-can-comment-in-internal.yml b/changelogs/unreleased/security-65756-ex-admin-attacker-can-comment-in-internal.yml
new file mode 100644
index 00000000000..3d9f480ba11
--- /dev/null
+++ b/changelogs/unreleased/security-65756-ex-admin-attacker-can-comment-in-internal.yml
@@ -0,0 +1,5 @@
+---
+title: Disallow unprivileged users from commenting on private repository commits
+merge_request:
+author:
+type: security
diff --git a/spec/policies/commit_policy_spec.rb b/spec/policies/commit_policy_spec.rb
index 41f6fb08426..40183f51e9e 100644
--- a/spec/policies/commit_policy_spec.rb
+++ b/spec/policies/commit_policy_spec.rb
@@ -8,28 +8,42 @@ describe CommitPolicy do
     let(:commit) { project.repository.head_commit }
     let(:policy) { described_class.new(user, commit) }
 
+    shared_examples 'can read commit and create a note' do
+      it 'can read commit' do
+        expect(policy).to be_allowed(:read_commit)
+      end
+
+      it 'can create a note' do
+        expect(policy).to be_allowed(:create_note)
+      end
+    end
+
+    shared_examples 'cannot read commit nor create a note' do
+      it 'can not read commit' do
+        expect(policy).to be_disallowed(:read_commit)
+      end
+
+      it 'can not create a note' do
+        expect(policy).to be_disallowed(:create_note)
+      end
+    end
+
     context 'when project is public' do
       let(:project) { create(:project, :public, :repository) }
 
-      it 'can read commit and create a note' do
-        expect(policy).to be_allowed(:read_commit)
-      end
+      it_behaves_like 'can read commit and create a note'
 
       context 'when repository access level is private' do
         let(:project) { create(:project, :public, :repository, :repository_private) }
 
-        it 'can not read commit and create a note' do
-          expect(policy).to be_disallowed(:read_commit)
-        end
+        it_behaves_like 'cannot read commit nor create a note'
 
         context 'when the user is a project member' do
           before do
             project.add_developer(user)
           end
 
-          it 'can read commit and create a note' do
-            expect(policy).to be_allowed(:read_commit)
-          end
+          it_behaves_like 'can read commit and create a note'
         end
       end
     end
@@ -37,9 +51,7 @@ describe CommitPolicy do
     context 'when project is private' do
       let(:project) { create(:project, :private, :repository) }
 
-      it 'can not read commit and create a note' do
-        expect(policy).to be_disallowed(:read_commit)
-      end
+      it_behaves_like 'cannot read commit nor create a note'
 
       context 'when the user is a project member' do
         before do
@@ -50,6 +62,18 @@ describe CommitPolicy do
           expect(policy).to be_allowed(:read_commit)
         end
       end
+
+      context 'when the user is a guest' do
+        before do
+          project.add_guest(user)
+        end
+
+        it_behaves_like 'cannot read commit nor create a note'
+
+        it 'cannot download code' do
+          expect(policy).to be_disallowed(:download_code)
+        end
+      end
     end
   end
 end