1284878 - display a scary warning when the user is about to share Firefox or the whole screen

Status: VERIFIED FIXED

(Firefox :: Site Permissions, defect, P2)

Description

[Florian Quèze [:florian]]

As part of the screensharing whitelist removal work, we need to show a warning when the user is about to share Firefox or the whole screen, to increase the chances of the user understanding the risk.

UX mockup in https://mozilla.invisionapp.com/share/AF71R266U#/screens/152779760

Bug for the copy of the warning string: bug 1280403

Comment 1

[Erin Lancaster [:elan]]

P1 after the preview work is complete, prior to telemetry.

Comment 2

[Florian Quèze [:florian]]

Attachment: WIP v1

Most of the work done, what remains:
- show a warning triangle icon
- make clicks on the Learn More link open https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/screenshare-safety
- (optional) set margin-inline-start to 0 on lots of elements in this panel to get correct alignment of the preview box with the menulist boxes and with the labels above. This is already misaligned with mismatched margins, but the big preview box now with a border makes it even more visible. This could be left for a follow-up polish bug.

Comment 3

[Florian Quèze [:florian]]

Attachment: Patch v2

Currently the warning will be displayed for whole screens; if you want to test the warning for Firefox windows, you'll need to apply locally the patch from bug 1311048.

Comment 4

[:Gijs (he/him)]

Comment on attachment 8807237 [details] [diff] [review]
Patch v2

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

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +485,5 @@
>  getUserMedia.shareCameraAndAudioCapture.message = Would you like to share your camera and this tab’s audio with %S?
>  getUserMedia.shareScreenAndMicrophone.message = Would you like to share your microphone and screen with %S?
>  getUserMedia.shareScreenAndAudioCapture.message = Would you like to share this tab’s audio and your screen with %S?
>  getUserMedia.shareAudioCapture.message = Would you like to share this tab’s audio with %S?
> +getUserMedia.shareScreenWarning.message = Only share screens with sites you trust. Sharing can allow deceptive sites to browse as you and steal your private data.

Where did this copy come from? What does "browse as you" mean here? I didn't think screen sharing allowed the party receiving the media stream to control your browser directly...

::: browser/modules/webrtcUI.jsm
@@ +536,5 @@
>  
> +          let warning = chromeDoc.getElementById("webRTC-previewWarning");
> +          let chromeWin = chromeDoc.defaultView;
> +          if (event.target.scary) {
> +            warning.hidden = false;

Nit: put warning.hidden = event.target.scary; on the outside of the if block and drop the else.

@@ +548,5 @@
> +                chromeDoc.getElementById("bundle_brand").getString("brandShortName");
> +              string = bundle.getFormattedString("getUserMedia.shareFirefoxWarning.message",
> +                                                 [brand], 1);
> +            }
> +            warning.firstChild.data = string + " ";

This is not a good idea for l10n.

Can we instead stick the <label> in the string, have the localization note indicate not to translate it, and then assign to innerHTML and manipulate the label? afterwards? It doesn't need an ID, you can just querySelector("label"), to keep the markup inside the l10n string simple. As a bonus, you won't need the separate learn more entity, and l10n gets more flexibility to manipulate the link's incorporation in the text, plus more context when translating.

::: browser/themes/shared/notification-icons.inc.css
@@ +154,5 @@
>  .screen-icon.blocked-permission-icon {
>    list-style-image: url(chrome://browser/skin/notification-icons.svg#screen-blocked);
>  }
>  
> +#webRTC-preview:not([hidden="true"]) {

Nit: Should be able to just use :not([hidden])

@@ +157,5 @@
>  
> +#webRTC-preview:not([hidden="true"]) {
> +  display: -moz-stack;
> +  border-radius: 4px;
> +  border: 1px solid #757575;

Should this just be GrayText?

@@ +171,5 @@
>    max-height: 200px;
>  }
>  
> +#webRTC-previewWarning {
> +  background: rgba(255, 217, 99, .8) url("chrome://browser/skin/warning-white.svg") no-repeat .5em .5em;

Why the semi-transparency? Feels like we should just use an opaque color, to avoid not fitting in with however the panel is themed by the OS theme.

::: browser/themes/shared/warning-white.svg
@@ +1,4 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="20" height="20" viewBox="0 0 60 60">

Why is this width+height=20, but with a 60x60 viewbox?

Can we normalize this? :-)

