Skip to content

Conversation

vadz
Copy link
Contributor

@vadz vadz commented Jul 8, 2017

This is, IMO, a pretty important, even if simple, change as it makes applications C++ REST SDK to work out of the box on many corporate networks where they currently don't work by default and, if a fixed PAC URL is used, can't be made to work at all (see #182). With this change, they should work correctly in all cases, provided IE Internet access settings are configured correctly which is, IMO, always the case (at least if Internet access is allowed at all).

Please notice that this PR includes the commit already submitted as #495 as it reuses a struct declared there, so if you apply this PR entirely, you can close #495.

ashwinravianandan and others added 7 commits May 26, 2017 23:59
…cause this reason we now using the WINHTTP_ACCESS_TYPE_AUTOMATIC_PROXY flag for proxy settings
Ensure that the strings inside this struct are always freed, which wasn't the
case before.
Ensure that the strings inside this struct are always freed, which wasn't the
case before.
Implement the recommended behaviour for "well-written WinHTTP apps", see
https://blogs.msdn.microsoft.com/ieinternals/2013/10/11/understanding-web-proxy-configuration/

I.e. use IE proxy settings for the current user unless WinHTTP proxy is
explicitly defined (which seems to be quite rare in practice). This makes
applications using C++ REST SDK work out of the box on the networks where IE
is configured to access the Internet (which will almost always be the case),
whether via a fixed proxy or using proxy-configuration, using either WPAD or
fixed PAC URL (this fixes microsoft#182
in passing).
@msftclas
Copy link

msftclas commented Jul 8, 2017

@vadz,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

vadz and others added 9 commits July 9, 2017 01:31
Adding a return statement after __assume(0) resulted in "warning C4702:
unreachable code" when building with MSVC, which broke the build as warnings
are fatal due to the use of /WX options.

Fix this by using this return statement only with the Intel compiler, which
apparently needs it (see e3f81c4).
Fix unreachable statement warning from MSVC after ICC warning fix
There is no message body for 1xx, 203 and 304 HTTP responses (see
section 3.3.3/1 of RFC 7230), yet we tried to read it nevertheless when
using ASIO backend, resulting in "Failed to read response body"
exception being thrown.

Fix this by explicitly skipping reading the body for these status codes.
Avoid spurious errors when handling HTTP responses without body
- http://www.cnn.com redirects users from countries outside of the US
  to the "http://edition.cnn.com/" drop location
- Use http://edition.cnn.com, which does not redirect - even in the US
- Fix microsoft#27
@xqp
Copy link
Contributor

xqp commented Jul 12, 2017

ah great.

i have a similar pr for this (#500).
However, I like yours very well. Maybe you can check if the os version is greater 8.1 and then you can use the WINHTTP_ACCESS_TYPE_AUTOMATIC_PROXY flag to discover the proxy automatically.

👍

- http://www.google.com redirects country specifically
- Use http://code.google.com, which does not redirect (so far)
@vadz
Copy link
Contributor Author

vadz commented Jul 12, 2017

Thanks, I didn't know about WINHTTP_ACCESS_TYPE_AUTOMATIC_PROXY and it indeed looks like the right way to do it under 8.1+. However I'm not sure if this flag does anything different from what my patch does manually and as I need this to work under Windows 7 too anyhow, I have to keep this code. So the question becomes whether it's still worth using the flag and make the code more complicated or if we can just use the same code under all Windows versions?

@xqp
Copy link
Contributor

xqp commented Jul 12, 2017

Thanks for your answer :)

Unfortunately you call 'WinHttpOpen' with a depricated flag on Win8.1 and greater.
See the table from dwAccessType :
WINHTTP_ACCESS_TYPE_DEFAULT_PROXY
https://msdn.microsoft.com/library/windows/desktop/aa384098(v=vs.85).aspx

@vadz
Copy link
Contributor Author

vadz commented Jul 12, 2017

Well, the fact that it's deprecated doesn't really mean anything. It definitely still works (I've tested under Windows 10) and it's very unlikely to stop working in the observable future.

Of course, in (very) long term we should just use WINHTTP_ACCESS_TYPE_AUTOMATIC_PROXY, but if I'm correct in that it doesn't do anything more than my manual code (this is the important question, really), then I'd prefer to switch to it once we can drop support for Windows < 8.1 entirely and replace the code in my patch with it, instead of having both this code and the flag.

ras0219-msft and others added 7 commits July 12, 2017 14:37
- Do not include xlocale.h on systems, where __GLIBC__ is defined
  xlocale.h has been removed from glibc 2.26
  The include of locale.h in asyncrt_utils.h is sufficient
  Further details:
  https://sourceware.org/git/?p=glibc.git;a=commit;h=f0be25b
- Fixes microsoft#485
@reneme
Copy link
Contributor

reneme commented Jul 21, 2017

Thanks for your pull request! We (@xqp and me) tried the patch in our target environment and it worked fine finding the right proxy configuration!

There was an issue though, that is related to WinHTTP:
When using HTTPS, WinHTTP needs to send an HTTP CONNECT request to the proxy. This message is not encrypted and readable by the proxy. Our target environment uses the User-Agent: header in this HTTP message to decide if traffic should go out or not. Unfortunately WinHTTP doesn't send this header by default and we didn't find a way to set it.

Apparently setting the user agent in the WinHttpOpen() call used to work but doesn't anymore.

Maybe you happen to know what could be a workaround for this?

@vadz
Copy link
Contributor Author

vadz commented Jul 21, 2017

@reneme Sorry, I don't know anything about this, I didn't test with a proxy performing User-Agent checks (why would anybody do that!?) and if the Microsoft person at the linked answer says that there is no workaround, I'm inclined to believe him... Perhaps you can use ASIO backend under Windows instead of WinHTTP one? Of course, you wouldn't be able to automatically use IE proxy with it then.

Patrik Fiedler and others added 22 commits August 3, 2017 14:30
Fixed proxy credentials for http_client_asio
FIX Build error on Ubuntu Xenial when using OpenSSH 1.1.0b
…_by_empty_string

Fix/win32 encryption throws by empty string
Adding 308 "Permanent Redirect" to http_constants.dat
Add support for retrieving remote address in HTTP listener
Fix memory leak of WINHTTP_PROXY_INFO fields
…_xlocale

Fix build error with glibc 2.26, xlocale.h
…revocation-pr

enable certificate revocation check with winhttp
@ras0219-msft ras0219-msft merged commit fc20c17 into microsoft:master Aug 26, 2017
@ras0219-msft
Copy link
Contributor

Thanks for the PR! I've merged this with #500, so the general proxy detection logic should be used both when the client is set to "Default" as well as "Auto Configure".

@vadz vadz deleted the use-ie-proxy-by-default branch September 7, 2017 23:41
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.