1606604 - Build the remote agent on all release channels

Status: RESOLVED FIXED

(Remote Protocol :: Agent, task, P3)

Description

[Andreas Tolfsen ❲:ato❳]

We currently only build the remote agent on the Nightly release channel. We should expand to target all branches.

The remote agent component will still remained preffed-off for other release channels than Nightly.

Comment 1

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

Hi Daniel. We may want to enable building the Remote Agent by default for beta/release soon. As such we wonder if we would need another security review beside the one we already got for Nightly (bug 1542229) a while ago. Thanks!

Comment 2

[Daniel Veditz [:dveditz]]

Is the Marionette-like visual indicator implemented? That's a requirement for Release.

Have you changed the architecture or the commands?

Comment 3

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

(In reply to Daniel Veditz [:dveditz] from comment #2)

Is the Marionette-like visual indicator implemented? That's a requirement for Release.

Yes, both Marionette and the Remote Agent share the same visual cue when they are enabled.

Have you changed the architecture or the commands?

There were the following two changes:

1. Given that we want to allow users of the upcoming Selenium 4 to opt-in for the Remote Agent via webdriver capabilities (see the meta bug 1669746) we had to separate the handling of the visual cue for both of them. Mainly because of a crash when both were enabled at the same time. This work was done via bug 1669698. It means we now have dedicated observer notifications for Marionette and Remote Agent, which both are used to set the visual cue.

2. To stay in par with Chrome I removed the --remote-debugger command line argument (bug 1669230). Now only --remote-debugger-port can be used, to set the port of the HTTP connection for localhost, which on connection will be upgraded to a websocket connection.

Otherwise lots of work has been put into the general API support for CDP. A lot of new commands and events have been implemented. When you ask for commands, do you also mean those? I don't think because of the constant development it would cause lots of security reviews, but I thought it would be good to ask.

Comment 4

[Daniel Veditz [:dveditz]]

both Marionette and the Remote Agent share the same visual cue when they are enabled.

Great

Now only --remote-debugger-port can be used, to set the port of the HTTP connection for localhost, which on connection will be upgraded to a websocket connection.

But it's still a required command-line option without which the remote agent is not started, right? IOW, if you define a port we understand that to mean you want Firefox to use that port. That sounds fine: it's still an explicit opt-in action people have to take, and that a normal browsing session started by clicking a desktop icon or opening a .HTML file with the default browser won't have the remote agent enabled (ignore people who boobytrap their shortcut links or mess with the windows registry to add params -- that's still opting in).

If so that all sounds fine.

When you ask for commands, do you also mean those?

Yeah, I did, but maybe I'm sorry I asked :-) There are two main types of worries there:

1. Is a given command itself dangerous or bad for privacy? Well, yes, they probably all are by design given the kind of control this feature is designed for. If we stick with "standard" commands implemented by other browsers also at least we know there will be a chance for lots of feedback on commands that go too far.
2. Is the implementation of a command a security problem? It's not hard to imagine an otherwise-acceptable command being implemented in a way that does dangerous things with untrusted input, for example. Hopefully regular code review catches this kind of issue because I don't have the bandwidth for security code review. But it might be worth spending the time looking over the list and flagging some of the interesting-looking ones with potential issues for further follow-up.

This kind of effort doesn't need to be a "blocking" issue. With respect to 1) I'd be mostly interested in the Firefox-only commands and would mostly trust the larger community to find the rough edges with the CDP-defined ones. Effort around 2) would be ongoing as new commands are implemented. Might be a good idea to start with the current list though.

Comment 5

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

(In reply to Daniel Veditz [:dveditz] from comment #4)

But it's still a required command-line option without which the remote agent is not started, right? IOW, if you define a port we understand that to mean you want Firefox to use that port. That sounds fine: it's still an explicit opt-in action people have to take, and that a normal browsing session started by clicking a desktop icon or opening a .HTML file with the default browser won't have the remote agent enabled (ignore people who boobytrap their shortcut links or mess with the windows registry to add params -- that's still opting in).

That is correct. You have to opt-in via the command line option. It's also possible via the preference, but not only by just flipping the value of remote.enabled (similar to marionette.enabled) to turn it on/off in the same session. The preference is only evaluated during startup.

1. Is a given command itself dangerous or bad for privacy? Well, yes, they probably all are by design given the kind of control this feature is designed for. If we stick with "standard" commands implemented by other browsers also at least we know there will be a chance for lots of feedback on commands that go too far.

Given that the Remote Agent (Protocol) implements parts of CDP (Chrome DevTools Protocol) the concerns would be similar to the Firefox Devtools protocol. Yes, it's expected that there are APIs to get/set sensitive data.

