Comments

catch created an issue. See original summary.

niko-’s picture

Hi
Latest D7 patch from #2510104 attached

niko-’s picture

Issue tags: +Needs tests
andypost’s picture

Status: Active » Needs review
Issue tags: -Needs tests

There's a tests already, only assertRaw() probably could be used to make sure

+++ b/includes/common.inc
@@ -4460,9 +4460,16 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+        $js_element['#attributes'] = array(
+            // This type attribute prevents this from being parsed as an
+            // inline script.
+            'type' => 'application/json',
+            'data-drupal-selector' => 'drupal-settings-json',

looks there should be 2 spaces indent

Fabianx’s picture

Title: Convert drupalSettings from JavaScript to JSON, to allow for CSP in the future [7.x backport] » [D7] Convert drupalSettings from JavaScript to JSON, to allow for CSP in the future

Re-titling

Fabianx’s picture

Status: Needs review » Needs work

Thanks for working on this issue!

+++ b/includes/common.inc
@@ -4460,9 +4460,16 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
-        $js_element['#value_prefix'] = $embed_prefix;
-        $js_element['#value'] = 'jQuery.extend(Drupal.settings, ' . drupal_json_encode(drupal_array_merge_deep_array($item['data'])) . ");";
-        $js_element['#value_suffix'] = $embed_suffix;
+        $js_element['#attributes'] = array(
+            // This type attribute prevents this from being parsed as an
+            // inline script.
+            'type' => 'application/json',
+            'data-drupal-selector' => 'drupal-settings-json',
+        );
+        $js_element['#value'] = drupal_json_encode(drupal_array_merge_deep_array($item['data']));
+        $output .= theme('html_tag', array('element' => $js_element));
+        $js_element = $element;
+        $js_element['#attributes']['src'] = file_create_url('misc/drupal-settings-loader.js') . $query_string_separator .  REQUEST_TIME;

This part needs to be opt-in via a variable_get() as we can't break BC as others might rely on the old behavior in various ways.

The test will need to account for both.

aerozeppelin’s picture

Status: Needs work » Needs review
FileSize
19.81 KB
26.19 KB

An attempt to write tests as per #6.

unknownguy’s picture

It works nice, RTBC ?

Heine’s picture

Status: Needs review » Needs work

Patch in #7 doesn't work for me. Chrome will not execute the src script when type = "application/json", so no settings are available.

Could also use the hardened selector from #2592925: Harden drupalSettings selector against XSS when CSP is enabled.

unknownguy’s picture

That's weird I had tested it with the latest version of Chrome under windows. What's your Chrome version? Did you enable the variable?

Heine’s picture

> Did you enable the variable?

Yes.

Chrome version is 56.0.2924.76 (64-bit) on MacOS Sierra.

In console it lists regarding the script tag:

"Fetching scripts with an invalid type/language attributes is deprecated and will be removed in M56, around January 2017. See https://www.chromestatus.com/features/5760718284521472 for more details."

I guess this means the settings JSON and script need separate tags.

vmachado’s picture

Assigned: Unassigned » vmachado
vmachado’s picture

Status: Needs work » Needs review
FileSize
40.12 KB
1.3 KB

@Heine is correct!!!

After I took a look in the fix for this issue on Drupal 8 I could see that it loads the JSON content and the JavaScript file into two separate script tags.

This behavior probably has been lost when the D7 backport was made.

Attaching a new patch fixing the issue.

DamienMcKenna’s picture

Patch #13 included the patch file itself, so this removes it. It also wraps some of the comment lines at char 80, and adds 'use strict' to the new JS file.

DamienMcKenna’s picture

The tests pass, and it includes a variable requested by FabianX to control whether the functionality is enabled or disabled, so I think it could use another good review.

lisotton’s picture

Assigned: vmachado » lisotton

I'll work on review.

lisotton’s picture

Status: Needs review » Needs work
FileSize
24.65 KB

"use strict" should be between quotes.
Without quotes, browser shows a error message:
Error use strict

But why use "use strict" in this file if none of javascripts included in Drupal core is using "use strict"?

lisotton’s picture

Assigned: lisotton » Unassigned