From 62b95a04337661e3fa17093708b57000054442d9 Mon Sep 17 00:00:00 2001 From: Patryk Ludwikowski Date: Wed, 27 Jul 2022 23:15:05 +0200 Subject: [PATCH] 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 --- lib/commands/diff.js | 14 ++++---- lib/commands/link.js | 4 +-- .../test/lib/commands/link.js.test.cjs | 5 +++ test/lib/commands/link.js | 36 +++++++++++++++++++ .../arborist/lib/arborist/build-ideal-tree.js | 4 +-- .../arborist/lib/arborist/load-actual.js | 2 +- .../arborist/lib/arborist/load-virtual.js | 2 +- workspaces/arborist/lib/arborist/reify.js | 2 +- workspaces/arborist/lib/consistent-resolve.js | 4 +-- workspaces/arborist/lib/link.js | 2 +- workspaces/arborist/lib/node.js | 2 +- workspaces/arborist/lib/shrinkwrap.js | 6 ++-- 12 files changed, 62 insertions(+), 21 deletions(-) diff --git a/lib/commands/diff.js b/lib/commands/diff.js index b8a64bd98a039..bbd6fae6680ca 100644 --- a/lib/commands/diff.js +++ b/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}` }) diff --git a/lib/commands/link.js b/lib/commands/link.js index b0b889ea787fd..7bce73ed2bb6f 100644 --- a/lib/commands/link.js +++ b/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, diff --git a/tap-snapshots/test/lib/commands/link.js.test.cjs b/tap-snapshots/test/lib/commands/link.js.test.cjs index a9a10b20a2f83..e01409e4ce196 100644 --- a/tap-snapshots/test/lib/commands/link.js.test.cjs +++ b/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 diff --git a/test/lib/commands/link.js b/test/lib/commands/link.js index a01de0b247990..5bd7a3f1480ae 100644 --- a/test/lib/commands/link.js +++ b/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') +}) diff --git a/workspaces/arborist/lib/arborist/build-ideal-tree.js b/workspaces/arborist/lib/arborist/build-ideal-tree.js index da2652c449a1c..0e98ed6fc533c 100644 --- a/workspaces/arborist/lib/arborist/build-ideal-tree.js +++ b/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 diff --git a/workspaces/arborist/lib/arborist/load-actual.js b/workspaces/arborist/lib/arborist/load-actual.js index 43351b69034af..d4eabe8c0fdfd 100644 --- a/workspaces/arborist/lib/arborist/load-actual.js +++ b/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 } diff --git a/workspaces/arborist/lib/arborist/load-virtual.js b/workspaces/arborist/lib/arborist/load-virtual.js index fb3f334747fc8..947659f177eef 100644 --- a/workspaces/arborist/lib/arborist/load-virtual.js +++ b/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 = [ diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index faf016c704010..4f1061e4abe50 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/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 { diff --git a/workspaces/arborist/lib/consistent-resolve.js b/workspaces/arborist/lib/consistent-resolve.js index e34e40a46d002..5308dc7e2f95e 100644 --- a/workspaces/arborist/lib/consistent-resolve.js +++ b/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) }` diff --git a/workspaces/arborist/lib/link.js b/workspaces/arborist/lib/link.js index dcce8c0d3dfa5..6fed063772b6a 100644 --- a/workspaces/arborist/lib/link.js +++ b/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 } diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index d731e5f617908..66d46d746abf3 100644 --- a/workspaces/arborist/lib/node.js +++ b/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' }) } } diff --git a/workspaces/arborist/lib/shrinkwrap.js b/workspaces/arborist/lib/shrinkwrap.js index 3305bac4914be..e2180fd4c8076 100644 --- a/workspaces/arborist/lib/shrinkwrap.js +++ b/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 }