search

oqtane / oqtane.framework

CMS & Application Framework for Blazor & .NET MAUI
http://www.oqtane.org
MIT License
1.9k stars 547 forks source link

[BUG] Integrity error when loading external js resource #4812

Open mdmontesinos opened 2 weeks ago

mdmontesinos commented 2 weeks ago

Oqtane Info

Version - 5.2.4 Render Mode - Static Interactivity - Server Database - SQL Server

Describe the bug

Sometimes when loading a js external resource, it fails due to 'integrity' issues with the computed hash. For example, I'm using Swiper.js and including it from its cdn:

new Resource {
    ResourceType = ResourceType.Script,
    Url = "https://cdn.jsdelivr.net/npm/swiper@11/swiper-element-bundle.min.js",
    CrossOrigin = "anonymous",
    Location = ResourceLocation.Body,
    Reload = true
}

(Swiper js installation instructions do not include the integrity)

And I've got this several times, although it's inconsistent and I don't know yet the cause:

image

Also note that this happened both in an Azure deployment and local environment.

This results in failing to import the module:

image

Expected Behavior

Steps To Reproduce

Anything else?

sbwalker commented 2 weeks ago

@mdmontesinos your Resource declaration does not include the Integrity property, however this is a requirement for many CDNs. This is an example from the Oqtane Theme:

                new Resource { ResourceType = ResourceType.Stylesheet, Url = "https://cdnjs.cloudflare.com/ajax/libs/bootswatch/5.3.3/cyborg/bootstrap.min.css",
                    Integrity = "sha512-M+Wrv9LTvQe81gFD2ZE3xxPTN5V2n1iLCXsldIxXvfs6tP+6VihBCwCMBkkjkQUZVmEHBsowb9Vqsq1et1teEg==",
                    CrossOrigin = "anonymous" },
mdmontesinos commented 2 weeks ago

@sbwalker Yes, I know. As I mentioned earlier, Swiperjs does not provide an integrity hash in its installation guide, so I assumed it wasn't required and it had something to do with Oqtane js handling.

sbwalker commented 2 weeks ago

Are you using WebAssembly? If so, there is some documentation related to this topic:

https://learn.microsoft.com/en-us/aspnet/core/blazor/host-and-deploy/webassembly?view=aspnetcore-8.0#resolve-integrity-check-failures

mdmontesinos commented 2 weeks ago

No, it's static render mode, with server interactivity (although I've gotten the error with all static modules and anonymous user)

sbwalker commented 2 weeks ago

I don't think this would be related to Oqtane... Oqtane simply creates the script tag and puts it into the page output - the browser interprets the script tag and does the validation of the remote resource

mdmontesinos commented 2 weeks ago

Thanks for the feedback, I'll keep investigating on my side.

mdmontesinos commented 2 weeks ago

I ended up locking to a specific version of the library (i.e. 14.1.1 instead of generic 14 that could change without me noticing) so that I can manually compute the integrity hash and add it to the resource declaration. The error doesn't seem to appear anymore.

mdmontesinos commented 2 weeks ago

@sbwalker Sorry for reopening the issue, but actually the error has not disappeared. I actually believe it's related to the "Reload" property of the resources and PageScript, because it generates a preload link.

When first loading the page where the script is requested, it works fine and the integrity error does not show up. However, if the first load is in a different page and I then navigate to the page (with enhanced-navigation), the error appears.

Also, I have several resources from cdn with the integrity value, but only some of them fail. And even sometimes one fails and the others not, and another time, they change.

This is an example of a script that fails when enh-navigating:

Resource declaration:

new Resource {
    ResourceType = ResourceType.Script,
    Url = "https://unpkg.com/imagesloaded@5.0.0/imagesloaded.pkgd.min.js",
    CrossOrigin = "anonymous",
    Integrity = "sha384-e3sbGkYzJZpi7OdZc2eUoj7saI8K/Qbn+kPTdWyUQloiKIc9HRH4RUWFVxTonzTg",
    Location = ResourceLocation.Body,
    Reload = true
},

Page output:

<link rel="modulepreload" href="https://unpkg.com/imagesloaded@5.0.0/imagesloaded.pkgd.min.js" integrity="sha384-e3sbGkYzJZpi7OdZc2eUoj7saI8K/Qbn+kPTdWyUQloiKIc9HRH4RUWFVxTonzTg" crossorigin="anonymous">
<page-script src="https://unpkg.com/imagesloaded@5.0.0/imagesloaded.pkgd.min.js"></page-script>

Error:

