Implement HTML5 sandbox attribute for IFRAMEs

[Mechanical snail]

Firefox should support the HTML 5 sandbox attribute on <iframe>s:

sandbox HTML5 only
    If specified as an empty string, this attribute enables extra restrictions on the content that can appear in the inline frame. The value of the attribute can be a space-separated list of tokens that lift particular restrictions. Valid tokens are:

        allow-same-origin: Allows the content to be treated as being from the same origin as the containing document. If this keyword is not used, the embedded content is treated as being from a unique origin.
        allow-top-navigation: Allows the embedded browsing context to navigate (load) content from the top-level browsing context. If this keyword is not used, this operation is not allowed.
        allow-forms: Allows the embedded browsing context to submit forms. If this keyword is not used, this operation is not allowed.
        allow-scripts: Allows the embedded browsing context to run scripts (but not create pop-up windows). If this keyword is not used, this operation is not allowed.

[Nrlz (nrlz)]

User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4

IE has an attribute that can be specified on IFRAMEs that can lower the security zone of that iframe only. see: http://msdn.microsoft.com/workshop/author/dhtml/reference/properties/security.asp
It can be used to restrict javascript and other dynamic behaviors of content in that iframes.

I suggest we implement something similar, which can be a big help for webmail providers as it can provide a fallback mechanism incase they fail to filter out all dynamic (malicious) content from emails.

The current approaches to filtering out such content are:
1. filtering out all unsafe tags and styles (difficult)
2. loading the (semi-filtered) content in an iframe from another domain (to trigger the natural cross-domain security protection)

With the iframe security attribute, we can have the browser restrict the content which is much more efficient and error-proof.

In order to remain standards compliant, perhaps such a feature should be triggered with javascript instead of with HTML markup.

Reproducible: Always

[Jruderman (jruderman)]

Sounds like a good idea. Hixie, is there anything similar in W3C or WhatWG specs?

[Annevk (annevk)]

Triggering it with javascript would make it more standards compliant? :-O

FWIW, there is no such feature anywhere (yet).

[Ian-hixie (ian-hixie)]

WHATWG has had several proposals for sandboxing ideas like this. I haven't looked at any in detail yet; Gerv might know more.

[Jruderman (jruderman)]

*** Bug 372805 has been marked as a duplicate of this bug. ***

[Ian-ianmacfarlane (ian-ianmacfarlane)]

Preventing access to parent documents (parent, top, etc) would probably be sufficient for the large majority of use cases (throw a permission denied error to JavaScript trying to access this).

This isn't just useful for webmail providers, it's useful in a number of other cases, for example Yahoo! Images use it, and I'm currently writing an internal company tool that would benefit greatly from this. I'm sure there's tons of other use cases for embedding bits of other websites in a page.

I would dearly love to have this for Firefox 3.

[Gervase Markham (gerv-mozilla)]

See http://www.gerv.net/security/content-restrictions/, which allows you to make this sort of restriction.

Gerv

[Chottan-mottan (chottan-mottan)]

*** Bug 416574 has been marked as a duplicate of this bug. ***

[Dveditz (dveditz)]

We'd probably only implement this if it got standardized in HTML5. Would it make more sense to simply copy IE for broader compatibility, or come up with a more useful (more flexible? more powerful?) set of restrictions but not compatible?

[Dveditz (dveditz)]

HTML5 now has a "sandbox" attribute, different but the same kind of idea. Since HTML5 is on a standards track that's probably the more useful version of this idea, although it is by no means finalized.

[Dveditz (dveditz)]

*** Bug 458143 has been marked as a duplicate of this bug. ***

[Teoli2003 (teoli2003)]

It looks like the HTML5 IFrame's sandbox attribute has been implemented in Webkit (WebKit@51577) and is available in the latest Chrome 4 beta.

One question:
Should the title of this bug be updated to "[HTML5] Implement IFRAME's sandbox attribute" or should a new bug be filled?

[Abarth-mozilla (abarth-mozilla)]

> It looks like the HTML5 IFrame's sandbox attribute has been implemented in
> Webkit (WebKit@51577) and is available in the latest Chrome 4 beta.

The @sandbox attribute is not available in the beta channel, but it is available in the dev channel, which will become Chrome 5.

[Arturadib (arturadib)]

Because of its implementation in Chrome/Safari over the last several months, the HTML5 sandbox attribute is already getting a lot of attention from the community. Try Googling "html5 sandbox" to see how many articles are already dedicated to the topic.

Is Firefox en route to implementing HTML5's sandbox attribute any time soon?

[Lh-bennett (lh-bennett)]

This has now shown up in IE10's Preview #2.

[Xavier-grosjean (xavier-grosjean)]

While the feature is documented here:
https://developer.mozilla.org/en/HTML/Element/iframe
it does not seem to work, or at least "allow-top-navigation" does not work, since its absence still allows an iframe to modify the top window location.

Is there any hope at all ?

[Imelven-s (imelven-s)]

(In reply to Xavier from comment #15)
> While the feature is documented here:
> https://developer.mozilla.org/en/HTML/Element/iframe
> it does not seem to work, or at least "allow-top-navigation" does not work,
> since its absence still allows an iframe to modify the top window location.
>
> Is there any hope at all ?

i am (very slowly due to other tasks) working on this. there is no sandbox attribute support in Firefox currently. i hope to at least get some prototype/rough/experimental patches up to show possible implementation routes.

[Imelven-s (imelven-s)]

Created attachment 574383
WIP - experimental prototype v1

a Work In Progress patch - this is an experimental prototype implementing some very basic parts of the iframe sandbox. i'm continuing to iterate on this patch, i'm putting it up here in case others are interested and to possibly collect more feedback on the direction i'm going. there are many parts of the spec still yet to be implemented.

[Imelven-s (imelven-s)]

Created attachment 574480
WIP - experimental prototype

now with less silly bugs !

the next steps are to figure out more of the attribute handling using nsDOMSettableTokenList and add parsing for some of the values, probably starting with allow-scripts and allow-same-domain

[Imelven-s (imelven-s)]

Created attachment 575603
WIP - experimental prototype

this patch adds a possible implementation for allow-scripts and allow-same-origin. i'm looking at allow-top-navigation at the moment and then planning on moving on to allow-forms.

there are also many TODO's and questions in this patch still :)

[Imelven-s (imelven-s)]

Created attachment 575617
WIP - experimental prototype

a possible implementation for 'allow-forms'

still working on 'allow-top-navigation' - and there are still many details to iron out after that

[Imelven-s (imelven-s)]

Comment on attachment 575617
WIP - experimental prototype

this patch passes the browserscope.org iframe sandbox check fwiw :)

