diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2022-09-28 21:59:16 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2022-09-28 21:59:16 +0000 |
commit | ba2dd425136ba32ccb9793b5c10e5f26910970a2 (patch) | |
tree | e8955d82a92ae42c378a6efdd5d8249837708703 | |
parent | 16918719748469eb301797d7ec94da59269fa197 (diff) | |
download | gitlab-ce-ba2dd425136ba32ccb9793b5c10e5f26910970a2.tar.gz |
Add latest changes from gitlab-org/security/gitlab@15-2-stable-ee
13 files changed, 248 insertions, 90 deletions
diff --git a/app/assets/javascripts/vue_merge_request_widget/components/extensions/child_content.vue b/app/assets/javascripts/vue_merge_request_widget/components/extensions/child_content.vue index 38f83a61b30..4c646cc1481 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/extensions/child_content.vue +++ b/app/assets/javascripts/vue_merge_request_widget/components/extensions/child_content.vue @@ -1,5 +1,6 @@ <script> import { GlBadge, GlLink, GlSafeHtmlDirective, GlModalDirective } from '@gitlab/ui'; +import { isArray } from 'lodash'; import StatusIcon from './status_icon.vue'; import Actions from './actions.vue'; import { generateText } from './utils'; @@ -35,6 +36,20 @@ export default { required: true, }, }, + computed: { + subtext() { + const { subtext } = this.data; + if (subtext) { + if (isArray(subtext)) { + return subtext.map((t) => generateText(t)).join('<br />'); + } + + return generateText(subtext); + } + + return null; + }, + }, methods: { isArray(arr) { return Array.isArray(arr); @@ -91,11 +106,7 @@ export default { @clickedAction="onClickedAction" /> </div> - <p - v-if="data.subtext" - v-safe-html="generateText(data.subtext)" - class="gl-m-0 gl-font-sm" - ></p> + <p v-if="subtext" v-safe-html="subtext" class="gl-m-0 gl-font-sm"></p> </div> </div> <template v-if="data.children && level === 2"> diff --git a/app/assets/javascripts/vue_merge_request_widget/components/extensions/utils.js b/app/assets/javascripts/vue_merge_request_widget/components/extensions/utils.js index cba12507eba..757178ee336 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/extensions/utils.js +++ b/app/assets/javascripts/vue_merge_request_widget/components/extensions/utils.js @@ -35,6 +35,9 @@ const textStyleTags = { [getStartTag('small')]: '<span class="gl-font-sm gl-text-gray-700">', }; +const escapeText = (text) => + document.createElement('div').appendChild(document.createTextNode(text)).parentNode.innerHTML; + const createText = (text) => { return text .replace( @@ -61,7 +64,7 @@ const createText = (text) => { export const generateText = (text) => { if (typeof text === 'string') { - return createText(text); + return createText(escapeText(text)); } else if ( typeof text === 'object' && typeof text.text === 'string' && @@ -69,8 +72,8 @@ export const generateText = (text) => { ) { return createText( `${ - text.prependText ? `${text.prependText} ` : '' - }<a class="gl-text-decoration-underline" href="${text.href}">${text.text}</a>`, + text.prependText ? `${escapeText(text.prependText)} ` : '' + }<a class="gl-text-decoration-underline" href="${text.href}">${escapeText(text.text)}</a>`, ); } diff --git a/app/assets/javascripts/vue_merge_request_widget/extensions/code_quality/index.js b/app/assets/javascripts/vue_merge_request_widget/extensions/code_quality/index.js index 2477429af5b..68347ac269e 100644 --- a/app/assets/javascripts/vue_merge_request_widget/extensions/code_quality/index.js +++ b/app/assets/javascripts/vue_merge_request_widget/extensions/code_quality/index.js @@ -19,25 +19,23 @@ export default { if (errorSummary.errored >= 1 && errorSummary.resolved >= 1) { const improvements = sprintf( n__( - '%{strongOpen}%{errors}%{strongClose} point', - '%{strongOpen}%{errors}%{strongClose} points', + '%{strong_start}%{errors}%{strong_end} point', + '%{strong_start}%{errors}%{strong_end} points', resolvedErrors.length, ), { errors: resolvedErrors.length, - strongOpen: '<strong>', - strongClose: '</strong>', }, false, ); const degradations = sprintf( n__( - '%{strongOpen}%{errors}%{strongClose} point', - '%{strongOpen}%{errors}%{strongClose} points', + '%{strong_start}%{errors}%{strong_end} point', + '%{strong_start}%{errors}%{strong_end} points', newErrors.length, ), - { errors: newErrors.length, strongOpen: '<strong>', strongClose: '</strong>' }, + { errors: newErrors.length }, false, ); return sprintf( @@ -96,14 +94,11 @@ export default { this.collapsedData.resolvedErrors.map((e) => { return fullData.push({ text: `${capitalizeFirstCharacter(e.severity)} - ${e.description}`, - subtext: sprintf( - s__(`ciReport|in %{open_link}${e.file_path}:${e.line}%{close_link}`), - { - open_link: `<a class="gl-text-decoration-underline" href="${e.urlPath}">`, - close_link: '</a>', - }, - false, - ), + subtext: { + prependText: s__(`ciReport|in`), + text: `${e.file_path}:${e.line}`, + href: e.urlPath, + }, icon: { name: SEVERITY_ICONS_EXTENSION[e.severity], }, diff --git a/app/assets/javascripts/vue_merge_request_widget/extensions/terraform/index.js b/app/assets/javascripts/vue_merge_request_widget/extensions/terraform/index.js index 6611aedcb07..626a99f7d64 100644 --- a/app/assets/javascripts/vue_merge_request_widget/extensions/terraform/index.js +++ b/app/assets/javascripts/vue_merge_request_widget/extensions/terraform/index.js @@ -63,13 +63,16 @@ export default { if (valid.length) { title = validText; if (invalid.length) { - subtitle = sprintf(`<br>%{small_start}${invalidText}%{small_end}`); + subtitle = invalidText; } } else { title = invalidText; } - return `${title}${subtitle}`; + return { + subject: title, + meta: subtitle, + }; }, fetchCollapsedData() { return axios @@ -152,9 +155,8 @@ export default { } return { - text: `${title} - <br> - ${subtitle}`, + text: title, + supportingText: subtitle, icon: { name: iconName }, actions, }; diff --git a/app/assets/javascripts/vue_merge_request_widget/extensions/test_report/utils.js b/app/assets/javascripts/vue_merge_request_widget/extensions/test_report/utils.js index 4ffd06de61f..4823f3b7a0b 100644 --- a/app/assets/javascripts/vue_merge_request_widget/extensions/test_report/utils.js +++ b/app/assets/javascripts/vue_merge_request_widget/extensions/test_report/utils.js @@ -60,7 +60,7 @@ export const reportSubTextBuilder = ({ suite_errors, summary }) => { if (suite_errors?.base) { errors.push(`${i18n.baseReportParsingError} ${suite_errors.base}`); } - return errors.join('<br />'); + return errors; } return recentFailuresTextBuilder(summary); }; diff --git a/app/policies/issuable_policy.rb b/app/policies/issuable_policy.rb index f1efcb25331..128f9c5a836 100644 --- a/app/policies/issuable_policy.rb +++ b/app/policies/issuable_policy.rb @@ -21,6 +21,12 @@ class IssuablePolicy < BasePolicy enable :reopen_issue end + # This rule replicates permissions in NotePolicy#can_read_confidential and it's used in + # TodoPolicy for performance reasons + rule { can?(:reporter_access) | assignee_or_author | admin }.policy do + enable :read_confidential_notes + end + rule { can?(:read_merge_request) & assignee_or_author }.policy do enable :update_merge_request enable :reopen_merge_request diff --git a/app/policies/note_policy.rb b/app/policies/note_policy.rb index e85f18f2d37..1bffcc5aea2 100644 --- a/app/policies/note_policy.rb +++ b/app/policies/note_policy.rb @@ -20,6 +20,7 @@ class NotePolicy < BasePolicy condition(:confidential, scope: :subject) { @subject.confidential? } + # If this condition changes IssuablePolicy#read_confidential_notes should be updated too condition(:can_read_confidential) do access_level >= Gitlab::Access::REPORTER || @subject.noteable_assignee_or_author?(@user) || admin? end diff --git a/app/policies/todo_policy.rb b/app/policies/todo_policy.rb index 6237fbc50fa..5c24964f24a 100644 --- a/app/policies/todo_policy.rb +++ b/app/policies/todo_policy.rb @@ -5,10 +5,25 @@ class TodoPolicy < BasePolicy condition(:own_todo) do @user && @subject.user_id == @user.id end + + desc "User can read the todo's target" condition(:can_read_target) do @user && @subject.target&.readable_by?(@user) end + desc "Todo has confidential note" + condition(:has_confidential_note, scope: :subject) { @subject&.note&.confidential? } + + desc "User can read the todo's confidential note" + condition(:can_read_todo_confidential_note) do + @user && @user.can?(:read_confidential_notes, @subject.target) + end + rule { own_todo & can_read_target }.enable :read_todo - rule { own_todo & can_read_target }.enable :update_todo + rule { can?(:read_todo) }.enable :update_todo + + rule { has_confidential_note & ~can_read_todo_confidential_note }.policy do + prevent :read_todo + prevent :update_todo + end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 2d47db7b18c..f6afee50e0c 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -990,11 +990,6 @@ msgstr "" msgid "%{start} to %{end}" msgstr "" -msgid "%{strongOpen}%{errors}%{strongClose} point" -msgid_plural "%{strongOpen}%{errors}%{strongClose} points" -msgstr[0] "" -msgstr[1] "" - msgid "%{strongOpen}Warning:%{strongClose} SAML group links can cause GitLab to automatically remove members from groups." msgstr "" @@ -1029,6 +1024,11 @@ msgid_plural "%{strong_start}%{count} members%{strong_end} must approve to merge msgstr[0] "" msgstr[1] "" +msgid "%{strong_start}%{errors}%{strong_end} point" +msgid_plural "%{strong_start}%{errors}%{strong_end} points" +msgstr[0] "" +msgstr[1] "" + msgid "%{strong_start}%{human_size}%{strong_end} Project Storage" msgstr "" diff --git a/spec/policies/issuable_policy_spec.rb b/spec/policies/issuable_policy_spec.rb index 5e2a307e959..1fcf66fe580 100644 --- a/spec/policies/issuable_policy_spec.rb +++ b/spec/policies/issuable_policy_spec.rb @@ -31,6 +31,10 @@ RSpec.describe IssuablePolicy, models: true do expect(policies).to be_allowed(:resolve_note) end + it 'allows reading confidential notes' do + expect(policies).to be_allowed(:read_confidential_notes) + end + context 'when user is able to read project' do it 'enables user to read and update issuables' do expect(policies).to be_allowed(:read_issue, :update_issue, :reopen_issue, :read_merge_request, :update_merge_request, :reopen_merge_request) @@ -86,6 +90,15 @@ RSpec.describe IssuablePolicy, models: true do end end + context 'when user is assignee of issuable' do + let(:issue) { create(:issue, project: project, assignees: [user]) } + let(:policies) { described_class.new(user, issue) } + + it 'allows reading confidential notes' do + expect(policies).to be_allowed(:read_confidential_notes) + end + end + context 'when discussion is locked for the issuable' do let(:issue) { create(:issue, project: project, discussion_locked: true) } diff --git a/spec/policies/todo_policy_spec.rb b/spec/policies/todo_policy_spec.rb index 16435b21666..34ba7bf9276 100644 --- a/spec/policies/todo_policy_spec.rb +++ b/spec/policies/todo_policy_spec.rb @@ -3,53 +3,100 @@ require 'spec_helper' RSpec.describe TodoPolicy do - let_it_be(:author) { create(:user) } - - let_it_be(:user1) { create(:user) } - let_it_be(:user2) { create(:user) } - let_it_be(:user3) { create(:user) } + using RSpec::Parameterized::TableSyntax let_it_be(:project) { create(:project) } let_it_be(:issue) { create(:issue, project: project) } - - let_it_be(:todo1) { create(:todo, author: author, user: user1, issue: issue) } - let_it_be(:todo2) { create(:todo, author: author, user: user2, issue: issue) } - let_it_be(:todo3) { create(:todo, author: author, user: user2) } - let_it_be(:todo4) { create(:todo, author: author, user: user3, issue: issue) } + let_it_be(:author) { create(:user) } def permissions(user, todo) described_class.new(user, todo) end - before_all do - project.add_developer(user1) - project.add_developer(user2) + shared_examples 'grants the expected permissions' do |policy| + it do + if allowed + expect(permissions(user, todo)).to be_allowed(policy) + else + expect(permissions(user, todo)).to be_disallowed(policy) + end + end end describe 'own_todo' do - it 'allows owners to access their own todos if they can read todo target' do - [ - [user1, todo1], - [user2, todo2] - ].each do |user, todo| - expect(permissions(user, todo)).to be_allowed(:read_todo) - end + let_it_be(:user1) { create(:user) } + let_it_be(:user2) { create(:user) } + let_it_be(:user3) { create(:user) } + + let_it_be(:todo1) { create(:todo, author: author, user: user1, issue: issue) } + let_it_be(:todo2) { create(:todo, author: author, user: user2, issue: issue) } + let_it_be(:todo3) { create(:todo, author: author, user: user2) } + let_it_be(:todo4) { create(:todo, author: author, user: user3, issue: issue) } + + where(:user, :todo, :allowed) do + ref(:user1) | ref(:todo1) | true + ref(:user2) | ref(:todo2) | true + ref(:user1) | ref(:todo2) | false + ref(:user1) | ref(:todo3) | false + ref(:user2) | ref(:todo1) | false + ref(:user2) | ref(:todo4) | false + ref(:user3) | ref(:todo1) | false + ref(:user3) | ref(:todo2) | false + ref(:user3) | ref(:todo3) | false + ref(:user3) | ref(:todo4) | false + ref(:user2) | ref(:todo3) | false end - it 'does not allow users to access todos of other users' do - [ - [user1, todo2], - [user1, todo3], - [user2, todo1], - [user2, todo4], - [user3, todo1], - [user3, todo2], - [user3, todo3], - [user2, todo3], - [user3, todo4] - ].each do |user, todo| - expect(permissions(user, todo)).to be_disallowed(:read_todo) - end + before_all do + project.add_developer(user1) + project.add_developer(user2) + end + + with_them do + it_behaves_like 'grants the expected permissions', :read_todo + end + end + + describe 'read_note' do + let_it_be(:non_member) { create(:user) } + let_it_be(:guest) { create(:user) } + let_it_be(:reporter) { create(:user) } + + let_it_be(:note) { create(:note, noteable: issue, project: project) } + let_it_be(:internal) { create(:note, :confidential, noteable: issue, project: project) } + + let_it_be(:no_note_todo1) { create(:todo, author: author, user: reporter, issue: issue) } + let_it_be(:note_todo1) { create(:todo, note: note, author: author, user: reporter, issue: issue) } + let_it_be(:internal_note_todo1) { create(:todo, note: internal, author: author, user: reporter, issue: issue) } + + let_it_be(:no_note_todo2) { create(:todo, author: author, user: guest, issue: issue) } + let_it_be(:note_todo2) { create(:todo, note: note, author: author, user: guest, issue: issue) } + let_it_be(:internal_note_todo2) { create(:todo, note: internal, author: author, user: guest, issue: issue) } + + let_it_be(:no_note_todo3) { create(:todo, author: author, user: non_member, issue: issue) } + let_it_be(:note_todo3) { create(:todo, note: note, author: author, user: non_member, issue: issue) } + let_it_be(:internal_note_todo3) { create(:todo, note: internal, author: author, user: non_member, issue: issue) } + + where(:user, :todo, :allowed) do + ref(:reporter) | ref(:no_note_todo1) | true + ref(:reporter) | ref(:note_todo1) | true + ref(:reporter) | ref(:internal_note_todo1) | true + ref(:guest) | ref(:no_note_todo2) | true + ref(:guest) | ref(:note_todo2) | true + ref(:guest) | ref(:internal_note_todo2) | false + ref(:non_member) | ref(:no_note_todo3) | false + ref(:non_member) | ref(:note_todo3) | false + ref(:non_member) | ref(:internal_note_todo3) | false + end + + before_all do + project.add_guest(guest) + project.add_reporter(reporter) + end + + with_them do + it_behaves_like 'grants the expected permissions', :read_todo + it_behaves_like 'grants the expected permissions', :update_todo end end end diff --git a/spec/requests/api/todos_spec.rb b/spec/requests/api/todos_spec.rb index 0944bfb6ba6..d75ca0d4f6f 100644 --- a/spec/requests/api/todos_spec.rb +++ b/spec/requests/api/todos_spec.rb @@ -224,7 +224,7 @@ RSpec.describe API::Todos do project: project_1, target: create(:design, issue: issue), author: create(:user), - note: create(:note, project: project_1, note: "I am note, hear me roar")) + note: create(:note, :confidential, project: project_1, note: "I am note, hear me roar")) end def api_request diff --git a/spec/services/todos/allowed_target_filter_service_spec.rb b/spec/services/todos/allowed_target_filter_service_spec.rb index 707df8e8514..1d2b1b044db 100644 --- a/spec/services/todos/allowed_target_filter_service_spec.rb +++ b/spec/services/todos/allowed_target_filter_service_spec.rb @@ -10,14 +10,23 @@ RSpec.describe Todos::AllowedTargetFilterService do let_it_be(:unauthorized_group) { create(:group, :private) } let_it_be(:unauthorized_project) { create(:project, group: unauthorized_group) } let_it_be(:user) { create(:user) } + let_it_be(:authorized_issue) { create(:issue, project: authorized_project) } let_it_be(:authorized_issue_todo) { create(:todo, project: authorized_project, target: authorized_issue, user: user) } + let_it_be(:authorized_note) { create(:note, noteable: authorized_issue, project: authorized_project) } + let_it_be(:authorized_note_todo) { create(:todo, project: authorized_project, target: authorized_issue, note: authorized_note, user: user) } + let_it_be(:confidential_issue) { create(:issue, :confidential, project: authorized_project) } + let_it_be(:confidential_issue_todo) { create(:todo, project: authorized_project, target: confidential_issue, user: user) } + let_it_be(:confidential_note) { create(:note, :confidential, noteable: confidential_issue, project: authorized_project) } + let_it_be(:confidential_note_todo) { create(:todo, project: authorized_project, target: authorized_issue, note: confidential_note, user: user) } let_it_be(:unauthorized_issue) { create(:issue, project: unauthorized_project) } let_it_be(:unauthorized_issue_todo) { create(:todo, project: unauthorized_project, target: unauthorized_issue, user: user) } let_it_be(:authorized_design) { create(:design, issue: authorized_issue) } let_it_be(:authorized_design_todo) { create(:todo, project: authorized_project, target: authorized_design, user: user) } let_it_be(:unauthorized_design) { create(:design, issue: unauthorized_issue) } let_it_be(:unauthorized_design_todo) { create(:todo, project: unauthorized_project, target: unauthorized_design, user: user) } + let_it_be(:unauthorized_note) { create(:note, noteable: unauthorized_issue, project: unauthorized_project) } + let_it_be(:unauthorized_note_todo) { create(:todo, project: unauthorized_project, target: unauthorized_issue, note: unauthorized_note, user: user) } # Cannot use let_it_be with MRs let(:authorized_mr) { create(:merge_request, source_project: authorized_project) } @@ -25,35 +34,91 @@ RSpec.describe Todos::AllowedTargetFilterService do let(:unauthorized_mr) { create(:merge_request, source_project: unauthorized_project) } let(:unauthorized_mr_todo) { create(:todo, project: unauthorized_project, user: user, target: unauthorized_mr) } - before_all do - authorized_group.add_developer(user) - end - describe '#execute' do + let(:all_todos) { authorized_todos + unauthorized_todos } + subject(:execute_service) { described_class.new(all_todos, user).execute } - let!(:all_todos) { authorized_todos + unauthorized_todos } + shared_examples 'allowed Todos filter' do + before do + enable_design_management + end - let(:authorized_todos) do - [ - authorized_mr_todo, - authorized_issue_todo, - authorized_design_todo - ] + it { is_expected.to match_array(authorized_todos) } end - let(:unauthorized_todos) do - [ - unauthorized_mr_todo, - unauthorized_issue_todo, - unauthorized_design_todo - ] + context 'with reporter user' do + before_all do + authorized_group.add_reporter(user) + end + + it_behaves_like 'allowed Todos filter' do + let(:authorized_todos) do + [ + authorized_mr_todo, + authorized_issue_todo, + confidential_issue_todo, + confidential_note_todo, + authorized_design_todo + ] + end + + let(:unauthorized_todos) do + [ + unauthorized_mr_todo, + unauthorized_issue_todo, + unauthorized_note_todo, + unauthorized_design_todo + ] + end + end end - before do - enable_design_management + context 'with guest user' do + before_all do + authorized_group.add_guest(user) + end + + it_behaves_like 'allowed Todos filter' do + let(:authorized_todos) do + [ + authorized_issue_todo, + authorized_design_todo + ] + end + + let(:unauthorized_todos) do + [ + authorized_mr_todo, + confidential_issue_todo, + confidential_note_todo, + unauthorized_mr_todo, + unauthorized_issue_todo, + unauthorized_note_todo, + unauthorized_design_todo + ] + end + end end - it { is_expected.to match_array(authorized_todos) } + context 'with a non-member user' do + it_behaves_like 'allowed Todos filter' do + let(:authorized_todos) { [] } + + let(:unauthorized_todos) do + [ + authorized_issue_todo, + authorized_design_todo, + authorized_mr_todo, + confidential_issue_todo, + confidential_note_todo, + unauthorized_mr_todo, + unauthorized_issue_todo, + unauthorized_note_todo, + unauthorized_design_todo + ] + end + end + end end end |