Convert drupalSettings from JavaScript to JSON, to allow for CSP in the future

Affect the JS api

It wouldn't, you already cannot use #attached to attach inline JS. You'd have to use ['#attached']['html_head'] already.

General +1, of course.

I don't think this needs to be critical though. And it's not a bug, I think.

Sounds like security hardening and so major ... but on the other hand its an API change.

It wouldn't, you already cannot use #attached to attach inline JS. You'd have to use ['#attached']['html_head'] already.

Given that this really should be major?

Why is this marked critical?

As Wim said, we don't allow any manual setting of inline javascript at the moment.

Also presumably even if we change how this gets rendered, the actual settings will still end up in drupalSettings.

So no API change here.

Secure by Default FTW.

Not using ID to keep consistency with #1305882: drupal_html_id() considered harmful; remove ajax_html_ids to use GET (not POST) AJAX requests and #2509970: Apply and use data-drupal-selector consistently..

Settings were meant to be usable without drupal.js file, so I added a new file dealing with settings only instead of putting that in drupal.js Has to be weight -18 because object needs to exist for other JS.

+++ b/core/core.libraries.yml
@@ -52,6 +52,8 @@ drupal:
+  js:
+    misc/drupalSettings.js: { weight: -18 }

Is -18 enough, should this not be -100 or such to make it less likely someone coming before that?

--

Was wondering if it was not best practice to pass the 'window' in the function as argument and parameter, but I guess that is redundant as window is global everywhere still ...

--

Would be RTBC from me, added beta eval, but needs a CR.

I've been working on CSP with new projects; excellent patch! +1 from me.
I verified that using JSON.parse doesn't impact the browser requirements for D8.
I also agree this is not an API change.

Settings were meant to be usable without drupal.js file, so I added a new file dealing with settings only instead of putting that in drupal.js Has to be weight -18 because object needs to exist for other JS.

I don't understand why the weight is necessary; everything that uses drupalSettings should declare a dependency on it, and that's already happening. So what subtle nasty problem is forcing us to specify this weight?

** I GIVE A WARNING HERE, PLEASE DON'T COMMIT IT QUICKLY **

Thanks!!!

@wim, weight needed for the same reason as #1945262: Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering is impossible right now, contextual links.

Ok. Could you add a comment to the YML file explaining the exact reasons for that weight? That'll make it much easier to remove weights in the future, if all things using weights actually explain the exact rationale.

#19: Should be the same cost overall, we are deprecating the use of IDs for selecting elements, so core should show a good example here. However in this case this element should be unique, so semantically ID would be correct.

#16: What is quickly? What is not? A week? A month? Can you elaborate your concerns some more?

I presume the ajax callbacks need some work here.

The reason that drupalSettings is there is that is is always available. This was a problem in past Drupal versions that it was not always or not early enough available in some situations. Now you remove this from the page and other scripts cannot rely on it as the misc/drupalSettings.js need to downloaded first before the window.drupalSettings get's set. This may block code execution.

At least it will block it for Google Analytics that runs the earliest possible and async. When GA code runs the object may not exists. No good idea to me.

Are you guys trying to destroy modules like Google Analytics? I get this fealing more and more.

I don't think this should be backported to D7. This could break all contrib js, or at lease most js that depends on Drupal.settings.

It would make all the js wait for Drupal.settings to be loaded, thus adding a dependency on the modules that wasn't there. This is a breaking api change for D7. However, I think a patch for D7 that someone could incorporate into a make file for CPS Drupal7 would be a good idea.

#23: Yes, we can backport that. Like the other changes it would be completely optional and opt-in via a variable, hence not BC breaking.

opt-in is good.

Made the selector a bit more explicit to avoid catching extra things.

@21, it was discussed a bit in an other issue (the one which removed inline js if I remember correctly), I'm not keen on making an API for core to create global variables all over the place. After all anything can be added to drupalSettings and mapped to whatever afterwards with module code.

@22 From what I saw on CSP we can have inline js but it's a bit more cumbersome to manage. Does not look impossible though: http://www.w3.org/TR/CSP/#source-list-valid-hashes. Since we know what the code of drupalSettings.js is, we know the hash and we can add it to the policy, keeping this piece of code inline, no d/l issue then.

