search

lquixada / cross-fetch

Universal WHATWG Fetch API for Node, Browsers and React Native.
MIT License
1.67k stars 104 forks source link

Please allow minor and patch updates #129

Closed janaagaard75 closed 2 years ago

janaagaard75 commented 2 years ago

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 commented 2 years ago

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

janaagaard75 commented 2 years ago

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 commented 2 years ago

Do you have any updates to this issue?

gabrielmicko commented 2 years ago

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

janaagaard75 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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