Comment 31 for bug 1022741

(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

will fix, sorry.

> >--- 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)
> &&
> (mSandboxFlags &
> SANDBOX_FLAGS_ALLOW_SAME_DOMAIN) != SANDBOX_FLAGS_ALLOW_SAME_DOMAIN);

thanks, much cleaner, will fix.

> >--- a/dom/interfaces/html/nsIDOMHTMLIFrameElement.idl
> >+++ b/dom/interfaces/html/nsIDOMHTMLIFrameElement.idl
> >-
> >+
> >+ // HTML5 sandbox attribute
>
> This interface is specified in HTML, so no need for the comment. Also, leave
> the two empty lines between the HTML-specified members and the Mozilla
> extensions.

Thanks, will fix.

> Also check the entire patch for tabs and trailing whitespace, and make sure
> the comments you add are full sentences (including capital letters and full
> stops).

Will do - sorry, this patch wasn't totally cleaned up, thanks for the sanity check on the code, i will clean it all up ! :)