Closed
Bug 1337272
Opened 8 years ago
Closed 7 years ago
Convert tests within toolkit/ to not rely on principal inheritance for data: URIs
Categories
(Core :: DOM: Security, defect, P2)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla56
| Tracking | Status | |
|---|---|---|
| firefox56 | --- | fixed |
People
(Reporter: ckerschb, Assigned: ckerschb)
References
Details
(Whiteboard: [domsecurity-active])
Attachments
(1 file, 2 obsolete files)
|
12.13 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
Flip pref [1] to 'false' and update tests within toolkit/ to still work with new data: URI behavior.
[1] pref ("security.data_uri.inherit_security_context", true);
|
Assignee | |
Updated•8 years ago
|
|
|
Assignee | |
Updated•7 years ago
|
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Priority: P3 → P2
Whiteboard: [domsecurity-backlog1] → [domsecurity-active]
|
|
Assignee | |
Comment 1•7 years ago
|
||
Hey Gijs, we are updating the way the data: URI inheritance security model works within Firefox. By flipping the pref security.data_uri.unique_opaque_origin data: URIs become cross origin with the including context. We identified 3 tests with toolkit/ that are failing because of that:
(a) toolkit/components/windowwatcher/test/test_named_window.html
(b) toolkit/content/tests/mochitest/test_mousecapture.xhtml
(c) toolkit/content/tests/widgets/test_videocontrols_jsdisabled.html
I was able to resolve the problems within (a) and (c) by just loading the data: URI from an external resource. However, I am facing problems with (b). While in theory the test should work with my updates it still fails (see error dump underneath). One of the problems is, that I don't fully understand what the test is doing. E.g. would it be ok to replace the percent encoding. I guess yes, but also not certain why it was written that way in the first place. I saw you did a bunch of updates/reviews for that file. I was wondering if you have any suggestions on how to update that test to comply with the new data: URI security model. I also tried using SpecialPowers.wrap(iframe[0]) instead of loading the data: URI from an external resource, also no luck with that. It seems that SpecialPowers.wrap() can not forward all of the functionality. If there is an easier way to rewrite that test, or if you have any other suggestions, please let me know.
failed | uncaught exception - TypeError: aTarget.getBoundingClientRect is not a function at synthesizeMouse@http://mochi.test:8888/tests/SimpleTest/EventUtils.js:325:14 runCaptureTest@http://mochi.test:8888/tests/toolkit/content/tests/mochitest/test_mousecapture.xhtml:229:3
runTests@http://mochi.test:8888/tests/toolkit/content/tests/mochitest/test_mousecapture.xhtml:188:3
focusedOrLoaded/<@http://mochi.test:8888/tests/SimpleTest/SimpleTest.js:795:59
Attachment #8887001 -
Flags: feedback?(gijskruitbosch+bugs)
|
||
Comment 2•7 years ago
|
||
Comment on attachment 8887001 [details] [diff] [review]
bug_1337272_tests_toolkit.patch
Review of attachment 8887001 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/components/windowwatcher/test/test_named_window.html
@@ +20,5 @@
> <script type="application/javascript">
> "use strict";
>
> const NAME = "my_window";
> + const TARGET_PATH = "http://mochi.test:8888/tests/toolkit/components/windowwatcher/test/";
Here and elsewhere, please don't hardcode full test paths. You can probably use the DOM `location` object to get most of this, and/or a suitable helper provided by mochitest.
::: toolkit/content/tests/mochitest/test_mousecapture.xhtml
@@ -301,5 @@
> - <iframe width="100" height="100"
> - src="data:text/html,%3Cbody style%3D'font-size%3A 40pt%3B'%3E.%3Cb id%3D'b'%3EThis%3C/b%3E is some text%3Cdiv id='fixed' style='position: fixed; left: 55px; top: 5px; width: 10px; height: 10px'%3E.%3C/div%3E%3C/body%3E"/>
> -
> - <iframe width="100" height="100"
> - src="data:text/html,%3Cframeset cols='50%, 50%'%3E%3Cframe src='about:blank'%3E%3Cframe src='about:blank'%3E%3C/frameset%3E"/>
Your new file has a newline, and so the last child of the (generated DOM) <html> element is not the <frameset> but the text node, which means the test fails because it uses documentElement.lastChild here:
https://dxr.mozilla.org/mozilla-central/rev/e0b0865639cebc1b5afa0268a4b073fcdde0e69c/toolkit/content/tests/mochitest/test_mousecapture.xhtml#186
replace with documentElement.lastElementChild, and the test at least runs to completion. Don't know if other issues I'm seeing are specific to my hidpi Windows setup or genuine further issues.
::: toolkit/content/tests/widgets/file_videocontrols_jsdisabled.sjs
@@ +4,5 @@
> +function handleRequest(request, response) {
> + response.setHeader("Cache-Control", "no-cache", false);
> + response.setHeader("Content-Type", "text/html", false);
> + let videoURL = request.queryString;
> + response.write('<video src="' + videoURL + '" controls autoplay=true></video><script>window.testExpando = true;</scr" + "ipt>');
Why does this need to be an sjs file rather than a plain HTML file?
Note that because this is a JS file and no longer JS included in a .html file, you don't need to split up the closing script tag. This code would also be easier to read if you used a template string.
Attachment #8887001 -
Flags: feedback?(gijskruitbosch+bugs) → feedback+
|
|
Assignee | |
Comment 3•7 years ago
|
||
Gijs, your suggestion makes all of the tests work. Replaced the *.sjs with an html file and further use location instead of hardcoded paths. Thanks for your hints and help!
Attachment #8887001 -
Attachment is obsolete: true
Attachment #8887448 -
Flags: review?(gijskruitbosch+bugs)
|
|
||
Comment 4•7 years ago
|
||
Comment on attachment 8887448 [details] [diff] [review]
bug_1337272_tests_toolkit.patch
Review of attachment 8887448 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/components/windowwatcher/test/test_named_window.html
@@ +21,5 @@
> "use strict";
>
> const NAME = "my_window";
> + const TARGET_PATH = location.href.replace("test_named_window.html", "");
> + const TARGET_URL = TARGET_PATH + "file_named_window.html";
Could do this as a single replace as you've done elsewhere, I think?
Attachment #8887448 -
Flags: review?(gijskruitbosch+bugs) → review+
|
|
Assignee | |
Comment 5•7 years ago
|
||
(In reply to :Gijs from comment #4)
> ::: toolkit/components/windowwatcher/test/test_named_window.html
> > + const TARGET_PATH = location.href.replace("test_named_window.html", "");
> > + const TARGET_URL = TARGET_PATH + "file_named_window.html";
>
> Could do this as a single replace as you've done elsewhere, I think?
Indeed - done.
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b669da0374a2
Convert tests within toolkit/ to not rely on principal inheritance for data: URIs. r=gijs
|
||
Comment 7•7 years ago
|
||
Backed out for linting failure at test_mousecapture.xhtml:208: 'topPos' is assigned a value but never used:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d8bee539dc277c9c0ebf997c7efdd251906faba
Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b669da0374a2d7d7d946983865c0a799362725d9&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=115209648&repo=mozilla-inbound
> TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/content/tests/mochitest/test_mousecapture.xhtml:208:7 | 'topPos' is assigned a value but never used. (no-unused-vars)
Flags: needinfo?(ckerschb)
|
|
Assignee | |
Comment 8•7 years ago
|
||
Hey Gijs, I am passing topPos using queryParameter to the subfile. Sorry for not checking eslint the first time around - rrrh :-(
Attachment #8887448 -
Attachment is obsolete: true
Flags: needinfo?(ckerschb)
Attachment #8887457 -
Flags: review?(gijskruitbosch+bugs)
|
|
||
Comment 9•7 years ago
|
||
Comment on attachment 8887457 [details] [diff] [review]
bug_1337272_tests_toolkit.patch
Review of attachment 8887457 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/content/tests/mochitest/file_mousecapture2.html
@@ +14,5 @@
> + }
> + return "";
> +}
> +let topPos = getQueryParam("topPos");
> +document.getElementById("myStyle").style.marginTop = topPos;
...marginTop = new URL(location.href).searchParams.get("topPos");
and then remove the self-implemented getQueryParam :-)
Attachment #8887457 -
Flags: review?(gijskruitbosch+bugs) → review+
|
||
Comment 10•7 years ago
|
||
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33a050957853
Convert tests within toolkit/ to not rely on principal inheritance for data: URIs. r=gijs
|
||
Comment 11•7 years ago
|
||
| bugherder | ||
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•