705271 - changing iframe/browser type attribute from chrome to content after being added should throw error

Status: NEW

(Core :: XUL, defect)

Description

[Andrew Williamson [:eviljeff]]

The type attribute on XUL iframe and browser tags is settable to "content" after the element has been added to the DOM tree even though the level of access is fixed once the element has been added.

https://developer.mozilla.org/en/XUL/iframe#a-browser.type

Developers who do this can then mistakenly load remote websites into xul iframe/browsers as chrome when they believe they are safely loaded as content, creating a security risk for the user.

Setting the type attribute on an iframe/browser after being added to the document should throw an error to protect the user and alert the developer to the issue.

Comment 1

[Neil Deakin]

Note that the type attribute can be changed later, and is, for example, by the tabbrowser between 'content' and 'content-primary'

Comment 2

[Andrew Williamson [:eviljeff]]

summary updated slightly to reflect its just chrome -> content that's the issue.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Are we talking log an error to the console, or are we talking throw an exception?  I can see doing both, once we have a frameloader...

Comment 4

[Andrew Williamson [:eviljeff]]

I'd prefer throwing an exception - it would break some addons but arguably they're broken at the moment in a way.  

I'm cc'ing Jorge on this in case he has an opinion on addon compatibility.

Comment 5

[Jorge Villalobos [:jorgev] (he/him)]

I agree with making it an exception. Flagging for add-on compat, since we would need to communicate this when implemented.

Comment 6

[:Gijs (he/him)]

It looks like we sort of try to cope with this in https://searchfox.org/mozilla-central/source/dom/base/nsFrameLoader.cpp#3101 now, but that seems sketchy to me (and is intermingled with changes relating to the primary attribute). Nika, is there any reason we shouldn't just cache the type value when the XUL Frame Element's BindToTree fires and rely only on that instead of attribute gets? This would require reinsertion of the framing element if consumers want to change it, that seems OK to me, and it seems like some of the current code could be simplified if we did this.

Alternatively, if we really want to support changing this without reinsertion (what should happen to the browsingcontext? Presumably that needs recreating if we're changing the outcome of calling IsContent()...), perhaps we can clarify what this method is trying to do for changes to primary vs. type attributes?

Comment 7

[Nika Layzell [:nika] (ni? for response)]

(In reply to :Gijs (he/him) from comment #6)

It looks like we sort of try to cope with this in https://searchfox.org/mozilla-central/source/dom/base/nsFrameLoader.cpp#3101 now, but that seems sketchy to me (and is intermingled with changes relating to the primary attribute).

I think that method call is a bit deceptive, and is only used to control the mPrimaryContentShell attribute on the AppWindow object (https://searchfox.org/mozilla-central/rev/202a285024f174c2d2bf2152d9cba90a03723eab/xpfe/appshell/AppWindow.cpp#2030,2035,2043). It never actually changes the type of the inner nsDocShell, as that property is completely immutable.

Nika, is there any reason we shouldn't just cache the type value when the XUL Frame Element's BindToTree fires and rely only on that instead of attribute gets? This would require reinsertion of the framing element if consumers want to change it, that seems OK to me, and it seems like some of the current code could be simplified if we did this.

It's already required to re-insert the framing element if consumers want to change it, so that wouldn't be a significant change here. It's already sort-of cached, as when the BrowsingContext is created (which is performed within BindToTree from the nsFrameLoader's constructor), it reads the type attribute once, and caches it as the mType field on the created BrowsingContext, which is completely immutable (via https://searchfox.org/mozilla-central/rev/202a285024f174c2d2bf2152d9cba90a03723eab/dom/base/nsFrameLoader.cpp#237).

We should probably ensure that it's only read in this one place, and change other code which would read the attribute to instead inspect the BrowsingContext within the nsFrameLoader to achieve this same effect.