1398574 - Update tests within websockets/ to comply with new toplevel data: URI navigation policy

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P2)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1398574_websocket_tests.patch

James, we are about to block toplevel data: URI navigations within Firefox (because those navigations are mostly only used for phishing attempts). Flipping the pref |security.data_uri.block_toplevel_data_uri_navigations| causes the following web-platform tests to fail:
1: websockets/unload-a-document/001.html
2: websockets/unload-a-document/001.html?wss

Instead of rewriting those tests I guess it's easier to keep the pref set to false for those tests. Agreed?

Comment 2

[James Graham [:jgraham]]

Comment on attachment 8906355 [details] [diff] [review]
bug_1398574_websocket_tests.patch

I'd much prefer we fix the test. In general having the tests run in our default configuration is much nicer (e.g. it allows third parties to get the same results without applying our custom metadata so e.g. http://results.web-platform-tests.org will get the right result). Also, if we are doing this I guess other browsers also are, so it makes the tests more likely to work in those browsers. In this case it looks like it should be fairly trivial to fix so I think we should do that.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1398574_websocket_tests.patch

Agreed. I also updated the test 002.html which is marked as failing within our testsuite but has the same setup as 001.html. I also had to remove the ws.onclose part of the test. Potentially that's an artifact of data: URI that ws.onclose was not called? Worst case we need an additional review from a websocket peer.

Comment 4

[James Graham [:jgraham]]

Comment on attachment 8906569 [details] [diff] [review]
bug_1398574_websocket_tests.patch

Review of attachment 8906569 [details] [diff] [review]:
-----------------------------------------------------------------

Thaks for doing this. I think it would help if a websocket peer reviewed the functional change because I can't wrk out why 002.html should work in the first place (it seems to rely on webscockets not affecting the bfcache, which seems improbable). I also think that there's a bug in the test and controller.token should be controller.uuid. In any case I can't see why a close event would fire there.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8906569 [details] [diff] [review]
bug_1398574_websocket_tests.patch

(In reply to James Graham [:jgraham] from comment #4)
> Comment on attachment 8906569 [details] [diff] [review]
> bug_1398574_websocket_tests.patch
> 
> Review of attachment 8906569 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thaks for doing this. I think it would help if a websocket peer reviewed the
> functional change because I can't wrk out why 002.html should work in the
> first place (it seems to rely on webscockets not affecting the bfcache,
> which seems improbable). I also think that there's a bug in the test and
> controller.token should be controller.uuid. In any case I can't see why a
> close event would fire there.

Valentin or Dragana, can you take a quick look at that updated wpt test to make sure we are not doing anything crazy?

Comment 6

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8906569 [details] [diff] [review]
bug_1398574_websocket_tests.patch

Looks good to me.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8906569 [details] [diff] [review]
bug_1398574_websocket_tests.patch

Thanks Valentin - In that case I guess there is no need for daraganas review anymore.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c284ff4354a
Update tests within websockets/ to comply with new toplevel data: URI navigation policy. r=jgraham,valentin

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2c284ff4354a