diff options
author | GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com> | 2022-09-29 12:35:27 +0000 |
---|---|---|
committer | GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com> | 2022-09-29 12:35:27 +0000 |
commit | 7b81da9dd7775c048776d8b4acd117e022bce3ce (patch) | |
tree | 3d100d07c2000b13311907b428896771e5753e0c | |
parent | 70d9f335be46efecb1328cd2b7da3f3e17516d7d (diff) | |
parent | 9fae26274b002b81fd17ceedfc90cfbf2690b1bb (diff) | |
download | gitlab-ce-7b81da9dd7775c048776d8b4acd117e022bce3ce.tar.gz |
Merge remote-tracking branch 'dev/15-4-stable' into 15-4-stable
64 files changed, 743 insertions, 181 deletions
diff --git a/.rubocop_todo/style/string_concatenation.yml b/.rubocop_todo/style/string_concatenation.yml index 3dd708d2c49..315ce3701a8 100644 --- a/.rubocop_todo/style/string_concatenation.yml +++ b/.rubocop_todo/style/string_concatenation.yml @@ -265,6 +265,7 @@ Style/StringConcatenation: - 'spec/models/integrations/campfire_spec.rb' - 'spec/models/integrations/chat_message/pipeline_message_spec.rb' - 'spec/models/integrations/chat_message/push_message_spec.rb' + - 'spec/models/integrations/datadog_spec.rb' - 'spec/models/integrations/jenkins_spec.rb' - 'spec/models/merge_request_diff_spec.rb' - 'spec/models/merge_request_spec.rb' diff --git a/CHANGELOG.md b/CHANGELOG.md index cebce972519..981136a2ed2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,26 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 15.4.1 (2022-09-29) + +### Security (15 changes) + +- [Redact user's private email in group member event webhook](gitlab-org/security/gitlab@f556c625f37d1be801b54c5a1ff3dd37434d48e4) ([merge request](gitlab-org/security/gitlab!2809)) +- [Redact secrets from WebHookLogs](gitlab-org/security/gitlab@7101edbc7fc27e2d2d23b8f9f84611943b310b71) ([merge request](gitlab-org/security/gitlab!2805)) +- [Forbid creating a tag using default branch name](gitlab-org/security/gitlab@ba3e62fc30f475b9334440409f5bad481b3c5dd6) ([merge request](gitlab-org/security/gitlab!2798)) +- [Sanitize Url and check for valid numerical errorId in error tracking](gitlab-org/security/gitlab@fba573834091aec7bde7856bfddd080cc74fb3ae) ([merge request](gitlab-org/security/gitlab!2819)) +- [Add security protection for Github](gitlab-org/security/gitlab@6265bdb12496d34f30d9ae6889288c6857fd4fd0) ([merge request](gitlab-org/security/gitlab!2803)) +- [Fix leaking emails in WebHookLogs](gitlab-org/security/gitlab@7580a2d62cd421b5176a3ce7f23c7d192e69989e) ([merge request](gitlab-org/security/gitlab!2806)) +- [Restrict max duration to 1 year for trace display](gitlab-org/security/gitlab@e1162719cc9e62692c911c992175d6ef3b5f996f) ([merge request](gitlab-org/security/gitlab!2817)) +- [Use UntrustedRegexp for upload rewriter](gitlab-org/security/gitlab@fde2bb115242a9af3678e5c8547c7c9ccd2b0c1e) ([merge request](gitlab-org/security/gitlab!2790)) +- [Validate httpUrlToRepo to be http or https only](gitlab-org/security/gitlab@d56ebc1a207618ec846e6ee2c842d3a5019444b7) ([merge request](gitlab-org/security/gitlab!2811)) +- [Respect instance level rule for editing approval rules](gitlab-org/security/gitlab@dc5dd5be3f3f681ca499d3a59eb469bd12dad51b) ([merge request](gitlab-org/security/gitlab!2796)) +- [Prevent users creating issues in ay project via board/issues controller](gitlab-org/security/gitlab@e0b09653ff468b65a73155a2e28077a0e94dc7e8) ([merge request](gitlab-org/security/gitlab!2781)) +- [Prevent serialization of sensible attributes from JsonCache](gitlab-org/security/gitlab@d1842119756b8a69a5d1b14ebd902dc2f4b24dbf) ([merge request](gitlab-org/security/gitlab!2818)) +- [Update TodoPolicy to handle confidential notes](gitlab-org/security/gitlab@cddab943af028c4653dacdd832be5e3e8ac778d3) ([merge request](gitlab-org/security/gitlab!2833)) +- [Enforce group IP restriction on Dependency Proxy](gitlab-org/security/gitlab@fff740c7ab046c5e8ef6495ffa3b45228e11841a) ([merge request](gitlab-org/security/gitlab!2801)) +- [Fixes XSS in widget extensions](gitlab-org/security/gitlab@459becb7a1b0336ddf67f867eecbdf37d579f881) ([merge request](gitlab-org/security/gitlab!2832)) + ## 15.4.0 (2022-09-21) ### Added (162 changes) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 92edc454ca6..65d9b5b1094 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -15.4.0
\ No newline at end of file +15.4.1
\ No newline at end of file @@ -1 +1 @@ -15.4.0
\ No newline at end of file +15.4.1
\ No newline at end of file diff --git a/app/assets/javascripts/error_tracking/components/error_tracking_list.vue b/app/assets/javascripts/error_tracking/components/error_tracking_list.vue index a07428dafea..de4b11699fc 100644 --- a/app/assets/javascripts/error_tracking/components/error_tracking_list.vue +++ b/app/assets/javascripts/error_tracking/components/error_tracking_list.vue @@ -22,12 +22,16 @@ import AccessorUtils from '~/lib/utils/accessor'; import { __ } from '~/locale'; import Tracking from '~/tracking'; import TimeAgo from '~/vue_shared/components/time_ago_tooltip.vue'; +import { sanitizeUrl } from '~/lib/utils/url_utility'; import { trackErrorListViewsOptions, trackErrorStatusUpdateOptions } from '../utils'; import { I18N_ERROR_TRACKING_LIST } from '../constants'; import ErrorTrackingActions from './error_tracking_actions.vue'; export const tableDataClass = 'table-col d-flex d-md-table-cell align-items-center'; +const isValidErrorId = (errorId) => { + return /^[0-9]+$/.test(errorId); +}; export default { FIRST_PAGE: 1, PREV_PAGE: 1, @@ -202,6 +206,9 @@ export default { this.searchByQuery(text); }, getDetailsLink(errorId) { + if (!isValidErrorId(errorId)) { + return 'about:blank'; + } return `error_tracking/${errorId}/details`; }, goToNextPage() { @@ -222,7 +229,10 @@ export default { return filter === this.statusFilter; }, getIssueUpdatePath(errorId) { - return `/${this.projectPath}/-/error_tracking/${errorId}.json`; + if (!isValidErrorId(errorId)) { + return 'about:blank'; + } + return sanitizeUrl(`/${this.projectPath}/-/error_tracking/${errorId}.json`); }, filterErrors(status, label) { this.filterValue = label; 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 52c9f047b76..a10e5efa0e7 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 Actions from '../action_buttons.vue'; import StatusIcon from './status_icon.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); @@ -93,11 +108,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 6896f8831e8..37f9964d23a 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: suiteErrors, summary }) => if (suiteErrors?.base) { errors.push(`${i18n.baseReportParsingError} ${suiteErrors.base}`); } - return errors.join('<br />'); + return errors; } return recentFailuresTextBuilder(summary); }; diff --git a/app/models/concerns/integrations/has_web_hook.rb b/app/models/concerns/integrations/has_web_hook.rb index e6ca6cc7938..5fd71f3d72f 100644 --- a/app/models/concerns/integrations/has_web_hook.rb +++ b/app/models/concerns/integrations/has_web_hook.rb @@ -14,6 +14,11 @@ module Integrations raise NotImplementedError end + # Return the url variables to be used for the webhook. + def url_variables + raise NotImplementedError + end + # Return whether the webhook should use SSL verification. def hook_ssl_verification if respond_to?(:enable_ssl_verification) @@ -26,7 +31,11 @@ module Integrations # Create or update the webhook, raising an exception if it cannot be saved. def update_web_hook! hook = service_hook || build_service_hook - hook.url = hook_url if hook.url != hook_url # avoid reencryption + + # Avoid reencryption + hook.url = hook_url if hook.url != hook_url + hook.url_variables = url_variables if hook.url_variables != url_variables + hook.enable_ssl_verification = hook_ssl_verification hook.save! if hook.changed? hook diff --git a/app/models/concerns/safe_url.rb b/app/models/concerns/safe_url.rb index 7dce05bddba..d6378e6ac6f 100644 --- a/app/models/concerns/safe_url.rb +++ b/app/models/concerns/safe_url.rb @@ -3,13 +3,16 @@ module SafeUrl extend ActiveSupport::Concern + # Return the URL with obfuscated userinfo + # and keeping it intact def safe_url(allowed_usernames: []) return if url.nil? - uri = URI.parse(url) + escaped = Addressable::URI.escape(url) + uri = URI.parse(escaped) uri.password = '*****' if uri.password uri.user = '*****' if uri.user && allowed_usernames.exclude?(uri.user) - uri.to_s - rescue URI::Error + Addressable::URI.unescape(uri.to_s) + rescue URI::Error, TypeError end end diff --git a/app/models/hooks/web_hook_log.rb b/app/models/hooks/web_hook_log.rb index 3fc3f193f19..c32957fbef9 100644 --- a/app/models/hooks/web_hook_log.rb +++ b/app/models/hooks/web_hook_log.rb @@ -22,7 +22,7 @@ class WebHookLog < ApplicationRecord validates :web_hook, presence: true before_save :obfuscate_basic_auth - before_save :redact_author_email + before_save :redact_user_emails def self.recent where(created_at: 2.days.ago.beginning_of_day..Time.zone.now) @@ -54,9 +54,9 @@ class WebHookLog < ApplicationRecord self.url = safe_url end - def redact_author_email - return unless self.request_data.dig('commit', 'author', 'email').present? - - self.request_data['commit']['author']['email'] = _('[REDACTED]') + def redact_user_emails + self.request_data.deep_transform_values! do |value| + value =~ URI::MailTo::EMAIL_REGEXP ? _('[REDACTED]') : value + end end end diff --git a/app/models/integrations/buildkite.rb b/app/models/integrations/buildkite.rb index 7a48e71b934..f2d2aca3ffe 100644 --- a/app/models/integrations/buildkite.rb +++ b/app/models/integrations/buildkite.rb @@ -50,7 +50,11 @@ module Integrations override :hook_url def hook_url - "#{buildkite_endpoint('webhook')}/deliver/#{webhook_token}" + "#{buildkite_endpoint('webhook')}/deliver/{webhook_token}" + end + + def url_variables + { 'webhook_token' => webhook_token } end def execute(data) diff --git a/app/models/integrations/datadog.rb b/app/models/integrations/datadog.rb index 4479725a33b..c9407aa738e 100644 --- a/app/models/integrations/datadog.rb +++ b/app/models/integrations/datadog.rb @@ -154,13 +154,17 @@ module Integrations url = api_url.presence || sprintf(URL_TEMPLATE, datadog_domain: datadog_domain) url = URI.parse(url) query = { - "dd-api-key" => api_key, + "dd-api-key" => 'THIS_VALUE_WILL_BE_REPLACED', service: datadog_service.presence, env: datadog_env.presence, tags: datadog_tags_query_param.presence }.compact url.query = query.to_query - url.to_s + url.to_s.gsub('THIS_VALUE_WILL_BE_REPLACED', '{api_key}') + end + + def url_variables + { 'api_key' => api_key } end def execute(data) diff --git a/app/models/integrations/drone_ci.rb b/app/models/integrations/drone_ci.rb index de69afeba6a..d1a64aa96d4 100644 --- a/app/models/integrations/drone_ci.rb +++ b/app/models/integrations/drone_ci.rb @@ -106,7 +106,11 @@ module Integrations override :hook_url def hook_url - [drone_url, "/hook", "?owner=#{project.namespace.full_path}", "&name=#{project.path}", "&access_token=#{token}"].join + [drone_url, "/hook", "?owner=#{project.namespace.full_path}", "&name=#{project.path}", "&access_token={token}"].join + end + + def url_variables + { 'token' => token } end override :update_web_hook! diff --git a/app/models/integrations/jenkins.rb b/app/models/integrations/jenkins.rb index c68b5fd2a96..74a6449f4f9 100644 --- a/app/models/integrations/jenkins.rb +++ b/app/models/integrations/jenkins.rb @@ -69,6 +69,10 @@ module Integrations url.to_s end + def url_variables + {} + end + def self.supported_events %w(push merge_request tag_push) end diff --git a/app/models/integrations/packagist.rb b/app/models/integrations/packagist.rb index f91404dab23..7177c82a167 100644 --- a/app/models/integrations/packagist.rb +++ b/app/models/integrations/packagist.rb @@ -66,7 +66,11 @@ module Integrations override :hook_url def hook_url base_url = server.presence || 'https://packagist.org' - "#{base_url}/api/update-package?username=#{username}&apiToken=#{token}" + "#{base_url}/api/update-package?username={username}&apiToken={token}" + end + + def url_variables + { 'username' => username, 'token' => token } end end end diff --git a/app/models/user.rb b/app/models/user.rb index 8825c18ea48..3f07e1b1ec0 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -2159,6 +2159,10 @@ class User < ApplicationRecord (Date.current - created_at.to_date).to_i end + def webhook_email + public_email.presence || _('[REDACTED]') + end + protected # override, from Devise::Validatable diff --git a/app/policies/issuable_policy.rb b/app/policies/issuable_policy.rb index e5913bab726..e864ce8752a 100644 --- a/app/policies/issuable_policy.rb +++ b/app/policies/issuable_policy.rb @@ -22,6 +22,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/app/services/base_project_service.rb b/app/services/base_project_service.rb index 1bf4a235a79..8cb6b632a9e 100644 --- a/app/services/base_project_service.rb +++ b/app/services/base_project_service.rb @@ -7,7 +7,9 @@ class BaseProjectService < ::BaseContainerService attr_accessor :project def initialize(project:, current_user: nil, params: {}) - super(container: project, current_user: current_user, params: params) + # we need to exclude project params since they may come from external requests. project should always + # be passed as part of the service's initializer + super(container: project, current_user: current_user, params: params.except(:project, :project_id)) @project = project end diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index bf5be708060..7250ce5c0b0 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -14,7 +14,12 @@ class FileUploader < GitlabUploader include ObjectStorage::Concern prepend ObjectStorage::Extension::RecordsUploads - MARKDOWN_PATTERN = %r{\!?\[.*?\]\(/uploads/(?<secret>[0-9a-f]{32})/(?<file>.*?)\)}.freeze + # This pattern is vulnerable to malicious inputs, so use Gitlab::UntrustedRegexp + # to place bounds on execution time + MARKDOWN_PATTERN = Gitlab::UntrustedRegexp.new( + '!?\[.*?\]\(/uploads/(?P<secret>[0-9a-f]{32})/(?P<file>.*?)\)' + ) + DYNAMIC_PATH_PATTERN = %r{.*(?<secret>\b(\h{10}|\h{32}))\/(?<identifier>.*)}.freeze VALID_SECRET_PATTERN = %r{\A\h{10,32}\z}.freeze diff --git a/lib/bulk_imports/common/pipelines/wiki_pipeline.rb b/lib/bulk_imports/common/pipelines/wiki_pipeline.rb index 6900835b14d..0f92c1f1210 100644 --- a/lib/bulk_imports/common/pipelines/wiki_pipeline.rb +++ b/lib/bulk_imports/common/pipelines/wiki_pipeline.rb @@ -22,7 +22,7 @@ module BulkImports wiki = context.portable.wiki url = data[:url].sub("://", "://oauth2:#{context.configuration.access_token}@") - Gitlab::UrlBlocker.validate!(url, allow_local_network: allow_local_requests?, allow_localhost: allow_local_requests?) + Gitlab::UrlBlocker.validate!(url, schemes: %w[http https], allow_local_network: allow_local_requests?, allow_localhost: allow_local_requests?) wiki.ensure_repository wiki.repository.fetch_as_mirror(url) diff --git a/lib/bulk_imports/projects/pipelines/repository_pipeline.rb b/lib/bulk_imports/projects/pipelines/repository_pipeline.rb index f5ccc1dd922..a2b1f8c5176 100644 --- a/lib/bulk_imports/projects/pipelines/repository_pipeline.rb +++ b/lib/bulk_imports/projects/pipelines/repository_pipeline.rb @@ -21,7 +21,7 @@ module BulkImports url = url.sub("://", "://oauth2:#{context.configuration.access_token}@") project = context.portable - Gitlab::UrlBlocker.validate!(url, allow_local_network: allow_local_requests?, allow_localhost: allow_local_requests?) + Gitlab::UrlBlocker.validate!(url, schemes: %w[http https], allow_local_network: allow_local_requests?, allow_localhost: allow_local_requests?) project.ensure_repository project.repository.fetch_as_mirror(url) diff --git a/lib/bulk_imports/projects/pipelines/snippets_repository_pipeline.rb b/lib/bulk_imports/projects/pipelines/snippets_repository_pipeline.rb index 6d423717a51..e29601927be 100644 --- a/lib/bulk_imports/projects/pipelines/snippets_repository_pipeline.rb +++ b/lib/bulk_imports/projects/pipelines/snippets_repository_pipeline.rb @@ -55,7 +55,9 @@ module BulkImports Gitlab::UrlBlocker.validate!( url, allow_local_network: allow_local_requests?, - allow_localhost: allow_local_requests?) + allow_localhost: allow_local_requests?, + schemes: %w[http https] + ) end def cleanup_snippet_repository(snippet) diff --git a/lib/error_tracking/sentry_client.rb b/lib/error_tracking/sentry_client.rb index 029389ab5d6..713cec7a7d6 100644 --- a/lib/error_tracking/sentry_client.rb +++ b/lib/error_tracking/sentry_client.rb @@ -10,6 +10,7 @@ module ErrorTracking Error = Class.new(StandardError) MissingKeysError = Class.new(StandardError) + InvalidFieldValueError = Class.new(StandardError) ResponseInvalidSizeError = Class.new(StandardError) RESPONSE_SIZE_LIMIT = 1.megabyte @@ -110,5 +111,15 @@ module ErrorTracking def raise_error(message) raise SentryClient::Error, message end + + def ensure_numeric!(field, value) + return value if /\A\d+\z/.match?(value) + + raise_invalid_field_value!(field, "#{value.inspect} is not numeric") + end + + def raise_invalid_field_value!(field, message) + raise InvalidFieldValueError, %(Sentry API response contains invalid value for field "#{field}": #{message}) + end end end diff --git a/lib/error_tracking/sentry_client/event.rb b/lib/error_tracking/sentry_client/event.rb index 1db31abeeb2..d8ae81f5411 100644 --- a/lib/error_tracking/sentry_client/event.rb +++ b/lib/error_tracking/sentry_client/event.rb @@ -16,7 +16,7 @@ module ErrorTracking Gitlab::ErrorTracking::ErrorEvent.new( project_id: event['projectID'], - issue_id: event['groupID'], + issue_id: ensure_numeric!('issue_id', event['groupID']), date_received: event['dateReceived'], stack_trace_entries: stack_trace ) diff --git a/lib/error_tracking/sentry_client/issue.rb b/lib/error_tracking/sentry_client/issue.rb index 3c846eb0635..5e2e0787a3f 100644 --- a/lib/error_tracking/sentry_client/issue.rb +++ b/lib/error_tracking/sentry_client/issue.rb @@ -114,8 +114,10 @@ module ErrorTracking end def map_to_error(issue) + id = ensure_numeric!('id', issue.fetch('id')) + Gitlab::ErrorTracking::Error.new( - id: issue.fetch('id'), + id: id, first_seen: issue.fetch('firstSeen', nil), last_seen: issue.fetch('lastSeen', nil), title: issue.fetch('title', nil), @@ -124,7 +126,7 @@ module ErrorTracking count: issue.fetch('count', nil), message: issue.dig('metadata', 'value'), culprit: issue.fetch('culprit', nil), - external_url: issue_url(issue.fetch('id')), + external_url: issue_url(id), short_id: issue.fetch('shortId', nil), status: issue.fetch('status', nil), frequency: issue.dig('stats', '24h'), @@ -135,8 +137,10 @@ module ErrorTracking end def map_to_detailed_error(issue) + id = ensure_numeric!('id', issue.fetch('id')) + Gitlab::ErrorTracking::DetailedError.new( - id: issue.fetch('id'), + id: id, first_seen: issue.fetch('firstSeen', nil), last_seen: issue.fetch('lastSeen', nil), tags: extract_tags(issue), @@ -146,7 +150,7 @@ module ErrorTracking count: issue.fetch('count', nil), message: issue.dig('metadata', 'value'), culprit: issue.fetch('culprit', nil), - external_url: issue_url(issue.fetch('id')), + external_url: issue_url(id), external_base_url: project_url, short_id: issue.fetch('shortId', nil), status: issue.fetch('status', nil), diff --git a/lib/gitlab/checks/tag_check.rb b/lib/gitlab/checks/tag_check.rb index 5dd7720b67d..007a775eaf5 100644 --- a/lib/gitlab/checks/tag_check.rb +++ b/lib/gitlab/checks/tag_check.rb @@ -9,11 +9,13 @@ module Gitlab delete_protected_tag: 'You are not allowed to delete protected tags from this project. '\ 'Only a project maintainer or owner can delete a protected tag.', delete_protected_tag_non_web: 'You can only delete protected tags using the web interface.', - create_protected_tag: 'You are not allowed to create this tag as it is protected.' + create_protected_tag: 'You are not allowed to create this tag as it is protected.', + default_branch_collision: 'You cannot use default branch name to create a tag' }.freeze LOG_MESSAGES = { tag_checks: "Checking if you are allowed to change existing tags...", + default_branch_collision_check: "Checking if you are providing a valid tag name...", protected_tag_checks: "Checking if you are creating, updating or deleting a protected tag..." }.freeze @@ -26,6 +28,7 @@ module Gitlab end end + default_branch_collision_check protected_tag_checks end @@ -52,6 +55,14 @@ module Gitlab end end end + + def default_branch_collision_check + logger.log_timed(LOG_MESSAGES[:default_branch_collision_check]) do + if creation? && tag_name == project.default_branch + raise GitAccess::ForbiddenError, ERROR_MESSAGES[:default_branch_collision] + end + end + end end end end diff --git a/lib/gitlab/ci/ansi2json/line.rb b/lib/gitlab/ci/ansi2json/line.rb index e48080993ab..abe2f272ca7 100644 --- a/lib/gitlab/ci/ansi2json/line.rb +++ b/lib/gitlab/ci/ansi2json/line.rb @@ -80,7 +80,8 @@ module Gitlab end def set_section_duration(duration_in_seconds) - duration = ActiveSupport::Duration.build(duration_in_seconds.to_i) + normalized_duration_in_seconds = duration_in_seconds.to_i.clamp(0, 1.year) + duration = ActiveSupport::Duration.build(normalized_duration_in_seconds) hours = duration.in_hours.floor hours = hours > 0 ? "%02d" % hours : nil minutes = "%02d" % duration.parts[:minutes].to_i diff --git a/lib/gitlab/gfm/uploads_rewriter.rb b/lib/gitlab/gfm/uploads_rewriter.rb index b0bf68f4204..58b46a85aae 100644 --- a/lib/gitlab/gfm/uploads_rewriter.rb +++ b/lib/gitlab/gfm/uploads_rewriter.rb @@ -23,33 +23,24 @@ module Gitlab def rewrite(target_parent) return @text unless needs_rewrite? - @text.gsub!(@pattern) do |markdown| - file = find_file($~[:secret], $~[:file]) - # No file will be returned for a path traversal - next if file.nil? + @target_parent = target_parent - break markdown unless file.try(:exists?) - - klass = target_parent.is_a?(Namespace) ? NamespaceFileUploader : FileUploader - moved = klass.copy_to(file, target_parent) - - moved_markdown = moved.markdown_link - - # Prevents rewrite of plain links as embedded - if was_embedded?(markdown) - moved_markdown - else - moved_markdown.delete_prefix('!') - end + rewritten_text = Gitlab::StringRegexMarker.new(@text).mark(@pattern) do |markdown, left:, right:, mode:| + transform_markdown(markdown) end + + # MarkdownContentRewriterService relies on the text being changed _in place_. + @text.gsub!(@text, rewritten_text) end def needs_rewrite? strong_memoize(:needs_rewrite) do - FileUploader::MARKDOWN_PATTERN.match?(@text) + @pattern.match?(@text) end end + private + def was_embedded?(markdown) markdown.starts_with?("!") end @@ -57,6 +48,28 @@ module Gitlab def find_file(secret, file_name) UploaderFinder.new(@source_project, secret, file_name).execute end + + def transform_markdown(markdown) + match = @pattern.match(markdown) + file = find_file(match[:secret], match[:file]) + + # No file will be returned for a path traversal + return '' if file.nil? + + return markdown unless file.try(:exists?) + + klass = @target_parent.is_a?(Namespace) ? NamespaceFileUploader : FileUploader + moved = klass.copy_to(file, @target_parent) + + moved_markdown = moved.markdown_link + + # Prevents rewrite of plain links as embedded + if was_embedded?(markdown) + moved_markdown + else + moved_markdown.delete_prefix('!') + end + end end end end diff --git a/lib/gitlab/hook_data/group_member_builder.rb b/lib/gitlab/hook_data/group_member_builder.rb index 2998550a4b5..d70885018e9 100644 --- a/lib/gitlab/hook_data/group_member_builder.rb +++ b/lib/gitlab/hook_data/group_member_builder.rb @@ -39,7 +39,7 @@ module Gitlab group_id: group_member.group.id, user_username: group_member.user.username, user_name: group_member.user.name, - user_email: group_member.user.email, + user_email: group_member.user.webhook_email, user_id: group_member.user.id, group_access: group_member.human_access, expires_at: group_member.expires_at&.xmlschema diff --git a/lib/gitlab/json_cache.rb b/lib/gitlab/json_cache.rb index d5c018cfc68..d2916a01809 100644 --- a/lib/gitlab/json_cache.rb +++ b/lib/gitlab/json_cache.rb @@ -43,9 +43,7 @@ module Gitlab end def write(key, value, options = nil) - # As we use json as the serialization format, return everything from - # ActiveModel objects included encrypted values. - backend.write(cache_key(key), value.to_json(unsafe_serialization_hash: true), options) + backend.write(cache_key(key), value.to_json, options) end def fetch(key, options = {}, &block) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 8654dc7f0ed..4569a1b447b 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1024,11 +1024,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 "" @@ -1065,6 +1060,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/controllers/boards/issues_controller_spec.rb b/spec/controllers/boards/issues_controller_spec.rb index 1fd249eba69..3e1cdfccc61 100644 --- a/spec/controllers/boards/issues_controller_spec.rb +++ b/spec/controllers/boards/issues_controller_spec.rb @@ -427,6 +427,22 @@ RSpec.describe Boards::IssuesController do end describe 'POST create' do + context 'when trying to create issue on an unauthorized project' do + let(:unauthorized_project) { create(:project, :private) } + let(:issue_params) { { project_id: unauthorized_project.id } } + + it 'creates the issue on the board\'s project' do + expect do + create_issue user: user, board: board, list: list1, title: 'New issue', additional_issue_params: issue_params + end.to change(Issue, :count).by(1) + + created_issue = Issue.last + + expect(created_issue.project).to eq(project) + expect(unauthorized_project.reload.issues.count).to eq(0) + end + end + context 'with valid params' do before do create_issue user: user, board: board, list: list1, title: 'New issue' @@ -500,13 +516,13 @@ RSpec.describe Boards::IssuesController do end end - def create_issue(user:, board:, list:, title:) + def create_issue(user:, board:, list:, title:, additional_issue_params: {}) sign_in(user) post :create, params: { board_id: board.to_param, list_id: list.to_param, - issue: { title: title, project_id: project.id } + issue: { title: title, project_id: project.id }.merge(additional_issue_params) }, format: :json end diff --git a/spec/frontend/error_tracking/components/error_tracking_list_spec.js b/spec/frontend/error_tracking/components/error_tracking_list_spec.js index 805ada54509..adb2eaaf04e 100644 --- a/spec/frontend/error_tracking/components/error_tracking_list_spec.js +++ b/spec/frontend/error_tracking/components/error_tracking_list_spec.js @@ -314,6 +314,43 @@ describe('ErrorTrackingList', () => { }); }); + describe('when the resolve button is clicked with non numberic error id', () => { + beforeEach(() => { + store.state.list.loading = false; + store.state.list.errors = [ + { + id: 'abc', + title: 'PG::ConnectionBad: FATAL', + type: 'error', + userCount: 0, + count: '53', + firstSeen: '2019-05-30T07:21:46Z', + lastSeen: '2019-11-06T03:21:39Z', + status: 'unresolved', + }, + ]; + + mountComponent({ + stubs: { + GlTable: false, + GlLink: false, + }, + }); + }); + + it('should show about:blank link', () => { + findErrorActions().vm.$emit('update-issue-status', { + errorId: 'abc', + status: 'resolved', + }); + + expect(actions.updateStatus).toHaveBeenCalledWith(expect.anything(), { + endpoint: 'about:blank', + status: 'resolved', + }); + }); + }); + describe('When error tracking is disabled and user is not allowed to enable it', () => { beforeEach(() => { mountComponent({ diff --git a/spec/lib/bulk_imports/projects/pipelines/repository_pipeline_spec.rb b/spec/lib/bulk_imports/projects/pipelines/repository_pipeline_spec.rb index 38b22538e70..a968104fc91 100644 --- a/spec/lib/bulk_imports/projects/pipelines/repository_pipeline_spec.rb +++ b/spec/lib/bulk_imports/projects/pipelines/repository_pipeline_spec.rb @@ -20,8 +20,9 @@ RSpec.describe BulkImports::Projects::Pipelines::RepositoryPipeline do ) end - let_it_be(:tracker) { create(:bulk_import_tracker, entity: entity) } - let_it_be(:context) { BulkImports::Pipeline::Context.new(tracker) } + let_it_be_with_reload(:tracker) { create(:bulk_import_tracker, entity: entity) } + + let(:context) { BulkImports::Pipeline::Context.new(tracker) } let(:extracted_data) { BulkImports::Pipeline::ExtractedData.new(data: project_data) } @@ -61,7 +62,7 @@ RSpec.describe BulkImports::Projects::Pipelines::RepositoryPipeline do context 'blocked local networks' do let(:project_data) { { 'httpUrlToRepo' => 'http://localhost/foo.git' } } - it 'imports new repository into destination project' do + it 'prevents import' do allow(Gitlab.config.gitlab).to receive(:host).and_return('notlocalhost.gitlab.com') allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_web_hooks_and_services?).and_return(false) @@ -70,6 +71,18 @@ RSpec.describe BulkImports::Projects::Pipelines::RepositoryPipeline do expect(context.entity.failed?).to eq(true) end end + + context 'when scheme is blocked' do + let(:project_data) { { 'httpUrlToRepo' => 'file://example/tmp/foo.git' } } + + it 'prevents import' do + pipeline.run + + expect(context.entity.failed?).to eq(true) + expect(context.entity.failures.first).to be_present + expect(context.entity.failures.first.exception_message).to eq('Only allowed schemes are http, https') + end + end end describe '#after_run' do diff --git a/spec/lib/bulk_imports/projects/pipelines/snippets_repository_pipeline_spec.rb b/spec/lib/bulk_imports/projects/pipelines/snippets_repository_pipeline_spec.rb index 4d12b49e2c0..dfd01cdf4bb 100644 --- a/spec/lib/bulk_imports/projects/pipelines/snippets_repository_pipeline_spec.rb +++ b/spec/lib/bulk_imports/projects/pipelines/snippets_repository_pipeline_spec.rb @@ -135,9 +135,25 @@ RSpec.describe BulkImports::Projects::Pipelines::SnippetsRepositoryPipeline do end context 'when url is invalid' do - let(:http_url_to_repo) { 'http://0.0.0.0' } + context 'when not a real URL' do + let(:http_url_to_repo) { 'http://0.0.0.0' } - it_behaves_like 'skippable snippet' + it_behaves_like 'skippable snippet' + end + + context 'when scheme is blocked' do + let(:http_url_to_repo) { 'file://example.com/foo/bar/snippets/42.git' } + + it_behaves_like 'skippable snippet' + + it 'logs the failure' do + pipeline.run + + expect(tracker.failed?).to eq(true) + expect(tracker.entity.failures.first).to be_present + expect(tracker.entity.failures.first.exception_message).to eq('Only allowed schemes are http, https') + end + end end context 'when snippet is invalid' do diff --git a/spec/lib/error_tracking/sentry_client/event_spec.rb b/spec/lib/error_tracking/sentry_client/event_spec.rb index d65bfa31018..e7a9db771b6 100644 --- a/spec/lib/error_tracking/sentry_client/event_spec.rb +++ b/spec/lib/error_tracking/sentry_client/event_spec.rb @@ -72,5 +72,13 @@ RSpec.describe ErrorTracking::SentryClient do end end end + + it_behaves_like 'non-numeric input handling in Sentry response', 'issue_id' do + let(:sentry_api_response) do + sample_response.tap do |event| + event[:groupID] = id_input + end + end + end end end diff --git a/spec/lib/error_tracking/sentry_client/issue_spec.rb b/spec/lib/error_tracking/sentry_client/issue_spec.rb index 1468a1ff7eb..ac6a4b9e8cd 100644 --- a/spec/lib/error_tracking/sentry_client/issue_spec.rb +++ b/spec/lib/error_tracking/sentry_client/issue_spec.rb @@ -199,6 +199,15 @@ RSpec.describe ErrorTracking::SentryClient::Issue do it_behaves_like 'issues have correct return type', Gitlab::ErrorTracking::Error it_behaves_like 'issues have correct length', 3 end + + it_behaves_like 'non-numeric input handling in Sentry response', 'id' do + let(:sentry_api_response) do + issues_sample_response.first(1).map do |issue| + issue[:id] = id_input + issue + end + end + end end describe '#issue_details' do @@ -208,8 +217,8 @@ RSpec.describe ErrorTracking::SentryClient::Issue do ) end - let(:sentry_request_url) { "#{sentry_url}/issues/#{issue_id}/" } let(:sentry_api_response) { issue_sample_response } + let(:sentry_request_url) { "#{sentry_url}/issues/#{issue_id}/" } let!(:sentry_api_request) { stub_sentry_request(sentry_request_url, body: sentry_api_response) } subject { client.issue_details(issue_id: issue_id) } @@ -298,6 +307,14 @@ RSpec.describe ErrorTracking::SentryClient::Issue do expect(subject.tags).to eq({ level: issue_sample_response['level'], logger: issue_sample_response['logger'] }) end end + + it_behaves_like 'non-numeric input handling in Sentry response', 'id' do + let(:sentry_api_response) do + issue_sample_response.tap do |issue| + issue[:id] = id_input + end + end + end end describe '#update_issue' do diff --git a/spec/lib/gitlab/checks/tag_check_spec.rb b/spec/lib/gitlab/checks/tag_check_spec.rb index 6cd3a2d1c07..50ffa5fad10 100644 --- a/spec/lib/gitlab/checks/tag_check_spec.rb +++ b/spec/lib/gitlab/checks/tag_check_spec.rb @@ -81,6 +81,14 @@ RSpec.describe Gitlab::Checks::TagCheck do it 'allows tag creation' do expect { subject.validate! }.not_to raise_error end + + context 'when tag name is the same as default branch' do + let(:ref) { "refs/tags/#{project.default_branch}" } + + it 'is prevented' do + expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, /cannot use default branch name to create a tag/) + end + end end end end diff --git a/spec/lib/gitlab/ci/ansi2json/line_spec.rb b/spec/lib/gitlab/ci/ansi2json/line_spec.rb index d16750d19f1..b8563bb1d1c 100644 --- a/spec/lib/gitlab/ci/ansi2json/line_spec.rb +++ b/spec/lib/gitlab/ci/ansi2json/line_spec.rb @@ -87,6 +87,8 @@ RSpec.describe Gitlab::Ci::Ansi2json::Line do 1.minute + 15.seconds | '01:15' 13.hours + 14.minutes + 15.seconds | '13:14:15' 1.day + 13.hours + 14.minutes + 15.seconds | '37:14:15' + Float::MAX | '8765:00:00' + 10**10000 | '8765:00:00' end with_them do diff --git a/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb b/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb index a16f96a7d11..b1bff242f33 100644 --- a/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb +++ b/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb @@ -23,8 +23,9 @@ RSpec.describe Gitlab::Gfm::UploadsRewriter do end def referenced_files(text, project) - referenced_files = text.scan(FileUploader::MARKDOWN_PATTERN).map do - UploaderFinder.new(project, $~[:secret], $~[:file]).execute + scanner = FileUploader::MARKDOWN_PATTERN.scan(text) + referenced_files = scanner.map do |match| + UploaderFinder.new(project, match[0], match[1]).execute end referenced_files.compact.select(&:exists?) @@ -32,7 +33,9 @@ RSpec.describe Gitlab::Gfm::UploadsRewriter do shared_examples "files are accessible" do describe '#rewrite' do - let!(:new_text) { rewriter.rewrite(new_project) } + subject(:rewrite) { new_text } + + let(:new_text) { rewriter.rewrite(new_project) } let(:old_files) { [image_uploader, zip_uploader] } let(:new_files) do @@ -43,11 +46,15 @@ RSpec.describe Gitlab::Gfm::UploadsRewriter do let(:new_paths) { new_files.map(&:path) } it 'rewrites content' do + rewrite + expect(new_text).not_to eq text expect(new_text.length).to eq text.length end it 'copies files' do + rewrite + expect(new_files).to all(exist) expect(old_paths).not_to match_array new_paths expect(old_paths).to all(include(old_project.disk_path)) @@ -55,10 +62,14 @@ RSpec.describe Gitlab::Gfm::UploadsRewriter do end it 'does not remove old files' do + rewrite + expect(old_files).to all(exist) end it 'generates a new secret for each file' do + rewrite + expect(new_paths).not_to include image_uploader.secret expect(new_paths).not_to include zip_uploader.secret end @@ -68,6 +79,8 @@ RSpec.describe Gitlab::Gfm::UploadsRewriter do allow(finder).to receive(:execute).and_return(nil) end + rewrite + expect(new_files).to be_empty end end @@ -84,6 +97,16 @@ RSpec.describe Gitlab::Gfm::UploadsRewriter do expect(moved_text.scan(/\A\[.*?\]/).count).to eq(1) end + it 'does not casue a timeout on pathological text' do + text = '[!l' * 30000 + + Timeout.timeout(3) do + moved_text = described_class.new(text, nil, old_project, user).rewrite(new_project) + + expect(moved_text).to eq(text) + end + end + context "file are stored locally" do include_examples "files are accessible" end diff --git a/spec/lib/gitlab/hook_data/group_member_builder_spec.rb b/spec/lib/gitlab/hook_data/group_member_builder_spec.rb index 35ce31ab897..1551bdbaa9c 100644 --- a/spec/lib/gitlab/hook_data/group_member_builder_spec.rb +++ b/spec/lib/gitlab/hook_data/group_member_builder_spec.rb @@ -26,7 +26,7 @@ RSpec.describe Gitlab::HookData::GroupMemberBuilder do expect(data[:group_id]).to eq(group.id) expect(data[:user_username]).to eq(group_member.user.username) expect(data[:user_name]).to eq(group_member.user.name) - expect(data[:user_email]).to eq(group_member.user.email) + expect(data[:user_email]).to eq(group_member.user.webhook_email) expect(data[:user_id]).to eq(group_member.user.id) expect(data[:group_access]).to eq('Developer') expect(data[:created_at]).to eq(group_member.created_at&.xmlschema) diff --git a/spec/models/concerns/integrations/has_web_hook_spec.rb b/spec/models/concerns/integrations/has_web_hook_spec.rb new file mode 100644 index 00000000000..9061cb90f90 --- /dev/null +++ b/spec/models/concerns/integrations/has_web_hook_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Integrations::HasWebHook do + let(:integration_class) do + Class.new(Integration) do + include Integrations::HasWebHook + end + end + + let(:integration) { integration_class.new } + + context 'when hook_url and url_variables are not implemented' do + it { expect { integration.hook_url }.to raise_error(NotImplementedError) } + it { expect { integration.url_variables }.to raise_error(NotImplementedError) } + end + + context 'when integration does not respond to enable_ssl_verification' do + it { expect(integration.hook_ssl_verification).to eq true } + end + + context 'when integration responds to enable_ssl_verification' do + let(:integration) { build(:drone_ci_integration) } + + it { expect(integration.hook_ssl_verification).to eq true } + end +end diff --git a/spec/models/hooks/web_hook_log_spec.rb b/spec/models/hooks/web_hook_log_spec.rb index 8ff8a1c3865..3441dfda7d6 100644 --- a/spec/models/hooks/web_hook_log_spec.rb +++ b/spec/models/hooks/web_hook_log_spec.rb @@ -44,26 +44,49 @@ RSpec.describe WebHookLog do end end - context 'with author email' do + context "with users' emails" do let(:author) { create(:user) } + let(:user) { create(:user) } let(:web_hook_log) { create(:web_hook_log, request_data: data) } let(:data) do { - commit: { - author: { - name: author.name, - email: author.email + user: { + name: user.name, + email: user.email + }, + commits: [ + { + user: { + name: author.name, + email: author.email + } + }, + { + user: { + name: user.name, + email: user.email + } } - } + ] }.deep_stringify_keys end - it "redacts author's email" do - expect(web_hook_log.request_data['commit']).to match a_hash_including( - 'author' => { - 'name' => author.name, - 'email' => _('[REDACTED]') - } + it "redacts users' emails" do + expect(web_hook_log.request_data['user']).to match a_hash_including( + 'name' => user.name, + 'email' => _('[REDACTED]') + ) + expect(web_hook_log.request_data['commits'].pluck('user')).to match_array( + [ + { + 'name' => author.name, + 'email' => _('[REDACTED]') + }, + { + 'name' => user.name, + 'email' => _('[REDACTED]') + } + ] ) end end diff --git a/spec/models/integrations/buildkite_spec.rb b/spec/models/integrations/buildkite_spec.rb index 38ceb5db49c..c720dc6d418 100644 --- a/spec/models/integrations/buildkite_spec.rb +++ b/spec/models/integrations/buildkite_spec.rb @@ -19,7 +19,7 @@ RSpec.describe Integrations::Buildkite, :use_clean_rails_memory_store_caching do end it_behaves_like Integrations::HasWebHook do - let(:hook_url) { 'https://webhook.buildkite.com/deliver/secret-sauce-webhook-token' } + let(:hook_url) { 'https://webhook.buildkite.com/deliver/{webhook_token}' } end describe 'Validations' do @@ -68,7 +68,7 @@ RSpec.describe Integrations::Buildkite, :use_clean_rails_memory_store_caching do describe '#hook_url' do it 'returns the webhook url' do expect(integration.hook_url).to eq( - 'https://webhook.buildkite.com/deliver/secret-sauce-webhook-token' + 'https://webhook.buildkite.com/deliver/{webhook_token}' ) end end diff --git a/spec/models/integrations/datadog_spec.rb b/spec/models/integrations/datadog_spec.rb index 4ac684e8ff0..b7da6a79e44 100644 --- a/spec/models/integrations/datadog_spec.rb +++ b/spec/models/integrations/datadog_spec.rb @@ -19,6 +19,7 @@ RSpec.describe Integrations::Datadog do let(:dd_tags) { '' } let(:expected_hook_url) { default_url + "?dd-api-key=#{api_key}&env=#{dd_env}&service=#{dd_service}" } + let(:hook_url) { default_url + "?dd-api-key={api_key}&env=#{dd_env}&service=#{dd_service}" } let(:instance) do described_class.new( @@ -48,7 +49,7 @@ RSpec.describe Integrations::Datadog do it_behaves_like Integrations::HasWebHook do let(:integration) { instance } - let(:hook_url) { "#{described_class::URL_TEMPLATE % { datadog_domain: dd_site }}?dd-api-key=#{api_key}&env=#{dd_env}&service=#{dd_service}" } + let(:hook_url) { "#{described_class::URL_TEMPLATE % { datadog_domain: dd_site }}?dd-api-key={api_key}&env=#{dd_env}&service=#{dd_service}" } end describe 'validations' do @@ -132,18 +133,18 @@ RSpec.describe Integrations::Datadog do subject { instance.hook_url } context 'with standard site URL' do - it { is_expected.to eq(expected_hook_url) } + it { is_expected.to eq(hook_url) } end context 'with custom URL' do let(:api_url) { 'https://webhook-intake.datad0g.com/api/v2/webhook' } - it { is_expected.to eq(api_url + "?dd-api-key=#{api_key}&env=#{dd_env}&service=#{dd_service}") } + it { is_expected.to eq(api_url + "?dd-api-key={api_key}&env=#{dd_env}&service=#{dd_service}") } context 'blank' do let(:api_url) { '' } - it { is_expected.to eq(expected_hook_url) } + it { is_expected.to eq(hook_url) } end end @@ -152,19 +153,19 @@ RSpec.describe Integrations::Datadog do let(:dd_env) { '' } let(:dd_tags) { '' } - it { is_expected.to eq(default_url + "?dd-api-key=#{api_key}") } + it { is_expected.to eq(default_url + "?dd-api-key={api_key}") } end context 'with custom tags' do let(:dd_tags) { "key:value\nkey2:value, 2" } let(:escaped_tags) { CGI.escape("key:value,\"key2:value, 2\"") } - it { is_expected.to eq(expected_hook_url + "&tags=#{escaped_tags}") } + it { is_expected.to eq(hook_url + "&tags=#{escaped_tags}") } context 'and empty lines' do let(:dd_tags) { "key:value\r\n\n\n\nkey2:value, 2\n" } - it { is_expected.to eq(expected_hook_url + "&tags=#{escaped_tags}") } + it { is_expected.to eq(hook_url + "&tags=#{escaped_tags}") } end end end diff --git a/spec/models/integrations/drone_ci_spec.rb b/spec/models/integrations/drone_ci_spec.rb index 905fee075ad..f3203a6e69d 100644 --- a/spec/models/integrations/drone_ci_spec.rb +++ b/spec/models/integrations/drone_ci_spec.rb @@ -116,7 +116,7 @@ RSpec.describe Integrations::DroneCi, :use_clean_rails_memory_store_caching do include_context :drone_ci_integration let(:integration) { drone } - let(:hook_url) { "#{drone_url}/hook?owner=#{project.namespace.full_path}&name=#{project.path}&access_token=#{token}" } + let(:hook_url) { "#{drone_url}/hook?owner=#{project.namespace.full_path}&name=#{project.path}&access_token={token}" } it 'does not create a hook if project is not present' do integration.project = nil diff --git a/spec/models/integrations/packagist_spec.rb b/spec/models/integrations/packagist_spec.rb index d1976e73e2e..e078debd126 100644 --- a/spec/models/integrations/packagist_spec.rb +++ b/spec/models/integrations/packagist_spec.rb @@ -26,7 +26,7 @@ RSpec.describe Integrations::Packagist do it_behaves_like Integrations::HasWebHook do let(:integration) { described_class.new(packagist_params) } - let(:hook_url) { "#{packagist_server}/api/update-package?username=#{packagist_username}&apiToken=#{packagist_token}" } + let(:hook_url) { "#{packagist_server}/api/update-package?username={username}&apiToken={token}" } end it_behaves_like Integrations::ResetSecretFields do diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 04f2c7f9176..3cc34681ad6 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -6866,6 +6866,25 @@ RSpec.describe User do end end + describe '#webhook_email' do + let(:user) { build(:user, public_email: nil) } + + context 'when public email is present' do + before do + user.public_email = "hello@hello.com" + end + it 'returns public email' do + expect(user.webhook_email).to eq(user.public_email) + end + end + + context 'when public email is nil' do + it 'returns [REDACTED]' do + expect(user.webhook_email).to eq(_('[REDACTED]')) + end + end + end + describe 'user credit card validation' do context 'when user is initialized' do let(:user) { build(:user) } diff --git a/spec/policies/issuable_policy_spec.rb b/spec/policies/issuable_policy_spec.rb index fd7ec5917d6..c02294571ff 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) } @@ -138,6 +151,10 @@ RSpec.describe IssuablePolicy, models: true do it 'does not allow timelogs creation' do expect(permissions(guest, issue)).to be_disallowed(:create_timelog) end + + it 'does not allow reading confidential notes' do + expect(permissions(guest, issue)).to be_disallowed(:read_confidential_notes) + end end context 'when user is a guest member of the project and the author of the issuable' do @@ -152,6 +169,10 @@ RSpec.describe IssuablePolicy, models: true do it 'allows timelogs creation' do expect(permissions(reporter, issue)).to be_allowed(:create_timelog) end + + it 'allows reading confidential notes' do + expect(permissions(reporter, issue)).to be_allowed(:read_confidential_notes) + end end context 'when subject is a Merge Request' do 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/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index 4a84862b9d5..3d52dc07c4f 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -47,6 +47,19 @@ RSpec.describe Issues::CreateService do due_date: Date.tomorrow } end + context 'when an unauthorized project_id is provided' do + let(:unauthorized_project) { create(:project) } + + before do + opts[:project_id] = unauthorized_project.id + end + + it 'ignores the project_id param and creates issue in the given project' do + expect(issue.project).to eq(project) + expect(unauthorized_project.reload.issues.count).to eq(0) + end + end + it 'works if base work item types were not created yet' do WorkItems::Type.delete_all diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 8a2e9ed74f7..634a4206d48 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -69,6 +69,23 @@ RSpec.describe Issues::UpdateService, :mailer do } end + context 'when an unauthorized project_id is provided' do + let(:unauthorized_project) { create(:project) } + + before do + opts[:project_id] = unauthorized_project.id + end + + it 'ignores the project_id param and does not update the issue\'s project' do + expect do + update_issue(opts) + unauthorized_project.reload + end.to not_change { unauthorized_project.issues.count } + + expect(issue.project).to eq(project) + end + end + it 'updates the issue with the given params' do expect(TodosDestroyer::ConfidentialIssueWorker).not_to receive(:perform_in) diff --git a/spec/services/notes/copy_service_spec.rb b/spec/services/notes/copy_service_spec.rb index f146a49e929..2fa9a462bb9 100644 --- a/spec/services/notes/copy_service_spec.rb +++ b/spec/services/notes/copy_service_spec.rb @@ -146,8 +146,10 @@ RSpec.describe Notes::CopyService do new_note = to_noteable.notes.first aggregate_failures do - expect(note.note).to match(/Simple text with image: #{FileUploader::MARKDOWN_PATTERN}/o) - expect(new_note.note).to match(/Simple text with image: #{FileUploader::MARKDOWN_PATTERN}/o) + expect(note.note).to match(/Simple text with image:/o) + expect(FileUploader::MARKDOWN_PATTERN.match(note.note)).not_to be_nil + expect(new_note.note).to match(/Simple text with image:/o) + expect(FileUploader::MARKDOWN_PATTERN.match(new_note.note)).not_to be_nil expect(note.note).not_to eq(new_note.note) expect(note.note_html).not_to eq(new_note.note_html) end 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 diff --git a/spec/support/shared_examples/bulk_imports/common/pipelines/wiki_pipeline_examples.rb b/spec/support/shared_examples/bulk_imports/common/pipelines/wiki_pipeline_examples.rb index 06800f7cded..7e7460cd602 100644 --- a/spec/support/shared_examples/bulk_imports/common/pipelines/wiki_pipeline_examples.rb +++ b/spec/support/shared_examples/bulk_imports/common/pipelines/wiki_pipeline_examples.rb @@ -4,8 +4,9 @@ RSpec.shared_examples 'wiki pipeline imports a wiki for an entity' do describe '#run' do let_it_be(:bulk_import_configuration) { create(:bulk_import_configuration, bulk_import: bulk_import) } - let_it_be(:tracker) { create(:bulk_import_tracker, entity: entity) } - let_it_be(:context) { BulkImports::Pipeline::Context.new(tracker) } + let_it_be_with_reload(:tracker) { create(:bulk_import_tracker, entity: entity) } + + let(:context) { BulkImports::Pipeline::Context.new(tracker) } let(:extracted_data) { BulkImports::Pipeline::ExtractedData.new(data: {}) } @@ -40,5 +41,21 @@ RSpec.shared_examples 'wiki pipeline imports a wiki for an entity' do expect { subject.run }.not_to raise_error end end + + context 'when scheme is blocked' do + it 'prevents import' do + # Force bulk_import_configuration to have a file:// URL + bulk_import_configuration.url = 'file://example.com' + bulk_import_configuration.save!(validate: false) + + expect(subject).to receive(:source_wiki_exists?).and_return(true) + + subject.run + + expect(tracker.failed?).to eq(true) + expect(tracker.entity.failures.first).to be_present + expect(tracker.entity.failures.first.exception_message).to eq('Only allowed schemes are http, https') + end + end end end diff --git a/spec/support/shared_examples/controllers/concerns/web_hooks/integrations_hook_log_actions_shared_examples.rb b/spec/support/shared_examples/controllers/concerns/web_hooks/integrations_hook_log_actions_shared_examples.rb index 62c9c3508a8..56a5dcb10b2 100644 --- a/spec/support/shared_examples/controllers/concerns/web_hooks/integrations_hook_log_actions_shared_examples.rb +++ b/spec/support/shared_examples/controllers/concerns/web_hooks/integrations_hook_log_actions_shared_examples.rb @@ -26,7 +26,7 @@ RSpec.shared_examples WebHooks::HookLogActions do describe 'POST #retry' do it 'executes the hook and redirects to the service form' do - stub_request(:post, web_hook.url) + stub_request(:post, web_hook.interpolated_url) expect_next_found_instance_of(web_hook.class) do |hook| expect(hook).to receive(:execute).and_call_original diff --git a/spec/support/shared_examples/lib/sentry/client_shared_examples.rb b/spec/support/shared_examples/lib/sentry/client_shared_examples.rb index 1c0e0061385..71b32005c55 100644 --- a/spec/support/shared_examples/lib/sentry/client_shared_examples.rb +++ b/spec/support/shared_examples/lib/sentry/client_shared_examples.rb @@ -59,6 +59,22 @@ RSpec.shared_examples 'maps Sentry exceptions' do |http_method| end end +RSpec.shared_examples 'non-numeric input handling in Sentry response' do |field| + context 'with non-numeric error id' do + where(:id_input) do + ['string', '-1', '1\n2'] + end + + with_them do + it 'raises exception' do + message = %(Sentry API response contains invalid value for field "#{field}": #{id_input.inspect} is not numeric) + + expect { subject }.to raise_error(ErrorTracking::SentryClient::InvalidFieldValueError, message) + end + end + end +end + # Expects to following variables: # - subject # - sentry_api_response diff --git a/spec/support/shared_examples/models/integrations/has_web_hook_shared_examples.rb b/spec/support/shared_examples/models/integrations/has_web_hook_shared_examples.rb index 2f693edeb53..e309aa50c6e 100644 --- a/spec/support/shared_examples/models/integrations/has_web_hook_shared_examples.rb +++ b/spec/support/shared_examples/models/integrations/has_web_hook_shared_examples.rb @@ -37,6 +37,12 @@ RSpec.shared_examples Integrations::HasWebHook do end end + describe '#url_variables' do + it 'returns a string' do + expect(integration.url_variables).to be_a(Hash) + end + end + describe '#hook_ssl_verification' do it 'returns a boolean' do expect(integration.hook_ssl_verification).to be_in([true, false]) |