Closed Bug 1694418 Opened 7 months ago Closed 5 months ago

Update http auth modal prompt for proton

Categories

(Toolkit :: Notifications and Alerts, enhancement, P3)

Desktop
All
enhancement

Tracking

()

VERIFIED FIXED
89 Branch
Tracking Status
firefox89 --- verified

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Keywords: helpwanted, Whiteboard: [proton-modals])

Attachments

(2 files)

The prompt title should be the domain (x-ref bug 1693008) - no protocol.

The prompt text should be:

This site is asking you to sign in.

(We deliberately want to stop displaying the "realm" information)

The input fields should be "Username" and "Password".
They should appear above the input fields.

The button texts should be:

Sign in

Cancel

See complete spec on figma.

Keywords: helpwanted
Whiteboard: [proton-modals]

Update priority to reflect proton priorities.

Priority: P3 → P2

What about for cross-origin cases? Currently: "1$S is requesting your username and password. WARNING: Your password will not be sent to the website you are currently visiting"

Flags: needinfo?(bmikel)

Would this be classified as a JavaScript Modal Dialog: OnBeforeUnload () modal? And, do you have a screenshot of the current modal + copy that you can share with me?

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

(In reply to Meridel from comment #3)

Would this be classified as a JavaScript Modal Dialog: OnBeforeUnload () modal?

No, it's the same as the normal http auth dialog for which the spec has complete text and a design, but with an extra warning text at the end of the descriptive text.

And, do you have a screenshot of the current modal + copy that you can share with me?

You can see the dialog yourself by copy-pasting this URL:

data:text/html,<iframe src="https://jigsaw.w3.org/HTTP/Basic/">

into the URL bar in a new tab.

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

Thanks, Gijs. I have a draft and have flagged Emanuela to review in the deck. This is a bit unusual because a warning icon may be warranted but the placement isn't quite right. Will circle back.

Emanuela, please see my proposal (which is not working), and let me know your thoughts. Happy to meet and discuss!

https://docs.google.com/presentation/d/1YtPJEvUigRybLkuoALYvl63MVg_a6-Db4orDzNdEkIk/edit#slide=id.gc50cde94a0_3_0

Flags: needinfo?(mwalkington) → needinfo?(emanuela)

Copy has been updated and is reflected in slide 47 of the content deck, as well as in the Figma page. Signed off by security team and legal.

Copy deck: https://docs.google.com/presentation/d/1YtPJEvUigRybLkuoALYvl63MVg_a6-Db4orDzNdEkIk/edit#slide=id.gc6499ad969_0_0
Figma: https://www.figma.com/file/FjUe6ORvXZgJvI3rPuTV33/Desktop-UI-(Mozilla-Confidential-)?node-id=5%3A2

Flags: needinfo?(emanuela)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED

The patch in bug 1699183 moves the icon we want to use here.

See Also: → 1699183

Marking as P1. Per experience review we agreed to mark as P1 bug the ones that will block MR1.

Priority: P2 → P1
Priority: P1 → P3
Attachment #9209871 - Attachment description: Bug 1694418 - update the messaging in the http auth prompt for proton, r?mtigley → Bug 1694418 - update username/password strings in the http auth dialog and pre-land other strings, r?mtigley
Keywords: leave-open
Attachment #9209870 - Attachment description: Bug 1694418 - use the channel URI for http auth modal prompt titles and indicate insecure x-origin ones with a separate icon, r?pbz → WIP: Bug 1694418 - show origins in the title of the auth prompt, r?pbz!,mtigley!
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8bb3057a5996
update username/password strings in the http auth dialog and pre-land other strings, r=mtigley,fluent-reviewers,flod

Backed out for failures on browser_ext_optionsPage_modals.js

backout: https://hg.mozilla.org/integration/autoland/rev/9328d26aec28ef3ac4f0d8d82a241ec944d652e2

push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=8bb3057a5996e04407204b45a68713577a59974a&selectedTaskRun=apiPgKHZTo6lfEEU-yAxUw.0

failure log: https://treeherder.mozilla.org/logviewer?job_id=336705716&repo=autoland&lineNumber=13759

[task 2021-04-15T21:03:39.484Z] 21:03:39 INFO - TEST-INFO | screenshot: exit 0
[task 2021-04-15T21:03:39.485Z] 21:03:39 INFO - Buffered messages logged at 21:03:38
[task 2021-04-15T21:03:39.485Z] 21:03:39 INFO - Entering test bound test_tab_options_modals
[task 2021-04-15T21:03:39.485Z] 21:03:39 INFO - Extension loaded
[task 2021-04-15T21:03:39.486Z] 21:03:39 INFO - Buffered messages logged at 21:03:39
[task 2021-04-15T21:03:39.486Z] 21:03:39 INFO - Console message: Warning: attempting to write 20155 bytes to preference extensions.webextensions.uuids. This is bad for general performance and memory usage. Such an amount of data should rather be written to an external file. This preference will not be sent to any content processes.
[task 2021-04-15T21:03:39.487Z] 21:03:39 INFO - Wait the options_ui modal to be opened
[task 2021-04-15T21:03:39.487Z] 21:03:39 INFO - Buffered messages finished
[task 2021-04-15T21:03:39.487Z] 21:03:39 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_optionsPage_modals.js | Expect a tab modal opened for the about addons tab - 0 == 1 - JS frame :: chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_optionsPage_modals.js :: test_tab_options_modals :: line 71
[task 2021-04-15T21:03:39.487Z] 21:03:39 INFO - Stack trace:
[task 2021-04-15T21:03:39.488Z] 21:03:39 INFO - chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_optionsPage_modals.js:test_tab_options_modals:71
[task 2021-04-15T21:03:39.488Z] 21:03:39 INFO - Not taking screenshot here: see the one that was previously logged

also failing: https://treeherder.mozilla.org/logviewer?job_id=336702545&repo=autoland&lineNumber=12870

[task 2021-04-15T20:48:29.455Z] 20:48:29 ERROR - TEST-UNEXPECTED-ERROR | testing/marionette/harness/marionette_harness/tests/unit/test_execute_async_script.py TestExecuteAsyncContent.test_return_value_on_alert | marionette_driver.errors.ScriptTimeoutException: Timed out after 1000 ms
[task 2021-04-15T20:48:29.455Z] 20:48:29 INFO - stacktrace:
[task 2021-04-15T20:48:29.456Z] 20:48:29 INFO - WebDriverError@chrome://marionette/content/error.js:181:5
[task 2021-04-15T20:48:29.456Z] 20:48:29 INFO - ScriptTimeoutError@chrome://marionette/content/error.js:423:5
[task 2021-04-15T20:48:29.456Z] 20:48:29 INFO - evaluate.sandbox/timeoutPromise</scriptTimeoutID<@chrome://marionette/content/evaluate.js:107:16
[task 2021-04-15T20:48:29.457Z] 20:48:29 INFO - notify@resource://gre/modules/Timer.jsm:62:17
[task 2021-04-15T20:48:29.457Z] 20:48:29 INFO - Traceback (most recent call last):
[task 2021-04-15T20:48:29.457Z] 20:48:29 INFO - File "C:\Users\task_1618518821\build\venv\lib\site-packages\marionette_harness\marionette_test\testcases.py", line 214, in run
[task 2021-04-15T20:48:29.457Z] 20:48:29 INFO - testMethod()
[task 2021-04-15T20:48:29.458Z] 20:48:29 INFO - File "C:\Users\task_1618518821\build\tests\marionette\tests\testing\marionette\harness\marionette_harness\tests\unit\test_execute_async_script.py", line 207, in test_return_value_on_alert
[task 2021-04-15T20:48:29.458Z] 20:48:29 INFO - res = self.marionette.execute_async_script("alert()")
[task 2021-04-15T20:48:29.459Z] 20:48:29 INFO - File "C:\Users\task_1618518821\build\venv\lib\site-packages\marionette_driver\marionette.py", line 1729, in execute_async_script
[task 2021-04-15T20:48:29.459Z] 20:48:29 INFO - rv = self._send_message("WebDriver:ExecuteAsyncScript", body, key="value")
[task 2021-04-15T20:48:29.460Z] 20:48:29 INFO - File "C:\Users\task_1618518821\build\venv\lib\site-packages\marionette_driver\decorators.py", line 27, in _
[task 2021-04-15T20:48:29.460Z] 20:48:29 INFO - return func(*args, **kwargs)
[task 2021-04-15T20:48:29.460Z] 20:48:29 INFO - File "C:\Users\task_1618518821\build\venv\lib\site-packages\marionette_driver\marionette.py", line 629, in _send_message
[task 2021-04-15T20:48:29.461Z] 20:48:29 INFO - self._handle_error(err)
[task 2021-04-15T20:48:29.461Z] 20:48:29 INFO - File "C:\Users\task_1618518821\build\venv\lib\site-packages\marionette_driver\marionette.py", line 651, in _handle_error
[task 2021-04-15T20:48:29.461Z] 20:48:29 INFO - raise errors.lookup(error)(message, stacktrace=stacktrace)
[task 2021-04-15T20:48:29.462Z] 20:48:29 INFO - TEST-INFO took 1075ms

Flags: needinfo?(gijskruitbosch+bugs)

Bah, we apparently use tabmodalprompts for http auth still in some edgecases? Relanding without the DTD removal for now, that'll fix the tests...

Flags: needinfo?(gijskruitbosch+bugs)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/81a9057ae48b
update username/password strings in the http auth dialog and pre-land other strings, r=mtigley,fluent-reviewers,flod
Attachment #9209870 - Attachment description: WIP: Bug 1694418 - show origins in the title of the auth prompt, r?pbz!,mtigley! → Bug 1694418 - show origins in the title of the auth prompt, r?pbz!,mtigley!
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/669b31a70306
show origins in the title of the auth prompt, r=pbz,mtigley

Oops, forgot to clear leave-open.

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → 89 Branch

This issue is verified as fixed in our latest beta 89.0b5 bulds on Mac, Windows and Ubuntu.

Status: RESOLVED → VERIFIED

Out of curiosity:

(We deliberately want to stop displaying the "realm" information)

Why did you decide to do that?

(In reply to Julian Reschke from comment #21)

Out of curiosity:

(We deliberately want to stop displaying the "realm" information)

Why did you decide to do that?

In no particular order:

  • it's a spoofing vector
  • in adversarial situations it's basically arbitrary attacker-controlled text displayed in browser UI, which is tricky to display safely at the best of times, even besides spoofing
  • other browsers do not display it
  • cases where the realm is "necessary" because there is more than 1 realm on the same host are (a) setups that we should not encourage, and (b) vanishingly rare, (c) in most of those cases, we expect users know how they end up somewhere ("I clicked a link to app X instead of app Y")
You need to log in before you can comment on or make changes to this bug.