Skip to content

Conversation

@jgeurts
Copy link

@jgeurts jgeurts commented Oct 17, 2020

Pretty much all dependencies are very out of date. There are a few deprecation messages and security concerns. This expands on #211 to update versions of all npms.

This also sets the minimum node version to the oldest current LTS version.

* Set minimum node version to oldest LTS
@jgeurts
Copy link
Author

jgeurts commented Oct 17, 2020

@prime-time please let me know if I can do anything else to get this merged and a new npm version published!

@ghost
Copy link

ghost commented Oct 27, 2020

This has just become very relevant since a security advisory has been created for xml-crypto < 2.0.0

@jgeurts
Copy link
Author

jgeurts commented Oct 27, 2020

@mcab Can you please let me know what needs to happen to get this merged?

@mcab
Copy link
Member

mcab commented Oct 27, 2020

Hey there! Thanks for the PR.

Bumping all of these packages at once would be nice, but we want to guarantee the library operates correctly post-updates. I'm interested in figuring out why all of them must be updated at once. Can we work on addressing critical issues first?

From npm audit:

❯ npm audit

                       === npm audit security report ===

# Run  npm install --save-dev mocha@8.2.0  to resolve 3 vulnerabilities
SEMVER WARNING: Recommended action is a potentially breaking change
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Critical      │ Command Injection                                            │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ growl                                                        │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ mocha [dev]                                                  │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ mocha > growl                                                │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://npmjs.com/advisories/146                             │
└───────────────┴──────────────────────────────────────────────────────────────┘


┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Low           │ Regular Expression Denial of Service                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ debug                                                        │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ mocha [dev]                                                  │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ mocha > debug                                                │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://npmjs.com/advisories/534                             │
└───────────────┴──────────────────────────────────────────────────────────────┘


┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Low           │ Prototype Pollution                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ minimist                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ mocha [dev]                                                  │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ mocha > mkdirp > minimist                                    │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://npmjs.com/advisories/1179                            │
└───────────────┴──────────────────────────────────────────────────────────────┘

mocha is the main test runner for this repo. Bumping up to version ^8.2.0 without testing if the tests still work results in the test suite no longer working:

> make test
✖ ERROR: --compilers is DEPRECATED and no longer supported.
          See https://git.io/vdcSr for migration information.
make: *** [test] Error 1

Similarly, bumping up async, debug, and xml-crypto up major versions might cause unforeseen consequences.


From installing the package:

❯ npm i saml2-js
npm WARN deprecated lodash-node@2.4.1: This package is discontinued. Use lodash@^4.0.0.
npm WARN notsup Unsupported engine for xmlbuilder@2.2.1: wanted: {"node":"0.8.x || 0.10.x"} (current: {"node":"12.14.1","npm":"6.14.8"})
npm WARN notsup Not compatible with your version of node/npm: xmlbuilder@2.2.1

Primarily, it's from xmlbuilder. To my knowledge, https://github.com/oozcitak/xmlbuilder-js has been replaced by https://github.com/oozcitak/xmlbuilder2, but xmlbuilder2 has been "redesigned from the ground up," which makes me want to triple check on how we call and use the library.


With that being said, I'd be happy to take a closer look to validate there's no major breakage, but that will take a few days of work.

@jgeurts
Copy link
Author

jgeurts commented Oct 28, 2020

Thanks for taking a look! Apologies about the issues with Mocha. I pushed another commit that gets mocha running with the latest version. I also reverted upgrading to xmlbuilder2 for now, since that could probably be handled at a later time in a different PR.

After reverting back to xmlbuilder, I tested each npm package update and they all passed the tests. Until I got to xml-crypto.

Unfortunately, the changes to xml-crypto seem to break a number of tests and it wasn't readily apparent to me why or what the upgrade process should be for that library. The syntax used for xpath() and SignedXml seemed to still be supported.

@mcab
Copy link
Member

mcab commented Feb 4, 2021

Hi there,

Ended up using this as the basis for #227. We'll update xml-crypto in #215, so this becomes no-op for now. Thanks for the initial push to get this library updated!

@mcab mcab closed this Feb 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants