From 6fc0dfdbdabb6946e590648e5cbb030b101a2d9f Mon Sep 17 00:00:00 2001
From: Ruy Adorno <ruyadorno@hotmail.com>
Date: Fri, 25 Jun 2021 17:07:41 -0400
Subject: [PATCH] fix: node.isDescendantOf follows fs hierarchy

Previously, the `node.isDescendantOf` method would only check for direct
parents of that node, this may cause unexpected side effects on build
ideal tree and reify when working with linked dependencies.

This change modifies it so that `node.isDescendantOf` now looks at
`node.resolveParent` (making sure `fsParents` are included in the
lookup) to determine wether that node is a descendant of another.

This also made it possible to refactor the logic that checks for an
`external` dep in build-ideal-tree to use `node.isDescendantOf` instead
of a regex check in the realpath of the link.

Relates to: https://github.com/npm/arborist/pull/290
Relates to: https://github.com/npm/cli/issues/2900
---
 lib/arborist/build-ideal-tree.js |  7 ++++---
 lib/arborist/reify.js            |  2 +-
 lib/node.js                      | 10 +++++++++-
 test/arborist/audit.js           |  2 +-
 test/node.js                     |  6 ++++++
 5 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/lib/arborist/build-ideal-tree.js b/lib/arborist/build-ideal-tree.js
index 5db11eb38..f885fd8af 100644
--- a/lib/arborist/build-ideal-tree.js
+++ b/lib/arborist/build-ideal-tree.js
@@ -1421,7 +1421,7 @@ This is a one-time fix-up, please be patient...
     // prune anything deeper in the tree that can be replaced by this
     if (this.idealTree) {
       for (const node of this.idealTree.inventory.query('name', newDep.name)) {
-        if (node.isDescendantOf(target))
+        if (!node.isTop && node.isDescendantOf(target))
           this[_pruneDedupable](node, false)
       }
     }
@@ -1819,7 +1819,7 @@ This is a one-time fix-up, please be patient...
     const current = target !== entryEdge.from && target.resolve(dep.name)
     if (current) {
       for (const edge of current.edgesIn.values()) {
-        if (edge.from.isDescendantOf(target) && edge.valid) {
+        if (!edge.from.isTop && edge.from.isDescendantOf(target) && edge.valid) {
           if (!edge.satisfiedBy(dep))
             return CONFLICT
         }
@@ -1876,7 +1876,8 @@ This is a one-time fix-up, please be patient...
       if (link.root !== this.idealTree)
         continue
 
-      const external = /^\.\.(\/|$)/.test(relpath(this.path, link.realpath))
+      const tree = this.idealTree.target || this.idealTree
+      const external = !link.target.isDescendantOf(tree)
 
       // outside the root, somebody else's problem, ignore it
       if (external && !this[_follow])
diff --git a/lib/arborist/reify.js b/lib/arborist/reify.js
index f259a69b5..f4d5baad1 100644
--- a/lib/arborist/reify.js
+++ b/lib/arborist/reify.js
@@ -407,7 +407,7 @@ module.exports = cls => class Reifier extends cls {
     // node.parent is checked to make sure this is a node that's in the tree, and
     // not the parent-less top level nodes
     const filter = node =>
-      node.isDescendantOf(this.idealTree) &&
+      node.top.isProjectRoot &&
         (node.peer && this[_omitPeer] ||
           node.dev && this[_omitDev] ||
           node.optional && this[_omitOptional] ||
diff --git a/lib/node.js b/lib/node.js
index c21bc46cf..51acbe322 100644
--- a/lib/node.js
+++ b/lib/node.js
@@ -409,7 +409,7 @@ class Node {
   }
 
   isDescendantOf (node) {
-    for (let p = this; p; p = p.parent) {
+    for (let p = this; p; p = p.resolveParent) {
       if (p === node)
         return true
     }
@@ -1197,6 +1197,14 @@ class Node {
     return this.isTop ? this : this.parent.top
   }
 
+  get isFsTop () {
+    return !this.fsParent
+  }
+
+  get fsTop () {
+    return this.isFsTop ? this : this.fsParent.fsTop
+  }
+
   get resolveParent () {
     return this.parent || this.fsParent
   }
diff --git a/test/arborist/audit.js b/test/arborist/audit.js
index 75d97c131..ec3f60732 100644
--- a/test/arborist/audit.js
+++ b/test/arborist/audit.js
@@ -101,7 +101,7 @@ t.test('audit in a workspace', async t => {
   t.equal(auditReport.get('mkdirp').nodes.size, 1)
   t.strictSame(auditReport.toJSON().vulnerabilities.mkdirp.nodes, ['packages/a/node_modules/mkdirp'])
   t.equal(auditReport.get('minimist').nodes.size, 1)
-  t.strictSame(auditReport.toJSON().vulnerabilities.minimist.nodes, ['packages/a/node_modules/minimist'])
+  t.strictSame(auditReport.toJSON().vulnerabilities.minimist.nodes, ['node_modules/minimist'])
 
   const fixed = await newArb(path, { workspaces: ['b'] }).audit({ fix: true })
   t.equal(fixed.children.get('a').target.children.get('mkdirp').version, '0.5.0', 'did not fix a')
diff --git a/test/node.js b/test/node.js
index cb6944936..0210fda79 100644
--- a/test/node.js
+++ b/test/node.js
@@ -2496,6 +2496,12 @@ t.test('canDedupe()', t => {
 
   t.equal(top.children.get('a').canDedupe(), true)
 
+  // check fsTop and isDescendantOf
+  t.equal(top.isDescendantOf(root), true)
+  t.equal(top.isFsTop, false)
+  t.equal(top.fsTop, root)
+  t.equal(top.children.get('a').isFsTop, true)
+
   t.end()
 })