Re: [PATCH 4/6] urlmon: Add some error handling to the http protocol.

[Jacek Caban]

Hi David,

On 12/22/10 12:50 AM, David Hedberg wrote:
> ---
>   dlls/urlmon/http.c      |   99 +++++++++++++++++++++++++++++++++++++++++++++--
>   dlls/urlmon/tests/url.c |   25 ++++++++++++
>   2 files changed, 120 insertions(+), 4 deletions(-)
>
> diff --git a/dlls/urlmon/http.c b/dlls/urlmon/http.c
> index cfa2fc6..1ed5e15 100644
> --- a/dlls/urlmon/http.c
> +++ b/dlls/urlmon/http.c
> @@ -80,6 +80,92 @@ static LPWSTR query_http_info(HttpProtocol *This, DWORD option)
>       return ret;
>   }
>
> +static HRESULT handle_http_error(HttpProtocol *This, DWORD error)
> +{
> +    IWindowForBindingUI *wfb_ui;
> +    IHttpSecurity *http_security;
> +    BOOL security_problem;
> +    HRESULT hres;
> +
> +    switch(error) {
> +    case ERROR_INTERNET_SEC_CERT_DATE_INVALID:
> +    case ERROR_INTERNET_SEC_CERT_CN_INVALID:
> +    case ERROR_INTERNET_HTTP_TO_HTTPS_ON_REDIR:
> +    case ERROR_INTERNET_HTTPS_TO_HTTP_ON_REDIR:
> +    case ERROR_HTTP_REDIRECT_NEEDS_CONFIRMATION:
> +    case ERROR_INTERNET_INVALID_CA:
> +    case ERROR_INTERNET_CLIENT_AUTH_CERT_NEEDED:
> +        security_problem = TRUE;
> +        break;
> +    default:
> +        security_problem = FALSE;
> +    }
> +
> +    if(security_problem) {
> +        hres = IUnknown_QueryService((IUnknown*)This->base.protocol_sink,

You shouldn't need the cast here.
> +&IID_IHttpSecurity,&IID_IHttpSecurity, (void **)&http_security);
> +        if(SUCCEEDED(hres)) {
> +            hres = IHttpSecurity_OnSecurityProblem(http_security, error);
> +            IHttpSecurity_Release(http_security);
> +
> +            if(hres == S_OK || hres == E_ABORT)
> +                return E_ABORT;
> +            if(hres == RPC_E_RETRY)
> +                return RPC_E_RETRY;
> +            if(hres != S_FALSE)
> +                return INET_E_SECURITY_PROBLEM;
> +        }
> +    }
> +
> +    hres = IUnknown_QueryService((IUnknown*)This->base.protocol_sink,
> +&IID_IWindowForBindingUI,&IID_IWindowForBindingUI, (void **)&wfb_ui);

Same here.
> +    if(SUCCEEDED(hres)) {
> +        HWND hwnd;
> +        const IID *iid_reason;
> +
> +        if(security_problem)
> +            iid_reason =&IID_IHttpSecurity;
> +        else if(error == ERROR_INTERNET_INCORRECT_PASSWORD)
> +            iid_reason =&IID_IAuthenticate;
> +        else
> +            iid_reason =&IID_IWindowForBindingUI;
> +
> +        hres = IWindowForBindingUI_GetWindow(wfb_ui, iid_reason,&hwnd);
> +        if(SUCCEEDED(hres)&&  hwnd)
> +        {
> +            DWORD res;
> +
> +            res = InternetErrorDlg(hwnd, This->base.request, error,
> +                                   FLAGS_ERROR_UI_FLAGS_CHANGE_OPTIONS | FLAGS_ERROR_UI_FLAGS_GENERATE_DATA,
> +                                   NULL);
> +
> +            if(res == ERROR_INTERNET_FORCE_RETRY || res == ERROR_SUCCESS)
> +                hres = RPC_E_RETRY;
> +            else
> +                hres = E_FAIL;
> +        }
> +        IWindowForBindingUI_Release(wfb_ui);
> +    }
> +
> +    if(hres == RPC_E_RETRY)
> +        return hres;
> +
> +    switch(error)
> +    {
> +    case ERROR_INTERNET_SEC_CERT_DATE_INVALID:
> +    case ERROR_INTERNET_SEC_CERT_CN_INVALID:
> +    case ERROR_INTERNET_INVALID_CA:
> +    case ERROR_INTERNET_CLIENT_AUTH_CERT_NEEDED:
> +        return INET_E_INVALID_CERTIFICATE;
> +    case ERROR_INTERNET_HTTP_TO_HTTPS_ON_REDIR:
> +    case ERROR_INTERNET_HTTPS_TO_HTTP_ON_REDIR:
> +    case ERROR_HTTP_REDIRECT_NEEDS_CONFIRMATION:
> +        return INET_E_REDIRECT_FAILED;
> +    default:
> +        return INET_E_DOWNLOAD_FAILURE;
> +    }
> +}
> +
>   static ULONG send_http_request(HttpProtocol *This)
>   {
>       INTERNET_BUFFERSW send_buffer = {sizeof(INTERNET_BUFFERSW)};
> @@ -282,13 +368,18 @@ static HRESULT HttpProtocol_open_request(Protocol *prot, IUri *uri, DWORD reques
>       if(!res)
>           WARN("InternetSetOption(INTERNET_OPTION_HTTP_DECODING) failed: %08x\n", GetLastError());
>
> -    error = send_http_request(This);
> +    do {
> +        error = send_http_request(This);
>
> -    if(error == ERROR_IO_PENDING || error == ERROR_SUCCESS)
> -        return S_OK;
> +        if(error == ERROR_IO_PENDING || error == ERROR_SUCCESS)
> +            return S_OK;
> +
> +        hres = handle_http_error(This, error);
> +
> +    } while(hres == RPC_E_RETRY);
>
> WARN("HttpSendRequest failed: %d\n", error);
> -    return INET_E_DOWNLOAD_FAILURE;
> +    return hres;
>   }
>
> static HRESULT HttpProtocol_end_request(Protocol *protocol)
> diff --git a/dlls/urlmon/tests/url.c b/dlls/urlmon/tests/url.c
> index 875294b..c2cf152 100644
> --- a/dlls/urlmon/tests/url.c
> +++ b/dlls/urlmon/tests/url.c
> @@ -91,10 +91,12 @@ DEFINE_EXPECT(QueryInterface_IInternetBindInfo);
>   DEFINE_EXPECT(QueryInterface_IAuthenticate);
>   DEFINE_EXPECT(QueryInterface_IInternetProtocol);
>   DEFINE_EXPECT(QueryInterface_IWindowForBindingUI);
> +DEFINE_EXPECT(QueryInterface_IHttpSecurity);
>   DEFINE_EXPECT(QueryService_IAuthenticate);
>   DEFINE_EXPECT(QueryService_IInternetProtocol);
>   DEFINE_EXPECT(QueryService_IInternetBindInfo);
>   DEFINE_EXPECT(QueryService_IWindowForBindingUI);
> +DEFINE_EXPECT(QueryService_IHttpSecurity);
>   DEFINE_EXPECT(BeginningTransaction);
>   DEFINE_EXPECT(OnResponse);
>   DEFINE_EXPECT(QueryInterface_IHttpNegotiate2);
> @@ -627,6 +629,7 @@ static HRESULT WINAPI Protocol_Start(IInternetProtocol *iface, LPCWSTR szUrl,
>           IServiceProvider *service_provider;
>           IHttpNegotiate *http_negotiate;
>           IHttpNegotiate2 *http_negotiate2;
> +        IHttpSecurity *http_security;
>           LPWSTR ua = (LPWSTR)0xdeadbeef, accept_mimes[256];
>           LPWSTR additional_headers = (LPWSTR)0xdeadbeef;
>           BYTE sec_id[100];
> @@ -708,6 +711,18 @@ static HRESULT WINAPI Protocol_Start(IInternetProtocol *iface, LPCWSTR szUrl,
>           ok(hres == E_FAIL, "GetRootSecurityId failed: %08x, expected E_FAIL\n", hres);
>           ok(size == no_callback ? 512 : 13, "size=%d\n", size);
>
> +        if(!no_callback) {
> +            SET_EXPECT(QueryService_IHttpSecurity);
> +            SET_EXPECT(QueryInterface_IHttpSecurity);
> +        }
> +        hres = IServiceProvider_QueryService(service_provider,&IID_IHttpSecurity,
> +&IID_IHttpSecurity, (void**)&http_security);
> +        ok(hres == E_NOINTERFACE, "QueryService failed: 0x%08x\n", hres);
> +        if(!no_callback) {
> +            CHECK_CALLED(QueryService_IHttpSecurity);
> +            CHECK_CALLED(QueryInterface_IHttpSecurity);
> +        }
> +
>           IServiceProvider_Release(service_provider);
>
> IInternetProtocolSink_AddRef(pOIProtSink);
> @@ -1212,6 +1227,11 @@ static HRESULT WINAPI ServiceProvider_QueryService(IServiceProvider *iface,
>           return E_NOTIMPL;
>       }
>
> +    if(IsEqualGUID(&IID_IHttpSecurity, guidService)) {
> +        CHECK_EXPECT(QueryService_IHttpSecurity);
> +        return E_NOTIMPL;
> +    }
> +
>       ok(0, "unexpected service %s\n", debugstr_guid(guidService));
>       return E_NOINTERFACE;
>   }
> @@ -1292,6 +1312,11 @@ static HRESULT WINAPI statusclb_QueryInterface(IBindStatusCallbackEx *iface, REF
>           CHECK_EXPECT2(QueryInterface_IWindowForBindingUI);
>           return E_NOINTERFACE;
>       }
> +    else if(IsEqualGUID(&IID_IHttpSecurity, riid))
> +    {
> +        CHECK_EXPECT2(QueryInterface_IHttpSecurity);
> +        return E_NOINTERFACE;
> +    }
>       else
>       {
>           ok(0, "unexpected interface %s\n", debugstr_guid(riid));

Please add some real tests. At least a connection to an invalid 
certificate should be easy to achieve.

Cheers,
     Jacek

[PATCH 4/6] urlmon: Add some error handling to the http protocol.

[David Hedberg]

Hi Jacek,

On Wed, Dec 22, 2010 at 13:07, Jacek Caban <jacek(a)codeweavers.com> wrote:
> > +    if(security_problem) {
> > +        hres = IUnknown_QueryService((IUnknown*)This->base.protocol_sink,
>
> You shouldn't need the cast here.

I get warnings if I leave the casts out though, and I can't see any
obvious problem with the declarations. Any ideas?
> Please add some real tests. At least a connection to an invalid certificate
> should be easy to achieve.

I'll look into adding some better tests.

Thanks,
David