Closed Bug 1615588 Opened 8 months ago Closed 6 months ago

Extend nsIPromptService to support tab modal system prompts

Categories

(Firefox :: Security, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 77
Tracking Status
firefox77 --- fixed

People

(Reporter: pbz, Assigned: pbz)

References

(Blocks 3 open bugs)

Details

Attachments

(9 files)

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.

Blocks: 1594214
Priority: -- → P1
Blocks: 616849

Depends on D66449

Blocks: 1621737
Blocks: 1622817
Blocks: 1622836
Attachment #9132639 - Attachment description: Bug 1615588 - Extended nsIPromptService to support tab modal prompts. r=johannh! → Bug 1615588 - Extended nsIPromptService to support tab modal prompts. r=johannh!,MattN!

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

Pushed by pzuhlcke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/264e642042b1
Extended nsIPromptService to support tab modal prompts. r=johannh,MattN
https://hg.mozilla.org/integration/autoland/rev/32bb87f48b13
TabModalPrompt(Box): Added tab level system prompts. r=MattN
https://hg.mozilla.org/integration/autoland/rev/7839b95ef76c
SearchOneOffs: Use prompt service instead of nsIPrompt. r=johannh
https://hg.mozilla.org/integration/autoland/rev/474aca043834
pageActions: Use prompt service instead of nsIPrompt. r=johannh
https://hg.mozilla.org/integration/autoland/rev/751cca7566a8
Updated prompt tests. r=marionette-reviewers,johannh,whimboo

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

Flags: needinfo?(pbz)
Blocks: 1629808

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

Flags: needinfo?(pbz)

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

Attachment #9140025 - Attachment description: Bug 1615588 - Extended GeckoView prompt implementation to support prompting by BrowsingContext. r=droeh! → Bug 1615588 - Extended GeckoView prompt implementation to support prompting by BrowsingContext. r=snorp
Pushed by pzuhlcke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fd8230b13071
Extended nsIPromptService to support tab modal prompts. r=johannh,MattN,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/b1d2ccd21a7e
TabModalPrompt(Box): Added tab level system prompts. r=MattN
https://hg.mozilla.org/integration/autoland/rev/3c85f19b11ac
Extended GeckoView prompt implementation to support prompting by BrowsingContext. r=snorp
https://hg.mozilla.org/integration/autoland/rev/d4df36221ce6
nsDocumentViewer: Use prompt service instead of nsIPrompt. r=johannh,jfkthame
https://hg.mozilla.org/integration/autoland/rev/1517eb2e42e9
SearchOneOffs: Use prompt service instead of nsIPrompt. r=johannh
https://hg.mozilla.org/integration/autoland/rev/b8a3bd3cb0a8
pageActions: Use prompt service instead of nsIPrompt. r=johannh
https://hg.mozilla.org/integration/autoland/rev/4b428169709d
Updated prompt tests. r=marionette-reviewers,johannh,whimboo

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.

Duplicate of this bug: 1594214
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/c1122301facc
Fix Thunderbird Prompts following bug 1615588 changes. r=mkmelin
See Also: → 1631147
Regressions: 1631171
Regressions: 1632199
Blocks: 1632805
Blocks: 1633370
You need to log in before you can comment on or make changes to this bug.