From e5693c56c6b96b6605cdc55e97dc06b7f7959af1 Mon Sep 17 00:00:00 2001 From: Jonathan Hefner Date: Tue, 8 Nov 2022 15:25:57 -0600 Subject: [PATCH] Add AS::ParameterFilter.precompile_filters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ActiveSupport::ParameterFilter.precompile_filters` precompiles filters that otherwise would be passed directly to `ParameterFilter.new`. Depending on the quantity and types of filters, precompilation can improve filtering performance, especially in the case where the `ParameterFilter` instance cannot be retained, such as with per-request instances in `ActionDispatch::Http::FilterParameters`. **Benchmark script** ```ruby # frozen_string_literal: true require "benchmark/ips" require "benchmark/memory" require "active_support" require "active_support/parameter_filter" ootb = [:passw, :secret, :token, :_key, :crypt, :salt, :certificate, :otp, :ssn] mixed = [:passw, "secret", /token/, :crypt, "salt", /certificate/, "user.otp", /user\.ssn/, proc {}] precompiled_ootb = ActiveSupport::ParameterFilter.precompile_filters(ootb) precompiled_mixed = ActiveSupport::ParameterFilter.precompile_filters(mixed) params = { "user" => { "name" => :name, "email" => :email, "password" => :password, "ssn" => :ssn, "locations" => [ { "city" => :city, "country" => :country }, { "city" => :city, "country" => :country }, ], } } Benchmark.ips do |x| x.report("ootb") do ActiveSupport::ParameterFilter.new(ootb).filter(params) end x.report("precompiled ootb") do ActiveSupport::ParameterFilter.new(precompiled_ootb).filter(params) end x.compare! end Benchmark.ips do |x| x.report("mixed") do ActiveSupport::ParameterFilter.new(mixed).filter(params) end x.report("precompiled mixed") do ActiveSupport::ParameterFilter.new(precompiled_mixed).filter(params) end x.compare! end Benchmark.memory do |x| x.report("ootb") do ActiveSupport::ParameterFilter.new(ootb).filter(params) end x.report("precompiled ootb") do ActiveSupport::ParameterFilter.new(precompiled_ootb).filter(params) end end Benchmark.memory do |x| x.report("mixed") do ActiveSupport::ParameterFilter.new(mixed).filter(params) end x.report("precompiled mixed") do ActiveSupport::ParameterFilter.new(precompiled_mixed).filter(params) end end ``` **Results** ``` Warming up -------------------------------------- ootb 2.151k i/100ms precompiled ootb 4.251k i/100ms Calculating ------------------------------------- ootb 21.567k (± 1.1%) i/s - 109.701k in 5.086983s precompiled ootb 42.840k (± 0.8%) i/s - 216.801k in 5.061022s Comparison: precompiled ootb: 42840.4 i/s ootb: 21567.5 i/s - 1.99x (± 0.00) slower ``` ``` Warming up -------------------------------------- mixed 1.622k i/100ms precompiled mixed 2.455k i/100ms Calculating ------------------------------------- mixed 16.085k (± 1.3%) i/s - 81.100k in 5.042764s precompiled mixed 24.640k (± 1.0%) i/s - 125.205k in 5.081988s Comparison: precompiled mixed: 24639.6 i/s mixed: 16085.0 i/s - 1.53x (± 0.00) slower ``` ``` Calculating ------------------------------------- ootb 2.684k memsize ( 0.000 retained) 30.000 objects ( 0.000 retained) 10.000 strings ( 0.000 retained) precompiled ootb 1.104k memsize ( 0.000 retained) 9.000 objects ( 0.000 retained) 1.000 strings ( 0.000 retained) ``` ``` Calculating ------------------------------------- mixed 3.541k memsize ( 0.000 retained) 46.000 objects ( 0.000 retained) 20.000 strings ( 0.000 retained) precompiled mixed 1.856k memsize ( 0.000 retained) 29.000 objects ( 0.000 retained) 13.000 strings ( 0.000 retained) ``` This commit also adds `config.precompile_filter_parameters`, which enables precompilation of `config.filter_parameters`. It defaults to `true` for `config.load_defaults 7.1` and above. --- .../lib/active_support/parameter_filter.rb | 29 +++++++++++++ activesupport/test/parameter_filter_test.rb | 24 +++++++++++ guides/source/configuring.md | 15 +++++++ railties/CHANGELOG.md | 10 +++++ railties/lib/rails/application.rb | 10 ++++- .../lib/rails/application/configuration.rb | 3 +- .../new_framework_defaults_7_1.rb.tt | 4 ++ .../test/application/configuration_test.rb | 41 +++++++++++++++++++ 8 files changed, 134 insertions(+), 2 deletions(-) diff --git a/activesupport/lib/active_support/parameter_filter.rb b/activesupport/lib/active_support/parameter_filter.rb index 5d457e04b4..4ded025ff0 100644 --- a/activesupport/lib/active_support/parameter_filter.rb +++ b/activesupport/lib/active_support/parameter_filter.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "active_support/core_ext/object/duplicable" +require "active_support/core_ext/array/extract" module ActiveSupport # +ParameterFilter+ allows you to specify keys for sensitive data from @@ -32,6 +33,34 @@ module ActiveSupport class ParameterFilter FILTERED = "[FILTERED]" # :nodoc: + # Precompiles an array of filters that otherwise would be passed directly to + # #initialize. Depending on the quantity and types of filters, + # precompilation can improve filtering performance, especially in the case + # where the ParameterFilter instance itself cannot be retained (but the + # precompiled filters can be retained). + # + # filters = [/foo/, :bar, "nested.baz", /nested\.qux/] + # + # precompiled = ActiveSupport::ParameterFilter.precompile_filters(filters) + # # => [/(?-mix:foo)|(?i:bar)/, /(?i:nested\.baz)|(?-mix:nested\.qux)/] + # + # ActiveSupport::ParameterFilter.new(precompiled) + # + def self.precompile_filters(filters) + filters, patterns = filters.partition { |filter| filter.is_a?(Proc) } + + patterns.map! do |pattern| + pattern.is_a?(Regexp) ? pattern : "(?i:#{Regexp.escape pattern.to_s})" + end + + deep_patterns = patterns.extract! { |pattern| pattern.to_s.include?("\\.") } + + filters << Regexp.new(patterns.join("|")) if patterns.any? + filters << Regexp.new(deep_patterns.join("|")) if deep_patterns.any? + + filters + end + # Create instance with given filters. Supported type of filters are +String+, +Regexp+, and +Proc+. # Other types of filters are treated as +String+ using +to_s+. # For +Proc+ filters, key, value, and optional original hash is passed to block arguments. diff --git a/activesupport/test/parameter_filter_test.rb b/activesupport/test/parameter_filter_test.rb index 2dae9b0eeb..d38ecb2225 100644 --- a/activesupport/test/parameter_filter_test.rb +++ b/activesupport/test/parameter_filter_test.rb @@ -121,4 +121,28 @@ class ParameterFilterTest < ActiveSupport::TestCase assert_equal after_filter, parameter_filter.filter(before_filter) end end + + test "precompile_filters" do + patterns = [/A.a/, /b.B/i, "ccC", :ddD] + keys = ["Aaa", "Bbb", "Ccc", "Ddd"] + deep_patterns = [/A\.a/, /b\.B/i, "c.C", :"d.D"] + deep_keys = ["A.a", "B.b", "C.c", "D.d"] + procs = [proc { }, proc { }] + + precompiled = ActiveSupport::ParameterFilter.precompile_filters([*patterns, *deep_patterns, *procs]) + + assert_equal 2, precompiled.grep(Regexp).length + assert_equal 2 + procs.length, precompiled.length + + regexp = precompiled.find { |filter| filter.to_s.include?(patterns.first.to_s) } + keys.each { |key| assert_match regexp, key } + assert_no_match regexp, keys.first.swapcase + + deep_regexp = precompiled.find { |filter| filter.to_s.include?(deep_patterns.first.to_s) } + deep_keys.each { |deep_key| assert_match deep_regexp, deep_key } + assert_no_match deep_regexp, deep_keys.first.swapcase + + assert_not_equal regexp, deep_regexp + assert_equal procs, precompiled & procs + end end diff --git a/guides/source/configuring.md b/guides/source/configuring.md index 29afeb3940..38976a02f9 100644 --- a/guides/source/configuring.md +++ b/guides/source/configuring.md @@ -74,6 +74,7 @@ Below are the default values associated with each target version. In cases of co - [`config.active_support.raise_on_invalid_cache_expiration_time`](#config-active-support-raise-on-invalid-cache-expiration-time): `true` - [`config.add_autoload_paths_to_load_path`](#config-add-autoload-paths-to-load-path): `false` - [`config.log_file_size`](#config-log-file-size): `100 * 1024 * 1024` +- [`config.precompile_filter_parameters`](#config-precompile-filter-parameters): `true` #### Default Values for Target Version 7.0 @@ -387,6 +388,20 @@ config.logger = ActiveSupport::TaggedLogging.new(mylogger) Allows you to configure the application's middleware. This is covered in depth in the [Configuring Middleware](#configuring-middleware) section below. +#### `config.precompile_filter_parameters` + +When `true`, will precompile [`config.filter_parameters`](#config-filter-parameters) +using [`ActiveSupport::ParameterFilter.precompile_filters`][]. + +The default value depends on the `config.load_defaults` target version: + +| Starting with version | The default value is | +| --------------------- | -------------------- | +| (original) | `false` | +| 7.1 | `true` | + +[`ActiveSupport::ParameterFilter.precompile_filters`]: https://api.rubyonrails.org/classes/ActiveSupport/ParameterFilter.html#method-c-precompile_filters + #### `config.public_file_server.enabled` Configures Rails to serve static files from the public directory. This option defaults to `true`, but in the production environment it is set to `false` because the server software (e.g. NGINX or Apache) used to run the application should serve static files instead. If you are running or testing your app in production using WEBrick (it is not recommended to use WEBrick in production), set the option to `true`. Otherwise, you won't be able to use page caching and request for files that exist under the public directory. diff --git a/railties/CHANGELOG.md b/railties/CHANGELOG.md index d6134c2fd8..e0f7855e57 100644 --- a/railties/CHANGELOG.md +++ b/railties/CHANGELOG.md @@ -1,3 +1,13 @@ +* Add `config.precompile_filter_parameters`, which enables precompilation of + `config.filter_parameters` using `ActiveSupport::ParameterFilter.precompile_filters`. + Precompilation can improve filtering performance, depending on the quantity + and types of filters. + + `config.precompile_filter_parameters` defaults to `true` for + `config.load_defaults 7.1` and above. + + *Jonathan Hefner* + * Add `after_routes_loaded` hook to `Rails::Railtie::Configuration` for engines to add a hook to be called after application routes have been loaded. diff --git a/railties/lib/rails/application.rb b/railties/lib/rails/application.rb index 990a264208..2fc3fa0ace 100644 --- a/railties/lib/rails/application.rb +++ b/railties/lib/rails/application.rb @@ -301,7 +301,7 @@ def config_for(name, env: Rails.env) # will be used by middlewares and engines to configure themselves. def env_config @app_env_config ||= super.merge( - "action_dispatch.parameter_filter" => config.filter_parameters, + "action_dispatch.parameter_filter" => filter_parameters, "action_dispatch.redirect_filter" => config.filter_redirect, "action_dispatch.secret_key_base" => secret_key_base, "action_dispatch.show_exceptions" => config.action_dispatch.show_exceptions, @@ -675,5 +675,13 @@ def build_middleware def coerce_same_site_protection(protection) protection.respond_to?(:call) ? protection : proc { protection } end + + def filter_parameters + if config.precompile_filter_parameters + ActiveSupport::ParameterFilter.precompile_filters(config.filter_parameters) + else + config.filter_parameters + end + end end end diff --git a/railties/lib/rails/application/configuration.rb b/railties/lib/rails/application/configuration.rb index f02a1fb438..99e99dae4f 100644 --- a/railties/lib/rails/application/configuration.rb +++ b/railties/lib/rails/application/configuration.rb @@ -12,7 +12,7 @@ class Application class Configuration < ::Rails::Engine::Configuration attr_accessor :allow_concurrency, :asset_host, :autoflush_log, :cache_classes, :cache_store, :consider_all_requests_local, :console, - :eager_load, :exceptions_app, :file_watcher, :filter_parameters, + :eager_load, :exceptions_app, :file_watcher, :filter_parameters, :precompile_filter_parameters, :force_ssl, :helpers_paths, :hosts, :host_authorization, :logger, :log_formatter, :log_tags, :railties_order, :relative_url_root, :secret_key_base, :ssl_options, :public_file_server, @@ -278,6 +278,7 @@ def load_defaults(target_version) load_defaults "7.0" self.add_autoload_paths_to_load_path = false + self.precompile_filter_parameters = true if Rails.env.development? || Rails.env.test? self.log_file_size = 100 * 1024 * 1024 diff --git a/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_1.rb.tt b/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_1.rb.tt index 429df13672..e04ff450f3 100644 --- a/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_1.rb.tt +++ b/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_1.rb.tt @@ -117,3 +117,7 @@ # The previous behavior was to validate the presence of the parent record, which performed an extra query # to get the parent every time the child record was updated, even when parent has not changed. # Rails.application.config.active_record.belongs_to_required_validates_foreign_key = false + +# Enable precompilation of `config.filter_parameters`. Precompilation can +# improve filtering performance, depending on the quantity and types of filters. +# Rails.application.config.precompile_filter_parameters = true diff --git a/railties/test/application/configuration_test.rb b/railties/test/application/configuration_test.rb index 1fd3ad4893..5447be1bb0 100644 --- a/railties/test/application/configuration_test.rb +++ b/railties/test/application/configuration_test.rb @@ -521,6 +521,7 @@ class Comment < ActiveRecord::Base end test "filter_parameters should be able to set via config.filter_parameters in an initializer" do + remove_from_config '.*config\.load_defaults.*\n' app_file "config/initializers/filter_parameters_logging.rb", <<-RUBY Rails.application.config.filter_parameters += [ :password, :foo, 'bar' ] RUBY @@ -530,6 +531,46 @@ class Comment < ActiveRecord::Base assert_equal [:password, :foo, "bar"], Rails.application.env_config["action_dispatch.parameter_filter"] end + test "filter_parameters is precompiled when config.precompile_filter_parameters is true" do + filters = [/foo/, :bar, "baz.qux"] + + add_to_config <<~RUBY + config.filter_parameters += #{filters.inspect} + config.precompile_filter_parameters = true + RUBY + + app "development" + + assert_equal ActiveSupport::ParameterFilter.precompile_filters(filters), Rails.application.env_config["action_dispatch.parameter_filter"] + end + + test "filter_parameters is not precompiled when config.precompile_filter_parameters is false" do + filters = [/foo/, :bar, "baz.qux"] + + add_to_config <<~RUBY + config.filter_parameters += #{filters.inspect} + config.precompile_filter_parameters = false + RUBY + + app "development" + + assert_equal filters, Rails.application.env_config["action_dispatch.parameter_filter"] + end + + test "config.precompile_filter_parameters is true by default for new apps" do + app "development" + + assert Rails.application.config.precompile_filter_parameters + end + + test "config.precompile_filter_parameters is false by default for upgraded apps" do + remove_from_config '.*config\.load_defaults.*\n' + add_to_config 'config.load_defaults "7.0"' + app "development" + + assert_not Rails.application.config.precompile_filter_parameters + end + test "config.to_prepare is forwarded to ActionDispatch" do $prepared = false