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

Adds CORS possibility to nginx #14

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

petergallagher
Copy link

defaults to * for vagrant and ansible_host otherwise

@martijnvermaat
Copy link
Contributor

Hi Peter, thanks for the PR, I think it makes sense to add this!

I would however just use the existing variable mutalyzer_server_name since that is supposed to be the domain name for accessing the website and webservices. ansible_host is what is used by Ansible to connect to the machine, which can be very different.

@petergallagher
Copy link
Author

Ah yes, I see, that does make more sense. I've updated the PR.

@martijnvermaat
Copy link
Contributor

I'm sorry, I was confused. The Access-Control-Allow-Origin header should contain the origin (domain plus scheme) that is allowed to access the resource. It does not make sense to set it to the host that is serving the resource.

In case of Mutalyzer, non of the API responses are personalized (there is no authentication of any kind), so I would say it is safe (and easiest) to always set it to *. Also, if it is set to anything other than *, Mutalyzer should also include Origin in the Vary response header (see MDN).

In other words, if we want to enable CORS with the Mutalyzer JSON API, I think it's best to just include this header with a non-configurable value of *.

@petergallagher
Copy link
Author

Sorry for the delay in replying. Ultimately it's up to you, but I wouldn't recommend forcing the CORS header to be *. It could be alright for a default, but I was looking in to deploying the API and using it alongside another piece of software and for that I would want to be able to configure the headers differently for development and production servers at least.

@martijnvermaat
Copy link
Contributor

Well, the point of the same-origin policy is to not give third parties access to resources on the user's behalf (i.e., using it's cookies, HTTP auth, etc). This is irrelevant for resources that behave the same for all users (i.e., don't have any form of sessions). For those kind of resources, the * CORS origin is appropriate (and cookies etc are actually not even sent when using *).

This applies to the Mutalyzer JSON webservice. It has no state, no user sessions. The CORS headers are specific for the resource (path), so even if you host other resources on the same domain which do have user sessions, this should not be an issue.

One reason I can see to not implement this is the possibility for Mutalyzer to get sessions in the future. During development of such a feature, one could forget to change the * CORS allowed origin.

Regardless, I think the PR makes sense and could definitely be useful to people. I'm not the Mutalyzer maintainer anymore, so I'll leave it at that ;)

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.

2 participants