Closed Bug 1734840 Opened 3 years ago Closed 2 years ago

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

Categories

(DevTools :: Framework, task)

task

Tracking

(firefox104 fixed)

RESOLVED FIXED
104 Branch
Tracking Status
firefox104 --- fixed

People

(Reporter: nchevobbe, Assigned: sebo)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

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

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.

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

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

Flags: needinfo?(sebastianzartner)

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

Sebastian

Flags: needinfo?(sebastianzartner)

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.

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.

Assignee: nobody → sebastianzartner
Status: NEW → ASSIGNED
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/850d9b814040
Added parsing for custom formatters. r=devtools-backward-compat-reviewers,nchevobbe
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 104 Branch
Blocks: 1800040
Attachment #9256117 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: