We should add tests that verify the cookie behavior for third-party WebSocket requests and responses.
<rdar://problem/42048729>
Created attachment 344736 [details] Patch
Comment on attachment 344736 [details] 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-cookie-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-cookie-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&value=value#http://127.0.0.1:8000/websocket/tests/hybi/websocket-cookie-overwrite-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?
(In reply to Daniel Bates from comment #3) > Comment on attachment 344736 [details] > 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. OK, I'll make use of shared code. > > > 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. I can switch to using echo-cookies.php. Unfortunately, it doesn't echo document.cookie which I think we always should check along with what's sent to the server. > > 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. I see you use echo-iframe-src.php to output third-party cookies. But I want to also test setting a new cookie *as third-party* after having a set a cookie as first party. Do we have infrastructure for that? > > LayoutTests/http/tests/websocket/tests/hybi/websocket-blocked-from-setting-cookie-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-cookie-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&value=value#http://127.0.0.1:8000/websocket/tests/hybi/websocket-cookie-overwrite-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?
This reply got posted before I was done. Continuing below. (In reply to John Wilander from comment #4) > (In reply to Daniel Bates from comment #3) > > Comment on attachment 344736 [details] > > 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. > > OK, I'll make use of shared code. > > > > > > 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. > > I can switch to using echo-cookies.php. Unfortunately, it doesn't echo > document.cookie which I think we always should check along with what's sent > to the server. > > > > 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. > > I see you use echo-iframe-src.php to output third-party cookies. But I want > to also test setting a new cookie *as third-party* after having a set a > cookie as first party. Do we have infrastructure for that? > > > > LayoutTests/http/tests/websocket/tests/hybi/websocket-blocked-from-setting-cookie-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. Sure. You added this test in https://bugs.webkit.org/show_bug.cgi?id=184100, but I can change the function name as part of moving it. > > > > > LayoutTests/http/tests/websocket/tests/hybi/websocket-blocked-from-setting-cookie-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. I did that first but then I noticed that we call description() without checks so we're already causing a JavaScript error, right? > > > > > 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&value=value#http://127.0.0.1:8000/websocket/tests/hybi/websocket-cookie-overwrite-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? Sure. You added the test in https://bugs.webkit.org/show_bug.cgi?id=184100. Since third-parties without existing cookies cannot set cookies at all, your fix in https://bugs.webkit.org/show_bug.cgi?id=184100 may regress without us noticing since the third-party cookie setting doesn't create a cookie. My change adds a roundtrip to make sure localhost has a cookie which is the prerequisite for setting a new cookie as third-party.
Created attachment 344894 [details] Patch
Comment on attachment 344894 [details] Patch Looks good to me. More tests!
Comment on attachment 344894 [details] Patch Thanks, Alex!
Comment on attachment 344894 [details] Patch Clearing flags on attachment: 344894 Committed r233830: <https://trac.webkit.org/changeset/233830>
All reviewed patches have been landed. Closing bug.
Looks like these two tests: http/tests/websocket/tests/hybi/websocket-allowed-setting-cookie-as-third-party.html http/tests/websocket/tests/hybi/websocket-cookie-overwrite-behavior.html Are now constant failures on Sierra Debug and Release WK2 as of r233830: <https://trac.webkit.org/changeset/233830/webkit> Test Histories: <https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fwebsocket%2Ftests%2Fhybi%2Fwebsocket-allowed-setting-cookie-as-third-party.html%20%20http%2Ftests%2Fwebsocket%2Ftests%2Fhybi%2Fwebsocket-cookie-overwrite-behavior.html> http/tests/websocket/tests/hybi/websocket-allowed-setting-cookie-as-third-party.html Output Diff: <https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r233845%20(10576)/http/tests/websocket/tests/hybi/websocket-allowed-setting-cookie-as-third-party-diff.txt> http/tests/websocket/tests/hybi/websocket-cookie-overwrite-behavior.html Output Diff: <https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r233845%20(10576)/http/tests/websocket/tests/hybi/websocket-cookie-overwrite-behavior-diff.txt>
(In reply to Truitt Savell from comment #11) > Looks like these two tests: > http/tests/websocket/tests/hybi/websocket-allowed-setting-cookie-as-third- > party.html > http/tests/websocket/tests/hybi/websocket-cookie-overwrite-behavior.html > > Are now constant failures on Sierra Debug and Release WK2 as of r233830: > <https://trac.webkit.org/changeset/233830/webkit> > > > Test Histories: > <https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > html#showAllRuns=true&tests=http%2Ftests%2Fwebsocket%2Ftests%2Fhybi%2Fwebsock > et-allowed-setting-cookie-as-third-party. > html%20%20http%2Ftests%2Fwebsocket%2Ftests%2Fhybi%2Fwebsocket-cookie- > overwrite-behavior.html> > > http/tests/websocket/tests/hybi/websocket-allowed-setting-cookie-as-third- > party.html Output Diff: > > <https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/ > r233845%20(10576)/http/tests/websocket/tests/hybi/websocket-allowed-setting- > cookie-as-third-party-diff.txt> > > http/tests/websocket/tests/hybi/websocket-cookie-overwrite-behavior.html > Output Diff: > > <https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/ > r233845%20(10576)/http/tests/websocket/tests/hybi/websocket-cookie-overwrite- > behavior-diff.txt> It's not setting the cookie for the third-party WebSocket so we're failing closed. Probably a Sierra bug in underlying layers. We should skip it.
(In reply to John Wilander from comment #12) > (In reply to Truitt Savell from comment #11) > > Looks like these two tests: > > http/tests/websocket/tests/hybi/websocket-allowed-setting-cookie-as-third- > > party.html > > http/tests/websocket/tests/hybi/websocket-cookie-overwrite-behavior.html > > > > Are now constant failures on Sierra Debug and Release WK2 as of r233830: > > <https://trac.webkit.org/changeset/233830/webkit> > > > > > > Test Histories: > > <https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > > html#showAllRuns=true&tests=http%2Ftests%2Fwebsocket%2Ftests%2Fhybi%2Fwebsock > > et-allowed-setting-cookie-as-third-party. > > html%20%20http%2Ftests%2Fwebsocket%2Ftests%2Fhybi%2Fwebsocket-cookie- > > overwrite-behavior.html> > > > > http/tests/websocket/tests/hybi/websocket-allowed-setting-cookie-as-third- > > party.html Output Diff: > > > > <https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/ > > r233845%20(10576)/http/tests/websocket/tests/hybi/websocket-allowed-setting- > > cookie-as-third-party-diff.txt> > > > > http/tests/websocket/tests/hybi/websocket-cookie-overwrite-behavior.html > > Output Diff: > > > > <https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/ > > r233845%20(10576)/http/tests/websocket/tests/hybi/websocket-cookie-overwrite- > > behavior-diff.txt> > > It's not setting the cookie for the third-party WebSocket so we're failing > closed. Probably a Sierra bug in underlying layers. We should skip it. I misinterpreted this issue. These tests are failing on all MacOS platforms except High Sierra.
(In reply to Truitt Savell from comment #13) > (In reply to John Wilander from comment #12) > > (In reply to Truitt Savell from comment #11) > > > Looks like these two tests: > > > http/tests/websocket/tests/hybi/websocket-allowed-setting-cookie-as-third- > > > party.html > > > http/tests/websocket/tests/hybi/websocket-cookie-overwrite-behavior.html > > > > > > Are now constant failures on Sierra Debug and Release WK2 as of r233830: > > > <https://trac.webkit.org/changeset/233830/webkit> > > > > > > > > > Test Histories: > > > <https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > > > html#showAllRuns=true&tests=http%2Ftests%2Fwebsocket%2Ftests%2Fhybi%2Fwebsock > > > et-allowed-setting-cookie-as-third-party. > > > html%20%20http%2Ftests%2Fwebsocket%2Ftests%2Fhybi%2Fwebsocket-cookie- > > > overwrite-behavior.html> > > > > > > http/tests/websocket/tests/hybi/websocket-allowed-setting-cookie-as-third- > > > party.html Output Diff: > > > > > > <https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/ > > > r233845%20(10576)/http/tests/websocket/tests/hybi/websocket-allowed-setting- > > > cookie-as-third-party-diff.txt> > > > > > > http/tests/websocket/tests/hybi/websocket-cookie-overwrite-behavior.html > > > Output Diff: > > > > > > <https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/ > > > r233845%20(10576)/http/tests/websocket/tests/hybi/websocket-cookie-overwrite- > > > behavior-diff.txt> > > > > It's not setting the cookie for the third-party WebSocket so we're failing > > closed. Probably a Sierra bug in underlying layers. We should skip it. > > I misinterpreted this issue. These tests are failing on all MacOS platforms > except High Sierra. So it's failing on El Cap and Sierra and working on High Sierra and Mojave. Or are you saying it's failing on Mojave?
failing on Mojave WK2 as well
(In reply to Truitt Savell from comment #15) > failing on Mojave WK2 as well I just ran them on Mojave (debug build) without problem and I don't see failures on the bots. The tests were developed on Mojave.
*** Bug 187658 has been marked as a duplicate of this bug. ***
*** Bug 187707 has been marked as a duplicate of this bug. ***