Created attachment 348675 [details]
PATCH

Comment on attachment 348675 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=348675&action=review

> Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:-385
> -    auto localRequest = request;

Stop using passed request directly. Use resourceHandleInternal's firstRequest, which is already stripped.

Comment on attachment 348675 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=348675&action=review

> LayoutTests/TestExpectations:49
> +http/tests/curl [ Skip ]

Why skip this test on other platforms?  It ought to behave the same in all platforms.

Comment on attachment 348675 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=348675&action=review

>> LayoutTests/TestExpectations:49
>> +http/tests/curl [ Skip ]
> 
> Why skip this test on other platforms?  It ought to behave the same in all platforms.

I meant to put tests which libcurl behaves differently than other port. Is it valuable to make such tests for all ports? If so, I'll put this into other shared place.

Comment on attachment 348675 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=348675&action=review

>>> LayoutTests/TestExpectations:49
>>> +http/tests/curl [ Skip ]
>> 
>> Why skip this test on other platforms?  It ought to behave the same in all platforms.
> 
> I meant to put tests which libcurl behaves differently than other port. Is it valuable to make such tests for all ports? If so, I'll put this into other shared place.

Are there a lot of those? I would expect that it would be a handful of differences that can be mixed and matched by other ports, so identifying them by port name is not great.

But also, is it really desirable to introduce differences like this? Any potential benefit will likely be overshadowed by confusion caused by lack of consistency between WebKit ports.

Comment on attachment 348675 [details]
PATCH

If there's a good reason to have this networking behavior, it should be the same for all ports.

Comment on attachment 348675 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=348675&action=review

>>>> LayoutTests/TestExpectations:49
>>>> +http/tests/curl [ Skip ]
>>> 
>>> Why skip this test on other platforms?  It ought to behave the same in all platforms.
>> 
>> I meant to put tests which libcurl behaves differently than other port. Is it valuable to make such tests for all ports? If so, I'll put this into other shared place.
> 
> Are there a lot of those? I would expect that it would be a handful of differences that can be mixed and matched by other ports, so identifying them by port name is not great.
> 
> But also, is it really desirable to introduce differences like this? Any potential benefit will likely be overshadowed by confusion caused by lack of consistency between WebKit ports.

No, I'm not trying to introduce differences. I'm reducing the differences based on libcurl implementation. I move them to proper place.

Created attachment 348834 [details]
PATCH

Comment on attachment 348834 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=348834&action=review

> LayoutTests/ChangeLog:9
> +        * http/tests/resources/basic-auth/authenticate.php: Added.
> +        * http/tests/resources/basic-auth/authorize.php: Added.

This adds a new directory for the test, which is the right thing to do because we want to avoid the engine preemptively sending credentials used by other tests that could share a directory otherwise.

But the name and location of the new directory are generic, and invites future additions. I think that it should be named by the test (http/tests/xmlhttprequest/resources/url-with-credentials/).

> LayoutTests/http/tests/resources/basic-auth/authorize.php:5
> +    header('WWW-Authenticate: Basic realm="Curl Only"');

Why "Curl only"?

> LayoutTests/http/tests/xmlhttprequest/url-with-credentials.html:7
> +    /*
> +     * If the request contains credentials in its url, it should be stripped from it.
> +     * Also first attempt shouldn't contain basic auth header
> +     */

Here as well, I don't see why we would want to hide the explanation from test output.

Also, "it should be stripped" -> "they should be stripped".

> LayoutTests/http/tests/xmlhttprequest/url-with-credentials.html:23
> +    /* First trial must be access without credentials. */
> +    req.open('GET', '/resources/basic-auth/authenticate.php', false, 'foo', 'bar');
> +    req.send(null);
> +    document.writeln(req.responseText);
> +
> +    /* Send auth info after getting authorization header. */
> +    req.open('GET', '/resources/basic-auth/authorize.php', false, 'foo', 'bar');
> +    req.send(null);
> +    document.writeln(req.responseText);

This doesn't test what the comment claims, as there aren't any credentials in the URL here.

Comment on attachment 348834 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=348834&action=review

