krakenjs / lusca

Application security for express apps.
Other
1.79k stars 123 forks source link

Remove "engineStrict" in preparation for npm 3+ #47

Open totherik opened 9 years ago

totherik commented 9 years ago

Upon install:

npm WARN engineStrict Per-package engineStrict (found in package.json for lusca)
npm WARN engineStrict won't be used in npm 3+. Use the config setting `engine-strict` instead.
Trott commented 9 years ago

Not sure if it's a feature or a bug, but due to engineStrict, the current version of lusca does not install with node 4.0.0 rc1. (node 4.0.0 is due to come out Monday, 07-Sep-2015.)

This means that kraken-js will not install with version 4.0.0.

$ node -v
v4.0.0-rc.1
$ npm -v
2.14.2
$ npm install lusca@1.3.0
npm WARN package.json helloworld@0.1.0 No repository field.
npm WARN package.json helloworld@0.1.0 No license field.
npm ERR! Darwin 14.5.0
npm ERR! argv "/Users/trott/.nvm/versions/node/v4.0.0-rc.1/bin/node" "/Users/trott/.nvm/versions/node/v4.0.0-rc.1/bin/npm" "install" "lusca@1.3.0"
npm ERR! node v4.0.0-rc.1
npm ERR! npm  v2.14.2
npm ERR! code ENOTSUP

npm ERR! notsup Unsupported
npm ERR! notsup Not compatible with your version of node/npm: lusca@1.3.0
npm ERR! notsup Required: {"node":">=0.8.x"}
npm ERR! notsup Actual:   {"npm":"2.14.2","node":"4.0.0-rc.1"}

npm ERR! Please include the following file with any support request:
npm ERR!     /Users/trott/HelloWorld/npm-debug.log
$
Trott commented 9 years ago

In case anyone is really into details, here's the npm-debug.log. This is in a project generated with the kraken-js Yeoman generator, so there's a little bit of extra noise in the log.

