Skip to content

Avoid calling content_security_policy_nonce internally #389

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

Merged

Conversation

eugeneius
Copy link
Contributor

Rails 5.2 adds support for configuring a Content-Security-Policy header, including adding nonces to tags produced by the javascript_tag helper.

Unfortunately, Rails and this gem now both define a helper named content_security_policy_nonce:

https://github.com/rails/rails/blob/v5.2.0.rc2/actionpack/lib/action_controller/metal/content_security_policy.rb#L44
https://github.com/twitter/secureheaders/blob/v5.0.5/lib/secure_headers/view_helper.rb#L69

The Rails helper wins over the Secure Headers one, and helpers like nonced_javascript_tag currently raise this error on Rails 5.2:

ArgumentError: wrong number of arguments (given 1, expected 0)

By using a method with a different name internally, we avoid clashing with the Rails implementation, and nonced_javascript_tag works again.

Rails 5.2 adds support for configuring a Content-Security-Policy header,
including adding nonces to tags produced by the `javascript_tag` helper.

Unfortunately, Rails and this gem now both define a helper named
`content_security_policy_nonce`:

https://github.com/rails/rails/blob/v5.2.0.rc2/actionpack/lib/action_controller/metal/content_security_policy.rb#L44
https://github.com/twitter/secureheaders/blob/v5.0.5/lib/secure_headers/view_helper.rb#L69

The Rails helper wins over the Secure Headers one, and helpers like
`nonced_javascript_tag` currently raise this error on Rails 5.2:

    ArgumentError: wrong number of arguments (given 1, expected 0)

By using a method with a different name internally, we avoid clashing
with the Rails implementation, and `nonced_javascript_tag` works again.
@CLAassistant
Copy link

CLAassistant commented Mar 25, 2018

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@jacobbednarz jacobbednarz left a comment

Choose a reason for hiding this comment

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

Nice find! 🏆

I'm not usually a fan of the underscore prefixed methods but it seems reasonable here.

@eugeneius
Copy link
Contributor Author

I have no strong opinion on the particular method name, and will happily change it to anything else. 🙂

@oreoshake
Copy link
Contributor

This is going to break things 😢. While people should use the tag helpers, I'm sure someone is using the raw value for something. Luckily, 6.0 hasn't been released so we can sneak this in there. It was meant to be a private API to begin with. The name collision is unfortunate but it's a polluted namespace.

@oreoshake oreoshake merged commit a39ae20 into github:master Mar 26, 2018
@oreoshake
Copy link
Contributor

Released in v6.0.0.alpha02.

@eugeneius
Copy link
Contributor Author

Thanks @oreoshake!

I think you may have missed this line though: content_security_policy_nonce will still work as before on Rails < 5.2. Unless I got something wrong, the only behavioural change here is that the tag helper methods (e.g. nonced_javascript_tag) will work correctly on Rails 5.2; everything else stays the same.

@paulfri
Copy link
Contributor

paulfri commented Mar 26, 2018

@eugeneius We were using content_security_policy_nonce directly in a couple cases (some third-party script tags), so this is definitely a breaking change on Rails 5.1.

@eugeneius
Copy link
Contributor Author

I'm not saying that Rails didn't break Secure Headers; I'm saying that this patch makes Secure Headers mostly compatible with Rails 5.2, without affecting its compatibility with earlier versions.

As such I think it's not a breaking change, and could be backported to 5.0.x so that there's a non-prerelease version available that (mostly) works with Rails 5.2.0 when it's out in a couple of weeks.

@paulfri
Copy link
Contributor

paulfri commented Mar 26, 2018

It may be backwards-compatible with Rails (in that nonced_javascript_tag works on both Rails 5.1 and 5.2), but it's definitely not backwards compatible with some Rails applications, since a public-accessible API content_security_policy_nonce is being renamed (and is now inferred to be private with the prefixed _).

@eugeneius
Copy link
Contributor Author

content_security_policy_nonce is still available as an alias to the underscore-prefixed version. Rails 5.1 applications will still be able to call it, just like before. If calling content_security_policy_nonce doesn't work in your app when using v6.0.0.alpha02, please tell me about the error so that I can fix it!

@paulfri
Copy link
Contributor

paulfri commented Mar 27, 2018

I completely missed that, sorry! We already updated to use the tag helpers (and 6.0.0.alpha02) but it looks like it's indeed backwards-compatible and could be backported to 5.x.

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

Successfully merging this pull request may close these issues.

5 participants