pwolanin, nod_: nonce and hashes are part of CSP Level 2, which is not supported by IE at all (not even Edge). CSP2 is not even on their roadmap. So hashes are not likely to be usable for some time. I have a pull requset pending to get CSP2 information on caniuse.com.

haas: I understand your pain. I've spend the last several days trying to get CSP running on a project, and eliminate all inline code. It's a big task.
In my test implementation, GA works just fine with no inline code. You can still load ga.js with async. But you are correct that the calls to _gaq.push and friends would block waiting for drupalSettings.js to load. I'm not convinced this matters much though, in general this will be a very small delay, and Google now uses window.performance for measuring page timing, so the timing of the script execution shouldn't skew the measurements anyway. Worst case, if the ga.module authors found the small delay unacceptable, it would not be hard to work around – it would just means sites using ga.module cannot have CSP (which is something I think you should be concerned about, but it's your call of course).
In the next week I should have some real production data to show if there's truly any measurable effect on the data collected by Google Analytics. Here's the rough flow in my test:

<script src="//www.google-analytics.com/ga.js" async></script>
<script type="application/json" id="drupalSettings">
{"foo":"bar"}
</script>
<script src="drupalSettings.js"></script>
<script src="my_analytics.js"></script> <!-- contains _gaq.push() code -->

Long term, I think inline scripts are going to have to be deprecated, like inline CSS and the <font> tag. Drupal core should be leading this trend.

@Grendzy: I'm the GA maintainer. Push is deprecated. Universal Analytics is current.

See Optimizing the critical rendering path

And your external file solution requires 127.000.000.000 files on disk d.o... See the referenced case please.

Inline js/css is not deprecated. It is mobile first.

Thanks for the feedback - I admit I'm less familiar with Universal Analytics. Regarding the 127.000.000.000 file permutations, that is not something I would advocate. The strategy my project is using puts all the configuration data in drupalSettings, and then a loads a single static JS file whose behavior is governed by the contents of drupalSettings. The drupalSettings data is never stored on disk, and there's no constraints on how many permutations can be generated during page rendering.

In any case, I do agree #attached should allow inline code - I was previously unfamiliar with the issue of removal of that feature. In general I think Drupal should allow any valid HTML feature.

For me, this issue is about removing inline code from core – we cannot stop contrib from injecting inline code, but core should not require it, otherwise nobody can effectively use CSP.

It will be very interesting to see where the web ends up on this. People in the security community are working hard to eliminate unsafe inline code, but I can see the speed advantages. I'm curious, do you think HTTP/2 multiplexing will eliminate the performance cost of external scripts resources? My own perspective is a faster page load is not worth 47% of sites having XSS vulnerabilities (WhiteHat's number). Eventually either CSP2, or HTTP/2 will solve this problem, but in 2015 it's a tough choice.

Why not using the hash logic for all browsers except ie?

As long as settings are in an external file they will be added tooo late to the page and all slow down / block all async code.

In many cases drupalsettings cannot used any longer than an i need to reinvent/write my own drupalSettings api. That really suxxx.

Please do not commit this. Leave as is and use inline key in csp. Compatibility is here more importand than any non-existing issues you'd like to stop.

I'm not aware of a single inline js issue that required a drupal module security release. Can you share some examples where we had a security issues with inline js in past 5 years?

XSS is about a third of all D.O. security advisories – many, many hundreds over the years.
See: http://crackingdrupal.com/blog/greggles/what-kinds-security-problems-exi... (sorry I don't have more precise numbers).

Allowing Drupal's users to choose effective Content-Security-Policies, which block inline scripts, will mitigate the great majority of XSS vulnerabilities (although the security team would still issue advisories when XSS bugs are found).

Re #39 - xss is steady at about 50% of issues per year since 2006. See http://cardcorp.github.io/drupal-security-attacks/#/6 and http://cardcorp.github.io/drupal-security-attacks/#/7

I agree with the philosophy that core should be pushing for CSP by default and if a contrib chooses to break from that behavior that would be up to the contrib module.

#38, given all you need to do in your custom inline JS is to do:

var settingsElement = document.querySelector('script[type="application/json"][data-drupal-selector="drupal-settings-json"]');
var drupalSettings = JSON.parse(settingsElement.textContent);

