1713775 - Marionette should throw a SessionNotCreatedError when "webSocketUrl" capability is passed

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P2)

Description

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

As seen on bug 1693004 Marionette inappropriately passes-through unknown capabilities and their values. That means in case of webSocketUrl not being support the value true will be returned. Whereby the WebDriver BiDi spec requires a string containing the host and port of the WebSocket server.

We should try to get this patch landed as soon as possible and uplift to beta (90).

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1713775 - [marionette] Reject new sessions specifying unknown capabilities

Comment 2

[Julian Descottes [:jdescottes]]

Henrik: Hi! Reading https://bugzilla.mozilla.org/show_bug.cgi?id=1693004#c4 (& the conversation on Matrix) I assume that we want to throw when an unknown capability is passed.

Can you confirm this is the expected change requested here?

Comment 3

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

Yes, that looks fine and correctly returns the session not created error. Due to the simplicity of the patch we shouldn't have a problem to get it uplifted to beta once landed on central. Thanks a lot for the quick turnaround!

Comment 4

[Julian Descottes [:jdescottes]]

Thanks for the feedback!

After running the tests I had a few failures related to the following capability used in a webdriver test: https://searchfox.org/mozilla-central/rev/74f3c420ee54001059e1850bef3be876749ff873/testing/web-platform/tests/webdriver/tests/new_session/support/create.py#38-41

So I investigated a bit more, and I have a concern about throwing for any unknown capability. According to https://developer.mozilla.org/en-US/docs/Web/WebDriver/Capabilities#vendor-specific_capabilities we can expect clients to set chrome specific capapbilities (which by definition we don't handle). There is even a mention that third party might define capabilities.

I am worried that consumers might rely on this. For instance, they might always pass chrome: and firefox: capabilities, assuming only the relevant ones will be consumed, and the other ones will be ignored? Such consumers would now have to be careful to only pass firefox-compatible capabilities when driving Firefox. Maybe it's not a valid concern, but I'd like your feedback on that.

Implementation-wise, I also realized that not all capabilities are explicitly processed in fromJSON (more precisely in _match). The list of those capabilities is browserName, browserVersion, platformName, platformVersion, moz:buildID, moz:headless, moz:processID, moz:profile, moz:shutdownTimeout. I think it will be easier if we have a map listing all supported capabilities and check this upfront instead of doing a check nested in the switch block.

Comment 5

[Julian Descottes [:jdescottes]]

Maybe we should throw for non-prefixed (prefix:) capabilities and treat prefixed capabilities differently?

Comment 6

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

Thanks Julian. That was helpful feedback and contains pieces I didn't thought about. As such I briefly discussed with Julian on Zoom. So maybe the suggested solution was wrong, and we need a better way here. Some questions that came up were:

1. What happens to browser vendor prefixed capabilities? Should those be returned by Marionette? How does chromedriver handle capabilities like moz:webdriverClick?

2. Should we really disallow the session creation? I just read the WebDriver spec again and found the following: Otherwise, if name is the key of an extension capability, let match value be the result of trying implementation-specific steps to match on name with value. If the match is not successful, return success with data null. Doesn't it just mean that the value is replaced with null if capability matching fails, and as result the capability will be dropped when serializing the data to JSON?

3. Maybe we could implement webSocketUrl in a way that it fails the session creation if BiDi is not available. And that for the time being hard-coded to always fail in current Nightly builds and Firefox 90 beta until the BiDi code has been landed? Note that a version of geckodriver that is not supporting webSocketUrl also refuses to create the session interestingly with a invalid argument error but not session not created (this might be a different thing to fix).

It would be good to further get the WebDriver spec pieces discussed with James too.

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 1713775 - [marionette] Fail session creation if the webSocketUrl capability is passed

Until a BiDi implementation can be successfully started, creating a session with the webSocketUrl capability should throw.

Comment 8

[Julian Descottes [:jdescottes]]

Thanks for the summary Henrik!

What happens to browser vendor prefixed capabilities? Should those be returned by Marionette?
How does chromedriver handle capabilities like moz:webdriverClick?

As you suggested, I modified a wdspec test to assert this and ran it against chrome product. My test:

import pytest

from tests.support.asserts import assert_success

