Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update xml2js package #4

Merged
merged 1 commit into from
Feb 12, 2024
Merged

Update xml2js package #4

merged 1 commit into from
Feb 12, 2024

Conversation

Siyer2
Copy link
Contributor

@Siyer2 Siyer2 commented Apr 11, 2023

Need to update the xml2js package because of a vulnerability. Thanks!

@lorand-horvath
Copy link

lorand-horvath commented Apr 20, 2023

@mattdesl Would you please merge this PR?
The security vulnerability present in the xml2js ^0.4 dependency https://www.npmjs.com/package/xml2js?activeTab=versions is causing issues for JIMP users jimp-dev/jimp#1223

@benmccann
Copy link

@mattdesl could you please take a look at this? Almost 1m / week are exposed to a security issue until this package is updated

@lorand-horvath
Copy link

Looks like @mattdesl abandoned this package.

@Willibaur
Copy link

@masihyeganeh could you please have a look at this ❓

@mattdesl mattdesl merged commit 1e5fda8 into mattdesl:master Feb 12, 2024
@mattdesl
Copy link
Owner

Merged and patched on npm.

@lorand-horvath
Copy link

Merged and patched on npm.

@mattdesl Thank you!

@bakasmarius
Copy link

Merged and patched on npm.

@mattdesl have you seen my PR regarding this?
Simply updating xml2js results in the modified functionality of parse-bmfront-xml itself, so the users might end up seeing added [Object: null prototype] everywhere.

@lorand-horvath
Copy link

lorand-horvath commented Feb 13, 2024

@bakasmarius Keeping in mind that this PR merge updated to xml2js 0.5, does the newer xml2js 0.6.2 have the same problem? I assume the previous version 0.4.23 (with the vulnerability) was fine?

@bakasmarius
Copy link

@bakasmarius Keeping in mind that this PR merge updated to xml2js 0.5, does the newer xml2js 0.6.2 have the same problem? I assume the previous version 0.4.23 (with the vulnerability) was fine?

I haven't tried running parse-bmfont-xml tests on xml2js 0.6.2 as at the time of the creation of my PR the newest version was 0.5.0.
But if I remember correctly, adding [Object: null prototype] everywhere was the actual security vulnerability fix for xml2js and that broke parse-bmfont-xml tests because the expected output changed.
Since there are no pipelines/checks set up in this repo, one has to manually(locally) run the tests in order to notice that simply bumping xml2js version might cause unexpected side effects.
The modification in my PR (stringifying JSON and then parsing it) results in JSON without [Object: null prototype] (the way it was before).

@mattdesl
Copy link
Owner

I pushed another patch, 1.1.6 that uses Object.assign to create a shallow copy of the object from xml2js before storing it in the output. This seemed a little better than JSON back-and-forth which felt a little like a hack. It means that the minimum JS requirement has gone up slightly, but I assume most libraries depending on this will be able to polyfill Object.assign as it's been around for ages now.

Let me know if that has fixed everything.

@bakasmarius
Copy link

I pushed another patch, 1.1.6 that uses Object.assign to create a shallow copy of the object from xml2js before storing it in the output. This seemed a little better than JSON back-and-forth which felt a little like a hack. It means that the minimum JS requirement has gone up slightly, but I assume most libraries depending on this will be able to polyfill Object.assign as it's been around for ages now.

Let me know if that has fixed everything.

If the tests pass with Object.assign, then I think you can simply close my PR without merging it.

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.

6 participants