So that does not look like a large blocker to me to get this issue in.

RTBC from me, however leaving at needs review as it is a potentially disruptive change.

What else do we need to be able to enable CSP in Core via a variable or setting?

A special header or such?

And how many of these xss issues are caused by inline js? Percentage values are useless.

Won't fix for this case!

#43: All XSS is Inline JS, if someone does insert as title <script>alert('xss');</script> and it is not sanitized the browser executes it.

That is the Cross-Site-Scripting (XSS) issue itself.

By preventing any untrusted Javascript, which includes all non-hashed inline JS, those issues are made pretty much impossible.

The drupalSettings is Drupal Core's last inline Javascript at this moment.

Moving this settings to external file has performance penalty. It also disallows developer from using drupal settings In many situations in future what is an absolute no-go. Additionally it does not stop xss as this can also happen with files using variables/query params/etc. drupal settings have never ever been used to inject any code. It is safe. This change is NOT mobile first.

#43: All XSS is Inline JS, if someone does insert as title <script>alert('xss');</script> and it is not sanitized the browser executes it.

Not necessarily. In most cases if you are allowed to insert <script>alert('xss');</script> you may be allowed to insert <script src='//maliciousite/file.js'></script> too.

CSP would help in these cases, because the Javascript files are only loaded from the same domain by default (can also be set restricted to a whitelist of domains). But then, AFAICS if attackers are able to upload files to that domain the issue would still persist (it would be mitigated, though).

Moving this settings to external file has performance penalty. It also disallows developer from using drupal settings In many situations in future what is an absolute no-go. Additionally it does not stop xss as this can also happen with files using variables/query params/etc. drupal settings have never ever been used to inject any code. It is safe. This change is NOT mobile first.

I think removing all inline Javascript from core is the way to go. I'm not sure I'm following, the current patch is not moving the settings to a separate file. Can you elaborate more why do you think these problems would exist?

CSP would help in these cases, because the Javascript files are only loaded from the same domain by default (can also be set restricted to a whitelist of domains). But then, AFAICS if attackers are able to upload files to that domain the issue would still persist (it would be mitigated, though).

An example of this is drupal.org site, where any registered user can upload files to that domain.

If someone is able to write <script src='https://www.drupal.org/files/issues/2510104-37.patch'></script> (and 2510104-37.patch contains alert('xss') for example) the JavaScript would run, even with CSP enabled.

If someone is able to write <script src='https://www.drupal.org/files/issues/2510104-37.patch'></script> (and 2510104-37.patch contains alert('xss') for example) the JavaScript would run, even with CSP enabled.

Actually, this can be prevented by adding "X-Content-Type-Options: nosniff" or "Content-disposition: attachment" HTTP response headers. I've just opened an issue for enabling this at Drupal.org #2513366: Add X-Content-Type-Options: nosniff HTTP response header.

That looks like a security improvement for Drupal core too, should I open an issue for it?

Oh I see, thanks for the heads up

Can you elaborate more why do you think these problems would exist?

See the presentation in #33.

The settings do not exists in the page with this page and block page loading until the external file is loaded. GA module as an example do not need to wait and can already run in these seconds. Every additional http request need to be avoided as dns/round trip takes time and slow down page rendering. And we have no behavior/event when the settings are loaded.

Every additional http request need to be avoided as dns/round trip takes time and slow down page rendering.

Can we measure the performance impact of this? It is true that if not using HTTP/2, an extra connection for core/misc/drupalSettings.js would be needed, but browsers should cache that file for subsequent requests.

By the way, as drupalSettings.js file does not contain the settings, is the name of the file confusing?

#51:

To be able to sue the settings earlier use:

var settingsElement = document.querySelector('script[type="application/json"][data-drupal-selector="drupal-settings-json"]');
var drupalSettings = JSON.parse(settingsElement.textContent);

The advantage compared to current core is that there are not even ordering issues, where you have to wait until settings have actually been executed, but they are immediately available to your code (once the DOM element was loaded).

Again:

This move _does not affect_ inline javascript. It just means inline JS needs to do the same work of parsing JSON to get the settings out, which is acceptable.

#52: Yes, lets use drupalSettingsLoader.js instead. CNW for that.

