Closed Bug 1376865 Opened 7 years ago Closed 6 years ago

Do not display Canvas Prompt unless triggered by user input

Categories

(Core :: Graphics: Canvas2D, enhancement, P2)

Product:
Core
Component:
Graphics: Canvas2D
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox59 --- verified
firefox60 --- verified

People

(Reporter: tjr, Assigned: tjr)

References

(Blocks 3 open bugs)

Details

(Keywords: feature, Whiteboard: [tor][fingerprinting][gfx-noted][fp:m4])

Attachments

(2 files)

Bug 1376865 Automatically decline the canvas permission if it is not in response to user input
59 bytes, text/x-review-board-request
baku
: review+
lizzard
: approval-mozilla-beta+
Details
Bug 1376865 Add a test for Canvas Image Prompts triggered by user inputs.
59 bytes, text/x-review-board-request
baku
: review+
Details 
The Canvas Prompt in Bug 967895 occurs when image extraction happens. We could reduce prompt fatigue, while neutering most (all?) tracking mechanisms, and _still_ (hopefully) enable pages to still function by not showing the prompt if extraction is attempted in the first... second(?) of page load. 

We should be able to do this because if extraction occurs that quickly, we can assume it was not triggered by user interaction.

If later on, the page attempts extraction, we show the prompt. It's likely the user tried to do something, it doesn't work, the prompt shows, they allow it, and reattempt. Hopefully it works. If not they would need to reload the page (and we would need to grant extraction without prompt after the reload.)
Whiteboard: [tor] → [tor][fingerprinting]
It would be better if we could actually detect user interaction (keyboard event or mouse click). Is there a reliable way to do that kind of detection?

