Merge branch '4.2'

* 4.2: [DI] Check service IDs are valid
symfony · Apr 16, 2019 · 899985e · 899985e
2 parents 96aee57 + 3fd01ab
commit 899985e
Show file tree

Hide file tree

Showing 12 changed files with 239 additions and 39 deletions.
diff --git a/src/Symfony/Bridge/ProxyManager/LazyProxy/PhpDumper/ProxyDumper.php b/src/Symfony/Bridge/ProxyManager/LazyProxy/PhpDumper/ProxyDumper.php
@@ -53,7 +53,7 @@ public function getProxyFactoryCode(Definition $definition, $id, $factoryCode =
         $instantiation = 'return';
 
         if ($definition->isShared()) {
-            $instantiation .= sprintf(' $this->%s[\'%s\'] =', $definition->isPublic() && !$definition->isPrivate() ? 'services' : 'privates', $id);
+            $instantiation .= sprintf(' $this->%s[%s] =', $definition->isPublic() && !$definition->isPrivate() ? 'services' : 'privates', var_export($id, true));
         }
 
         if (null === $factoryCode) {

diff --git a/src/Symfony/Component/DependencyInjection/ContainerBuilder.php b/src/Symfony/Component/DependencyInjection/ContainerBuilder.php
@@ -834,6 +834,10 @@ public function setAlias($alias, $id)
     {
         $alias = (string) $alias;
 
+        if ('' === $alias || '\\' === $alias[-1] || \strlen($alias) !== strcspn($alias, "\0\r\n'")) {
+            throw new InvalidArgumentException(sprintf('Invalid alias id: "%s"', $alias));
+        }
+
         if (\is_string($id)) {
             $id = new Alias($id);
         } elseif (!$id instanceof Alias) {
@@ -987,6 +991,10 @@ public function setDefinition($id, Definition $definition)
 
         $id = (string) $id;
 
+        if ('' === $id || '\\' === $id[-1] || \strlen($id) !== strcspn($id, "\0\r\n'")) {
+            throw new InvalidArgumentException(sprintf('Invalid service id: "%s"', $id));
+        }
+
         unset($this->aliasDefinitions[$id], $this->removedIds[$id]);
 
         return $this->definitions[$id] = $definition;

diff --git a/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php b/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
@@ -514,7 +514,7 @@ private function addServiceInstance(string $id, Definition $definition, bool $is
         }
 
         if (!$isProxyCandidate && $definition->isShared() && !isset($this->singleUsePrivateIds[$id]) && null === $lastWitherIndex) {
-            $instantiation = sprintf('$this->%s[\'%s\'] = %s', $this->container->getDefinition($id)->isPublic() ? 'services' : 'privates', $id, $isSimpleInstance ? '' : '$instance');
+            $instantiation = sprintf('$this->%s[%s] = %s', $this->container->getDefinition($id)->isPublic() ? 'services' : 'privates', $this->doExport($id), $isSimpleInstance ? '' : '$instance');
         } elseif (!$isSimpleInstance) {
             $instantiation = '$instance';
         }
@@ -698,6 +698,9 @@ private function addService(string $id, Definition $definition): array
      * Gets the $public '$id'$shared$autowired service.
      *
      * $return
+EOF;
+            $code = str_replace('*/', ' ', $code).<<<EOF
+
      */
     protected function {$methodName}($lazyInitialization)
     {
@@ -711,8 +714,8 @@ protected function {$methodName}($lazyInitialization)
         $code .= $this->addServiceInclude($id, $definition);
 
         if ($this->getProxyDumper()->isProxyCandidate($definition)) {
-            $factoryCode = $asFile ? ($definition->isShared() ? "\$this->load('%s.php', false)" : "\$this->factories['%2$s'](false)") : '$this->%s(false)';
-            $code .= $this->getProxyDumper()->getProxyFactoryCode($definition, $id, sprintf($factoryCode, $methodName, $this->export($id)));
+            $factoryCode = $asFile ? ($definition->isShared() ? "\$this->load('%s.php', false)" : "\$this->factories[%2\$s](false)") : '$this->%s(false)';
+            $code .= $this->getProxyDumper()->getProxyFactoryCode($definition, $id, sprintf($factoryCode, $methodName, $this->doExport($id)));
         }
 
         if ($definition->isDeprecated()) {
@@ -786,14 +789,14 @@ private function addInlineReference(string $id, Definition $definition, string $
 
         $code .= sprintf(<<<'EOTXT'
 
-        if (isset($this->%s['%s'])) {
-            return $this->%1$s['%2$s'];
+        if (isset($this->%s[%s])) {
+            return $this->%1$s[%2$s];
         }
 
 EOTXT
             ,
             $this->container->getDefinition($id)->isPublic() ? 'services' : 'privates',
-            $id
+            $this->doExport($id)
         );
 
         return $code;
@@ -885,7 +888,7 @@ private function generateServiceFiles(array $services)
                         $code = ["\n", $code];
                     }
                     $code[1] = implode("\n", array_map(function ($line) { return $line ? '    '.$line : $line; }, explode("\n", $code[1])));
-                    $factory = sprintf('$this->factories%s[\'%s\']', $definition->isPublic() ? '' : "['service_container']", $id);
+                    $factory = sprintf('$this->factories%s[%s]', $definition->isPublic() ? '' : "['service_container']", $this->doExport($id));
                     $lazyloadInitialization = $definition->isLazy() ? '$lazyLoad = true' : '';
 
                     $code[1] = sprintf("%s = function (%s) {\n%s};\n\nreturn %1\$s();\n", $factory, $lazyloadInitialization, $code[1]);
@@ -1453,14 +1456,14 @@ private function getServiceConditionals($value): string
             if (!$this->container->hasDefinition($service)) {
                 return 'false';
             }
-            $conditions[] = sprintf("isset(\$this->%s['%s'])", $this->container->getDefinition($service)->isPublic() ? 'services' : 'privates', $service);
+            $conditions[] = sprintf("isset(\$this->%s[%s])", $this->container->getDefinition($service)->isPublic() ? 'services' : 'privates', $this->doExport($service));
         }
         foreach (ContainerBuilder::getServiceConditionals($value) as $service) {
             if ($this->container->hasDefinition($service) && !$this->container->getDefinition($service)->isPublic()) {
                 continue;
             }
 
-            $conditions[] = sprintf("\$this->has('%s')", $service);
+            $conditions[] = sprintf("\$this->has(%s)", $this->doExport($service));
         }
 
         if (!$conditions) {
@@ -1579,7 +1582,7 @@ private function dumpValue($value, bool $interpolate = true): string
                         $serviceMap .= sprintf("\n            %s => [%s, %s, %s, %s],",
                             $this->export($k),
                             $this->export($definition->isShared() ? ($definition->isPublic() ? 'services' : 'privates') : false),
-                            $this->export($id),
+                            $this->doExport($id),
                             $this->export(ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE !== $v->getInvalidBehavior() && !\is_string($load) ? $this->generateMethodName($id).($load ? '.php' : '') : null),
                             $this->export($load)
                         );
@@ -1677,11 +1680,11 @@ private function dumpParameter(string $name): string
             }
 
             if (!preg_match("/\\\$this->(?:getEnv\('(?:\w++:)*+\w++'\)|targetDirs\[\d++\])/", $dumpedValue)) {
-                return sprintf("\$this->parameters['%s']", $name);
+                return sprintf('$this->parameters[%s]', $this->doExport($name));
             }
         }
 
-        return sprintf("\$this->getParameter('%s')", $name);
+        return sprintf('$this->getParameter(%s)', $this->doExport($name));
     }
 
     private function getServiceCall(string $id, Reference $reference = null): string
@@ -1696,7 +1699,7 @@ private function getServiceCall(string $id, Reference $reference = null): string
 
         if ($this->container->hasDefinition($id) && $definition = $this->container->getDefinition($id)) {
             if ($definition->isSynthetic()) {
-                $code = sprintf('$this->get(\'%s\'%s)', $id, null !== $reference ? ', '.$reference->getInvalidBehavior() : '');
+                $code = sprintf('$this->get(%s%s)', $this->doExport($id), null !== $reference ? ', '.$reference->getInvalidBehavior() : '');
             } elseif (null !== $reference && ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE === $reference->getInvalidBehavior()) {
                 $code = 'null';
                 if (!$definition->isShared()) {
@@ -1710,20 +1713,20 @@ private function getServiceCall(string $id, Reference $reference = null): string
                 }
                 $code = $this->addNewInstance($definition, '', $id);
                 if ($definition->isShared() && !isset($this->singleUsePrivateIds[$id])) {
-                    $code = sprintf('$this->%s[\'%s\'] = %s', $definition->isPublic() ? 'services' : 'privates', $id, $code);
+                    $code = sprintf('$this->%s[%s] = %s', $definition->isPublic() ? 'services' : 'privates', $this->doExport($id), $code);
                 }
                 $code = "($code)";
             } elseif ($this->asFiles && !$this->isHotPath($definition)) {
                 $code = sprintf("\$this->load('%s.php')", $this->generateMethodName($id));
                 if (!$definition->isShared()) {
-                    $factory = sprintf('$this->factories%s[\'%s\']', $definition->isPublic() ? '' : "['service_container']", $id);
+                    $factory = sprintf('$this->factories%s[%s]', $definition->isPublic() ? '' : "['service_container']", $this->doExport($id));
                     $code = sprintf('(isset(%s) ? %1$s() : %s)', $factory, $code);
                 }
             } else {
                 $code = sprintf('$this->%s()', $this->generateMethodName($id));
             }
             if ($definition->isShared() && !isset($this->singleUsePrivateIds[$id])) {
-                $code = sprintf('($this->%s[\'%s\'] ?? %s)', $definition->isPublic() ? 'services' : 'privates', $id, $code);
+                $code = sprintf('($this->%s[%s] ?? %s)', $definition->isPublic() ? 'services' : 'privates', $this->doExport($id), $code);
             }
 
             return $code;
@@ -1732,12 +1735,12 @@ private function getServiceCall(string $id, Reference $reference = null): string
             return 'null';
         }
         if (null !== $reference && ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE < $reference->getInvalidBehavior()) {
-            $code = sprintf('$this->get(\'%s\', /* ContainerInterface::NULL_ON_INVALID_REFERENCE */ %d)', $id, ContainerInterface::NULL_ON_INVALID_REFERENCE);
+            $code = sprintf('$this->get(%s, /* ContainerInterface::NULL_ON_INVALID_REFERENCE */ %d)', $this->doExport($id), ContainerInterface::NULL_ON_INVALID_REFERENCE);
         } else {
-            $code = sprintf('$this->get(\'%s\')', $id);
+            $code = sprintf('$this->get(%s)', $this->doExport($id));
         }
 
-        return sprintf('($this->services[\'%s\'] ?? %s)', $id, $code);
+        return sprintf('($this->services[%s] ?? %s)', $this->doExport($id), $code);
     }
 
     /**

diff --git a/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php b/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php
@@ -198,6 +198,38 @@ public function testNonSharedServicesReturnsDifferentInstances()
         $this->assertNotSame($builder->get('bar'), $builder->get('bar'));
     }
 
+    /**
+     * @expectedException \Symfony\Component\DependencyInjection\Exception\InvalidArgumentException
+     * @dataProvider provideBadId
+     */
+    public function testBadAliasId($id)
+    {
+        $builder = new ContainerBuilder();
+        $builder->setAlias($id, 'foo');
+    }
+
+    /**
+     * @expectedException \Symfony\Component\DependencyInjection\Exception\InvalidArgumentException
+     * @dataProvider provideBadId
+     */
+    public function testBadDefinitionId($id)
+    {
+        $builder = new ContainerBuilder();
+        $builder->setDefinition($id, new Definition('Foo'));
+    }
+
+    public function provideBadId()
+    {
+        return [
+            [''],
+            ["\0"],
+            ["\r"],
+            ["\n"],
+            ["'"],
+            ['ab\\'],
+        ];
+    }
+
     /**
      * @expectedException        \Symfony\Component\DependencyInjection\Exception\RuntimeException
      * @expectedExceptionMessage You have requested a synthetic service ("foo"). The DIC does not know how to construct this service.

diff --git a/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php b/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php
@@ -264,12 +264,18 @@ public function testAddServiceIdWithUnsupportedCharacters()
     {
         $class = 'Symfony_DI_PhpDumper_Test_Unsupported_Characters';
         $container = new ContainerBuilder();
+        $container->setParameter("'", 'oh-no');
+        $container->register("foo*/oh-no", 'FooClass')->setPublic(true);
         $container->register('bar$', 'FooClass')->setPublic(true);
         $container->register('bar$!', 'FooClass')->setPublic(true);
         $container->compile();
         $dumper = new PhpDumper($container);
-        eval('?>'.$dumper->dump(['class' => $class]));
 
+        $this->assertStringEqualsFile(self::$fixturesPath.'/php/services_unsupported_characters.php', $dumper->dump(['class' => $class]));
+
+        require_once self::$fixturesPath.'/php/services_unsupported_characters.php';
+
+        $this->assertTrue(method_exists($class, 'getFooOhNoService'));
         $this->assertTrue(method_exists($class, 'getBarService'));
         $this->assertTrue(method_exists($class, 'getBar2Service'));
     }

diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services33.php b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services33.php
@@ -55,7 +55,7 @@ public function getRemovedIds()
      */
     protected function getFooService()
     {
-        return $this->services['Bar\Foo'] = new \Bar\Foo();
+        return $this->services['Bar\\Foo'] = new \Bar\Foo();
     }
 
     /**
@@ -65,6 +65,6 @@ protected function getFooService()
      */
     protected function getFoo2Service()
     {
-        return $this->services['Foo\Foo'] = new \Foo\Foo();
+        return $this->services['Foo\\Foo'] = new \Foo\Foo();
     }
 }
diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_adawson.php b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_adawson.php
@@ -60,11 +60,11 @@ public function getRemovedIds()
      */
     protected function getBusService()
     {
-        $a = ($this->services['App\Db'] ?? $this->getDbService());
+        $a = ($this->services['App\\Db'] ?? $this->getDbService());
 
-        $this->services['App\Bus'] = $instance = new \App\Bus($a);
+        $this->services['App\\Bus'] = $instance = new \App\Bus($a);
 
-        $b = ($this->privates['App\Schema'] ?? $this->getSchemaService());
+        $b = ($this->privates['App\\Schema'] ?? $this->getSchemaService());
         $c = new \App\Registry();
         $c->processor = [0 => $a, 1 => $instance];
 
@@ -83,9 +83,9 @@ protected function getBusService()
      */
     protected function getDbService()
     {
-        $this->services['App\Db'] = $instance = new \App\Db();
+        $this->services['App\\Db'] = $instance = new \App\Db();
 
-        $instance->schema = ($this->privates['App\Schema'] ?? $this->getSchemaService());
+        $instance->schema = ($this->privates['App\\Schema'] ?? $this->getSchemaService());
 
         return $instance;
     }
@@ -97,12 +97,12 @@ protected function getDbService()
      */
     protected function getSchemaService()
     {
-        $a = ($this->services['App\Db'] ?? $this->getDbService());
+        $a = ($this->services['App\\Db'] ?? $this->getDbService());
 
-        if (isset($this->privates['App\Schema'])) {
-            return $this->privates['App\Schema'];
+        if (isset($this->privates['App\\Schema'])) {
+            return $this->privates['App\\Schema'];
         }
 
-        return $this->privates['App\Schema'] = new \App\Schema($a);
+        return $this->privates['App\\Schema'] = new \App\Schema($a);
     }
 }
diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_inline_requires.php b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_inline_requires.php
@@ -70,7 +70,7 @@ public function getRemovedIds()
      */
     protected function getParentNotExistsService()
     {
-        return $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\ParentNotExists'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\ParentNotExists();
+        return $this->services['Symfony\\Component\\DependencyInjection\\Tests\\Fixtures\\ParentNotExists'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\ParentNotExists();
     }
 
     /**
@@ -80,7 +80,7 @@ protected function getParentNotExistsService()
      */
     protected function getC1Service()
     {
-        return $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C1'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C1();
+        return $this->services['Symfony\\Component\\DependencyInjection\\Tests\\Fixtures\\includes\\HotPath\\C1'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C1();
     }
 
     /**
@@ -93,7 +93,7 @@ protected function getC2Service()
         include_once $this->targetDirs[1].'/includes/HotPath/C2.php';
         include_once $this->targetDirs[1].'/includes/HotPath/C3.php';
 
-        return $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C2'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C2(new \Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C3());
+        return $this->services['Symfony\\Component\\DependencyInjection\\Tests\\Fixtures\\includes\\HotPath\\C2'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C2(new \Symfony\Component\DependencyInjection\Tests\Fixtures\includes\HotPath\C3());
     }
 
     public function getParameter($name)

diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_inline_self_ref.php b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_inline_self_ref.php
@@ -59,7 +59,7 @@ protected function getFooService()
         $b = new \App\Baz($a);
         $b->bar = $a;
 
-        $this->services['App\Foo'] = $instance = new \App\Foo($b);
+        $this->services['App\\Foo'] = $instance = new \App\Foo($b);
 
         $a->foo = $instance;
 

diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_rot13_env.php b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_rot13_env.php
@@ -60,7 +60,7 @@ public function getRemovedIds()
      */
     protected function getRot13EnvVarProcessorService()
     {
-        return $this->services['Symfony\Component\DependencyInjection\Tests\Dumper\Rot13EnvVarProcessor'] = new \Symfony\Component\DependencyInjection\Tests\Dumper\Rot13EnvVarProcessor();
+        return $this->services['Symfony\\Component\\DependencyInjection\\Tests\\Dumper\\Rot13EnvVarProcessor'] = new \Symfony\Component\DependencyInjection\Tests\Dumper\Rot13EnvVarProcessor();
     }
 
     /**

diff --git a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_subscriber.php b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_subscriber.php
@@ -60,7 +60,7 @@ public function getRemovedIds()
      */
     protected function getTestServiceSubscriberService()
     {
-        return $this->services['Symfony\Component\DependencyInjection\Tests\Fixtures\TestServiceSubscriber'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\TestServiceSubscriber();
+        return $this->services['Symfony\\Component\\DependencyInjection\\Tests\\Fixtures\\TestServiceSubscriber'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\TestServiceSubscriber();
     }
 
     /**
@@ -90,6 +90,6 @@ protected function getFooServiceService()
      */
     protected function getCustomDefinitionService()
     {
-        return $this->privates['Symfony\Component\DependencyInjection\Tests\Fixtures\CustomDefinition'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\CustomDefinition();
+        return $this->privates['Symfony\\Component\\DependencyInjection\\Tests\\Fixtures\\CustomDefinition'] = new \Symfony\Component\DependencyInjection\Tests\Fixtures\CustomDefinition();
     }
 }