1970330 - Add CSP directive for reftest.xhtml to allow removing the exception for CSP checks for Marionette

Status: RESOLVED FIXED

(Testing :: Marionette Client and Harness, defect, P2)

Description

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

After moving out all the Marionette chrome test files over on bug 1344267, we also need a solution for the files related to reftests. This is a bit more complicated given that it spans across Marionette unit tests and web-platform-tests. Especially that the latter will have to work upstream as well it's not that clear to me yet where to put the relevant xhtml and js files.

Simon, the reftest.xhtml file was as well affected by the wanted CSP changes, or not?

Comment 1

[Simon Friedberger [:simonf]]

It seems I disabled it outside of automation: https://searchfox.org/mozilla-central/source/dom/security/nsContentSecurityUtils.cpp#1983 and I haven't received any bug reports. Does this contradict your statement about using it in WPTs?

Comment 2

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

(In reply to Simon Friedberger (:simonf) from comment #1)

It seems I disabled it outside of automation: https://searchfox.org/mozilla-central/source/dom/security/nsContentSecurityUtils.cpp#1983 and I haven't received any bug reports. Does this contradict your statement about using it in WPTs?

This is the folder as used by the old-style reftest harness: https://searchfox.org/mozilla-central/source/layout/tools/reftest/

With Marionette - as what web-platform-tests require - the location of the chrome files is chrome://remote/content/marionette/ at the moment. But at least for wpt tests we set the disable all security pref so it means that Cu.isInAutomation should be true in our CI.

Comment 3

[James Graham [:jgraham]]

For wpt reftests we do need to run them in the version of the browser we release to end users.

The way it currently works is that the implementation is part of the public marionette API. As far as I know there aren't any other consumers, but it's hard to be sure. So even though the feature is only for running tests, it's not test-only in the same way as stuff that only runs gecko CI; it's part of the product as much as other marionette/WebDriver features.

I don't fully know the context for this bug, but if the current setup is untenable for security reasons, we could consider moving the HTML wrapper file into wpt. But it would be extremely strange to have a product feature that depends on the consumer providing a very specific HTML file that is not itself bundled with the product. So I hope we can find some other solution here.

Comment 4

[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 5

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

Ok, so it looks like that we have to keep the file where it currently is. As such lets see if it will be enough to add the required CSP directive to the XHTML file to make the crashes go away with the following lines removed:

https://searchfox.org/mozilla-central/rev/1eb4f27ece7cb96ac94b635ad0dc95c00d1443db/dom/security/nsContentSecurityUtils.cpp#1980-1984

Comment 6

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

I pushed a try build with the updated content as mentioned above and it looks all good:
https://treeherder.mozilla.org/jobs?repo=try&revision=b1c9aaac4756a43a4057c427f672f05a152df844

Simon, are those changes what you expect to see? If yes, I can upload the patches for review. Thanks.

Comment 7

[Simon Friedberger [:simonf]]

Yes, that looks great!

Comment 8

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

Attachment: Bug 1970330 - [marionette] Add CSP directives to reftest.xhtml.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/4737f6e80ba5
https://hg.mozilla.org/integration/autoland/rev/f3b5d6cf26b1
[marionette] Add CSP directives to reftest.xhtml. r=webdriver-reviewers,jdescottes

Comment 10

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/f3b5d6cf26b1