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

Allow to pass options to csp_meta_tag #35269

Merged
merged 1 commit into from Feb 16, 2019

Conversation

y-yagi
Copy link
Member

@y-yagi y-yagi commented Feb 14, 2019

Currently csp_meta_tag generates name attribute only.
However, in libraries like Material-UI and JSS, expect that the meta tag that contains the nonce with property attribute.

https://material-ui.com/css-in-js/advanced/#how-does-one-implement-csp
https://github.com/cssinjs/jss/blob/master/docs/csp.md

This patch allows csp_meta_tag to specify arbitrary options and allows nonce to be passed to those libraries.

@rails-bot rails-bot bot added the actionview label Feb 14, 2019
@@ -14,9 +14,11 @@ module CspHelper
# This is used by the Rails UJS helper to create dynamically
# loaded inline <script> elements.
#
def csp_meta_tag
def csp_meta_tag(options = {})
Copy link
Member

Choose a reason for hiding this comment

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

How about using splat hash?

Since Ruby 2.6, this options hash would cause a warning like here:

def foo(a = {}) 
  a
end

a = { foo: "foo" }

p foo(a.merge(bar: "bar"))
p foo(**a, bar: "bar")
% ruby -w xxx.rb
{:foo=>"foo", :bar=>"bar"}
xxx.rb:1: warning: in `foo': the last argument was passed as a single Hash
xxx.rb:8: warning: although a splat keyword arguments here
{:foo=>"foo", :bar=>"bar"}

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, make sense. I fixed.

@y-yagi y-yagi force-pushed the allow_to_pass_options_to_csp_meta_tag branch from 1c436a5 to c9e682d Compare February 14, 2019 08:17
tag("meta", name: "csp-nonce", content: content_security_policy_nonce)
options[:name] = "csp-nonce"
options[:content] = content_security_policy_nonce
tag("meta", options)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could switch to tag.meta while we’re here.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about keeping the line the same but unsplatting the options at the end of the tag call?

Copy link
Member Author

Choose a reason for hiding this comment

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

In tag("meta") and tag.meta, the generated tags are slightly different.

helper.tag("meta", name: "csp-nonce")
#=> "<meta name=\"csp-nonce\" />"
helper.tag.meta(name: "csp-nonce")
#=> "<meta name=\"csp-nonce\"> 

Of course, since meta tag is a void element, it is not necessary to have a close tag. However, the tag generated by csrf_meta_tags have a close tag, and I think that it is better to behave similarly so that the closing tag is generated.

What about keeping the line the same but unsplatting the options at the end of the tag call?

👍 I fixed.

Copy link
Member

Choose a reason for hiding this comment

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

What about keeping the line the same but unsplatting the options at the end of the tag call?

It would cause a warning https://travis-ci.org/rails/rails/jobs/493601600#L1254-L1256.

Copy link
Member Author

Choose a reason for hiding this comment

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

I screwed up! Thanks again!

@y-yagi y-yagi force-pushed the allow_to_pass_options_to_csp_meta_tag branch from c9e682d to 3f6b618 Compare February 15, 2019 05:30
Currently `csp_meta_tag` generates `name` attribute only.
However, in libraries like `Material-UI` and `JSS`, expect that the meta tag
that contains the nonce with `property` attribute.

https://material-ui.com/css-in-js/advanced/#how-does-one-implement-csp
https://github.com/cssinjs/jss/blob/master/docs/csp.md

This patch allows `csp_meta_tag` to specify arbitrary options and
allows `nonce` to be passed to those libraries.
@y-yagi y-yagi force-pushed the allow_to_pass_options_to_csp_meta_tag branch from 3f6b618 to 9693733 Compare February 16, 2019 00:37
@y-yagi y-yagi merged commit f851e4a into rails:master Feb 16, 2019
@y-yagi y-yagi deleted the allow_to_pass_options_to_csp_meta_tag branch February 16, 2019 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants