Closed Bug 1335985 Opened 7 years ago Closed 7 years ago

Add delay before an action can be taken on a persistent notification when the window is reactivated

Categories

(Toolkit :: Add-ons Manager, defect, P1)

51 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla54
Tracking Status
firefox52 --- unaffected
firefox53 --- fixed
firefox54 --- verified

People

(Reporter: aswan, Assigned: florian)

References

Details

(Whiteboard: [fxprivacy])

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #1316460 +++

See https://bugzilla.mozilla.org/show_bug.cgi?id=1316460#c3
Copying the contents of the comment linked above since that bug is also private

Yes, you need the delay for permission dialogs, and it should be AFTER the dialog is displayed and BEFORE the "dangerous" button is active/enabled (install, grant permission, etc.). It's OK if the safe cancel/deny choice is active right away. See bug 162020. The delay tries to account for reaction time when the prompt suddenly appears under a lured click. The user can't start to react until they see the prompt so the delay does no good until that point.

In the past doorhangers went away if the window lost focus. Among other things this side-stepped the security problem of having the window-with-prompt obscured by another window that disappears right as the user clicks on something on top of the dangerous button. If the doorhanger sticks around then the button needs to be disabled when the window loses focus, and start the delay timer again when it regains focus.
I notice that we only appear to impose this delay for installing extensions and something related to downloads.  It doesn't apply to other permissions notifications (eg, gelocation, webrtc, etc).  Is it meant to?
I'm asking since it affects how we go about implementing it, ie whether it should be implemented generically in a central place or can just be limited to extension installation notifications.
Flags: needinfo?(dveditz)
Those other prompts definitely used to have a delay (bug 583175). Apparently it was ripped out without asking why it was there, but we still have the vestigial security.notification_enable_delay pref. (What other historical spoofing defenses are we missing?)

There's no security magic in making the button -look- disabled or calling attention to the fact that it is. Our first gen UI used OS widgets and the easiest thing was to set the disabled state and we got the corresponding look, but that was an artifact, not the goal. Ideally our delay wouldn't call attention to itself, and since we're drawing our own buttons it doesn't need to look any different when it's disabled. By the time the user sees and reacts to a legit prompt they won't even notice it was disabled at first. Even if they were expecting the prompt they are almost certainly clicking something elsewhere on the page (install button? start video chat?) and will take time to get their mouse over to the prompt. But if they are tricked into clicking on it by removing an overlay or popping up the prompt at the right time their click will run into a non-working button until they have time to react.

