Dealing with broken symlinks

isaacs/node-glob#170 made it so that globs that match
broken symlinks are returned as file paths. This patch works around that issue
by catching exceptions thrown when opening files.

I opted to handle exceptions instead of filtering out broken symlinks in
fs.getAllFiles() because of the comment here: https://nodejs.org/api/fs.html#fs_fs_stat_path_options_callback

I changed the file content getters to return undefined explicitly when no file
content is returned. This is to be consistent with what's already there

lib/file_system.js

@@ -63,9 +63,17 @@ class FileSystem {
     return this.glob(globs, {cwd: this.targetDir, nocase: !!nocase})
   }
 
-  isBinaryFile (relativeFile, lineCount) {
+  isBinaryFile (relativeFile) {
     const file = path.resolve(this.targetDir, relativeFile)
-    return isBinaryFile.sync(file)
+    try {
+      return isBinaryFile.sync(file)
+    } catch (e) {
+      // File doesn't exist or is a directory, so it isn't a binary file
+      if (e.message.includes('ENOENT')) {
+        return false
+      }
+      throw e
+    }
   }
 
   shouldInclude (path) {
@@ -80,14 +88,24 @@ class FileSystem {
   getFileContents (relativeFile) {
     const file = path.resolve(this.targetDir, relativeFile)
     if (fs.existsSync(file) && fs.statSync(file).isFile()) {
-      return fs.readFileSync(file)
+      return fs.readFileSync(file).toString()
     }
+    return void 0
   }
 
-  readLines (relativeFile, lineCount) {
+  getFileLines (relativeFile, lineCount) {
     const file = path.resolve(this.targetDir, relativeFile)
     const fs = require('fs')
-    var fd = fs.openSync(path.resolve(this.targetDir, file), 'r')
+    let fd
+    try {
+      fd = fs.openSync(path.resolve(this.targetDir, file), 'r')
+    } catch (e) {
+      // File doesn't exist or is a directory
+      if (e.message.includes('ENOENT')) {
+        return void 0
+      }
+      throw e
+    }
     var bufferSize = 1024
     var buffer = Buffer.alloc(bufferSize)
     var lines = ''

rules/file-contents.js

@@ -14,10 +14,13 @@ module.exports = function (fileSystem, rule) {
   }
 
   const results = files.map(file => {
-    const fileContents = fs.getFileContents(file)
+    let fileContents = fs.getFileContents(file)
+    if (fileContents === undefined) {
+      fileContents = ''
+    }
     const regexp = new RegExp(options.content, options.flags)
 
-    const passed = fileContents && fileContents.toString().search(regexp) !== -1
+    const passed = fileContents.search(regexp) >= 0
     const message = `File ${file} ${passed ? 'contains' : 'doesn\'t contain'} ${getContent()}`
 
     return new Result(rule, message, file, passed)

rules/file-not-contents.js

@@ -14,10 +14,13 @@ module.exports = function (fileSystem, rule) {
   }
 
   const results = files.map(file => {
-    const fileContents = fs.getFileContents(file)
-    const regexp = new RegExp(options.content, options.flags)
+    let fileContents = fs.getFileContents(file)
+    if (fileContents === undefined) {
+      fileContents = ''
+    }
 
-    const passed = fileContents && fileContents.toString().search(regexp) === -1
+    const regexp = new RegExp(options.content, options.flags)
+    const passed = fileContents.search(regexp) === -1
     const message = `File ${file} ${passed ? 'doesn\'t contain' : 'contains'} ${options.content}`
 
     return new Result(rule, message, file, passed)

rules/file-starts-with.js

@@ -34,14 +34,12 @@ module.exports = function (fileSystem, rule) {
     )
   }
 
-  if (filteredFiles.length === 0 && options['succeed-on-non-existent']) {
-    const message = `not found: (${options.files.join(', ')})`
-    return [new Result(rule, message, null, true)]
-  }
-
-  let results = []
+  const results = []
   filteredFiles.forEach(file => {
-    const lines = fs.readLines(file, options.lineCount)
+    const lines = fs.getFileLines(file, options.lineCount)
+    if (!lines) {
+      return
+    }
     const misses = options.patterns.filter((pattern) => {
       const regexp = new RegExp(pattern, options.flags)
       return !lines.match(regexp)
@@ -58,5 +56,10 @@ module.exports = function (fileSystem, rule) {
     results.push(new Result(rule, message, file, passed))
   })
 
+  if (results.length === 0 && options['succeed-on-non-existent']) {
+    const message = `not found: (${options.files.join(', ')})`
+    return [new Result(rule, message, null, true)]
+  }
+
   return results
 }

tests/rules/broken_symlink_for_test

@@ -0,0 +1 @@
+tests/rules/nonexistantfile

tests/rules/file_contents_tests.js

@@ -4,6 +4,7 @@
 const chai = require('chai')
 const expect = chai.expect
 const Result = require('../../lib/result')
+const FileSystem = require('../../lib/file_system')
 
 describe('rule', () => {
   describe('files_contents', () => {
@@ -153,5 +154,22 @@ describe('rule', () => {
 
       expect(actual).to.deep.equal(expected)
     })
+
+    it('should handle broken symlinks', () => {
+      const brokenSymlink = './tests/rules/broken_symlink_for_test'
+      const stat = require('fs').lstatSync(brokenSymlink)
+      expect(stat.isSymbolicLink()).to.equal(true)
+      const fs = new FileSystem(require('path').resolve('.'))
+
+      const rule = {
+        options: {
+          files: [brokenSymlink],
+          lineCount: 1,
+          patterns: ['something']
+        }
+      }
+      const actual = fileContents(fs, rule)
+      expect(actual.length).to.equal(0)
+    })
   })
 })

tests/rules/file_not_contents_tests.js

@@ -4,10 +4,11 @@
 const chai = require('chai')
 const expect = chai.expect
 const Result = require('../../lib/result')
+const FileSystem = require('../../lib/file_system')
 
 describe('rule', () => {
   describe('files_not_contents', () => {
-    const fileContents = require('../../rules/file-not-contents')
+    const fileNotContents = require('../../rules/file-not-contents')
 
     it('returns passes if requested file contents do not exist', () => {
       const rule = {
@@ -35,7 +36,7 @@ describe('rule', () => {
         )
       ]
 
-      const actual = fileContents(null, rule)
+      const actual = fileNotContents(null, rule)
       expect(actual).to.deep.equal(expected)
     })
 
@@ -65,7 +66,7 @@ describe('rule', () => {
         )
       ]
 
-      const actual = fileContents(null, rule)
+      const actual = fileNotContents(null, rule)
 
       expect(actual).to.deep.equal(expected)
     })
@@ -88,7 +89,7 @@ describe('rule', () => {
         }
       }
 
-      const actual = fileContents(null, rule)
+      const actual = fileNotContents(null, rule)
       const expected = [
         new Result(
           rule,
@@ -117,9 +118,25 @@ describe('rule', () => {
         }
       }
 
-      const actual = fileContents(null, rule)
+      const actual = fileNotContents(null, rule)
       const expected = []
       expect(actual).to.deep.equal(expected)
     })
+    it('should handle broken symlinks', () => {
+      const brokenSymlink = './tests/rules/broken_symlink_for_test'
+      const stat = require('fs').lstatSync(brokenSymlink)
+      expect(stat.isSymbolicLink()).to.equal(true)
+      const fs = new FileSystem(require('path').resolve('.'))
+
+      const rule = {
+        options: {
+          files: [brokenSymlink],
+          lineCount: 1,
+          patterns: ['something']
+        }
+      }
+      const actual = fileNotContents(fs, rule)
+      expect(actual.length).to.equal(0)
+    })
   })
 })

tests/rules/file_starts_with_tests.js

@@ -38,7 +38,7 @@ describe('rule', () => {
             findAllFiles () {
               return ['somefile.js']
             },
-            readLines () {
+            getFileLines () {
               return 'some javascript code'
             },
             targetDir: '.'
@@ -115,7 +115,7 @@ describe('rule', () => {
             findAllFiles () {
               return ['Skip/paBle-path.js', 'afile.js', 'badextension.sVg']
             },
-            readLines () {
+            getFileLines () {
               return 'some javascript code'
             },
             targetDir: '.'
@@ -156,5 +156,22 @@ describe('rule', () => {
 
       expect(actual.length).to.equal(0)
     })
+
+    it('should handle broken symlinks', () => {
+      const brokenSymlink = './tests/rules/broken_symlink_for_test'
+      const stat = require('fs').lstatSync(brokenSymlink)
+      expect(stat.isSymbolicLink()).to.equal(true)
+      const fs = new FileSystem(require('path').resolve('.'))
+
+      const rule = {
+        options: {
+          files: [brokenSymlink],
+          lineCount: 1,
+          patterns: ['something']
+        }
+      }
+      const actual = fileStartsWith(fs, rule)
+      expect(actual.length).to.equal(0)
+    })
   })
 })