Moving synchronous inline JS into JSON with an external script load.

[jonathanKingston]

Removal of synchronous inline JavaScript script tags.

Description

This aids adoption of CSP by preventing inline JavaScript. However this new function shouldn't be used elsewhere and should be considered deprecated when adopted.

Along with #14339 and #14318 I was able to get through the cart process with a CSP without unsafe-inline which will prevent script injection.

Due to these global variables being loaded in many places, the refactoring to make them work with require are out of the scope of this patch, however it would be a good follow up bug to fix this too.

This still requires further work for CSP to be enabled everywhere however this will protect customers if no modules are used.

Fixed Issues (if relevant)

1. Inline JS on the frontend of the checkout

Manual testing scenarios

1. Use the checkout with with a CSP header of Header set Content-Security-Policy "default-src 'self' http:; script-src 'self' http: 'unsafe-eval'; style-src 'self' http: 'unsafe-inline'"

Contribution checklist

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds on Travis CI are green)

[shakyShane]

Please, more of this

[VladimirZaets]

Hello @jonathanKingston. We can't remove public and protected methods from classes that marked as API by backward compatibility reason. We should mark those methods as deprecated.
You can read more about our backward compatibility here
http://devdocs.magento.com/guides/v2.3/contributor-guide/backward-compatible-development/index.html

[jonathanKingston]

@VladimirZaets updated the code to mark the functions as deprecated instead of removing them.

[VladimirZaets]

Hi, @jonathanKingston , due to Magento backward-compatible guide we can't rename public methods.
Please create the new method.

[VladimirZaets]

Hi @jonathanKingston . We don't want to introduce new code that already is deprecated. (It's about outputLegacyJavaScript method and folder legacyinit at all.
Also, we have our mechanism for rewrite inline javascript. It's <script type="text/x-magento-init" /> tag that is valid Magento way.
Please use it and if you need some help we will happy to help you.

[VladimirZaets]

Hi @jonathanKingston, do you have any updates?

[jonathanKingston]

Please use it and if you need some help we will happy to help you.

This isn't possible to make backwards compatible at all. All the PRs I have submitted have been rejected over slight minor changes to internal functions which aren't ever documented.

However changing this to use init blocks as suggested would make the code async which would cause race conditions on documented functions and variables that are in the global scope.

Essentially the reason for this legacy code is to point at the fact this code needs to be rewritten in an async manner and ideally not exposed on the global namespace. This however isn't possible without breaking backwards compat.

I'm happy to change getGiftOptionsConfigJson as you mention it impacts some level of backwards compat, however the change you suggest won't work without risking breaking all magento installs that use this JavaScript.

[VladimirZaets]

Hi @jonathanKingston.

Essentially the reason for this legacy code is to point at the fact this code needs to be rewritten in an async manner and ideally not exposed on the global namespace. This however isn't possible without breaking backwards compat.

You are right, we have a lot of limitations regarding backward compatibility, that's the main reason why we still have a lot of legacies code. Due to it, we try to think through about code design and make large refactoring one time.

As to this issue, we already have the code design and our core team will make refactoring in the near future

[jonathanKingston]

@VladimirZaets I'm not entirely sure why this was closed on the decision that I created a legacy function to allow for backwards compat.

This code one of the last blockers e-commerce owners have from removing "unsafe-inline" from script-src in CSP. This vastly reduces the attack vectors of Magento (granted "unsafe-eval" is still needed which is also super bad, however each step helps lock down exploitation).

If the answer is that store owners need to refactor and move to PWA or Magento 3.0 to gain this security then this is a long life-cycle of abuse.

[VladimirZaets]

Hi @jonathanKingston.
You introduce new code that already is legacy, we try don't do that.
When I said "near future", I didn't mean PWA or Magento 3.0, that's mean 2.3 release I think.
We want to remove data from global variables at all and make asynchronous loading.
(If you want I can provide additional information about implementation)

In the current situation, I'll be ok if you move inline scripts to the separate file for ensuring the CSP.
But this should be natively and transparent, without "magic" outputLegacyJavaScript method.

We can use data attributes for pass the data to js file.
It's will looks something like:
<script src="filepath" data-config="<?= $block->method() ?>">

[VladimirZaets]

@jonathanKingston
We can discuss it in the Slack, I think it's will be more comfortable.