Performance won't be impacted as the drupalSettingsLoader.js is added to the aggregate as it is part of the drupalSettings library - that already exists.

It wouldn't help at all if contrib does inline script. and the result is:

- sites implement CSP will drop inline modules or patch the modules..etc (no much diff than now)
- modules with inline script may implement the same drupalSettings code again and again.
- any scripts required dynamic data would be another requests (either from custom data itself or adopt script) ( at least 3 for most sites, e.g., 1 comment system, 1 social widgets, 1 tracking code)

this is lose-lose.

There should be some mechanisms in API for adding these inline scripts to (ONE) external script file automatically. Ideally, in other hand Drupal should print drupalSetting function for modules' use inline JS API. And at the best case, we should also add nonce features.

** Either now and then, any scripts required drupalSettings + onDOMContentLoaded event or with adopt script for Drupal are big failure at beginning for performance.

#54: In my experience many contrib modules have followed core in best practice once core enforced that best-practice. (In this case: Security)

Yes it means little snippets you get from webpage X need more work, but we can likely find solutions for that.

- sites implement CSP will drop inline modules or patch the modules..etc (no much diff than now)

It was not possible to implement CSP so far as core needed to be patched. Therefore there is no data to that, also Drupal 8 is the first release to discourage inline Javascript usage. (to be discussed in the other issue).

- modules with inline script may implement the same drupalSettings code again and again.

Yes, that is fine. Or someone writes a module that those depend on to bring back the old code that they all can rely on. Contrib can do so, but it would impact their market share likely.

I (personally) would rather go with a module that is less feature-rich, but allows my site to stay secure than one that disables the security.

- any scripts required dynamic data would be another requests (either from custom data itself or adopt script) ( at least 3 for most sites, e.g., 1 comment system, 1 social widgets, 1 tracking code)

Well, there is sites out there (e.g. GitHub) that use those widgets, so surely that problem can't be that bad.

Using the libraries API we can automatically emit safe headers for external JS added via the proper API (e.g. libraries) :). How is that?

We can even detect all possible sub-directories and emit the top-level ones as allowed.

There should be some mechanisms in API for adding these inline scripts to (ONE) external script file automatically. Ideally, in other hand Drupal should print drupalSetting function for modules' use inline JS API. And at the best case, we should also add nonce features.

Lets discuss inline JS in the other issue. We are seriously thinking of bringing the flexibility it gives back, but with a way that is compatible with the advanced caching system Drupal 8 has.

** Either now and then, any scripts required drupalSettings + onDOMContentLoaded event or with adopt script for Drupal are big failure at beginning for performance.

The parseJSON won't be worse than executing the JS in the first place and if contrib coordinates on it then it is also just executed once. I can't see any performance nightmare coming up here. Can you elaborate some more where you see the problems?

we can't rename the library like that, It'll break all existing dependencies. Also the rename is not accurate, it doesn't load anything, just parses it. If there is something to rename it's the js file itself, not the library name.

settingsElement should always be there, because it's guaranteed to be added by using drupalSettings in the library def. Desn't hurt to make sure. We don't use sloppy equals (eslint error), need a !==.

Looks good to me.

Setting to RTBC to get core committer attention.

We also need a change record now.

Note to core committers:

This move is still a little controversial in the community, so please leave this RTBC for some time.

Changing to better status as it has only negative effects.

As there is an easy work-around as stated in #53 and #55, back to RTBC.

NW for change record.

Created https://www.drupal.org/node/2513818 as change record.

This move is still a little controversial in the community

Let us post it on a core group then, so we can get as much visibility as we can. NW for g.d.o post.

Uhm, lets keep at RTBC, we need some core maintainers looking this over.

Note to core committers:

This move is still a little controversial in the community, so please leave this RTBC for some time.

Edit:

Who can do such a post?

Anyone who is the member of https://groups.drupal.org/core. I don't get all the urgency of setting it to RTBC. There are many different ways to bring it to core committers attention. I'm still incline to change it to NR so that we can sort everything properly.

I like this personally, there's no API change for the vast majority of JavaScript, and the workaround in #53/#55 is straightforward for anything affected.

