-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add support for HTTP redirecti in ASIO-based http_client #1328
Add support for HTTP redirecti in ASIO-based http_client #1328
Conversation
57a975b
to
01764ab
Compare
Current implementation in this PR is influenced by whatwg/fetch#977.
Since this PR gives user code the control to disable automatic redirect (by |
I have attempted to prove this by adding the following snippet to if (client_config().max_redirects() == 0)
{
// Disable auto redirects
DWORD redirectPolicy = WINHTTP_OPTION_REDIRECT_POLICY_NEVER;
if (!WinHttpSetOption(winhttp_context->m_request_handle,
WINHTTP_OPTION_REDIRECT_POLICY,
&redirectPolicy,
sizeof(redirectPolicy)))
{
auto errorCode = GetLastError();
request->report_error(errorCode, build_error_msg(errorCode, "Setting redirect policcy"));
return;
}
// Note, using WINHTTP_OPTION_DISABLE_FEATURE with WINHTTP_DISABLE_REDIRECTS here doesn't seem to work
}
else
{
// Set max auto redirects
DWORD maxRedirects = (DWORD)client_config().max_redirects();
if (!WinHttpSetOption(winhttp_context->m_request_handle,
WINHTTP_OPTION_MAX_HTTP_AUTOMATIC_REDIRECTS,
&maxRedirects,
sizeof(maxRedirects)))
{
auto errorCode = GetLastError();
request->report_error(errorCode, build_error_msg(errorCode, "Setting max automatic redirects"));
return;
}
// (Dis)allow HTTPS to HTTP redirects
DWORD redirectPolicy = client_config().https_to_http_redirects()
? WINHTTP_OPTION_REDIRECT_POLICY_ALWAYS
: WINHTTP_OPTION_REDIRECT_POLICY_DISALLOW_HTTPS_TO_HTTP;
if (!WinHttpSetOption(winhttp_context->m_request_handle,
WINHTTP_OPTION_REDIRECT_POLICY,
&redirectPolicy,
sizeof(redirectPolicy)))
{
auto errorCode = GetLastError();
request->report_error(errorCode, build_error_msg(errorCode, "Setting redirect policcy"));
return;
}
} As noted in the comment, I found that Also, requesting a URL which requires more redirects than the |
By adding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would really like to see those test cases since there are a ton of extractable conditionals here; everything else are nitpicks. Thanks for your contribution, this is great!
If users are going to be relying on this for security reasons having some of the backends ignore it seems to be .... problematic :) |
I realised this needs reworking. If I hoist the whole thing outside of the user-code I'll come back... |
…the existing support in the WinHTTP-based build configuration
a598c39
to
6a4830b
Compare
@BillyONeal I've resolved the issue I noted and addressed some of your questions. If you have a chance to review commit 6a4830b, that'd be great. How do you think we should best surface After that I can either make the config options specific to the ASIO backend, or potentially make the WinHTTP additions to http_client_winhttp.cpp to obey the Thanks. |
…ps_to_http_redirects to the WinHTTP backend
Done. |
Now that this PR supports configurable HTTP redirection for WinHTTP as well as Boost.ASIO, I have added a whole new suite of functional test cases. These test that the most common scenarios behave similarly for both underlying implementations, and then document where differences are currently unavoidable. I think I'm done with this, thanks, @BillyONeal. |
Btw, this may be a can of worms, but why are the tests disabled for the Pipelines WinHttpPAL job? cpprestsdk/azure-pipelines.yml Lines 207 to 210 in b94bc32
|
…utside server https://http.badssl.com provided for this purpose, with a redirect from 'https' (port 443) to 'http' (port 80)
98e47db
to
14c272d
Compare
Because the owners of "WinHttpPAL" don't support or care about those cases. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make the suggestion change and merge
@@ -10,12 +10,21 @@ | |||
****/ | |||
|
|||
#include "stdafx.h" | |||
#ifdef _WIN32 | |||
#include <VersionHelpers.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure if this ought to be a compile-time or run-time check.
1301c32
to
550c5a4
Compare
Hmm, not sure what's going on here, why VersionHelpers.h doesn't compile, @BillyONeal? |
Thanks for your contribution! |
Please mention my name next time you need attention from winhttppal. The reason test suite is not fully enabled is that there are known problems with compression support. If there is a way to run the suite while suppressing some tests, please let me know. I can certainly post an update. |
@franksinankaya the cpprestsdk/Release/tests/common/TestRunner/test_runner.cpp Lines 36 to 39 in f4124a9
On the other hand, looking through the test suites, there are many instances of test cases being |
Ok, I will post a PR if this is the directory @BillyONeal wants to go. |
Or we could add exclusion support to the tool. |
HTTP redirect is currently supported by the WinHttp back-end, but has to be handled manually in user code when C++ REST SDK is built with the ASIO-based back-end. This kind of platform difference is frustrating for users.
Solving this issue has been requested several times, e.g. #222. A previous PR #373 was abandoned.
This PR enables us to pass the 'outside tests' that were identified as failing in #27, even after reverting both PR #499 (which worked around the problem by permitting
MovedPermanent
in the test) and another similar case from commit 4e19c0c.If this PR seems acceptable, it should be straightforward to use the new
http_client_config
options in thehttp_client_winhttp
implementation also, which would solve #171 more elegantly.Thoughts on how the headers sent with the redirect request are decided would be welcome!