[webkit-reviews] review granted: [Bug 189198] [Curl][WebKitLegacy] Stop sending credential embedded in the url via XHR. : [Attachment 349087] PATCH

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Basuke Suzuki
<Basuke.Suzuki at sony.com>'s request for review:
Bug 189198: [Curl][WebKitLegacy] Stop sending credential embedded in the url
via XHR.
https://bugs.webkit.org/show_bug.cgi?id=189198

Attachment 349087: PATCH

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




--- Comment #16 from Alexey Proskuryakov <ap at webkit.org> ---
Comment on attachment 349087
  --> https://bugs.webkit.org/attachment.cgi?id=349087
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.