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

RESOLVED FIXED in Firefox 57

Status

()

Core
DOM: Security
P1
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

3 months ago
Blocks: 1380959
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
(Assignee)

Comment 1

3 months ago
Created attachment 8905424 [details] [diff] [review]
bug_1397657_toolkit_tests.patch

Francois, 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 a test within toolkit/ to fail:
* browser_page_change_print_original.js

Please note that I didn't add an empty line and the end for the two files on purpose, because the old test setup explicitly required a string comparison to make the test succeed - I didn't want to touch that.
Attachment #8905424 - Flags: review?(francois)
(Assignee)

Updated

3 months ago
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [domsecurity-backlog1] → [domsecurity-active]
(Assignee)

Comment 2

3 months ago
Created attachment 8906003 [details] [diff] [review]
bug_1397657_toolkit_tests.patch

Francois, apparently eslint is not amused by not having empty lines at the end of the file. Instead I am using trim() now, otherwise we face the line ending problem because textContent would include \n.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fe2deb311fe28e7e43e0616ddf9368a1ce555d7
Attachment #8905424 - Attachment is obsolete: true
Attachment #8905424 - Flags: review?(francois)
Attachment #8906003 - Flags: review?(francois)
Comment on attachment 8906003 [details] [diff] [review]
bug_1397657_toolkit_tests.patch

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

That looks fine to me, but it may be good to have someone who actually knows about this test reviewing your change.
Attachment #8906003 - Flags: review?(francois) → review+
(Assignee)

Comment 4

2 months ago
Created attachment 8906348 [details] [diff] [review]
bug_1397657_toolkit_tests.patch
Attachment #8906003 - Attachment is obsolete: true
Attachment #8906348 - Flags: review+
(Assignee)

Comment 5

2 months ago
Comment on attachment 8906348 [details] [diff] [review]
bug_1397657_toolkit_tests.patch

Dave, let me summarize the test changes here real quick:
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 tests within toolkit/ to fail:
* browser_page_change_print_original.js
For this test I am loading the data: URI from an external file. Please note that I have to use trim() because otherwise we are facing a line ending problem because textContent would include the \n.

* toolkit/mozapps/extensions/test/xpinstall/browser_datauri.js
Since Bug 1394554 we are also blocking redirects to data: URIs. I looked at browser_datauri.js and there is this comment on top "Checks that a chained redirect through a data URI and javascript is blocked", hence I think it makes sense to explicitly flip the pref to false, so this test keeps testing what it's supposed to be testing.

Are you fine with those changes?
Attachment #8906348 - Flags: review?(dtownsend)
Comment on attachment 8906348 [details] [diff] [review]
bug_1397657_toolkit_tests.patch

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

Sounds fine.
Attachment #8906348 - Flags: review?(dtownsend) → review+

Comment 7

2 months ago
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff02238411b7
Update tests within toolkit/ to comply with new toplevel data: URI navigation policy. r=francois,dtownsend
https://hg.mozilla.org/mozilla-central/rev/ff02238411b7
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.