8000 Unable to resolve env() in (Security) configuration · Issue #26747 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Unable to resolve env() in (Security) configuration #26747

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.

Sign up for GitHub

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

Jump to bottom
Closed
sstok opened this issue Apr 2, 2018 · 11 comments
Closed

Unable to resolve env() in (Security) configuration #26747

sstok opened this issue Apr 2, 2018 · 11 comments
Labels
Bug DependencyInjection Status: Needs Review
Milestone
4.1

Comments

@sstok
Copy link
Contributor
sstok commented Apr 2, 2018
Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version 4.1-dev

Since (I suspect) #23888 using env() in a firewall configuration no longer works.

The path "security.firewalls.main.remember_me.secret" cannot contain an empty value, but got "".

In VariableNode.php line 85:

  [Symfony\Component\Config\Definition\Exception\InvalidConfigurationException]
  The path "security.firewalls.main.remember_me.secret" cannot contain an empty value, but got "".


Exception trace:
 Symfony\Component\Config\Definition\VariableNode->finalizeValue() at /Users/sebastiaanstok/Sites/symfony-41/my_project/vendor/symfony/config/Definition/BaseNode.php:413
 Symfony\Component\Config\Definition\BaseNode->finalize() at /Users/sebastiaanstok/Sites/symfony-41/my_project/vendor/symfony/config/Definition/BaseNode.php:402
 Symfony\Component\Config\Definition\BaseNode->finalize() at /Users/sebastiaanstok/Sites/symfony-41/my_project/vendor/symfony/config/Definition/ArrayNode.php:248
 Symfony\Component\Config\Definition\ArrayNode->finalizeValue() at /Users/sebastiaanstok/Sites/symfony-41/my_project/vendor/symfony/config/Definition/BaseNode.php:413
 Symfony\Component\Config\Definition\BaseNode->finalize() at /Users/sebastiaanstok/Sites/symfony-41/my_project/vendor/symfony/config/Definition/ArrayNode.php:248
 Symfony\Component\Config\Definition\ArrayNode->finalizeValue() at /Users/sebastiaanstok/Sites/symfony-41/my_project/vendor/symfony/config/Definition/BaseNode.php:413
 Symfony\Component\Config\Definition\BaseNode->finalize() at /Users/sebastiaanstok/Sites/symfony-41/my_project/vendor/symfony/config/Definition/PrototypedArrayNode.php:195
 Symfony\Component\Config\Definition\PrototypedArrayNode->finalizeValue() at /Users/sebastiaanstok/Sites/symfony-41/my_project/vendor/symfony/config/Definition/BaseNode.php:413
 Symfony\Component\Config\Definition\BaseNode->finalize() at /Users/sebastiaanstok/Sites/symfony-41/my_project/vendor/symfony/config/Definition/ArrayNode.php:248
 Symfony\Component\Config\Definition\ArrayNode->finalizeValue() at /Users/sebastiaanstok/Sites/symfony-41/my_project/vendor/symfony/config/Definition/BaseNode.php:413
 Symfony\Component\Config\Definition\BaseNode->finalize() at /Users/sebastiaanstok/Sites/symfony-41/my_project/vendor/symfony/config/Definition/Processor.php:39
 Symfony\Component\Config\Definition\Processor->process() at /Users/sebastiaanstok/Sites/symfony-41/my_project/vendor/symfony/config/Definition/Processor.php:52
 Symfony\Component\Config\Definition\Processor->processConfiguration() at /Users/sebastiaanstok/Sites/symfony-41/my_project/vendor/symfony/dependency-injection/Compiler/ValidateEnvPlaceholdersPass.php:80
 Symfony\Component\DependencyInjection\Compiler\ValidateEnvPlaceholdersPass->process() at /Users/sebastiaanstok/Sites/symfony-41/my_project/vendor/symfony/dependency-injection/Compiler/Compiler.php:95
 Symfony\Component\DependencyInjection\Compiler\Compiler->compile() at /Users/sebastiaanstok/Sites/symfony-41/my_project/vendor/symfony/dependency-injection/ContainerBuilder.php:717
 Symfony\Component\DependencyInjection\ContainerBuilder->compile() at /Users/sebastiaanstok/Sites/symfony-41/my_project/vendor/symfony/http-kernel/Kernel.php:513
 Symfony\Component\HttpKernel\Kernel->initializeContainer() at /Users/sebastiaanstok/Sites/symfony-41/my_project/vendor/symfony/http-kernel/Kernel.php:125
 Symfony\Component\HttpKernel\Kernel->boot() at /Users/sebastiaanstok/Sites/symfony-41/my_project/vendor/symfony/framework-bundle/Console/Application.php:65
 Symfony\Bundle\FrameworkBundle\Console\Application->doRun() at /Users/sebastiaanstok/Sites/symfony-41/my_project/vendor/symfony/console/Application.php:145
 Symfony\Component\Console\Application->run() at /Users/sebastiaanstok/Sites/symfony-41/my_project/bin/console:39

To reproduce:

composer create-project symfony/website-skeleton my_project 
composer config minimum-stability dev
composer remove symfony/lts
composer update

Now update config/packages/security.yaml with:

