[webkit-reviews] review denied: [Bug 187541] Flesh out WebSocket cookie tests to cover cookie policy for third-party resources : [Attachment 344736] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied John Wilander
<wilander at apple.com>'s request for review:
Bug 187541: Flesh out WebSocket cookie tests to cover cookie policy for
third-party resources
https://bugs.webkit.org/show_bug.cgi?id=187541

Attachment 344736: Patch

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




--- Comment #3 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 344736
  --> https://bugs.webkit.org/attachment.cgi?id=344736
Patch

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

r- for the duplicate code and duplicate functionally in helper scripts. We
should share more code with tests and resources in
LayoutTests/http/tests/cookies and its subdirectories.

> LayoutTests/http/tests/websocket/tests/hybi/resources/get-cookies.php:18
> +<?php
> +if(!isset($_COOKIE[$_GET["name1"]])) {
> +    echo "Did not receive cookie named '" . $_GET["name1"] . "'.<br>";
> +} else {
> +    echo "Received cookie named '" . $_GET["name1"] . "'.<br>";
> +}
> +if(!empty($_GET["name2"])) {
> +    if(!isset($_COOKIE[$_GET["name2"]])) {
> +	   echo "Did not receive cookie named '" . $_GET["name2"] . "'.<br>";
> +    } else {
> +	   echo "Received cookie named '" . $_GET["name2"] . "'.<br>";
> +    }
> +}
> +?>
> +<p id="output"></p>
> +<script>
> +    document.getElementById("output").textContent = "Client-side
document.cookie: " + document.cookie;
> +</script>

I am not a fan of this script given that it only retrieves the contents of at
most two cookies and how we use it in the tests below to underutilize the
functionality provided by js-test.js to assert cookie values. I would prefer
that we write the tests below using js-test functionality. If you have your
heart set on "dump"ing the cookies then we should make use of the existing
helper scripts LayoutTests/http/tests/cookies/resources/echo-cookies.php or
LayoutTests/http/tests/cookies/resources/getCookies.cgi instead of adding
yet-another-cookie-dumping-script.

> LayoutTests/http/tests/websocket/tests/hybi/resources/set-cookie.php:8
> +<?php
> +setcookie($_GET["name"], $_GET["value"], (time()+60*60*24*30), "/");
> +?>
> +<script>
> +if (document.location.hash) {
> +    setTimeout("document.location.href =
document.location.hash.substring(1)", 10);
> +}
> +</script>

I do not see the need for this script. We should make use of setCookie()
defined in LayoutTests/http/tests/cookies/resources/cookie-utilities.js. You
can see examples of its usage in the tests in
LayoutTests/http/tests/cookies/same-site.

>
LayoutTests/http/tests/websocket/tests/hybi/websocket-blocked-from-setting-cook
ie-as-third-party.html:15
> +	   function setCookieFromHost(host)
> +	   {
> +	       var promise = new Promise(resolve => {
> +		   var websocket = new
WebSocket(`ws://${host}:8880/websocket/tests/hybi/cookie?set`);
> +		   websocket.onclose = () => resolve();
> +	       });
> +	       return promise;
> +	   }

We should not duplicate this function in every test. We should move it to a
shared file and then reference the file. I suggest we come up with a better
name for this function, maybe setCookieUsingWebSocketFromHost or
setWebSocketCookieFromHost, and put this function with our other cookie utility
functions in file LayoutTests/http/tests/cookies/resources/cookie-utilities.js.

>
LayoutTests/http/tests/websocket/tests/hybi/websocket-blocked-from-setting-cook
ie-as-third-party.html:37
> +    testRunner.dumpChildFramesAsText();

This will cause a JavaScript error when running this test outside
DumpRenderTree/WebKitTestRunner. I suggest we condition this line on the
presence of window.testRunner.

>
LayoutTests/http/tests/websocket/tests/hybi/websocket-cookie-overwrite-behavior
.html:59
> +    switch (document.location.hash) {
> +	   case "":
> +	       document.location.href =
"http://localhost:8000/websocket/tests/hybi/resources/set-cookie.php?name=bar&v
alue=value#http://127.0.0.1:8000/websocket/tests/hybi/websocket-cookie-overwrit
e-behavior.html#cookieSetAsFirstParty";
> +	       break;
> +	   case "#cookieSetAsFirstParty":
> +	       debug("Same origin WebSocket:");
> +	       await testSameOriginCookie();
> +	       debug("<br>Cross origin WebSocket:");
> +	       await testCrossOriginCookie();
> +	       finishJSTest();
> +	       break;
> +    }

Can you please explain the purpose of this test?