[Imelven-s (imelven-s)]

Microsoft has a public test suite for HTML5 iframe sandbox at http://samples.msdn.microsoft.com/ietestcenter/#html5Sandbox

the current prototype patch passes ~50% of the tests there (some of the failures are due to no allow-top-navigation yet, no cookie/localStorage restrictions, no video autoplay restriction yet, etc as a high level summary of what's still to be done)

as part of the work on implementing iframe sandbox, we will need mochitests of our own in the tree to test the functionality as outlined in the HTML5 spec. i will probably end up working on making this happen also.

[Imelven-s (imelven-s)]

Created attachment 578433
WIP - experimental prototype

adds a possible implementation of blocking window.open, <a target = "_blank"> and showModalDialog()

[Imelven-s (imelven-s)]

Created attachment 578799
WIP - experimental prototype

a possible implementation of allow-top-navigation that attempts to also implement the navigation rules

this patch passes 20 of the 30 tests MS test suite now. i need to look into why the XHR tests pass - i assume it's because we block XHR for a null principal, which a sandboxed iframe has and we let the iframe have its natural principal when allow-same-domain is specified.

the MS video autoplay tests don't work in Firefox since they use MP4/H264 format video which we don't support. the MS autofocus test passes in current Firefox, so i need to look into that more as well. i also would like to have some more testing around frame navigation that the tests in the MS suite (which this current patch passes).

i think i'm going start working on the test suite at this point due to the above.

it would be awesome if some people could take a look at the patch and tell me if the code is sane at all. there's a fair number of TODO's in it and a lot of questions around what to do if things fail.

[Khuey (khuey)]

Comment on attachment 578799
WIP - experimental prototype

Review of attachment 578799:
-----------------------------------------------------------------

Drive by comments. These should definitely not be taken as exhaustive.

Looks like this doesn't attempt to cover plugins? Also looks like this doesn't attempt to deal with scripts? I expect those are going to be a significant amount of work.

We should detect the situation called out in the first warning (and maybe the second too?) at http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#attr-iframe-sandbox and send a warning to the error console.

Reading the spec and the proto-patch here, I think storing the sandbox flags on both the docshell and the document is going to be a source of great pain. I think you're going to want one canonical set of sandbox flags (on the outer window, perhaps?).

Also, you really should request f? from a specific person. I gave an f-, but at this stage that is to be expected.

Finally, this is going to need lots of tests ...

::: content/base/public/nsContentUtils.h
@@ +1838,5 @@
> static bool SetUpChannelOwner(nsIPrincipal* aLoadingPrincipal,
> nsIChannel* aChannel,
> nsIURI* aURI,
> + bool aSetUpForAboutBlank,
> + bool aForceOwner);

You could make aForceOwner default to false here.

@@ +1864,5 @@
> + *
> + * @param aAttribute the value of the sandbox attribute as an nsIDOMDOMSettableTokenList
> + * @return the set of flags
> + */
> + static PRUint32 ParseSandboxAttributeToFlags(nsIDOMDOMSettableTokenList * aAttribute);

80 character lines please.

::: content/base/public/nsIDocument.h
@@ +462,5 @@
> }
>
> /**
> + * Set the sandbox flags for this document. This should only happen at
> + * at document load, the flags are immutable after being set at load time.

Can you assert that this is being called during load? (by checking for READYSTATE_LOADING or something?)

@@ +471,5 @@
> + mSandboxFlags = aSandboxFlags;
> + }
> +
> + /**
> + * Get the sandox flags for this document

sandbox

@@ +1786,5 @@
>
> + // The sandbox flags on the document. These reflect the value of the sandbox attribute of the
> + // associated IFRAME or CSP-protectable content, if existent. These are set at load time and
> + //are immutable - see nsDocShell.idl for the possible flags
> + PRUint32 mSandboxFlags;

nsIDocument needs an IID bump for this.

::: content/base/src/nsContentUtils.cpp
@@ +699,5 @@
> + * @param aAttribute the value of the sandbox attribute as an nsIDOMDOMSettableTokenList
> + * @return the set of flags
> + */
> +PRUint32
> +nsContentUtils::ParseSandboxAttributeToFlags(nsIDOMDOMSettableTokenList * aAttribute)

You should rewrite this function to use nsCharSeparatedTokenizer<nsContentUtils::IsHTMLWhitespace>. As it is it'll match things like "allow-scriptsallow-same-origin" which are not allowed per spec.

I'd give extra points for putting a

typedef nsCharSeparatedTokenizer<nsContentUtils::IsHTMLWhitespace> HTMLSplitOnSpacesTokenizer (or somethi...

[Khuey (khuey)]

> 3) Fail closed (-1 effectively does this if somebody forgets the check).

Actually, -1 does not do this (it does the opposite!). I had the polarity of the flags wrong. That said, I think after you rewrite this function nothing it in will be fallible and you won't need to care.

[Ms2ger (ms2ger)]

Comment on attachment 578799
WIP - experimental prototype