- how to add inline script right after drupalSettings while API has been removed ? #2391025: Add support for inline JS/CSS with #attached
- Patch #60 hasn't fix the script name.

No doubt about loss on performance of this patch. But technically we can solve by other way ( only if you won't followed Drupal way/best practice... )

** Either now and then, any scripts required drupalSettings + onDOMContentLoaded event or with adopt script for Drupal are big failure at beginning for performance.

As I mentioned before, take GA as example,

- it should drop any drupalSettings usage.
- drop jQuery.ready on http://cgit.drupalcode.org/google_analytics/tree/google_analytics.js#n7
- make it async loading.

#71: The change record has the information, it is also in #53 and #55, but again here:

Any inline script that runs during the page load (not after document.ready()) could use:

var settingsElement = document.querySelector('script[type="application/json"][data-drupal-selector="drupal-settings-json"]');
var drupalSettings = JSON.parse(settingsElement.textContent);

- Patch #60 uses drupalSettingsLoader.js. So I cannot see which rename you want?

google_analytics.js is fine as is, because by document.ready the drupalSettings are already long set?

Any initial script loading blocks the document.ready() event, so I am not sure what performance you want to gain / loose here?

- Patch #60 uses drupalSettingsLoader.js. So I cannot see which rename you want?

@see #58, #59.

Any inline script that runs during the page load (not after document.ready()) could use:

- Because you won't run inline script on DOMContentLoaded event [document.ready()], it won't work. Therefore, you should put your code right after drupalSettings data.
- If you put custom drupalSettings code after ALL external scripts, it will be a long wait for file downloads. (No one do it because drupalsetttings.js is available now)

Workaround for above problem:
A. drop usage of drupalSettings on your inline script. (broken Drupal best practice? If not, we can stop here, nothing worth to discuss here so..)
B. custom DOM element exist function to catch drupalSettings tag when it's available. (Not recommended)
C. add back inline API to make it possible to add inline script right after drupalSettings.

Any initial script loading blocks the document.ready() event, so I am not sure what performance you want to gain / loose here?

The first sentence answered your own question.
If Drupal assumed everything should be run inside/after Drupal.attachBehaviors, then I will keep silent.

After core commiters have seen this, we can now go back to 'needs review' and wait for the discussion on the g.d.o/core post.

Is a pretty reasonable answer - if you are writing a dynamic script element, why would you put the data your script needs into drupalSettings?

Because I have been told in the other inline case I should do all via settings? Something impossible, but I have been told this.

The first sentence answered your own question.
If Drupal assumed everything should be run inside/after Drupal.attachBehaviors, then I will keep silent.

Every 3rd party library does not know anything about Drupal and will not implement attachBehaviors. I have also been told from core team in a past discussion I should wait for dom ready and it is not worth the energy to implement attachBehaviors in GA.

We should also review JavaScript settings moved from Drupal.settings to global drupalSettings variable again as this documentation is no longer correct.

Practically this changes:

• In the HTML source, drupalSettings is printed before all other JS script on the page.
• drupalSettings is available right away, no need to wait for an attach behavior to be called.

That assumes you have no inline JS at all.

Fail. We will have inline JS as it is part of the html spec.

You suggested to use drupalSettings for some things and in some cases it may be used for, but only if it exists in the page content extreme early. Now you remove it and it can no longer used and we need to re-invent the wheel.

On the other side you suggested something for valid GA edge cases that don't work. As it is still valid to use drupalSettings before all files have been downloaded, you are breaking this now e.g. async code ($preprocess = FALSE) not combined with drupalSettings that tries to run before the aggregated monster JS has been downloaded and populated the drupalSettings object. Additionally you disallow inline_js what breaks more things, just because you think it is not best practice and all code need to wait for attachBehaviour or document.ready. There are things that cannot or must not wait for attachBehaviour and document.ready, but need the drupalSettings.

We always end in situations where something breaks that should not.

Now you remove it and it can no longer used and we need to re-invent the wheel.

This is absolutely untrue.

For what most be the TENTH time repeating this: any inline script can use

var settingsElement = document.querySelector('script[type="application/json"][data-drupal-selector="drupal-settings-json"]');
var drupalSettings = JSON.parse(settingsElement.textContent);

The only change is that inline JS needs to take care about reading drupalSettings on its own. Because it may run at any time, it needs to take care of that itself.

The only change is that inline JS needs to take care about reading drupalSettings on its own. Because it may run at any time, it needs to take care of that itself.

This will need to be documented for module maintainers that are porting modules from D7.
https://www.drupal.org/node/2274843 is a doc page on JS in Drupal8.

Okay let me re-phrase what I discussed with nod_: droplet is right there is a potential performance problem.

Lets analyze all cases for both before and after patch:

1. No Javascript files / No Javascript settings / Inlined CSS

• Before: DOMContentLoaded event immediately after all HTML has been loaded
• After: DOMContentLoaded event immediately after all HTML has been loaded

=> No change.

2. No Javascript files / Javascript settings / Inlined CSS

• Before: DOMContentLoaded event immediately after all HTML has been loaded and settings JS has been executed
• After: DOMContentLoaded needs to wait for the drupalSettingsLoader.js (which is in the footer) to load.

=> DomContentLoaded fires later - however the web page been painted already and the user can interact with it - as long a drupalSettingsLoader.js is in the footer.

=> No real change, because no-one reacts to DomContentLoaded.

3. No Javascript files / Inlined Javascript + Javascript settings / Inlined CSS

• Before: DOMContentLoaded event immediately after all HTML has been loaded and settings JS and inline JS has been executed
• After: DOMContentLoaded needs to wait for the drupalSettingsLoader.js to load, inlined JS that waits for DomContentLoaded is executed deferred.

=> DomContentLoaded fires later - an inline script that depends on DomContentLoaded is executed deferred. The page has been painted already and the user can interact with it.

4. Some Javascript files / Inlined Javascript + Javascript settings / Inlined CSS

• Before: DOMContentLoaded event needs to wait for all JS files + HTML to load.
• After: DOMContentLoaded event needs to wait for all JS files + HTML to load.

=> No change.

Analysis

In case 1 there is no change as the drupalSettingsLoader.js is only added when settings are present. This is not the case for anonymous users by default in Drupal.

In case 2 there is a theoretical change, but without a consumer of the DomContentLoaded event and with scripts in the footer, there is no practical impact of that as the page has already been painted when the script tag loading from an external file is seen. There are also no consumers of the drupalSettings.

In case 4 there is no impact at all, because once there are already scripts on the page one more to parse the settings does not change any performance.

Case 3 is where it gets interesting. For that there needs to be several pre-cursors:

• - The only JS added is the one that is inline.
• - That inline JS uses drupalSettings.

Therefore:

If you NEED to use inline JS AND your site does not have any other Javascript files AND your inline JS depends on the DomContentLoaded event, then don't use drupalSettings, but instead hardcode the choices directly in your inline Javascript.

@nod_: Is there any other use cases you can think of?

TL;DR:

In practice the move of drupalSettings from an inline JS to being available after inclusion via an external file should not make a difference perfomance wise. Inline Javascript can choose to avoid using drupalSettings in case there is no other Javascript files on the page.

To mitigate Case 3 I suggest we make the drupalSettings optionally appear in the head as inline JS. This shouldn't be an issue because this is to mitigate the problems that come from inline js needing drupalSettings.

Note that drupalSettings will automatically live in the header if and only if >=1 JS file that depends on it lives in the header.

@pwolanin, "secure by default" is not "secure all the time." If a developer puts their own inline js then CSP will fail so why force drupalSettings to be CSP compliant if the a contrib inline js is causing CSP to fail. This will enhance the developer experience.

Either way is fine, I just don't see the point in forcing all the developers that don't care (assumed by the use of inline js) to do the extra check if CSP is already broken by them using inline js in the first place. Core will still be able to implement CSP and contrib will always be able to break CSP if it isn't a requirement for the project at hand.

#90 We currently however do not support inline JS in the API. That ship has sailed for now. (same as we have a container and a symfony router and routes (instead of paths) and many other changes.)

Hence inline JS concerns should not block this.

If / When we bring back inline JS, we can talk about a way to put the settings inline, too and then disable CSP.

At the moment any support for the inline case would just be confusing, because we don't support it and inline JS also does not need drupalSettings.

I think @droplet or @nod_ need to RTBC this.

My warnings at #16 is unlocked, that was another issues. And my other comments on this topic was mainly talking about "inline JS API" really. Since inline JS API is not in CORE anymore, the changes of this patch has no significant difference on sites with aggregated JS enabled.

I think we still need to fix the script name, @see #58, 59.
- Patch #60 hasn't fix the script name.

@#86,
Case 3 & 4 are same cases.
Any inline scripts should not be deferred. So that if any inline script depends on DomContentLoaded, they should consider to move to external JS.

#94: Thank you so much for your review!

#60 uses drupalSettingsLoader.js as the name for the script. Is that correct or what should it be named?

Good with me, the js file is named drupalSettingsLoader.js and the library name doesn't change, no problems on my end.

#86 is a very good summary of what's going on.

Wouldn't it be nice to have a g.d.o post before the commit for more visibility?

#97: As discussed in IRC, we came to consensus here and got sign-off by both subsystem maintainers, so there is no more controversy on this part, so I don't think we need more discussions :).

I've read through all the issue comments and whilst I understand that @hass has concerns they have been mitigated by the work-around presented by @Wim Leers in #81. Allowing Drupal sites to use a CSP which prevents inline js is a big step forward and one that we should take.

Committed 94615a1 and pushed to 8.0.x. Thanks!

diff --git a/core/misc/drupalSettingsLoader.js b/core/misc/drupalSettingsLoader.js
index eb871d2..ab3911d 100644
--- a/core/misc/drupalSettingsLoader.js
+++ b/core/misc/drupalSettingsLoader.js
@@ -16,7 +16,7 @@
    *
    * @type {object}
    */
-  window.drupalSettings = {}
+  window.drupalSettings = {};
 
   if (settingsElement !== null) {
     window.drupalSettings = JSON.parse(settingsElement.textContent);

Fixed on commit.

So is the plan now to packport this to D7 as an optional setting?

#103: Yes, indeed.

We missed something #2532604: Fix reference to "var drupalSettings" in NoJavaScriptAnonymousTest

Actually postponed on #2538274: DbUpdateController has broken mainteneance mode logic, and update.php doesn't run due to aggregated JS assets then this could probably just revert the revert.

Exactly. This just happened to uncover a pre-existing bug, but given that bug would've been a critical blocker for the upcoming beta, we chose to revert this non-critical patch instead.

Would be funny to see a reverted revert in the commit logs :)

Will be neither the first nor last time that happens ;)

