Closed Bug 1398574 Opened 7 years ago Closed 7 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file, 1 obsolete file)

      No description provided.
Assignee: nobody → ckerschb
Blocks: 1380959
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
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?
Attachment #8906355 - Flags: review?(james)
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.
Attachment #8906355 - Flags: review?(james) → review-
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.
Attachment #8906355 - Attachment is obsolete: true
Attachment #8906569 - Flags: review?(james)
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.
Attachment #8906569 - Flags: review?(james) → review+
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?
Attachment #8906569 - Flags: review?(valentin.gosu)
Attachment #8906569 - Flags: review?(dd.mozilla)
Comment on attachment 8906569 [details] [diff] [review]
bug_1398574_websocket_tests.patch

Looks good to me.
Attachment #8906569 - Flags: review?(valentin.gosu) → review+
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.
Attachment #8906569 - Flags: review?(dd.mozilla)
Pushed by mozilla@christophkerschbaumer.com:
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
https://hg.mozilla.org/mozilla-central/rev/2c284ff4354a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.