[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 #43508|review?                     |review-
               Flag|                            |




--- Comment #51 from Darin Adler <darin at apple.com>  2009-11-19 11:13:33 PST ---
(From update of attachment 43508)
Looking good. Nearly done. A few small mistakes, and one bigger mistake based
on bad advice from me.

I miss the canAccessDatabase function from your earlier patch; didn't see why
you rolled that back. Maybe because you didn't want a separate
canAccessStorage?

>  bool ScriptController::isEnabled()
>  {
>      Settings* settings = m_frame->settings();
> -    return m_frame->loader()->client()->allowJavaScript(settings && settings->isJavaScriptEnabled());
> +    return m_frame->loader()->client()->allowJavaScript(settings && settings->isJavaScriptEnabled() && !m_frame->loader()->isSandboxed(SandboxScripts));
>  }

Loose end for after this lands: ScriptController::isEnabled needs to be
renamed. It already checked more than just whether JavaScript was enabled.
Maybe canExecuteScripts is the right name.

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

Loose end for after this lands: Test case showing the problem? Bug report for
the problem?

>                   attribute [ConvertNullToNullString] DOMString cookie
> -                     /*setter raises (DOMException)*/;
> +                     setter raises (DOMException),
> +                     getter raises (DOMException);

Loose end for after this lands: Bug report for the problem that just plain
"raises" does not work. Should be easy to fix.

> +bool HTMLAppletElement::isJavaEnabled() const

Same problem with this name as with ScriptController::isEnabled, but this is a
new name, not an existing name. Should be named something like canEmbedJava
instead. Think of it as completing this sentence "the applet element
<functionName>" and you can see that "the applet element is Java enabled" does
not work at all.

I'd be willing to approve landing this patch if you have firm plans to rename
afterwards.

> +    SandboxFlags flags;
> +    // Parse the unordered set of unique space-separated tokens.
> +    if (attribute->isNull())
> +        flags = SandboxNone;

We love early return in the WebKit coding style. It might be nice to just do
this before even declaring a local variable, and avoid nesting the whole body
of the function inside an else.

> +void FrameLoader::ownerElementSandboxFlagsChanged()
> +{
> +    updateSandboxFlags();
> +}

This could be inlined by defining it in the class definition in the header.
Probably not important, though there seems to be no downside to doing this.

> +void FrameLoader::updateSandboxFlags()
> +{
> +    updateSandboxFlagsForSingleFrame();
> +
> +    for (Frame* affectedFrame = m_frame; affectedFrame; affectedFrame = affectedFrame->tree()->traverseNext(m_frame))
> +        affectedFrame->loader()->updateSandboxFlagsForSingleFrame();
> +}

This calls updateSandboxFlagsForSingleFrame twice on the current frame. The
first call to updateSandboxFlagsForSingleFrame outside the loop can be removed.

But also, I made you break this with my suggestion! The
updateSandboxFlagsForSingleFrame function only works if the parent updates its
flags before the child. And this non-recursive algorithm does not guarantee
that. So you probably need to go back to the recursive version. Sorry.

> +    m_frame->document()->securityOrigin()->setSandboxFlags(m_sandboxFlags);

Probably OK as is, but we might want to change this to just call a function
named updateSandboxFlags on the Document rather than digging directly into the
security origin. The document already has to know that it gets its sandbox
flags from the frame, and it seems a little strange for half the knowledge of
that to be in Document and the other half in FrameLoader.

> +    , m_sandboxFlags(SandboxNone)

I wonder if maybe the default should instead be entirely sandboxed. Is there
any code path where the default matters? Same applies to FrameLoader. We always
set the flags later in initialization, so maybe it's OK to fully sandbox during
initialization. Not an important issue, but this does seem backwards from usual
security designs to assume everything is allowed. It would be good to notice if
we attempted anything so early that we did not yet have sandbox flags set, so
you could even imagine a special value "not yet set" and you would assert if
ever asked to make a sandboxing decision based on that value.

Is there any problem with the fact that documents that never go into a frame,
such as ones made with XMLHttpRequest, have no sandbox flags set? Maybe we
should set their sandbox flags to prohibit everything or set them to the "don't
you dare use this" value?

I did not carefully review the tests.

You don't need to do everything I mentioned on comments -- I don't want to make
this take forever. But please do fix the things that are definitely broken like
the bad updateSandboxFlags algorithm I suggested.

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