Closed Bug 1270648 Opened 4 years ago Closed 4 years ago

allowfullscreen is not correctly reflected in sandbox flags

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: bzbarsky, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

(Whiteboard: btpp-active)

Attachments

(5 files)

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?
Flags: needinfo?(tanvi)
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?
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...
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.
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.
Assignee: nobody → bugzilla
I think in practice that is the difference, yes.
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.
Depends on: 1190641
Flags: needinfo?(tanvi)
Whiteboard: btpp-active
Thanks for taking this Xidorn!
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/
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/
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.
Attachment #8752628 - Flags: review?(bugs) → review+
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.
Attachment #8752629 - Flags: review?(bugs) → review+
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 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
Attachment #8752630 - Flags: review?(bugs) → review+
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
Attachment #8752631 - Flags: review?(bugs) → review+
Attachment #8752632 - Flags: review?(bugs) → review+
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 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 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 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 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 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 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.
Attachment #8752631 - Flags: review+ → review?(bugs)
Don't you just want to update the flag in XULDocument::StartDocumentLoad. Feels scary to default to true for the flag.
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
Attachment #8752631 - Flags: review?(bugs)
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/
Attachment #8752631 - Flags: review?(bugs)
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 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 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 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 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
Attachment #8752631 - Flags: review?(bugs) → review+
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
Depends on: 1277556
Depends on: 1279613
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.