fix: allow hash character in paths (#5122)

* fix: allow link from path with hash character

* fix: allow hash character in path in other places

* Remove extra semicolon

Author: AgainPsychoX

lib/commands/diff.js

@@ -106,7 +106,7 @@ class Diff extends BaseCommand {
       const pkgName = await this.packageName(this.prefix)
       return [
         `${pkgName}@${this.npm.config.get('tag')}`,
-        `file:${this.prefix}`,
+        `file:${this.prefix.replace(/#/g, '%23')}`,
       ]
     }
 
@@ -134,7 +134,7 @@ class Diff extends BaseCommand {
       }
       return [
         `${pkgName}@${a}`,
-        `file:${this.prefix}`,
+        `file:${this.prefix.replace(/#/g, '%23')}`,
       ]
     }
 
@@ -165,7 +165,7 @@ class Diff extends BaseCommand {
         }
         return [
           `${spec.name}@${spec.fetchSpec}`,
-          `file:${this.prefix}`,
+          `file:${this.prefix.replace(/#/g, '%23')}`,
         ]
       }
 
@@ -178,7 +178,7 @@ class Diff extends BaseCommand {
         }
       }
 
-      const aSpec = `file:${node.realpath}`
+      const aSpec = `file:${node.realpath.replace(/#/g, '%23')}`
 
       // finds what version of the package to compare against, if a exact
       // version or tag was passed than it should use that, otherwise
@@ -211,8 +211,8 @@ class Diff extends BaseCommand {
       ]
     } else if (spec.type === 'directory') {
       return [
-        `file:${spec.fetchSpec}`,
-        `file:${this.prefix}`,
+        `file:${spec.fetchSpec.replace(/#/g, '%23')}`,
+        `file:${this.prefix.replace(/#/g, '%23')}`,
       ]
     } else {
       throw this.usageError(`Spec type ${spec.type} not supported.`)
@@ -279,7 +279,7 @@ class Diff extends BaseCommand {
 
       const res = !node || !node.package || !node.package.version
         ? spec.fetchSpec
-        : `file:${node.realpath}`
+        : `file:${node.realpath.replace(/#/g, '%23')}`
 
       return `${spec.name}@${res}`
     })

lib/commands/link.js

@@ -122,7 +122,7 @@ class Link extends ArboristWorkspaceCmd {
       ...this.npm.flatOptions,
       prune: false,
       path: this.npm.prefix,
-      add: names.map(l => `file:${resolve(globalTop, 'node_modules', l)}`),
+      add: names.map(l => `file:${resolve(globalTop, 'node_modules', l).replace(/#/g, '%23')}`),
       save,
       workspaces: this.workspaceNames,
     })
