nelmio / NelmioCorsBundle

Adds CORS (Cross-Origin Resource Sharing) headers support in your Symfony application
https://symfony.com/bundles/NelmioCorsBundle/
MIT License
1.89k stars 108 forks source link

fix: correct interpretation of environment variables #187

Closed fldv closed 4 months ago

fldv commented 1 year ago

There is a problem with the interpretation of environment variables in the current system.

When we want to add an environment variable in the configuration file currently like this:

        '^/v1/':
            allow_origin: ['%env(CORS_ALLOW_ORIGIN)%']
            allow_headers: ['%env(CORS_ALLOW_HEADERS)%']
            allow_methods: ['%env(CORS_ALLOW_METHOD_ONE)%', '%env(CORS_ALLOW_METHOD_TWO)%']

Only the "allow_origin" key works because there is no processing done behind the user's back.

On the other keys, a treatment is applied (putting everything in upper or lower case).

Only when you want to add an environment variable, this is automatically managed by Symfony in the form of a hash.

However, when these modifications are made to our values (which are actually environment variable keys) this modifies the key and Symfony no longer finds the associated value.

For example, our "%env(CORS_ALLOW_HEADERS)%" key becomes "env_588d27cb6d976748_string_CORS_ALLOW_ORIGIN_717607a427d05ca1694bd72e43dce0ea" at the time of interpretation by Symfony, but is later transformed by the bundle into "env_588d27cb6d976748_string_cors_allow_origin_717607a427d05ca1694bd72e43dce0ea" (all lowercase). And the Symfony, no longer knows how to return the value of this key.

The idea to resolve the problem and to be able to use environment variables everywhere, is to test if they can be solved with the "resolveEnvPlaceholders" function of Symfony. If so, we leave as is, otherwise we add the filter.

Titoine commented 10 months ago

This problem started to occur after migrating from Symfony 6.1 -> 6.2. Thanks for the fix.

Seldaek commented 10 months ago

This may fix the problem as long as the env vars' value have the correct case, but if not things will break IMO, so we need to move the strtolower to another location and do it at runtime and not in the extension I'd say..

Seldaek commented 4 months ago

Closing as duplicate of #188 which fixes this in a better way IMO