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

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

Open
joserodriguez16 opened this issue Jun 3, 2024 · 19 comments
Open

Comments

@joserodriguez16
Copy link

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
Copy link
Collaborator

@joserodriguez16 Why was this marked as completed?

@PeterHewat
Copy link
Contributor

PR #693 removed unused dependencies (including replace-in-file)

@joserodriguez16
Copy link
Author

@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
Copy link
Collaborator

@PeterHewat These dependencies are used by the schematics.

@shaharkazaz shaharkazaz reopened this Jul 6, 2024
@skatterwe
Copy link

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
Copy link
Collaborator

@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
Copy link

@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
Copy link

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

skatterwe pushed a commit to skatterwe/transloco that referenced this issue Aug 6, 2024
Update replace-in-flight to ^8.1.0 to resolve CWE-772 in inflight

✅ Closes: jsverse#768
@skatterwe
Copy link

skatterwe commented Aug 6, 2024

@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
Copy link
Collaborator

@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
Copy link

@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
Copy link
Collaborator

@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
Copy link

@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
Copy link

Please stop using inflight

npm warn deprecated [email protected]: 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
Copy link

Any updates on this?

@skatterwe
Copy link

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. #768 (comment) this was my initial try

@shaharkazaz
Copy link
Collaborator

@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
Copy link

@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
Copy link
Collaborator

@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.

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

No branches or pull requests

6 participants