[webkit-reviews] review denied: [Bug 21288] Implement HTML5's sandbox attribute for iframes : [Attachment 42473] HTML5 IFrame sandboxing

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Yuan Song <song.yuan at ericsson.com>'s
request for review:
Bug 21288: Implement HTML5's sandbox attribute for iframes
https://bugs.webkit.org/show_bug.cgi?id=21288

Attachment 42473: HTML5 IFrame sandboxing
https://bugs.webkit.org/attachment.cgi?id=42473&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
This patch is very good.  There is one design issue that I'd like you to
change.  Instead of storing the sandbox information only on the ownerElement,
we should copy the information to the document's securityOrigin.  (We might as
well copy the whole bit array.)  This is to make sure w can keep track of the
document's privileges after it get detached from the frame.

Also, we should unify SecurityOrigin::m_noAccess with the sandbox bits.  If you
see how to do that, we can do that in the first patch.	Otherwise, we can do it
in a second patch.

Detailed comments below:

+ String Document::cookie(ExceptionCode &ec) const

In this function, you want to get the sandbox information out of the
securityOrigin instead of the Frame to handle the case when the document is
detached from the frame (i.e., with m_frame is null).

+ Document::initSecurityContext

Basically, you want to copy the sandbox state into securityOrigin here.  That
way you can have wit with all the Document-lifetime objects that need it.

+ HTMLFrameOwnerElement* ownerElement = document()->frame()->ownerElement();

Careful!  frame() can be null!	Once you move the sandbox state into
SecurityOrigin, then you can just get it from the document() and not have to
worry about frame() being null.

+  // Parse the unordered set of unique space-separated tokens.

Please move the parser code into a separate function.  Also, you should add
some tests that exercise this parser, e.g., by using mixed case and crazy
parser cases.

+ isIFrameSandboxStringWhitespace

Why do we need a special function for this?  Why not just use IsASCIIWhitespace
or some-such?  If the spec really requires this goofy set of characters, we can
change the spec to be sane.

+ bool originSandboxed() const { return m_originSandboxed; }

We should combine these concepts.

+ src="data:text/html,

Please don't use data URLs in your test unless you're specifically testing the
interaction of sandbox and data URLs.  We already sandbox data URLs to a
certain extent, so it's a bit unclear what you're testing.

+ navigated_frames ++;

No space before the ++ operator.

+
LayoutTests/fast/frames/resources/sandboxed-iframe-document-cookie-read-denied.
html

Please test cookie operators in HTTP tests.  The rules for cookies interacting
with file URLs are tricky and not what we want to test here.

+  11		text run at (92,18) width 268: "full render tree to include
Iframe contents.)"

You can use layoutTestController.dumpChildFramesAsText (or something like that)
to achieve the same effect while keeping a text test (which are infinitely
easier to deal with).