issue 15: compare Host and Origin to check if req is xdomain

r0man / ring-cors

Ring middleware for Cross-Origin Resource Sharing.

http://github.com/r0man/ring-cors

166 stars 44 forks source link

issue 15: compare Host and Origin to check if req is xdomain #17

Closed gbuisson closed 3 years ago

gbuisson commented 7 years ago

closes #15

This changes the xdomain request detection from only looking for an Origin Header to comparing Origin and Host headers.

I'm not certain this is a reliable way to do it, there is a discussion on stackoverflow http://stackoverflow.com/questions/14444914/origin-and-host-headers-for-same-domain-requests proposing to check for the X-Requested-With header instead so I would suggest that you double check before merging it ;-)

r0man commented 7 years ago

@gbuisson This looks good, but I have 2 questions.

Does this handle the default HTTP and HTTPS ports? Like what happens if the Host header is "example.com:80" and the Origin is "http://example.com/"? Is this treated as same origin or a cross orgin request? How should this be handled.
Could you add a line to the README and explain how this works. That would be nice.

Thanks, Roman.

gbuisson commented 7 years ago

So the RFC stipulates that the port shall be added to the Host header only if it's not standard (80 for HTTP, 443 for HTTPS), so a Hostheader value looking like foo.com:80is not standard. That being said I only tested it with Chrome and Firefox, some other browsers could show a different behavior.

https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.23

I will definitely add a description the README.

I think we should let that PR live some time and test it on the field as it's not critical. However, maybe we should improve the tests, for instance instead of testing handcrafted ring requests with maps, I would spawn an instrumented HTTP server loaded with the middleware and simulate real XMLHTTPRequests with all the headers etc.

r0man commented 7 years ago

@gbuisson Thanks for the explanation. If you want to add integration tests I'm all for it, but I don't want to require it. It's up to you! And yes, I think we should test it out for a while before cutting a release. So give me a go, when you think I should merge it and I'll publish it as a snapshot.