[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





--- Comment #18 from Darin Adler <darin at apple.com>  2009-11-05 17:40:59 PDT ---
(From update of attachment 42473)
> -                     /*setter raises (DOMException)*/;
> +                     getter raises (DOMException),
> +                     setter raises (/*DOMException*/);

I don't understand the syntax here. It's not right to have "setter raises ()"
is it? I suggest entirely commenting the setter out as before. Does sandboxing
not cause trying to set the cookie to raise an exception?

> +    , m_sandboxFlagsDecl(0)

Is this "decl" abbreviation needed? I think a better name would be
m_sandboxFlagsFromAttribute.

> +typedef uint8_t SandboxFlags;

We would normally use an enum for this, even though the values are flags. See
examples like PaintLayerFlags in RenderLayer.h for a design pattern to follow.

> +    SandboxFlags m_sandboxFlagsDecl;

We normally try to avoid having protected data members. How about exposing a
protected setter function instead. You probably do not need a getter. One
advantage of that is that updateSandboxFlags could be called automatically
instead of relying on the derived class to do so.

> +    } else if (attr->name() == sandboxAttr) {
> +        SandboxFlags newSandboxFlagsDecl;
> +        // Parse the unordered set of unique space-separated tokens.
> +        if (attr->isNull())
> +            newSandboxFlagsDecl = SandboxFlagsNone;
> +        else {
> +            newSandboxFlagsDecl = SandboxFlagsAll;
> +            const UChar* characters = attr->value().characters();
> +            unsigned length = attr->value().length();
> +            unsigned start = 0;
> +            while (true) {
> +                while (start < length && isIFrameSandboxStringWhitespace(characters[start]))
> +                    ++start;
> +                if (start >= length)
> +                    break;
> +                unsigned end = start + 1;
> +                while (end < length && !isIFrameSandboxStringWhitespace(characters[end]))
> +                    ++end;
> +
> +                // Turn off the corresponding sandbox flag if it's set as "allowed".
> +                AtomicString sandboxToken = AtomicString(characters + start, end - start);
> +                if (equalIgnoringCase(sandboxToken, "allow-same-origin"))
> +                    newSandboxFlagsDecl &= ~SandboxFlagsOrigin;
> +                else if (equalIgnoringCase(sandboxToken, "allow-forms"))
> +                    newSandboxFlagsDecl &= ~SandboxFlagsForms;
> +                else if (equalIgnoringCase(sandboxToken, "allow-scripts"))
> +                    newSandboxFlagsDecl &= ~SandboxFlagsScripts;
> +
> +                start = end + 1;
> +            }
> +        }
> +
> +        if (newSandboxFlagsDecl != m_sandboxFlagsDecl) {
> +            m_sandboxFlagsDecl = newSandboxFlagsDecl;
> +            updateSandboxFlags();
> +        }

This is too much code to have all inline here. You should factor all or some of
this out into a separate parsing function.

It doesn't make sense to construct an AtomicString just so you can call
equalIgnoringCase on it. You can just create a String, and if you do it by
calling substring you might even enefit from a substring sharing optimization
and avoid making a copy of the characters. It would be slightly better to write
a charactersEqualIgnoringCase function for PlatformString.h -- long run we
don't want to require creating a String object just to compare something with
it.

> +inline bool isIFrameSandboxStringWhitespace(UChar c)
> +{
> +    return c == ' ' || c == '\r' || c == '\n' || c == '\t' || c == '\f';
> +}

Do we really need a unique function for this? I guess this handles 0x000B
differently from isASCIISpace, but it's really frustrating to have yet
*another* whitespace function. Does sandboxing really need its own? Since this
is the same as isClassWhitespace, maybe there is a pattern here. Maybe we can
put this in ASCIICType.h if we can figure out a name to give it to distinguish
it from isASCIISpace. Maybe if we audit them we will find that the other
clients of isASCIISpace really don't want true for 0x000B. Wouldn't that be
nice if we could just have the one function! I'm surprised that web technology
isn't consistent on this.

Looks like pretty good test coverage.

Since the sandboxing string has a custom parser, you need a test that tests all
the edge cases; we don't want to learn about something like a buffer. All the
various whitespace characters and other characters. Empty string. Leading
whitespace, trailing, more than one space in the middle, etc. I didn't see that
in any of the tests.

It's great to have lots of tests. But can we do more than one test in a single
test file? It seems it would be better to have a smaller number of test files
that tested a larger combination of things.

Normally we ask people to follow WebKit coding style for the test code too and
here you have not done that.

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