The other issue is that if a page fails to work correctly because access to canvas data is blocked, the user may not know why. I don't remember if it is possible to add the prompt without showing it, but that would help (the user could then notice the canvas icon and click to open the prompt).
Priority: -- → P2
Whiteboard: [tor][fingerprinting] → [tor][fingerprinting][gfx-noted]
(In reply to Mark Smith (:mcs) from comment #1)
> It would be better if we could actually detect user interaction (keyboard
> event or mouse click). Is there a reliable way to do that kind of detection?

I like this idea. Element.requestFullScreen also requires a user interaction to succeed. Here's the code that detects it:
https://dxr.mozilla.org/mozilla-central/rev/211d4dd61025c0a40caea7a54c9066e051bdde8c/dom/base/nsContentUtils.cpp#7265
Whiteboard: [tor][fingerprinting][gfx-noted] → [tor][fingerprinting][gfx-noted][fp-backlog]
Whiteboard: [tor][fingerprinting][gfx-noted][fp-backlog] → [tor][fingerprinting][gfx-noted][fp:m4]
P2 based on this one being SO ANNOYING
Priority: P3 → P2
So I think the way to go is definitely to use IsHandlingUserInput().  What I'm wondering is if there is a way to unblock a page if we do this. We should enable the user to grant the permission (and have that grant persist on a reload) so if this breaks something there is at least _some_ way to recover...  I guess the alternative is to put in a pref that would disable the behavior, but I like that idea a lot less.
(In reply to Tom Ritter [:tjr] from comment #4)
> So I think the way to go is definitely to use IsHandlingUserInput().  What
> I'm wondering is if there is a way to unblock a page if we do this. We
> should enable the user to grant the permission (and have that grant persist
> on a reload) so if this breaks something there is at least _some_ way to
> recover...  I guess the alternative is to put in a pref that would disable
> the behavior, but I like that idea a lot less.

If we decide to use IsHandlingUserInput(), then I would suggest we pause the script while we show a prompt and synchronously wait for the user to decide to permit or reject before resuming. Thus the blocking prompt would behave like `window.confirm()`.

I think a blocking prompt is reasonable in this case, because the user has just signaled an intention (via a click, say) and the script is immediately asking for permission (another click) to perform an operation it claims is required to fulfill that intention. So (1) the user is already in "clicking" mode and (2) the prompt appears immediately after the click, so provides better context to the user.
My guess is that strictly requiring IsHandlingUserInput() for showing the doorhanger is virtually equivalent to denying everywhere. If you look at the (Nightly) telemetry for some other permission prompts (where in my idea it's actually much more common for them to be triggered through buttons than for canvas) you can see that handling user input is still pretty rare: https://mzl.la/2E1GRe1

You _could_ do something that I've been considering for notification permissions for some time:

Show the doorhanger as hidden (with only the identity block icon visible), unless it was triggered through IsHandlingUserInput(). This would allow users to make their choice in a less intrusive way by clicking the identity block icon to optionally show the doorhanger (and it would be a nice test run for mainstream permissions).

That may be incompatible with making a permission doorhanger block script execution on a page, but that is definitely a non-trivial task and it sounds pretty invasive and non-intuitive for the user.
Was able to make some headway on this.  I tested it with the following simple test:

> <script	type="text/javascript">
>     let width = 64, height = 64;
>     let canvas = document.createElement("canvas");
>     canvas.setAttribute("id", "can");
>     canvas.setAttribute("width", width);
>     canvas.setAttribute("height", height);
>     document.body.appendChild(canvas);
> 
>     let context = canvas.getContext("2d");
>     context.fillStyle = "cyan";
>     context.fillRect(0, 0, width, height);
>     var data = canvas.toDataURL();
>     console.log("Canvas Output (Automatic): " + data);
> 
>     function youClicked() {
>        	let canvas = document.getElementById("can");
>         var data = canvas.toDataURL();
>        	console.log("Canvas Output (Click): " + data);
>     }
> </script>
> 
> <input type="button" onclick="youClicked()">Extract</input>

I'm sending it through a try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dea60ce6438f4693f8f806b47527bf34512a5244

I have not updated the canvas test, so I expect it to fail. Getting the test working will probably be the hardest part of this bug =/

Arthur: I did not attempt the Script Pause part, I actually couldn't find your notes on how that would work.
I added a test but... it's not good.

1) It doesn't work. 

I can't get 'await BrowserTestUtils.synthesizeMouse("#clickme", 5, 5, {type: "mousedown"}, browser);' to actually trigger the event and cause the prompt. I tried {type: "mousedown"} (with the onmousedown event handler specified) and {} (with the onclick event handler specified).  Both of those work fine and the test succeeds when I manually mousedown or click on the button in a non-headles test run - but neither of them work without me manually performing the action.

2) The test doesn't test the most important thing: that the prompt is _not_ shown when a canvas image extraction request is made without user input.  The only way I could think of writing a test for "A prompt is not shown" is by awaiting a shown prompt, and then someone telling the test harness "This test is supposed to time out. A time out is a success, and anything else is a failure."  Besides that being ugly, I don't know how to write that.
(In reply to Tom Ritter [:tjr] from comment #8)

> Arthur: I did not attempt the Script Pause part, I actually couldn't find
> your notes on how that would work.

My naive idea is that a modal prompt or doorhanger could appear asking the user to allow or deny image extraction. The main content thread would block while waiting for the user to respond.

The code at https://dxr.mozilla.org/mozilla-central/rev/1fd674ed91f96c8f0bbb36c827d74108dee0146f/dom/base/nsGlobalWindowOuter.cpp#4634
seems to show how to show a prompt synchronously. I'm not sure if this synchronous mechanism could be adapted to a doorhanger. It might not be desirable either, as doorhangers are usually non-modal.

Perhaps the best option could be to simply call window.confirm(), or rather the C++ equivalent and show the modal prompt that way.

Note we won't need to block for canvas.toBlob() because it returns the extracted image via a callback. So the callback could just be delayed until the user clicks allow or deny.
(In reply to Tom Ritter [:tjr] from comment #10)

> 2) The test doesn't test the most important thing: that the prompt is _not_
> shown when a canvas image extraction request is made without user input. 
> The only way I could think of writing a test for "A prompt is not shown" is
> by awaiting a shown prompt, and then someone telling the test harness "This
> test is supposed to time out. A time out is a success, and anything else is
> a failure."  Besides that being ugly, I don't know how to write that.

In the past I remember a test where we needed to suppress an event of type X. The answer we came up with was something like:
 let promiseEventX = ListenForOneEventX()
 Attempt to trigger ForbiddenEventX
 Trigger DummyEventX
 let eventFound = await promiseEventX
 if (eventFound === ForbiddenEventX) ok(false, "Failed to suppress ForbiddenEventX")
 if (eventFound === DummyEventX) ok(true, "Correctly suppressed ForbiddenEventX")
What's the technical advantage of blocking the page? The outlined flow for that sounds like a pretty inconsistent UX, so I wonder what reasons should make this absolutely necessary.
(In reply to Johann Hofmann [:johannh] from comment #15)
> What's the technical advantage of blocking the page? The outlined flow for
> that sounds like a pretty inconsistent UX, so I wonder what reasons should
> make this absolutely necessary.

The canvas.toDataURL() API is synchronous (It returns the extracted image data directly without use promises or callbacks.) Right now, the canvas image extraction permission prompt in Firefox and Tor Browser does not block. That means that canvas.toDataURL() automatically fails the first time it is called, before the user has a chance to click Allow. Only once toDataURL is called again, typically when the user refreshes the page, can the Allow permission setting have any effect.

So by changing this behavior to a blocking permission prompt, we can make the user's decision apply immediately to the first time toDataURL is called. The user will then have a chance of keeping their web app's workflow going without any breakage or need to refresh.

You're right that the UX is not consistent with other kinds of permission-gated Web APIs that are promise-based. But I think the present asynchronous UX is even worse, because image extraction initially fails, even though the user has chosen Allow.

A blocking prompt is consistent with window.confirm() and window.alert(), so as I mentioned in comment 13, it might be easier for users to understand if we use window.confirm for this purpose as well, rather than the traditional permissions doorhanger.
Comment on attachment 8951891 [details]
Bug 1376865 Automatically decline the canvas permission if it is not in response to user input

https://reviewboard.mozilla.org/r/221164/#review227346

I would use DOMPrefs everywhere instead of having a nsContentUtils static method.
Plus, I don't see why we need the changes in nsRFPService. You don't do anything when the notification is received, and we already have an atomic boolean in DOMPrefs.

::: dom/base/nsContentUtils.h:286
(Diff revision 1)
>    // Check whether we should avoid leaking distinguishing information to JS/CSS.
>    // This function can be called both in the main thread and worker threads.
>    static bool ShouldResistFingerprinting();
>    static bool ShouldResistFingerprinting(nsIDocShell* aDocShell);
>    static bool ShouldResistFingerprinting(nsIDocument* aDoc);
> +  static bool EnableAutoDeclineCanvasPrompts();

I would use DOMPrefs directly. This is not needed.

::: dom/base/nsContentUtils.cpp:2404
(Diff revision 1)
>    return !isChrome && ShouldResistFingerprinting();
>  }
>  
>  /* static */
> +bool
> +nsContentUtils::EnableAutoDeclineCanvasPrompts()

here as well.

::: dom/canvas/CanvasUtils.cpp:148
(Diff revision 1)
>      }
>  
>      // At this point, permission is unknown (nsIPermissionManager::UNKNOWN_ACTION).
> +
> +    // Check if the request is in response to user input
> +    if (nsContentUtils::EnableAutoDeclineCanvasPrompts() && !EventStateManager::IsHandlingUserInput()) {

if (DOMPrefs::EnableAutoDeclineCanvasPrompts() && ...

::: toolkit/components/resistfingerprinting/nsRFPService.cpp:127
(Diff revision 1)
> + */
> +/* static */
> +bool
> +nsRFPService::EnableAutoDeclineCanvasPrompts()
> +{
> +  return sAutoDeclineCanvasPermissionPrompts;

Also this one can be done via DOMPrefs.

::: toolkit/components/resistfingerprinting/nsRFPService.cpp:351
(Diff revision 1)
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    rv = prefs->AddObserver(RFP_TIMER_VALUE_PREF, this, false);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  rv = prefs->AddObserver(RFP_CANVAS_PREF, this, false);

You are not checking this in ::Observe(). Why having this observer?
Attachment #8951891 - Flags: review?(amarchesini) → review-
Comment on attachment 8951943 [details]
Bug 1376865 Add a test for Canvas Image Prompts triggered by user inputs.

https://reviewboard.mozilla.org/r/221214/#review227348
Attachment #8951943 - Flags: review?(amarchesini) → review+
Comment on attachment 8951943 [details]
Bug 1376865 Add a test for Canvas Image Prompts triggered by user inputs.

https://reviewboard.mozilla.org/r/221214/#review228532

::: browser/base/content/test/permissions/browser_canvas_fingerprinting_resistance.js:155
(Diff revision 2)
> +
> +async function withNewTabInput(grantPermission, browser) {
> +  await ContentTask.spawn(browser, null, initTab);
> +  await enableResistFingerprinting(true);
> +  let popupShown = promisePopupShown();
> +  await BrowserTestUtils.synthesizeMouse("#clickme", 5, 5, {type: "mousedown"}, browser);

You need to wrap this with E10SUtils.wrapHandlingUserinput. You can also use synthesizeMouseAtCenter
(In reply to Johann Hofmann [:johannh] from comment #19)
> You need to wrap this with E10SUtils.wrapHandlingUserinput. You can also use
> synthesizeMouseAtCenter

Thank you! Okay I fixed that up, the test works, and resolved the stuff from the first path. I think this is good to go now!
Summary: Do not display Canvas Prompt unless triggered after page load → Do not display Canvas Prompt unless triggered by user input
Assignee: nobody → tom
Comment on attachment 8951943 [details]
Bug 1376865 Add a test for Canvas Image Prompts triggered by user inputs.

https://reviewboard.mozilla.org/r/221214/#review228658


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/base/content/test/permissions/browser_canvas_fingerprinting_resistance.js:157
(Diff revision 3)
> +  await ContentTask.spawn(browser, null, initTab);
> +  await enableResistFingerprinting(true);
> +  let popupShown = promisePopupShown();
> +  await ContentTask.spawn(browser, null, function(host) {
> +    E10SUtils.wrapHandlingUserInput(content, true, function() {
> +      var button = content.document.getElementById("clickme")

Error: Missing semicolon. [eslint: semi]
Comment on attachment 8951891 [details]
Bug 1376865 Automatically decline the canvas permission if it is not in response to user input

Approval Request Comment
[Feature/Bug causing the regression]: The (non-default) Resist Fingerprinting mode throws Canvas prompts all over the web. It's annoying. This will resolve 99% of that.

[User impact if declined]: The users who have enabled Resist Fingerprinting will have an annoying experience for another release cycle.

[Is this code covered by automated tests?]: Yes

[Has the fix been verified in Nightly?]: No, I'd be happy to have it bake there for a little bit, but I'd like to see this in 59.

[Needs manual test from QE? If yes, steps to reproduce]: Yes, actually that would be great.  


1) Go to about:config and turn on privacy.resistFingerprinting
2) Go to https://ritter.vg/misc/transient/canvas.html and confirm that no prompt appears when you open the page.
3) Click the button. Confirm that the prompt appears.


[List of other uplifts needed for the feature/fix]: None

[Is the change risky?]: No. 

[Why is the change risky/not risky?]: For default users who haven't flipped this hidden pref, we don't do anything except AddAtomicBoolVarCache.  

For users who have flipped the pref, it's not a large amount of new logic (~5 lines of code)

[String changes made/needed]: None
Attachment #8951891 - Flags: approval-mozilla-beta?
Comment on attachment 8951891 [details]
Bug 1376865 Automatically decline the canvas permission if it is not in response to user input

https://reviewboard.mozilla.org/r/221164/#review229020

::: toolkit/components/resistfingerprinting/nsRFPService.h:252
(Diff revision 2)
>                                      const WidgetKeyboardEvent* aKeyboardEvent,
>                                      SpoofingKeyboardCode& aOut);
>  
>    static Atomic<bool, Relaxed> sPrivacyResistFingerprinting;
>    static Atomic<bool, Relaxed> sPrivacyTimerPrecisionReduction;
> +  static Atomic<bool, Relaxed> sAutoDeclineCanvasPermissionPrompts;

You don't need this.

::: toolkit/components/resistfingerprinting/nsRFPService.cpp:340
(Diff revision 2)
>  
>    Preferences::AddAtomicBoolVarCache(&sPrivacyTimerPrecisionReduction,
>                                       RFP_TIMER_PREF,
>                                       true);
> -
> +  Preferences::AddAtomicBoolVarCache(&sAutoDeclineCanvasPermissionPrompts,
> +                                     RFP_CANVAS_PREF,

You can remove this and any other change in nsRFPService.cpp. Right?
Attachment #8951891 - Flags: review?(amarchesini) → review-
(In reply to Andrea Marchesini [:baku] from comment #26)
> You can remove this and any other change in nsRFPService.cpp. Right?

Yes, that's right; fixed.
Comment on attachment 8951891 [details]
Bug 1376865 Automatically decline the canvas permission if it is not in response to user input

https://reviewboard.mozilla.org/r/221164/#review229186
Attachment #8951891 - Flags: review?(amarchesini) → review+
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/ba0944d01aa8
Automatically decline the canvas permission if it is not in response to user input r=baku
https://hg.mozilla.org/integration/autoland/rev/03556f5a85d7
Add a test for Canvas Image Prompts triggered by user inputs. r=baku
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ba0944d01aa8
https://hg.mozilla.org/mozilla-central/rev/03556f5a85d7
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Tom or Arthur, can I just confirm...

- Canvas extraction prompts are now reduced to those that are "caused by user interaction".
- So all **the other** canvas extractions (non user interaction caused ones) are what? Automatically blocked, right?
- And **all** canvas extraction attempts can still be overridden by the canvas site permissions?
(In reply to Simon Mainey from comment #33)
> Tom or Arthur, can I just confirm...
> 
> - Canvas extraction prompts are now reduced to those that are "caused by
> user interaction".

Yes

> - So all **the other** canvas extractions (non user interaction caused ones)
> are what? Automatically blocked, right?

Well, we don't _block_ canvas image extraction ever, but if you haven't granted the permission the output of the extraction is a 10px by 10px white square. (At least I think it's 10px by 10px....)

> - And **all** canvas extraction attempts can still be overridden by the
> canvas site permissions?

Yes, if you grant permission, all canvas extractions (those initiated or not initiated by user input) give the real data.
Just wanted to confirm the default, i.e for non-prompts. So good, it's the white square :) Thanks
Andrei, here is another issue where we'd love to have last minute verification to feel confident about getting this into 59. Can your team help out?
status-firefox59: --- → affected
Flags: qe-verify+
Flags: needinfo?(andrei.vaida)
Comment on attachment 8951891 [details]
Bug 1376865 Automatically decline the canvas permission if it is not in response to user input

Given that this will only affect users who have flipped the (hidden) pref I'm ok with uplifting this to beta 59. Should land for the beta 14 build.
Attachment #8951891 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I have reproduced this issue using Firefox  56.0a1 (2017.06.28) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 59.0b14, 60.0a1 latest nightly on Win 8.1 x64, Mac OS X 10.11.5 and Ubuntu 14.04 x64.
Status: RESOLVED → VERIFIED
status-firefox59: fixed → verified
status-firefox60: fixed → verified
Flags: qe-verify+
Flags: needinfo?(andrei.vaida)
Blocks: 1631673
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: