bloomreach / spa-sdk

Apache License 2.0
17 stars 14 forks source link

Security - Bump @xmldom/xmldom to patched 0.7.6 #2

Closed sforsberg closed 1 year ago

sforsberg commented 1 year ago

Resolves the following critical security vulnerability found in @xmldom/xmldom @ <0.7.6: https://github.com/advisories/GHSA-9pgh-qqpf-7wqj

npm audit

$ spa-sdk git:(main) npm audit
# npm audit report

@xmldom/xmldom  <0.7.6
Severity: critical
Improperly Controlled Modification of Object Prototype Attributes ('Prototype Pollution') in @xmldom/xmldom and xmldom - https://github.com/advisories/GHSA-9pgh-qqpf-7wqj
fix available via `npm audit fix --force`
Will install @xmldom/xmldom@0.7.6, which is outside the stated dependency range
node_modules/@xmldom/xmldom
...
beetlerom commented 1 year ago

Fixes https://issues.onehippo.com/browse/SPASDK-142 && https://issues.onehippo.com/browse/SECURITY-381 @hachok Can we make this part of 17.1.0?

hachok commented 1 year ago

Fixes https://issues.onehippo.com/browse/SPASDK-142 && https://issues.onehippo.com/browse/SECURITY-381 @hachok Can we make this part of 17.1.0?

Sure, I'll make this part of 17.1.0 or 17.1.1

DavidSint commented 1 year ago

Can this security patch be included in previous versions of the SDK e.g. v15, v16 etc.?

beetlerom commented 1 year ago

+1 for back porting since it's a critical issue. I also suggest latest 15, 16 & 17.

beetlerom commented 1 year ago

@DavidSint Is it an option to upgrade to v17? While there are breaking changes, they are literally just one-liners in the configuration object of the initialize function. I recommend to use the latest versions since they address other vulnerabilities as well.

DavidSint commented 1 year ago

@DavidSint Is it an option to upgrade to v17? While there are breaking changes, they are literally just one-liners in the configuration object of the initialize function. I recommend to use the latest versions since they address other vulnerabilities as well.

Maybe, but that's a separate question. I think this critical vulnerability should still be fixed in previous versions. I guess the only reason not to would be if bloomreach doesn't support previous versions?

beetlerom commented 1 year ago

In general, the idea of making security patches across multiple packages across multiple frameworks is quite costly testing wise, even with automation. For this reason we prefer not to patch older versions since we can use that time to address other issues.

For that reason, patching in v17 alone would be the best of both worlds.

If upgrading is totally not at option for you then we can consider patching this for you in an older version.

beetlerom commented 1 year ago

This vulnerability has been addressed in v17.1.0. We are still discussing backporting to v15 &16.

beetlerom commented 1 year ago

@DavidSint Have you had a chance to see if you can use v17? Thanks!

DavidSint commented 1 year ago

@DavidSint Have you had a chance to see if you can use v17? Thanks!

I haven't personally had the chance, no. Our BR colleague has unfortunately been ill the last few days. Out of interest are there any BR spa-sdk upgrade guides?

rutger-gerritsen commented 1 year ago

@beetlerom @xmldom/xmldom 0.7.6 also contains a critical security vulnerability └─ @xmldom/xmldom: 0.7.6 ├─ Issue: xmldom allows multiple root nodes in a DOM ├─ URL: https://github.com/advisories/GHSA-crh6-fp67-6883 ├─ Severity: critical ├─ Vulnerable Versions: <0.7.7 ├─ Patched Versions: >=0.7.7 ├─ Via: @bloomreach/spa-sdk, @bloomreach/react-sdk └─ Recommendation: Upgrade to version 0.7.7 or later

beetlerom commented 1 year ago

@DavidSint Let's move the conversation about upgrade to this issue, please. Which version are you currently using?

beetlerom commented 1 year ago

@rutger-gerritsen Thanks for reporting! Tracking this in this issue, we will have a fix available soon.

hachok commented 1 year ago

17.1.1 has been released. It fixes @xmldom/xmldom vulnerability issues(patched to 0.7.9). I'm closing this MR. Let me know if you still experience this issue.