[ENG-2586] Harden version comparison with stricter normalization and expanded tests

.github/workflows/tests.yml

@@ -11,8 +11,7 @@ jobs:
     runs-on: ubuntu-latest
     strategy:
       matrix:
-        php: ['8.1', '8.2', '8.3']
-
+        php: ['8.2', '8.3', '8.4']
     name: PHP ${{ matrix.php }}
 
     steps:
@@ -24,8 +23,16 @@ jobs:
           php-version: ${{ matrix.php }}
           coverage: none
 
+      - name: Cache Composer dependencies
+        uses: actions/cache@v4
+        with:
+          path: vendor
+          key: ${{ runner.os }}-composer-${{ hashFiles('composer.lock') }}
+          restore-keys: |
+            ${{ runner.os }}-composer-
+
       - name: Install dependencies
-        run: composer install --no-interaction --prefer-dist
+        run: composer install --no-progress --no-interaction --prefer-dist
 
       - name: Run tests
         run: vendor/bin/pest

.gitignore

@@ -1,3 +1,4 @@
 /vendor/
 composer.lock
 .phpunit.result.cache
+.phpunit.cache

composer.json

@@ -4,7 +4,7 @@
     "type": "library",
     "license": "MIT",
     "require": {
-        "php": "^8.1"
+        "php": "^8.2"
     },
     "require-dev": {
         "pestphp/pest": "^2.0|^3.0"

phpunit.xml

@@ -0,0 +1,16 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<phpunit
+    colors="true"
+    cacheDirectory=".phpunit.cache"
+>
+    <testsuites>
+        <testsuite name="Tests">
+            <directory>tests</directory>
+        </testsuite>
+    </testsuites>
+    <source>
+        <include>
+            <directory>src</directory>
+        </include>
+    </source>
+</phpunit>

src/CheckVersionVulnerability.php

@@ -17,43 +17,52 @@ public function __construct(private readonly CompareVersions $compareVersions) {
     public function execute(string $currentVersion, string $affectedIn, VersionStrategy $strategy = VersionStrategy::Standard): bool
     {
         $affectedIn = trim($affectedIn);
+        $currentVersion = trim($currentVersion);
 
-        if (empty($affectedIn) || empty($currentVersion)) {
+        if ($affectedIn === '' || $currentVersion === '') {
             return false;
         }
 
         if ($affectedIn === '*') {
             return true;
         }
 
-        $currentVersion = str_replace('v', '', strtolower($currentVersion));
+        $currentVersion = $this->normalizeVersion($currentVersion);
 
-        if (strpos($affectedIn, '<') !== false) {
-            $affectedIn = preg_replace('/(<=?) *([0-9a-zA-Z\.]*)/m', '$1 $2', $affectedIn);
-            $parts = explode(' ', $affectedIn);
-
-            return $this->compareVersions->execute($currentVersion, $parts[1], $parts[0], $strategy);
+        if (preg_match('/^(<=?)\s*(\S+)/', $affectedIn, $matches)) {
+            return $this->compareVersions->execute($currentVersion, $matches[2], $matches[1], $strategy);
         }
 
-        if (strpos($affectedIn, ',') !== false) {
+        if (str_contains($affectedIn, ',')) {
             $versions = explode(',', $affectedIn);
 
             foreach ($versions as $version) {
-                if (trim($version) == $currentVersion) {
+                if ($this->normalizeVersion($version) === $currentVersion) {
                     return true;
                 }
             }
 
             return false;
         }
 
-        if (strpos($affectedIn, '-') !== false) {
-            $parts = explode('-', $affectedIn);
+        if (preg_match('/^\d+(?:\.\d+)*\s*-\s*\d+(?:\.\d+)*$/', $affectedIn)) {
+            $parts = explode('-', $affectedIn, 2);
 
             return $this->compareVersions->execute($currentVersion, trim($parts[0]), '>=', $strategy)
                 && $this->compareVersions->execute($currentVersion, trim($parts[1]), '<=', $strategy);
         }
 
-        return $currentVersion == $affectedIn;
+        return $currentVersion === $this->normalizeVersion($affectedIn);
+    }
+
+    private function normalizeVersion(string $version): string
+    {
+        $version = strtolower(trim($version));
+
+        if (str_starts_with($version, 'v')) {
+            $version = substr($version, 1);
+        }
+
+        return $version;
     }
 }

src/Drupal/CheckDrupalVersionVulnerability.php

@@ -23,16 +23,17 @@ public function __construct(
     public function execute(string $currentVersion, string $affectedIn, VersionStrategy $strategy = VersionStrategy::Standard): bool
     {
         $affectedIn = trim($affectedIn);
+        $currentVersion = trim($currentVersion);
 
-        if (empty($affectedIn) || empty($currentVersion)) {
+        if ($affectedIn === '' || $currentVersion === '') {
             return false;
         }
 
         if ($affectedIn === '*') {
             return true;
         }
 
-        if (stripos($affectedIn, ',') !== false) {
+        if (str_contains($affectedIn, ',')) {
             $versions = explode(',', $affectedIn);
 
             foreach ($versions as $version) {
@@ -44,7 +45,7 @@ public function execute(string $currentVersion, string $affectedIn, VersionStrat
             return false;
         }
 
-        $currentVersion = str_replace('v', '', strtolower($currentVersion));
+        $currentVersion = $this->normalizeVersion($currentVersion);
 
         if (! $this->normalizer->majorVersionsMatch($affectedIn, $currentVersion)) {
             return false;
@@ -53,20 +54,28 @@ public function execute(string $currentVersion, string $affectedIn, VersionStrat
         $affectedIn = $this->normalizer->normalize($affectedIn);
         $currentVersion = $this->normalizer->normalize($currentVersion);
 
-        if (strpos($affectedIn, '<') !== false) {
-            $affectedIn = preg_replace('/(<=?) *([0-9a-zA-Z\.]*)/m', '$1 $2', $affectedIn);
-            $parts = explode(' ', $affectedIn);
-
-            return $this->compareVersions->execute($currentVersion, $parts[1], $parts[0], $strategy);
+        if (preg_match('/^(<=?)\s*(\S+)/', $affectedIn, $matches)) {
+            return $this->compareVersions->execute($currentVersion, $matches[2], $matches[1], $strategy);
         }
 
-        if (strpos($affectedIn, '-') !== false) {
-            $parts = explode('-', $affectedIn);
+        if (preg_match('/^\d+(?:\.\d+)*\s*-\s*\d+(?:\.\d+)*$/', $affectedIn)) {
+            $parts = explode('-', $affectedIn, 2);
 
             return $this->compareVersions->execute($currentVersion, trim($parts[0]), '>=', $strategy)
                 && $this->compareVersions->execute($currentVersion, trim($parts[1]), '<=', $strategy);
         }
 
-        return $currentVersion == $affectedIn;
+        return $currentVersion === $this->normalizeVersion($affectedIn);
+    }
+
+    private function normalizeVersion(string $version): string
+    {
+        $version = strtolower(trim($version));
+
+        if (str_starts_with($version, 'v')) {
+            $version = substr($version, 1);
+        }
+
+        return $version;
     }
 }

tests/CheckVersionVulnerabilityTest.php

@@ -64,6 +64,51 @@
     expect($this->action->execute('v1.5', '1.5'))->toBeTrue();
 });
 
+it('does not corrupt versions containing v in body', function (string $currentVersion, string $affectedIn, bool $expected) {
+    expect($this->action->execute($currentVersion, $affectedIn))->toBe($expected);
+})->with([
+    'revision exact match' => ['6.3-revision-0', '6.3-revision-0', true],
+    'dev exact match' => ['1.0-dev', '1.0-dev', true],
+    'preview exact match' => ['5.0.37.preview', '5.0.37.preview', true],
+]);
+
+it('handles exact match for versions with dashes', function (string $currentVersion, string $affectedIn, bool $expected) {
+    expect($this->action->execute($currentVersion, $affectedIn))->toBe($expected);
+})->with([
+    'beta suffix: match' => ['4.5.5-beta', '4.5.5-beta', true],
+    'beta suffix: no match' => ['4.5.6', '4.5.5-beta', false],
+    'revision suffix: match' => ['6.3-revision-0', '6.3-revision-0', true],
+    'revision suffix: no match' => ['6.3-revision-1', '6.3-revision-0', false],
+    'alpha segment: match' => ['12.8-a.1', '12.8-a.1', true],
+    'alpha segment: no match' => ['12.8-a.3', '12.8-a.1', false],
+]);
+
+it('does not match versions via scientific notation coercion', function () {
+    expect($this->action->execute('10', '1e1'))->toBeFalse()
+        ->and($this->action->execute('10', '1e1, 2.0'))->toBeFalse()
+        ->and($this->action->execute('100', '1e2'))->toBeFalse();
+});
+
+it('handles version zero as valid input', function () {
+    expect($this->action->execute('0', '<= 1.0'))->toBeTrue()
+        ->and($this->action->execute('0', '*'))->toBeTrue();
+});
+
+it('normalizes v prefix in affectedIn for comma and exact paths', function () {
+    expect($this->action->execute('v1.5', 'v1.0, v1.5, v2.0'))->toBeTrue()
+        ->and($this->action->execute('V1.5', 'V1.5'))->toBeTrue();
+});
+
+it('rejects whitespace-only current version', function () {
+    expect($this->action->execute('   ', '<= 3.0'))->toBeFalse()
+        ->and($this->action->execute('   ', '*'))->toBeFalse();
+});
+
+it('returns false for malformed operator expressions', function () {
+    expect($this->action->execute('3.5', '3.41<'))->toBeFalse()
+        ->and($this->action->execute('3.5', 'version < 3.41'))->toBeFalse();
+});
+
 it('handles real-world version formats', function (string $currentVersion, string $affectedIn, bool $expected) {
     expect($this->action->execute($currentVersion, $affectedIn))->toBe($expected);
 })->with([
@@ -72,5 +117,21 @@
     'suffixed decaf: vulnerable' => ['5.0.37.decaf', '<= 5.0.37.decaf', true],
     'suffixed decaf: fixed' => ['5.0.53.decaf', '<= 5.0.37.decaf', false],
     'pre-release beta: vulnerable' => ['4.5.5-beta', '<= 4.5.5-beta', true],
+    'pre-release beta: fixed' => ['4.5.6', '<= 4.5.5-beta', false],
+    'alpha segment: vulnerable' => ['12.8-a.1', '<= 12.8-a.1', true],
+    'alpha segment: fixed' => ['12.8-a.3', '<= 12.8-a.1', false],
+    'compound dash: vulnerable' => ['25.07010000-WP6.8.1-JB5.11.5', '<= 25.07010000-WP6.8.1-JB5.11.5', true],
+    'compound dash: fixed' => ['25.09000000-WP6.8.2-JB5.12.0', '<= 25.07010000-WP6.8.1-JB5.11.5', false],
     'revision-based: vulnerable' => ['6.3-revision-0', '<= 6.3-revision-0', true],
+    'revision-based: fixed' => ['6.3-revision-1', '<= 6.3-revision-0', false],
 ]);
+
+it('applies decimal strategy to strict less-than operator', function () {
+    expect($this->action->execute('3.5', '< 3.41', VersionStrategy::DecimalNormalized))->toBeFalse()
+        ->and($this->action->execute('3.40', '< 3.41', VersionStrategy::DecimalNormalized))->toBeTrue();
+});
+
+it('applies decimal strategy to range format', function () {
+    expect($this->action->execute('3.5', '3.41-3.6', VersionStrategy::DecimalNormalized))->toBeTrue()
+        ->and($this->action->execute('3.39', '3.41-3.6', VersionStrategy::DecimalNormalized))->toBeFalse();
+});

tests/Drupal/CheckDrupalVersionVulnerabilityTest.php

@@ -52,3 +52,36 @@
     expect($this->action->execute('8.x-3.5', '8.x-3.5'))->toBeTrue()
         ->and($this->action->execute('8.x-3.6', '8.x-3.5'))->toBeFalse();
 });
+
+it('does not corrupt Drupal versions containing v in body', function () {
+    expect($this->action->execute('8.x-3.5-dev', '<= 8.x-3.5-dev'))->toBeTrue();
+});
+
+it('handles exact match for Drupal versions with dashes after prefix stripping', function () {
+    expect($this->action->execute('8.x-3.5-beta', '8.x-3.5-beta'))->toBeTrue()
+        ->and($this->action->execute('8.x-3.6', '8.x-3.5-beta'))->toBeFalse();
+});
+
+it('does not match Drupal versions via scientific notation coercion', function () {
+    expect($this->action->execute('10', '1e1'))->toBeFalse();
+});
+
+it('handles version zero as valid Drupal input', function () {
+    expect($this->action->execute('0', '<= 1.0'))->toBeTrue()
+        ->and($this->action->execute('0', '*'))->toBeTrue();
+});
+
+it('rejects whitespace-only Drupal current version', function () {
+    expect($this->action->execute('   ', '<= 3.0'))->toBeFalse()
+        ->and($this->action->execute('   ', '*'))->toBeFalse();
+});
+
+it('returns false for malformed Drupal operator expressions', function () {
+    expect($this->action->execute('3.5', '3.41<'))->toBeFalse()
+        ->and($this->action->execute('3.5', 'version < 3.41'))->toBeFalse();
+});
+
+it('handles Drupal with decimal normalized strategy', function () {
+    expect($this->action->execute('8.x-3.5', '<= 8.x-3.41', \Patchstack\VersionCompare\VersionStrategy::DecimalNormalized))->toBeFalse()
+        ->and($this->action->execute('8.x-3.40', '<= 8.x-3.41', \Patchstack\VersionCompare\VersionStrategy::DecimalNormalized))->toBeTrue();
+});

tests/VersionStrategyTest.php

@@ -0,0 +1,10 @@
+<?php
+
+declare(strict_types=1);
+
+use Patchstack\VersionCompare\VersionStrategy;
+
+it('creates strategy from abnormal version flag', function () {
+    expect(VersionStrategy::fromAbnormalFlag(true))->toBe(VersionStrategy::DecimalNormalized)
+        ->and(VersionStrategy::fromAbnormalFlag(false))->toBe(VersionStrategy::Standard);
+});