Fix the type of HTMLIFrameElement.sandbox

RESOLVED FIXED in mozilla29

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Ehsan, Assigned: Deian Stefan)

Tracking

({addon-compat, dev-doc-complete, site-compat})

Trunk
mozilla29
addon-compat, dev-doc-complete, site-compat
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

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

Comment 1

4 years ago
Created attachment 8347909 [details] [diff] [review]
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.
Attachment #8347909 - Flags: review?(ehsan)
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.  ;-)
Attachment #8347909 - Flags: review?(ehsan) → review?(bzbarsky)
(Assignee)

Comment 3

4 years ago
Created attachment 8348448 [details] [diff] [review]
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.
Assignee: nobody → deian
Attachment #8347909 - Attachment is obsolete: true
Attachment #8348448 - Flags: review?(bzbarsky)
Attachment #8347909 - Flags: review?(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?  :(
Attachment #8348448 - Flags: review?(bzbarsky) → review+
Oh, and need tests.
(Assignee)

Comment 6

4 years ago
Created attachment 8349859 [details] [diff] [review]
0001-Bug-845057-Part-1-Use-DOMSettableToken-for-iframe-sa.patch

Addressing Boris' commentes.
Carrying r+ from bz
Attachment #8348448 - Attachment is obsolete: true
(Assignee)

Comment 7

4 years ago
Created attachment 8349860 [details] [diff] [review]
0002-Bug-845057-Part-2-Tests-sanity-checking-sandbox-DOMS.patch
Attachment #8349860 - Flags: review?(bzbarsky)
(Assignee)

Comment 8

4 years ago
Created attachment 8349862 [details] [diff] [review]
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.
Attachment #8349862 - Flags: review?(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.
Attachment #8349860 - Flags: review?(bzbarsky) → review+
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?
Attachment #8349862 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 11

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

Comment 12

4 years ago
Created attachment 8350428 [details] [diff] [review]
0001-Bug-845057-Part-1-Use-DOMSettableToken-for-iframe-sa.patch

Carrying r+ from bz.
Attachment #8349859 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
Created attachment 8350429 [details] [diff] [review]
0002-Bug-845057-Part-2-Tests-sanity-checking-sandbox-DOMS.patch

Carrying r+ from bz.
Attachment #8349860 - Attachment is obsolete: true
(Assignee)

Comment 14

4 years ago
Created attachment 8350430 [details] [diff] [review]
0003-Bug-845057-Part-3-ParseSandboxAttributeToFlags-uses-.patch

Carrying r+ from bz.
Added mixed-case attribute to an iframe of the general iframe tests.
Attachment #8349862 - Attachment is obsolete: true
Attachment #8350430 - Flags: feedback?(bzbarsky)
Comment on attachment 8350430 [details] [diff] [review]
0003-Bug-845057-Part-3-ParseSandboxAttributeToFlags-uses-.patch

Looks good, thanks.
Attachment #8350430 - Flags: feedback?(bzbarsky) → feedback+
(Assignee)

Comment 16

4 years ago
Try: https://tbpl.mozilla.org/?tree=Try&rev=a0bb48638414
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 17

4 years ago
Created attachment 8351045 [details] [diff] [review]
0003-Bug-845057-Part-3-ParseSandboxAttributeToFlags-uses-.patch

Carrying r+ from bz.
Nits (minor clean up comments in test), this was the try push.
Attachment #8350430 - Attachment is obsolete: true
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
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9976b6765bbf
https://hg.mozilla.org/mozilla-central/rev/1e7b84ab87c4
https://hg.mozilla.org/mozilla-central/rev/2e5d20af43eb
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement
https://developer.mozilla.org/en-US/Firefox/Releases/29/Site_Compatibility
Keywords: addon-compat, dev-doc-complete, site-compat
OS: Mac OS X → All
Hardware: x86 → All
You need to log in before you can comment on or make changes to this bug.