Failed to find a valid digest in the 'integrity' attribute for resource 'https://unpkg.com/imagesloaded@5.0.0/imagesloaded.pkgd.min.js' with computed SHA-384 integrity 'e3sbGkYzJZpi7OdZc2eUoj7saI8K/Qbn+kPTdWyUQloiKIc9HRH4RUWFVxTonzTg'. The resource has been blocked.

I'm using https://www.srihash.org/ to compute the hash.

sbwalker commented 2 weeks ago

Oqtane is specifying the integrity and crossorigin in the exact format documented by Microsoft:

https://github.com/MackinnonBuck/blazor-page-script/commit/44ad6833b8375a9eaa9b0fa9ace70e024b823f2d

I think this will need to be escalated to Mackinnon Buck.

sbwalker commented 2 weeks ago

One other item to mention is that not all JavaScript libraries need to be reloaded on every page navigation. Generally they only need to be reloaded if they contain "onload" logic. If you look at the Arsha theme it only specifies Reload for the custom scripts which are part of theme (as they use "onload") - whereas all of the other references to JavaScript libraries (ie. Swiper) do not specify Reload.

mdmontesinos commented 2 weeks ago

Usually for me, when I don't specifiy Reload, most libraries won't work properly, and it's also hard to determine when one does theoretically require it.

I'll try to get a minimal repo of the integrity issue for Mackinnon Buck.

mdmontesinos commented 2 weeks ago

page-script was actually made obsolete by the new BlazorJsComponents, which I believe were presented in the TrailBlazor conference.

mdmontesinos commented 2 weeks ago

Oqtane is specifying the integrity and crossorigin in the exact format documented by Microsoft:

MackinnonBuck/blazor-page-script@44ad683

I think this will need to be escalated to Mackinnon Buck.

And this change was never integrated into the main branch and released in his repo, so perhaps it wasn't properly tested, and I have no way to create a minimal repo without using Oqtane.

mdmontesinos commented 2 weeks ago

I wasn't able to reproduce with a regular Blazor app (no interactivity) using Mackinnon Buck's PageScript cloned from the branch that contains the integrity and crossorigin.

Therefore, it seems to be specific to Oqtane in some way. To clarify the issue and reproduce:

As a current workaround, you must remove both CrossOrigin and Integrity from the resource. If any of them are not empty, page-script will try to add the link with modulepreload and it will fail. Of course, not having a crossorigin and integrity for external cdn resources is a security risk and vulnerability, so this should be fixed.

In my original comment, the resource for swiper contained the CrossOrigin property but not the integrity, which is why it was failing.

new Resource {
    ResourceType = ResourceType.Script,
    Url = "https://cdn.jsdelivr.net/npm/swiper@11/swiper-element-bundle.min.js",
    //CrossOrigin = "anonymous",
    Location = ResourceLocation.Body,
    Reload = true
}

One other item to mention is that not all JavaScript libraries need to be reloaded on every page navigation. Generally they only need to be reloaded if they contain "onload" logic. If you look at the Arsha theme it only specifies Reload for the custom scripts which are part of theme (as they use "onload") - whereas all of the other references to JavaScript libraries (ie. Swiper) do not specify Reload.

This is only true for resources defined on the theme, as they are always added on the first page load (i.e. bootstrap). However, for resources defined in modules (such as swiper for a oqtane slider module), they might get added for the first time after an enhanced-navigation, which is why they all require the Reload property.

zyhfish commented 2 weeks ago

hmm, I'm not able to reproduce it with the exact steps: image image

sbwalker commented 2 weeks ago

@zyhfish this looks strange:

image

zyhfish commented 2 weeks ago

this is because only the integrity property value is empty: image

sbwalker commented 2 weeks ago

@mdmontesinos indicated that he specified both the integrity and crossorigin properties:

"To clarify the issue and reproduce:

Create a module (not theme) and declare a js resource with integrity and crossorigin properties, and Reload=true (i.e.):

new Resource {
    ResourceType = ResourceType.Script,
    Url = "https://unpkg.com/imagesloaded@5.0.0/imagesloaded.pkgd.min.js",
    CrossOrigin = "anonymous",
    Integrity = "sha384-e3sbGkYzJZpi7OdZc2eUoj7saI8K/Qbn+kPTdWyUQloiKIc9HRH4RUWFVxTonzTg",
    Location = ResourceLocation.Body,
    Reload = true
},
zyhfish commented 2 weeks ago

there also have a content in @mdmontesinos 's comment: "As a current workaround, you must remove both CrossOrigin and Integrity from the resource."

mdmontesinos commented 2 weeks ago