To that end I worry that security.notification_enable_delay of 500ms was too short and prefer the 1s of security.dialog_enable_delay. The original 2 second delay was ridiculous and the count-down timer drawing attention to itself even more so. There's no reason to have two different values; reaction time is reaction time. I'd like to stick with the longer 1000ms delay unless we have user studies (even anecdotal) that users are noticing or having to double-click.
Flags: needinfo?(dveditz)
So, to be clear, my interpretation of the above comment is that this should ideally be implemented in a generic way for popup notifications with the delay capability enabled for other permissions notifications as well, but we can handle other notifications in a separate bug.  Florian, do you have any time for the PopupNotifications side of this?  I can take a stab at it and have you review if you've got your hands full right now...
Flags: needinfo?(florian)
(In reply to Andrew Swan [:aswan] from comment #4)
> So, to be clear, my interpretation of the above comment is that this should
> ideally be implemented in a generic way for popup notifications with the
> delay capability enabled for other permissions notifications as well, but we
> can handle other notifications in a separate bug.

It may be easier to implement in a generic way anyway. One thing to be careful about is that we need to keep a way to disable this delay for popupnotification tests; except for the test testing this delay specifically.

> Florian, do you have any
> time for the PopupNotifications side of this?

Not before at least a few days.

> I can take a stab at it and
> have you review if you've got your hands full right now...

If we don't need any visible change, can we just record a timestamp when dispatching the 'shown' event, and ignore the primary action if the recorded timestamp isn't old enough?

Do we really need to keep this bug hidden?
Flags: needinfo?(florian)
Whiteboard: [fxprivacy][triage]
Assignee: nobody → florian
Whiteboard: [fxprivacy][triage] → [fxprivacy][triaged]
Popup notifications already have a 500ms delay before any action can be activated.

(In reply to Andrew Swan [:aswan] from comment #1)

> In the past doorhangers went away if the window lost focus.
> [...] If the doorhanger
> sticks around then the button needs to be disabled when the window loses
> focus, and start the delay timer again when it regains focus.

We need to handle this case. Actually, it worked by accident until I fixed bug 1325223.
Group: mozilla-employee-confidential
Priority: -- → P1
Summary: Add delay before activating button on permissions dialogs → Add delay before an action can be taken on a persistent notification when the window is reactivated
Whiteboard: [fxprivacy][triaged] → [fxprivacy]
Attached patch PatchSplinter Review
Attachment #8838645 - Flags: review?(past)
Comment on attachment 8838645 [details] [diff] [review]
Patch

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

Oh, I can see how _showPanel() used to make it work before bug 1325223. Good catch!
Attachment #8838645 - Flags: review?(past) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a62ce221207c4db6f542426a68cfa0dbb91d118c
Bug 1335985 - reset timeShown on window activation so that persistent notifications get the security delay again, r=past.
https://hg.mozilla.org/mozilla-central/rev/a62ce221207c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8838645 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: feature in bug 1004061, but the bug wasn't actually visible until I fixed bug 1325223.
[User impact if declined]: possible to confuse a user into accepting a permission prompt without reading it by briefly placing a popup window above the prompt. This came up during a security review.
[Is this code covered by automated tests?]: yes.
[Has the fix been verified in Nightly?]: Not by QA, but automated tests passed.
[Needs manual test from QE? If yes, steps to reproduce]: I'm not sure QA is required here, but steps to reproduce are:
1. From about:config, set security.notification_enable_delay to a higher value.
2. Do something that triggers a permission prompt. Eg. click the 'camera' button on http://permission.site/
3. Move focus to another window.
4. Move the focus back to the Firefox window.
5. Click the 'Allow' button in the permission prompt.

With this bug fixed, clicks before the delay in ms set in about:config will be ignored. Without the fix, a click will grant permission immediately.

[List of other uplifts needed for the feature/fix]: none.
[Is the change risky?]: no
[Why is the change risky/not risky?]: self contained fix (only 2 new lines of code) and covered by tests.
[String changes made/needed]: none
Attachment #8838645 - Flags: approval-mozilla-aurora?
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(brindusa.tot)
I tested on Mac OS X 10.10, Windows 10 with FF Nighlty 54.0a1(2017-02-22) and the issue is no longer reproducible. On Ubuntu 16.04 with the same FF version I'm still able to reproduce the issue, a click grants permission immediately. 

Florian did you notice that?
Flags: needinfo?(brindusa.tot) → needinfo?(florian)
Comment on attachment 8838645 [details] [diff] [review]
Patch

Fix verified for Mac, let's uplift it. 
Sounds like we still need to file a followup bug for other platforms.
Attachment #8838645 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to ovidiu boca[:Ovidiu] from comment #14)
> I tested on Mac OS X 10.10, Windows 10 with FF Nighlty 54.0a1(2017-02-22)
> and the issue is no longer reproducible. On Ubuntu 16.04 with the same FF
> version I'm still able to reproduce the issue, a click grants permission
> immediately. 
> 
> Florian did you notice that?

I just tested on Ubuntu 16.04 and there's indeed a behavior difference compared to Mac.

On Ubuntu, in the step "3. Move focus to another window." from comment 12, if the other window is another browser window, the delay will be applied correctly. If it's another window from another application (or even another non-browser window from Firefox, like the error console), the delay isn't applied again.

Luckily, the other window being a browser window is the only case that really matters for security (the threat we are trying to protect our users against is a website causing a prompt to appear, showing a popup window above the prompt, and then closing that window right before the user clicks a button in it, so that the click ends up on the 'allow' button of the prompt on the first window).

If this is also the behavior you are seeing (ie. if it works for you on ubuntu when using another browser window), I think we can mark this bug as verified.
Flags: needinfo?(florian)
Here is my scenario: 

1. From about:config, set security.notification_enable_delay to a higher value.
2. Open 2 browser windows.
3. In 1 window go to http://permission.site/  and click the 'microphone' button
4. Move focus to the second window.
5. Click on "Allow" in the first window.

Actual result: The "Allow" button is triggered on the first click.
Expected result: The delay is applied and only on the second click on "Allow" the button is triggered.

Tested on Ubuntu 16.04 with FF Nightly 54.0a1(2017-03-01)

Florian, please tell me if I understood correctly the expected result part?
Flags: needinfo?(florian)
(In reply to ovidiu boca[:Ovidiu] from comment #18)

> Florian, please tell me if I understood correctly the expected result part?

You understood correctly. I made a mistake in my previous Linux testing.

The problem we have on Linux is that the click on the action button is handled on mouse down (like all clicks on Linux), and we only receive the activate event (and so reset the delay) after this has already happened.

The behavior difference between browser windows and other windows that I observed before was actually not related to the kind of window, but it was just me clicking first somewhere inside the window before clicking the 'allow' button a couple ms later.

Could you please file a follow-up bug for this linux-specific issue? We may be able to just ignore clicks on the action buttons in windows that are inactive. But anyway, we'll need to track uplifting and verifying this follow-up fix, so a new bug is better.
Flags: needinfo?(florian)
Depends on: 1343571
Thanks, Florian, I filed a new bug as you suggested, bug 1343571.

Based on the fact that I tested on Mac OS X 10.10, Windows 10 with FF Nightly 54.0a1(2017-03-01) and the issue is no longer reproducible and the Linux-specific issue is handled on bug 1343571, I will mark this as Verified: Fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.