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

added check for runtime env vars in config #111

Closed krcrawford closed 4 years ago

krcrawford commented 5 years ago

Firstly, I understand the CORS concept and I'm not using the origin wildcard in production, but this PR should fix some unexpected behavior.

In my experience, and perhaps I may have been approaching a problem incorrectly, Symfony's environment vars are not yet resolved when the NelmioCorsExtension is reached.

If I have:

allow_origin: ['%env(CORS_ALLOW_ORIGIN)%']

in my config, Symfony populates this with a temp env var, not the real value but a placeholder, initially. Looks like the extension beats Symfony to parsing that value, i.e. the env var is not yet populated with its true value.

Consequently, the NelmioCorsExtension doesn't read the * that I have set in my .env, and skips over the following due to a negative string match, i.e. env_XXXXXX_CORS_ALLOW_ORIGIN_XXXXX_env != '*':

$options['allow_origin'] = true`;

The env var value may work as expected for other values, but it's not picking up the swap * for true in the extension.

The PR does more of a runtime check against what should be a boolean, and picks up where the extension lost the value.

Feel free to enlighten me, or merge this PR.

a-melnikov commented 5 years ago

Hi! Is any plan (road map) to merge it in the nearest future?

nicolas-grekas commented 4 years ago

Why using an env var here, while a parameter would do the job?

krcrawford commented 4 years ago

@nicolas-grekas Well then the recipe would need to change instead of this, correct? https://github.com/symfony/recipes/blob/master/nelmio/cors-bundle/1.5/config/packages/nelmio_cors.yaml

nicolas-grekas commented 4 years ago

@dunglas explained me why env vars are legit here so all good to move this forward on my side.

Seldaek commented 4 years ago

@krcrawford do you have a sec to also update allow_headers like @dunglas mentioned?