Skip to content

Commit d2fb589

Browse files
nicolas-grekasnicolas-grekas
committed
[DI] Check service IDs are valid
1 parent 4585a41 commit d2fb589

12 files changed

+283
-54
lines changed

‎src/Symfony/Bridge/ProxyManager/LazyProxy/PhpDumper/ProxyDumper.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ public function getProxyFactoryCode(Definition $definition, $id, $factoryCode =
5858
$instantiation = 'return';
5959

6060
if ($definition->isShared()) {
61-
$instantiation .= sprintf(' $this->%s[\'%s\'] =', \method_exists(ContainerBuilder::class, 'addClassResource') || ($definition->isPublic() && !$definition->isPrivate()) ? 'services' : 'privates', $id);
61+
$instantiation .= sprintf(' $this->%s[%s] =', \method_exists(ContainerBuilder::class, 'addClassResource') || ($definition->isPublic() && !$definition->isPrivate()) ? 'services' : 'privates', var_export($id, true));
6262
}
6363

6464
if (null === $factoryCode) {

‎src/Symfony/Component/DependencyInjection/ContainerBuilder.php

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -868,6 +868,10 @@ public function setAlias($alias, $id)
868868
{
869869
$alias = $this->normalizeId($alias);
870870

871+
if ('' === $alias || '\\' === substr($alias, -1) || \strlen($alias) !== strcspn($alias, "\0\r\n'")) {
872+
throw new InvalidArgumentException(sprintf('Invalid alias id: "%s"', $alias));
873+
}
874+
871875
if (\is_string($id)) {
872876
$id = new Alias($this->normalizeId($id));
873877
} elseif (!$id instanceof Alias) {
@@ -1021,6 +1025,10 @@ public function setDefinition($id, Definition $definition)
10211025

10221026
$id = $this->normalizeId($id);
10231027

1028+
if ('' === $id || '\\' === substr($id, -1) || \strlen($id) !== strcspn($id, "\0\r\n'")) {
1029+
throw new InvalidArgumentException(sprintf('Invalid service id: "%s"', $id));
1030+
}
1031+
10241032
unset($this->aliasDefinitions[$id], $this->removedIds[$id]);
10251033

10261034
return $this->definitions[$id] = $definition;

‎src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -483,7 +483,7 @@ private function addServiceInstance($id, Definition $definition, $isSimpleInstan
483483
$instantiation = '';
484484

485485
if (!$isProxyCandidate && $definition->isShared()) {
486-
$instantiation = "\$this->services['$id'] = ".($isSimpleInstance ? '' : '$instance');
486+
$instantiation = sprintf('$this->services[%s] = %s', $this->doExport($id), $isSimpleInstance ? '' : '$instance');
487487
} elseif (!$isSimpleInstance) {
488488
$instantiation = '$instance';
489489
}
@@ -679,6 +679,9 @@ private function addService($id, Definition $definition, &$file = null)
679679
* Gets the $public '$id'$shared$autowired service.
680680
*
681681
* $return
682+
EOF;
683+
$code = str_replace('*/', ' ', $code).<<<EOF
684+
682685
*/
683686
protected function {$methodName}($lazyInitialization)
684687
{
@@ -693,7 +696,7 @@ protected function {$methodName}($lazyInitialization)
693696

694697
if ($this->getProxyDumper()->isProxyCandidate($definition)) {
695698
$factoryCode = $asFile ? "\$this->load('%s.php', false)" : '$this->%s(false)';
696-
$code .= $this->getProxyDumper()->getProxyFactoryCode($definition, $id, sprintf($factoryCode, $methodName));
699+
$code .= $this->getProxyDumper()->getProxyFactoryCode($definition, $id, sprintf($factoryCode, $methodName, $this->doExport($id)));
697700
}
698701

699702
if ($definition->isDeprecated()) {
@@ -767,14 +770,14 @@ private function addInlineReference($id, Definition $definition, $targetId, $for
767770

768771
$code .= sprintf(<<<'EOTXT'
769772
770-
if (isset($this->%s['%s'])) {
771-
return $this->%1$s['%2$s'];
773+
if (isset($this->%s[%s])) {
774+
return $this->%1$s[%2$s];
772775
}
773776

774777
EOTXT
775778
,
776779
'services',
777-
$id
780+
$this->doExport($id)
778781
);
779782

780783
return $code;
@@ -1530,14 +1533,14 @@ private function getServiceConditionals($value)
15301533
if (!$this->container->hasDefinition($service)) {
15311534
return 'false';
15321535
}
1533-
$conditions[] = sprintf("isset(\$this->services['%s'])", $service);
1536+
$conditions[] = sprintf('isset($this->services[%s])', $this->doExport($service));
15341537
}
15351538
foreach (ContainerBuilder::getServiceConditionals($value) as $service) {
15361539
if ($this->container->hasDefinition($service) && !$this->container->getDefinition($service)->isPublic()) {
15371540
continue;
15381541
}
15391542

1540-
$conditions[] = sprintf("\$this->has('%s')", $service);
1543+
$conditions[] = sprintf('$this->has(%s)', $this->doExport($service));
15411544
}
15421545

15431546
if (!$conditions) {
@@ -1776,6 +1779,8 @@ private function dumpLiteralClass($class)
17761779
*/
17771780
private function dumpParameter($name)
17781781
{
1782+
$name = (string) $name;
1783+
17791784
if ($this->container->isCompiled() && $this->container->hasParameter($name)) {
17801785
$value = $this->container->getParameter($name);
17811786
$dumpedValue = $this->dumpValue($value, false);
@@ -1785,11 +1790,11 @@ private function dumpParameter($name)
17851790
}
17861791

17871792
if (!preg_match("/\\\$this->(?:getEnv\('(?:\w++:)*+\w++'\)|targetDirs\[\d++\])/", $dumpedValue)) {
1788-
return sprintf("\$this->parameters['%s']", $name);
1793+
return sprintf('$this->parameters[%s]', $this->doExport($name));
17891794
}
17901795
}
17911796

1792-
return sprintf("\$this->getParameter('%s')", $name);
1797+
return sprintf('$this->getParameter(%s)', $this->doExport($name));
17931798
}
17941799

17951800
/**
@@ -1813,7 +1818,7 @@ private function getServiceCall($id, Reference $reference = null)
18131818

18141819
if ($this->container->hasDefinition($id) && $definition = $this->container->getDefinition($id)) {
18151820
if ($definition->isSynthetic()) {
1816-
$code = sprintf('$this->get(\'%s\'%s)', $id, null !== $reference ? ', '.$reference->getInvalidBehavior() : '');
1821+
$code = sprintf('$this->get(%s%s)', $this->doExport($id), null !== $reference ? ', '.$reference->getInvalidBehavior() : '');
18171822
} elseif (null !== $reference && ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE === $reference->getInvalidBehavior()) {
18181823
$code = 'null';
18191824
if (!$definition->isShared()) {
@@ -1822,7 +1827,7 @@ private function getServiceCall($id, Reference $reference = null)
18221827
} elseif ($this->isTrivialInstance($definition)) {
18231828
$code = substr($this->addNewInstance($definition, '', '', $id), 8, -2);
18241829
if ($definition->isShared()) {
1825-
$code = sprintf('$this->services[\'%s\'] = %s', $id, $code);
1830+
$code = sprintf('$this->services[%s] = %s', $this->doExport($id), $code);
18261831
}
18271832
$code = "($code)";
18281833
} elseif ($this->asFiles && $definition->isShared() && !$this->isHotPath($definition)) {
@@ -1833,14 +1838,14 @@ private function getServiceCall($id, Reference $reference = null)
18331838
} elseif (null !== $reference && ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE === $reference->getInvalidBehavior()) {
18341839
return 'null';
18351840
} elseif (null !== $reference && ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE !== $reference->getInvalidBehavior()) {
1836-
$code = sprintf('$this->get(\'%s\', /* ContainerInterface::NULL_ON_INVALID_REFERENCE */ %d)', $id, ContainerInterface::NULL_ON_INVALID_REFERENCE);
1841+
$code = sprintf('$this->get(%s, /* ContainerInterface::NULL_ON_INVALID_REFERENCE */ %d)', $this->doExport($id), ContainerInterface::NULL_ON_INVALID_REFERENCE);
18371842
} else {
1838-
$code = sprintf('$this->get(\'%s\')', $id);
1843+
$code = sprintf('$this->get(%s)', $this->doExport($id));
18391844
}
18401845

18411846
// The following is PHP 5.5 syntax for what could be written as "(\$this->services['$id'] ?? $code)" on PHP>=7.0
18421847

1843-
return "\${(\$_ = isset(\$this->services['$id']) ? \$this->services['$id'] : $code) && false ?: '_'}";
1848+
return sprintf("\${(\$_ = isset(\$this->services[%s]) ? \$this->services[%1\$s] : %s) && false ?: '_'}", $this->doExport($id), $code);
18441849
}
18451850

18461851
/**

‎src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,38 @@ public function testNonSharedServicesReturnsDifferentInstances()
195195
$this->assertNotSame($builder->get('bar'), $builder->get('bar'));
196196
}
197197

198+
/**
199+
* @expectedException \Symfony\Component\DependencyInjection\Exception\InvalidArgumentException
200+
* @dataProvider provideBadId
201+
*/
202+
public function testBadAliasId($id)
203+
{
204+
$builder = new ContainerBuilder();
205+
$builder->setAlias($id, 'foo');
206+
}
207+
208+
/**
209+
* @expectedException \Symfony\Component\DependencyInjection\Exception\InvalidArgumentException
210+
* @dataProvider provideBadId
211+
*/
212+
public function testBadDefinitionId($id)
213+
{
214+
$builder = new ContainerBuilder();
215+
$builder->setDefinition($id, new Definition('Foo'));
216+
}
217+
218+
public function provideBadId()
219+
{
220+
return [
221+
[''],
222+
["\0"],
223+
["\r"],
224+
["\n"],
225+
["'"],
226+
['ab\\'],
227+
];
228+
}
229+
198230
/**
199231
* @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException
200232
* @expectedExceptionMessage You have requested a synthetic service ("foo"). The DIC does not know how to construct this service.

‎src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -234,12 +234,18 @@ public function testAddServiceIdWithUnsupportedCharacters()
234234
{
235235
$class = 'Symfony_DI_PhpDumper_Test_Unsupported_Characters';
236236
$container = new ContainerBuilder();
237+
$container->setParameter("'", 'oh-no');
238+
$container->register("foo*/oh-no", 'FooClass')->setPublic(true);
237239
$container->register('bar$', 'FooClass')->setPublic(true);
238240
$container->register('bar$!', 'FooClass')->setPublic(true);
239241
$container->compile();
240242
$dumper = new PhpDumper($container);
241-
eval('?>'.$dumper->dump(['class' => $class]));
242243

244+
$this->assertStringEqualsFile(self::$fixturesPath.'/php/services_unsupported_characters.php', $dumper->dump(['class' => $class]));
245+
246+
require_once self::$fixturesPath.'/php/services_unsupported_characters.php';
247+
248+
$this->assertTrue(method_exists($class, 'getFooOhNoService'));
243249
$this->assertTrue(method_exists($class, 'getBarService'));
244250
$this->assertTrue(method_exists($class, 'getBar2Service'));
245251
}

‎src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services33.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ public function isFrozen()
6666
*/
6767
protected function getFooService()
6868
{
69-
return $this->services['Bar\Foo'] = new \Bar\Foo();
69+
return $this->services['Bar\\Foo'] = new \Bar\Foo();
7070
}
7171

7272
/**
@@ -76,6 +76,6 @@ protected function getFooService()
7676
*/
7777
protected function getFoo2Service()
7878
{
79-
return $this->services['Foo\Foo'] = new \Foo\Foo();
79+
return $this->services['Foo\\Foo'] = new \Foo\Foo();
8080
}
8181
}

‎src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_adawson.php

Lines changed: 23 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -88,10 +88,10 @@ public function isFrozen()
8888
*/
8989
protected function getBusService()
9090
{
91-
$this->services['App\Bus'] = $instance = new \App\Bus(${($_ = isset($this->services['App\Db']) ? $this->services['App\Db'] : $this->getDbService()) && false ?: '_'});
91+
$this->services['App\\Bus'] = $instance = new \App\Bus(${($_ = isset($this->services['App\\Db']) ? $this->services['App\\Db'] : $this->getDbService()) && false ?: '_'});
9292

93-
$instance->handler1 = ${($_ = isset($this->services['App\Handler1']) ? $this->services['App\Handler1'] : $this->getHandler1Service()) && false ?: '_'};
94-
$instance->handler2 = ${($_ = isset($this->services['App\Handler2']) ? $this->services['App\Handler2'] : $this->getHandler2Service()) && false ?: '_'};
93+
$instance->handler1 = ${($_ = isset($this->services['App\\Handler1']) ? $this->services['App\\Handler1'] : $this->getHandler1Service()) && false ?: '_'};
94+
$instance->handler2 = ${($_ = isset($this->services['App\\Handler2']) ? $this->services['App\\Handler2'] : $this->getHandler2Service()) && false ?: '_'};
9595

9696
return $instance;
9797
}
@@ -103,9 +103,9 @@ protected function getBusService()
103103
*/
104104
protected function getDbService()
105105
{
106-
$this->services['App\Db'] = $instance = new \App\Db();
106+
$this->services['App\\Db'] = $instance = new \App\Db();
107107

108-
$instance->schema = ${($_ = isset($this->services['App\Schema']) ? $this->services['App\Schema'] : $this->getSchemaService()) && false ?: '_'};
108+
$instance->schema = ${($_ = isset($this->services['App\\Schema']) ? $this->services['App\\Schema'] : $this->getSchemaService()) && false ?: '_'};
109109

110110
return $instance;
111111
}
@@ -117,13 +117,13 @@ protected function getDbService()
117117
*/
118118
protected function getHandler1Service()
119119
{
120-
$a = ${($_ = isset($this->services['App\Processor']) ? $this->services['App\Processor'] : $this->getProcessorService()) && false ?: '_'};
120+
$a = ${($_ = isset($this->services['App\\Processor']) ? $this->services['App\\Processor'] : $this->getProcessorService()) && false ?: '_'};
121121

122-
if (isset($this->services['App\Handler1'])) {
123-
return $this->services['App\Handler1'];
122+
if (isset($this->services['App\\Handler1'])) {
123+
return $this->services['App\\Handler1'];
124124
}
125125

126-
return $this->services['App\Handler1'] = new \App\Handler1(${($_ = isset($this->services['App\Db']) ? $this->services['App\Db'] : $this->getDbService()) && false ?: '_'}, ${($_ = isset($this->services['App\Schema']) ? $this->services['App\Schema'] : $this->getSchemaService()) && false ?: '_'}, $a);
126+
return $this->services['App\\Handler1'] = new \App\Handler1(${($_ = isset($this->services['App\\Db']) ? $this->services['App\\Db'] : $this->getDbService()) && false ?: '_'}, ${($_ = isset($this->services['App\\Schema']) ? $this->services['App\\Schema'] : $this->getSchemaService()) && false ?: '_'}, $a);
127127
}
128128

129129
/**
@@ -133,13 +133,13 @@ protected function getHandler1Service()
133133
*/
134134
protected function getHandler2Service()
135135
{
136-
$a = ${($_ = isset($this->services['App\Processor']) ? $this->services['App\Processor'] : $this->getProcessorService()) && false ?: '_'};
136+
$a = ${($_ = isset($this->services['App\\Processor']) ? $this->services['App\\Processor'] : $this->getProcessorService()) && false ?: '_'};
137137

138-
if (isset($this->services['App\Handler2'])) {
139-
return $this->services['App\Handler2'];
138+
if (isset($this->services['App\\Handler2'])) {
139+
return $this->services['App\\Handler2'];
140140
}
141141

142-
return $this->services['App\Handler2'] = new \App\Handler2(${($_ = isset($this->services['App\Db']) ? $this->services['App\Db'] : $this->getDbService()) && false ?: '_'}, ${($_ = isset($this->services['App\Schema']) ? $this->services['App\Schema'] : $this->getSchemaService()) && false ?: '_'}, $a);
142+
return $this->services['App\\Handler2'] = new \App\Handler2(${($_ = isset($this->services['App\\Db']) ? $this->services['App\\Db'] : $this->getDbService()) && false ?: '_'}, ${($_ = isset($this->services['App\\Schema']) ? $this->services['App\\Schema'] : $this->getSchemaService()) && false ?: '_'}, $a);
143143
}
144144

145145
/**
@@ -149,13 +149,13 @@ protected function getHandler2Service()
149149
*/
150150
protected function getProcessorService()
151151
{
152-
$a = ${($_ = isset($this->services['App\Registry']) ? $this->services['App\Registry'] : $this->getRegistryService()) && false ?: '_'};
152+
$a = ${($_ = isset($this->services['App\\Registry']) ? $this->services['App\\Registry'] : $this->getRegistryService()) && false ?: '_'};
153153

154-
if (isset($this->services['App\Processor'])) {
155-
return $this->services['App\Processor'];
154+
if (isset($this->services['App\\Processor'])) {
155+
return $this->services['App\\Processor'];
156156
}
157157

158-
return $this->services['App\Processor'] = new \App\Processor($a, ${($_ = isset($this->services['App\Db']) ? $this->services['App\Db'] : $this->getDbService()) && false ?: '_'});
158+
return $this->services['App\\Processor'] = new \App\Processor($a, ${($_ = isset($this->services['App\\Db']) ? $this->services['App\\Db'] : $this->getDbService()) && false ?: '_'});
159159
}
160160

161161
/**
@@ -165,9 +165,9 @@ protected function getProcessorService()
165165
*/
166166
protected function getRegistryService()
167167
{
168-
$this->services['App\Registry'] = $instance = new \App\Registry();
168+
$this->services['App\\Registry'] = $instance = new \App\Registry();
169169

170-
$instance->processor = [0 => ${($_ = isset($this->services['App\Db']) ? $this->services['App\Db'] : $this->getDbService()) && false ?: '_'}, 1 => ${($_ = isset($this->services['App\Bus']) ? $this->services['App\Bus'] : $this->getBusService()) && false ?: '_'}];
170+
$instance->processor = [0 => ${($_ = isset($this->services['App\\Db']) ? $this->services['App\\Db'] : $this->getDbService()) && false ?: '_'}, 1 => ${($_ = isset($this->services['App\\Bus']) ? $this->services['App\\Bus'] : $this->getBusService()) && false ?: '_'}];
171171

172172
return $instance;
173173
}
@@ -179,12 +179,12 @@ protected function getRegistryService()
179179
*/
180180
protected function getSchemaService()
181181
{
182-
$a = ${($_ = isset($this->services['App\Db']) ? $this->services['App\Db'] : $this->getDbService()) && false ?: '_'};
182+
$a = ${($_ = isset($this->services['App\\Db']) ? $this->services['App\\Db'] : $this->getDbService()) && false ?: '_'};
183183

184-
if (isset($this->services['App\Schema'])) {
185-
return $this->services['App\Schema'];
184+
if (isset($this->services['App\\Schema'])) {
185+
return $this->services['App\\Schema'];
186186
}
187187

188-
return $this->services['App\Schema'] = new \App\Schema($a);
188+
return $this->services['App\\Schema'] = new \App\Schema($a);
189189
}
190190
}

0 commit comments

Comments
 (0)