Closed Bug 1606797 Opened 4 years ago Closed 4 years ago

Do not allow more than one simultaneous "open external protocol" dialog to appear per originating tab

Categories

(Firefox :: File Handling, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 79
Tracking Status
firefox-esr68 --- wontfix
firefox72 --- wontfix
firefox73 --- wontfix
firefox74 --- wontfix
firefox75 --- wontfix
firefox76 --- wontfix
firefox77 --- wontfix
firefox78 --- fixed
firefox79 --- fixed

People

(Reporter: csasca, Assigned: Gijs)

References

(Depends on 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

Attached image Double the trouble.gif

Affected versions

  • Firefox 68.4.0esr
  • Firefox 70.0.1
  • Firefox 72.0b11
  • Firefox 73.0a1

Affected platforms

  • Windows 10 & 7
  • macOS 10.15

Steps to reproduce

  1. Open the following link and login with valid credentials
  2. If not available, create a new contact and add a phone number to it
  3. On the newly created contact, click on the phone number

Expected result

  • An external application window requesting the user to choose is shown

Actual result

  • Two windows will appear, requesting the same action

Regression range

  • I will see for a regression, if there is one.

Additional notes

  • The actual issue can be seen in the attachment.
  • On Ubuntu 18, a request to launch an external application will not appear, instead a page stating that "The address was not understood" will appear.
Has Regression Range: --- → no
Has STR: --- → yes

This is an issue with Outlook, I'm pretty sure? It'll be navigating to tel:<phonenumber> more than once.

I think we already have a bug on file about being able to open more than one of these dialogs, but I can't find it. Johann, do you remember? I would have thought we would have filed this after the zoommtg: stuff last year.

Flags: needinfo?(jhofmann)

The only thing I know of is bug 1361653, which I originally thought was only an issue on Android. But if a website is able to open an infinite amount of these that sounds annoying on desktop, too. Not sure how to consolidate these bugs now.

Flags: needinfo?(jhofmann)

This issue was reproducible on Windows 10 with Firefox Nightly version 74.0a1 (2020-01-14) (64-bit). Marked as affected.

The priority flag is not set for this bug.
:Gijs, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(gijskruitbosch+bugs)
Priority: -- → P3
Summary: Two external application windows will appear when a phone number is accessed on Outlook-People → Do not allow more than one simultaneous "open external protocol" dialog to appear per originating tab

Managed to reproduce the issue with older builds such as 25.0a1/32.0a1 builds before this don't seem to load the page at all.
It seems this is not a regression and as per comment 1, this would be an issue with Outlook.

Has Regression Range: no → ---

This issue was reproducible on latest Nightly version 74.0a1 (2020-02-04) on Windows 10

This issue was reproducible on Latest Nightly version 75.0a1 (2020-03-02) on Windows 10. Changed flag accordingly

This issue was reproducible on Latest Nightly version 76.0a1 (2020-03-18) (64-bit) on Windows 10. Changed flag accordingly

This issue is reproducible on Windows 10 with Firefox Nightly version 77.0a1 (2020-04-15) (64-bit)

Depends on: 1573736
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED

This patch adds the following constraints:

  • close the dialog when the originating context navigates;
  • close the dialog when the originating context closes / goes away;
  • do not allow more than 1 dialog at a time;
  • do not allow opening the dialog from an inactive (background) browser.

Depends on D77028

Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ecef5e940485
pass the triggering principal when opening external URIs, r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/47c229951c8b
do not allow navigating to external URIs in cross-origin disjoint browsing contexts, r=johannh,smaug
https://hg.mozilla.org/integration/autoland/rev/b4b26ce1c20e
restrict opening the 'open external protocol' dialog, r=johannh,fluent-reviewers,flod
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 79

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

See Also: → 1642966

The patch landed in nightly and beta is affected.
:Gijs, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(gijskruitbosch+bugs)

Comment on attachment 9156454 [details] [diff] [review]
Beta patches for protocol restrictions

Beta/Release Uplift Approval Request

  • User impact if declined: Confusing / DoS-style issues with the external protocol dialog
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: n/a
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): Medium because it's not that early in the cycle anymore. However, the patches are reasonably straightforward, have some automated test coverage, and they fix a few security/DoS style issues with external protocols, so I think they're worth taking. There's also an about:config pref we can use to disable the aspect of this patch with the most risk (disallowing external URLs opened on cross-origin popups).
  • String changes made/needed: not in this patch for uplift

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: DoS-style (sec-low/sec-vector) security issues prior to the release of the first ESR
  • User impact if declined: (as above)
  • Fix Landed on Version: 79
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): (as above)
  • String or UUID changes made by this patch: not in this patch for uplift
Attachment #9156454 - Flags: approval-mozilla-esr78?
Attachment #9156454 - Flags: approval-mozilla-beta?

Maybe we can let this ride to 79 and get the fix in ESR for 78.1?

(In reply to Julien Cristau [:jcristau] from comment #19)

Maybe we can let this ride to 79 and get the fix in ESR for 78.1?

I'm not worried about landing this on beta at this point in the cycle. However, iIf enterprise things use external protocol handlers, this change may affect them, and landing it on ESR after shipping the initial ESR release seems more risky (to them) than landing it before we ship the first one.

Flags: needinfo?(jcristau)
Comment on attachment 9156454 [details] [diff] [review]
Beta patches for protocol restrictions

alright then, approved for 78.0b8, it'll go into esr78 from there.
Flags: needinfo?(jcristau)
Attachment #9156454 - Flags: approval-mozilla-esr78?
Attachment #9156454 - Flags: approval-mozilla-beta?
Attachment #9156454 - Flags: approval-mozilla-beta+

Backed out for ESLint failures (and I'm guessing eventually other test failures too given the nature of the reported issue).
https://hg.mozilla.org/releases/mozilla-beta/rev/bdb9a8c9c312748fd764664e3808d2c0e1f4e2d3

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=306526838&repo=mozilla-beta&lineNumber=124

Flags: needinfo?(gijskruitbosch+bugs)

Sorry, unused variable left after removing the string changes, but there shouldn't have been any ill effects from that other than the eslint failure...

Attachment #9156454 - Attachment is obsolete: true
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 9157081 [details] [diff] [review]
Beta patches for protocol restrictions

Re-approved for 78.0b8. Thanks for the fix-up.
Attachment #9157081 - Attachment is patch: true
Attachment #9157081 - Flags: approval-mozilla-beta+
Regressions: 1650162
Regressions: 1651395
Regressions: 1651014
Regressions: 1651174
Regressions: 1652213
Regressions: 1683756
You need to log in before you can comment on or make changes to this bug.