We currently have tabprompts for prompts triggered by websites (window.alert, window.confirm, window.prompt). These I call "content" prompts.
We can't use content prompts for system prompting, because they are easily spoof-able and look like the belong to the web content.

The proposal is to add a "tab" prompt type which can be opened through the prompt service. It would still belong to a tab, thus not steal focus, improve usability and help with a lot of DoS issues we have with window level prompts. To prevent spoofing it should slightly overlap with the parent chrome, like we currently do it for the web payment dialog.

Where possible, our current system window prompts should be switched over to tab prompts.

I've attached a screenshot of my prototype implementation.

Depends on D66446

Depends on D66447

Depends on D66448

Depends on D66449

This patch updates the prompt code in browser.js and the tabprompts.jsm module
to support the two new prompt types: tab and content

• Updated TabModalPromptBox to support both prompt types
• Updated TabModalPrompt styles for tab prompt type

Depends on D66446

This is amazing! It's great to have this attack vector finally fixed.

Backed out for browser-chrome failures e.g. browser_beforeunload_duplicate_dialogs.js

backout: https://hg.mozilla.org/integration/autoland/rev/12522eae03eb4edaedb7b69f9ccd123b7be3ac9b

push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=297167226&revision=751cca7566a856d7d03b8a7ec8fa265fe32be473&searchStr=browser-chrome

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=297167226&repo=autoland&lineNumber=2357

[task 2020-04-10T17:51:55.641Z] 17:51:55 INFO - Buffered messages finished
[task 2020-04-10T17:51:55.641Z] 17:51:55 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js | Test timed out -
[task 2020-04-10T17:51:55.641Z] 17:51:55 INFO - GECKO(1697) | MEMORY STAT | vsize 7657MB | residentFast 336MB | heapAllocated 123MB
[task 2020-04-10T17:51:55.641Z] 17:51:55 INFO - TEST-OK | browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js | took 90188ms
[task 2020-04-10T17:51:55.641Z] 17:51:55 INFO - GECKO(1697) | [Child 1702: Main Thread]: I/DocShellAndDOMWindowLeak ++DOCSHELL 0x107410800 == 1 [pid = 1702] [id = {74362761-c8a5-3445-88dc-2f00b659188c}]
[task 2020-04-10T17:51:55.641Z] 17:51:55 INFO - GECKO(1697) | [Child 1702: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 1 (0x10dba0ac0) [pid = 1702] [serial = 14] [outer = 0x0]
[task 2020-04-10T17:51:55.641Z] 17:51:55 INFO - GECKO(1697) | [Child 1702, Main Thread] WARNING: NS_ENSURE_TRUE(mPresShell) failed: file /builds/worker/checkouts/gecko/layout/base/nsPresContext.cpp, line 844
[task 2020-04-10T17:51:55.642Z] 17:51:55 INFO - GECKO(1697) | [Child 1702: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 2 (0x10db88c00) [pid = 1702] [serial = 15] [outer = 0x10dba0ac0]
[task 2020-04-10T17:51:55.642Z] 17:51:55 INFO - GECKO(1697) | [Child 1702: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 3 (0x10dc6a800) [pid = 1702] [serial = 16] [outer = 0x10dba0ac0]
[task 2020-04-10T17:51:55.642Z] 17:51:55 INFO - checking window state
[task 2020-04-10T17:51:55.642Z] 17:51:55 INFO - Not taking screenshot here: see the one that was previously logged
[task 2020-04-10T17:51:55.642Z] 17:51:55 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js | Found a browser window after previous test timed out -
[task 2020-04-10T17:51:55.642Z] 17:51:55 INFO - Not taking screenshot here: see the one that was previously logged
[task 2020-04-10T17:51:55.642Z] 17:51:55 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js | Found a after previous test timed out -
[task 2020-04-10T17:51:55.642Z] 17:51:55 INFO - GECKO(1697) | must wait for focus

Also remote failures: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=297170440&repo=autoland&lineNumber=10609
Devtools: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=297170215&repo=autoland&lineNumber=134048

Depends on D67134

There was an issue with the patch stack order. I've fixed it and will re-land this once the GeckoView patch is r+.

I don't fully understand what's being proposed here. However, I have some a11y concerns.

(In reply to Paul Zühlcke [:pbz] from comment #0)

The proposal is to add a "tab" prompt type which can be opened through the prompt service. It would still belong to a tab, thus not steal focus,

So these prompts wouldn't get focus at all? This will very probably be a problem for keyboard and/or screen reader users, since they depend somewhat on focus to interact with things. For screen reader users, the prompt may not get reported at all, which is even worse.

and help with a lot of DoS issues we have with window level prompts.

Can you clarify the issue here? As I understand this, this doesn't change "content prompts", so what's the attack vector?

I've attached a screenshot of my prototype implementation.

Note that I'm totally blind, so I can't see the screen shot to understand the UX here.

Thank you for chiming in on a11y side Jamie, we probably should have included you earlier, apologies for that. Note that for this bug we are reusing the content prompt layout and thus should hopefully not run into a lot of issues.

So these prompts wouldn't get focus at all? This will very probably be a problem for keyboard and/or screen reader users, since they depend somewhat on focus to interact with things. For screen reader users, the prompt may not get reported at all, which is even worse.

I'll let Paul speak to this but I think what is meant is that the prompt will get focus but not disallow the user to focus other pieces of UI in the browser.

Can you clarify the issue here? As I understand this, this doesn't change "content prompts", so what's the attack vector?

The prompts we're replacing can still be triggered by web content, even though they are not content prompts. The attack vector is simply spawning them repeatedly so that the user can't interact with the browser anymore. This has been and is still actively exploited in the wild.

Note that I'm totally blind, so I can't see the screen shot to understand the UX here.

For now these prompts use the same layout as content prompts (from alert etc.), but we are planning a visual redesign that would probably not impact a11y much. We should still keep you in the loop on that.

Thanks!

Thanks Johann! And thank you Jamie for bringing up the a11y concerns.

Accessibility wise the new prompt type shouldn't change anything. As Johann said, it behaves very similar to the content (website) prompts.
For the visual redesign, separating tab and content prompts more, maybe we can also make this separation clear for users with screen readers.

This e-mail to dev-platform explains the issue a bit more in depth: https://groups.google.com/forum/#!topic/mozilla.dev.platform/00OOAwDuZ8o

Thanks for your replies.

(In reply to Paul Zühlcke [:pbz] from comment #16)

Accessibility wise the new prompt type shouldn't change anything. As Johann said, it behaves very similar to the content (website) prompts.

Just to assuage my paranoia :), could you tell me an easy way to trigger one of these so I can spot check once this lands?

For the visual redesign, separating tab and content prompts more, maybe we can also make this separation clear for users with screen readers.

I believe content prompts don't currently have a dialog title for a11y. Maybe we could use that to distinguish browser prompts? It's hard to know whether the security afforded by this extra verbosity outweighs the user annoyance caused by extra verbosity, though.

https://hg.mozilla.org/mozilla-central/rev/fd8230b13071
https://hg.mozilla.org/mozilla-central/rev/b1d2ccd21a7e
https://hg.mozilla.org/mozilla-central/rev/3c85f19b11ac
https://hg.mozilla.org/mozilla-central/rev/d4df36221ce6
https://hg.mozilla.org/mozilla-central/rev/1517eb2e42e9
https://hg.mozilla.org/mozilla-central/rev/b8a3bd3cb0a8
https://hg.mozilla.org/mozilla-central/rev/4b428169709d