Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the `nonce: true` option for `javascript_include_tag` helper. #32607

Merged

Conversation

@yaroslav
Copy link
Contributor

@yaroslav yaroslav commented Apr 17, 2018

Summary

Add the nonce: true option for javascript_include_tag helper to support automatic nonce generation for Content Security Policy. Works the same way as previously introduced javascript_tag nonce: true does.

This way, one does not have to do ..., nonce: content_security_policy_nonce everywhere in templates to do nonce-based script-src CSP.

@rails-bot
Copy link

@rails-bot rails-bot commented Apr 17, 2018

r? @schneems

(@rails-bot has picked a reviewer for you, use r? to override)

@yaroslav
Copy link
Contributor Author

@yaroslav yaroslav commented Apr 17, 2018

@rails-bot rails-bot assigned pixeltrix and unassigned schneems Apr 17, 2018
@yaroslav yaroslav force-pushed the yaroslav:feature/nonce-for-javascript_include_tag branch to 47013a7 Apr 17, 2018
@pixeltrix pixeltrix merged commit 185fce1 into rails:master Apr 18, 2018
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pixeltrix
Copy link
Member

@pixeltrix pixeltrix commented Apr 18, 2018

@yaroslav thanks! 👍

pixeltrix added a commit that referenced this pull request Apr 18, 2018
…include_tag

Add the `nonce: true` option for `javascript_include_tag` helper.
@TylerRick
Copy link
Contributor

@TylerRick TylerRick commented Sep 10, 2019

Is there a reason this can't just default to true?

Why do I have specifically to pass nonce: true to every single javascript_include_tag when I'm using a CSP?

If you are using a strict CSP (and you should be), it seems like you would want a nonce added to every script tag. Wouldn't you? (That's been my experience so far.) Otherwise that script tag will violate the CSP and give an error in the console.

But it is all too easy to forget to manually add nonce: true to some javascript_include_tag somewhere in your app. So nonce: true would be a more useful default to have.

And since it only adds an automatic nonce value if you have Content Security Policy enabled, I would think it would be a safe default.

So my question is, why wouldn't you want it to always add the nonce?

https://csp.withgoogle.com/docs/strict-csp.html recommends:

To enable a strict CSP policy, most applications will need to make the following changes:

  • Add a nonce attribute to all <script> elements. Some template systems can do this automatically.

To that end, I've been overriding javascript_include_tag and setting it as the default in my app like this:

  def javascript_include_tag(sources, **options)
    options[:nonce] = true unless options.key?(:nonce)
    super *sources, **options
  end

(If for some reason you ever didn't want a nonce somewhere, you could still override by passing nonce: false but that would be the exception, not the rule)

@pixeltrix
Copy link
Member

@pixeltrix pixeltrix commented Sep 15, 2019

Generally it's a good idea to have defaults that are secure and setting nonce: true by default would go against that principal. Since most javascript_include_tag usage would be covered under script-src 'self' it doesn't seem to me that applications would need to use nonce: true a whole lot. Support for nonce was primarily added to allow the inline JS for Rails UJS to execute - it's not something we'd generally recommend using to allow third-party JS to run.

Are you linking to a lot of external JS from a variety of domains? If so a more secure option would be to use Subresource Integrity than blindly adding a nonce value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants