Skip to content

Commit

Permalink
bug #27736 [Routing] fix too much greediness in host-matching regex (…
Browse files Browse the repository at this point in the history 
…nicolas-grekas)

This PR was merged into the 4.1 branch.

Discussion
----------

[Routing] fix too much greediness in host-matching regex

| Q             | A
| ------------- | ---
| Branch?       | 4.1
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #27721
| License       | MIT
| Doc PR        | -

Commits
-------

e16b302 [Routing] fix too much greediness in host-matching regex
  • Loading branch information
@nicolas-grekas
nicolas-grekas committed Jun 28, 2018
2 parents eb0a98f + e16b302 commit 3f4644b
Show file tree
Hide file tree
Showing 4 changed files with 101 additions and 91 deletions.
2 changes: 1 addition & 1 deletion src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php
View file
Expand Up @@ -379,7 +379,7 @@ private function compileDynamicRoutes(RouteCollection $collection, bool $matchHo
$hostRegex = '(?i:'.preg_replace_callback('#\?P<([^>]++)>#', $state->getVars, $rx[1]).')\.';
$state->hostVars = $state->vars;
} else {
$hostRegex = '(?:(?:[^.]*+\.)++)';
$hostRegex = '(?:(?:[^./]*+\.)++)';
$state->hostVars = array();
}
$state->mark += strlen($rx = ($prev ? ')' : '')."|{$hostRegex}(?");
Expand Down
90 changes: 45 additions & 45 deletions src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php
View file
Expand Up @@ -82,47 +82,47 @@ public function match($rawPathinfo)
$matchedPathinfo = $host.'.'.$pathinfo;
$regexList = array(
0 => '{^(?'
.'|(?:(?:[^.]*+\\.)++)(?'
.'|/foo/(baz|symfony)(*:46)'
.'|(?:(?:[^./]*+\\.)++)(?'
.'|/foo/(baz|symfony)(*:47)'
.'|/bar(?'
.'|/([^/]++)(*:69)'
.'|head/([^/]++)(*:89)'
.'|/([^/]++)(*:70)'
.'|head/([^/]++)(*:90)'
.')'
.'|/test/([^/]++)/(?'
.'|(*:115)'
.'|(*:116)'
.')'
.'|/([\']+)(*:131)'
.'|/([\']+)(*:132)'
.'|/a/(?'
.'|b\'b/([^/]++)(?'
.'|(*:160)'
.'|(*:168)'
.'|(*:161)'
.'|(*:169)'
.')'
.'|(.*)(*:181)'
.'|(.*)(*:182)'
.'|b\'b/([^/]++)(?'
.'|(*:204)'
.'|(*:212)'
.'|(*:205)'
.'|(*:213)'
.')'
.')'
.'|/multi/hello(?:/([^/]++))?(*:248)'
.'|/multi/hello(?:/([^/]++))?(*:249)'
.'|/([^/]++)/b/([^/]++)(?'
.'|(*:279)'
.'|(*:287)'
.'|(*:280)'
.'|(*:288)'
.')'
.'|/aba/([^/]++)(*:309)'
.'|/aba/([^/]++)(*:310)'
.')|(?i:([^\\.]++)\\.example\\.com)\\.(?'
.'|/route1(?'
.'|3/([^/]++)(*:371)'
.'|4/([^/]++)(*:389)'
.'|3/([^/]++)(*:372)'
.'|4/([^/]++)(*:390)'
.')'
.')|(?i:c\\.example\\.com)\\.(?'
.'|/route15/([^/]++)(*:441)'
.')|(?:(?:[^.]*+\\.)++)(?'
.'|/route16/([^/]++)(*:488)'
.'|/route15/([^/]++)(*:442)'
.')|(?:(?:[^./]*+\\.)++)(?'
.'|/route16/([^/]++)(*:490)'
.'|/a/(?'
.'|a\\.\\.\\.(*:509)'
.'|a\\.\\.\\.(*:511)'
.'|b/(?'
.'|([^/]++)(*:530)'
.'|c/([^/]++)(*:548)'
.'|([^/]++)(*:532)'
.'|c/([^/]++)(*:550)'
.')'
.')'
.')'
Expand All @@ -132,7 +132,7 @@ public function match($rawPathinfo)
foreach ($regexList as $offset => $regex) {
while (preg_match($regex, $matchedPathinfo, $matches)) {
switch ($m = (int) $matches['MARK']) {
case 115:
case 116:
$matches = array('foo' => $matches[1] ?? null);

// baz4
Expand All @@ -159,7 +159,7 @@ public function match($rawPathinfo)
not_bazbaz6:

break;
case 160:
case 161:
$matches = array('foo' => $matches[1] ?? null);

// foo1
Expand All @@ -173,14 +173,14 @@ public function match($rawPathinfo)
not_foo1:

break;
case 204:
case 205:
$matches = array('foo1' => $matches[1] ?? null);

// foo2
return $this->mergeDefaults(array('_route' => 'foo2') + $matches, array());

break;
case 279:
case 280:
$matches = array('_locale' => $matches[1] ?? null, 'foo' => $matches[2] ?? null);

// foo3
Expand All @@ -189,23 +189,23 @@ public function match($rawPathinfo)
break;
default:
$routes = array(
46 => array(array('_route' => 'foo', 'def' => 'test'), array('bar'), null, null),
69 => array(array('_route' => 'bar'), array('foo'), array('GET' => 0, 'HEAD' => 1), null),
89 => array(array('_route' => 'barhead'), array('foo'), array('GET' => 0), null),
131 => array(array('_route' => 'quoter'), array('quoter'), null, null),
168 => array(array('_route' => 'bar1'), array('bar'), null, null),
181 => array(array('_route' => 'overridden'), array('var'), null, null),
212 => array(array('_route' => 'bar2'), array('bar1'), null, null),
248 => array(array('_route' => 'helloWorld', 'who' => 'World!'), array('who'), null, null),
287 => array(array('_route' => 'bar3'), array('_locale', 'bar'), null, null),
309 => array(array('_route' => 'foo4'), array('foo'), null, null),
371 => array(array('_route' => 'route13'), array('var1', 'name'), null, null),
389 => array(array('_route' => 'route14', 'var1' => 'val'), array('var1', 'name'), null, null),
441 => array(array('_route' => 'route15'), array('name'), null, null),
488 => array(array('_route' => 'route16', 'var1' => 'val'), array('name'), null, null),
509 => array(array('_route' => 'a'), array(), null, null),
530 => array(array('_route' => 'b'), array('var'), null, null),
548 => array(array('_route' => 'c'), array('var'), null, null),
47 => array(array('_route' => 'foo', 'def' => 'test'), array('bar'), null, null),
70 => array(array('_route' => 'bar'), array('foo'), array('GET' => 0, 'HEAD' => 1), null),
90 => array(array('_route' => 'barhead'), array('foo'), array('GET' => 0), null),
132 => array(array('_route' => 'quoter'), array('quoter'), null, null),
169 => array(array('_route' => 'bar1'), array('bar'), null, null),
182 => array(array('_route' => 'overridden'), array('var'), null, null),
213 => array(array('_route' => 'bar2'), array('bar1'), null, null),
249 => array(array('_route' => 'helloWorld', 'who' => 'World!'), array('who'), null, null),
288 => array(array('_route' => 'bar3'), array('_locale', 'bar'), null, null),
310 => array(array('_route' => 'foo4'), array('foo'), null, null),
372 => array(array('_route' => 'route13'), array('var1', 'name'), null, null),
390 => array(array('_route' => 'route14', 'var1' => 'val'), array('var1', 'name'), null, null),
442 => array(array('_route' => 'route15'), array('name'), null, null),
490 => array(array('_route' => 'route16', 'var1' => 'val'), array('name'), null, null),
511 => array(array('_route' => 'a'), array(), null, null),
532 => array(array('_route' => 'b'), array('var'), null, null),
550 => array(array('_route' => 'c'), array('var'), null, null),
);

list($ret, $vars, $requiredMethods, $requiredSchemes) = $routes[$m];
Expand All @@ -231,7 +231,7 @@ public function match($rawPathinfo)
return $ret;
}

if (548 === $m) {
if (550 === $m) {
break;
}
$regex = substr_replace($regex, 'F', $m - $offset, 1 + strlen($m));
Expand Down
90 changes: 45 additions & 45 deletions src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php
View file
Expand Up @@ -119,47 +119,47 @@ private function doMatch(string $rawPathinfo, array &$allow = array(), array &$a
$matchedPathinfo = $host.'.'.$pathinfo;
$regexList = array(
0 => '{^(?'
.'|(?:(?:[^.]*+\\.)++)(?'
.'|/foo/(baz|symfony)(*:46)'
.'|(?:(?:[^./]*+\\.)++)(?'
.'|/foo/(baz|symfony)(*:47)'
.'|/bar(?'
.'|/([^/]++)(*:69)'
.'|head/([^/]++)(*:89)'
.'|/([^/]++)(*:70)'
.'|head/([^/]++)(*:90)'
.')'
.'|/test/([^/]++)/(?'
.'|(*:115)'
.'|(*:116)'
.')'
.'|/([\']+)(*:131)'
.'|/([\']+)(*:132)'
.'|/a/(?'
.'|b\'b/([^/]++)(?'
.'|(*:160)'
.'|(*:168)'
.'|(*:161)'
.'|(*:169)'
.')'
.'|(.*)(*:181)'
.'|(.*)(*:182)'
.'|b\'b/([^/]++)(?'
.'|(*:204)'
.'|(*:212)'
.'|(*:205)'
.'|(*:213)'
.')'
.')'
.'|/multi/hello(?:/([^/]++))?(*:248)'
.'|/multi/hello(?:/([^/]++))?(*:249)'
.'|/([^/]++)/b/([^/]++)(?'
.'|(*:279)'
.'|(*:287)'
.'|(*:280)'
.'|(*:288)'
.')'
.'|/aba/([^/]++)(*:309)'
.'|/aba/([^/]++)(*:310)'
.')|(?i:([^\\.]++)\\.example\\.com)\\.(?'
.'|/route1(?'
.'|3/([^/]++)(*:371)'
.'|4/([^/]++)(*:389)'
.'|3/([^/]++)(*:372)'
.'|4/([^/]++)(*:390)'
.')'
.')|(?i:c\\.example\\.com)\\.(?'
.'|/route15/([^/]++)(*:441)'
.')|(?:(?:[^.]*+\\.)++)(?'
.'|/route16/([^/]++)(*:488)'
.'|/route15/([^/]++)(*:442)'
.')|(?:(?:[^./]*+\\.)++)(?'
.'|/route16/([^/]++)(*:490)'
.'|/a/(?'
.'|a\\.\\.\\.(*:509)'
.'|a\\.\\.\\.(*:511)'
.'|b/(?'
.'|([^/]++)(*:530)'
.'|c/([^/]++)(*:548)'
.'|([^/]++)(*:532)'
.'|c/([^/]++)(*:550)'
.')'
.')'
.')'
Expand All @@ -169,7 +169,7 @@ private function doMatch(string $rawPathinfo, array &$allow = array(), array &$a
foreach ($regexList as $offset => $regex) {
while (preg_match($regex, $matchedPathinfo, $matches)) {
switch ($m = (int) $matches['MARK']) {
case 115:
case 116:
$matches = array('foo' => $matches[1] ?? null);

// baz4
Expand All @@ -196,7 +196,7 @@ private function doMatch(string $rawPathinfo, array &$allow = array(), array &$a
not_bazbaz6:

break;
case 160:
case 161:
$matches = array('foo' => $matches[1] ?? null);

// foo1
Expand All @@ -210,14 +210,14 @@ private function doMatch(string $rawPathinfo, array &$allow = array(), array &$a
not_foo1:

break;
case 204:
case 205:
$matches = array('foo1' => $matches[1] ?? null);

// foo2
return $this->mergeDefaults(array('_route' => 'foo2') + $matches, array());

break;
case 279:
case 280:
$matches = array('_locale' => $matches[1] ?? null, 'foo' => $matches[2] ?? null);

// foo3
Expand All @@ -226,23 +226,23 @@ private function doMatch(string $rawPathinfo, array &$allow = array(), array &$a
break;
default:
$routes = array(
46 => array(array('_route' => 'foo', 'def' => 'test'), array('bar'), null, null),
69 => array(array('_route' => 'bar'), array('foo'), array('GET' => 0, 'HEAD' => 1), null),
89 => array(array('_route' => 'barhead'), array('foo'), array('GET' => 0), null),
131 => array(array('_route' => 'quoter'), array('quoter'), null, null),
168 => array(array('_route' => 'bar1'), array('bar'), null, null),
181 => array(array('_route' => 'overridden'), array('var'), null, null),
212 => array(array('_route' => 'bar2'), array('bar1'), null, null),
248 => array(array('_route' => 'helloWorld', 'who' => 'World!'), array('who'), null, null),
287 => array(array('_route' => 'bar3'), array('_locale', 'bar'), null, null),
309 => array(array('_route' => 'foo4'), array('foo'), null, null),
371 => array(array('_route' => 'route13'), array('var1', 'name'), null, null),
389 => array(array('_route' => 'route14', 'var1' => 'val'), array('var1', 'name'), null, null),
441 => array(array('_route' => 'route15'), array('name'), null, null),
488 => array(array('_route' => 'route16', 'var1' => 'val'), array('name'), null, null),
509 => array(array('_route' => 'a'), array(), null, null),
530 => array(array('_route' => 'b'), array('var'), null, null),
548 => array(array('_route' => 'c'), array('var'), null, null),
47 => array(array('_route' => 'foo', 'def' => 'test'), array('bar'), null, null),
70 => array(array('_route' => 'bar'), array('foo'), array('GET' => 0, 'HEAD' => 1), null),
90 => array(array('_route' => 'barhead'), array('foo'), array('GET' => 0), null),
132 => array(array('_route' => 'quoter'), array('quoter'), null, null),
169 => array(array('_route' => 'bar1'), array('bar'), null, null),
182 => array(array('_route' => 'overridden'), array('var'), null, null),
213 => array(array('_route' => 'bar2'), array('bar1'), null, null),
249 => array(array('_route' => 'helloWorld', 'who' => 'World!'), array('who'), null, null),
288 => array(array('_route' => 'bar3'), array('_locale', 'bar'), null, null),
310 => array(array('_route' => 'foo4'), array('foo'), null, null),
372 => array(array('_route' => 'route13'), array('var1', 'name'), null, null),
390 => array(array('_route' => 'route14', 'var1' => 'val'), array('var1', 'name'), null, null),
442 => array(array('_route' => 'route15'), array('name'), null, null),
490 => array(array('_route' => 'route16', 'var1' => 'val'), array('name'), null, null),
511 => array(array('_route' => 'a'), array(), null, null),
532 => array(array('_route' => 'b'), array('var'), null, null),
550 => array(array('_route' => 'c'), array('var'), null, null),
);

list($ret, $vars, $requiredMethods, $requiredSchemes) = $routes[$m];
Expand All @@ -268,7 +268,7 @@ private function doMatch(string $rawPathinfo, array &$allow = array(), array &$a
return $ret;
}

if (548 === $m) {
if (550 === $m) {
break;
}
$regex = substr_replace($regex, 'F', $m - $offset, 1 + strlen($m));
Expand Down
10 changes: 10 additions & 0 deletions src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php
View file
Expand Up @@ -670,6 +670,16 @@ public function testUtf8AndMethodMatching()
$this->assertEquals('c', $matcher->match('/admin/api/package.json')['_route']);
}

public function testHostWithDot()
{
$coll = new RouteCollection();
$coll->add('a', new Route('/foo', array(), array(), array(), 'foo.example.com'));
$coll->add('b', new Route('/bar/{baz}'));

$matcher = $this->getUrlMatcher($coll);
$this->assertEquals('b', $matcher->match('/bar/abc.123')['_route']);
}

protected function getUrlMatcher(RouteCollection $routes, RequestContext $context = null)
{
return new UrlMatcher($routes, $context ?: new RequestContext());
Expand Down

0 comments on commit 3f4644b

Please sign in to comment.