def test_moz_prefixed_cap(new_session, add_browser_capabilities):
    response, _ = new_session({"capabilities": {
        "alwaysMatch": add_browser_capabilities({"moz:webdriverClick": True})}})
    value = assert_success(response)
    assert value["capabilities"]["moz:webdriverClick"] == True

This test passes against Firefox, and of course fails against Chrome.
Chrome simply doesn't return the capability but doesn't fail or throw.

This seems to answer your second point. We should drop extension capabilities from the returned data.
Some more data: if we test with another random extension capability: test:whatever, Chrome will not return it, whereas Firefox returns True.
We should fix this to be spec compliant.

When it comes to regular capabilities (without ":"), both Chrome and Firefox reject it at the driver level with InvalidArgumentException.

For the last use case which interests us here, ie regular capabilities accepted by the driver, but not supported by the endpoint (browser), I'm not sure how to test that for Chrome yet.

Comment 9

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

(In reply to Julian Descottes [:jdescottes] from comment #8)

This seems to answer your second point. We should drop extension capabilities from the returned data.
Some more data: if we test with another random extension capability: test:whatever, Chrome will not return it, whereas Firefox returns True.
We should fix this to be spec compliant.

That sounds good. Can you please file a new bug for this particular case? It can be handled all in one bug. Thanks for checking!

For the last use case which interests us here, ie regular capabilities accepted by the driver, but not supported by the endpoint (browser), I'm not sure how to test that for Chrome yet.

I would assume the browser should behave the same as the driver. If the given capability is unknown or cannot be handled no new session will be created. James, do you agree?

Comment 10

[James Graham [:jgraham]]

The spec seems to say that either a) you fail to create a new session or b) you return the set value (see the "otherwise" clause in step 3). I don't think it's clear enough if we should fail the session in case it's an extension capability we don't support.

If the capability isn't an extension capability we should have failed earlier in validate capabilities

Comment 11

[Julian Descottes [:jdescottes]]

(submitted unintentionally when updating the title)

Comment 12

[Julian Descottes [:jdescottes]]

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

That sounds good. Can you please file a new bug for this particular case? It can be handled all in one bug. Thanks for checking!

Filed Bug 1714114

Comment 13

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

(In reply to James Graham [:jgraham] from comment #10)

If the capability isn't an extension capability we should have failed earlier in validate capabilities

Yes, and that's what we do in geckodriver. But adding a new capability there which isn't supported in Marionette yet (or users run an older release of Firefox), would require Marionette to behave the same. As such we might have to update Julian's first patch on this bug to only raise a session not created error in case of non-extension capabilities.

Comment 14

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

And to add in such a case we wouldn't have to special-case webSocketUrl like in the current patch.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4693a77f9211
[marionette] Fail session creation if the webSocketUrl capability is passed r=remote-protocol-reviewers,marionette-reviewers,jgraham

Comment 16

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/4693a77f9211

Comment 17

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

Comment on attachment 9224690 [details]
Bug 1713775 - [marionette] Fail session creation if the webSocketUrl capability is passed

Beta/Release Uplift Approval Request

• User impact if declined: By using the webSocketUrl capability of WebDriver with a Firefox version that doesn't support WebDriver-BiDi the capability will be inappropriately passed-through. Instead Firefox has to fail to create a new WebDriver session.

If declined it will cause WebDriver clients to see a spec incompatible capability value.

• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Add's handling of a not-yet used WebDriver capability, and denies the WebDriver session creation.
• String changes made/needed:

Comment 18

[Julien Cristau [:jcristau]]

Comment on attachment 9224690 [details]
Bug 1713775 - [marionette] Fail session creation if the webSocketUrl capability is passed

approved for 90.0b3

Comment 19

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d9dc99ada64e

Comment 20

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

This fix might actually cause us issues when Selenium tries to enable WebDriver BiDi by default. Without the patch we would silently ignore the capability and let WebDriver HTTP tests still work. But right now we would always fail and not allow a session to be created. I would propose that we backout the patch from the 91ESR branch. Julian, what do you think?

Comment 21

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

Actually lets discuss in our triage meeting today.

Comment 22

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

Discussed last week on Element that we aren't going to back it out from any of our channels given that this is the correct behavior.