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

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied 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 348834: PATCH

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




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