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

apollo-server-env is dependent on a version of node-fetch with a security vulnerability #6065

Closed
justinmchase opened this issue Jan 27, 2022 · 8 comments

Comments

@justinmchase
Copy link

justinmchase commented Jan 27, 2022

> npm audit report
# npm audit report

node-fetch  < 3.1.1
node-fetch is vulnerable to Exposure of Sensitive Information to an Unauthorized Actor - https://huntr.dev/bounties/d26ab655-38d6-48b3-be15-f9ad6b6ae6f7,https://github.com/node-fetch/node-fetch/commit/36e47e8a6406185921e4985dcbeff140d73eaa10
> npm ls node-fetch
example@1.0.0 /code/example
└─┬ apollo-server-core@3.6.2
  └─┬ apollo-server-env@4.2.1
    └── node-fetch@2.6.7

It seems like you just need to update to a newer version of node-fetch 3.1.1 or greater (3.2.0 is latest currently)

@glasser
Copy link
Member

glasser commented Jan 27, 2022

That npm audit seems to be imprecise. The fix has also been backported to 2.6.7 (and we can't really take the major upgrade as v2 -> v3 drops support to use it in a build context that isn't ES Module-native), and as you can see above the latest version of Apollo Server does require 2.6.7.

Note that this particular vulnerability is unlikely to affect any use cases in Apollo Server anyway, and there are workarounds to allow you to use your favorite version of node-fetch or any other implementation of the fetch API. See https://github.com/apollographql/apollo-server/blob/main/CHANGELOG.md#v362 for details.

@glasser glasser closed this as completed Jan 27, 2022
@glasser
Copy link
Member

glasser commented Jan 27, 2022

For what it's worth, I can't reproduce this. Maybe the advisory has fixed with the appropriate versions already?

glasser@dsg-mbp 0 15:52:14 /tmp/x $ npm i node-fetch@2.6.7

added 4 packages, and audited 5 packages in 664ms

found 0 vulnerabilities
glasser@dsg-mbp 0 15:52:19 /tmp/x $ npm audit
found 0 vulnerabilities
glasser@dsg-mbp 0 15:52:24 /tmp/x $ npm audit report
found 0 vulnerabilities
glasser@dsg-mbp 0 15:52:29 /tmp/x $ npm i node-fetch@2.6.6

changed 1 package, and audited 5 packages in 508ms

1 high severity vulnerability

To address all issues, run:
  npm audit fix

Run `npm audit` for details.
glasser@dsg-mbp 0 15:53:05 /tmp/x $ npm audit 
# npm audit report

node-fetch  <2.6.7
Severity: high
node-fetch is vulnerable to Exposure of Sensitive Information to an Unauthorized Actor - https://github.com/advisories/GHSA-r683-j2x4-v87g
fix available via `npm audit fix`
node_modules/node-fetch

1 high severity vulnerability

To address all issues, run:
  npm audit fix

Going to the linked GHSA-r683-j2x4-v87g shows correctly that 2.6.7 is OK.

I see that your npm audit is linking to huntr.dev rather than the GH advisories. Have you somehow configured npm audit to use some other source of advisories?

@justinmchase
Copy link
Author

justinmchase commented Jan 28, 2022

I'm actually dependent on apollo-server-core which then depends on this and npm audit is giving me these errors. I abbreviated it so it was less complex, I'll review your notes and report back if I have anything relevant.

@justinmchase
Copy link
Author

justinmchase commented Jan 29, 2022

Ok so I'm trying to dig into this more and I am still seeing the issue.

I am following your steps here and I'm realizing you're installing node-fetch@2.6.7 which I see resolves the issue you are seeing there but the security issue I'm seeing (node-fetch is vulnerable to Exposure of Sensitive Information to an Unauthorized Actor), which is in < 3.1.1. I'm not sure why your audit report is different than mine though.

package.json version

    "apollo-server-core": "^3.6.2",
    "node-fetch": "^2.6.7",

node versions

