kotovalexarian-likes-gitlab/gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

Send TODOs for comments on commits correctly

Browse source 
At present, the TodoService uses the `:read_project` ability to decide
whether a user can read a note on a commit. However, commits can have a
visibility level that is more restricted than the project, so this is a
security issue.

This commit changes the code to use the `:read_commit` ability in this
case instead, which ensures TODOs are only generated for commit notes
if the users can see the commit.
This commit is contained in:
Nick Thomas 2019-08-22 16:05:07 +01:00
parent f7f3b3c3ef
commit 642f6b3816
No known key found for this signature in database
GPG key ID: 2A313A47AFADACE9
3 changed files with 110 additions and 18 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
app/services/todo_service.rb
View file

@ -314,11 +314,9 @@ class TodoService
end
def reject_users_without_access(users, parent, target)
if target.is_a?(Note) && target.for_issuable?
target = target.noteable
end
target = target.noteable if target.is_a?(Note)
if target.is_a?(Issuable)
if target.respond_to?(:to_ability_name)
select_users(users, :"read_#{target.to_ability_name}", target)
else
select_users(users, :read_project, parent)

5
changelogs/unreleased/security-64711-fix-commit-todos.yml Normal file
View file

@ -0,0 +1,5 @@
---
title: Send TODOs for comments on commits correctly
merge_request:
author:
type: security

117
spec/services/todo_service_spec.rb
View file

@ -436,25 +436,114 @@ describe TodoService do
should_create_todo(user: john_doe, target: confidential_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_confidential_issue)
end
context 'on commit' do
let(:project) { create(:project, :repository) }
context 'commits' do
let(:base_commit_todo_attrs) { { target_id: nil, target_type: 'Commit', author: john_doe } }
it 'creates a todo for each valid mentioned user when leaving a note on commit' do
service.new_note(note_on_commit, john_doe)
context 'leaving a note on a commit in a public project' do
let(:project) { create(:project, :repository, :public) }
it 'creates a todo for each valid mentioned user' do
expected_todo = base_commit_todo_attrs.merge(
action: Todo::MENTIONED,
note: note_on_commit,
commit_id: note_on_commit.commit_id
)
should_create_todo(user: member, target_id: nil, target_type: 'Commit', commit_id: note_on_commit.commit_id, author: john_doe, action: Todo::MENTIONED, note: note_on_commit)
should_create_todo(user: author, target_id: nil, target_type: 'Commit', commit_id: note_on_commit.commit_id, author: john_doe, action: Todo::MENTIONED, note: note_on_commit)
should_create_todo(user: john_doe, target_id: nil, target_type: 'Commit', commit_id: note_on_commit.commit_id, author: john_doe, action: Todo::MENTIONED, note: note_on_commit)
should_not_create_todo(user: non_member, target_id: nil, target_type: 'Commit', commit_id: note_on_commit.commit_id, author: john_doe, action: Todo::MENTIONED, note: note_on_commit)
service.new_note(note_on_commit, john_doe)
should_create_todo(expected_todo.merge(user: member))
should_create_todo(expected_todo.merge(user: author))
should_create_todo(expected_todo.merge(user: john_doe))
should_create_todo(expected_todo.merge(user: guest))
should_create_todo(expected_todo.merge(user: non_member))
end
it 'creates a directly addressed todo for each valid mentioned user' do
expected_todo = base_commit_todo_attrs.merge(
action: Todo::DIRECTLY_ADDRESSED,
note: addressed_note_on_commit,
commit_id: addressed_note_on_commit.commit_id
)
service.new_note(addressed_note_on_commit, john_doe)
should_create_todo(expected_todo.merge(user: member))
should_create_todo(expected_todo.merge(user: author))
should_create_todo(expected_todo.merge(user: john_doe))
should_create_todo(expected_todo.merge(user: guest))
should_create_todo(expected_todo.merge(user: non_member))
end
end
it 'creates a directly addressed todo for each valid mentioned user when leaving a note on commit' do
service.new_note(addressed_note_on_commit, john_doe)
context 'leaving a note on a commit in a public project with private code' do
let(:project) { create(:project, :repository, :public, :repository_private) }
should_create_todo(user: member, target_id: nil, target_type: 'Commit', commit_id: addressed_note_on_commit.commit_id, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_commit)
should_create_todo(user: author, target_id: nil, target_type: 'Commit', commit_id: addressed_note_on_commit.commit_id, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_commit)
should_create_todo(user: john_doe, target_id: nil, target_type: 'Commit', commit_id: addressed_note_on_commit.commit_id, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_commit)
should_not_create_todo(user: non_member, target_id: nil, target_type: 'Commit', commit_id: addressed_note_on_commit.commit_id, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_commit)
it 'creates a todo for each valid mentioned user' do
expected_todo = base_commit_todo_attrs.merge(
action: Todo::MENTIONED,
note: note_on_commit,
commit_id: note_on_commit.commit_id
)
service.new_note(note_on_commit, john_doe)
should_create_todo(expected_todo.merge(user: member))
should_create_todo(expected_todo.merge(user: author))
should_create_todo(expected_todo.merge(user: john_doe))
should_create_todo(expected_todo.merge(user: guest))
should_not_create_todo(expected_todo.merge(user: non_member))
end
it 'creates a directly addressed todo for each valid mentioned user' do
expected_todo = base_commit_todo_attrs.merge(
action: Todo::DIRECTLY_ADDRESSED,
note: addressed_note_on_commit,
commit_id: addressed_note_on_commit.commit_id
)
service.new_note(addressed_note_on_commit, john_doe)
should_create_todo(expected_todo.merge(user: member))
should_create_todo(expected_todo.merge(user: author))
should_create_todo(expected_todo.merge(user: john_doe))
should_create_todo(expected_todo.merge(user: guest))
should_not_create_todo(expected_todo.merge(user: non_member))
end
end
context 'leaving a note on a commit in a private project' do
let(:project) { create(:project, :repository, :private) }
it 'creates a todo for each valid mentioned user' do
expected_todo = base_commit_todo_attrs.merge(
action: Todo::MENTIONED,
note: note_on_commit,
commit_id: note_on_commit.commit_id
)
service.new_note(note_on_commit, john_doe)
should_create_todo(expected_todo.merge(user: member))
should_create_todo(expected_todo.merge(user: author))
should_create_todo(expected_todo.merge(user: john_doe))
should_not_create_todo(expected_todo.merge(user: guest))
should_not_create_todo(expected_todo.merge(user: non_member))
end
it 'creates a directly addressed todo for each valid mentioned user' do
expected_todo = base_commit_todo_attrs.merge(
action: Todo::DIRECTLY_ADDRESSED,
note: addressed_note_on_commit,
commit_id: addressed_note_on_commit.commit_id
)
service.new_note(addressed_note_on_commit, john_doe)
should_create_todo(expected_todo.merge(user: member))
should_create_todo(expected_todo.merge(user: author))
should_create_todo(expected_todo.merge(user: john_doe))
should_not_create_todo(expected_todo.merge(user: guest))
should_not_create_todo(expected_todo.merge(user: non_member))
end
end
end