(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 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?)

good idea - i'll look into this.
 
> @@ +471,5 @@
> > +    mSandboxFlags = aSandboxFlags;
> > +  }
> > +  
> > +  /** 
> > +   * Get the sandox flags for this document
> 
> sandbox

will fix.

> @@ +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.

thanks, will fix.

> ::: 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.

> @@ +702,5 @@
> > +PRUint32
> > +nsContentUtils::ParseSandboxAttributeToFlags(nsIDOMDOMSettableTokenList * aAttribute)
> > +{
> > +  // TODO: how to handle errors in this function ? if we're here, something has a sandbox
> > +  // attribute set and expects content to be sandboxed... perhaps consider failing closed ??
> 
> You can do this one of three ways:
> 
> 1) Return an explicit nsresult and pass the flags back in a PRUint32
> outparam.
> 2) Return a clearly bogus value for failure (e.g. -1)
> 3) Fail closed (-1 effectively does this if somebody forgets the check).
> 
> What in here is fallible though?  Simple tokenization and comparison should
> be infallible, I think.

yeah - i will look at that code to verify if it can fail when i make the change above.

> @@ +711,5 @@
> > +  out |= nsIDocShell::SANDBOX_FLAGS_SANDBOXED;
> > +  
> > +  bool allowed = false;
> > +  
> > +  // TODO: should i try to use atoms even though Contains wants a string ? 
> 
> Yes, you should use the atoms and do 'if (atom->Equals(token)) ...'

will do.

> ::: content/base/src/nsDocument.cpp
> @@ +2394,5 @@
> > +  nsCOMPtr<nsIDocShell> docShell = do_QueryInterface(aContainer);
> > +  
> > +  if (docShell) {
> > +     nsresult rv = docShell->GetSandboxFlags(&mSandboxFlags);
> > +	 NS_ENSURE_SUCCESS(rv, rv);
> 
> ugh tabs

sorry - i was mainly wanting to know if the approach was sane, apologies for the formatting/tabs/whitespace stuff, i knew that needed cleaning up. 

> ::: content/base/src/nsFrameLoader.cpp
> @@ +506,5 @@
> > +      // and it may not be - maybe fail closed (all restrictions) ??? 
> > +    }
> > +     
> > +    if (parentSandboxFlags) { 
> > +      // do the intersection with the current flags in the docshell!
> 
> I didn't look at this too closely, I assume that permissions can only be
> removed, not added, for children?
> 
> e.g. if I have:
> 
> - Top Level Doc (A)
>   - Sandboxed Iframe (B)
>      - Sandboxed Iframe (C)
> 
> And B has permissions 1 and 2 there's no way for C to have permission 3,
> right?

yes - permissions can only be tightened/removed, never added/loosened. ms2ger commented on this below, i know there's a bug in the existing patch with inheritance and i think ms2ger's suggestion is the right one after thinking it over for a bit. 

> ::: content/html/content/src/nsHTMLFormElement.cpp
> @@ +691,5 @@
> > +      PRUint32 sandboxFlags = doc->GetSandboxFlags();
> > +      
> > +      if (!(sandboxFlags & nsIDocShell::SANDBOX_FLAGS_SANDBOXED) || 
> > +          (sandboxFlags & nsIDocShell::SANDBOX_FLAGS_ALLOW_FORM_SUBMIT))
> > +        rv = DoSubmit(aEvent);
> 
> Does this check avoid doing form validation and the like when sandboxed? (I
> think it does).  You'll want tests for that.

i think it does too - but yes, this should be a test in the suite. Thanks. 

> ::: 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 !

> ::: docshell/base/nsDocShell.cpp
> @@ +755,5 @@
> >      mIsBeingDestroyed(false),
> >      mIsExecutingOnLoadHandler(false),
> >      mIsPrintingOrPP(false),
> > +    mSavingOldViewer(false),
> > +    mSandboxFlags(0)
> 
> Please order intializers in the same order variables are declared (doing
> otherwise will give warnings on gcc).

will do.

> @@ +2887,5 @@
> > +            // If our document is sandboxed, we need to do some extra checks
> > +            PRUint32 sandboxFlags = 0;
> > +           
> > +            if (mContentViewer) {
> > +                nsCOMPtr<nsIDocument> doc = mContentViewer->GetDocument();
> 
> No need to use an nsCOMPtr here.  nsIDocument* doc is fine.

will change.
 
> @@ +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. 

> @@ +4996,5 @@
> > +
> > +NS_IMETHODIMP
> > +nsDocShell::GetSandboxFlags(PRUint32  *aSandboxFlags)
> > +{
> > +  NS_ENSURE_ARG_POINTER(aSandboxFlags);
> 
> Usually we don't bother with this.  If you pass in null, you crash.

heh, ok, i'll drop it for consistency. 

> @@ +8201,5 @@
> > +            PRUint32 sandboxFlags = 0;
> > +            sandboxFlags = doc->GetSandboxFlags();
> > +            if (sandboxFlags & nsIDocShell::SANDBOX_FLAGS_SANDBOXED) {
> > +              return NS_ERROR_FAILURE;
> > +            }
> 
> Same comment as above about somebody who knows docshell looking at this.

thanks - i'll ping one of the folks you mentioned on this part.

> @@ +8996,5 @@
> > +      nsContentUtils::SetUpChannelOwner(ownerPrincipal, channel, aURI, true, true);
> > +    }
> > +    else {
> > +      nsContentUtils::SetUpChannelOwner(ownerPrincipal, channel, aURI, true, false);
> > +    }
> 
> I think this would be easier to follow with one callsite whose final
> parameter varies.

yeah, ms2ger has a good suggestion below here i'm going to take.

> ::: docshell/base/nsIDocShell.idl
> @@ +561,5 @@
> > +   * They are only used when loading new content, sandbox flags are also immutably set on the document
> > +   * when it is loaded. The sandbox flags of a document depend on the sandbox flags on its docshell
> > +   * and of its parent document, if any. 
> > +   */   
> > +  attribute unsigned long sandboxFlags;
> 
> nsIDocShell needs an IID bump for this.

thanks, will do.

> @@ +576,5 @@
> > +   
> > +  /**
> > +   * The content is sandboxed but allowed to have its natural domain. 
> > +   */
> > +  const unsigned long SANDBOX_FLAGS_ALLOW_SAME_DOMAIN = 0x4;
> 
> please make this ALLOW_SAME_ORIGIN so that the terminology is consistent.

will do.

> ::: dom/interfaces/html/nsIDOMHTMLIFrameElement.idl
> @@ +68,5 @@
> >    readonly attribute nsIDOMDocument   contentDocument;
> >    readonly attribute nsIDOMWindow     contentWindow;
> > +  
> > +  // HTML5 sandbox attribute
> > +  readonly attribute nsIDOMDOMSettableTokenList sandbox;
> 
> Needs an IID bump.

thanks, will do.