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

RESOLVED FIXED in mozilla7

Status

()

defect
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: ochameau, Assigned: janv)

Tracking

({dev-doc-complete})

unspecified
mozilla7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

3.05 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
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.
Boris, I'll take this bug, if you don't mind.
Assignee: nobody → Jan.Varga
Posted patch patch (obsolete) — Splinter Review
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?
myk: yes, I think so
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-
Posted patch updated patchSplinter Review
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+
I will land it and thanks for the review
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
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.
You need to log in before you can comment on or make changes to this bug.