security:
    # https://symfony.com/doc/current/security.html#where-do-users-come-from-user-providers
    providers:
        in_memory: { memory: ~ }
    firewalls:
        dev:
            pattern: ^/(_(profiler|wdt)|css|images|js)/
            security: false
        main:
            anonymous: true

            # activate different ways to authenticate

            # http_basic: true
            # https://symfony.com/doc/current/security.html#a-configuring-how-your-users-will-authenticate

            form_login: true
            # https://symfony.com/doc/current/security/form_login_setup.html
            
            remember_me:
                secret:               '%env(APP_SECRET)%' # This used to work
                token_provider:       ~
                catch_exceptions:     true
                name:                 MAIN_REMEMBERME
                lifetime:             604800 # one week

    # Easy way to control access for large sections of your site
    # Note: Only the *first* access control that matches will be used
    access_control:
        # - { path: ^/admin, roles: ROLE_ADMIN }
        # - { path: ^/profile, roles: ROLE_USER }

And run bin/console cache:clear.

I tried debugging the system, but somewhere along execution the Configuration Resolver looses the value. All env variables are present, there are placeholders, and framework.secret works without any issues but the firewall remember_me fails for some unknown reason 🤔
@xabbuh xabbuh added this to the 4.1 milestone Apr 3, 2018
@nicolas-grekas
Copy link
Member

ping @ro0NL

@ro0NL
Copy link
Contributor
ro0NL commented Apr 3, 2018 •
edited
Loading 
secret:               '%env(APP_SECRET)%' # This used to work

The question is would it work if APP_SECRET="", right now we test the config using an empty string (as envs can be empty strings). It conflicts with cannotBeEmpty().

symfony/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/RememberMeFactory.php

Lines 129 to 131 in e6302e2

$builder
->scalarNode('secret')->isRequired()->cannotBeEmpty()->end()
->scalarNode('token_provider')->end()

What removing the empty check pre-compile? And check emptyness during runtime?

Im not really convinced this is a bug, to me it's new 4.1 behavior really.

@nicolas-grekas
Copy link
Member

There will be more cases where cannotBeEmpty will choke. Why should we use empty values when checking for placeholders?

@ro0NL
Copy link
Contributor
ro0NL commented Apr 3, 2018

Why should we use empty values when checking for placeholders?

Because any env can be empty. Not sure what "non-empty" string to test for? Either way.. it still might end up being empty during runtime.

IMHO cannotBeEmpty conflicts with using envs which can be empty in fact.

Im still thinking about some other ways :)

@ro0NL
Copy link
Contributor
ro0NL commented Apr 3, 2018 •
edited
Loading

Maybe %env(required:SOME)% to validate for empty-ness on fetch, during runtime. For config we could then provide a non-empty string based on required prefix.

That would be in line with type validation, given you use bool: prefix for a bool node.

@nicolas-grekas
Copy link
Member

any env can be empty

that's not the most likely situation, so I would not target this as the default one...

IMHO cannotBeEmpty conflicts with using envs

it doesn't today, so accepting this statement means accepting a BC break, no-go

required:

All referenced envvars are already required. You mean non empty I suppose. Not sure it's needed. At some point, users are responsible for what they do.

So let's find a way to pass cannotBeEmpty.

@ro0NL
Copy link
Contributor
ro0NL commented Apr 3, 2018

it doesn't today

for the wrong reasons yes :)

You mean non empty I suppose.

Yes, but i thought required: would be most recognizable

EIther way.. im 👍 for something pragmatic also. Basically we suggest to skip empty-validation for placeholders right?

@sstok
Copy link
Contributor Author
sstok commented Apr 4, 2018

What about using %env(optional:APP_SECRET)% or something more elegant (default?) and keeping the other ones required.

@ro0NL
Copy link
Contributor
ro0NL commented Apr 4, 2018

I tend to prefer some explicit config too =/ technically should be something like:

secret: '%env(not_empty:APP_SECRET)%'

@nicolas-grekas
Copy link
Member

skip empty-validation for placeholders right

looks sensible to me

optional/not_empty/default

there is already a way to define default values for env placeholder.
and I wouldn't add complexity here: at some point, ppl are responsible for setting correct env vars.
+, this would mean accepting the BC break. no-go as said before.

@ro0NL ro0NL mentioned this issue Apr 4, 2018
Merged
@fabpot fabpot closed this as completed Apr 6, 2018
fabpot added a commit that referenced this issue Apr 6, 2018
@fabpot
bug #26799 [DI][Config] Fix empty env validation (ro0NL)
dbafc11 
This PR was merged into the 4.1-dev branch.

Discussion
----------

[DI][Config] Fix empty env validation

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #26747
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

<!--
Write a short README entry for your feature/bugfix here (replace this comment block.)
This will help people understand your PR and can be used as a start of the Doc PR.
Additionally:
 - Bug fixes must be submitted against the lowest branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the master branch.
-->

Commits
-------

d40a4f4 [DI][Config] Fix empty env validation
@ro0NL ro0NL mentioned this issue Apr 8, 2018
Closed
@ro0NL ro0NL mentioned this issue Oct 15, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug DependencyInjection Status: Needs Review
Projects
None yet
Milestone
4.1
Development

No branches or pull requests
6 participants
@fabpot @nicolas-grekas @sstok @ro0NL @xabbuh @carsonbot
0