Bug 32368 – Add defensive initialization of iframe sandbox flags

• Status: RESOLVED
• Resolution: FIXED
• Component: Frames

[Patrik Persson]

This is a followup to bug 21288, which concerned the implementation of
the HTML5 iframe sandbox attribute.  I'm curious whether it would be
possible to use more defensive initial values for the sandbox flags
than the current default of "SandboxNone" (indicating no sandboxing at
all).

This defensive technique is, to me, primarily about finding current
bugs and preventing future ones.  It is a design challenge rather than
a functional bug, so I don't have a test case to share.

Three classes contain such sandbox flags: HTMLFrameOwnerElement,
FrameLoader and SecurityOrigin.  I imagine three steps:

  1. On object instantiation, the sandbox flags are set to a defensive
     default value (SandboxAll, possibly including a special flag
     indicating an illegal value).

  2. At some point between 1 and 3, we know of a better value for the
     sandbox flags, and assign that value to the flags.

  3. We now start making decisions based on the sandbox flags.  To
     ensure that step 2 actually happened, we could ASSERT that the
     flag for an illegal value is not set.

The design challenge is to ensure step 2 always happens before step 3.
It was straight-forward for the FrameLoader (that part was included in
the landed patch for bug 21288), but not for HTMLFrameOwnerElement and
SecurityOrigin.  I wrote down some observations on this in the thread
for bug 21288, comment #58:

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

[Darin Adler]

I think for HTMLFrameOwnerElement this issue probably doesn't apply. Using the DOM you can create a frame element a step at a time, and you can start using it at any point, so there is no real "illegal value" time for the DOM elements.

For SecurityOrigin I think we might be able to make it happen, though.

[Adam Barth]

Created attachment 46230 [details]
Work in progress

[Adam Barth]

Created attachment 46242 [details]
Patch

[Darin Adler]

Comment on attachment 46242 [details]
Patch

>      if (shouldTreatURLSchemeAsNoAccess(m_protocol))
>          m_isUnique = true;
>  
> -    // If this ASSERT becomes false in the future, please consider the impact
> -    // of m_sandoxFlags on m_isUnique.
> -    ASSERT(m_sandboxFlags == SandboxNone);
> +    if (isSandboxed(SandboxOrigin))
> +        m_isUnique = true;

I suggest initializing m_unique to the correct value from the outset.

    m_isUnique(shouldTreatURLSchemeAsNoAccess(m_protocol) || isSandboxed(SandboxOrigin))

To some the if statements may be slightly clearer, so it seems OK to leave it this way if you think that way.

> +    static PassRefPtr<SecurityOrigin> create(const KURL&, SandboxFlags sandboxFlags = SandboxNone);

The argument name isn't needed here because the type speaks for itself.

[Adam Barth]

> > +    static PassRefPtr<SecurityOrigin> create(const KURL&, SandboxFlags sandboxFlags = SandboxNone);
> 
> The argument name isn't needed here because the type speaks for itself.

Oh, I didn't know you could omit the argument name and provide a default value.

Thanks for the review.

[Adam Barth]

Comments addressed and landed in http://trac.webkit.org/changeset/53056