1710425 - Only allow Marionette to switch to "chrome" context if system access flag is enabled

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement, P2)

Description

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

Follow-up from bug 1359472, which lands the first part of the required patches to not allow chrome context by default.

In general WebDriver implementations involve giving clients some privileged access to the browser. Therefore when enabling it it is assumed that users are happy with the security implications. However giving chrome access does provide additional capabilities compared to other drivers (more system access other than reading files, access to change the firefox UI, etc.).

It therefore might make sense to make this a separate opt-in feature. It can be done via an environment variable like MOZ_MARIONETTE_CHROME_CONTEXT, or a command line argument like --marionette-enable-chrome.

Comment 1

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

With bug 1944565 implemented Marionette can rely on the internal Remote Agent status flag if access to the parent process is allowed or not. Based on its value we can adjust the Marionette:SetContext command to allow allow or deny the request for switching to chrome scope.

Comment 2

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

We actually decided to include this bug into the next milestone to improve security when using the chrome scope testing capabilities with Marionette.

I'll have a patch ready soon which we will land early in the 138 Nightly cycle starting by next Monday.

For using chrome scope testing the --remote-allow-system-access argument needs to be specified for Firefox. For the Marionette client, mochitest and the wptrunner we are going to automatically enable it. But users of geckodriver will have to specify it manually. They can use the --allow-system-access argument of geckodriver, or specify it via their NewSession capabilities as argument for Firefox if only some tests would need this special privilege.

Comment 3

[Greg Mierzwinski [:sparky]]

:whimboo, just to be clear, will we need to specify --remote-allow-system-access and --allow-system-access or is it just one of them that needs to be specified?

Comment 4

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

(In reply to Greg Mierzwinski [:sparky] from comment #3)

:whimboo, just to be clear, will we need to specify --remote-allow-system-access and --allow-system-access or is it just one of them that needs to be specified?

If you are using geckodriver you can use its own --allow-system-access argument. It will internally forward this argument to Firefox. It will be set as long as geckodriver is running. If you need finer control you might want to pass it via the moz:firefoxOptions[args] capability.

Peter, do you think that you might have a chance to get it added to Browsertime soon? If it might take a while we could start landing the upcoming patches pref'ed off by default. That would allow testing before we enforce the pref to be enabled (and remove it eventually).

Comment 5

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

To prevent current WebDriver clients from getting broken because they do not yet pass in this argument to Firefox I'm going to add the Firefox preference remote.system-access-check.enabled which defaults to true but allows to get flipped to temporarily ignore the check. I'm most likely going to remove the preference in +2 versions of Firefox (after the next ESR).

Comment 6

[Peter Hedenskog]

Hi Henrik, sorry I missed this. Let me add --allow-system-access this weekend.

Comment 7

[Peter Hedenskog]

Attachment: Screenshot 2025-03-13 at 07.46.08.png

Connection failed to localhost when flag is enabled.

Comment 8

[Peter Hedenskog]

I updated to Geckodriver 0.36.0 and added the flag, but when the flag is used Firefox always opens with a "Connection failed to localhost". I'm attaching a screenshot (it's in Swedish though) and then continuous on with navigating to the page. Is that how it should work?

Comment 9

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

(In reply to Peter Hedenskog from comment #8)

I updated to Geckodriver 0.36.0 and added the flag, but when the flag is used Firefox always opens with a "Connection failed to localhost". I'm attaching a screenshot (it's in Swedish though) and then continuous on with navigating to the page. Is that how it should work?

Do you have a trace log that you can share? The option shouldn't have such a side-effect.

Comment 10

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

I've sent a try build with the latest changes and there seem to be failures for the telemetry-client tests. I tried to run the tests locally but they fail as well with and without my patches applied.

Chutten, is there maybe a problem with running those tests on try and locally?

Comment 11

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

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

I've sent a try build with the latest changes and there seem to be failures for the telemetry-client tests. I tried to run the tests locally but they fail as well with and without my patches applied.

Chutten, is there maybe a problem with running those tests on try and locally?

Chutten seems to be away. Jan-Erik can you maybe help me out with this telemetry client failure? Thanks.

Comment 12

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

Attachment: Bug 1710425 - [wdspec] Fix "deep_update" helper to also work with lists.

Comment 13

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

Attachment: Bug 1710425 - [wdspec] Don't parameterize capabilities marker and define default_capabilities fixture.

