Remove inline scripts for CSP

Sorry for the long delay.

It'd be good to fix this; especially for jQuery Migrate which - as you mention - is likely to be used by a lot of the installs of this module.

Writing out to a file sounds like it'd make sense.. presumably that'd typically go into an aggregated bundle.

If the JS snippet is relatively static, could the module provide the file itself rather than generating an ephemeral one?

Are there examples of other modules addressing this same issue?

Could the jQuery Migrate settings be done via Drupal.settings ? (just a thought with zero research behind it)

AFAICS there are at least 4 places where jQuery Update uses inline scripts:

$ grep -C2 -n 'type.*inline' jquery_update.module 
398-  $javascript['jquery']['js'][] = array(
399-    'data' => 'window.jQuery || document.write("<script src=\'' . base_path() . $path . '/replace/jquery/' . $version . '/jquery' . $min . '.js\'>\x3C/script>")',
400:    'type' => 'inline',
401-    'group' => JS_LIBRARY,
402-    'weight' => -19.999999999,
--
460-  $libraries['jquery.migrate']['js'][] = array(
461-    'data' => $data,
462:    'type' => 'inline',
463-    'group' => JS_LIBRARY,
464-
--
516-  $javascript['jquery.migrate']['js'][] = array(
517-    'data' => 'window.jQuery && window.jQuery.migrateWarnings || document.write("<script src=\'' . base_path() . $path . '/replace/jquery-migrate/' . $migrate_version . '/jquery-migrate' . $min . '.js\'>\x3C/script>")',
518:    'type' => 'inline',
519-    'group' => JS_LIBRARY,
520-    'weight' => -19.7999999999,
--
681-  $javascript['ui']['js'][] = array(
682-    'data' => 'window.jQuery.ui || document.write("<script src=\'' . base_path() . $path . $js_path . '\'>\x3C/script>")',
683:    'type' => 'inline',
684-    'group' => JS_LIBRARY,
685-    'weight' => -10.999999999,

I think we should aim to re-implement all of these using Drupal.settings

Then if/when 📌 [D7] Convert drupalSettings from JavaScript to JSON, to allow for CSP in the future RTBC lands, a decent CSP should be more realistic for D7 sites.

That would be an even nicer solution, for sure.

I've made a start on this; initially moving the pair of jQuery Migrate settings mentioned in the IS.

Couple of things to note..

The module's tests are not going to be much help here; we could try to add some basic tests to check what's appearing in the Drupal.settings JS (which may break if/when we convert that to JSON).

One of the original reasons these settings were implemented using inline JS may have been because of the weighting and ordering of libraries and they way they get loaded when jQuery Update is overriding core JS etc..

There are comments near where the Migrate settings were added inline to say:

  // Configure the jQuery Migrate plugin.
  // Note: This must be done after jQuery has loaded, but before the jQuery
  // migrate plugin has loaded.

..and the weights used in the hook_library_alter() are very specific.

I wouldn't be too surprised to find that this looks like it's working but it's actually happening too late / too early to work as properly intended..

..and tests won't help us figure that out. Which is fun.

If we really need to preserve the weights / ordering.. I suppose we could have a separate static file for each setting which we add in the library_alter and have that pick up each setting from Drupal.settings. I'm not entirely sure that makes any sense though.

Setting to NR for early feedback, but there are still 3 more inline settings to move across.

The 3 remaining inline JS libraries are all "backups" for when the original CDN functionality is used.

So for example:

  switch ($cdn) {
    case 'google':
      $cdn = '//ajax.googleapis.com/ajax/libs/jqueryui/' . $jqueryui_version . '/jquery-ui' . $min . '.js';
      jquery_update_jqueryui_cdn($cdn, $javascript, $path, $min, $names, $jqueryui_version);
      jquery_update_jqueryui_backup($javascript, $path, $min);
      break;

The inline JS is added by jquery_update_jqueryui_backup() and does something like:

'data' => 'window.jQuery.ui || document.write("<script src=\'' . base_path() . $path . $js_path . '\'>\x3C/script>")',

.. so that if the jQuery.ui library isn't successfully loaded from the configured CDN, an extra script tag is written which should pull in the local backup copy.

In other words, if you don't use the older CDN functionality - including if you use the newer "custom paths" settings to bring in libraries from 3rd party CDNs - then these snippets of inline JS will never be added to the page.

On that basis, I'm not so sure we need to change those.

If you want to implement CSP and inline scripts added by jQuery Update are an obstacle to that, one approach would be to use custom paths to load libraries from a CDN instead of original CDN functionality.

(That is assuming that we move the migrate settings we've already looked at successfully out of inline JS).

I've tweaked the MR as we're only adding a new .js file when we need to apply the settings for migrate (I had initially thought the file may apply several different settings).

This hopefully means it's less of a significant / global change, and we're able to stick with the very specific weighting etc.. for the file that's being added to $libraries.

However, in doing this I've had to add to the js file so that it's doing this:

var Drupal = Drupal || { "settings": {}, "behaviors": {}, "locale": {} };

...otherwise I was getting errors about Drupal not being defined, and then settings/behaviors/locale not being defined etc..

As far as I can see this is all working okay; I've tested manually with the jQuery Update defaults and checked with the admin and front-end theme with jQuery Migrate enabled.

My testing hasn't gone far beyond just checking for errors in the console and clicking around a bit to try to verify that nothing's obviously broken.

I would appreciate a review and/or some more manual testing before I commit this.

@mustanggb are you able to take a look please?

Doesn't seem like I have permission to code review, but I'll comment here instead.

Looks like it needs some sort of string to bool conversion.

e.g.
js/jquery_migrate.js

-         jQuery.migrateMute = Drupal.settings.jqueryUpdate.migrateMute;
-         jQuery.migrateTrace = Drupal.settings.jqueryUpdate.migrateTrace;
+         jQuery.migrateMute = (String(Drupal.settings.jqueryUpdate.migrateMute).toLowerCase() === 'true');
+         jQuery.migrateTrace = (String(Drupal.settings.jqueryUpdate.migrateTrace).toLowerCase() === 'true');

Other than that, works as expected in my manual testing.

Actually scrap that, I think it's better if we avoid the needless conversion back and forth in the first place.

i.e.
jquery_update.module

-         'migrateMute' => variable_get('jquery_update_jquery_migrate_warnings', FALSE) ? 'false' : 'true',
-         'migrateTrace' => variable_get('jquery_update_jquery_migrate_trace', FALSE) ? 'true' : 'false',
+         'migrateMute' => variable_get('jquery_update_jquery_migrate_warnings', FALSE) ? FALSE : TRUE,
+         'migrateTrace' => variable_get('jquery_update_jquery_migrate_trace', FALSE) ? TRUE : FALSE,

Yeah, I hadn't noticed any problems with those booleans.

However, it does seem like we could make it all a bit simpler and reduce the requirement for mental contortion with those ternaries :)

I think we can do:

        'migrateMute' => !(bool) variable_get('jquery_update_jquery_migrate_warnings', FALSE),
        'migrateTrace' => (bool) variable_get('jquery_update_jquery_migrate_trace', FALSE),

It's good if we're future-proofing this against the conversion of Drupal.settings to JSON, so it was lucky you had the patch in place.

Thanks for the review.

LGTM

Great, thanks for suggesting this @mustanggb, and your help getting it done.

Automatically closed - issue fixed for 2 weeks with no activity.