fix(dispatcher): surface preg_match() engine failures instead of treating them as no-match
All dispatcher strategies checked preg_match() results with `!== 1`, which conflates a genuine no-match (0) with a PCRE engine failure (false), e.g. PREG_BACKTRACK_LIMIT_ERROR from a catastrophically backtracking route pattern. Because dynamic routes for the same method are combined into shared regex chunks, one such route could make the preg_match() call for the whole chunk fail, silently turning an unrelated, cheap route in that chunk into a spurious NOT_FOUND. Add a shared matchRoute() helper on RegexBasedAbstract that calls preg_match() and throws a RuntimeException (including preg_last_error_msg()) when it returns false, while preserving the existing 0/1 no-match/match behavior. Update GroupCountBased, GroupPosBased, CharCountBased and MarkBased to use it. Fixes #167
This commit is contained in:
1 parent
5861a25d71
commit
dea7d75d42
6 files changed
+73
-9
No files matched your search
@@ -7,7 +7,6 @@ use FastRoute\Dispatcher\Result\Matched;
|
|||||||
|
|
||||||
use function assert;
|
use function assert;
|
||||||
use function end;
|
use function end;
|
||||||
use function preg_match;
|
|
||||||
|
|
||||||
/** @final */
|
/** @final */
|
||||||
class CharCountBased extends RegexBasedAbstract
|
class CharCountBased extends RegexBasedAbstract
|
||||||
@@ -18,7 +17,7 @@ class CharCountBased extends RegexBasedAbstract
|
|||||||
foreach ($routeData as $data) {
|
foreach ($routeData as $data) {
|
||||||
assert(isset($data['suffix']));
|
assert(isset($data['suffix']));
|
||||||
|
|
||||||
if (preg_match($data['regex'], $uri . $data['suffix'], $matches) !== 1) {
|
if ($this->matchRoute($data['regex'], $uri . $data['suffix'], $matches) !== 1) {
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -6,7 +6,6 @@ namespace FastRoute\Dispatcher;
|
|||||||
use FastRoute\Dispatcher\Result\Matched;
|
use FastRoute\Dispatcher\Result\Matched;
|
||||||
|
|
||||||
use function count;
|
use function count;
|
||||||
use function preg_match;
|
|
||||||
|
|
||||||
/** @final */
|
/** @final */
|
||||||
class GroupCountBased extends RegexBasedAbstract
|
class GroupCountBased extends RegexBasedAbstract
|
||||||
@@ -15,7 +14,7 @@ class GroupCountBased extends RegexBasedAbstract
|
|||||||
protected function dispatchVariableRoute(array $routeData, string $uri): ?Matched
|
protected function dispatchVariableRoute(array $routeData, string $uri): ?Matched
|
||||||
{
|
{
|
||||||
foreach ($routeData as $data) {
|
foreach ($routeData as $data) {
|
||||||
if (preg_match($data['regex'], $uri, $matches) !== 1) {
|
if ($this->matchRoute($data['regex'], $uri, $matches) !== 1) {
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -6,7 +6,6 @@ namespace FastRoute\Dispatcher;
|
|||||||
use FastRoute\Dispatcher\Result\Matched;
|
use FastRoute\Dispatcher\Result\Matched;
|
||||||
|
|
||||||
use function assert;
|
use function assert;
|
||||||
use function preg_match;
|
|
||||||
|
|
||||||
/** @final */
|
/** @final */
|
||||||
class GroupPosBased extends RegexBasedAbstract
|
class GroupPosBased extends RegexBasedAbstract
|
||||||
@@ -15,7 +14,7 @@ class GroupPosBased extends RegexBasedAbstract
|
|||||||
protected function dispatchVariableRoute(array $routeData, string $uri): ?Matched
|
protected function dispatchVariableRoute(array $routeData, string $uri): ?Matched
|
||||||
{
|
{
|
||||||
foreach ($routeData as $data) {
|
foreach ($routeData as $data) {
|
||||||
if (preg_match($data['regex'], $uri, $matches) !== 1) {
|
if ($this->matchRoute($data['regex'], $uri, $matches) !== 1) {
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -5,8 +5,6 @@ namespace FastRoute\Dispatcher;
|
|||||||
|
|
||||||
use FastRoute\Dispatcher\Result\Matched;
|
use FastRoute\Dispatcher\Result\Matched;
|
||||||
|
|
||||||
use function preg_match;
|
|
||||||
|
|
||||||
/** @final */
|
/** @final */
|
||||||
class MarkBased extends RegexBasedAbstract
|
class MarkBased extends RegexBasedAbstract
|
||||||
{
|
{
|
||||||
@@ -14,7 +12,7 @@ class MarkBased extends RegexBasedAbstract
|
|||||||
protected function dispatchVariableRoute(array $routeData, string $uri): ?Matched
|
protected function dispatchVariableRoute(array $routeData, string $uri): ?Matched
|
||||||
{
|
{
|
||||||
foreach ($routeData as $data) {
|
foreach ($routeData as $data) {
|
||||||
if (preg_match($data['regex'], $uri, $matches) !== 1) {
|
if ($this->matchRoute($data['regex'], $uri, $matches) !== 1) {
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -8,6 +8,11 @@ use FastRoute\Dispatcher;
|
|||||||
use FastRoute\Dispatcher\Result\Matched;
|
use FastRoute\Dispatcher\Result\Matched;
|
||||||
use FastRoute\Dispatcher\Result\MethodNotAllowed;
|
use FastRoute\Dispatcher\Result\MethodNotAllowed;
|
||||||
use FastRoute\Dispatcher\Result\NotMatched;
|
use FastRoute\Dispatcher\Result\NotMatched;
|
||||||
|
use RuntimeException;
|
||||||
|
|
||||||
|
use function preg_last_error_msg;
|
||||||
|
use function preg_match;
|
||||||
|
use function sprintf;
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* @internal
|
* @internal
|
||||||
@@ -35,6 +40,33 @@ abstract class RegexBasedAbstract implements Dispatcher
|
|||||||
/** @param DynamicRouteChunks $routeData */
|
/** @param DynamicRouteChunks $routeData */
|
||||||
abstract protected function dispatchVariableRoute(array $routeData, string $uri): ?Matched;
|
abstract protected function dispatchVariableRoute(array $routeData, string $uri): ?Matched;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Matches a compiled route regex against the subject, distinguishing a
|
||||||
|
* genuine "no match" (preg_match() returning 0) from a PCRE engine
|
||||||
|
* failure (preg_match() returning false).
|
||||||
|
*
|
||||||
|
* A regex engine failure - for example PREG_BACKTRACK_LIMIT_ERROR caused
|
||||||
|
* by a catastrophically backtracking route pattern - must not be treated
|
||||||
|
* as "no match". Silently swallowing it can make an unrelated, cheap
|
||||||
|
* route that happens to share the same combined regex chunk spuriously
|
||||||
|
* fail to match.
|
||||||
|
*
|
||||||
|
* @see https://github.com/nikic/FastRoute/issues/167
|
||||||
|
*
|
||||||
|
* @param array<int|string, mixed>|null $matches
|
||||||
|
*/
|
||||||
|
protected function matchRoute(string $regex, string $subject, ?array &$matches = null): int
|
||||||
|
{
|
||||||
|
$result = preg_match($regex, $subject, $matches);
|
||||||
|
if ($result === false) {
|
||||||
|
throw new RuntimeException(
|
||||||
|
sprintf('Regex matching failed for "%s": %s', $regex, preg_last_error_msg()),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
return $result;
|
||||||
|
}
|
||||||
|
|
||||||
public function dispatch(string $httpMethod, string $uri): Matched|NotMatched|MethodNotAllowed
|
public function dispatch(string $httpMethod, string $uri): Matched|NotMatched|MethodNotAllowed
|
||||||
{
|
{
|
||||||
if (isset($this->staticRouteMap[$httpMethod][$uri])) {
|
if (isset($this->staticRouteMap[$httpMethod][$uri])) {
|
||||||
|
|||||||
@@ -13,8 +13,11 @@ use FastRoute\Dispatcher\Result\MethodNotAllowed;
|
|||||||
use FastRoute\Dispatcher\Result\NotMatched;
|
use FastRoute\Dispatcher\Result\NotMatched;
|
||||||
use PHPUnit\Framework\Attributes as PHPUnit;
|
use PHPUnit\Framework\Attributes as PHPUnit;
|
||||||
use PHPUnit\Framework\TestCase;
|
use PHPUnit\Framework\TestCase;
|
||||||
|
use RuntimeException;
|
||||||
|
|
||||||
use function FastRoute\simpleDispatcher;
|
use function FastRoute\simpleDispatcher;
|
||||||
|
use function ini_set;
|
||||||
|
use function str_repeat;
|
||||||
|
|
||||||
/** @phpstan-import-type ExtraParameters from DataGenerator */
|
/** @phpstan-import-type ExtraParameters from DataGenerator */
|
||||||
abstract class DispatcherTestCase extends TestCase
|
abstract class DispatcherTestCase extends TestCase
|
||||||
@@ -177,6 +180,40 @@ abstract class DispatcherTestCase extends TestCase
|
|||||||
}, $this->generateDispatcherOptions());
|
}, $this->generateDispatcherOptions());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* A PCRE engine failure (e.g. PREG_BACKTRACK_LIMIT_ERROR triggered by a
|
||||||
|
* catastrophically backtracking route pattern) must not be silently
|
||||||
|
* treated as "no match". Otherwise, an unrelated, cheap route that
|
||||||
|
* happens to share the same combined regex chunk would spuriously
|
||||||
|
* receive a NOT_FOUND result instead of surfacing the engine error.
|
||||||
|
*
|
||||||
|
* @see https://github.com/nikic/FastRoute/issues/167
|
||||||
|
*/
|
||||||
|
#[PHPUnit\Test]
|
||||||
|
public function regexEngineFailureIsNotSilentlyTreatedAsNotFound(): void
|
||||||
|
{
|
||||||
|
$previousLimit = ini_set('pcre.backtrack_limit', '1000');
|
||||||
|
self::assertNotFalse($previousLimit, 'Unable to lower pcre.backtrack_limit for this test');
|
||||||
|
|
||||||
|
try {
|
||||||
|
$this->expectException(RuntimeException::class);
|
||||||
|
$this->expectExceptionMessageMatches('/Backtrack limit exhausted/');
|
||||||
|
|
||||||
|
$dispatcher = simpleDispatcher(static function (ConfigureRoutes $r): void {
|
||||||
|
// Catastrophically backtracking pattern.
|
||||||
|
$r->addRoute('GET', '/{p:(?:a?a?)*}/complicated', 'complicated_pattern');
|
||||||
|
// Cheap, unrelated route combined into the same regex chunk
|
||||||
|
// as the pattern above. It would match the request URI on
|
||||||
|
// its own, but the shared chunk's preg_match() call fails.
|
||||||
|
$r->addRoute('GET', '/{p:a+}', 'cheap_route');
|
||||||
|
}, $this->generateDispatcherOptions());
|
||||||
|
|
||||||
|
$dispatcher->dispatch('GET', '/' . str_repeat('a', 30));
|
||||||
|
} finally {
|
||||||
|
ini_set('pcre.backtrack_limit', $previousLimit);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/** @return iterable<string, array{0: string, 1: string, 2: Closure(ConfigureRoutes):void, 3: string, 4?: array<string, string>}> */
|
/** @return iterable<string, array{0: string, 1: string, 2: Closure(ConfigureRoutes):void, 3: string, 4?: array<string, string>}> */
|
||||||
public static function provideFoundDispatchCases(): iterable
|
public static function provideFoundDispatchCases(): iterable
|
||||||
{
|
{
|
||||||
|
|||||||
Reference in new issue
Block a user