Skip to content
This repository has been archived by the owner on Sep 15, 2020. It is now read-only.

add cors headers to ui/webserver fn responses #741

Merged
merged 2 commits into from
Jun 13, 2018

Conversation

neonphog
Copy link
Contributor

Adds Access-Control-Allow-Origin: * to outgoing response headers for /fn/ requests.

Notes

CORS is a tricky issue, since it's all client-side enforced, from a server/node perspective, there's really no reason not to allow all origins. Though really, it would probably be good if the host could configure their desired allow origins. Let me know if this is ok for now, or if you want me to add the configuration.

FYI, i need this header to support requests from the null origin. I.e. a web page hosted from file://.

Testing

Something in Hash is breaking for me on HEAD develop right now... but I am able to successfully run:

go test -v ./ui

@neonphog neonphog requested a review from zippy May 31, 2018 16:24
@zippy
Copy link
Member

zippy commented May 31, 2018

Hey, check this out, someone else did something on this too and I haven't worked on it: #635

@neonphog
Copy link
Contributor Author

Interesting, I don't think we need the allow-headers bit, at least MDN says Content-Type is always allowed: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Headers.

Per the other discussion on that PR, I'll go ahead and implement some configuration so the default can remain not adding the header.

@zippy
Copy link
Member

zippy commented May 31, 2018

Yah, I don't see much problem with this in any case.

@zippy zippy added the in-revew label Jun 1, 2018
@thedavidmeister
Copy link
Contributor

also in the future we could allow fns to accept and return headers in the request/response, as per #696

@zippy
Copy link
Member

zippy commented Jun 13, 2018

Any reason we can't just merge this @neonphog @thedavidmeister ?

@codecov-io
Copy link

Codecov Report

Merging #741 into develop will decrease coverage by 0.01%.
The diff coverage is 60%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #741      +/-   ##
===========================================
- Coverage     73.6%   73.59%   -0.02%     
===========================================
  Files           68       68              
  Lines        10557    10567      +10     
===========================================
+ Hits          7771     7777       +6     
- Misses        1945     1947       +2     
- Partials       841      843       +2
Impacted Files Coverage Δ
ui/webserver.go 52.29% <60%> (+0.46%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3797562...7215ad7. Read the comment docs.

@thedavidmeister thedavidmeister merged commit a960410 into holochain:develop Jun 13, 2018
@neonphog neonphog removed the in-revew label Jun 13, 2018
@thedavidmeister
Copy link
Contributor

@zippy maybe it's easier to just have reviewers merge if approved and tested?

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

Successfully merging this pull request may close these issues.

4 participants