Repository: lirantal/lockfile-lint
Pull request #12 (2019-10-11 20:49:27)

feat(schema): support protocol schema and refactor host validators
## Description

- Refactor current behavior of --allowed-hosts to actually use just the hostname instead of a full URL
- Introduces a protocol schema support to allow specifying protocols such as `https:` `git+ssh:` etc

## Types of changes

- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Related Issue

Addresses issues brought up in #11 

## Motivation and Context

#11 brought up interesting idea in terms of supporting protocols

## Checklist:

<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->

- [ ] I have updated the documentation (if required).
- [x] I have read the **CONTRIBUTING** document.
- [x] I have added tests to cover my changes.
- [x] All new and existing tests passed.
- [ ] I added a picture of a cute animal cause it's fun


lirantal: cc @nottoseethesun you might want to take a look
nottoseethesun: I can't try this out, because there's no way to run the program.

If I try to install locally, I get `Error: Cannot find module 'lockfile-lint-api'`.  There's no link in `node_modules/.bin`, and so I have to go into the actual package.

If I try to install globally or use npx, which I prefer to not do, then I get `-bash: lockfile-lint: command not found` or `lockfile-lint: command not found`.

nottoseethesun: Otherwise, lgtm, with the caveat that hopefully a few of the rest of the uri components could be supported soon, so that if a user wants to allow an npm package from github.com, they can limit it much more than all of github.   The most frequent use of `git+ssh` or `git+https` would occur though with intranet git installations, so in that case, a hostname of `github.foo.bar` would suffice, which appears to be supported here.
lirantal: @nottoseethesun yes, a bit tricky to get this tested on a local install with monorepo setup. Hopefully will have time later on to explain this out in the docs.

Thanks, appreciate the input!

Patch:
diff --git a/packages/lockfile-lint-api/index.js b/packages/lockfile-lint-api/index.js
index 4a86475..12adaef 100644
--- a/packages/lockfile-lint-api/index.js
+++ b/packages/lockfile-lint-api/index.js
@@ -2,10 +2,12 @@
 
 const ValidateHost = require('./src/validators/ValidateHost')
 const ValidateHttps = require('./src/validators/ValidateHttps')
+const ValidateScheme = require('./src/validators/ValidateScheme')
 const ParseLockfile = require('./src/ParseLockfile')
 
 module.exports = {
   ParseLockfile,
   ValidateHost,
-  ValidateHttps
+  ValidateHttps,
+  ValidateScheme
 }
