8000 [DI] Validate env vars in config by ro0NL · Pull Request #23888 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[DI] Validate env vars in config #23888

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.

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

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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 UPGRADE-4.1.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ Console
-------

* Deprecated the `setCrossingChar()` method in favor of the `setDefaultCrossingChar()` method in `TableStyle`.
* The `Processor` class has been made final

DependencyInjection
-------------------
Expand Down
1 change: 1 addition & 0 deletions UPGRADE-5.0.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ Config
------
10000
* Added the `getChildNodeDefinitions()` method to `ParentNodeDefinitionInterface`.
* The `Processor` class has been made final

Console
-------
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Component/Config/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ CHANGELOG

* added `setPathSeparator` method to `NodeBuilder` class
* added third `$pathSeparator` constructor argument to `BaseNode`
* the `Processor` class has been made final

4.0.0
-----
Expand Down
8 changes: 8 additions & 0 deletions src/Symfony/Component/Config/Definition/ArrayNode.php
Original file line number Diff line number Diff line change
Expand Up @@ -390,4 +390,12 @@ protected function mergeValues($leftSide, $rightSide)

return $leftSide;
}

/**
* {@inheritdoc}
*/
protected function allowPlaceholders(): bool
{
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have you made it false by default for ArrayNode? It seems this will limit how much of a config you can change in other peoples bundles, i.e. you can only put env(json:FOO) if the owner things of you in advance.

Copy link
Member

Choose a reason for hiding this comment

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

comments on closed PRs are likely to be lost, please consider opening a new issue/PR if you think something is buggy or can be improved. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case i asked for it on another closed PR :P But yeah.. not sure if there's any real issue now (hence i asked :D)

@mcfedr see symfony/symfony-docs#8382 (comment) , tried to explain the general proces there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, makes sense, so for simple_array nodes it will accept env(json:FOO) but not when it should validate the structure.

}
}
193 changes: 189 additions & 4 deletions src/Symfony/Component/Config/Definition/BaseNode.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use Symfony\Component\Config\Definition\Exception\ForbiddenOverwriteException;
use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException;
use Symfony\Component\Config\Definition\Exception\InvalidTypeException;
use Symfony\Component\Config\Definition\Exception\UnsetKeyException;

/**
* The base node class.
Expand All @@ -25,6 +26,9 @@ abstract class BaseNode implements NodeInterface
{
const DEFAULT_PATH_SEPARATOR = '.';

private static $placeholderUniquePrefix;
private static $placeholders = array();

protected $name;
protected $parent;
protected $normalizationClosures = array();
Expand All @@ -36,6 +40,8 @@ abstract class BaseNode implements NodeInterface
protected $attributes = array();
protected $pathSeparator;

private $handlingPlaceholder;

/**
* @throws \InvalidArgumentException if the name contains a period
*/
Expand All @@ -50,6 +56,47 @@ public function __construct(?string $name, NodeInterface $parent = null, string
$this->pathSeparator = $pathSeparator;
}

/**
* Register possible (dummy) values for a dynamic placeholder value.
*
* Matching configuration values will be processed with a provided value, one by one. After a provided value is
* successfully processed the configuration value is returned as is, thus preserving the placeholder.
*
* @internal
*/
public static function setPlaceholder(string $placeholder, array $values): void
{
if (!$values) {
throw new \InvalidArgumentException('At least one value must be provided.');
}

self::$placeholders[$placeholder] = $values;
}

/**
* Sets a common prefix for dynamic placeholder values.
*
* Matching configuration values will be skipped from being processed and are returned as is, thus preserving the
* placeholder. An exact match provided by {@see setPlaceholder()} might take precedence.
*
* @internal
*/
public static function setPlaceholderUniquePrefix(string $prefix): void
{
self::$placeholderUniquePrefix = $prefix;
}

