BigW Consortium Gitlab
Fix missing Note access checks in by moving Note#search to updated NoteFinder Split from !2024 to partially solve https://gitlab.com/gitlab-org/gitlab-ce/issues/23867 ## Which fixes are in this MR?⚠ - Potentially untested💣 - No test coverage🚥 - Test coverage of some sort exists (a test failed when error raised)🚦 - Test coverage of return value (a test failed when nil used)✅ - Permissions check tested ### Note lookup without access check - [x]✅ app/finders/notes_finder.rb:13 :download_code check - [x]✅ app/finders/notes_finder.rb:19 `SnippetsFinder` - [x]✅ app/models/note.rb:121 [`Issue#visible_to_user`] - [x]✅ lib/gitlab/project_search_results.rb:113 - This is the only use of `app/models/note.rb:121` above, but importantly has no access checks at all. This means it leaks MR comments and snippets when those features are `team-only` in addition to the issue comments which would be fixed by `app/models/note.rb:121`. - It is only called from SearchController where `can?(current_user, :download_code, @project)` is checked, so commit comments are not leaked. ### Previous discussions - [x] https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2024/diffs#b915c5267a63628b0bafd23d37792ae73ceae272_13_13 `: download_code` check on commit - [x] https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2024/diffs#b915c5267a63628b0bafd23d37792ae73ceae272_19_19 `SnippetsFinder` should be used - `SnippetsFinder` should check if the snippets feature is enabled -> https://gitlab.com/gitlab-org/gitlab-ce/issues/25223 ### Acceptance criteria met? - [x] Tests added for new code - [x] TODO comments removed - [x] Squashed and removed skipped tests - [x] Changelog entry - [ ] State Gitlab versions affected and issue severity in description - [ ] Create technical debt issue for NotesFinder. - Either split into `NotesFinder::ForTarget` and `NotesFinder::Search` or consider object per notable type such as `NotesFinder::OnIssue`. For the first option could create `NotesFinder::Base` which is either inherited from or which can be included in the other two. - Avoid case statement anti-pattern in this finder with use of `NotesFinder::OnCommit` etc. Consider something on the finder for this? `Model.finder(user, project)` - Move `inc_author` to the controller, and implement `related_notes` to replace `non_diff_notes`/`mr_and_commit_notes` See merge request !2035
Name |
Last commit
|
Last update |
---|---|---|
.. | ||
ci | Loading commit data... | |
concerns | Loading commit data... | |
cycle_analytics | Loading commit data... | |
hooks | Loading commit data... | |
issue | Loading commit data... | |
members | Loading commit data... | |
merge_request | Loading commit data... | |
network | Loading commit data... | |
project_services | Loading commit data... | |
protected_branch | Loading commit data... | |
.gitkeep | Loading commit data... | |
ability.rb | Loading commit data... | |
abuse_report.rb | Loading commit data... | |
appearance.rb | Loading commit data... | |
application_setting.rb | Loading commit data... | |
audit_event.rb | Loading commit data... | |
award_emoji.rb | Loading commit data... | |
blob.rb | Loading commit data... | |
board.rb | Loading commit data... | |
broadcast_message.rb | Loading commit data... | |
chat_name.rb | Loading commit data... | |
commit.rb | Loading commit data... | |
commit_range.rb | Loading commit data... | |
commit_status.rb | Loading commit data... | |
compare.rb | Loading commit data... | |
cycle_analytics.rb | Loading commit data... | |
deploy_key.rb | Loading commit data... | |
deploy_keys_project.rb | Loading commit data... | |
deployment.rb | Loading commit data... | |
diff_note.rb | Loading commit data... | |
discussion.rb | Loading commit data... | |
email.rb | Loading commit data... | |
environment.rb | Loading commit data... | |
event.rb | Loading commit data... | |
external_issue.rb | Loading commit data... | |
forked_project_link.rb | Loading commit data... | |
generic_commit_status.rb | Loading commit data... | |
global_label.rb | Loading commit data... | |
global_milestone.rb | Loading commit data... | |
group.rb | Loading commit data... | |
group_label.rb | Loading commit data... | |
guest.rb | Loading commit data... | |
identity.rb | Loading commit data... | |
issue.rb | Loading commit data... | |
issue_collection.rb | Loading commit data... | |
key.rb | Loading commit data... | |
label.rb | Loading commit data... | |
label_link.rb | Loading commit data... | |
label_priority.rb | Loading commit data... | |
legacy_diff_note.rb | Loading commit data... | |
lfs_object.rb | Loading commit data... | |
lfs_objects_project.rb | Loading commit data... | |
list.rb | Loading commit data... | |
member.rb | Loading commit data... | |
merge_request.rb | Loading commit data... | |
merge_request_diff.rb | Loading commit data... | |
merge_requests_closing_issues.rb | Loading commit data... | |
milestone.rb | Loading commit data... | |
namespace.rb | Loading commit data... | |
note.rb | Loading commit data... | |
notification_setting.rb | Loading commit data... | |
oauth_access_token.rb | Loading commit data... | |
personal_access_token.rb | Loading commit data... | |
personal_snippet.rb | Loading commit data... | |
project.rb | Loading commit data... | |
project_authorization.rb | Loading commit data... | |
project_feature.rb | Loading commit data... | |
project_group_link.rb | Loading commit data... | |
project_import_data.rb | Loading commit data... | |
project_label.rb | Loading commit data... | |
project_snippet.rb | Loading commit data... | |
project_team.rb | Loading commit data... | |
project_wiki.rb | Loading commit data... | |
protected_branch.rb | Loading commit data... | |
release.rb | Loading commit data... | |
repository.rb | Loading commit data... | |
route.rb | Loading commit data... | |
security_event.rb | Loading commit data... | |
sent_notification.rb | Loading commit data... | |
service.rb | Loading commit data... | |
snippet.rb | Loading commit data... | |
spam_log.rb | Loading commit data... | |
subscription.rb | Loading commit data... | |
todo.rb | Loading commit data... | |
tree.rb | Loading commit data... | |
trending_project.rb | Loading commit data... | |
u2f_registration.rb | Loading commit data... | |
user.rb | Loading commit data... | |
user_agent_detail.rb | Loading commit data... | |
users_star_project.rb | Loading commit data... | |
wiki_page.rb | Loading commit data... |