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
: WebKit
XML
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
: InRadar
:
:
  Show dependency treegraph
 
Reported: 2010-04-18 16:07 PST by
Modified: 2010-07-04 10:55 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-04-18 16:07:24 PST
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 From 2010-04-19 22:26:47 PST -------
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 From 2010-04-24 17:09:03 PST -------
> 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 From 2010-04-24 21:27:53 PST -------
Created an attachment (id=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 From 2010-04-25 13:56:02 PST -------
(From update of attachment 54228 [details])
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 From 2010-04-25 13:56:35 PST -------
<rdar://problem/7905150>
------- Comment #6 From 2010-04-26 08:34:48 PST -------
> 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 From 2010-04-26 08:36:32 PST -------
Created an attachment (id=54303) [details]
Proposed fix - take 2: Address Alexey's comments
------- Comment #8 From 2010-04-26 12:46:15 PST -------
(From update of attachment 54303 [details])
r=me

Do the tests pass in Firefox?
------- Comment #9 From 2010-04-27 16:47:51 PST -------
(In reply to comment #8)
> (From update of attachment 54303 [details] [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 From 2010-04-27 19:32:11 PST -------
(From update of attachment 54303 [details])
Clearing the cq flag as I will land it myself (the commit bot did not take it for some reason).
------- Comment #11 From 2010-04-27 20:10:17 PST -------
> 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 From 2010-04-27 20:32:18 PST -------
http://trac.webkit.org/changeset/58371 might have broken Tiger Intel Release
------- Comment #13 From 2010-04-27 20:45:04 PST -------
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 From 2010-04-28 09:35:24 PST -------
> 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 From 2010-05-17 13:31:18 PST -------
> 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 From 2010-07-04 08:07:59 PST -------
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 From 2010-07-04 10:55:18 PST -------
A publicly visible bug is not a great place to discuss security, we should probably take it to security@webkit.org.