noraj commented 6 years ago

Update

Resolved in node-sass@4.9.3. Upgrade to quiet npm.

hoek@2.16.3 is vulnerable to CVE-2018-3728

for node-sass the problem comes from requiring request@2.79.0 in the package.json

dependency tree is as follow for 4.8.3

node-sass@4.8.3
|-request@2.79.0
 |-hawk@3.1.3
  |-hoek@2.16.3

and is the same for 4.9.0:

node-sass@4.9.0
|-request@2.79.0
 |-hawk@3.1.3
  |-hoek@2.16.3

Fix

To fix this request@2.82.0 or superior is required.

Context

NPM version (npm -v): 5.8.0
Node version (node -v): v9.11.1

Node Process (node -p process.versions):

{ http_parser: '2.8.0',
node: '9.11.1',
v8: '6.2.414.46-node.23',
uv: '1.19.2',
zlib: '1.2.11',
ares: '1.13.0',
modules: '59',
nghttp2: '1.29.0',
napi: '3',
openssl: '1.0.2o',
icu: '61.1',
unicode: '10.0',
cldr: '33.0',
tz: '2018c' }

Node Platform (node -p process.platform): linux
Node architecture (node -p process.arch): x64

node-sass version (node -p "require('node-sass').info"):

node-sass   4.9.0   (Wrapper)   [JavaScript]
libsass     3.5.4   (Sass Compiler) [C/C++]

npm node-sass versions (npm ls node-sass):

├─┬ gulp-sass@4.0.1
│ └── node-sass@4.9.0  deduped
└── node-sass@4.9.0

Related issues

request:

https://github.com/request/request/issues/2926 https://github.com/request/request/issues/2874

node-sass:

https://github.com/sass/node-sass/issues/2352 https://github.com/sass/node-sass/issues/2288 https://github.com/sass/node-sass/issues/2262 https://github.com/sass/node-sass/issues/2252 https://github.com/sass/node-sass/pull/2170 https://github.com/sass/node-sass/pull/2256

Problem

xzyfer in #2352

It cannot be fixed without break node < 4 support

I also see in #2288 that the problem is solved in node-sass v5.

So this ticket need to stay opened until v5 is released. Please don't close it.

samrispaud commented 6 years ago

i see the fix is on master :) is there a timeline on when the next major or minor release will be with this change?

xzyfer commented 6 years ago

Subscribe to the v5 pr. I'm in holidays now so not for a couple weeks.

On Mon., 7 May 2018, 10:11 pm Sam Rispaud, notifications@github.com wrote:

i see the fix is on master :) is there a timeline on when the next major or minor release will be with this change?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/sass/node-sass/issues/2355#issuecomment-387188864, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjZWHnkAOwGZ6HRxWYeb6rXfVHkiwRkks5twKpTgaJpZM4Tq1z4 .

noraj commented 6 years ago

@samrispaud

That's what I said ;-)

I also see in #2288 that the problem is solved in node-sass v5.

So this ticket need to stay opened until v5 is released. Please don't close it.

cloakedninjas commented 6 years ago

What about creating a 4.10.0 release that just bumps the request dependency ?

xzyfer commented 6 years ago

Bumping the dependency would break support for Node < 4

On Wed., 9 May 2018, 12:41 pm Daniel Jackson, notifications@github.com wrote:

What about creating a 4.10.0 release that just bumps the request dependency ?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/sass/node-sass/issues/2355#issuecomment-387698434, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjZWPrx7wQEUkTqYNHbDloFb4PBwGhAks5twsfagaJpZM4Tq1z4 .

noraj commented 6 years ago

@cloakedninjas It's a wait and see for v5 release.

chriseppstein commented 6 years ago

As I understand it, you can change the node engine version requirement and do a minor release. It doesn't violate semver for the people on the old node versions because the new versions of the package are excluded from that code's dependency version resolution.

xzyfer commented 6 years ago

My understanding is that the engine property has no actual effect beyond npm producing a warning. The install will still happen.

The way to resolve this error without breaking BC is to replace request all together with something that's supports Node 0.10+

On Wed., 9 May 2018, 9:22 pm Chris Eppstein, notifications@github.com wrote:

As I understand it, you can change the node engine version requirement and do a minor release. It doesn't violate semver for the people on the old node versions because the new versions of the package are excluded from that code's dependency version resolution.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/sass/node-sass/issues/2355#issuecomment-387847684, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjZWJUyGWTrunvxfUG5PIvAVtwZ4xNBks5tw0IQgaJpZM4Tq1z4 .

ataylorme commented 6 years ago

