| Age | Commit message (Collapse) | Author |
|---|
|
Refactor snippets finder & dont return internal snippets for external users
See merge request !2094
|
|
|
|
|
|
|
|
|
|
|
|
|
|
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?
:warning: - Potentially untested
:bomb: - No test coverage
:traffic_light: - Test coverage of some sort exists (a test failed when error raised)
:vertical_traffic_light: - Test coverage of return value (a test failed when nil used)
:white_check_mark: - Permissions check tested
### Note lookup without access check
- [x] :white_check_mark: app/finders/notes_finder.rb:13 :download_code check
- [x] :white_check_mark: app/finders/notes_finder.rb:19 `SnippetsFinder`
- [x] :white_check_mark: app/models/note.rb:121 [`Issue#visible_to_user`]
- [x] :white_check_mark: 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
|
|
Replace MR access checks with use of MergeRequestsFinder
Split from !2024 to partially solve https://gitlab.com/gitlab-org/gitlab-ce/issues/23867
:warning: - Potentially untested
:bomb: - No test coverage
:traffic_light: - Test coverage of some sort exists (a test failed when error raised)
:vertical_traffic_light: - Test coverage of return value (a test failed when nil used)
:white_check_mark: - Permissions check tested
- [x] :bomb: app/finders/notes_finder.rb:17
- [x] :warning: app/views/layouts/nav/_project.html.haml:80 [`.count`]
- [x] :bomb: app/controllers/concerns/creates_commit.rb:84
- [x] :traffic_light: app/controllers/projects/commits_controller.rb:24
- [x] :traffic_light: app/controllers/projects/compare_controller.rb:56
- [x] :vertical_traffic_light: app/controllers/projects/discussions_controller.rb:29
- [x] :white_check_mark: app/controllers/projects/todos_controller.rb:27
- [x] :vertical_traffic_light: app/models/commit.rb:268
- [x] :white_check_mark: lib/gitlab/search_results.rb:71
- [x] https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2024/diffs#d1c10892daedb4d4dd3d4b12b6d071091eea83df_267_266 Memoize ` merged_merge_request(current_user)`
- [x] https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2024/diffs#d1c10892daedb4d4dd3d4b12b6d071091eea83df_248_247 Expected side effect for `merged_merge_request!`, consider `skip_authorization: true`.
- [x] https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2024/diffs#d1c10892daedb4d4dd3d4b12b6d071091eea83df_269_269 Scary use of unchecked `merged_merge_request?`
See merge request !2033
|
|
Replace issue access checks with use of IssuableFinder
Split from !2024 to partially solve https://gitlab.com/gitlab-org/gitlab-ce/issues/23867
## Which fixes are in this MR?
:warning: - Potentially untested
:bomb: - No test coverage
:traffic_light: - Test coverage of some sort exists (a test failed when error raised)
:vertical_traffic_light: - Test coverage of return value (a test failed when nil used)
:white_check_mark: - Permissions check tested
### Issue lookup with access check
Using `visible_to_user` likely makes these security issues too. See [Code smells](#code-smells).
- [x] :vertical_traffic_light: app/finders/notes_finder.rb:15 [`visible_to_user`]
- [x] :traffic_light: app/views/layouts/nav/_project.html.haml:73 [`visible_to_user`] [`.count`]
- [x] :white_check_mark: app/services/merge_requests/build_service.rb:84 [`issue.try(:confidential?)`]
- [x] :white_check_mark: lib/api/issues.rb:112 [`visible_to_user`]
- CHANGELOG: Prevented API returning issues set to 'Only team members' to everyone
- [x] :white_check_mark: lib/api/helpers.rb:126 [`can?(current_user, :read_issue, issue)`] Maybe here too?
- [x] :white_check_mark: lib/gitlab/search_results.rb:53 [`visible_to_user`]
### Previous discussions
- [ ] https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2024/diffs#b2ff264eddf9819d7693c14ae213d941494fe2b3_128_126
- [ ] https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2024/diffs#7b6375270d22f880bdcb085e47b519b426a5c6c7_87_87
See merge request !2031
|
|
|
|
# Conflicts:
# app/controllers/projects/merge_requests_controller.rb
# app/models/note.rb
# db/schema.rb
# spec/models/note_spec.rb
|
|
|
|
|
|
|
|
|
|
|
|
|
|
Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
|
|
Users who have not refreshed their browser tab will poll GitLab using
outdated JS. This change makes the server fall back to the old behavior
(send all comments) for old clients, instead of throwing an exception
for old clients.
|
|
|
|
|
|
|
|
Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
|
