[webkit-reviews] review denied: [Bug 21288] Implement HTML5's sandbox attribute for iframes : [Attachment 42959] Revised patch.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied  review:
Bug 21288: Implement HTML5's sandbox attribute for iframes
https://bugs.webkit.org/show_bug.cgi?id=21288

Attachment 42959: Revised patch.
https://bugs.webkit.org/attachment.cgi?id=42959&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    // Sandboxed iframes cannot open new auxiliary browsing contexts
> +    if (openerFrame && openerFrame->ownerElement()
> +	   && openerFrame->ownerElement()->isSandboxed(SandboxNavigation))
> +	   return 0;

I think it would be better to have an isSandboxed function you can call on the
script controller or some other object in the family of Frame objects. Having
the individual callers dig down to the owner element seems a little inelegant.
This would simplify some the call sites. I think that the fact that sandboxing
is controlled solely by the owner element is something we might change in the
future, and it would be best to have that policy localized.

Adam may have a suggestion about which object should have it. I believe in
general we would like a member of the Frame family of objects to know how
sandboxed the frame keeps its documents, and be consulted in cases where there
is not a document involved, and use the document's security origin in all cases
where there is a document involved. We want clients to not know specifically
that sandboxing comes from owner elements, and also ideally we want to make it
difficult to accidentally consult the wrong sandboxing flags (the current flags
on the frame instead of the ones on the document which may no longer be current
that frame, for example).

> +    HTMLFrameOwnerElement* ownerElement = m_frame->ownerElement();
> +    if (ownerElement && ownerElement->isSandboxed(SandboxScripts))
> +	   return false;

Same comment/question as above.

> +	   if (isHTMLDocument()) {
> +	       ExceptionCode ec = 0;   // Exception (for sandboxed documents)
ignored
> +	       static_cast<HTMLDocument*>(this)->setCookie(content, ec);
> +	   }

When ignoring an exception, you do not have to initialize the ExceptionCode,
and it's probably best to consistently not do it. If you were going to look at
the exception code, then you would be obligated to initialize it.

Normally we use a single space before comments, not three spaces. And we put
periods at the ends of comments.

> +    // Set the iframe sandbox origin flag from the corresponding property 
> +    // of its frame owner element (iframe)
> +    if (HTMLFrameOwnerElement* ownerElement = m_frame->ownerElement())
> +	   securityOrigin()->setSandboxFlags(ownerElement->sandboxFlags());

Again, might be good to get the sandbox flags from some Frame-family object
rather than actually going to the owner element explicitly.

> -    if (settings && settings->isJavaEnabled()) {
> +    if (settings && settings->isJavaEnabled()
> +	   && !document()->securityOrigin()->isSandboxed(SandboxPlugins)) {

>      if (!settings || !settings->isJavaEnabled())
>	   return 0;
>  
> +    if (inDocument() &&
document()->securityOrigin()->isSandboxed(SandboxPlugins))
> +	   return 0;

I'd like to see the above two checks factored to share code instead of
repeating the logic twice.

I know I should have commented on this in the earlier round, but I think the
actual sandbox flags should live in some object in the Frame family, not in the
owner element itself. The owner element could map the attribute to sandbox
flags, but the actual sandboxing state could be stored elsewhere, within the
Frame. This change isn't mandatory but it seems like something that could make
the code clearer and more elegant. And prepare us for having other ways to
control sandboxing other than attributes on the owner element.

> +void HTMLFrameOwnerElement::inheritSandboxFlags(SandboxFlags
flagsFromParent)
> +{
> +    m_sandboxFlags = m_sandboxFlagsFromAttribute | flagsFromParent;
> +
> +    if (inDocument())
> +	   document()->securityOrigin()->setSandboxFlags(m_sandboxFlags);

This seems wrong. It is changing the sandbox flags of a document based on an
element inside that document!

> +void HTMLIFrameElement::addSandboxStatus(MappedAttribute* attr)

In a new function I'd prefer you use a whole word, attribute, rather than the
abbreviation. Words are usually easier to read.

It's great that you factored this out. But I think a slightly better factoring
would be to factor our just the parsing. The call to
setSandboxFlagsFromAttribute could be in the caller. I think that's a slightly
clearer division of responsibilities.

> +		   ExceptionCode ec = 0;
> +		   // Ignore exception thrown by cookie() below -- the only
case an exception is
> +		   // thrown here is when the iframe is sandboxed, which will
never happen here
> +		   // because "doc" is the document of the main frame of the
page.
> +		   stringCookiesList += document->cookie(ec);
> +		   ASSERT(ec == 0);

The variable name is "document", not "doc", right?

The two sentences should each have periods. There's no need to splice them
together with a dash.

> +    HTMLFrameOwnerElement* ownerElement = m_frame->ownerElement();
> +    if (ownerElement && ownerElement->isSandboxed(SandboxForms))
> +	   return;

Same comment again about explicitly getting at the owner element.

> +	   HTMLFrameOwnerElement* ownerElement = m_frame->ownerElement();
> +	   if (ownerElement && ownerElement->isSandboxed(SandboxPlugins))
> +	       return false;

And here.

> +    // Sandboxed frames can only navigate their descendants.
> +    HTMLFrameOwnerElement* ownerElement = m_frame->ownerElement();
> +    if (ownerElement && ownerElement->isSandboxed(SandboxNavigation)
> +	   && !targetFrame->tree()->isDescendantOf(m_frame))
> +	   return false;

And here. I also think you should consider putting these things on one line. In
this project we tend to make longer lines and not break things so early, even
though I know on others they try to stick to 80 columns.

> +	   // Sandboxing status, copied from the document's ownerElement

I don't think "copied from the document's ownerElement" is a good comment. I
would instead say "determined by the frame" or something along those lines.
Also, please include a period.

> +    // Disable the usage of the database if the sandboxed origin browsing
context flags takes effect
> +    if (origin->isSandboxed(SandboxOrigin))
> +	   return false;

Need a period here.

> +		       if (cookiesEnabled(document)) {
> +			   ExceptionCode ec = 0;   // Exception (for sandboxed
documents) ignored
> +			   document->setCookie(m_handshake.serverSetCookie(),
ec);
> +		       }

When ignoring an exception, you do not have to initialize the ExceptionCode,
and it's probably best to consistently not do it. If you were going to look at
the exception code, then you would be obligated to initialize it.

Normally we use a single space before comments, not three spaces. And we put
periods at the ends of comments.

Changing to review- because of the apparent error in
HTMLFrameOwnerElement::inheritSandboxFlags. My other comments are not important
enough and I would not have changed the review state based on them, although I
think they could improve this work.

This is going to be a great feature to have. Thank you very much for your work
on it!