there also have a content in @mdmontesinos 's comment: "As a current workaround, you must remove both CrossOrigin and Integrity from the resource."

This seems to work, but external resources should always include crossOrigin and integrity due to security concerns.

Anyway, I'll try to create a minimal repo for a module that reproduces the issue.

mdmontesinos commented 1 week ago

@sbwalker @zyhfish I just created a minimal repo for a module which reproduces the issue.

https://github.com/mdmontesinos/mdmontesinos.Module.Issue4812

sbwalker commented 1 week ago

I can't reproduce...

image

mdmontesinos commented 1 week ago

Check the browser's console logs. It actually seems you are actually reproducing the error, as the masonry grid is not aligned as it should, it's behaving as a regular grid.

sbwalker commented 1 week ago

No errors in the console:

image

and I see the elements in the page source:

image

mdmontesinos commented 1 week ago

Are you doing a hard-refresh and clearing cache on a different page, and then navigating to the page that contains the module?

If so, then I don't know why it behaves differently for you or how to even debug it on my side.

sbwalker commented 1 week ago

I followed your steps to repro:

The steps do not say anything about hard refresh or clearing the cache

mdmontesinos commented 1 week ago

Yes, navigating in incognito should reproduce it as well. Then, I don't know what the problem is.

zyhfish commented 8 hours ago

when I'm trying to follow the steps, I saw error in console: image

it looks like a race condition issue, the module js is executed before the library scripts loaded.

sbwalker commented 7 hours ago

@zyhfish I was not able to reproduce... however if it is a race condition based on JavaScript library dependencies then I would investigate how the JavaScript is being declared and utilized...

https://github.com/mdmontesinos/mdmontesinos.Module.Issue4812/blob/master/Client/Modules/mdmontesinos.Module.Issue4812/ModuleInfo.cs

I do not understand why the references to masonry.pkgd.min.js and imagesloaded.pkgd.min.js are declared for injection in the Body and have Reload set to True. Reload should only be set to True if the library contains 'onload' events which need to be executed on page navigations. I would try loading these 2 scripts in the Head and setting Reload = false.

https://github.com/mdmontesinos/mdmontesinos.Module.Issue4812/blob/master/Server/wwwroot/Modules/mdmontesinos.Module.Issue4812/Module.js

not sure why this is using a setTimeout of 500 milliseconds?

mdmontesinos commented 4 hours ago

@sbwalker The resources are declared for injection in the Body due to performance reasons. If a js script is placed in the Head, it would delay the first render of the page, especially when loading multiple external and "big" resources.

As for the 500 milliseconds setTimeout, it was a way to wait for the external resources to load, as they are required in the custom script. I didn't bother to add more advances ways to achieve this in the minimal repo.

About the Reload property, I wasn't able to use external scripts yet if they are loaded after an enhanced navigation (not in the first page load), if Reload is not true.

@zyhfish The race condition is not the issue I was referring to, it can be solved with a polling and promise mechanism to ensure the Masonry object is defined befored executing the custom script. Again, I didn't want to add more complexity to the minimal repo.

sbwalker commented 3 hours ago

@mdmontesinos I tried out your repro module again... however I made the changes that i described above (ie. I loaded the external scripts in the Head and set Reload to false):

            Resources = [
                new Resource {
                    ResourceType = ResourceType.Script,
                    Url = "https://cdn.jsdelivr.net/npm/masonry-layout@4.2.2/dist/masonry.pkgd.min.js",
                    CrossOrigin = "anonymous",
                    Integrity = "sha384-GNFwBvfVxBkLMJpYMOABq3c+d3KnQxudP/mGPkzpZSTYykLBNsZEnG2D9G/X/+7D",
                    Location = ResourceLocation.Head,
                    Reload = false
                },
                new Resource {
                    ResourceType = ResourceType.Script,
                    Url = "https://unpkg.com/imagesloaded@5.0.0/imagesloaded.pkgd.min.js",
                    CrossOrigin = "anonymous",
                    Integrity = "sha384-e3sbGkYzJZpi7OdZc2eUoj7saI8K/Qbn+kPTdWyUQloiKIc9HRH4RUWFVxTonzTg",
                    Location = ResourceLocation.Head,
                    Reload = false
                },
                new Resource {
                    ES6Module = true,
                    ResourceType = ResourceType.Script,
                    Location = ResourceLocation.Body,
                    Url = "~/Module.js",
                    Reload = true,
                }
            ]

And the result seems to work fine:

image

You can clearly see that the external JavaScript references were included in the head including the integrity and crossorigin attributes... and the module.js was included in the body using the custom page-script element so that it is executed on every page transition.