fix: Make file: URLs (mostly) RFC 8909 compliant

The change made in e8c4301 was
reasonable, but had the unfortunate side effect of breaking backwards
compatibility for urls like 'file://.'.

Further investigation showed that we really weren't complying with RFC
8909's specifications regarding file: URLs, and even further
investigation than that showed that it'll be a breaking change if we do.

This patch isolates the non-8909 compliance bits into a single block
that can be disabled with an environment variable and easily removed
when we are willing to take on that breaking change.  The bug fixed by
the commit that broke file://. is still fixed, and we obey RFC8909
otherwise.

Author: isaacs

npa.js

@@ -6,7 +6,7 @@ module.exports.Result = Result
 const url = require('url')
 const HostedGit = require('hosted-git-info')
 const semver = require('semver')
-const path = require('path')
+const path = global.FAKE_WINDOWS ? require('path').win32 : require('path')
 const validatePackageName = require('validate-npm-package-name')
 const { homedir } = require('os')
 
@@ -151,42 +151,86 @@ function setGitCommittish (res, committish) {
   return res
 }
 
-const isAbsolutePath = /^[/]|^[A-Za-z]:/
-
-function resolvePath (where, spec) {
-  if (isAbsolutePath.test(spec))
-    return spec
-  return path.resolve(where, spec)
-}
-
-function isAbsolute (dir) {
-  if (dir[0] === '/')
-    return true
-  if (/^[A-Za-z]:/.test(dir))
-    return true
-  return false
-}
-
 function fromFile (res, where) {
   if (!where)
     where = process.cwd()
   res.type = isFilename.test(res.rawSpec) ? 'file' : 'directory'
   res.where = where
 
-  const spec = res.rawSpec.replace(/\\/g, '/')
-    .replace(/^file:[/]*([A-Za-z]:)/, '$1') // drive name paths on windows
-    .replace(/^file:(?:[/]*(~\/|\.*\/|[/]))?/, '$1')
-  if (/^~[/]/.test(spec)) {
-    // this is needed for windows and for file:~/foo/bar
-    res.fetchSpec = resolvePath(homedir(), spec.slice(2))
-    res.saveSpec = 'file:' + spec
-  } else {
-    res.fetchSpec = resolvePath(where, spec)
-    if (isAbsolute(spec))
-      res.saveSpec = 'file:' + spec
-    else
-      res.saveSpec = 'file:' + path.relative(where, res.fetchSpec)
+  // always put the '/' on where when resolving urls, or else
+  // file:foo from /path/to/bar goes to /path/to/foo, when we want
+  // it to be /path/to/foo/bar
+
+  let specUrl
+  let resolvedUrl
+  const prefix = (!/^file:/.test(res.rawSpec) ? 'file:' : '')
+  const rawWithPrefix = prefix + res.rawSpec
+  let rawNoPrefix = rawWithPrefix.replace(/^file:/, '')
+  try {
+    resolvedUrl = new url.URL(rawWithPrefix, `file://${path.resolve(where)}/`)
+    specUrl = new url.URL(rawWithPrefix)
+  } catch (originalError) {
+    const er = new Error('Invalid file: URL, must comply with RFC 8909')
+    throw Object.assign(er, {
+      raw: res.rawSpec,
+      spec: res,
+      where,
+      originalError,
+    })
+  }
+
+  // environment switch for testing
+  if (process.env.NPM_PACKAGE_ARG_8909_STRICT !== '1') {
+    // XXX backwards compatibility lack of compliance with 8909
+    // Remove when we want a breaking change to come into RFC compliance.
+    if (resolvedUrl.host && resolvedUrl.host !== 'localhost') {
+      const rawSpec = res.rawSpec.replace(/^file:\/\//, 'file:///')
+      resolvedUrl = new url.URL(rawSpec, `file://${path.resolve(where)}/`)
+      specUrl = new url.URL(rawSpec)
+      rawNoPrefix = rawSpec.replace(/^file:/, '')
+    }
+    // turn file:/../foo into file:../foo
+    if (/^\/\.\.?(\/|$)/.test(rawNoPrefix)) {
+      const rawSpec = res.rawSpec.replace(/^file:\//, 'file:')
+      resolvedUrl = new url.URL(rawSpec, `file://${path.resolve(where)}/`)
+      specUrl = new url.URL(rawSpec)
+      rawNoPrefix = rawSpec.replace(/^file:/, '')
+    }
+    // XXX end 8909 violation backwards compatibility section
   }
+
+  // file:foo - relative url to ./foo
+  // file:/foo - absolute path /foo
+  // file:///foo - absolute path to /foo, no authority host
+  // file://localhost/foo - absolute path to /foo, on localhost
+  // file://foo - absolute path to / on foo host (error!)
+  if (resolvedUrl.host && resolvedUrl.host !== 'localhost') {
+    const msg = `Invalid file: URL, must be absolute if // present`
+    throw Object.assign(new Error(msg), {
+      raw: res.rawSpec,
+      parsed: resolvedUrl,
+    })
+  }
+
+  // turn /C:/blah into just C:/blah on windows
+  let specPath = decodeURIComponent(specUrl.pathname)
+  let resolvedPath = decodeURIComponent(resolvedUrl.pathname)
+  if (isWindows) {
+    specPath = specPath.replace(/^\/+([a-z]:\/)/i, '$1')
+    resolvedPath = resolvedPath.replace(/^\/+([a-z]:\/)/i, '$1')
+  }
+
+  // replace ~ with homedir, but keep the ~ in the saveSpec
+  // otherwise, make it relative to where param
+  if (/^\/~(\/|$)/.test(specPath)) {
+    res.saveSpec = `file:${specPath.substr(1)}`
+    resolvedPath = path.resolve(homedir(), specPath.substr(3))
+  } else if (!path.isAbsolute(rawNoPrefix))
+    res.saveSpec = `file:${path.relative(where, resolvedPath)}`
+  else
+    res.saveSpec = `file:${path.resolve(resolvedPath)}`
+
+  res.fetchSpec = path.resolve(where, resolvedPath)
   return res
 }
 

test/basic.js

@@ -1,19 +1,24 @@
-var npa = require('../npa.js')
-var path = require('path')
-var os = require('os')
+const path = require('path').posix
+const os = require('os')
 
 const normalizePath = p => p && p.replace(/^[a-zA-Z]:/, '').replace(/\\/g, '/')
 
+const cwd = normalizePath(process.cwd())
+process.cwd = () => cwd
 const normalizePaths = spec => {
   spec.saveSpec = normalizePath(spec.saveSpec)
   spec.fetchSpec = normalizePath(spec.fetchSpec)
   return spec
 }
 
-require('tap').test('basic', function (t) {
+const t = require('tap')
+const npa = t.mock('../npa.js', { path })
+t.on('bailout', () => process.exit(1))
+
+t.test('basic', function (t) {
   t.setMaxListeners(999)
 
-  var tests = {
+  const tests = {
     'foo@1.2': {
       name: 'foo',
       escapedName: 'foo',
@@ -472,6 +477,14 @@ require('tap').test('basic', function (t) {
       raw: 'file:////path/to/foo',
     },
 
+    'file://.': {
+      name: null,
+      escapedName: null,
+      type: 'directory',
+      saveSpec: 'file:',
+      raw: 'file://.',
+    },
+
     'http://insecure.com/foo.tgz': {
       name: null,
       escapedName: null,
@@ -550,14 +563,14 @@ require('tap').test('basic', function (t) {
   }
 
   Object.keys(tests).forEach(function (arg) {
-    var res = normalizePaths(npa(arg, '/test/a/b'))
+    const res = normalizePaths(npa(arg, '/test/a/b'))
     t.ok(res instanceof npa.Result, arg + ' is a result')
     Object.keys(tests[arg]).forEach(function (key) {
       t.match(res[key], tests[arg][key], arg + ' [' + key + ']')
     })
   })
 
-  var objSpec = { name: 'foo', rawSpec: '1.2.3' }
+  let objSpec = { name: 'foo', rawSpec: '1.2.3' }
   t.equal(npa(objSpec, '/whatnot').toString(), 'foo@1.2.3', 'parsed object')
 
   objSpec.where = '/whatnot'
@@ -569,19 +582,23 @@ require('tap').test('basic', function (t) {
   objSpec = { raw: './foo/bar', where: '/here' }
   t.equal(normalizePath(npa(objSpec).fetchSpec), '/here/foo/bar', '`where` is reused')
 
-  var res = new npa.Result({ name: 'bar', rawSpec: './foo/bar' })
+  let res = new npa.Result({ name: 'bar', rawSpec: './foo/bar' })
   t.equal(res.toString(), 'bar@./foo/bar', 'toString with only rawSpec')
   res = new npa.Result({ rawSpec: './x/y' })
   t.equal(normalizePath(res.toString()), './x/y', 'toString with only rawSpec, no name')
   res = new npa.Result({ rawSpec: '' })
   t.equal(res.toString(), '', 'toString with nothing')
 
   objSpec = { raw: './foo/bar', where: '/here' }
-  t.equal(normalizePath(npa(objSpec, '/whatnot').fetchSpec), '/whatnot/foo/bar', '`where` arg overrides the one in the spec object')
+  t.equal(
+    normalizePath(npa(objSpec, '/whatnot').fetchSpec),
+    '/whatnot/foo/bar',
+    '`where` arg overrides the one in the spec object'
+  )
 
   t.equal(npa(npa('foo@1.2.3')).toString(), 'foo@1.2.3', 'spec is passthrough')
 
-  var parsedSpec = npa('./foo', './here')
+  const parsedSpec = npa('./foo', './here')
   t.equal(npa(parsedSpec), parsedSpec, 'reused if no where')
   t.equal(npa(parsedSpec, './here'), parsedSpec, 'reused if where matches')
   t.not(npa(parsedSpec, './there'), parsedSpec, 'new instance if where does not match')
@@ -607,14 +624,72 @@ require('tap').test('basic', function (t) {
     npa('foo@npm:foo/bar')
   }, 'aliases only work for registry deps')
 
-  t.has(npa.resolve('foo', '^1.2.3', '/test/a/b'), { type: 'range' }, 'npa.resolve')
-  t.has(normalizePaths(npa.resolve('foo', 'file:foo', '/test/a/b')), { type: 'directory', fetchSpec: '/test/a/b/foo' }, 'npa.resolve file:')
-  t.has(npa.resolve('foo', '../foo/bar', '/test/a/b'), { type: 'directory' }, 'npa.resolve no protocol')
-  t.has(npa.resolve('foo', 'file:../foo/bar', '/test/a/b'), { type: 'directory' }, 'npa.resolve file protocol')
-  t.has(npa.resolve('foo', 'file:../foo/bar.tgz', '/test/a/b'), { type: 'file' }, 'npa.resolve file protocol w/ tgz')
-  t.has(npa.resolve(null, '4.0.0', '/test/a/b'), { type: 'version', name: null }, 'npa.resolve with no name')
-  t.has(npa.resolve('foo', 'file:abc'), { type: 'directory', raw: 'foo@file:abc' }, 'npa.resolve sets raw right')
-  t.has(npa('./path/to/thing/package@1.2.3/'), { name: null, type: 'directory' }, 'npa with path in @ in it')
-  t.has(npa('path/to/thing/package@1.2.3'), { name: null, type: 'directory' }, 'npa w/o leading or trailing slash')
+  t.has(npa.resolve('foo', '^1.2.3', '/test/a/b'), {
+    type: 'range',
+  }, 'npa.resolve')
+  t.has(normalizePaths(npa.resolve('foo', 'file:foo', '/test/a/b')), {
+    type: 'directory',
+    fetchSpec: '/test/a/b/foo',
+  }, 'npa.resolve file:')
+  t.has(npa.resolve('foo', '../foo/bar', '/test/a/b'), {
+    type: 'directory',
+  }, 'npa.resolve no protocol')
+  t.has(npa.resolve('foo', 'file:../foo/bar', '/test/a/b'), {
+    type: 'directory',
+  }, 'npa.resolve file protocol')
+  t.has(npa.resolve('foo', 'file:../foo/bar.tgz', '/test/a/b'), {
+    type: 'file',
+  }, 'npa.resolve file protocol w/ tgz')
+  t.has(npa.resolve(null, '4.0.0', '/test/a/b'), {
+    type: 'version',
+    name: null,
+  }, 'npa.resolve with no name')
+  t.has(npa.resolve('foo', 'file:abc'), {
+    type: 'directory',
+    raw: 'foo@file:abc',
+  }, 'npa.resolve sets raw right')
+  t.has(npa('./path/to/thing/package@1.2.3/'), {
+    name: null,
+    type: 'directory',
+  }, 'npa with path in @ in it')
+  t.has(npa('path/to/thing/package@1.2.3'), {
+    name: null,
+    type: 'directory',
+  }, 'npa w/o leading or trailing slash')
+  t.end()
+})
+
+t.test('strict 8909 compliance mode', t => {
+  t.teardown(() => process.env.NPM_PACKAGE_ARG_8909_STRICT = '0')
+  process.env.NPM_PACKAGE_ARG_8909_STRICT = '1'
+
+  t.throws(() => npa('file://.'), {
+    message: 'Invalid file: URL, must be absolute if // present',
+    raw: 'file://.',
+  })
+
+  t.throws(() => npa('file://some/relative/path'), {
+    message: 'Invalid file: URL, must be absolute if // present',
+    raw: 'file://some/relative/path',
+  })
+
+  // I cannot for the life of me figure out how to make new URL('file:...')
+  // actually fail to parse.  it seems like it accepts any garbage you can
+  // throw at it.  However, because it theoretically CAN throw, here's a test.
+  t.throws(() => {
+    const npa = t.mock('../npa.js', {
+      url: {
+        URL: class {
+          constructor () {
+            throw new Error('thansk i haet it')
+          }
+        },
+      },
+    })
+    npa('file:thansk i haet it')
+  }, {
+    message: 'Invalid file: URL, must comply with RFC 8909',
+  })
+
   t.end()
 })