From 02a5e5a3e7fdc5e63e28f72f6597c99252630e9e Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Thu, 20 Apr 2023 15:13:02 -0700 Subject: [PATCH 1/5] Add benchmark-ips --- Gemfile | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Gemfile b/Gemfile index 78cca83b..007b8f12 100644 --- a/Gemfile +++ b/Gemfile @@ -3,6 +3,8 @@ source "https://rubygems.org" gemspec +gem "benchmark-ips" + group :test do gem "coveralls" gem "json" From f0a8df72ef278ff49f43ee0e7e51f650f3e31e7d Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Thu, 20 Apr 2023 15:15:35 -0700 Subject: [PATCH 2/5] Just use a hash for ContentSecurityPolicyConfig Previously this class had a large number of instance variables being used as storage, however they were almost entirely being accessed in a hash-like way using send and instance_variable_set, both of which are slower than just using a hash. By just using a hash we make this simpler and faster. We can also keep the hash sparse rather than using `nil` as a "not set" sentinel value. --- .../headers/content_security_policy_config.rb | 68 +++++-------------- 1 file changed, 18 insertions(+), 50 deletions(-) diff --git a/lib/secure_headers/headers/content_security_policy_config.rb b/lib/secure_headers/headers/content_security_policy_config.rb index 5d3c7550..5b824466 100644 --- a/lib/secure_headers/headers/content_security_policy_config.rb +++ b/lib/secure_headers/headers/content_security_policy_config.rb @@ -2,65 +2,33 @@ module SecureHeaders module DynamicConfig def self.included(base) - base.send(:attr_reader, *base.attrs) base.attrs.each do |attr| + base.send(:define_method, attr) do + @config[attr] + end base.send(:define_method, "#{attr}=") do |value| - if self.class.attrs.include?(attr) - write_attribute(attr, value) - else - raise ContentSecurityPolicyConfigError, "Unknown config directive: #{attr}=#{value}" - end + write_attribute(attr, value) end end end def initialize(hash) - @base_uri = nil - @block_all_mixed_content = nil - @child_src = nil - @connect_src = nil - @default_src = nil - @font_src = nil - @form_action = nil - @frame_ancestors = nil - @frame_src = nil - @img_src = nil - @manifest_src = nil - @media_src = nil - @navigate_to = nil - @object_src = nil - @plugin_types = nil - @prefetch_src = nil - @preserve_schemes = nil - @report_only = nil - @report_uri = nil - @require_sri_for = nil - @require_trusted_types_for = nil - @sandbox = nil - @script_nonce = nil - @script_src = nil - @script_src_elem = nil - @script_src_attr = nil - @style_nonce = nil - @style_src = nil - @style_src_elem = nil - @style_src_attr = nil - @trusted_types = nil - @worker_src = nil - @upgrade_insecure_requests = nil - @disable_nonce_backwards_compatibility = nil + @config = {} from_hash(hash) end + def initialize_copy(hash) + @config = hash.to_h + end + def update_directive(directive, value) - self.send("#{directive}=", value) + @config[directive] = value end def directive_value(directive) - if self.class.attrs.include?(directive) - self.send(directive) - end + # No need to check attrs, as we only assign valid keys + @config[directive] end def merge(new_hash) @@ -78,10 +46,7 @@ def append(new_hash) end def to_h - self.class.attrs.each_with_object({}) do |key, hash| - value = self.send(key) - hash[key] = value unless value.nil? - end + @config.dup end def dup @@ -114,8 +79,11 @@ def from_hash(hash) def write_attribute(attr, value) value = value.dup if PolicyManagement::DIRECTIVE_VALUE_TYPES[attr] == :source_list - attr_variable = "@#{attr}" - self.instance_variable_set(attr_variable, value) + if value.nil? + @config.delete(attr) + else + @config[attr] = value + end end end From 1230380fb5b5e56c950cc2840751d599fec156e8 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Thu, 20 Apr 2023 15:25:19 -0700 Subject: [PATCH 3/5] Remove method defintions from PolicyConfig These were only used in one place. We can avoid complexity by not defining them. --- lib/secure_headers/headers/content_security_policy.rb | 6 +++--- .../headers/content_security_policy_config.rb | 11 ----------- 2 files changed, 3 insertions(+), 14 deletions(-) diff --git a/lib/secure_headers/headers/content_security_policy.rb b/lib/secure_headers/headers/content_security_policy.rb index 566004b8..4a7b0d76 100644 --- a/lib/secure_headers/headers/content_security_policy.rb +++ b/lib/secure_headers/headers/content_security_policy.rb @@ -20,9 +20,9 @@ def initialize(config = nil) config end - @preserve_schemes = @config.preserve_schemes - @script_nonce = @config.script_nonce - @style_nonce = @config.style_nonce + @preserve_schemes = @config[:preserve_schemes] + @script_nonce = @config[:script_nonce] + @style_nonce = @config[:style_nonce] end ## diff --git a/lib/secure_headers/headers/content_security_policy_config.rb b/lib/secure_headers/headers/content_security_policy_config.rb index 5b824466..cb9a154f 100644 --- a/lib/secure_headers/headers/content_security_policy_config.rb +++ b/lib/secure_headers/headers/content_security_policy_config.rb @@ -1,17 +1,6 @@ # frozen_string_literal: true module SecureHeaders module DynamicConfig - def self.included(base) - base.attrs.each do |attr| - base.send(:define_method, attr) do - @config[attr] - end - base.send(:define_method, "#{attr}=") do |value| - write_attribute(attr, value) - end - end - end - def initialize(hash) @config = {} From 1db52a2dc9ae308a8222e9720063dac1556e0f19 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Thu, 20 Apr 2023 15:34:26 -0700 Subject: [PATCH 4/5] Misc perf improvements --- lib/secure_headers/configuration.rb | 11 ++++------- .../headers/content_security_policy_config.rb | 2 +- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/lib/secure_headers/configuration.rb b/lib/secure_headers/configuration.rb index 996ca74f..3e6a3a76 100644 --- a/lib/secure_headers/configuration.rb +++ b/lib/secure_headers/configuration.rb @@ -83,14 +83,11 @@ def named_append_or_override_exists?(name) # can lead to modifying parent objects. def deep_copy(config) return unless config - config.each_with_object({}) do |(key, value), hash| - hash[key] = - if value.is_a?(Array) - value.dup - else - value - end + result = {} + config.each_pair do |key, value| + result[key] = Array === value ? value.dup : value end + result end # Private: Returns the internal default configuration. This should only diff --git a/lib/secure_headers/headers/content_security_policy_config.rb b/lib/secure_headers/headers/content_security_policy_config.rb index cb9a154f..50ca6bd4 100644 --- a/lib/secure_headers/headers/content_security_policy_config.rb +++ b/lib/secure_headers/headers/content_security_policy_config.rb @@ -80,7 +80,7 @@ class ContentSecurityPolicyConfigError < StandardError; end class ContentSecurityPolicyConfig HEADER_NAME = "Content-Security-Policy".freeze - ATTRS = PolicyManagement::ALL_DIRECTIVES + PolicyManagement::META_CONFIGS + PolicyManagement::NONCES + ATTRS = Set.new(PolicyManagement::ALL_DIRECTIVES + PolicyManagement::META_CONFIGS + PolicyManagement::NONCES) def self.attrs ATTRS end From 79eb6c1769a677a7a9daaca9bfbba00e95395db0 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Thu, 20 Apr 2023 15:59:21 -0700 Subject: [PATCH 5/5] Work around bad rubocop rule For some reason Rubocop doesn't like the case equality operator (it's good) so we can just use a case statement I guess. --- lib/secure_headers/configuration.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/secure_headers/configuration.rb b/lib/secure_headers/configuration.rb index 3e6a3a76..2ebbf487 100644 --- a/lib/secure_headers/configuration.rb +++ b/lib/secure_headers/configuration.rb @@ -85,7 +85,13 @@ def deep_copy(config) return unless config result = {} config.each_pair do |key, value| - result[key] = Array === value ? value.dup : value + result[key] = + case value + when Array + value.dup + else + value + end end result end