diff --git a/packages/lockfile-lint-api/src/ParseLockfile.js b/packages/lockfile-lint-api/src/ParseLockfile.js
index dff8ebe..26784be 100644
--- a/packages/lockfile-lint-api/src/ParseLockfile.js
+++ b/packages/lockfile-lint-api/src/ParseLockfile.js
@@ -94,7 +94,7 @@ class ParseLockfile {
     for (const [depName, depMetadata] of Object.entries(npmDepsTree)) {
       const depMetadataShortend = {
         version: depMetadata.version,
-        resolved: depMetadata.resolved,
+        resolved: depMetadata.resolved ? depMetadata.resolved : depMetadata.version,
         integrity: depMetadata.integrity,
         requires: depMetadata.requires
       }
diff --git a/packages/lockfile-lint-api/src/validators/ValidateHost.js b/packages/lockfile-lint-api/src/validators/ValidateHost.js
index 6202d0b..c10517e 100644
--- a/packages/lockfile-lint-api/src/validators/ValidateHost.js
+++ b/packages/lockfile-lint-api/src/validators/ValidateHost.js
@@ -3,9 +3,9 @@
 const {URL} = require('url')
 
 const REGISTRY = {
-  npm: 'https://registry.npmjs.org',
-  yarn: 'https://registry.yarnpkg.com',
-  verdaccio: 'https://registry.verdaccio.org'
+  npm: 'registry.npmjs.org',
+  yarn: 'registry.yarnpkg.com',
+  verdaccio: 'registry.verdaccio.org'
 }
 
 module.exports = class ValidateHost {
@@ -35,10 +35,10 @@ module.exports = class ValidateHost {
         return REGISTRY[hostValue] ? REGISTRY[hostValue] : hostValue
       })
 
-      if (allowedHosts.indexOf(packageResolvedURL.origin) === -1) {
+      if (allowedHosts.indexOf(packageResolvedURL.host) === -1) {
         // throw new Error(`detected invalid origin for package: ${packageName}`)
         validationResult.errors.push({
-          message: `detected invalid origin for package: ${packageName}`,
+          message: `detected invalid host for package: ${packageName}`,
           package: packageName
         })
       }
diff --git a/packages/lockfile-lint-api/src/validators/ValidateScheme.js b/packages/lockfile-lint-api/src/validators/ValidateScheme.js
new file mode 100644
index 0000000..8edc3cf
--- /dev/null
+++ b/packages/lockfile-lint-api/src/validators/ValidateScheme.js
@@ -0,0 +1,41 @@
+'use strict'
+
+const {URL} = require('url')
+
+module.exports = class ValidateProtocol {
+  constructor ({packages} = {}) {
+    if (typeof packages !== 'object') {
+      throw new Error('expecting an object passed to validator constructor')
+    }
+
+    this.packages = packages
+  }
+
+  validate (schemes) {
+    if (!Array.isArray(schemes)) {
+      throw new Error('validate method requires an array')
+    }
+
+    let validationResult = {
+      type: 'success',
+      errors: []
+    }
+
+    for (const [packageName, packageMetadata] of Object.entries(this.packages)) {
+      const packageResolvedURL = new URL(packageMetadata.resolved)
+      if (schemes.indexOf(packageResolvedURL.protocol) === -1) {
+        // throw new Error(`detected invalid origin for package: ${packageName}`)
+        validationResult.errors.push({
+          message: `detected invalid scheme for package: ${packageName}`,
+          package: packageName
+        })
+      }
+    }
+
+    if (validationResult.errors.length !== 0) {
+      validationResult.type = 'error'
+    }
+
+    return validationResult
+  }
+}
diff --git a/packages/lockfile-lint/bin/lockfile-lint.js b/packages/lockfile-lint/bin/lockfile-lint.js
index 6b06ea7..e79703a 100755
--- a/packages/lockfile-lint/bin/lockfile-lint.js
+++ b/packages/lockfile-lint/bin/lockfile-lint.js
@@ -9,7 +9,8 @@ debug(`parsed the following CLI arguments: ${JSON.stringify(cli)}`)
 let validators = []
 const supportedValidators = new Map([
   ['allowed-hosts', 'validateHosts'],
-  ['validate-https', 'validateHttps']
+  ['validate-https', 'validateHttps'],
+  ['allowed-schemes', 'validateSchemes']
 ])
 
 for (const [commandArgument, commandValue] of Object.entries(cli)) {
diff --git a/packages/lockfile-lint/src/cli.js b/packages/lockfile-lint/src/cli.js
index 201ba12..e4ff3b9 100644
--- a/packages/lockfile-lint/src/cli.js
+++ b/packages/lockfile-lint/src/cli.js
@@ -4,7 +4,7 @@ const yargs = require('yargs')
 
 const argv = yargs
   .version()
-  .usage('Usage: lockfile-lint --path <path-to-lockfile>')
+  .usage('Usage: lockfile-lint --path <path-to-lockfile> --allowed-hosts yarn npm')
   .help('help')
   .alias('help', 'h')
   .options({
@@ -28,9 +28,18 @@ const argv = yargs
       alias: ['allowed-hosts'],
       type: 'array',
       describe: 'validates a whitelist of allowed hosts to be used for resources in the lockfile'
+    },
+    o: {
+      alias: ['allowed-schemes'],
+      type: 'array',
+      describe: 'validates a whitelist of allowed schemes to be used for resources in the lockfile'
     }
   })
   .example('lockfile-lint --path yarn.lock --validate-https')
+  .example('lockfile-lint --path yarn.lock --validate-https --allowed-hosts npm yarn verdaccio')
+  .example(
+    'lockfile-lint --path yarn.lock --allowed-schemes "https:" "git+ssh:" --allowed-hosts npm yarn verdaccio'
+  )
   .epilogue('curated by Liran Tal at https://github.com/lirantal/lockfile-lint').argv
 
 module.exports = argv
diff --git a/packages/lockfile-lint/src/main.js b/packages/lockfile-lint/src/main.js
index db4cbe7..8cce8f0 100644
--- a/packages/lockfile-lint/src/main.js
+++ b/packages/lockfile-lint/src/main.js
@@ -1,11 +1,16 @@
 'use strict'
 
 const debug = require('debug')('lockfile-lint')
-const {ValidateHostManager, ValidateHttpsManager} = require('../src/validators')
+const {
+  ValidateHostManager,
+  ValidateHttpsManager,
+  ValidateSchemeManager
+} = require('../src/validators')
 
 const validatorFunctions = new Map([
   ['validateHosts', ValidateHostManager],
-  ['validateHttps', ValidateHttpsManager]
+  ['validateHttps', ValidateHttpsManager],
+  ['validateSchemes', ValidateSchemeManager]
 ])
 
 function runValidators ({type, path, validators} = {}) {
diff --git a/packages/lockfile-lint/src/validators/index.js b/packages/lockfile-lint/src/validators/index.js
index c45e623..56cfc8d 100644
--- a/packages/lockfile-lint/src/validators/index.js
+++ b/packages/lockfile-lint/src/validators/index.js
@@ -1,11 +1,29 @@
 'use strict'
 
-const {ValidateHost, ParseLockfile, ValidateHttps} = require('lockfile-lint-api')
+const {ValidateHost, ParseLockfile, ValidateHttps, ValidateScheme} = require('lockfile-lint-api')
 const debug = require('debug')
 
 module.exports = {
   ValidateHostManager,
-  ValidateHttpsManager
+  ValidateHttpsManager,
+  ValidateSchemeManager
+}
+
+function ValidateSchemeManager ({path, type, validatorOptions}) {
+  debug('validate-scheme-manager')(
+    `invoked with validator options: ${JSON.stringify(validatorOptions)}`
+  )
+
+  const options = {
+    lockfilePath: path,
+    lockfileType: type
+  }
+
+  const parser = new ParseLockfile(options)
+  const lockfile = parser.parseSync()
+  const validator = new ValidateScheme({packages: lockfile.object})
+
+  return validator.validate(validatorOptions)
 }
 
 function ValidateHostManager ({path, type, validatorOptions}) {


Test patch:
diff --git a/packages/lockfile-lint-api/__tests__/validators.host.test.js b/packages/lockfile-lint-api/__tests__/validators.host.test.js
index 251c76b..5c96bb9 100644
--- a/packages/lockfile-lint-api/__tests__/validators.host.test.js
+++ b/packages/lockfile-lint-api/__tests__/validators.host.test.js
@@ -35,12 +35,8 @@ describe('Validator: Host', () => {
       type: 'error',
       errors: [
         {
-          message: 'detected invalid origin for package: @babel/code-frame',
+          message: 'detected invalid host for package: @babel/code-frame',
           package: '@babel/code-frame'
-        },
-        {
-          message: 'detected invalid origin for package: meow',
-          package: 'meow'
         }
       ]
     })
@@ -121,4 +117,29 @@ describe('Validator: Host', () => {
       validator.validate(null)
     }).toThrowError(`validate method requires an array`)
   })
+
+  it('validator should allow for git-based URLs', () => {
+    const mockedPackages = {
+      '@babel/code-frame': {
+        resolved: 'https://registry.yarnpkg.com/@babel/code-frame/-/code-frame-7.0.0.tgz'
+      },
+      meow: {
+        resolved: 'git+ssh://lirantal@github.com/lirantal/dockly.git#1234567890'
+      },
+      '@babel/generator': {
+        resolved: 'https://registry.npmjs.org/@babel/generator/-/generator-7.4.4.tgz'
+      }
+    }
+
+    const validator = new ValidatorHost({packages: mockedPackages})
+    expect(validator.validate(['npm', 'github.com'])).toEqual({
+      type: 'error',
+      errors: [
+        {
+          message: 'detected invalid host for package: @babel/code-frame',
+          package: '@babel/code-frame'
+        }
+      ]
+    })
+  })
 })
diff --git a/packages/lockfile-lint-api/__tests__/validators.scheme.test.js b/packages/lockfile-lint-api/__tests__/validators.scheme.test.js
new file mode 100644
index 0000000..94c16c9
--- /dev/null
+++ b/packages/lockfile-lint-api/__tests__/validators.scheme.test.js
@@ -0,0 +1,61 @@
+const ValidatorScheme = require('../src/validators/ValidateScheme')
+
+describe('Validator: Protocol', () => {
+  it('validator should throw an error when provided a string', () => {
+    expect(() => new ValidatorScheme('ss')).toThrowError()
+  })
+  it('validator should throw an error when provided null', () => {
+    expect(() => new ValidatorScheme(null)).toThrowError()
+  })
+  it('validator should throw an error when provided an object', () => {
+    expect(() => new ValidatorScheme({asda: 1234})).toThrowError()
+  })
+
+  it('validator should fail if finding a non-https resource', () => {
+    const allowedProtocols = ['https:']
+    const mockedPackages = {
+      '@babel/code-frame': {
+        resolved: 'https://registry.npmjs.org/@babel/code-frame/-/code-frame-7.0.0.tgz'
+      },
+      meow: {
+        resolved: 'http://registry.npmjs.org/meow/-/meow-4.0.1.tgz'
+      },
+      '@babel/generator': {
+        resolved: 'git+ssh://registry.npmjs.org/@babel/generator/-/generator-7.4.4.tgz'
+      }
+    }
+    const validator = new ValidatorScheme({packages: mockedPackages})
+    expect(validator.validate(allowedProtocols)).toEqual({
+      type: 'error',
+      errors: [
+        {
+          message: `detected invalid scheme for package: meow`,
+          package: 'meow'
+        },
+        {
+          message: `detected invalid scheme for package: @babel/generator`,
+          package: '@babel/generator'
+        }
+      ]
+    })
+  })
+
+  it('validator should succeed if resources match provided protocols', () => {
+    const mockedPackages = {
+      '@babel/code-frame': {
+        resolved: 'https://registry.npmjs.org/@babel/code-frame/-/code-frame-7.0.0.tgz'
+      },
+      meow: {
+        resolved: 'https://registry.npmjs.org/meow/-/meow-4.0.1.tgz'
+      },
+      '@babel/generator': {
+        resolved: 'git+ssh://registry.npmjs.org/@babel/generator/-/generator-7.4.4.tgz'
+      }
+    }
+    const validator = new ValidatorScheme({packages: mockedPackages})
+    expect(validator.validate(['https:', 'git+ssh:'])).toEqual({
+      type: 'success',
+      errors: []
+    })
+  })
+})
diff --git a/packages/lockfile-lint/__tests__/cli.test.js b/packages/lockfile-lint/__tests__/cli.test.js
index f067ca0..d0cf4f9 100644
--- a/packages/lockfile-lint/__tests__/cli.test.js
+++ b/packages/lockfile-lint/__tests__/cli.test.js
@@ -35,7 +35,7 @@ describe('CLI tests', () => {
     })
   })
 
