1684469 - Basic Auth prompter open in one tab prevents other Basic Auth tabs from loading

Status: VERIFIED FIXED

(Firefox :: Security, defect, P1)

Description

[Ori Avtalion (salty-horse)]

Using latest nightly.

1. Visit a website that requires basic authentication: https://httpbin.org/basic-auth/a/a
   The prompter dialog will appear docked to the top of the tab. Ignore it
2. Open a new tab with another address that requires basic authentication: https://httpbin.org/basic-auth/b/b

It will appear to load indefinitely.
If you forgot that the first tab was prompting you for something, you'd be confused, thinking there was an issue with it.

The prompter for the second tab will only appear once the first tab's dialog is closed, or the tab is closed.

Comment 1

[B.J. Herbison]

I see the same behavior on Nightly 86 and Firefox 84 on Linux: The second page doesn't load and needs to be reloaded after the first tab with the authentication is closed before I see the second authentication prompt.

Comment 2

[Johann Hofmann [:johannh]]

Paul, that feels like a regression from tab modal auth prompts. Can you investigate and set the correct regressing bug and regression range if that's the case?

Thank you :)

Comment 3

[Clara Guerrero ( Need Info Brindusa Tot Please)]

Hi , is this ubuntu specific?
I'm not able to replicate on windows 10 pro using 86.0a1 (2021-01-06) (64-bit)
Best,
Clara

Comment 4

[Ori Avtalion (salty-horse)]

Happens to me on Windows 10 Home with 86.0a1 20210106093742.
Make sure not to touch the first Basic Auth dialog. Keep it open when opening the new tab.

Comment 5

[Hans-Peter Jansen]

I'm suffering from this issue with FF 84.0{,.1} on openSUSE Tumbleweed.

My common setup contains a huge set of state (159 windows with a couple of tabs, ATM).

Restarting is a cruel, since it really depends on locating the correct order of auth dialogs in the tabs of any of those windows, since you serialize them at your own discretion.

This is about to render the usability of Firefox to nil from my humble POV.

If there's a proposal to fix this issue, I'm ready and prepared to apply patches on my own builds, if necessary, or is there a way to disable the tab modal auth dialogs?

Would be glad to to test any approach.

Thanks.

Comment 6

[Emma Zühlcke [:emz]]

I've initially thought this would be a direct regression of Bug 613785. However, I can reproduce with Firefox 81 which uses the window prompts, if I test with two browser windows. Only one auth window prompt will be opened at a time. I can even reproduce this behavior with Firefox 20.
The issue just becomes more apparent with tab prompts, because they don't block the whole window and can be in a background tab.
This looks like Bug 1029779. There is queuing logic here: https://searchfox.org/mozilla-central/rev/c59d9181cbcd8356ce9271723e31be11641e7010/toolkit/components/passwordmgr/LoginManagerAuthPrompter.jsm#871

Comment 7

[Emma Zühlcke [:emz]]

From some quick testing it seems to work if I update the caller to prompt sync (it already does this as a fallback): https://searchfox.org/mozilla-central/rev/c59d9181cbcd8356ce9271723e31be11641e7010/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp#1194,1204

asyncPromptAuth was originally created to support queueing multiple auth prompts. The concern was that multiple window prompts could be confusing. However, with tab prompts thats no longer a concern.

The ideal fix would probably to change the auth prompt to an async prompt method that does not perform queuing. Similar to asyncPromptUsernameAndPassword, but with different signature. The caller (nsHttpChannelAuthProvider) would have to be refactored to handle the JS Promise.
I assume this would also fix the issues we have with the auth prompt when it spins a nested event loop. See https://bugzilla.mozilla.org/show_bug.cgi?id=1685337#c2

Comment 8

[Hans-Peter Jansen]

With the password dialog appearing in a separate windows, even if serialized, one can locate such windows using the window manager.

With the current scheme, an open auth dialog is blocking any other auth dialog from that host. Since nothing indicates the locked state other than reload spinning the reload wheel, users have no indication, why it is blocking. I've learned it the hard way, that this is due an auth dialog in some another tab of any open window/tab.

In pathologic cases (which includes me of course), I have ten auth dialogs to submit in the correct order for a single host, that appear unordered one after another. I'm not sure, if you can imagine, how that feels. Again, it's not the serial nature of auth dialogs, that is so frustrating, it's the fact, that there's no indication, where the next auth dialog is lurking. With >700 open tabs, this is a plain PITA.