>> LayoutTests/ChangeLog:9
>> +        * http/tests/resources/basic-auth/authorize.php: Added.
> 
> This adds a new directory for the test, which is the right thing to do because we want to avoid the engine preemptively sending credentials used by other tests that could share a directory otherwise.
> 
> But the name and location of the new directory are generic, and invites future additions. I think that it should be named by the test (http/tests/xmlhttprequest/resources/url-with-credentials/).

Okay.

>> LayoutTests/http/tests/resources/basic-auth/authorize.php:5
>> +    header('WWW-Authenticate: Basic realm="Curl Only"');
> 
> Why "Curl only"?

Not any more.

>> LayoutTests/http/tests/xmlhttprequest/url-with-credentials.html:7
>> +     */
> 
> Here as well, I don't see why we would want to hide the explanation from test output.
> 
> Also, "it should be stripped" -> "they should be stripped".

Right. Thank you.

>> LayoutTests/http/tests/xmlhttprequest/url-with-credentials.html:23
>> +    document.writeln(req.responseText);
> 
> This doesn't test what the comment claims, as there aren't any credentials in the URL here.

ResourceHandle is created with the url which include those credentials .
https://github.com/WebKit/webkit/blob/master/Source/WebCore/xml/XMLHttpRequest.cpp#L389

> ResourceHandle is created with the url which include those credentials .
> https://github.com/WebKit/webkit/blob/master/Source/WebCore/xml/XMLHttpRequest.cpp#L389

This is an implementation detail that is not appropriate to be explained in a test. What is the observable behavior change being made here?

Created attachment 349070 [details]
PATCH

Make urls used in the test more explicitly obvious.
Rewrote test code.

Comment on attachment 349070 [details]
PATCH

Attachment 349070 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9118735

New failing tests:
http/tests/xmlhttprequest/url-with-credentials.html

Created attachment 349083 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

Created attachment 349087 [details]
PATCH

Comment on attachment 349087 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=349087&action=review

> LayoutTests/http/tests/xmlhttprequest/url-with-credentials.html:2
> +    <script src="/js-test-resources/js-test-pre.js"></script>

In new tests, /js-test-resources/js-test.js is preferable, unless the test checks for something that can be affected by trickier machinery in that version. I don't think that there is anything like that here.

> LayoutTests/http/tests/xmlhttprequest/url-with-credentials.html:5
> +        description(`If the request contains credentials in its url, they should be stripped from it.
> +                     Also first attempt shouldn't contain basic auth header.`);

It's so surprising that we don't seem to already have a test for this. But I looked, and I couldn't find one.

Out of curiosity, does the test pass as is in Chrome and Firefox?