>--- a/content/base/public/nsIDocument.h
>+++ b/content/base/public/nsIDocument.h
> /**
>+ * Set the sandbox flags for this document. This should only happen at
>+ * at document load, the flags are immutable after being set at load time.
>+ * @ see nsDocShell.idl for the possible flags

nsDocShell.idl doesn't exist. (Three times or so.)

>--- a/content/base/src/nsFrameLoader.cpp
>+++ b/content/base/src/nsFrameLoader.cpp
>+ nsCOMPtr<nsIDocShellTreeItem> curDocShellItem(do_QueryInterface(mDocShell));

'nsCOMPtr<nsIDocShellTreeItem> curDocShellItem = do_QueryInterface(mDocShell);'

>+ // is this an <iframe> with a sandbox attribute or a frame whose parent
>+ // is sandboxed ?
>+ if (mOwnerContent->IsHTML(nsGkAtoms::iframe) &&
>+ (parentSandboxFlags || mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::sandbox, sandboxAttr))) {

Instead of this, add a nsHTMLIFrameElement* nsHTMLIFrameElement::FromContent(nsIContent*) and expose a method on nsHTMLIFrameElement that returns the flags, and move the code you added to nsContentUtils there. Also use it in nsHTMLIFrameElement::AfterSetAttr.

>+ PRUint32 sandboxFlags = 0;
>+
>+ nsCOMPtr<nsIDOMHTMLIFrameElement> iframe = do_QueryInterface(mOwnerContent);
>+
>+ if (iframe) {
>+ nsCOMPtr<nsIDOMDOMSettableTokenList> sandbox;
>+
>+ nsresult result = iframe->GetSandbox(getter_AddRefs(sandbox));
>+ // TODO : what to do if this fails ? (see below also)
>+
>+ sandboxFlags = nsContentUtils::ParseSandboxAttributeToFlags(sandbox);
>+ // TODO: what to do if this fails ? the content was intended to be sandboxed
>+ // and it may not be - maybe fail closed (all restrictions) ???
>+ }
>+
>+ if (parentSandboxFlags) {
>+ // do the intersection with the current flags in the docshell!

Doesn't sandboxFlags &= parentSandboxFlags do the right thing, and if not, why not?

>+ }
>+
>+ else {

} else {

>--- a/content/html/content/src/nsHTMLIFrameElement.cpp
>+++ b/content/html/content/src/nsHTMLIFrameElement.cpp
>+ virtual nsresult AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>+ const nsAString* aValue,
>+ bool aNotify);

Indentation

>--- a/docshell/base/nsDocShell.cpp
>+++ b/docshell/base/nsDocShell.cpp
>+ // If the content is sandboxed, force the passed in owner (which should be
>+ // a null principal) to be set on the channel, UNLESS the sandbox allow-same-domain flag is also
>+ // set. In that case, allow the content to asssume its "natural domain" as it
>+ // normally would.
>+ if ((mSandboxFlags & SANDBOX_FLAGS_SANDBOXED) &&
>+ (mSandboxFlags & SANDBOX_FLAGS_ALLOW_SAME_DOMAIN) != SANDBOX_FLAGS_ALLOW_SAME_DOMAIN) {
>+ nsContentUtils::SetUpChannelOwner(ownerPrincipal, channel, aURI, true, true);
>+ }
>+ else {
>+ nsContentUtils::SetUpChannelOwner(ownerPrincipal, channel, aURI, true, false);
>+ }

nsContentUtils::SetUpChannelOwner(ownerPrincipal, channel, aURI, true,
                                  (mSandboxFlags & SANDBOX_FLAGS_SANDBOXED) &&
             ...

[Imelven-s (imelven-s)]

Thanks very much for the feedback khuey and Ms2ger !

Adding a note here that on try this crashed in a mochitest :

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/browser_navigateaway2.js | Exited with code 1 during test run
PROCESS-CRASH | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/browser_navigateaway2.js | application crashed (minidump found)
Thread 0 (crashed)

[Imelven-s (imelven-s)]

(In reply to Kyle Huey [:khuey] (<email address hidden>) from comment #25)
> Comment on attachment 578799 [diff] [details] [review]
> WIP - experimental prototype
>
> Review of attachment 578799 [diff] [details] [review]:
> -----------------------------------------------------------------
>
> Drive by comments. These should definitely not be taken as exhaustive.
>
> Looks like this doesn't attempt to cover plugins? Also looks like this
> doesn't attempt to deal with scripts? I expect those are going to be a
> significant amount of work.

Plugins and scripts are both handled by this patch. The frame loader code does :
+ if (!(sandboxFlags & nsIDocShell::SANDBOX_FLAGS_ALLOW_SCRIPTS)) {
+ mDocShell->SetAllowJavascript(false);
+ }
+
+ // _always_ disallow plugins when sandboxed
+ mDocShell->SetAllowPlugins(false);

> We should detect the situation called out in the first warning (and maybe
> the second too?) at
> http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-
> element.html#attr-iframe-sandbox and send a warning to the error console.

This sounds really good to me.

> Reading the spec and the proto-patch here, I think storing the sandbox flags
> on both the docshell and the document is going to be a source of great pain.
> I think you're going to want one canonical set of sandbox flags (on the
> outer window, perhaps?).

Yeah, i discussed the docshell/document split with bz initially - i need to go back and reread those emails. We also use the flags on the document when doing checks. We may not need to store them on the docshell at all ? It seems like it would be better to just read the current value of the attribute when the load happens and use that as the fixed set of flags.

> Also, you really should request f? from a specific person. I gave an f-,
> but at this stage that is to be expected.

Right. I was going to ping bz, sicking and smaug (those are the folks i have been emailing with discussing this work) to see who were good folks to ask specifically for feedback - i _really_ appreciate you giving drive by feedback before i got to that though ! :)

> Finally, this is going to need lots of tests ...

I wholeheartedly agree - a test suite is needed for this to land, bz also suggested submitting our tests back to the W3C as well.

> ::: content/base/public/nsContentUtils.h
> @@ +1838,5 @@
> > static bool SetUpChannelOwner(nsIPrincipal* aLoadingPrincipal,
> > nsIChannel* aChannel,
> > nsIURI* aURI,
> > + bool aSetUpForAboutBlank,
> > + bool aForceOwner);
>
> You could make aForceOwner default to false here.

will do.

> @@ +1864,5 @@
> > + *
> > + * @param aAttribute the value of the sandbox attribute as an nsIDOMDOMSettableTokenList
> > + * @return the set of flags
> > + */
> > + static PRUint32 ParseSandboxAttributeToFlags(nsIDOMDOMSettableTokenList * aAttribute);
>
> 80 character lines please.

will fix.

