Throw EEXIST when ensureSymlink dest is a broken symlink

lib/ensure/__tests__/symlink.test.js

@@ -7,6 +7,8 @@ const os = require('os')
 const fse = require('../..')
 const path = require('path')
 const assert = require('assert')
+const proxyquire = require('proxyquire')
+const realFs = require('../../fs')
 const _symlinkPaths = require('../symlink-paths')
 const symlinkPathsSync = _symlinkPaths.symlinkPathsSync
 const ensureSymlink = fse.ensureSymlink
@@ -399,4 +401,75 @@ describe('fse-ensure-symlink', () => {
       assert.strictEqual(content, 'content')
     })
   })
+
+  // https://github.com/jprichardson/node-fs-extra/issues/925
+  describe('ensureSymlink() when dest is an existing broken symlink (issue #925)', () => {
+    it('should fail with EEXIST (not ENOENT) when dest is a broken symlink', async () => {
+      const srcpath = path.join(TEST_DIR, 'src-925.txt')
+      const dstpath = path.join(TEST_DIR, 'broken-link-925')
+      fs.writeFileSync(srcpath, 'src\n')
+      fs.symlinkSync(path.join(TEST_DIR, 'missing-925.txt'), dstpath)
+      assert.strictEqual(fs.lstatSync(dstpath).isSymbolicLink(), true)
+
+      await assert.rejects(ensureSymlink(srcpath, dstpath), { code: 'EEXIST' })
+    })
+
+    it('should fail with EEXIST (not ENOENT) when dest is a broken symlink (sync)', () => {
+      const srcpath = path.join(TEST_DIR, 'src-925-sync.txt')
+      const dstpath = path.join(TEST_DIR, 'broken-link-925-sync')
+      fs.writeFileSync(srcpath, 'src\n')
+      fs.symlinkSync(path.join(TEST_DIR, 'missing-925-sync.txt'), dstpath)
+      assert.strictEqual(fs.lstatSync(dstpath).isSymbolicLink(), true)
+
+      assert.throws(() => ensureSymlinkSync(srcpath, dstpath), { code: 'EEXIST' })
+    })
+  })
+
+  // When checking whether an existing dest symlink points at src, only an
+  // ENOENT (broken link) should be swallowed. Any other error must propagate.
+  describe('ensureSymlink() when statting the existing dest fails for a non-ENOENT reason', () => {
+    it('should rethrow the error instead of swallowing it', async () => {
+      const srcpath = path.join(TEST_DIR, 'src-eacces.txt')
+      const dstpath = path.join(TEST_DIR, 'dst-symlink-eacces')
+      fs.writeFileSync(srcpath, 'src\n')
+      fs.symlinkSync(path.join(TEST_DIR, 'missing-eacces.txt'), dstpath)
+      assert.strictEqual(fs.lstatSync(dstpath).isSymbolicLink(), true)
+
+      const fsStub = {
+        stat (p, ...args) {
+          if (p === dstpath) {
+            const err = new Error('EACCES: permission denied, stat')
+            err.code = 'EACCES'
+            return Promise.reject(err)
+          }
+          return realFs.stat(p, ...args)
+        }
+      }
+      const { createSymlink } = proxyquire('../symlink', { '../fs': fsStub })
+
+      await assert.rejects(createSymlink(srcpath, dstpath), { code: 'EACCES' })
+    })
+
+    it('should rethrow the error instead of swallowing it (sync)', () => {
+      const srcpath = path.join(TEST_DIR, 'src-eacces-sync.txt')
+      const dstpath = path.join(TEST_DIR, 'dst-symlink-eacces-sync')
+      fs.writeFileSync(srcpath, 'src\n')
+      fs.symlinkSync(path.join(TEST_DIR, 'missing-eacces-sync.txt'), dstpath)
+      assert.strictEqual(fs.lstatSync(dstpath).isSymbolicLink(), true)
+
+      const fsStub = {
+        statSync (p, ...args) {
+          if (p === dstpath) {
+            const err = new Error('EACCES: permission denied, stat')
+            err.code = 'EACCES'
+            throw err
+          }
+          return realFs.statSync(p, ...args)
+        }
+      }
+      const { createSymlinkSync } = proxyquire('../symlink', { '../fs': fsStub })
+
+      assert.throws(() => createSymlinkSync(srcpath, dstpath), { code: 'EACCES' })
+    })
+  })
 })

lib/ensure/symlink.js

@@ -35,8 +35,16 @@ async function createSymlink (srcpath, dstpath, type) {
       }
     }
 
-    const dstStat = await fs.stat(dstpath, { bigint: true })
-    if (areIdentical(srcStat, dstStat)) return
+    // Following the existing symlink fails with ENOENT if it's broken; in that
+    // case fall through so fs.symlink reports EEXIST, rather than leaking the
+    // stat ENOENT. Any other error is unexpected, so rethrow it.
+    let dstStat
+    try {
+      dstStat = await fs.stat(dstpath, { bigint: true })
+    } catch (err) {
+      if (err.code !== 'ENOENT') throw err
+    }
+    if (dstStat && areIdentical(srcStat, dstStat)) return
   }
 
   const relative = await symlinkPaths(srcpath, dstpath)
@@ -72,8 +80,16 @@ function createSymlinkSync (srcpath, dstpath, type) {
       }
     }
 
-    const dstStat = fs.statSync(dstpath, { bigint: true })
-    if (areIdentical(srcStat, dstStat)) return
+    // Following the existing symlink fails with ENOENT if it's broken; in that
+    // case fall through so fs.symlinkSync reports EEXIST, rather than leaking the
+    // stat ENOENT. Any other error is unexpected, so rethrow it.
+    let dstStat
+    try {
+      dstStat = fs.statSync(dstpath, { bigint: true })
+    } catch (err) {
+      if (err.code !== 'ENOENT') throw err
+    }
+    if (dstStat && areIdentical(srcStat, dstStat)) return
   }
 
   const relative = symlinkPathsSync(srcpath, dstpath)