Node 4 became end of life on May 1st (source). Can you provide some background on the decision to continue supporting it after end of life rather than pushing out a security fix? I'm not disagreeing with any decision but rather seek to understand it. For example, are there a large number of Node 4 users?

noraj commented 6 years ago

Following this we should at least be under nodejs 6.x.

@ataylorme Personally I'm a archlinux user so it's nodejs 10.x for me :)

florianbrinkmann commented 6 years ago

node-gyp, another dependency of node-sass, also uses the old version of the request library. I asked if they would update it, but the answer was no. Does that mean that the security issue will remain because both request dependencies will be loaded (the updated one from node-sass and the old one from node-gyp)?

brlodi commented 6 years ago

node-gyp is targeting just "request": "2", which will match any version 2.x.x. It should use whatever version of request is pulled in by other dependencies (with deduplication) or the most recent 2.x.x release if nothing else in the tree requires it.

chriseppstein commented 6 years ago

@xzyfer huh, I guess that's true now that engine-strict defaults to false and engineStrict is no longer supported in package.json. that's super irritating.

florianbrinkmann commented 6 years ago

node-gyp is targeting just "request": "2", which will match any version 2.x.x. It should use whatever version of request is pulled in by other dependencies (with deduplication) or the most recent 2.x.x release if nothing else in the tree requires it.

Ah okay, did not understand that before (I got the info on the node-gyp repo, too), but now I got it, thanks! :)

lsimone commented 6 years ago

so, is there a way to solve the issue github is warning me about?

brlodi commented 6 years ago

@florianbrinkmann no worries, I was two sentences into a comment agreeing with you over on the node-gyp issue before my brain finished processing what a bare semver "2" meant

johndatserakis commented 6 years ago

@lsimone current consensus I think is to wait for the v5 merge https://github.com/sass/node-sass/pull/2312 as @xzyfer mentioned. I'm waiting for the same thing.

brlodi commented 6 years ago

@lsimone alternatively, if you need to fix it immediately and your project will support it, you can roll back to node-sass@4.7.0 exactly as mentioned in this comment on #2288.

ntwb commented 6 years ago

There is no version 4.7.0 published on npmjs.com, 4.6.9 and 4.7.1

https://www.npmjs.com/package/node-sass?activeTab=versions

There is a 4.7.0 tag here on GitHub though: https://github.com/sass/node-sass/tree/v4.7.0

lysla commented 6 years ago

hey, still confirmed we waiting v5 for this fix? aside of the 4.7.0 downgrade as a temporary workaround

generalov commented 6 years ago

Yarn users could use "resolutions" in package.json for temporary workaround.

 "resolutions": {
    "node-sass/**/request": "^2.82.0"
  }

https://yarnpkg.com/lang/en/docs/selective-version-resolutions/

Fingel commented 6 years ago

Bumping the dependency would break support for Node < 4

In my opinion, fixing security vulnerabilities should be higher priority than supporting deprecated runtimes. Now that Github sends out vulnerability reports, any project depending on node-sass has been looking pretty bad in the last 3 weeks.

nschonni commented 6 years ago

non official response as an issue janitor on this project We appreciate that security checkers are flagging the dependency, and that concerns you. request is used to download the binaries for our package only, but because of the prototype pollution aspect that can still be a concern. We have fallback binary download options through https://github.com/sass/node-sass#binary-configuration-parameters, and our version of request shouldn't interfere with your production version that is pinned to a better version of request. We are not going to break backwards compatibility in our v4, but we've already addressed this in the v5 branch. I only mop up issues, and @xzyfer is on a well deserved vacation, so chiming in that we need to address this isn't going to move this faster. I've tried to avoid ad hominem attacks, and I ask you all to do the same ❤️ ❤️ ❤️

chase-moskal commented 6 years ago

why hello everybody, i am the workaround fairy

update node-sass in your package.json to a commit in the v5 branch

"node-sass": "git+https://github.com/sass/node-sass.git#a8005ad3e34b3310536888c299a186623a178f80",

now the security warnings are gone, and i'm in the future :+1:

guidobouman commented 6 years ago

Hey @xzyfer & @nschonni,

How do you suggest we install 4.7.0, when that version is not published to NPM? See @ntwb's comment. We are unable to use the suggested downgrade workaround as that version just does not exist on NPM.

xzyfer commented 6 years ago

Look at the GitHub releases for one before we pinned the request package, and use a version < that one.

On Thu., 7 Jun. 2018, 8:35 am Guido Bouman, notifications@github.com wrote:

Hey @xzyfer https://github.com/xzyfer & @nschonni https://github.com/nschonni,

