diff options
| author | GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com> | 2021-07-01 16:03:22 +0000 |
|---|---|---|
| committer | GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com> | 2021-07-01 16:03:22 +0000 |
| commit | 3aad3a0b6ffb1a0fe36db41f81e8bbd3728e5f80 (patch) | |
| tree | 69cfc1a4f82d309ca58b361546824b44221b6585 | |
| parent | 76b84b42f64b8009cc181d5da0c656a8a521986d (diff) | |
| parent | bac4ee4a9e2bc845fd5c91240cccaa293cb4f847 (diff) | |
| download | gitlab-ce-3aad3a0b6ffb1a0fe36db41f81e8bbd3728e5f80.tar.gz | |
Merge remote-tracking branch 'dev/14-0-stable' into 14-0-stable
62 files changed, 773 insertions, 204 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index cf54b07c991..afba0f8b97f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,29 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 14.0.2 (2021-07-01) + +### Added (1 change) + +- [Added omniauth_user check when verifying user cap](gitlab-org/security/gitlab@68c5d856fbf83f5f5ade562ea84b6aa06db96c60) ([merge request](gitlab-org/security/gitlab!1501)) **GitLab Enterprise Edition** + +### Security (14 changes) + +- [Update rdoc to 6.3.1](gitlab-org/security/gitlab@341334cbb2d822f6aa057933934b819c34b87932) ([merge request](gitlab-org/security/gitlab!1533)) +- [Forbid GET requests with mutations](gitlab-org/security/gitlab@895c99b35efa6795fb050bfb4ef4574f3e32a373) ([merge request](gitlab-org/security/gitlab!1528)) +- [Prevent GraphQL API access by deactivated users](gitlab-org/security/gitlab@2dda4163dadc04b59ee3367990b72bee933adf9b) ([merge request](gitlab-org/security/gitlab!1525)) +- [Add sanitizing for name field](gitlab-org/security/gitlab@ecb5a598b87d670906df67ed4432426a375efa05) ([merge request](gitlab-org/security/gitlab!1499)) +- [Copy feature visibility settings to a fork](gitlab-org/security/gitlab@fcc87978b1c865c8bdcb3fc5d8dc221b7370192c) ([merge request](gitlab-org/security/gitlab!1522)) +- [Fix XSS on audit log for feature flag actions](gitlab-org/security/gitlab@94fc41d49e828a6457f1de31f2b239b087679c12) ([merge request](gitlab-org/security/gitlab!1521)) +- [Avoid disclosing project in web IDE](gitlab-org/security/gitlab@9de99878401713bc5f3a76ca85901dc3a9ca0cd8) ([merge request](gitlab-org/security/gitlab!1511)) +- [Sanitize input on pasteGFM](gitlab-org/security/gitlab@7bb97cfa11a11bb0725bc707dec73831e16fe177) ([merge request](gitlab-org/security/gitlab!1514)) +- [Fix merge request diff display issue with unsupported encoding](gitlab-org/security/gitlab@8c21afdce6c6214c14db1863df1aad80ed501377) ([merge request](gitlab-org/security/gitlab!1509)) +- [Fix deploy key fallback issue in protected branch](gitlab-org/security/gitlab@a24aa5412a8f1dad01359de6b2f0b66bb741f5d4) ([merge request](gitlab-org/security/gitlab!1508)) +- [Add total http read timeout](gitlab-org/security/gitlab@cf4e0aa0a3f668fb63de6721d062c3157fdd9f84) ([merge request](gitlab-org/security/gitlab!1507)) +- [Allow only same-origin URLs for Edit Release Cancel button](gitlab-org/security/gitlab@4b78e1e31f0a23b964953b1766d156e12a75115f) ([merge request](gitlab-org/security/gitlab!1506)) +- [Update Nokogiri to 1.11.4](gitlab-org/security/gitlab@c43001973ca1b684b4719f5559819179be2394da) ([merge request](gitlab-org/security/gitlab!1500)) +- [Add new username validation](gitlab-org/security/gitlab@c904a128f2c2262288d00f673294423316318f4d) ([merge request](gitlab-org/security/gitlab!1498)) + ## 14.0.1 (2021-06-24) ### Fixed (3 changes) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 8d2e58b40f0..112969d1040 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -14.0.1
\ No newline at end of file +14.0.2
\ No newline at end of file @@ -157,7 +157,7 @@ gem 'github-markup', '~> 1.7.0', require: 'github/markup' gem 'commonmarker', '~> 0.21' gem 'kramdown', '~> 2.3.1' gem 'RedCloth', '~> 4.3.2' -gem 'rdoc', '~> 6.1.2' +gem 'gitlab-rdoc', '~> 6.3.2', require: 'rdoc' # We need this fork until rdoc releases a new version. See https://gitlab.com/gitlab-org/gitlab/-/issues/334695 gem 'org-ruby', '~> 0.9.12' gem 'creole', '~> 0.5.0' gem 'wikicloth', '0.8.1' @@ -168,7 +168,7 @@ gem 'asciidoctor-kroki', '~> 0.4.0', require: false gem 'rouge', '~> 3.26.0' gem 'truncato', '~> 0.7.11' gem 'bootstrap_form', '~> 4.2.0' -gem 'nokogiri', '~> 1.11.1' +gem 'nokogiri', '~> 1.11.4' gem 'escape_utils', '~> 1.1' # Calendar rendering diff --git a/Gemfile.lock b/Gemfile.lock index ef31d72a5dd..0555f933922 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -500,6 +500,7 @@ GEM openid_connect (~> 1.2) gitlab-pg_query (2.0.4) google-protobuf (>= 3.17.1) + gitlab-rdoc (6.3.2) gitlab-sidekiq-fetcher (0.5.6) sidekiq (~> 5) gitlab-styles (6.2.0) @@ -799,7 +800,7 @@ GEM netrc (0.11.0) nio4r (2.5.4) no_proxy_fix (0.1.2) - nokogiri (1.11.3) + nokogiri (1.11.4) mini_portile2 (~> 2.5.0) racc (~> 1.4) nokogumbo (2.0.2) @@ -1024,7 +1025,6 @@ GEM msgpack (>= 0.4.3) optimist (>= 3.0.0) rchardet (1.8.0) - rdoc (6.1.2) re2 (1.2.0) recaptcha (4.13.1) json @@ -1497,6 +1497,7 @@ DEPENDENCIES gitlab-net-dns (~> 0.9.1) gitlab-omniauth-openid-connect (~> 0.4.0) gitlab-pg_query (~> 2.0.4) + gitlab-rdoc (~> 6.3.2) gitlab-sidekiq-fetcher (= 0.5.6) gitlab-styles (~> 6.2.0) gitlab_chronic_duration (~> 0.10.6.2) @@ -1557,7 +1558,7 @@ DEPENDENCIES net-ldap (~> 0.16.3) net-ntp net-ssh (~> 6.0) - nokogiri (~> 1.11.1) + nokogiri (~> 1.11.4) oauth2 (~> 1.4) octokit (~> 4.15) ohai (~> 16.10) @@ -1605,7 +1606,6 @@ DEPENDENCIES rainbow (~> 3.0) rblineprof (~> 0.3.6) rbtrace (~> 0.4) - rdoc (~> 6.1.2) re2 (~> 1.2.0) recaptcha (~> 4.11) redis (~> 4.0) @@ -1 +1 @@ -14.0.1
\ No newline at end of file +14.0.2
\ No newline at end of file diff --git a/app/assets/javascripts/behaviors/markdown/copy_as_gfm.js b/app/assets/javascripts/behaviors/markdown/copy_as_gfm.js index 9a8af79210e..19ebab36481 100644 --- a/app/assets/javascripts/behaviors/markdown/copy_as_gfm.js +++ b/app/assets/javascripts/behaviors/markdown/copy_as_gfm.js @@ -1,4 +1,5 @@ import $ from 'jquery'; +import { sanitize } from '~/lib/dompurify'; import { getSelectedFragment, insertText } from '~/lib/utils/common_utils'; export class CopyAsGFM { @@ -69,7 +70,7 @@ export class CopyAsGFM { } else { // Due to the async copy call we are not able to produce gfm so we transform the cached HTML const div = document.createElement('div'); - div.innerHTML = gfmHtml; + div.innerHTML = sanitize(gfmHtml); CopyAsGFM.nodeToGFM(div) .then((transformedGfm) => { CopyAsGFM.insertPastedText(e.target, text, transformedGfm); diff --git a/app/assets/javascripts/lib/utils/url_utility.js b/app/assets/javascripts/lib/utils/url_utility.js index 48abc072675..d68b41b7f7a 100644 --- a/app/assets/javascripts/lib/utils/url_utility.js +++ b/app/assets/javascripts/lib/utils/url_utility.js @@ -545,3 +545,27 @@ export function getURLOrigin(url) { return null; } } + +/** + * Returns `true` if the given `url` resolves to the same origin the page is served + * from; otherwise, returns `false`. + * + * The `url` may be absolute or relative. + * + * @param {string} url The URL to check. + * @returns {boolean} + */ +export function isSameOriginUrl(url) { + if (typeof url !== 'string') { + return false; + } + + const { origin } = window.location; + + try { + return new URL(url, origin).origin === origin; + } catch { + // Invalid URLs cannot have the same origin + return false; + } +} diff --git a/app/assets/javascripts/releases/components/app_edit_new.vue b/app/assets/javascripts/releases/components/app_edit_new.vue index aecd0d6371e..3774f97a060 100644 --- a/app/assets/javascripts/releases/components/app_edit_new.vue +++ b/app/assets/javascripts/releases/components/app_edit_new.vue @@ -2,6 +2,7 @@ import { GlButton, GlFormInput, GlFormGroup, GlSprintf } from '@gitlab/ui'; import { mapState, mapActions, mapGetters } from 'vuex'; import { getParameterByName } from '~/lib/utils/common_utils'; +import { isSameOriginUrl } from '~/lib/utils/url_utility'; import { __ } from '~/locale'; import MilestoneCombobox from '~/milestones/components/milestone_combobox.vue'; import { BACK_URL_PARAM } from '~/releases/constants'; @@ -65,7 +66,13 @@ export default { }, }, cancelPath() { - return getParameterByName(BACK_URL_PARAM) || this.releasesPagePath; + const backUrl = getParameterByName(BACK_URL_PARAM); + + if (isSameOriginUrl(backUrl)) { + return backUrl; + } + + return this.releasesPagePath; }, saveButtonLabel() { return this.isExistingRelease ? __('Save changes') : __('Create release'); diff --git a/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/not_diffable.vue b/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/not_diffable.vue index d4d3038f066..5a6b1c19027 100644 --- a/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/not_diffable.vue +++ b/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/not_diffable.vue @@ -1,5 +1,5 @@ <template> <div class="nothing-here-block"> - {{ __('This diff was suppressed by a .gitattributes entry.') }} + {{ __("File suppressed by a .gitattributes entry or the file's encoding is unsupported.") }} </div> </template> diff --git a/app/controllers/graphql_controller.rb b/app/controllers/graphql_controller.rb index 725d8b62c77..515fbd7b482 100644 --- a/app/controllers/graphql_controller.rb +++ b/app/controllers/graphql_controller.rb @@ -20,12 +20,16 @@ class GraphqlController < ApplicationController # around in GraphiQL. protect_from_forgery with: :null_session, only: :execute - before_action :authorize_access_api! + # must come first: current_user is set up here before_action(only: [:execute]) { authenticate_sessionless_user!(:api) } + + before_action :authorize_access_api! before_action :set_user_last_activity before_action :track_vs_code_usage before_action :disable_query_limiting + before_action :disallow_mutations_for_get + # Since we deactivate authentication from the main ApplicationController and # defer it to :authorize_access_api!, we need to override the bypass session # callback execution order here @@ -62,6 +66,25 @@ class GraphqlController < ApplicationController private + def disallow_mutations_for_get + return unless request.get? || request.head? + return unless any_mutating_query? + + raise ::Gitlab::Graphql::Errors::ArgumentError, "Mutations are forbidden in #{request.request_method} requests" + end + + def any_mutating_query? + if multiplex? + multiplex_queries.any? { |q| mutation?(q[:query], q[:operation_name]) } + else + mutation?(query) + end + end + + def mutation?(query_string, operation_name = params[:operationName]) + ::GraphQL::Query.new(GitlabSchema, query_string, operation_name: operation_name).mutation? + end + # Tests may mark some GraphQL queries as exempt from SQL query limits def disable_query_limiting return unless Gitlab::QueryLimiting.enabled_for_env? @@ -130,7 +153,9 @@ class GraphqlController < ApplicationController end def authorize_access_api! - access_denied!("API not accessible for user.") unless can?(current_user, :access_api) + return if can?(current_user, :access_api) + + render_error('API not accessible for user', status: :forbidden) end # Overridden from the ApplicationController to make the response look like diff --git a/app/controllers/ide_controller.rb b/app/controllers/ide_controller.rb index 4c7a91ee602..44beceb4f48 100644 --- a/app/controllers/ide_controller.rb +++ b/app/controllers/ide_controller.rb @@ -7,6 +7,8 @@ class IdeController < ApplicationController include StaticObjectExternalStorageCSP include Gitlab::Utils::StrongMemoize + before_action :authorize_read_project! + before_action do push_frontend_feature_flag(:build_service_proxy) push_frontend_feature_flag(:schema_linting) @@ -22,6 +24,10 @@ class IdeController < ApplicationController private + def authorize_read_project! + render_404 unless can?(current_user, :read_project, project) + end + def define_index_vars return unless project diff --git a/app/graphql/mutations/echo.rb b/app/graphql/mutations/echo.rb new file mode 100644 index 00000000000..61d39009ba4 --- /dev/null +++ b/app/graphql/mutations/echo.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module Mutations + class Echo < BaseMutation + graphql_name 'EchoCreate' + description <<~DOC + A mutation that does not perform any changes. + + This is expected to be used for testing of endpoints, to verify + that a user has mutation access. + DOC + + argument :errors, + type: [::GraphQL::STRING_TYPE], + required: false, + description: 'Errors to return to the user.' + + argument :messages, + type: [::GraphQL::STRING_TYPE], + as: :echoes, + required: false, + description: 'Messages to return to the user.' + + field :echoes, + type: [::GraphQL::STRING_TYPE], + null: true, + description: 'Messages returned to the user.' + + def resolve(**args) + args + end + end +end diff --git a/app/graphql/types/mutation_type.rb b/app/graphql/types/mutation_type.rb index 6b1146f8f09..6d3327f9735 100644 --- a/app/graphql/types/mutation_type.rb +++ b/app/graphql/types/mutation_type.rb @@ -104,6 +104,7 @@ module Types mount_mutation Mutations::Ci::RunnersRegistrationToken::Reset, feature_flag: :runner_graphql_query mount_mutation Mutations::Namespace::PackageSettings::Update mount_mutation Mutations::UserCallouts::Create + mount_mutation Mutations::Echo end end diff --git a/app/models/audit_event.rb b/app/models/audit_event.rb index aff7eef4622..11036b76fc1 100644 --- a/app/models/audit_event.rb +++ b/app/models/audit_event.rb @@ -32,6 +32,9 @@ class AuditEvent < ApplicationRecord scope :by_author_id, -> (author_id) { where(author_id: author_id) } after_initialize :initialize_details + + before_validation :sanitize_message + # Note: The intention is to remove this once refactoring of AuditEvent # has proceeded further. # @@ -83,6 +86,14 @@ class AuditEvent < ApplicationRecord private + def sanitize_message + message = details[:custom_message] + + return unless message + + self.details = details.merge(custom_message: Sanitize.clean(message)) + end + def default_author_value ::Gitlab::Audit::NullAuthor.for(author_id, (self[:author_name] || details[:author_name])) end diff --git a/app/models/concerns/integrations/slack_mattermost_notifier.rb b/app/models/concerns/integrations/slack_mattermost_notifier.rb index a919fc840fd..cb6fafa8de0 100644 --- a/app/models/concerns/integrations/slack_mattermost_notifier.rb +++ b/app/models/concerns/integrations/slack_mattermost_notifier.rb @@ -17,7 +17,7 @@ module Integrations class HTTPClient def self.post(uri, params = {}) params.delete(:http_options) # these are internal to the client and we do not want them - Gitlab::HTTP.post(uri, body: params) + Gitlab::HTTP.post(uri, body: params, use_read_total_timeout: true) end end end diff --git a/app/models/integrations/bamboo.rb b/app/models/integrations/bamboo.rb index dbd7aedf4fe..fef2774c593 100644 --- a/app/models/integrations/bamboo.rb +++ b/app/models/integrations/bamboo.rb @@ -173,6 +173,7 @@ module Integrations query_params[:os_authType] = 'basic' params[:basic_auth] = basic_auth + params[:use_read_total_timeout] = true params end diff --git a/app/models/integrations/base_issue_tracker.rb b/app/models/integrations/base_issue_tracker.rb index 6c24f762cd5..3fd67205e92 100644 --- a/app/models/integrations/base_issue_tracker.rb +++ b/app/models/integrations/base_issue_tracker.rb @@ -107,7 +107,7 @@ module Integrations result = false begin - response = Gitlab::HTTP.head(self.project_url, verify: true) + response = Gitlab::HTTP.head(self.project_url, verify: true, use_read_total_timeout: true) if response message = "#{self.type} received response #{response.code} when attempting to connect to #{self.project_url}" diff --git a/app/models/integrations/drone_ci.rb b/app/models/integrations/drone_ci.rb index 096f7093b8c..0f021356815 100644 --- a/app/models/integrations/drone_ci.rb +++ b/app/models/integrations/drone_ci.rb @@ -51,9 +51,12 @@ module Integrations end def calculate_reactive_cache(sha, ref) - response = Gitlab::HTTP.try_get(commit_status_path(sha, ref), + response = Gitlab::HTTP.try_get( + commit_status_path(sha, ref), verify: enable_ssl_verification, - extra_log_info: { project_id: project_id }) + extra_log_info: { project_id: project_id }, + use_read_total_timeout: true + ) status = if response && response.code == 200 && response['status'] diff --git a/app/models/integrations/external_wiki.rb b/app/models/integrations/external_wiki.rb index fec435443fa..2a8d598117b 100644 --- a/app/models/integrations/external_wiki.rb +++ b/app/models/integrations/external_wiki.rb @@ -39,7 +39,7 @@ module Integrations end def execute(_data) - response = Gitlab::HTTP.get(properties['external_wiki_url'], verify: true) + response = Gitlab::HTTP.get(properties['external_wiki_url'], verify: true, use_read_total_timeout: true) response.body if response.code == 200 rescue StandardError nil diff --git a/app/models/integrations/mock_ci.rb b/app/models/integrations/mock_ci.rb index d31f6381767..a0eae9e4abf 100644 --- a/app/models/integrations/mock_ci.rb +++ b/app/models/integrations/mock_ci.rb @@ -55,7 +55,7 @@ module Integrations # # => 'running' # def commit_status(sha, ref) - response = Gitlab::HTTP.get(commit_status_path(sha), verify: false) + response = Gitlab::HTTP.get(commit_status_path(sha), verify: false, use_read_total_timeout: true) read_commit_status(response) rescue Errno::ECONNREFUSED :error diff --git a/app/models/integrations/teamcity.rb b/app/models/integrations/teamcity.rb index 8284d5963ae..3f14c5d82b3 100644 --- a/app/models/integrations/teamcity.rb +++ b/app/models/integrations/teamcity.rb @@ -170,7 +170,7 @@ module Integrations end def get_path(path) - Gitlab::HTTP.try_get(build_url(path), verify: false, basic_auth: basic_auth, extra_log_info: { project_id: project_id }) + Gitlab::HTTP.try_get(build_url(path), verify: false, basic_auth: basic_auth, extra_log_info: { project_id: project_id }, use_read_total_timeout: true) end def post_to_build_queue(data, branch) @@ -180,7 +180,8 @@ module Integrations "<buildType id=#{build_type.encode(xml: :attr)}/>"\ '</build>', headers: { 'Content-type' => 'application/xml' }, - basic_auth: basic_auth + basic_auth: basic_auth, + use_read_total_timeout: true ) end diff --git a/app/models/integrations/unify_circuit.rb b/app/models/integrations/unify_circuit.rb index 03363c7c8b0..834222834e9 100644 --- a/app/models/integrations/unify_circuit.rb +++ b/app/models/integrations/unify_circuit.rb @@ -49,7 +49,8 @@ module Integrations response = Gitlab::HTTP.post(webhook, body: { subject: message.project_name, text: message.summary, - markdown: true + markdown: true, + use_read_total_timeout: true }.to_json) response if response.success? diff --git a/app/models/integrations/webex_teams.rb b/app/models/integrations/webex_teams.rb index 3f420331035..6fd82a32035 100644 --- a/app/models/integrations/webex_teams.rb +++ b/app/models/integrations/webex_teams.rb @@ -44,7 +44,7 @@ module Integrations def notify(message, opts) header = { 'Content-Type' => 'application/json' } - response = Gitlab::HTTP.post(webhook, headers: header, body: { markdown: message.summary }.to_json) + response = Gitlab::HTTP.post(webhook, headers: header, body: { markdown: message.summary }.to_json, use_read_total_timeout: true) response if response.success? end diff --git a/app/models/protected_branch/push_access_level.rb b/app/models/protected_branch/push_access_level.rb index ea51dca8a42..5248834a2f2 100644 --- a/app/models/protected_branch/push_access_level.rb +++ b/app/models/protected_branch/push_access_level.rb @@ -20,7 +20,7 @@ class ProtectedBranch::PushAccessLevel < ApplicationRecord def check_access(user) if user && deploy_key.present? - return true if user.can?(:read_project, project) && enabled_deploy_key_for_user?(deploy_key, user) + return user.can?(:read_project, project) && enabled_deploy_key_for_user?(deploy_key, user) end super diff --git a/app/models/user.rb b/app/models/user.rb index 5fbd6271589..52bf9149ee2 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -236,6 +236,7 @@ class User < ApplicationRecord validate :owns_commit_email, if: :commit_email_changed? validate :signup_domain_valid?, on: :create, if: ->(user) { !user.created_by_id } validate :check_email_restrictions, on: :create, if: ->(user) { !user.created_by_id } + validate :check_username_format, if: :username_changed? validates :theme_id, allow_nil: true, inclusion: { in: Gitlab::Themes.valid_ids, message: _("%{placeholder} is not a valid theme") % { placeholder: '%{value}' } } @@ -1257,12 +1258,23 @@ class User < ApplicationRecord end def sanitize_attrs + sanitize_links + sanitize_name + end + + def sanitize_links %i[skype linkedin twitter].each do |attr| value = self[attr] self[attr] = Sanitize.clean(value) if value.present? end end + def sanitize_name + return unless self.name + + self.name = self.name.gsub(%r{</?[^>]*>}, '') + end + def set_notification_email if notification_email.blank? || all_emails.exclude?(notification_email) self.notification_email = email @@ -2082,6 +2094,12 @@ class User < ApplicationRecord end end + def check_username_format + return if username.blank? || Mime::EXTENSION_LOOKUP.keys.none? { |type| username.end_with?(type) } + + errors.add(:username, _('ending with MIME type format is not allowed.')) + end + def groups_with_developer_maintainer_project_access project_creation_levels = [::Gitlab::Access::DEVELOPER_MAINTAINER_PROJECT_ACCESS] diff --git a/app/services/feature_flags/base_service.rb b/app/services/feature_flags/base_service.rb index f48f95e2550..d041703803b 100644 --- a/app/services/feature_flags/base_service.rb +++ b/app/services/feature_flags/base_service.rb @@ -49,9 +49,9 @@ module FeatureFlags end def created_scope_message(scope) - "Created rule <strong>#{scope.environment_scope}</strong> "\ - "and set it as <strong>#{scope.active ? "active" : "inactive"}</strong> "\ - "with strategies <strong>#{scope.strategies}</strong>." + "Created rule #{scope.environment_scope} "\ + "and set it as #{scope.active ? "active" : "inactive"} "\ + "with strategies #{scope.strategies}." end def feature_flag_by_name diff --git a/app/services/feature_flags/create_service.rb b/app/services/feature_flags/create_service.rb index de3a55d10fc..5c87af561d5 100644 --- a/app/services/feature_flags/create_service.rb +++ b/app/services/feature_flags/create_service.rb @@ -22,8 +22,7 @@ module FeatureFlags private def audit_message(feature_flag) - message_parts = ["Created feature flag <strong>#{feature_flag.name}</strong>", - "with description <strong>\"#{feature_flag.description}\"</strong>."] + message_parts = ["Created feature flag #{feature_flag.name} with description \"#{feature_flag.description}\"."] message_parts += feature_flag.scopes.map do |scope| created_scope_message(scope) diff --git a/app/services/feature_flags/destroy_service.rb b/app/services/feature_flags/destroy_service.rb index c77e3e03ec3..b131a349fc7 100644 --- a/app/services/feature_flags/destroy_service.rb +++ b/app/services/feature_flags/destroy_service.rb @@ -23,7 +23,7 @@ module FeatureFlags end def audit_message(feature_flag) - "Deleted feature flag <strong>#{feature_flag.name}</strong>." + "Deleted feature flag #{feature_flag.name}." end def can_destroy?(feature_flag) diff --git a/app/services/feature_flags/update_service.rb b/app/services/feature_flags/update_service.rb index d956d4b3357..f5ab6f4005b 100644 --- a/app/services/feature_flags/update_service.rb +++ b/app/services/feature_flags/update_service.rb @@ -45,14 +45,14 @@ module FeatureFlags return if changes.empty? - "Updated feature flag <strong>#{feature_flag.name}</strong>. " + changes.join(" ") + "Updated feature flag #{feature_flag.name}. " + changes.join(" ") end def changed_attributes_messages(feature_flag) feature_flag.changes.slice(*AUDITABLE_ATTRIBUTES).map do |attribute_name, changes| "Updated #{attribute_name} "\ - "from <strong>\"#{changes.first}\"</strong> to "\ - "<strong>\"#{changes.second}\"</strong>." + "from \"#{changes.first}\" to "\ + "\"#{changes.second}\"." end end @@ -69,17 +69,17 @@ module FeatureFlags end def deleted_scope_message(scope) - "Deleted rule <strong>#{scope.environment_scope}</strong>." + "Deleted rule #{scope.environment_scope}." end def updated_scope_message(scope) changes = scope.changes.slice(*AUDITABLE_SCOPE_ATTRIBUTES_HUMAN_NAMES.keys) return if changes.empty? - message = "Updated rule <strong>#{scope.environment_scope}</strong> " + message = "Updated rule #{scope.environment_scope} " message += changes.map do |attribute_name, change| name = AUDITABLE_SCOPE_ATTRIBUTES_HUMAN_NAMES[attribute_name] - "#{name} from <strong>#{change.first}</strong> to <strong>#{change.second}</strong>" + "#{name} from #{change.first} to #{change.second}" end.join(' ') message + '.' diff --git a/app/services/projects/fork_service.rb b/app/services/projects/fork_service.rb index fd9b64a4ee0..3cee1b5975a 100644 --- a/app/services/projects/fork_service.rb +++ b/app/services/projects/fork_service.rb @@ -34,8 +34,9 @@ module Projects new_project = CreateService.new(current_user, new_fork_params).execute return new_project unless new_project.persisted? - builds_access_level = @project.project_feature.builds_access_level - new_project.project_feature.update(builds_access_level: builds_access_level) + new_project.project_feature.update!( + @project.project_feature.slice(ProjectFeature::FEATURES.map { |f| "#{f}_access_level" }) + ) new_project end diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index 77d2139b3d1..1d5b38575bb 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -42,6 +42,7 @@ class WebHookService @uniqueness_token = uniqueness_token @request_options = { timeout: Gitlab.config.gitlab.webhook_timeout, + use_read_total_timeout: true, allow_local_requests: hook.allow_local_requests? } end @@ -68,7 +69,7 @@ class WebHookService { status: :success, http_status: response.code, - message: response.to_s + message: response.body } rescue *Gitlab::HTTP::HTTP_ERRORS, Gitlab::Json::LimitedEncoder::LimitExceeded, URI::InvalidURIError => e diff --git a/app/views/projects/diffs/viewers/_not_diffable.html.haml b/app/views/projects/diffs/viewers/_not_diffable.html.haml index 7c55e272f56..63034331f6a 100644 --- a/app/views/projects/diffs/viewers/_not_diffable.html.haml +++ b/app/views/projects/diffs/viewers/_not_diffable.html.haml @@ -1,2 +1,2 @@ .nothing-here-block - = _("This diff was suppressed by a .gitattributes entry.") + = _("File suppressed by a .gitattributes entry or the file's encoding is unsupported.") diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index e84769fa568..ffdf0ea5fd3 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -1895,6 +1895,31 @@ Input type: `DiscussionToggleResolveInput` | <a id="mutationdiscussiontoggleresolvediscussion"></a>`discussion` | [`Discussion`](#discussion) | The discussion after mutation. | | <a id="mutationdiscussiontoggleresolveerrors"></a>`errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | +### `Mutation.echoCreate` + +A mutation that does not perform any changes. + +This is expected to be used for testing of endpoints, to verify +that a user has mutation access. + +Input type: `EchoCreateInput` + +#### Arguments + +| Name | Type | Description | +| ---- | ---- | ----------- | +| <a id="mutationechocreateclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| <a id="mutationechocreateerrors"></a>`errors` | [`[String!]`](#string) | Errors to return to the user. | +| <a id="mutationechocreatemessages"></a>`messages` | [`[String!]`](#string) | Messages to return to the user. | + +#### Fields + +| Name | Type | Description | +| ---- | ---- | ----------- | +| <a id="mutationechocreateclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| <a id="mutationechocreateechoes"></a>`echoes` | [`[String!]`](#string) | Messages returned to the user. | +| <a id="mutationechocreateerrors"></a>`errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | + ### `Mutation.enableDevopsAdoptionNamespace` **BETA** This endpoint is subject to change without notice. diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index dcd4bbdabf5..35581952f4a 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -250,7 +250,7 @@ module Gitlab end def diffable? - repository.attributes(file_path).fetch('diff') { true } + diffable_by_attribute? && !text_with_binary_notice? end def binary_in_repo? @@ -366,6 +366,15 @@ module Gitlab private + def diffable_by_attribute? + repository.attributes(file_path).fetch('diff') { true } + end + + # NOTE: Files with unsupported encodings (e.g. UTF-16) are treated as binary by git, but they are recognized as text files during encoding detection. These files have `Binary files a/filename and b/filename differ' as their raw diff content which cannot be used. We need to handle this special case and avoid displaying incorrect diff. + def text_with_binary_notice? + text? && has_binary_notice? + end + def fetch_blob(sha, path) return unless sha diff --git a/lib/gitlab/diff/parser.rb b/lib/gitlab/diff/parser.rb index 4a47e4b80b6..adb711ca89f 100644 --- a/lib/gitlab/diff/parser.rb +++ b/lib/gitlab/diff/parser.rb @@ -6,7 +6,7 @@ module Gitlab include Enumerable def parse(lines, diff_file: nil) - return [] if lines.blank? + return [] if lines.blank? || Git::Diff.has_binary_notice?(lines.first) @lines = lines line_obj_index = 0 diff --git a/lib/gitlab/git/diff.rb b/lib/gitlab/git/diff.rb index 53df0b7b389..8325eadce2f 100644 --- a/lib/gitlab/git/diff.rb +++ b/lib/gitlab/git/diff.rb @@ -33,6 +33,8 @@ module Gitlab SERIALIZE_KEYS = %i(diff new_path old_path a_mode b_mode new_file renamed_file deleted_file too_large).freeze + BINARY_NOTICE_PATTERN = %r(Binary files a\/(.*) and b\/(.*) differ).freeze + class << self def between(repo, head, base, options = {}, *paths) straight = options.delete(:straight) || false @@ -131,8 +133,13 @@ module Gitlab def patch_hard_limit_bytes Gitlab::CurrentSettings.diff_max_patch_bytes end - end + def has_binary_notice?(text) + return false unless text.present? + + text.start_with?(BINARY_NOTICE_PATTERN) + end + end def initialize(raw_diff, expanded: true) @expanded = expanded @@ -215,7 +222,7 @@ module Gitlab end def has_binary_notice? - @diff.start_with?('Binary') + self.class.has_binary_notice?(@diff) end private diff --git a/lib/gitlab/http.rb b/lib/gitlab/http.rb index be87dcc0ff9..7e45cd216f5 100644 --- a/lib/gitlab/http.rb +++ b/lib/gitlab/http.rb @@ -8,9 +8,10 @@ module Gitlab class HTTP BlockedUrlError = Class.new(StandardError) RedirectionTooDeep = Class.new(StandardError) + ReadTotalTimeout = Class.new(Net::ReadTimeout) HTTP_TIMEOUT_ERRORS = [ - Net::OpenTimeout, Net::ReadTimeout, Net::WriteTimeout + Net::OpenTimeout, Net::ReadTimeout, Net::WriteTimeout, Gitlab::HTTP::ReadTotalTimeout ].freeze HTTP_ERRORS = HTTP_TIMEOUT_ERRORS + [ SocketError, OpenSSL::SSL::SSLError, OpenSSL::OpenSSLError, @@ -23,6 +24,7 @@ module Gitlab read_timeout: 20, write_timeout: 30 }.freeze + DEFAULT_READ_TOTAL_TIMEOUT = 20.seconds include HTTParty # rubocop:disable Gitlab/HTTParty @@ -41,7 +43,19 @@ module Gitlab options end - httparty_perform_request(http_method, path, options_with_timeouts, &block) + unless options.has_key?(:use_read_total_timeout) + return httparty_perform_request(http_method, path, options_with_timeouts, &block) + end + + start_time = Gitlab::Metrics::System.monotonic_time + read_total_timeout = options.fetch(:timeout, DEFAULT_READ_TOTAL_TIMEOUT) + + httparty_perform_request(http_method, path, options_with_timeouts) do |fragment| + elapsed = Gitlab::Metrics::System.monotonic_time - start_time + raise ReadTotalTimeout, "Request timed out after #{elapsed} seconds" if elapsed > read_total_timeout + + block.call fragment if block + end rescue HTTParty::RedirectionTooDeep raise RedirectionTooDeep rescue *HTTP_ERRORS => e diff --git a/locale/gitlab.pot b/locale/gitlab.pot index f34bea613e8..feb3d972d2a 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -14032,6 +14032,9 @@ msgstr "" msgid "File renamed with no changes." msgstr "" +msgid "File suppressed by a .gitattributes entry or the file's encoding is unsupported." +msgstr "" + msgid "File synchronization concurrency limit" msgstr "" @@ -33273,9 +33276,6 @@ msgstr "" msgid "This diff is collapsed." msgstr "" -msgid "This diff was suppressed by a .gitattributes entry." -msgstr "" - msgid "This directory" msgstr "" @@ -38542,6 +38542,9 @@ msgstr "" msgid "encrypted: needs to be a :required, :optional or :migrating!" msgstr "" +msgid "ending with MIME type format is not allowed." +msgstr "" + msgid "entries cannot be larger than 255 characters" msgstr "" diff --git a/spec/controllers/graphql_controller_spec.rb b/spec/controllers/graphql_controller_spec.rb index f2d86b1b166..aed97a01a72 100644 --- a/spec/controllers/graphql_controller_spec.rb +++ b/spec/controllers/graphql_controller_spec.rb @@ -44,7 +44,7 @@ RSpec.describe GraphqlController do expect(response).to have_gitlab_http_status(:ok) end - it 'returns access denied template when user cannot access API' do + it 'returns forbidden when user cannot access API' do # User cannot access API in a couple of cases # * When user is internal(like ghost users) # * When user is blocked @@ -54,7 +54,9 @@ RSpec.describe GraphqlController do post :execute expect(response).to have_gitlab_http_status(:forbidden) - expect(response).to render_template('errors/access_denied') + expect(json_response).to include( + 'errors' => include(a_hash_including('message' => /API not accessible/)) + ) end it 'updates the users last_activity_on field' do diff --git a/spec/features/expand_collapse_diffs_spec.rb b/spec/features/expand_collapse_diffs_spec.rb index cbd1ae628d1..add4af2bcdb 100644 --- a/spec/features/expand_collapse_diffs_spec.rb +++ b/spec/features/expand_collapse_diffs_spec.rb @@ -253,7 +253,7 @@ RSpec.describe 'Expand and collapse diffs', :js do click_link('Expand all') # Wait for elements to appear to ensure full page reload - expect(page).to have_content('This diff was suppressed by a .gitattributes entry') + expect(page).to have_content("File suppressed by a .gitattributes entry or the file's encoding is unsupported.") expect(page).to have_content('This source diff could not be displayed because it is too large.') expect(page).to have_content('too_large_image.jpg') find('.note-textarea') diff --git a/spec/features/projects/diffs/diff_show_spec.rb b/spec/features/projects/diffs/diff_show_spec.rb index e47f36c4b7a..56506ada3ce 100644 --- a/spec/features/projects/diffs/diff_show_spec.rb +++ b/spec/features/projects/diffs/diff_show_spec.rb @@ -174,4 +174,14 @@ RSpec.describe 'Diff file viewer', :js, :with_clean_rails_cache do end end end + + context 'when the the encoding of the file is unsupported' do + before do + visit_commit('f05a98786e4274708e1fa118c7ad3a29d1d1b9a3') + end + + it 'shows it is not diffable' do + expect(page).to have_content("File suppressed by a .gitattributes entry or the file's encoding is unsupported.") + end + end end diff --git a/spec/features/snippets/notes_on_personal_snippets_spec.rb b/spec/features/snippets/notes_on_personal_snippets_spec.rb index 47dad9bd88e..e03f71c5352 100644 --- a/spec/features/snippets/notes_on_personal_snippets_spec.rb +++ b/spec/features/snippets/notes_on_personal_snippets_spec.rb @@ -65,18 +65,6 @@ RSpec.describe 'Comments on personal snippets', :js do expect(page).to have_content(user_name) end end - - context 'when the author name contains HTML' do - let(:user_name) { '<h1><a href="https://bad.link/malicious.exe" class="evil">Fake Content<img class="fake-icon" src="image.png"></a></h1>' } - - it 'renders the name as plain text' do - visit snippet_path(snippet) - - content = find("#note_#{snippet_notes[0].id} .note-header-author-name").text - - expect(content).to eq user_name - end - end end context 'when submitting a note' do diff --git a/spec/frontend/behaviors/copy_as_gfm_spec.js b/spec/frontend/behaviors/copy_as_gfm_spec.js index acff990e84a..557b609f5f9 100644 --- a/spec/frontend/behaviors/copy_as_gfm_spec.js +++ b/spec/frontend/behaviors/copy_as_gfm_spec.js @@ -1,50 +1,54 @@ import initCopyAsGFM, { CopyAsGFM } from '~/behaviors/markdown/copy_as_gfm'; -import * as commonUtils from '~/lib/utils/common_utils'; describe('CopyAsGFM', () => { describe('CopyAsGFM.pasteGFM', () => { - function callPasteGFM() { + let target; + + beforeEach(() => { + target = document.createElement('input'); + target.value = 'This is code: '; + }); + + // When GFM code is copied, we put the regular plain text + // on the clipboard as `text/plain`, and the GFM as `text/x-gfm`. + // This emulates the behavior of `getData` with that data. + function callPasteGFM(data = { 'text/plain': 'code', 'text/x-gfm': '`code`' }) { const e = { originalEvent: { clipboardData: { getData(mimeType) { - // When GFM code is copied, we put the regular plain text - // on the clipboard as `text/plain`, and the GFM as `text/x-gfm`. - // This emulates the behavior of `getData` with that data. - if (mimeType === 'text/plain') { - return 'code'; - } - if (mimeType === 'text/x-gfm') { - return '`code`'; - } - return null; + return data[mimeType] || null; }, }, }, preventDefault() {}, + target, }; CopyAsGFM.pasteGFM(e); } it('wraps pasted code when not already in code tags', () => { - jest.spyOn(commonUtils, 'insertText').mockImplementation((el, textFunc) => { - const insertedText = textFunc('This is code: ', ''); + callPasteGFM(); - expect(insertedText).toEqual('`code`'); - }); + expect(target.value).toBe('This is code: `code`'); + }); + + it('does not wrap pasted code when already in code tags', () => { + target.value = 'This is code: `'; callPasteGFM(); + + expect(target.value).toBe('This is code: `code'); }); - it('does not wrap pasted code when already in code tags', () => { - jest.spyOn(commonUtils, 'insertText').mockImplementation((el, textFunc) => { - const insertedText = textFunc('This is code: `', '`'); + it('does not allow xss in x-gfm-html', () => { + const testEl = document.createElement('div'); + jest.spyOn(document, 'createElement').mockReturnValueOnce(testEl); - expect(insertedText).toEqual('code'); - }); + callPasteGFM({ 'text/plain': 'code', 'text/x-gfm-html': 'code<img/src/onerror=alert(1)>' }); - callPasteGFM(); + expect(testEl.innerHTML).toBe('code<img src="">'); }); }); diff --git a/spec/frontend/lib/utils/url_utility_spec.js b/spec/frontend/lib/utils/url_utility_spec.js index 305d3de3c53..31c78681994 100644 --- a/spec/frontend/lib/utils/url_utility_spec.js +++ b/spec/frontend/lib/utils/url_utility_spec.js @@ -1,3 +1,4 @@ +import { TEST_HOST } from 'helpers/test_constants'; import * as urlUtils from '~/lib/utils/url_utility'; const shas = { @@ -923,4 +924,37 @@ describe('URL utility', () => { expect(urlUtils.encodeSaferUrl(input)).toBe(input); }); }); + + describe('isSameOriginUrl', () => { + // eslint-disable-next-line no-script-url + const javascriptUrl = 'javascript:alert(1)'; + + beforeEach(() => { + setWindowLocation({ origin: TEST_HOST }); + }); + + it.each` + url | expected + ${TEST_HOST} | ${true} + ${`${TEST_HOST}/a/path`} | ${true} + ${'//test.host/no-protocol'} | ${true} + ${'/a/root/relative/path'} | ${true} + ${'a/relative/path'} | ${true} + ${'#hash'} | ${true} + ${'?param=foo'} | ${true} + ${''} | ${true} + ${'../../../'} | ${true} + ${`${TEST_HOST}:8080/wrong-port`} | ${false} + ${'ws://test.host/wrong-protocol'} | ${false} + ${'http://phishing.test'} | ${false} + ${'//phishing.test'} | ${false} + ${'//invalid:url'} | ${false} + ${javascriptUrl} | ${false} + ${'data:,Hello%2C%20World%21'} | ${false} + ${null} | ${false} + ${undefined} | ${false} + `('returns $expected given $url', ({ url, expected }) => { + expect(urlUtils.isSameOriginUrl(url)).toBe(expected); + }); + }); }); diff --git a/spec/frontend/releases/components/app_edit_new_spec.js b/spec/frontend/releases/components/app_edit_new_spec.js index 65ed6d6166f..748b48dacaa 100644 --- a/spec/frontend/releases/components/app_edit_new_spec.js +++ b/spec/frontend/releases/components/app_edit_new_spec.js @@ -4,6 +4,7 @@ import MockAdapter from 'axios-mock-adapter'; import { merge } from 'lodash'; import Vuex from 'vuex'; import { getJSONFixture } from 'helpers/fixtures'; +import { TEST_HOST } from 'helpers/test_constants'; import * as commonUtils from '~/lib/utils/common_utils'; import ReleaseEditNewApp from '~/releases/components/app_edit_new.vue'; import AssetLinksForm from '~/releases/components/asset_links_form.vue'; @@ -11,6 +12,7 @@ import { BACK_URL_PARAM } from '~/releases/constants'; const originalRelease = getJSONFixture('api/releases/release.json'); const originalMilestones = originalRelease.milestones; +const releasesPagePath = 'path/to/releases/page'; describe('Release edit/new component', () => { let wrapper; @@ -24,7 +26,7 @@ describe('Release edit/new component', () => { state = { release, markdownDocsPath: 'path/to/markdown/docs', - releasesPagePath: 'path/to/releases/page', + releasesPagePath, projectId: '8', groupId: '42', groupMilestonesAvailable: true, @@ -75,6 +77,8 @@ describe('Release edit/new component', () => { }; beforeEach(() => { + global.jsdom.reconfigure({ url: TEST_HOST }); + mock = new MockAdapter(axios); gon.api_version = 'v4'; @@ -146,22 +150,33 @@ describe('Release edit/new component', () => { }); }); - describe(`when the URL contains a "${BACK_URL_PARAM}" parameter`, () => { - const backUrl = 'https://example.gitlab.com/back/url'; - - beforeEach(async () => { - commonUtils.getParameterByName = jest - .fn() - .mockImplementation((paramToGet) => ({ [BACK_URL_PARAM]: backUrl }[paramToGet])); + // eslint-disable-next-line no-script-url + const xssBackUrl = 'javascript:alert(1)'; + describe.each` + backUrl | expectedHref + ${`${TEST_HOST}/back/url`} | ${`${TEST_HOST}/back/url`} + ${`/back/url?page=2`} | ${`/back/url?page=2`} + ${`back/url?page=3`} | ${`back/url?page=3`} + ${'http://phishing.test/back/url'} | ${releasesPagePath} + ${'//phishing.test/back/url'} | ${releasesPagePath} + ${xssBackUrl} | ${releasesPagePath} + `( + `when the URL contains a "${BACK_URL_PARAM}=$backUrl" parameter`, + ({ backUrl, expectedHref }) => { + beforeEach(async () => { + global.jsdom.reconfigure({ + url: `${TEST_HOST}?${BACK_URL_PARAM}=${encodeURIComponent(backUrl)}`, + }); - await factory(); - }); + await factory(); + }); - it('renders a "Cancel" button with an href pointing to the main Releases page', () => { - const cancelButton = wrapper.find('.js-cancel-button'); - expect(cancelButton.attributes().href).toBe(backUrl); - }); - }); + it(`renders a "Cancel" button with an href pointing to ${expectedHref}`, () => { + const cancelButton = wrapper.find('.js-cancel-button'); + expect(cancelButton.attributes().href).toBe(expectedHref); + }); + }, + ); describe('when creating a new release', () => { beforeEach(async () => { diff --git a/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb b/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb index 03a9b9bd21e..d401c42fed7 100644 --- a/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb +++ b/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb @@ -23,7 +23,7 @@ RSpec.describe Gitlab::Diff::FileCollection::MergeRequestDiff do it 'does not highlight files marked as undiffable in .gitattributes' do allow_next_instance_of(Gitlab::Diff::File) do |instance| - allow(instance).to receive(:diffable?).and_return(false) + allow(instance).to receive(:diffable_by_attribute?).and_return(false) end expect_next_instance_of(Gitlab::Diff::File) do |instance| diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb index 78be89c449b..1800d2d6b60 100644 --- a/spec/lib/gitlab/diff/file_spec.rb +++ b/spec/lib/gitlab/diff/file_spec.rb @@ -186,26 +186,46 @@ RSpec.describe Gitlab::Diff::File do end describe '#diffable?' do - let(:commit) { project.commit('1a0b36b3cdad1d2ee32457c102a8c0b7056fa863') } - let(:diffs) { commit.diffs } + context 'when attributes exist' do + let(:commit) { project.commit('1a0b36b3cdad1d2ee32457c102a8c0b7056fa863') } + let(:diffs) { commit.diffs } - before do - info_dir_path = Gitlab::GitalyClient::StorageSettings.allow_disk_access do - File.join(project.repository.path_to_repo, 'info') + before do + info_dir_path = Gitlab::GitalyClient::StorageSettings.allow_disk_access do + File.join(project.repository.path_to_repo, 'info') + end + + FileUtils.mkdir(info_dir_path) unless File.exist?(info_dir_path) + File.write(File.join(info_dir_path, 'attributes'), "*.md -diff\n") end - FileUtils.mkdir(info_dir_path) unless File.exist?(info_dir_path) - File.write(File.join(info_dir_path, 'attributes'), "*.md -diff\n") + it "returns true for files that do not have attributes" do + diff_file = diffs.diff_file_with_new_path('LICENSE') + expect(diff_file.diffable?).to be_truthy + end + + it "returns false for files that have been marked as not being diffable in attributes" do + diff_file = diffs.diff_file_with_new_path('README.md') + expect(diff_file.diffable?).to be_falsey + end end - it "returns true for files that do not have attributes" do - diff_file = diffs.diff_file_with_new_path('LICENSE') - expect(diff_file.diffable?).to be_truthy + context 'when the text has binary notice' do + let(:commit) { project.commit('f05a98786e4274708e1fa118c7ad3a29d1d1b9a3') } + let(:diff_file) { commit.diffs.diff_file_with_new_path('VERSION') } + + it "returns false" do + expect(diff_file.diffable?).to be_falsey + end end - it "returns false for files that have been marked as not being diffable in attributes" do - diff_file = diffs.diff_file_with_new_path('README.md') - expect(diff_file.diffable?).to be_falsey + context 'when the content is binary' do + let(:commit) { project.commit('2f63565e7aac07bcdadb654e253078b727143ec4') } + let(:diff_file) { commit.diffs.diff_file_with_new_path('files/images/6049019_460s.jpg') } + + it "returns true" do + expect(diff_file.diffable?).to be_truthy + end end end @@ -729,6 +749,18 @@ RSpec.describe Gitlab::Diff::File do end end + context 'when the the encoding of the file is unsupported' do + let(:commit) { project.commit('f05a98786e4274708e1fa118c7ad3a29d1d1b9a3') } + let(:diff_file) { commit.diffs.diff_file_with_new_path('VERSION') } + + it 'returns a Not Diffable viewer' do + expect(diff_file.simple_viewer).to be_a(DiffViewer::NotDiffable) + end + + it { expect(diff_file.highlighted_diff_lines).to eq([]) } + it { expect(diff_file.parallel_diff_lines).to eq([]) } + end + describe '#diff_hunk' do context 'when first line is a match' do let(:raw_diff) do diff --git a/spec/lib/gitlab/diff/parser_spec.rb b/spec/lib/gitlab/diff/parser_spec.rb index 7448ae0b2ea..c8069f82f04 100644 --- a/spec/lib/gitlab/diff/parser_spec.rb +++ b/spec/lib/gitlab/diff/parser_spec.rb @@ -146,6 +146,16 @@ eos it { expect(parser.parse(nil)).to eq([]) } end + context 'when it is a binary notice' do + let(:diff) do + <<~END + Binary files a/test and b/test differ + END + end + + it { expect(parser.parse(diff.each_line)).to eq([]) } + end + describe 'tolerates special diff markers in a content' do it "counts lines correctly" do diff = <<~END diff --git a/spec/lib/gitlab/http_spec.rb b/spec/lib/gitlab/http_spec.rb index 308f7f46251..71e80de9f89 100644 --- a/spec/lib/gitlab/http_spec.rb +++ b/spec/lib/gitlab/http_spec.rb @@ -27,6 +27,47 @@ RSpec.describe Gitlab::HTTP do end end + context 'when reading the response is too slow' do + before do + stub_const("#{described_class}::DEFAULT_READ_TOTAL_TIMEOUT", 0.001.seconds) + + WebMock.stub_request(:post, /.*/).to_return do |request| + sleep 0.002.seconds + { body: 'I\m slow', status: 200 } + end + end + + let(:options) { {} } + + subject(:request_slow_responder) { described_class.post('http://example.org', **options) } + + specify do + expect { request_slow_responder }.not_to raise_error + end + + context 'with use_read_total_timeout option' do + let(:options) { { use_read_total_timeout: true } } + + it 'raises a timeout error' do + expect { request_slow_responder }.to raise_error(Gitlab::HTTP::ReadTotalTimeout, /Request timed out after ?([0-9]*[.])?[0-9]+ seconds/) + end + + context 'and timeout option' do + let(:options) { { use_read_total_timeout: true, timeout: 10.seconds } } + + it 'overrides the default timeout when timeout option is present' do + expect { request_slow_responder }.not_to raise_error + end + end + end + end + + it 'calls a block' do + WebMock.stub_request(:post, /.*/) + + expect { |b| described_class.post('http://example.org', &b) }.to yield_with_args + end + describe 'allow_local_requests_from_web_hooks_and_services is' do before do WebMock.stub_request(:get, /.*/).to_return(status: 200, body: 'Success') diff --git a/spec/models/audit_event_spec.rb b/spec/models/audit_event_spec.rb index 5c87c2e68db..bc603bc5ab6 100644 --- a/spec/models/audit_event_spec.rb +++ b/spec/models/audit_event_spec.rb @@ -3,9 +3,6 @@ require 'spec_helper' RSpec.describe AuditEvent do - let_it_be(:audit_event) { create(:project_audit_event) } - subject { audit_event } - describe 'validations' do include_examples 'validates IP address' do let(:attribute) { :ip_address } @@ -13,6 +10,15 @@ RSpec.describe AuditEvent do end end + it 'sanitizes custom_message in the details hash' do + audit_event = create(:project_audit_event, details: { target_id: 678, custom_message: '<strong>Arnold</strong>' }) + + expect(audit_event.details).to include( + target_id: 678, + custom_message: 'Arnold' + ) + end + describe '#as_json' do context 'ip_address' do subject { build(:group_audit_event, ip_address: '192.168.1.1').as_json } diff --git a/spec/models/protected_branch/push_access_level_spec.rb b/spec/models/protected_branch/push_access_level_spec.rb index 17a589f0485..fa84cd660cb 100644 --- a/spec/models/protected_branch/push_access_level_spec.rb +++ b/spec/models/protected_branch/push_access_level_spec.rb @@ -44,7 +44,7 @@ RSpec.describe ProtectedBranch::PushAccessLevel do let(:can_push) { true } before_all do - project.add_guest(user) + project.add_maintainer(user) end context 'when this push_access_level is tied to a deploy key' do diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index e5c86e69ffc..2185df0609e 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -387,6 +387,19 @@ RSpec.describe User do expect(user.errors.full_messages).to eq(['Username has already been taken']) end end + + it 'validates format' do + Mime::EXTENSION_LOOKUP.keys.each do |type| + user = build(:user, username: "test.#{type}") + + expect(user).not_to be_valid + expect(user.errors.full_messages).to include('Username ending with MIME type format is not allowed.') + end + end + + it 'validates format on updated record' do + expect(create(:user).update(username: 'profile.html')).to be_falsey + end end it 'has a DB-level NOT NULL constraint on projects_limit' do @@ -2882,7 +2895,7 @@ RSpec.describe User do end describe '#sanitize_attrs' do - let(:user) { build(:user, name: 'test & user', skype: 'test&user') } + let(:user) { build(:user, name: 'test <& user', skype: 'test&user') } it 'encodes HTML entities in the Skype attribute' do expect { user.sanitize_attrs }.to change { user.skype }.to('test&user') @@ -2891,6 +2904,25 @@ RSpec.describe User do it 'does not encode HTML entities in the name attribute' do expect { user.sanitize_attrs }.not_to change { user.name } end + + it 'sanitizes attr from html tags' do + user = create(:user, name: '<a href="//example.com">Test<a>', twitter: '<a href="//evil.com">https://twitter.com<a>') + + expect(user.name).to eq('Test') + expect(user.twitter).to eq('https://twitter.com') + end + + it 'sanitizes attr from js scripts' do + user = create(:user, name: '<script>alert("Test")</script>') + + expect(user.name).to eq("alert(\"Test\")") + end + + it 'sanitizes attr from iframe scripts' do + user = create(:user, name: 'User"><iframe src=javascript:alert()><iframe>') + + expect(user.name).to eq('User">') + end end describe '#starred?' do diff --git a/spec/policies/global_policy_spec.rb b/spec/policies/global_policy_spec.rb index e88619b9527..85026ced466 100644 --- a/spec/policies/global_policy_spec.rb +++ b/spec/policies/global_policy_spec.rb @@ -239,6 +239,14 @@ RSpec.describe GlobalPolicy do it { is_expected.not_to be_allowed(:access_api) } end + context 'with a deactivated user' do + before do + current_user.deactivate! + end + + it { is_expected.not_to be_allowed(:access_api) } + end + context 'user with expired password' do before do current_user.update!(password_expires_at: 2.minutes.ago) diff --git a/spec/requests/api/graphql_spec.rb b/spec/requests/api/graphql_spec.rb index a336d74b135..463fca43cb5 100644 --- a/spec/requests/api/graphql_spec.rb +++ b/spec/requests/api/graphql_spec.rb @@ -6,6 +6,9 @@ RSpec.describe 'GraphQL' do include AfterNextHelpers let(:query) { graphql_query_for('echo', text: 'Hello world') } + let(:mutation) { 'mutation { echoCreate(input: { messages: ["hello", "world"] }) { echoes } }' } + + let_it_be(:user) { create(:user) } describe 'logging' do shared_examples 'logging a graphql query' do @@ -70,6 +73,139 @@ RSpec.describe 'GraphQL' do end end + context 'when executing mutations' do + let(:mutation_with_variables) do + <<~GQL + mutation($a: String!, $b: String!) { + echoCreate(input: { messages: [$a, $b] }) { echoes } + } + GQL + end + + context 'with POST' do + it 'succeeds' do + post_graphql(mutation, current_user: user) + + expect(graphql_data_at(:echo_create, :echoes)).to eq %w[hello world] + end + + context 'with variables' do + it 'succeeds' do + post_graphql(mutation_with_variables, current_user: user, variables: { a: 'Yo', b: 'there' }) + + expect(graphql_data_at(:echo_create, :echoes)).to eq %w[Yo there] + end + end + end + + context 'with GET' do + it 'fails' do + get_graphql(mutation, current_user: user) + + expect(graphql_errors).to include(a_hash_including('message' => /Mutations are forbidden/)) + end + + context 'with variables' do + it 'fails' do + get_graphql(mutation_with_variables, current_user: user, variables: { a: 'Yo', b: 'there' }) + + expect(graphql_errors).to include(a_hash_including('message' => /Mutations are forbidden/)) + end + end + end + end + + context 'when executing queries' do + context 'with POST' do + it 'succeeds' do + post_graphql(query, current_user: user) + + expect(graphql_data_at(:echo)).to include 'Hello world' + end + end + + context 'with GET' do + it 'succeeds' do + get_graphql(query, current_user: user) + + expect(graphql_data_at(:echo)).to include 'Hello world' + end + end + end + + context 'when selecting a query by operation name' do + let(:query) { "query A #{graphql_query_for('echo', text: 'Hello world')}" } + let(:mutation) { 'mutation B { echoCreate(input: { messages: ["hello", "world"] }) { echoes } }' } + + let(:combined) { [query, mutation].join("\n\n") } + + context 'with POST' do + it 'succeeds when selecting the query' do + post_graphql(combined, current_user: user, params: { operationName: 'A' }) + + resp = json_response + + expect(resp.dig('data', 'echo')).to include 'Hello world' + end + + it 'succeeds when selecting the mutation' do + post_graphql(combined, current_user: user, params: { operationName: 'B' }) + + resp = json_response + + expect(resp.dig('data', 'echoCreate', 'echoes')).to eq %w[hello world] + end + end + + context 'with GET' do + it 'succeeds when selecting the query' do + get_graphql(combined, current_user: user, params: { operationName: 'A' }) + + resp = json_response + + expect(resp.dig('data', 'echo')).to include 'Hello world' + end + + it 'fails when selecting the mutation' do + get_graphql(combined, current_user: user, params: { operationName: 'B' }) + + resp = json_response + + expect(resp.dig('errors', 0, 'message')).to include "Mutations are forbidden" + end + end + end + + context 'when batching mutations and queries' do + let(:batched) do + [ + { query: "query A #{graphql_query_for('echo', text: 'Hello world')}" }, + { query: 'mutation B { echoCreate(input: { messages: ["hello", "world"] }) { echoes } }' } + ] + end + + context 'with POST' do + it 'succeeds' do + post_multiplex(batched, current_user: user) + + resp = json_response + + expect(resp.dig(0, 'data', 'echo')).to include 'Hello world' + expect(resp.dig(1, 'data', 'echoCreate', 'echoes')).to eq %w[hello world] + end + end + + context 'with GET' do + it 'fails with a helpful error message' do + get_multiplex(batched, current_user: user) + + resp = json_response + + expect(resp.dig('errors', 0, 'message')).to include "Mutations are forbidden" + end + end + end + context 'with invalid variables' do it 'returns an error' do post_graphql(query, variables: "This is not JSON") @@ -80,8 +216,6 @@ RSpec.describe 'GraphQL' do end describe 'authentication', :allow_forgery_protection do - let(:user) { create(:user) } - it 'allows access to public data without authentication' do post_graphql(query) @@ -109,11 +243,9 @@ RSpec.describe 'GraphQL' do context 'with token authentication' do let(:token) { create(:personal_access_token) } - before do + it 'authenticates users with a PAT' do stub_authentication_activity_metrics(debug: false) - end - it 'authenticates users with a PAT' do expect(authentication_metrics) .to increment(:user_authenticated_counter) .and increment(:user_session_override_counter) @@ -124,6 +256,14 @@ RSpec.describe 'GraphQL' do expect(graphql_data['echo']).to eq("\"#{token.user.username}\" says: Hello world") end + it 'prevents access by deactived users' do + token.user.deactivate! + + post_graphql(query, headers: { 'PRIVATE-TOKEN' => token.token }) + + expect(graphql_errors).to include({ 'message' => /API not accessible/ }) + end + context 'when the personal access token has no api scope' do it 'does not log the user in' do token.update!(scopes: [:read_user]) diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index e7e26c34a83..529a75af122 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -56,7 +56,7 @@ RSpec.describe API::Projects do let_it_be(:project, reload: true) { create(:project, :repository, create_branch: 'something_else', namespace: user.namespace) } let_it_be(:project2, reload: true) { create(:project, namespace: user.namespace) } let_it_be(:project_member) { create(:project_member, :developer, user: user3, project: project) } - let_it_be(:user4) { create(:user, username: 'user.with.dot') } + let_it_be(:user4) { create(:user, username: 'user.withdot') } let_it_be(:project3, reload: true) do create(:project, :private, diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index a9231b65c8f..d724cb9612c 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe API::Users do let_it_be(:admin) { create(:admin) } - let_it_be(:user, reload: true) { create(:user, username: 'user.with.dot') } + let_it_be(:user, reload: true) { create(:user, username: 'user.withdot') } let_it_be(:key) { create(:key, user: user) } let_it_be(:gpg_key) { create(:gpg_key, user: user) } let_it_be(:email) { create(:email, user: user) } diff --git a/spec/requests/ide_controller_spec.rb b/spec/requests/ide_controller_spec.rb index 9b0d8dcd828..4bf1e43ba40 100644 --- a/spec/requests/ide_controller_spec.rb +++ b/spec/requests/ide_controller_spec.rb @@ -3,7 +3,14 @@ require 'spec_helper' RSpec.describe IdeController do - let_it_be(:project) { create(:project, :public) } + let_it_be(:reporter) { create(:user) } + + let_it_be(:project) do + create(:project, :private).tap do |p| + p.add_reporter(reporter) + end + end + let_it_be(:creator) { project.creator } let_it_be(:other_user) { create(:user) } @@ -14,48 +21,62 @@ RSpec.describe IdeController do sign_in(user) end - it 'increases the views counter' do - expect(Gitlab::UsageDataCounters::WebIdeCounter).to receive(:increment_views_count) - - get ide_url - end - describe '#index', :aggregate_failures do subject { get route } - shared_examples 'user cannot push code' do - include ProjectForksHelper - - let(:user) { other_user } + shared_examples 'user access rights check' do + context 'user can read project' do + it 'increases the views counter' do + expect(Gitlab::UsageDataCounters::WebIdeCounter).to receive(:increment_views_count) - context 'when user does not have fork' do - it 'instantiates fork_info instance var with fork_path and return 200' do subject - - expect(response).to have_gitlab_http_status(:ok) - expect(assigns(:project)).to eq project - expect(assigns(:fork_info)).to eq({ fork_path: controller.helpers.ide_fork_and_edit_path(project, branch, '', with_notice: false) }) end - it 'has nil fork_info if user cannot fork' do - project.project_feature.update!(forking_access_level: ProjectFeature::DISABLED) + context 'user can read project but cannot push code' do + include ProjectForksHelper - subject + let(:user) { reporter } - expect(response).to have_gitlab_http_status(:ok) - expect(assigns(:fork_info)).to be_nil + context 'when user does not have fork' do + it 'instantiates fork_info instance var with fork_path and returns 200' do + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(assigns(:project)).to eq project + expect(assigns(:fork_info)).to eq({ fork_path: controller.helpers.ide_fork_and_edit_path(project, branch, '', with_notice: false) }) + end + + it 'has nil fork_info if user cannot fork' do + project.project_feature.update!(forking_access_level: ProjectFeature::DISABLED) + + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(assigns(:fork_info)).to be_nil + end + end + + context 'when user has fork' do + let!(:fork) { fork_project(project, user, repository: true, namespace: user.namespace) } + + it 'instantiates fork_info instance var with ide_path and returns 200' do + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(assigns(:project)).to eq project + expect(assigns(:fork_info)).to eq({ ide_path: controller.helpers.ide_edit_path(fork, branch, '') }) + end + end end end - context 'when user has fork' do - let!(:fork) { fork_project(project, user, repository: true, namespace: user.namespace) } + context 'user cannot read project' do + let(:user) { other_user } - it 'instantiates fork_info instance var with ide_path and return 200' do + it 'returns 404' do subject - expect(response).to have_gitlab_http_status(:ok) - expect(assigns(:project)).to eq project - expect(assigns(:fork_info)).to eq({ ide_path: controller.helpers.ide_edit_path(fork, branch, '') }) + expect(response).to have_gitlab_http_status(:not_found) end end end @@ -63,37 +84,27 @@ RSpec.describe IdeController do context '/-/ide' do let(:route) { '/-/ide' } - it 'does not instantiate any instance var and return 200' do + it 'returns 404' do subject - expect(response).to have_gitlab_http_status(:ok) - expect(assigns(:project)).to be_nil - expect(assigns(:branch)).to be_nil - expect(assigns(:path)).to be_nil - expect(assigns(:merge_request)).to be_nil - expect(assigns(:fork_info)).to be_nil + expect(response).to have_gitlab_http_status(:not_found) end end context '/-/ide/project' do let(:route) { '/-/ide/project' } - it 'does not instantiate any instance var and return 200' do + it 'returns 404' do subject - expect(response).to have_gitlab_http_status(:ok) - expect(assigns(:project)).to be_nil - expect(assigns(:branch)).to be_nil - expect(assigns(:path)).to be_nil - expect(assigns(:merge_request)).to be_nil - expect(assigns(:fork_info)).to be_nil + expect(response).to have_gitlab_http_status(:not_found) end end context '/-/ide/project/:project' do let(:route) { "/-/ide/project/#{project.full_path}" } - it 'instantiates project instance var and return 200' do + it 'instantiates project instance var and returns 200' do subject expect(response).to have_gitlab_http_status(:ok) @@ -104,13 +115,13 @@ RSpec.describe IdeController do expect(assigns(:fork_info)).to be_nil end - it_behaves_like 'user cannot push code' + it_behaves_like 'user access rights check' %w(edit blob tree).each do |action| context "/-/ide/project/:project/#{action}" do let(:route) { "/-/ide/project/#{project.full_path}/#{action}" } - it 'instantiates project instance var and return 200' do + it 'instantiates project instance var and returns 200' do subject expect(response).to have_gitlab_http_status(:ok) @@ -121,13 +132,13 @@ RSpec.describe IdeController do expect(assigns(:fork_info)).to be_nil end - it_behaves_like 'user cannot push code' + it_behaves_like 'user access rights check' context "/-/ide/project/:project/#{action}/:branch" do let(:branch) { 'master' } let(:route) { "/-/ide/project/#{project.full_path}/#{action}/#{branch}" } - it 'instantiates project and branch instance vars and return 200' do + it 'instantiates project and branch instance vars and returns 200' do subject expect(response).to have_gitlab_http_status(:ok) @@ -138,13 +149,13 @@ RSpec.describe IdeController do expect(assigns(:fork_info)).to be_nil end - it_behaves_like 'user cannot push code' + it_behaves_like 'user access rights check' context "/-/ide/project/:project/#{action}/:branch/-" do let(:branch) { 'branch/slash' } let(:route) { "/-/ide/project/#{project.full_path}/#{action}/#{branch}/-" } - it 'instantiates project and branch instance vars and return 200' do + it 'instantiates project and branch instance vars and returns 200' do subject expect(response).to have_gitlab_http_status(:ok) @@ -155,13 +166,13 @@ RSpec.describe IdeController do expect(assigns(:fork_info)).to be_nil end - it_behaves_like 'user cannot push code' + it_behaves_like 'user access rights check' context "/-/ide/project/:project/#{action}/:branch/-/:path" do let(:branch) { 'master' } let(:route) { "/-/ide/project/#{project.full_path}/#{action}/#{branch}/-/foo/.bar" } - it 'instantiates project, branch, and path instance vars and return 200' do + it 'instantiates project, branch, and path instance vars and returns 200' do subject expect(response).to have_gitlab_http_status(:ok) @@ -172,7 +183,7 @@ RSpec.describe IdeController do expect(assigns(:fork_info)).to be_nil end - it_behaves_like 'user cannot push code' + it_behaves_like 'user access rights check' end end end @@ -184,7 +195,7 @@ RSpec.describe IdeController do let(:route) { "/-/ide/project/#{project.full_path}/merge_requests/#{merge_request.id}" } - it 'instantiates project and merge_request instance vars and return 200' do + it 'instantiates project and merge_request instance vars and returns 200' do subject expect(response).to have_gitlab_http_status(:ok) @@ -195,7 +206,7 @@ RSpec.describe IdeController do expect(assigns(:fork_info)).to be_nil end - it_behaves_like 'user cannot push code' + it_behaves_like 'user access rights check' end end end diff --git a/spec/services/feature_flags/create_service_spec.rb b/spec/services/feature_flags/create_service_spec.rb index 2e0c162ebc1..4eb2b25fb64 100644 --- a/spec/services/feature_flags/create_service_spec.rb +++ b/spec/services/feature_flags/create_service_spec.rb @@ -68,12 +68,12 @@ RSpec.describe FeatureFlags::CreateService do end it 'creates audit event' do - expected_message = 'Created feature flag <strong>feature_flag</strong> '\ - 'with description <strong>"description"</strong>. '\ - 'Created rule <strong>*</strong> and set it as <strong>active</strong> '\ - 'with strategies <strong>[{"name"=>"default", "parameters"=>{}}]</strong>. '\ - 'Created rule <strong>production</strong> and set it as <strong>inactive</strong> '\ - 'with strategies <strong>[{"name"=>"default", "parameters"=>{}}]</strong>.' + expected_message = 'Created feature flag feature_flag '\ + 'with description "description". '\ + 'Created rule * and set it as active '\ + 'with strategies [{"name"=>"default", "parameters"=>{}}]. '\ + 'Created rule production and set it as inactive '\ + 'with strategies [{"name"=>"default", "parameters"=>{}}].' expect { subject }.to change { AuditEvent.count }.by(1) expect(AuditEvent.last.details[:custom_message]).to eq(expected_message) diff --git a/spec/services/feature_flags/destroy_service_spec.rb b/spec/services/feature_flags/destroy_service_spec.rb index ee30474873c..d3796ef6b4d 100644 --- a/spec/services/feature_flags/destroy_service_spec.rb +++ b/spec/services/feature_flags/destroy_service_spec.rb @@ -33,7 +33,7 @@ RSpec.describe FeatureFlags::DestroyService do it 'creates audit log' do expect { subject }.to change { AuditEvent.count }.by(1) - expect(audit_event_message).to eq("Deleted feature flag <strong>#{feature_flag.name}</strong>.") + expect(audit_event_message).to eq("Deleted feature flag #{feature_flag.name}.") end context 'when user is reporter' do diff --git a/spec/services/feature_flags/update_service_spec.rb b/spec/services/feature_flags/update_service_spec.rb index d838549891a..4858139d60a 100644 --- a/spec/services/feature_flags/update_service_spec.rb +++ b/spec/services/feature_flags/update_service_spec.rb @@ -38,9 +38,9 @@ RSpec.describe FeatureFlags::UpdateService do expect { subject }.to change { AuditEvent.count }.by(1) expect(audit_event_message).to( - eq("Updated feature flag <strong>new_name</strong>. "\ - "Updated name from <strong>\"#{name_was}\"</strong> "\ - "to <strong>\"new_name\"</strong>.") + eq("Updated feature flag new_name. "\ + "Updated name from \"#{name_was}\" "\ + "to \"new_name\".") ) end @@ -94,8 +94,8 @@ RSpec.describe FeatureFlags::UpdateService do it 'creates audit event with changed description' do expect { subject }.to change { AuditEvent.count }.by(1) expect(audit_event_message).to( - include("Updated description from <strong>\"\"</strong>"\ - " to <strong>\"new description\"</strong>.") + include("Updated description from \"\""\ + " to \"new description\".") ) end end @@ -110,7 +110,7 @@ RSpec.describe FeatureFlags::UpdateService do it 'creates audit event about changing active state' do expect { subject }.to change { AuditEvent.count }.by(1) expect(audit_event_message).to( - include('Updated active from <strong>"true"</strong> to <strong>"false"</strong>.') + include('Updated active from "true" to "false".') ) end diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb index 276656656ec..d710e4a777f 100644 --- a/spec/services/projects/fork_service_spec.rb +++ b/spec/services/projects/fork_service_spec.rb @@ -184,14 +184,6 @@ RSpec.describe Projects::ForkService do end end - context 'GitLab CI is enabled' do - it "forks and enables CI for fork" do - @from_project.enable_ci - @to_project = fork_project(@from_project, @to_user, using_service: true) - expect(@to_project.builds_enabled?).to be_truthy - end - end - context "CI/CD settings" do let(:to_project) { fork_project(@from_project, @to_user, using_service: true) } @@ -366,6 +358,19 @@ RSpec.describe Projects::ForkService do expect(forked_project.visibility_level).to eq(Gitlab::VisibilityLevel::PRIVATE) end + + it 'copies project features visibility settings to the fork', :aggregate_failures do + attrs = ProjectFeature::FEATURES.to_h do |f| + ["#{f}_access_level", ProjectFeature::PRIVATE] + end + + public_project.project_feature.update!(attrs) + + user = create(:user, developer_projects: [public_project]) + forked_project = described_class.new(public_project, user).execute + + expect(forked_project.project_feature.slice(attrs.keys)).to eq(attrs) + end end end diff --git a/spec/support/helpers/graphql_helpers.rb b/spec/support/helpers/graphql_helpers.rb index 4857fa63114..38cf828ca5e 100644 --- a/spec/support/helpers/graphql_helpers.rb +++ b/spec/support/helpers/graphql_helpers.rb @@ -400,8 +400,13 @@ module GraphqlHelpers post api('/', current_user, version: 'graphql'), params: { _json: queries }, headers: headers end - def post_graphql(query, current_user: nil, variables: nil, headers: {}, token: {}) - params = { query: query, variables: serialize_variables(variables) } + def get_multiplex(queries, current_user: nil, headers: {}) + path = "/?#{queries.to_query('_json')}" + get api(path, current_user, version: 'graphql'), headers: headers + end + + def post_graphql(query, current_user: nil, variables: nil, headers: {}, token: {}, params: {}) + params = params.merge(query: query, variables: serialize_variables(variables)) post api('/', current_user, version: 'graphql', **token), params: params, headers: headers return unless graphql_errors @@ -410,6 +415,18 @@ module GraphqlHelpers expect(graphql_errors).not_to include(a_hash_including('message' => 'Internal server error')) end + def get_graphql(query, current_user: nil, variables: nil, headers: {}, token: {}, params: {}) + vars = "variables=#{CGI.escape(serialize_variables(variables))}" if variables + params = params.to_a.map { |k, v| v.to_query(k) } + path = ["/?query=#{CGI.escape(query)}", vars, *params].join('&') + get api(path, current_user, version: 'graphql', **token), headers: headers + + return unless graphql_errors + + # Errors are acceptable, but not this one: + expect(graphql_errors).not_to include(a_hash_including('message' => 'Internal server error')) + end + def post_graphql_mutation(mutation, current_user: nil, token: {}) post_graphql(mutation.query, current_user: current_user, |