Skip to content

added a check for x-host header on CDN use#31

Open
dnmitev wants to merge 2 commits intogreengerong:masterfrom
dnmitev:master
Open

added a check for x-host header on CDN use#31
dnmitev wants to merge 2 commits intogreengerong:masterfrom
dnmitev:master

Conversation

@dnmitev
Copy link

@dnmitev dnmitev commented Nov 10, 2017

We have found out that the ASP.NET MVC middleware is failling with wrong url when there is CDN, The X-Host header provided by the CDN was not taken into consideration so I have applied a check for that

@thoop
Copy link
Contributor

thoop commented Nov 10, 2017

Which CDN's use X-Host? I haven't seen that header added before.

@dnmitev
Copy link
Author

dnmitev commented Nov 10, 2017

@thoop we are using the Azure CDN and we have the problem there that the prerender.io is providing the url of the traffic manager instead of using the X-Host header which is the correct one. For an instance, if we have the address www.my-address.com which uses the my-traffic-manager.manager.com, the X-Host would containt "my-address.com" while the prerender.io would provide my-traffic-manager.manager.com. In other words we would have https://my-traffic-manager.com/users indexed instead of http://my-address.com/users

@thoop
Copy link
Contributor

thoop commented Nov 10, 2017

Ah, I see. Instead of using the X-Host header, would it be better to include an option for you to set in the middleware config to override the incoming Host to whatever you set in the middleware Host configuration?

That way everyone would be able to use it instead of just the people on Azure that have the X-Host header coming in. It's also a little more secure to use a config override instead of trusting the incoming Host when proxying.

@dnmitev
Copy link
Author

dnmitev commented Nov 18, 2017

@thoop I have applied the x-host header use from the config. Otherwise the x-host header is standard for CDNs as far as I know, so it's not just for Azure.

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