The easiest fix even for older releases would be switching back to auth dialog windows, wouldn't it? Can you point me to a related commit, that I might be able to revert for our openSUSE eco system? That would be great.

Comment 9

[Hans-Peter Jansen]

s/easiest fix/easiest workaround/

While at it, is there a list display of all tabs available? If so, a mitigation would be to indicate the fact of a (auth) locked tab.

Comment 10

[:Gijs (he/him)]

(In reply to Hans-Peter Jansen from comment #8)

Note that there were a number of spoofing and/or DoS style attacks possible with the window auth dialogs, which the tab-modal dialog resolves. I don't think reverting this change is likely to be a good choice in general (most people never encountering http auth that they are actually interested in rather than being spoofed/attacked by it, or perhaps very rarely), though I can see why you would want to in your particular case.

The easiest fix even for older releases would be switching back to auth dialog windows, wouldn't it? Can you point me to a related commit, that I might be able to revert for our openSUSE eco system? That would be great.

It's simpler than that; you can flip prompts.modalType.httpAuth to 3 in about:config.

Comment 11

[Johann Hofmann [:johannh]]

So I agree that this isn't a "direct" regression of bug 613785 (as in bug 613785 didn't add the broken code), but it did expose this issue so I do think we should treat it as a regression to fix from that bug. I think Paul is already looking at this. Please assign yourself when you're more confident with a solution or let me know if you don't have time for this. :)

In the meantime I also agree with Gijs and Paul that we should not revert this change.

Thanks!

Comment 12

[Emma Zühlcke [:emz]]

I've got a patch draft for this. Currently running some tests. The changes are:

1. Remove queue code in LoginManagerAuthPrompter to allow multiple prompts to be shown at the same time
2. Refactor LoginManagerAuthPrompter + Prompter.jsm (and their interfaces) to make asyncPromptAuth fully async.
   After doing some preliminary testing it looks like this also fixes Bug 1685337 because we no longer spin the event loop.

This regresses auth window prompts slightly. However, window auth prompts are not a supported configuration. We should unship the code for that (+ pref) soon.

Comment 13

[Emma Zühlcke [:emz]]

Enabling multiple auth prompts per tab surfaced some issues with our TabDialogBox/SubDialog implementation. I've added the these as blockers.

Comment 14

[Hans-Peter Jansen]

