1792803 - Load JSON viewer modules from resource://devtools/

Status: RESOLVED FIXED

(DevTools :: JSON Viewer, task)

Description

[Alexandre Poirot [:ochameau]]

For now, JSON Viewer is loading the DevTools CommonJS modules via dedicated URL:
resource://devtools-client-jsonview/ (mapped to devtools/client/jsonview/)
resource://devtools-client-shared/ (mapped to devtools/client/shared/)
chrome://devtools-jsonview-styles/content/ (mapped to devtools/client/jsonview/css/
https://searchfox.org/mozilla-central/rev/91ed81b76015e49ebd55a6de5df2b034456c15e8/devtools/shared/jar.mn#18-20

These mapping were introduced in order to use contentaccessible=yes flag so that the unprivileged JSON plaintext document is able to load these modules via script tag created by Require.js.

This pattern actually blocks to convertion of all DevTools modules to ES Modules (bug 1525652).
That's because JSON Viewer uses modules from devtools/shared.
But these modules are meant to switch to absolute URL for pulling imported modules (bug 1789835). When doing so, they will use the "regular" DevTools URL, which isn't made accessible to unprivileged documents. URLs like resource://devtools/shared/event-emitter.js.
The issue in doing so is that when loaded from JSONViewer, it will fail because it won't be able to load resource://devtools/shared/event-emitter.js and instead only be able to load resource://devtools-client-shared/event-emitter.js.

We might be able to hack something in Require.js config to somehow map URLs back to JSONViewer specific resource URLs.
But this will no longer be possible when we move to ES Modules as import statements will be the standard native API and we don't support custom loaders yet to introduce some handcrafted URL/path mapping.

So that it would be nice to revisit how JSONViewer is implemented in order to make it less special and instead, make it load the regular resource://devtools/ URLs.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1792803 - [devtools] Load JSON viewer modules from resource://devtools/.

Comment 2

[Alexandre Poirot [:ochameau]]

try is green:
https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=RgT4ZuoGT-yASVx8KF8eXQ.0&revision=3da070b4c3a71726412e2dab23073865eb70a73b

I've added a test to assert that the JSON viewer isn't enabled for iframes.
It starts failing if we remove the check against request.loadInfo?.isTopLevelLoad in the sniffer:
https://searchfox.org/mozilla-central/rev/91ed81b76015e49ebd55a6de5df2b034456c15e8/devtools/client/jsonview/Sniffer.sys.mjs#43
We might actually repeat this check from the converter as well?

JSON Viewer has been originaly introduced in bug 1132203.
But various tweaks have been introduced over time to iron it against possible trigger of the privileged code from content code.
Bug 1295309, bug 1297361, bug 1333210.

With this patch I kind to go backward regarding what has been done in these old bugs.
But I'm aligning with what has been done for PDF.js:
https://searchfox.org/mozilla-central/rev/91ed81b76015e49ebd55a6de5df2b034456c15e8/toolkit/components/pdfjs/content/PdfStreamConverter.jsm#1286-1298

Is there any reason why we wouldn't be able to use the same trick as PDF.js? It sounds like we are having similar constraints?

Comment 3

[Alexandre Poirot [:ochameau]]

Gijs, Any feedback about https://phabricator.services.mozilla.com/D158318?

Doing something along these lines, i.e. allow loading resource://devtools/ files for the JSON Viewer code running in the plaintext html document created to display the JSON, would help unblock migrating DevTools to ES Modules.
I'm open to alternatives, but here, I mostly tried to align to PDF.js patterns.

If that looks ok to you, I would like to followup and align some more and also have a real html file, like PDF.js.
And I wish we could stop manipulating the document from the converter and dispatch/listen for DOM events (if there is better practice, let me know).

Comment 4

[:Gijs (out for now; he/him)]

