From 811416bf635cdbd6a21750eeb4a4b1b6a458103b Mon Sep 17 00:00:00 2001 From: DualFroz Date: Thu, 3 Sep 2026 14:28:48 +0200 Subject: [PATCH] fix: distinguish PCRE failure from a mismatch in Assert::regex() preg_match() has three outcomes -- 1, 0 and false -- but regex() folded false into the "no match" branch. An uncompilable pattern or an exhausted PCRE limit therefore leaked a raw PHP warning out of the library and then reported the misleading "The value ... does not match the expected pattern." Check for false separately and report the pattern together with preg_last_error_msg(), following the existing false === $x = nativeCall() handling in Assert::strlen(). The caller's custom message is intentionally not reused for that branch, since it describes a mismatch that never happened. Matching values and genuine mismatches are unaffected. --- PR.md | 181 +++++++++++++++++++++++++++++++++++++++++++ src/Assert.php | 10 ++- tests/AssertTest.php | 61 +++++++++++++++ 3 files changed, 251 insertions(+), 1 deletion(-) create mode 100644 PR.md diff --git a/PR.md b/PR.md new file mode 100644 index 0000000..cea66ea --- /dev/null +++ b/PR.md @@ -0,0 +1,181 @@ +# Make Assert::regex() report PCRE failures instead of a bogus mismatch + +Fixes #119 (one instance of it -- see "Scope" below). + +## Problem + +`Assert::regex()` treats the return value of `preg_match()` as a boolean. `preg_match()` +has three outcomes, not two: `1` (match), `0` (no match) and `false` (the engine could not +run the match at all). Folding `false` into the `0` branch produces two bad behaviours. + +**1. A raw PHP warning escapes from inside the library, followed by a misleading exception.** + +```php +Assert::regex('abc', '/(/'); +``` + +on PHP 8.3.6 before this change: + +``` +PHP Warning: preg_match(): Compilation failed: missing closing parenthesis at offset 1 in .../src/Assert.php on line 1510 +Webmozart\Assert\InvalidArgumentException: The value "abc" does not match the expected pattern. +``` + +The warning is emitted from library code the caller does not control, and the exception +then blames the value for what is actually a broken pattern. + +**2. A PCRE engine failure is silently reclassified as a failed assertion.** + +With `pcre.backtrack_limit=100`: + +```php +Assert::regex(str_repeat('a', 30).'c', '/^(a+)+$/'); +// Webmozart\Assert\InvalidArgumentException: The value "aaaa...c" does not match the expected pattern. +// preg_last_error_msg() === 'Backtrack limit exhausted' +``` + +Here `preg_match()` returned `false` because it hit the backtrack limit -- it never +determined whether the value matches. The caller is told the value failed validation. +That is the more dangerous of the two: on a large input, a resource limit becomes an +indistinguishable "invalid input" error, and the real cause is invisible. + +The same masking happens for `PREG_BAD_UTF8_ERROR`, `PREG_JIT_STACKLIMIT_ERROR`, and so on. + +## Root cause + +`src/Assert.php:1510` + +```php +if (!\preg_match($pattern, $value)) { +``` + +`!false` and `!0` are both `true`, so the "engine failed" and "value did not match" cases +are indistinguishable, and nothing suppresses or reports the warning `preg_match()` raises +on its way out. + +## Solution + +Separate the three outcomes, mirroring how this repository already handles a native +function that can return `false` (`Assert::strlen()`, `src/Assert.php:2590`: +`if (false === $encoding = \mb_detect_encoding($value)) {`). + +```php +if (false === $result = @\preg_match($pattern, $value)) { + static::reportInvalidArgument(\sprintf( + 'The pattern %s could not be evaluated: %s.', + static::valueToString($pattern), + \preg_last_error_msg() + )); +} + +if (0 === $result) { + // unchanged: the existing mismatch message, including the caller's custom message +} +``` + +Now: + +``` +Assert::regex('abc', '/(/'); + -> InvalidArgumentException: The pattern "/(/" could not be evaluated: Internal error. + (no PHP warning) + +Assert::regex(str_repeat('a', 30).'c', '/^(a+)+$/'); // pcre.backtrack_limit=100 + -> InvalidArgumentException: The pattern "/^(a+)+$/" could not be evaluated: Backtrack limit exhausted. +``` + +Notes on the two deliberate choices here: + +- **The caller's custom `$message` is not applied to the engine-failure branch.** A custom + message such as `'The value %s is not a valid slug.'` describes a mismatch. Reusing it for + a pattern that never compiled would preserve exactly the confusion this change removes. +- **The diagnostic comes from `preg_last_error_msg()` rather than from the raised warning + text.** Capturing the warning text ("Compilation failed: missing closing parenthesis at + offset 1") would require `set_error_handler()`/`restore_error_handler()`, and both are + listed in Psalm's `dictionaries/ImpureFunctionsList.php`, so calling them inside a method + annotated `@psalm-pure` is the wrong trade. `preg_match()` and `preg_last_error_msg()` are + not in that list. + +Behaviour on the normal paths is unchanged: a matching value still returns the value, and a +genuine mismatch still throws with the existing default or custom message. No public +signature changes, so no BC break. + +## Scope + +Issue #119 is broadly worded ("Some assertions call functions that may generate a warning +... e.g. preg_match, strlen"). **This PR fixes only `regex()`**, which is one instance of +that class of problem. I did not extend it to `length()`, `contains()` or the `count*` +assertions, because I have no reproduction showing a defect there, and guessing at those +would make the change harder to review. + +One sibling is worth flagging explicitly, though I left it out to keep this PR to a single +behaviour change. `notRegex()` (`src/Assert.php:1533` before this change) has the same root +shape with the opposite and arguably worse symptom -- because it checks the truthiness of +`preg_match()` directly, a pattern that fails to compile makes the assertion silently +**pass**: + +```php +Assert::notRegex('abc', '/(/'); // emits a PHP warning, then returns 'abc' successfully +``` + +Happy to fold `notRegex()` into this PR, or file it separately, whichever you prefer. + +## How this was tested + +Environment: PHP 8.3.6, Composer 2.10.2, on commit `2ccb7c2` ("Prepare for 2.4.1 release"). + +Four tests were added to `tests/AssertTest.php`, following the existing standalone-test +convention (`testResourceOfTypeCustomMessage`, `testEnumAssertionErrorMessage`): + +- `testRegexRejectsAnUncompilablePattern` +- `testRegexSuppressesTheWarningForAnUncompilablePattern` +- `testRegexDoesNotUseTheCustomMessageForAnUncompilablePattern` +- `testRegexRejectsAPatternExceedingTheBacktrackLimit` + +**Each new test fails without the source change and passes with it.** + +With `src/Assert.php` reverted to its original state and only the new tests applied: + +``` +$ ./tools/phpunit/vendor/bin/phpunit --filter 'testRegex' +1) Webmozart\Assert\Tests\AssertTest::testRegexRejectsAnUncompilablePattern +2) Webmozart\Assert\Tests\AssertTest::testRegexSuppressesTheWarningForAnUncompilablePattern +3) Webmozart\Assert\Tests\AssertTest::testRegexDoesNotUseTheCustomMessageForAnUncompilablePattern +4) Webmozart\Assert\Tests\AssertTest::testRegexRejectsAPatternExceedingTheBacktrackLimit +FAILURES! +Tests: 4, Assertions: 6, Failures: 4, Warnings: 1. +``` + +With the fix applied: + +``` +$ ./tools/phpunit/vendor/bin/phpunit --filter 'testRegex' +OK (4 tests, 6 assertions) +``` + +Full suite before the change: + +``` +$ composer install && composer --working-dir=tools/phpunit install && ./tools/phpunit/vendor/bin/phpunit +Tests: 4906, Assertions: 5823, Notices: 2, Skipped: 56. +``` + +Full suite after the change: + +``` +$ ./tools/phpunit/vendor/bin/phpunit +Tests: 4910, Assertions: 5829, Notices: 2, Skipped: 56. +``` + +Same 2 notices and 56 skips in both runs -- they are pre-existing on a clean checkout +(the notices originate from `tests/AssertTest.php:618`) and are unrelated to this change. +The 4 added tests account for the entire delta. + +One caveat on local verification: `composer run cs-check` and `composer run static-analysis` +could **not** be run on this machine. `friendsofphp/php-cs-fixer v3.92.3` requires PHP >= 8.4 +via `symfony/console v8.0.3`, and `vimeo/psalm 6.14.3` refuses to start with +`Psalm requires a PHP version ">= 8.3.16". You are running 8.3.6.` The added code was instead +matched by hand to the conventions already in the tree: Yoda comparisons (`false === $x`, as +in `src/Assert.php:829/897/2590`), global classes imported rather than written with a leading +backslash (as with `LogicException`, `Error`, `Exception` in `tests/AssertTest.php`), and +concatenation without surrounding spaces. CI will be the real check on both of those gates. diff --git a/src/Assert.php b/src/Assert.php index 31c861b..8544ea9 100644 --- a/src/Assert.php +++ b/src/Assert.php @@ -1507,7 +1507,15 @@ class Assert static::string($value); static::string($pattern); - if (!\preg_match($pattern, $value)) { + if (false === $result = @\preg_match($pattern, $value)) { + static::reportInvalidArgument(\sprintf( + 'The pattern %s could not be evaluated: %s.', + static::valueToString($pattern), + \preg_last_error_msg() + )); + } + + if (0 === $result) { $message = self::resolveMessage($message); static::reportInvalidArgument(\sprintf( $message ?: 'The value %s does not match the expected pattern.', diff --git a/tests/AssertTest.php b/tests/AssertTest.php index f0ec064..38f0362 100644 --- a/tests/AssertTest.php +++ b/tests/AssertTest.php @@ -19,6 +19,7 @@ use DateTime; use DateTimeImmutable; use Error; use Exception; +use InvalidArgumentException; use LogicException; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; @@ -991,6 +992,66 @@ class AssertTest extends TestCase Assert::null(DummyEnum::CaseName, 'Expected null. Got: %s'); } + public function testRegexRejectsAnUncompilablePattern(): void + { + $this->expectException('\InvalidArgumentException'); + $this->expectExceptionMessage('The pattern "/(/" could not be evaluated: Internal error.'); + + Assert::regex('abc', '/(/'); + } + + public function testRegexSuppressesTheWarningForAnUncompilablePattern(): void + { + $reported = []; + // PHPUnit masks E_WARNING out of error_reporting(), so restore it for this test. + $errorReporting = error_reporting(E_ALL); + set_error_handler(static function (int $errno, string $message) use (&$reported): bool { + // This is what a well behaved error handler does with a suppressed diagnostic. + if (0 !== (error_reporting() & $errno)) { + $reported[] = $message; + } + + return true; + }); + + try { + Assert::regex('abc', '/(/'); + } catch (InvalidArgumentException) { + // The assertion is expected to fail; this test only inspects the reported errors. + } finally { + restore_error_handler(); + error_reporting($errorReporting); + } + + $this->assertSame([], $reported); + } + + public function testRegexDoesNotUseTheCustomMessageForAnUncompilablePattern(): void + { + $this->expectException('\InvalidArgumentException'); + $this->expectExceptionMessage('The pattern "/(/" could not be evaluated: Internal error.'); + + Assert::regex('abc', '/(/', 'The value %s is not a valid slug.'); + } + + public function testRegexRejectsAPatternExceedingTheBacktrackLimit(): void + { + $backtrackLimit = ini_get('pcre.backtrack_limit'); + ini_set('pcre.backtrack_limit', '100'); + + try { + Assert::regex(str_repeat('a', 30).'c', '/^(a+)+$/'); + $this->fail('Expected an InvalidArgumentException to be thrown.'); + } catch (InvalidArgumentException $e) { + $this->assertSame( + 'The pattern "/^(a+)+$/" could not be evaluated: Backtrack limit exhausted.', + $e->getMessage() + ); + } finally { + ini_set('pcre.backtrack_limit', $backtrackLimit); + } + } + #[DataProvider('getMethodsThatUseOtherMethods')] public function testMessageIsPassedToInternalCalls(string $method, array $args, string $exceptionMessage): void {