Merge branch '295621-rename-asset-proxy-whitelist-setting' into 'master'

Rename asset_proxy_whitelist column

See merge request gitlab-org/gitlab!50824

app/helpers/application_settings_helper.rb

@@ -181,7 +181,7 @@ module ApplicationSettingsHelper
       :asset_proxy_enabled,
       :asset_proxy_secret_key,
       :asset_proxy_url,
-      :asset_proxy_whitelist,
+      :asset_proxy_allowlist,
       :static_objects_external_storage_auth_token,
       :static_objects_external_storage_url,
       :authorized_keys_enabled,
@@ -355,9 +355,11 @@ module ApplicationSettingsHelper
     ]
   end
 
+  # ok to remove in REST API v5
   def deprecated_attributes
     [
-      :admin_notification_email # ok to remove in REST API v5
+      :admin_notification_email,
+      :asset_proxy_whitelist
     ]
   end
 

app/models/application_setting.rb

@@ -43,7 +43,7 @@ class ApplicationSetting < ApplicationRecord
   serialize :domain_allowlist, Array # rubocop:disable Cop/ActiveRecordSerialize
   serialize :domain_denylist, Array # rubocop:disable Cop/ActiveRecordSerialize
   serialize :repository_storages # rubocop:disable Cop/ActiveRecordSerialize
-  serialize :asset_proxy_whitelist, Array # rubocop:disable Cop/ActiveRecordSerialize
+  serialize :asset_proxy_allowlist, Array # rubocop:disable Cop/ActiveRecordSerialize
 
   cache_markdown_field :sign_in_text
   cache_markdown_field :help_page_text

app/models/application_setting_implementation.rb

@@ -269,13 +269,13 @@ module ApplicationSettingImplementation
     self.protected_paths = strings_to_array(values)
   end
 
-  def asset_proxy_whitelist=(values)
+  def asset_proxy_allowlist=(values)
     values = strings_to_array(values) if values.is_a?(String)
 
-    # make sure we always whitelist the running host
+    # make sure we always allow the running host
     values << Gitlab.config.gitlab.host unless values.include?(Gitlab.config.gitlab.host)
 
-    self[:asset_proxy_whitelist] = values
+    self[:asset_proxy_allowlist] = values
   end
 
   def repository_storages

app/services/application_settings/update_service.rb

@@ -6,7 +6,7 @@ module ApplicationSettings
 
     attr_reader :params, :application_setting
 
-    MARKDOWN_CACHE_INVALIDATING_PARAMS = %w(asset_proxy_enabled asset_proxy_url asset_proxy_secret_key asset_proxy_whitelist).freeze
+    MARKDOWN_CACHE_INVALIDATING_PARAMS = %w(asset_proxy_enabled asset_proxy_url asset_proxy_secret_key asset_proxy_allowlist).freeze
 
     def execute
       result = update_settings

changelogs/unreleased/295621-rename-asset-proxy-whitelist-setting.yml

+---
+title: Rename asset_proxy_whitelist column on application_settings
+merge_request: 50824
+author:
+type: changed

db/migrate/20210105052034_rename_asset_proxy_whitelist_on_application_settings.rb

+# frozen_string_literal: true
+
+class RenameAssetProxyWhitelistOnApplicationSettings < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers::V2
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    rename_column_concurrently :application_settings,
+      :asset_proxy_whitelist,
+      :asset_proxy_allowlist
+  end
+
+  def down
+    undo_rename_column_concurrently :application_settings,
+      :asset_proxy_whitelist,
+      :asset_proxy_allowlist
+  end
+end

db/post_migrate/20210105052229_clean_up_asset_proxy_whitelist_rename_on_application_settings.rb

+# frozen_string_literal: true
+
+class CleanUpAssetProxyWhitelistRenameOnApplicationSettings < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers::V2
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    cleanup_concurrent_column_rename :application_settings,
+      :asset_proxy_whitelist,
+      :asset_proxy_allowlist
+  end
+
+  def down
+    undo_cleanup_concurrent_column_rename :application_settings,
+      :asset_proxy_whitelist,
+      :asset_proxy_allowlist
+  end
+end

db/schema_migrations/20210105052034

+4eef64fb237f783cdb07e012356d48a4ec9afc349721de1c53cf3def95e83858
\ No newline at end of file

db/schema_migrations/20210105052229

+ef994f0c65154825906fb0952b9b3073f4cb0692f01c90280edf06a4ea2ec339
\ No newline at end of file

db/structure.sql

