Setting two CSP headers, abstract out dynamic pieces #281
Conversation
|
Where would you put this in terms of completion? Would you be confident in trying this out on an existing implementation yet? (with the exception of the |
|
I wouldn't use this implementation yet. The |
|
All good 👍 Just checking whether I should run this against our internal test suite and see if it pops up with anything but I'll hold off until the |
| @@ -121,7 +119,8 @@ def deep_copy_if_hash(value) | |||
| def initialize(&block) | |||
| self.hpkp = OPT_OUT | |||
| self.referrer_policy = OPT_OUT | |||
| self.csp = self.class.send(:deep_copy, CSP::DEFAULT_CONFIG) | |||
| self.csp = ContentSecurityPolicyConfig::DEFAULT | |||
There was a problem hiding this comment.
Would smarter defaults be to swap these conditions and allow a report only by default?
self.csp = OPT_OUT
self.csp_report_only = ContentSecurityPolicyConfig::DEFAULTOr would this break backwards compatibility with existing implementations?
There was a problem hiding this comment.
Well moving to report only shouldn't break existing implementations, but the previous default was an enforced policy. The change to enforce-by-default was made at the last major version bump iirc so there's no reason to go "backwards" here. This PR does change the default policy to something more realistic and secure at the same time.
There was a problem hiding this comment.
Sounds perfectly sane :)
oreoshake
changed the title
oreoshake
changed the title
oreoshake
changed the title
|
I removed the WIP label. I still need to update the docs and take another glance for quality over the change, but this should be ready for use. |
|
Awesome! I'll pull this locally and run it through it's paces in our setup. :) |
4 similar comments
|
Ah, makes sense. So if one was looking to only use a report only policy, it should be something like this? |
|
Correct. It doesn't feel right, however. Maybe adding smarts around detecting intention won't be so bad. |
Seems like a reasonable pattern to follow seeing that is how all the other directives are removed/ignored. Perhaps a sensible default of Using the following to have a report only policy results in SecureHeaders::Configuration.default do |config|
config.csp = SecureHeaders::OPT_OUT
config.csp_report_only = {
report_uri: %w(https://reporting-endpoint),
# .. snip
}
end |
Are you saying to make
The latter option was what I was going to try. |
@jacobbednarz can you provide the full stacktrace/config? I just plopped that in and it worked (but needed a |
|
Gemfile (running at 7600b90) Config (some of these directive values have been changed but it still results in the same error). SecureHeaders::Configuration.default do |config|
config.csp = SecureHeaders::OPT_OUT
config.csp_report_only = {
report_uri: %w(https://my-report-only-uri),
preserve_schemes: true,
default_src: %w(* data:),
img_src: %w(* data:),
style_src: %w(* 'self' 'unsafe-inline'),
font_src: %w('self' data:),
script_src: %w(
*
data:
'self'
'unsafe-eval'
'unsafe-inline'
),
connect_src: %w(*),
base_uri: %w(*),
}
endStacktrace |
|
I agree with
as it makes the most sense to me. |
|
🆒 added that in 3225ed6 and I believe I fixed your error too. I don't have an explanation, however. |
3 similar comments
|
My apologies, it was never the default |
|
This is now passing our internal test suite too! Very nice effort ✨ |
|
@jacobbednarz are you using this now? I don't have any plans to use this feature at this time and I'm a little uncomfortable deploying a feature I'm not using. I'd love it if this code could bake in a real world scenario for some time. |
|
I'm happy to get our app running this branch however we are currently only using a report only policy but there are plans in the near future to look at an enforced policy for some of our directives so if you don't mind holding this pull request open for a bit, I'd be happy to get this running in our production environment. |
|
🆒 I'm in no rush at this time. I'm not using it 😄. If anyone else has a chance to test drive it, that'd be ✨ |
|
This has been working great in production without any regressions identified using a |
|
I'm going to merge this branch and do a pre-release |
|
Released in 3.5.0.pre |
Fixes #256, #281
@modifiedbit strategy isn't working perfectly, I'm seeing objects being created when policies are not modified.Setting two headers has been an often requested feature that has always had a workaround but was never a first class feature. This PR allows setting both headers.
In the process of supporting both, I decided to move away from hash-backed configs and towards class-based configs - mostly because it made debugging things very hard and mistyping a hash key is far to easy. This has the added benefit of not having to perform "deep clones" all the time and relying on hash voodoo. This is much clearer even if some of the operations are just glorified hash operations.
This also adds a
@modifiedattribute which signals whether the cached headers can be used or not. This removes the need to track adynamic_csp, choose between two configs, and use theidempotent_operationscall which was all just a bit too funky and created the need for a lot of boilerplate code.When writing support for
override_content_security_policy_directives/append_content_security_policy_directives, I realized that the default policy is not very user-friendly and insecure at the same time. Updating the default policy should make it easier for people to use the default setting and make their policy dynamic as needed. The new policy is less restrictive for the most part, with the exception ofobject-src 'none'which is just an unsafe default.The reason the
DynamicConfigstuff is extracted into a module with some hackery is so that it can be reused when implementing FeaturePolicy which will benefit from the dynamic helper functions and non-hash-backed data. I like the idea, but the implementation looks a bit janky. I'll look into how ActiveRecord handles it'schanged?function. I'm also questioning the use ofupdate_directive/directive_value.