Bug 37781 - [XHR] Cross-Origin synchronous request with credential raises NETWORK_ERR
: [XHR] Cross-Origin synchronous request with credential raises NETWORK_ERR
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: XML
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To: Julien Chaffraix
: InRadar
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-18 16:07 PDT by Julien Chaffraix
Modified: 2010-07-04 10:55 PDT (History)
6 users (show)

See Also:


Attachments
Proposed fix: Clear the credentials from the actual request (7.49 KB, patch)
2010-04-24 21:27 PDT, Julien Chaffraix
ap: review-
jchaffraix: commit‑queue-
Details | Formatted Diff | Diff
Proposed fix - take 2: Address Alexey's comments (11.46 KB, patch)
2010-04-26 08:36 PDT, Julien Chaffraix
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2010-04-18 16:07:24 PDT
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.
Comment 1 Alexey Proskuryakov 2010-04-19 22:26:47 PDT
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];
Comment 2 Julien Chaffraix 2010-04-24 17:09:03 PDT
> 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.
Comment 3 Julien Chaffraix 2010-04-24 21:27:53 PDT
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.
Comment 4 Alexey Proskuryakov 2010-04-25 13:56:02 PDT
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?
Comment 5 Alexey Proskuryakov 2010-04-25 13:56:35 PDT
<rdar://problem/7905150>
Comment 6 Julien Chaffraix 2010-04-26 08:34:48 PDT
> 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.
Comment 7 Julien Chaffraix 2010-04-26 08:36:32 PDT
Created attachment 54303 [details]
Proposed fix - take 2: Address Alexey's comments
Comment 8 Alexey Proskuryakov 2010-04-26 12:46:15 PDT
Comment on attachment 54303 [details]
Proposed fix - take 2: Address Alexey's comments

r=me

Do the tests pass in Firefox?
Comment 9 David Kilzer (:ddkilzer) 2010-04-27 16:47:51 PDT
(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?
Comment 10 Julien Chaffraix 2010-04-27 19:32:11 PDT
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).
Comment 11 Julien Chaffraix 2010-04-27 20:10:17 PDT
> 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.
Comment 12 WebKit Review Bot 2010-04-27 20:32:18 PDT
http://trac.webkit.org/changeset/58371 might have broken Tiger Intel Release
Comment 13 Julien Chaffraix 2010-04-27 20:45:04 PDT
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.
Comment 14 Julien Chaffraix 2010-04-28 09:35:24 PDT
> 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.
Comment 15 Alexey Proskuryakov 2010-05-17 13:31:18 PDT
> 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.
Comment 16 Chris Evans 2010-07-04 08:07:59 PDT
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.
Comment 17 Alexey Proskuryakov 2010-07-04 10:55:18 PDT
A publicly visible bug is not a great place to discuss security, we should probably take it to security@webkit.org.