Popup window can cover fullscreen security UI
Categories
(Core :: DOM: Core & HTML, defect, P3)
Tracking
()
People
(Reporter: avi, Assigned: emz)
References
(Blocks 1 open bug)
Details
(Keywords: csectype-spoof, sec-moderate, Whiteboard: [parity Chrome and Edge][post-critsmash-triage][adv-main74+])
Attachments
(9 files, 2 obsolete files)
933 bytes,
text/html
|
Details | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
459 bytes,
text/plain
|
Details |
Comment 1•7 years ago
|
||
Updated•7 years ago
|
Comment 2•7 years ago
|
||
Updated•7 years ago
|
Comment 3•7 years ago
|
||
Comment 4•7 years ago
|
||
Comment 5•7 years ago
|
||
Comment 7•6 years ago
|
||
I am not sure if Xidorn has bandwidth to implement this since he's not full-time employee.
NI Ethan to put this into the security team's radar.
Comment 8•6 years ago
|
||
Johann, this is an issue that our team can handle?
Comment 10•6 years ago
|
||
Paul might be able to give it a shot, but to set expectations this is more of a backlog item for us. I'll add it to the list :)
Comment 11•6 years ago
•
|
||
(In reply to Johann Hofmann [:johannh] from comment #10)
Paul might be able to give it a shot, but to set expectations this is more of a backlog item for us. I'll add it to the list :)
Sounds good. Thanks!
Comment 12•6 years ago
|
||
Thanks! I indeed may not have bandwidth to work on this myself at the moment, but I'm always happy to answer any question and possibly review the code, so feel free to flag me if there's anything I can help!
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 13•6 years ago
•
|
||
IIUC, the problem we want to solve is that, the security UI should not be
blocked by anything when we show it. In that sense, I think we can solve it
in an easier way:
- when we about to show the security UI, we check that we do have the focus,
if not, kick the document out from fullscreen, and- when the notification is on screen, if we lose the focus, kick the
document out from fullscreen.
I've done some testing and also implemented this behaviour by listening for the blur
event. However this can interfere with normal usage. For example, opening a Video in fullscreen and then doing something on a secondary monitor would cause the browser to leave fullscreen.
I think the approach Chromium uses, to leave fullscreen on window.open()
and also on window.focus()
might be a good solution. Alternatively we could only behave this way while the fullscreen warning is shown.
Comment 14•6 years ago
|
||
I think the approach Chromium uses, to leave fullscreen on window.open() and also on window.focus() might be a good solution.
It might be a good solution but I also think there can be reasonable usecase triggering this. A imaginary case, for example, if user is watching a sample movie in fullscreen, and when it comes to part requiring payment, the website may pop up a payment window, and when the payment finishes, user can continue the movie in fullscreen.
Alternatively we could only behave this way while the fullscreen warning is shown.
That's exactly what I meant.
Assignee | ||
Comment 15•6 years ago
|
||
Assignee | ||
Comment 16•5 years ago
•
|
||
Apologies for the delayed update on this. I've looked into various options and also submitted a patch, but I'm not happy with it yet. Only leaving fullscreen when the fs warning is on screen requires a lot of refactoring, since we only know about the fs warning state in the frontend. The event has to be passed all the way through the focus manager to the frontend code. This includes extending the webidl interfaces to pass caller type (if the open/focus was triggered by the website or the system).
Going for Chromiums approach and exiting fullscreen on window.open
and window.focus
, would make for a much cleaner patch. We could handle everything in the backend. It would also help with other 'evil traps' where websites abuse popups in fullscreen to trick users into installing addons (Bug 1596189).
While there are some valid use cases for popups in fullscreen this wouldn't fully break workflows. Websites are aware of the fullscreen state and can avoid this behavior; given the market share of Chromium based browsers they already do. An example use case is Google Slides, where the user can open speaker notes in a popup. In Chromium, if the notes window is opened while in (DOM) fullscreen, the fullscreen is cancelled and the user has to enter it again. That's an extra click.
Assignee | ||
Comment 17•5 years ago
|
||
Assignee | ||
Comment 18•5 years ago
|
||
Depends on D55809
Assignee | ||
Comment 19•5 years ago
|
||
Depends on D55810
Assignee | ||
Comment 20•5 years ago
|
||
Depends on D55811
Assignee | ||
Comment 21•5 years ago
|
||
Depends on D55812
Updated•5 years ago
|
Comment 22•5 years ago
|
||
That seems reasonable to me. Thank you for all the work, Paul.
Assignee | ||
Comment 23•5 years ago
|
||
Depends on D55813
Updated•5 years ago
|
Assignee | ||
Comment 24•5 years ago
|
||
Depends on D56391
Comment 25•5 years ago
|
||
Backed out for build bustages failures in nsWindow.h:
https://hg.mozilla.org/integration/autoland/rev/d94afce0c18b72e8febfaa68a21fcb7790d1118c
Build failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=281441289&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=3d08c3cce53325d6623178e60d1ca5d38b4721c5
/builds/worker/workspace/build/src/widget/cocoa/nsChildView.h:310:16: error: 'SetFocus' marked 'override' but does not override any member functions
/builds/worker/workspace/build/src/widget/cocoa/nsChildView.h:282:7: error: abstract class is marked 'final' [-Werror,-Wabstract-final-class]
/builds/worker/workspace/build/src/widget/cocoa/nsChildView.h:310:16: error: 'nsChildView::SetFocus' hides overloaded virtual function [-Werror,-Woverloaded-virtual]
/builds/worker/workspace/build/src/widget/cocoa/nsCocoaWindow.h:235:16: error: 'SetFocus' marked 'override' but does not override any member functions
/builds/worker/workspace/build/src/widget/cocoa/nsCocoaWindow.h:207:7: error: abstract class is marked 'final' [-Werror,-Wabstract-final-class]
/builds/worker/workspace/build/src/widget/cocoa/nsCocoaWindow.h:235:16: error: 'nsCocoaWindow::SetFocus' hides overloaded virtual function [-Werror,-Woverloaded-virtual]
Eslint failure: https://treeherder.mozilla.org/logviewer.html#?job_id=281436170&repo=autoland
browser/base/content/test/fullscreen/open_and_focus_helper.html:32:37 | Prefer boolean length check (mozilla/prefer-boolean-length-check)
Had landed before as
https://hg.mozilla.org/integration/autoland/rev/a303b775a51b3df084ccd88d457ed2c0f8c7a4a3
https://hg.mozilla.org/integration/autoland/rev/8351a8b1d96ad8ff5b501d94b3b2c76c79db4293
https://hg.mozilla.org/integration/autoland/rev/c81f3d5b9bf33e648b9d7b4e427d1c9a8a10c38e
https://hg.mozilla.org/integration/autoland/rev/a8a4fa63f5b20f2dcc0b11e0496076e435c7e83c
https://hg.mozilla.org/integration/autoland/rev/62fc84c8ce9978f4df6d65ac6e8d8e4939b0bb65
https://hg.mozilla.org/integration/autoland/rev/49d03dd89b1759bdf28f50ad5330d5a2f9651803
https://hg.mozilla.org/integration/autoland/rev/3d08c3cce53325d6623178e60d1ca5d38b4721c5
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Comment 26•5 years ago
|
||
https://hg.mozilla.org/integration/autoland/rev/325eec16f032c255567b1081f941735677d9c5b2
https://hg.mozilla.org/integration/autoland/rev/f09a7b5da021be3b821e571fd8cb27db8d72dc63
https://hg.mozilla.org/integration/autoland/rev/5cb2b4727452b54b0ecf07c10178e01e1f8bc4a1
https://hg.mozilla.org/integration/autoland/rev/10fde12558b7cf7fb1f2849d8b2bdbf3b5b196c9
https://hg.mozilla.org/integration/autoland/rev/7594dab35ba4fcb5ce33e58cfae265aa76787f8d
https://hg.mozilla.org/integration/autoland/rev/03b7899094663bba35802764ac8f15d9f28f69da
https://hg.mozilla.org/integration/autoland/rev/a03b869d45a9680b63d45d96f217c2c476b20d09
https://hg.mozilla.org/mozilla-central/rev/325eec16f032
https://hg.mozilla.org/mozilla-central/rev/f09a7b5da021
https://hg.mozilla.org/mozilla-central/rev/5cb2b4727452
https://hg.mozilla.org/mozilla-central/rev/10fde12558b7
https://hg.mozilla.org/mozilla-central/rev/7594dab35ba4
https://hg.mozilla.org/mozilla-central/rev/03b789909466
https://hg.mozilla.org/mozilla-central/rev/a03b869d45a9
Updated•5 years ago
|
Comment 27•5 years ago
|
||
I see no reason to rush this fix to release. LMK if you feel strongly otherwise.
Comment 28•5 years ago
|
||
Dan, Tom suggests in bug 1596189 comment 17 that we should open up this bug and I think it makes sense. The patch is public and I think since the projected attack is mostly about spoofing or irritating the user, there's no risk of an "active exploit" appearing in the small window of time until the fix hits release. It made sense to keep it sensitive while the time to fix was unknown but now I don't think we're at risk.
If you agree, can you open this up, please?
Comment 29•5 years ago
|
||
I have attempted verification with the "fullscreenpopuptest.html" attachment using the following steps:
- Open browser.
- Load the attachment (login to Bugzilla required).
- Make sure that the browser window is on the primary screen (if more than one is being used).
- Click on the "Make pop-up" button.
- Click on the "Toggle fullscreen" button.
- Click on the "Focus pop-up" button.
Affected: The pop-up window is displayed and overlaps the full-screen related notification.
Fixed: The pop-up window DOES NOT overlap the full-screen related notification. However, the user is forced to leave full-screen mode.
These are my results on Nightly v74.0a1 from 2020-01-31:
- Windows 10: The pop-up window DOES NOT overlap the full-screen related notification. However, the user is forced to leave full-screen mode. Also, in this case, the pop-up does not stay in focus because the normal window gets focused after leaving full-screen mode.
- Ubuntu 18.04: The pop-up window DOES NOT overlap the full-screen related notification. However, the user is forced to leave full-screen mode. Also, in this case, the pop-up will stay in focus (as seen in the Chrome browser).
- Mac OS 10.14.6: The pop-up window DOES NOT overlap the full-screen related notification. However, the user is forced to leave full-screen mode. Also, in this case, the pop-up does not stay in focus because the normal window gets focused after leaving full-screen mode.
Should a new bug be logged for the secondary issue seen on Windows 10 and Mac OS?
Comment 30•5 years ago
|
||
Hsin-Tsai, I am requesting information from you in hope that you could answer some questions regarding the fix. Which are:
One issue that I observed is that the browser will instantly leave the full-screen mode when the pop-up is focused, but I am assuming that this is the compromise we are making in favor of not covering the notification about exiting full-screen. Can you confirm this is a valid approach?
A second issue is that the pop-up that gets focused will lose its focus because the "normal" browser window will be focused when leaving full-screen mode. This issue occurs only on Windows and Mac OS. Is this a valid issue? Should it be logged?
Assignee | ||
Comment 31•5 years ago
•
|
||
Thanks for verifying the fix!
(In reply to Bodea Daniel [:danibodea] from comment #30)
Hsin-Tsai, I am requesting information from you in hope that you could answer some questions regarding the fix. Which are:
One issue that I observed is that the browser will instantly leave the full-screen mode when the pop-up is focused, but I am assuming that this is the compromise we are making in favor of not covering the notification about exiting full-screen. Can you confirm this is a valid approach?
Yes, this is the approach we opted for. See discussion above.
A second issue is that the pop-up that gets focused will lose its focus because the "normal" browser window will be focused when leaving full-screen mode. This issue occurs only on Windows and Mac OS. Is this a valid issue? Should it be logged?
Yes, that's a bit inconsistent. The behavior on Linux is the correct one, we should honor the focus request, so the window should be in focus after leaving fullscreen. I've filed Bug 1613065.
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 33•5 years ago
|
||
Comment 34•5 years ago
|
||
Updated•5 years ago
|
Description
•