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

npm audit fails about xml2js (from load-bmfont) #1223

Closed
edi9999 opened this issue Apr 19, 2023 · 20 comments · Fixed by #1285
Closed

npm audit fails about xml2js (from load-bmfont) #1223

edi9999 opened this issue Apr 19, 2023 · 20 comments · Fixed by #1285
Labels
released This issue/pull request has been released.

Comments

@edi9999
Copy link
Contributor

edi9999 commented Apr 19, 2023

On my project, I am using jimp, and just found out that the current latest version : 0.22.7, has a vulnerable dependency.

Here is the output of npm audit :

xml2js  <0.5.0
Severity: high
xml2js is vulnerable to prototype pollution  - https://github.com/advisories/GHSA-776f-qx25-q3cc
fix available via `npm audit fix --force`
Will install jimp@0.3.5, which is a breaking change
node_modules/xml2js
  parse-bmfont-xml  *
  Depends on vulnerable versions of xml2js
  node_modules/parse-bmfont-xml
    load-bmfont  >=1.1.0
    Depends on vulnerable versions of parse-bmfont-xml
    node_modules/load-bmfont
      @jimp/plugin-print  *
      Depends on vulnerable versions of load-bmfont
      node_modules/@jimp/plugin-print
        @jimp/plugins  *
        Depends on vulnerable versions of @jimp/plugin-print
        node_modules/@jimp/plugins
          jimp  >=0.3.6-alpha.5
          Depends on vulnerable versions of @jimp/plugins
          node_modules/jimp

6 high severity vulnerabilities

To address all issues (including breaking changes), run:
  npm audit fix --force