> LayoutTests/http/tests/xmlhttprequest/url-with-credentials.html:16
> +        doTest(

Since this function is called while parsing, there is a race between finishing the test and finishing parsing the HTML document. I n particular, <div id="description"></div> may not be parsed yet by the time shouldBeEqualToString is called.

Please start the test from load event handler.

Comment on attachment 349087 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=349087&action=review

>> LayoutTests/http/tests/xmlhttprequest/url-with-credentials.html:2
>> +    <script src="/js-test-resources/js-test-pre.js"></script>
> 
> In new tests, /js-test-resources/js-test.js is preferable, unless the test checks for something that can be affected by trickier machinery in that version. I don't think that there is anything like that here.

Got it.

>> LayoutTests/http/tests/xmlhttprequest/url-with-credentials.html:5
>> +                     Also first attempt shouldn't contain basic auth header.`);
> 
> It's so surprising that we don't seem to already have a test for this. But I looked, and I couldn't find one.
> 
> Out of curiosity, does the test pass as is in Chrome and Firefox?

Yes. This test passes in both browsers.

>> LayoutTests/http/tests/xmlhttprequest/url-with-credentials.html:16
>> +        doTest(
> 
> Since this function is called while parsing, there is a race between finishing the test and finishing parsing the HTML document. I n particular, <div id="description"></div> may not be parsed yet by the time shouldBeEqualToString is called.
> 
> Please start the test from load event handler.

Wow. This is the worst mistake if I am a web developer. Thank you.

Created attachment 349195 [details]
FIX

Thank you for r+

Comment on attachment 349195 [details]
FIX

Rejecting attachment 349195 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 349195, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/9133006

Created attachment 349201 [details]
Fix

Comment on attachment 349201 [details]
Fix

Clearing flags on attachment: 349201

Committed r235813: <https://trac.webkit.org/changeset/235813>

All reviewed patches have been landed.  Closing bug.

<rdar://problem/44242558>

(In reply to Basuke Suzuki from comment #17)
> > Out of curiosity, does the test pass as is in Chrome and Firefox?
> 
> Yes. This test passes in both browsers.

It passes in Chrome, but it does not pass in Firefox on Linux. The test also fails in WebKit GTK.

Both Firefox and WebKit GTK omit the user and password on the first request, but once a 401 WWW-Authenticate has been received keep sending them for further requests made soon thereafter.

Actually, I would like to know the a rationale for the behavior covered in the test, since there is an obvious overhead in it: every request needing authentication has to be sent twice.

What is the risk in sending the user/password directly instead of waiting for the HTTP 401? HTTP's RFC makes it explicit that this is not required.

I can come up with a hypothetical scenario for it, but it's a bit convoluted:

Alice uses a non-HTTPS application from a trusted intranet in her laptop. At some point, she moves to Bob's café with an untrusted Wi-Fi network without closing her browser and the application makes an XHR HTTP request. If the network has a server in the same IP address and TCP port that does accept HTTP requests, Alice's password will be sent in clear in an untrusted network. The behiavor tested in the patch makes it so that would only happen if in addition that new HTTP server also requested HTTP authentication.

I can come up with only two scenarios where the IP address and TCP port match, accept HTTP and have not been set up by a malicious actor: network devices with common IP addresses configured over HTTP (mainly router/access points on 192.168.0.1) and captive portals. Even then, the exposure would only be prevented if the sever from the source network used HTTP Authorization and the one in the destination network did not.

And all of this over plain HTTP which is known to be insecure. In addition, such guarantees don't exist for other commonly used authentication methods such as cookies or custom form fields or headers.

> Actually, I would like to know the a rationale for the behavior covered in
> the test, since there is an obvious overhead in it: every request needing
> authentication has to be sent twice.
> 
> What is the risk in sending the user/password directly instead of waiting
> for the HTTP 401? HTTP's RFC makes it explicit that this is not required.

I follow the behavior of Mac port so that I cannot tell the true story behind the decision, but I think it's reasonable choice not to send a credential until we know that the authentication is required by the server actually. Also we will have chance which authentication scheme is requested. If the digest auth is the preferable to the server, sending basic auth header at the initial request is the risk which can solve.

Alexey or Alex may give us more detail behind the scene about it.

We do preemptively send basic authentication credentials in many cases.  Maybe there's something about the sync/async xhr use in this test that does something different.  It's probably not a big deal, though, because at worst it will do an extra round trip, but if you're using sync XHR you probably don't care about performance anyways.

Ideally we'd stop sending credentials over unencrypted connections, but I don't think the internet is quite ready for that yet.

I found firefox fails because of my test case logic mistake. I've added async test before landing. The mistake was the order of execution.

1. sync - no www-authenticate header.
2. sync - with www-authenticate header. <-- credentials are stored here.
3. async - no www-authenticate header. (credentials are stored so that they've sent with initial request, then FAIL)
4. async - with www-authenticate header. (PASS, but not 100% sure supplied credentials are used).

Right test should switch step 2 and 3, and put different credential to step 4 to ensure the supplied credentials are used. With this fix, firefox on Windows passes a test.

Chrome and WebKit behave differently. The strategy seems not storing credentials from url. That's why they pass the test.

I'll open bug to improve this test.

(In reply to Alex Christensen from comment #26)
> We do preemptively send basic authentication credentials in many cases. 

I believe that happens only when the protection space is known. And also there's a bug that in that case the scheme is assumed that basic authentication. That's because there's no information about scheme in credential store.


> Maybe there's something about the sync/async xhr use in this test that does
> something different.  It's probably not a big deal, though, because at worst
> it will do an extra round trip, but if you're using sync XHR you probably
> don't care about performance anyways.

it's not based on the sync/async difference. I don't think we pass credentials to unknown protection space even when the credential are supplied, from url or argument of XHR.open().


> Ideally we'd stop sending credentials over unencrypted connections, but I
> don't think the internet is quite ready for that yet.

The Internet follows if Apple decides that choice :)