Add option to skip hashing if unsafe-inline is present #15
Conversation
swac force-pushed the allow-unsafe-inline branch 2 times, most recently from 7318cda to d341222 Compare 
plugin.js Outdated
| enabled: true, | ||
| hashingMethod: 'sha256' | ||
| hashingMethod: 'sha256', | ||
| hashIfUnsafeInlinePresent: true |
There was a problem hiding this comment.
I think it would be best to infer this setting from the policy which is explicitly passed in by the dev - if they set unsafe-inline, then we can skip creating hashes instead of requiring them to set a setting
There was a problem hiding this comment.
Thanks for reviewing 😄! I considered inferring the hashing behavior, but I realized that might complicate things in a different way. The default policy already contains unsafe-inline, so I'd need to save a copy of the passed-in policy in addition to the merged policy. Then the code that does the hashing would get a bit more complex as well, since it'd need to compare against the user-specified policy, but operate on the merged policy. If that's fine, I can go ahead and make the change.
There was a problem hiding this comment.
I think thats ok to do - really, the default policy exists simply to provide something incase the user hasn't specified the policy themselves - we can always assume the default policy will have unsafe-inlines, so we just have to do a check against the user defined one like you've mentioned.
Thanks!
plugin.js Outdated
| const inlineSrc = $('script:not([src])') | ||
| .map((i, element) => this.hash($(element).html())) | ||
| .get(); | ||
| policyObj['script-src'] = flattenedPolicyObjScriptSrc.concat(inlineSrc); |
There was a problem hiding this comment.
Let's move creating this into it's own function - it can have the interface:
createObjPolicy($, selector) { return $(selector) .map((i, element) => this.hash($(element).html())) .get(); } Then we can add (pseudo):
policyObj['style-src'] = hasUnsafeInline ? don't do anything: this.createPolicyObj($, 'style:not([href])'); etc. 
swac force-pushed the allow-unsafe-inline branch from d341222 to f166ca9 Compare 
|
|
| @AnujRNair apologies for the long turnaround here. I just updated the code based on your feedback. Thanks! |
Codecov Report
@@ Coverage Diff @@ ## master #15 +/- ## ========================================== + Coverage 98.54% 98.71% +0.17% ========================================== Files 2 2 Lines 137 156 +19 Branches 7 8 +1 ========================================== + Hits 135 154 +19 Misses 2 2
Continue to review full report at Codecov.
|
AnujRNair left a comment
AnujRNair left a comment There was a problem hiding this comment.
Thanks for the update, this is looking great! 2 small comments added
plugin.js Outdated
| // Wrapped in flatten([]) to handle both when policy is a string and an array | ||
| const flattenedUserPolicy = flatten(userPolicyObj[policyName]); | ||
| if (flattenedUserPolicy.includes("'unsafe-inline'")) { | ||
| return policyObj[policyName]; |
There was a problem hiding this comment.
I think we want to return the userPolicyObj here, no? If the user has defined unsafe inline then we want to return their defined policy - I guess it's almost the same since the policies are merged in the constructor, but for readability to other devs, we should use the correct variable reference :)
We should also extend this behaviour for unsafe-eval as well!
| return compileCb(null, htmlPluginData); | ||
| } | ||
| | ||
| createPolicyObj($, policyName, selector, policyObj, userPolicyObj) { |
There was a problem hiding this comment.
Could we add a doc block for this function please?
swac force-pushed the allow-unsafe-inline branch from f166ca9 to 4e3fc00 Compare swac force-pushed the allow-unsafe-inline branch from 4e3fc00 to f858181 Compare | @AnujRNair I made the changes you requested. As for |
| Thanks for the change! Let's start with this, and if we find we need to expand this more in the future, we can do |
This reverts commit 5d642ce.
3 participants
Summary
Modern browsers ignore the
'unsafe-inline'CSP setting if any hash or nonce exists in that directive. This means that this plugin's automaticsha256hashes that it generates for inline scripts and styles implicitly disable'unsafe-inline'for modern browsers even if the developer explicitly wants'unsafe-inline'enabled.This PR addresses the issue by adding an option to the plugin config to disable the implicit hashing. Setting
hashIfUnsafeInlinePresenttofalsewhen initializing the plugin will make it skip hashing if ascript-srcorstyle-srcdirective contains an'unsafe-inline'value.Requirements (place an
xin each[ ])