Make "Send Alert Text" WebDriver spec conformant

RESOLVED FIXED in Firefox 67

Status

enhancement
P1
normal
RESOLVED FIXED
7 months ago
3 months ago

People

(Reporter: ato, Assigned: whimboo)

Tracking

(Blocks 2 bugs)

Trunk
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(5 attachments)

Reporter

Description

7 months ago
whimboo mentioned during the TPAC meeting that WebDriver:ElementSendKeys
works to enter input in to the HTTP basic auth dialogues.  This
dialogue is a ChromeWindow and it sounds suspicious that we allow
user interaction with chrome elements when we're in content context.
Presumably this means that you can find chrome elements and sneak
your way into chrome scope.
Reporter

Updated

7 months ago
Group: mozilla-employee-confidential, core-security-release
Reporter

Updated

7 months ago
Group: mozilla-employee-confidential
Reporter

Updated

7 months ago
Group: mozilla-employee-confidential
Reporter

Updated

7 months ago
Blocks: 1355883
Reporter

Comment 1

7 months ago
I appear to have screwed up something with the permissions on this bug.
Note, that this most likely is working because we do NOT only listen for tab modal dialogs, but also real modal dialogs: 

https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/testing/marionette/modal.js#18

Note that "common-dialog-loaded" would be necessary once we want to observe the HTTP authentication dialog.
Group: mozilla-employee-confidential
Group: core-security-release → mozilla-employee-confidential
Keywords: sec-other
Andreas, I wonder how this might be in conflict to what you mentioned at:
https://github.com/w3c/webdriver/issues/1086#issuecomment-433967524

If that is true we should be able to turn the `prompts.tab_modal.enabled` preference to any value, and should still pass all the tests. By that we would also allow access for content scripts to interact with modal dialogs, which would be similar to HTTP authentication dialogs.

Maybe I miss something?
Reporter

Comment 4

7 months ago
Neither content scripts or WebDriver interaction commands (click,
send keys) should be allowed to interact with chrome windows or UI
that is not naturally part of the web content.  Regardless if it’s
tab- or global modal, this is UI that is not part of web content.
The only command allowed to interact with these are the alert
commands, so we should never expect that e.g. Element Send Keys
which produces DOM events through the Pointer Events spec to affect
user prompts.
Oh, I think I now know where the confusion is! You may have missed my final comment on this topic when we spoke about HTTP authentication at TPAC. It is NOT `Element Send Keys` I was talking about but `Send Alert Text`, like:

> session.alert.text = "user" + Keys.TAB + "password"

So I assume that there is nothing security related then, which needs further investigation and discussion.
Reporter

Comment 6

7 months ago
Thanks for the clarification on which command we are dealing with.
If Send Alert Text allows the tab PUA key to be used, this is also
a problem becuase it is not meant to be based on the action
primitives, quoting the Send Alert Text algorithm: [1]

> 6. Perform user agent dependent steps to set the value of current
> user prompt’s text field to text.

I would not expect sending a PUA tab key to Send Alert Text to work.
If it does support that, the special PUA keys could allow a user
to break out of web content to interacting with chrome UI, and that
is what we need to prevent happening.

Does that make sense?

  [1] https://w3c.github.io/webdriver/#send-alert-text
Reporter

Updated

7 months ago
Summary: Review element interaction with HTTP basic auth dialogues → Send Alert Text allows special PUA keys to be used, which could break out of web content
Oh, I was already wondering about step 6 and what "user agent dependent steps" mean. Currently it uses the same implementation as `Element Send Keys`, and as such handles the special keys.

So you are saying that we don't expect people to enter Unicode characters and such in the user and password field? And that we should only allow ASCII characters? Or does it mean that we should directly set the value property of both input fields?
Reporter

Comment 8

