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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

gbuisson
Copy link
Contributor

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
Copy link
Owner

r0man commented Jan 13, 2017

@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
Copy link
Contributor Author

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 Host header 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
Copy link
Owner

r0man commented Jan 13, 2017

@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.

@gbuisson gbuisson closed this May 10, 2021
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.

Only Trigger the middleware on Origin/Host mismatch
2 participants