Skip to content

Commit

Permalink
Make SecureSecurityPolicyConfig significantly faster (#506)
Browse files Browse the repository at this point in the history
We have been seeing this gem a lot in profiles. Must of this slowness
seems to come from overuse of instance variables in `DynamicConfig` and
attempting to use them basically as a hash (which we can do much faster
with a hash 😅)

The first commit of these is the most important, but the other two also
significantly speed things up.

There is definitely more improvement available here, we seem to be
overly cautious in duplicating arrays, and we also seem to convert
unnecessarily between hashes and the config object, but I think this is
the best place to start.

<details>
<summary>Benchmark:</summary>

```
require "secure_headers"
require "benchmark/ips"

# Copied from README
MyCSPConfig = SecureHeaders::ContentSecurityPolicyConfig.new(
  preserve_schemes: true, # default: false. Schemes are removed from host sources to save bytes and discourage mixed content.
  disable_nonce_backwards_compatibility: true, # default: false. If false, `unsafe-inline` will be added automatically when using nonces. If true, it won't. See #403 for why you'd want this.

  # directive values: these values will directly translate into source directives
  default_src: %w('none'),
  base_uri: %w('self'),
  block_all_mixed_content: true, # see https://www.w3.org/TR/mixed-content/
  child_src: %w('self'), # if child-src isn't supported, the value for frame-src will be set.
  connect_src: %w(wss:),
  font_src: %w('self' data:),
  form_action: %w('self' github.com),
  frame_ancestors: %w('none'),
  img_src: %w(mycdn.com data:),
  manifest_src: %w('self'),
  media_src: %w(utoob.com),
  object_src: %w('self'),
  sandbox: true, # true and [] will set a maximally restrictive setting
  plugin_types: %w(application/x-shockwave-flash),
  script_src: %w('self'),
  script_src_elem: %w('self'),
  script_src_attr: %w('self'),
  style_src: %w('unsafe-inline'),
  style_src_elem: %w('unsafe-inline'),
  style_src_attr: %w('unsafe-inline'),
  worker_src: %w('self'),
  upgrade_insecure_requests: true, # see https://www.w3.org/TR/upgrade-insecure-requests/
  report_uri: %w(https://report-uri.io/example-csp)
)


Benchmark.ips do |x|
  x.report "csp_config.to_h" do
    MyCSPConfig.to_h
  end

  x.report "csp_config.append" do
    MyCSPConfig.append({})
  end

  x.report "new(config).value" do
    SecureHeaders::ContentSecurityPolicy.new(MyCSPConfig).value
  end
end
```

</details>


**Before:**

```
$ be ruby bench.rb
Warming up --------------------------------------
     csp_config.to_h    13.737k i/100ms
   csp_config.append     2.105k i/100ms
   new(config).value     1.429k i/100ms
Calculating -------------------------------------
     csp_config.to_h    139.988k (± 0.3%) i/s -    700.587k in   5.004666s
   csp_config.append     21.133k (± 2.4%) i/s -    107.355k in   5.082856s
   new(config).value     14.298k (± 0.4%) i/s -     72.879k in   5.097116s
```


**After:**

```
$ be ruby bench.rb
Warming up --------------------------------------
     csp_config.to_h   123.784k i/100ms
   csp_config.append     4.181k i/100ms
   new(config).value     1.617k i/100ms
Calculating -------------------------------------
     csp_config.to_h      1.238M (± 3.1%) i/s -      6.189M in   5.003769s
   csp_config.append     40.921k (± 1.0%) i/s -    204.869k in   5.006924s
   new(config).value     16.095k (± 0.4%) i/s -     80.850k in   5.023259s
```

`to_h` is 10x faster, `append` is 2x faster, and .value (which was not
the target of these optimizations but I didn't want to see it regress)
is slightly faster

---------

Co-authored-by: Kylie Stradley <[email protected]>
  • Loading branch information
jhawthorn and KyFaSt authored Aug 11, 2023
1 parent ff9797f commit 7a23cb6
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 63 deletions.
2 changes: 2 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ source "https://rubygems.org"

gemspec

gem "benchmark-ips"

group :test do
gem "coveralls"
gem "json"
Expand Down
9 changes: 6 additions & 3 deletions lib/secure_headers/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -83,14 +83,17 @@ 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)
result = {}
config.each_pair do |key, value|
result[key] =
case value
when Array
value.dup
else
value
end
end
result
end

# Private: Returns the internal default configuration. This should only
Expand Down
6 changes: 3 additions & 3 deletions lib/secure_headers/headers/content_security_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

##
Expand Down
72 changes: 15 additions & 57 deletions lib/secure_headers/headers/content_security_policy_config.rb
Original file line number Diff line number Diff line change
@@ -1,65 +1,23 @@
# frozen_string_literal: true
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 |value|
if self.class.attrs.include?(attr)
write_attribute(attr, value)
else
raise ContentSecurityPolicyConfigError, "Unknown config directive: #{attr}=#{value}"
end
end
end
end

def initialize(hash)
@base_uri = 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)
Expand All @@ -77,10 +35,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
Expand Down Expand Up @@ -113,16 +68,19 @@ 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

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
Expand Down

0 comments on commit 7a23cb6

Please sign in to comment.