SAP / spartacus

Spartacus is a lean, Angular-based JavaScript storefront for SAP Commerce Cloud that communicates exclusively through the Commerce REST API.
Apache License 2.0
732 stars 379 forks source link

[Master Ticket]Third Party Library Security Updates #12618

Closed giancorderoortiz closed 2 years ago

giancorderoortiz commented 3 years ago

Please update the following third party libraries as specified in each corresponding security ticket. (Only lists dependencies that require manual update. All others handed by dependabot chores) Applicable to 4.0 release.

Pending:

  1. trim -> See #12599
  2. lodash -> See #12600
  3. ecstatic -> #12605
  4. Glob-parent -> https://github.com/SAP/spartacus/issues/13025

Completed:

  1. ssri -> See #12597
  2. hosted-git-info -> See #12598
  3. socket.io -> See #12603
  4. axios -> See #12604
  5. ua-parser-js -> #12601
  6. is-svg -> #12602
  7. webpack-subresource-integrity -> #12607
  8. node-forge -> #12608
  9. xmlhttprequest-ssl -> #12609. *** CRITICAL
  10. Css-what -> https://github.com/SAP/spartacus/issues/13027 (risk accepted)
  11. object-path -> #12606
giancorderoortiz commented 3 years ago

@Xymmer , @zebercut, @hackergil per consultation with Michael @meeque

giancorderoortiz commented 3 years ago

Upgrade to angular12 may fix some of these. The dependency tree is quite complex for some of the libraries that require update.

giancorderoortiz commented 3 years ago

@Platonn master ticket for third party library updates.

giancorderoortiz commented 3 years ago

@meeque fyi

meeque commented 3 years ago

Hi all, today I've had the chance to discuss this topic with @Platonn. That helped me to understand these vulnerabilities better on a technical level. I think that I have overreacted in previous discussions that we had elsewhere. I now understand that all remaining dependabot alerts that are referenced in this issue affect devDependencies of the storefrontapp project only. And that the storefrontapp is not even published on npm.

Therefore I believe that it would be legitimate to accept the risk of the remaining HIGH and MEDIUM dependency vulnerabilities that are referenced in this issue. (The CRITICAL aka VERY HIGH ones have already been resolved.)

That said, I still believe that it would be best to address these vulnerabilities (and future ones) by upgrading to non-vulnerable library versions whenever possible. As far as I understand we haven't leveraged the following options yet:

  1. Use yarn upgrade or yarn upgrade-interactive to move to the latest library versions that are compatible with the version ranges currently specified in the pacakge.json. This would hopefully also upgrade some transitive dependencies to non-vulnerable versions.
  2. Add resolutions to package.json to lift transitive dependencies to non-vulnerable versions. This could help getting rid of vulnerable library versions even though they are required transitively.

There is a slight chance that this might break stuff, but imho it's worth a shot. If something breaks, build or tests will likely reveal that, and allow us to roll back specific changes.

meeque commented 3 years ago

Imho it would be great prevent similar problems in the future by running yarn upgrade as often as possible. E.g. by having a build job that would warn us when dependencies are outdated. (I think that might be possible with yarn upgrade --frozen-lockfile.)

As I've learned now, this might be less important for the storefrontapp module, but it might be a good idea for other modules, like @spartacus/core and many others. Most of those use peerDependencies with rather lax version requirements. That gives a lot of flexibility to Spartacus users, but it also burdens them with a lot of responsibility. They will frequently have to deal with vulnerable libs and lead the same discussions as here. But in their case, vulnerable libs will likely have direct impact on the security of their production systems.

I wonder, if it would be feasible to monitor the peerDependencies of all Spartacus modules, and bump up the version ranges whenever vulnerable library versions would be resolved (even transitively). We might want to restrict such upgrades to patch version or minor version changes, but we should probably avoid major version changes.

Could that help to keep Spartacus users safer? Or would it cause more problems than it would solve? Frankly, I have no idea. Any thougths?

giancorderoortiz commented 3 years ago

@Xymmer, @hackergil can the recommendations given by Michael be incorporated in our CI?

giancorderoortiz commented 3 years ago

Remaining open issues to be handled after 4.0 release.

Xymmer commented 2 years ago

Bill labeled open tickets as https://github.com/SAP/spartacus/labels/3rd-party-lib-updates for validation to do them as we move to jira