@@ +1,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="20" height="20" viewBox="0 0 60 60">
> +  <path fill="#fff" stroke="#cdad51" stroke-width="2" d="M49.316,42.867L33.829,12.7c-0.879-1.715-2.274-2.7-3.828-2.7c-1.554,0-2.949,0.985-3.829,2.702 L10.685,42.864c-0.869,1.69-0.913,3.482-0.121,4.909C11.35,49.187,12.817,50,14.591,50h30.82c1.772,0,3.24-0.81,4.023-2.224 C50.227,46.349,50.185,44.56,49.316,42.867z M32.176,22.304l-0.48,14.304h-3.424L27.76,22.304H32.176z M30,44.896 c-1.44,0-2.592-1.184-2.592-2.592c0-1.44,1.152-2.592,2.592-2.592c1.472,0,2.592,1.152,2.592,2.592 C32.592,43.712,31.472,44.896,30,44.896z"/>

The choice of stroke here is... interesting. Why not black?

Comment 5

[Florian Quèze [:florian]]

(In reply to :Gijs Kruitbosch from comment #4)

Thanks for the review!

> ::: browser/locales/en-US/chrome/browser/browser.properties
> @@ +485,5 @@
> >  getUserMedia.shareCameraAndAudioCapture.message = Would you like to share your camera and this tab’s audio with %S?
> >  getUserMedia.shareScreenAndMicrophone.message = Would you like to share your microphone and screen with %S?
> >  getUserMedia.shareScreenAndAudioCapture.message = Would you like to share this tab’s audio and your screen with %S?
> >  getUserMedia.shareAudioCapture.message = Would you like to share this tab’s audio with %S?
> > +getUserMedia.shareScreenWarning.message = Only share screens with sites you trust. Sharing can allow deceptive sites to browse as you and steal your private data.
> 
> Where did this copy come from? What does "browse as you" mean here? I didn't
> think screen sharing allowed the party receiving the media stream to control
> your browser directly...

It comes from bug 1280403 after a very long (years...) discussion. This non-obvious risk is the very reason screen sharing has been behind a whitelist since we introduced it.

What it means technically is that a webpage that can have a stream of itself (either by sharing Firefox or sharing the whole screen) can insert in itself a subframe of a different origin, eg. an iframe of your mail account, and scrap data from the stream.

> @@ +548,5 @@
> > +                chromeDoc.getElementById("bundle_brand").getString("brandShortName");
> > +              string = bundle.getFormattedString("getUserMedia.shareFirefoxWarning.message",
> > +                                                 [brand], 1);
> > +            }
> > +            warning.firstChild.data = string + " ";
> 
> This is not a good idea for l10n.
> 
> Can we instead stick the <label> in the string, have the localization note
> indicate not to translate it, and then assign to innerHTML and manipulate
> the label? afterwards? It doesn't need an ID, you can just
> querySelector("label"), to keep the markup inside the l10n string simple. As
> a bonus, you won't need the separate learn more entity, and l10n gets more
> flexibility to manipulate the link's incorporation in the text, plus more
> context when translating.

Do you mean include <label>Learn More</label> in the localized string? Isn't that fragile?

flod, what's your preference here?

> @@ +157,5 @@
> >  
> > +#webRTC-preview:not([hidden="true"]) {
> > +  display: -moz-stack;
> > +  border-radius: 4px;
> > +  border: 1px solid #757575;
> 
> Should this just be GrayText?

I can try. I just picked the color on the mockup at https://mozilla.invisionapp.com/share/AF71R266U#/screens/152779760

> @@ +171,5 @@
> >    max-height: 200px;
> >  }
> >  
> > +#webRTC-previewWarning {
> > +  background: rgba(255, 217, 99, .8) url("chrome://browser/skin/warning-white.svg") no-repeat .5em .5em;
> 
> Why the semi-transparency? Feels like we should just use an opaque color, to
> avoid not fitting in with however the panel is themed by the OS theme.

The semi-transparency is to avoid hiding completely the bottom of the preview. This was definitely in the mockup.

> ::: browser/themes/shared/warning-white.svg
> @@ +1,4 @@
> > +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> > +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> > +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> > +<svg xmlns="http://www.w3.org/2000/svg" width="20" height="20" viewBox="0 0 60 60">
> 
> Why is this width+height=20, but with a 60x60 viewbox?
> 
> Can we normalize this? :-)

