Unify origin sandbox flag with m_noAccess in SecurityOrigin

• Component: Frames
• Status: RESOLVED FIXED

[Patrik Persson]

This is a followup to bug 21288, which concerned the implementation of
the HTML5 iframe sandbox attribute.

One of the locations where sandbox flags are managed is
SecurityOrigin.  Prior to the sandbox implementation, there was
already an attribute 'm_noAccess' whose meaning was very similar to
that of the origin sandbox flag 'SandboxOrigin' in m_sandboxFlags.
Currently, both flags are maintained and checked separately within the
SecurityOrigin.

We propose to unify m_noAccess with SandboxOrigin.  There was some
discussion on this in comments 8, 10 and 14 for bug 21288:

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

[Adam Barth]

Created attachment 46200 [details]
Patch

[Adam Barth]

Created attachment 46201 [details]
Patch

[Darin Adler]

Comment on attachment 46201 [details]
Patch

> +    // This method exists because we treat data URLs as havine a unique origin,

Typo: "havine".

> +    // contrary to the current (9/19/2009) draft of the HTML5 specification.
> +    // We still want to let folks paint data URLs onto untainted canvases, so
> +    // we special case data URLs below.  If we change to match HTML5 w.r.t.
> +    // data URL security, then we can remove this method in favor of
> +    // !canRequest.

This is a member function, and C++ does not have methods, so I suggest we call it a "function" in this comment.

>      if (url.protocolIs("data"))
>          return false;
>  
> +void SecurityOrigin::setSandboxFlags(SandboxFlags flags)
> +{
> +    m_sandboxFlags = flags;
> +    if (isSandboxed(SandboxOrigin))
> +        m_isUnique = true;
> +}

This can correctly handle tightening of sandbox flags, but not loosening of them. At the very least we should be asserting that the origin restriction is never loosened here, because we then would have to set m_isUnique to shouldTreatURLSchemeAsNoAccess(m_protocol).

> -    // Sandboxing status as determined by the frame.

Why did you remove this comment. I assume you had a good reason, but I don't know what it is.

> -    void setSandboxFlags(SandboxFlags flags) { m_sandboxFlags = flags; }
> +    void setSandboxFlags(SandboxFlags flags);

I suggest removing the argument name here.

> +    // The origin is a globally unique identifier assigned when the Document is
> +    // created.  http://www.whatwg.org/specs/web-apps/current-work/#sandboxOrigin
> +    //
> +    // There's a subtle difference between a unique origin and an origin that
> +    // has the SandboxOrigin flag set.  The later implies the former, and, in
> +    // addition, the SandboxOrigin flag is inherited by iframes.
> +    bool isUnique() const { return m_isUnique; }
> +

Periods, not two.

I think you mean "the latter" rather than "the later".

> +    // When using the string value, it's important to remember that it might be
> +    // "null".  This happens when this SecurityOrigin is unique For example,

Missing period after "unique".

> -    bool m_noAccess;
> -    bool m_universalAccess;
> -    bool m_domainWasSetInDOM;
> -    bool m_canLoadLocalResources;
> +    bool m_isUnique : 1;
> +    bool m_universalAccess : 1;
> +    bool m_domainWasSetInDOM : 1;
> +    bool m_canLoadLocalResources : 1;

Why the bit fields here? Are there enough SecurityOrigin objects that this has a significant memory cost we want to reduce? Does this make code smaller or faster? I think we don't have good a reason to use them here.

review- because I'm sure you'll want to fix some of these

[Darin Adler]

(In reply to comment #3)
> Periods, not two.

I meant to say, "We use one space after periods, not two."

[Adam Barth]

Created attachment 46228 [details]
Patch

[Adam Barth]

(In reply to comment #3)
> Typo: "havine".

Fixed.

> This is a member function, and C++ does not have methods, so I suggest we call
> it a "function" in this comment.

Fixed all occurrences in this file.

> This can correctly handle tightening of sandbox flags, but not loosening of
> them.

I added a comment to the code explaining why the code is doing this.  There's a bug here, but it's the opposite of what you think.  I'd like to fix that in the "defensive initialization" bug, if that works for you.

> Why did you remove this comment. I assume you had a good reason, but I don't
> know what it is.

The comment wasn't really adding any value.  It's not really intrinsic in the notion of these sandbox flags what the current API is for controlling them.

> I suggest removing the argument name here.

Fixed.

> Periods, not two.

Fixed all occurrences in this file.

> I think you mean "the latter" rather than "the later".

Fixed.

> Missing period after "unique".

Fixed.

> Why the bit fields here?

I've removed them.  I don't think they matter.

[Darin Adler]

Comment on attachment 46228 [details]
Patch

> +    // FIXME: Our current behavior here is buggy because we need to
> +    //        distinguish between the sandbox flags at creation and the
> +    //        sandbox flags that might come about later.

Can we construct a test case to demonstrate this?

r=me

[Adam Barth]

> Can we construct a test case to demonstrate this?

Yes, we can.  I was going to write one, but I'm off to dinner.  I'll write one soon.

[Adam Barth]

Committed r53049: <http://trac.webkit.org/changeset/53049>