Andreas Tolfsen ❲:ato❳ Reporter
	Description • 6 years 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.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 1 • 6 years ago

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

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 2 • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 3 • 6 years ago

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?

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 4 • 6 years 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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 5 • 6 years ago

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.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 6 • 6 years 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

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 7 • 6 years ago

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?

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 8 • 6 years 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.

	James Graham [:jgraham]
	Comment 9 • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 10 • 6 years ago

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.

	James Graham [:jgraham]
	Comment 11 • 6 years ago

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

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 12 • 6 years 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.

	Pulsebot
	Comment 20 • 5 years 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

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 22 • 5 years ago

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

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 23 • 5 years ago

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)