Replace replace-in-file package to resolve CWE-772 in inflight - Githubissues

jsverse / transloco

🚀 😍 The internationalization (i18n) library for Angular

https://jsverse.github.io/transloco/

MIT License

2.04k stars 199 forks source link

Replace replace-in-file package to resolve CWE-772 in inflight #768

Open joserodriguez16 opened 6 months ago

joserodriguez16 commented 6 months ago

replace-in-file uses glob ^8.1.0, which uses inflight which is a vulnerable package that doesn’t get updates. An issue on the replace-in-file repo has been open since March 11 of this year but the vulnerability has not been addressed. Therefore, it should be replaced. Inflight vulnerability details:

CWE-772 https://cwe.mitre.org/data/definitions/772.html

Snyk analysis https://security.snyk.io/package/npm/inflight/1.0.6

shaharkazaz commented 5 months ago

@joserodriguez16 Why was this marked as completed?

PeterHewat commented 5 months ago

PR https://github.com/jsverse/transloco/pull/693 removed unused dependencies (including replace-in-file)

joserodriguez16 commented 5 months ago

@joserodriguez16 Why was this marked as completed?

Sorry I should've left a comment. replace-in-file was recently updated to v8 and the vulnerability was addressed. I was going to open another issue proposing an update to the dependency

shaharkazaz commented 4 months ago

@PeterHewat These dependencies are used by the schematics.

skatterwe commented 3 months ago

Is there anything that can be done to help out here? i suppose all that is needed is a version bump to v8 right?

shaharkazaz commented 3 months ago

@skatterwe The package is now ESM which means all the schematics need an upgrade to use it. You are welcome to open a PR for it 🙌

skatterwe commented 3 months ago

@shaharkazaz if you guide me a bit on what i need to take care of in your pull requests etc i can surely have a look (commit message formates, branch name policies etc)

skatterwe commented 3 months ago

Ah found some hints in the contributing file. i'll see what i can do :)

skatterwe commented 3 months ago

@shaharkazaz as what would i commit this? i initially did as feature. but that somehow feels a bit weird. so before i'll raise a PR can you please let me know as what I should commit? Do you consider a version bump a refactor or a chore change or should I stick with feature?

shaharkazaz commented 3 months ago

@skatterwe hm... we can make it a fix I guess as this isn't actually adding a feature. We need to understand if this is somehow a breaking change it might be sue to the minimum node version needed to run the schematics no?

skatterwe commented 3 months ago

@shaharkazaz yeah I was also wondering about the question with the breaking change. I tried linking the dist from my fork within some of my projects and it worked all fine (without any changes) but I remember too well the flat update and how that caused us to update the tests later on 😅

but is fix with a breaking change something you consider ok commit wise? feels a bit strange. just let me know what you think best, then I'll create a new branch with the commit settings you would like :)

shaharkazaz commented 3 months ago

@skatterwe My suspect is the minimum node version required. Currently, the minimum Angular version is 16 which supports node v16, I vaguely remember that you need node 18 something for the ESM support but I'm not sure. If there is a breaking change that would be it.

Regarding the commit type, let's go with feature.

skatterwe commented 3 months ago

@shaharkazaz sorry was away. Just looking into it again. Reading from this: https://nodejs.medium.com/announcing-core-node-js-support-for-ecmascript-modules-c5d6dc29b663 the node version itself should be able to support ESM. I've tried running your tests with node 16 (to be precise: v16.20.2). That fails (not just on my branch with the replace-in-file update but also on master) because NX is using some internal functions that are not available in node 16.

       NX   (0 , node_os_1.availableParallelism) is not a function

any other suggestions how to double check that node 16 will work?

abdallahbedir2 commented 3 months ago

Please stop using inflight

npm warn deprecated inflight@1.0.6: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful.

stefanello57 commented 2 months ago

Any updates on this?

skatterwe commented 2 months ago

I need more information how to test if its a breaking change. just switching the node version in that project somewhat didn't reveal much. other than that there is a branch ready to raise a pr

@shaharkazaz can you please advise me on how to best check if there is a breaking change. https://github.com/jsverse/transloco/issues/768#issuecomment-2298136773 this was my initial try

shaharkazaz commented 2 months ago

@skatterwe Transloco is requiring Angular v16 which support node versions: ^16.14.0 || ^18.10.0. if we can't guarantee that node 16.14 works this is a breaking change as we need to set the minimum node version higher than what Angular supports by default.

Sorry for the delayed replies, I generally haven't been much around in Tranlsoco due to my reserve duty service.

skatterwe commented 2 months ago

@shaharkazaz yeah I understood that. I was wondering if you have any other idea on how to verify if it works with node 16.14.0.

from the references found it should not be breaking, but i would like to verify and failed to do so so far.

shaharkazaz commented 2 months ago

@skatterwe The easiest way is to create a new Angular workspace and use node v16.14 to create it. Personally I use nvm to manage my active node version which makes it pretty easy to switch back and forward.