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

Feature idea: Always pass nonce if config is set #99

Closed
colinxfleming opened this issue Dec 30, 2019 · 6 comments · Fixed by #133
Closed

Feature idea: Always pass nonce if config is set #99

colinxfleming opened this issue Dec 30, 2019 · 6 comments · Fixed by #133

Comments

@colinxfleming
Copy link
Contributor

hi @nikolalsvk ! It looks like render_async is increasingly starting to take initializer configs, like config.turbolinks = true or config.jquery = true. I kinda like that pattern a lot, as the turbolinks option is removing a lot of redundant stuff from my codebase, and I appreciate that!

A similar thing I am doing (to literally every render_async call) is html_options: { nonce: true } because I need to get the javascript past my CSP. So I was wondering: Do you have any interest in adding a 'turn on nonces by default' as an option in RenderAsync.configuration, or do you think it would be overkill? Would probably be something like (spitballing):

RenderAsync.configure do |config|
  config.nonces = true
end

# _render_async.html.erb
...
javascript_tag html_options.merge({nonce: RenderAsync.configuration.nonces}) do
  ...
end

and that would basically do the same thing as what you can currently accomplish with:

render_async users_path, html_options: { nonce: true }

(The other thing I kinda thought about was a default_html_options config, which would merge into html_options but I think I like the specific option better.)

More than happy to take a stab at this work if it sounds good or interesting? I kinda think it would be a nice 'turn on extra security feature by default' kinda deal. Let me know what you think, or if I can clarify anything.

@walterdavis
Copy link

Would merge be the best choice here? Would that allow override on an individual element where (for some reason) you didn't want to include the nonce? What about ||= in there somewhere?

@colinxfleming
Copy link
Contributor Author

@walterdavis oh yeah, it's almost definitely not the best choice for an actual implementation - was just some late night pseudocode to get the idea across.

@nikolalsvk
Copy link
Owner

This sounds pretty good and useful to people who use nonce! I'd say you can go ahead and take a stab at implementing this. I'd be more than glad to help out with coding / review 👍

@nikolalsvk
Copy link
Owner

Hey, @colinxfleming, I released a config option for nonce in 2.1.8 version. Check it out and let me know if it works for you.

You can now do this:

RenderAsync.configure do |config|
  config.nonces = true
end

And not care about passing nonce in each render_async call.

Cheers 🍻

@colinxfleming
Copy link
Contributor Author

@nikolalsvk sorry for posting the idea and then promptly not having the time and space to work on it! But this works fantastic; thank you very much! Stoked to put it to work in my corner of the world.

@nikolalsvk
Copy link
Owner

No problem, @colinxfleming. Let me know if it works for you and if we can add anything else.

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

Successfully merging a pull request may close this issue.

3 participants