8000 [Routing] Disallow object usage inside Route by paxal · Pull Request #27727 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Routing] Disallow object usage inside Route #27727

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 28, 2018
Merged

[Routing] Disallow object usage inside Route #27727

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e16b302
[Routing] fix too much greediness in host-matching regex
nicolas-grekas Jun 27, 2018
8e0acfc
[HttpFoundation] missing namespace for RedisProxy
bonfante Jun 27, 2018
eb0a98f
bug #27754 [HttpFoundation] missing namespace for RedisProxy (Bonfante)
nicolas-grekas Jun 28, 2018
3f4644b
bug #27736 [Routing] fix too much greediness in host-matching regex (…
nicolas-grekas Jun 28, 2018
426fb45
[Routing] Disallow object usage inside Route
paxal Jun 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Symfony/Component/HttpFoundation/Session/Storage/Handler/RedisSessionHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
namespace Symfony\Component\HttpFoundation\Session\Storage\Handler;

use Predis\Response\ErrorInterface;
use Symfony\Component\Cache\Traits\RedisProxy;

/**
* Redis based session storage handler based on the Redis class
Expand Down
6 changes: 5 additions & 1 deletion src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
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 Expand Up @@ -746,6 +746,10 @@ public static function export($value): string
return 'null';
}
if (!\is_array($value)) {
if (\is_object($value)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (\is_object($value) && !method_exists($value, '__set_state')) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even objects with __set_state are broken, as the logic currently replaces new lines by a concatenation with "\n" instead (because when var_export-ing a scalar, new lines appear only inside strings).

If we want to keep support for objects with __set_state, it should be handled as a separate case (and I would put it before the if (!is_array()) case then, rather than nesting in it)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even objects with __set_state are broken

indeed, then let's keep it unsupported :)

Copy link
Contributor Author
@paxal paxal Jun 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of this PR is to disallow object usage, and avoid to export objects (via var_export). Thus, the fact that __set_state exists or not is not relevant. The dumped file does not work when we export an object (because of \n replacements).

Did not see the comments above :)

throw new \InvalidArgumentException('Symfony\Component\Routing\Route cannot contain objects.');
}

return str_replace("\n", '\'."\n".\'', var_export($value, true));
}
if (!$value) {
Expand Down
90 changes: 45 additions & 45 deletions src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php
View file
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
12 changes: 12 additions & 0 deletions src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,18 @@ private function generateDumpedMatcher(RouteCollection $collection, $redirectabl

return $this->matcherClass;
}

/**
* @expectedException \InvalidArgumentException
* @expectedExceptionMessage Symfony\Component\Routing\Route cannot contain objects
*/
public function testGenerateDumperMatcherWithObject()
{
$routeCollection = new RouteCollection();
$routeCollection->add('_', new Route('/', array(new \stdClass())));
$dumper = new PhpMatcherDumper($routeCollection);
$dumper->dump();
}
}

abstract class RedirectableUrlMatcherStub extends UrlMatcher implements RedirectableUrlMatcherInterface
Expand Down
10 changes: 10 additions & 0 deletions src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
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