Do you know an automated way to do it? The actual size of the icon is really 16px, I would also like to get rid of the empty areas around it.

> @@ +1,5 @@
> > +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> > +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> > +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> > +<svg xmlns="http://www.w3.org/2000/svg" width="20" height="20" viewBox="0 0 60 60">
> > +  <path fill="#fff" stroke="#cdad51" stroke-width="2" d="M49.316,42.867L33.829,12.7c-0.879-1.715-2.274-2.7-3.828-2.7c-1.554,0-2.949,0.985-3.829,2.702 L10.685,42.864c-0.869,1.69-0.913,3.482-0.121,4.909C11.35,49.187,12.817,50,14.591,50h30.82c1.772,0,3.24-0.81,4.023-2.224 C50.227,46.349,50.185,44.56,49.316,42.867z M32.176,22.304l-0.48,14.304h-3.424L27.76,22.304H32.176z M30,44.896 c-1.44,0-2.592-1.184-2.592-2.592c0-1.44,1.152-2.592,2.592-2.592c1.472,0,2.592,1.152,2.592,2.592 C32.592,43.712,31.472,44.896,30,44.896z"/>
> 
> The choice of stroke here is... interesting. Why not black?

It's again just a color I picked from the mockup. I guess black with partial opacity would be good.

Comment 6

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #5)

> > @@ +548,5 @@
> > > +                chromeDoc.getElementById("bundle_brand").getString("brandShortName");
> > > +              string = bundle.getFormattedString("getUserMedia.shareFirefoxWarning.message",
> > > +                                                 [brand], 1);
> > > +            }
> > > +            warning.firstChild.data = string + " ";
> > 
> > This is not a good idea for l10n.
> > 
> > Can we instead stick the <label> in the string, have the localization note
> > indicate not to translate it, and then assign to innerHTML and manipulate
> > the label? afterwards? It doesn't need an ID, you can just
> > querySelector("label"), to keep the markup inside the l10n string simple. As
> > a bonus, you won't need the separate learn more entity, and l10n gets more
> > flexibility to manipulate the link's incorporation in the text, plus more
> > context when translating.
> 
> Do you mean include <label>Learn More</label> in the localized string? Isn't
> that fragile?
> 
> flod, what's your preference here?

I guess we can just re-use the solution we settled on in bug 1111153.

Comment 7

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #5)

> > ::: browser/themes/shared/warning-white.svg

> > > +<svg xmlns="http://www.w3.org/2000/svg" width="20" height="20" viewBox="0 0 60 60">
> > 
> > Why is this width+height=20, but with a 60x60 viewbox?
> > 
> > Can we normalize this? :-)
> 
> Do you know an automated way to do it? The actual size of the icon is really
> 16px, I would also like to get rid of the empty areas around it.

I figured it out with Inkscape.

Comment 8

[Florian Quèze [:florian]]

Attachment: Patch v3

Comment 9

[Francesco Lodolo [:flod]]

Approach from bug 111153 looks definitely better than hard-coding concatenations.

Comment 10

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8669d6d4b436de48a73bb913ce0d9e23e33b92d
Bug 1284878 - display a scary warning when the user is about to share Firefox or the whole screen, r=Gijs.

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/e8669d6d4b43

Comment 12

[Paul Silaghi, QA [:pauly]]

(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> Currently the warning will be displayed for whole screens; if you want to
> test the warning for Firefox windows, you'll need to apply locally the patch
> from bug 1311048.
Bug 1311048 looks fixed but I'm still not able to share the Firefox window/app in order to test the warning. Is this expected? Tested on 54.0a1 (2017-02-13) Win 7.

Comment 13

[Florian Quèze [:florian]]

(In reply to Paul Silaghi, QA [:pauly] from comment #12)
> (In reply to Florian Quèze [:florian] [:flo] from comment #3)
> > Currently the warning will be displayed for whole screens; if you want to
> > test the warning for Firefox windows, you'll need to apply locally the patch
> > from bug 1311048.
> Bug 1311048 looks fixed but I'm still not able to share the Firefox
> window/app in order to test the warning. Is this expected? Tested on 54.0a1
> (2017-02-13) Win 7.

I think it's expected that you can't share the Firefox application. You should be able to share Firefox windows though.

Comment 14

[Paul Silaghi, QA [:pauly]]

Confirmed the warning is displayed on screen/window sharing. Verified fixed FX 52b6 Win 7, Ubuntu 14.04, OS X 10.11.