(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 (aName == nsGkAtoms::sandbox && aNameSpaceID == kNameSpaceID_None) {
> > > +    // parse the new value of the sandbox attribute, and if we have a docshell
> > > +    // set its sandbox flags appropriately
> > 
> > When is a sandbox attribute set supposed to take effect?
> > 
> > "While the sandbox attribute is specified, the iframe element's nested
> > browsing context must have the flags given in the following list set. In
> > addition, any browsing contexts nested within an iframe, either directly or
> > indirectly, must have all the flags set on them as were set on the iframe's
> > Document's browsing context when the iframe's Document was created."
> > 
> > So it sounds like a sandbox attr change should only take effect for the next
> > document in the iframe.  Setting them on the docshell immediately means
> > they'll take affect for future subloads even on the same document, right? 
> > You'll want either tests to show that this isn't the case or to change the
> > code so that it isn't (and then tests :-)).
> 
> yeah, that's my reading of the spec - an attr change only takes effect when
> the next document is loaded in the iframe. Future subloads are an important
> case to point out here, thanks - i believe that they should be based on the
> flags on the document, NOT the docshell. And yes, this needs tests around it
> for sure !

Yes, lots of tests. :-)
 
> > @@ +2944,5 @@
> > > +                        parentAsItem = tmp;
> > > +                    }
> > > +                }
> > > +            } 
> > > +            
> > 
> > Somebody who understand docshell (read bz, smaug) should look at this stuff.
> > In my five minutes of looking at docshell it looks like this logic might fit
> > better in CanAccessItem.
> 
> i discussed this over email with bz and asked about putting this in
> CanAccessItem and in the end, it was decided that it shouldn't. 

Ok.