> ::: content/base/public/nsIDocument.h
> @@ +462,5 @@
> > }
> >
> > /**
> > + * Set the sandbox flags for this docume...

[Imelven-s (imelven-s)]

(In reply to Ms2ger from comment #27)
> Comment on attachment 578799 [diff] [details] [review]
> WIP - experimental prototype

Again, thanks very much for jumping on this patch and giving me feedback, it's greatly appreciated :)

> >--- a/content/base/public/nsIDocument.h
> >+++ b/content/base/public/nsIDocument.h
> > /**
> >+ * Set the sandbox flags for this document. This should only happen at
> >+ * at document load, the flags are immutable after being set at load time.
> >+ * @ see nsDocShell.idl for the possible flags
>
> nsDocShell.idl doesn't exist. (Three times or so.)

oops, sorry, will fix.

> >--- a/content/base/src/nsFrameLoader.cpp
> >+++ b/content/base/src/nsFrameLoader.cpp
> >+ nsCOMPtr<nsIDocShellTreeItem> curDocShellItem(do_QueryInterface(mDocShell));
>
> 'nsCOMPtr<nsIDocShellTreeItem> curDocShellItem =
> do_QueryInterface(mDocShell);'

thanks, will fix.

> >+ // is this an <iframe> with a sandbox attribute or a frame whose parent
> >+ // is sandboxed ?
> >+ if (mOwnerContent->IsHTML(nsGkAtoms::iframe) &&
> >+ (parentSandboxFlags || mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::sandbox, sandboxAttr))) {
>
> Instead of this, add a nsHTMLIFrameElement*
> nsHTMLIFrameElement::FromContent(nsIContent*) and expose a method on
> nsHTMLIFrameElement that returns the flags, and move the code you added to
> nsContentUtils there. Also use it in nsHTMLIFrameElement::AfterSetAttr.

ok, i took a look at an example FromContent() on another element and I think I understand
what you'd like me to do here. I put the code to parse the flags etc in nsContentUtils so it could be used by CSP Sandbox (bug 671389) - does that change your opinion at all on how to rework this ? (that's why i didn't put the flag parsing in the iframe element originally). We also discussed support @sandbox on <frame>, which it seemed like there was interest in (just as an fyi).

> >+ PRUint32 sandboxFlags = 0;
> >+
> >+ nsCOMPtr<nsIDOMHTMLIFrameElement> iframe = do_QueryInterface(mOwnerContent);
> >+
> >+ if (iframe) {
> >+ nsCOMPtr<nsIDOMDOMSettableTokenList> sandbox;
> >+
> >+ nsresult result = iframe->GetSandbox(getter_AddRefs(sandbox));
> >+ // TODO : what to do if this fails ? (see below also)
> >+
> >+ sandboxFlags = nsContentUtils::ParseSandboxAttributeToFlags(sandbox);
> >+ // TODO: what to do if this fails ? the content was intended to be sandboxed
> >+ // and it may not be - maybe fail closed (all restrictions) ???
> >+ }
> >+
> >+ if (parentSandboxFlags) {
> >+ // do the intersection with the current flags in the docshell!
>
> Doesn't sandboxFlags &= parentSandboxFlags do the right thing, and if not,
> why not?

yeah. as i mentioned above, i think &= is the right thing here. i will fix this.

> >+ }
> >+
> >+ else {
>
> } else {
>
> >--- a/content/html/content/src/nsHTMLIFrameElement.cpp
> >+++ b/content/html/content/src/nsHTMLIFrameElement.cpp
> >+ virtual nsresult AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
> >+ const nsAString* aValue,
> >+ bool aNotify);
>
> Indentation

wil...

[Imelven-s (imelven-s)]

(In reply to Kyle Huey [:khuey] (<email address hidden>) from comment #26)
> > 3) Fail closed (-1 effectively does this if somebody forgets the check).
>
> Actually, -1 does not do this (it does the opposite!). I had the polarity
> of the flags wrong. That said, I think after you rewrite this function
> nothing it in will be fallible and you won't need to care.

re: the polarity of the flags, in addition to all the other stuff i need to address from the feedback from you and ms2ger, i think i want to rework the patch so the flags have the exact same semantics as the HTML5 spec. i think i just resisted their blacklisting approach to sandbox permissions. i think it's most important to follow the spec here though.

[Khuey (khuey)]

(In reply to Ian Melven :imelven from comment #29)
> (In reply to Kyle Huey [:khuey] (<email address hidden>) from comment #25)
> > Comment on attachment 578799 [diff] [details] [review] [diff] [details] [review]
> > WIP - experimental prototype
> >
> > Review of attachment 578799 [diff] [details] [review] [diff] [details] [review]:
> > -----------------------------------------------------------------
> >
> > Drive by comments. These should definitely not be taken as exhaustive.
> >
> > Looks like this doesn't attempt to cover plugins? Also looks like this
> > doesn't attempt to deal with scripts? I expect those are going to be a
> > significant amount of work.
>
> Plugins and scripts are both handled by this patch. The frame loader code
> does :
> + if (!(sandboxFlags & nsIDocShell::SANDBOX_FLAGS_ALLOW_SCRIPTS)) {
> + mDocShell->SetAllowJavascript(false);
> + }
> +
> + // _always_ disallow plugins when sandboxed
> + mDocShell->SetAllowPlugins(false);

Ok. For plugins you'll want tests for both plugins (embed, object, etc) and plugin documents (a subframe whose URL is a type loaded by a plugin).

For JavaScript, does disabling JS on the docshell affect XBL bindings? That'll break video controls, among other things.

It looks like SetAllowPlugins(false) and SetAllowJavascript(false) will block plugin and javascript *loads* in addition to execution/instantiation, which I don't believe is the desired behavior here (this is script observable too, at least for plugin subdocuments (do <script>s have load events?)).

> > Finally, this is going to need lots of tests ...
>
> I wholeheartedly agree - a test suite is needed for this to land, bz also
> suggested submitting our tests back to the W3C as well.

Indeed.

> > ::: content/base/src/nsContentUtils.cpp
> > @@ +699,5 @@
> > > + * @param aAttribute the value of the sandbox attribute as an nsIDOMDOMSettableTokenList
> > > + * @return the set of flags
> > > + */
> > > +PRUint32
> > > +nsContentUtils::ParseSandboxAttributeToFlags(nsIDOMDOMSettableTokenList * aAttribute)
> >
> > You should rewrite this function to use
> > nsCharSeparatedTokenizer<nsContentUtils::IsHTMLWhitespace>. As it is it'll
> > match things like "allow-scriptsallow-same-origin" which are not allowed per
> > spec.
> >
> > I'd give extra points for putting a
> >
> > typedef nsCharSeparatedTokenizer<nsContentUtils::IsHTMLWhitespace>
> > HTMLSplitOnSpacesTokenizer (or something)
> >
> > in nsContentUtils.h and changing the existing users in the tree.
>
> yeah, this patch fails the MS test on whitespace etc. so i knew there were
> some issues here - thanks for the suggestion. i'm not opposed to doing the
> HTMLSplitOnSpacesTokenizer approach here, i could split off the work to
> change existing users and add the typedef as a separate blocking bug and
> knock that out first.

Yeah, you don't need to do that here (or even before). But filing a bug for it so someone else can do it would be nice ;-) Might be a decent good first bug.