/**
* Resets all current placeholders available.
*
* @internal
*/
public static function resetPlaceholders(): void
{
self::$placeholderUniquePrefix = null;
self::$placeholders = array();
Copy link
Member

Choose a reason for hiding this comment

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

Having static properties like this is very dangerous as we need to make sure that they are reset properly. I can see the finally calls here and there to reset this, but is there is any other way? Avoiding static properties would be great.

Copy link
Member

Choose a reason for hiding this comment

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

It's not that dangerous: placeholders are unique strings so there is not really any way to mess up with them, even if someone forgets to reset them.
About removing the static state, that's really hard...
BUT, @ro0NL we should make these methods @internal for sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can settle indeed, i dont see a elegant workaround to avoid static currently, that is without overhauling the config component itself :)

Now marked internal.

Copy link
Contributor Author
@ro0NL ro0NL Mar 22, 2018

Choose a reason for hiding this comment

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

also we deal with it properly where needed; it's not scattered all over the environment.

}

public function setAttribute($key, $value)
{
$this->attributes[$key] = $value;
Expand Down Expand Up @@ -249,8 +296,34 @@ final public function merge($leftSide, $rightSide)
));
}

$this->validateType($leftSide);
$this->validateType($rightSide);
if ($leftSide !== $leftPlaceholders = self::resolvePlaceholderValue($leftSide)) {
foreach ($leftPlaceholders as $leftPlaceholder) {
$this->handlingPlaceholder = $leftSide;
try {
$this->merge($leftPlaceholder, $rightSide);
} finally {
$this->handlingPlaceholder = null;
}
}

return $rightSide;
}

if ($rightSide !== $rightPlaceholders = self::resolvePlaceholderValue($rightSide)) {
foreach ($rightPlaceholders as $rightPlaceholder) {
$this->handlingPlaceholder = $rightSide;
try {
$this->merge($leftSide, $rightPlaceholder);
} finally {
$this->handlingPlaceholder = null;
}
}

return $rightSide;
}

$this->doValidateType($leftSide);
$this->doValidateType($rightSide);

return $this->mergeValues($leftSide, $rightSide);
}
Expand All @@ -267,6 +340,20 @@ final public function normalize($value)
$value = $closure($value);
}

// resolve placeholder value
if ($value !== $placeholders = self::resolvePlaceholderValue($value)) {
foreach ($placeholders as $placeholder) {
$this->handlingPlaceholder = $value;
try {
$this->normalize($placeholder);
} finally {
$this->handlingPlaceholder = null;
}
}

return $value;
}

// replace value with their equivalent
foreach ($this->equivalentValues as $data) {
if ($data[0] === $value) {
Expand All @@ -275,7 +362,7 @@ final public function normalize($value)
}

// validate type
$this->validateType($value);
$this->doValidateType($value);

// normalize value
return $this->normalizeValue($value);
Expand Down Expand Up @@ -308,7 +395,20 @@ public function getParent()
*/
final public function finalize($value)
{
$this->validateType($value);
if ($value !== $placeholders = self::resolvePlaceholderValue($value)) {
foreach ($placeholders as $placeholder) {
$this->handlingPlaceholder = $value;
try {
$this->finalize($placeholder);
} finally {
$this->handlingPlaceholder = null;
10000 }
}

return $value;
}

$this->doValidateType($value);

$value = $this->finalizeValue($value);

Expand All @@ -318,6 +418,10 @@ final public function finalize($value)
try {
$value = $closure($value);
} catch (Exception $e) {
if ($e instanceof UnsetKeyException && null !== $this->handlingPlaceholder) {
continue;
}

throw $e;
} catch (\Exception $e) {
throw new InvalidConfigurationException(sprintf('Invalid configuration for path "%s": %s', $this->getPath(), $e->getMessage()), $e->getCode(), $e);
Expand Down Expand Up @@ -363,4 +467,85 @@ abstract protected function mergeValues($leftSide, $rightSide);
* @return mixed The finalized value
*/
abstract protected function finalizeValue($value);

/**
* Tests if placeholder values are allowed for this node.
*/
protected function allowPlaceholders(): bool
{
return true;
}

/**
* Gets allowed dynamic types for this node.
*/
protected function getValidPlaceholderTypes(): array
{
return array();
}

private static function resolvePlaceholderValue($value)
{
if (\is_string($value)) {
if (isset(self::$placeholders[$value])) {
return self::$placeholders[$value];
}

if (0 === strpos($value, self::$placeholderUniquePrefix)) {
return array();
}
}

return $value;
}

private static function getType($value): string
{
switch ($type = \gettype($value)) {
case 'boolean':
return 'bool';
case 'double':
return 'float';
case 'integer':
return 'int';
}

return $type;
}

private function doValidateType($value): void
{
if (null === $this->handlingPlaceholder || null === $value) {
$this->validateType($value);

return;
}

if (!$this->allowPlaceholders()) {
$e = new InvalidTypeException(sprintf('A dynamic value is not compatible with a "%s" node type at path "%s".', get_class($this), $this->getPath()));
$e->setPath($this->getPath());

throw $e;
}

$knownTypes = array_keys(self::$placeholders[$this->handlingPlaceholder]);
$validTypes = $this->getValidPlaceholderTypes();

if (array_diff($knownTypes, $validTypes)) {
$e = new InvalidTypeException(sprintf(
'Invalid type for path "%s". Expected %s, but got %s.',
$this->getPath(),
1 === count($validTypes) ? '"'.reset($validTypes).'"' : 'one of "'.implode('", "', $validTypes).'"',
1 === count($knownTypes) ? '"'.reset($knownTypes).'"' : 'one of "'.implode('", "', $knownTypes).'"'
));
if ($hint = $this->getInfo()) {
$e->addHint($hint);
}
$e->setPath($this->getPath());

throw $e;
}

$this->validateType($value);
}
}
8 changes: 8 additions & 0 deletions src/Symfony/Component/Config/Definition/BooleanNode.php
Original file line number Diff line number Diff line change
Expand Up @@ -48,4 +48,12 @@ protected function isValueEmpty($value)
// a boolean value cannot be empty
return false;
}

/**
* {@inheritdoc}
*/
protected function getValidPlaceholderTypes(): array
{
return array('bool');
}
}
8 changes: 8 additions & 0 deletions src/Symfony/Component/Config/Definition/EnumNode.php
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,12 @@ protected function finalizeValue($value)

