Closed Bug 1315317 Opened 3 years ago Closed 3 years ago

Plugin notification can appear on the wrong tab

Categories

(Firefox :: Site Identity, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 52
Tracking Status
firefox49 --- affected
firefox-esr45 --- wontfix
firefox50 --- affected
firefox51 + fixed
firefox52 --- fixed

People

(Reporter: mossop, Assigned: mconley)

References

Details

(Keywords: csectype-spoof, sec-low, Whiteboard: [adv-main51-])

Attachments

(2 files, 2 obsolete files)

When I middle click on a link it opens as a background tab leaving the current tab focused. Sometimes when the new tab attempts to use flash the popup notification asking me to allow it appears on the current tab.

This is perhaps a means to spoof the user into thinking it is the current site asking to use flash rather than one in a different tab, though at least we include the hostname in the notification.
How recent is your build? This sounds like what should have been fixed in bug 1314125.
Flags: needinfo?(dtownsend)
(In reply to :Gijs Kruitbosch from comment #1)
> How recent is your build? This sounds like what should have been fixed in
> bug 1314125.

I'm on today's nightly.
Flags: needinfo?(dtownsend)
Mike, can you take a look, esp. given uplift intentions in bug 1314125 ?
Flags: needinfo?(mconley)
Hey Mossop,

Can you give me an example of a site with a link that you're suggesting? I'm not able to reproduce when middle-clicking a link to go to homestarrunner.com (for example).
Flags: needinfo?(mconley) → needinfo?(dtownsend)
It happens intermittently so that's fun. I've seen it twice when middle clicking on reddit to sites like theverge.com.
Flags: needinfo?(dtownsend)
I think it happens intermittently based on the ads you're served. I just reproduced, but it took me a few times.

STR:

1) Visit https://www.reddit.com/search?q=url%3Atheverge.com&restrict_sr=&sort=relevance&t=all
2) Start right-clicking on theverge.com links - NOT the Reddit links, but the ones below the main headlines, with the link icons next to them
3) Wait and see if a PopupNotification appears asking to activate Flash for The Verge while you're on a Reddit site.

