Content inside xul:iframe with type=content is always able to resize top chrome window

RESOLVED FIXED in mozilla7

Status

()

Core
XUL
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: ochameau, Assigned: janv)

Tracking

({dev-doc-complete})

unspecified
mozilla7
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
There is a common issue when you are writing firefox extension and we faced it in Jetpack too.
There is multiple usecases where you want to display a remote content that you doesn't necessary trust. For example: sidebars, panels, widgets, ... 
So you use XUL Iframe with type attribute set to "content" in order to have a secured iframe, like the browser do with websites. 
But as websites are able to move and resize browser windows, your extension components (sidebar, widgets,...) is going to resize the browser window too.
And we really don't want that a sidebar resize the browser window!

We would really apprieciate having a new attribute on docShell, like allowWindowControl that would allow to block all move and resize commands coming from iframe document!
https://developer.mozilla.org/en/nsIDocShell

Here is a piece of code that highlight this behavior:
var win = window.top.opener;
var doc = win.document;
var iframe = doc.createElement("iframe");
iframe.setAttribute("type","content");
iframe.setAttribute("src","data:text/html,<script>window.resizeTo(200,200);</script>");
doc.documentElement.appendChild(iframe);

You may simply paste this code in a freshly opened JS console (not an automatically opened one with -jsconsole argument)
Boris: is this feasible (and is this the right Bugzilla component for this bug)?
This is definitely feasible.  The simplest approach is probably to change nsGlobalWindow::CanMoveResizeWindows to not be static and have it check a property on the docshell.  The IsFrame() check that always goes hand in hand with it could go into that method too, or even into the docshell thing (so force subframes to not allow this on the docshell level).

As for component... there are several pieces here.  One is adding the docshell API; the other is exposing it to XUL authors.
Hmm, I suspect the docshell API by itself would be sufficient for Jetpack's needs, even without extra exposure through XUL (f.e. via a "bad-content-no-donut" iframe type), since the Add-on SDK has access to the docshells of the iframes it creates.

*Randomly guessing that jst is the guy to suggest someone to take that on.*

jst: can you recommend someone to do the work bz described in comment 2?
Fwiw, if we don't have someone else who obviously needs a small project, I could just do that.  Lemme know.
(Assignee)

Comment 5

7 years ago
Boris, I'll take this bug, if you don't mind.
Go for it!
(Assignee)

Updated

7 years ago
Assignee: nobody → Jan.Varga
(Assignee)

Comment 7

7 years ago
Created attachment 515735 [details] [diff] [review]
patch
(Assignee)

Comment 8

7 years ago
Here is modified JS for testing:
var win = window.top.opener;
var doc = win.document;
var iframe = doc.createElement("iframe");
iframe.setAttribute("type","content");
iframe.setAttribute("src","data:text/html,<button onclick='window.resizeTo(200,250)'>Resize me</button>");
doc.documentElement.appendChild(iframe);
iframe.docShell.allowWindowControl = false;

I didn't add a check for iframes (subframes) for now.
Status: NEW → ASSIGNED
Jan: is your patch ready for review?
(Assignee)

Comment 10

7 years ago
myk: yes, I think so
(Assignee)

Updated

7 years ago
Attachment #515735 - Flags: review?(bzbarsky)
Comment on attachment 515735 [details] [diff] [review]
patch

>+  nsIDocShell* docShell = nsContentUtils::GetDocShellFromCaller();

Why is that the right docshell to use, as opposed to mDocShell?
Surely it would be easiest to drop an mPrimary check into nsContentTreeOwner::SetSize and any related methods? This would also stop background tabs from inadvertently changing the window size.
Attachment #515735 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 13

6 years ago
Created attachment 536554 [details] [diff] [review]
updated patch
Attachment #515735 - Attachment is obsolete: true
Comment on attachment 536554 [details] [diff] [review]
updated patch

Erm, it looks like there's a patch sitting around waiting for review again.
Attachment #536554 - Flags: review?(bzbarsky)
Comment on attachment 536554 [details] [diff] [review]
updated patch

r=me
Attachment #536554 - Flags: review?(bzbarsky) → review+
Keywords: checkin-needed
(Assignee)

Comment 16

6 years ago
I will land it and thanks for the review
(Assignee)

Comment 17

6 years ago
Pushed:
http://hg.mozilla.org/mozilla-central/rev/babf3b1dfdbc
http://hg.mozilla.org/mozilla-central/rev/3878e0b60ff2

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Keywords: dev-doc-needed
(Reporter)

Comment 18

6 years ago
I'm now using this feature in jetpack and it works like a charm,
Thanks Jan!
Added the new attribute to the reference documentation:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDocShell#Attributes

And mentioned on Firefox 7 for developers.
Keywords: dev-doc-needed → dev-doc-complete
Blocks: 726530
You need to log in before you can comment on or make changes to this bug.