Skip to content

Conversation

@leMaik
Copy link
Contributor

@leMaik leMaik commented Sep 25, 2024

I'm currently using a fork of this library, so I guess it's time to contribute my changes back to upstream. 🙃

  • createCipher and createDecipher are deprecated in Node.js and cause warnings. This resolves the warnings without changing the logic. Changing this to the more secure createCipheriv and createDecipheriv would require storing IVs and break existing key files, so I kept it as is for now (it was changed to a wrong implementation in an earlier version of this PR)
  • xml-crypto 4.x has a critical vulnerability, so I updated to 6.x. This in turn requires Node.js 16 or later, so I bumped the minimum version of this library in the readme (given that the oldest still supported Node.js version is Node.js 18, I hope that this is fine; if we update to Node.js >= 21, we could even get rid of rock-req and use the built-in fetch instead).
  • The initialization process involved a lot of trial-and-error for me. I extended the README to include my findings, and help new users setup their EBICS accounts.

@leMaik
Copy link
Contributor Author

leMaik commented Sep 25, 2024

Node.js 19 EOL'd over a year ago and doesn't even know the CVE that is causing problems. I'll remove it from the GitHub workflow and add Node.js 22 (upcoming LTS) instead. 18/20/22 are the currently supported versions.

Edit: I have no idea why npm ci fails for Node.js 22. It works fine locally.

@leMaik leMaik changed the title Improve readme, update deprecated APIs and xml-crypto. Improve readme, update xml-crypto, fix ci. Sep 25, 2024
@nanov
Copy link
Contributor

nanov commented Nov 9, 2024

This is incredibly good work, still trying to find out with build fails on node 22

@leMaik
Copy link
Contributor Author

leMaik commented Mar 26, 2025

Time to get this merged. 💪

I was now able to reproduce the npm ci issue on NodeJS 22 and it is caused by libxmljs not working on NodeJS 22 (see libxmljs/libxmljs#660). libxmljs seems to be unmaintained and libxmljs2, while supporting NodeJS 22, is officially unmaintained, so it's not an option. I chose xmllint-wasm, which doesn't depend on any native stuff so shouldn't break with future NodeJS updates, even if it becomes unmaintained.

Edit: TypeError: crypto.createDecipher is not a function on NodeJS 22, ie. the deprecated crypto methods were removed. I updated it to use the newer crypto.createDecipheriv with a user-provided iv and fallback to a compatible key+iv derivation. In other words: At least with aes-256-cbc everything still works. Users should however provide a proper key as passphrase (with enough bits for the selected algorithm) and an iv (with enough bits for the selected algorithm).

Edit 2: NodeJS 22 doesn't allow pkcs1 private decryption (earlier node versions would require --security-revert to enable it. The library now uses node-rsa to implement the decryption. Unless EBICS standards change, we don't have a choice here I guess.

@leMaik leMaik force-pushed the misc-updates branch 2 times, most recently from 967abe0 to 49fc29a Compare March 26, 2025 14:18
@leMaik
Copy link
Contributor Author

leMaik commented Mar 26, 2025

@nanov CI is green for all three NodeJS versions now. Let me know what you think. 🙏

Regarding encryption/decryption: I think this is an acceptable solution for now. It might cause issues when users who use something other than aes-256-cbc upgrade from NodeJS <22 to NodeJS 22+, but then again the library didn't work at all on NodeJS 22+ before. A breaking release can remove the legacy key derivation (and require users to provide a proper key and iv themselves).

@leMaik leMaik changed the title Improve readme, update xml-crypto, fix ci. Improve readme, update xml-crypto, fix ci, replace deprecated crypto methods and make add compatibility with NodeJS 22. Mar 26, 2025
@nanov nanov merged commit 3e4ea41 into node-ebics:master Apr 2, 2025
3 checks passed
@nanov
Copy link
Contributor

nanov commented Apr 2, 2025

Merged, released as 5.0.0 - thank you so much!

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