It seems that @jimp/plugin-print uses :

  • load-bmfont (this package was lastly published 3 years ago)
  • load-bmfont uses parse-bmfont-xml (thia package was lastly published 5 years ago)
  • parse-bmfont-xml uses xml2js ^0.4.5 (which won't pull in 0.5 which fixes the vuln).

There is an issue in parse-bmfont-xml to upgrade to xml2js 0.5.0 : mattdesl/parse-bmfont-xml#6

@lorand-horvath
Copy link

lorand-horvath commented Apr 20, 2023

A quick and dirty solution until parse-bmfont-xml bumps xml2js to 0.5.0 is to add an override to your package.json Leonidas-from-XIV/node-xml2js#671 (comment)

  "overrides": {
    "jimp": {
      "xml2js": "^0.5.0"
    }
  }

and npm install.

Note: overrides are only available since npm 8.3

@zmedgyes
Copy link
Contributor

zmedgyes commented Apr 21, 2023

An alternative / temporary solution could be to create a custom-configured Jimp by using @jimp/custom , if you know, you are not using features from the @jimp/plugin-print plugin. With that you can eliminate the problematic dependecy manually.
Please note, that you might run into some issues with that, if you are using Typescript (depending on your configuration, I made PR where I try to make the modular configuration more available: #1225 ).

@blastshielddown
Copy link

FWIW I'm now seeing this vulnerability categorized as moderate. Not sure what has changed or if npm audit works differently on different node versions. I'm on v16.20.0.

@lorand-horvath
Copy link

lorand-horvath commented Jun 12, 2023

It's high time for parse-bmfont-xml to be updated to include the latest xml2js 0.6.0 https://github.com/Leonidas-from-XIV/node-xml2js/tags but there's no movement in any of the already existing PRs:
mattdesl/parse-bmfont-xml#4
mattdesl/parse-bmfont-xml#5

If this won't progress, would the jimp devs fork it or find an alternative, please ? @hipstersmoothie @Marsup @zmedgyes @sjoerd108

@hipstersmoothie
Copy link
Collaborator

If someone want to fork those deps into the jimp org and do the update I'll help make it happen

@pzrq
Copy link

pzrq commented Jul 24, 2023

A quick and dirty solution until parse-bmfont-xml bumps xml2js to 0.5.0 is to add an override to your package.json Leonidas-from-XIV/node-xml2js#671 (comment)

  "overrides": {
    "jimp": {
      "xml2js": "^0.5.0"
    }
  }

and npm install.

Note: overrides are only available since npm 8.3

The nested form did not work for me in npm 9.6.3, though the following did reliably generate the upgraded non-vulnerable package-lock.json I needed on npm install:

  "overrides": {
    "xml2js": "^0.5.0"
  }

@lorand-horvath
Copy link

lorand-horvath commented Jul 24, 2023

@pzrq The nested form works perfectly fine. But there's a catch. Whenever you install a new package, e.g. npm install some-package the defined override will not be taken into account at all, so the old xml2js will be reinstalled. This is some very strange quirk of npm and many people have run into this issue. The solution is to delete node_modules and package-lock.json and then do a clean npm install of all packages. This will apply the override as expected.
I'm not sure if this is what you have run into, but most certainly many are struggling with this, still.

@isahann
Copy link

isahann commented Aug 4, 2023

Is there any progress available on the work in this vulnerability? I'm having the same issue here and I'm using v0.22.10.

@edi9999
Copy link
Contributor Author

edi9999 commented Sep 14, 2023

No, same issue for me as of now.

@RazvanVuscan
Copy link

This issue is still occurring for me as well.

@lorand-horvath
Copy link

Very strange that nobody bothers to actually fix this #1223 (comment)

@RazvanVuscan
Copy link

RazvanVuscan commented Sep 21, 2023

@lorand-horvath from your original workaround, I've tried adding the override to my package.json , deleted package-lock.json and node_modules, but I still see the vulnerability during the yarn audit process. Seems v. 0.4.5 keeps getting pulled in.

Am I doing something wrong?

@lorand-horvath
Copy link

lorand-horvath commented Sep 21, 2023

@RazvanVuscan As per #1223 (comment) :
In package.json add this, which will override xml2js to the currently latest version 0.6.2:

  "overrides": {
    "jimp": {
      "xml2js": "^0.6.0"
    }
  }

Then delete package-lock.json and node_modules.
Then npm install. This will only work if you have npm 8.3 or newer - you can check with npm -v.
This should work. If not, what version of node do you have? node -v

Edit: I see you're using yarn instead of npm. I haven't worked much with it. I'm not sure if and since what version of yarn are overrides supported, you'd have to dig a bit and find out. If you do, please let me know!

@RazvanVuscan
Copy link

RazvanVuscan commented Sep 21, 2023

@lorand-horvath yeah, I actually use nvm, and my node version is 20.0.0. And yes, I use yarn instead of npm because of speed reasons 😄 .

But your suggestions did provide me a solution, and as per https://classic.yarnpkg.com/en/docs/selective-version-resolutions/#toc-how-to-use-it.

If you are using yarn, add this to your package.json for a quick and dirty solution:

    "resolutions": {
        "jimp/@jimp/plugins/@jimp/plugin-print/load-bmfont/parse-bmfont-xml/xml2js": "^0.6.0"
    }

yarn uses resolutions not overrides 😄 .

skipjack added a commit to skipjack/free-flag-icons that referenced this issue Oct 8, 2023
See jimp-dev/jimp#1223, that workaround
doesn't seem to have any downsides when running `npm run collect`.
@benmccann
Copy link
Contributor

Perhaps jimp should not install all plugins automatically? I have no need to print bitmap fonts on my files, but plugin-print is automatically included. It feels like something that should be made into an optional peer dependency

@RonaldPM
Copy link

RonaldPM commented Jan 9, 2024

Seems like issue is causing security tools such as Snyk to flag Jimp. Is there a plan for a fix yet?

@lorand-horvath
Copy link

Fixed in mattdesl/parse-bmfont-xml#4

@benmccann
Copy link
Contributor

Hurray! There's nothing left to do here as users can now update their lockfiles without any changes necessary in jimp

That being said, I do think it'd be an improvement if all jimp plugins were not automatically installed: #1223 (comment)

@hipstersmoothie
Copy link
Collaborator

If anyone wants to submit a pr to update our deps I'd approve! not installing all the default plugins is a pretty big breaking change and I don't think breaking changes are too worth it for this project

@hipstersmoothie
Copy link
Collaborator

🚀 Issue was released in v1.0.2 🚀

@hipstersmoothie hipstersmoothie added the released This issue/pull request has been released. label Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants