845057 - Fix the type of HTMLIFrameElement.sandbox

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[(no longer active)]

In bug 844169, we implemented the sandbox property with type string.  We should fix it based on the latest version of the spec.

Comment 1

[Deian Stefan]

Attachment: 0001-Bug-845057-Use-tokens-for-iframe-sandbox-property.patch

In addition to moving from string attribute to DOMSettableToken, this
patch cleans up the sandbox attribute code a bit. Some of the cleanup
(e.g., calling GetSandboxFlags vs ParseSandboxAttributeToFlags in
AfterSetAttr) results in slightly slower code, but I suspect this
doesn't really matter.

To avoid duplicating attribute to flags code, I am stringifying the
token list and calling the existing ParseSandboxAttributeToFlags code.

We can use this alternative (likely faster function) implementation:

> uint32_t 
> ParseSandboxAttributeToFlags(const nsDOMTokenList& aList)
> {
> #define IF_SBOXATTR(str, flag) \
>   if (aList.Contains(NS_ConvertASCIItoUTF16((str)), err) && \
>       !err.Failed()) { out &= (flag); } \
>   NS_ASSERTION(!err.Failed(), "Sandbox attribute check failed");
> 
>   // If there's a sandbox attribute at all (and there is if this is
>   // being called), start off by setting all the restriction flags.
>   uint32_t out = SANDBOXED_NAVIGATION |
>                  SANDBOXED_AUXILIARY_NAVIGATION |
>                  SANDBOXED_TOPLEVEL_NAVIGATION |
>                  SANDBOXED_PLUGINS |
>                  SANDBOXED_ORIGIN |
>                  SANDBOXED_FORMS |
>                  SANDBOXED_SCRIPTS |
>                  SANDBOXED_AUTOMATIC_FEATURES |
>                  SANDBOXED_POINTER_LOCK |
>                  SANDBOXED_DOMAIN;
> 
>   ErrorResult err;
> 
>   IF_SBOXATTR("allow-same-origin",~SANDBOXED_ORIGIN)
>   IF_SBOXATTR("allow-forms", ~SANDBOXED_FORMS)
>   IF_SBOXATTR("allow-scripts", (~SANDBOXED_ORIGIN) & 
>       (~SANDBOXED_AUTOMATIC_FEATURES))
>   IF_SBOXATTR("allow-top-navigation", ~SANDBOXED_TOPLEVEL_NAVIGATION)
>   IF_SBOXATTR("allow-pointer-lock", ~SANDBOXED_POINTER_LOCK)
>   IF_SBOXATTR("allow-popups", ~SANDBOXED_AUXILIARY_NAVIGATION)
> 
>   return out;
> #undef IF_SBOXATTR
> }