How do you suggest we install 4.7.0, when that version is not published to NPM? See @ntwb https://github.com/ntwb's comment. We are unable to use the suggested downgrade workaround as that version just does not exist on NPM.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sass/node-sass/issues/2355#issuecomment-395309348, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjZWMLEvCKLAORn7ho3y10EN8D_KQ0Yks5t6MnKgaJpZM4Tq1z4 .

guidobouman commented 6 years ago

That would mean using 4.6.1, instead of 4.7.0, missing out on the memory fixes in 4.7.0, right?

vernondegoede commented 6 years ago

@guidobouman Why not temporarily use the git tag? https://github.com/mollie/api-documentation/blob/master/package.json#L25

xzyfer commented 6 years ago

It would appear so. It's unfortunate but the request package breaking semver has left us little choice.

On Thu., 7 Jun. 2018, 11:41 am Vernon de Goede, notifications@github.com wrote:

@guidobouman https://github.com/guidobouman Why not temporarily use the git tag? https://github.com/mollie/api-documentation/blob/master/package.json#L25

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sass/node-sass/issues/2355#issuecomment-395359275, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjZWOfKixWmH_KRGKNvXsfQ8uQTwt4Pks5t6PVKgaJpZM4Tq1z4 .

guidobouman commented 6 years ago

Pushing 4.7.0 to npm would allow us to use the memory enhancements with the semver broken version of request, while default users will still get 4.7.1++.

@vernondegoede That means including an untagged, unstable version of the package. The package-lock.json can not lock this in properly, preventing predictable builds on our CI.

... 💡: On the other hand, I could use the 4.7.0 tag from git!

"node-sass": "git+https://github.com/sass/node-sass.git#v4.7.0",

xzyfer commented 6 years ago

If 4.7.0 is no longer published then it was unpublished for a reason. Also an unpublished version cannot be republished.

On Thu., 7 Jun. 2018, 12:52 pm Guido Bouman, notifications@github.com wrote:

Pushing 4.7.0 to npm would allow us to use the memory enhancements with the semver broken version of request, while default users will still get 4.7.1++.

@vernondegoede https://github.com/vernondegoede That means including an untagged, unstable version of the package. The package-lock.json can not lock this in properly, preventing predictable builds on our CI. On the other hand, I could use the 4.7.0 tag from git...

"node-sass": "git+https://github.com/sass/node-sass.git#4.7.0",

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sass/node-sass/issues/2355#issuecomment-395378560, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjZWG_N_Q-myQHRZQhKvPZu_jsw2rAiks5t6QX4gaJpZM4Tq1z4 .

guidobouman commented 6 years ago

With keeping 4.7.1 published I don't see why one would unpublish 4.7.0. Especially if this is the diff: https://github.com/sass/node-sass/compare/v4.7.0...v4.7.1 I guess it was because 4.7.0 broke semver along with request 2.81.0. Sure, but that's what v4.7.1 is for.

Anyhow, I'm using the git tag for now, please do not unpublish that as well.

matthew-white commented 6 years ago

In line with the comments about node-gyp above, I'd been able to pull in the latest version of request by specifying an earlier version of node-sass. However, node-gyp version 3.6.3 and on, released a week ago, pins request to <2.82.0, which means that I've had to pin node-gyp to 3.6.2 in addition to specifying an earlier version of node-sass. That's working well, but I'm wondering if that has any implications for node-sass v5. Is it possible that v5 would pin node-gyp or would wait on node-gyp v4, which will use the latest version of request?

The issue in the node-gyp repo is here: https://github.com/nodejs/node-gyp/issues/1439.

bnoordhuis commented 6 years ago

node-gyp maintainer here. I'd recommend dropping the explicit dependency on node-gyp in your package.json and instead use the node-gyp is bundled with npm and is implicitly available.

xzyfer commented 6 years ago

We've needed to bump the minimum version a couple times to work around node-gyp issues. I'm not sure we can safely rely on what is bundled with npm.

I can try digging up the specific issues when I'm back from vacation next week.

On Thu., 14 Jun. 2018, 8:33 pm Ben Noordhuis, notifications@github.com wrote:

node-gyp maintainer here. I'd recommend dropping the explicit dependency on node-gyp in your package.json and instead use the node-gyp is bundled with npm and is implicitly available.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sass/node-sass/issues/2355#issuecomment-397395712, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjZWDZ2e_qwXSYH-hwo8B_SaCgFW-7Vks5t8qyOgaJpZM4Tq1z4 .

Rohithzr commented 6 years ago

