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




--- Comment #31 from Darin Adler <darin at apple.com>  2009-11-16 09:58:47 PST ---
(From update of attachment 43307)
Exciting work. Looking forward to getting this feature in.

Below I'll say "the frame loader" to mean whatever object in the frame
collection we use for the sandboxing logic. I think probably the frame loader.
The Frame object itself should not have new functions.

> +bool HTMLAppletElement::isJavaEnabled() const
> +{
> +    if (!inDocument())
> +        return false;

This is a policy change. Before there was no check of inDocument. I presume
this change is done because it fixes a bug. Is there a test that shows why this
improves things? Can this bug fix be done in a separate patch?

> +void HTMLFrameOwnerElement::setSandboxFlagsFromAttribute(SandboxFlags flagsFromAttribute)
> +{
> +    m_sandboxFlagsFromAttribute = flagsFromAttribute;
> +    updateSandboxFlags();
> +}

I think the argument can just be named "flags". I also think that maybe
m_sandboxFlags would be fine for the data member. After all, these are just the
element's flags now, not the frame's so the "from attribute" distinction may
not be necessary. These are the sandbox flags provided by the element.

> +void HTMLFrameOwnerElement::updateSandboxFlags()
> +{
> +    SandboxFlags flagsFromParent = m_sandboxFlagsFromAttribute;
> +    
> +    // Inherit sandbox flags from the parent frame, if there is one.
> +    Frame* parentFrame;
> +    if (inDocument() && (parentFrame = document()->frame()))
> +        flagsFromParent |= parentFrame->sandboxFlags();
> +
> +    if (contentFrame())
> +        contentFrame()->inheritSandboxFlags(flagsFromParent);
> +}

The logic here should be moved to FrameLoader. There's no reason to take a trip
through the owner element to do this operation. A frame can find its own parent
and there is no need for the owner element to pass the flags that it gets from
the parent document. It is appropriate for the owner element to pass in the
flags it is specifying when telling the frame the have changed, although even
that is not entirely necessary. The function in the frame loader could be
called setOwnerElementSandboxFlags or ownerElementSandboxFlagsChanged or even
updateSandboxFlags if you decide to have it get the flags directly rather than
taking an argument.

The rules about inheritance of sandbox flags do *not* belong in the owner
element code, but rather in the frame loader. A frame's inheritance of its
parent's sandbox flags should be done without being triggered by the owner
element, simply because it's installed in the frame tree with a particular
parent, and should not even depend on the presence of an owner element.

> +void HTMLFrameOwnerElement::insertedIntoDocument()
> +{
> +    HTMLElement::insertedIntoDocument();
> +
> +    updateSandboxFlags();
> +}

If insertedIntoDocument is one place we need to update these flags, what about
when the element is removed from a document? Is there a good reason that can't
affect sandbox flags or doesn't matter? Could we include a test case to test
this? Does the frame already exist when insertedIntoDocument is called? If not,
then do we really need the updateSandboxFlags call? Which test fails if you
remove this function override?

> +#include "Frame.h"
>  #include "HTMLElement.h"

This is something we do not want to do; including "Frame.h" in another header
is something to be avoided if possible, because Frame.h in turn includes a
*lot* of other files. The SandboxFlags enum should be put into a smaller header
file. FrameLoaderTypes.h is probably the right choice. We don't want all files
that include HTMLFrameOwnerElement.h to include Frame.h too and all the files
it includes. In general we try to not "include the world"; this is the reason
for files like FrameLoaderTypes.h, RenderStyleContents.h, ScrollBehavior.h,
PointerEventsHitRules.h, and others.

It's especially strange that this file includes "Frame.h" when the sandbox
flags enum is in SecurityOrigin.h. But even SecurityOrigin.h probably should
not be included here.

> +            AtomicString sandboxToken = AtomicString(characters + start, end - start);
> +            if (equalIgnoringCase(sandboxToken, allowSameOrigin))
> +                newSandboxFlags &= ~SandboxOrigin;
> +            else if (equalIgnoringCase(sandboxToken, allowForms))
> +                newSandboxFlags &= ~SandboxForms;
> +            else if (equalIgnoringCase(sandboxToken, allowScripts))
> +                newSandboxFlags &= ~SandboxScripts;

It's a mistake to construct an AtomicString here. There is extra cost, and no
benefit, to making an AtomicString instead of just a String. Similarly, the
constants for the allow strings can just be const char*. There is little to no
benefit to constructing an AtomicString for each of them. Comparing with a
const char* is roughly the same performance as comparing with a string object
if ignoring case.

Longer term, we should create a new function, charactersEqualIgnoringCase(const
UChar*, size_t, const char*), and use it in this case; short term you should
just make a String and use equalIgnoringCase.

I would call the SandboxFlags variable just "flags", not "newSandboxFlags",
given the purpose of the function. The extra words aren't really needed inside
a function with the focused job of parsing.

> +                ASSERT(ec == 0);

WebKit coding style writes this ASSERT(!ec) rather than with "== 0".

> +        void inheritSandboxFlags(SandboxFlags); // Propagate sandbox flags to descendants in the frame tree.

This seems like the wrong interface. The caller should not be passing sandbox
flags in. See my other comments above.

These functions should be on FrameLoader rather than Frame. It's too bad
FrameLoader is so big, but putting new members directly into Frame is even
worse than adding new FrameLoader members.

> +    // Disable database access if the sandboxed origin flag is set.
> +    if (origin->isSandboxed(SandboxOrigin))
> +        return false;

Checks directly on the sandboxing setting seem a little low level to me. Is
there some higher level question we can ask the security origin? Maybe
canAccess or canRequest?

I think it is strange that isSameSchemeHostPort is checking the sandboxing
flags. That function now doesn't make as much sense as before, because it can
return false even if you ask if a security origin is the same as itself. Are
you sure that's the bst design for this? Can we look at call sites and consider
a different approach?

I'm not sure all the isSandboxed calls are talking to the right frame. In
particular, I am surprised that createWindow is checking openerFrame rather
than either lexicalFrame or dynamicFrame and that FrameLoader::submitForm is
checking the frame rather than the document. Those might be right, but I'm not
sure they are.

Where is the code where a new document gets its security origin flags set based
on the frame it is created in?

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