Check SubResource Integrity

[hishamco]

Fixes #9941

[hishamco]

@agriffard @deanmarcussen what I notice that many files in my local don't have the same SRI, for example ~/OrchardCore.Resources/Scripts/js.cookie.js so when I download that file from CDN I got the same exact SRI

Could you please let me know if there's something I can do? That's why the Test fails in both my end and GitHub

[hishamco]

The following are the file that have the exact same SRI:

• ~/OrchardCore.Resources/Scripts/jquery-ui-i18n.min.js
• ~/OrchardCore.Resources/Scripts/vue-multiselect.min.js
• ~/OrchardCore.Resources/Scripts/vue-multiselect.min.js

[deanmarcussen]

I would suggest picking a specific file that doesn't match, and using the online https://www.srihash.org/ that I suspect @agriffard is using to test why one is not working, perhaps they are using different hashing methods.

[hishamco]

@agriffard are using SHA384 when you generate the hash in SRI? Or are you using another one?

[agriffard]

@agriffard are using SHA384 when you generate the hash in SRI? Or are you using another one?

Yes, I do. I always use https://www.srihash.org/ and the default encryption is SHA-384.

[hishamco]

@agriffard how can I ensure that all the resources in my side are up to date, so I can validate the SRI for them

[hishamco]

FYI I tried npm run but I'm getting an error

[hishamco]

@agriffard could you let me know what should I do here to make a successful restore for npm packages. I need to finalize this PR

[agriffard]

@agriffard could you let me know what should I do here to make a successful restore for npm packages. I need to finalize this PR

npm install

[hishamco]

[Skrypt]

@hishamco Update your node.js and/or npm to latest LTS version.

[hishamco]

I already updated Node JS, I need to check again, coz it success with UI Testing project .. I will have a try

[hishamco]

[Skrypt]

Once you installed the new node.js and npm try and do a cleanup of the node_modules folder. Then redo npm install.

[hishamco]

@Skrypt is package-lock.json play an important role, what I notice is when I download the exact file from the CDN the SRI validated successfully. This make me think that some of my local assest are not updated

I tried npm run rebuild with no luck :(

[Skrypt]

Sometimes I do a git clean -xdf to clean also the package-lock.json files and retrieve those from the repository. It can be a node_modules cache on your computer too. There are so many things that can go wrong in node.js dependency hell...

[hishamco]

@agriffard this PR is ready, I know the build will fail because I discover there are many invalid SRI, so I will create another PR to fix them. Once you merge the other PR we could rebase on main then the build will pass

[agriffard]

I cancelled the workflows, the build was running without progress for 1h40.

[hishamco]

That's why I didn't the build failed, I might need to check the logs

[hishamco]

Seems the resources take time to validate the SRI, which is weird!!

[Piedone]

Is this something you'd like to revisit any time soon @hishamco? Would be quite useful.

[hishamco]

Sure, there are many PRs on my list :)

[hishamco]

Seems the old issue in the testing has been resolved https://github.com/OrchardCMS/OrchardCore/pull/9947#issuecomment-1568750419

@Piedone I think we're ready to merge this now

[Piedone]

Let me know if you'd like a review here.

[hishamco]

@Piedone time to merge :)

[Piedone]

Please request review when you're done, because I just noticed this randomly.

[sebastienros]

Use the new static instances instead of creating new ones: https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.sha384.hashdata?view=net-8.0

[sebastienros]

I am right that these will run also locally? If so would you want to create an XUnit attribute to add on the methods which would skip the test if the "CI" environment variable is defined?

public class CiOnlyFactAttribute : FactAttribute
{
    public override string Skip
    {
        get
        {
            // "CI" is defined by GitHub actions
            // "BUILD_BUILDID" is defined by Azure DevOps

            if (Environment.GetEnvironmentVariable("BUILD_BUILDID") != null || 
                Environment.GetEnvironmentVariable("CI") != null)
            {
                return $"nameof(CiOnlyFactAttribute) tests are not run locally. To run them locally create a \"CI\" environment variable.";
            }

            return null!;
        }
    }
}

[hishamco]

We could do that, but I'm not sure do we still need Azure DevOps, while we're using GitHub actions?

[Piedone]

Why would we skip these in CI?