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

fixes #25 add basic CORS support #207

Merged
merged 3 commits into from Sep 13, 2015

Conversation

Projects
None yet
4 participants
@McFoggy
Copy link
Contributor

McFoggy commented Jul 8, 2015

Find embedded a small contribution to add a basic CORS support to NanoHttpd webserver.

I got inspired from:

@kasakara

This comment has been minimized.

Copy link

kasakara commented Jul 9, 2015

Thanks

Pubudu Kasakara

Sent from my iPhone

On 8 Jul 2015, at 8:09 pm, Matthieu Brouillard notifications@github.com wrote:

Find embedded a small contribution to add a basic CORS support to NanoHttpd webserver.

I got inspired from:

@kasakara answer in the comment
@AdamBien in his CORS project
You can view, comment on, or merge this pull request online at:

#207

Commit Summary

fixes #25 add basic CORS support
File Changes

M README.md (4)
M webserver/src/main/java/fi/iki/elonen/SimpleWebServer.java (65)
A webserver/src/test/java/fi/iki/elonen/AbstractTestHttpServer.java (67)
A webserver/src/test/java/fi/iki/elonen/TestCorsHttpServer.java (154)
M webserver/src/test/java/fi/iki/elonen/TestHttpServer.java (25)
Patch Links:

https://github.com/NanoHttpd/nanohttpd/pull/207.patch
https://github.com/NanoHttpd/nanohttpd/pull/207.diff

Reply to this email directly or view it on GitHub.

@elonen

This comment has been minimized.

Copy link
Member

elonen commented Jul 18, 2015

Thank you @McFoggy. Looks solid.
A question, though: would it be better to perhaps use String corsAllowedDomain instead of boolean useCORS in the SimpleWebServer() constructor, to support specifying allowed domain(s) without changing the code? And/or make addCORSHeaders(...) protected instead of private?

@McFoggy

This comment has been minimized.

Copy link
Contributor Author

McFoggy commented Jul 20, 2015

@elonen changing visibility from private to protected makes sense.
For the other proposition, what would you like to see? Instead of a boolean value that allows *, you'd like to receive a string parameter as program argument and set this value directly to the Access-Control-Allow-Origin?
I am not against it of course but this would allow to open only one domain to CORS (see this answer on SO).
Not sure what is the real target/usage of SimpleWebServer. Personally I use it in development environments not in production envrionements where my CORS logic is delegated to nginx/apache or servlet application filters if I need application logic in CORS handling.

@elonen

This comment has been minimized.

Copy link
Member

elonen commented Aug 8, 2015

I believe Access-Control-Allow-Origin can be either a single domain or a space separated list (see http://www.w3.org/TR/cors/#access-control-allow-origin-response-header), so a single parameter should do.

@McFoggy

This comment has been minimized.

Copy link
Contributor Author

McFoggy commented Aug 9, 2015

@elonen find the enhancements to allow to define origin. If you want I can rebase the branch.

@elonen

This comment has been minimized.

Copy link
Member

elonen commented Aug 9, 2015

Very nice. No need to rebase, IMO.
Any objections to merging this @ritchieGitHub ?

@kasakara

This comment has been minimized.

Copy link

kasakara commented Aug 9, 2015

Should be ok.

Sent from my iPhone

On 9 Aug 2015, at 3:58 pm, Matthieu Brouillard notifications@github.com wrote:

@elonen find the enhancements to allow to define origin. If you want I can rebase the branch.


Reply to this email directly or view it on GitHub.

@ritchieGitHub

This comment has been minimized.

Copy link
Member

ritchieGitHub commented Sep 13, 2015

no this seems is a good pul request (delivers unit tests ;-) )

ritchieGitHub added a commit that referenced this pull request Sep 13, 2015

Merge pull request #207 from McFoggy/cors-support
fixes #25 add basic CORS support

@ritchieGitHub ritchieGitHub merged commit 4a3649d into NanoHttpd:master Sep 13, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 74.951%
Details

@McFoggy McFoggy deleted the McFoggy:cors-support branch Sep 13, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.