diff options
author | Rémy Coutable <remy@rymai.me> | 2019-08-05 11:55:16 +0300 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2019-08-05 11:55:16 +0300 |
commit | f74387d2982845b0fb5219ccf3fd128d75400472 (patch) | |
tree | 3cfbc60f1a62a1d308f1d1c98ad4a969721b873b | |
parent | e6a05e5a63b3b336179a4ed67df76ba7ad08fadb (diff) | |
parent | 260225eb5753a18e9e33ec2bb9c1bfc76d613448 (diff) |
Merge branch 'georgekoltsov/55474-outbound-setting-system-hooks' into 'master'
Add outbound setting for system hooks
See merge request gitlab-org/gitlab-ce!31177
36 files changed, 258 insertions, 75 deletions
diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 3847a35fbab..acbcf0ded17 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -160,6 +160,8 @@ module ApplicationSettingsHelper :akismet_api_key, :akismet_enabled, :allow_local_requests_from_hooks_and_services, + :allow_local_requests_from_web_hooks_and_services, + :allow_local_requests_from_system_hooks, :dns_rebinding_protection_enabled, :archive_builds_in_human_readable, :authorized_keys_enabled, diff --git a/app/models/application_setting_implementation.rb b/app/models/application_setting_implementation.rb index 4bb09bf3b53..b7a4d7aa803 100644 --- a/app/models/application_setting_implementation.rb +++ b/app/models/application_setting_implementation.rb @@ -21,7 +21,8 @@ module ApplicationSettingImplementation { after_sign_up_text: nil, akismet_enabled: false, - allow_local_requests_from_hooks_and_services: false, + allow_local_requests_from_web_hooks_and_services: false, + allow_local_requests_from_system_hooks: true, dns_rebinding_protection_enabled: true, authorized_keys_enabled: true, # TODO default to false if the instance is configured to use AuthorizedKeysCommand container_registry_token_expire_delay: 5, diff --git a/app/models/hooks/system_hook.rb b/app/models/hooks/system_hook.rb index 90b4588a325..3d54d17e787 100644 --- a/app/models/hooks/system_hook.rb +++ b/app/models/hooks/system_hook.rb @@ -14,8 +14,10 @@ class SystemHook < WebHook default_value_for :repository_update_events, true default_value_for :merge_requests_events, false + validates :url, system_hook_url: true + # Allow urls pointing localhost and the local network def allow_local_requests? - true + Gitlab::CurrentSettings.allow_local_requests_from_system_hooks? end end diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb index daf7ff4b771..16fc7fdbd48 100644 --- a/app/models/hooks/web_hook.rb +++ b/app/models/hooks/web_hook.rb @@ -15,8 +15,8 @@ class WebHook < ApplicationRecord has_many :web_hook_logs, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent - validates :url, presence: true, public_url: { allow_localhost: lambda(&:allow_local_requests?), - allow_local_network: lambda(&:allow_local_requests?) } + validates :url, presence: true + validates :url, public_url: true, unless: ->(hook) { hook.is_a?(SystemHook) } validates :token, format: { without: /\n/ } validates :push_events_branch_filter, branch_filter: true @@ -35,6 +35,6 @@ class WebHook < ApplicationRecord # Allow urls pointing localhost and the local network def allow_local_requests? - false + Gitlab::CurrentSettings.allow_local_requests_from_web_hooks_and_services? end end diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index 6d675c026bb..8c294218708 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -17,8 +17,10 @@ class WebHookService @hook = hook @data = data @hook_name = hook_name.to_s - @request_options = { timeout: Gitlab.config.gitlab.webhook_timeout } - @request_options.merge!(allow_local_requests: true) if @hook.is_a?(SystemHook) + @request_options = { + timeout: Gitlab.config.gitlab.webhook_timeout, + allow_local_requests: hook.allow_local_requests? + } end def execute diff --git a/app/validators/addressable_url_validator.rb b/app/validators/addressable_url_validator.rb index 273e15ef925..bb445499cee 100644 --- a/app/validators/addressable_url_validator.rb +++ b/app/validators/addressable_url_validator.rb @@ -107,6 +107,6 @@ class AddressableUrlValidator < ActiveModel::EachValidator # calls this validator. # # See https://gitlab.com/gitlab-org/gitlab-ee/issues/9833 - ApplicationSetting.current&.allow_local_requests_from_hooks_and_services? + ApplicationSetting.current&.allow_local_requests_from_web_hooks_and_services? end end diff --git a/app/validators/system_hook_url_validator.rb b/app/validators/system_hook_url_validator.rb new file mode 100644 index 00000000000..f4253006dad --- /dev/null +++ b/app/validators/system_hook_url_validator.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +# SystemHookUrlValidator +# +# Custom validator specific to SystemHook URLs. This validator works like AddressableUrlValidator but +# it blocks urls pointing to localhost or the local network depending on +# ApplicationSetting.allow_local_requests_from_system_hooks +# +# Example: +# class SystemHook < WebHook +# validates :url, system_hook_url: true +# end +# +class SystemHookUrlValidator < PublicUrlValidator + def self.allow_setting_local_requests? + ApplicationSetting.current&.allow_local_requests_from_system_hooks? + end +end diff --git a/app/views/admin/application_settings/_outbound.html.haml b/app/views/admin/application_settings/_outbound.html.haml index 4fecdb59e1d..ad26f52aea7 100644 --- a/app/views/admin/application_settings/_outbound.html.haml +++ b/app/views/admin/application_settings/_outbound.html.haml @@ -4,9 +4,13 @@ %fieldset .form-group .form-check - = f.check_box :allow_local_requests_from_hooks_and_services, class: 'form-check-input' - = f.label :allow_local_requests_from_hooks_and_services, class: 'form-check-label' do - Allow requests to the local network from hooks and services + = f.check_box :allow_local_requests_from_web_hooks_and_services, class: 'form-check-input' + = f.label :allow_local_requests_from_web_hooks_and_services, class: 'form-check-label' do + = _('Allow requests to the local network from web hooks and services') + .form-check + = f.check_box :allow_local_requests_from_system_hooks, class: 'form-check-input' + = f.label :allow_local_requests_from_system_hooks, class: 'form-check-label' do + = _('Allow requests to the local network from system hooks') .form-group = f.label :outbound_local_requests_whitelist_raw, class: 'label-bold' do diff --git a/changelogs/unreleased/georgekoltsov-55474-outbound-setting-system-hooks.yml b/changelogs/unreleased/georgekoltsov-55474-outbound-setting-system-hooks.yml new file mode 100644 index 00000000000..fb1acb1e9f5 --- /dev/null +++ b/changelogs/unreleased/georgekoltsov-55474-outbound-setting-system-hooks.yml @@ -0,0 +1,5 @@ +--- +title: Add new outbound network requests application setting for system hooks +merge_request: 31177 +author: +type: added diff --git a/db/migrate/20190726101050_rename_allow_local_requests_from_hooks_and_services_application_setting.rb b/db/migrate/20190726101050_rename_allow_local_requests_from_hooks_and_services_application_setting.rb new file mode 100644 index 00000000000..ac65e8d745c --- /dev/null +++ b/db/migrate/20190726101050_rename_allow_local_requests_from_hooks_and_services_application_setting.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class RenameAllowLocalRequestsFromHooksAndServicesApplicationSetting < ActiveRecord::Migration[5.2] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + rename_column_concurrently :application_settings, :allow_local_requests_from_hooks_and_services, :allow_local_requests_from_web_hooks_and_services + end + + def down + cleanup_concurrent_column_rename :application_settings, :allow_local_requests_from_web_hooks_and_services, :allow_local_requests_from_hooks_and_services + end +end diff --git a/db/migrate/20190726101133_add_allow_local_requests_from_system_hooks_to_application_settings.rb b/db/migrate/20190726101133_add_allow_local_requests_from_system_hooks_to_application_settings.rb new file mode 100644 index 00000000000..95d4f956f93 --- /dev/null +++ b/db/migrate/20190726101133_add_allow_local_requests_from_system_hooks_to_application_settings.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +class AddAllowLocalRequestsFromSystemHooksToApplicationSettings < ActiveRecord::Migration[5.2] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + add_column(:application_settings, :allow_local_requests_from_system_hooks, + :boolean, + default: true, + null: false) + end + + def down + remove_column(:application_settings, :allow_local_requests_from_system_hooks) + end +end diff --git a/db/post_migrate/20190801114109_cleanup_allow_local_requests_from_hooks_and_services_application_setting_rename.rb b/db/post_migrate/20190801114109_cleanup_allow_local_requests_from_hooks_and_services_application_setting_rename.rb new file mode 100644 index 00000000000..127e44254ac --- /dev/null +++ b/db/post_migrate/20190801114109_cleanup_allow_local_requests_from_hooks_and_services_application_setting_rename.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class CleanupAllowLocalRequestsFromHooksAndServicesApplicationSettingRename < ActiveRecord::Migration[5.2] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + cleanup_concurrent_column_rename :application_settings, :allow_local_requests_from_hooks_and_services, :allow_local_requests_from_web_hooks_and_services + end + + def down + rename_column_concurrently :application_settings, :allow_local_requests_from_web_hooks_and_services, :allow_local_requests_from_hooks_and_services + end +end diff --git a/db/schema.rb b/db/schema.rb index 709f9ce2541..fe3fa597006 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2019_07_31_084415) do +ActiveRecord::Schema.define(version: 2019_08_01_114109) do # These are extensions that must be enabled in order to support this database enable_extension "pg_trgm" @@ -183,7 +183,6 @@ ActiveRecord::Schema.define(version: 2019_07_31_084415) do t.string "external_authorization_service_default_label" t.boolean "pages_domain_verification_enabled", default: true, null: false t.string "user_default_internal_regex" - t.boolean "allow_local_requests_from_hooks_and_services", default: false, null: false t.float "external_authorization_service_timeout", default: 0.5 t.text "external_auth_client_cert" t.text "encrypted_external_auth_client_key" @@ -230,6 +229,8 @@ ActiveRecord::Schema.define(version: 2019_07_31_084415) do t.string "grafana_url", default: "/-/grafana", null: false t.string "outbound_local_requests_whitelist", limit: 255, default: [], null: false, array: true t.integer "raw_blob_request_limit", default: 300, null: false + t.boolean "allow_local_requests_from_web_hooks_and_services", default: false, null: false + t.boolean "allow_local_requests_from_system_hooks", default: true, null: false 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 ["usage_stats_set_by_user_id"], name: "index_application_settings_on_usage_stats_set_by_user_id" diff --git a/doc/api/settings.md b/doc/api/settings.md index c3ac70f0579..83125aff264 100644 --- a/doc/api/settings.md +++ b/doc/api/settings.md @@ -64,7 +64,10 @@ Example response: "performance_bar_allowed_group_id": 42, "instance_statistics_visibility_private": false, "user_show_add_ssh_key_message": true, - "local_markdown_version": 0 + "local_markdown_version": 0, + "allow_local_requests_from_hooks_and_services": true, + "allow_local_requests_from_web_hooks_and_services": true, + "allow_local_requests_from_system_hooks": false } ``` @@ -138,7 +141,10 @@ Example response: "user_show_add_ssh_key_message": true, "file_template_project_id": 1, "local_markdown_version": 0, - "geo_node_allowed_ips": "0.0.0.0/0, ::/0" + "geo_node_allowed_ips": "0.0.0.0/0, ::/0", + "allow_local_requests_from_hooks_and_services": true, + "allow_local_requests_from_web_hooks_and_services": true, + "allow_local_requests_from_system_hooks": false } ``` @@ -177,7 +183,9 @@ are listed in the descriptions of the relevant settings. | `akismet_api_key` | string | required by: `akismet_enabled` | API key for akismet spam protection. | | `akismet_enabled` | boolean | no | (**If enabled, requires:** `akismet_api_key`) Enable or disable akismet spam protection. | | `allow_group_owners_to_manage_ldap` | boolean | no | **(PREMIUM)** Set to `true` to allow group owners to manage LDAP | -| `allow_local_requests_from_hooks_and_services` | boolean | no | Allow requests to the local network from hooks and services. | +| `allow_local_requests_from_hooks_and_services` | boolean | no | (Deprecated: Use `allow_local_requests_from_web_hooks_and_services` instead) Allow requests to the local network from hooks and services. | +| `allow_local_requests_from_web_hooks_and_services` | boolean | no | Allow requests to the local network from web hooks and services. | +| `allow_local_requests_from_system_hooks` | boolean | no | Allow requests to the local network from system hooks. | | `authorized_keys_enabled` | boolean | no | By default, we write to the `authorized_keys` file to support Git over SSH without additional configuration. GitLab can be optimized to authenticate SSH keys via the database file. Only disable this if you have configured your OpenSSH server to use the AuthorizedKeysCommand. | | `auto_devops_domain` | string | no | Specify a domain to use by default for every project's Auto Review Apps and Auto Deploy stages. | | `auto_devops_enabled` | boolean | no | Enable Auto DevOps for projects by default. It will automatically build, test, and deploy applications based on a predefined CI/CD configuration. | diff --git a/doc/security/img/outbound_requests_section.png b/doc/security/img/outbound_requests_section.png Binary files differdeleted file mode 100644 index f7783f34cdd..00000000000 --- a/doc/security/img/outbound_requests_section.png +++ /dev/null diff --git a/doc/security/img/outbound_requests_section_v12_2.png b/doc/security/img/outbound_requests_section_v12_2.png Binary files differnew file mode 100644 index 00000000000..4fd3c7d9fce --- /dev/null +++ b/doc/security/img/outbound_requests_section_v12_2.png diff --git a/doc/security/webhooks.md b/doc/security/webhooks.md index 1194234a295..7ece9407ac0 100644 --- a/doc/security/webhooks.md +++ b/doc/security/webhooks.md @@ -34,15 +34,16 @@ to 127.0.0.1, ::1 and 0.0.0.0, as well as IPv4 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16 and IPv6 site-local (ffc0::/10) addresses won't be allowed. This behavior can be overridden by enabling the option *"Allow requests to the -local network from hooks and services"* in the *"Outbound requests"* section +local network from web hooks and services"* in the *"Outbound requests"* section inside the Admin area under **Settings** (`/admin/application_settings/network`): -![Outbound requests admin settings](img/outbound_requests_section.png) +![Outbound requests admin settings](img/outbound_requests_section_v12_2.png) ->**Note:** -*System hooks* are exempt from this protection because they are set up by -admins. +NOTE: **Note:** +*System hooks* are enabled to make requests to local network by default since they are +set up by administrators. However, you can turn this off by disabling the +**Allow requests to the local network from system hooks** option. <!-- ## Troubleshooting diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 2d6dd18d4ea..2f5ce3d4003 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -1162,6 +1162,7 @@ module API attributes = ::ApplicationSettingsHelper.visible_attributes attributes.delete(:performance_bar_allowed_group_path) attributes.delete(:performance_bar_enabled) + attributes.delete(:allow_local_requests_from_hooks_and_services) attributes end @@ -1180,6 +1181,7 @@ module API # support legacy names, can be removed in v5 expose :password_authentication_enabled_for_web, as: :password_authentication_enabled expose :password_authentication_enabled_for_web, as: :signin_enabled + expose :allow_local_requests_from_web_hooks_and_services, as: :allow_local_requests_from_hooks_and_services end # deprecated old Release representation diff --git a/lib/api/settings.rb b/lib/api/settings.rb index aa9e879160d..196ef1fcdfa 100644 --- a/lib/api/settings.rb +++ b/lib/api/settings.rb @@ -124,6 +124,7 @@ module API optional :usage_ping_enabled, type: Boolean, desc: 'Every week GitLab will report license usage back to GitLab, Inc.' optional :instance_statistics_visibility_private, type: Boolean, desc: 'When set to `true` Instance statistics will only be available to admins' optional :local_markdown_version, type: Integer, desc: "Local markdown version, increase this value when any cached markdown should be invalidated" + optional :allow_local_requests_from_hooks_and_services, type: Boolean, desc: 'Deprecated: Use :allow_local_requests_from_web_hooks_and_services instead. Allow requests to the local network from hooks and services.' # support legacy names, can be removed in v5 ApplicationSetting::SUPPORTED_KEY_TYPES.each do |type| optional :"#{type}_key_restriction", @@ -158,6 +159,11 @@ module API attrs[:password_authentication_enabled_for_web] = attrs.delete(:password_authentication_enabled) end + # support legacy names, can be removed in v5 + if attrs.has_key?(:allow_local_requests_from_hooks_and_services) + attrs[:allow_local_requests_from_web_hooks_and_services] = attrs.delete(:allow_local_requests_from_hooks_and_services) + end + attrs = filter_attributes_using_license(attrs) if ApplicationSettings::UpdateService.new(current_settings, current_user, attrs).execute diff --git a/lib/gitlab/http_connection_adapter.rb b/lib/gitlab/http_connection_adapter.rb index 41eab3658bc..84eb60f3a5d 100644 --- a/lib/gitlab/http_connection_adapter.rb +++ b/lib/gitlab/http_connection_adapter.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true # This class is part of the Gitlab::HTTP wrapper. Depending on the value -# of the global setting allow_local_requests_from_hooks_and_services this adapter +# of the global setting allow_local_requests_from_web_hooks_and_services this adapter # will allow/block connection to internal IPs and/or urls. # # This functionality can be overridden by providing the setting the option @@ -38,7 +38,7 @@ module Gitlab end def allow_settings_local_requests? - Gitlab::CurrentSettings.allow_local_requests_from_hooks_and_services? + Gitlab::CurrentSettings.allow_local_requests_from_web_hooks_and_services? end end end diff --git a/lib/gitlab/kubernetes/kube_client.rb b/lib/gitlab/kubernetes/kube_client.rb index 1350924cd76..64317225ec6 100644 --- a/lib/gitlab/kubernetes/kube_client.rb +++ b/lib/gitlab/kubernetes/kube_client.rb @@ -128,7 +128,7 @@ module Gitlab private def validate_url! - return if Gitlab::CurrentSettings.allow_local_requests_from_hooks_and_services? + return if Gitlab::CurrentSettings.allow_local_requests_from_web_hooks_and_services? Gitlab::UrlBlocker.validate!(api_prefix, allow_local_network: false) end diff --git a/lib/gitlab/octokit/middleware.rb b/lib/gitlab/octokit/middleware.rb index 2f762957d1b..2dd7d08a58b 100644 --- a/lib/gitlab/octokit/middleware.rb +++ b/lib/gitlab/octokit/middleware.rb @@ -16,7 +16,7 @@ module Gitlab private def allow_local_requests? - Gitlab::CurrentSettings.allow_local_requests_from_hooks_and_services? + Gitlab::CurrentSettings.allow_local_requests_from_web_hooks_and_services? end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 117625e717f..d462bb03adf 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -948,6 +948,12 @@ msgstr "" msgid "Allow requests to the local network from hooks and services." msgstr "" +msgid "Allow requests to the local network from system hooks" +msgstr "" + +msgid "Allow requests to the local network from web hooks and services" +msgstr "" + msgid "Allow this key to push to repository as well? (Default only allows pull access.)" msgstr "" diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb index c77605f3869..ddd87404003 100644 --- a/spec/features/admin/admin_settings_spec.rb +++ b/spec/features/admin/admin_settings_spec.rb @@ -338,14 +338,17 @@ describe 'Admin updates settings' do visit network_admin_application_settings_path page.within('.as-outbound') do - check 'Allow requests to the local network from hooks and services' + check 'Allow requests to the local network from web hooks and services' + # Enabled by default + uncheck 'Allow requests to the local network from system hooks' # Enabled by default uncheck 'Enforce DNS rebinding attack protection' click_button 'Save changes' end expect(page).to have_content "Application settings saved successfully" - expect(current_settings.allow_local_requests_from_hooks_and_services).to be true + expect(current_settings.allow_local_requests_from_web_hooks_and_services).to be true + expect(current_settings.allow_local_requests_from_system_hooks).to be false expect(current_settings.dns_rebinding_protection_enabled).to be false end end diff --git a/spec/lib/gitlab/http_spec.rb b/spec/lib/gitlab/http_spec.rb index 158f77cab2c..d3f9be845dd 100644 --- a/spec/lib/gitlab/http_spec.rb +++ b/spec/lib/gitlab/http_spec.rb @@ -23,14 +23,14 @@ describe Gitlab::HTTP do end end - describe 'allow_local_requests_from_hooks_and_services is' do + describe 'allow_local_requests_from_web_hooks_and_services is' do before do WebMock.stub_request(:get, /.*/).to_return(status: 200, body: 'Success') end context 'disabled' do before do - allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(false) + allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_web_hooks_and_services?).and_return(false) end it 'deny requests to localhost' do @@ -52,7 +52,7 @@ describe Gitlab::HTTP do context 'enabled' do before do - allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(true) + allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_web_hooks_and_services?).and_return(true) end it 'allow requests to localhost' do diff --git a/spec/lib/gitlab/kubernetes/kube_client_spec.rb b/spec/lib/gitlab/kubernetes/kube_client_spec.rb index 97ebb5f1554..f49d4e23e39 100644 --- a/spec/lib/gitlab/kubernetes/kube_client_spec.rb +++ b/spec/lib/gitlab/kubernetes/kube_client_spec.rb @@ -58,7 +58,7 @@ describe Gitlab::Kubernetes::KubeClient do context 'when local requests are allowed' do before do - stub_application_setting(allow_local_requests_from_hooks_and_services: true) + stub_application_setting(allow_local_requests_from_web_hooks_and_services: true) end it 'allows local addresses' do diff --git a/spec/lib/gitlab/octokit/middleware_spec.rb b/spec/lib/gitlab/octokit/middleware_spec.rb index 7f2b523f5b7..43f6d13f7ba 100644 --- a/spec/lib/gitlab/octokit/middleware_spec.rb +++ b/spec/lib/gitlab/octokit/middleware_spec.rb @@ -30,7 +30,7 @@ describe Gitlab::Octokit::Middleware do context 'when localhost requests are not allowed' do before do - stub_application_setting(allow_local_requests_from_hooks_and_services: false) + stub_application_setting(allow_local_requests_from_web_hooks_and_services: false) end it_behaves_like 'Local URL' @@ -38,7 +38,7 @@ describe Gitlab::Octokit::Middleware do context 'when localhost requests are allowed' do before do - stub_application_setting(allow_local_requests_from_hooks_and_services: true) + stub_application_setting(allow_local_requests_from_web_hooks_and_services: true) end it_behaves_like 'Public URL' @@ -50,7 +50,7 @@ describe Gitlab::Octokit::Middleware do context 'when local network requests are not allowed' do before do - stub_application_setting(allow_local_requests_from_hooks_and_services: false) + stub_application_setting(allow_local_requests_from_web_hooks_and_services: false) end it_behaves_like 'Local URL' @@ -58,7 +58,7 @@ describe Gitlab::Octokit::Middleware do context 'when local network requests are allowed' do before do - stub_application_setting(allow_local_requests_from_hooks_and_services: true) + stub_application_setting(allow_local_requests_from_web_hooks_and_services: true) end it_behaves_like 'Public URL' diff --git a/spec/models/clusters/platforms/kubernetes_spec.rb b/spec/models/clusters/platforms/kubernetes_spec.rb index 471769e4aab..5811016ea4d 100644 --- a/spec/models/clusters/platforms/kubernetes_spec.rb +++ b/spec/models/clusters/platforms/kubernetes_spec.rb @@ -106,7 +106,7 @@ describe Clusters::Platforms::Kubernetes do before do allow(ApplicationSetting) .to receive(:current) - .and_return(ApplicationSetting.build_from_defaults(allow_local_requests_from_hooks_and_services: true)) + .and_return(ApplicationSetting.build_from_defaults(allow_local_requests_from_web_hooks_and_services: true)) end it { expect(kubernetes.save).to be_truthy } diff --git a/spec/models/lfs_download_object_spec.rb b/spec/models/lfs_download_object_spec.rb index effd8b08124..8b53effe98f 100644 --- a/spec/models/lfs_download_object_spec.rb +++ b/spec/models/lfs_download_object_spec.rb @@ -50,7 +50,7 @@ describe LfsDownloadObject do before do allow(ApplicationSetting) .to receive(:current) - .and_return(ApplicationSetting.build_from_defaults(allow_local_requests_from_hooks_and_services: setting)) + .and_return(ApplicationSetting.build_from_defaults(allow_local_requests_from_web_hooks_and_services: setting)) end context 'are allowed' do diff --git a/spec/requests/api/settings_spec.rb b/spec/requests/api/settings_spec.rb index 8a60980fe80..184c00a356a 100644 --- a/spec/requests/api/settings_spec.rb +++ b/spec/requests/api/settings_spec.rb @@ -25,6 +25,9 @@ describe API::Settings, 'Settings' do expect(json_response['ed25519_key_restriction']).to eq(0) expect(json_response['performance_bar_allowed_group_id']).to be_nil expect(json_response['instance_statistics_visibility_private']).to be(false) + expect(json_response['allow_local_requests_from_hooks_and_services']).to be(false) + expect(json_response['allow_local_requests_from_web_hooks_and_services']).to be(false) + expect(json_response['allow_local_requests_from_system_hooks']).to be(true) expect(json_response).not_to have_key('performance_bar_allowed_group_path') expect(json_response).not_to have_key('performance_bar_enabled') end @@ -67,7 +70,9 @@ describe API::Settings, 'Settings' do instance_statistics_visibility_private: true, diff_max_patch_bytes: 150_000, default_branch_protection: ::Gitlab::Access::PROTECTION_DEV_CAN_MERGE, - local_markdown_version: 3 + local_markdown_version: 3, + allow_local_requests_from_web_hooks_and_services: true, + allow_local_requests_from_system_hooks: false } expect(response).to have_gitlab_http_status(200) @@ -95,6 +100,8 @@ describe API::Settings, 'Settings' do expect(json_response['diff_max_patch_bytes']).to eq(150_000) expect(json_response['default_branch_protection']).to eq(Gitlab::Access::PROTECTION_DEV_CAN_MERGE) expect(json_response['local_markdown_version']).to eq(3) + expect(json_response['allow_local_requests_from_web_hooks_and_services']).to eq(true) + expect(json_response['allow_local_requests_from_system_hooks']).to eq(false) end end @@ -117,6 +124,14 @@ describe API::Settings, 'Settings' do expect(json_response['performance_bar_allowed_group_id']).to be_nil end + it 'supports legacy allow_local_requests_from_hooks_and_services' do + put api("/application/settings", admin), + params: { allow_local_requests_from_hooks_and_services: true } + + expect(response).to have_gitlab_http_status(200) + expect(json_response['allow_local_requests_from_hooks_and_services']).to eq(true) + end + context 'external policy classification settings' do let(:settings) do { diff --git a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb index 75d534c59bf..970e82e7107 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb @@ -17,7 +17,7 @@ describe Projects::LfsPointers::LfsDownloadService do before do ApplicationSetting.create_from_defaults - stub_application_setting(allow_local_requests_from_hooks_and_services: local_request_setting) + stub_application_setting(allow_local_requests_from_web_hooks_and_services: local_request_setting) allow(project).to receive(:lfs_enabled?).and_return(true) end diff --git a/spec/services/self_monitoring/project/create_service_spec.rb b/spec/services/self_monitoring/project/create_service_spec.rb index a1e7aaf45f2..7d4faba526b 100644 --- a/spec/services/self_monitoring/project/create_service_spec.rb +++ b/spec/services/self_monitoring/project/create_service_spec.rb @@ -37,7 +37,7 @@ describe SelfMonitoring::Project::CreateService do allow(ApplicationSetting) .to receive(:current) .and_return( - ApplicationSetting.build_from_defaults(allow_local_requests_from_hooks_and_services: true) + ApplicationSetting.build_from_defaults(allow_local_requests_from_web_hooks_and_services: true) ) end @@ -95,7 +95,7 @@ describe SelfMonitoring::Project::CreateService do allow(ApplicationSetting) .to receive(:current) .and_return( - ApplicationSetting.build_from_defaults(allow_local_requests_from_hooks_and_services: false) + ApplicationSetting.build_from_defaults(allow_local_requests_from_web_hooks_and_services: false) ) end diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 37bafc0c002..50167a2e059 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -19,17 +19,37 @@ describe WebHookService do let(:service_instance) { described_class.new(project_hook, data, :push_hooks) } describe '#initialize' do - it 'allow_local_requests is true if hook is a SystemHook' do - instance = described_class.new(build(:system_hook), data, :system_hook) - expect(instance.request_options[:allow_local_requests]).to be_truthy + before do + stub_application_setting(setting_name => setting) end - it 'allow_local_requests is false if hook is not a SystemHook' do - %i(project_hook service_hook web_hook_log).each do |hook| - instance = described_class.new(build(hook), data, hook) - expect(instance.request_options[:allow_local_requests]).to be_falsey + shared_examples_for 'respects outbound network setting' do + context 'when local requests are allowed' do + let(:setting) { true } + + it { expect(hook.request_options[:allow_local_requests]).to be_truthy } + end + + context 'when local requests are not allowed' do + let(:setting) { false } + + it { expect(hook.request_options[:allow_local_requests]).to be_falsey } end end + + context 'when SystemHook' do + let(:setting_name) { :allow_local_requests_from_system_hooks } + let(:hook) { described_class.new(build(:system_hook), data, :system_hook) } + + include_examples 'respects outbound network setting' + end + + context 'when ProjectHook' do + let(:setting_name) { :allow_local_requests_from_web_hooks_and_services } + let(:hook) { described_class.new(build(:project_hook), data, :project_hook) } + + include_examples 'respects outbound network setting' + end end describe '#execute' do diff --git a/spec/support/shared_examples/url_validator_examples.rb b/spec/support/shared_examples/url_validator_examples.rb index 16fceddb605..c5a775fefb6 100644 --- a/spec/support/shared_examples/url_validator_examples.rb +++ b/spec/support/shared_examples/url_validator_examples.rb @@ -1,12 +1,12 @@ # frozen_string_literal: true RSpec.shared_examples 'url validator examples' do |schemes| - let(:validator) { described_class.new(attributes: [:link_url], **options) } - let!(:badge) { build(:badge, link_url: 'http://www.example.com') } + describe '#validate' do + let(:validator) { described_class.new(attributes: [:link_url], **options) } + let(:badge) { build(:badge, link_url: 'http://www.example.com') } - subject { validator.validate(badge) } + subject { validator.validate(badge) } - describe '#validate' do context 'with no options' do let(:options) { {} } @@ -42,3 +42,52 @@ RSpec.shared_examples 'url validator examples' do |schemes| end end end + +RSpec.shared_examples 'public url validator examples' do |setting| + let(:validator) { described_class.new(attributes: [:link_url]) } + let(:badge) { build(:badge, link_url: 'http://www.example.com') } + + subject { validator.validate(badge) } + + context 'by default' do + it 'blocks urls pointing to localhost' do + badge.link_url = 'https://127.0.0.1' + + subject + + expect(badge.errors).to be_present + end + + it 'blocks urls pointing to the local network' do + badge.link_url = 'https://192.168.1.1' + + subject + + expect(badge.errors).to be_present + end + end + + context 'when local requests are allowed' do + let!(:settings) { create(:application_setting) } + + before do + stub_application_setting(setting) + end + + it 'does not block urls pointing to localhost' do + badge.link_url = 'https://127.0.0.1' + + subject + + expect(badge.errors).not_to be_present + end + + it 'does not block urls pointing to the local network' do + badge.link_url = 'https://192.168.1.1' + + subject + + expect(badge.errors).not_to be_present + end + end +end diff --git a/spec/validators/public_url_validator_spec.rb b/spec/validators/public_url_validator_spec.rb index f6364fb1dd5..3cbf1002730 100644 --- a/spec/validators/public_url_validator_spec.rb +++ b/spec/validators/public_url_validator_spec.rb @@ -2,27 +2,5 @@ require 'spec_helper' describe PublicUrlValidator do include_examples 'url validator examples', AddressableUrlValidator::DEFAULT_OPTIONS[:schemes] - - context 'by default' do - let(:validator) { described_class.new(attributes: [:link_url]) } - let!(:badge) { build(:badge, link_url: 'http://www.example.com') } - - subject { validator.validate(badge) } - - it 'blocks urls pointing to localhost' do - badge.link_url = 'https://127.0.0.1' - - subject - - expect(badge.errors).to be_present - end - - it 'blocks urls pointing to the local network' do - badge.link_url = 'https://192.168.1.1' - - subject - - expect(badge.errors).to be_present - end - end + include_examples 'public url validator examples', allow_local_requests_from_web_hooks_and_services: true end diff --git a/spec/validators/system_hook_url_validator_spec.rb b/spec/validators/system_hook_url_validator_spec.rb new file mode 100644 index 00000000000..02384bbd1ce --- /dev/null +++ b/spec/validators/system_hook_url_validator_spec.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe SystemHookUrlValidator do + include_examples 'url validator examples', AddressableUrlValidator::DEFAULT_OPTIONS[:schemes] + include_examples 'public url validator examples', allow_local_requests_from_system_hooks: true +end |