Make canvas permission a first-class permission

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: cfu, Assigned: cfu)

Tracking

(Blocks 1 bug)

unspecified
Firefox 59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: [fingerprinting])

Attachments

(7 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
We introduced a site permission prompt for canvas data extraction in bug 967895.  However, the permission is not listed in the site information panel so users have no chance to change it, as mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1382111#c22

    Another issue is that there is no easy way to figure out whether this
    particular site has permission to access the canvas. Normally, I would expect
    to see this information after clicking the "i" icon in the URL bar, but it
    only says that "you have not granted this site any special permissions", which
    is not true because I granted it the permanent permission to access the
    canvas. How can I deny this website from accessing my canvas in the future?
(Assignee)

Comment 2

2 years ago
The patch simply fixes the problem, and this image is how it looks.  Do we have to hide it when privacy.resistFingerprinting is false?
Flags: needinfo?(tom)
Flags: needinfo?(arthuredelstein)
Yes, we should not show this if p.rP is false.  

However, it may be the case that it doesn't appear unless it is set.  Please test with the following configurations:

1. Never having turned on p.RP, do we ever see mention of this permission?
2. If I turn on p.rP, I should see the permission and be able to set or clear it like anything else
3. If I turn off p.rP, does mention of the permission go away?
Flags: needinfo?(tom)
Flags: needinfo?(arthuredelstein)
Duplicate of this bug: 1415874
Component: Notifications and Alerts → Site Identity and Permission Panels
Product: Toolkit → Firefox
Assignee: nobody → cfu
Priority: -- → P1
(Assignee)

Updated

a year ago
Attachment #8924464 - Attachment is obsolete: true
(Assignee)

Comment 7

a year ago
Added another patch that hides canvas permission from the site information panel when privacy.resistFingerprinting is false.
So

(In reply to Tom Ritter [:tjr] from comment #3)
> Yes, we should not show this if p.rP is false.  
> 
> However, it may be the case that it doesn't appear unless it is set.  Please
> test with the following configurations:
> 
> 1. Never having turned on p.RP, do we ever see mention of this permission?

No, because the permission can't be decided.

> 2. If I turn on p.rP, I should see the permission and be able to set or
> clear it like anything else

Yes.

> 3. If I turn off p.rP, does mention of the permission go away?

Yes, it will not be listed in the site information panel.

Updated

a year ago
Blocks: 1419938
(Assignee)

Updated

a year ago
Attachment #8930845 - Flags: review?(jhofmann)
Attachment #8930846 - Flags: review?(jhofmann)

Comment 8

a year ago
mozreview-review
Comment on attachment 8930845 [details]
Bug 1413780 - Make canvas permission visible in site information panel.

https://reviewboard.mozilla.org/r/201944/#review207854

Please filter canvas permissions in the Page Info window as well (https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0/browser/base/content/pageinfo/permissions.js#20)

Thanks!
Attachment #8930845 - Flags: review?(jhofmann) → review-

Comment 9

a year ago
mozreview-review
Comment on attachment 8930846 [details]
Bug 1413780 - Hide canvas permission from site information panel when privacy.resistFingerprinting is false.

https://reviewboard.mozilla.org/r/201946/#review207864

Please also ignore canvas permissions here: https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0/browser/base/content/browser.js#7609

to avoid showing the little dot indicator on the (i) icon to indicate ALLOW permissions or the blocked icon for the BLOCK state.
Attachment #8930846 - Flags: review?(jhofmann) → review-
(In reply to Johann Hofmann [:johannh] from comment #9)
> Comment on attachment 8930846 [details]
> Bug 1413780 - Hide canvas permission from site information panel when
> privacy.resistFingerprinting is false.
> 
> https://reviewboard.mozilla.org/r/201946/#review207864
> 
> Please also ignore canvas permissions here:
> https://searchfox.org/mozilla-central/rev/
> 33c90c196bc405e628bc868a4f4ba29b992478c0/browser/base/content/browser.js#7609
> 
> to avoid showing the little dot indicator on the (i) icon to indicate ALLOW
> permissions or the blocked icon for the BLOCK state.

That reminds me: there's no permission icon for canvas in the identity popup and no "blocked" icon in the identity block. (Try blocking and allowing geolocation or notifications for a comparison).

For the ALLOW case (to display it in the identity popup), you can just add a CSS rule here (similar to the other permissions):

https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0/browser/themes/shared/notification-icons.inc.css#29

For the BLOCK case you'd need another canvas icon with a strikethrough (we used to embed the strikethrough as a separate path in the SVGs but I think it's flattened now).

Maybe Bryan can get us one quickly? :)

Thanks!
Flags: needinfo?(bbell)
(Assignee)

Comment 11

a year ago
(In reply to Johann Hofmann [:johannh] from comment #10)
> That reminds me: there's no permission icon for canvas in the identity popup
> and no "blocked" icon in the identity block. (Try blocking and allowing
> geolocation or notifications for a comparison).
> 
> For the ALLOW case (to display it in the identity popup), you can just add a
> CSS rule here (similar to the other permissions):
> 
> https://searchfox.org/mozilla-central/rev/
> 33c90c196bc405e628bc868a4f4ba29b992478c0/browser/themes/shared/notification-
> icons.inc.css#29
> 
> For the BLOCK case you'd need another canvas icon with a strikethrough (we
> used to embed the strikethrough as a separate path in the SVGs but I think
> it's flattened now).
> 
> Maybe Bryan can get us one quickly? :)
> 
> Thanks!

Besides this is our current icon
https://searchfox.org/mozilla-central/source/browser/themes/shared/notification-icons/canvas.svg

Thanks for help!
Whiteboard: [fingerprinting]
(Assignee)

Comment 12

a year ago
Our UX helper suggests that we should ask Eric Pang for the block icon, who provided the current icon for us in bug 1382111.


Hi Eric,

Could you please help with the block icon of canvas permission? Thanks!
Flags: needinfo?(bbell) → needinfo?(epang)
Posted file Icon - SVGs.zip
Hi Chung, I've attached svgs of the icons. Note that I've updated both states.  The palette didn't work well in the disabled state. ni me if anything else is needed!  Thanks!
Flags: needinfo?(epang) → needinfo?(cfu)
(Assignee)

Comment 14

a year ago
Thank you very much! I will update them.
Flags: needinfo?(cfu)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8930846 - Flags: review?(jhofmann)
Attachment #8934144 - Flags: review?(jhofmann)
Attachment #8930845 - Flags: review?(jhofmann)
(Assignee)

Updated

a year ago
Attachment #8930845 - Flags: review?(jhofmann)
(Assignee)

Updated

a year ago
Attachment #8930846 - Flags: review?(jhofmann)
(Assignee)

Comment 18

a year ago
I applied the newly designed icons with some minor modifications. The differences from the original ones (attachment 8933469 [details]) are:
1. change the dimensions to 16x16
2. set fill style as an attribute of <svg> instead of CSS.
3. simplify the code by removing transform (using SVGO)

And I also made the blocked icon shown on the left of the URL bar (like denying camera/microphone).

Please take a look again, thanks!

Comment 19

a year ago
mozreview-review
Comment on attachment 8930845 [details]
Bug 1413780 - Make canvas permission visible in site information panel.

https://reviewboard.mozilla.org/r/201944/#review212776

You might want to change this test to add the new permission, similarly to what persistent-storage did when it was still prefed-off (though I can we can remove that condition now) https://searchfox.org/mozilla-central/source/browser/modules/test/unit/test_SitePermissions.js#18,109
Attachment #8930845 - Flags: review?(jhofmann) → review+

Comment 20

a year ago
mozreview-review
Comment on attachment 8930845 [details]
Bug 1413780 - Make canvas permission visible in site information panel.

https://reviewboard.mozilla.org/r/201944/#review212778

::: browser/locales/en-US/chrome/browser/sitePermissions.properties:41
(Diff revision 2)
>  permission.popup.label = Open Pop-up Windows
>  permission.geo.label = Access Your Location
>  permission.shortcuts.label = Override Keyboard Shortcuts
>  permission.focus-tab-by-prompt.label = Switch to this Tab
>  permission.persistent-storage.label = Store Data in Persistent Storage
> +permission.canvas.label = Extract canvas data

Oh, also, looking at the other labels, I think this this should be:

Extract Canvas Data

Comment 21

a year ago
mozreview-review
Comment on attachment 8930846 [details]
Bug 1413780 - Hide canvas permission from site information panel when privacy.resistFingerprinting is false.

https://reviewboard.mozilla.org/r/201946/#review212712

::: browser/modules/SitePermissions.jsm:225
(Diff revision 2)
>      for (let permission of this.getAllByURI(browser.currentURI)) {
>        permissions[permission.id] = permission;
>      }
>  
> +    // Hide canvas permission when privacy.resistFingerprinting is false.
> +    if (!Services.prefs.getBoolPref("privacy.resistFingerprinting")) {

getAllForBrowser calls getAllByURI to get the permissions, so I'd say the better place to put this is where we already filter out the "install" permission, currently in line 178.
Attachment #8930846 - Flags: review?(jhofmann) → review-

Comment 22

a year ago
mozreview-review
Comment on attachment 8934144 [details]
Bug 1413780 - Update icon design and add blocked effect.

https://reviewboard.mozilla.org/r/205094/#review212710

Looks good but it would be great if we could get rid of the slash in the class name.

::: browser/themes/shared/notification-icons.inc.css:118
(Diff revision 1)
>    list-style-image: url(chrome://browser/skin/notification-icons/screen-blocked.svg);
>  }
>  
>  #canvas-notification-icon,
> -.popup-notification-icon[popupid="canvas-permissions-prompt"] {
> +.popup-notification-icon[popupid="canvas-permissions-prompt"],
> +.canvas\/extractData-icon {

Is there any reason you can't simplify this classname to .canvas-icon on the image element? Please do, otherwise.
Attachment #8934144 - Flags: review?(jhofmann) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8930846 - Flags: review?(jhofmann)
Attachment #8936942 - Flags: review?(jhofmann)
(Assignee)

Updated

a year ago
Attachment #8936943 - Flags: review?(jhofmann)
(Assignee)

Updated

a year ago
Attachment #8930846 - Flags: review?(jhofmann)
(Assignee)

Comment 28

a year ago
(In reply to Johann Hofmann [:johannh] from comment #19)
> Comment on attachment 8930845 [details]
> Bug 1413780 - Make canvas permission visible in site information panel.
> 
> https://reviewboard.mozilla.org/r/201944/#review212776
> 
> You might want to change this test to add the new permission, similarly to
> what persistent-storage did when it was still prefed-off (though I can we
> can remove that condition now)
> https://searchfox.org/mozilla-central/source/browser/modules/test/unit/
> test_SitePermissions.js#18,109

Please review Attachment #8936942 [details]

(In reply to Johann Hofmann [:johannh] from comment #22)
> Comment on attachment 8934144 [details]
> Bug 1413780 - Update icon design and add blocked effect.
> 
> https://reviewboard.mozilla.org/r/205094/#review212710
> 
> Looks good but it would be great if we could get rid of the slash in the
> class name.
> 
> ::: browser/themes/shared/notification-icons.inc.css:118
> (Diff revision 1)
> >    list-style-image: url(chrome://browser/skin/notification-icons/screen-blocked.svg);
> >  }
> >  
> >  #canvas-notification-icon,
> > -.popup-notification-icon[popupid="canvas-permissions-prompt"] {
> > +.popup-notification-icon[popupid="canvas-permissions-prompt"],
> > +.canvas\/extractData-icon {
> 
> Is there any reason you can't simplify this classname to .canvas-icon on the
> image element? Please do, otherwise.

It is related to the permission name, so I added a patch (Attachment #8936943 [details]) that changes the permission name from "canvas/extractData" to "canvas". Please take a look.

Thanks!

Comment 29

a year ago
mozreview-review
Comment on attachment 8930846 [details]
Bug 1413780 - Hide canvas permission from site information panel when privacy.resistFingerprinting is false.

https://reviewboard.mozilla.org/r/201946/#review214124

::: browser/modules/SitePermissions.jsm:183
(Diff revision 3)
>          if (permission.type == "install") {
>            continue;
>          }
> +
> +        // Hide canvas permission when privacy.resistFingerprinting is false.
> +        if ((permission.type == "canvas/extractData") &&

Nit: you don't need the parens around this.
Attachment #8930846 - Flags: review?(jhofmann) → review+

Comment 30

a year ago
mozreview-review
Comment on attachment 8936942 [details]
Bug 1413780 - Add a test for canvas permission to test_SitePermissions.js

https://reviewboard.mozilla.org/r/207672/#review214126

That test should work but what I was mostly concerned about is that test_SitePermissions.js will fail in testPermissionsListing() and testCanvasPermission() in any build that has privacy.resistFingerprinting prefed on. It might be worth adding a condition to these tests like persistent-storage does it.
Attachment #8936942 - Flags: review?(jhofmann)

Comment 31

a year ago
mozreview-review
Comment on attachment 8936943 [details]
Bug 1413780 - Change permission name from canvas/extractData to canvas.

https://reviewboard.mozilla.org/r/207674/#review214132

r+ because this should technically work, but think carefully about the consequences. Everyone who has already set any canvas/extractData permission will lose those permissions and get prompted again. Since this is a prefed-off feature I wouldn't assume that this impacts a lot of users, I just wanted to make sure we're aware of it.

I should have really required a different permission name in my original review, so this is probably a good change.
Attachment #8936943 - Flags: review?(jhofmann) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 37

a year ago
(In reply to Johann Hofmann [:johannh] from comment #30)
> Comment on attachment 8936942 [details]
> Bug 1413780 - Add a test for canvas permission to test_SitePermissions.js
> 
> https://reviewboard.mozilla.org/r/207672/#review214126
> 
> That test should work but what I was mostly concerned about is that
> test_SitePermissions.js will fail in testPermissionsListing() and
> testCanvasPermission() in any build that has privacy.resistFingerprinting
> prefed on. It might be worth adding a condition to these tests like
> persistent-storage does it.

Sorry I misunderstood it. I fixed testPermissionsListing(), and I guess the new testCanvasPermission() doesn't need update because privacy.resistFingerprinting is explicitly set in it.

(In reply to Johann Hofmann [:johannh] from comment #31)
> Comment on attachment 8936943 [details]
> Bug 1413780 - Change permission name from canvas/extractData to canvas.
> 
> https://reviewboard.mozilla.org/r/207674/#review214132
> 
> r+ because this should technically work, but think carefully about the
> consequences. Everyone who has already set any canvas/extractData permission
> will lose those permissions and get prompted again. Since this is a
> prefed-off feature I wouldn't assume that this impacts a lot of users, I
> just wanted to make sure we're aware of it.
> 
> I should have really required a different permission name in my original
> review, so this is probably a good change.

The name "canvas/extractData" looked good to me because it was more unique, but I didn't know it related to the classname. Sorry for that.

Comment 38

a year ago
mozreview-review
Comment on attachment 8936942 [details]
Bug 1413780 - Add a test for canvas permission to test_SitePermissions.js

https://reviewboard.mozilla.org/r/207672/#review214854

Ah, sorry, that was a typo on my side. I meant testExactHostMatch() instead of testCanvasPermission(). I've marked the function below. Otherwise this seems good to me...

::: browser/modules/test/unit/test_SitePermissions.js:109
(Diff revision 2)
>  add_task(async function testExactHostMatch() {
>    let uri = Services.io.newURI("https://example.com");
>    let subUri = Services.io.newURI("https://test1.example.com");
>  
>    let exactHostMatched = ["desktop-notification", "focus-tab-by-prompt", "camera",
>                            "microphone", "screen", "geo"];

Please add the same condition here again :)
Attachment #8936942 - Flags: review?(jhofmann) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 44

a year ago
I added it to testExactHostMatch() and made it look like the persistent-storage one, using a global const RESIST_FINGERPRINTING = Services.prefs.getBoolPref("privacy.resistFingerprinting").

I really appreciate your help!
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 45

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ce3ba591b61a
Make canvas permission visible in site information panel. r=johannh
https://hg.mozilla.org/integration/autoland/rev/0731f1b72c49
Hide canvas permission from site information panel when privacy.resistFingerprinting is false. r=johannh
https://hg.mozilla.org/integration/autoland/rev/577004586b2d
Update icon design and add blocked effect. r=johannh
https://hg.mozilla.org/integration/autoland/rev/d09dd833d6f0
Add a test for canvas permission to test_SitePermissions.js r=johannh
https://hg.mozilla.org/integration/autoland/rev/acc307373696
Change permission name from canvas/extractData to canvas. r=johannh
Keywords: checkin-needed

Comment 47

a year ago
I don't see this on Firefox for Android ?

Comment 48

a year ago
running latest nightly
Tom, is there a plan to bring the permission prompt for canvas to Android? If so, we should probably file a bug on it.
Flags: needinfo?(tom)
(In reply to Johann Hofmann [:johannh] from comment #49)
> Tom, is there a plan to bring the permission prompt for canvas to Android?
> If so, we should probably file a bug on it.

There is no plan, but we should do that, yes. Bug 1432506
Flags: needinfo?(tom)
You need to log in before you can comment on or make changes to this bug.