(no longer active) Reporter
	Description • 11 years ago

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

	Deian Stefan Assignee
	Comment 1 • 11 years ago

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.

	(no longer active) Reporter
	Comment 2 • 11 years ago

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.  ;-)

	Deian Stefan Assignee
	Comment 3 • 11 years ago

(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.

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 11 years ago

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?  :(

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 11 years ago

Oh, and need tests.

	Deian Stefan Assignee
	Comment 6 • 11 years ago

Addressing Boris' commentes.
Carrying r+ from bz

	Deian Stefan Assignee
	Comment 8 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 11 years ago

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?

	Deian Stefan Assignee
	Comment 11 • 11 years ago

(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).

	Deian Stefan Assignee
	Comment 12 • 11 years ago

Carrying r+ from bz.

	Deian Stefan Assignee
	Comment 13 • 11 years ago

Carrying r+ from bz.

	Deian Stefan Assignee
	Comment 14 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 11 years ago

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

Looks good, thanks.

	Deian Stefan Assignee
	Comment 16 • 11 years ago

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

	Deian Stefan Assignee
	Comment 17 • 11 years ago

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

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 18 • 11 years ago

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

	Phil Ringnalda (:philor)
	Comment 19 • 11 years ago

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

	Kohei Yoshino [:kohei]
	Comment 20 • 10 years ago

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