return $value;
}

/**
* {@inheritdoc}
*/
protected function allowPlaceholders(): bool
{
return false;
}
}
8 changes: 8 additions & 0 deletions src/Symfony/Component/Config/Definition/FloatNode.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,12 @@ protected function validateType($value)
throw $ex;
}
}

/**
* {@inheritdoc}
*/
protected function getValidPlaceholderTypes(): array
{
return array('float');
}
}
8 changes: 8 additions & 0 deletions src/Symfony/Component/Config/Definition/IntegerNode.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,4 +35,12 @@ protected function validateType($value)
throw $ex;
}
}

/**
* {@inheritdoc}
*/
protected function getValidPlaceholderTypes(): array
{
return array('int');
}
}
2 changes: 2 additions & 0 deletions src/Symfony/Component/Config/Definition/Processor.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
* This class is the entry point for config normalization/merging/finalization.
*
* @author Johannes M. Schmitt <schmittjoh@gmail.com>
*
* @final since version 4.1
Copy link
Member

Choose a reason for hiding this comment

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

unrelated to this PR? If yes, should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

then: missing changelog/upgrade entries

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changelog entry added. Suggestion for an upgrade note?

Copy link
Member

Choose a reason for hiding this comment

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

yes!
in *4.1: The ... class has been made final
in *5.0: The ... class has been made final

:)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

*/
class Processor
{
Expand Down
8 changes: 8 additions & 0 deletions src/Symfony/Component/Config/Definition/ScalarNode.php
Original file line number Diff line number Diff line change
Expand Up @@ -54,4 +54,12 @@ protected function isValueEmpty($value)
{
return null === $value || '' === $value;
}

/**
* {@inheritdoc}
*/
protected function getValidPlaceholderTypes(): array
{
return array('bool', 'int', 'float', 'string');
}
}
1 change: 1 addition & 0 deletions src/Symfony/Component/DependencyInjection/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ CHANGELOG
* added PSR-11 `ContainerBagInterface` and its `ContainerBag` implementation to access parameters as-a-service
* added support for service's decorators autowiring
* deprecated the `TypedReference::canBeAutoregistered()` and `TypedReference::getRequiringClass()` methods
* environment variables are validated when used in extension configuration

4.0.0
-----
Expand Down
Loading
0