Wp-rocket: Local cache - minified 3rd-party resources having the integrity attribute are blocked

[vmanthos]

Before submitting an issue please check that you’ve completed the following steps:

• Made sure you’re on the latest version ✅
• Used the search feature to ensure that the bug hasn’t been reported before ✅

Describe the bug

Since 3.7 WP Rocket saves locally 3rd-party JavaScript/CSS files.

If the original URL contains the integrity attribute, and CSS/JavaScript minification is enabled, this leads to that resource being blocked, since the file's hash doesn't match with what's saved in the integrity.

To Reproduce

Steps to reproduce the behavior:

1. Load Bootstrap's CSS/JavaScript files.
2. Enable CSS/JavaScript minification, NOT combination.
3. Check the JavaScript console for errors.

Expected behavior

The files should load fine when saved locally, despite the integrate attribute.

Approved ideas for solution:

• https://github.com/wp-media/wp-rocket/issues/3121#issuecomment-697616840
• https://github.com/wp-media/wp-rocket/issues/3121#issuecomment-697972206

Screenshots

Additional context

A few thoughts on this:

If there is an integrity attribute in the URL we can:

1. remove it when we add that file from the local cache or
2. calculate the hash of the local file and replace the value of the attribute.

To serve the original purpose of the integrity, 1 doesn't look right, and 2 would most likely require a check of the original file's integrity.

Backlog Grooming (for WP Media dev team use only)

• [x] Reproduce the problem
• [x] Identify the root cause
• [x] Scope a solution
• [x] Estimate the effort

[camilamadronero]

Related https://secure.helpscout.net/conversation/1278389385/193723?folderId=2675957

[arunbasillal]

Quite an interesting bug.

I believe integrity exists so that third parties do not change the file after it's added to many websites. When we add the file to local cache, the file can no longer be changed at the source. So technically integrity is no longer relevant.

So can we remove it? I am thinking we can. Because cache/min isn't cleared every 8 hours. It's only cleared when cache is manually cleared, or if CSS settings are adjusted. So if the file is safe on the uncached version, then it's okay on the cache as well without integrity.

However, users might not check this since they might only check the cached version after updating a CSS option or clearing the cache. So we do have the responsibility to check for this in a way I think.

What can we do? Not sure how feasible this is.

• Validate the integrity and remove the integrity if validation passes
• Do not store the file locally if validation fails.
• A different simpler approach is to not store the file locally at all if integrity is mentioned in the source code.

With the upcoming CSS related features for 3.8, all of this might no longer be relevant.

What do you think @GeekPress @Tabrisrp @vmanthos ?

[Tabrisrp]

Removing it might leave users with questions to support, asking why it has been removed, and why we are removing security checks.

The simpler approach right now, especially considering what is coming in 3.8, would be to add an additional check for the integrity attribute. If it's present, we exclude the file from the minification process.

[GeekPress]

Important note: Minify CSS option will be still here in 3.8.

[arunbasillal]

@GeekPress

The simpler approach right now, especially considering what is coming in 3.8, would be to add an additional check for the integrity attribute. If it's present, we exclude the file from the minification process.

What do you think about this? I think this is the safest and easiest approach for now. Wouldn't add much effort considering we are already packed for 3.8.

[GeekPress]

@arunbasillal Based on what I found below, the integrity attribute is useless once the CSS file is stored on our server:

integrity - defines the hash value of a resource (like a checksum) that has to be matched to make the browser execute it. The hash ensures that the file was unmodified and contains expected data. This way browser will not load different (e.g. malicious) resources. Imagine a situation in which your JavaScript files were hacked on the CDN, and there was no way of knowing it. The integrity attribute prevents loading content that does not match.

If I understand right, the integrity attribute is to be safe in case an external file has been hacked.

[arunbasillal]

@GeekPress You are exactly right on this. However the concern is three fold.

• Before removing the integrity, we need to confirm the integrity is right. (Reason added here: https://github.com/wp-media/wp-rocket/issues/3121#issuecomment-694915587)
• If we remove the integrity, as Remy mentioned some users might complain.
• With the updates we are planning for 3.8, putting in effort to fix this the hard way might be a waste of effort.

What do you think?

[vmanthos]

@arunbasillal

Just a small note. The integrity issue is there for JavaScript files too. So, regardless of the changes in 3.8 this will have to be tackled one way or the other.

[NataliaDrause]

JS minification related ticket: https://secure.helpscout.net/conversation/1285649081/195726/

[GeekPress]

Before removing the integrity, we need to confirm the integrity is right.

Do we have an idea if it's going to be time consuming to make the check?

If we remove the integrity, as Remy mentioned some users might complain.

The majority of our users aren't performance expert. I will be surprised to get a ton of questions about that.

With the updates we are planning for 3.8, putting in effort to fix this the hard way might be a waste of effort.

It seems we will still need that for JS minification.

@arunbasillal If calculating the hash is fast and not difficult, what could be the reason to not do it?

[WordPresseur]

CSS Minification related ticket: https://secure.helpscout.net/conversation/1288125313/196463?folderId=1213662

[arunbasillal]

@GeekPress calculating the hash should be easy, but doing that for a local file wouldn't make sense. Since the file is already local there is no need for the integrity attribute.

The other option is to remove the integrity variable completely. But this creates a problem:

• Suppose the original file was modified at the source and the expected data is different.
• Without WP Rocket and if the integrity attribute existed, browser wouldn't process the file.
• But if we remove the integrity value and store it locally, then browser will process the file and customer wouldn't know. Defeating the purpose of the integrity variable.

Since we have to do this for JS and CSS (even after 3.8), I think the ideal solution is:

• Validate the integrity and remove the integrity if validation passes.
• Do not store the file locally if validation fails.

[Tabrisrp]

To validate the integrity, we would have to do the following during the PHP processing:

• Check if the currently matched link/script tag has the integrity attribute
• Use the value to determine which algorithm was used to created the value (sha256/384/512)
• Use the same algorithm on the downloaded content to verify that the value we generated is the same as the one in the attribute

So this is doable and will have a small impact on performance during the optimization process.

[GeekPress]

@Tabrisrp Thanks for the details.

@arunbasillal Your plan makes total sense, let's go with it.

[DahmaniAdame]

Another related ticket - https://secure.helpscout.net/conversation/1291238332/197230

[Tabrisrp]

Reproduce the problem

Reproduced it using CSS with the integrity attribute

Identify the root cause

If the original URL contains the integrity attribute, and CSS/JavaScript minification is enabled, this leads to that resource being blocked, since the file's hash doesn't match with what's saved in the integrity.

Scope a solution

In the AssetsLocalCache class, add 2 new methods:

• has_integrity(): checks if the current processed CSS/JS tag has the integrity attribute. Returns boolean
• is_integrity_valid(): checks if the content integrity corresponds to the expected value. Use the integrity attribute value to determine which algorithm was used to created the hash (sha256/384/512) Use the same algorithm on the downloaded content to verify that the value we generated is the same as the one in the attribute. Returns boolean, false if the check failed, true if it passed.

If the integrity check fails, bail-out and don't process the file. If it's successful, remove the integrity attribute from the tag.

For the above, we will have to modify all the minify/combine CSS/JS classes.

Estimate the effort

Effort [S]

[vmanthos]

Related ticket: https://secure.helpscout.net/conversation/1311357434/203013/

[webtrainingwheels]

https://secure.helpscout.net/conversation/1312842290/203485?folderId=377611