diff --git a/app/controllers/concerns/notes_actions.rb b/app/controllers/concerns/notes_actions.rb
index 777b147e2dd24a3cd95a7cdae8563bef160f9152..0319948a12f26ba2ea3aa147d0dd8388392bfc76 100644
--- a/app/controllers/concerns/notes_actions.rb
+++ b/app/controllers/concerns/notes_actions.rb
@@ -6,6 +6,7 @@ module NotesActions
extend ActiveSupport::Concern
included do
+ prepend_before_action :normalize_create_params, only: [:create]
before_action :set_polling_interval_header, only: [:index]
before_action :require_noteable!, only: [:index, :create]
before_action :authorize_admin_note!, only: [:update, :destroy]
@@ -247,6 +248,15 @@ def discussion_serializer
DiscussionSerializer.new(project: project, noteable: noteable, current_user: current_user, note_entity: ProjectNoteEntity)
end
+ # Avoids checking permissions in the wrong object - this ensures that the object we checked permissions for
+ # is the object we're actually creating a note in.
+ def normalize_create_params
+ params[:note].try do |note|
+ note[:noteable_id] = params[:target_id]
+ note[:noteable_type] = params[:target_type].classify
+ end
+ end
+
def note_project
strong_memoize(:note_project) do
next nil unless project
diff --git a/app/mailers/emails/notes.rb b/app/mailers/emails/notes.rb
index d3284e90568ca63a3dc75f15f20d1cd03975fbcf..1b3c1f9a8a9076990f4e64888f83e7df5eac24c8 100644
--- a/app/mailers/emails/notes.rb
+++ b/app/mailers/emails/notes.rb
@@ -26,7 +26,7 @@ def note_merge_request_email(recipient_id, note_id)
mail_answer_note_thread(@merge_request, @note, note_thread_options(recipient_id))
end
- def note_snippet_email(recipient_id, note_id)
+ def note_project_snippet_email(recipient_id, note_id)
setup_note_mail(note_id, recipient_id)
@snippet = @note.noteable
diff --git a/app/models/note.rb b/app/models/note.rb
index 592efb714f327e6df544cc0fca2c35c38b2adadd..a6ae4f58ac48cb0ef9390054ec8797fc93fbd1f9 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -324,7 +324,7 @@ def contains_emoji_only?
end
def to_ability_name
- for_personal_snippet? ? 'personal_snippet' : noteable_type.underscore
+ for_snippet? ? noteable.class.name.underscore : noteable_type.underscore
end
def can_be_discussion_note?
diff --git a/app/policies/commit_policy.rb b/app/policies/commit_policy.rb
index 67e9bc12804555e001d3e81793c40335e1423ecf..4d4f0ba92674f4e2d24d398605772d2abd3fb15e 100644
--- a/app/policies/commit_policy.rb
+++ b/app/policies/commit_policy.rb
@@ -2,4 +2,6 @@
class CommitPolicy < BasePolicy
delegate { @subject.project }
+
+ rule { can?(:download_code) }.enable :read_commit
end
diff --git a/app/policies/note_policy.rb b/app/policies/note_policy.rb
index bbc2b48b8567e24813d0305c871df7a22d96b545..f22843b6463761f4ea40e99b2e6b563ff5624cd8 100644
--- a/app/policies/note_policy.rb
+++ b/app/policies/note_policy.rb
@@ -9,8 +9,17 @@ class NotePolicy < BasePolicy
condition(:editable, scope: :subject) { @subject.editable? }
+ condition(:can_read_noteable) { can?(:"read_#{@subject.to_ability_name}") }
+
rule { ~editable }.prevent :admin_note
+ # If user can't read the issue/MR/etc then they should not be allowed to do anything to their own notes
+ rule { ~can_read_noteable }.policy do
+ prevent :read_note
+ prevent :admin_note
+ prevent :resolve_note
+ end
+
rule { is_author }.policy do
enable :read_note
enable :admin_note
diff --git a/app/views/notify/note_snippet_email.html.haml b/app/views/notify/note_project_snippet_email.html.haml
similarity index 100%
rename from app/views/notify/note_snippet_email.html.haml
rename to app/views/notify/note_project_snippet_email.html.haml
diff --git a/app/views/notify/note_snippet_email.text.erb b/app/views/notify/note_project_snippet_email.text.erb
similarity index 100%
rename from app/views/notify/note_snippet_email.text.erb
rename to app/views/notify/note_project_snippet_email.text.erb
diff --git a/changelogs/unreleased/security-guest-comments.yml b/changelogs/unreleased/security-guest-comments.yml
new file mode 100644
index 0000000000000000000000000000000000000000..2c99512433ba5d7d46517eb800381c8985fda963
--- /dev/null
+++ b/changelogs/unreleased/security-guest-comments.yml
@@ -0,0 +1,5 @@
+---
+title: Fixed ability to comment on locked/confidential issues.
+merge_request:
+author:
+type: security
diff --git a/changelogs/unreleased/security-guest-comments_2.yml b/changelogs/unreleased/security-guest-comments_2.yml
new file mode 100644
index 0000000000000000000000000000000000000000..be6f2d6a49070986464b81b8a3c15974b5d53bae
--- /dev/null
+++ b/changelogs/unreleased/security-guest-comments_2.yml
@@ -0,0 +1,5 @@
+---
+title: Fixed ability of guest users to edit/delete comments on locked or confidential issues.
+merge_request:
+author:
+type: security
diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb
index 9ac7b8ee8a83f04ed00e565db50f84f422c12c43..d2a260683621c1fa611ff07dc91e4c0e065c1125 100644
--- a/spec/controllers/projects/notes_controller_spec.rb
+++ b/spec/controllers/projects/notes_controller_spec.rb
@@ -283,14 +283,14 @@
def post_create(extra_params = {})
post :create, {
- note: { note: 'some other note' },
- namespace_id: project.namespace,
- project_id: project,
- target_type: 'merge_request',
- target_id: merge_request.id,
- note_project_id: forked_project.id,
- in_reply_to_discussion_id: existing_comment.discussion_id
- }.merge(extra_params)
+ note: { note: 'some other note', noteable_id: merge_request.id },
+ namespace_id: project.namespace,
+ project_id: project,
+ target_type: 'merge_request',
+ target_id: merge_request.id,
+ note_project_id: forked_project.id,
+ in_reply_to_discussion_id: existing_comment.discussion_id
+ }.merge(extra_params)
end
context 'when the note_project_id is not correct' do
@@ -324,6 +324,30 @@ def post_create(extra_params = {})
end
end
+ context 'when target_id and noteable_id do not match' do
+ let(:locked_issue) { create(:issue, :locked, project: project) }
+ let(:issue) {create(:issue, project: project)}
+
+ before do
+ project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC)
+ project.project_member(user).destroy
+ end
+
+ it 'uses target_id and ignores noteable_id' do
+ request_params = {
+ note: { note: 'some note', noteable_type: 'Issue', noteable_id: locked_issue.id },
+ target_type: 'issue',
+ target_id: issue.id,
+ project_id: project,
+ namespace_id: project.namespace
+ }
+
+ expect { post :create, request_params }.to change { issue.notes.count }.by(1)
+ .and change { locked_issue.notes.count }.by(0)
+ expect(response).to have_gitlab_http_status(302)
+ end
+ end
+
context 'when the merge request discussion is locked' do
before do
project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC)
@@ -376,35 +400,60 @@ def post_create(extra_params = {})
end
describe 'PUT update' do
- let(:request_params) do
- {
- namespace_id: project.namespace,
- project_id: project,
- id: note,
- format: :json,
- note: {
- note: "New comment"
+ context "should update the note with a valid issue" do
+ let(:request_params) do
+ {
+ namespace_id: project.namespace,
+ project_id: project,
+ id: note,
+ format: :json,
+ note: {
+ note: "New comment"
+ }
}
- }
- end
+ end
- before do
- sign_in(note.author)
- project.add_developer(note.author)
+ before do
+ sign_in(note.author)
+ project.add_developer(note.author)
+ end
+
+ it "updates the note" do
+ expect { put :update, request_params }.to change { note.reload.note }
+ end
end
+ context "doesnt update the note" do
+ let(:issue) { create(:issue, :confidential, project: project) }
+ let(:note) { create(:note, noteable: issue, project: project) }
- it "updates the note" do
- expect { put :update, request_params }.to change { note.reload.note }
+ before do
+ sign_in(user)
+ project.add_guest(user)
+ end
+
+ it "disallows edits when the issue is confidential and the user has guest permissions" do
+ request_params = {
+ namespace_id: project.namespace,
+ project_id: project,
+ id: note,
+ format: :json,
+ note: {
+ note: "New comment"
+ }
+ }
+ expect { put :update, request_params }.not_to change { note.reload.note }
+ expect(response).to have_gitlab_http_status(404)
+ end
end
end
describe 'DELETE destroy' do
let(:request_params) do
{
- namespace_id: project.namespace,
- project_id: project,
- id: note,
- format: :js
+ namespace_id: project.namespace,
+ project_id: project,
+ id: note,
+ format: :js
}
end
diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index ff1a5aa2536ce90387c5bc8b9354f941c3fde7e5..150c00e4bfe809d4b487d0d812bb5c4a20024475 100644
--- a/spec/mailers/notify_spec.rb
+++ b/spec/mailers/notify_spec.rb
@@ -522,7 +522,7 @@ def have_referable_subject(referable, reply: false)
let(:project_snippet) { create(:project_snippet, project: project) }
let(:project_snippet_note) { create(:note_on_project_snippet, project: project, noteable: project_snippet) }
- subject { described_class.note_snippet_email(project_snippet_note.author_id, project_snippet_note.id) }
+ subject { described_class.note_project_snippet_email(project_snippet_note.author_id, project_snippet_note.id) }
it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do
let(:model) { project_snippet }
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index f9be61e4768907c1fe13afef1eb422e1a3ee3593..bcdfe3cf1eb14e50e1257f1573b6b3e8cb06b3b5 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -517,7 +517,7 @@ def retrieve_participants
describe '#to_ability_name' do
it 'returns snippet for a project snippet note' do
- expect(build(:note_on_project_snippet).to_ability_name).to eq('snippet')
+ expect(build(:note_on_project_snippet).to_ability_name).to eq('project_snippet')
end
it 'returns personal_snippet for a personal snippet note' do
diff --git a/spec/policies/note_policy_spec.rb b/spec/policies/note_policy_spec.rb
index e8096358f7df2e21e60dcde7db5a755e048db051..7e25c53e77c30ca35d141afbd54460639124c2cc 100644
--- a/spec/policies/note_policy_spec.rb
+++ b/spec/policies/note_policy_spec.rb
@@ -10,11 +10,50 @@ def policies(noteable = nil)
return @policies if @policies
noteable ||= issue
- note = create(:note, noteable: noteable, author: user, project: project)
+ note = if noteable.is_a?(Commit)
+ create(:note_on_commit, commit_id: noteable.id, author: user, project: project)
+ else
+ create(:note, noteable: noteable, author: user, project: project)
+ end
@policies = described_class.new(user, note)
end
+ shared_examples_for 'a discussion with a private noteable' do
+ let(:noteable) { issue }
+ let(:policy) { policies(noteable) }
+
+ context 'when the note author can no longer see the noteable' do
+ it 'can not edit nor read the note' do
+ expect(policy).to be_disallowed(:admin_note)
+ expect(policy).to be_disallowed(:resolve_note)
+ expect(policy).to be_disallowed(:read_note)
+ end
+ end
+
+ context 'when the note author can still see the noteable' do
+ before do
+ project.add_developer(user)
+ end
+
+ it 'can edit the note' do
+ expect(policy).to be_allowed(:admin_note)
+ expect(policy).to be_allowed(:resolve_note)
+ expect(policy).to be_allowed(:read_note)
+ end
+ end
+ end
+
+ context 'when the project is private' do
+ let(:project) { create(:project, :private, :repository) }
+
+ context 'when the noteable is a commit' do
+ it_behaves_like 'a discussion with a private noteable' do
+ let(:noteable) { project.repository.head_commit }
+ end
+ end
+ end
+
context 'when the project is public' do
context 'when the note author is not a project member' do
it 'can edit a note' do
@@ -24,14 +63,48 @@ def policies(noteable = nil)
end
end
- context 'when the noteable is a snippet' do
+ context 'when the noteable is a project snippet' do
+ it 'can edit note' do
+ policies = policies(create(:project_snippet, :public, project: project))
+
+ expect(policies).to be_allowed(:admin_note)
+ expect(policies).to be_allowed(:resolve_note)
+ expect(policies).to be_allowed(:read_note)
+ end
+
+ context 'when it is private' do
+ it_behaves_like 'a discussion with a private noteable' do
+ let(:noteable) { create(:project_snippet, :private, project: project) }
+ end
+ end
+ end
+
+ context 'when the noteable is a personal snippet' do
it 'can edit note' do
- policies = policies(create(:project_snippet, project: project))
+ policies = policies(create(:personal_snippet, :public))
expect(policies).to be_allowed(:admin_note)
expect(policies).to be_allowed(:resolve_note)
expect(policies).to be_allowed(:read_note)
end
+
+ context 'when it is private' do
+ it 'can not edit nor read the note' do
+ policies = policies(create(:personal_snippet, :private))
+
+ expect(policies).to be_disallowed(:admin_note)
+ expect(policies).to be_disallowed(:resolve_note)
+ expect(policies).to be_disallowed(:read_note)
+ end
+ end
+ end
+
+ context 'when a discussion is confidential' do
+ before do
+ issue.update_attribute(:confidential, true)
+ end
+
+ it_behaves_like 'a discussion with a private noteable'
end
context 'when a discussion is locked' do