7 months ago
(In reply to Henrik Skupin (:whimboo) from comment #7)

> So you are saying that we don't expect people to enter Unicode
> characters and such in the user and password field? And that we
> should only allow ASCII characters? Or does it mean that we should
> directly set the value property of both input fields?

We need to support Unicode input, but we
don’t want the special PUA keys mentioned in
https://w3c.github.io/webdriver/#dfn-normalised-key-value to not
have any effect as they could break us out of web content.

In other words, if the input is "foou\E004bar" (U+E004 is Tab) we
will set that value on the XUL <input> element literally.  Since
U+E004 is a private-use area Unicode character it will obviously not
be visible to the naked eye.
It seems like maybe it would be more consistent to set a literal tab character in the input. I assume that's already possible to achieve using copy+paste; in this case it seems like send keys should be equivalent to pasing the string into the selected field.
FYI the problem doesn't appear when using the following code:

> session.alert.text = "user\tpassword"

(In reply to Andreas Tolfsen ❲:ato❳ from comment #8)
> We need to support Unicode input, but we
> don’t want the special PUA keys mentioned in
> https://w3c.github.io/webdriver/#dfn-normalised-key-value to not
> have any effect as they could break us out of web content.

They actually shouldn't be able to break out given that opening the modal dialog spins the event loop, and as such blocks everything else until it get closed.

> In other words, if the input is "foou\E004bar" (U+E004 is Tab) we
> will set that value on the XUL <input> element literally.  Since
> U+E004 is a private-use area Unicode character it will obviously not
> be visible to the naked eye.

We could simply do this by directly setting the value, and not using the call to `interaction.sendKeysToElement()`. 

Please note that Firefox displays the square placeholder for U+E004. But if that is fine I can make that change.
> Please note that Firefox displays the square placeholder for U+E004. But if that is fine I can make that change.

Reading the spec there's nothing there about handling PUA characters in alerts, so ato is right they should just be treated as literals and if Firefox displays a placeholder that's correct behaviour.
Reporter

Comment 12

7 months ago
(In reply to Henrik Skupin (:whimboo) from comment #10)

> FYI the problem doesn't appear when using the following code:
>
> > session.alert.text = "user\tpassword"

FWIW a tabulator character is not equivalent to the (invisible)
private user area character U+E004.

> We could simply do this by directly setting the value, and not
> using the call to `interaction.sendKeysToElement()`.

Yes this sounds like the right fix: the value should just be set
literally.
Reporter

Updated

4 months ago
Priority: -- → P3

Interestingly I cannot reproduce the behavior anymore that I can switch to the password field when using \t. So something else might have fixed it. Anyway, we still want to set the value directly.

While working on this bug I noticed a couple of other issues with Send Alert Text, which I will fix as separate commits on this bug.

Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P3 → P2
Group: mozilla-employee-confidential

Actually all those changes require an update of the bug's summary. With the upcoming patch series the command will be spec conformant.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e845d27e557523b65562d8373bacccf738959410

Blocks: webdriver
Keywords: sec-other
Priority: P2 → P1
Summary: Send Alert Text allows special PUA keys to be used, which could break out of web content → Make "Send Alert Text" WebDriver spec conformant

Per WebDriver spec the "value" property with a list of characters
is not allowed anymore.

Comment 20

3 months ago
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7fef29b4607b
[wdclient] "Alert Send Text" has to use "text" property. r=ato
https://hg.mozilla.org/integration/autoland/rev/ef80cafbfcf5
[wdspec] Add basic HTTP auth test for "Send Alert Text". r=ato
https://hg.mozilla.org/integration/autoland/rev/57d79c0b44b6
[marionette] Raise "invalid argument" error in "Send Alert Text" if text is not of type string. r=ato
https://hg.mozilla.org/integration/autoland/rev/3646c27586d4
[marionette] Check for valid prompt type in "Send Alert Text". r=ato
https://hg.mozilla.org/integration/autoland/rev/6b9548ef048c
[marionette] "Send Alert Text" has to use the text field's value property. r=ato
Depends on: 1528462
Depends on: 1528463
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15440 for changes under testing/web-platform/tests
Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/15440
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/UmbvZ82hQDKoueqE3OMDPQ)
You need to log in before you can comment on or make changes to this bug.