0 info it worked if it ends with ok
1 verbose cli [ '/Users/trott/.nvm/versions/node/v4.0.0-rc.1/bin/node',
1 verbose cli   '/Users/trott/.nvm/versions/node/v4.0.0-rc.1/bin/npm',
1 verbose cli   'install',
1 verbose cli   'lusca@1.3.0' ]
2 info using npm@2.14.2
3 info using node@v4.0.0-rc.1
4 verbose install initial load of /Users/trott/HelloWorld/package.json
5 warn package.json helloworld@0.1.0 No repository field.
6 warn package.json helloworld@0.1.0 No license field.
7 verbose installManyTop reading scoped package data from /Users/trott/HelloWorld/node_modules/construx/package.json
8 verbose installManyTop reading scoped package data from /Users/trott/HelloWorld/node_modules/construx-copier/package.json
9 verbose installManyTop reading scoped package data from /Users/trott/HelloWorld/node_modules/express/package.json
10 verbose installManyTop reading scoped package data from /Users/trott/HelloWorld/node_modules/grunt/package.json
11 verbose installManyTop reading scoped package data from /Users/trott/HelloWorld/node_modules/grunt-cli/package.json
12 verbose installManyTop reading scoped package data from /Users/trott/HelloWorld/node_modules/grunt-config-dir/package.json
13 verbose installManyTop reading scoped package data from /Users/trott/HelloWorld/node_modules/grunt-contrib-clean/package.json
14 verbose installManyTop reading scoped package data from /Users/trott/HelloWorld/node_modules/grunt-contrib-jshint/package.json
15 verbose installManyTop reading scoped package data from /Users/trott/HelloWorld/node_modules/grunt-copy-to/package.json
16 verbose installManyTop reading scoped package data from /Users/trott/HelloWorld/node_modules/grunt-mocha-cli/package.json
17 verbose installManyTop reading scoped package data from /Users/trott/HelloWorld/node_modules/mocha/package.json
18 verbose installManyTop reading scoped package data from /Users/trott/HelloWorld/node_modules/supertest/package.json
19 info package.json construx-copier@1.0.0 license should be a valid SPDX license expression
20 info package.json construx@1.0.0 No license field.
21 info package.json grunt@0.4.5 No license field.
22 info package.json grunt-cli@0.1.13 No license field.
23 info package.json grunt-config-dir@0.3.2 No license field.
24 info package.json grunt-contrib-clean@0.6.0 No license field.
25 info package.json grunt-contrib-jshint@0.10.0 No license field.
26 info package.json mocha@1.21.5 No license field.
27 verbose readDependencies loading dependencies from /Users/trott/HelloWorld/package.json
28 silly cache add args [ 'lusca@1.3.0', null ]
29 verbose cache add spec lusca@1.3.0
30 silly cache add parsed spec Result {
30 silly cache add   raw: 'lusca@1.3.0',
30 silly cache add   scope: null,
30 silly cache add   name: 'lusca',
30 silly cache add   rawSpec: '1.3.0',
30 silly cache add   spec: '1.3.0',
30 silly cache add   type: 'version' }
31 silly addNamed lusca@1.3.0
32 verbose addNamed "1.3.0" is a plain semver version for lusca
33 silly mapToRegistry name lusca
34 silly mapToRegistry using default registry
35 silly mapToRegistry registry https://registry.npmjs.org/
36 silly mapToRegistry uri https://registry.npmjs.org/lusca
37 verbose addNameVersion registry:https://registry.npmjs.org/lusca not in flight; fetching
38 verbose request uri https://registry.npmjs.org/lusca
39 verbose request no auth needed
40 info attempt registry request try #1 at 3:16:52 PM
41 verbose request id ac96349f0b9844ce
42 verbose etag "C6PL1LX5GUQOXN2DE3KKBNHW1"
43 http request GET https://registry.npmjs.org/lusca
44 http 304 https://registry.npmjs.org/lusca
45 silly get cb [ 304,
45 silly get   { date: 'Sun, 06 Sep 2015 22:16:52 GMT',
45 silly get     via: '1.1 varnish',
45 silly get     'cache-control': 'max-age=60',
45 silly get     etag: '"C6PL1LX5GUQOXN2DE3KKBNHW1"',
45 silly get     age: '0',
45 silly get     connection: 'keep-alive',
45 silly get     'x-served-by': 'cache-lax1425-LAX',
45 silly get     'x-cache': 'HIT',
45 silly get     'x-cache-hits': '1',
45 silly get     'x-timer': 'S1441577812.645293,VS0,VE46',
45 silly get     vary: 'Accept' } ]
46 verbose etag https://registry.npmjs.org/lusca from cache
47 verbose get saving lusca to /Users/trott/.npm/registry.npmjs.org/lusca/.cache.json
48 silly cache afterAdd lusca@1.3.0
49 verbose afterAdd /Users/trott/.npm/lusca/1.3.0/package/package.json not in flight; writing
50 verbose afterAdd /Users/trott/.npm/lusca/1.3.0/package/package.json written
51 silly install resolved [ { name: 'lusca',
51 silly install resolved     version: '1.3.0',
51 silly install resolved     description: 'Application security for express.',
51 silly install resolved     main: 'index',
51 silly install resolved     scripts: { test: 'grunt test' },
51 silly install resolved     repository:
51 silly install resolved      { type: 'git',
51 silly install resolved        url: 'git+https://github.com/krakenjs/lusca.git' },
51 silly install resolved     author: { name: 'Jeff Harrell', email: 'jeharrell@paypal.com' },
51 silly install resolved     publishConfig: { registry: 'https://registry.npmjs.org' },
51 silly install resolved     licenses: [ [Object] ],
51 silly install resolved     engines: { node: '>=0.8.x' },
51 silly install resolved     engineStrict: true,
51 silly install resolved     devDependencies:
51 silly install resolved      { 'body-parser': '^1.6.3',
51 silly install resolved        'cookie-parser': '^1.3.2',
51 silly install resolved        'cookie-session': '^1.0.2',
51 silly install resolved        'data-driven': '^1.0.0',
51 silly install resolved        errorhandler: '^1.1.1',
51 silly install resolved        express: '^4.3.8',
51 silly install resolved        'express-session': '^1.7.5',
51 silly install resolved        grunt: '~0.4.1',
51 silly install resolved        'grunt-contrib-jshint': '~0.7.0',
51 silly install resolved        'grunt-mocha-test': '~0.7.0',
51 silly install resolved        jshint: '*',
51 silly install resolved        supertest: '^0.13.0' },
51 silly install resolved     gitHead: '6c9a4663a58448497acd7e4aee7f35ae2f47e55d',
51 silly install resolved     bugs: { url: 'https://github.com/krakenjs/lusca/issues' },
51 silly install resolved     homepage: 'https://github.com/krakenjs/lusca#readme',
51 silly install resolved     _id: 'lusca@1.3.0',
51 silly install resolved     _shasum: '637986bbc43ab98f1a850b86b665696b5ae5e159',
51 silly install resolved     _from: 'lusca@1.3.0',
51 silly install resolved     _npmVersion: '2.12.1',
51 silly install resolved     _nodeVersion: '0.12.7',
51 silly install resolved     _npmUser: { name: 'jasisk', email: 'jasisk@gmail.com' },
51 silly install resolved     maintainers: [ [Object], [Object], [Object], [Object], [Object] ],
51 silly install resolved     dist:
51 silly install resolved      { shasum: '637986bbc43ab98f1a850b86b665696b5ae5e159',
51 silly install resolved        tarball: 'http://registry.npmjs.org/lusca/-/lusca-1.3.0.tgz' },
51 silly install resolved     directories: {},
51 silly install resolved     _resolved: 'https://registry.npmjs.org/lusca/-/lusca-1.3.0.tgz',
51 silly install resolved     readme: 'ERROR: No README data found!' } ]
52 info install lusca@1.3.0 into /Users/trott/HelloWorld
53 info installOne lusca@1.3.0
54 verbose installOne of lusca to /Users/trott/HelloWorld not in flight; installing
55 verbose lock using /Users/trott/.npm/_locks/lusca-6e1af7cdf062972e.lock for /Users/trott/HelloWorld/node_modules/lusca
56 verbose unlock done using /Users/trott/.npm/_locks/lusca-6e1af7cdf062972e.lock for /Users/trott/HelloWorld/node_modules/lusca
57 verbose stack Error: Unsupported
57 verbose stack     at checkEngine (/Users/trott/.nvm/versions/node/v4.0.0-rc.1/lib/node_modules/npm/node_modules/npm-install-checks/index.js:16:16)
57 verbose stack     at Array.<anonymous> (/Users/trott/.nvm/versions/node/v4.0.0-rc.1/lib/node_modules/npm/node_modules/slide/lib/bind-actor.js:15:8)
57 verbose stack     at LOOP (/Users/trott/.nvm/versions/node/v4.0.0-rc.1/lib/node_modules/npm/node_modules/slide/lib/chain.js:15:14)
57 verbose stack     at chain (/Users/trott/.nvm/versions/node/v4.0.0-rc.1/lib/node_modules/npm/node_modules/slide/lib/chain.js:20:5)
57 verbose stack     at /Users/trott/.nvm/versions/node/v4.0.0-rc.1/lib/node_modules/npm/lib/install.js:1038:5
57 verbose stack     at /Users/trott/.nvm/versions/node/v4.0.0-rc.1/lib/node_modules/npm/lib/utils/locker.js:39:7
57 verbose stack     at cb (/Users/trott/.nvm/versions/node/v4.0.0-rc.1/lib/node_modules/npm/node_modules/lockfile/lockfile.js:149:38)
57 verbose stack     at /Users/trott/.nvm/versions/node/v4.0.0-rc.1/lib/node_modules/npm/node_modules/lockfile/lockfile.js:171:16
57 verbose stack     at /Users/trott/.nvm/versions/node/v4.0.0-rc.1/lib/node_modules/npm/node_modules/graceful-fs/graceful-fs.js:42:10
57 verbose stack     at FSReqWrap.oncomplete (fs.js:82:15)
58 verbose pkgid lusca@1.3.0
59 verbose cwd /Users/trott/HelloWorld
60 error Darwin 14.5.0
61 error argv "/Users/trott/.nvm/versions/node/v4.0.0-rc.1/bin/node" "/Users/trott/.nvm/versions/node/v4.0.0-rc.1/bin/npm" "install" "lusca@1.3.0"
62 error node v4.0.0-rc.1
63 error npm  v2.14.2
64 error code ENOTSUP
65 error notsup Unsupported
65 error notsup Not compatible with your version of node/npm: lusca@1.3.0
65 error notsup Required: {"node":">=0.8.x"}
65 error notsup Actual:   {"npm":"2.14.2","node":"4.0.0-rc.1"}
66 verbose exit [ 1, true ]
thefourtheye commented 9 years ago

