Merge pull request #110 from UmbrellaDocs/109fix-regex

Fix: Fixed failing ignore and replacement patterns

Author: gaurav-nelson

index.js

@@ -61,7 +61,7 @@ program
         // Update the current file name
         currentFile = file
         if (!cmd.json) {
-          spinner.text = `Checking ${currentFile}...\n`
+          spinner.text = `Checking ${currentFile}...`
         }
 
         // Increment file count for statistics
@@ -120,7 +120,7 @@ program
                   `${currentFile}:${linkStatusObj.line_number}:${linkStatusObj.position.start.column}: 🚫 ${linkStatusObj.link} Status:${linkStatusObj.status_code}${linkStatusObj.error_message ? ` ${linkStatusObj.error_message}` : ' Cannot reach link'}`
                 )
               )
-              spinner.start(`Checking ${currentFile}...\n`)
+              spinner.start(`Checking ${currentFile}...`)
             }
             hasErrorLinks = true
           } else if (

lib/handle-links-modification.js

@@ -9,20 +9,65 @@
  *
  * @returns {Array} The modified nodes.
  */
-import { escapeRegExp } from 'lodash-es'
 
 function doReplacements(nodes, opts = {}) {
   const { ignorePatterns = [], replacementPatterns = [], baseUrl } = opts
 
+  // Safer regex compilation with timeout protection
+  function createSafeRegex(pattern) {
+    try {
+      // Validate pattern complexity before creating RegExp
+      // Check for common problematic patterns that could lead to ReDoS
+      if (
+        pattern.includes('(.*)*') ||
+        pattern.includes('(.+)+') ||
+        pattern.match(/\([^)]+\)\+\+/) ||
+        pattern.match(/\(\[.*?\]\+\)\+/) ||
+        pattern.match(/\(a\+\)\+/)
+      ) {
+        console.warn(`Potentially unsafe regex pattern detected: ${pattern}`)
+        return null
+      }
+
+      // Apply length limits for safety
+      if (pattern.length > 100) {
+        console.warn(
+          `Pattern exceeds maximum safe length: ${pattern.substring(0, 50)}...`
+        )
+        return null
+      }
+
+      return new RegExp(pattern)
+    } catch (e) {
+      console.warn(`Invalid regex pattern: ${pattern}. Error: ${e.message}`)
+      return null
+    }
+  }
+
+  // Pre-compile regular expressions with safer approach
+  const ignoreRegexes = ignorePatterns
+    .map(({ pattern }) => createSafeRegex(pattern))
+    .filter(Boolean)
+
+  const replacementRegexes = replacementPatterns
+    .map(({ pattern, replacement }) => {
+      const regex = createSafeRegex(pattern)
+      return regex ? { regex, replacement } : null
+    })
+    .filter(Boolean)
+
   return nodes.filter((node) => {
     let { url } = node
+
     // Skip link checking if it matches any ignore pattern
     if (
-      ignorePatterns.some(({ pattern }) => {
-        // Sanitize the pattern before creating the RegExp
-        const sanitizedPattern = escapeRegExp(pattern)
-        const regex = new RegExp(sanitizedPattern)
-        return regex.test(url)
+      ignoreRegexes.some((regex) => {
+        try {
+          return regex.test(url)
+        } catch (e) {
+          console.warn(`Error testing URL against pattern: ${e.message}`)
+          return false
+        }
       })
     ) {
       return false // Exclude this node
@@ -34,13 +79,23 @@ function doReplacements(nodes, opts = {}) {
     }
 
     // Replace link URL based on replacement patterns
-    replacementPatterns.forEach(({ pattern, replacement }) => {
-      // Sanitize the pattern before creating the RegExp
-      const sanitizedPattern = escapeRegExp(pattern)
-      url = url.replace(new RegExp(sanitizedPattern), replacement)
+    replacementRegexes.forEach(({ regex, replacement }) => {
+      try {
+        // Use a safer string replace approach
+        const oldUrl = url
+        url = url.replace(regex, replacement)
+
+        // If replacement leads to an extremely long string, revert
+        if (url.length > oldUrl.length * 3 && url.length > 2000) {
+          console.warn(`Suspicious replacement result detected. Reverting.`)
+          url = oldUrl
+        }
+      } catch (e) {
+        console.warn(`Error replacing URL: ${e.message}`)
+      }
     })
-    node.url = url
 
+    node.url = url
     return true // Include this node
   })
 }

package-lock.json

@@ -1,6 +1,6 @@
 {
   "name": "@umbrelladocs/linkspector",
-  "version": "0.4.1",
+  "version": "0.4.2",
   "description": "Uncover broken links in your content.",
   "type": "module",
   "main": "linkspector.js",
@@ -56,7 +56,6 @@
     "joi": "^17.13.3",
     "js-yaml": "^4.1.0",
     "kleur": "^4.1.5",
-    "lodash-es": "^4.17.21",
     "ora": "^8.2.0",
     "puppeteer": "^24.4.0",
     "remark-gfm": "^4.0.1",

package.json

@@ -0,0 +1,18 @@
+# Test Patterns
+
+## Links that should be ignored
+
+- [Ignored Link 1](https://ignored-domain.example.com/page1)
+- [Ignored Link 2](https://ignored-domain.example.com/page2)
+- [Ignored Link 3](https://another-ignored.example.com/test)
+
+## Links that should be replaced
+
+- [Replace Example 1](https://example.com/old/path1)
+- [Replace Example 2](https://example.com/old/path2)
+- [Replace Example 3](https://replace-domain.example.com/path3)
+
+## Normal links that should be checked
+
+- [Google](https://www.google.com)
+- [GitHub](https://github.com)

test/fixtures/patterns/patterns.md

@@ -0,0 +1,71 @@
+import { expect, test } from 'vitest'
+import { linkspector } from './linkspector.js'
+
+let cmd = {
+  json: true,
+}
+
+test('linkspector should correctly apply ignorePatterns and replacementPatterns', async () => {
+  let currentFile = ''
+  let results = []
+
+  for await (const { file, result } of linkspector(
+    './test/fixtures/patterns/patternsTest.yml',
+    cmd
+  )) {
+    currentFile = file
+    for (const linkStatusObj of result) {
+      results.push({
+        file: currentFile,
+        link: linkStatusObj.link,
+        status_code: linkStatusObj.status_code,
+        line_number: linkStatusObj.line_number,
+        position: linkStatusObj.position,
+        status: linkStatusObj.status,
+        error_message: linkStatusObj.error_message,
+      })
+    }
+  }
+
+  // Test expectations for pattern checks
+
+  // 1. Check that ignored links are not in the results
+  const ignoredLinks = [
+    'https://ignored-domain.example.com/page1',
+    'https://ignored-domain.example.com/page2',
+    'https://another-ignored.example.com/test',
+  ]
+
+  ignoredLinks.forEach((link) => {
+    expect(results.find((r) => r.link === link)).toBeUndefined()
+  })
+
+  // 2. Check that replacement patterns were applied
+  expect(
+    results.find((r) => r.link === 'https://example.com/new/path1')
+  ).toBeDefined()
+  expect(
+    results.find((r) => r.link === 'https://example.com/new/path2')
+  ).toBeDefined()
+  expect(
+    results.find((r) => r.link === 'https://new-domain.example.com/path3')
+  ).toBeDefined()
+
+  // 3. Confirm original links (before replacement) are not in the results
+  expect(
+    results.find((r) => r.link === 'https://example.com/old/path1')
+  ).toBeUndefined()
+  expect(
+    results.find((r) => r.link === 'https://example.com/old/path2')
+  ).toBeUndefined()
+  expect(
+    results.find((r) => r.link === 'https://replace-domain.example.com/path3')
+  ).toBeUndefined()
+
+  // 4. Check that normal links are still being checked
+  expect(results.find((r) => r.link === 'https://www.google.com')).toBeDefined()
+  expect(results.find((r) => r.link === 'https://github.com')).toBeDefined()
+
+  // Total number of links should be 5 (2 normal + 3 replaced)
+  expect(results.length).toBe(5)
+})

test/fixtures/patterns/patterns.test.js

@@ -0,0 +1,10 @@
+dirs:
+  - ./test/fixtures/patterns
+ignorePatterns:
+  - pattern: '^https://ignored-domain.example.com/.*$'
+  - pattern: '^https://another-ignored.example.com/.*$'
+replacementPatterns:
+  - pattern: 'https://example.com/old/(.*)'
+    replacement: 'https://example.com/new/$1'
+  - pattern: 'https://replace-domain.example.com/(.*)'
+    replacement: 'https://new-domain.example.com/$1'