1905323 - Fully remove confirm auth prompts

Status: RESOLVED FIXED

(Firefox :: Security, task, P3)

Description

[Emma Zühlcke [:emz]]

Bug 1905322 disables them. This bug is for fully removing related code.

This includes the prefs
network.auth.confirmAuth.enabled
prompts.modalType.confirmAuth

Comment 1

[Emma Zühlcke [:emz]]

Looks like these prefs along with the prompt code are good to remove. We don't have any plans on re-enabling.

Comment 2

[[:skhan]]

Was speaking in regards with this offline and suggesting below

Looks like it is already disabled and we’ve haven’t received any complaints (not that I am aware of). Since desktop is doing it and other browsers on mobile(Chrome, Bing, Brave) don’t have it either, we are okay to remove it for mobile android too

Comment 3

[Kipchumba Chelilim [:mrchumbastic]]

Hi! I'm a new contributor and will start working on this.

Comment 4

[Kipchumba Chelilim [:mrchumbastic]]

I'm working through updates and found two tests that look like they're exclusively testing these auth prompts. Wanted to confirm before removing any testing files. Should netwerk/test/unit/test_SuperfluousAuth.js and dom/security/test/https-first/browser_superfluos_auth.js be removed as part of this update?

Comment 5

[Emma Zühlcke [:emz]]

