Skip to content
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

[DI] Validate env vars in config #23888

Closed
wants to merge 7 commits into from
Closed

[DI] Validate env vars in config #23888

wants to merge 7 commits into from

Conversation

@ro0NL
Copy link
Contributor

@ro0NL ro0NL commented Aug 14, 2017

Q A
Branch? 4.1/master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #22151, #25868
License MIT
Doc PR symfony/symfony-docs#8382

This PR registers the env placeholders in Config\BaseNode with its default value or an empty string. It doesnt request real env vars during compilation,

What it does is if a config value exactly matches a env placeholder, we validate/normalize the default value/empty string but we keep returning the env placeholder as usual. If a placeholder occurs in the middle of a string it also proceeds as usual.

The latter to me is OK as you need to expect any string value during runtime anyway, including the empty string.

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Aug 16, 2017

I suggest wait for #23901 before continuing on this topic - that will provide a much better base to fix this issue.

Loading

@ro0NL
Copy link
Contributor Author

@ro0NL ro0NL commented Sep 7, 2017

@nicolas-grekas could you briefly describe the path you have in mind for this, and how it leverages getProvidedTypes?

From a technical pov this approach was the only thing i could come up with, regarding decoupled state between di and config. I like how it works out-of-the-box for everyone :)

My idea here was to generate a better default value using getEnv() actually, probably calling it before being compiled (as that's allowed anyway ^^)

Loading

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Sep 8, 2017

The env placeholders contain the prefixes - so we can extract that, and use EnvPlaceholderParameterBag::getProvidedTypes() to get the type of each placeholder.
That should be enough, isn't it?

Loading

@ro0NL
Copy link
Contributor Author

@ro0NL ro0NL commented Sep 9, 2017

@nicolas-grekas first round, wdyt?

Loading

*/
public static function setPlaceholder($placeholder, $type)
{
switch ($type) {
Copy link
Contributor Author

@ro0NL ro0NL Sep 9, 2017

Choose a reason for hiding this comment

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

IMO. can be done at the calling side..

Loading

@@ -92,7 +91,7 @@ public function __construct()
public function getPasses()
{
return array_merge(
array($this->mergePass),
array(new RegisterEnvVarProcessorsPass(), $this->mergePass),
Copy link
Contributor Author

@ro0NL ro0NL Sep 9, 2017

Choose a reason for hiding this comment

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

now runs even earlier.. anything to do? Patched PhpDumper test for now.

Loading

Copy link
Member

@nicolas-grekas nicolas-grekas Sep 9, 2017

Choose a reason for hiding this comment

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

LGTM

Loading

Copy link
Member

@nicolas-grekas nicolas-grekas Oct 6, 2017

Choose a reason for hiding this comment

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

Wait, I just realized that this may just make it impossible to register a new env processor in practice, isn't it?

Loading

continue;
}
foreach ($placeholders as $placeholder) {
BaseNode::setPlaceholder($placeholder, reset($envTypes[$prefix]));
Copy link
Contributor Author

@ro0NL ro0NL Sep 9, 2017

Choose a reason for hiding this comment

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

What about getParam("env($env)") as default value instead, if available :) That justifies computing default values on the calling side.

Loading

Copy link
Member

@nicolas-grekas nicolas-grekas Sep 9, 2017

Choose a reason for hiding this comment

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

Let's do. We also need to validate the type of the default value before calling setPlaceholder (its type needs to be in $envTypes).

Loading

Copy link
Member

@nicolas-grekas nicolas-grekas Sep 9, 2017

Choose a reason for hiding this comment

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

BUT, calling "reset" here prevents taking advantage of the "multiple types provided" info. So maybe pass an array instead, $type=>$value?

Loading

@@ -43,14 +44,23 @@ public function process(ContainerBuilder $container)
foreach ($class::getProvidedTypes() as $prefix => $type) {
$processors[$prefix] = new ServiceClosureArgument(new Reference($id));
$types[$prefix] = self::validateProvidedTypes($type, $class);

if (!$types[$prefix]) {
Copy link
Member

@nicolas-grekas nicolas-grekas Sep 9, 2017

Choose a reason for hiding this comment

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

can this happen at all? explode always creates an array with at least one element, isn't it?

Loading

Copy link
Contributor Author

@ro0NL ro0NL Sep 9, 2017

Choose a reason for hiding this comment

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

can this happen at all?

nope :)

Loading

@ro0NL ro0NL changed the title [DI] Validate env vars in config [WIP][DI] Validate env vars in config Sep 9, 2017
@ro0NL
Copy link
Contributor Author

@ro0NL ro0NL commented Sep 9, 2017

@nicolas-grekas pushed latest changes real quick. Think this is heading towards what you have in mind; see BaseNode.

Loading

foreach (self::$placeholders[$value] as $placeholder) {
try {
$this->normalize($placeholder);
break;
Copy link
Member

@nicolas-grekas nicolas-grekas Sep 9, 2017

Choose a reason for hiding this comment

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

should be return $value;
and the current return below should be throw $e;

Loading

Copy link
Contributor Author

@ro0NL ro0NL Sep 10, 2017

Choose a reason for hiding this comment

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

now with test :)

Loading

@ro0NL ro0NL changed the title [WIP][DI] Validate env vars in config [DI] Validate env vars in config Sep 10, 2017
@ro0NL
Copy link
Contributor Author

@ro0NL ro0NL commented Sep 10, 2017

Tests looking good. Tried to keep the big code blob at a minimum, but tweaks are welcome :)

Status: needs review

deps=high failure expected.

Loading

foreach (self::$placeholders[$leftSide] as $placeholder) {
try {
$this->validateType($placeholder);
break;
Copy link
Member

@nicolas-grekas nicolas-grekas Sep 10, 2017

Choose a reason for hiding this comment

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

return + throw here also (and below)

Loading

Copy link
Member

@nicolas-grekas nicolas-grekas Sep 10, 2017

Choose a reason for hiding this comment

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

or at least, the throw part is missing if return is not for here

Loading

@ro0NL
Copy link
Contributor Author

@ro0NL ro0NL commented Sep 10, 2017

@nicolas-grekas now with test for validating on merge :) see simplified BaseNode::resolvePlaceholderValues() + merge().

Loading

@@ -79,6 +79,13 @@ public function mergeEnvPlaceholders(self $bag)
$this->envPlaceholders[$env] += $placeholders;
}
}
foreach ($bag->getProvidedTypes() as $prefix => $types) {
Copy link
Member

@nicolas-grekas nicolas-grekas Sep 10, 2017

Choose a reason for hiding this comment

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

I think this file doesn't need to be modified: merging happens after setProvidedTypes is called, and should not happen anymore after

Loading

Copy link
Contributor Author

@ro0NL ro0NL Sep 10, 2017

Choose a reason for hiding this comment

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

They get lost in MergeExtensionConfigurationParameterBag::__construct as we overwrite $resolvingBag.

But agree; this method is called mergeEnvPlaceholders.. so doesnt imply it merges types. Now fixed.

Loading

@ro0NL
Copy link
Contributor Author

@ro0NL ro0NL commented Sep 10, 2017

@nicolas-grekas what about this;

node: 'value' > "process" > node: 'normalized-prefix value'

But then with env placeholders where "env_NAME_x" becomes "normalized-prefix env_NAME_x".

I think current behavior is OK as envs are normalized values (nothing happens with them after compile, i.e. no config normalization, except processing). Thus you'd have NAME="normalized-prefix value".

WDYT?

edit: hm that only applies if a type conversion happens, and we lose the placeholder. For string-to-string we could in fact return the processed value if it still contains the original placeholder (and perhaps if not also). Not sure who's exactly responsible here :)

Loading

@symfony symfony deleted a comment from ro0NL Sep 11, 2017
$default = null;
if ($hasEnv = (false === $i && $defaultEnvBag->has("env($env)"))) {
switch ($type = gettype($default = $defaultEnvBag->get("env($env)"))) {
case 'boolean': $type = 'bool'; break;
Copy link
Member

@nicolas-grekas nicolas-grekas Sep 11, 2017

Choose a reason for hiding this comment

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

should we rename our allowedTypes to match those of gettype and remove that switch?

Loading

Copy link
Contributor Author

@ro0NL ro0NL Sep 11, 2017

Choose a reason for hiding this comment

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

Think so 👍 preferring FQ over short names.

Loading

Copy link
Contributor Author

@ro0NL ro0NL Sep 11, 2017

Choose a reason for hiding this comment

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

While at it should we infer boolean|integer|float|string from scalar while resolving? Nice to have perhaps.

Loading

case 'float': $values[$type] = 0.0; break;
case 'int': $values[$type] = 0; break;
case 'string': $values[$type] = ''; break;
default: $values[$type] = null; break;
Copy link
Member

@nicolas-grekas nicolas-grekas Sep 11, 2017

Choose a reason for hiding this comment

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

is this case possible? if not, default should be string, isn't it?

Loading

Copy link
Contributor Author

@ro0NL ro0NL Sep 11, 2017

Choose a reason for hiding this comment

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

only if we forget to update it after adding types. Should be never, just a safety net.

default: throw then? In case of non-fatal behavior id say null.. why assume string?

Loading

Copy link
Contributor Author

@ro0NL ro0NL Sep 11, 2017

Choose a reason for hiding this comment

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

no default case and let php crash? fine fore me :)

Loading

@cdaguerre
Copy link

@cdaguerre cdaguerre commented Mar 19, 2018

Does the milestone change mean it won't make it into the 3.4 branch?

Loading

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

(with minor comments)

Loading

return self::$placeholders[$value];
}

if (0 === strpos($value, self::$placeholderUniquePrefix, 0)) {
Copy link
Member

@nicolas-grekas nicolas-grekas Mar 21, 2018

Choose a reason for hiding this comment

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

3rd arg not needed

Loading


private static function getType($value): string
{
switch ($type = gettype($value)) {
Copy link
Member

@nicolas-grekas nicolas-grekas Mar 21, 2018

Choose a reason for hiding this comment

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

\gettype

Loading

@@ -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

@nicolas-grekas nicolas-grekas Mar 21, 2018

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.

Loading

Copy link
Contributor Author

@ro0NL ro0NL Mar 21, 2018

Choose a reason for hiding this comment

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

Loading

Copy link
Member

@nicolas-grekas nicolas-grekas Mar 21, 2018

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

Loading

Copy link
Contributor Author

@ro0NL ro0NL Mar 21, 2018

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?

Loading

Copy link
Member

@nicolas-grekas nicolas-grekas Mar 21, 2018

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

:)

Loading

Copy link
Contributor Author

@ro0NL ro0NL Mar 21, 2018

Choose a reason for hiding this comment

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

Done :)

Loading

*/
public function getEnvPlaceholderUniquePrefix(): string
{
return $this->envPlaceholderUniquePrefix ?? $this->envPlaceholderUniquePrefix = 'env_'.bin2hex(random_bytes(16));
Copy link
Member

@nicolas-grekas nicolas-grekas Mar 21, 2018

Choose a reason for hiding this comment

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

8 instead of 16 is enough (we use 7 bytes in ContainerBuilder::hash())

Loading

@@ -50,6 +56,39 @@ 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
Copy link
Member

@fabpot fabpot Mar 22, 2018

Choose a reason for hiding this comment

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

Registers

Loading

Copy link
Member

@fabpot fabpot Mar 22, 2018

Choose a reason for hiding this comment

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

I would move all sentences but the first one as a phpdoc description.

Loading

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.

You mean split into short & long description right? Did that for now.

Loading

}

/**
* Set a common prefix for dynamic placeholder values. Matching configuration values will be skipped from being
Copy link
Member

@fabpot fabpot Mar 22, 2018

Choose a reason for hiding this comment

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

Sets + same comment as above about separating the first sentence from the other ones.

Loading

}

/**
* Reset all current placeholders available.
Copy link
Member

@fabpot fabpot Mar 22, 2018

Choose a reason for hiding this comment

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

Resets

Loading

public static function resetPlaceholders(): void
{
self::$placeholderUniquePrefix = null;
self::$placeholders = array();
Copy link
Member

@fabpot fabpot Mar 22, 2018

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.

Loading

Copy link
Member

@nicolas-grekas nicolas-grekas Mar 22, 2018

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.

Loading

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.

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.

Loading

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.

Loading

@@ -363,4 +459,85 @@ public function getParent()
* @return mixed The finalized value
*/
abstract protected function finalizeValue($value);

/**
* Test if placeholder values are allowed for this node.
Copy link
Member

@fabpot fabpot Mar 22, 2018

Choose a reason for hiding this comment

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

Tests

Loading

}

/**
* Get allowed dynamic types for this node.
Copy link
Member

@fabpot fabpot Mar 22, 2018

Choose a reason for hiding this comment

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

Gets

Loading

/**
* Resets all current placeholders available.
*/
public static function resetPlaceholders(): void
Copy link
Member

@nicolas-grekas nicolas-grekas Mar 23, 2018

Choose a reason for hiding this comment

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

this one also needs to be internal

Loading

Copy link
Contributor Author

@ro0NL ro0NL Mar 23, 2018

Choose a reason for hiding this comment

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

oops :) updated.

Loading

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Mar 27, 2018

Thank you @ro0NL.

Loading

@ro0NL ro0NL deleted the config/placeholders branch Mar 27, 2018
$container = new ContainerBuilder();
$container->registerExtension(new EnvExtension());
$container->prependExtensionConfig('env_extension', array(
'simple_array_node' => '%env(json:FOO)%',
Copy link
Contributor

@mcfedr mcfedr Mar 27, 2018

Choose a reason for hiding this comment

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

json: can return a simple_array, e.g. in the case the env is something like [1, 2] - of course json doesnt guarrentee that its like this, it could also be {"a": 1, "b": 2}, so its a bit of a question, is it valid or not - I think its more useful if it passes validation.

Loading

Copy link
Contributor Author

@ro0NL ro0NL Mar 27, 2018

Choose a reason for hiding this comment

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

yeah i think we need to reassure / revise integration a bit. Or put different im curious if the change to json prefix affects the possibilities here.

Loading

Copy link
Contributor

@mcfedr mcfedr Mar 27, 2018

Choose a reason for hiding this comment

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

the json change only added the possibility to accept null in addition to array so shouldn't change anything here - this was on purpose so that its still possible to do some validation based on it always being an array.

I wonder if it would make sense to add a simple_array: decoder to further help with validation

Loading

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

@mcfedr mcfedr Mar 27, 2018

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.

Loading

Copy link
Member

@chalasr chalasr Mar 27, 2018

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

Loading

Copy link
Contributor Author

@ro0NL ro0NL Mar 27, 2018

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.

Loading

Copy link
Contributor

@mcfedr mcfedr Mar 27, 2018

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.

Loading

@Majkl578
Copy link
Contributor

@Majkl578 Majkl578 commented May 31, 2018

FYI: Just got hit by this BC break when upgrading to 4.1.0:

In ValidateEnvPlaceholdersPass.php line 54:
                                                                                                              
  [Symfony\Component\DependencyInjection\Exception\LogicException]                                            
  Invalid type for env parameter "env(ABC)". Expected "string", but got "float".

for configuration with env defaults:

parameters:
    env(ABC): 1.2

Issue & failing test coming soon.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants