[PATCH 0/1] MR3725: urlmon: fix HTTP redirects when auto redirection is disabled
If DISABLEAUTOREDIRECTS is set in BINDINFO options, urlmon does not set HTTP verb to GET when handling redirections. Although HTTP specification is vague on the correct behaviour here, many web servers expect this. This is what's causing the "400 Bad Request" error when user tries to log into GMail accounts using Outlook. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3725
From: Yuxuan Shui <yshui(a)codeweavers.com> If DISABLEAUTOREDIRECTS is set in BINDINFO options, urlmon does not set HTTP verb to GET when handling redirections. Although HTTP specification is vague on the correct behaviour here, many web servers expect this. This is what's causing the "400 Bad Request" error when user tries to log into GMail accounts using Outlook. --- dlls/urlmon/bindprot.c | 13 +++++++++++++ dlls/urlmon/urlmon_main.h | 1 + 2 files changed, 14 insertions(+) diff --git a/dlls/urlmon/bindprot.c b/dlls/urlmon/bindprot.c index b09acdd5c3a..d3543de876f 100644 --- a/dlls/urlmon/bindprot.c +++ b/dlls/urlmon/bindprot.c @@ -18,6 +18,7 @@ #include "urlmon_main.h" #include "wine/debug.h" +#include "wininet.h" WINE_DEFAULT_DEBUG_CHANNEL(urlmon); @@ -1014,6 +1015,7 @@ static HRESULT WINAPI ProtocolSinkHandler_ReportData(IInternetProtocolSink *ifac static HRESULT handle_redirect(BindProtocol *This, const WCHAR *url) { HRESULT hres; + IWinInetHttpInfo *http_info = NULL; if(This->redirect_callback) { VARIANT_BOOL cancel = VARIANT_FALSE; @@ -1028,6 +1030,14 @@ static HRESULT handle_redirect(BindProtocol *This, const WCHAR *url) return hres; } + if (This->protocol_unk && IUnknown_QueryInterface(This->protocol_unk, &IID_IWinInetHttpInfo, (void **)&http_info) == S_OK) { + DWORD status_code = 0, size = sizeof(DWORD); + hres = IWinInetHttpInfo_QueryInfo(http_info, HTTP_QUERY_STATUS_CODE | HTTP_QUERY_FLAG_NUMBER, + &status_code, &size, NULL, NULL); + if (hres == S_OK && status_code != HTTP_STATUS_REDIRECT_KEEP_VERB) + This->redirect_override_verb = TRUE; + } + IInternetProtocol_Terminate(This->protocol, 0); /* should this be done in StartEx? */ release_protocol_handler(This); @@ -1101,6 +1111,9 @@ static HRESULT WINAPI BindInfo_GetBindInfo(IInternetBindInfo *iface, return hres; } + if (This->redirect_override_verb) + pbindinfo->dwBindVerb = BINDVERB_GET; + if((pbindinfo->dwOptions & BINDINFO_OPTIONS_DISABLEAUTOREDIRECTS) && !This->redirect_callback) { IServiceProvider *service_provider; diff --git a/dlls/urlmon/urlmon_main.h b/dlls/urlmon/urlmon_main.h index 81b0d629f53..c2e3ffe7057 100644 --- a/dlls/urlmon/urlmon_main.h +++ b/dlls/urlmon/urlmon_main.h @@ -192,6 +192,7 @@ typedef struct { BOOL reported_result; BOOL reported_mime; + BOOL redirect_override_verb; DWORD pi; DWORD bscf; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3725
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=136991 Your paranoid android. === debian11 (32 bit report) === urlmon: protocol.c:2295: Test failed: unexpected call QueryInterface_IWinInetHttpInfo protocol.c:2295: Test failed: unexpected call QueryInterface_IWinInetHttpInfo === debian11b (64 bit WoW report) === urlmon: protocol.c:2295: Test failed: unexpected call QueryInterface_IWinInetHttpInfo protocol.c:2295: Test failed: unexpected call QueryInterface_IWinInetHttpInfo
I am not sure if the approach I've taken here is acceptable, but this is probably the way that requires the least amount of changes. I also need to come up with a test case for this... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3725#note_44191
See tests/protocol.c for a test case that uses an emulated HTTP plugable protocol handler. We already have some redirection tests there. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3725#note_44195
I think Windows doesn't even bother to check if the status code is 307 or not. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3725#note_44295
On Tue Sep 5 12:11:49 2023 +0000, Yuxuan Shui wrote:
I think Windows doesn't even bother to check if the status code is 307 or not. I was wrong. I don't know how windows find out about the status code though. It does query for an unknown interface "f3d8f080-a5eb-476f-9d19-a5ef24e5c2e6" maybe this is it?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3725#note_44297
On Tue Sep 5 12:11:49 2023 +0000, Yuxuan Shui wrote:
I was wrong. I don't know how windows find out about the status code though. It does query for an unknown interface "f3d8f080-a5eb-476f-9d19-a5ef24e5c2e6" maybe this is it? Perhaps it lets wininet handle it by passing INTERNET_FLAG_NO_AUTO_REDIRECT?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3725#note_44313
On Tue Sep 5 13:44:00 2023 +0000, Hans Leidekker wrote:
Perhaps it lets wininet handle it by passing INTERNET_FLAG_NO_AUTO_REDIRECT? I don't think it can. `wininet` has no concept of `IBindCallbackRedirect`, which `urlmon` needs to call to decide whether to proceed with redirect.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3725#note_44317
On Tue Sep 5 14:12:05 2023 +0000, Yuxuan Shui wrote:
I don't think it can. `wininet` has no concept of `IBindCallbackRedirect`, which `urlmon` needs to call to decide whether to proceed with redirect. Hmm, Windows is definitely doing something odd. When using `CreateBinding` and native Windows HTTP protocol implementation, it _does not_ call `IBindCallbackRedirect::Redirect`. However when using `CreateBinding` with our emulated protocol, it _does_.
(Both cases have `DISABLEAUTOREDIRECTS`). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3725#note_44330
On Tue Sep 5 17:06:55 2023 +0000, Yuxuan Shui wrote:
Hmm, Windows is definitely doing something odd. When using `CreateBinding` and native Windows HTTP protocol implementation, it _does not_ call `IBindCallbackRedirect::Redirect`. However when using `CreateBinding` with our emulated protocol, it _does_. (Both cases have `DISABLEAUTOREDIRECTS`). When Windows does not call `IBindCallbackRedirect::Redirect`, it just `ReportResult` with `INET_E_REDIRECT_FAILED` and the request stops there.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3725#note_44331
On Tue Sep 5 17:36:56 2023 +0000, Yuxuan Shui wrote:
When Windows does not call `IBindCallbackRedirect::Redirect`, it just `ReportResult` with `INET_E_REDIRECT_FAILED` and the request stops there. Edit: ~~could be that our `call_continue` is stopping early.~~ Edit2: No. Figuring out the exact behavior of native urlmon seems to be very involved.
@jacek Do you think the solution proposed in this MR is acceptable? In which case I can create a test case with a couple of todos and call it a day... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3725#note_44334
On Tue Sep 5 17:39:15 2023 +0000, Yuxuan Shui wrote:
Figuring out the exact behavior of native urlmon seems to be very involved. @jacek Do you think the solution proposed in this MR is acceptable? In which case I can create a test case with a couple of todos and call it a day... This stuff is barely documented, so I don't know how it's supposed to work until I can see a test. It's fine to have some todo_wine for some implementation details, but it should at least show that we're doing things in the right place.
For example, I could imagine that we shouldn't change bindprot.c at all and that it's a responsibility of its caller to handle that correctly in its `GetBindInfo` implementation. That would be somewhere in binding.c for the most common case of binding using `CreateURLMoniker`. If that's the case, then adding a test to tests/url.c may be more interesting. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3725#note_44390
caller to handle that correctly in its `GetBindInfo` implementation
hmm, plausible but I think I have evidence against this. In our emulated protocol tests we have control of the `IBindInfo` but Windows still changes the verb as expected. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3725#note_44396
On Wed Sep 6 09:59:33 2023 +0000, Yuxuan Shui wrote:
caller to handle that correctly in its `GetBindInfo` implementation hmm, plausible but I think I have evidence against this. In our emulated protocol tests we have control of the `IBindInfo` but Windows still changes the verb as expected. OK, that's already an interesting finding. From your description, it sounds like the actual http protocol handler behaves differently than our emulated one. We don't have a test of redirecting POST request with the real handler, did you try to add a test for that? Maybe it reports redirection somehow differently in that case.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3725#note_44407
We don't have a test of redirecting POST request with the real handler, did you try to add a test for that?
I did, and I discovered [this bug](https://bugs.winehq.org/show_bug.cgi?id=55527). And that was why I was asking if we have an HTTP endpoint that redirects POST requests. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3725#note_44412
On Wed Sep 6 11:36:55 2023 +0000, Yuxuan Shui wrote:
We don't have a test of redirecting POST request with the real handler, did you try to add a test for that? I did, and I discovered [this bug](https://bugs.winehq.org/show_bug.cgi?id=55527). And that was why I was asking if we have an HTTP endpoint that redirects POST requests. See https://gitlab.winehq.org/winehq/tools/-/tree/master/winetest/tests for our server-side scripts.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3725#note_44414
participants (5)
-
Hans Leidekker (＠hans) -
Jacek Caban (＠jacek)
-
Marvin -
Yuxuan Shui
-
Yuxuan Shui (＠yshui)