@Trott Looks like its a bug in npm. Ideally 4.0.0-rc.1 is GTE 0.8.x.

Trott commented 9 years ago

@thefourtheye Uh...good point, actually. Wonder if it's a bug in npm 2.14.2 or just a bug in the slightly funky/hacked version that had to be distributed with RC1...

aredridel commented 9 years ago

It is not exactly. -pre releases don't match anything but themselves.

Trott commented 9 years ago

Cool, so basically, everything I've said everywhere above is completely wrong. Excellent. As you were.

Trott commented 9 years ago

You will likely be unsurprised to hear that it works just fine with node 3.3.0/npm 2.13.3. So everything is awesome after all.

thefourtheye commented 9 years ago

I feel that this is a bug in semver module. It simply says the 4.0.0-rc.1 doesn't satisfy >=0.8.x just because >=0.8.x doesn't have pre-release tags. But pre-release versions should be compared only when the major, minor and patch versions match.

aredridel commented 9 years ago

Yeah. For >=, this surprises me. For more strict comparison, the existing behavior makes sense, but I find it surprising for very general matches like >=

thefourtheye commented 9 years ago

The section responsible for this decision is https://github.com/npm/node-semver/blob/v5.0.1/semver.js#L1062-L1083

  if (version.prerelease.length) {
    // Find the set of versions that are allowed to have prereleases
    // For example, ^1.2.3-pr.1 desugars to >=1.2.3-pr.1 <2.0.0
    // That should allow `1.2.3-pr.2` to pass.
    // However, `1.2.4-alpha.notready` should NOT be allowed,
    // even though it's within the range set by the comparators.
    for (var i = 0; i < set.length; i++) {
      debug(set[i].semver);
      if (set[i].semver === ANY)
        continue;

      if (set[i].semver.prerelease.length > 0) {
        var allowed = set[i].semver;
        if (allowed.major === version.major &&
            allowed.minor === version.minor &&
            allowed.patch === version.patch)
          return true;
      }
    }

    // Version has a -pre, but it's not one of the ones we like.
    return false;
thefourtheye commented 9 years ago

In this case, version is 4.0.0-rc.1 and set is an array of length 1, with set[0] being <SemVer "0.8.0">. set[i].semver.prerelease is an empty array. So, we skip the for and return false.

thefourtheye commented 9 years ago

But node-semver docs, clearly says this case will fail.

If a version has a prerelease tag (for example, 1.2.3-alpha.3) then it will only be allowed to satisfy comparator sets if at least one comparator with the same [major, minor, patch] tuple also has a prerelease tag.

In our case, version is 4.0.0-rc.1 and comparator is 8.0.0 :'(

isaacs commented 9 years ago

According to the SemVer specification, prerelease versions are not to be used by general public, and are considered "unstable", carrying a higher than normal risk of unintended breaking changes. In particular, they may be missing features that are expected to exist in the official release, so even very broad ranges are liable to violate expectations if prerelease versions match.

In consideration of this, the semver npm module does not match any version range against a version with a prerelease tag, unless the range specifier itself contains a prerelease of the same tuple, indicating that the consumer is specifically opting into this prerelease version family, and acknowledges the higher level of risk.

When there is an official 4.0.0 release, it will match against >=0.8.0. However, until then, the warnings are doing their job, and alerting the user to a not-yet-blessed version of the platform.

In the fast and furious world of small module authorship, this restriction is important and necessary in practice. However, since Node moves much more deliberately, and is extremely conservative with respect to API change, so the restriction feels overly cautious. Engines are not like normal dependencies.

That is the reason why npm v3 removed the author-specified engineStrict field in package.json; it is virtually impossible for the author of a module to know whether or not their module will work with future versions of Node, and while a warning may be indicated, failing to install makes it overly difficult to even determine if the module works on the new platform prior to official launch!

thefourtheye commented 9 years ago

failing to install makes it overly difficult to even determine if the module works on the new platform prior to official launch!

This is exactly what I had in my mind. If the modules fail to install, there is no point in doing nightly and RC builds. Glad that npm v3 just issues a warning. Will the semver module also relax its rules and allow valid comparisons like this case?

thefourtheye commented 9 years ago

@issacs Related https://github.com/nodejs/node/issues/2223

isaacs commented 9 years ago

Will the semver module also relax its rules and allow valid comparisons like this case?

No. As I explained above, this is working as designed, and following the intent of the SemVer specification. The semver module is behaving as designed. I recommend upgrading to npm@3, and ignoring or disabling the warnings if you know that they are not relevant.

thefourtheye commented 9 years ago

@issacs Cool. Thanks for explaining :-)