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 {