@xzyfer do the tests cover the issues you are mentioning? removing node-gyp gave me

6101 passing (1m)
580 pending

xzyfer commented 6 years ago

I don't believe we have tests for the scripts. We also cannot rely on Travis because it out tests the latest release of each version. So an older release of Node 4 might fail to install for instance.

I'll dig up an example issue.

On Thu., 14 Jun. 2018, 9:26 pm Rohit Hazra, notifications@github.com wrote:

@xzyfer https://github.com/xzyfer do the tests cover the issues you are mentioning? removing node-gyp gave me

6101 passing (1m) 580 pending

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sass/node-sass/issues/2355#issuecomment-397410803, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjZWPVPJTpFnIzpkpb1NFR1uCt3lP4Hks5t8rjpgaJpZM4Tq1z4 .

xzyfer commented 6 years ago

Actually it looks like it's been two years since we've had issue. It's probably safe to remove the node-gyp dependency. We'll give it a shot in the release, thanks

Rohithzr commented 6 years ago

Ill make a PR.

Rohithzr commented 6 years ago

@bnoordhuis removing node-gyp from dependencies is causing build failure, so is it possible that node-gyp is not installed with some node+npm installations?

xzyfer commented 6 years ago

It'll be related to how we call node-gyp in scripts/build.js

Rohithzr commented 6 years ago

@xzyfer ill try and update the script.

marbuser commented 6 years ago

Sorry but can someone explain to me if there is a workaround for this currently? Is there a specific way those of us using the latest version of node can use v5 currently or is this just a sit and wait type scenario?

Rohithzr commented 6 years ago

@xzyfer the script is working, but i had to do npm install -g node-gyp once.

var globalBinPath = spawn.sync('npm', ['bin', '-g'], { stdout: 'inherit' }).stdout.toString().trim();

  var args = [require.resolve(globalBinPath+'/node-gyp'), 'rebuild', '--verbose'].concat(
    ['libsass_ext', 'libsass_cflags', 'libsass_ldflags', 'libsass_library'].map(function(subject) {
      return ['--', subject, '=', process.env[subject.toUpperCase()] || ''].join('');
    })).concat(options.args);

Rohithzr commented 6 years ago

@xzyfer i was thinking of something like this, ill try to build this on CI

/**
 * find node-gyp
 */

function getNodeGyp(){
  var globalBinPath = spawn.sync('npm', ['bin', '-g'], { stdout: 'inherit' }).stdout.toString().trim();
  var localBinPath = spawn.sync('npm', ['bin'], { stdout: 'inherit' }).stdout.toString().trim();
  var nodeGypExec = null;

  try{
    nodeGypExec = require.resolve(globalBinPath+'/node-gyp');
  }catch(errorGlobal){
    console.error('unable to resolve node-gyp globally!`');
  }
  if(!nodeGypExec){
    try{
      nodeGypExec = require.resolve(localBinPath+'/node-gyp');
    }catch(errorGlobal){
      console.error('unable to resolve node-gyp locally!`');
    }
  }

  if(!nodeGypExec){
    console.error('unable to resolve node-gyp. Try running `npm install node-gyp -g`.');
  }
  return nodeGypExec;
}

Rohithzr commented 6 years ago

@xzyfer node-gyp is required to be installed either locally or globaly. see https://travis-ci.org/sass/node-sass/jobs/393589241

guidobouman commented 6 years ago

@marbuser I would suggest using a git tag for the time being;

"devDependencies": {
  "node-sass": "git+https://github.com/sass/node-sass.git#v4.7.0"
}

v4.7.0 is the most recent / performant stable version that allows installation of the fixed version of request.

Work on v5 seems a little stale. If you feel experimental you could try that one through git, referencing v5 behind the hash.

bra1n commented 6 years ago

@guidobouman that doesn't work:

└─┬ node-sass@4.7.0 (git+https://github.com/sass/node-sass.git#a51eca7f8bafdc9bafab76d2305fedc64503304c)
  └─┬ request@2.79.0
    └─┬ hawk@3.1.3
      ├─┬ boom@2.10.1
      │ └── hoek@2.16.3  deduped
      ├── hoek@2.16.3
      └─┬ sntp@1.0.9
        └── hoek@2.16.3  deduped

guidobouman commented 6 years ago

@bra1n Then you have a different package limiting the request version to 2.79.0

bra1n commented 6 years ago

@guidobouman Good point. I actually do have another package like that... it's node-sass@^4.8.3 from gulp-sass. Argh. Seems like they're aware of this issue.

sass / node-sass

Security issue in dependencies #2355

Update

Fix

Context

Related issues

Problem