1837963 - Consider disallowing opening a picker from a background page

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Edgar Chen [:edgar]]

(This is split from bug 1828276 comment #8)

I think we should not allow a background page can open a picker and even switch itself to foreground. It's worth to check how other browser behave. However, in general, open a picker requires a user gesture, so blocking a background page from opening a picker should not cause web compatibility problem. Because in most of case "a-background-page == no-user-gesture", unless user interact with the page, switch it to background then tries to open a picker, and expect that work, but I think that is a very corner case.

This is from a fullscreen sec bug, though we have a solution for that, bug 1821884, but not sure if opening-pickers-from-background might cause other bugs, so file this bug for tracking and mark as a sec bug, too.

Comment 1

[Daniel Veditz [:dveditz]]

I'm not sure what this bug is tracking, which leaves me unsure what security rating to give it. Blocking pickers-from-background in cases other than fullscreen? Tracking regressions ("might cause other bugs") from bug 1821884? The "edge case" of opening something from a page and then switching focus to another window/tab before the picker can open? That last one is known to be exploitable on mobile, at least.

Comment 2

[Edgar Chen [:edgar]]

This bug is created to track potential issues of opening-pickers-from-background other than fullscreen.

(In reply to Daniel Veditz [:dveditz] from comment #1)

The "edge case" of opening something from a page and then switching focus to another window/tab before the picker can open? That last one is known to be exploitable on mobile, at least.

Do we have a bug on mobile?

Comment 3

[Edgar Chen [:edgar]]

Attachment: Bug 1837963 - Disallow opening file pickers from background tabs;

Comment 4

[Edgar Chen [:edgar]]

Attachment: Bug 1837963 - Disallow opening color pickers from background tabs;

Comment 5

[Edgar Chen [:edgar]]

Attachment: Bug 1837963 - Introduce nsBaseColorPicker;

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36f4b96eb15b
Disallow opening file pickers from background tabs; r=geckoview-reviewers,emilio,m_kato
https://hg.mozilla.org/integration/autoland/rev/90114d0e72d1
Introduce nsBaseColorPicker; r=win-reviewers,mac-reviewers,emilio,gstoll,mstange
https://hg.mozilla.org/integration/autoland/rev/5ed6d107133f
Disallow opening color pickers from background tabs; r=geckoview-reviewers,emilio,m_kato
https://hg.mozilla.org/integration/autoland/rev/6465dbafe002
apply code formatting via Lando

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for causing build bustages on nsColorPicker.cpp:
https://hg.mozilla.org/integration/autoland/rev/ec9441f956a20c64d517c8cc84f40149939d71ba

Push with bustage
Failure log

[task 2025-01-14T13:08:12.807Z] 13:08:12    ERROR -  /builds/worker/checkouts/gecko/widget/gtk/nsColorPicker.cpp:97:15: error: use of undeclared identifier 'aColorPickerShownCallback'
[task 2025-01-14T13:08:12.807Z] 13:08:12     INFO -     97 |   mCallback = aColorPickerShownCallback;
[task 2025-01-14T13:08:12.807Z] 13:08:12     INFO -        |               ^

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7c4052454ec2
Disallow opening file pickers from background tabs; r=geckoview-reviewers,emilio,m_kato
https://hg.mozilla.org/integration/autoland/rev/d19b71505a16
Introduce nsBaseColorPicker; r=win-reviewers,mac-reviewers,emilio,gstoll,mstange
https://hg.mozilla.org/integration/autoland/rev/cd000de4a239
Disallow opening color pickers from background tabs; r=geckoview-reviewers,emilio,m_kato

Comment 9

[Sandor Molnar[:smolnar]]

Backed out for causing build bustages

Backout link: https://hg.mozilla.org/integration/autoland/rev/2bd4c737942733221ce01f49c6995e9cd5122700

Push with failures

Failure log -> ERROR - /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:49:38: error: member access into incomplete type 'mozilla::dom::BrowsingContext'

Failure log ->ERROR - /builds/worker/checkouts/gecko/widget/windows/nsColorPicker.cpp(193,16): error: function declared 'stdcall' here was previously declared without calling convention

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7907d00425fb
Disallow opening file pickers from background tabs; r=geckoview-reviewers,emilio,m_kato
https://hg.mozilla.org/integration/autoland/rev/a4ef712fa44d
Introduce nsBaseColorPicker; r=win-reviewers,mac-reviewers,emilio,gstoll,mstange
https://hg.mozilla.org/integration/autoland/rev/e49da4b8b2d0
Disallow opening color pickers from background tabs; r=geckoview-reviewers,emilio,m_kato

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7907d00425fb
https://hg.mozilla.org/mozilla-central/rev/a4ef712fa44d
https://hg.mozilla.org/mozilla-central/rev/e49da4b8b2d0

Comment 12

[Donal Meehan [:dmeehan]]

:edgar I see this is a sec want, do we need to uplift this to beta and esr128?

Comment 13

[Ryan VanderMeulen [:RyanVM]]

I'd rather let this get some bake time rather than uplifting to Beta in the last week of the cycle. Not super concerned about ESR either, but leaving that option open for now until Edgar has a chance to reply.

Comment 14

[Edgar Chen [:edgar]]

Not super concern about ESR, either. And I would like this get some bake time on Beta/Release.

Comment 15

[Rob Wu [:robwu]]

I noticed that the behavioral change introduced here does not have any test coverage.

Since the patches have caused a regression (bug 1949092), I decided to not only add test coverage for the regressed functionality, but also a simple test case for the common case (a tab browser), at https://phabricator.services.mozilla.com/D238843 (as test_canOpenModalPicker_in_tab).