-
Notifications
You must be signed in to change notification settings - Fork 479
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
Add flag to control CORS Origin header #504
Conversation
header.Set("Access-Control-Allow-Origin", origin) | ||
header.Set("Vary", "Origin") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just for correctness ( https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Origin#cors_and_caching )
Thank you very much for this PR, but I have one question: Is it not necessary to validate the the Origin header in the request matches |
The most simplistic setup on apache/nginx is also just adding the origin with no further logic applied. I'd say this is okay to go for the very first iteration. refs https://ubiq.co/tech-blog/enable-cors-apache-web-server/ (I just grabbed to first hit on google search 🙈 ) @sean9999 @Acconut is there anything I can help out with to get this PR going? THX a lot |
Yea adding this would be nice.. For now I use |
You're right. I checked the spec, and a CORS compliant server is supposed to refuse a connection or return an error in that case. I'll try adding that in. https://www.w3.org/TR/2020/SPSD-cors-20200602/#resource-requests |
Sounds good! After that, I think we can merge this :) |
Took the freedom to rebase this PR -> #942 |
For anyone interested. This would be the last point that needs to be done. If this check is implement, we can move towards merging such a feature :) |
can be closed as well - merged as part of #987 |
This is superseeded by #997. Thank you for all your help! |
I added a flag to allow the user to set their own Access-Control-Allow-Origin header. I feel it's proper to be able to set this header because CORS was designed with this use-case in mind. You need to be able explicitly say what origin can access your API. Using a reverse-proxy felt kludgy, because the app already sets CORS headers. It is not offloading that responsibility. This resolves #135 and #386.
I developed this for personal use because I am running tusd in a Kubernetes environment, and because of the way k8s handles network routing via Ingress controller, setting the proper Origin header was infeasible (even with --behind-proxy turned on). Using a reverse proxy felt wrong because k8s already does a lot of network indirection.
I hope you find this useful