Check SubResource Integrity #9947

hishamco · 2021-07-21T21:04:30Z

Fixes #9941
Fixes #11915

hishamco · 2021-07-21T22:31:14Z

@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 · 2021-07-21T22:36:39Z

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 · 2021-08-29T07:45:32Z

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 · 2021-08-29T13:07:30Z

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

agriffard · 2021-08-29T13:32:54Z

@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 · 2021-08-29T14:36:03Z

@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 · 2021-08-29T16:25:42Z

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

hishamco · 2021-09-26T11:00:13Z

@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 · 2021-09-27T21:07:33Z

@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 · 2021-09-29T09:54:43Z

Skrypt · 2021-10-05T14:02:50Z

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

hishamco · 2021-10-05T14:34:45Z

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

hishamco · 2021-10-05T15:13:27Z

Skrypt · 2021-10-05T16:47:05Z

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

hishamco · 2021-10-16T21:55:33Z

@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 · 2021-10-17T21:16:30Z

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 · 2023-05-30T13:30:40Z

@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 · 2023-05-30T15:13:29Z

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

hishamco · 2023-05-30T15:30:59Z

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

hishamco · 2023-05-30T16:42:59Z

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

Piedone · 2024-01-16T01:24:12Z

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

hishamco · 2024-01-16T08:03:03Z

Sure, there are many PRs on my list :)

Piedone · 2024-02-04T21:54:22Z

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

Piedone

See #9947 (comment)

hishamco · 2024-02-26T07:05:46Z

@Piedone time to merge :)

Piedone · 2024-02-26T12:52:14Z

test/OrchardCore.Tests/Modules/OrchardCore.Resources/SubResourceIntegrityTests.cs

@@ -6,6 +6,8 @@ namespace OrchardCore.Tests.Modules.OrchardCore.Resources;

 public class SubResourceIntegrityTests
 {
+ private static readonly HttpClient _httpClient = new();


You also need to dispose this. It doesn't actually need to be static, you can just add a using var into the test method.

You also need to dispose this

Agree

It doesn't actually need to be static

The GetSubResourceIntegrityAsync() is static

And you can change that, because you have the infinite power of Visual Studio at your hand :). However, I'm not arguing for that. You can just pass HttpClient into the method. The latest implementation of disposing a static field from an instance method is a wrong approach to this.

And you can change that, because you have the infinite power of Visual Studio at your hand

Sure :)

I committed what I think is simpler, please check.

Piedone · 2024-03-01T19:15:42Z

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

hishamco · 2024-03-01T19:59:51Z

test/OrchardCore.Tests/Modules/OrchardCore.Resources/SubResourceIntegrityTests.cs

 // Assert
 var resourceManifest = resourceManagementOptions.ResourceManifests.First();

+ using var httpClient = new HttpClient();


@Piedone If you remember you told me to use the same instance of HttpClient which you insisted a lot before, now I see you are a new instance :)

This is still one instance during the lifetime of the test, but we don't go the roundabout way of passing a field to a local function.

No problem, I think we are fine now for merge this, right?

If you're OK with it then yes.

sebastienros · 2024-03-05T20:29:21Z

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 · 2024-03-06T01:15:45Z

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 · 2024-03-06T07:58:39Z

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

Piedone · 2024-03-06T18:21:55Z

Why would we skip these in CI?

sebastienros · 2024-03-06T19:37:21Z

The opposite, skip these locally but not in CI. My wording was wrong, the code is correct ;)

hishamco · 2024-03-08T02:17:11Z

I thought what the Seb meant at the first glance 😂, I will submit a PR

hishamco · 2024-03-11T14:22:17Z

The opposite, skip these locally but not in CI. My wording was wrong, the code is correct ;)

Seb seems the code is wrong, because the fact should be null when the environment valriable exists not the opposite :)

Co-authored-by: Zoltán Lehóczky <zoltan.lehoczky@lombiq.com>

Check SubResourceIntegrity

fdd8191

Check Url & UrlDebug

