1734840 - Investigate a safe way to execute custom formatters hooks from DevTools server

Status: RESOLVED FIXED

(DevTools :: Framework, task)

Description

[Nicolas Chevobbe [:nchevobbe]]

Custom formatters register methods on the content page global that needs to be executed so we can get the wanted representation of such object.
We should make sure that executing such hooks:

• wouldn't cause Security issue (i.e. getting access to privileged code)
• wouldn't cause privacy issue (e.g. disallow any requests / storing data on cookies or storage, …)

We could try to take advantage of our instant evaluation feature [1], and bail out as soon as one of those hooks would have an effect on some state of the page.
Those hooks should only consumes the logged object and return a custom representation for it, so it shouldn't be a blocker for the custom formatters authors, while providing us some guarantees.

Another idea would be to execute the hooks in some kind of Sandbox, but I'm still not sure how that could be done while still maintaining access to the logged object (and maybe the classes defined by a library that would need them?)

[1] https://searchfox.org/mozilla-central/rev/477950cf9ca9c9bb5ff6f34e0d0f6ca4718ea798/devtools/server/actors/webconsole/eval-with-debugger.js#108,120

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Note that this is only one of the aspect we'll look into, we'll probably have restrictive ways of turning this feature on (I wonder if we could maintain a list of integrity hashes (see https://developer.mozilla.org/en-US/docs/Web/Security/Subresource_Integrity) that would be valid formatters.
Or we could only accept formatters as webextensions that we would review and accept.
There might be other areas we could look into to make this as safe as possible.

Comment 2

[Sebastian Zartner [:sebo]]

As Antonin Hildebrand pointed out, the Chrome DevTools implementation for the UI can be found at https://github.com/ChromeDevTools/devtools-frontend/blob/main/front_end/ui/legacy/components/object_ui/CustomPreviewComponent.ts.

In addition to avoiding security and privacy issues plus blocking code from changing the state of the page, the presentation of those formatters within the Console needs to be considered. The Chrome DevTools implementation, for example, restricts the JsonML to a few elements, namely div, span, ol, li, table, tr, and td. They do not restrict the styling, as far as I can see. Allowing arbitrary layout CSS might break the display within the Console, though. I'm not totally sure how to achieve that yet, but I assume the contain property could help here, maybe in addition with some restrictions regarding CSS properties.

Sebastian

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Hello Sebo, I wanted to check how things are going for this :)
Are you blocked on anything, would you need some help ?

Comment 4

[Sebastian Zartner [:sebo]]

Hi Nicolas! Basically I was missing general feedback from your team. I'll write you a separate email regarding this.

Sebastian

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Attachment: WIP: Bug 1734840 - [devtools] Safely run custom formatters hooks.

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

So I tinkered with what we have for eager evaluation in the console, and things seem to work well!
The idea is to reuse the makeSideEffectFreeDebugger function (devtools/server/actors/webconsole/eval-with-debugger.js#327), which already does the heavy work of checking if any stateful expression is triggered.
Then, we can use this special debugger object to wrap and call the custom formatter hooks.
You can see an early implementation in https://phabricator.services.mozilla.com/D134261

If I try to modify the window object in a custom formatter, it is not executed and we move on to the next formatter.
I did not tested that extensively, but it looks like a good lead to dig and run more test against :)

The nice part of makeSideEffectFreeDebugger is that it already has a "time out" mechanism, meaning that if a hook takes more than 100ms to run, we'll ignore it and move on (https://searchfox.org/mozilla-central/rev/7d17fd1fe9f0005a2fb19e5d53da4741b06a98ba/devtools/server/actors/webconsole/eval-with-debugger.js#345-353) We might want to be able to tweak that, so the duration could be a param of the function, in case we need just a bit more for the custom formatter.

Comment 7

[Sebastian Zartner [:sebo]]

Attachment: Bug 1734840 - Added parsing for custom formatters. r=nchevobbe

The first step of parsing custom formatters for objects is checking a website for a global array called devtoolsFormatters.
If this array exists, it is looped over to find a custom formatter that can handle the object.
In case there is one, its header and hasBody methods are executed using makeSideeffectFreeDebugger() to ensure to get a result without causing any negative side effects.
Those results are then returned together with the index of the custom formatter.
This index is used in case the custom formatter's body is expandedö. In that case the body property is also executed in a safe way and its value then returned.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/850d9b814040
Added parsing for custom formatters. r=devtools-backward-compat-reviewers,nchevobbe

Comment 9

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/850d9b814040