fix: properly parse paths with non-url characters

Also removed the magic windows global for testing, fixing the tests to mock process.platform instead.

Closes: #193

lib/npa.js

@@ -4,15 +4,16 @@ module.exports.resolve = resolve
 module.exports.toPurl = toPurl
 module.exports.Result = Result
 
-const { URL } = require('url')
+const isWindows = process.platform === 'win32'
+
+const { URL, pathToFileURL } = require('node:url')
+const path = isWindows ? require('node:path').win32 : require('node:path')
+const { homedir } = require('node:os')
 const HostedGit = require('hosted-git-info')
 const semver = require('semver')
-const path = global.FAKE_WINDOWS ? require('path').win32 : require('path')
 const validatePackageName = require('validate-npm-package-name')
-const { homedir } = require('os')
 const { log } = require('proc-log')
 
-const isWindows = process.platform === 'win32' || global.FAKE_WINDOWS
 const hasSlashes = isWindows ? /\\|[/]/ : /[/]/
 const isURL = /^(?:git[+])?[a-z]+:/i
 const isGit = /^[^@]+@[^:.]+\.[^:]+:.+$/i
@@ -54,7 +55,30 @@ function npa (arg, where) {
   return resolve(name, spec, where, arg)
 }
 
-const isFilespec = isWindows ? /^(?:[.]|~[/]|[/\\]|[a-zA-Z]:)/ : /^(?:[.]|~[/]|[/]|[a-zA-Z]:)/
+function fileURLFromWhere (url, where) {
+  // always put the '/' on where when resolving where, or else file:foo from /path/to/bar goes to /path/to/foo, when we want it to be /path/to/bar/foo
+  return new URL(url, `${pathToFileURL(`${path.resolve(where)}`, { windows: isWindows }).href}/`)
+}
+
+function isFileSpec (spec) {
+  if (!spec) {
+    return false
+  }
+  if (spec.toLowerCase().startsWith('file:')) {
+    return true
+  }
+  if (isWindows) {
+    return /^(?:[.]|~[/]|[/\\]|[a-zA-Z]:)/.test(spec)
+  }
+  return /^(?:[.]|~[/]|[/]|[a-zA-Z]:)/.test(spec)
+}
+
+function isAliasSpec (spec) {
+  if (!spec) {
+    return false
+  }
+  return spec.toLowerCase().startsWith('npm:')
+}
 
 function resolve (name, spec, where, arg) {
   const res = new Result({
@@ -68,9 +92,9 @@ function resolve (name, spec, where, arg) {
     res.setName(name)
   }
 
-  if (spec && (isFilespec.test(spec) || /^file:/i.test(spec))) {
+  if (isFileSpec(spec)) {
     return fromFile(res, where)
-  } else if (spec && /^npm:/i.test(spec)) {
+  } else if (isAliasSpec(spec)) {
     return fromAlias(res, where)
   }
 
@@ -235,17 +259,13 @@ function fromFile (res, where) {
   res.type = isFilename.test(res.rawSpec) ? 'file' : 'directory'
   res.where = where
 
-  // 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/bar/foo
-
   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(rawWithPrefix, `file://${path.resolve(where)}/`)
+    resolvedUrl = fileURLFromWhere(rawWithPrefix, where)
     specUrl = new URL(rawWithPrefix)
   } catch (originalError) {
     const er = new Error('Invalid file: URL, must comply with RFC 8089')
@@ -260,7 +280,7 @@ function fromFile (res, where) {
   // XXX backwards compatibility lack of compliance with RFC 8089
   if (resolvedUrl.host && resolvedUrl.host !== 'localhost') {
     const rawSpec = res.rawSpec.replace(/^file:\/\//, 'file:///')
-    resolvedUrl = new URL(rawSpec, `file://${path.resolve(where)}/`)
+    resolvedUrl = fileURLFromWhere(rawSpec, where)
     specUrl = new URL(rawSpec)
     rawNoPrefix = rawSpec.replace(/^file:/, '')
   }
@@ -269,7 +289,7 @@ function fromFile (res, where) {
   // in the previous step to make it a file protocol url with a leading slash
   if (/^\/{1,3}\.\.?(\/|$)/.test(rawNoPrefix)) {
     const rawSpec = res.rawSpec.replace(/^file:\/{1,3}/, 'file:')
-    resolvedUrl = new URL(rawSpec, `file://${path.resolve(where)}/`)
+    resolvedUrl = fileURLFromWhere(rawSpec, where)
     specUrl = new URL(rawSpec)
     rawNoPrefix = rawSpec.replace(/^file:/, '')
   }

test/basic.js

@@ -1,5 +1,5 @@
-const path = require('path').posix
-const os = require('os')
+const path = require('node:path').posix
+const os = require('node:os')
 
 const normalizePath = p => p && p.replace(/^[a-zA-Z]:/, '').replace(/\\/g, '/')
 
@@ -13,7 +13,6 @@ const normalizePaths = spec => {
 
 const t = require('tap')
 const npa = t.mock('..', { path })
-t.on('bailout', () => process.exit(1))
 
 t.test('basic', function (t) {
   t.setMaxListeners(999)
@@ -635,10 +634,13 @@ t.test('basic', function (t) {
   }
 
   Object.keys(tests).forEach(function (arg) {
-    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 + ']')
+    t.test(arg, t => {
+      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 + ']')
+      })
+      t.end()
     })
   })
 
@@ -735,9 +737,25 @@ t.test('basic', function (t) {
   t.end()
 })
 
+t.test('cwd paths with non URI compatible components', t => {
+  // eslint-disable-next-line max-len
+  const where = '/tmp/ !"$%&\'()*+,-.0123456789:;<=>@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~'
+  const originalCwd = process.cwd
+  t.teardown(() => {
+    process.cwd = originalCwd
+  })
+  process.cwd = () => where
+  t.has(npa('./'), {
+    name: null,
+    type: 'directory',
+    where,
+  })
+  t.end()
+})
+
 t.test('invalid url', t => {
   const broken = t.mock('..', {
-    url: {
+    'node:url': {
       URL: class {
         constructor () {
           throw new Error('something went wrong')

test/windows.js

@@ -1,9 +1,13 @@
-global.FAKE_WINDOWS = true
+// redefine process.platform before any requires so that we don't cache a require that got the non-redefined value
+const { platform } = process
+Object.defineProperty(process, 'platform', { value: 'win32' })
 
-const npa = require('..')
 const t = require('tap')
+const npa = require('..')
 
-t.on('bailout', () => process.exit(1))
+t.teardown(() => {
+  Object.defineProperty(process, 'platform', { value: platform })
+})
 
 const cases = {
   'C:\\x\\y\\z': {
@@ -95,9 +99,12 @@ const cases = {
 
 t.test('parse a windows path', function (t) {
   Object.keys(cases).forEach(function (c) {
-    const expect = cases[c]
-    const actual = npa(c, 'C:\\test\\path')
-    t.has(actual, expect, c)
+    t.test(c, t => {
+      const expect = cases[c]
+      const actual = npa(c, 'C:\\test\\path')
+      t.has(actual, expect, c)
+      t.end()
+    })
   })
   t.end()
 })