Review usage of an html iframe to load arbitrary html content in netmonitor's preview sidebar
Categories
(DevTools :: Netmonitor, task, P2)
Tracking
(firefox107 fixed)
Tracking | Status | |
---|---|---|
firefox107 | --- | fixed |
People
(Reporter: ochameau, Assigned: ochameau)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 1 obsolete file)
Bug 1637869 is highlighting that the Network Monitor is trying to load arbitrary HTML content via an HTML Iframe over here:
https://searchfox.org/mozilla-central/rev/501eb4718d73870892d28f31a99b46f4783efaa0/devtools/client/netmonitor/src/components/previews/HtmlPreview.js#20-23
iframe({
sandbox: "",
srcDoc: typeof htmlBody === "string" ? htmlBody : "",
})
DevTools frontend is running in the parent process, with chrome privileges.
First, the "Toolbox", chrome://devtools/content/framework/toolbox.xhtml
, is loaded in browser.xhtml via a XUL Iframe:
https://searchfox.org/mozilla-central/rev/ea7f70dac1c5fd18400f6d2a92679777d4b21492/devtools/client/framework/toolbox-hosts.js#412-413
It is using type="content"
, but as we are loading a chrome:// URL, we are still running with chrome privileges.
Then, the network monitor is loading chrome://devtools/content/netmonitor/index.html
document in a another, nested, XUL Iframe:
https://searchfox.org/mozilla-central/rev/ea7f70dac1c5fd18400f6d2a92679777d4b21492/devtools/client/framework/toolbox.js#2482
And finally, we add these third iframe I mentioned in HtmlPreview
component. This time an HTML one, which tries to use sandbox
attribute.
Should we try to run this arbitrary code, i.e. content of HTML served by the webpage:
- in a content process?
- with content privileges? Is sandbox working as expected?
<xul:browser type="content" remote="true">
sounds like a better and safer option here, compared to <iframe sandbox>
?
I quickly tried to use a <xul:browser> from this HtmlPreview
component, but I'm not sure that the browser custom element is designed to work from such nested html document.
Would there be some attributes available on html Iframes equivalent to type=content and remote=true, at least for html iframes running in the parent process, from chrome documents??
Otherwise I have some doubt regarding this feature.
Any inner resource (JS, Image, Iframes) displayed in the previewed HTML document may be re-fetched from the network and be different from what has been recorded by the network monitor.
Should the preview prevent fetching any inner resource ? (I think there is a few handful attributes on DocShell that may be helpful)
Or should the preview ensure fetching the cached resource ? The one we recorded and are displaying in the netmonitor ? (Sounds more complex!)
Assignee | ||
Comment 1•4 years ago
|
||
An additional note to followup on all what has been said on bug 1637869.
This crash/failure from bug 1637869 isn't related to DAMP helpers.
I confirm that it comes from the load of:
http://web.bild.de-talos/fis/tp5n/bild.de/web.bild.de/cont/schlaglichter/teaser.html@ifw=476&ifh=47&ifs=no&ct=Bild-Channel+Home.html
But this isn't being loaded via addTab/addTrustedTab, nor any DAMP test script.
It is indirectly loaded from the test script, very precisely from here:
https://searchfox.org/mozilla-central/rev/ea7f70dac1c5fd18400f6d2a92679777d4b21492/testing/talos/talos/tests/devtools/addon/content/tests/netmonitor/netmonitor-helpers.js#137
This call will open the HTML Preview sidebar, and try to load the bild.de HTML content in the sidebar, via the <html:iframe sandbox>, and this HTML content includes a regular html <iframe> loading the http://web.bild.de-talos/fis... URL. This is where the test fails.
Assignee | ||
Comment 2•4 years ago
|
||
Honza, it would be great to unblock jya. (This prevent landing bug 1637869) Do you have any suggestion/feedback about this?
Assignee | ||
Comment 3•4 years ago
|
||
But it looks like <xul:browser> doesn't like netmonitor setup, or creating it dynamically like this?
Assignee | ||
Comment 4•4 years ago
|
||
@bomsy suggested me to try with <xul:iframe> as using <xul:browser> lead to such error:
JavaScript error: chrome://global/content/elements/browser-custom-element.js, line 1212: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIScriptSecurityManager.getLoadContextContentPrincipal]
And the browser element doesn't display anything.
Unfortunately, xul:iframe doesn't throw any error, but still doesn't display anything.
The STR to trigger this HtmlPreview code is to open the netmonitor, load an html page, click on the request for the html page and select the Preview sidebar on the right.
Assignee | ||
Comment 5•4 years ago
|
||
This is just to allow creating XUL Iframe/Browser, otherwise we can't create element from another xmlns from React.
Assignee | ||
Comment 6•4 years ago
|
||
No exception, but still display nothing.
Assignee | ||
Comment 7•4 years ago
|
||
Ok, so, the issue is about <browser type=content>
being used for the Toolbox document.
It prevents inner documents from using nested content browser element.
Nika pointed out that WebExtension is using such nested content browser elements and this only works thanks to this whitelist:
https://searchfox.org/mozilla-central/rev/7dafc35406b9c945189c617d427f5458933fd3fb/dom/base/nsFrameLoader.cpp#2509-2535
If we want to use nested browser element, and I think we do, we have to add netmonitor.xhtml URL to this whitelist.
Assignee | ||
Comment 8•4 years ago
|
||
Assignee | ||
Comment 9•4 years ago
|
||
Adding netmonitor document to the whitelist works and allows using xul:iframe for loading the preview!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=be6adaf24aafe7d3ab8c6aa8d44a9761a7fdfa31
This fixes running the preview in a content process (instead of parent) and with type=content
(I imagine it may be more restrictive than sandbox=""
?
But this still keep open questions on which we should followup. Any inner resource (JS, CSS, Iframe, Images, ...) will be refetched from the network. This can be even more confusing with remote debugging as this will use a different user agent! See end of comment 0 for more info about that.
Updated•4 years ago
|
Comment 10•4 years ago
|
||
Any inner resource (JS, CSS, Iframe, Images, ...) will be refetched from the network.
Agreed, no requests should be triggered by previewing HTML sources.
Assignee | ||
Comment 11•4 years ago
|
||
These patches fails on DAMP:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=303944523&repo=try&lineNumber=1017
I imagine it triggers similar issue as jya is having, where the load event on the preview's iframe doesn't fire.
https://searchfox.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/tests/netmonitor/netmonitor-helpers.js#140
Assignee | ||
Comment 12•4 years ago
|
||
Oops, wait scratch all that from comment 11, I made a typo and was still using an HTML iframe in the changeset I pushed to try.
Assignee | ||
Comment 13•4 years ago
•
|
||
Updated•4 years ago
|
Assignee | ||
Comment 14•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&selectedTaskRun=A5njCyEmTz-Wz6-EcCS8Zw-0&revision=d0ab6677315e2250a2d9cdbfc57ad4c8c697fc3c
Still the same DAMP failure, but it looks like I broke the sidebar completely.
Could we possibly have no mochitest covering this feature? All mochitest are passing, that's concerning.
I'll do a fresh build overnight to investigate this further.
Updated•4 years ago
|
Comment 15•4 years ago
|
||
I just reported a follow up:
Bug 1644095 - No HTTP request should be triggered by previewing HTML response
Honza
Comment 16•4 years ago
|
||
Alex, are you still planning to work on this one? There are already patches attached. Would review help at this point?
Honza
Assignee | ||
Comment 18•2 years ago
|
||
Try run after rebase:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e84b9680d97ccb903488b2f3893621f2c6046146
Updated•2 years ago
|
Updated•2 years ago
|
Assignee | ||
Comment 19•2 years ago
|
||
Comment 20•2 years ago
|
||
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/52bbd332bdd9 Add netmonitor to the whitelist of nested content browsers. r=smaug https://hg.mozilla.org/integration/autoland/rev/4e39348144b3 [devtools] Refactor HtmlPreview to create the iframe dynamically r=bomsy https://hg.mozilla.org/integration/autoland/rev/419078dcf414 [devtools] Use content and remote iframe for loading HTML preview in netmonitor. r=bomsy https://hg.mozilla.org/integration/autoland/rev/5fa87e754895 [devtools] Extend coverage of HTML previews in the netmonitor. r=bomsy
Comment 21•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/52bbd332bdd9
https://hg.mozilla.org/mozilla-central/rev/4e39348144b3
https://hg.mozilla.org/mozilla-central/rev/419078dcf414
https://hg.mozilla.org/mozilla-central/rev/5fa87e754895
Description
•