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

Overwrite HTTP Host header #40

Closed

Conversation

georgemp
Copy link
Contributor

Currently, cpprestsdk ouputs the host header in the form of 'Host: hostname:port'

Example: Host : microsoft.com:443

Some servers have trouble handling the port number on the host header. It might be useful to allow overriding the host header (like curl does). This PR allows us to specify a host header in our request. If present, it will print that out on the http request. Else, it will default to the current host:port format

@msftclas
Copy link

Hi @georgemp, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!
We will now validate the agreement and then real humans will evaluate your PR.

TTYL, MSBOT;

@@ -57,6 +57,11 @@ static utility::string_t flatten_http_headers(const http_headers &headers)
utility::string_t flattened_headers;
for(auto iter = headers.begin(); iter != headers.end(); ++iter)
{
utility::string_t header_name = iter->first;
http_headers::_case_insensitive_cmp cmp;
if (!cmp(header_name, header_names::host) && !cmp(header_names::host, header_name)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, our windows http client implementation is in http_client_winhttp.cpp. We should make sure that we dont break anything there when modifying these common files.

@kavyako
Copy link
Contributor

kavyako commented Jan 2, 2016

I have added some comments. Please take a look. I can merge the changes once they are addressed.
Also, would really appreciate if you could add a unit test, Release\tests\functional\http\client\header_tests.cpp seems like the right place for that.

comparisons
2. Added test case to test overwriting the host header
@georgemp
Copy link
Contributor Author

georgemp commented Jan 4, 2016

Hi @kavyako ,
Thank you for taking a look at this. I didn't realize there was a direct function str_icmp to do case-insensitive string comparisons. I have switched the code to use that.

I have also added a unit test to handle testing (with and without overwriting the http_host_header).

Unfortunately, I don't have a windows machine to test against windows at the moment. I hope the changes will not affect windows clients. If there is anything else, you would like me to add, let me know

@kavyako
Copy link
Contributor

kavyako commented Jan 4, 2016

The flatten_http_headers() where we add all the headers to the flattened_headers is called by the windows client too. Since we are changing it to skip adding the HOST header, I think it will affect the windows implemenation.
I will have to check if this change is OK on windows (.i.e if WinHTTP also adds the header later ) or not. I will make the required changes for windows if necessary and merge the pull request. Thanks!

@kavyako kavyako self-assigned this Jan 4, 2016
@georgemp
Copy link
Contributor Author

georgemp commented Jan 5, 2016

Hi @kavyako
I believe cpprest did the following on OS X (prior to this pull request)

  1. Add host header with port in http_client_asio.cpp - start_request().
  2. Add user specified header in http_client_impl.cpp's flatten_http_headers. At this point, if the user had specified a host header, it would be duplicated on the request i.e the request would have 2 Host header lines

Since, we are now allowing the user to optionally supply a host header, I am suppressing the duplication that would have otherwise occurred in step 2 above (by just skipping user specified host headers as they are anyway always printed in step 1).

So, now on windows, I believe the following would apply

  1. Any user specified host header would be skipped.
  2. Host header would probably still get printed the same way it was prior to the patch (with or without the port number appended)

@kavyako
Copy link
Contributor

kavyako commented Jan 19, 2016

I verified the PR on windows and it does break windows clients. Any user specified host header will not be sent with the request, since it has been removed from flatten_http_headers.
One suggestion to fix this is to not modify flatten_http_headers. So we dont break other clients (windows here). And for the boost asio based clients, add the default Host header only if user has not specified one.
I can make these changes and merge the PR.

@kavyako
Copy link
Contributor

kavyako commented Jan 19, 2016

This has been merged to the development branch. Also fixed few build breaks on Windows.

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.

3 participants