[Webkit-unassigned] [Bug 21288] Implement HTML5's sandbox attribute for iframes

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=21288


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #43367|review?                     |review-
               Flag|                            |




--- Comment #38 from Darin Adler <darin at apple.com>  2009-11-17 23:04:52 PST ---
(From update of attachment 43367)
Looks great. Very few issues remain.

> +    if (m_frame->loader()->isSandboxed(SandboxScripts))
> +        return false;
> +        
>      Settings* settings = m_frame->settings();
>      return m_frame->loader()->client()->allowJavaScript(settings && settings->isJavaScriptEnabled());

I think maybe the sandboxing should contribute to the allowJavaScript argument
rather than unconditionally returning false here. Ultimate control should go to
the frame loader client. Not sure if this will ever matter in practice.

> +    // FIXME: the HTML5 DOM spec states that this attribute can raise an
> +    // INVALID_STATE_ERR exception on getting if the Document has no
> +    // browsing context.

The word "the" should have a capital "T".

> +void HTMLFrameOwnerElement::setSandboxFlags(SandboxFlags flags)
> +{
> +    m_sandboxFlags = flags;
> +    
> +    if (Frame* frame = contentFrame())
> +        frame->loader()->ownerElementSandboxFlagsChanged();
> +}

Often in a function like this we would return early if m_sandboxFlags was
already == flags. Maybe that's just excess not-really-helpful code?

> +void FrameLoader::ownerElementSandboxFlagsChanged()
> +{
> +    m_sandboxFlags = SandboxNone;
> +    
> +    if (Frame* parentFrame = m_frame->tree()->parent())
> +        m_sandboxFlags |= parentFrame->loader()->sandboxFlags();
> +
> +    if (HTMLFrameOwnerElement* ownerElement = m_frame->ownerElement())
> +        m_sandboxFlags |= ownerElement->sandboxFlags();
> +
> +    if (Document* doc = m_frame->document()) 
> +        doc->securityOrigin()->setSandboxFlags(m_sandboxFlags);

I'd prefer "document" over "doc" here for the local variable name.

> +    for (Frame* child = m_frame->tree()->firstChild(); child; child = child->tree()->nextSibling())
> +        child->loader()->ownerElementSandboxFlagsChanged();
> +}

I suggest renaming this function to updateSandboxFlags and calling it from both
FrameLoader::ownerElementSandboxFlagsChanged and FrameLoader::init. Then you
can get rid of the call to this function in Frame.cpp.

It would be slightly nicer if this used a non-recursive algorithm. You could
put most of the logic into a private function named
updateSandboxFlagsForSingleFrame and then walk all descendant frames without
using recursion by using tree()->traverseNext(). On the other hand, I suppose
this is not a real issue, since we can't have an arbitrarily deeply nested
frame tree. Still, I'd like it slightly better that way.

    for (Frame* affectedFrame = m_frame; affectedFrame; affectedFrame =
affectedFrame->tree()->traverseNext(m_frame))
        affectedFrame->loader()->updateSandboxFlagsForSingleFrame();

I also think we might want to compute the new sandbox flags and do an early
return in the case where m_sandboxFlags hasn't changed. Again, not sure that
really is an improvement.

> @@ -369,6 +371,9 @@ bool SecurityOrigin::equal(const Securit
>      if (m_domainWasSetInDOM && m_domain != other->m_domain)
>          return false;
>  
> +    if (isSandboxed(SandboxOrigin) || other->isSandboxed(SandboxOrigin))
> +        return false;
> +
>      return true;

I still don't think this is the right. The function name should not be equal if
it has a special rule that says it will return false if either frame is
sandboxed. The function name should change if its behavior is changing like
this. How many different callers of SecurityOrigin::equal are there? Can we
easily change the name of the function? What is the concept of the function?
Can we change it to be a canAccessAsPartOfSameOrigin function or something like
that but less wordy? It's not really an "equal" check if it's actually an check
to see if some action is allowed.

Even though this is almost perfect, I think I'll say review- because of the
SecurityOrigin::equal design issue.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.