Implement review fixes: semver library, scoping, case-insensitive matching, error logging, and configurable fail behavior (#7)

* Initial plan * Implement PR review comment fixes: semver library, error handling, case-insensitive matching, and rows scoping Co-authored-by: felickz <1760475+felickz@users.noreply.github.com> * Fix formatting and rebuild dist folder Co-authored-by: felickz <1760475+felickz@users.noreply.github.com> * Fix fail-closed logic and remove redundant @types/semver Co-authored-by: felickz <1760475+felickz@users.noreply.github.com> * Apply review feedback: fix empty range handling, add trimming, implement range check caching Co-authored-by: felickz <1760475+felickz@users.noreply.github.com> * Apply review feedback: align fail-closed behavior for empty version, fix TypeScript typing, normalize cache keys Co-authored-by: felickz <1760475+felickz@users.noreply.github.com> * Fix linter errors, optimize cache keys, and improve trimming logic Co-authored-by: felickz <1760475+felickz@users.noreply.github.com> * Add fail-open option for patch selection and optimize with preTrimmed flag Co-authored-by: felickz <1760475+felickz@users.noreply.github.com> * Enforce fail-closed with explicit validation, fix debug messages, normalize cache keys Co-authored-by: felickz <1760475+felickz@users.noreply.github.com> * Fix unreachable ternary in debug message and eliminate duplicate trim operation Co-authored-by: felickz <1760475+felickz@users.noreply.github.com> * Normalize eco comparison and add preNormalized option to avoid duplicate range conversion Co-authored-by: felickz <1760475+felickz@users.noreply.github.com> * Remove unnecessary cache, fix function signature, and correct semver comment Co-authored-by: felickz <1760475+felickz@users.noreply.github.com> * Make includePrerelease conditional based on version type to preserve range semantics Co-authored-by: felickz <1760475+felickz@users.noreply.github.com> * Improve debug message to report both invalid version and range when applicable Co-authored-by: felickz <1760475+felickz@users.noreply.github.com> * Convert to JSDoc, add explicit type annotation, and remove redundant initializer Co-authored-by: felickz <1760475+felickz@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: felickz <1760475+felickz@users.noreply.github.com>
2026-02-08 16:05:04 -05:00
parent 2af9bac14d
commit ee66ea100d
5 changed files with 240 additions and 122 deletions
@@ -1647,54 +1647,70 @@ exports.addSnapshotWarnings = addSnapshotWarnings;
 exports.addDeniedToSummary = addDeniedToSummary;
 const core = __importStar(__nccwpck_require__(37484));
 const utils_1 = __nccwpck_require__(69277);
+const semver = __importStar(__nccwpck_require__(62088));
 const icons = {
    check: '✅',
    cross: '❌',
    warning: '⚠️'
 };
 const MAX_SCANNED_FILES_BYTES = 1048576;
-// Helper to check if a version falls within a vulnerable range
-// Supports basic semver comparisons like ">= 8.0.0, <= 8.0.20"
-function versionInRange(version, range) {
-    if (!version || !range)
-        return false;
-    // Parse version into comparable parts
-    const vParts = version.split('.').map(p => parseInt(p, 10));
-    // Handle range formats like ">= 8.0.0, <= 8.0.20"
-    const conditions = range.split(',').map(c => c.trim());
-    for (const condition of conditions) {
-        const match = condition.match(/([><=]+)\s*(\d+(?:\.\d+)*)/);
-        if (!match)
-            continue;
-        const [, operator, rangeVer] = match;
-        const rParts = rangeVer.split('.').map(p => parseInt(p, 10));
-        // Compare versions part by part
-        let cmp = 0;
-        for (let i = 0; i < Math.max(vParts.length, rParts.length); i++) {
-            const v = vParts[i] || 0;
-            const r = rParts[i] || 0;
-            if (v > r) {
-                cmp = 1;
-                break;
-            }
-            else if (v < r) {
-                cmp = -1;
-                break;
-            }
+/**
+ * Helper to check if a version falls within a vulnerable range.
+ * Uses the `semver` library for proper prerelease handling and range parsing.
+ *
+ * @param version - The version to check (can be pre-trimmed).
+ * @param range - The version range to check against (can be pre-trimmed and/or pre-normalized).
+ * @param options - Configuration options.
+ * @param options.preTrimmed - If true, assumes inputs are already trimmed (optimization).
+ * @param options.preNormalized - If true, assumes range is already normalized (comma-to-space conversion done).
+ * @param options.failClosed - If true, returns true (vulnerable) on errors; if false, returns false (no match).
+ * @returns `true` if the version is considered within the vulnerable range (or on fail-closed), otherwise `false`.
+ */
+function versionInRange(version, range, options = {}) {
+    const { preTrimmed = false, preNormalized = false, failClosed = true } = options;
+    // Trim inputs if not pre-trimmed
+    const trimmedVersion = preTrimmed ? version : (version === null || version === void 0 ? void 0 : version.trim()) || '';
+    const trimmedRange = preTrimmed ? range : (range === null || range === void 0 ? void 0 : range.trim()) || '';
+    if (!trimmedVersion) {
+        if (failClosed) {
+            core.debug(`Empty or missing version for range "${range}". Treating as vulnerable (fail closed).`);
        }
-        // Check if condition is satisfied
-        if (operator === '>=' && cmp < 0)
-            return false;
-        if (operator === '>' && cmp <= 0)
-            return false;
-        if (operator === '<=' && cmp > 0)
-            return false;
-        if (operator === '<' && cmp >= 0)
-            return false;
-        if (operator === '=' && cmp !== 0)
-            return false;
+        return failClosed;
    }
-    return true;
+    if (!trimmedRange) {
+        if (failClosed) {
+            core.debug(`Empty or missing version range for version "${version}". Treating as vulnerable (fail closed).`);
+        }
+        return failClosed;
+    }
+    // Convert GitHub API range format to semver-compatible format if not already normalized
+    // GitHub uses: ">= 8.0.0, <= 8.0.20"
+    // Semver accepts: ">= 8.0.0 <= 8.0.20" (operators may be followed by a space)
+    const semverRange = preNormalized
+        ? trimmedRange
+        : trimmedRange.replace(/,\s*/g, ' ');
+    // Validate version and range explicitly to enforce fail-closed semantics
+    // semver.satisfies() typically returns false for invalid inputs without throwing
+    const validVersion = semver.valid(trimmedVersion);
+    const validRange = semver.validRange(semverRange);
+    if (!validVersion || !validRange) {
+        if (failClosed) {
+            const issues = [];
+            if (!validVersion)
+                issues.push('version');
+            if (!validRange)
+                issues.push('version range');
+            core.debug(`Invalid ${issues.join(' and ')}: version="${version}", range="${range}". Treating as vulnerable (fail closed).`);
+        }
+        return failClosed;
+    }
+    // Both version and range are valid; perform the satisfies check
+    // Only include prereleases when the version being checked is itself a prerelease
+    // to avoid changing range semantics globally
+    const isPrerelease = semver.prerelease(validVersion) !== null;
+    return semver.satisfies(validVersion, validRange, {
+        includePrerelease: isPrerelease
+    });
 }
 function extractPatchVersionId(patchData) {
    // Handle string format (current API response)
@@ -1792,7 +1808,6 @@ function addChangeVulnerabilitiesToSummary(vulnerableChanges, severity) {
        if (vulnerableChanges.length === 0) {
            return;
        }
-        const rows = [];
        const manifests = (0, utils_1.getManifestsSet)(vulnerableChanges);
        // Build set of unique advisories to query
        const advisorySet = new Set();
@@ -1836,12 +1851,15 @@ function addChangeVulnerabilitiesToSummary(vulnerableChanges, severity) {
                }
            }
            catch (e) {
-                core.debug(`API call failed for ${advId}: ${e}`);
+                const errorMessage = e instanceof Error ? e.message : String(e);
+                core.debug(`API call failed for ${advId}: ${errorMessage}`);
                patchInfo[advId] = [];
            }
        })));
        core.summary.addHeading('Vulnerabilities', 2);
        for (const manifest of manifests) {
+            // Create fresh rows array for each manifest to avoid accumulation
+            const rows = [];
            for (const change of vulnerableChanges.filter(pkg => pkg.manifest === manifest)) {
                let previous_package = '';
                let previous_version = '';
@@ -1850,20 +1868,37 @@ function addChangeVulnerabilitiesToSummary(vulnerableChanges, severity) {
                        previous_version === change.version;
                    // Look up patch version by matching package name, ecosystem, and version range
                    let patchVer = 'N/A';
-                    const advData = patchInfo[vuln.advisory_ghsa_id];
-                    if (advData && advData.length > 0) {
-                        const normalizedEco = change.ecosystem.toLowerCase();
-                        core.debug(`Looking up patch for ${change.name}@${change.version} (${normalizedEco}) in ${vuln.advisory_ghsa_id}`);
-                        // Find matching entry by ecosystem, package name, and version range
-                        const matchingEntry = advData.find(entry => entry.eco === normalizedEco &&
-                            entry.pkg === change.name &&
-                            versionInRange(change.version, entry.range));
-                        if (matchingEntry) {
-                            patchVer = matchingEntry.patch;
+                    const advisoryEntries = patchInfo[vuln.advisory_ghsa_id];
+                    if (advisoryEntries && advisoryEntries.length > 0) {
+                        const ecoLowercase = change.ecosystem.toLowerCase();
+                        const packageLowercase = change.name.toLowerCase();
+                        const normalizedChangeVersion = change.version.trim();
+                        core.debug(`Looking up patch for ${change.name}@${change.version} (${ecoLowercase}) in ${vuln.advisory_ghsa_id}`);
+                        // Find matching entry by ecosystem, package name (case-insensitive), and version range
+                        let foundEntry;
+                        for (const vulnEntry of advisoryEntries) {
+                            if (vulnEntry.eco.toLowerCase() !== ecoLowercase)
+                                continue;
+                            if (vulnEntry.pkg.toLowerCase() !== packageLowercase)
+                                continue;
+                            // Normalize range once for both cache key and function call
+                            const trimmedRangeOnce = vulnEntry.range.trim();
+                            const normalizedRange = trimmedRangeOnce.replace(/,\s*/g, ' ');
+                            // Use fail-open (failClosed: false) for patch selection to avoid
+                            // incorrectly matching on invalid ranges
+                            // Use preTrimmed and preNormalized optimizations since we've done both
+                            const isInRange = versionInRange(normalizedChangeVersion, normalizedRange, { preTrimmed: true, preNormalized: true, failClosed: false });
+                            if (isInRange) {
+                                foundEntry = vulnEntry;
+                                break;
+                            }
+                        }
+                        if (foundEntry) {
+                            patchVer = foundEntry.patch;
                            core.debug(`Found patch version ${patchVer} for ${change.name}@${change.version}`);
                        }
                        else {
-                            core.debug(`No matching patch found for ${change.name}@${change.version}. Available entries: ${JSON.stringify(advData)}`);
+                            core.debug(`No matching patch found for ${change.name}@${change.version}. Available entries: ${JSON.stringify(advisoryEntries)}`);
                        }
                    }
                    else {
@@ -62883,6 +62918,7 @@ const isSatisfiable = (comparators, options) => {
 // already replaced the hyphen ranges
 // turn into a set of JUST comparators.
 const parseComparator = (comp, options) => {
+  comp = comp.replace(re[t.BUILD], '')
  debug('comp', comp, options)
  comp = replaceCarets(comp, options)
  debug('caret', comp)
@@ -63303,11 +63339,25 @@ class SemVer {
      other = new SemVer(other, this.options)
    }

-    return (
-      compareIdentifiers(this.major, other.major) ||
-      compareIdentifiers(this.minor, other.minor) ||
-      compareIdentifiers(this.patch, other.patch)
-    )
+    if (this.major < other.major) {
+      return -1
+    }
+    if (this.major > other.major) {
+      return 1
+    }
+    if (this.minor < other.minor) {
+      return -1
+    }
+    if (this.minor > other.minor) {
+      return 1
+    }
+    if (this.patch < other.patch) {
+      return -1
+    }
+    if (this.patch > other.patch) {
+      return 1
+    }
+    return 0
  }

  comparePre (other) {
@@ -63765,7 +63815,7 @@ const diff = (version1, version2) => {
    return prefix + 'patch'
  }

-  // high and low are preleases
+  // high and low are prereleases
  return 'prerelease'
 }

@@ -64208,6 +64258,10 @@ module.exports = debug

 const numeric = /^[0-9]+$/
 const compareIdentifiers = (a, b) => {
+  if (typeof a === 'number' && typeof b === 'number') {
+    return a === b ? 0 : a < b ? -1 : 1
+  }
+
  const anum = numeric.test(a)
  const bnum = numeric.test(b)

@@ -64392,8 +64446,8 @@ createToken('MAINVERSIONLOOSE', `(${src[t.NUMERICIDENTIFIERLOOSE]})\\.` +

 // ## Pre-release Version Identifier
 // A numeric identifier, or a non-numeric identifier.
-// Non-numberic identifiers include numberic identifiers but can be longer.
-// Therefore non-numberic identifiers must go first.
+// Non-numeric identifiers include numeric identifiers but can be longer.
+// Therefore non-numeric identifiers must go first.

 createToken('PRERELEASEIDENTIFIER', `(?:${src[t.NONNUMERICIDENTIFIER]
 }|${src[t.NUMERICIDENTIFIER]})`)
@@ -64915,7 +64969,7 @@ const compare = __nccwpck_require__(78469)
 // - If LT
 //   - If LT.semver is greater than any < or <= comp in C, return false
 //   - If LT is <=, and LT.semver does not satisfy every C, return false
-//   - If GT.semver has a prerelease, and not in prerelease mode
+//   - If LT.semver has a prerelease, and not in prerelease mode
 //     - If no C has a prerelease and the LT.semver tuple, return false
 // - Else return true

@@ -20,6 +20,7 @@
        "got": "^14.4.7",
        "jest": "^29.7.0",
        "octokit": "^3.1.2",
+        "semver": "^7.7.4",
        "spdx-expression-parse": "^3.0.1",
        "spdx-satisfies": "^6.0.0",
        "ts-jest": "^29.4.1",
@@ -2649,10 +2650,11 @@
      }
    },
    "node_modules/@types/semver": {
-      "version": "7.5.5",
-      "resolved": "https://registry.npmjs.org/@types/semver/-/semver-7.5.5.tgz",
-      "integrity": "sha512-+d+WYC1BxJ6yVOgUgzK8gWvp5qF8ssV5r4nsDcZWKRWcDQLQ619tvWAxJQYGgBrO1MnLJC7a5GtiYsAoQ47dJg==",
-      "dev": true
+      "version": "7.7.1",
+      "resolved": "https://registry.npmjs.org/@types/semver/-/semver-7.7.1.tgz",
+      "integrity": "sha512-FmgJfu+MOcQ370SD0ev7EI8TlCAfKYU+B4m5T3yXc1CiRN94g/SZPtsCkk506aUDtlMnFZvasDwHHUcZUEaYuA==",
+      "dev": true,
+      "license": "MIT"
    },
    "node_modules/@types/spdx-expression-parse": {
      "version": "3.0.5",
@@ -8264,9 +8266,9 @@
      }
    },
    "node_modules/semver": {
-      "version": "7.7.2",
-      "resolved": "https://registry.npmjs.org/semver/-/semver-7.7.2.tgz",
-      "integrity": "sha512-RF0Fw+rO5AMf9MAyaRXI4AV0Ulj5lMHqVxxdSgiVbixSCXoEmmX/jk0CuJw4+3SqroYO9VoUh+HcuJivvtJemA==",
+      "version": "7.7.4",
+      "resolved": "https://registry.npmjs.org/semver/-/semver-7.7.4.tgz",
+      "integrity": "sha512-vFKC2IEtQnVhpT78h1Yp8wzwrf8CM+MzKMHGJZfBtzhZNycRFnXsHk6E5TxIkkMsgNS7mdX3AGB7x2QM2di4lA==",
      "license": "ISC",
      "bin": {
        "semver": "bin/semver.js"
@@ -36,6 +36,7 @@
    "got": "^14.4.7",
    "jest": "^29.7.0",
    "octokit": "^3.1.2",
+    "semver": "^7.7.4",
    "spdx-expression-parse": "^3.0.1",
    "spdx-satisfies": "^6.0.0",
    "ts-jest": "^29.4.1",
@@ -8,6 +8,7 @@ import {
  renderUrl,
  octokitClient
 } from './utils'
+import * as semver from 'semver'

 const icons = {
  check: '✅',
@@ -17,47 +18,81 @@ const icons = {

 const MAX_SCANNED_FILES_BYTES = 1048576

-// Helper to check if a version falls within a vulnerable range
-// Supports basic semver comparisons like ">= 8.0.0, <= 8.0.20"
-function versionInRange(version: string, range: string): boolean {
-  if (!version || !range) return false
+/**
+ * Helper to check if a version falls within a vulnerable range.
+ * Uses the `semver` library for proper prerelease handling and range parsing.
+ *
+ * @param version - The version to check (can be pre-trimmed).
+ * @param range - The version range to check against (can be pre-trimmed and/or pre-normalized).
+ * @param options - Configuration options.
+ * @param options.preTrimmed - If true, assumes inputs are already trimmed (optimization).
+ * @param options.preNormalized - If true, assumes range is already normalized (comma-to-space conversion done).
+ * @param options.failClosed - If true, returns true (vulnerable) on errors; if false, returns false (no match).
+ * @returns `true` if the version is considered within the vulnerable range (or on fail-closed), otherwise `false`.
+ */
+function versionInRange(
+  version: string | undefined,
+  range: string | undefined,
+  options: {
+    preTrimmed?: boolean
+    preNormalized?: boolean
+    failClosed?: boolean
+  } = {}
+): boolean {
+  const {preTrimmed = false, preNormalized = false, failClosed = true} = options

-  // Parse version into comparable parts
-  const vParts = version.split('.').map(p => parseInt(p, 10))
+  // Trim inputs if not pre-trimmed
+  const trimmedVersion = preTrimmed ? version : version?.trim() || ''
+  const trimmedRange = preTrimmed ? range : range?.trim() || ''

-  // Handle range formats like ">= 8.0.0, <= 8.0.20"
-  const conditions = range.split(',').map(c => c.trim())
-
-  for (const condition of conditions) {
-    const match = condition.match(/([><=]+)\s*(\d+(?:\.\d+)*)/)
-    if (!match) continue
-
-    const [, operator, rangeVer] = match
-    const rParts = rangeVer.split('.').map(p => parseInt(p, 10))
-
-    // Compare versions part by part
-    let cmp = 0
-    for (let i = 0; i < Math.max(vParts.length, rParts.length); i++) {
-      const v = vParts[i] || 0
-      const r = rParts[i] || 0
-      if (v > r) {
-        cmp = 1
-        break
-      } else if (v < r) {
-        cmp = -1
-        break
-      }
+  if (!trimmedVersion) {
+    if (failClosed) {
+      core.debug(
+        `Empty or missing version for range "${range}". Treating as vulnerable (fail closed).`
+      )
    }
-
-    // Check if condition is satisfied
-    if (operator === '>=' && cmp < 0) return false
-    if (operator === '>' && cmp <= 0) return false
-    if (operator === '<=' && cmp > 0) return false
-    if (operator === '<' && cmp >= 0) return false
-    if (operator === '=' && cmp !== 0) return false
+    return failClosed
+  }
+  if (!trimmedRange) {
+    if (failClosed) {
+      core.debug(
+        `Empty or missing version range for version "${version}". Treating as vulnerable (fail closed).`
+      )
+    }
+    return failClosed
  }

-  return true
+  // Convert GitHub API range format to semver-compatible format if not already normalized
+  // GitHub uses: ">= 8.0.0, <= 8.0.20"
+  // Semver accepts: ">= 8.0.0 <= 8.0.20" (operators may be followed by a space)
+  const semverRange = preNormalized
+    ? trimmedRange
+    : trimmedRange.replace(/,\s*/g, ' ')
+
+  // Validate version and range explicitly to enforce fail-closed semantics
+  // semver.satisfies() typically returns false for invalid inputs without throwing
+  const validVersion = semver.valid(trimmedVersion)
+  const validRange = semver.validRange(semverRange)
+
+  if (!validVersion || !validRange) {
+    if (failClosed) {
+      const issues: string[] = []
+      if (!validVersion) issues.push('version')
+      if (!validRange) issues.push('version range')
+      core.debug(
+        `Invalid ${issues.join(' and ')}: version="${version}", range="${range}". Treating as vulnerable (fail closed).`
+      )
+    }
+    return failClosed
+  }
+
+  // Both version and range are valid; perform the satisfies check
+  // Only include prereleases when the version being checked is itself a prerelease
+  // to avoid changing range semantics globally
+  const isPrerelease = semver.prerelease(validVersion) !== null
+  return semver.satisfies(validVersion, validRange, {
+    includePrerelease: isPrerelease
+  })
 }

 function extractPatchVersionId(patchData: unknown): string | null {
@@ -201,7 +236,6 @@ export async function addChangeVulnerabilitiesToSummary(
    return
  }

-  const rows: SummaryTableRow[] = []
  const manifests = getManifestsSet(vulnerableChanges)

  // Build set of unique advisories to query
@@ -258,7 +292,8 @@ export async function addChangeVulnerabilitiesToSummary(
          }
        }
      } catch (e) {
-        core.debug(`API call failed for ${advId}: ${e}`)
+        const errorMessage = e instanceof Error ? e.message : String(e)
+        core.debug(`API call failed for ${advId}: ${errorMessage}`)
        patchInfo[advId] = []
      }
    })
@@ -267,6 +302,9 @@ export async function addChangeVulnerabilitiesToSummary(
  core.summary.addHeading('Vulnerabilities', 2)

  for (const manifest of manifests) {
+    // Create fresh rows array for each manifest to avoid accumulation
+    const rows: SummaryTableRow[] = []
+
    for (const change of vulnerableChanges.filter(
      pkg => pkg.manifest === manifest
    )) {
@@ -279,27 +317,50 @@ export async function addChangeVulnerabilitiesToSummary(

        // Look up patch version by matching package name, ecosystem, and version range
        let patchVer = 'N/A'
-        const advData = patchInfo[vuln.advisory_ghsa_id]
-        if (advData && advData.length > 0) {
-          const normalizedEco = change.ecosystem.toLowerCase()
+        const advisoryEntries = patchInfo[vuln.advisory_ghsa_id]
+        if (advisoryEntries && advisoryEntries.length > 0) {
+          const ecoLowercase = change.ecosystem.toLowerCase()
+          const packageLowercase = change.name.toLowerCase()
+          const normalizedChangeVersion = change.version.trim()
          core.debug(
-            `Looking up patch for ${change.name}@${change.version} (${normalizedEco}) in ${vuln.advisory_ghsa_id}`
+            `Looking up patch for ${change.name}@${change.version} (${ecoLowercase}) in ${vuln.advisory_ghsa_id}`
          )
-          // Find matching entry by ecosystem, package name, and version range
-          const matchingEntry = advData.find(
-            entry =>
-              entry.eco === normalizedEco &&
-              entry.pkg === change.name &&
-              versionInRange(change.version, entry.range)
-          )
-          if (matchingEntry) {
-            patchVer = matchingEntry.patch
+
+          // Find matching entry by ecosystem, package name (case-insensitive), and version range
+          let foundEntry:
+            | {eco: string; pkg: string; range: string; patch: string}
+            | undefined
+          for (const vulnEntry of advisoryEntries) {
+            if (vulnEntry.eco.toLowerCase() !== ecoLowercase) continue
+            if (vulnEntry.pkg.toLowerCase() !== packageLowercase) continue
+
+            // Normalize range once for both cache key and function call
+            const trimmedRangeOnce = vulnEntry.range.trim()
+            const normalizedRange = trimmedRangeOnce.replace(/,\s*/g, ' ')
+
+            // Use fail-open (failClosed: false) for patch selection to avoid
+            // incorrectly matching on invalid ranges
+            // Use preTrimmed and preNormalized optimizations since we've done both
+            const isInRange = versionInRange(
+              normalizedChangeVersion,
+              normalizedRange,
+              {preTrimmed: true, preNormalized: true, failClosed: false}
+            )
+
+            if (isInRange) {
+              foundEntry = vulnEntry
+              break
+            }
+          }
+
+          if (foundEntry) {
+            patchVer = foundEntry.patch
            core.debug(
              `Found patch version ${patchVer} for ${change.name}@${change.version}`
            )
          } else {
            core.debug(
-              `No matching patch found for ${change.name}@${change.version}. Available entries: ${JSON.stringify(advData)}`
+              `No matching patch found for ${change.name}@${change.version}. Available entries: ${JSON.stringify(advisoryEntries)}`
            )
          }
        } else {