1270648 - allowfullscreen is not correctly reflected in sandbox flags

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Per spec, a sandboxed iframe without allowfullscreen should disallow fullscreen requests inside itself.  Crucially, per spec this is true even if allowfullscreen is later added to the iframe element, and is true for any popups opened from inside the iframe, since those inherit the sandboxing flags.  We don't seem to do this correctly.  Testcase:

  data:text/html,<iframe sandbox="allow-scripts" srcdoc='<div onclick="this.mozRequestFullScreen(this);">Click me; am I fullscreen?</div>' onload="this.setAttribute('allowfullscreen', '')"></iframe>

Tanvi, do you know who might have time to fix this?

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

I don't completely understand the spec. It seems to me the "fullscreen enabled flag" is set only when the attribute is present at the moment the document is loaded, regardless of whether the iframe is sandboxed. I'm not sure how the "sandboxed fullscreen browsing context flag" differs from that flag. Probably they are set at different time?

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

And it seems "sandboxed fullscreen browsing context flag" affects the behavior via "fullscreen enabled flag" as well, but both of them are derived from "allowfullscreen" attribute. This seems pretty confusing...

Comment 3

[Boris Zbarsky [:bzbarsky]]

The model the spec has is that whether fullscreen is enabled for a document is an immutable property of that document (unlike what we do, where we compute it dynamically at request time).

There are two boolean flags that control the behavior that are both set at document creation time.  The "fullscreen enabled flag" is set if the document is a toplevel document, or if it's in an <iframe>, that iframe has allowfullscreen, and the iframe's ownerDocument already has the "fullscreen enabled flag".

The "sanboxed fullscreen browsing context flag" is set based on various sandboxing stuff.  It's also affected by the "allowfullscreen" attribute, in that it's _not_ set if that attribute is present.  Note that unlike the "fullscreen enabled flag" this one propagates to windows you open, unless allow-popups-to-escape-sandbox is used.

To allow fullscreen, the "fullscreen enabled flag" must be set and the "sanboxed fullscreen browsing context flag" must not be set.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

So the only difference between the two flag is whether the popup is allowed to enter fullscreen, correct?

Thanks for explaining. I should definitely look into this.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I think in practice that is the difference, yes.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

It seems there are some conflicts between the patch I'm writing and the patches proposed in bug 1190641. Make it depend on that, and I'll start working on this after that bug gets fixed.

Also since I've taken this bug, cancel ni? Tanvi.

Comment 7

[Tanvi Vyas[:tanvi]]

Thanks for taking this Xidorn!

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1270648 part 1 - Move #include out from sandbox keyword list. r?smaug

Review commit: https://reviewboard.mozilla.org/r/52668/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52668/

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1270648 part 2 - Merge nsContentUtils::ParseSandboxAttributeToFlags into HTMLIFrameElement::GetSandboxFlags. r?smaug

nsContentUtils::ParseSandboxAttributeToFlags is not used anywhere else,
and given that sandbox flags would also be affected by allowfullscreen
attribute, this function alone could be misused.

Review commit: https://reviewboard.mozilla.org/r/52670/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52670/

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1270648 part 3 - Make document state not affect the value of fullscreenEnabled. r?smaug

The function GetFullscreenError() is only used in fullscreen element
ready check and computing value of fullscreenEnabled attribute. And for
the latter case, the state of the document (visibility and subdocument
fullscreen) should not be considered as a factor per spec.

In addition, allowing chrome to enter fullscreen when the document is
invisible or already has a fullscreen subdocument would likely ruin
things, as we have assumptions elsewhere those should not happen at all.

Review commit: https://reviewboard.mozilla.org/r/52672/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52672/

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1270648 part 4 - Make fullscreen enabled flag not be affected after document is loaded. r?smaug

Review commit: https://reviewboard.mozilla.org/r/52674/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52674/

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1270648 part 5 - Setup the sandboxed fullscreen flag and make it work properly. r?smaug

Review commit: https://reviewboard.mozilla.org/r/52676/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52676/

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

I thought bz is more familiar with the sandbox part here, as he was working on bug 1190641. But it seems bz is not currently accepting review request... Sorry, smaug.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8752628 [details]
MozReview Request: Bug 1270648 part 1 - Move #include out from sandbox keyword list. r?smaug

https://reviewboard.mozilla.org/r/52668/#review49664

Not sure why when the list after all contains stuff from nsSandboxFlags.h. But fine.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8752629 [details]
MozReview Request: Bug 1270648 part 2 - Merge nsContentUtils::ParseSandboxAttributeToFlags into HTMLIFrameElement::GetSandboxFlags. r?smaug

https://reviewboard.mozilla.org/r/52670/#review49666