Reverted the revert.

:D

Per #103/#104.

Caused another critical problem: #2560521: Javascript does not work on beta 12 => HEAD update.php

So can we revert the revert of the revert? :P

Do we have tests for this?

Edit: I reread this and realized that it sounds snarky. It is not meant to be snarky. It is meant as a question with no snark.

Added #2592925: Harden drupalSettings selector against XSS when CSP is enabled for 8.0.x as a quick follow-up.

Hi @David_Rothstein

Do you think that This will break many custom modules?

I know .. It could do that

Not sure if that is backward compatible. or has a legacy options
Not sure if we should have the security kit module to do some work for us.

We need to test this first .. As if we update we may run into many issues with many modules.

I was running into some panel pages and panelizer, and IPE issues .. then landed on this issue.

Thanks.

Shouldn't break too many things. In D7 settings are ready just before the page is ready, a tiny amount of time before attachBehaviors is called. After that patch it would be the same.

Thanks Theodore,

A testing party will prove that.

I'm now concerned about that Drupal 7 website settings will be exposed to crowlers.

Is it that before we only read the data from the object?

Not sure if public websites should read the settings from other the Drupal.settings websites

Rewarded time :)

Hi

Drupal 7 backport attached.

Great that there's a working #D7 patch now. Just need tests and manual testing.

@RajabNatshah any suggestions on how to systematically test for contrib & custom modules with this in D7?

@mgifford I working on D7 tests modification for this issue

Partly tests added

d7 patch including tests

According to our current backport policy, a separate issue should be created for Drupal 7 and added to the related issues. Once that is done, this issue can be set to fixed. Thanks!

An attempt to fix failing tests for D7.

Here's the 7.x issue, moving this to fixed. #2783153: [D7] Convert drupalSettings from JavaScript to JSON, to allow for CSP in the future.