> node --version
v16.13.1
> npm --version
8.1.2
> npm audit report
# npm audit report

node-fetch  < 3.1.1
node-fetch is vulnerable to Exposure of Sensitive Information to an Unauthorized Actor - https://huntr.dev/bounties/d26ab655-38d6-48b3-be15-f9ad6b6ae6f7,https://github.com/node-fetch/node-fetch/commit/36e47e8a6406185921e4985dcbeff140d73eaa10
fix available via `npm audit fix --force`
Will install node-fetch@3.2.0, which is a breaking change
node_modules/node-fetch
  apollo-server-env  *
  Depends on vulnerable versions of node-fetch
  node_modules/apollo-server-env
    apollo-datasource  *
    Depends on vulnerable versions of apollo-server-env
    node_modules/apollo-datasource
    apollo-server-core  >=2.0.0-beta.0
    Depends on vulnerable versions of apollo-server-env
    Depends on vulnerable versions of apollo-server-types
    node_modules/apollo-server-core
      apollo-server-express  >=2.0.0-beta.0
      Depends on vulnerable versions of apollo-server-core
      Depends on vulnerable versions of apollo-server-types
      node_modules/apollo-server-express
    apollo-server-types  *
    Depends on vulnerable versions of apollo-server-env
    node_modules/apollo-server-types
      apollo-server-plugin-base  >=0.6.0-alpha.0
      Depends on vulnerable versions of apollo-server-types
      node_modules/apollo-server-plugin-base

7 low severity vulnerabilities

To address issues that do not require attention, run:
  npm audit fix

To address all issues (including breaking changes), run:
  npm audit fix --force
> npm ls node-fetch
my-lib@3.2.0 /Users/jchase24/code/my-app
├─┬ apollo-server-core@3.6.2
│ └─┬ apollo-server-env@4.2.1
│   └── node-fetch@2.6.7 deduped
└── node-fetch@2.6.7

So then I tried to run

npm i node-fetch@3.2.0
npm audit fix

But this of course does not update the node-fetch dependency for apollo-server-env because it is cannot match that major version upgrade.

> npm ls node-fetch
my-lib@3.2.0 /Users/jchase24/code/my-app
├─┬ apollo-server-core@3.6.2
│ └─┬ apollo-server-env@4.2.1
│   └── node-fetch@2.6.7
└── node-fetch@3.2.0

So the confusion here comes from my npm audit reporting different issues than your npm audit not sure why that is off hand.

But then the solutions seem to be:

  1. Upgrade apollo-server-env to depend on node-fetch@3.1.1 or later
  2. Replace node-fetch dependency with something else
  3. Maybe I should ask node-fetch to issue a patch fix to version 2.6.7? Or maybe the issue they reported should be more specific?

@justinmchase
Copy link
Author

Here is the security patch release:

https://github.com/node-fetch/node-fetch/releases/tag/v3.1.1

@justinmchase
Copy link
Author

justinmchase commented Jan 29, 2022

In their actual fix they have this comment:

Should patch v2 also (not many can update to esm)

So that seems to confirm point #3 perhaps? Can we re-open this issue and then I'll file an issue on their repo asking them to patch v2.6 and link to this?

@glasser
Copy link
Member

glasser commented Jan 29, 2022

The actual advisory I linked above makes it clear that the fix is in both 3.1.1 and 2.6.7. You must have your npm audit configured to talk to some other advisory source whose metadata is imprecise.

In any case there's no actual security issue here -- the issue only occurs if you use node-fetch to talk to an URL that will redirect to an attacker controlled URL, and we don't do that in Apollo Server (unless you do that in a server that you use apollo-datasource-rest to talk to). Open redirectors are generally considered to be a bad idea anyway.

@justinmchase
Copy link
Author

Ok, I think I'm catching up now. Thank you for your patience. We are indeed using on-prem github and an on-prem artifactory which works as a proxy/cache for public npm. I don't know how the audit is working exactly (or not as the case may be). Anyway, not your problem sorry for the noise!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants