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

Please allow minor and patch updates #129

Closed
janaagaard75 opened this issue Jan 29, 2022 · 10 comments · Fixed by #132
Closed

Please allow minor and patch updates #129

janaagaard75 opened this issue Jan 29, 2022 · 10 comments · Fixed by #132

Comments

@janaagaard75
Copy link

Hi cross-fetch. I would like to understand why this repository isn't allow minor and patch updates to the dependencies. My best guess is that the tighter control allows you to avoid issues that might occur because a dependency was updated, but is that really happening that frequently? Even if you guys are fast to patch your dependencies, not allowing minor and patch updates puts a pretty big update burden on all the packages that use cross-fetch.

@lquixada
Copy link
Owner

a new cross-fetch version was just released with a dependency update. do you mind elaborating a bit more?

@janaagaard75
Copy link
Author

I can give an example: Gatsby was recently updated to version 4.6. They have a transient dependency on cross-fetch through url-loader 6.10.1*. That version of url-loader is unfortunately using cross-fetch 3.1.4 and therefore the vulnerable node-fetch 2.6.1. The earliest one with a fix is 2.6.7.

As you point out yourself, cross-fetch has been updated, but that update won't react Gatsby until all the packages in the dependency graph that use exact versions have been updated. If cross-fetch 3.1.4 had allowed minor or even patch updates of node-fetch, then the vulnerability fix of node-fetch would reach end users of Gatsby as soon as they update the packages with npm update or yarn upgrade.

*) Dependency graph: gatsby > eslint-plugin-graphql > graphql-config > @graphql-tools/url-loader > cross-fetch > node-fetch.

(I hope this description was somewhat readable, and that I didn't mess up any of the version numbers.)

@janaagaard75
Copy link
Author

Do you have any updates to this issue?

@gabrielmicko
Copy link

gabrielmicko commented Feb 16, 2022

This is a security issue, +1 node-fetch is vulnerable to Exposure of Sensitive Information to an Unauthorized Actor

@janaagaard75
Copy link
Author

Would it be possible release a parallel version of cross-fetch that is identical to this, except it has carets in package.json?

I don't know exactly how this would work. If an issue is found, it would require that the users are skilled enough to switch this version of cross-fetch, which is not that obvious when cross-fetch is a transient dependency. I haven't seen this done anywhere else, but I really hope we somehow could move forward with this issue.

@gabrielmicko
Copy link

I had to get rid of the this package and switch to ESM modules, so I can install newest version of node-fetch. node-fetch v3 only supports ESM modules and I needed it for apollo to work. Such a headache...

@janaagaard75
Copy link
Author

I had to get rid of the this package and switch to ESM modules, so I can install newest version of node-fetch. node-fetch v3 only supports ESM modules and I needed it for apollo to work. Such a headache...

Allowing MINOR and PATCH updates in package.json would now allow MAJOR version changes, @gabrielmicko.

@janaagaard75
Copy link
Author

janaagaard75 commented Mar 14, 2022

Sorry for asking aging, but do you have any updates to this issue, @lquixada? To me, it seems like a small change to make, but I would very much like to hear if you don't agree.

@lquixada
Copy link
Owner

@janaagaard75 Sorry for the late reply. I feel the discussion boils down to "no range" vs "caret range" vs "tilde range". So first let me address why "tilde range" might not be a good idea:

TLDR is "caret range" > "tilde range". That leaves us with the "no range" vs "caret range" dilemma.

Currently a comprehensive suite of tests run against cross-fetch on different environments to ensure fetch api implementations are consistent across the board. When those tests are executed, a fixed version of whatwg-fetch and node-fetch is used so results remain deterministic. If a caret range is used, that consistency can no longer be guaranteed since different minor versions can potentially introduce bugs or regressions on clients without further notice.

However I'm leaning toward adding ^ for two reasons:

  1. AFAIK there's no track record of relevant bugs or regressions on newer releases of node-fetch@2.x.
  2. Security fixes (more than regular bug fixes) should be delivered as fast as possible, specially on legacy code and on transient dependencies.

Ideally dependent packages such as @graphql-tools/url-loader should be the ones to add cross-fetch with a caret range. I feel this could be the right approach if the first item above does not hold true.

@janaagaard75
Copy link
Author

Thanks a lot. Not only for merging the pull request @lquixada, but also for detailing why you prefer the caret over the tilde.

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

Successfully merging a pull request may close this issue.

3 participants