-  test('Linting a file that has wrong origin should display an error message and use exit code 1', done => {
+  test('Linting a file that has wrong host should display an error message and use exit code 1', done => {
     const process = childProcess.spawn(cliExecPath, [
       '--type',
       'yarn',
@@ -59,7 +59,7 @@ describe('CLI tests', () => {
     })
   })
 
-  test('Linting a file that has wrong origin should return exit code 1', done => {
+  test('Linting a file that has wrong host should return exit code 1', done => {
     const process = childProcess.spawn(cliExecPath, [
       '--type',
       'yarn',
diff --git a/packages/lockfile-lint/__tests__/fixtures/package-lock-git-scheme.json b/packages/lockfile-lint/__tests__/fixtures/package-lock-git-scheme.json
new file mode 100644
index 0000000..5988aea
--- /dev/null
+++ b/packages/lockfile-lint/__tests__/fixtures/package-lock-git-scheme.json
@@ -0,0 +1,20 @@
+{
+  "name": "babas",
+  "version": "1.0.0",
+  "lockfileVersion": 1,
+  "requires": true,
+  "dependencies": {
+    "debug": {
+      "version": "4.1.1",
+      "resolved": "https://registry.npmjs.org/debug/-/debug-4.1.1.tgz",
+      "integrity": "sha512-pYAIzeRo8J6KPEaJ0VWOh5Pzkbw/RetuzehGM7QRRX5he4fPHx2rdKMB256ehJCkX+XRQm16eZLqLNS8RSZXZw==",
+      "requires": {
+        "ms": "^2.1.1"
+      }
+    },
+    "ms": {
+      "version": "git+https://github.com/zeit/ms.git#adf1eb282d29fe3c405d205a3854177b86a97c1f",
+      "from": "git+https://github.com/zeit/ms.git#master"
+    }
+  }
+}
diff --git a/packages/lockfile-lint/__tests__/fixtures/yarn-lock-schemes.lock b/packages/lockfile-lint/__tests__/fixtures/yarn-lock-schemes.lock
new file mode 100644
index 0000000..060f46e
--- /dev/null
+++ b/packages/lockfile-lint/__tests__/fixtures/yarn-lock-schemes.lock
@@ -0,0 +1,19 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
+# yarn lockfile v1
+
+
+debug@^4.1.1:
+  version "4.1.1"
+  resolved "https://registry.yarnpkg.com/debug/-/debug-4.1.1.tgz#3b72260255109c6b589cee050f1d516139664791"
+  integrity sha512-pYAIzeRo8J6KPEaJ0VWOh5Pzkbw/RetuzehGM7QRRX5he4fPHx2rdKMB256ehJCkX+XRQm16eZLqLNS8RSZXZw==
+  dependencies:
+    ms "^2.1.1"
+
+ms@^2.1.1:
+  version "2.1.2"
+  resolved "https://registry.yarnpkg.com/ms/-/ms-2.1.2.tgz#d09d1f357b443f493382a8eb3ccd183872ae6009"
+  integrity sha512-sGkPx+VjMtmA6MX27oA4FBFELFCZZ4S4XqeGOXCv68tT+jb3vk/RyaKWP0PTKyWtmLSM0b+adUTEvbs1PEaH2w==
+
+"ms@git+https://github.com/zeit/ms#master":
+  version "2.1.2"
+  resolved "git+https://github.com/zeit/ms#adf1eb282d29fe3c405d205a3854177b86a97c1f"
diff --git a/packages/lockfile-lint/__tests__/main.test.js b/packages/lockfile-lint/__tests__/main.test.js
index f6657c0..dc34c6f 100644
--- a/packages/lockfile-lint/__tests__/main.test.js
+++ b/packages/lockfile-lint/__tests__/main.test.js
@@ -118,4 +118,90 @@ describe('Main CLI logic', () => {
       expect(result.validatorSuccesses).toEqual(1)
     })
   })
+
+  describe('validateSchemes', () => {
+    test('should fail validating allowed schemes for a package-lock.json file', () => {
+      const lockfilePath = `${__dirname}/fixtures/package-lock-git-scheme.json`
+      const lockfileType = 'npm'
+      const validators = [
+        {
+          name: 'validateSchemes',
+          options: ['https']
+        }
+      ]
+
+      const result = main.runValidators({
+        path: lockfilePath,
+        type: lockfileType,
+        validators
+      })
+
+      expect(result.validatorFailures).toEqual(2)
+      expect(result.validatorCount).toEqual(1)
+      expect(result.validatorSuccesses).toEqual(0)
+    })
+
+    test('should succeed validating allowed schemes for a package-lock.json file', () => {
+      const lockfilePath = `${__dirname}/fixtures/package-lock-git-scheme.json`
+      const lockfileType = 'npm'
+      const validators = [
+        {
+          name: 'validateSchemes',
+          options: ['https:', 'git+https:']
+        }
+      ]
+
+      const result = main.runValidators({
+        path: lockfilePath,
+        type: lockfileType,
+        validators
+      })
+
+      expect(result.validatorFailures).toEqual(0)
+      expect(result.validatorCount).toEqual(1)
+      expect(result.validatorSuccesses).toEqual(1)
+    })
+
+    test('should fail validating allowed schemes for a yarn.lock file', () => {
+      const lockfilePath = `${__dirname}/fixtures/yarn-lock-schemes.lock`
+      const lockfileType = 'yarn'
+      const validators = [
+        {
+          name: 'validateSchemes',
+          options: ['https']
+        }
+      ]
+
+      const result = main.runValidators({
+        path: lockfilePath,
+        type: lockfileType,
+        validators
+      })
+
+      expect(result.validatorFailures).toEqual(3)
+      expect(result.validatorCount).toEqual(1)
+      expect(result.validatorSuccesses).toEqual(0)
+    })
+
+    test('should succeed validating allowed schemes for a yarn.lock file', () => {
+      const lockfilePath = `${__dirname}/fixtures/yarn-lock-schemes.lock`
+      const lockfileType = 'yarn'
+      const validators = [
+        {
+          name: 'validateSchemes',
+          options: ['https:', 'git+https:']
+        }
+      ]
+
+      const result = main.runValidators({
+        path: lockfilePath,
+        type: lockfileType,
+        validators
+      })
+
+      expect(result.validatorFailures).toEqual(0)
+      expect(result.validatorCount).toEqual(1)
+      expect(result.validatorSuccesses).toEqual(1)
+    })
+  })
 })