::: dom/base/nsContentUtils.h
(Diff revision 1)
>    static nsresult GetLocalizedString(PropertiesFile aFile,
>                                       const char* aKey,
>                                       nsXPIDLString& aResult);
>  
>    /**
> -   * A helper function that parses a sandbox attribute (of an <iframe> or

Hmm, so this was used for CSP directives too. What do we do for that these days...
Ok, I see https://bugzilla.mozilla.org/show_bug.cgi?id=671389 is still ongoing but this patch shouldn't affect to that too badly.

::: dom/html/HTMLIFrameElement.cpp:250
(Diff revision 1)
>  
>  uint32_t
>  HTMLIFrameElement::GetSandboxFlags()
>  {
>    const nsAttrValue* sandboxAttr = GetParsedAttr(nsGkAtoms::sandbox);
> -  return nsContentUtils::ParseSandboxAttributeToFlags(sandboxAttr);
> +  // No sandbox attribute, no sandbox flags.

You could just use normal style here.
so 
if (...) {
  ...
}

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8752630 [details]
MozReview Request: Bug 1270648 part 3 - Make document state not affect the value of fullscreenEnabled. r?smaug

https://reviewboard.mozilla.org/r/52672/#review49672

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8752631 [details]
MozReview Request: Bug 1270648 part 4 - Make fullscreen enabled flag not be affected after document is loaded. r?smaug

https://reviewboard.mozilla.org/r/52674/#review49676

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8752632 [details]
MozReview Request: Bug 1270648 part 5 - Setup the sandboxed fullscreen flag and make it work properly. r?smaug

https://reviewboard.mozilla.org/r/52676/#review49678

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8752628 [details]
MozReview Request: Bug 1270648 part 1 - Move #include out from sandbox keyword list. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52668/diff/1-2/

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8752629 [details]
MozReview Request: Bug 1270648 part 2 - Merge nsContentUtils::ParseSandboxAttributeToFlags into HTMLIFrameElement::GetSandboxFlags. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52670/diff/1-2/

Comment 21

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8752630 [details]
MozReview Request: Bug 1270648 part 3 - Make document state not affect the value of fullscreenEnabled. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52672/diff/1-2/

Comment 22

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8752631 [details]
MozReview Request: Bug 1270648 part 4 - Make fullscreen enabled flag not be affected after document is loaded. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52674/diff/1-2/

Comment 23

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8752632 [details]
MozReview Request: Bug 1270648 part 5 - Setup the sandboxed fullscreen flag and make it work properly. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52676/diff/1-2/

Comment 24

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8752631 [details]
MozReview Request: Bug 1270648 part 4 - Make fullscreen enabled flag not be affected after document is loaded. r?smaug

Fail to enter fullscreen at all with the previous patch...

It seems StartDocumentLoad is never called for the top level browser document, so we need to have fullscreen enabled flag set by default.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Don't you just want to update the flag in XULDocument::StartDocumentLoad. Feels scary to default to true for the flag.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8752631 [details]
MozReview Request: Bug 1270648 part 4 - Make fullscreen enabled flag not be affected after document is loaded. r?smaug

https://reviewboard.mozilla.org/r/52674/#review49944

Comment 27

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8752628 [details]
MozReview Request: Bug 1270648 part 1 - Move #include out from sandbox keyword list. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52668/diff/2-3/

Comment 28

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8752629 [details]
MozReview Request: Bug 1270648 part 2 - Merge nsContentUtils::ParseSandboxAttributeToFlags into HTMLIFrameElement::GetSandboxFlags. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52670/diff/2-3/

Comment 29

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8752630 [details]
MozReview Request: Bug 1270648 part 3 - Make document state not affect the value of fullscreenEnabled. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52672/diff/2-3/

Comment 30

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8752631 [details]
MozReview Request: Bug 1270648 part 4 - Make fullscreen enabled flag not be affected after document is loaded. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52674/diff/2-3/

Comment 31

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8752632 [details]
MozReview Request: Bug 1270648 part 5 - Setup the sandboxed fullscreen flag and make it work properly. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52676/diff/2-3/

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8752631 [details]
MozReview Request: Bug 1270648 part 4 - Make fullscreen enabled flag not be affected after document is loaded. r?smaug

https://reviewboard.mozilla.org/r/52674/#review49970

Comment 33

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1585944a91831310581777033ebae204a42c32eb
Bug 1270648 part 1 - Move #include out from sandbox keyword list. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb5a3ba5e1412535ad5fd6cf822ca36780984cf3
Bug 1270648 part 2 - Merge nsContentUtils::ParseSandboxAttributeToFlags into HTMLIFrameElement::GetSandboxFlags. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/1fcf8a5711ed198fd5e77125d7edb1dea130fee2
Bug 1270648 part 3 - Make document state not affect the value of fullscreenEnabled. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/99b914f79c698eaa69a912453e71f6eb639e76c3
Bug 1270648 part 4 - Make fullscreen enabled flag not be affected after document is loaded. r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/6179d77a0c072e2e394be3faccd1d4bda2f4b402
Bug 1270648 part 5 - Setup the sandboxed fullscreen flag and make it work properly. r=smaug

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1585944a9183
https://hg.mozilla.org/mozilla-central/rev/eb5a3ba5e141
https://hg.mozilla.org/mozilla-central/rev/1fcf8a5711ed
https://hg.mozilla.org/mozilla-central/rev/99b914f79c69
https://hg.mozilla.org/mozilla-central/rev/6179d77a0c07