(In reply to Alexandre Poirot [:ochameau] from comment #3)

Sincere apologies for the delay here.

Gijs, Any feedback about https://phabricator.services.mozilla.com/D158318?

Using the JSON viewer's principal as the owner for the http request seems... potentially not great. I commented inline.

Doing something along these lines, i.e. allow loading resource://devtools/ files for the JSON Viewer code running in the plaintext html document created to display the JSON, would help unblock migrating DevTools to ES Modules.

I'm not sure I understand what this means, or why it unblocks the migration? Why do we need the request for the JSON to happen with the JSON viewer's resource principal set as the owner? What's the relation to the migration?

I'm open to alternatives, but here, I mostly tried to align to PDF.js patterns.

This just makes me nervous that what PDF.js is doing is also unsafe/incorrect. 😭

I'm probably wrong. I guess owner is different from the loading/triggering principal. But it's still... confusing. To say the least. Christoph, can you help un-confuse me?

And I wish we could stop manipulating the document from the converter and dispatch/listen for DOM events (if there is better practice, let me know).

I'm afraid I don't really follow this question, either. I don't see anything relating to this in the patch so I'm not sure what this is about.

Comment 5

[Alexandre Poirot [:ochameau]]

(In reply to :Gijs (he/him) from comment #4)

Doing something along these lines, i.e. allow loading resource://devtools/ files for the JSON Viewer code running in the plaintext html document created to display the JSON, would help unblock migrating DevTools to ES Modules.

I'm not sure I understand what this means, or why it unblocks the migration? Why do we need the request for the JSON to happen with the JSON viewer's resource principal set as the owner? What's the relation to the migration?

The challenges regarding the migration from commonjs to ESM is that:

• we have to use absolute URIs for imports, like resource://devtools/module.js (instead of absolute require path, like devtools/module) (or relative path, but those are fine here)
• we can't hack the ESM loader to remap imported URIs to some other place (which we do a lot today in devtools in various place. And that's what I'm chasing here and other bugs).

Then when you combine this with JSONView specialties it is impossible to migrate as-is.
JSONViewer load modules via two custom resource:// mappings:
resource://devtools-client-jsonview/ which maps to devtools/client/jsonview/
resource://devtools-client-shared/ which maps to devtools/client/shared/
These custom mappings were made to set contentaccessible=yes on these two, so that these complex code hacking the JSON document, which is unprivileged, can load these two URIs.
My issue is that JSONViewer load modules from devtools/client/shared/, which are going to use resource://devtools/ (the regular one, without contentaccessible=yes).
At the end of the day, because of ESM, we ought to use a unique resource:// URL mapping for everything.

Here my call for action was to tweak the privileges of JSONViewer code to be able to load the regular resource://devtools/ URIs.
I went for this, because I considered it bad to expose any internal modules of devtools to content.
As of today... webpages can fetch all files from resource://devtools-client-jsonview/ and resource://devtools-client-shared/...

But if that's fine exposing browser files to the web, I can do the other way around and set contentaccessible=yes on the resource://devtools/ mapping. And get rid of the jsonview-specific mappings.

And I wish we could stop manipulating the document from the converter and dispatch/listen for DOM events (if there is better practice, let me know).

I'm afraid I don't really follow this question, either. I don't see anything relating to this in the patch so I'm not sure what this is about.

Sorry, that was yet another topic for followup. I'll raise this question later if needed once we figure out the URIs issue.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Freddy volunteered to take a look here and he and I will coordinate.

Comment 7

[Frederik Braun [:freddy]]

I think we should avoid making fetches from a resource:// principal.

Do I correctly remember that contentaccessible= is per-jar and not per file?
As long as the resources in e.g., resource://devtools-client-jsonview/ are static assets and source code, I would much rather suggest we make them web-accessible, though file granularity would be preferable..

We could also explore a file-specific exception in our caps (or dom/security or wherever this check lives?) and allow-list requests for some of those JS files specifically? But that might be a bit too hacky.

Comment 8

[Alexandre Poirot [:ochameau]]

(In reply to Frederik Braun [:freddy] from comment #7)

I think we should avoid making fetches from a resource:// principal.

Do I correctly remember that contentaccessible= is per-jar and not per file?

AFAIK for devtools it is per resource URL "domain" and then mapped to a folder in the omni.jar:
https://searchfox.org/mozilla-central/rev/b4150d1c6fae0c51c522df2d2c939cf5ad331d4c/devtools/shared/jar.mn#17-20
In %modules/devtools/ should be present all files mentioned in moz.build via DevToolsModules.

As long as the resources in e.g., resource://devtools-client-jsonview/ are static assets and source code,

JSONView pulls various JS Modules and CSS files from devtools/client/jsonview/ and devtools/client/shared/.
But if we allow resource://devtools/ to JSONView, it will be the whole devtools/ folder, but only files mentioned via DevToolsModules.

I would much rather suggest we make them web-accessible, though file granularity would be preferable..

Note that maintaining the precise list of all nested dependencies of JSONViewer will be something quite hard to maintain.
So it would be nice to assume that JSONViewer can load any module from resource://devtools/, as any other code of devtools.

Should we revisit more broadly how this feature has been implemented?
It looks quite hacky to replace the plain text html document with the JSON viewer document.

• Would it help to load the JSONviewer from an iframe within the JSON document instead of replacing the JSON document?
  I imagine it will still be disallowed to load resource:// within an <iframe>, but may be that can be made possible without using owner/triggeringPrincipal, nor contentaccessible?

• Is there a way to display some privileged UI/document on top of the JSON document without doing it from the content document?
  In the inspector, we are using insertAnonymousContent, but it is still limited to the principal of the page...

• Could we possibly redirect to a resource:// URI whose original/displayed URI would be the JSON one?

Comment 9

[Oriol Brufau [:Oriol]]

If you don't like the approach of replacing the JSON document with the JSON viewer, then consider other approaches together with the PDF viewer, since it uses the same approach.

BTW, the PDF viewer loads scripts from resource://pdf.js/ just like the JSON viewer loads scripts from resource://devtools-client-jsonview/, so again changes should probably be applied to both.

Comment 10

[Alexandre Poirot [:ochameau]]

(In reply to Oriol Brufau [:Oriol] from comment #9)

If you don't like the approach of replacing the JSON document with the JSON viewer, then consider other approaches together with the PDF viewer, since it uses the same approach.

BTW, the PDF viewer loads scripts from resource://pdf.js/ just like the JSON viewer loads scripts from resource://devtools-client-jsonview/, so again changes should probably be applied to both.

Sure. If we ultimately confirm that using owner is wrong, we should revisit PDF.js implementation.

But note that we might have different constraints between DevTools and PDF.js.

PDF.js seems to have a limited set of files to be loaded. It seems constrained to:
https://searchfox.org/mozilla-central/source/toolkit/components/pdfjs/content/web
So that it is easier to review and limit files we expose to content via contentaccessible flag.
Whereas, as said in comment 8, the exact set of files loaded by JSONViewer is more complex to define and review.

Comment 12

[Alexandre Poirot [:ochameau]]

The topic of contentaccessible has been revived by emilio while working on bug 1824886.
I've opened bug 1825390 to see if we could simply toggle contentaccessible=yes to the whole devtools folder exposed via resource://devtools,
i.e this mapping:
https://searchfox.org/mozilla-central/rev/0907529ff72c456ddb47839f5f7ba16291f28dce/devtools/shared/jar.mn#17

%   resource devtools %modules/devtools/ 

And then drop the mappings which were specific to JSONViewer:
https://searchfox.org/mozilla-central/rev/0907529ff72c456ddb47839f5f7ba16291f28dce/devtools/shared/jar.mn#18-20

Comment 13

[:Gijs (out for now; he/him)]

(In reply to Alexandre Poirot [:ochameau] from comment #12)

The topic of contentaccessible has been revived by emilio while working on bug 1824886.
I've opened bug 1825390 to see if we could simply toggle contentaccessible=yes to the whole devtools folder exposed via resource://devtools,
i.e this mapping:
https://searchfox.org/mozilla-central/rev/0907529ff72c456ddb47839f5f7ba16291f28dce/devtools/shared/jar.mn#17

%   resource devtools %modules/devtools/ 

And then drop the mappings which were specific to JSONViewer:
https://searchfox.org/mozilla-central/rev/0907529ff72c456ddb47839f5f7ba16291f28dce/devtools/shared/jar.mn#18-20

This type of thing has previously hurt us, e.g. bug 1432358 , so please be cautious about exposing a bunch of stuff to the web.

Comment 14

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1792803 - [devtools] Stop exposing devtools resource to content to implement JSON Viewer.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/90b2d34eaeb1
[devtools] Load JSON viewer modules from resource://devtools/. r=devtools-reviewers,Gijs,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/46b5603cd2b8
[devtools] Stop exposing devtools resource to content to implement JSON Viewer. r=devtools-reviewers,nchevobbe

Comment 16

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/90b2d34eaeb1
https://hg.mozilla.org/mozilla-central/rev/46b5603cd2b8