> > ::: content/html/content/src/nsHTMLIFrameElement.cpp
> > @@ +306,5 @@
> > > + bool aNotify)
> > > +{
> > > + if (aN...

[Roc-ocallahan (roc-ocallahan)]

(In reply to Kyle Huey [:khuey] (<email address hidden>) from comment #32)
> For JavaScript, does disabling JS on the docshell affect XBL bindings?
> That'll break video controls, among other things.

That is true but it's not Ian's problem.

[Bugzilla-x-0x (bugzilla-x-0x)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #33)
>
> That is true but it's not Ian's problem.

But it's a problem that might become more visible once this here is implemented.
Bug 449358 is tracking it BTW.

[Imelven-s (imelven-s)]

Created attachment 585561
WIP - reworked prototype

- nsIDocument, nsIDocShell, nsIDOMHTMLIFrameElement IIDs bumped

- added nsHTMLIFrameElement::FromContent(nsIContent*)

- added PRUint32 nsHTMLIFrameElement::GetSandboxFlags()

- to better match the HTML5 spec, break the flags up more, reverse their polarity and rename them.

- change sandbox attribute parsing to use nsCharSeparatedTokenizerTemplate<nsContentUtils::IsHTMLWhitespace>

- add NULL check to fix crash with chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/browser_navigateaway2.js

- lots of other misc. cleanup from feedback from ms2ger and khuey

next up:

doing some more debugging of the MS test suite tests that I think should be passing but are not - and then moving on to document.cookie and blocking it appropriately.

The big outstanding question at the moment : do I want to make the tests for this manual (as the Microsoft test suite is) or attempt to make automated tests ? Manual tests have the problem that they don't always get run on every checkin - automated tests would need (I think) some sort of event that happens when something is blocked (CSP does this with an observable notification, for example) - the con here is this doesn't test the thing attempting to be blocked actually is blocked, just that the event was fired.

[Imelven-s (imelven-s)]

Created attachment 585605
WIP - prototype

Do case insensitive compare when looking at sandbox attribute and comparing it to the appropriate atoms. Makes another of the MS sandbox test cases pass (http://samples.msdn.microsoft.com/ietestcenter/html5/sandbox/sandbox-attribute-value-tokens-separated-by-space-characters.htm)

[Imelven-s (imelven-s)]

Also need to re-examine keeping the flags on the docshell and the document. Right now this patch essentially only uses the docshell flags to pass the correct flags on to the document when its loaded. The frame loader always checks (and parses) the actual current value of the sandbox attribute on the IFRAME being loaded and uses this to set the flags on the docshell - if this seems acceptable, the AfterSetAttr() code in nsHTMLIFrameElement needs to be removed.

[Roc-ocallahan (roc-ocallahan)]

(In reply to Ian Melven :imelven from comment #35)
> Manual tests have the problem that they don't always get run on
> every checkin - automated tests would need (I think) some sort of event that
> happens when something is blocked (CSP does this with an observable
> notification, for example) - the con here is this doesn't test the thing
> attempting to be blocked actually is blocked, just that the event was fired.

Automated tests of course :-). Manual tests are a bit useless.

I think most of the features can be automatically checked fairly easily. In some cases you may need chrome privileges to inject synthetic events into the sandboxed document. Are there any features that you can't think of a way to test automatically?

[Imelven-s (imelven-s)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #38)
> (In reply to Ian Melven :imelven from comment #35)
> > Manual tests have the problem that they don't always get run on
> > every checkin - automated tests would need (I think) some sort of event that
> > happens when something is blocked (CSP does this with an observable
> > notification, for example) - the con here is this doesn't test the thing
> > attempting to be blocked actually is blocked, just that the event was fired.
>
> Automated tests of course :-). Manual tests are a bit useless.
>
> I think most of the features can be automatically checked fairly easily. In
> some cases you may need chrome privileges to inject synthetic events into
> the sandboxed document. Are there any features that you can't think of a way
> to test automatically?

Thanks for the feedback ! Can't think of anything specific offhand that I'm certain will be impossible to test automatically, maybe blocking video autoplay ? I'll see how it shakes out when I attempt writing the automated tests.

[Imelven-s (imelven-s)]

(In reply to Olli Pettay [:smaug] from comment #160)
> Comment on attachment 625329
> iframe sandbox v15
>
> ...which reminds me, does the patch handle about:blank document properly?
> Those documents which are created in
> nsDocShell::CreateAboutBlankContentViewer

I tested manually, and it looks to me like about:blank is correctly sandboxed.

[Imelven-s (imelven-s)]

(In reply to David-Sarah Hopwood from comment #183)
> (In reply to Ian Melven :imelven from comment #182)
> > IE 10 and Webkit (Chome) do both throw exceptions (security error) on
> > accessing document.cookie from a sandboxed document, even when the site has
> > not set any cookies.
>
> OK, can someone raise it with WHATWG to get the spec changed for the
> no-cookie case?

I posted to the WHATWG list, thanks for bringing this up ! Adam Barth agrees that the existing behavior, which is as you described, makes sense, as do I.

[Imelven-s (imelven-s)]

Created attachment 633731
iframe sandbox tests - inheritance v5

Fix the problem with the inheritance tests never finishing - carrying over r+

[Imelven-s (imelven-s)]

Created attachment 633735
iframe sandbox v17

I checked manually to make sure plugins behave as expected when adding and removing a sandbox attribute and think I've addressed the previous round of review comments.

[R-bugs-h (r-bugs-h)]

Comment on attachment 633735
iframe sandbox v17

>+ static PRUint32 ParseSandboxAttributeToFlags(const nsAString&
>+ aSandboxAttr);
Odd indentation. aSandboxAttr should fit in to the previous line.

>+ if (token.LowerCaseEqualsLiteral("allow-scripts")) {
>+ out &= ~SANDBOXED_SCRIPTS;
>+ } else if (token.LowerCaseEqualsLiteral("allow-same-origin")) {
>+ out &= ~SANDBOXED_ORIGIN;
>+ } else if (token.LowerCaseEqualsLiteral("allow-forms")) {
>+ out &= ~SANDBOXED_FORMS;
>+ } else if (token.LowerCaseEqualsLiteral("allow-scripts")) {
>+ // allow-scripts removes both SANDBOXED_SCRIPTS and
>+ // SANDBOXED_AUTOMATIC_FEATURES.
>+ out &= ~SANDBOXED_SCRIPTS;
>+ out &= ~SANDBOXED_AUTOMATIC_FEATURES;
>+ } else if (token.LowerCaseEqualsLiteral("allow-top-navigation")) {
>+ out &= ~SANDBOXED_TOPLEVEL_NAVIGATION;
>+ }
>+ }
>+ }

So why you don't handle allow-popups?
And why you handle allow-scripts twice?
Do you have any tests for not having SANDBOXED_AUTOMATIC_FEATURES?

[Imelven-s (imelven-s)]

allow-popups seems to have made it into the spec - i would like to add it to this initial implementation.

[Imelven-s (imelven-s)]

After a little more thought, i think it makes more sense to focus on landing this initial iframe sandbox support and handling allow-popups in a follow up, i've filed bug 766282 as this follow up. Right now, the current patch for this bug always blocks popups, so this is a 'fail closed' situation - there's just no way to opt in to allowing popups yet.

As discussed with smaug on irc, there's no automatic features tests yet because automatic features are also intended to be done in a followup, which is bug 752551.

[Imelven-s (imelven-s)]

Created attachment 634584
iframe sandbox v18

(In reply to Olli Pettay [:smaug] from comment #188)
> Comment on attachment 633735
> iframe sandbox v17
>
>
> >+ static PRUint32 ParseSandboxAttributeToFlags(const nsAString&
> >+ aSandboxAttr);
> Odd indentation. aSandboxAttr should fit in to the previous line.

oops, it does, fixed - thanks !

> So why you don't handle allow-popups?

filed a followup for allow-popups, see previous comment for details

> And why you handle allow-scripts twice?

oops - thanks for catching this ! fixed in this patch.

> Do you have any tests for not having SANDBOXED_AUTOMATIC_FEATURES?

explained in previous comment

thanks !

[Imelven-s (imelven-s)]

Created attachment 634620
workaround for workers needing a string origin v2

unbitrot, carrying over r+

[Imelven-s (imelven-s)]

I was curious just how dependent on 754202 these patches are so i pushed just the iframe sandbox tests to try : https://tbpl.mozilla.org/?tree=Try&rev=b4c4bdc7cb94

the results look pretty good, actually. i'm going to try using a Nightly with iframe sandbox support for a bit to get a better sense of the stability.

[Imelven-s (imelven-s)]

Dev suggested making SetSandboxFlags fail if the flags would be less restrictive than the currently are - with a special case for clearing the flags completely. This seems like a nice little bit of defense in depth, I'll look at adding this some time in the near future, could always be done as a quick followup too.

[Dev-akhawe+mozilla (dev-akhawe+mozilla)]

Further, a 'IntersectSandboxFlags' function that implements the intersection algorithm that you implemented for nested sandboxes, which I can then use for CSP Sandbox.

[Imelven-s (imelven-s)]

(In reply to Devdatta Akhawe from comment #195)
> Further, a 'IntersectSandboxFlags' function that implements the intersection
> algorithm that you implemented for nested sandboxes, which I can then use
> for CSP Sandbox.

that's just a OR between the parent and the child's flags, since the flags always add restrictions, see the code in nsFrameLoader in the patch

[Imelven-s (imelven-s)]

Looking at the try results again I got "15217 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_iframe_sandbox_plugins.html | an unexpected uncaught JS exception reported through window.onerror - TypeError: p.startWatchingInstanceCount is not a function at http://mochi.test:8888/tests/content/html/content/test/test_iframe_sandbox_plugins.html:31" on Android opt.

[R-bugs-h (r-bugs-h)]

Comment on attachment 634584
iframe sandbox v18

>+
>+ // Set up the actual sandboxing for plugins as specified.
>+ if (sandboxFlags & SANDBOXED_PLUGINS) {
>+ mDocShell->SetAllowPlugins(false);
>+ }
>+ }
>+ }
So it is never possible to enable plugins if they are once disabled for an iframe?

>
>+ // If this document is sandboxed without 'allow-scripts', abort.
>+ if (mDocument->SandboxFlags() & SANDBOXED_SCRIPTS) {
>+ // REVIEW : this probably isn't the right error..
>+ return NS_ERROR_DOM_SECURITY_ERR;
>+ }
Do we want to return error at all? Would NS_OK work - just don't execute or load the script?

[Dev-akhawe+mozilla (dev-akhawe+mozilla)]

I think throwing a security error is useful. Say I wanted to allow scripts but had a typo and ended up saying allw-scripts. It is useful to debug that during testing if the console shows me a security error "refused to execute scripts"

[Imelven-s (imelven-s)]

(In reply to Olli Pettay [:smaug] from comment #198)
> Comment on attachment 634584
> iframe sandbox v18
>
> >+
> >+ // Set up the actual sandboxing for plugins as specified.
> >+ if (sandboxFlags & SANDBOXED_PLUGINS) {
> >+ mDocShell->SetAllowPlugins(false);
> >+ }
> >+ }
> >+ }
> So it is never possible to enable plugins if they are once disabled for an
> iframe?

as discussed on IRC, if the sandbox attribute is removed from the iframe, plugins should be re-enabled for the next document loaded - reading back through the comments, i realized i tested the other case (adding a sandbox attribute and reloading blocks plugins) but not this case. I'll fix this case and see if I can add a test to the mochitests for it as well.

> >
> >+ // If this document is sandboxed without 'allow-scripts', abort.
> >+ if (mDocument->SandboxFlags() & SANDBOXED_SCRIPTS) {
> >+ // REVIEW : this probably isn't the right error..
> >+ return NS_ERROR_DOM_SECURITY_ERR;
> >+ }
> Do we want to return error at all? Would NS_OK work - just don't execute or
> load the script?

The closest piece of the spec I can find says :

"If scripting is disabled for browsing context passed to this algorithm, then abort these steps, as if the script did nothing but return void."

which implies to me there should not be an error. It's probably a good idea for me to check what the other browsers do in this case as well.

[Imelven-s (imelven-s)]

(In reply to Ian Melven :imelven from comment #200)
> > >
> > >+ // If this document is sandboxed without 'allow-scripts', abort.
> > >+ if (mDocument->SandboxFlags() & SANDBOXED_SCRIPTS) {
> > >+ // REVIEW : this probably isn't the right error..
> > >+ return NS_ERROR_DOM_SECURITY_ERR;
> > >+ }
> > Do we want to return error at all? Would NS_OK work - just don't execute or
> > load the script?
>
> The closest piece of the spec I can find says :
>
> "If scripting is disabled for browsing context passed to this algorithm,
> then abort these steps, as if the script did nothing but return void."
>
> which implies to me there should not be an error. It's probably a good idea
> for me to check what the other browsers do in this case as well.

I verified that Chrome and IE 10 don't throw an error in this case, i'll change the patch to return NS_OK as suggested when i clean up the plugin stuff. Thanks for the suggestion here, smaug !

[Daira Hopwood (daira)]

It seems to me that the spec (and other browsers) saying not to throw a visible error, does not imply that there can't or shouldn't be a warning on the console. Warnings are purely a quality of implementation issue, and I agree with Devdatta Akhawe in comment:199 that it might help debugging.

[Imelven-s (imelven-s)]

(In reply to David-Sarah Hopwood from comment #202)
> It seems to me that the spec (and other browsers) saying not to throw a
> visible error, does not imply that there can't or shouldn't be a warning on
> the console. Warnings are purely a quality of implementation issue, and I
> agree with Devdatta Akhawe in comment:199 that it might help debugging.

That's a reasonable point, i suggest this would be best as a followup, please feel free to file a bug for the warning, dependent on this bug. There's already another followup bug to add logging when an iframe's sandbox attribute is such that the sandbox could be removed, see bug 752559.

[Daira Hopwood (daira)]

(In reply to Ian Melven :imelven from comment #203)
> That's a reasonable point, i suggest this would be best as a followup,
> please feel free to file a bug for the warning, dependent on this bug.

Filed as bug 768664.

[Imelven-s (imelven-s)]

(In reply to David-Sarah Hopwood from comment #204)
> (In reply to Ian Melven :imelven from comment #203)
> > That's a reasonable point, i suggest this would be best as a followup,
> > please feel free to file a bug for the warning, dependent on this bug.
>
> Filed as bug 768664.

Thank you for filing ! I like that the bug is quite general - and it fits in nicely with our efforts to do more logging around CSP and mixed content issues in the console as well.

[Imelven-s (imelven-s)]

(In reply to Ian Melven :imelven from comment #197)
> Looking at the try results again I got "15217 ERROR TEST-UNEXPECTED-FAIL |
> /tests/content/html/content/test/test_iframe_sandbox_plugins.html | an
> unexpected uncaught JS exception reported through window.onerror -
> TypeError: p.startWatchingInstanceCount is not a function at
> http://mochi.test:8888/tests/content/html/content/test/
> test_iframe_sandbox_plugins.html:31" on Android opt.

i spoke to jmaher on irc and it seems that plugin tests should be skipped on Android. i'll take care of that in the updated plugin test patch i'm working on.

[Imelven-s (imelven-s)]

Created attachment 638540
workaround for workers needing a string origin v3

Unbitrot, carrying over r+

[Imelven-s (imelven-s)]

Created attachment 638541
iframe sandbox v19

* Add a flag to docshell to track if a sandbox attribute was cleared from a document - if so, plugins need to be re-enabled for the next document loaded in the docshell. I'm open to other ways of doing this, so please let me know if another approach is preferred.

* Change the script blocking to return NS_OK as discussed in comment 200 and comment 201

[Imelven-s (imelven-s)]

Created attachment 638547
iframe sandbox tests - plugins v3

Add these tests :

* test that when a plugin is loaded in an unsandboxed iframe, a sandbox attribute is then added to the iframe and the document containing the plugin is reloaded, the plugin does not load in the sandboxed iframe

* test that when when a sandboxed iframe's sandbox attribute is removed,
and a new document is loaded into the iframe, the plugin loads

I still need to make these tests be skipped on Android, will do that shortly.

[Imelven-s (imelven-s)]

Created attachment 638823
iframe sandbox tests - plugins v4

Skip the plugin tests on Android (thanks for the help jmaher!)

Johnny, not sure if adding the new tests warrants more review so I r?'d to you instead of carrying over the previous r+ :)

[R-bugs-h (r-bugs-h)]

Comment on attachment 638541
iframe sandbox v19

>+ // If this document is being loaded by a docshell, copy its sandbox flags
>+ // to the document. These are immutable after being set here.
>+ nsCOMPtr<nsIDocShell> docShell = do_QueryInterface(aContainer);
>+
>+ if (docShell) {
>+ nsresult rv = docShell->GetSandboxFlags(&mSandboxFlags);
>+ NS_ENSURE_SUCCESS(rv, rv);
>+
>+ // Additionally, if this docshell had its document's sandbox
>+ // attribute removed prior to this load, we need to re-enable
>+ // plugins for this docshell.
>+ bool sandboxCleared;
>+ if (NS_SUCCEEDED(docShell->GetSandboxCleared(&sandboxCleared)) &&
>+ sandboxCleared) {
>+ docShell->SetAllowPlugins(true);
>+ docShell->SetSandboxCleared(false);
>+ }
>+ }
This doesn't work if you remove sandbox attribute and add it then back and then load a new document.

>+ // Sandboxed document check: javascript: URI's are disabled
>+ // in a sandboxed document unless 'allow-scripts' was specified.
>+ nsCOMPtr<nsIDocument> doc = do_QueryInterface(aOriginalInnerWindow->GetExtantDocument());
No need for nsCOMPtr if you use aOriginalInnerWindow->GetExtantDoc()

[Imelven-s (imelven-s)]

(In reply to Olli Pettay [:smaug] from comment #211)
> Comment on attachment 638541
> iframe sandbox v19
>
>
> >+ // If this document is being loaded by a docshell, copy its sandbox flags
> >+ // to the document. These are immutable after being set here.
> >+ nsCOMPtr<nsIDocShell> docShell = do_QueryInterface(aContainer);
> >+
> >+ if (docShell) {
> >+ nsresult rv = docShell->GetSandboxFlags(&mSandboxFlags);
> >+ NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+ // Additionally, if this docshell had its document's sandbox
> >+ // attribute removed prior to this load, we need to re-enable
> >+ // plugins for this docshell.
> >+ bool sandboxCleared;
> >+ if (NS_SUCCEEDED(docShell->GetSandboxCleared(&sandboxCleared)) &&
> >+ sandboxCleared) {
> >+ docShell->SetAllowPlugins(true);
> >+ docShell->SetSandboxCleared(false);
> >+ }
> >+ }
> This doesn't work if you remove sandbox attribute and add it then back and
> then load a new document.

Gah, thanks for catching that.. I'll fix that. Seems like I can handle that case by doing a SetSandboxCleared(false) in AfterSetAttr if the attribute is being added.

> >+ // Sandboxed document check: javascript: URI's are disabled
> >+ // in a sandboxed document unless 'allow-scripts' was specified.
> >+ nsCOMPtr<nsIDocument> doc = do_QueryInterface(aOriginalInnerWindow->GetExtantDocument());
> No need for nsCOMPtr if you use aOriginalInnerWindow->GetExtantDoc()

cool, i'll fix that up too.

Thanks for your ongoing reviews !

[Dev-akhawe+mozilla (dev-akhawe+mozilla)]

In bug 671389, we apply the sandbox flags to the document and not the docshell (since in CSP, the sandbox is only for the document). Relying on docshell flags for enforcing the "disallow plugins" requirement, won't work for CSP sandbox. Does anyone have a suggestion for how to fix this concern?

[Imelven-s (imelven-s)]

Created attachment 639552
iframe sandbox v20

This fixes the issue discussed in comment 211 and 212 involving adding and removing the sandbox attribute and correctly enabling and disabling plugins. I tested adding and removing the attribute in various combinations and then navigating the sandbox document.

I did a try run earlier this week and there were problems with some of the tests timing out, I'm looking into that now.

[R-bugs-h (r-bugs-h)]

Comment on attachment 639552
iframe sandbox v20

>+ // If this document is being loaded by a docshell, copy its sandbox flags
>+ // to the document. These are immutable after being set here.
>+ nsCOMPtr<nsIDocShell> docShell = do_QueryInterface(aContainer);
>+
>+ if (docShell) {
>+ nsresult rv = docShell->GetSandboxFlags(&mSandboxFlags);
>+ NS_ENSURE_SUCCESS(rv, rv);
>+
>+ // Additionally, if this docshell had its document's sandbox
>+ // attribute removed prior to this load, we need to re-enable
>+ // plugins for this docshell.
>+ bool sandboxCleared;
>+ if (NS_SUCCEEDED(docShell->GetSandboxCleared(&sandboxCleared)) &&
>+ sandboxCleared) {
>+ docShell->SetAllowPlugins(true);
>+ docShell->SetSandboxCleared(false);

This looks wrong. What if user has explicitly disabled all the plugins?
Also, I don't understand how the sandboxCleared handles the case when sandbox attribute
isn't removed but just changed.

[Imelven-s (imelven-s)]

(In reply to Olli Pettay [:smaug] from comment #215)
> Comment on attachment 639552
> iframe sandbox v20
>
>
> >+ // If this document is being loaded by a docshell, copy its sandbox flags
> >+ // to the document. These are immutable after being set here.
> >+ nsCOMPtr<nsIDocShell> docShell = do_QueryInterface(aContainer);
> >+
> >+ if (docShell) {
> >+ nsresult rv = docShell->GetSandboxFlags(&mSandboxFlags);
> >+ NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+ // Additionally, if this docshell had its document's sandbox
> >+ // attribute removed prior to this load, we need to re-enable
> >+ // plugins for this docshell.
> >+ bool sandboxCleared;
> >+ if (NS_SUCCEEDED(docShell->GetSandboxCleared(&sandboxCleared)) &&
> >+ sandboxCleared) {
> >+ docShell->SetAllowPlugins(true);
> >+ docShell->SetSandboxCleared(false);
>
> This looks wrong. What if user has explicitly disabled all the plugins?

I'm not sure how to handle the 'plugins are explicitly disabled' case. I thought about setting another flag if the sandbox disabled plugins but this doesn't fix the problem. I suppose I could check the pref (i assume there is one) but that seems a little heavyweight? I'll look to see if there's other ways I can tell the user has explicitly disabled plugins.

> Also, I don't understand how the sandboxCleared handles the case when
> sandbox attribute
> isn't removed but just changed.

In this case I think plugins should already be disabled from when the sandbox attribute was first added and sandboxCleared doesn't need to do anything, or am I missing something ?

[R-bugs-h (r-bugs-h)]

But what if the new value for the sandbox should enable plugins?

[Imelven-s (imelven-s)]

(In reply to Olli Pettay [:smaug] from comment #217)
> But what if the new value for the sandbox should enable plugins?

plugins are always disabled in a sandboxed document, per the spec (unless the plugin is 'securable' which means it understands and obeys any sandboxing restrictions on the document embedding it - afaik no plugin yet does this).

[Imelven-s (imelven-s)]

(In reply to Ian Melven :imelven from comment #216)
>
> I'm not sure how to handle the 'plugins are explicitly disabled' case. I
> thought about setting another flag if the sandbox disabled plugins but this
> doesn't fix the problem. I suppose I could check the pref (i assume there is
> one) but that seems a little heavyweight? I'll look to see if there's other
> ways I can tell the user has explicitly disabled plugins.

keeler pointed me to some code in nsPluginHost that checks the pref plugin.disable

http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginHost.cpp#318

318 mPluginsDisabled = Preferences::GetBool("plugin.disable", false);

I need to look into the plugin code more to see where the docshell flag is checked to see if there's an issue here, it may be that the pref overrides whether plugins are enabled on the docshell or not (it should, since they will be in the usual, non-sandboxed case).

[Bzbarsky (bzbarsky)]

Why do we need this sandboxcleared stuff at all?

I thought the model was @sandbox sets flags on docshell. Document inherits docshell flags at creation time. All checks use the document's flags. Are we actually using SetAllowPlugins on the docshell to disable plugins as far as sandbox is concerned? If so, that's wrong. What we should do instead is find the places that GetAllowPlugins and have those ask the _document_ instead. And the document can GetAllowPlugins(), and if true check its sandbox flags and so forth.

[Bzbarsky (bzbarsky)]

In particular, you probably just need to fix nsWebBrowserContentPolicy.cpp here, I bet.

[Imelven-s (imelven-s)]

(In reply to Boris Zbarsky (:bz) from comment #221)
> In particular, you probably just need to fix nsWebBrowserContentPolicy.cpp
> here, I bet.

ok, this approach makes a lot of sense and is more consistent with the rest of the functionality and the spec as well. It also addresses Dev's concern with disabling plugins via CSP sandbox and follows the approach he discussed with dveditz, of asking the docshell first and then asking the document (really meaning see if it has SANDBOXED_PLUGINS set)

Thanks Boris !!

[Chris Coulson (chrisccoulson)]

Sorry, but it is just completely pointless to duplicate an upstream bug here for no real reason, especially one that is already assigned and making good progress upstream, where there really isn't anything for us to do here or where opening it won't help progress the upstream bug. In addition to that, Launchpad spams everybody assigned to the upstream bug, which is really, really annoying.

What did you expect would happen by reporting this? Please don't do it again.