1502360 - Make "Send Alert Text" WebDriver spec conformant

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement, P1)

Description

[Andreas Tolfsen ❲:ato❳]

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.

Comment 1

[Andreas Tolfsen ❲:ato❳]

I appear to have screwed up something with the permissions on this bug.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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?

Comment 4

[Andreas Tolfsen ❲:ato❳]

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.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 6

[Andreas Tolfsen ❲:ato❳]

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

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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?

Comment 8

[Andreas Tolfsen ❲:ato❳]

(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.

Comment 9

[James Graham [:jgraham]]

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.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 11

[James Graham [:jgraham]]

> 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.

Comment 12

[Andreas Tolfsen ❲:ato❳]

(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.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1502360 - [wdclient] "Alert Send Text" has to use "text" property. r?ato

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

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1502360 - [wdspec] Add basic HTTP auth test for "Send Alert Text". r?ato

Depends on D19998

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1502360 - [marionette] Raise "invalid argument" error in "Send Alert Text" if text is not of type string. r?ato

Depends on D19999

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1502360 - [marionette] Check for valid prompt type in "Send Alert Text". r?ato

Depends on D20000

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1502360 - [marionette] "Send Alert Text" has to use the text field's value property. r?ato

Depends on D20002

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/7fef29b4607b
https://hg.mozilla.org/mozilla-central/rev/ef80cafbfcf5
https://hg.mozilla.org/mozilla-central/rev/57d79c0b44b6
https://hg.mozilla.org/mozilla-central/rev/3646c27586d4
https://hg.mozilla.org/mozilla-central/rev/6b9548ef048c

Comment 22

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15440 for changes under testing/web-platform/tests

Comment 23

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

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)