@@ -9293,7 +9293,6 @@ CREATE TABLE application_settings (
     instance_administration_project_id bigint,
     asset_proxy_enabled boolean DEFAULT false NOT NULL,
     asset_proxy_url character varying,
-    asset_proxy_whitelist text,
     encrypted_asset_proxy_secret_key text,
     encrypted_asset_proxy_secret_key_iv character varying,
     static_objects_external_storage_url character varying(255),
@@ -9412,6 +9411,7 @@ CREATE TABLE application_settings (
     container_registry_cleanup_tags_service_max_list_size integer DEFAULT 200 NOT NULL,
     enforce_ssh_key_expiration boolean DEFAULT false NOT NULL,
     git_two_factor_session_expiry integer DEFAULT 15 NOT NULL,
+    asset_proxy_allowlist text,
     CONSTRAINT app_settings_container_reg_cleanup_tags_max_list_size_positive CHECK ((container_registry_cleanup_tags_service_max_list_size >= 0)),
     CONSTRAINT app_settings_registry_exp_policies_worker_capacity_positive CHECK ((container_registry_expiration_policies_worker_capacity >= 0)),
     CONSTRAINT check_17d9558205 CHECK ((char_length((kroki_url)::text) <= 1024)),

doc/api/settings.md

@@ -77,6 +77,7 @@ Example response:
   "asset_proxy_enabled": true,
   "asset_proxy_url": "https://assets.example.com",
   "asset_proxy_whitelist": ["example.com", "*.example.com", "your-instance.com"],
+  "asset_proxy_allowlist": ["example.com", "*.example.com", "your-instance.com"],
   "npm_package_requests_forwarding": true,
   "snippet_size_limit": 52428800,
   "issues_create_limit": 300,
@@ -166,7 +167,7 @@ Example response:
   "local_markdown_version": 0,
   "asset_proxy_enabled": true,
   "asset_proxy_url": "https://assets.example.com",
-  "asset_proxy_whitelist": ["example.com", "*.example.com", "your-instance.com"],
+  "asset_proxy_allowlist": ["example.com", "*.example.com", "your-instance.com"],
   "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,
@@ -219,7 +220,8 @@ listed in the descriptions of the relevant settings.
 | `asset_proxy_enabled`                    | boolean          | no                                   | (**If enabled, requires:** `asset_proxy_url`) Enable proxying of assets. GitLab restart is required to apply changes. |
 | `asset_proxy_secret_key`                 | string           | no                                   | Shared secret with the asset proxy server. GitLab restart is required to apply changes. |
 | `asset_proxy_url`                        | string           | no                                   | URL of the asset proxy server. GitLab restart is required to apply changes. |
-| `asset_proxy_whitelist`                  | string or array of strings | no                         | Assets that match these domain(s) are **not** proxied. Wildcards allowed. Your GitLab installation URL is automatically allowlisted. GitLab restart is required to apply changes. |
+| `asset_proxy_whitelist`                  | string or array of strings | no                         | (Deprecated: Use `asset_proxy_allowlist` instead) Assets that match these domain(s) are **not** proxied. Wildcards allowed. Your GitLab installation URL is automatically allowlisted. GitLab restart is required to apply changes. |
+| `asset_proxy_allowlist`                  | string or array of strings | no                         | Assets that match these domain(s) are **not** proxied. Wildcards allowed. Your GitLab installation URL is automatically allowlisted. GitLab restart is required to apply changes. |
 | `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 automatically builds, tests, and deploys applications based on a predefined CI/CD configuration. |

doc/security/asset_proxy.md

@@ -51,7 +51,8 @@ To install a Camo server as an asset proxy:
    | `asset_proxy_enabled`    | Enable proxying of assets. If enabled, requires: `asset_proxy_url`).                                                                 |
    | `asset_proxy_secret_key` | Shared secret with the asset proxy server.                                                                                           |
    | `asset_proxy_url`        | URL of the asset proxy server.                                                                                                       |
-   | `asset_proxy_whitelist`  | Assets that match these domain(s) are NOT proxied. Wildcards allowed. Your GitLab installation URL is automatically whitelisted. |
+   | `asset_proxy_whitelist`  | (Deprecated: Use `asset_proxy_allowlist` instead) Assets that match these domain(s) are NOT proxied. Wildcards allowed. Your GitLab installation URL is automatically allowed.         |
+   | `asset_proxy_allowlist`  | Assets that match these domain(s) are NOT proxied. Wildcards allowed. Your GitLab installation URL is automatically allowed.         |
 
 1. Restart the server for the changes to take effect. Each time you change any values for the asset
    proxy, you need to restart the server.

lib/api/entities/application_setting.rb

@@ -31,6 +31,7 @@ module API
       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
+      expose :asset_proxy_allowlist, as: :asset_proxy_whitelist
     end
   end
 end

lib/api/settings.rb

@@ -42,7 +42,8 @@ module API
       optional :asset_proxy_enabled, type: Boolean, desc: 'Enable proxying of assets'
       optional :asset_proxy_url, type: String, desc: 'URL of the asset proxy server'
       optional :asset_proxy_secret_key, type: String, desc: 'Shared secret with the asset proxy server'
-      optional :asset_proxy_whitelist, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Assets that match these domain(s) will NOT be proxied. Wildcards allowed. Your GitLab installation URL is automatically whitelisted.'
+      optional :asset_proxy_whitelist, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Deprecated: Use :asset_proxy_allowlist instead. Assets that match these domain(s) will NOT be proxied. Wildcards allowed. Your GitLab installation URL is automatically whitelisted.'
+      optional :asset_proxy_allowlist, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Assets that match these domain(s) will NOT be proxied. Wildcards allowed. Your GitLab installation URL is automatically allowed.'
       optional :container_registry_token_expire_delay, type: Integer, desc: 'Authorization token duration (minutes)'
       optional :default_artifacts_expire_in, type: String, desc: "Set the default expiration time for each job's artifacts"
       optional :default_ci_config_path, type: String, desc: 'The instance default CI configuration path for new projects'
@@ -211,6 +212,11 @@ module API
         attrs[:abuse_notification_email] = attrs.delete(:admin_notification_email)
       end
 
+      # support legacy names, can be removed in v5
+      if attrs.has_key?(:asset_proxy_whitelist)
+        attrs[:asset_proxy_allowlist] = attrs.delete(:asset_proxy_whitelist)
+      end
+
       # since 13.0 it's not possible to disable hashed storage - support can be removed in 14.0
       attrs.delete(:hashed_storage_enabled) if attrs.has_key?(:hashed_storage_enabled)
 

lib/banzai/filter/asset_proxy_filter.rb

@@ -59,7 +59,9 @@ module Banzai
       end
 
       def self.determine_allowlist(application_settings)
-        application_settings.asset_proxy_whitelist.presence || [Gitlab.config.gitlab.host]
+        application_settings.try(:asset_proxy_allowlist).presence ||
+          application_settings.try(:asset_proxy_whitelist).presence ||
+          [Gitlab.config.gitlab.host]
       end
     end
   end

spec/lib/banzai/filter/asset_proxy_filter_spec.rb

@@ -28,7 +28,7 @@ RSpec.describe Banzai::Filter::AssetProxyFilter do
       stub_application_setting(asset_proxy_enabled: true)
       stub_application_setting(asset_proxy_secret_key: 'shared-secret')
       stub_application_setting(asset_proxy_url: 'https://assets.example.com')
-      stub_application_setting(asset_proxy_whitelist: %w(gitlab.com *.mydomain.com))
+      stub_application_setting(asset_proxy_allowlist: %w(gitlab.com *.mydomain.com))
 
       described_class.initialize_settings
 
@@ -39,16 +39,26 @@ RSpec.describe Banzai::Filter::AssetProxyFilter do
       expect(Gitlab.config.asset_proxy.domain_regexp).to eq(/^(gitlab\.com|.*?\.mydomain\.com)$/i)
     end
 
-    context 'when whitelist is empty' do
+    context 'when allowlist is empty' do
       it 'defaults to the install domain' do
         stub_application_setting(asset_proxy_enabled: true)
-        stub_application_setting(asset_proxy_whitelist: [])
+        stub_application_setting(asset_proxy_allowlist: [])
 
         described_class.initialize_settings
 
         expect(Gitlab.config.asset_proxy.allowlist).to eq [Gitlab.config.gitlab.host]
       end
     end
+
+    it 'supports deprecated whitelist settings' do
+      stub_application_setting(asset_proxy_enabled: true)
+      stub_application_setting(asset_proxy_whitelist: %w(foo.com bar.com))
+      stub_application_setting(asset_proxy_allowlist: [])
+
+      described_class.initialize_settings
+
+      expect(Gitlab.config.asset_proxy.allowlist).to eq %w(foo.com bar.com)
+    end
   end
 
   context 'when properly configured' do

spec/models/application_setting_spec.rb

@@ -635,28 +635,28 @@ RSpec.describe ApplicationSetting do
         end
       end
 
-      describe '#asset_proxy_whitelist' do
+      describe '#asset_proxy_allowlist' do
         context 'when given an Array' do
           it 'sets the domains and adds current running host' do
-            setting.asset_proxy_whitelist = ['example.com', 'assets.example.com']
-            expect(setting.asset_proxy_whitelist).to eq(['example.com', 'assets.example.com', 'localhost'])
+            setting.asset_proxy_allowlist = ['example.com', 'assets.example.com']
+            expect(setting.asset_proxy_allowlist).to eq(['example.com', 'assets.example.com', 'localhost'])
           end
         end
 
         context 'when given a String' do
           it 'sets multiple domains with spaces' do
-            setting.asset_proxy_whitelist = 'example.com *.example.com'
-            expect(setting.asset_proxy_whitelist).to eq(['example.com', '*.example.com', 'localhost'])
+            setting.asset_proxy_allowlist = 'example.com *.example.com'
+            expect(setting.asset_proxy_allowlist).to eq(['example.com', '*.example.com', 'localhost'])
           end
 
           it 'sets multiple domains with newlines and a space' do
-            setting.asset_proxy_whitelist = "example.com\n *.example.com"
-            expect(setting.asset_proxy_whitelist).to eq(['example.com', '*.example.com', 'localhost'])
+            setting.asset_proxy_allowlist = "example.com\n *.example.com"
+            expect(setting.asset_proxy_allowlist).to eq(['example.com', '*.example.com', 'localhost'])
           end
 
           it 'sets multiple domains with commas' do
-            setting.asset_proxy_whitelist = "example.com, *.example.com"
-            expect(setting.asset_proxy_whitelist).to eq(['example.com', '*.example.com', 'localhost'])
+            setting.asset_proxy_allowlist = "example.com, *.example.com"
+            expect(setting.asset_proxy_allowlist).to eq(['example.com', '*.example.com', 'localhost'])
           end
         end
       end

spec/requests/api/settings_spec.rb

@@ -199,6 +199,14 @@ RSpec.describe API::Settings, 'Settings' do
       expect(json_response['allow_local_requests_from_hooks_and_services']).to eq(true)
     end
 
+    it 'supports legacy asset_proxy_whitelist' do
+      put api("/application/settings", admin),
+        params: { asset_proxy_whitelist: ['example.com', '*.example.com'] }
+
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(json_response['asset_proxy_allowlist']).to eq(['example.com', '*.example.com', 'localhost'])
+    end
+
     it 'disables ability to switch to legacy storage' do
       put api("/application/settings", admin),
           params: { hashed_storage_enabled: false }
@@ -362,24 +370,24 @@ RSpec.describe API::Settings, 'Settings' do
             asset_proxy_enabled: true,
             asset_proxy_url: 'http://assets.example.com',
             asset_proxy_secret_key: 'shared secret',
-            asset_proxy_whitelist: ['example.com', '*.example.com']
+            asset_proxy_allowlist: ['example.com', '*.example.com']
           }
 
         expect(response).to have_gitlab_http_status(:ok)
         expect(json_response['asset_proxy_enabled']).to be(true)
         expect(json_response['asset_proxy_url']).to eq('http://assets.example.com')
         expect(json_response['asset_proxy_secret_key']).to be_nil
-        expect(json_response['asset_proxy_whitelist']).to eq(['example.com', '*.example.com', 'localhost'])
+        expect(json_response['asset_proxy_allowlist']).to eq(['example.com', '*.example.com', 'localhost'])
       end
 
-      it 'allows a string for asset_proxy_whitelist' do
+      it 'allows a string for asset_proxy_allowlist' do
         put api('/application/settings', admin),
           params: {
-            asset_proxy_whitelist: 'example.com, *.example.com'
+            asset_proxy_allowlist: 'example.com, *.example.com'
           }
 
         expect(response).to have_gitlab_http_status(:ok)
-        expect(json_response['asset_proxy_whitelist']).to eq(['example.com', '*.example.com', 'localhost'])
+        expect(json_response['asset_proxy_allowlist']).to eq(['example.com', '*.example.com', 'localhost'])
       end
     end
 

spec/services/application_settings/update_service_spec.rb

@@ -122,7 +122,7 @@ RSpec.describe ApplicationSettings::UpdateService do
     it_behaves_like 'invalidates markdown cache', { asset_proxy_enabled: true }
     it_behaves_like 'invalidates markdown cache', { asset_proxy_url: 'http://test.com' }
     it_behaves_like 'invalidates markdown cache', { asset_proxy_secret_key: 'another secret' }
-    it_behaves_like 'invalidates markdown cache', { asset_proxy_whitelist: ['domain.com'] }
+    it_behaves_like 'invalidates markdown cache', { asset_proxy_allowlist: ['domain.com'] }
 
     context 'when also setting the local_markdown_version' do
       let(:params) { { asset_proxy_enabled: true, local_markdown_version: 12 } }