But we might end up needing the current version for bug 671389.  The
alternative is to use this new function and figure out what we
actually need for bug 671389 once the (latter's) patch is updated.

Comment 2

[(no longer active)]

Comment on attachment 8347909 [details] [diff] [review]
0001-Bug-845057-Use-tokens-for-iframe-sandbox-property.patch

Review of attachment 8347909 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks a lot for the patch! Here's a couple of nits, but Boris should probably review the patch.

::: content/html/content/src/HTMLIFrameElement.cpp
@@ +11,4 @@
>  #include "nsRuleData.h"
>  #include "nsStyleConsts.h"
>  #include "nsContentUtils.h"
> +#include "nsDOMSettableTokenList.h"

You don't need this since you're including it in the header already.

::: content/html/content/src/HTMLIFrameElement.h
@@ +82,3 @@
>    {
> +    return
> +      const_cast<HTMLIFrameElement*>(this)->GetTokenList(nsGkAtoms::sandbox);

Making the method const and then const_casting the const-ness of this away doesn't really make sense.  ;-)

Comment 3

[Deian Stefan]

Attachment: 0001-Bug-845057-Use-tokens-for-iframe-sandbox-property.patch

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> Comment on attachment 8347909 [details] [diff] [review]
> 0001-Bug-845057-Use-tokens-for-iframe-sandbox-property.patch
> 
> Review of attachment 8347909 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks a lot for the patch! Here's a couple of nits, but Boris should
> probably review the patch.
> 
> ::: content/html/content/src/HTMLIFrameElement.cpp
> @@ +11,4 @@
> >  #include "nsRuleData.h"
> >  #include "nsStyleConsts.h"
> >  #include "nsContentUtils.h"
> > +#include "nsDOMSettableTokenList.h"
> 
> You don't need this since you're including it in the header already.
> 
> ::: content/html/content/src/HTMLIFrameElement.h
> @@ +82,3 @@
> >    {
> > +    return
> > +      const_cast<HTMLIFrameElement*>(this)->GetTokenList(nsGkAtoms::sandbox);
> 
> Making the method const and then const_casting the const-ness of this away
> doesn't really make sense.  ;-)

Woops, fixed this in a different patch... thanks for catching this.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8348448 [details] [diff] [review]
0001-Bug-845057-Use-tokens-for-iframe-sandbox-property.patch

> HTMLIFrameElement::GetSandboxFlags()
>+      sandbox->Stringify(sandboxAttr);

If we really want the string, we should just GetAttr.

But I believe that after this patch the only caller of ParseSandboxAttributeToFlags is this function.  So what we should do is just change ParseSandboxAttributeToFlags to take a parsed nsAttrValue (or even the nsDOMTokenList, but nsAttrValue makes more sense to me I think) and not redo all that parsing work.

I'd like to see this part fixed up, as a patch on top of this one.

>+    if (sandbox) {

Can't be null.

I assume we've given up on convincing hixie that this API is a bad idea?  :(

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, and need tests.

Comment 6

[Deian Stefan]

Attachment: 0001-Bug-845057-Part-1-Use-DOMSettableToken-for-iframe-sa.patch

Addressing Boris' commentes.
Carrying r+ from bz

Comment 7

[Deian Stefan]

Attachment: 0002-Bug-845057-Part-2-Tests-sanity-checking-sandbox-DOMS.patch

Comment 8

[Deian Stefan]

Attachment: 0003-Bug-845057-Part-3-ParseSandboxAttributeToFlags-uses-.patch

Change ParseSandboxAttributeToFlags to take a parsed nsAttrValue. Went with this over nsDOMSettableToken since it seems like it will be useful for bug 671389 as well.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8349860 [details] [diff] [review]
0002-Bug-845057-Part-2-Tests-sanity-checking-sandbox-DOMS.patch

Check that the add() call changed the return value of getAttribute() too?

r=me with that.

And ideally we'd have tests that verify that changing the token list affects sandboxing of the child document.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8349862 [details] [diff] [review]
0003-Bug-845057-Part-3-ParseSandboxAttributeToFlags-uses-.patch

>+  if (sandboxAttr) {

I'd reverse this test and do an early return.

>+    // If there's a sandbox attribute at all (and there is if this is
>+    // being called)

The parenthetical no longer matches reality.

>+  if (sandboxAttr->Contains(NS_ConvertASCIItoUTF16((str)))) { out &= (flag); }

This looks wrong.  What about:

  <iframe sandbox="ALLOW-SCRIPTS">

?  The old code would remove the SANDBOXED_SCRIPTS flag in that situation, but I believe your new code will not.

It's probably better to just create atoms for these strings in nsGkAtoms and then use the atom form of Contains(), which lets you do ASCII-case-insensitive checks.  And please add a test for this, if we don't have one already!

Also, I'd rename "flag" to "flags" and do "out &= ~(flags)", and then you don't have to put '~' in every single entry.  The allow-scripts case would pass "SANDBOXED_SCRIPTS | SANDBOXED_AUTOMATIC_FEATURES" as flags.

r=me with those changes, but please run the result by me again?

Comment 11

[Deian Stefan]

(In reply to Boris Zbarsky [:bz] (Vacation Dec 19 to Jan 1) from comment #9)
> Comment on attachment 8349860 [details] [diff] [review]
> 0002-Bug-845057-Part-2-Tests-sanity-checking-sandbox-DOMS.patch
> 
> Check that the add() call changed the return value of getAttribute() too?
>
> 
> r=me with that.

Thanks for taking a look! Will do. (Same goes with the other patch)

> And ideally we'd have tests that verify that changing the token list affects
> sandboxing of the child document.

Yes, ideally. I was going to add some here, but I think they should be in a separate bug.
Especially, since changing the attribute by setting 'allow-same-origin' has no effect (
see bug 951990).

Comment 12

[Deian Stefan]

Attachment: 0001-Bug-845057-Part-1-Use-DOMSettableToken-for-iframe-sa.patch

Carrying r+ from bz.

Comment 13

[Deian Stefan]

Attachment: 0002-Bug-845057-Part-2-Tests-sanity-checking-sandbox-DOMS.patch

Carrying r+ from bz.

Comment 14

[Deian Stefan]

Attachment: 0003-Bug-845057-Part-3-ParseSandboxAttributeToFlags-uses-.patch

Carrying r+ from bz.
Added mixed-case attribute to an iframe of the general iframe tests.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8350430 [details] [diff] [review]
0003-Bug-845057-Part-3-ParseSandboxAttributeToFlags-uses-.patch

Looks good, thanks.

Comment 16

[Deian Stefan]

Try: https://tbpl.mozilla.org/?tree=Try&rev=a0bb48638414

Comment 17

[Deian Stefan]

Attachment: 0003-Bug-845057-Part-3-ParseSandboxAttributeToFlags-uses-.patch

Carrying r+ from bz.
Nits (minor clean up comments in test), this was the try push.

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9976b6765bbf
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e7b84ab87c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e5d20af43eb

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9976b6765bbf
https://hg.mozilla.org/mozilla-central/rev/1e7b84ab87c4
https://hg.mozilla.org/mozilla-central/rev/2e5d20af43eb

Comment 20

[Kohei Yoshino]

https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement
https://developer.mozilla.org/en-US/Firefox/Releases/29/Site_Compatibility