@@ -133,7 +133,7 @@ class Link extends ArboristWorkspaceCmd {
   async linkPkg () {
     const wsp = this.workspacePaths
     const paths = wsp && wsp.length ? wsp : [this.npm.prefix]
-    const add = paths.map(path => `file:${path}`)
+    const add = paths.map(path => `file:${path.replace(/#/g, '%23')}`)
     const globalTop = resolve(this.npm.globalDir, '..')
     const arb = new Arborist({
       ...this.npm.flatOptions,

tap-snapshots/test/lib/commands/link.js.test.cjs

@@ -5,6 +5,11 @@
  * Make sure to inspect the output below.  Do not ignore changes!
  */
 'use strict'
+exports[`test/lib/commands/link.js TAP hash character in working directory path > should create a global link to current pkg, even within path with hash 1`] = `
+{CWD}/test/lib/commands/tap-testdir-link-hash-character-in-working-directory-path/global-prefix/lib/node_modules/test-pkg-link -> {CWD}/test/lib/commands/tap-testdir-link-hash-character-in-working-directory-path/i_like_#_in_my_paths/test-pkg-link
+
+`
+
 exports[`test/lib/commands/link.js TAP link global linked pkg to local nm when using args > should create a local symlink to global pkg 1`] = `
 {CWD}/test/lib/commands/tap-testdir-link-link-global-linked-pkg-to-local-nm-when-using-args/my-project/node_modules/@myscope/bar -> {CWD}/test/lib/commands/tap-testdir-link-link-global-linked-pkg-to-local-nm-when-using-args/global-prefix/lib/node_modules/@myscope/bar
 {CWD}/test/lib/commands/tap-testdir-link-link-global-linked-pkg-to-local-nm-when-using-args/my-project/node_modules/@myscope/linked -> {CWD}/test/lib/commands/tap-testdir-link-link-global-linked-pkg-to-local-nm-when-using-args/scoped-linked

test/lib/commands/link.js

@@ -514,3 +514,39 @@ t.test('--global option', async t => {
     'should throw an useful error'
   )
 })
+
+t.test('hash character in working directory path', async t => {
+  const testdir = t.testdir({
+    'global-prefix': {
+      lib: {
+        node_modules: {
+          a: {
+            'package.json': JSON.stringify({
+              name: 'a',
+              version: '1.0.0',
+            }),
+          },
+        },
+      },
+    },
+    'i_like_#_in_my_paths': {
+      'test-pkg-link': {
+        'package.json': JSON.stringify({
+          name: 'test-pkg-link',
+          version: '1.0.0',
+        }),
+      },
+    },
+  })
+  npm.globalDir = resolve(testdir, 'global-prefix', 'lib', 'node_modules')
+  npm.prefix = resolve(testdir, 'i_like_#_in_my_paths', 'test-pkg-link')
+
+  link.workspacePaths = null
+  await link.exec([])
+  const links = await printLinks({
+    path: resolve(npm.globalDir, '..'),
+    global: true,
+  })
+
+  t.matchSnapshot(links, 'should create a global link to current pkg, even within path with hash')
+})

workspaces/arborist/lib/arborist/build-ideal-tree.js

@@ -484,7 +484,7 @@ Try using the package name instead, e.g:
             .catch(/* istanbul ignore next */ er => null)
           if (st && st.isSymbolicLink()) {
             const target = await readlink(dir)
-            const real = resolve(dirname(dir), target)
+            const real = resolve(dirname(dir), target).replace(/#/g, '%23')
             tree.package.dependencies[name] = `file:${real}`
           } else {
             tree.package.dependencies[name] = '*'
@@ -603,7 +603,7 @@ Try using the package name instead, e.g:
     if (filepath) {
       const { name } = spec
       const tree = this.idealTree.target
-      spec = npa(`file:${relpath(tree.path, filepath)}`, tree.path)
+      spec = npa(`file:${relpath(tree.path, filepath).replace(/#/g, '%23')}`, tree.path)
       spec.name = name
     }
     return spec

workspaces/arborist/lib/arborist/load-actual.js

@@ -196,7 +196,7 @@ module.exports = cls => class ActualLoader extends cls {
       const actualRoot = tree.isLink ? tree.target : tree
       const { dependencies = {} } = actualRoot.package
       for (const [name, kid] of actualRoot.children.entries()) {
-        const def = kid.isLink ? `file:${kid.realpath}` : '*'
+        const def = kid.isLink ? `file:${kid.realpath.replace(/#/g, '%23')}` : '*'
         dependencies[name] = dependencies[name] || def
       }
       actualRoot.package = { ...actualRoot.package, dependencies }

workspaces/arborist/lib/arborist/load-virtual.js

@@ -162,7 +162,7 @@ module.exports = cls => class VirtualLoader extends cls {
       lockfile: s.data,
     })
     for (const [name, path] of workspaces.entries()) {
-      lockWS.push(['workspace', name, `file:${path}`])
+      lockWS.push(['workspace', name, `file:${path.replace(/#/g, '%23')}`])
     }
 
     const lockEdges = [

workspaces/arborist/lib/arborist/reify.js

@@ -1241,7 +1241,7 @@ module.exports = cls => class Reifier extends cls {
             // path initially, in which case we can end up with the wrong
             // thing, so just get the ultimate fetchSpec and relativize it.
             const p = req.fetchSpec.replace(/^file:/, '')
-            const rel = relpath(addTree.realpath, p)
+            const rel = relpath(addTree.realpath, p).replace(/#/g, '%23')
             newSpec = `file:${rel}`
           }
         } else {

workspaces/arborist/lib/consistent-resolve.js

@@ -20,8 +20,8 @@ const consistentResolve = (resolved, fromPath, toPath, relPaths = false) => {
       raw,
     } = npa(resolved, fromPath)
     const isPath = type === 'file' || type === 'directory'
-    return isPath && !relPaths ? `file:${fetchSpec}`
-      : isPath ? 'file:' + (toPath ? relpath(toPath, fetchSpec) : fetchSpec)
+    return isPath && !relPaths ? `file:${fetchSpec.replace(/#/g, '%23')}`
+      : isPath ? 'file:' + (toPath ? relpath(toPath, fetchSpec.replace(/#/g, '%23')) : fetchSpec.replace(/#/g, '%23'))
       : hosted ? `git+${
         hosted.auth ? hosted.https(hostedOpt) : hosted.sshurl(hostedOpt)
       }`

workspaces/arborist/lib/link.js

@@ -118,7 +118,7 @@ class Link extends Node {
     // the path/realpath guard is there for the benefit of setting
     // these things in the "wrong" order
     return this.path && this.realpath
-      ? `file:${relpath(dirname(this.path), this.realpath)}`
+      ? `file:${relpath(dirname(this.path), this.realpath).replace(/#/g, '%23')}`
       : null
   }
 

workspaces/arborist/lib/node.js

@@ -824,7 +824,7 @@ class Node {
     }
 
     for (const [name, path] of this[_workspaces].entries()) {
-      new Edge({ from: this, name, spec: `file:${path}`, type: 'workspace' })
+      new Edge({ from: this, name, spec: `file:${path.replace(/#/g, '%23')}`, type: 'workspace' })
     }
   }
 

workspaces/arborist/lib/shrinkwrap.js

@@ -815,7 +815,7 @@ class Shrinkwrap {
       const pathFixed = !resolved ? null
         : !/^file:/.test(resolved) ? resolved
         // resolve onto the metadata path
-        : `file:${resolve(this.path, resolved.slice(5))}`
+        : `file:${resolve(this.path, resolved.slice(5)).replace(/#/g, '%23')}`
 
       // if we have one, only set the other if it matches
       // otherwise it could be for a completely different thing.
@@ -996,7 +996,7 @@ class Shrinkwrap {
       : npa.resolve(node.name, edge.spec, edge.from.realpath)
 
     if (node.isLink) {
-      lock.version = `file:${relpath(this.path, node.realpath)}`
+      lock.version = `file:${relpath(this.path, node.realpath).replace(/#/g, '%23')}`
     } else if (spec && (spec.type === 'file' || spec.type === 'remote')) {
       lock.version = spec.saveSpec
     } else if (spec && spec.type === 'git' || rSpec.type === 'git') {
@@ -1074,7 +1074,7 @@ class Shrinkwrap {
             // this especially shows up with workspace edges when the root
             // node is also a workspace in the set.
             const p = resolve(node.realpath, spec.slice('file:'.length))
-            set[k] = `file:${relpath(node.realpath, p)}`
+            set[k] = `file:${relpath(node.realpath, p).replace(/#/g, '%23')}`
           } else {
             set[k] = spec
           }