When using the "capabilities" marker there is actually no
need to parameterize the test. It will only add a "capabilities0-"
prefix to the test names, which doesn't give any additional
information.

Further it should be possible for other folders to
customize the default set of capabilities. As such
a new "default_capabilities" fixture has been added
and the existing "capabilities" fixture will now
retrieve the current default capabilities and merge
them when necessary with any capabilities markers.

Comment 14

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

Attachment: Bug 1710425 - [wdspec] Add "allow_system_access" marker for Mozilla specific tests using the chrome context.

Comment 15

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

Attachment: Bug 1710425 - [wpt] Update Marionette executor to use "--remote-allow-system-access" argument for Firefox.

Comment 16

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

Attachment: Bug 1710425 - [mochitest] Allow system access permissions for Marionette to run scripts in chrome scope.

Comment 17

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

Attachment: Bug 1710425 - [marionette-client] Allow system access by default for now.

Comment 18

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

Attachment: Bug 1710425 - [marionette] Only allow Marionette to switch to "chrome" context if system access flag is enabled.

Comment 19

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

Attachment: Bug 1710425 - [raptor] Temporarily skip system access check in Marionette.

Comment 20

[Peter Hedenskog]

Ok I added it here:
https://gist.github.com/soulgalore/87333fbe02d870f13d2b2982e006ed56

I added most verbose in Browsertime, hope that is enough.

Comment 21

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

