importmap-rails with minimal js changes #4396
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #4396 +/- ##
==========================================
- Coverage 96.99% 96.96% -0.03%
==========================================
Files 339 339
Lines 7611 7617 +6
==========================================
+ Hits 7382 7386 +4
- Misses 229 231 +2 ☔ View full report in Codecov by Sentry. |
martinemde
changed the title
test/system/autocompletes_test.rb
Outdated
| @@ -19,7 +19,7 @@ class AutocompletesTest < ApplicationSystemTestCase | |||
| def wait_for_ajax | |||
| Timeout.timeout(Capybara.default_max_wait_time) do | |||
| loop do | |||
| active = page.evaluate_script("jQuery.active") | |||
| active = page.evaluate_script("if (jQuery !== undefined) { jQuery.active }") | |||
There was a problem hiding this comment.
Selenium::WebDriver::Error::JavascriptError: javascript error: Unexpected token 'if'
There was a problem hiding this comment.
hmm. This is a little annoying because accessing jQuery on window is not safe with imports since it may not be loaded yet. I'm struggling a bit here with how to check if it's ready.
There was a problem hiding this comment.
ok, I've established without a doubt that selenium is not running the javascript for autocomplete. The content security policy is blocking all local javascript files in test.
The urls during test are like: http://localhost:31337/assets/application-525f2ca7a98c72861092d17a0cb1e9e976154d231fae18ebf534d9a4a813bdf0.js
It seems like the only solution is to allow unsafe_inline. This is the only other place I found a definitive answer: single-spa/create-single-spa#44 (comment)
There was a problem hiding this comment.
why is it required now when it wasn't before?
There was a problem hiding this comment.
the fundamental functionality of importmap-rails is to make the browser do a bunch of ESM loading instead of bundling (eg, javascript files that contain import statements and reference other URLs). I think that means you need a working CSP policy for importmaps, and you probably didn't need one before because the JS files were bundled into a single named file loaded with a script tag and without invoking the browser's ESM loader.
There was a problem hiding this comment.
The importmap script tag gets evaluated as an unsafe-inline in selenium on localhost in test. I don't know why this works in dev but not in selenium, but it's refusing to even read the script tag.
This is what it looks like in localhost (I don't know if the nonce being blank is actually "true" or if the inspector removed the nonce. Rails should add the nonce automatically to this tag.)
<script type="importmap" data-turbo-track="reload" nonce="">{
"imports": {
"application": "/assets/application-525f2ca7a98c72861092d17a0cb1e9e976154d231fae18ebf534d9a4a813bdf0.js",
"github-buttons": "/assets/github-buttons-b817acbc589043daf780bcecd371ed09aced4b41485524cf39c2838d9382ef36.js",
"webauthn-json": "/assets/webauthn-json-2f2298307e84a8f8cfc35b5a642f4570c1702dd46fdd518ef38950b9b5daf348.js",
"jquery": "/assets/jquery-9292661fe0d8c5ef2ef35f5ca64d541d70c87e9f6d7f2716d646591a295b7f36.js",
"@rails/ujs": "/assets/@rails--ujs-8fbd650218b6b146a78aa5d7d6cd8eb8d9da87cc6d504aab7fc3c6b64e94d34b.js",
"clipboard": "/assets/clipboard-90e4dc523c2b03c3455a0fbd8a56f76fbcf720363dc8b8d12c12b6eeb7b4bd97.js"
}
}</script>
unsafe-inline is ignored by browsers that support nonce. You can take our CSP and paste it here to see the recommended changes. It suggests adding unsafe inline for backwards compatibility with older browsers that don't support nonce.
CSP in this branch:
default-src 'self'; font-src 'self' https://fonts.gstatic.com; img-src 'self' https://secure.gaug.es https://gravatar.com https://www.gravatar.com https://secure.gravatar.com https://*.fastly-insights.com https://avatars.githubusercontent.com; object-src 'none'; script-src 'self' https://secure.gaug.es https://www.fastly-insights.com https://unpkg.com/@hotwired/stimulus/dist/stimulus.umd.js https://unpkg.com/stimulus-rails-nested-form/dist/stimulus-rails-nested-form.umd.js https://ga.jspm.io/npm:[email protected]/dist/es-module-shims.js 'nonce-e2ad7278280e7bead9d9db2cd5c8ee17'; style-src 'self' https://fonts.googleapis.com 'nonce-e2ad7278280e7bead9d9db2cd5c8ee17'; connect-src 'self' https://s3-us-west-2.amazonaws.com/rubygems-dumps/ https://*.fastly-insights.com https://fastly-insights.com https://api.github.com http://localhost:*; form-action 'self' https://github.com/login/oauth/authorize; frame-ancestors 'self'; report-uri
(I tried adding recommended Got it.policy.base_uri :none and it's not being added to the CSP by rails 🤷♂️)
There was a problem hiding this comment.
if there's a nonce, shouldn't it be used by the CSP?
There was a problem hiding this comment.
The result of this thread is that the nonce isn't always set on the first request because the session may not be updated during that request. Ultimately now we're adding the sha256 of the importmap to the CSP.
martinemde
force-pushed
the
martinemde/importmap
branch
from
517f27a
to
e0c3ecc
Compare
martinemde
force-pushed
the
martinemde/importmap
branch
3 times, most recently
from
033ffb6
to
2fd6b81
Compare
|
I don't plan to fix the code climate complaints since they're just in the javascript I moved and I'm planning to replace them with stimulus controllers. |
|
This is ready despite the ❌ |
Add importmap:verify to check vendored javascript Run importmap:verify as part of lint Update content security policy based on recommendations from google csp validator in order to get importmap script tag to load. Use a random base64 if a nonce isn't available from the session Use script-src: sha256-* instead of nonce for importmap script tag This allows the importmap to be cached without creating a long lived nonce in the cache Fix issue with blank cookie causing rack-test crash
martinemde
force-pushed
the
martinemde/importmap
branch
from
02ddaff
to
088631b
Compare
In response to discussion and my plan in #4392, I've created a PR with just the importmap changes. This PR makes no changes to the javascript or sprockets besides what was necessary. This is working and passes tests locally.
This can complement #4123 by creating a foundation for importmap from which we can choose any other changes to the asset pipeline.
Note: I chose to pin to downloaded javascripts rather than remote ones because that most closely resembles our existing process. If we'd like to pin to hosted javascripts we could change that in a follow up.