Bug 37781 – [XHR] Cross-Origin synchronous request with credential raises NETWORK_ERR

• Component: XML
• Status: RESOLVED FIXED

[Julien Chaffraix]

The cause of this is in DocumentThreadableLoader.cpp:loadRequest

    // FIXME: FrameLoader::loadSynchronously() does not tell us whether a redirect happened or not, so we guess by comparing the
    // request and response URLs. This isn't a perfect test though, since a server can serve a redirect to the same URL that was
    // requested.
    if (request.url() != response.url() && !isAllowedRedirect(response.url())) {
        m_client->didFailRedirectCheck();
        return;
    }

The request has credentials whereas the response does not, thus the failure.

[Alexey Proskuryakov]

Do you have a test case? On Mac OS X, credentials are stripped from URLs before a connection is created:

    // Take user/pass out of the URL.
    // Credentials for ftp can only be passed in URL, the connection:didReceiveAuthenticationChallenge: delegate call won't be made.
    if ((delegate->m_user || delegate->m_pass) && url.protocolInHTTPFamily()) {
        ResourceRequest requestWithoutCredentials = request;
        requestWithoutCredentials.removeCredentials();
        connection = [[NSURLConnection alloc] initWithRequest:requestWithoutCredentials.nsURLRequest() delegate:delegate startImmediately:NO];

[Julien Chaffraix]

> Do you have a test case? 

It took me quite so time to retrieve it but I have it now (I thought it was trivial enough to avoid writing one and I was wrong). I need to clean it up before posting it.

> On Mac OS X, credentials are stripped from URLs before
> a connection is created:
> 
>     // Take user/pass out of the URL.
>     // Credentials for ftp can only be passed in URL, the
> connection:didReceiveAuthenticationChallenge: delegate call won't be made.
>     if ((delegate->m_user || delegate->m_pass) && url.protocolInHTTPFamily()) {
>         ResourceRequest requestWithoutCredentials = request;
>         requestWithoutCredentials.removeCredentials();
>         connection = [[NSURLConnection alloc]
> initWithRequest:requestWithoutCredentials.nsURLRequest() delegate:delegate
> startImmediately:NO];

You are partly right. Currently the credentials are stripped at 3 levels:
- the DocumentThreadableLoader code for cross-origin requests
- the ResourceHandleInternal
- the network level (it depends on the platform)

I may have missed some call sites as our credential management is far from being straightforward.

[Julien Chaffraix]

Created attachment 54228 [details]
Proposed fix: Clear the credentials from the actual request

Actually I went ahead a corrected the code. The test case is part of the patch FYI.

[Alexey Proskuryakov]

Comment on attachment 54228 [details]
Proposed fix: Clear the credentials from the actual request

Good catch! This is a regression, and another fallback from XMLHttpRequest -> DocumentThreadableLoader code move.

The test is ineffective in DRT - with ToT, it prints PASSED and calls notifyDone(), so failure printout is lost:

PASSED
FAILED: got exception NETWORK_ERR: XMLHttpRequest Exception 101

Your proposed change modifies the common code path - don't we need a regression test for async case?

With this change, all code paths in DocumentThreadableLoader now remove credentials. Rather than duplicating the code, it seems that you could just move it to DocumentThreadableLoader constructor.

+    // request and response URLs (note: the comparison will fail if any credential is involved). This isn't a perfect test though,

I don't understand this new comment in parentheses. Could you make it more specific?

[Alexey Proskuryakov]

<rdar://problem/7905150>

[Julien Chaffraix]

> The test is ineffective in DRT - with ToT, it prints PASSED and calls
> notifyDone(), so failure printout is lost:
> 
> PASSED
> FAILED: got exception NETWORK_ERR: XMLHttpRequest Exception 101

I thought I had tested this and made sure it would fail badly in ToT. I will updated that.

> Your proposed change modifies the common code path - don't we need a regression
> test for async case?

We do!

> With this change, all code paths in DocumentThreadableLoader now remove
> credentials. Rather than duplicating the code, it seems that you could just
> move it to DocumentThreadableLoader constructor.

Completely, it would prevent any more corner cases but like this bug.

> +    // request and response URLs (note: the comparison will fail if any
> credential is involved). This isn't a perfect test though,
> 
> I don't understand this new comment in parentheses. Could you make it more
> specific?

Yes. The idea is that comparing the request and response URLs as strings will fail if any credential is involved.

[Julien Chaffraix]

Created attachment 54303 [details]
Proposed fix - take 2: Address Alexey's comments

[Alexey Proskuryakov]

Comment on attachment 54303 [details]
Proposed fix - take 2: Address Alexey's comments

r=me

Do the tests pass in Firefox?

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> (From update of attachment 54303 [details])
> Do the tests pass in Firefox?

http://127.0.0.1:8000/xmlhttprequest/access-control-preflight-credential-async.html

FAILED: got exception Access to restricted URI denied


http://127.0.0.1:8000/xmlhttprequest/access-control-preflight-credential-sync.html

FAILED: got exception Access to restricted URI denied


It looks to me like they pass, but throw a different type of exception in Firefox 3.6.3.

Julien, can you land this soon?

[Julien Chaffraix]

Comment on attachment 54303 [details]
Proposed fix - take 2: Address Alexey's comments

Clearing the cq flag as I will land it myself (the commit bot did not take it for some reason).

[Julien Chaffraix]

> It looks to me like they pass, but throw a different type of exception in
> Firefox 3.6.3.

Thanks for the testing. The test should not throw any exception as we are doing legal cross-site requests.

> Julien, can you land this soon?

Landed in r58371 (skipped the test cases on Qt in r58372). Sorry for the delay.

[WebKit Review Bot]

http://trac.webkit.org/changeset/58371 might have broken Tiger Intel Release

[Julien Chaffraix]

I rolled out the changes as it rendered the Qt build bot unreliable and failed constantly on Tiger (see http://build.webkit.org/results/Tiger%20Intel%20Release/r58371%20%2811340%29/results.html).

I will investigate the Qt issues. For Tiger, it looks like the patch did not change anything: it is still getting a NETWORK_ERR.

[Julien Chaffraix]

> I will investigate the Qt issues.

I could not reproduce the failure locally so I guess it must have been some noise on the bot.

> For Tiger, it looks like the patch did not
> change anything: it is still getting a NETWORK_ERR.

Opened https://bugs.webkit.org/show_bug.cgi?id=38265 and added the 2 tests on the Skipped list.

Committed the patch in r58409.

[Alexey Proskuryakov]

> Thanks for the testing. The test should not throw any exception as we are doing legal cross-site requests.

In other words, the credentials should be silently ignored, but Firefox raises an exception. The real wrong thing to do would be to honor the credentials.

[Chris Evans]

Does anyone have a summary of the security impact of this? The bug title suggests that it's more of a failure that a security bug.

[Alexey Proskuryakov]

A publicly visible bug is not a great place to discuss security, we should probably take it to [e-mail].