-
Notifications
You must be signed in to change notification settings - Fork 287
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
Bump node-fetch to 2.6.7 to resolve security issue #204
base: master
Are you sure you want to change the base?
Conversation
@matthew-andrews Hi! Please merge this PR. It's very important for some packages, which depends on this |
How soon until this PR get merged? Does this update also require a version? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I look forward to this update.
@matthew-andrews could you please comment on the status of this high-severity security issue? |
Hey all... is this library still maintained? With 6.6M download each week, I hoped so... but I don't see any PR not release for 1.5 year... Is there still a hope? cc @Jxck |
@gcruchon I share your concerns. FWIW due to the lack of response here I've since switched to cross-fetch as suggested in this project's README. Transition was straightforward. |
@roberttaylor426 @matthew-andrews Any updates on if this PR will complete or not? We have nested dependencies (dependency of a dependency) that uses this package. This PR would resolve one of our security issue that we can't really fix otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good change. Let's merge!
Any ETA for this merge? |
Year 2023 and still, the PR has not been merged... We're stuck with the security warning forever I guess... |
@matthew-andrews please merge |
i dont understand why this is necessary. the semver should match the newer version … can someone help explain why it's necessary ? |
Sorry if I made you confuse. All I need is a patch or minor version of isomorphic-fetch@2 due to some package that use the ^2.x.x version and we cannot upgrade it to use your isomorphic-fetch@3. Please let me know if it is valid. |
hmm, this pull request is not really going to help for v2 because version v2 relies on node-fetch v1.x.x/whatwg-fetch v0.x.x … the only change between v2 and v3 is this upgrade to use the later versions of node-fetch(v2.x.x)/whatwg-fetch(v3.x.x) so that doesn't really make sense. is there a version of node-fetch from the v1.x.x that passes your security check? i guess not … probably the more correct thing to do is upgrade to the v3 branch of isomorphic-fetch … and get fbjs to upgrade also. |
just to prove this pull request is not necessary:
as you can see, npm will happily download v2.6.9 with the currently released version of isomorphic-fetch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NPM's node-fetch package versions until 3.2.10 are subject to a MaxListenersExceededWarning: Possible EventEmitter memory leak detected warning and to a Regular Expression Denial of Service bug. Therefore, I think that node-fetch should be upgraded to version 3.2.10 instead of 2.6.7, since the former is the earliest non-vulnerable version of that package.
The issue is that some of us with legacy code to maintain cannot easily update our intermediate dependencies, which leaves us requiring a 2.x branch of isomorphic-fetch; if a point release of the 2.x version could be made that updated this dependency it would bring a lot of older applications up to more secure code. |
@matthew-andrews you're right in saying that this PR will not resolve the issue at hand. @rjstanford is right; what needs to happen is we need a new |
See here and here.
Addresses #202.