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

nelmio/cors-bundle: update CORS_ALLOW_ORIGIN #487

Closed
wants to merge 5 commits into from
Closed

nelmio/cors-bundle: update CORS_ALLOW_ORIGIN #487

wants to merge 5 commits into from

Conversation

ro0NL
Copy link
Contributor

@ro0NL ro0NL commented Nov 10, 2018

Q A
License MIT

fixes

.env: line 48: syntax error near unexpected token `('
.env: line 48: `CORS_ALLOW_ORIGIN=^https?://localhost(:[0-9]+)?$'

with source .env

after

echo $CORS_ALLOW_ORIGIN
^https?://localhost(:[0-9]+)?$

let me know if im missing something 🤔

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

@ro0NL
Copy link
Contributor Author

ro0NL commented Nov 10, 2018

see https://3v4l.org/rP8Cj

$ FOO=^https?://localhost\(:[0-9]+\)?$
$ echo $FOO
^https?://localhost(:[0-9]+)?$

@ro0NL
Copy link
Contributor Author

ro0NL commented Nov 10, 2018

/cc @dunglas ?

@nicolas-grekas
Copy link
Member

Use single quotes around the value instead?

@ro0NL
Copy link
Contributor Author

ro0NL commented Nov 11, 2018

but it's not generated like that, so default code is invalid currently.

@ro0NL
Copy link
Contributor Author

ro0NL commented Nov 11, 2018

wait, we can put the single quotes in the recipe yes. Not sure we prefer one or another actually..

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

@ro0NL
Copy link
Contributor Author

ro0NL commented Nov 11, 2018

works 👌

Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

Can you check if it works with Docker Compose too please?

@ro0NL
Copy link
Contributor Author

ro0NL commented Nov 11, 2018

Hm, not sure actually. Im using https://github.com/lando/lando and i see it generates the following

version: '3.2'
services:
  appserver:
    image: 'devwithlando/php:7.1-fpm'
    environment:
      TERM: xterm
      COMPOSER_ALLOW_SUPERUSER: 1
      PATH: >-
        /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/var/www/.composer/vendor/bin
      LANDO_WEBROOT: /app/public
      LANDO_SERVICE_NAME: appserver
      LANDO_SERVICE_TYPE: php
      LANDO_MOUNT: /app
      COLUMNS: 256
      LANDO: 'ON'
      LANDO_CONFIG_DIR: $LANDO_ENGINE_CONF
      LANDO_DOMAIN: lndo.site
      LANDO_APP_NAME: myproject
      LANDO_APP_ROOT: /home/roland/dev/my-project
      LANDO_APP_ROOT_BIND: /home/roland/dev/my-project
      LANDO_HOST_OS: linux
      LANDO_HOST_UID: '1001'
      LANDO_HOST_GID: '1001'
      LANDO_HOST_IP: 192.168.178.18
      LANDO_WEBROOT_USER: www-data
      LANDO_WEBROOT_GROUP: www-data
      LANDO_WEBROOT_UID: '33'
      LANDO_WEBROOT_GID: '33'
      APP_ENV: dev
      APP_SECRET: c017ebf223052640d2eb426b8cb0d35c
      CORS_ALLOW_ORIGIN: '^https?://localhost(:[0-9]+)?$'

Starting it already screams:

ERROR: Invalid interpolation format for "environment" option in service "appserver": "^https?://localhost(:[0-9]+)?$"

But other values are also wrapped in single quotes 🤔

echo $CORS_ALLOW_ORIGIN in the container gives an empty string now :(

source .env in the container works as expected though,

With single quotes it seems the $ needs double escaping: $$. But then it differs with sourcing manually.

@nicolas-grekas
Copy link
Member

There's only one valid and supported way to parse a .env file, it's with a shell - ie source .env.
If other tools didn't implement proper parsing rules, that's their issue IMHO.

@ro0NL
Copy link
Contributor Author

ro0NL commented Nov 11, 2018

Tend to agree.. not sure if this is tied to Docker or Lando though.

So @dunglas can you confirm it still works for you? I can only say sourcing the current value doesnt.

@ro0NL
Copy link
Contributor Author

ro0NL commented Nov 11, 2018

$ docker run -it --rm --env-file .env bash:4.4
bash-4.4# echo $CORS_ALLOW_ORIGIN
'^https?://localhost(:[0-9]+)?$'

That's wrong at least.

@nicolas-grekas
Copy link
Member

Does \ escaping work better?

@ro0NL
Copy link
Contributor Author

ro0NL commented Nov 11, 2018

Not really :(

$ source .env
$ echo $CORS_ALLOW_ORIGIN
^https?://localhost(:[0-9]+)?$
$ docker run -it --rm --env-file .env bash:4.4
bash-4.4# echo $CORS_ALLOW_ORIGIN
^https?://localhost\(:[0-9]+\)?$

@ro0NL
Copy link
Contributor Author

ro0NL commented Nov 11, 2018

Forwarding env does work 🤷‍♂️

export CORS_ALLOW_ORIGIN
$ echo $CORS_ALLOW_ORIGIN
^https?://localhost(:[0-9]+)?$
$ docker run -it --rm --env CORS_ALLOW_ORIGIN bash:4.4
bash-4.4# echo $CORS_ALLOW_ORIGIN
^https?://localhost(:[0-9]+)?$

@nicolas-grekas
Copy link
Member

Sure: the issue is the .env parser they use. Once the env var is set, it is not parsed anymore.

@ro0NL
Copy link
Contributor Author

ro0NL commented Nov 11, 2018

Think so, per https://docs.docker.com/engine/reference/commandline/run/#set-environment-variables--e---env---env-file

to set simple (non-array) environment variables

I think "simple" is key here...

@ro0NL
Copy link
Contributor Author

ro0NL commented Nov 11, 2018

To move forward.. big 👍 for being compatible with sourcing manually.

We can keep the default in the recipe "simple" maybe, to serve both users.

More structurally, perhaps clarify this with a comment on top in the generated env files. Mentioning the escaping / docker traps.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 11, 2018

I'm wondering: how do rails' and nodejs' dotenv parse .env files? Do they all parse shell-like or are we the only ones?

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 11, 2018

It looks like both nodejs and rails support single and double quoted strings:

@ro0NL
Copy link
Contributor Author

ro0NL commented Jun 26, 2019

i've concluded sharing .env between different concepts (Symfony and Docker) is a bad practice, or impractical at least.

For anyone coming here from google&co, i advise to move docker-compose out of the project root to a dedicated scoped devops/docker folder.

Closing as such.

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

Successfully merging this pull request may close these issues.

None yet

3 participants