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

CORS added for cross-site requests and readme updated #398

Merged
merged 5 commits into from
Apr 5, 2019

Conversation

simonpoole
Copy link
Contributor

@simonpoole simonpoole commented Apr 4, 2019

This is based on #373 and preserves rahulhaque commits but simplifies things a tiny bit and rebases it on the current master.

Currently the -cors flag is all or nothing, but I would expect that people running photon that want more fine grained control would put it behind a reverse proxy in any case.

It doesn't resolve the mystery of why the previous code that added a Access-Control-Allow-Origin header seems to have worked for some (for example myself) and not for others.

Copy link
Collaborator

@lonvia lonvia left a comment

Choose a reason for hiding this comment

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

I tend to agree that while we are at it, we should allow to set a custom origin. Not sure if it is better to make --cors a string parameter or add an additional --cors-origin parameter. The latter has the slight advantage that the default case '*' is shorter to type (in particular as I see a lot of people failing to hand in an asterisk as paramter.

src/main/java/de/komoot/photon/App.java Show resolved Hide resolved
@lonvia
Copy link
Collaborator

lonvia commented Apr 5, 2019

I suspect that the difference if the old version works or not is if the caller does an OPTION request first.

@simonpoole
Copy link
Contributor Author

simonpoole commented Apr 5, 2019

As a tendency I would prefer -cors-all without an argument and -cors (or -cors-origin) with one.

Added in 268e13a actually used -cors-any and -cors-origin as this makes things clearer IMHO.

@simonpoole
Copy link
Contributor Author

I suspect that the difference if the old version works or not is if the caller does an OPTION request first.

I assume so too, I just couldn't recreate with any of the (very mainstream and current) browsers I use.

Copy link
Collaborator

@lonvia lonvia left a comment

Choose a reason for hiding this comment

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

Looks good to me. @karussell, any objections?

@karussell
Copy link
Collaborator

This looks good. (Although I would not expose photon to the public and instead use nginx or similar tools to proxy it for unrelated security reasons)

@prasadrajuv
Copy link

add https (ssl) support to avoid Blocked loading mixed active content in browser because geolocation support removed for http protocol

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.

5 participants