2. Is the implementation of a command a security problem? It's not hard to imagine an otherwise-acceptable command being implemented in a way that does dangerous things with untrusted input, for example. Hopefully regular code review catches this kind of issue because I don't have the bandwidth for security code review. But it might be worth spending the time looking over the list and flagging some of the interesting-looking ones with potential issues for further follow-up.

The commands we have implemented do in nearly all the cases type checks, given that there is no schema validation in-place. Even if something would be wrong here, other commands like Runtime.evaluate and Runtime.callFunctionOn basically allow to run arbitrary JavaScript code in the content process's sandbox. But all that already passed the initial security review.

This kind of effort doesn't need to be a "blocking" issue. With respect to 1) I'd be mostly interested in the Firefox-only commands and would mostly trust the larger community to find the rough edges with the CDP-defined ones. Effort around 2) would be ongoing as new commands are implemented. Might be a good idea to start with the current list though.

Note that for 1) we do not have Firefox-only commands. What we implement is what CDP defines. Means there is nothing added on top of it. In regards of 2) it would be good to know if we would have to ask for sec-review for each and every command? It would cause a lot of extra turnaround time, that we would have to keep in mind.

Thanks for your replies.

Comment 6

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

Attachment: Bug 1606604 - [remote] Build the remote agent on all release channels.

Comment 7

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

With this patch applied there are unexpected passes of cookie tests for Firefox beta builds. The underlying reason here is that for Nightly we run with lax by default enabled, which is an experimental feature and enabled by default. But that is not available on beta.

It also means that the cookie tests in Puppeteer unit tests aren't written for lax by default enabled. As such we will have to wait until that has been taken into account. I will inform Mathias.

Comment 8

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

Attachment: Bug 1606604 - [remote] Disable experimental SameSite=Lax by default.

Comment 9

[Mathias Bynens]

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

With this patch applied there are unexpected passes of cookie tests for Firefox beta builds. The underlying reason here is that for Nightly we run with lax by default enabled, which is an experimental feature and enabled by default. But that is not available on beta.

It also means that the cookie tests in Puppeteer unit tests aren't written for lax by default enabled. As such we will have to wait until that has been taken into account. I will inform Mathias.

SameSite=lax is enabled by default in Chrome 85+: https://www.chromestatus.com/feature/5088147346030592 The latest Puppeteer already uses 85+, so it'd be surprising if our tests would be incompatible with it. Which test in particular are you talking about?

Relevant Puppeteer roll commits/test changes:

• https://github.com/puppeteer/puppeteer/commit/e8232890a4b61d0dc7032e22665dc8bb0ea69556#diff-e594d69f2e63e83f7266760e92acf767f3878fb92df627c32d3d11e7cbba0dbc
• https://github.com/puppeteer/puppeteer/commit/e89e264a16ff403925896104dbd3d34ff97dac86#diff-953fae7cbe44fa545271a5b0dd45cfe11e24420922f5ade4c54b1ef7f83551e4

Comment 10

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

Interesting. Since that has been enabled we haven't had a look at our cookie code. But as it looks like the failure happens because we always return "sameSite\": \"Lax\ as part of the cookie data even when lax is default. So I assume that CDP doesn't return this property for the default setting.

Comment 11

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

So that's indeed the case here. I filed bug 1683617 to get this fixed in our implementation.

Comment 12

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

The behavior of CDP is actually more complicated. As such we will go ahead and simply disable lax by default for now, and work on bug 1683617 once we have the necessary information and time.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c6c88dc5956
[remote] Disable experimental SameSite=Lax by default. r=remote-protocol-reviewers,jgraham
https://hg.mozilla.org/integration/autoland/rev/7357db102337
[remote] Build the remote agent on all release channels. r=webdriver-reviewers,remote-protocol-reviewers,firefox-build-system-reviewers,jgraham,mhentges

Comment 14

[Cristian Brindusan [:cbrindusan]]

Backed out 2 changesets (Bug 1606604) for build bustages on remote_default.
https://hg.mozilla.org/integration/autoland/rev/5bbe059fd4ea7c4561d4c96c0f89bd99988ab979

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=7357db10233773daf5a9ab0d84cc850ec4949fb5&selectedTaskRun=VR9bdABLSy6Vecyg3YuAaQ.0

Failure log:
https://treeherder.mozilla.org/logviewer?job_id=325210324&repo=autoland&lineNumber=879

Comment 15

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

It's failing because I missed to also remove the milestone from the decorator. Will fix it right away.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/213adb1c4163
[remote] Disable experimental SameSite=Lax by default. r=remote-protocol-reviewers,jgraham
https://hg.mozilla.org/integration/autoland/rev/d0a1a16b7aa0
[remote] Build the remote agent on all release channels. r=webdriver-reviewers,remote-protocol-reviewers,firefox-build-system-reviewers,jgraham,mhentges

Comment 17

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/213adb1c4163
https://hg.mozilla.org/mozilla-central/rev/d0a1a16b7aa0