Remove "backward compatible" script helper #6391
Conversation
**Why**: Because it obscures its true purpose, which is to create a nonced script tag, already expressible in more concise terms using the default Rails APIs.
| @@ -22,8 +22,7 @@ | |||
| content_for?(:title) ? raw("#{yield(:title)} - #{APP_NAME}") : APP_NAME, | |||
| ) %> | |||
|
|
|||
|
|
|||
| <%= backwards_compatible_javascript_tag do %> | |||
| <%= javascript_tag(nonce: true) do %> | |||
There was a problem hiding this comment.
should we add a rubocop (or ERBlint?) to make sure that we always set nonce: true when calling javascript_tag?
There was a problem hiding this comment.
should we add a rubocop (or ERBlint?) to make sure that we always set
nonce: truewhen callingjavascript_tag?
Perhaps. Though, to my own review comment below, a nonce is only required if there's content of the script tag which is expected to be executed inline. I imagine it adds some complexity to the lint.
| @@ -1,4 +1,3 @@ | |||
| <!-- <%= t('notices.dap_participation') %> --> | |||
| <% dap_source = 'https://dap.digitalgov.gov/Universal-Federated-Analytics-Min.js?agency=GSA&subagency=TTS' %> | |||
| <%= backwards_compatible_javascript_tag({ src: dap_source, async: true, id: '_fed_an_ua_tag' }) do %> | |||
| <% end %> | |||
| <%= javascript_tag(src: dap_source, async: true, id: '_fed_an_ua_tag', nonce: true) %> | |||
There was a problem hiding this comment.
I wonder if this even needs nonce 🤔
There was a problem hiding this comment.
doesn't our CSP require a nonce for all JS?
identity-idp/config/initializers/content_security_policy.rb
Lines 62 to 65 in 2ff7db6
There was a problem hiding this comment.
I wonder if this even needs
nonce🤔
I just checked source of https://secure.login.gov and it doesn't assign the nonce attribute. I'm assuming Rails bails when it sees there's no content to be nonced.
There was a problem hiding this comment.
doesn't our CSP require a nonce for all JS?
For scripts loaded over the network, the CSP constraint is satisfied by the 'self' directive (or, in the case of CDN, the asset_host config). It's only for inline JavaScript that we need these nonce attributes.
There was a problem hiding this comment.
oh hm ok
Also wow I think putting the session ID (the cookie value) in the HTML source like that is not good, I wish we could hash it or pick something different
There was a problem hiding this comment.
Hm, yeah, I'd also expect it to be unique per instance too. @jmhooper do you recall if there was a particular rationale here? In other examples I see that it's configured to generate a random value?
Edit: Actually, it looks like even if the generator produces a unique value on each call, the same value is reused for the duration of the request.
There was a problem hiding this comment.
Since it's not related, I'm gonna merge as-is. I was planning to make a ticket to track that, though I'd want to confirm there's actually something to be done.
changelog: Internal, JavaScript, Remove unnecessary script helpers
javascript_tag seems to only be happy with block content. No need to overcomplicate? Just output the HTML tag since there's nothing dynamic here.
Why: Because it obscures its true purpose, which is to create a nonced script tag, already expressible in more concise terms using the default Rails APIs.
It had some more value previously (prior to #5974) when we were juggling between two CSP implementations, but now that we're settled on the Rails implementation, the helper doesn't seem valuable anymore.