Node-http-proxy: NPM audit reports the package with high vulnerability (Denial of Service)

Created on 14 May 2020  路  51Comments  路  Source: http-party/node-http-proxy

Most helpful comment

And there goes CI pipelines across the globe...

All 51 comments

And there goes CI pipelines across the globe...

Seems like a key bit here that limits the scope:

This is only possible when the proxy server sets headers in the proxy request using the proxyReq.setHeader function.

Wouldn't this be a short term work around - proxyReq.headers['arr'] = "I'm a pirate" ?

I get the security warning from NPM about this package too. Using react-scripts (https://github.com/facebook/create-react-app)

Yeah, projects using webpack-dev-server will have this warning.

Is there a fix for this in the works?

Our pipeline returns this audit failure
High
Denial of Service
Package http-proxy
Patched in No patch available
Dependency of react-scripts
Path react-scripts > webpack-dev-server > http-proxy-middleware > http-proxy
More info https://nodesecurity.io/advisories/1486

npm audit warns in projects with webpack-dev-server
webpack-dev-server > http-proxy-middleware > http-proxy

sam withyarn audit

So you can provisionally fix this by moving webpack-dev-server or react-scripts (whichever package is causing the issue) to devDependencies.

Once the package is in your devDependencies you can run npm audit --prod and it will not audit the package as it only audits the node packages in your dependencies with the --prod arg passed to the CLI.

npm audit warns in projects with webpack-dev-server
webpack-dev-server > http-proxy-middleware > http-proxy

Same for react-scripts as it depends on webpack-dev-server
react-scripts > webpack-dev-server > http-proxy-middleware > http-proxy

audit-ci npm dependency can also be used instead of npm audit to whitelist this particular dependency.

audit-ci npm dependency can also be used instead of npm audit to whitelist this particular dependency.

Yes, if you use https://github.com/IBM/audit-ci, you can whitelist http-proxy like that:

$ npx audit-ci --high --whitelist=http-proxy

Caution

It's not recommended to use packages with vulnerabilities. In my case I've had to whitelist this in order for Travis CI to continue working.

audit-ci npm dependency can also be used instead of npm audit to whitelist this particular dependency.

Yes, if you use https://github.com/IBM/audit-ci, you can whitelist http-proxy like that:

$ npx audit-ci --high --whitelist=http-proxy

Sorry for noob question, but does this mean it isn't risky to use locally?

audit-ci npm dependency can also be used instead of npm audit to whitelist this particular dependency.

Yes, if you use https://github.com/IBM/audit-ci, you can whitelist http-proxy like that:

$ npx audit-ci --high --whitelist=http-proxy

Sorry for noob question, but does this mean it isn't risky to use locally?

In theory it still is but by whitelisting you're intentionally ignoring the vulnerability. However i think this particulair vulnerability is more of a threat when published on a server.

@Jackques, I tried running the whitelist command and it had no luck for me. Would it work if you were to CRA and then run --> npm uninstall http-proxy ?

@Jackques, I tried running the whitelist command and it had no luck for me. Would it work if you were to CRA and then run --> npm uninstall http-proxy ?

If you use https://github.com/IBM/audit-ci then you should be able to set the error id (which relates to the security vul. issue id) in a .json. Check the audit-cli package for details. No need to reinstall the package in order to whitelist it. Once you have got the security id in your witelist the package should no longer appear in the security report.

@Jackques, Thank you! What I meant to say was that what if you dont whitelist it?
In my specfic situation:
I am working in a group repo with branches and I am tying to install the dependencies for the client from create-react-app (where as I go to the master branch, git pull'd --> ran npm i | npm install). At that point i was saying what if you uninstall http-proxy at ^that point(after installing dependencies ad seeing the warning) instead of whitelisting? If that's a clearer way to ask the question?

Else:
Again I thank you for your help!

I'm actually more concerned as to what specifically is wrong within the http-proxy package that is causing the error in the first place.

Disappointed that people are recommending whitelisting http-proxy or worse still completely disabling audit of devDependencies in response to this. I would guess that 90% of the people that read this thread and take that advice probably don't understand the full implications of it and will not reinstate the audit after http-proxy is fixed.

It's not just vulnerabilities that audit guards against but intentional exploits also. Just think about what things the code in those devDependencies has access to when it runs on your local machine or build agent:

  • Probably some private keys, database passwords, etc.
  • If you're in the cloud your build agent may well have privileged access to your cloud resources so it can deploy/modify things.
  • That code may not be directly included in your production JS files but it's able to access and manipulate those files so it could rewrite them any way it wanted.
  • It could duplicate itself to any npm packages you publish from your build agent

In summary: please don't do that.

According to the advisory, this was reported back in February.
Count me disappointed as well.

What's the eta on this getting fixed?

Maybe this happened due to a potential protoypePollutionAttack; possibly?

https://blog.0daylabs.com/2019/02/15/prototype-pollution-javascript/
I noticed that besides the DoS vulnerability(http-proxy<< "High"), there is a "Low" vulnerability via yargs-parser package for ProtoType Pollution....so maybe this could have resulted from a prototype pollution.

Even better than whitelisting http-proxy it should be possible to whitelist the specific vulnerability using audit-ci. And then remove the whitelist after it's fixed.

The problem is described here, in #1328.

Simplest fix is to just not send the proxyReq event when the expect header is present. See https://github.com/http-party/node-http-proxy/pull/1447 for a hotfix, and feel free to leave comments if you do not agree and have a better suggestion. :)

Disappointed that people are recommending whitelisting http-proxy or worse still completely disabling audit of devDependencies in response to this. I would guess that 90% of the people that read this thread and take that advice probably don't understand the full implications of it and will not reinstate the audit after http-proxy is fixed.

It's not just vulnerabilities that audit guards against but intentional exploits also. Just think about what things the code in those devDependencies has access to when it runs on your local machine or build agent:

* Probably some private keys, database passwords, etc.

* If you're in the cloud your build agent may well have privileged access to your cloud resources so it can deploy/modify things.

* That code may not be directly included in your production JS files but it's able to access and manipulate those files so it could rewrite them any way it wanted.

* It could duplicate itself to any npm packages you publish from your build agent

In summary: please don't do that.

I'm definitely sure 99.99% of the people fully understand the risks and issues with those SUGGESTIONS. I wouldn't do it if I was going to release a production version which would access my cloud resources, keys, etc! I suggest focusing on fixing the issue rather than expressing useless opinions. We are not 2-year-old developers!

Using http-proxy with autoRewrite: true , and got breakdown. :(

I get the security warning from NPM about this package too. Using gatsby and netlify-cli:

gatsby > webpack-dev-server > http-proxy-middleware > http-proxy
netlify-cli > http-proxy
netlify-cli > http-proxy-middleware > http-proxy

will be resolved once/if https://github.com/http-party/node-http-proxy/pull/1447 is merged and released.

@Jackques, Thank you! What I meant to say was that what if you dont whitelist it?
In my specfic situation:
I am working in a group repo with branches and I am tying to install the dependencies for the client from create-react-app (where as I go to the master branch, git pull'd --> ran npm i | npm install). At that point i was saying what if you uninstall http-proxy at ^that point(after installing dependencies ad seeing the warning) instead of whitelisting? If that's a clearer way to ask the question?

Else:
Again I thank you for your help!

Well if you were to have http-proxy as a direct dependancy (which i doubt since it's also a dependancy of eventually angular, react etc.) then it would resolve the error. But like i said in between the paranthesis, you probably have an app built on on of those previously mentioned frameworks or at the very least something which relies on the http-proxy dependancy, thus you cannot resolve this issue by uninstalling it unless you install the very package which ultimately has http-proxy as a dependancy.

If #1447 is merged and released this exploit after audit will be resolved? It's a good fact to run npm audit fix on all our React apps live on the web, isn't it?

when i go here for more info, it says,
"No fix is currently available. Consider using an alternative package until a fix is made available."
So is there any alternative package available ?

The npm advisory lists the newly published 1.18.1 as still being affected: https://www.npmjs.com/advisories/1486/versions

Not sure if that is normal and it still needs to be reviewed by someone at npm, or #1447 unfortunately didn't do the trick.

That is normal: they work on a whitelist :)

I've sent an email to security@npm with links to this issue & the PR, commenting that 1.18.1 patches the advisory & should be whitelisted.

Wonderful! Thank you @G-Rath!

when i go here for more info, it says,
"No fix is currently available. Consider using an alternative package until a fix is made available."
So is there any alternative package available ?

Well yes, but since most of us use packages (e.g. http-proxy-middleware) which in turn use http-proxy as a dependancy those packages will need to use the alternatives themselves first.
And yes i am aware of a dirty hack we could exploit by changing the hash in the package-lock file but the packages which are dependant on http-proxy also need to update the api's when switching to another package other than http-proxy.

Thanks @G-Rath can you post on here when it has been whitelisted?
Preparing for the update in my package-lock.json file

"http-proxy": {
      "version": "1.18.1",
      "resolved": "https://registry.npmjs.org/http-proxy/-/http-proxy-1.18.1.tgz",
      "integrity": "sha512-7mz/721AbnJwIVbnaSv1Cz3Am0ZLT/UBwkC92VlxhXv/k/BBQfM2fXElQNC27BVGr0uwUpplYPQM9LnaBMR5NQ==",
      "requires": {}
    }

Got the info from https://registry.npmjs.org/http-proxy

The advisory has been updated: https://www.npmjs.com/advisories/1486

Remediation
Upgrade to version 1.18.1 or later

Since released and advisory is updated, I'm closing this.

Anyone mind telling me how to make the update with npm? DO i just change the version number in the package-lock.json file or ....?

@dosstx

  • npm audit fix should fix it for you (now that the audit is resolved with a patch version).
    _or_
  • If you are able to (no other breakages), deleting and regenerating the lock file from scratch (rm package-lock.json; npm i) should also pick up the new version.
    _or_
  • manually deleting the current resolution for http-proxy (in the pacakge-lock.json file) and re-running npm i.

Edit: see @Hypnosphi's comment for how to narrow down the focus https://github.com/http-party/node-http-proxy/issues/1446#issuecomment-630827241

Add this to your package.json, so that packages that depend on http-proxy will resolve it to the patched version.

"resolutions": {
  "http-proxy": "^1.18.1"
}

Yarn supports the resolutions syntax out-of-the-box.

If using NPM, see https://www.npmjs.com/package/npm-force-resolutions

Forcing resolutions (even if range) is a bad practice and should be avoided if possible, as it may cause other breakages. Not to say that it wouldn't work in this specific instance (it probably would).

Sorry y'all I was asleep when the email came in:

Hi Gareth,

Thank you for contacting the npm security team. We have updated the advisory and versions >=1.18.1 are marked as unnaffected.

Forcing resolutions (even if range) is a bad practice and should be avoided if possible, as it may cause other breakages. Not to say that it wouldn't work in this specific instance (it probably would).

That is a blanket statement. Forcing resolutions works fine in our project. But I would agree that it needs to be evaluated on a case-by-case basis.

Valid reasons for using: https://classic.yarnpkg.com/en/docs/selective-version-resolutions/#toc-why-would-you-want-to-do-this

Forcing resolution forces all instances of that package in the tree to use the same major. This can obviously break (it's one of the things that semver solves).

Of course, if your tree already contains the same major, it can work, and even improve deduping. Or, if usages of those packages is limited, it might not blow up. Or, if the majors are compatible, it might also work.

In addition, using it for production dependencies does not affect your consumers. That's an important aspect usually forgotten.

It's one of those functionalities that you better understand the implications before using, and I wouldn't personally suggest it in yet-another-issue as a go-to solution.

EDIT: I remember people also suggesting it for the mkdirp vulnerability, and in that instance it was completely incorrect, as the API of the library completely changed between majors.

Thanks, I am using netlify and I was also getting the same error yesterday. It got resolved through npm audit fix and just delete http-proxy from node-modules then do npm i.

                   === npm audit security report ===


                             Manual Review
         Some vulnerabilities require your attention to resolve

      Visit https://go.npm.me/audit-guide for additional guidance

High Denial of Service

Package http-proxy

Patched in No patch available

Dependency of react-scripts

Path react-scripts > webpack-dev-server > http-proxy-middleware >
http-proxy

More info https://npmjs.com/advisories/1486

Low Prototype Pollution

Package yargs-parser

Patched in >=13.1.2 <14.0.0 || >=15.0.1 <16.0.0 || >=18.1.2

Dependency of react-scripts

Path react-scripts > webpack-dev-server > yargs > yargs-parser

More info https://npmjs.com/advisories/1500

found 2 vulnerabilities (1 low, 1 high) in 1690 scanned packages
2 vulnerabilities require manual review. See the full report for details.

@AviVahl

Forcing resolution forces all instances of that package in the tree to use the same major.

That's why I always specify the parent, e.g:

"resolutions": {
  "**/http-proxy-middleware/http-proxy": "^1.18.1"
}

Or even the full path:

"resolutions": {
  "react-scripts/webpack-dev-server/http-proxy-middleware/http-proxy": "^1.18.1"
}

I can't say I think this is much better.

http-proxy-middleware uses semver caret for http-proxy so there's absolutely no reason to use a forced resolution. Just let it resolve to the latest version. The original range covers the new patch version.

Referring to the above example, forced resolutions might actually cause future breakage. Consider a case where http-proxy releases a breaking v2.0.0, and http-proxy-middleware upgrades, and the forced range is left in place (such configurations have a tendency to stick around)... Stuff can then break.. Yeah, yarn would warn about incompatible ranges. Developers love to ignore these warnings.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

tbc1 picture tbc1  路  4Comments

guillaume-at-palo picture guillaume-at-palo  路  4Comments

mbret picture mbret  路  5Comments

jsmylnycky picture jsmylnycky  路  3Comments

adi518 picture adi518  路  3Comments