[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


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|abarth at webkit.org           |webkit-unassigned at lists.web
                   |                            |kit.org




--- Comment #14 from Adam Barth <abarth at webkit.org>  2009-11-05 08:38:54 PDT ---
(In reply to comment #10)
> * The spec associates the sandbox status with the iframe and its
>   nested browsing context (we chose to use the ownerElement for this),
>   independent of the current document in the frame. Is there a
>   particular advantage with associating the sandbox status with the
>   document (beyond as a useful shorthand when checking)?

The spec doesn't quite understand detached documents in perfect detail.  We
could ask Ian to spec this completely, but it might not be worth while.  In the
case of this patch, we want to do this to avoid crashing on the line I
indicated.

>   This would essentially be a cached copy of the ownerElement's
>   sandbox status. When the document is detached from its frame, the
>   cached sandbox status becomes stale. Or am I getting it wrong?

That's right.  It's fine for things to get stale when the document is detached.
 We just want to avoid:

1) Crashing
2) Accidentally given privileges back to the scripts associated with that
document.

> * What do you mean by unifying SecurityOrigin::m_noAccess with the
>   sandbox bits?

The m_noAccess flag is implementing the same concept from the spec as one fo
the sandbox flags (the concept of a unique origin).  Any differences between
m_noAccess and the lack of an allow-same-origin directive are bugs.  I suspect
m_noAccess has a number of such bugs because it pre-dates the concept getting
added to the spec.  :)

>   If you mean treating m_noAccess as a sixth flag along
>   with the sandbox flags, then I see what you mean (assuming the
>   modification discussed in the previous bullet is made). But if you
>   mean unifying m_noAccess and the origin sandbox flag, then I'd
>   prefer to duck the unification (for now).

That's fine.  I can do that in a follow up patch if you'd rather not do it
yourself.

>   The reason is that SecurityOrigin::m_noAccess and the origin sandbox
>   flag work slightly differently. Take the SecurityOrigin::equal()
>   method for example. When comparing two identical "data:" URLs (i.e.,
>   m_noAccess is set) it returns true, unless any of them is
>   sandboxed. Mapping this flag to the origin sandbox flag would make
>   equal() behave differently for "data:" URLs. If this change is
>   desirable, then we propose doing it in a separate patch.

We'd have to look at where equals is used, but I'm pretty sure we want the
behavior you have for the sandbox flag for noAccess too.

> * The funny syntax for the IDL 'cookie' setter is an attempt to work
>   around (what seems to me like) an IDL quirk. I'd actually like to
>   omit the line about the setter entirely (the setter throws no
>   exceptions), but then the exception status of the getter seemed to
>   somehow "leak" to the setter too in the generated code (as if though
>   the setter was declared to throw DOMException).

Can we fix the code generator to avoid this workaround?

>   Should we add a comment to Document.idl on this (possibly along with
>   a separate bug report), or are we just doing it wrong?

In general, we're not shy about fixing bugs in the code generator.  I'm fine
with fixing the code generator in another patch (either before or after this
patch), but I'd like to hear Alexey's view on this topic.

> We hope we don't come off as too defensive to your comments -- they
> are very valid, we mostly agree, and we're happy to revise the patch.

In general, there is a lot of back and forth in WebKit reviews.  The goal of
these reviews is to keep the quality of the code as high as possible.  If this
is your first patch, you've done a remarkable job.  Most folks start off with
small patches to get acquainted with the process.

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