1398792 - WebDriver:GetElementProperty cannot access node properties as set by web content

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

[Andreas Tolfsen ❲:ato❳]

The getElementProperty message handler in testing/marionette/listener.js does not appear to be using the content principal of the element.  This means a property set on an element after load as completed will not be accessible.

See https://github.com/mozilla/geckodriver/issues/938 for more details.

More investigation is needed.

Comment 1

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

A simplified Marionette testcase looks like the following:

        self.marionette.navigate(inline("""
        <p id="foo">foo
        <script>
          document.querySelector("p").bar = 42;
        </script>
        """))
        elem = self.marionette.find_element(By.ID, "foo")
        self.assertEqual(elem.get_property("bar"), 42)

Log output:

1654844659996	Marionette	DEBUG	3 -> [0,3,"WebDriver:FindElement",{"value":"foo","using":"id"}]
1654844659998	Marionette	TRACE	[40] MarionetteCommands actor created for window id 4294967298
1654844660002	Marionette	DEBUG	3 <- [1,3,null,{"value":{"element-6066-11e4-a52e-4f735466cecf":"ca013428-a595-4725-8d91-92142a164b17"}}]
1654844660003	Marionette	DEBUG	3 -> [0,4,"WebDriver:GetElementProperty",{"id":"ca013428-a595-4725-8d91-92142a164b17","name":"bar"}]
1654844660003	Marionette	DEBUG	3 <- [1,4,null,{"value":null}]

Then this fails with AssertionError: None != 42.

Given that this issue is still present with our JSWindowActor implementation and got marked as P2 a while ago, we should at least discuss what to do.

Comment 2

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

I wonder if this could be related / similar to bug 1420923 (window globals not exposed to injected script).

Comment 3

[James Graham [:jgraham]]

This is because the command is running in chrome JS, so we get the usual X-ray behaviour where we see the underlying DOM object rather than the content-modified DOM object. So to fix this we either need to waive X-rays for this call, or we need to inject some script into the underlying document in order to read the property.

Comment 4

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

So this behavior should then be related to all the WebDriver classic commands that interact with elements (retrieving some kind of information from the node), right? As it sounds like we should then internally not directly access the element but actually execute a script to get the appropriate value:

https://searchfox.org/mozilla-central/rev/2398ab33adcea896838b3da678ff6480dbb98b9a/remote/marionette/actors/MarionetteCommandsChild.jsm#301

Not sure if there would be another way of accessing the data then which would allow us to waive X-rays.

Comment 5

[James Graham [:jgraham]]

I think the opposite: we generally want to use the X-rayed value except in this one specific case where we're retrieving a js property from the element.

To waive X-rays see https://firefox-source-docs.mozilla.org/dom/scriptSecurity/xray_vision.html#waiving-xray-vision

Comment 6

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

The potential fix here will be the following:

-    return typeof elem[name] != "undefined" ? elem[name] : null;
+    const el = Cu.waiveXrays(elem);
+    return typeof el[name] != "undefined" ? el[name] : null;

James I assume that we also have to waive XRays for some other commands that could potentially access content defined properties? WebDriver:ExecuteScript has issues as well, but waiving XRays here doesn't help. So something more might be involved. But that's something we could spin off into its own bug.

Comment 7

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

We could waive XRays already in evaluate.fromJSON() when we resolve the WebElement reference:
https://searchfox.org/mozilla-central/rev/26a6a38fb515dbab0bb459c40ec4b877477eefef/remote/marionette/evaluate.js#262

Would this have any unexpected and obvious side-effects that we actually don't want?

Comment 8

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

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #7)

We could waive XRays already in evaluate.fromJSON() when we resolve the WebElement reference:
https://searchfox.org/mozilla-central/rev/26a6a38fb515dbab0bb459c40ec4b877477eefef/remote/marionette/evaluate.js#262

This wont work given that in this case all of our commands would loose the ability to access properties only available in privileged scope. So we will have to limit the the scope to only these commands that need it, and maybe it's just this command.

Comment 9

[James Graham [:jgraham]]

Let's scope this bug just to GetElementProperty. For scripting, have a way to reuse the BiDi module rather than a seperate marionette implementation would be better than trying to fix everything in two places.

Comment 11

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

Attachment: Bug 1398792 - [marionette] Waive Xrays in "WebDriver:GetElementProperty" to get unfiltered access to untrusted elements.

Comment 12

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

Attachment: Bug 1398792 - [wdspec] Enhance "Get Element Property" tests for Web References.

Depends on D154219

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1231beb253ea
[marionette] Waive Xrays in "WebDriver:GetElementProperty" to get unfiltered access to untrusted elements. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/bab0dfba4a7c
[wdspec] Enhance "Get Element Property" tests for Web References. r=webdriver-reviewers,jdescottes

Comment 14

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

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

Comment 15

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/1231beb253ea
https://hg.mozilla.org/mozilla-central/rev/bab0dfba4a7c

Comment 16

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

Upstream PR merged by moz-wptsync-bot