770ae47

agriffard mentioned this pull request Jul 30, 2021

js-cookie 3.0.1 #10004

Merged

hishamco changed the title ~~Check SubResourceIntegrity~~ Check SubResource Integrity Oct 5, 2021

hishamco added 2 commits May 29, 2023 15:16

Merge remote-tracking branch 'origin/main' into hishamco/#9941

66b1814

Fix the SRI test

c162034

hishamco added the ready label May 30, 2023

Merge branch 'main' into hishamco/#9941

6a91a1f

hishamco requested a review from Piedone February 4, 2024 22:39

Piedone requested changes Feb 4, 2024

View reviewed changes

Merge branch 'main' into hishamco/#9941

d1bcf61

hishamco requested a review from Piedone February 23, 2024 23:08

hishamco added 2 commits February 26, 2024 09:57

Make HttpClient static

988ac51

Merge branch 'main' into hishamco/#9941

974e536

Piedone requested changes Feb 26, 2024

View reviewed changes

hishamco added 3 commits February 26, 2024 16:06

Merge branch 'main' into hishamco/#9941

ba79174

Implement IDisposable

b95c135

Address feedback

b18aa14

Simplifying HttpClient management

ed151a9

hishamco commented Mar 1, 2024

View reviewed changes

Piedone approved these changes Mar 1, 2024

View reviewed changes

Merge branch 'main' into hishamco/#9941

f4c5e07

hishamco merged commit 4847f89 into main Mar 1, 2024
5 checks passed

hishamco deleted the hishamco/#9941 branch March 1, 2024 21:13

hishamco mentioned this pull request Mar 5, 2024

Use SHA384.HashDataAsync #15461

Merged

hishamco mentioned this pull request Mar 11, 2024

Don't run SRI locally be default #15486

Merged

urbanit pushed a commit to urbanit/OrchardCore that referenced this pull request Mar 18, 2024

Check SubResource Integrity (OrchardCMS#9947)

93a646c

Co-authored-by: Zoltán Lehóczky <zoltan.lehoczky@lombiq.com>

Piedone mentioned this pull request Apr 25, 2024

Check resource definition checksum as part of the CI build #11915

Closed

Check SubResource Integrity #9947

Check SubResource Integrity #9947

Conversation

hishamco commented Jul 21, 2021 • edited by Piedone Loading

hishamco commented Jul 21, 2021

hishamco commented Jul 21, 2021

deanmarcussen commented Aug 29, 2021

hishamco commented Aug 29, 2021

agriffard commented Aug 29, 2021

hishamco commented Aug 29, 2021

hishamco commented Aug 29, 2021

hishamco commented Sep 26, 2021

agriffard commented Sep 27, 2021

hishamco commented Sep 29, 2021

Skrypt commented Oct 5, 2021

hishamco commented Oct 5, 2021

hishamco commented Oct 5, 2021

Skrypt commented Oct 5, 2021

hishamco commented Oct 16, 2021

Skrypt commented Oct 17, 2021

hishamco commented May 30, 2023

agriffard commented May 30, 2023 • edited Loading

hishamco commented May 30, 2023

hishamco commented May 30, 2023

Piedone commented Jan 16, 2024

hishamco commented Jan 16, 2024

Piedone commented Feb 4, 2024

Piedone left a comment

Choose a reason for hiding this comment

hishamco commented Feb 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Piedone commented Mar 1, 2024

hishamco Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastienros commented Mar 5, 2024

sebastienros commented Mar 6, 2024

hishamco commented Mar 6, 2024 • edited Loading

Piedone commented Mar 6, 2024

sebastienros commented Mar 6, 2024

hishamco commented Mar 8, 2024

hishamco commented Mar 11, 2024

hishamco commented Jul 21, 2021 •

edited by Piedone

Loading

agriffard commented May 30, 2023 •

edited

Loading

hishamco Mar 1, 2024 •

edited

Loading

hishamco commented Mar 6, 2024 •

edited

Loading