1851788 - Only accept "chrome" browsing context ids for BiDi commands when "system access" is enabled

Status: RESOLVED FIXED

(Remote Protocol :: WebDriver BiDi, task, P2)

Description

[Julian Descottes [:jdescottes]]

STRs:

• try to send a navigate or reload command to a chrome browsing context id (eg "1" or "2")

ER: Should fail with noSuchFrame
AR: Throws the following error:

 {
  "type": "error",
  "id": 6,
  "error": "unknown error",
  "message": "TypeError: can't access property \"browsingContext\", webProgress is null",
  "stacktrace": "#awaitNavigation@chrome://remote/content/webdriver-bidi/modules/root/browsingContext.sys.mjs:1043:21\nreload@chrome://remote/content/webdriver-bidi/modules/root/browsingContext.sys.mjs:935:32\nhandleCommand@chrome://remote/content/shared/messagehandler/MessageHandler.sys.mjs:255:33\nexecute@chrome://remote/content/shared/webdriver/Session.sys.mjs:274:32\nonPacket@chrome://remote/content/webdriver-bidi/WebDriverBiDiConnection.sys.mjs:211:37\nonMessage@chrome://remote/content/server/WebSocketTransport.sys.mjs:127:18\nhandleEvent@chrome://remote/content/server/WebSocketTransport.sys.mjs:109:14\n"
}

Comment 1

[Julian Descottes [:jdescottes]]

As discussed in the meeting, any command which allows to send a specific context id might pass the BrowsingContext id of a chrome context. By default, chrome Browsing Contexts have quite predictable ids: "1", "2", "3"...

Our context ids are a combination of UUIDs generated by tab manager (for top level contexts) and actual platform BrowsingContext ids, which means that we will fallback to BrowsingContext.get when the user provides a context id. And in that case we will start trying to apply the command to this chrome context.

After testing, it doesn't seem that any existing command can effectively interact with those contexts.
Commands which need to use the child actor can't work because our actors are only created for messageManagerGroups: ["browsers"], and we also throw in any case around https://searchfox.org/mozilla-central/rev/0d51e137a45a3657b39be3c27d4c7271fdb11d59/remote/shared/messagehandler/transports/RootTransport.sys.mjs#114

So only fully parent process commands could actually try to run against chrome contexts. If I listed them right

• browser.close: does not support a context argument
• browsingContext.close: no effect
• browsingContext.navigate: fails when trying to access the webprogress
• browsingContext.reload: fails when trying to access the webprogress
• browsingContext.print: fails with 0x80070057 (NS_ERROR_ILLEGAL_VALUE)
• browsingContext.setViewport: fails with TypeError: can't access property \"style\", browser is null

Nevertheless, we should make sure the browsing contexts we retrieve are content contexts until we support chrome scope.
Or maybe we should always create UUIDs, even for non-toplevel browsing contexts.

Comment 2

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

Thanks for testing Julian. Given that none of the commands allow escaping from content and run code that is not wanted I feel that we could potentially make this a confidential bug only?

Comment 3

[Julian Descottes [:jdescottes]]

I agree we can remove the security flag but we are not able to do it ourselves.

Freddy: can you or someone from the security group remove the security flag?

Comment 4

[Frederik Braun [:freddy]]

Done. Do you also want the "confidential Mozilla employee bug" removed?

Comment 5

[Julian Descottes [:jdescottes]]

Thanks!

Do you also want the "confidential Mozilla employee bug" removed?

No this is fine, we want to keep this internal until it is fixed.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:whimboo, could you have a look please?

For more information, please visit BugBot documentation.

Comment 7

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

Slightly rewording the bug's summary to avoid overloading the context identifier by adopting the term scope for consistency.

Comment 8

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

I think that we can remove the confidential flag from this bug given that it will be a feature that is publicly usable.

Comment 9

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

With the patch as landed for bug 1774436 a while ago we now have a centralized _getNavigable helper method on the RootBiDiModule class.

And as part of my patch on bug 1944568 there is now logic to only allow chrome browsing contexts when the chrome scope is passed as option to this method.

Given that no other command is passing in the scope option yet, none of them can currently interact with chrome browsing contexts. Commands that we want to allow to run for chrome browsing contexts would have to set this option.

Now the name of the option is a bit misleading and I think we should rename it to allowChrome. Then we can enable BiDi commands step by step when they support chrome scope just by passing in true as value. Once all commands support it we probably can remove it. The check for allow system access I would do within that method.

Comment 10

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

Attachment: Bug 1851788 - [webdriver-bidi] Enhance "browsingContext.getTree" command for chrome scope when root parameter is set.

Comment 11

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

Attachment: Bug 1851788 - [wdspec] Add tests for cookie methods not supporting chrome browsing contexts.

Comment 12

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

Attachment: Bug 1851788 - [remote] Add extra checks when retrieving child browsing contexts by navigable id.

Comment 13

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

Attachment: Bug 1851788 - [webdriver-bidi] "browsingContext.close" has to use _getNavigable as well.

Comment 14

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

The double negation in the summary makes it harder to understand. Lets simplify.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/95529d6d2435
https://hg.mozilla.org/integration/autoland/rev/9b6f9a67e9e9
[remote] Add extra checks when retrieving child browsing contexts by navigable id. r=Sasha
https://github.com/mozilla-firefox/firefox/commit/7a607605563f
https://hg.mozilla.org/integration/autoland/rev/8241575051b8
[webdriver-bidi] "browsingContext.close" has to use _getNavigable as well. r=jdescottes
https://github.com/mozilla-firefox/firefox/commit/50c4de911c37
https://hg.mozilla.org/integration/autoland/rev/24c1cb89f759
[webdriver-bidi] Enhance "browsingContext.getTree" command for chrome scope when root parameter is set. r=Sasha,jdescottes
https://github.com/mozilla-firefox/firefox/commit/be218488315d
https://hg.mozilla.org/integration/autoland/rev/e192fe1902ed
[wdspec] Add tests for cookie methods not supporting chrome browsing contexts. r=jdescottes

Comment 16

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/9b6f9a67e9e9
https://hg.mozilla.org/mozilla-central/rev/8241575051b8
https://hg.mozilla.org/mozilla-central/rev/24c1cb89f759
https://hg.mozilla.org/mozilla-central/rev/e192fe1902ed