Implement SRI for aggregated assets

Needs a re-roll, change would have to happen in the *Lazy versions of those classes now.

Adding a reroll of #17.

Thanks, @AkashKumar07! 😊

Unfortunately, #18 has not yet been addressed. It's still happening only in the deprecated classes 😇

This also still needs test coverage and profiling (see #15).

Trait added, Changes done in Lazy versions of classes also.

Have not reviewed.

Moving to NW for the test coverage. But good to see all the current tests passed!

Done Apache benchmark testing on local.

Screenshots attached.
On 11.x without SRI:

With Sha-256

With Sha-384

With Sha-512

I don't think apache bench will give useful results here because the optimizer logic is cached per set of assets. Having said that sha384 and sha512 appear noticeably slower than HEAD and sha256 so if that's repeatable, then something might be getting picked up.

@catch should this be moved back to NW?

@smustgrave no I think we need better benchmarking - I would probably have used microtime() style benchmarking for this so it's possible to compare only the hashing logic and not the entire request.

Added microtime in patch for testing, and below are my findings:

sha512:
Logged-In, home page after cache clear:
CSS Execution Time: 0.00030803680419922 seconds
CSS Execution Time: 0.00040578842163086 seconds
JS Execution Time: 2.0980834960938E-5 seconds
JS Execution Time: 0.0010080337524414 seconds

Anonymous, home page after cache clear:
CSS Execution Time: 5.2928924560547E-5 seconds
CSS Execution Time: 0.00041604042053223 seconds
JS Execution Time: 0.00011801719665527 seconds

sha384
Logged-In, home page after cache clear:
CSS Execution Time: 0.00030779838562012 seconds
CSS Execution Time: 0.00040507316589355 seconds
JS Execution Time: 1.8119812011719E-5 seconds
JS Execution Time: 0.0010049343109131 seconds

Anonymous, home page after cache clear:
CSS Execution Time: 9.2029571533203E-5 seconds
CSS Execution Time: 0.00089097023010254 seconds
JS Execution Time: 0.00011897087097168 seconds

sha256:
Logged-In, home page after cache clear:
CSS Execution Time: 0.00047683715820312 seconds
CSS Execution Time: 0.00062894821166992 seconds
JS Execution Time: 2.0980834960938E-5 seconds
JS Execution Time: 0.0015959739685059 seconds

Anonymous, home page after cache clear:
CSS Execution Time: 7.2002410888672E-5 seconds
CSS Execution Time: 0.00062084197998047 seconds
JS Execution Time: 0.00017786026000977 seconds

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Test failure seems to be related to #31

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

I think 'seconds' in the patch should be 'microseconds'?

Avoided test bot for now.

Skimming the discussion, I'm not sure which result was giving multiple seconds. Do you mean the _really_ small results that are in scientific notation with E-5?

I'm curious how this is able to generate hashes of files that are lazily generated. Some more digging to understand the patch I guess unless someone has a hint.

Marking NW and adding needs tests back because there are not tests in the patch. We can continue discussing performance impact outside that fact.

Do you mean the _really_ small results that are in scientific notation with E-5?

Doh... yes, missed that the E-5...

Also #15 asked for the method to be in a trait, I agree that feels reasonable even though its not the rule of 3

1. +++ b/core/lib/Drupal/Core/Asset/CssCollectionOptimizer.php
   @@ -150,6 +152,8 @@ public function optimize(array $css_assets, array $libraries) {
   +              // Implement SRI.
   
   +++ b/core/lib/Drupal/Core/Asset/JsCollectionOptimizer.php
   @@ -149,6 +151,8 @@ public function optimize(array $js_assets, array $libraries) {
   +              // Implement SRI.
   

   🤓 Nit: I think these comments can be omitted now?

2. +++ b/core/tests/Drupal/FunctionalTests/Asset/AssetOptimizationTest.php
   @@ -337,4 +337,23 @@ protected function invalidExclude(string $url): string {
   +    $this->assertStringContainsString('integrity="sha512-', $page->getContent());
   

   Shouldn't we also test the correctness of the hash?

   If we get this wrong, the consequences could be pretty big? Browsers would REFUSE to execute the JS or apply the CSS! See https://developer.mozilla.org/en-US/docs/Web/Security/Subresource_Integr....

   … or I guess that would mean all of our functional JS tests would fail? 🤔

   Can we make the SRI attribute intentionally incorrect for a moment to check whether that would indeed those tests to fail? 🙏

+++ b/core/lib/Drupal/Core/Asset/AssetSetSriTrait.php
index ec3b805c971..006fe21e8ba 100644
--- a/core/lib/Drupal/Core/Asset/CssCollectionOptimizer.php

Not sure about adding this to the deprecated classes - I guess it doesn't hurt, but could also be left out.

If we get this wrong, the consequences could be pretty big? Browsers would REFUSE to execute the JS or apply the CSS! See https://developer.mozilla.org/en-US/docs/Web/Security/Subresource_Integr....

… or I guess that would mean all of our functional JS tests would fail?

I think we need to check what happens in this case:

🐛 Aggregated URL hashes for assets can mismatch due to different order of library assets Active

i.e. correct asset definitions, but a mis-matching hash, which is due to indeterminate asset order with the same sets of libraries in certain situations. Ideally we'd never, ever end up in that situation, but at the moment we do.

Addressed #42 and #41 except

Can we make the SRI attribute intentionally incorrect for a moment to check whether that would indeed those tests to fail?