(In reply to Kipchumba Chelilim [:mrchumbastic] from comment #4)

I'm working through updates and found two tests that look like they're exclusively testing these auth prompts. Wanted to confirm before removing any testing files. Should netwerk/test/unit/test_SuperfluousAuth.js and dom/security/test/https-first/browser_superfluos_auth.js be removed as part of this update?

Yes, they can be removed. Thanks!

I've assigned you to the bug.

Comment 6

[Kipchumba Chelilim [:mrchumbastic]]

Thanks! I removed those tests and modified the others that rely on these prefs:

• accessible/tests/browser/tree/browser_test_nsIAccessibleDocument_URL.js
• toolkit/components/extensions/test/xpcshell/test_ext_dnr_redirect_transform.js
• toolkit/components/places/tests/browser/browser_visituri.js
• widget/tests/browser/browser_test_InputContextURI.js

All the modified tests pass - are there any others I should run? If not, I'll submit a patch in Phabricator for your review!

Comment 7

[Kipchumba Chelilim [:mrchumbastic]]

Attachment: Bug 1905323 - Fully remove confirm auth prompts, prefs, and related tests. r=emz!

Comment 8

[Emma Zühlcke [:emz]]

(In reply to Kipchumba Chelilim [:mrchumbastic] from comment #6)

Thanks! I removed those tests and modified the others that rely on these prefs:

• accessible/tests/browser/tree/browser_test_nsIAccessibleDocument_URL.js
• toolkit/components/extensions/test/xpcshell/test_ext_dnr_redirect_transform.js
• toolkit/components/places/tests/browser/browser_visituri.js
• widget/tests/browser/browser_test_InputContextURI.js

All the modified tests pass - are there any others I should run? If not, I'll submit a patch in Phabricator for your review!

Ideally we should run this on the try server to see if there are other tests failing. Do you have access to that? See https://firefox-source-docs.mozilla.org/tools/try/configuration.html
If not I can also trigger a try run for you.

Comment 9

[Kipchumba Chelilim [:mrchumbastic]]

(In reply to Emma Zühlcke [:emz] from comment #8)

(In reply to Kipchumba Chelilim [:mrchumbastic] from comment #6)

Thanks! I removed those tests and modified the others that rely on these prefs:

• accessible/tests/browser/tree/browser_test_nsIAccessibleDocument_URL.js
• toolkit/components/extensions/test/xpcshell/test_ext_dnr_redirect_transform.js
• toolkit/components/places/tests/browser/browser_visituri.js
• widget/tests/browser/browser_test_InputContextURI.js

All the modified tests pass - are there any others I should run? If not, I'll submit a patch in Phabricator for your review!

Ideally we should run this on the try server to see if there are other tests failing. Do you have access to that? See https://firefox-source-docs.mozilla.org/tools/try/configuration.html
If not I can also trigger a try run for you.

I don't have commit access. If you trigger the run and share the link I can take a look at the results and make any updates needed.

Comment 10

[Kipchumba Chelilim [:mrchumbastic]]

Friendly bump, I don't have commit access yet. If you can trigger the run and send me the try push link I can take it from there.

Comment 11

[Emma Zühlcke [:emz]]

See my comment here: https://phabricator.services.mozilla.com/D271294#9412364

Comment 12

[Rob Wu [:robwu]]

Emma, I see that this bug is a follow-up to bug 1905322, which in turn is a response to bug 1904934.

Your remark in comment 11 of bug 1904934 suggests that other browsers strip the credentials before navigation (is this just visually or are the credentials dropped from the request entirely?):

It looks like other browsers simply remove the auth part from the URI and navigate normally.

Is the removal of the prompt UI & pref all that is needed, or do we need to track more work to align with other browsers?
I can imagine us keeping it to avoid breaking backwards compatibility, but wanted to confirm whether this is truly intended.

I looked for Chrome's intent-to-deprecate of embedded credentials, and https://chromestatus.com/feature/5669008342777856 links to an open spec PR at https://github.com/whatwg/fetch/pull/465 . Interestingly, a comment on that issue links to https://bugzilla.mozilla.org/show_bug.cgi?id=1340200, which was closed as a duplicate of bug 479038, titled "Should the suspicious-auth warning apply to all loads? Should URI userinfo be banned?". Clearly, the UI is being removed here in this bug, so it looks like the first part of the bug title can now be dropped.

P.S. I am taking a closer look because the patch here touches test_ext_dnr_redirect_transform.js and there are multiple test cases in that file that verify that a navigation to a URL with embedded credentials still have embedded credentials, such as: https://searchfox.org/firefox-main/rev/89b2affdc5d2a1588763e5cb4ac046093c4136a9/toolkit/components/extensions/test/xpcshell/test_ext_dnr_redirect_transform.js#298

Comment 13

[Emma Zühlcke [:emz]]

(In reply to Rob Wu [:robwu] from comment #12)

Emma, I see that this bug is a follow-up to bug 1905322, which in turn is a response to bug 1904934.

Your remark in comment 11 of bug 1904934 suggests that other browsers strip the credentials before navigation (is this just visually or are the credentials dropped from the request entirely?):

It's been too long, I don't remember. We'd have to look into it again. Seems easy enough to test with a basic http server.

Is the removal of the prompt UI & pref all that is needed, or do we need to track more work to align with other browsers?
I can imagine us keeping it to avoid breaking backwards compatibility, but wanted to confirm whether this is truly intended.

We should file a bug to investigate whether we need to make behavior changes for URIs with credentials. The removal of the disabled prompt code in this bug isn't blocked by this though as this isn't a behavior change. Ideally we could just ignore any credentials in URIs, but I suspect we're going to break a long tail of things...

I looked for Chrome's intent-to-deprecate of embedded credentials, and https://chromestatus.com/feature/5669008342777856 links to an open spec PR at https://github.com/whatwg/fetch/pull/465 . Interestingly, a comment on that issue links to https://bugzilla.mozilla.org/show_bug.cgi?id=1340200, which was closed as a duplicate of bug 479038, titled "Should the suspicious-auth warning apply to all loads? Should URI userinfo be banned?". Clearly, the UI is being removed here in this bug, so it looks like the first part of the bug title can now be dropped.

Agreed, updated the bug title.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/0b73f6575e3b
https://hg.mozilla.org/integration/autoland/rev/9b5ae7a5a322
Fully remove confirm auth prompts, prefs, and related tests. r=emz,necko-reviewers,extension-reviewers,valentin,robwu,urlbar-reviewers,Sasha

Comment 15

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/9b5ae7a5a322