(In reply to :Gijs (he/him) from comment #10)

It's simpler than that; you can flip prompts.modalType.httpAuth to 3 in about:config.

I've tested this in a couple of setups and it makes the situation manageable again.

Thank you Gijs, that hint is much appreciated.

Paul: Obviously, you hit some otherwise harder to exploit issues, that will be a significant win for the project, when fixed. Good luck with the approach.

Comment 15

[Emma Zühlcke [:emz]]

Attachment: Bug 1684469 - Allow showing multiple async auth prompts at the same time. r=sfoster!

• Made asyncPromptAuth fully async.
• Removed auth prompt queuing from LoginManagerAuthPrompter asyncPromptAuth implementation.
• If there are multiple auth prompts with the same target in a tab, consolidate them.
• Removed unused method asyncPromptAuthBC.
• Fixed an issue with PromptTestUtils#waitForPrompt where it didn't always return
  the correct prompt.
• Added test for multi tab auth prompts.

Comment 16

[Sam Foster [:sfoster] (he/him)]

Attachment: httpauth-iframes.html - testcase with iframe'd http auth-protected pages from different origins

It looks like the current patch handles this case ok, but I wanted to upload it here for reference. The order of the challenges is going to depend on which server responds first, but they should be correctly queued, so when the first prompt is closed, the 2nd should appear.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/111af4c2bf6b
Allow showing multiple async auth prompts at the same time. r=necko-reviewers,sfoster,agi

Comment 18

[Natalia Csoregi [:nataliaCs]]

Backed out for auth and dialog box failures.

backout: https://hg.mozilla.org/integration/autoland/rev/35d4ab75c406e70cbec79b2ca00fd347ae843fdc

push: https://treeherder.mozilla.org/jobs?repo=autoland&revision=111af4c2bf6bfad9a928fe8fb25c9bc480da50be&group_state=expanded

failure logs:

• TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/browser/browser_basicAuth_multiTab.js | Uncaught exception - at resource://testing-common/PromptTestUtils.jsm:175 - TypeError: can't access property "_dialogs", dialogBox._dialogManager is undefined
• TEST-UNEXPECTED-FAIL | browser/base/content/test/tabPrompts/browser_confirmFolderUpload.js | Uncaught exception - at resource://testing-common/PromptTestUtils.jsm:175 - TypeError: can't access property "_dialogs", dialogBox._dialogManager is undefined
• TEST-UNEXPECTED-FAIL | browser/base/content/test/tabdialogs/browser_tabdialogbox_focus.js | Uncaught exception - at chrome://mochitests/content/browser/browser/base/content/test/tabdialogs/browser_tabdialogbox_focus.js:100 - TypeError: can't access property "_dialogs", dialogBox._dialogManager is undefined

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2aa38ecfb55
Allow showing multiple async auth prompts at the same time. r=necko-reviewers,sfoster,agi

Comment 20

[Sandor Molnar[:smolnar]]

Backed out for causing mochitest failures in browser_basicAuth_multiTab.

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

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunning%2Cpending%2Crunnable&searchStr=mochi%2Ce10s&revision=c2d9108fee938f4ce0031f47ce2195895b6a898a&selectedTaskRun=UBb2e_d3QCimoC2jwSadnw.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=327824465&repo=autoland&lineNumber=11279

:INFO - GECKO(2835) | [Parent 2835, Main Thread] WARNING: Extra shutdown CC: 'i < NORMAL_SHUTDOWN_COLLECTIONS', file /builds/worker/checkouts/gecko/xpcom/base/nsCycleCollector.cpp:3364
[task 2021-01-26T13:24:58.635Z] 13:24:58 INFO - GECKO(2835) | [Parent 2835, Main Thread] WARNING: NS_ENSURE_TRUE(Preferences::InitStaticMembers()) failed: file /builds/worker/checkouts/gecko/modules/libpref/Preferences.cpp:4378
[task 2021-01-26T13:24:58.691Z] 13:24:58 INFO - TEST-INFO | Main app process: exit 0
[task 2021-01-26T13:24:58.692Z] 13:24:58 INFO - TEST-INFO | Confirming we saw 366 DOCSHELL created and 366 destroyed log strings.
[task 2021-01-26T13:24:58.693Z] 13:24:58 INFO - TEST-INFO | Confirming we saw 1071 DOMWINDOW created and 1071 destroyed log strings.
[task 2021-01-26T13:24:58.693Z] 13:24:58 ERROR - TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/browser/browser_basicAuth_multiTab.js | leaked 2 window(s) until shutdown [url = about:blank]
[task 2021-01-26T13:24:58.694Z] 13:24:58 INFO - TEST-INFO | toolkit/components/passwordmgr/test/browser/browser_basicAuth_multiTab.js | windows(s) leaked: [pid = 2835] [serial = 44], [pid = 2835] [serial = 53]
[task 2021-01-26T13:24:58.694Z] 13:24:58 INFO - TEST-INFO | toolkit/components/passwordmgr/test/browser/browser_basicAuth_multiTab.js | This test created 1 hidden window(s):

Comment 21

[Emma Zühlcke [:emz]]

There were some intermittent issues with tab creation and teardown in the browser_basicAuth_multiTab test. Verify tests on try are green now.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc6aa1a6191b
Allow showing multiple async auth prompts at the same time. r=necko-reviewers,sfoster,agi

Comment 23

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/dc6aa1a6191b

Comment 24

[Emma Zühlcke [:emz]]

We shouldn't uplift this. This is not a recent regression and the patch feels risky, given that we're changing the interface and how prompts are queued.

Comment 25

[Hans-Peter Jansen]

With all due respect, but I beg to differ.

As described in #8, the change to modal auth dialogs could lead to very unpleasant behavior, that could render FF almost unusable in certain situations.

Comment 26

[Emma Zühlcke [:emz]]

(In reply to Hans-Peter Jansen from comment #25)

With all due respect, but I beg to differ.

As described in #8, the change to modal auth dialogs could lead to very unpleasant behavior, that could render FF almost unusable in certain situations.

Thanks for raising these concerns. To be clear, the issue has been fixed for Firefox 87. My comment was about not wanting to fast-track this by uplifting to 86, because we want to ensure the change is stable.

Comment 27

[Daniel Bodea [:danibodea]]

I have reproduced this issue in Beta v86.0b5 and Release v85.0 and verified the fix in Nightly v87.0a1 on Windows 10, Ubuntu 20 and Mac OS 11.2.
In the affected builds, the second opened basic authentification web-page does not display the authentification modal, while on the fixed build, the authentification modal gets displayed on pages even if the first one is ignored.
I deem this bug verified.