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

feat: allow configuration of cors domains and credentials #159

Merged
merged 7 commits into from
Jun 27, 2023

Conversation

jhleao
Copy link
Contributor

@jhleao jhleao commented May 26, 2023

Description

Problem

For applications that use cookie-based auth, CORS domain wildcards are not allowed. Need to specify explicit domains instead. Plus, Access-Control-Allow-Credentials must be set to true.

Solution

This makes CORS domains optionally configurable via CORS_ALLOW_ORIGINS, same for Access-Control-Allow-Credentials via CORS_ALLOW_CREDENTIALS. If no configuration is specified, current behavior is kept.

@dbarrosop
Copy link
Member

dbarrosop commented May 28, 2023

Thanks for the PR. However, we need to slightly change the implementation:

  1. Instead or reading directly from env vars, use command flags (i.e. cors-allow-origins), defaulting to the current value *.
  2. Pass the value of the resolved flags from cmd down to SetupRouter

Thanks again for the contribution.

@dbarrosop
Copy link
Member

Thanks! Looks good to me but tests need to be fixed, I think you just need to pass the new parameters to SetupRouter in the tests that use it.

@jhleao
Copy link
Contributor Author

jhleao commented Jun 27, 2023

@dbarrosop I couldn't get these tests to pass locally... But I believe the problem to be on my dev environment. Can you trigger CI again?

@dbarrosop
Copy link
Member

Nice! Thanks a lot for the contribution.

@dbarrosop dbarrosop merged commit f32b784 into nhost:main Jun 27, 2023
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.

3 participants