Skip to content

Commit

Permalink
[DependencyInjection] Validate class names and factory methods
Browse files Browse the repository at this point in the history
  • Loading branch information
nicolas-grekas committed Dec 3, 2015
1 parent 13fda1c commit b2e3850
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 6 deletions.
25 changes: 19 additions & 6 deletions src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
Expand Up @@ -759,6 +759,10 @@ private function addNewInstance($id, Definition $definition, $return, $instantia
if (null !== $definition->getFactory()) {
$callable = $definition->getFactory();
if (is_array($callable)) {
if (!preg_match('/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$/', $callable[1])) {
throw new RuntimeException(sprintf('Cannot dump definition because of invalid factory method (%s)', $callable[1] ?: 'n/a'));
}

if ($callable[0] instanceof Reference
|| ($callable[0] instanceof Definition && $this->definitionVariables->contains($callable[0]))) {
return sprintf(" $return{$instantiation}%s->%s(%s);\n", $this->dumpValue($callable[0]), $callable[1], $arguments ? implode(', ', $arguments) : '');
Expand Down Expand Up @@ -1310,8 +1314,12 @@ private function dumpValue($value, $interpolate = true)
}

if (is_array($factory)) {
if (!preg_match('/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$/', $factory[1])) {
throw new RuntimeException(sprintf('Cannot dump definition because of invalid factory method (%s)', $factory[1] ?: 'n/a'));
}

if (is_string($factory[0])) {
return sprintf('\\%s::%s(%s)', $factory[0], $factory[1], implode(', ', $arguments));
return sprintf('%s::%s(%s)', $this->dumpLiteralClass($this->dumpValue($factory[0])), $factory[1], implode(', ', $arguments));
}

if ($factory[0] instanceof Definition) {
Expand Down Expand Up @@ -1342,12 +1350,8 @@ private function dumpValue($value, $interpolate = true)
if (null === $class) {
throw new RuntimeException('Cannot dump definitions which have no class nor factory.');
}
$class = $this->dumpValue($class);
if (false !== strpos($class, '$')) {
throw new RuntimeException('Cannot dump definitions which have a variable class name.');
}

return sprintf('new \\%s(%s)', substr(str_replace('\\\\', '\\', $class), 1, -1), implode(', ', $arguments));
return sprintf('new %s(%s)', $this->dumpLiteralClass($this->dumpValue($class)), implode(', ', $arguments));
} elseif ($value instanceof Variable) {
return '$'.$value;
} elseif ($value instanceof Reference) {
Expand Down Expand Up @@ -1388,9 +1392,18 @@ private function dumpValue($value, $interpolate = true)
* @param string $class
*
* @return string
*
* @throws RuntimeException
*/
private function dumpLiteralClass($class)
{
if (false !== strpos($class, '$')) {
throw new RuntimeException('Cannot dump definitions which have a variable class name.');
}
if (0 !== strpos($class, "'") || !preg_match('/^\'[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*(\\\{2}[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*)*\'$/', $class)) {

This comment has been minimized.

Copy link
@jmcclell

jmcclell Dec 23, 2015

This is failing for valid class names. A simple project that uses JMS Serializer, for example, will complaining that 'Metadata\MetadataFactory' is an invalid class name.

This comment has been minimized.

Copy link
@stof

stof Dec 23, 2015

Member

this looks weird to me, as the (exported) class name matches the regex and so will be valid. Please open an issue with a complete description to reproduce it.

A comment on an old commit is one of the worse way to report an issue, as anyone not reading all their github email notifications (or having disabled them) will never see it, and other people will forget about the issue report as soon as they close the page where the comment was posted

This comment has been minimized.

Copy link
@jmcclell

jmcclell Dec 23, 2015

Working on a reproducible test case for you guys to open an issue, left the comment more for myself to recall the problem area :)

throw new RuntimeException(sprintf('Cannot dump definition because of invalid class name (%s)', $class ?: 'n/a'));
}

return '\\'.substr(str_replace('\\\\', '\\', $class), 1, -1);
}

Expand Down
Expand Up @@ -154,6 +154,31 @@ public function testAddServiceInvalidServiceId()
$dumper->dump();
}

/**
* @dataProvider provideInvalidFactories
* @expectedException Symfony\Component\DependencyInjection\Exception\RuntimeException
* @expectedExceptionMessage Cannot dump definition
*/
public function testInvalidFactories($factory)
{
$container = new ContainerBuilder();
$def = new Definition('stdClass');
$def->setFactory($factory);
$container->setDefinition('bar', $def);
$dumper = new PhpDumper($container);
$dumper->dump();
}

public function provideInvalidFactories()
{
return array(
array(array('', 'method')),
array(array('class', '')),
array(array('...', 'method')),
array(array('class', '...')),
);
}

public function testAliases()
{
$container = include self::$fixturesPath.'/containers/container9.php';
Expand Down

0 comments on commit b2e3850

Please sign in to comment.