diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d9176fca4..e3120bcb48 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,24 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 14.2.6 (2021-10-28) + +### Security (13 changes) + +- [Highlight usage of unicode bidi characters](gitlab-org/security/gitlab@18a768bb3cd19b6dc780bb85d91a93605ec8aa4f) ([merge request](gitlab-org/security/gitlab!1939)) +- [Fix dompurify.js to prevent path traversal attacks](gitlab-org/security/gitlab@cfd7c715162c22060b9b80268ef501a9e604421a) ([merge request](gitlab-org/security/gitlab!1931)) +- [Refresh authorizations on transfer of groups having project shares](gitlab-org/security/gitlab@3fc08eb869156a090b015e78da79c8ced16a7162) ([merge request](gitlab-org/security/gitlab!1918)) +- [Do not allow Applications API to create apps with blank scopes](gitlab-org/security/gitlab@c4ffc8c0ee5356bcb9b76dbfa92517589b4225a8) ([merge request](gitlab-org/security/gitlab!1924)) +- [Don't allow author to resolve discussions when MR is locked via GraphQL](gitlab-org/security/gitlab@fe2d0b6f250b60619da97f162c93c9e645daf4af) ([merge request](gitlab-org/security/gitlab!1921)) +- [Workhorse: Allow uploading only a single file](gitlab-org/security/gitlab@89b04599592b7dfc0e4883cfde5d3ecd9ea855b2) ([merge request](gitlab-org/security/gitlab!1915)) +- [Group owners should see SCIM token only once](gitlab-org/security/gitlab@d52c1e41f38039db075a7a3418b8eb9ed8474c2a) ([merge request](gitlab-org/security/gitlab!1908)) **GitLab Enterprise Edition** +- [Respect visibility level settings when updating project via API](gitlab-org/security/gitlab@3051d6a00d1a56133a77ecd24313bafb4565d576) ([merge request](gitlab-org/security/gitlab!1905)) +- [Avoid decoding the whole tiff image on isTIFF check](gitlab-org/security/gitlab@bab7f45def8fc81fe4b0961a21b4c90a60358ff9) ([merge request](gitlab-org/security/gitlab!1901)) +- [Adding a '[redacted]' to mask private email addresses](gitlab-org/security/gitlab@8eb9749f40b87b9b49b034bceb263219a4d3b114) ([merge request](gitlab-org/security/gitlab!1895)) +- [Do not display the root password by default](gitlab-org/security/gitlab@4ccf08b6645b9f616657edd266d9d31e3602d170) ([merge request](gitlab-org/security/gitlab!1802)) +- [Set PipelineSchedules to inactive](gitlab-org/security/gitlab@ebee16945325d22ceb5c07b7ba48df6fd0b2f067) ([merge request](gitlab-org/security/gitlab!1878)) +- [Remove external_webhook_token from exported project](gitlab-org/security/gitlab@f3ef12185902f3ed5c9d62ffce07418fd704a753) ([merge request](gitlab-org/security/gitlab!1865)) + ## 14.2.5 (2021-09-30) ### Security (28 changes) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 997fcf30e1..d14841bd9a 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -14.2.5 \ No newline at end of file +14.2.6 \ No newline at end of file diff --git a/VERSION b/VERSION index 997fcf30e1..d14841bd9a 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -14.2.5 \ No newline at end of file +14.2.6 \ No newline at end of file diff --git a/app/assets/javascripts/lib/dompurify.js b/app/assets/javascripts/lib/dompurify.js index a026f76e51..c375a99724 100644 --- a/app/assets/javascripts/lib/dompurify.js +++ b/app/assets/javascripts/lib/dompurify.js @@ -1,5 +1,5 @@ import { sanitize as dompurifySanitize, addHook } from 'dompurify'; -import { getBaseURL, relativePathToAbsolute } from '~/lib/utils/url_utility'; +import { getNormalizedURL, getBaseURL, relativePathToAbsolute } from '~/lib/utils/url_utility'; const defaultConfig = { // Safely allow SVG tags @@ -11,12 +11,14 @@ const defaultConfig = { // Only icons urls from `gon` are allowed const getAllowedIconUrls = (gon = window.gon) => - [gon.sprite_file_icons, gon.sprite_icons].filter(Boolean); + [gon.sprite_file_icons, gon.sprite_icons] + .filter(Boolean) + .map((path) => relativePathToAbsolute(path, getBaseURL())); -const isUrlAllowed = (url) => getAllowedIconUrls().some((allowedUrl) => url.startsWith(allowedUrl)); +const isUrlAllowed = (url) => + getAllowedIconUrls().some((allowedUrl) => getNormalizedURL(url).startsWith(allowedUrl)); -const isHrefSafe = (url) => - isUrlAllowed(url) || isUrlAllowed(relativePathToAbsolute(url, getBaseURL())); +const isHrefSafe = (url) => url.match(/^#/) || isUrlAllowed(url); const removeUnsafeHref = (node, attr) => { if (!node.hasAttribute(attr)) { @@ -36,13 +38,14 @@ const removeUnsafeHref = (node, attr) => { * * * + * It validates both href & xlink:href attributes. + * Note that `xlink:href` is deprecated, but still in use + * https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/xlink:href + * * @param {Object} node - Node to sanitize */ const sanitizeSvgIcon = (node) => { removeUnsafeHref(node, 'href'); - - // Note: `xlink:href` is deprecated, but still in use - // https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/xlink:href removeUnsafeHref(node, 'xlink:href'); }; diff --git a/app/assets/javascripts/lib/utils/url_utility.js b/app/assets/javascripts/lib/utils/url_utility.js index e9772232ea..c10e102292 100644 --- a/app/assets/javascripts/lib/utils/url_utility.js +++ b/app/assets/javascripts/lib/utils/url_utility.js @@ -397,6 +397,24 @@ export function isSafeURL(url) { } } +/** + * Returns a normalized url + * + * https://gitlab.com/foo/../baz => https://gitlab.com/baz + * + * @param {String} url - URL to be transformed + * @param {String?} baseUrl - current base URL + * @returns {String} + */ +export const getNormalizedURL = (url, baseUrl) => { + const base = baseUrl || getBaseURL(); + try { + return new URL(url, base).href; + } catch (e) { + return ''; + } +}; + export function getWebSocketProtocol() { return window.location.protocol.replace('http', 'ws'); } diff --git a/app/assets/javascripts/pages/projects/shared/permissions/components/settings_panel.vue b/app/assets/javascripts/pages/projects/shared/permissions/components/settings_panel.vue index 261f7af7ef..c53d367ed7 100644 --- a/app/assets/javascripts/pages/projects/shared/permissions/components/settings_panel.vue +++ b/app/assets/javascripts/pages/projects/shared/permissions/components/settings_panel.vue @@ -37,6 +37,10 @@ export default { securityAndComplianceLabel: s__('ProjectSettings|Security & Compliance'), snippetsLabel: s__('ProjectSettings|Snippets'), wikiLabel: s__('ProjectSettings|Wiki'), + pucWarningLabel: s__('ProjectSettings|Warn about Potentially Unwanted Characters'), + pucWarningHelpText: s__( + 'ProjectSettings|Highlight the usage of hidden unicode characters. These have innocent uses for right-to-left languages, but can also be used in potential exploits.', + ), }, components: { @@ -178,6 +182,7 @@ export default { securityAndComplianceAccessLevel: featureAccessLevel.PROJECT_MEMBERS, operationsAccessLevel: featureAccessLevel.EVERYONE, containerRegistryAccessLevel: featureAccessLevel.EVERYONE, + warnAboutPotentiallyUnwantedCharacters: true, lfsEnabled: true, requestAccessEnabled: true, highlightChangesClass: false, @@ -752,5 +757,19 @@ export default { }} + + + + {{ $options.i18n.pucWarningLabel }} + + + diff --git a/app/assets/javascripts/snippets/components/show.vue b/app/assets/javascripts/snippets/components/show.vue index 46629a569e..35d88d5ec8 100644 --- a/app/assets/javascripts/snippets/components/show.vue +++ b/app/assets/javascripts/snippets/components/show.vue @@ -66,7 +66,13 @@ export default { data-qa-selector="clone_button" /> - + diff --git a/app/assets/stylesheets/framework/highlight.scss b/app/assets/stylesheets/framework/highlight.scss index b4a1d9f997..122c605e60 100644 --- a/app/assets/stylesheets/framework/highlight.scss +++ b/app/assets/stylesheets/framework/highlight.scss @@ -85,3 +85,9 @@ td.line-numbers { line-height: 1; } + +.project-highlight-puc .unicode-bidi::before { + content: '�'; + cursor: pointer; + text-decoration: underline wavy $red-500; +} diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index b0e690a74c..a3a0d1e2ca 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -398,6 +398,7 @@ class ProjectsController < Projects::ApplicationController show_default_award_emojis squash_option mr_default_target_self + warn_about_potentially_unwanted_characters ] end diff --git a/app/graphql/mutations/issues/set_severity.rb b/app/graphql/mutations/issues/set_severity.rb index 778563ba05..872a0e7b33 100644 --- a/app/graphql/mutations/issues/set_severity.rb +++ b/app/graphql/mutations/issues/set_severity.rb @@ -8,6 +8,8 @@ module Mutations argument :severity, Types::IssuableSeverityEnum, required: true, description: 'Set the incident severity level.' + authorize :admin_issue + def resolve(project_path:, iid:, severity:) issue = authorized_find!(project_path: project_path, iid: iid) project = issue.project diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index f30223f6f1..fc6d675246 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -377,6 +377,12 @@ module ProjectsHelper } end + def project_classes(project) + return "project-highlight-puc" if project.warn_about_potentially_unwanted_characters? + + "" + end + private def tab_ability_map @@ -533,6 +539,7 @@ module ProjectsHelper metricsDashboardAccessLevel: feature.metrics_dashboard_access_level, operationsAccessLevel: feature.operations_access_level, showDefaultAwardEmojis: project.show_default_award_emojis?, + warnAboutPotentiallyUnwantedCharacters: project.warn_about_potentially_unwanted_characters?, securityAndComplianceAccessLevel: project.security_and_compliance_access_level, containerRegistryAccessLevel: feature.container_registry_access_level } diff --git a/app/models/concerns/resolvable_discussion.rb b/app/models/concerns/resolvable_discussion.rb index 3e1e5faee5..60e1dde17b 100644 --- a/app/models/concerns/resolvable_discussion.rb +++ b/app/models/concerns/resolvable_discussion.rb @@ -81,8 +81,7 @@ module ResolvableDiscussion return false unless current_user return false unless resolvable? - current_user == self.noteable.try(:author) || - current_user.can?(:resolve_note, self.project) + current_user.can?(:resolve_note, self.noteable) end def resolve!(current_user) diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 261639a4ec..1f903eba71 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -26,6 +26,8 @@ class Namespace < ApplicationRecord SHARED_RUNNERS_SETTINGS = %w[disabled_and_unoverridable disabled_with_override enabled].freeze URL_MAX_LENGTH = 255 + PATH_TRAILING_VIOLATIONS = %w[.git .atom .].freeze + cache_markdown_field :description, pipeline: :description has_many :projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent @@ -162,9 +164,14 @@ class Namespace < ApplicationRecord # Remove everything that's not in the list of allowed characters. path.gsub!(/[^a-zA-Z0-9_\-\.]/, "") # Remove trailing violations ('.atom', '.git', or '.') - path.gsub!(/(\.atom|\.git|\.)*\z/, "") + loop do + orig = path + PATH_TRAILING_VIOLATIONS.each { |ext| path = path.chomp(ext) } + break if orig == path + end + # Remove leading violations ('-') - path.gsub!(/\A\-+/, "") + path.gsub!(/\A\-+/, "") # Users with the great usernames of "." or ".." would end up with a blank username. # Work around that by setting their username to "blank", followed by a counter. diff --git a/app/models/project.rb b/app/models/project.rb index 81b04e1316..67fc53c5ff 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -412,8 +412,8 @@ class Project < ApplicationRecord :container_registry_access_level, :container_registry_enabled?, to: :project_feature, allow_nil: true alias_method :container_registry_enabled, :container_registry_enabled? - delegate :show_default_award_emojis, :show_default_award_emojis=, - :show_default_award_emojis?, + delegate :show_default_award_emojis, :show_default_award_emojis=, :show_default_award_emojis?, + :warn_about_potentially_unwanted_characters, :warn_about_potentially_unwanted_characters=, :warn_about_potentially_unwanted_characters?, to: :project_setting, allow_nil: true delegate :scheduled?, :started?, :in_progress?, :failed?, :finished?, prefix: :import, to: :import_state, allow_nil: true @@ -2669,8 +2669,23 @@ class Project < ApplicationRecord ci_cd_settings.group_runners_enabled? end + def visible_group_links(for_user:) + user = for_user + links = project_group_links_with_preload + user.max_member_access_for_group_ids(links.map(&:group_id)) if user && links.any? + + DeclarativePolicy.user_scope do + links.select { Ability.allowed?(user, :read_group, _1.group) } + end + end + private + # overridden in EE + def project_group_links_with_preload + project_group_links + end + def find_integration(integrations, name) integrations.find { _1.to_param == name } end diff --git a/app/models/project_group_link.rb b/app/models/project_group_link.rb index d704f4c2c8..8394ebe1df 100644 --- a/app/models/project_group_link.rb +++ b/app/models/project_group_link.rb @@ -15,6 +15,7 @@ class ProjectGroupLink < ApplicationRecord validate :different_group scope :non_guests, -> { where('group_access > ?', Gitlab::Access::GUEST) } + scope :in_group, -> (group_ids) { where(group_id: group_ids) } alias_method :shared_with_group, :group diff --git a/app/models/user.rb b/app/models/user.rb index 08b25d7a6f..3eb4035c89 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1459,7 +1459,7 @@ class User < ApplicationRecord name: name, username: username, avatar_url: avatar_url(only_path: false), - email: email + email: public_email.presence || _('[REDACTED]') } end diff --git a/app/policies/issuable_policy.rb b/app/policies/issuable_policy.rb index 61263e47d7..39ce26526e 100644 --- a/app/policies/issuable_policy.rb +++ b/app/policies/issuable_policy.rb @@ -11,6 +11,8 @@ class IssuablePolicy < BasePolicy @user && @subject.assignee_or_author?(@user) end + condition(:is_author) { @subject&.author == @user } + rule { can?(:guest_access) & assignee_or_author }.policy do enable :read_issue enable :update_issue @@ -20,6 +22,10 @@ class IssuablePolicy < BasePolicy enable :reopen_merge_request end + rule { is_author }.policy do + enable :resolve_note + end + rule { locked & ~is_project_member }.policy do prevent :create_note prevent :admin_note diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 54b11ea604..b40bfbf5a7 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -221,6 +221,7 @@ class ProjectPolicy < BasePolicy enable :set_note_created_at enable :set_emails_disabled enable :set_show_default_award_emojis + enable :set_warn_about_potentially_unwanted_characters end rule { can?(:guest_access) }.policy do diff --git a/app/services/concerns/update_visibility_level.rb b/app/services/concerns/update_visibility_level.rb index b7a161f508..4cd14a2fb5 100644 --- a/app/services/concerns/update_visibility_level.rb +++ b/app/services/concerns/update_visibility_level.rb @@ -1,13 +1,17 @@ # frozen_string_literal: true module UpdateVisibilityLevel + # check that user is allowed to set specified visibility_level def valid_visibility_level_change?(target, new_visibility) - # check that user is allowed to set specified visibility_level - if new_visibility && new_visibility.to_i != target.visibility_level - unless can?(current_user, :change_visibility_level, target) && - Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility) + return true unless new_visibility - deny_visibility_level(target, new_visibility) + new_visibility_level = Gitlab::VisibilityLevel.level_value(new_visibility) + + if new_visibility_level != target.visibility_level_value + unless can?(current_user, :change_visibility_level, target) && + Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility_level) + + deny_visibility_level(target, new_visibility_level) return false end end diff --git a/app/services/groups/transfer_service.rb b/app/services/groups/transfer_service.rb index b7eae06b96..d0aea848af 100644 --- a/app/services/groups/transfer_service.rb +++ b/app/services/groups/transfer_service.rb @@ -177,6 +177,14 @@ module Groups # schedule refreshing projects for all the members of the group @group.refresh_members_authorized_projects + + # When a group is transferred, it also affects who gets access to the projects shared to + # the subgroups within its hierarchy, so we also schedule jobs that refresh authorizations for all such shared projects. + project_group_shares_within_the_hierarchy = ProjectGroupLink.in_group(group.self_and_descendants.select(:id)) + + project_group_shares_within_the_hierarchy.find_each do |project_group_link| + AuthorizedProjectUpdate::ProjectRecalculateWorker.perform_async(project_group_link.project_id) + end end def raise_transfer_error(message) diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb index 1ad43b051b..2d6334251a 100644 --- a/app/services/groups/update_service.rb +++ b/app/services/groups/update_service.rb @@ -15,7 +15,7 @@ module Groups return false end - return false unless valid_visibility_level_change?(group, params[:visibility_level]) + return false unless valid_visibility_level_change?(group, group.visibility_attribute_value(params)) return false unless valid_share_with_group_lock_change? @@ -77,7 +77,7 @@ module Groups end def after_update - if group.previous_changes.include?(:visibility_level) && group.private? + if group.previous_changes.include?(group.visibility_level_field) && group.private? # don't enqueue immediately to prevent todos removal in case of a mistake TodosDestroyer::GroupPrivateWorker.perform_in(Todo::WAIT_FOR_DELETE, group.id) end diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 0984238517..4532d46ddd 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -139,6 +139,7 @@ class IssuableBaseService < ::BaseProjectService def filter_severity(issuable) severity = params.delete(:severity) return unless severity && issuable.supports_severity? + return unless can_admin_issuable?(issuable) severity = IssuableSeverity::DEFAULT unless IssuableSeverity.severities.key?(severity) return if severity == issuable.severity diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index d6e7f165d7..621068febf 100644 --- a/app/services/projects/update_service.rb +++ b/app/services/projects/update_service.rb @@ -49,7 +49,7 @@ module Projects private def validate! - unless valid_visibility_level_change?(project, params[:visibility_level]) + unless valid_visibility_level_change?(project, project.visibility_attribute_value(params)) raise ValidationError, s_('UpdateProject|New visibility level not allowed!') end diff --git a/app/views/layouts/project.html.haml b/app/views/layouts/project.html.haml index 2df502d289..a54e0351d2 100644 --- a/app/views/layouts/project.html.haml +++ b/app/views/layouts/project.html.haml @@ -6,6 +6,7 @@ - display_subscription_banner! - display_namespace_storage_limit_alert! - @left_sidebar = true +- @content_class = [@content_class, project_classes(@project)].compact.join(" ") - content_for :project_javascripts do - project = @target_project || @project diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index f7b1c2f756..36eeb7f851 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -176,8 +176,10 @@ production: &base ## Application settings cache expiry in seconds (default: 60) # application_settings_cache_seconds: 60 - ## Print initial root password to stdout during initialization (default: true) - # display_initial_root_password: true + ## Print initial root password to stdout during initialization (default: false) + # WARNING: setting this to true means that the root password will be printed in + # plaintext. This can be a security risk. + # display_initial_root_password: false ## Reply by email # Allow users to comment on issues and merge requests by replying to notification emails. diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index e71f1e1b02..6d61755f03 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -217,8 +217,7 @@ Settings.gitlab['no_todos_messages'] ||= YAML.load_file(Rails.root.join('config' Settings.gitlab['impersonation_enabled'] ||= true if Settings.gitlab['impersonation_enabled'].nil? Settings.gitlab['usage_ping_enabled'] = true if Settings.gitlab['usage_ping_enabled'].nil? Settings.gitlab['max_request_duration_seconds'] ||= 57 - -Settings.gitlab['display_initial_root_password'] = true if Settings.gitlab['display_initial_root_password'].nil? +Settings.gitlab['display_initial_root_password'] = false if Settings.gitlab['display_initial_root_password'].nil? Gitlab.ee do Settings.gitlab['mirror_max_delay'] ||= 300 diff --git a/db/fixtures/production/002_admin.rb b/db/fixtures/production/002_admin.rb index b6a6da3a18..b4710bc3e9 100644 --- a/db/fixtures/production/002_admin.rb +++ b/db/fixtures/production/002_admin.rb @@ -26,7 +26,7 @@ if user.persisted? if ::Settings.gitlab['display_initial_root_password'] puts "password: #{user_args[:password]}".color(:green) else - puts "password: *** - You opted not to display initial root password to STDOUT." + puts "password: ******".color(:green) end else puts "password: You'll be prompted to create one on your first visit.".color(:green) diff --git a/db/migrate/20210929144453_add_warn_about_potentially_unwanted_characters_to_project_settings.rb b/db/migrate/20210929144453_add_warn_about_potentially_unwanted_characters_to_project_settings.rb new file mode 100644 index 0000000000..a2d1ea5779 --- /dev/null +++ b/db/migrate/20210929144453_add_warn_about_potentially_unwanted_characters_to_project_settings.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +# See https://docs.gitlab.com/ee/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddWarnAboutPotentiallyUnwantedCharactersToProjectSettings < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + with_lock_retries do + add_column :project_settings, :warn_about_potentially_unwanted_characters, :boolean, null: false, default: true + end + end + + def down + with_lock_retries do + remove_column :project_settings, :warn_about_potentially_unwanted_characters + end + end +end diff --git a/db/schema_migrations/20210929144453 b/db/schema_migrations/20210929144453 new file mode 100644 index 0000000000..753ea50c27 --- /dev/null +++ b/db/schema_migrations/20210929144453 @@ -0,0 +1 @@ +0f808c27d19e6a38d4aa31f2dd820fe226681af84e05c4af47213409b2043e5a \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index eee73636eb..d4023d06be 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -17302,6 +17302,7 @@ CREATE TABLE project_settings ( cve_id_request_enabled boolean DEFAULT true NOT NULL, mr_default_target_self boolean DEFAULT false NOT NULL, previous_default_branch text, + warn_about_potentially_unwanted_characters boolean DEFAULT true NOT NULL, CONSTRAINT check_3a03e7557a CHECK ((char_length(previous_default_branch) <= 4096)), CONSTRAINT check_bde223416c CHECK ((show_default_award_emojis IS NOT NULL)) ); diff --git a/lib/api/applications.rb b/lib/api/applications.rb index be482272b2..346bd6ccfe 100644 --- a/lib/api/applications.rb +++ b/lib/api/applications.rb @@ -15,7 +15,7 @@ module API params do requires :name, type: String, desc: 'Application name' requires :redirect_uri, type: String, desc: 'Application redirect URI' - requires :scopes, type: String, desc: 'Application scopes' + requires :scopes, type: String, desc: 'Application scopes', allow_blank: false optional :confidential, type: Boolean, default: true, desc: 'Application will be used where the client secret is confidential' diff --git a/lib/api/entities/project.rb b/lib/api/entities/project.rb index 890b42ed8c..e8e6935a28 100644 --- a/lib/api/entities/project.rb +++ b/lib/api/entities/project.rb @@ -100,7 +100,9 @@ module API expose :build_coverage_regex expose :ci_config_path, if: -> (project, options) { Ability.allowed?(options[:current_user], :download_code, project) } expose :shared_with_groups do |project, options| - SharedGroupWithProject.represent(project.project_group_links, options) + user = options[:current_user] + + SharedGroupWithProject.represent(project.visible_group_links(for_user: user), options) end expose :only_allow_merge_if_pipeline_succeeds expose :allow_merge_on_skipped_pipeline diff --git a/lib/api/helpers/projects_helpers.rb b/lib/api/helpers/projects_helpers.rb index becd25595a..30edbe9112 100644 --- a/lib/api/helpers/projects_helpers.rb +++ b/lib/api/helpers/projects_helpers.rb @@ -39,6 +39,7 @@ module API optional :emails_disabled, type: Boolean, desc: 'Disable email notifications' optional :show_default_award_emojis, type: Boolean, desc: 'Show default award emojis' + optional :warn_about_potentially_unwanted_characters, type: Boolean, desc: 'Warn about Potentially Unwanted Characters' optional :shared_runners_enabled, type: Boolean, desc: 'Flag indication if shared runners are enabled for that project' optional :resolve_outdated_diff_discussions, type: Boolean, desc: 'Automatically resolve merge request diffs discussions on lines changed with a push' optional :remove_source_branch_after_merge, type: Boolean, desc: 'Remove the source branch by default after merge' diff --git a/lib/api/project_import.rb b/lib/api/project_import.rb index 039f7b4be4..6e9b7fe4ce 100644 --- a/lib/api/project_import.rb +++ b/lib/api/project_import.rb @@ -10,6 +10,8 @@ module API feature_category :importers + before { authenticate! unless route.settings[:skip_authentication] } + helpers do def import_params declared_params(include_missing: false) @@ -110,6 +112,7 @@ module API detail 'This feature was introduced in GitLab 10.6.' success Entities::ProjectImportStatus end + route_setting :skip_authentication, true get ':id/import' do present user_project, with: Entities::ProjectImportStatus end diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 3ffedd46c3..2ea7248d03 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -423,7 +423,7 @@ module API authorize_admin_project attrs = declared_params(include_missing: false) authorize! :rename_project, user_project if attrs[:name].present? - authorize! :change_visibility_level, user_project if attrs[:visibility].present? + authorize! :change_visibility_level, user_project if user_project.visibility_attribute_present?(attrs) attrs = translate_params_for_compatibility(attrs) filter_attributes_using_license!(attrs) diff --git a/lib/gitlab/background_migration/user_mentions/models/group.rb b/lib/gitlab/background_migration/user_mentions/models/group.rb index a8b4b59b06..310723570c 100644 --- a/lib/gitlab/background_migration/user_mentions/models/group.rb +++ b/lib/gitlab/background_migration/user_mentions/models/group.rb @@ -11,6 +11,10 @@ module Gitlab has_one :saml_provider + def root_saml_provider + root_ancestor.saml_provider + end + def self.declarative_policy_class "GroupPolicy" end diff --git a/lib/gitlab/import_export/project/import_export.yml b/lib/gitlab/import_export/project/import_export.yml index eac671a3a0..d0cb362134 100644 --- a/lib/gitlab/import_export/project/import_export.yml +++ b/lib/gitlab/import_export/project/import_export.yml @@ -171,9 +171,11 @@ excluded_attributes: - :marked_for_deletion_by_user_id - :compliance_framework_setting - :show_default_award_emojis + - :warn_about_potentially_unwanted_characters - :services - :exported_protected_branches - :repository_size_limit + - :external_webhook_token namespaces: - :runners_token - :runners_token_encrypted @@ -377,6 +379,8 @@ excluded_attributes: system_note_metadata: - :description_version_id - :note_id + pipeline_schedules: + - :active methods: notes: - :type diff --git a/lib/gitlab/import_export/project/relation_factory.rb b/lib/gitlab/import_export/project/relation_factory.rb index 102fcedd2f..888a5a10f2 100644 --- a/lib/gitlab/import_export/project/relation_factory.rb +++ b/lib/gitlab/import_export/project/relation_factory.rb @@ -84,6 +84,7 @@ module Gitlab when :'Ci::Pipeline' then setup_pipeline when *BUILD_MODELS then setup_build when :issues then setup_issue + when :'Ci::PipelineSchedule' then setup_pipeline_schedule end update_project_references @@ -143,6 +144,10 @@ module Gitlab @relation_hash['relative_position'] = compute_relative_position end + def setup_pipeline_schedule + @relation_hash['active'] = false + end + def compute_relative_position return unless max_relative_position diff --git a/lib/gitlab/unicode.rb b/lib/gitlab/unicode.rb new file mode 100644 index 0000000000..b49c5647da --- /dev/null +++ b/lib/gitlab/unicode.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module Gitlab + class Unicode + # Regular expression for identifying bidirectional control + # characters in UTF-8 strings + # + # Documentation on how this works: + # https://idiosyncratic-ruby.com/41-proper-unicoding.html + BIDI_REGEXP = /\p{Bidi Control}/.freeze + + class << self + # Warning message used to highlight bidi characters in the GUI + def bidi_warning + _("Potentially unwanted character detected: Unicode BiDi Control") + end + end + end +end diff --git a/lib/gitlab/visibility_level.rb b/lib/gitlab/visibility_level.rb index 64029d4d3f..d378e558b8 100644 --- a/lib/gitlab/visibility_level.rb +++ b/lib/gitlab/visibility_level.rb @@ -155,6 +155,14 @@ module Gitlab false end + def visibility_attribute_value(attributes) + visibility_level_attributes.each do |attr| + return attributes[attr] if attributes.has_key?(attr) + end + + nil + end + def visibility_level_attributes [visibility_level_field, visibility_level_field.to_s, :visibility, 'visibility'] diff --git a/lib/rouge/formatters/html_gitlab.rb b/lib/rouge/formatters/html_gitlab.rb index e0e9677fac..9e76225fc5 100644 --- a/lib/rouge/formatters/html_gitlab.rb +++ b/lib/rouge/formatters/html_gitlab.rb @@ -21,12 +21,24 @@ module Rouge is_first = false yield %() - line.each { |token, value| yield span(token, value.chomp! || value) } + + line.each do |token, value| + yield highlight_unicode_control_characters(span(token, value.chomp! || value)) + end + yield %() @line_number += 1 end end + + private + + def highlight_unicode_control_characters(text) + text.gsub(Gitlab::Unicode::BIDI_REGEXP) do |char| + %(#{char}) + end + end end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 6b4fa438f8..12924ffedf 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -25164,6 +25164,9 @@ msgstr "" msgid "Postman collection file path or URL" msgstr "" +msgid "Potentially unwanted character detected: Unicode BiDi Control" +msgstr "" + msgid "Pre-defined push rules." msgstr "" @@ -26253,6 +26256,9 @@ msgstr "" msgid "ProjectSettings|Global" msgstr "" +msgid "ProjectSettings|Highlight the usage of hidden unicode characters. These have innocent uses for right-to-left languages, but can also be used in potential exploits." +msgstr "" + msgid "ProjectSettings|Internal" msgstr "" @@ -26442,6 +26448,9 @@ msgstr "" msgid "ProjectSettings|Visualize the project's performance metrics." msgstr "" +msgid "ProjectSettings|Warn about Potentially Unwanted Characters" +msgstr "" + msgid "ProjectSettings|What are badges?" msgstr "" @@ -38764,6 +38773,9 @@ msgstr "" msgid "[No reason]" msgstr "" +msgid "[REDACTED]" +msgstr "" + msgid "[Redacted]" msgstr "" diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index 9d07006185..5f3843928d 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -312,6 +312,34 @@ RSpec.describe ProjectsController do expect { get_show }.not_to change { Gitlab::GitalyClient.get_request_count } end + + describe "PUC highlighting" do + render_views + + before do + expect(controller).to receive(:find_routable!).and_return(public_project) + end + + context "option is enabled" do + it "adds the highlighting class" do + expect(public_project).to receive(:warn_about_potentially_unwanted_characters?).and_return(true) + + get_show + + expect(response.body).to have_css(".project-highlight-puc") + end + end + + context "option is disabled" do + it "doesn't add the highlighting class" do + expect(public_project).to receive(:warn_about_potentially_unwanted_characters?).and_return(false) + + get_show + + expect(response.body).not_to have_css(".project-highlight-puc") + end + end + end end context "when the url contains .atom" do diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 476c57f2d8..1907d7236e 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -19,6 +19,10 @@ FactoryBot.define do admin { true } end + trait :public_email do + public_email { email } + end + trait :blocked do after(:build) { |user, _| user.block! } end diff --git a/spec/frontend/lib/dompurify_spec.js b/spec/frontend/lib/dompurify_spec.js index fa8dbb12a0..f3ccc13e85 100644 --- a/spec/frontend/lib/dompurify_spec.js +++ b/spec/frontend/lib/dompurify_spec.js @@ -22,12 +22,16 @@ const safeUrls = { const unsafeUrls = [ '/an/evil/url', '../../../evil/url', - 'https://evil.url/assets/icons-123a.svg', + 'https://evil.url/assets/icons-123a.svg#test', 'https://evil.url/assets/icons-456b.svg', `https://evil.url/${rootGon.sprite_icons}`, `https://evil.url/${rootGon.sprite_file_icons}`, `https://evil.url/${absoluteGon.sprite_icons}`, `https://evil.url/${absoluteGon.sprite_file_icons}`, + `${rootGon.sprite_icons}/../evil/path`, + `${rootGon.sprite_file_icons}/../../evil/path`, + `${absoluteGon.sprite_icons}/../evil/path`, + `${absoluteGon.sprite_file_icons}/../../https://evil.url`, ]; const forbiddenDataAttrs = ['data-remote', 'data-url', 'data-type', 'data-method']; diff --git a/spec/frontend/lib/utils/url_utility_spec.js b/spec/frontend/lib/utils/url_utility_spec.js index c8ac7ffc9d..24874804dd 100644 --- a/spec/frontend/lib/utils/url_utility_spec.js +++ b/spec/frontend/lib/utils/url_utility_spec.js @@ -607,6 +607,27 @@ describe('URL utility', () => { }); }); + describe('getNormalizedURL', () => { + it.each` + url | base | result + ${'./foo'} | ${''} | ${'http://test.host/foo'} + ${'../john.md'} | ${''} | ${'http://test.host/john.md'} + ${'/images/img.png'} | ${'https://gitlab.com'} | ${'https://gitlab.com/images/img.png'} + ${'/images/../img.png'} | ${'https://gitlab.com'} | ${'https://gitlab.com/img.png'} + ${'/images/./img.png'} | ${'https://gitlab.com'} | ${'https://gitlab.com/images/img.png'} + ${'./images/img.png'} | ${'https://gitlab.com/user/project'} | ${'https://gitlab.com/user/images/img.png'} + ${'../images/../img.png'} | ${'https://gitlab.com/user/project'} | ${'https://gitlab.com/img.png'} + ${'/images/img.png'} | ${'https://gitlab.com/user/project'} | ${'https://gitlab.com/images/img.png'} + ${'/images/../img.png'} | ${'https://gitlab.com/user/project'} | ${'https://gitlab.com/img.png'} + ${'/images/./img.png'} | ${'https://gitlab.com/user/project'} | ${'https://gitlab.com/images/img.png'} + `( + 'converts url "$url" with base "$base" to normalized url => "expected"', + ({ url, base, result }) => { + expect(urlUtils.getNormalizedURL(url, base)).toBe(result); + }, + ); + }); + describe('getWebSocketProtocol', () => { it.each` protocol | expectation diff --git a/spec/frontend/pages/projects/shared/permissions/components/settings_panel_spec.js b/spec/frontend/pages/projects/shared/permissions/components/settings_panel_spec.js index 1e562419f3..0020269e4e 100644 --- a/spec/frontend/pages/projects/shared/permissions/components/settings_panel_spec.js +++ b/spec/frontend/pages/projects/shared/permissions/components/settings_panel_spec.js @@ -27,6 +27,7 @@ const defaultProps = { emailsDisabled: false, packagesEnabled: true, showDefaultAwardEmojis: true, + warnAboutPotentiallyUnwantedCharacters: true, }, isGitlabCom: true, canDisableEmails: true, @@ -97,6 +98,10 @@ describe('Settings Panel', () => { const findEmailSettings = () => wrapper.find({ ref: 'email-settings' }); const findShowDefaultAwardEmojis = () => wrapper.find('input[name="project[project_setting_attributes][show_default_award_emojis]"]'); + const findWarnAboutPuc = () => + wrapper.find( + 'input[name="project[project_setting_attributes][warn_about_potentially_unwanted_characters]"]', + ); const findMetricsVisibilitySettings = () => wrapper.find({ ref: 'metrics-visibility-settings' }); const findOperationsSettings = () => wrapper.find({ ref: 'operations-settings' }); @@ -539,6 +544,14 @@ describe('Settings Panel', () => { }); }); + describe('Warn about potentially unwanted characters', () => { + it('should have a "Warn about Potentially Unwanted Characters" input', () => { + wrapper = mountComponent(); + + expect(findWarnAboutPuc().exists()).toBe(true); + }); + }); + describe('Metrics dashboard', () => { it('should show the metrics dashboard access toggle', () => { wrapper = mountComponent(); diff --git a/spec/graphql/mutations/discussions/toggle_resolve_spec.rb b/spec/graphql/mutations/discussions/toggle_resolve_spec.rb index b03c6cb094..8c11279a80 100644 --- a/spec/graphql/mutations/discussions/toggle_resolve_spec.rb +++ b/spec/graphql/mutations/discussions/toggle_resolve_spec.rb @@ -7,6 +7,7 @@ RSpec.describe Mutations::Discussions::ToggleResolve do described_class.new(object: nil, context: { current_user: user }, field: nil) end + let_it_be(:author) { create(:user) } let_it_be(:project) { create(:project, :repository) } describe '#resolve' do @@ -136,20 +137,37 @@ RSpec.describe Mutations::Discussions::ToggleResolve do end end end + + context 'when user is the author and discussion is locked' do + let(:user) { author } + + before do + issuable.update!(discussion_locked: true) + end + + it 'raises an error' do + expect { mutation.resolve(id: id_arg, resolve: resolve_arg) }.to raise_error( + Gitlab::Graphql::Errors::ResourceNotAvailable, + "The resource that you are attempting to access does not exist or you don't have permission to perform this action" + ) + end + end end context 'when discussion is on a merge request' do - let_it_be(:noteable) { create(:merge_request, source_project: project) } + let_it_be(:noteable) { create(:merge_request, source_project: project, author: author) } let(:discussion) { create(:diff_note_on_merge_request, noteable: noteable, project: project).to_discussion } + let(:issuable) { noteable } it_behaves_like 'a working resolve method' end context 'when discussion is on a design' do - let_it_be(:noteable) { create(:design, :with_file, issue: create(:issue, project: project)) } + let_it_be(:noteable) { create(:design, :with_file, issue: create(:issue, project: project, author: author)) } let(:discussion) { create(:diff_note_on_design, noteable: noteable, project: project).to_discussion } + let(:issuable) { noteable.issue } it_behaves_like 'a working resolve method' end diff --git a/spec/graphql/mutations/issues/set_severity_spec.rb b/spec/graphql/mutations/issues/set_severity_spec.rb index 7ce9c7f662..84736fe7ee 100644 --- a/spec/graphql/mutations/issues/set_severity_spec.rb +++ b/spec/graphql/mutations/issues/set_severity_spec.rb @@ -3,26 +3,58 @@ require 'spec_helper' RSpec.describe Mutations::Issues::SetSeverity do - let_it_be(:user) { create(:user) } - let_it_be(:issue) { create(:incident) } + let_it_be(:project) { create(:project) } + let_it_be(:guest) { create(:user) } + let_it_be(:reporter) { create(:user) } + let_it_be(:issue) { create(:incident, project: project) } let(:mutation) { described_class.new(object: nil, context: { current_user: user }, field: nil) } - specify { expect(described_class).to require_graphql_authorizations(:update_issue) } + specify { expect(described_class).to require_graphql_authorizations(:update_issue, :admin_issue) } + + before_all do + project.add_guest(guest) + project.add_reporter(reporter) + end describe '#resolve' do let(:severity) { 'critical' } - let(:mutated_incident) { subject[:issue] } - subject(:resolve) { mutation.resolve(project_path: issue.project.full_path, iid: issue.iid, severity: severity) } + subject(:resolve) do + mutation.resolve( + project_path: issue.project.full_path, + iid: issue.iid, + severity: severity + ) + end - it_behaves_like 'permission level for issue mutation is correctly verified' + context 'as guest' do + let(:user) { guest } - context 'when the user can update the issue' do - before do - issue.project.add_developer(user) + it 'raises an error' do + expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) end + context 'and also author' do + let!(:issue) { create(:incident, project: project, author: user) } + + it 'raises an error' do + expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) + end + end + + context 'and also assignee' do + let!(:issue) { create(:incident, project: project, assignee_ids: [user.id]) } + + it 'raises an error' do + expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) + end + end + end + + context 'as reporter' do + let(:user) { reporter } + context 'when issue type is incident' do context 'when severity has a correct value' do it 'updates severity' do @@ -48,9 +80,9 @@ RSpec.describe Mutations::Issues::SetSeverity do end context 'when issue type is not incident' do - let!(:issue) { create(:issue) } + let!(:issue) { create(:issue, project: project) } - it 'does not updates the issue' do + it 'does not update the issue' do expect { resolve }.not_to change { issue.updated_at } end end diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb index 4dac4403f7..a519b10fe8 100644 --- a/spec/helpers/projects_helper_spec.rb +++ b/spec/helpers/projects_helper_spec.rb @@ -953,4 +953,26 @@ RSpec.describe ProjectsHelper do ) end end + + describe '#project_classes' do + subject { helper.project_classes(project) } + + it { is_expected.to be_a(String) } + + context 'PUC highlighting enabled' do + before do + project.warn_about_potentially_unwanted_characters = true + end + + it { is_expected.to include('project-highlight-puc') } + end + + context 'PUC highlighting disabled' do + before do + project.warn_about_potentially_unwanted_characters = false + end + + it { is_expected.not_to include('project-highlight-puc') } + end + end end diff --git a/spec/lib/api/entities/project_spec.rb b/spec/lib/api/entities/project_spec.rb new file mode 100644 index 0000000000..8d1c3aa878 --- /dev/null +++ b/spec/lib/api/entities/project_spec.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::API::Entities::Project do + let(:project) { create(:project, :public) } + let(:current_user) { create(:user) } + let(:options) { { current_user: current_user } } + + let(:entity) do + ::API::Entities::Project.new(project, options) + end + + subject(:json) { entity.as_json } + + describe '.shared_with_groups' do + let(:group) { create(:group, :private) } + + before do + project.project_group_links.create!(group: group) + end + + context 'when the current user does not have access to the group' do + it 'is empty' do + expect(json[:shared_with_groups]).to be_empty + end + end + + context 'when the current user has access to the group' do + before do + group.add_guest(current_user) + end + + it 'contains information about the shared group' do + expect(json[:shared_with_groups]).to contain_exactly(include(group_id: group.id)) + end + end + end +end diff --git a/spec/lib/gitlab/data_builder/build_spec.rb b/spec/lib/gitlab/data_builder/build_spec.rb index 325fdb9092..9cee0802e8 100644 --- a/spec/lib/gitlab/data_builder/build_spec.rb +++ b/spec/lib/gitlab/data_builder/build_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Gitlab::DataBuilder::Build do let!(:tag_names) { %w(tag-1 tag-2) } let(:runner) { create(:ci_runner, :instance, tag_list: tag_names.map { |n| ActsAsTaggableOn::Tag.create!(name: n)}) } - let(:user) { create(:user) } + let(:user) { create(:user, :public_email) } let(:build) { create(:ci_build, :running, runner: runner, user: user) } describe '.build' do diff --git a/spec/lib/gitlab/data_builder/pipeline_spec.rb b/spec/lib/gitlab/data_builder/pipeline_spec.rb index 0e574c7aa8..8b57da8e60 100644 --- a/spec/lib/gitlab/data_builder/pipeline_spec.rb +++ b/spec/lib/gitlab/data_builder/pipeline_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Gitlab::DataBuilder::Pipeline do - let_it_be(:user) { create(:user) } + let_it_be(:user) { create(:user, :public_email) } let_it_be(:project) { create(:project, :repository) } let_it_be_with_reload(:pipeline) do @@ -46,7 +46,7 @@ RSpec.describe Gitlab::DataBuilder::Pipeline do name: user.name, username: user.username, avatar_url: user.avatar_url(only_path: false), - email: user.email + email: user.public_email }) end diff --git a/spec/lib/gitlab/import_export/project/relation_factory_spec.rb b/spec/lib/gitlab/import_export/project/relation_factory_spec.rb index 88bd71d3d6..49df231392 100644 --- a/spec/lib/gitlab/import_export/project/relation_factory_spec.rb +++ b/spec/lib/gitlab/import_export/project/relation_factory_spec.rb @@ -267,6 +267,35 @@ RSpec.describe Gitlab::ImportExport::Project::RelationFactory, :use_clean_rails_ end end + context 'pipeline_schedule' do + let(:relation_sym) { :pipeline_schedules } + let(:relation_hash) do + { + "id": 3, + "created_at": "2016-07-22T08:55:44.161Z", + "updated_at": "2016-07-22T08:55:44.161Z", + "description": "pipeline schedule", + "ref": "main", + "cron": "0 4 * * 0", + "cron_timezone": "UTC", + "active": value, + "project_id": project.id + } + end + + subject { created_object.active } + + [true, false].each do |v| + context "when relation_hash has active set to #{v}" do + let(:value) { v } + + it "the created object is not active" do + expect(created_object.active).to eq(false) + end + end + end + end + # `project_id`, `described_class.USER_REFERENCES`, noteable_id, target_id, and some project IDs are already # re-assigned by described_class. context 'Potentially hazardous foreign keys' do diff --git a/spec/lib/gitlab/import_export/project/tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project/tree_restorer_spec.rb index 518a933782..f512f49764 100644 --- a/spec/lib/gitlab/import_export/project/tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/project/tree_restorer_spec.rb @@ -376,7 +376,7 @@ RSpec.describe Gitlab::ImportExport::Project::TreeRestorer do expect(pipeline_schedule.ref).to eq('master') expect(pipeline_schedule.cron).to eq('0 4 * * 0') expect(pipeline_schedule.cron_timezone).to eq('UTC') - expect(pipeline_schedule.active).to eq(true) + expect(pipeline_schedule.active).to eq(false) end end diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index b238a4733e..e555886657 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -555,7 +555,6 @@ Project: - merge_requests_rebase_enabled - jobs_cache_index - external_authorization_classification_label -- external_webhook_token - pages_https_only - merge_requests_disable_committers_approval - require_password_to_approve diff --git a/spec/lib/gitlab/unicode_spec.rb b/spec/lib/gitlab/unicode_spec.rb new file mode 100644 index 0000000000..68f3266ecc --- /dev/null +++ b/spec/lib/gitlab/unicode_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe Gitlab::Unicode do + describe described_class::BIDI_REGEXP do + using RSpec::Parameterized::TableSyntax + + where(:bidi_string, :match) do + "\u2066" | true # left-to-right isolate + "\u2067" | true # right-to-left isolate + "\u2068" | true # first strong isolate + "\u2069" | true # pop directional isolate + "\u202a" | true # left-to-right embedding + "\u202b" | true # right-to-left embedding + "\u202c" | true # pop directional formatting + "\u202d" | true # left-to-right override + "\u202e" | true # right-to-left override + "\u2066foobar" | true + "" | false + "foo" | false + "\u2713" | false # checkmark + end + + with_them do + let(:utf8_string) { bidi_string.encode("utf-8") } + + it "matches only the bidi characters" do + expect(utf8_string.match?(subject)).to eq(match) + end + end + end +end diff --git a/spec/lib/rouge/formatters/html_gitlab_spec.rb b/spec/lib/rouge/formatters/html_gitlab_spec.rb index d45c8c2a8c..6fa0ac1c4b 100644 --- a/spec/lib/rouge/formatters/html_gitlab_spec.rb +++ b/spec/lib/rouge/formatters/html_gitlab_spec.rb @@ -36,5 +36,26 @@ RSpec.describe Rouge::Formatters::HTMLGitlab do is_expected.to eq(code) end end + + context 'when unicode control characters are used' do + let(:lang) { 'javascript' } + let(:tokens) { lexer.lex(code, continue: false) } + let(:code) do + <<~JS + #!/usr/bin/env node + + var accessLevel = "user"; + if (accessLevel != "user‮ ⁦// Check if admin⁩ ⁦") { + console.log("You are an admin."); + } + JS + end + + it 'highlights the control characters' do + message = "Potentially unwanted character detected: Unicode BiDi Control" + + is_expected.to include(%{}).exactly(4).times + end + end end end diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index da89eccc3b..3ddf7370fe 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -7,7 +7,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do include StubRequests include Ci::SourcePipelineHelpers - let_it_be(:user) { create(:user) } + let_it_be(:user) { create(:user, :public_email) } let_it_be(:namespace) { create_default(:namespace).freeze } let_it_be(:project) { create_default(:project, :repository).freeze } diff --git a/spec/models/concerns/resolvable_discussion_spec.rb b/spec/models/concerns/resolvable_discussion_spec.rb index c0e5ddc23b..fc154738f1 100644 --- a/spec/models/concerns/resolvable_discussion_spec.rb +++ b/spec/models/concerns/resolvable_discussion_spec.rb @@ -188,6 +188,16 @@ RSpec.describe Discussion, ResolvableDiscussion do it "returns true" do expect(subject.can_resolve?(current_user)).to be true end + + context 'when noteable is locked' do + before do + allow(subject.noteable).to receive(:discussion_locked?).and_return(true) + end + + it 'returns false' do + expect(subject.can_resolve?(current_user)).to be_falsey + end + end end context "when the signed in user can push to the project" do @@ -200,8 +210,11 @@ RSpec.describe Discussion, ResolvableDiscussion do end context "when the noteable has no author" do + before do + subject.noteable.author = nil + end + it "returns true" do - expect(noteable).to receive(:author).and_return(nil) expect(subject.can_resolve?(current_user)).to be true end end @@ -213,8 +226,11 @@ RSpec.describe Discussion, ResolvableDiscussion do end context "when the noteable has no author" do + before do + subject.noteable.author = nil + end + it "returns false" do - expect(noteable).to receive(:author).and_return(nil) expect(subject.can_resolve?(current_user)).to be false end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index d8f3a63d22..15b751ebe8 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -660,6 +660,19 @@ RSpec.describe Project, factory_default: :keep do it { is_expected.to delegate_method(:container_registry_enabled?).to(:project_feature) } it { is_expected.to delegate_method(:container_registry_access_level).to(:project_feature) } + describe 'project settings' do + %i( + show_default_award_emojis + show_default_award_emojis= + show_default_award_emojis? + warn_about_potentially_unwanted_characters + warn_about_potentially_unwanted_characters= + warn_about_potentially_unwanted_characters? + ).each do |method| + it { is_expected.to delegate_method(method).to(:project_setting).with_arguments(allow_nil: true) } + end + end + include_examples 'ci_cd_settings delegation' do # Skip attributes defined in EE code let(:exclude_attributes) do diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 714cb24f27..448bcde01d 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -5511,16 +5511,29 @@ RSpec.describe User do end describe '#hook_attrs' do - it 'includes id, name, username, avatar_url, and email' do - user = create(:user) - user_attributes = { + let(:user) { create(:user) } + let(:user_attributes) do + { id: user.id, name: user.name, username: user.username, - avatar_url: user.avatar_url(only_path: false), - email: user.email + avatar_url: user.avatar_url(only_path: false) } - expect(user.hook_attrs).to eq(user_attributes) + end + + context 'with a public email' do + it 'includes id, name, username, avatar_url, and email' do + user.public_email = "hello@hello.com" + user_attributes[:email] = user.public_email + expect(user.hook_attrs).to eq(user_attributes) + end + end + + context 'without a public email' do + it "does not include email if user's email is private" do + user_attributes[:email] = "[REDACTED]" + expect(user.hook_attrs).to eq(user_attributes) + end end end diff --git a/spec/policies/issuable_policy_spec.rb b/spec/policies/issuable_policy_spec.rb index 86b04ccda5..eeb298e853 100644 --- a/spec/policies/issuable_policy_spec.rb +++ b/spec/policies/issuable_policy_spec.rb @@ -13,6 +13,10 @@ RSpec.describe IssuablePolicy, models: true do let(:merge_request) { create(:merge_request, source_project: project, author: user) } let(:policies) { described_class.new(user, merge_request) } + it 'allows resolving notes' do + expect(policies).to be_allowed(:resolve_note) + 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) @@ -43,16 +47,24 @@ RSpec.describe IssuablePolicy, models: true do it 'can not create a note nor award emojis' do expect(policies).to be_disallowed(:create_note, :award_emoji) end + + it 'does not allow resolving note' do + expect(policies).to be_disallowed(:resolve_note) + end end context 'when the user is a project member' do before do - project.add_guest(user) + project.add_developer(user) end it 'can create a note and award emojis' do expect(policies).to be_allowed(:create_note, :award_emoji) end + + it 'allows resolving notes' do + expect(policies).to be_allowed(:resolve_note) + end end end end diff --git a/spec/requests/api/applications_spec.rb b/spec/requests/api/applications_spec.rb index 959e68e6a0..022451553e 100644 --- a/spec/requests/api/applications_spec.rb +++ b/spec/requests/api/applications_spec.rb @@ -5,13 +5,14 @@ require 'spec_helper' RSpec.describe API::Applications, :api do let(:admin_user) { create(:user, admin: true) } let(:user) { create(:user, admin: false) } - let!(:application) { create(:application, name: 'another_application', owner: nil, redirect_uri: 'http://other_application.url', scopes: '') } + let(:scopes) { 'api' } + let!(:application) { create(:application, name: 'another_application', owner: nil, redirect_uri: 'http://other_application.url', scopes: scopes) } describe 'POST /applications' do context 'authenticated and authorized user' do it 'creates and returns an OAuth application' do expect do - post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'http://application.url', scopes: '' } + post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'http://application.url', scopes: scopes } end.to change { Doorkeeper::Application.count }.by 1 application = Doorkeeper::Application.find_by(name: 'application_name', redirect_uri: 'http://application.url') @@ -22,11 +23,12 @@ RSpec.describe API::Applications, :api do expect(json_response['secret']).to eq application.secret expect(json_response['callback_url']).to eq application.redirect_uri expect(json_response['confidential']).to eq application.confidential + expect(application.scopes.to_s).to eq('api') end it 'does not allow creating an application with the wrong redirect_uri format' do expect do - post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'http://', scopes: '' } + post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'http://', scopes: scopes } end.not_to change { Doorkeeper::Application.count } expect(response).to have_gitlab_http_status(:bad_request) @@ -36,7 +38,7 @@ RSpec.describe API::Applications, :api do it 'does not allow creating an application with a forbidden URI format' do expect do - post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'javascript://alert()', scopes: '' } + post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'javascript://alert()', scopes: scopes } end.not_to change { Doorkeeper::Application.count } expect(response).to have_gitlab_http_status(:bad_request) @@ -46,7 +48,7 @@ RSpec.describe API::Applications, :api do it 'does not allow creating an application without a name' do expect do - post api('/applications', admin_user), params: { redirect_uri: 'http://application.url', scopes: '' } + post api('/applications', admin_user), params: { redirect_uri: 'http://application.url', scopes: scopes } end.not_to change { Doorkeeper::Application.count } expect(response).to have_gitlab_http_status(:bad_request) @@ -56,7 +58,7 @@ RSpec.describe API::Applications, :api do it 'does not allow creating an application without a redirect_uri' do expect do - post api('/applications', admin_user), params: { name: 'application_name', scopes: '' } + post api('/applications', admin_user), params: { name: 'application_name', scopes: scopes } end.not_to change { Doorkeeper::Application.count } expect(response).to have_gitlab_http_status(:bad_request) @@ -64,19 +66,59 @@ RSpec.describe API::Applications, :api do expect(json_response['error']).to eq('redirect_uri is missing') end - it 'does not allow creating an application without scopes' do + it 'does not allow creating an application without specifying `scopes`' do expect do post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'http://application.url' } end.not_to change { Doorkeeper::Application.count } expect(response).to have_gitlab_http_status(:bad_request) expect(json_response).to be_a Hash - expect(json_response['error']).to eq('scopes is missing') + expect(json_response['error']).to eq('scopes is missing, scopes is empty') + end + + it 'does not allow creating an application with blank `scopes`' do + expect do + post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'http://application.url', scopes: '' } + end.not_to change { Doorkeeper::Application.count } + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['error']).to eq('scopes is empty') + end + + it 'does not allow creating an application with invalid `scopes`' do + expect do + post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'http://application.url', scopes: 'non_existent_scope' } + end.not_to change { Doorkeeper::Application.count } + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['message']['scopes'][0]).to eq('doesn\'t match configured on the server.') + end + + context 'multiple scopes' do + it 'creates an application with multiple `scopes` when each scope specified is seperated by a space' do + expect do + post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'http://application.url', scopes: 'api read_user' } + end.to change { Doorkeeper::Application.count }.by 1 + + application = Doorkeeper::Application.last + + expect(response).to have_gitlab_http_status(:created) + expect(application.scopes.to_s).to eq('api read_user') + end + + it 'does not allow creating an application with multiple `scopes` when one of the scopes is invalid' do + expect do + post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'http://application.url', scopes: 'api non_existent_scope' } + end.not_to change { Doorkeeper::Application.count } + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['message']['scopes'][0]).to eq('doesn\'t match configured on the server.') + end end it 'defaults to creating an application with confidential' do expect do - post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'http://application.url', scopes: '', confidential: nil } + post api('/applications', admin_user), params: { name: 'application_name', redirect_uri: 'http://application.url', scopes: scopes, confidential: nil } end.to change { Doorkeeper::Application.count }.by(1) expect(response).to have_gitlab_http_status(:created) @@ -89,7 +131,7 @@ RSpec.describe API::Applications, :api do context 'authorized user without authorization' do it 'does not create application' do expect do - post api('/applications', user), params: { name: 'application_name', redirect_uri: 'http://application.url', scopes: '' } + post api('/applications', user), params: { name: 'application_name', redirect_uri: 'http://application.url', scopes: scopes } end.not_to change { Doorkeeper::Application.count } expect(response).to have_gitlab_http_status(:forbidden) diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index 30df47ccc4..8c801e333f 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -758,6 +758,15 @@ RSpec.describe API::Groups do expect(json_response['prevent_sharing_groups_outside_hierarchy']).to eq(true) end + it 'does not update visibility_level if it is restricted' do + stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::INTERNAL]) + + put api("/groups/#{group1.id}", user1), params: { visibility: 'internal' } + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['message']['visibility_level']).to include('internal has been restricted by your GitLab administrator') + end + context 'updating the `default_branch_protection` attribute' do subject do put api("/groups/#{group1.id}", user1), params: { default_branch_protection: ::Gitlab::Access::PROTECTION_NONE } @@ -845,6 +854,15 @@ RSpec.describe API::Groups do expect(response).to have_gitlab_http_status(:ok) expect(json_response['name']).to eq(new_group_name) end + + it 'ignores visibility level restrictions' do + stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::INTERNAL]) + + put api("/groups/#{group1.id}", admin), params: { visibility: 'internal' } + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['visibility']).to eq('internal') + end end context 'when authenticated as an user that can see the group' do diff --git a/spec/requests/api/project_attributes.yml b/spec/requests/api/project_attributes.yml index c5bcedd491..47eccacf47 100644 --- a/spec/requests/api/project_attributes.yml +++ b/spec/requests/api/project_attributes.yml @@ -137,6 +137,7 @@ project_setting: - has_confluence - has_vulnerabilities - prevent_merge_without_jira_issue + - warn_about_potentially_unwanted_characters - previous_default_branch - project_id - push_rule_id diff --git a/spec/requests/api/project_import_spec.rb b/spec/requests/api/project_import_spec.rb index d3b24eb383..0c9e125cc9 100644 --- a/spec/requests/api/project_import_spec.rb +++ b/spec/requests/api/project_import_spec.rb @@ -16,6 +16,16 @@ RSpec.describe API::ProjectImport do namespace.add_owner(user) end + shared_examples 'requires authentication' do + let(:user) { nil } + + it 'returns 401' do + subject + + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + describe 'POST /projects/import' do subject { upload_archive(file_upload, workhorse_headers, params) } @@ -32,6 +42,8 @@ RSpec.describe API::ProjectImport do allow(ImportExportUploader).to receive(:workhorse_upload_path).and_return('/') end + it_behaves_like 'requires authentication' + it 'executes a limited number of queries' do control_count = ActiveRecord::QueryRecorder.new { subject }.count @@ -281,6 +293,10 @@ RSpec.describe API::ProjectImport do end describe 'POST /projects/remote-import' do + subject do + post api('/projects/remote-import', user), params: params + end + let(:params) do { path: 'test-import', @@ -288,10 +304,12 @@ RSpec.describe API::ProjectImport do } end + it_behaves_like 'requires authentication' + it 'returns NOT FOUND when the feature is disabled' do stub_feature_flags(import_project_from_remote_file: false) - post api('/projects/remote-import', user), params: params + subject expect(response).to have_gitlab_http_status(:not_found) end @@ -315,7 +333,7 @@ RSpec.describe API::ProjectImport do .to receive(:execute) .and_return(service_response) - post api('/projects/remote-import', user), params: params + subject expect(response).to have_gitlab_http_status(:created) expect(json_response).to include({ @@ -338,7 +356,7 @@ RSpec.describe API::ProjectImport do .to receive(:execute) .and_return(service_response) - post api('/projects/remote-import', user), params: params + subject expect(response).to have_gitlab_http_status(:bad_request) expect(json_response).to eq({ @@ -350,6 +368,14 @@ RSpec.describe API::ProjectImport do end describe 'GET /projects/:id/import' do + it 'public project accessible for an unauthenticated user' do + project = create(:project, :public) + + get api("/projects/#{project.id}/import", nil) + + expect(response).to have_gitlab_http_status(:ok) + end + it 'returns the import status' do project = create(:project, :import_started) project.add_maintainer(user) @@ -376,6 +402,8 @@ RSpec.describe API::ProjectImport do describe 'POST /projects/import/authorize' do subject { post api('/projects/import/authorize', user), headers: workhorse_headers } + it_behaves_like 'requires authentication' + it 'authorizes importing project with workhorse header' do subject diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 6e0549d519..49e7c387c8 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -990,7 +990,7 @@ RSpec.describe API::Projects do expect do get api('/projects', admin) - end.not_to exceed_query_limit(control.count) + end.not_to exceed_query_limit(control) end end end @@ -3203,6 +3203,15 @@ RSpec.describe API::Projects do expect(json_response['visibility']).to eq('private') end + it 'does not update visibility_level if it is restricted' do + stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::INTERNAL]) + + put api("/projects/#{project3.id}", user), params: { visibility: 'internal' } + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['message']['visibility_level']).to include('internal has been restricted by your GitLab administrator') + end + it 'does not update name to existing name' do project_param = { name: project3.name } @@ -3526,6 +3535,19 @@ RSpec.describe API::Projects do end end + context 'when authenticated as the admin' do + let_it_be(:admin) { create(:admin) } + + it 'ignores visibility level restrictions' do + stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::INTERNAL]) + + put api("/projects/#{project3.id}", admin), params: { visibility: 'internal' } + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['visibility']).to eq('internal') + end + end + context 'when updating repository storage' do let(:unknown_storage) { 'new-storage' } let(:new_project) { create(:project, :repository, namespace: user.namespace) } diff --git a/spec/services/groups/transfer_service_spec.rb b/spec/services/groups/transfer_service_spec.rb index 889b555174..83337aad6f 100644 --- a/spec/services/groups/transfer_service_spec.rb +++ b/spec/services/groups/transfer_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Groups::TransferService do +RSpec.describe Groups::TransferService, :sidekiq_inline do let_it_be(:user) { create(:user) } let_it_be(:new_parent_group) { create(:group, :public) } @@ -594,6 +594,59 @@ RSpec.describe Groups::TransferService do transfer_service.execute(new_parent_group) end end + + context 'transferring groups with shared_projects' do + let_it_be_with_reload(:shared_project) { create(:project, :public) } + + shared_examples_for 'drops the authorizations of ancestor members from the old hierarchy' do + it 'drops the authorizations of ancestor members from the old hierarchy' do + expect { transfer_service.execute(new_parent_group) }.to change { + ProjectAuthorization.where(project: shared_project, user: old_group_member).size + }.from(1).to(0) + end + end + + context 'when the group that has existing project share is transferred' do + before do + create(:project_group_link, :maintainer, project: shared_project, group: group) + end + + it_behaves_like 'drops the authorizations of ancestor members from the old hierarchy' + end + + context 'when the group whose subgroup has an existing project share is transferred' do + let_it_be_with_reload(:subgroup) { create(:group, :private, parent: group) } + + before do + create(:project_group_link, :maintainer, project: shared_project, group: subgroup) + end + + it_behaves_like 'drops the authorizations of ancestor members from the old hierarchy' + end + end + + context 'when a group that has existing group share is transferred' do + let(:shared_with_group) { group } + + let_it_be(:member_of_shared_with_group) { create(:user) } + let_it_be(:shared_group) { create(:group, :private) } + let_it_be(:project_in_shared_group) { create(:project, namespace: shared_group) } + + before do + shared_with_group.add_developer(member_of_shared_with_group) + create(:group_group_link, :maintainer, shared_group: shared_group, shared_with_group: shared_with_group) + shared_with_group.refresh_members_authorized_projects + end + + it 'retains the authorizations of direct members' do + expect { transfer_service.execute(new_parent_group) }.not_to change { + ProjectAuthorization.where( + project: project_in_shared_group, + user: member_of_shared_with_group, + access_level: Gitlab::Access::DEVELOPER).size + }.from(1) + end + end end end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 29ac7df88e..27e4351241 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe Issues::UpdateService, :mailer do let_it_be(:user) { create(:user) } let_it_be(:user2) { create(:user) } let_it_be(:user3) { create(:user) } + let_it_be(:guest) { create(:user) } let_it_be(:group) { create(:group, :public) } let_it_be(:project, reload: true) { create(:project, :repository, group: group) } let_it_be(:label) { create(:label, project: project) } @@ -24,6 +25,7 @@ RSpec.describe Issues::UpdateService, :mailer do project.add_maintainer(user) project.add_developer(user2) project.add_developer(user3) + project.add_guest(guest) end describe 'execute' do @@ -95,9 +97,7 @@ RSpec.describe Issues::UpdateService, :mailer do end context 'user is a guest' do - before do - project.add_guest(user) - end + let(:user) { guest } it 'does not assign the sentry error' do update_issue(opts) @@ -245,11 +245,7 @@ RSpec.describe Issues::UpdateService, :mailer do context 'from issue to restricted issue types' do context 'without sufficient permissions' do - let(:user) { create(:user) } - - before do - project.add_guest(user) - end + let(:user) { guest } it 'does nothing to the labels' do expect { update_issue(issue_type: 'issue') }.not_to change(issue.labels, :count) @@ -394,12 +390,6 @@ RSpec.describe Issues::UpdateService, :mailer do end context 'when current user cannot admin issues in the project' do - let(:guest) { create(:user) } - - before do - project.add_guest(guest) - end - it 'filters out params that cannot be set without the :admin_issue permission' do described_class.new( project: project, current_user: guest, params: opts.merge( @@ -1100,6 +1090,24 @@ RSpec.describe Issues::UpdateService, :mailer do it_behaves_like 'does not change the severity' end + + context 'as guest' do + let(:user) { guest } + + it_behaves_like 'does not change the severity' + + context 'and also author' do + let(:issue) { create(:incident, project: project, author: user) } + + it_behaves_like 'does not change the severity' + end + + context 'and also assignee' do + let(:issue) { create(:incident, project: project, assignee_ids: [user.id]) } + + it_behaves_like 'does not change the severity' + end + end end context 'when severity has been set before' do diff --git a/workhorse/internal/artifacts/artifacts_upload_test.go b/workhorse/internal/artifacts/artifacts_upload_test.go index ce078c7855..488636043b 100644 --- a/workhorse/internal/artifacts/artifacts_upload_test.go +++ b/workhorse/internal/artifacts/artifacts_upload_test.go @@ -270,7 +270,7 @@ func TestUploadHandlerForMultipleFiles(t *testing.T) { require.NoError(t, s.writer.Close()) response := testUploadArtifacts(t, s.writer.FormDataContentType(), s.url, s.buffer) - require.Equal(t, http.StatusInternalServerError, response.Code) + require.Equal(t, http.StatusBadRequest, response.Code) } func TestUploadFormProcessing(t *testing.T) { diff --git a/workhorse/internal/upload/exif/testdata/takes_lot_of_memory_to_decode.tiff b/workhorse/internal/upload/exif/testdata/takes_lot_of_memory_to_decode.tiff new file mode 100644 index 0000000000..6935cb130d Binary files /dev/null and b/workhorse/internal/upload/exif/testdata/takes_lot_of_memory_to_decode.tiff differ diff --git a/workhorse/internal/upload/rewrite.go b/workhorse/internal/upload/rewrite.go index 7945fa1f34..3dfab12018 100644 --- a/workhorse/internal/upload/rewrite.go +++ b/workhorse/internal/upload/rewrite.go @@ -25,7 +25,10 @@ import ( ) // ErrInjectedClientParam means that the client sent a parameter that overrides one of our own fields -var ErrInjectedClientParam = errors.New("injected client parameter") +var ( + ErrInjectedClientParam = errors.New("injected client parameter") + ErrMultipleFilesUploaded = errors.New("upload request contains more than one file") +) var ( multipartUploadRequests = promauto.NewCounterVec( @@ -114,6 +117,10 @@ func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, pr } func (rew *rewriter) handleFilePart(ctx context.Context, name string, p *multipart.Part, opts *filestore.SaveFileOpts) error { + if rew.filter.Count() > 0 { + return ErrMultipleFilesUploaded + } + multipartFiles.WithLabelValues(rew.filter.Name()).Inc() filename := filepath.Base(p.FileName()) @@ -226,7 +233,7 @@ func handleExifUpload(ctx context.Context, r io.Reader, filename string, imageTy } func isTIFF(r io.Reader) bool { - _, err := tiff.Decode(r) + _, err := tiff.DecodeConfig(r) if err == nil { return true } diff --git a/workhorse/internal/upload/rewrite_test.go b/workhorse/internal/upload/rewrite_test.go index 6fc41c3fef..e3f33a0248 100644 --- a/workhorse/internal/upload/rewrite_test.go +++ b/workhorse/internal/upload/rewrite_test.go @@ -2,6 +2,7 @@ package upload import ( "os" + "runtime" "testing" "github.com/stretchr/testify/require" @@ -29,6 +30,10 @@ func TestImageTypeRecongition(t *testing.T) { filename: "exif/testdata/sample_exif_invalid.jpg", isJPEG: false, isTIFF: false, + }, { + filename: "exif/testdata/takes_lot_of_memory_to_decode.tiff", // File from https://gitlab.com/gitlab-org/gitlab/-/issues/341363 + isJPEG: false, + isTIFF: true, }, } @@ -36,8 +41,16 @@ func TestImageTypeRecongition(t *testing.T) { t.Run(test.filename, func(t *testing.T) { input, err := os.Open(test.filename) require.NoError(t, err) + + var m runtime.MemStats + runtime.ReadMemStats(&m) + start := m.TotalAlloc + require.Equal(t, test.isJPEG, isJPEG(input)) require.Equal(t, test.isTIFF, isTIFF(input)) + + runtime.ReadMemStats(&m) + require.Less(t, m.TotalAlloc-start, uint64(50000), "must take reasonable amount of memory to recognise the type") }) } } diff --git a/workhorse/internal/upload/uploads.go b/workhorse/internal/upload/uploads.go index 7a93fa4a9d..8f4c8bb95f 100644 --- a/workhorse/internal/upload/uploads.go +++ b/workhorse/internal/upload/uploads.go @@ -21,6 +21,7 @@ type MultipartFormProcessor interface { ProcessField(ctx context.Context, formName string, writer *multipart.Writer) error Finalize(ctx context.Context) error Name() string + Count() int } func HandleFileUploads(w http.ResponseWriter, r *http.Request, h http.Handler, preauth *api.Response, filter MultipartFormProcessor, opts *filestore.SaveFileOpts) { @@ -34,6 +35,8 @@ func HandleFileUploads(w http.ResponseWriter, r *http.Request, h http.Handler, p switch err { case ErrInjectedClientParam: helper.CaptureAndFail(w, r, err, "Bad Request", http.StatusBadRequest) + case ErrMultipleFilesUploaded: + helper.CaptureAndFail(w, r, err, "Uploading multiple files is not allowed", http.StatusBadRequest) case http.ErrNotMultipart: h.ServeHTTP(w, r) case filestore.ErrEntityTooLarge: diff --git a/workhorse/internal/upload/uploads_test.go b/workhorse/internal/upload/uploads_test.go index 5fae5cf7bc..a655cabd2e 100644 --- a/workhorse/internal/upload/uploads_test.go +++ b/workhorse/internal/upload/uploads_test.go @@ -18,7 +18,6 @@ import ( "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitlab/workhorse/internal/api" - "gitlab.com/gitlab-org/gitlab/workhorse/internal/filestore" "gitlab.com/gitlab-org/gitlab/workhorse/internal/helper" "gitlab.com/gitlab-org/gitlab/workhorse/internal/objectstore/test" "gitlab.com/gitlab-org/gitlab/workhorse/internal/proxy" @@ -28,11 +27,7 @@ import ( var nilHandler = http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) -type testFormProcessor struct{} - -func (a *testFormProcessor) ProcessFile(ctx context.Context, formName string, file *filestore.FileHandler, writer *multipart.Writer) error { - return nil -} +type testFormProcessor struct{ SavedFileTracker } func (a *testFormProcessor) ProcessField(ctx context.Context, formName string, writer *multipart.Writer) error { if formName != "token" && !strings.HasPrefix(formName, "file.") && !strings.HasPrefix(formName, "other.") { @@ -45,10 +40,6 @@ func (a *testFormProcessor) Finalize(ctx context.Context) error { return nil } -func (a *testFormProcessor) Name() string { - return "" -} - func TestUploadTempPathRequirement(t *testing.T) { apiResponse := &api.Response{} preparer := &DefaultPreparer{} @@ -259,6 +250,38 @@ func TestUploadProcessingField(t *testing.T) { require.Equal(t, 500, response.Code) } +func TestUploadingMultipleFiles(t *testing.T) { + testhelper.ConfigureSecret() + + tempPath, err := ioutil.TempDir("", "uploads") + require.NoError(t, err) + defer os.RemoveAll(tempPath) + + var buffer bytes.Buffer + + writer := multipart.NewWriter(&buffer) + _, err = writer.CreateFormFile("file", "my.file") + require.NoError(t, err) + _, err = writer.CreateFormFile("file", "my.file") + require.NoError(t, err) + require.NoError(t, writer.Close()) + + httpRequest, err := http.NewRequest("PUT", "/url/path", &buffer) + require.NoError(t, err) + httpRequest.Header.Set("Content-Type", writer.FormDataContentType()) + + response := httptest.NewRecorder() + apiResponse := &api.Response{TempPath: tempPath} + preparer := &DefaultPreparer{} + opts, _, err := preparer.Prepare(apiResponse) + require.NoError(t, err) + + HandleFileUploads(response, httpRequest, nilHandler, apiResponse, &testFormProcessor{}, opts) + + require.Equal(t, 400, response.Code) + require.Equal(t, "Uploading multiple files is not allowed\n", response.Body.String()) +} + func TestUploadProcessingFile(t *testing.T) { tempPath, err := ioutil.TempDir("", "uploads") require.NoError(t, err)