Implement SRI for aggregated assets

You've got my attention! 👀

should that be done for CSS as well?

crossorigin feels like scope creep though, and could be added in the library definition directly (or by default if we wanted to) (not for aggregated assets) I feel like the crossorigin config should be handled by modules such as CDN that allows files to be served from a different domain.

all right so crossorigin attribute is required when using SRI per: https://w3c.github.io/webappsec-subresource-integrity/#cross-origin-data...

The appropriate values for the attribute are "", "anonymous", "use-credentials": https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/crossorigin

So the patch in #9 which sets "anonymous" is correct and adding this attribute is not scope creep.

Putting as NW because we need to handle the CSS as well, and we'll need a couple of tests as well.

1. +++ b/core/lib/Drupal/Core/Asset/CssCollectionOptimizer.php
   @@ -160,6 +163,28 @@ public function optimize(array $css_assets) {
   +  protected function addSriArguments(array &$css_assets, string $data) {
   

   🤓 Nits:

   1. I think "arguments" was intended to be "attributes" here?
   2. I think we can add the void return type?

2. +++ b/core/lib/Drupal/Core/Asset/CssCollectionOptimizer.php
   @@ -160,6 +163,28 @@ public function optimize(array $css_assets) {
   +      $hash = base64_encode(hash('sha384', $data, TRUE));
   

   🤔

   1. Why sha384 and not sha256 or sha512?
   2. What's the performance impact? Is it measurable at all? (A simple test using ab would suffice IMHO.)

3. +++ b/core/lib/Drupal/Core/Asset/JsCollectionOptimizer.php
   @@ -159,6 +162,28 @@ public function optimize(array $js_assets) {
   +  /**
   +   * Implement SRI for a given group of JS assets.
   +   *
   +   * @param array $js_assets
   +   *   A group of JS assets.
   +   * @param string $data
   +   *   Content of the asset file.
   +   */
   +  protected function addSriArguments(array &$js_assets, string $data) {
   +    if (!isset($js_assets['attributes']['integrity'])) {
   +      $hash = base64_encode(hash('sha384', $data, TRUE));
   +      $attributes = [
   +        'integrity' => 'sha384-' . $hash,
   +        'crossorigin' => 'anonymous',
   +      ];
   +      if (!isset($js_assets['attributes'])) {
   +        $js_assets['attributes'] = [];
   +      }
   +      $js_assets['attributes'] = array_merge($attributes, $js_assets['attributes']);
   +    }
   +  }
   +
   

   🙏 This is 100% the same. Can we move this into a small trait that both classes reuse?

Tried to address point number 1 and 2.1 of comment #15, keeping the status needs work for point 2.2 on comment #15.

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.
Also strongest hash used.
I am not sure about 2.2 and how to write test to validate it. Any reference will be helpful.
Thanks

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'?

No that's wrong, it really is taking multiple seconds afaict, seems a bit much.

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:

#3397713: [Performance regression] Starting with Drupal 10.1, some sites hit PHP for every page view due to aggregated asset URL hash mismatch from different order of asset items

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?

I've applied the patch from comment #44 and everything works fine when a user is logged in, but any aggregated css/js is blocked for anonymous users.

Failed to find a valid digest in the 'integrity' attribute for resource 'XXX' with computed SHA-512 integrity 'XXX'. The resource has been blocked.

I found this issue after SRI has been flagged as a need by a PCI compliance audit for one of our sites.

Checked the original approach and it does not apply anymore, as expected because the old deprecated classes have been already removed.

However, I feel that we are in a kind of rabbithole here.

Per #1014086: Stampedes and cold cache performance issues with css/js aggregation, the aggregated assets are not being generated on the fly for very good reasons. However, for SRI we need the actual assets to calculate the hash and include it in the original HTML markup.

Do you have in mind any possible approach that could satisfy both the needs of #1014086: Stampedes and cold cache performance issues with css/js aggregation and allow to implement SRI in a generic way for Drupal sites?

https://developer.mozilla.org/en-US/docs/Web/Security/Defenses/Subresour... very specifically says that SRI is intended for files loaded from CDNs, are these audits really flagging it as a need for locally hosted assets? Seems contrary to the whole point since if your own server gets compromised then they can do a lot more than tamper with aggregated assets on the filesystem.