GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Make SecureSecurityPolicyConfig significantly faster by jhawthorn · Pull Request #506 · github/secure_headers

Conversation

@jhawthorn

@jhawthorn jhawthorn commented Apr 20, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

Benchmark:
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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

oreoshake and KyFaSt reacted with heart emoji
All reactions
  • ❤️ 2 reactions
jhawthorn added 5 commits April 20, 2023 15:36
@jhawthorn
Add benchmark-ips
02a5e5a
@jhawthorn
Just use a hash for ContentSecurityPolicyConfig
f0a8df7
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.
@jhawthorn
Remove method defintions from PolicyConfig
1230380
These were only used in one place. We can avoid complexity by not
defining them.
@jhawthorn
Misc perf improvements
1db52a2
@jhawthorn
Work around bad rubocop rule
79eb6c1
For some reason Rubocop doesn't like the case equality operator (it's
good) so we can just use a case statement I guess.
KyFaSt
KyFaSt approved these changes Jul 10, 2023
View reviewed changes

@KyFaSt KyFaSt left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jhawthorn, thanks so much for this PR. We were able to test and verify this change in the github project, feel free to merge. We have another change we'd like to add so you don't need to cut a release, our team will cut the release once the change is added. In favor of making the smallest possible change, our team will cut a separate release for this PR once you have merged the PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@KyFaSt
KyFaSt self-requested a review July 10, 2023 21:33
KyFaSt
KyFaSt approved these changes Jul 18, 2023
View reviewed changes
@KyFaSt
Merge branch 'main' into just_use_a_hash
dc6c534
@KyFaSt
KyFaSt merged commit 7a23cb6 into github:main Aug 11, 2023
Ninja-mann07
Ninja-mann07 reviewed Aug 16, 2023
View reviewed changes

@Ninja-mann07 Ninja-mann07 left a comment

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#506

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Eric-Guo added a commit to Eric-Guo/openproject that referenced this pull request Oct 17, 2024
@Eric-Guo
Fix spec due to github/secure_headers#506
e60e98d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@KyFaSt KyFaSt KyFaSt approved these changes
+1 more reviewer
@Ninja-mann07 Ninja-mann07 Ninja-mann07 left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@jhawthorn @KyFaSt @Ninja-mann07
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.