Frameright / image-display-control-web-component

🚀 Next-gen responsive <img>
https://webc.frameright.io
MIT License
15 stars 0 forks source link

`npm audit fix` #10

Open lourot opened 1 year ago

lourot commented 1 year ago

At the moment, NPM detects a high severity vulnerability in one of our dependency. However npm audit fix doesn't help:

$ npm audit fix

up to date, audited 774 packages in 3s

133 packages are looking for funding
  run `npm fund` for details

# npm audit report

marked  <=4.0.9
Severity: high
Marked ReDoS due to email addresses being evaluated in quadratic time - https://github.com/advisories/GHSA-xf5p-87ch-gxw2
Inefficient Regular Expression Complexity in marked - https://github.com/advisories/GHSA-5v2h-r2cx-5xgj
Inefficient Regular Expression Complexity in marked - https://github.com/advisories/GHSA-rrrm-qjm4-v8hf
fix available via `npm audit fix`
node_modules/marked

1 high severity vulnerability

To address all issues, run:
  npm audit fix
MarkovianPD commented 1 year ago

I would like to take a crack at this!

lourot commented 1 year ago

Thank you @MarkovianPD !

MarkovianPD commented 1 year ago

Thank you @MarkovianPD !

@AurelienLourot Absolutely! I'm new to contributions trying to get my foot in the door anyway possible and help out! I created a fork to mess around with it before your reply and from what I could dig up and conclude is that you'll want to upgrade npm package "marked" to version 0.6.2. I'm not sure if I can do that by editing the package.json version to the updated version and it will upgrade? It does also seem you're using a 0.3.X version and I assume you're using an older version for a reason and upgrading may not be the best solution? Any and all feedback would be appreciated!

lourot commented 1 year ago

marked is not one of our direct dependencies: https://github.com/Frameright/image-display-control-web-component/blob/main/image-display-control/package.json#L44

It must be a dependency of one of all these dependencies. So if we're using an old version, it's not on purpose from us, but it might be on purpose from one of these dependencies, I'm not sure. If pinning explicitly the latest marked in our package.json works, I'm happy with that workaround :) Thanks!

MarkovianPD commented 1 year ago

Very interesting! If you could explained the process of pinning the latest in your package.json I would be thankful and more than happy to get it done asap!

lourot commented 1 year ago

I was thinking of adding "marked": "^4.2.12" inside our devDependencies in package.json. I just tried it out, it didn't help:

$ rm -rf package-lock.json node_modules/
$ npm install
[...]

1 high severity vulnerability

To address all issues, run:
  npm audit fix

Run `npm audit` for details.

$ npm audit fix
[...]
fix available via `npm audit fix`
node_modules/markdown-spellcheck/node_modules/marked

1 high severity vulnerability

To address all issues, run:
  npm audit fix

This is interesting however as it's showing me that it's our dependency markdown-spellcheck that is pulling an old marked. This project looks abandoned actually:

So contacting the maintainer to update their dependency to marked probably won't help. Are there newer/better tools for spellchecking markdown files these days, that we could use instead of markdown-spellcheck?

MarkovianPD commented 1 year ago

This is the only alternative I could find, https://socket.dev/npm/package/md-spell it says it is loosely based on lukeapage's node-markdown. I'm not sure this would accomplish exactly what you're looking for. Pertaining to the ugprade some users experiencing this issue in the past have commented that running npm upgrade solved the issue I would recommend at least trying this but as mentioned earlier there may be some issues if you're not wanting to upgrade specific packages. Multiple users responded to the recommendation stating that had worked for their situation.

lourot commented 1 year ago

npm upgrade doesn't help in my case. I think this is because markdown-spellcheck pins ^0.7.0: https://github.com/lukeapage/node-markdown-spellcheck/blob/master/package.json#L40

Which means "any version between 0.7.0 and 1", so this prevents us from getting the latest marked.

https://www.npmjs.com/package/md-spell has no link to a github repository, so I'm not sure where the code lives. There is a Tab Code there and I can see in package.json that they don't use marked anymore, which is good. They seem to use hunspell-spellchecker as a dependency, which hasn't been published for 8 years: https://www.npmjs.com/package/hunspell-spellchecker . I fear we would be switching to another abandoned project and run into the same kind of issues

MarkovianPD commented 1 year ago

I see, great catch I believe you're correct about jumping to another abandoned project. I believe I don't have sufficient knowledge to assist further with this but thank you for letting me have the chance to help you! I hope I can assist with other contributions in the future. If you find anything more on this npm audit and I can help I'll be ecstatic to help out!

ilu commented 1 year ago

@MarkovianPD @AurelienLourot

I think the cspell package could be used as an alternative to markdown-spellcheck:

It might require some configuring to have it spellcheck only the markdown files, but this guide might be helpful: https://tjaddison.com/blog/2021/02/spell-checking-your-markdown-blog-posts-with-cspell/

I don't have time right now to dig deeper, but happy to help you @MarkovianPD if you want to pursue this issue :)

MarkovianPD commented 1 year ago

@ilu Thank you for chiming in! I'll take a look at it when I can and see if any of it makes sense haha I'd love to crack the case and get it solved!

MarkovianPD commented 1 year ago

Thank you all for the help trying to get to the bottom of this! I've taken a read up on the cspell blog post and I believe this is beyond my expertise at the moment.

ilu commented 1 year ago

Thank you all for the help trying to get to the bottom of this! I've taken a read up on the cspell blog post and I believe this is beyond my expertise at the moment.

No worries @MarkovianPD, thank you for having a look. Please drop by again to see if any new issues would be suitable for you :)

lourot commented 1 year ago

FYI the spellchecker is now gone but we still have some npm audit issues. Keeping this open for now.

lourot commented 1 year ago

My mistake, I had forgotten to actually remove the dependency. Now the remaining npm audit issues are only moderate.