(In reply to Peter Hedenskog from comment #20)

Ok I added it here:
https://gist.github.com/soulgalore/87333fbe02d870f13d2b2982e006ed56

I added most verbose in Browsertime, hope that is enough.

Thanks. That looks good. If you want to test it locally you can download a build for your platform from this try job.

Comment 22

[Jan-Erik Rediger [:janerik]]

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

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

I've sent a try build with the latest changes and there seem to be failures for the telemetry-client tests. I tried to run the tests locally but they fail as well with and without my patches applied.

Chutten, is there maybe a problem with running those tests on try and locally?

Chutten seems to be away. Jan-Erik can you maybe help me out with this telemetry client failure? Thanks.

I cannot reproduce a test failure on these locally on current m-c.
chutten is back on Monday, either he or I will take another look then. Keeping the ni? as a reminder.

Comment 23

[Rob Wu [:robwu]]

Do these patches change any telemetry behavior? If yes, try running tests without artifact builds to see if it makes a difference. ./mach try accepts the --no-artifact flag for that.

Comment 24

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

(In reply to Rob Wu [:robwu] from comment #23)

Do these patches change any telemetry behavior? If yes, try running tests without artifact builds to see if it makes a difference. ./mach try accepts the --no-artifact flag for that.

No, it doesn't change anything telemetry related. It's only that we require an additional argument to be set for Firefox to allow execution of JavaScript in the parent process via Marionette. The Marionette client, which the telemetry harness uses as well, does it automatically. I'll try building a full build locally and check if that may work - and indeed might be related to an artifact build (we had some differences in the past).

Comment 25

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

Jan-Erik, the hint from Rob is actually correct! Those hangs in the Telemetry client tests seem to only happen for artifact builds. I now tried to run all the tests with a full build and there is no timeout at all. So I think that I can ignore these failures given that they aren't related to my changes.

Comment 26

[Jan-Erik Rediger [:janerik]]

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

Jan-Erik, the hint from Rob is actually correct! Those hangs in the Telemetry client tests seem to only happen for artifact builds. I now tried to run all the tests with a full build and there is no timeout at all. So I think that I can ignore these failures given that they aren't related to my changes.

Thanks! We had artifact build issues in the past, but I hoped I had fixed this. I file a bug on our side to take a look separate from this patch set.

Comment 27

[Rob Wu [:robwu]]

I was wondering what the bug was, and found it at bug 1954458

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/21aa0a7e9ee7
[wdspec] Fix "deep_update" helper to also work with lists. r=webdriver-reviewers,Sasha
https://hg.mozilla.org/integration/autoland/rev/16f45d2bf555
[wdspec] Don't parameterize capabilities marker and define default_capabilities fixture. r=webdriver-reviewers,Sasha
https://hg.mozilla.org/integration/autoland/rev/e3236853c767
[wdspec] Add "allow_system_access" marker for Mozilla specific tests using the chrome context. r=webdriver-reviewers,Sasha
https://hg.mozilla.org/integration/autoland/rev/547515ba766f
[wpt] Update Marionette executor to use "--remote-allow-system-access" argument for Firefox. r=Sasha
https://hg.mozilla.org/integration/autoland/rev/160f7d0ebcd7
[mochitest] Use "--remote-allow-system-access" command line argument for Firefox to run scripts in chrome scope. r=jmaher
https://hg.mozilla.org/integration/autoland/rev/d068ede3ae98
[marionette-client] Allow system access by default for now. r=webdriver-reviewers,Sasha
https://hg.mozilla.org/integration/autoland/rev/6307451bab04
[marionette] Only allow Marionette to switch to "chrome" context if system access flag is enabled. r=webdriver-reviewers,Sasha
https://hg.mozilla.org/integration/autoland/rev/fd986dce31fc
[raptor] Temporarily skip system access check in Marionette. r=perftest-reviewers,sparky

Comment 29

[Sandor Molnar[:smolnar]]

Backed out for causing wpt & lint failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/3e7d2dc07f55d2df2e18cc6cf6c0ac032c9ddfa6

Push with failures

Failure log lint failure #1

Failure log lint failure #2

Failure log wpt

Failure log bc

Comment 30

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

There were the following issues with the changes:

• Running the verify job for the new /_mozilla/webdriver/harness/system_access.py test fails because we do not clear the preference when testing the opt-out of the allow system access check. So it leaks into the default behavior test causing it to fail.
• wpt tests on Android were failing because I missed to update the wpt runner code for Android so that the --remote-allow-system-access argument was not passed to Firefox.

I'll update the patches and reland the patch set.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/53515a7bb8d2
[wdspec] Fix "deep_update" helper to also work with lists. r=webdriver-reviewers,Sasha
https://hg.mozilla.org/integration/autoland/rev/8bab64b88459
[wdspec] Don't parameterize capabilities marker and define default_capabilities fixture. r=webdriver-reviewers,Sasha
https://hg.mozilla.org/integration/autoland/rev/6449d5e81a61
[wdspec] Add "allow_system_access" marker for Mozilla specific tests using the chrome context. r=webdriver-reviewers,Sasha
https://hg.mozilla.org/integration/autoland/rev/2043911f92d9
[wpt] Update Marionette executor to use "--remote-allow-system-access" argument for Firefox. r=Sasha
https://hg.mozilla.org/integration/autoland/rev/551aee3c6180
[mochitest] Use "--remote-allow-system-access" command line argument for Firefox to run scripts in chrome scope. r=jmaher
https://hg.mozilla.org/integration/autoland/rev/3de060f1d300
[marionette-client] Allow system access by default for now. r=webdriver-reviewers,Sasha
https://hg.mozilla.org/integration/autoland/rev/6b7587922973
[marionette] Only allow Marionette to switch to "chrome" context if system access flag is enabled. r=webdriver-reviewers,Sasha
https://hg.mozilla.org/integration/autoland/rev/1f0bd0bd3122
[raptor] Temporarily skip system access check in Marionette. r=perftest-reviewers,sparky

Comment 32

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

Attachment: Bug 1710425 - [wdspec] Fix linter failure for new system_access.py test

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b20cc537b8db
[wdspec] Fix linter failure for new system_access.py test CLOSED TREE

Comment 34

[Norisz Fay [:noriszfay]]

Backed out together with Bug 1944565 for causing multiple failures

Backout link

Push with failures - bc
Push with failures - wd
Push with failures - lint

Failure log - bc
Failure log - wd
Failure log - lint

Comment 35

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

(In reply to Norisz Fay [:noriszfay] from comment #34)

Push with failures - bc

These browser chrome test failures are caused by D241412, which basically only adds the --remote-allow-system-access command line argument when starting Firefox. It's not clear to me how this changes the way in writing to and reading from the Window Registry. This is the failing test:

https://searchfox.org/mozilla-central/rev/e47d3061be57b52385c0ef947bac032e4e80d606/browser/base/content/test/startup/browser_preXULSkeletonUIRegistry.js#50-81

Alex, given that you wrote this test do you have an idea why this single line change could have caused this test to fail?

Comment 36

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

So I've filed bug 1955535 to get this specific case handled separately.

As a workaround we can directly set the environment variable MOZ_REMOTE_ALLOW_SYSTEM_ACCESS in the mochitest runner code instead of the command line argument. Locally this will make the test pass for me. I'll update the relevant revision.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fcf388fd59a4
[wdspec] Fix "deep_update" helper to also work with lists. r=webdriver-reviewers,Sasha
https://hg.mozilla.org/integration/autoland/rev/49c2c0db9fdd
[wdspec] Don't parameterize capabilities marker and define default_capabilities fixture. r=webdriver-reviewers,Sasha
https://hg.mozilla.org/integration/autoland/rev/4fb2dcbd7270
[wdspec] Add "allow_system_access" marker for Mozilla specific tests using the chrome context. r=webdriver-reviewers,Sasha
https://hg.mozilla.org/integration/autoland/rev/9bead19a453a
[wpt] Update Marionette executor to use "--remote-allow-system-access" argument for Firefox. r=Sasha
https://hg.mozilla.org/integration/autoland/rev/e359c48000b4
[mochitest] Allow system access permissions for Marionette to run scripts in chrome scope. r=jmaher
https://hg.mozilla.org/integration/autoland/rev/a27f33458aa7
[marionette-client] Allow system access by default for now. r=webdriver-reviewers,Sasha
https://hg.mozilla.org/integration/autoland/rev/5424a8c31bd9
[marionette] Only allow Marionette to switch to "chrome" context if system access flag is enabled. r=webdriver-reviewers,Sasha
https://hg.mozilla.org/integration/autoland/rev/be1d2942df72
[raptor] Temporarily skip system access check in Marionette. r=perftest-reviewers,sparky

Comment 38

[agoloman]

Backed out for causing failures @test_no_errors_clean_profile.py.

• Backout link
• Push with failures
• Failure Log

Comment 39

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

It's the patch on bug 1944565 which needs the update.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3bbbf850fc7b
[wdspec] Fix "deep_update" helper to also work with lists. r=webdriver-reviewers,Sasha
https://hg.mozilla.org/integration/autoland/rev/82eb986c6712
[wdspec] Don't parameterize capabilities marker and define default_capabilities fixture. r=webdriver-reviewers,Sasha
https://hg.mozilla.org/integration/autoland/rev/2e01172bc3f7
[wdspec] Add "allow_system_access" marker for Mozilla specific tests using the chrome context. r=webdriver-reviewers,Sasha
https://hg.mozilla.org/integration/autoland/rev/c765284053d2
[wpt] Update Marionette executor to use "--remote-allow-system-access" argument for Firefox. r=Sasha
https://hg.mozilla.org/integration/autoland/rev/2f95a4cdb113
[mochitest] Allow system access permissions for Marionette to run scripts in chrome scope. r=jmaher
https://hg.mozilla.org/integration/autoland/rev/69229209fa01
[marionette-client] Allow system access by default for now. r=webdriver-reviewers,Sasha
https://hg.mozilla.org/integration/autoland/rev/ea986d7c5fd0
[marionette] Only allow Marionette to switch to "chrome" context if system access flag is enabled. r=webdriver-reviewers,Sasha
https://hg.mozilla.org/integration/autoland/rev/713732e682e8
[raptor] Temporarily skip system access check in Marionette. r=perftest-reviewers,sparky

Comment 41

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/713732e682e8d3d1f9c69869cf5ba6c3424636f3
https://hg.mozilla.org/mozilla-central/rev/ea986d7c5fd0c05a3907c2754acd0aa236cf625b
https://hg.mozilla.org/mozilla-central/rev/69229209fa01ee918d7e33216fca31df96741610
https://hg.mozilla.org/mozilla-central/rev/2f95a4cdb113fa9fca1a748088460ea783bfa845
https://hg.mozilla.org/mozilla-central/rev/c765284053d2d43d86f0cb197d0e55a1ca41bcb6
https://hg.mozilla.org/mozilla-central/rev/2e01172bc3f70a21343b97f2a33886c42842fb52
https://hg.mozilla.org/mozilla-central/rev/82eb986c67124bd3cef054d6f1933075cda9491b
https://hg.mozilla.org/mozilla-central/rev/3bbbf850fc7b3b5c5da1f00102681508d78c60c1

Comment 42

[Julian Descottes [:jdescottes]]

Hi Daniel, would it be OK to make this bug and the dependency Bug 1944565 non confidential?
We'd like to mention them in our release notes.

Comment 43

[Daniel Veditz [:dveditz]]

Looks fine to me, and for the other dependency bug 1359472 too.