Use correct CSP nonce behavior when 'unsafe-inline' is set #1041
Conversation
waltjones
force-pushed
the
wj-snippet-csp-nonce
branch
from
f0d1db5 to
af660be
Compare
| def nonce_present | ||
| # Rails will add the nonce to script_src automatically, when script_src is present. | ||
| # Rails will add the nonce to script_src when the generator is set. | ||
| Rails.application.config.content_security_policy_nonce_generator = lambda { |_| SecureRandom.base64(16) } | ||
|
|
||
| Rails.application.config.content_security_policy do |policy| | ||
| policy.script_src :self, :https | ||
| end | ||
| end | ||
|
|
||
| def nonce_not_present | ||
| Rails.application.config.content_security_policy_nonce_generator = nil | ||
|
|
||
| Rails.application.config.content_security_policy do |policy| | ||
| policy.script_src :self, :https | ||
| end |
There was a problem hiding this comment.
It seems like some functions (here and below) have redundant definition and it might be wise to extract some methods. However, as it's spec file it's probably better to leave it in the current state, so I only marking it for taking under consideration.
There was a problem hiding this comment.
I might be misunderstanding what you're looking at, but while the method names are the same in both places, the implementation is different for Rails 5 content security policy vs the secure headers gem that is tested in the other case.
There was a problem hiding this comment.
I'm not talking about method names but the implementation of the methods.
The only difference between nonce_present and nonce_not_present methods is this:
Rails.application.config.content_security_policy_nonce_generator = lambda { |_| SecureRandom.base64(16) } OR nil
The rest of the code is equivalent:
Rails.application.config.content_security_policy do |policy|
policy.script_src :self, :https
endSimilar pattern happens to other functions below this snippet.
There was a problem hiding this comment.
@waltjones just for clarification. I don't think there is anything wrong and/or needing refactoring here. As long as it was intended to duplicate same logic per test, it's okay to keep redundant lines as this simplify and isolate tests. I only wanted to raise your attention in case this was unintended.
There was a problem hiding this comment.
@bxsx Apologies, it took me awhile to get back to respond to your comment.
It's intentional. I'd say it's merely a coincidence that two of the current test cases have the same directive list for now. (And a third one doesn't.) For the shared method to remain useful in the future, it would need to accept an arbitrary set of directives. Rails.application.config.content_security_policy already does this, but wrapping it could still be valuable. If that method changes in a future Rails version, we only want to branch on Rails version in one place. So for a wrapper, we definitely want to accept an arbitrary set of directives, so all of the current test cases can use it, and any that are added later that also have different directives. (Or if one of the current test cases changes its directives.)
As the code stands, we'd be replacing a method that accepts a block with a wrapper method that accepts a block. Not much code reduction, but still valuable in the future if we need to branch on different Rails versions.
If this were used in more than a few places, I might go ahead and do that now. Maybe? We'd just be betting on this part of Rails changing at some point and most likely in this case it won't.
FWIW, I'd probably say the same if it were in non-test code, assuming there was the same volatility in what directives would be needed each time, it's already a single method call, and it's used a handful of times or less.
Description of the change
When including a script tag for the Rollbar.js snippet, and when deciding whether to add a CSP nonce to that script tag, this PR always adds the nonce if it is detected that nonce behavior has been enabled. It does this when using either the Rails 5 CSP feature or the secure_headers gem.
Background
Originally, the secure_headers logic would not add the nonce if the 'unsafe-inline' directive was present. When support for Rails 5 CSP was added, the same policy was followed there as well. (#809)
This prevents compatibility with browsers that implement CSP1 but don't implement the nonce behavior of CSP2. While the CSP2 spec leaves this undefined, the consensus among browsers is to allow both the nonce and 'unsafe-inline' in the script_src directive. #1010 attempted to fix this, but allowed the nonce to be added to script_src even when the app didn't intend to use nonces. (This is because in the secure_headers gem, anytime a nonce is generated it will automatically be added to the script_src directive.)
Solution
When using secure_headers, check the value in the
::SecureHeaders::NONCE_KEYof the request env. If the app is using nonces, this will be set and therefore a nonce should be used.For Rails 5 CSP, no change was needed because the user must set the nonce generator in order for a nonce to be added. If the generator is not set, the current logic already does the correct thing.
Note: Unrelated to this PR, a new version of the rexml gem is incompatible with Ruby 2.0, breaking CI for Rails 3.x builds. This PR includes an update (af660be) for this.
Type of change
Related issues
ch83823
Development
Code review