1160017 - Get rid of permission for DOM fullscreen

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

After we implement the new transition for DOM fullscreen, we should be able to get rid of permission management for that API.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 1 - remove permission from dom part

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 2 - remove permission from browser part

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 3 - remove fullscreen from permission managing ui

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

I won't land this patchset before bug 1160014 finishes.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8633874 [details] [diff] [review]
patch 2 - remove permission from browser part

There are more stylesheets using the obscure-browser attribute. Please update those too.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 2 - remove permission from browser part

Comment 7

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

Comment on attachment 8633873 [details] [diff] [review]
patch 1 - remove permission from dom part

So someone from the security team has approved this new a bit scary behavior?

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Olli Pettay [:smaug] from comment #7)
> Comment on attachment 8633873 [details] [diff] [review]
> patch 1 - remove permission from dom part
> 
> So someone from the security team has approved this new a bit scary behavior?

Yes, with the transition implemented in bug 1160014, which makes it clearer for entering fullscreen.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #8)
> (In reply to Olli Pettay [:smaug] from comment #7)
> > Comment on attachment 8633873 [details] [diff] [review]
> > patch 1 - remove permission from dom part
> > 
> > So someone from the security team has approved this new a bit scary behavior?
> 
> Yes, with the transition implemented in bug 1160014, which makes it clearer
> for entering fullscreen.

Cool. To make it easier for interested folks, can you point us to exactly the place / comment where this got security sign-off?

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #9)
> Cool. To make it easier for interested folks, can you point us to exactly
> the place / comment where this got security sign-off?

It seems to me this decision has not been not published anywhere yet. Sheila Mooney told us in a private email that Daniel Veditz and Richard Barnes have agreed with this change. ni? them for confirmation.

Comment 11

[Jet Villegas (inactive)]

Please review this change as part of a fullscreen re-implementation as discussed with the UX and Security teams and described with a video posted here:

http://people.mozilla.org/~mverdi/projects/fullscreen/

While this change might appear to be a reduction in security, the ultimate goal is to reduce reliance on plugins for fullscreen videos & games--a much bigger security win.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #8)
> (In reply to Olli Pettay [:smaug] from comment #7)
> > Comment on attachment 8633873 [details] [diff] [review]
> > patch 1 - remove permission from dom part
> > 
> > So someone from the security team has approved this new a bit scary behavior?
> 
> Yes, with the transition implemented in bug 1160014, which makes it clearer
> for entering fullscreen.

While I am all for reducing permissions prompts, the fullscreen one was never that odious (especially since it only showed up after the fullscreen effect already happened).  I don't see how a transition reduces security concerns, though.  If the user is sitting at the computer, then yes, they'd notice a transition happened (but if it transitioned to something that looked suspiciously like a desktop, would they realize it's a web page).

However, it's trivial to have a good idea of when the user is inactive.  Waiting until the user has been inactive for, say, 30 minutes and then fullscreen-transitioning to something that looks like an OS-appropriate password prompt seems like a pretty trivial spoof.  Is this something we're comfortable with being a non-issue?

Comment 13

[Dão Gottwald [:dao]]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #12)
> However, it's trivial to have a good idea of when the user is inactive. 
> Waiting until the user has been inactive for, say, 30 minutes and then
> fullscreen-transitioning to something that looks like an OS-appropriate
> password prompt seems like a pretty trivial spoof.  Is this something we're
> comfortable with being a non-issue?

We could probably display the "foo.com is now fullscreen" message until the user becomes active...

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to Dão Gottwald [:dao] from comment #13)
> (In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #12)
> > However, it's trivial to have a good idea of when the user is inactive. 
> > Waiting until the user has been inactive for, say, 30 minutes and then
> > fullscreen-transitioning to something that looks like an OS-appropriate
> > password prompt seems like a pretty trivial spoof.  Is this something we're
> > comfortable with being a non-issue?
> 
> We could probably display the "foo.com is now fullscreen" message until the
> user becomes active...

That's a great idea.  Make sure that message stays on the screen and kick off the timer to fade it out only when user activity is seen.

Comment 15

[Jet Villegas (inactive)]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #14)
> (In reply to Dão Gottwald [:dao] from comment #13)
> > We could probably display the "foo.com is now fullscreen" message until the
> > user becomes active...
> 
> That's a great idea.  Make sure that message stays on the screen and kick
> off the timer to fade it out only when user activity is seen.

I like it too. I would start the timer from the last user gesture (ie. the click that entered fullscreen.) If that timer expires because fullscreen was delayed (eg. in the attack described,) to start the timer from the first user gesture after fullscreen was entered. I think this nicely keeps the friction low while protecting from spoofing.

Comment 16

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

Need to have some way to hide the warning using keyboard too, but yes, sounds like a rather nice UI.

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #12)
> While I am all for reducing permissions prompts, the fullscreen one was
> never that odious (especially since it only showed up after the fullscreen
> effect already happened).  I don't see how a transition reduces security
> concerns, though.  If the user is sitting at the computer, then yes, they'd
> notice a transition happened (but if it transitioned to something that
> looked suspiciously like a desktop, would they realize it's a web page).

We still shows a fullscreen warning.

> However, it's trivial to have a good idea of when the user is inactive. 
> Waiting until the user has been inactive for, say, 30 minutes and then
> fullscreen-transitioning to something that looks like an OS-appropriate
> password prompt seems like a pretty trivial spoof.  Is this something we're
> comfortable with being a non-issue?

Are we able to address this issue now with the permission requirement? If a website, which has been in fullscreen, wants, it can always switch to a UI like that.

Note that we still require one user input for requesting fullscreen, which means website is still not able to enter fullscreen without user's action, and transition enables users to see clearly how the change is related to their action.

(In reply to Dão Gottwald [:dao] from comment #13)
> We could probably display the "foo.com is now fullscreen" message until the
> user becomes active...

We can't. If a user is watching a movie, he/she may not interact with any input device for a very long time. We must not show such message for that. And also it is not possible for us to distinguish whether a user is still there, or has left the computer for a while, unless we do some face recognition from the camera.

Comment 18

[Dão Gottwald [:dao]]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #17)
> (In reply to Dão Gottwald [:dao] from comment #13)
> > We could probably display the "foo.com is now fullscreen" message until the
> > user becomes active...
> 
> We can't. If a user is watching a movie, he/she may not interact with any
> input device for a very long time. We must not show such message for that.

In order to go fullscreen while watching a movie, users usually interact with the UI, which means we'd already consider them "active" at that point.

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Dão Gottwald [:dao] from comment #18)
> 
> In order to go fullscreen while watching a movie, users usually interact
> with the UI, which means we'd already consider them "active" at that point.

We always require a user input event for entering fullscreen. Website can never go to fullscreen if user doesn't do anything within one second before.

The patches here just removes the additional step where user needs to explicitly approve the permission.

The logic is that, if the transition and the warning are obvious enough, the website would not be able to control the screen without user's awareness, and thus we do not need to ask for explicit approval in addition.

Comment 20

[Dão Gottwald [:dao]]

This means the scenario from comment 12 isn't actually possible?

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

I thought the scenario from comment 12 is about, a website has entered fullscreen, and then turns to some spoof interface later, which is a case inevitably vulnerable even we ask for approval.

If that comment is about that a website can enter fullscreen anytime it wants, which is currently only blocked by the permission approval, then I have to say it is not that case.

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

OK, I've finished all patches for bug 1160014 (though Mac one and GTK one are still pending for review). Per my discussion with jet today, I'm going to land this bug first now.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b76ca34d720

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb5617ed89cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/c052a1018d44
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b66287d1071

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bb5617ed89cf
https://hg.mozilla.org/mozilla-central/rev/c052a1018d44
https://hg.mozilla.org/mozilla-central/rev/2b66287d1071

Comment 25

[Daniel Veditz [:dveditz]]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #10)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #9)
> > Cool. To make it easier for interested folks, can you point us to exactly
> > the place / comment where this got security sign-off?
> 
> It seems to me this decision has not been not published anywhere yet. Sheila
> Mooney told us in a private email that Daniel Veditz and Richard Barnes have
> agreed with this change. ni? them for confirmation.

I thought you were part of the conversation too? Can't remember if it was in a bug or a mail thread, but we approved it given a mock up of a transition that showed
 * user action required
 * a transition effect
 * a notice that XXX has gone fullscreen that stays on screen for a couple of seconds (1.5? 3?)
   The mockup had a specific behavior that tried to strike a balance between annoying enough
   to be noticed but not _too_ annoying.

Comment 26

[avada]

So this effectively forces me to watch the annoying fullscreen warning every single time? The first thing I did when HTML5 videos became available is disable it via full-screen-api.approval-required;false.
Why not provide a "don't warn me again" checkbox like the norm is for annoying nonsense like this.

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

Comment 25 has answered my question in comment 10. Remove all the ni?s.

Comment 28

[Vasilica Tamas, QA [:vtamas]]

Verified on Firefox 42 beta 8 (20151019161651) under Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.10.4 and the fullscreen modal dialog looks like: http://i.imgur.com/0vpIGXj.jpg .

I’m not sure if this is the expected design because it does not correspond to the mockups from Comment 11 ( http://people.mozilla.org/~mverdi/projects/fullscreen/ ). 


Xidorn, could you please confirm that this ( http://i.imgur.com/0vpIGXj.jpg ) is the intended design for Firefox 42?

Comment 29

[Xidorn Quan [:xidorn] UTC+10]

Yes, I think this is what we are shipping for Firefox 42, because I failed to land bug 1160023 on this version.

Comment 30

[Vasilica Tamas, QA [:vtamas]]

Thanks Xidorn for your prompt answer! 

Based on Comment 28 and Comment 29, I am marking this bug as Verified Fixed.