:(

*sigh*, investigating.
Note that I'm also able to reproduce this from before bug 1314125 landed. I suspect this is related to bug 1308677.
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Attached patch [WIP] Regression test (obsolete) — Splinter Review
MozReview-Commit-ID: 2neRqZsn68
Comment on attachment 8807752 [details] [diff] [review]
Make sure PopupNotification reshows only occur when the tab is in the foreground

Guh, this appears to be a pretty old bug. :/
Attachment #8807752 - Flags: review?(gijskruitbosch+bugs)
Attached patch Regression testSplinter Review
MozReview-Commit-ID: 2neRqZsn68
Attachment #8807752 - Attachment is obsolete: true
Attachment #8807752 - Flags: review?(gijskruitbosch+bugs)
Attachment #8807797 - Flags: review?(gijskruitbosch+bugs)
Attachment #8807798 - Flags: review?(gijskruitbosch+bugs)
I think this has been a bug for a while, but wasn't something we noticed until bug 1292317 landed.
Comment on attachment 8807797 [details] [diff] [review]
Regression test

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

::: browser/base/content/test/popupNotifications/browser.ini
@@ +13,5 @@
>  [browser_popupNotification_4.js]
>  skip-if = (os == "linux" && (debug || asan))
>  [browser_popupNotification_checkbox.js]
>  skip-if = (os == "linux" && (debug || asan))
> +[browser_reshow_in_background.js]

If all the other ones are skipped on linux debug/asan, should we skip this there too?

::: browser/base/content/test/popupNotifications/browser_reshow_in_background.js
@@ +15,5 @@
> +  let seenEvents = [];
> +
> +  let options = {
> +    dismissed: false,
> +    eventCallback: (popupEvent) => {

nit: might as well use

eventCallback(popupEvent) {

instead of the arrow function.

@@ +24,5 @@
> +  let notification =
> +    PopupNotifications.show(tabC.linkedBrowser, "test-notification",
> +                            "", "plugins-notification-icon",
> +                            null, null, options);
> +  Assert.equal(seenEvents.length, 0, "Should have seen no events yet.");

I wonder if this would produce a more useful failure message if you used Assert.deepEqual(seenEvents, []) (here and below, of course). It would actually indicate which events were in the array, I think.
Attachment #8807797 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8807798 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #15)
> Comment on attachment 8807797 [details] [diff] [review]
> Regression test
> 
> Review of attachment 8807797 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/test/popupNotifications/browser.ini
> @@ +13,5 @@
> >  [browser_popupNotification_4.js]
> >  skip-if = (os == "linux" && (debug || asan))
> >  [browser_popupNotification_checkbox.js]
> >  skip-if = (os == "linux" && (debug || asan))
> > +[browser_reshow_in_background.js]
> 
> If all the other ones are skipped on linux debug/asan, should we skip this
> there too?

Looking at my trybuild, I think the answer is "yes". :)

> 
> :::
> browser/base/content/test/popupNotifications/browser_reshow_in_background.js
> @@ +15,5 @@
> > +  let seenEvents = [];
> > +
> > +  let options = {
> > +    dismissed: false,
> > +    eventCallback: (popupEvent) => {
> 
> nit: might as well use
> 
> eventCallback(popupEvent) {
> 
> instead of the arrow function.
> 
> @@ +24,5 @@
> > +  let notification =
> > +    PopupNotifications.show(tabC.linkedBrowser, "test-notification",
> > +                            "", "plugins-notification-icon",
> > +                            null, null, options);
> > +  Assert.equal(seenEvents.length, 0, "Should have seen no events yet.");
> 
> I wonder if this would produce a more useful failure message if you used
> Assert.deepEqual(seenEvents, []) (here and below, of course). It would
> actually indicate which events were in the array, I think.

Good idea, thanks!
https://hg.mozilla.org/mozilla-central/rev/7e61b840c162
https://hg.mozilla.org/mozilla-central/rev/23667d366a5d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Group: firefox-core-security → core-security-release
Dan, should this have a rating (and possibly sec approval?)
We could potentially uplift the fix here to 51 while it's still in aurora (before the merge next Monday morning)
Flags: needinfo?(dveditz)
We don't need to hide this minor spoofing issue. If it happens enough to give a bad experience in 51 then sure, uplift, but otherwise we don't need to on security grounds.
Group: core-security-release
Flags: needinfo?(dveditz)
Hi :mconley,
Do you think this is worth uplifting to Beta51?
Flags: needinfo?(mconley)
Comment on attachment 8807798 [details] [diff] [review]
Make sure PopupNotification reshows only occur when the tab is in the foreground

Approval Request Comment
[Feature/regressing bug #]:

This appears to have always been a bug.

[User impact if declined]:

Any code, add-on or otherwise, that uses reshow() for PopupNotifications can have the doorhanger appear for the wrong tab.

[Describe test coverage new/current, TreeHerder]:

Test included in a later patch in this series.

[Risks and why]: 

Low risk.

[String/UUID change made/needed]:

None.
Flags: needinfo?(mconley)
Attachment #8807798 - Flags: approval-mozilla-beta?
Attachment #8807798 - Flags: approval-mozilla-aurora?
Comment on attachment 8807797 [details] [diff] [review]
Regression test

Approval Request Comment

Regression test for this bug.
Attachment #8807797 - Flags: approval-mozilla-beta?
Attachment #8807797 - Flags: approval-mozilla-aurora?
Comment on attachment 8807798 [details] [diff] [review]
Make sure PopupNotification reshows only occur when the tab is in the foreground

Since merge is passed, 52 is aurora. No need to uplift to 52 aurora. Remove the aurora flag.
Fix an issue related to plugin notification. Beta51+. Should be in 51 beta 2.
Attachment #8807798 - Flags: approval-mozilla-beta?
Attachment #8807798 - Flags: approval-mozilla-beta+
Attachment #8807798 - Flags: approval-mozilla-aurora?
Attachment #8807798 - Flags: approval-mozilla-aurora-
Attachment #8807797 - Flags: approval-mozilla-beta?
Attachment #8807797 - Flags: approval-mozilla-beta+
Attachment #8807797 - Flags: approval-mozilla-aurora?
Attachment #8807797 - Flags: approval-mozilla-aurora-
Flags: needinfo?(mconley)
Whiteboard: [adv-main51-]
You need to log in before you can comment on or make changes to this bug.