diff options
52 files changed, 1270 insertions, 267 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 9201e691c3f..bea21092b43 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,20 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 12.5.1 + +### Security (8 changes) + +- Check permissions before showing a forked project's source. +- Encrypt application setting tokens. +- Update Workhorse and Gitaly to fix a security issue. +- Hide commit counts from guest users in Cycle Analytics. +- Limit potential for DNS rebind SSRF in chat notifications. +- Ensure are cleaned by ImportExport::AttributeCleaner. +- Remove notes regarding Related Branches from Issue activity feeds for guest users. +- Escape namespace in label references to prevent XSS. + + ## 12.5.0 ### Security (15 changes) @@ -353,6 +367,21 @@ entry. - Change selects from default browser style to custom style. +## 12.4.4 + +### Security (9 changes) + +- Check permissions before showing a forked project's source. +- Encrypt application setting tokens. +- Update Workhorse and Gitaly to fix a security issue. +- Hide commit counts from guest users in Cycle Analytics. +- Limit potential for DNS rebind SSRF in chat notifications. +- Fix 500 error caused by invalid byte sequences in links. +- Ensure are cleaned by ImportExport::AttributeCleaner. +- Remove notes regarding Related Branches from Issue activity feeds for guest users. +- Escape namespace in label references to prevent XSS. + + ## 12.4.3 ### Fixed (2 changes) @@ -721,6 +750,21 @@ entry. - Remove Postgresql specific setup tasks and move to schema.rb. +## 12.3.7 + +### Security (9 changes) + +- Check permissions before showing a forked project's source. +- Encrypt application setting tokens. +- Update Workhorse and Gitaly to fix a security issue. +- Hide commit counts from guest users in Cycle Analytics. +- Limit potential for DNS rebind SSRF in chat notifications. +- Fix 500 error caused by invalid byte sequences in links. +- Ensure are cleaned by ImportExport::AttributeCleaner. +- Remove notes regarding Related Branches from Issue activity feeds for guest users. +- Escape namespace in label references to prevent XSS. + + ## 12.3.4 ### Fixed (2 changes) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index dc87e8af82f..22d6771a47d 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -1.74.0 +1.72.1 diff --git a/GITLAB_WORKHORSE_VERSION b/GITLAB_WORKHORSE_VERSION index 2a5dd0d6389..6092827e644 100644 --- a/GITLAB_WORKHORSE_VERSION +++ b/GITLAB_WORKHORSE_VERSION @@ -1 +1 @@ -8.14.0 +8.14.1 diff --git a/app/assets/javascripts/cycle_analytics/cycle_analytics_bundle.js b/app/assets/javascripts/cycle_analytics/cycle_analytics_bundle.js index cd67ba5fab8..1074ce0e744 100644 --- a/app/assets/javascripts/cycle_analytics/cycle_analytics_bundle.js +++ b/app/assets/javascripts/cycle_analytics/cycle_analytics_bundle.js @@ -58,10 +58,16 @@ export default () => { service: this.createCycleAnalyticsService(cycleAnalyticsEl.dataset.requestPath), }; }, + defaultNumberOfSummaryItems: 3, computed: { currentStage() { return this.store.currentActiveStage(); }, + summaryTableColumnClass() { + return this.state.summary.length === this.$options.defaultNumberOfSummaryItems + ? 'col-sm-3' + : 'col-sm-4'; + }, }, created() { // Conditional check placed here to prevent this method from being called on the diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb index f7e33c09928..9d81d3fad07 100644 --- a/app/controllers/admin/application_settings_controller.rb +++ b/app/controllers/admin/application_settings_controller.rb @@ -86,6 +86,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController params[:application_setting][:import_sources]&.delete("") params[:application_setting][:restricted_visibility_levels]&.delete("") + params[:application_setting].delete(:elasticsearch_aws_secret_access_key) if params[:application_setting][:elasticsearch_aws_secret_access_key].blank? # TODO Remove domain_blacklist_raw in APIv5 (See https://gitlab.com/gitlab-org/gitlab-foss/issues/67204) params.delete(:domain_blacklist_raw) if params[:domain_blacklist_file] params.delete(:domain_blacklist_raw) if params[:domain_blacklist] diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index 64d61bba71a..466c782fc77 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -110,19 +110,26 @@ module ProjectsHelper { project_full_name: project.full_name } end - def remove_fork_project_message(project) - _("You are going to remove the fork relationship to source project %{forked_from_project}. Are you ABSOLUTELY sure?") % - { forked_from_project: fork_source_name(project) } - end + def remove_fork_project_description_message(project) + source = visible_fork_source(project) - def fork_source_name(project) - if @project.fork_source - @project.fork_source.full_name + if source + _('This will remove the fork relationship between this project and %{fork_source}.') % + { fork_source: link_to(source.full_name, project_path(source)) } else - @project.fork_network&.deleted_root_project_name + _('This will remove the fork relationship between this project and other projects in the fork network.') end end + def remove_fork_project_warning_message(project) + _("You are going to remove the fork relationship from %{project_full_name}. Are you ABSOLUTELY sure?") % + { project_full_name: project.full_name } + end + + def visible_fork_source(project) + project.fork_source if project.fork_source && can?(current_user, :read_project, project.fork_source) + end + def project_nav_tabs @nav_tabs ||= get_project_nav_tabs(@project, current_user) end diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 4028d711fd1..72605af433f 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -313,29 +313,25 @@ class ApplicationSetting < ApplicationRecord algorithm: 'aes-256-cbc', insecure_mode: true - attr_encrypted :external_auth_client_key, - mode: :per_attribute_iv, - key: Settings.attr_encrypted_db_key_base_truncated, - algorithm: 'aes-256-gcm', - encode: true - - attr_encrypted :external_auth_client_key_pass, - mode: :per_attribute_iv, - key: Settings.attr_encrypted_db_key_base_truncated, - algorithm: 'aes-256-gcm', - encode: true - - attr_encrypted :lets_encrypt_private_key, - mode: :per_attribute_iv, - key: Settings.attr_encrypted_db_key_base_truncated, - algorithm: 'aes-256-gcm', - encode: true + private_class_method def self.encryption_options_base_truncated_aes_256_gcm + { + mode: :per_attribute_iv, + key: Settings.attr_encrypted_db_key_base_truncated, + algorithm: 'aes-256-gcm', + encode: true + } + end - attr_encrypted :eks_secret_access_key, - mode: :per_attribute_iv, - key: Settings.attr_encrypted_db_key_base_truncated, - algorithm: 'aes-256-gcm', - encode: true + attr_encrypted :external_auth_client_key, encryption_options_base_truncated_aes_256_gcm + attr_encrypted :external_auth_client_key_pass, encryption_options_base_truncated_aes_256_gcm + attr_encrypted :lets_encrypt_private_key, encryption_options_base_truncated_aes_256_gcm + attr_encrypted :eks_secret_access_key, encryption_options_base_truncated_aes_256_gcm + attr_encrypted :akismet_api_key, encryption_options_base_truncated_aes_256_gcm + attr_encrypted :elasticsearch_aws_secret_access_key, encryption_options_base_truncated_aes_256_gcm + attr_encrypted :recaptcha_private_key, encryption_options_base_truncated_aes_256_gcm + attr_encrypted :recaptcha_site_key, encryption_options_base_truncated_aes_256_gcm + attr_encrypted :slack_app_secret, encryption_options_base_truncated_aes_256_gcm + attr_encrypted :slack_app_verification_token, encryption_options_base_truncated_aes_256_gcm before_validation :ensure_uuid! diff --git a/app/models/note.rb b/app/models/note.rb index 1996fd4bff5..4e3700f3eaa 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -37,6 +37,10 @@ class Note < ApplicationRecord redact_field :note + TYPES_RESTRICTED_BY_ABILITY = { + branch: :download_code + }.freeze + # Aliases to make application_helper#edited_time_ago_with_tooltip helper work properly with notes. # See https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/10392/diffs#note_28719102 alias_attribute :last_edited_at, :updated_at @@ -341,7 +345,7 @@ class Note < ApplicationRecord end def visible_for?(user) - !cross_reference_not_visible_for?(user) + !cross_reference_not_visible_for?(user) && system_note_viewable_by?(user) end def award_emoji? @@ -497,6 +501,15 @@ class Note < ApplicationRecord private + def system_note_viewable_by?(user) + return true unless system_note_metadata + + restriction = TYPES_RESTRICTED_BY_ABILITY[system_note_metadata.action.to_sym] + return Ability.allowed?(user, restriction, project) if restriction + + true + end + def keep_around_commit project.repository.keep_around(self.commit_id) end diff --git a/app/models/project.rb b/app/models/project.rb index 67225d7fb78..4cfbb963ddf 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -512,7 +512,11 @@ class Project < ApplicationRecord # This scope returns projects where user has access to both the project and the feature. def self.filter_by_feature_visibility(feature, user) - with_feature_available_for_user(feature, user).public_or_visible_to_user(user) + with_feature_available_for_user(feature, user) + .public_or_visible_to_user( + user, + ProjectFeature.required_minimum_access_level_for_private_project(feature) + ) end scope :active, -> { joins(:issues, :notes, :merge_requests).order('issues.created_at, notes.created_at, merge_requests.created_at DESC') } diff --git a/app/models/project_feature.rb b/app/models/project_feature.rb index 2013f620b5b..caa65d32c86 100644 --- a/app/models/project_feature.rb +++ b/app/models/project_feature.rb @@ -24,6 +24,7 @@ class ProjectFeature < ApplicationRecord FEATURES = %i(issues merge_requests wiki snippets builds repository pages).freeze PRIVATE_FEATURES_MIN_ACCESS_LEVEL = { merge_requests: Gitlab::Access::REPORTER }.freeze + PRIVATE_FEATURES_MIN_ACCESS_LEVEL_FOR_PRIVATE_PROJECT = { repository: Gitlab::Access::REPORTER }.freeze STRING_OPTIONS = HashWithIndifferentAccess.new({ 'disabled' => DISABLED, 'private' => PRIVATE, @@ -51,6 +52,15 @@ class ProjectFeature < ApplicationRecord PRIVATE_FEATURES_MIN_ACCESS_LEVEL.fetch(feature, Gitlab::Access::GUEST) end + # Guest users can perform certain features on public and internal projects, but not private projects. + def required_minimum_access_level_for_private_project(feature) + feature = ensure_feature!(feature) + + PRIVATE_FEATURES_MIN_ACCESS_LEVEL_FOR_PRIVATE_PROJECT.fetch(feature) do + required_minimum_access_level(feature) + end + end + def access_level_from_str(level) STRING_OPTIONS.fetch(level) end diff --git a/app/models/project_services/chat_notification_service.rb b/app/models/project_services/chat_notification_service.rb index ecea1a5b630..b84a79453c1 100644 --- a/app/models/project_services/chat_notification_service.rb +++ b/app/models/project_services/chat_notification_service.rb @@ -113,12 +113,9 @@ class ChatNotificationService < Service private + # every notifier must implement this independently def notify(message, opts) - Slack::Notifier.new(webhook, opts).ping( - message.pretext, - attachments: message.attachments, - fallback: message.fallback - ) + raise NotImplementedError end def custom_data(data) diff --git a/app/models/project_services/mattermost_service.rb b/app/models/project_services/mattermost_service.rb index b8bc83b870e..c1055db78e5 100644 --- a/app/models/project_services/mattermost_service.rb +++ b/app/models/project_services/mattermost_service.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class MattermostService < ChatNotificationService + include ::SlackService::Notifier + def title 'Mattermost notifications' end diff --git a/app/models/project_services/slack_service.rb b/app/models/project_services/slack_service.rb index 482808255f9..7290964f442 100644 --- a/app/models/project_services/slack_service.rb +++ b/app/models/project_services/slack_service.rb @@ -30,4 +30,28 @@ class SlackService < ChatNotificationService def webhook_placeholder 'https://hooks.slack.com/services/…' end + + module Notifier + private + + def notify(message, opts) + # See https://github.com/stevenosloan/slack-notifier#custom-http-client + notifier = Slack::Notifier.new(webhook, opts.merge(http_client: HTTPClient)) + + notifier.ping( + message.pretext, + attachments: message.attachments, + fallback: message.fallback + ) + end + + 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) + end + end + end + + include Notifier end diff --git a/app/views/projects/_home_panel.html.haml b/app/views/projects/_home_panel.html.haml index b7c4114d485..daedc52f298 100644 --- a/app/views/projects/_home_panel.html.haml +++ b/app/views/projects/_home_panel.html.haml @@ -74,13 +74,12 @@ - if @project.forked? %p - - if @project.fork_source + - source = visible_fork_source(@project) + - if source #{ s_('ForkedFromProjectPath|Forked from') } - = link_to project_path(@project.fork_source) do - = fork_source_name(@project) + = link_to source.full_name, project_path(source) - else - - deleted_message = s_('ForkedFromProjectPath|Forked from %{project_name} (deleted)') - = deleted_message % { project_name: fork_source_name(@project) } + = s_('ForkedFromProjectPath|Forked from an inaccessible project') = render_if_exists "projects/home_mirror" diff --git a/app/views/projects/cycle_analytics/show.html.haml b/app/views/projects/cycle_analytics/show.html.haml index 7fedd1ab785..1691af9dfdd 100644 --- a/app/views/projects/cycle_analytics/show.html.haml +++ b/app/views/projects/cycle_analytics/show.html.haml @@ -13,10 +13,10 @@ .content-block .container-fluid .row - .col-sm-3.col-12.column{ "v-for" => "item in state.summary" } + .col-12.column{ "v-for" => "item in state.summary", ":class" => "summaryTableColumnClass" } %h3.header {{ item.value }} %p.text {{ item.title }} - .col-sm-3.col-12.column + .col-12.column{ ":class" => "summaryTableColumnClass" } .dropdown.inline.js-ca-dropdown %button.dropdown-menu-toggle{ "data-toggle" => "dropdown", :type => "button" } %span.dropdown-label {{ n__('Last %d day', 'Last %d days', 30) }} diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index b5e24cbbffb..328fdd0be10 100644 --- a/app/views/projects/edit.html.haml +++ b/app/views/projects/edit.html.haml @@ -126,17 +126,12 @@ - if @project.forked? && can?(current_user, :remove_fork_project, @project) .sub-section %h4.danger-title= _('Remove fork relationship') - %p - = _('This will remove the fork relationship to source project') - = succeed "." do - - if @project.fork_source - = link_to(fork_source_name(@project), project_path(@project.fork_source)) - - else - = fork_source_name(@project) + %p= remove_fork_project_description_message(@project) + = form_for([@project.namespace.becomes(Namespace), @project], url: remove_fork_project_path(@project), method: :delete, remote: true, html: { class: 'transfer-project' }) do |f| %p %strong= _('Once removed, the fork relationship cannot be restored and you will no longer be able to send merge requests to the source.') - = button_to _('Remove fork relationship'), '#', class: "btn btn-remove js-confirm-danger", data: { "confirm-danger-message" => remove_fork_project_message(@project) } + = button_to _('Remove fork relationship'), '#', class: "btn btn-remove js-confirm-danger", data: { "confirm-danger-message" => remove_fork_project_warning_message(@project) } - if can?(current_user, :remove_project, @project) .sub-section diff --git a/changelogs/unreleased/security-29660-update-dependencies.yml b/changelogs/unreleased/security-29660-update-dependencies.yml new file mode 100644 index 00000000000..283d951e69e --- /dev/null +++ b/changelogs/unreleased/security-29660-update-dependencies.yml @@ -0,0 +1,5 @@ +--- +title: Update Workhorse and Gitaly to fix a security issue +merge_request: +author: +type: security diff --git a/config/initializers/hangouts_chat_http_override.rb b/config/initializers/hangouts_chat_http_override.rb new file mode 100644 index 00000000000..4fd886697e4 --- /dev/null +++ b/config/initializers/hangouts_chat_http_override.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module HangoutsChat + class Sender + class HTTP + module GitlabHTTPOverride + extend ::Gitlab::Utils::Override + + attr_reader :uri + + # see https://github.com/enzinia/hangouts-chat/blob/6a509f61a56e757f8f417578b393b94423831ff7/lib/hangouts_chat/http.rb + override :post + def post(payload) + httparty_response = Gitlab::HTTP.post( + uri, + body: payload.to_json, + headers: { 'Content-Type' => 'application/json' }, + parse: nil # disables automatic response parsing + ) + net_http_response = httparty_response.response + # The rest of the integration expects a Net::HTTP response + net_http_response + end + end + + prepend GitlabHTTPOverride + end + end +end diff --git a/db/migrate/20191120084627_add_encrypted_fields_to_application_settings.rb b/db/migrate/20191120084627_add_encrypted_fields_to_application_settings.rb new file mode 100644 index 00000000000..4e0886a5121 --- /dev/null +++ b/db/migrate/20191120084627_add_encrypted_fields_to_application_settings.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +class AddEncryptedFieldsToApplicationSettings < ActiveRecord::Migration[5.2] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + PLAINTEXT_ATTRIBUTES = %w[ + akismet_api_key + elasticsearch_aws_secret_access_key + recaptcha_private_key + recaptcha_site_key + slack_app_secret + slack_app_verification_token + ].freeze + + def up + PLAINTEXT_ATTRIBUTES.each do |plaintext_attribute| + add_column :application_settings, "encrypted_#{plaintext_attribute}", :text + add_column :application_settings, "encrypted_#{plaintext_attribute}_iv", :string, limit: 255 + end + end + + def down + PLAINTEXT_ATTRIBUTES.each do |plaintext_attribute| + remove_column :application_settings, "encrypted_#{plaintext_attribute}" + remove_column :application_settings, "encrypted_#{plaintext_attribute}_iv" + end + end +end diff --git a/db/migrate/20191120115530_encrypt_plaintext_attributes_on_application_settings.rb b/db/migrate/20191120115530_encrypt_plaintext_attributes_on_application_settings.rb new file mode 100644 index 00000000000..e6b9a40ad4f --- /dev/null +++ b/db/migrate/20191120115530_encrypt_plaintext_attributes_on_application_settings.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +class EncryptPlaintextAttributesOnApplicationSettings < ActiveRecord::Migration[5.2] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + PLAINTEXT_ATTRIBUTES = %w[ + akismet_api_key + elasticsearch_aws_secret_access_key + recaptcha_private_key + recaptcha_site_key + slack_app_secret + slack_app_verification_token + ].freeze + + class ApplicationSetting < ActiveRecord::Base + self.table_name = 'application_settings' + + def self.encryption_options_base_truncated_aes_256_gcm + { + mode: :per_attribute_iv, + key: Gitlab::Application.secrets.db_key_base[0..31], + algorithm: 'aes-256-gcm', + encode: true + } + end + + attr_encrypted :akismet_api_key, encryption_options_base_truncated_aes_256_gcm + attr_encrypted :elasticsearch_aws_secret_access_key, encryption_options_base_truncated_aes_256_gcm + attr_encrypted :recaptcha_private_key, encryption_options_base_truncated_aes_256_gcm + attr_encrypted :recaptcha_site_key, encryption_options_base_truncated_aes_256_gcm + attr_encrypted :slack_app_secret, encryption_options_base_truncated_aes_256_gcm + attr_encrypted :slack_app_verification_token, encryption_options_base_truncated_aes_256_gcm + + def akismet_api_key + decrypt(:akismet_api_key, self[:encrypted_akismet_api_key]) || self[:akismet_api_key] + end + + def elasticsearch_aws_secret_access_key + decrypt(:elasticsearch_aws_secret_access_key, self[:encrypted_elasticsearch_aws_secret_access_key]) || self[:elasticsearch_aws_secret_access_key] + end + + def recaptcha_private_key + decrypt(:recaptcha_private_key, self[:encrypted_recaptcha_private_key]) || self[:recaptcha_private_key] + end + + def recaptcha_site_key + decrypt(:recaptcha_site_key, self[:encrypted_recaptcha_site_key]) || self[:recaptcha_site_key] + end + + def slack_app_secret + decrypt(:slack_app_secret, self[:encrypted_slack_app_secret]) || self[:slack_app_secret] + end + + def slack_app_verification_token + decrypt(:slack_app_verification_token, self[:encrypted_slack_app_verification_token]) || self[:slack_app_verification_token] + end + end + + def up + ApplicationSetting.find_each do |application_setting| + # We are using the setter from attr_encrypted gem to encrypt the data. + # The gem updates the two columns needed to decrypt the value: + # - "encrypted_#{plaintext_attribute}" + # - "encrypted_#{plaintext_attribute}_iv" + application_setting.assign_attributes( + PLAINTEXT_ATTRIBUTES.each_with_object({}) do |plaintext_attribute, attributes| + attributes[plaintext_attribute] = application_setting.send(plaintext_attribute) + end + ) + application_setting.save(validate: false) + end + end + + def down + ApplicationSetting.find_each do |application_setting| + application_setting.update_columns( + PLAINTEXT_ATTRIBUTES.each_with_object({}) do |plaintext_attribute, attributes| + attributes[plaintext_attribute] = application_setting.send(plaintext_attribute) + attributes["encrypted_#{plaintext_attribute}"] = nil + attributes["encrypted_#{plaintext_attribute}_iv"] = nil + end + ) + end + end +end diff --git a/db/post_migrate/20191122135327_remove_plaintext_columns_from_application_settings.rb b/db/post_migrate/20191122135327_remove_plaintext_columns_from_application_settings.rb new file mode 100644 index 00000000000..b5cd58b10a8 --- /dev/null +++ b/db/post_migrate/20191122135327_remove_plaintext_columns_from_application_settings.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +class RemovePlaintextColumnsFromApplicationSettings < ActiveRecord::Migration[5.2] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + PLAINTEXT_ATTRIBUTES = %w[ + akismet_api_key + elasticsearch_aws_secret_access_key + recaptcha_private_key + recaptcha_site_key + slack_app_secret + slack_app_verification_token + ].freeze + + def up + PLAINTEXT_ATTRIBUTES.each do |plaintext_attribute| + remove_column :application_settings, plaintext_attribute + end + end + + def down + PLAINTEXT_ATTRIBUTES.each do |plaintext_attribute| + add_column :application_settings, plaintext_attribute, :text + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 2de76973ca7..090e9586686 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -180,11 +180,8 @@ ActiveRecord::Schema.define(version: 2019_11_24_150431) do t.integer "metrics_timeout", default: 10 t.integer "metrics_method_call_threshold", default: 10 t.boolean "recaptcha_enabled", default: false - t.string "recaptcha_site_key" - t.string "recaptcha_private_key" t.integer "metrics_port", default: 8089 t.boolean "akismet_enabled", default: false - t.string "akismet_api_key" t.integer "metrics_sample_interval", default: 15 t.boolean "email_author_in_body", default: false t.integer "default_group_visibility" @@ -231,7 +228,6 @@ ActiveRecord::Schema.define(version: 2019_11_24_150431) do t.boolean "elasticsearch_aws", default: false, null: false t.string "elasticsearch_aws_region", default: "us-east-1" t.string "elasticsearch_aws_access_key" - t.string "elasticsearch_aws_secret_access_key" t.integer "geo_status_timeout", default: 10 t.string "uuid" t.decimal "polling_interval_multiplier", default: "1.0", null: false @@ -247,8 +243,6 @@ ActiveRecord::Schema.define(version: 2019_11_24_150431) do t.string "help_page_support_url" t.boolean "slack_app_enabled", default: false t.string "slack_app_id" - t.string "slack_app_secret" - t.string "slack_app_verification_token" t.integer "performance_bar_allowed_group_id" t.boolean "allow_group_owners_to_manage_ldap", default: true, null: false t.boolean "hashed_storage_enabled", default: true, null: false @@ -355,6 +349,18 @@ ActiveRecord::Schema.define(version: 2019_11_24_150431) do t.boolean "sourcegraph_enabled", default: false, null: false t.string "sourcegraph_url", limit: 255 t.boolean "sourcegraph_public_only", default: true, null: false + t.text "encrypted_akismet_api_key" + t.string "encrypted_akismet_api_key_iv", limit: 255 + t.text "encrypted_elasticsearch_aws_secret_access_key" + t.string "encrypted_elasticsearch_aws_secret_access_key_iv", limit: 255 + t.text "encrypted_recaptcha_private_key" + t.string "encrypted_recaptcha_private_key_iv", limit: 255 + t.text "encrypted_recaptcha_site_key" + t.string "encrypted_recaptcha_site_key_iv", limit: 255 + t.text "encrypted_slack_app_secret" + t.string "encrypted_slack_app_secret_iv", limit: 255 + t.text "encrypted_slack_app_verification_token" + t.string "encrypted_slack_app_verification_token_iv", limit: 255 t.index ["custom_project_templates_group_id"], name: "index_application_settings_on_custom_project_templates_group_id" t.index ["file_template_project_id"], name: "index_application_settings_on_file_template_project_id" t.index ["instance_administration_project_id"], name: "index_applicationsettings_on_instance_administration_project_id" diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 6ca1d8bd483..22c88a7f7f0 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -283,7 +283,9 @@ module API expose :shared_runners_enabled expose :lfs_enabled?, as: :lfs_enabled expose :creator_id - expose :forked_from_project, using: Entities::BasicProjectDetails, if: lambda { |project, options| project.forked? } + expose :forked_from_project, using: Entities::BasicProjectDetails, if: ->(project, options) do + project.forked? && Ability.allowed?(options[:current_user], :read_project, project.forked_from_project) + end expose :import_status expose :import_error, if: lambda { |_project, options| options[:user_can_admin_project] } do |project| diff --git a/lib/banzai/filter/label_reference_filter.rb b/lib/banzai/filter/label_reference_filter.rb index db620c65237..609ea8fb5ca 100644 --- a/lib/banzai/filter/label_reference_filter.rb +++ b/lib/banzai/filter/label_reference_filter.rb @@ -89,7 +89,7 @@ module Banzai parent_from_ref = from_ref_cached(project_path) reference = parent_from_ref.to_human_reference(parent) - label_suffix = " <i>in #{reference}</i>" if reference.present? + label_suffix = " <i>in #{ERB::Util.html_escape(reference)}</i>" if reference.present? end presenter = object.present(issuable_subject: parent) diff --git a/lib/banzai/filter/relative_link_filter.rb b/lib/banzai/filter/relative_link_filter.rb index c7589e69262..583b0081319 100644 --- a/lib/banzai/filter/relative_link_filter.rb +++ b/lib/banzai/filter/relative_link_filter.rb @@ -172,7 +172,7 @@ module Banzai end def cleaned_file_path(uri) - Addressable::URI.unescape(uri.path).delete("\0").chomp("/") + Addressable::URI.unescape(uri.path).scrub.delete("\0").chomp("/") end def relative_file_path(uri) diff --git a/lib/gitlab/cycle_analytics/stage_summary.rb b/lib/gitlab/cycle_analytics/stage_summary.rb index ea440c441b7..9c75d4bb455 100644 --- a/lib/gitlab/cycle_analytics/stage_summary.rb +++ b/lib/gitlab/cycle_analytics/stage_summary.rb @@ -11,13 +11,29 @@ module Gitlab end def data - [serialize(Summary::Issue.new(project: @project, from: @from, to: @to, current_user: @current_user)), - serialize(Summary::Commit.new(project: @project, from: @from, to: @to)), - serialize(Summary::Deploy.new(project: @project, from: @from, to: @to))] + summary = [issue_stats] + summary << commit_stats if user_has_sufficient_access? + summary << deploy_stats end private + def issue_stats + serialize(Summary::Issue.new(project: @project, from: @from, to: @to, current_user: @current_user)) + end + + def commit_stats + serialize(Summary::Commit.new(project: @project, from: @from, to: @to)) + end + + def deploy_stats + serialize(Summary::Deploy.new(project: @project, from: @from, to: @to)) + end + + def user_has_sufficient_access? + @project.team.member?(@current_user, Gitlab::Access::REPORTER) + end + def serialize(summary_object) AnalyticsSummarySerializer.new.represent(summary_object) end diff --git a/lib/gitlab/import_export/attribute_cleaner.rb b/lib/gitlab/import_export/attribute_cleaner.rb index b2fe9592c06..50fec9f3eb9 100644 --- a/lib/gitlab/import_export/attribute_cleaner.rb +++ b/lib/gitlab/import_export/attribute_cleaner.rb @@ -4,7 +4,7 @@ module Gitlab module ImportExport class AttributeCleaner ALLOWED_REFERENCES = RelationFactory::PROJECT_REFERENCES + RelationFactory::USER_REFERENCES + %w[group_id commit_id] - PROHIBITED_REFERENCES = Regexp.union(/\Acached_markdown_version\Z/, /_id\Z/, /_html\Z/).freeze + PROHIBITED_REFERENCES = Regexp.union(/\Acached_markdown_version\Z/, /_id\Z/, /_ids\Z/, /_html\Z/).freeze def self.clean(*args) new(*args).clean diff --git a/lib/microsoft_teams/notifier.rb b/lib/microsoft_teams/notifier.rb index a7dcd322e27..340bf709f5e 100644 --- a/lib/microsoft_teams/notifier.rb +++ b/lib/microsoft_teams/notifier.rb @@ -14,7 +14,6 @@ module MicrosoftTeams response = Gitlab::HTTP.post( @webhook.to_str, headers: @header, - allow_local_requests: true, body: body(options) ) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 3ff2257d2e5..1a229a72165 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -7705,7 +7705,7 @@ msgstr "" msgid "ForkedFromProjectPath|Forked from" msgstr "" -msgid "ForkedFromProjectPath|Forked from %{project_name} (deleted)" +msgid "ForkedFromProjectPath|Forked from an inaccessible project" msgstr "" msgid "Forking in progress" @@ -17933,7 +17933,10 @@ msgstr "" msgid "This will redirect you to an external sign in page." msgstr "" -msgid "This will remove the fork relationship to source project" +msgid "This will remove the fork relationship between this project and %{fork_source}." +msgstr "" + +msgid "This will remove the fork relationship between this project and other projects in the fork network." msgstr "" msgid "Those emails automatically become issues (with the comments becoming the email conversation) listed here." @@ -19852,7 +19855,7 @@ msgstr "" msgid "You are going to remove %{project_full_name}. Removed project CANNOT be restored! Are you ABSOLUTELY sure?" msgstr "" -msgid "You are going to remove the fork relationship to source project %{forked_from_project}. Are you ABSOLUTELY sure?" +msgid "You are going to remove the fork relationship from %{project_full_name}. Are you ABSOLUTELY sure?" msgstr "" msgid "You are going to transfer %{project_full_name} to another owner. Are you ABSOLUTELY sure?" diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 8770a5ee303..29746bbd863 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -1435,6 +1435,43 @@ describe Projects::IssuesController do expect { get :discussions, params: { namespace_id: project.namespace, project_id: project, id: issue.iid } }.not_to exceed_query_limit(control_count) end end + + context 'private project' do + let!(:branch_note) { create(:discussion_note_on_issue, :system, noteable: issue, project: project) } + let!(:commit_note) { create(:discussion_note_on_issue, :system, noteable: issue, project: project) } + let!(:branch_note_meta) { create(:system_note_metadata, note: branch_note, action: "branch") } + let!(:commit_note_meta) { create(:system_note_metadata, note: commit_note, action: "commit") } + + context 'user is allowed access' do + before do + project.add_user(user, :maintainer) + end + + it 'displays all available notes' do + get :discussions, params: { namespace_id: project.namespace, project_id: project, id: issue.iid } + + expect(json_response.length).to eq(3) + end + end + + context 'user is a guest' do + let(:json_response_note_ids) do + json_response.collect { |discussion| discussion["notes"] }.flatten + .collect { |note| note["id"].to_i } + end + + before do + project.add_guest(user) + end + + it 'does not display notes w/type listed in TYPES_RESTRICTED_BY_ACCESS_LEVEL' do + get :discussions, params: { namespace_id: project.namespace, project_id: project, id: issue.iid } + + expect(json_response.length).to eq(2) + expect(json_response_note_ids).not_to include(branch_note.id) + end + end + end end end diff --git a/spec/features/cycle_analytics_spec.rb b/spec/features/cycle_analytics_spec.rb index 0fc4841ee0e..e9751aa2e72 100644 --- a/spec/features/cycle_analytics_spec.rb +++ b/spec/features/cycle_analytics_spec.rb @@ -112,6 +112,10 @@ describe 'Cycle Analytics', :js do wait_for_requests end + it 'does not show the commit stats' do + expect(page).to have_no_selector(:xpath, commits_counter_selector) + end + it 'needs permissions to see restricted stages' do expect(find('.stage-events')).to have_content(issue.title) @@ -127,8 +131,12 @@ describe 'Cycle Analytics', :js do find(:xpath, "//p[contains(text(),'New Issue')]/preceding-sibling::h3") end + def commits_counter_selector + "//p[contains(text(),'Commits')]/preceding-sibling::h3" + end + def commits_counter - find(:xpath, "//p[contains(text(),'Commits')]/preceding-sibling::h3") + find(:xpath, commits_counter_selector) end def deploys_counter diff --git a/spec/features/projects_spec.rb b/spec/features/projects_spec.rb index 90e48f3c230..47f32e0113c 100644 --- a/spec/features/projects_spec.rb +++ b/spec/features/projects_spec.rb @@ -202,13 +202,13 @@ describe 'Project' do expect(page).not_to have_content('Forked from') end - it 'shows the name of the deleted project when the source was deleted', :sidekiq_might_not_need_inline do + it 'does not show the name of the deleted project when the source was deleted', :sidekiq_might_not_need_inline do forked_project Projects::DestroyService.new(base_project, base_project.owner).execute visit project_path(forked_project) - expect(page).to have_content("Forked from #{base_project.full_name} (deleted)") + expect(page).to have_content('Forked from an inaccessible project') end context 'a fork of a fork' do diff --git a/spec/initializers/hangouts_chat_http_override_spec.rb b/spec/initializers/hangouts_chat_http_override_spec.rb new file mode 100644 index 00000000000..0eee891799f --- /dev/null +++ b/spec/initializers/hangouts_chat_http_override_spec.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe 'HangoutsChat::Sender Gitlab::HTTP override' do + describe 'HangoutsChat::Sender::HTTP#post' do + it 'calls Gitlab::HTTP.post with default protection settings' do + webhook_url = 'https://example.gitlab.com' + payload = { key: 'value' } + http = HangoutsChat::Sender::HTTP.new(webhook_url) + mock_response = double(response: 'the response') + + expect(Gitlab::HTTP).to receive(:post) + .with( + URI.parse(webhook_url), + body: payload.to_json, + headers: { 'Content-Type' => 'application/json' }, + parse: nil + ) + .and_return(mock_response) + + expect(http.post(payload)).to eq(mock_response.response) + end + + it_behaves_like 'a request using Gitlab::UrlBlocker' do + let(:http_method) { :post } + let(:url_blocked_error_class) { Gitlab::HTTP::BlockedUrlError } + + def make_request(uri) + HangoutsChat::Sender::HTTP.new(uri).post({}) + end + end + end +end diff --git a/spec/initializers/rest-client-hostname_override_spec.rb b/spec/initializers/rest-client-hostname_override_spec.rb index 90a0305c9a9..7e36656ba1c 100644 --- a/spec/initializers/rest-client-hostname_override_spec.rb +++ b/spec/initializers/rest-client-hostname_override_spec.rb @@ -3,147 +3,12 @@ require 'spec_helper' describe 'rest-client dns rebinding protection' do - include StubRequests + it_behaves_like 'a request using Gitlab::UrlBlocker' do + let(:http_method) { :get } + let(:url_blocked_error_class) { ArgumentError } - context 'when local requests are not allowed' do - it 'allows an external request with http' do - request_stub = stub_full_request('http://example.com', ip_address: '93.184.216.34') - - RestClient.get('http://example.com/') - - expect(request_stub).to have_been_requested - end - - it 'allows an external request with https' do - request_stub = stub_full_request('https://example.com', ip_address: '93.184.216.34') - - RestClient.get('https://example.com/') - - expect(request_stub).to have_been_requested - end - - it 'raises error when it is a request that resolves to a local address' do - stub_full_request('https://example.com', ip_address: '172.16.0.0') - - expect { RestClient.get('https://example.com') } - .to raise_error(ArgumentError, - "URL 'https://example.com' is blocked: Requests to the local network are not allowed") - end - - it 'raises error when it is a request that resolves to a localhost address' do - stub_full_request('https://example.com', ip_address: '127.0.0.1') - - expect { RestClient.get('https://example.com') } - .to raise_error(ArgumentError, - "URL 'https://example.com' is blocked: Requests to localhost are not allowed") - end - - it 'raises error when it is a request to local address' do - expect { RestClient.get('http://172.16.0.0') } - .to raise_error(ArgumentError, - "URL 'http://172.16.0.0' is blocked: Requests to the local network are not allowed") - end - - it 'raises error when it is a request to localhost address' do - expect { RestClient.get('http://127.0.0.1') } - .to raise_error(ArgumentError, - "URL 'http://127.0.0.1' is blocked: Requests to localhost are not allowed") - end - end - - context 'when port different from URL scheme is used' do - it 'allows the request' do - request_stub = stub_full_request('https://example.com:8080', ip_address: '93.184.216.34') - - RestClient.get('https://example.com:8080/') - - expect(request_stub).to have_been_requested - end - - it 'raises error when it is a request to local address' do - expect { RestClient.get('https://172.16.0.0:8080') } - .to raise_error(ArgumentError, - "URL 'https://172.16.0.0:8080' is blocked: Requests to the local network are not allowed") - end - - it 'raises error when it is a request to localhost address' do - expect { RestClient.get('https://127.0.0.1:8080') } - .to raise_error(ArgumentError, - "URL 'https://127.0.0.1:8080' is blocked: Requests to localhost are not allowed") - end - end - - context 'when DNS rebinding protection is disabled' do - before do - stub_application_setting(dns_rebinding_protection_enabled: false) - end - - it 'allows the request' do - request_stub = stub_request(:get, 'https://example.com') - - RestClient.get('https://example.com/') - - expect(request_stub).to have_been_requested - end - end - - context 'when http(s) proxy environment variable is set' do - before do - stub_env('https_proxy' => 'https://my.proxy') - end - - it 'allows the request' do - request_stub = stub_request(:get, 'https://example.com') - - RestClient.get('https://example.com/') - - expect(request_stub).to have_been_requested - end - end - - context 'when local requests are allowed' do - before do - stub_application_setting(allow_local_requests_from_web_hooks_and_services: true) - end - - it 'allows an external request' do - request_stub = stub_full_request('https://example.com', ip_address: '93.184.216.34') - - RestClient.get('https://example.com/') - - expect(request_stub).to have_been_requested - end - - it 'allows an external request that resolves to a local address' do - request_stub = stub_full_request('https://example.com', ip_address: '172.16.0.0') - - RestClient.get('https://example.com/') - - expect(request_stub).to have_been_requested - end - - it 'allows an external request that resolves to a localhost address' do - request_stub = stub_full_request('https://example.com', ip_address: '127.0.0.1') - - RestClient.get('https://example.com/') - - expect(request_stub).to have_been_requested - end - - it 'allows a local address request' do - request_stub = stub_request(:get, 'http://172.16.0.0') - - RestClient.get('http://172.16.0.0') - - expect(request_stub).to have_been_requested - end - - it 'allows a localhost address request' do - request_stub = stub_request(:get, 'http://127.0.0.1') - - RestClient.get('http://127.0.0.1') - - expect(request_stub).to have_been_requested + def make_request(uri) + RestClient.get(uri) end end end diff --git a/spec/lib/banzai/filter/label_reference_filter_spec.rb b/spec/lib/banzai/filter/label_reference_filter_spec.rb index 35e99d2586e..66af26bc51c 100644 --- a/spec/lib/banzai/filter/label_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/label_reference_filter_spec.rb @@ -521,6 +521,15 @@ describe Banzai::Filter::LabelReferenceFilter do expect(reference_filter(act).to_html).to eq exp end + + context 'when group name has HTML entities' do + let(:another_group) { create(:group, name: '<img src=x onerror=alert(1)>', path: 'another_group') } + + it 'escapes the HTML entities' do + expect(result.text) + .to eq "See #{group_label.name} in #{another_project.full_name}" + end + end end describe 'cross-project / same-group_label complete reference' do diff --git a/spec/lib/banzai/filter/relative_link_filter_spec.rb b/spec/lib/banzai/filter/relative_link_filter_spec.rb index 046c346a7ac..371c7a2347c 100644 --- a/spec/lib/banzai/filter/relative_link_filter_spec.rb +++ b/spec/lib/banzai/filter/relative_link_filter_spec.rb @@ -119,6 +119,11 @@ describe Banzai::Filter::RelativeLinkFilter do expect { filter(act) }.not_to raise_error end + it 'does not raise an exception on URIs containing invalid utf-8 byte sequences' do + act = link("%FF") + expect { filter(act) }.not_to raise_error + end + it 'does not raise an exception with a garbled path' do act = link("open(/var/tmp/):%20/location%0Afrom:%20/test") expect { filter(act) }.not_to raise_error diff --git a/spec/lib/gitlab/cycle_analytics/stage_summary_spec.rb b/spec/lib/gitlab/cycle_analytics/stage_summary_spec.rb index 8f9dac6d281..94edef20296 100644 --- a/spec/lib/gitlab/cycle_analytics/stage_summary_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/stage_summary_spec.rb @@ -6,6 +6,11 @@ describe Gitlab::CycleAnalytics::StageSummary do let(:project) { create(:project, :repository) } let(:options) { { from: 1.day.ago, current_user: user } } let(:user) { create(:user, :admin) } + + before do + project.add_maintainer(user) + end + let(:stage_summary) { described_class.new(project, options).data } describe "#new_issues" do @@ -86,6 +91,24 @@ describe Gitlab::CycleAnalytics::StageSummary do expect(subject).to eq(2) end end + + context 'when a guest user is signed in' do + let(:guest_user) { create(:user) } + + before do + project.add_guest(guest_user) + options.merge!({ current_user: guest_user }) + end + + it 'does not include commit stats' do + data = described_class.new(project, options).data + expect(includes_commits?(data)).to be_falsy + end + + def includes_commits?(data) + data.any? { |h| h["title"] == 'Commits' } + end + end end describe "#deploys" do diff --git a/spec/lib/gitlab/import_export/attribute_cleaner_spec.rb b/spec/lib/gitlab/import_export/attribute_cleaner_spec.rb index c4052415ab0..44192c4639d 100644 --- a/spec/lib/gitlab/import_export/attribute_cleaner_spec.rb +++ b/spec/lib/gitlab/import_export/attribute_cleaner_spec.rb @@ -26,7 +26,10 @@ describe Gitlab::ImportExport::AttributeCleaner do '_html' => '<p>perfectly ordinary html</p>', 'cached_markdown_version' => 12345, 'group_id' => 99, - 'commit_id' => 99 + 'commit_id' => 99, + 'issue_ids' => [1, 2, 3], + 'merge_request_ids' => [1, 2, 3], + 'note_ids' => [1, 2, 3] } end diff --git a/spec/migrations/encrypt_plaintext_attributes_on_application_settings_spec.rb b/spec/migrations/encrypt_plaintext_attributes_on_application_settings_spec.rb new file mode 100644 index 00000000000..122da7b3d72 --- /dev/null +++ b/spec/migrations/encrypt_plaintext_attributes_on_application_settings_spec.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +require 'spec_helper' +require Rails.root.join('db', 'migrate', '20191120115530_encrypt_plaintext_attributes_on_application_settings.rb') + +describe EncryptPlaintextAttributesOnApplicationSettings, :migration do + let(:migration) { described_class.new } + let(:application_settings) { table(:application_settings) } + let(:plaintext) { 'secret-token' } + + PLAINTEXT_ATTRIBUTES = %w[ + akismet_api_key + elasticsearch_aws_secret_access_key + recaptcha_private_key + recaptcha_site_key + slack_app_secret + slack_app_verification_token + ].freeze + + describe '#up' do + it 'encrypts token and saves it' do + application_setting = application_settings.create + application_setting.update_columns( + PLAINTEXT_ATTRIBUTES.each_with_object({}) do |plaintext_attribute, attributes| + attributes[plaintext_attribute] = plaintext + end + ) + + migration.up + + application_setting.reload + PLAINTEXT_ATTRIBUTES.each do |plaintext_attribute| + expect(application_setting[plaintext_attribute]).not_to be_nil + expect(application_setting["encrypted_#{plaintext_attribute}"]).not_to be_nil + expect(application_setting["encrypted_#{plaintext_attribute}_iv"]).not_to be_nil + end + end + end + + describe '#down' do + it 'decrypts encrypted token and saves it' do + application_setting = application_settings.create( + PLAINTEXT_ATTRIBUTES.each_with_object({}) do |plaintext_attribute, attributes| + attributes[plaintext_attribute] = plaintext + end + ) + + migration.down + + application_setting.reload + PLAINTEXT_ATTRIBUTES.each do |plaintext_attribute| + expect(application_setting[plaintext_attribute]).to eq(plaintext) + expect(application_setting["encrypted_#{plaintext_attribute}"]).to be_nil + expect(application_setting["encrypted_#{plaintext_attribute}_iv"]).to be_nil + end + end + end +end diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index b09e699a8a7..74f2fc1bb61 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -285,6 +285,70 @@ describe Note do end end + describe "#visible_for?" do + using RSpec::Parameterized::TableSyntax + + let_it_be(:note) { create(:note) } + let_it_be(:user) { create(:user) } + + where(:cross_reference_visible, :system_note_viewable, :result) do + true | true | false + false | true | true + false | false | false + end + + with_them do + it "returns expected result" do + expect(note).to receive(:cross_reference_not_visible_for?).and_return(cross_reference_visible) + + unless cross_reference_visible + expect(note).to receive(:system_note_viewable_by?) + .with(user).and_return(system_note_viewable) + end + + expect(note.visible_for?(user)).to eq result + end + end + end + + describe "#system_note_viewable_by?(user)" do + let_it_be(:note) { create(:note) } + let_it_be(:user) { create(:user) } + let!(:metadata) { create(:system_note_metadata, note: note, action: "branch") } + + context "when system_note_metadata is not present" do + it "returns true" do + expect(note).to receive(:system_note_metadata).and_return(nil) + + expect(note.send(:system_note_viewable_by?, user)).to be_truthy + end + end + + context "system_note_metadata isn't of type 'branch'" do + before do + metadata.action = "not_a_branch" + end + + it "returns true" do + expect(note.send(:system_note_viewable_by?, user)).to be_truthy + end + end + + context "user doesn't have :download_code ability" do + it "returns false" do + expect(note.send(:system_note_viewable_by?, user)).to be_falsey + end + end + + context "user has the :download_code ability" do + it "returns true" do + expect(Ability).to receive(:allowed?).with(user, :download_code, note.project).and_return(true) + + expect(note.send(:system_note_viewable_by?, user)).to be_truthy + end + end + end + describe "cross_reference_not_visible_for?" do let(:private_user) { create(:user) } let(:private_project) { create(:project, namespace: private_user.namespace) { |p| p.add_maintainer(private_user) } } diff --git a/spec/models/project_feature_spec.rb b/spec/models/project_feature_spec.rb index 5b448c343a0..9ce1b8fd895 100644 --- a/spec/models/project_feature_spec.rb +++ b/spec/models/project_feature_spec.rb @@ -6,6 +6,16 @@ describe ProjectFeature do let(:project) { create(:project) } let(:user) { create(:user) } + describe 'PRIVATE_FEATURES_MIN_ACCESS_LEVEL_FOR_PRIVATE_PROJECT' do + it 'has higher level than that of PRIVATE_FEATURES_MIN_ACCESS_LEVEL' do + described_class::PRIVATE_FEATURES_MIN_ACCESS_LEVEL_FOR_PRIVATE_PROJECT.each do |feature, level| + if generic_level = described_class::PRIVATE_FEATURES_MIN_ACCESS_LEVEL[feature] + expect(level).to be >= generic_level + end + end + end + end + describe '.quoted_access_level_column' do it 'returns the table name and quoted column name for a feature' do expected = '"project_features"."issues_access_level"' @@ -246,10 +256,24 @@ describe ProjectFeature do expect(described_class.required_minimum_access_level('merge_requests')).to eq(Gitlab::Access::REPORTER) end + it 'handles repository' do + expect(described_class.required_minimum_access_level(:repository)).to eq(Gitlab::Access::GUEST) + end + it 'raises error if feature is invalid' do expect do described_class.required_minimum_access_level(:foos) end.to raise_error end end + + describe '.required_minimum_access_level_for_private_project' do + it 'returns higher permission for repository' do + expect(described_class.required_minimum_access_level_for_private_project(:repository)).to eq(Gitlab::Access::REPORTER) + end + + it 'returns normal permission for issues' do + expect(described_class.required_minimum_access_level_for_private_project(:issues)).to eq(Gitlab::Access::GUEST) + end + end end diff --git a/spec/models/project_services/chat_notification_service_spec.rb b/spec/models/project_services/chat_notification_service_spec.rb index 6f4ddd223f6..e8c5f5d611a 100644 --- a/spec/models/project_services/chat_notification_service_spec.rb +++ b/spec/models/project_services/chat_notification_service_spec.rb @@ -30,7 +30,8 @@ describe ChatNotificationService do end describe '#execute' do - let(:chat_service) { described_class.new } + subject(:chat_service) { described_class.new } + let(:user) { create(:user) } let(:project) { create(:project, :repository) } let(:webhook_url) { 'https://example.gitlab.com/' } @@ -53,12 +54,7 @@ describe ChatNotificationService do subject.project = project data = Gitlab::DataBuilder::Push.build_sample(project, user) - expect(Slack::Notifier).to receive(:new) - .with(webhook_url, {}) - .and_return( - double(:slack_service).as_null_object - ) - + expect(chat_service).to receive(:notify).and_return(true) expect(chat_service.execute(data)).to be true end end @@ -68,12 +64,7 @@ describe ChatNotificationService do subject.project = create(:project, :empty_repo) data = Gitlab::DataBuilder::Push.build_sample(subject.project, user) - expect(Slack::Notifier).to receive(:new) - .with(webhook_url, {}) - .and_return( - double(:slack_service).as_null_object - ) - + expect(chat_service).to receive(:notify).and_return(true) expect(chat_service.execute(data)).to be true end end diff --git a/spec/models/project_services/slack_service_spec.rb b/spec/models/project_services/slack_service_spec.rb index f751dd6ffb9..93036ac7ec4 100644 --- a/spec/models/project_services/slack_service_spec.rb +++ b/spec/models/project_services/slack_service_spec.rb @@ -3,5 +3,5 @@ require 'spec_helper' describe SlackService do - it_behaves_like "slack or mattermost notifications", "Slack" + it_behaves_like "slack or mattermost notifications", 'Slack' end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 4e6a4bd6b51..ac6ae5e1cc6 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2924,7 +2924,7 @@ describe Project do end describe '#any_lfs_file_locks?', :request_store do - set(:project) { create(:project) } + let_it_be(:project) { create(:project) } it 'returns false when there are no LFS file locks' do expect(project.any_lfs_file_locks?).to be_falsey @@ -3411,6 +3411,20 @@ describe Project do expect(projects).to eq([public_project]) end end + + context 'min_access_level' do + let!(:private_project) { create(:project, :private) } + + before do + private_project.add_guest(user) + end + + it 'excludes projects when user does not have required minimum access level' do + projects = described_class.all.public_or_visible_to_user(user, Gitlab::Access::REPORTER) + + expect(projects).to contain_exactly(public_project) + end + end end describe '.ids_with_issuables_available_for' do @@ -3539,7 +3553,7 @@ describe Project do include ProjectHelpers using RSpec::Parameterized::TableSyntax - set(:group) { create(:group) } + let_it_be(:group) { create(:group) } let!(:project) { create(:project, project_level, namespace: group ) } let(:user) { create_user_from_membership(project, membership) } @@ -3562,6 +3576,66 @@ describe Project do end end end + + context 'issues' do + let(:feature) { Issue } + + where(:project_level, :feature_access_level, :membership, :expected_count) do + permission_table_for_guest_feature_access + end + + with_them do + it "respects visibility" do + update_feature_access_level(project, feature_access_level) + + expected_objects = expected_count == 1 ? [project] : [] + + expect( + described_class.filter_by_feature_visibility(feature, user) + ).to eq(expected_objects) + end + end + end + + context 'wiki' do + let(:feature) { :wiki } + + where(:project_level, :feature_access_level, :membership, :expected_count) do + permission_table_for_guest_feature_access + end + + with_them do + it "respects visibility" do + update_feature_access_level(project, feature_access_level) + + expected_objects = expected_count == 1 ? [project] : [] + + expect( + described_class.filter_by_feature_visibility(feature, user) + ).to eq(expected_objects) + end + end + end + + context 'code' do + let(:feature) { :repository } + + where(:project_level, :feature_access_level, :membership, :expected_count) do + permission_table_for_guest_feature_access_and_non_private_project_only + end + + with_them do + it "respects visibility" do + update_feature_access_level(project, feature_access_level) + + expected_objects = expected_count == 1 ? [project] : [] + + expect( + described_class.filter_by_feature_visibility(feature, user) + ).to eq(expected_objects) + end + end + end end describe '#pages_available?' do diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index f1447536e0f..cda2dd7d2f4 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -49,6 +49,8 @@ shared_examples 'languages and percentages JSON response' do end describe API::Projects do + include ProjectForksHelper + let(:user) { create(:user) } let(:user2) { create(:user) } let(:user3) { create(:user) } @@ -1163,6 +1165,18 @@ describe API::Projects do expect(json_response.keys).not_to include('permissions') end + context 'the project is a public fork' do + it 'hides details of a public fork parent' do + public_project = create(:project, :repository, :public) + fork = fork_project(public_project) + + get api("/projects/#{fork.id}") + + expect(response).to have_gitlab_http_status(200) + expect(json_response['forked_from_project']).to be_nil + end + end + context 'and the project has a private repository' do let(:project) { create(:project, :repository, :public, :repository_private) } let(:protected_attributes) { %w(default_branch ci_config_path) } @@ -1479,6 +1493,28 @@ describe API::Projects do end end + context 'the project is a fork' do + it 'shows details of a visible fork parent' do + fork = fork_project(project, user) + + get api("/projects/#{fork.id}", user) + + expect(response).to have_gitlab_http_status(200) + expect(json_response['forked_from_project']).to include('id' => project.id) + end + + it 'hides details of a hidden fork parent' do + fork = fork_project(project, user) + fork_user = create(:user) + fork.team.add_developer(fork_user) + + get api("/projects/#{fork.id}", fork_user) + + expect(response).to have_gitlab_http_status(200) + expect(json_response['forked_from_project']).to be_nil + end + end + describe 'permissions' do context 'all projects' do before do diff --git a/spec/support/helpers/project_helpers.rb b/spec/support/helpers/project_helpers.rb index 61056b47aed..89f0163b4b6 100644 --- a/spec/support/helpers/project_helpers.rb +++ b/spec/support/helpers/project_helpers.rb @@ -10,16 +10,18 @@ module ProjectHelpers nil when :non_member create(:user, name: membership) + when :admin + create(:user, :admin, name: 'admin') else create(:user, name: membership).tap { |u| target.add_user(u, membership) } end end def update_feature_access_level(project, access_level) - project.update!( - repository_access_level: access_level, - merge_requests_access_level: access_level, - builds_access_level: access_level - ) + features = ProjectFeature::FEATURES.dup + features.delete(:pages) + params = features.each_with_object({}) { |feature, h| h["#{feature}_access_level"] = access_level } + + project.update!(params) end end diff --git a/spec/support/helpers/search_helpers.rb b/spec/support/helpers/search_helpers.rb index d1d25fbabcd..db6e47459e9 100644 --- a/spec/support/helpers/search_helpers.rb +++ b/spec/support/helpers/search_helpers.rb @@ -20,6 +20,12 @@ module SearchHelpers end end + def has_search_scope?(scope) + page.within '.search-filter' do + has_link?(scope) + end + end + def max_limited_count Gitlab::SearchResults::COUNT_LIMIT_MESSAGE end diff --git a/spec/support/shared_contexts/policies/project_policy_table_shared_context.rb b/spec/support/shared_contexts/policies/project_policy_table_shared_context.rb index e666b346b8b..0a918ccde81 100644 --- a/spec/support/shared_contexts/policies/project_policy_table_shared_context.rb +++ b/spec/support/shared_contexts/policies/project_policy_table_shared_context.rb @@ -3,13 +3,28 @@ RSpec.shared_context 'ProjectPolicyTable context' do using RSpec::Parameterized::TableSyntax + let(:pendings) { {} } + let(:pending?) do + pendings.include?( + { + project_level: project_level, + feature_access_level: feature_access_level, + membership: membership, + expected_count: expected_count + } + ) + end + # rubocop:disable Metrics/AbcSize + # project_level, :feature_access_level, :membership, :expected_count def permission_table_for_reporter_feature_access + :public | :enabled | :admin | 1 :public | :enabled | :reporter | 1 :public | :enabled | :guest | 1 :public | :enabled | :non_member | 1 :public | :enabled | :anonymous | 1 + :public | :private | :admin | 1 :public | :private | :reporter | 1 :public | :private | :guest | 0 :public | :private | :non_member | 0 @@ -20,11 +35,13 @@ RSpec.shared_context 'ProjectPolicyTable context' do :public | :disabled | :non_member | 0 :public | :disabled | :anonymous | 0 + :internal | :enabled | :admin | 1 :internal | :enabled | :reporter | 1 :internal | :enabled | :guest | 1 :internal | :enabled | :non_member | 1 :internal | :enabled | :anonymous | 0 + :internal | :private | :admin | 1 :internal | :private | :reporter | 1 :internal | :private | :guest | 0 :internal | :private | :non_member | 0 @@ -35,11 +52,7 @@ RSpec.shared_context 'ProjectPolicyTable context' do :internal | :disabled | :non_member | 0 :internal | :disabled | :anonymous | 0 - :private | :enabled | :reporter | 1 - :private | :enabled | :guest | 1 - :private | :enabled | :non_member | 0 - :private | :enabled | :anonymous | 0 - + :private | :private | :admin | 1 :private | :private | :reporter | 1 :private | :private | :guest | 0 :private | :private | :non_member | 0 @@ -51,12 +64,15 @@ RSpec.shared_context 'ProjectPolicyTable context' do :private | :disabled | :anonymous | 0 end + # project_level, :feature_access_level, :membership, :expected_count def permission_table_for_guest_feature_access + :public | :enabled | :admin | 1 :public | :enabled | :reporter | 1 :public | :enabled | :guest | 1 :public | :enabled | :non_member | 1 :public | :enabled | :anonymous | 1 + :public | :private | :admin | 1 :public | :private | :reporter | 1 :public | :private | :guest | 1 :public | :private | :non_member | 0 @@ -67,11 +83,13 @@ RSpec.shared_context 'ProjectPolicyTable context' do :public | :disabled | :non_member | 0 :public | :disabled | :anonymous | 0 + :internal | :enabled | :admin | 1 :internal | :enabled | :reporter | 1 :internal | :enabled | :guest | 1 :internal | :enabled | :non_member | 1 :internal | :enabled | :anonymous | 0 + :internal | :private | :admin | 1 :internal | :private | :reporter | 1 :internal | :private | :guest | 1 :internal | :private | :non_member | 0 @@ -82,11 +100,7 @@ RSpec.shared_context 'ProjectPolicyTable context' do :internal | :disabled | :non_member | 0 :internal | :disabled | :anonymous | 0 - :private | :enabled | :reporter | 1 - :private | :enabled | :guest | 1 - :private | :enabled | :non_member | 0 - :private | :enabled | :anonymous | 0 - + :private | :private | :admin | 1 :private | :private | :reporter | 1 :private | :private | :guest | 1 :private | :private | :non_member | 0 @@ -98,6 +112,196 @@ RSpec.shared_context 'ProjectPolicyTable context' do :private | :disabled | :anonymous | 0 end + # This table is based on permission_table_for_guest_feature_access, + # but with a slight twist. + # Some features can be hidden away to GUEST, when project is private. + # (see ProjectFeature::PRIVATE_FEATURES_MIN_ACCESS_LEVEL_FOR_PRIVATE_PROJECT) + # This is the table for such features. + # + # e.g. `repository` feature has minimum requirement of GUEST, + # but a GUEST are prohibited from reading code if project is private. + # + # project_level, :feature_access_level, :membership, :expected_count + def permission_table_for_guest_feature_access_and_non_private_project_only + :public | :enabled | :admin | 1 + :public | :enabled | :reporter | 1 + :public | :enabled | :guest | 1 + :public | :enabled | :non_member | 1 + :public | :enabled | :anonymous | 1 + + :public | :private | :admin | 1 + :public | :private | :reporter | 1 + :public | :private | :guest | 1 + :public | :private | :non_member | 0 + :public | :private | :anonymous | 0 + + :public | :disabled | :reporter | 0 + :public | :disabled | :guest | 0 + :public | :disabled | :non_member | 0 + :public | :disabled | :anonymous | 0 + + :internal | :enabled | :admin | 1 + :internal | :enabled | :reporter | 1 + :internal | :enabled | :guest | 1 + :internal | :enabled | :non_member | 1 + :internal | :enabled | :anonymous | 0 + + :internal | :private | :admin | 1 + :internal | :private | :reporter | 1 + :internal | :private | :guest | 1 + :internal | :private | :non_member | 0 + :internal | :private | :anonymous | 0 + + :internal | :disabled | :reporter | 0 + :internal | :disabled | :guest | 0 + :internal | :disabled | :non_member | 0 + :internal | :disabled | :anonymous | 0 + + :private | :private | :admin | 1 + :private | :private | :reporter | 1 + :private | :private | :guest | 0 + :private | :private | :non_member | 0 + :private | :private | :anonymous | 0 + + :private | :disabled | :reporter | 0 + :private | :disabled | :guest | 0 + :private | :disabled | :non_member | 0 + :private | :disabled | :anonymous | 0 + end + + # :project_level, :issues_access_level, :merge_requests_access_level, :membership, :expected_count + def permission_table_for_milestone_access + :public | :enabled | :enabled | :admin | 1 + :public | :enabled | :enabled | :reporter | 1 + :public | :enabled | :enabled | :guest | 1 + :public | :enabled | :enabled | :non_member | 1 + :public | :enabled | :enabled | :anonymous | 1 + + :public | :enabled | :private | :admin | 1 + :public | :enabled | :private | :reporter | 1 + :public | :enabled | :private | :guest | 1 + :public | :enabled | :private | :non_member | 1 + :public | :enabled | :private | :anonymous | 1 + + :public | :enabled | :disabled | :admin | 1 + :public | :enabled | :disabled | :reporter | 1 + :public | :enabled | :disabled | :guest | 1 + :public | :enabled | :disabled | :non_member | 1 + :public | :enabled | :disabled | :anonymous | 1 + + :public | :private | :enabled | :admin | 1 + :public | :private | :enabled | :reporter | 1 + :public | :private | :enabled | :guest | 1 + :public | :private | :enabled | :non_member | 1 + :public | :private | :enabled | :anonymous | 1 + + :public | :private | :private | :admin | 1 + :public | :private | :private | :reporter | 1 + :public | :private | :private | :guest | 1 + :public | :private | :private | :non_member | 0 + :public | :private | :private | :anonymous | 0 + + :public | :private | :disabled | :admin | 1 + :public | :private | :disabled | :reporter | 1 + :public | :private | :disabled | :guest | 1 + :public | :private | :disabled | :non_member | 0 + :public | :private | :disabled | :anonymous | 0 + + :public | :disabled | :enabled | :admin | 1 + :public | :disabled | :enabled | :reporter | 1 + :public | :disabled | :enabled | :guest | 1 + :public | :disabled | :enabled | :non_member | 1 + :public | :disabled | :enabled | :anonymous | 1 + + :public | :disabled | :private | :admin | 1 + :public | :disabled | :private | :reporter | 1 + :public | :disabled | :private | :guest | 0 + :public | :disabled | :private | :non_member | 0 + :public | :disabled | :private | :anonymous | 0 + + :public | :disabled | :disabled | :reporter | 0 + :public | :disabled | :disabled | :guest | 0 + :public | :disabled | :disabled | :non_member | 0 + :public | :disabled | :disabled | :anonymous | 0 + + :internal | :enabled | :enabled | :admin | 1 + :internal | :enabled | :enabled | :reporter | 1 + :internal | :enabled | :enabled | :guest | 1 + :internal | :enabled | :enabled | :non_member | 1 + :internal | :enabled | :enabled | :anonymous | 0 + + :internal | :enabled | :private | :admin | 1 + :internal | :enabled | :private | :reporter | 1 + :internal | :enabled | :private | :guest | 1 + :internal | :enabled | :private | :non_member | 1 + :internal | :enabled | :private | :anonymous | 0 + + :internal | :enabled | :disabled | :admin | 1 + :internal | :enabled | :disabled | :reporter | 1 + :internal | :enabled | :disabled | :guest | 1 + :internal | :enabled | :disabled | :non_member | 1 + :internal | :enabled | :disabled | :anonymous | 0 + + :internal | :private | :enabled | :admin | 1 + :internal | :private | :enabled | :reporter | 1 + :internal | :private | :enabled | :guest | 1 + :internal | :private | :enabled | :non_member | 1 + :internal | :private | :enabled | :anonymous | 0 + + :internal | :private | :private | :admin | 1 + :internal | :private | :private | :reporter | 1 + :internal | :private | :private | :guest | 1 + :internal | :private | :private | :non_member | 0 + :internal | :private | :private | :anonymous | 0 + + :internal | :private | :disabled | :admin | 1 + :internal | :private | :disabled | :reporter | 1 + :internal | :private | :disabled | :guest | 1 + :internal | :private | :disabled | :non_member | 0 + :internal | :private | :disabled | :anonymous | 0 + + :internal | :disabled | :enabled | :admin | 1 + :internal | :disabled | :enabled | :reporter | 1 + :internal | :disabled | :enabled | :guest | 1 + :internal | :disabled | :enabled | :non_member | 1 + :internal | :disabled | :enabled | :anonymous | 0 + + :internal | :disabled | :private | :admin | 1 + :internal | :disabled | :private | :reporter | 1 + :internal | :disabled | :private | :guest | 0 + :internal | :disabled | :private | :non_member | 0 + :internal | :disabled | :private | :anonymous | 0 + + :internal | :disabled | :disabled | :reporter | 0 + :internal | :disabled | :disabled | :guest | 0 + :internal | :disabled | :disabled | :non_member | 0 + :internal | :disabled | :disabled | :anonymous | 0 + + :private | :private | :private | :admin | 1 + :private | :private | :private | :reporter | 1 + :private | :private | :private | :guest | 1 + :private | :private | :private | :non_member | 0 + :private | :private | :private | :anonymous | 0 + + :private | :private | :disabled | :admin | 1 + :private | :private | :disabled | :reporter | 1 + :private | :private | :disabled | :guest | 1 + :private | :private | :disabled | :non_member | 0 + :private | :private | :disabled | :anonymous | 0 + + :private | :disabled | :private | :admin | 1 + :private | :disabled | :private | :reporter | 1 + :private | :disabled | :private | :guest | 0 + :private | :disabled | :private | :non_member | 0 + :private | :disabled | :private | :anonymous | 0 + + :private | :disabled | :disabled | :reporter | 0 + :private | :disabled | :disabled | :guest | 0 + :private | :disabled | :disabled | :non_member | 0 + :private | :disabled | :disabled | :anonymous | 0 + end + + # :project_level, :membership, :expected_count def permission_table_for_project_access :public | :reporter | 1 :public | :guest | 1 diff --git a/spec/support/shared_examples/slack_mattermost_notifications_shared_examples.rb b/spec/support/shared_examples/slack_mattermost_notifications_shared_examples.rb index f15128d3e13..2b68e7bfa82 100644 --- a/spec/support/shared_examples/slack_mattermost_notifications_shared_examples.rb +++ b/spec/support/shared_examples/slack_mattermost_notifications_shared_examples.rb @@ -1,11 +1,13 @@ # frozen_string_literal: true RSpec.shared_examples 'slack or mattermost notifications' do |service_name| + include StubRequests + let(:chat_service) { described_class.new } - let(:webhook_url) { 'https://example.gitlab.com/' } + let(:webhook_url) { 'https://example.gitlab.com' } def execute_with_options(options) - receive(:new).with(webhook_url, options) + receive(:new).with(webhook_url, options.merge(http_client: SlackService::Notifier::HTTPClient)) .and_return(double(:slack_service).as_null_object) end @@ -38,9 +40,13 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| chat_service.branches_to_be_notified = branches_to_be_notified if branches_to_be_notified end + let!(:stubbed_resolved_hostname) do + stub_full_request(webhook_url, method: :post).request_pattern.uri_pattern.to_s + end + it "notifies about #{event_type} events" do chat_service.execute(data) - expect(WebMock).to have_requested(:post, webhook_url) + expect(WebMock).to have_requested(:post, stubbed_resolved_hostname) end end @@ -49,9 +55,13 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| chat_service.branches_to_be_notified = branches_to_be_notified if branches_to_be_notified end + let!(:stubbed_resolved_hostname) do + stub_full_request(webhook_url, method: :post).request_pattern.uri_pattern.to_s + end + it "notifies about #{event_type} events" do chat_service.execute(data) - expect(WebMock).not_to have_requested(:post, webhook_url) + expect(WebMock).not_to have_requested(:post, stubbed_resolved_hostname) end end @@ -66,6 +76,10 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| Gitlab::DataBuilder::Push.build_sample(project, user) end + let!(:stubbed_resolved_hostname) do + stub_full_request(webhook_url, method: :post).request_pattern.uri_pattern.to_s + end + before do allow(chat_service).to receive_messages( project: project, @@ -74,8 +88,6 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| webhook: webhook_url ) - WebMock.stub_request(:post, webhook_url) - issue_service = Issues::CreateService.new(project, user, issue_service_options) @issue = issue_service.execute @issues_sample_data = issue_service.hook_data(@issue, 'open') @@ -107,25 +119,25 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| it "calls #{service_name} API for push events" do chat_service.execute(data) - expect(WebMock).to have_requested(:post, webhook_url).once + expect(WebMock).to have_requested(:post, stubbed_resolved_hostname).once end it "calls #{service_name} API for issue events" do chat_service.execute(@issues_sample_data) - expect(WebMock).to have_requested(:post, webhook_url).once + expect(WebMock).to have_requested(:post, stubbed_resolved_hostname).once end it "calls #{service_name} API for merge requests events" do chat_service.execute(@merge_sample_data) - expect(WebMock).to have_requested(:post, webhook_url).once + expect(WebMock).to have_requested(:post, stubbed_resolved_hostname).once end it "calls #{service_name} API for wiki page events" do chat_service.execute(@wiki_page_sample_data) - expect(WebMock).to have_requested(:post, webhook_url).once + expect(WebMock).to have_requested(:post, stubbed_resolved_hostname).once end it "calls #{service_name} API for deployment events" do @@ -133,14 +145,14 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| chat_service.execute(deployment_event_data) - expect(WebMock).to have_requested(:post, webhook_url).once + expect(WebMock).to have_requested(:post, stubbed_resolved_hostname).once end it 'uses the username as an option for slack when configured' do allow(chat_service).to receive(:username).and_return(username) expect(Slack::Notifier).to receive(:new) - .with(webhook_url, username: username) + .with(webhook_url, username: username, http_client: SlackService::Notifier::HTTPClient) .and_return( double(:slack_service).as_null_object ) @@ -151,7 +163,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| it 'uses the channel as an option when it is configured' do allow(chat_service).to receive(:channel).and_return(channel) expect(Slack::Notifier).to receive(:new) - .with(webhook_url, channel: channel) + .with(webhook_url, channel: channel, http_client: SlackService::Notifier::HTTPClient) .and_return( double(:slack_service).as_null_object ) @@ -163,7 +175,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| chat_service.update(push_channel: "random") expect(Slack::Notifier).to receive(:new) - .with(webhook_url, channel: "random") + .with(webhook_url, channel: "random", http_client: SlackService::Notifier::HTTPClient) .and_return( double(:slack_service).as_null_object ) @@ -175,7 +187,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| chat_service.update(merge_request_channel: "random") expect(Slack::Notifier).to receive(:new) - .with(webhook_url, channel: "random") + .with(webhook_url, channel: "random", http_client: SlackService::Notifier::HTTPClient) .and_return( double(:slack_service).as_null_object ) @@ -187,7 +199,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| chat_service.update(issue_channel: "random") expect(Slack::Notifier).to receive(:new) - .with(webhook_url, channel: "random") + .with(webhook_url, channel: "random", http_client: SlackService::Notifier::HTTPClient) .and_return( double(:slack_service).as_null_object ) @@ -219,7 +231,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| chat_service.update(wiki_page_channel: "random") expect(Slack::Notifier).to receive(:new) - .with(webhook_url, channel: "random") + .with(webhook_url, channel: "random", http_client: SlackService::Notifier::HTTPClient) .and_return( double(:slack_service).as_null_object ) @@ -238,7 +250,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| note_data = Gitlab::DataBuilder::Note.build(issue_note, user) expect(Slack::Notifier).to receive(:new) - .with(webhook_url, channel: "random") + .with(webhook_url, channel: "random", http_client: SlackService::Notifier::HTTPClient) .and_return( double(:slack_service).as_null_object ) @@ -286,7 +298,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| webhook: webhook_url ) - WebMock.stub_request(:post, webhook_url) + stub_full_request(webhook_url, method: :post) end context 'on default branch' do @@ -461,7 +473,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| webhook: webhook_url ) - WebMock.stub_request(:post, webhook_url) + stub_full_request(webhook_url, method: :post) end context 'when commit comment event executed' do @@ -535,7 +547,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name| webhook: webhook_url ) - WebMock.stub_request(:post, webhook_url) + stub_full_request(webhook_url, method: :post) end context 'with succeeded pipeline' do diff --git a/spec/support/shared_examples/uses_gitlab_url_blocker_examples.rb b/spec/support/shared_examples/uses_gitlab_url_blocker_examples.rb new file mode 100644 index 00000000000..59c119e6d96 --- /dev/null +++ b/spec/support/shared_examples/uses_gitlab_url_blocker_examples.rb @@ -0,0 +1,155 @@ +# frozen_string_literal: true + +shared_examples 'a request using Gitlab::UrlBlocker' do + # Written to test internal patches against 3rd party libraries + # + # Expects the following to be available in the example contexts: + # + # make_request(uri): Wraps the request we want to test goes through Gitlab::HTTP + # http_method: :get, :post etc + # url_blocked_error_class: Probably just Gitlab::HTTP::BlockedUrlError + + include StubRequests + + context 'when local requests are not allowed' do + it 'allows an external request with http' do + request_stub = stub_full_request('http://example.com', method: http_method, ip_address: '93.184.216.34') + + make_request('http://example.com/') + + expect(request_stub).to have_been_requested + end + + it 'allows an external request with https' do + request_stub = stub_full_request('https://example.com', method: http_method, ip_address: '93.184.216.34') + + make_request('https://example.com/') + + expect(request_stub).to have_been_requested + end + + it 'raises error when it is a request that resolves to a local address' do + stub_full_request('https://example.com', method: http_method, ip_address: '172.16.0.0') + + expect { make_request('https://example.com') } + .to raise_error(url_blocked_error_class, + "URL 'https://example.com' is blocked: Requests to the local network are not allowed") + end + + it 'raises error when it is a request that resolves to a localhost address' do + stub_full_request('https://example.com', method: http_method, ip_address: '127.0.0.1') + + expect { make_request('https://example.com') } + .to raise_error(url_blocked_error_class, + "URL 'https://example.com' is blocked: Requests to localhost are not allowed") + end + + it 'raises error when it is a request to local address' do + expect { make_request('http://172.16.0.0') } + .to raise_error(url_blocked_error_class, + "URL 'http://172.16.0.0' is blocked: Requests to the local network are not allowed") + end + + it 'raises error when it is a request to localhost address' do + expect { make_request('http://127.0.0.1') } + .to raise_error(url_blocked_error_class, + "URL 'http://127.0.0.1' is blocked: Requests to localhost are not allowed") + end + end + + context 'when port different from URL scheme is used' do + it 'allows the request' do + request_stub = stub_full_request('https://example.com:8080', method: http_method, ip_address: '93.184.216.34') + + make_request('https://example.com:8080/') + + expect(request_stub).to have_been_requested + end + + it 'raises error when it is a request to local address' do + expect { make_request('https://172.16.0.0:8080') } + .to raise_error(url_blocked_error_class, + "URL 'https://172.16.0.0:8080' is blocked: Requests to the local network are not allowed") + end + + it 'raises error when it is a request to localhost address' do + expect { make_request('https://127.0.0.1:8080') } + .to raise_error(url_blocked_error_class, + "URL 'https://127.0.0.1:8080' is blocked: Requests to localhost are not allowed") + end + end + + context 'when DNS rebinding protection is disabled' do + before do + stub_application_setting(dns_rebinding_protection_enabled: false) + end + + it 'allows the request' do + request_stub = stub_request(http_method, 'https://example.com') + + make_request('https://example.com/') + + expect(request_stub).to have_been_requested + end + end + + context 'when http(s) proxy environment variable is set' do + before do + stub_env('https_proxy' => 'https://my.proxy') + end + + it 'allows the request' do + request_stub = stub_request(http_method, 'https://example.com') + + make_request('https://example.com/') + + expect(request_stub).to have_been_requested + end + end + + context 'when local requests are allowed' do + before do + stub_application_setting(allow_local_requests_from_web_hooks_and_services: true) + end + + it 'allows an external request' do + request_stub = stub_full_request('https://example.com', method: http_method, ip_address: '93.184.216.34') + + make_request('https://example.com/') + + expect(request_stub).to have_been_requested + end + + it 'allows an external request that resolves to a local address' do + request_stub = stub_full_request('https://example.com', method: http_method, ip_address: '172.16.0.0') + + make_request('https://example.com/') + + expect(request_stub).to have_been_requested + end + + it 'allows an external request that resolves to a localhost address' do + request_stub = stub_full_request('https://example.com', method: http_method, ip_address: '127.0.0.1') + + make_request('https://example.com/') + + expect(request_stub).to have_been_requested + end + + it 'allows a local address request' do + request_stub = stub_request(http_method, 'http://172.16.0.0') + + make_request('http://172.16.0.0') + + expect(request_stub).to have_been_requested + end + + it 'allows a localhost address request' do + request_stub = stub_request(http_method, 'http://127.0.0.1') + + make_request('http://127.0.0.1') + + expect(request_stub).to have_been_requested + end + end +end diff --git a/spec/views/projects/_home_panel.html.haml_spec.rb b/spec/views/projects/_home_panel.html.haml_spec.rb index 4d5b369e88e..9956144b601 100644 --- a/spec/views/projects/_home_panel.html.haml_spec.rb +++ b/spec/views/projects/_home_panel.html.haml_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' describe 'projects/_home_panel' do + include ProjectForksHelper + context 'notifications' do let(:project) { create(:project) } @@ -144,4 +146,36 @@ describe 'projects/_home_panel' do end end end + + context 'forks' do + let(:source_project) { create(:project, :repository) } + let(:project) { fork_project(source_project) } + let(:user) { create(:user) } + + before do + assign(:project, project) + + allow(view).to receive(:current_user).and_return(user) + end + + context 'user can read fork source' do + it 'shows the forked-from project' do + allow(view).to receive(:can?).with(user, :read_project, source_project).and_return(true) + + render + + expect(rendered).to have_content("Forked from #{source_project.full_name}") + end + end + + context 'user cannot read fork source' do + it 'does not show the forked-from project' do + allow(view).to receive(:can?).with(user, :read_project, source_project).and_return(false) + + render + + expect(rendered).to have_content("Forked from an inaccessible project") + end + end + end end diff --git a/spec/views/projects/edit.html.haml_spec.rb b/spec/views/projects/edit.html.haml_spec.rb index f576093ab45..40927a22dc4 100644 --- a/spec/views/projects/edit.html.haml_spec.rb +++ b/spec/views/projects/edit.html.haml_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' describe 'projects/edit' do include Devise::Test::ControllerHelpers + include ProjectForksHelper let(:project) { create(:project) } let(:user) { create(:admin) } @@ -26,4 +27,59 @@ describe 'projects/edit' do expect(rendered).not_to have_content('Export project') end end + + context 'forking' do + before do + assign(:project, project) + + allow(view).to receive(:current_user).and_return(user) + end + + context 'project is not a fork' do + it 'hides the remove fork relationship settings' do + render + + expect(rendered).not_to have_content('Remove fork relationship') + end + end + + context 'project is a fork' do + let(:source_project) { create(:project) } + let(:project) { fork_project(source_project) } + + it 'shows the remove fork relationship settings to an authorized user' do + allow(view).to receive(:can?).with(user, :remove_fork_project, project).and_return(true) + + render + + expect(rendered).to have_content('Remove fork relationship') + end + + it 'hides the fork relationship settings from an unauthorized user' do + allow(view).to receive(:can?).with(user, :remove_fork_project, project).and_return(false) + + render + + expect(rendered).not_to have_content('Remove fork relationship') + end + + it 'hides the fork source from an unauthorized user' do + allow(view).to receive(:can?).with(user, :read_project, source_project).and_return(false) + + render + + expect(rendered).to have_content('Remove fork relationship') + expect(rendered).not_to have_content(source_project.full_name) + end + + it 'shows the fork source to an authorized user' do + allow(view).to receive(:can?).with(user, :read_project, source_project).and_return(true) + + render + + expect(rendered).to have_content('Remove fork relationship') + expect(rendered).to have_content(source_project.full_name) + end + end + end end |