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: add config to specify CORS origins #2986

Merged
merged 2 commits into from
Oct 24, 2023

Conversation

taimoorzaeem
Copy link
Collaborator

This should fix #2441.

src/PostgREST/Response.hs Outdated Show resolved Hide resolved
test/io/test_io.py Outdated Show resolved Hide resolved
test/io/test_io.py Outdated Show resolved Hide resolved
test/io/test_io.py Outdated Show resolved Hide resolved
test/io/test_io.py Outdated Show resolved Hide resolved
@taimoorzaeem taimoorzaeem force-pushed the config/cors branch 2 times, most recently from 20b6af8 to 3fa6ab5 Compare October 23, 2023 09:49
@taimoorzaeem
Copy link
Collaborator Author

@laurenceisla

With config's type as Maybe [Text] if the config is not specified by the user then it would evaluate to Nothing. However if the user specifies this config as cors-allowed-origins = "" it would evaluate to Just [].

Could this be a problem? If

{ Wai.corsOrigins = Just ([], True)

and

{ Wai.corsOrigins = Nothing

both return Access-Control-Allow-Origin: * then it should be fine I think. WDYT?

Copy link
Member

@laurenceisla laurenceisla left a comment

Choose a reason for hiding this comment

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

both return Access-Control-Allow-Origin: * then it should be fine I think. WDYT?

Yes, I agree. Not setting anything or leaving it empty means "*" (any origin allowed). This also needs to be documented to avoid any confusion.

Edit: Ah, I just noticed that cors-allowed-origins = "" translates to Just ([""], True), not Just ([], True), so any preflight and CORS will be invalidated. I guess it makes sense, other configurations behave the same way when they are present but empty (e.g. db-extra-search-path). Again, needs to be documented. No need to change anything, you get that result due to the problem with the OPTIONS implementation that I mention below.


LGTM! Good job! The only thing that is missing is to tidy up the OPTIONS requests to better use the middleware. But I believe that is a bigger problem with the implementation of OPTIONS itself, so it's outside of the scope of this PR.

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Steve Chavez <[email protected]>
@steve-chavez
Copy link
Member

@taimoorzaeem Awesome work! 🚀

You can document it on https://postgrest.org/en/stable/references/api/cors.html

@steve-chavez steve-chavez merged commit d94286c into PostgREST:main Oct 24, 2023
31 checks passed
@taimoorzaeem taimoorzaeem deleted the config/cors branch October 25, 2023 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Is there a way to setup CORS origins? Nginx config overwritten by postgrest
3 participants