FeaturePolicy: fullscreen

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P2
normal
RESOLVED FIXED
8 months ago
2 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

({dev-doc-complete})

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: [domsecurity-backlog1] [domsecurity-active])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 months ago
Supporting for 'fullscreen' feature policy.
(Assignee)

Comment 1

8 months ago
Posted patch 4_feature_fullscreen.patch (obsolete) — Splinter Review
Attachment #9013217 - Flags: review?(bugs)
(Assignee)

Updated

8 months ago
Blocks: 1495364
Comment on attachment 9013217 [details] [diff] [review]
4_feature_fullscreen.patch

"FeturePolicy"
Comment on attachment 9013217 [details] [diff] [review]
4_feature_fullscreen.patch

I tried and failed to find a patch which implements 
FeaturePolicyUtils::IsFeatureAllowed.

I assume pages by default allow fullscreen? And then something may disable it?
Would it make sense to have
FeaturePolicyUtils::IsFeatureDenied, if that is the special case?

But anyhow, I can't review this without knowing what 
FeaturePolicyUtils::IsFeatureAllowed does
Attachment #9013217 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] (r- if the bug doesn't explain what the change(s) are about.) from comment #3)
> I tried and failed to find a patch which implements 
> FeaturePolicyUtils::IsFeatureAllowed.
> 
> I assume pages by default allow fullscreen? And then something may disable
> it?
> Would it make sense to have
> FeaturePolicyUtils::IsFeatureDenied, if that is the special case?
Or perhaps FeaturePolicyUtils::IsFeatureAllowed is fine, but it needs to be clear which features are enabled by default. Is that documented somewhere?
Priority: -- → P2
(Assignee)

Comment 5

8 months ago
Comment on attachment 9013217 [details] [diff] [review]
4_feature_fullscreen.patch

About the default value, yes I agree with you. But it's not a problem of this particular feature: we should discuss what the default policy value should for any existing feature.
Attachment #9013217 - Flags: review?(bugs)
Comment on attachment 9013217 [details] [diff] [review]
4_feature_fullscreen.patch

I don't see this patch testing the code path. We need to have tests which do X and keep doing X after this patch too. And then tests which ensure that feature policy can enabled or block fullscreen usage. You said there are wpts, but where exactly? I would expect something to be enabled in this bug.


s/FeturePolicy/FeaturePolicy/
Attachment #9013217 - Flags: review?(bugs) → review-
(Assignee)

Comment 7

8 months ago
test included
Attachment #9013217 - Attachment is obsolete: true
Attachment #9014083 - Flags: review?(bugs)
Comment on attachment 9014083 [details] [diff] [review]
4_feature_fullscreen.patch

>+++ b/dom/html/HTMLIFrameElement.cpp
>@@ -322,14 +322,18 @@ HTMLIFrameElement::RefreshFeaturePolicy(
>   }
> 
>   mFeaturePolicy->InheritPolicy(OwnerDoc()->Policy());
> 
>   if (AllowPaymentRequest()) {
>     mFeaturePolicy->MaybeSetAllowedPolicy(NS_LITERAL_STRING("payment"));
>   }
> 
>+  if (AllowFullscreen()) {
>+    mFeaturePolicy->MaybeSetAllowedPolicy(NS_LITERAL_STRING("fullscreen"));
>+  }
>+
ok, so this was missing in the previous patch after all.


>+function doRequestFullscreen() {
>+  function handler(evt) {
>+    document.removeEventListener("fullscreenchange", handler);
>+    document.removeEventListener("fullscreenerror", handler);
>+    parent.continueTest(evt.type);
>+  }
Don't you want to call document.exitFullscreen() if fullscreenchange fired.


>+
>+  let test = tests.shift();
>+
>+  // Create an iframe with an allowfullscreen but with allow "fullscreen none".
This comment is wrong. You aren't creating "fullscreen none" but use the values from the tests array.

>+  // The request should be denied, and we should not receive a fullscreenchange
>+  // event in this document.
and then fix also this.


>+  var iframe = document.createElement("iframe");
>+  iframe.setAttribute("allowfullscreen", "true");
>+  iframe.setAttribute("allow", test[0]);
>+  iframe.src = INNER_FILE;
>+  setupForInnerTest("an iframe+allowfullscreen but denied by FeaturePolicy", event => {
And comment needs a fix too



> FullscreenDeniedSubDocFullscreen=Request for fullscreen was denied because a subdocument of the document requesting fullscreen is already fullscreen.
> FullscreenDeniedNotDescendant=Request for fullscreen was denied because requesting element is not a descendant of the current fullscreen element.
> FullscreenDeniedNotFocusedTab=Request for fullscreen was denied because requesting element is not in the currently focused tab.
>+FullscreenDeniedFeaturePolicy=Request for fullscreen was denied because against FeturePolicy directives.
FeturePolicy. And there is something wrong with the sentence.
perhaps
...because of FeaturePolicy directives



Please make sure to run the test on try several times. fullscreen tests haven't been too stable in the past.


(https://fullscreen.spec.whatwg.org/#feature-policy-integration)
Attachment #9014083 - Flags: review?(bugs) → review+
(Assignee)

Comment 9

8 months ago
> (https://fullscreen.spec.whatwg.org/#feature-policy-integration)

Thanks for the review. Do you want to file a spec bug for the default allowlist? Spec says:
"This specification defines a policy-controlled feature identified by the string "fullscreen". Its default allowlist is 'self'."

But we are implementing 'all' for backward compatibility.
Flags: needinfo?(bugs)
But allowfullscreen makes the value all, right?

All I'm hoping is that pages which _don't_ use feature policy, don't break with all the feature policy stuff.
Flags: needinfo?(bugs)
(Assignee)

Comment 11

8 months ago
> But allowfullscreen makes the value all, right?

if allowfullscreen is an attribute, it creates a FeaturePolicy 'fullscreen *' if we don't have any other 'fullscreen' declared directives. I'll add a test for this.

Comment 13

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cafd172dc48a
Status: NEW → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Comment on attachment 9014083 [details] [diff] [review]
4_feature_fullscreen.patch

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

::: dom/locales/en-US/chrome/dom/dom.properties
@@ +69,5 @@
>  FullscreenDeniedLostWindow=Request for fullscreen was denied because we no longer have a window.
>  FullscreenDeniedSubDocFullscreen=Request for fullscreen was denied because a subdocument of the document requesting fullscreen is already fullscreen.
>  FullscreenDeniedNotDescendant=Request for fullscreen was denied because requesting element is not a descendant of the current fullscreen element.
>  FullscreenDeniedNotFocusedTab=Request for fullscreen was denied because requesting element is not in the currently focused tab.
> +FullscreenDeniedFeaturePolicy=Request for fullscreen was denied because against FeturePolicy directives.

FeaturePolicy, I guess?
Flags: needinfo?(amarchesini)
QA Contact: overholt
BTW, why is the string in the patch different from what actually landed?
https://hg.mozilla.org/mozilla-central/rev/cafd172dc48a#l7.12

> +FullscreenDeniedFeaturePolicy=Request for fullscreen was denied because of FeturePolicy directives.

P.S: no clue why opening a NI reset the QA Contact…
QA Contact: overholt
Hrm, I mentioned about FeturePolicy misspelling several times here. comment 2 and comment 6 and comment 8.
(Assignee)

Comment 17

8 months ago
I didn't find that misspelling in the patch. I thought it was in the code and not in the properties file. I was looking in the wrong place. I'm filing a bug to fix this typo.
Flags: needinfo?(amarchesini)
(Assignee)

Updated

8 months ago
Blocks: 1496894
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.