Skip to content
This repository has been archived by the owner on Jun 4, 2024. It is now read-only.

Add URL scheme for loading resources from an extension #3413

Closed
wants to merge 4 commits into from

Conversation

rgov
Copy link
Contributor

@rgov rgov commented May 20, 2022

Dabbling with foxglove/community#277, which is about making it so that extensions can load resources from their bundles.

This adds an x-foxglove-extension-rsrc: URL scheme which can be used to reference resources. The URL path begins with the ID of the extension, followed by the path to the resource within the bundle, as in:

x-foxglove-extension-rsrc://foxglove.studio-extension-turtlesim/dist/turtle.svg

Extensions can configure Webpack to automatically add this prefix to resources using:

config.output.publicPath =
    "x-foxglove-extension-rsrc://foxglove.studio-extension-turtlesim/dist/";

and then enable Asset Modules with

config.module.rules.push({
     test: /\.(png|svg|jpg|jpeg|gif)$/i,
     type: "asset/resource",
});

Then the extension can proceed with things like import TurtleIcon from "./turtle.svg" and it works as expected.


I don't really know much about Electron and find the extension loader code a bit confusing.

I tested this with loading an SVG resource from my extension, but I didn't try something with an additional level of indirection like a relative include from a CSS file. (Edit: Those work fine too.)

This configures the scheme to be allowed by the Content Security Policy for all resource types; I don't know why using protocol.registerSchemesAsPrivileged to disable CSP didn't work.

Fixed. ☢️ There is probably a security vulnerability with resources that are symlinks outside of an extension's bundle. I did an experiment with this by replacing an image file with a symlink into /tmp, and it still loaded my image.

I make an attempt at avoiding path traversal by making sure that the resulting file is still within the bundle, even after resolving symlinks.

@rgov
Copy link
Contributor Author

rgov commented May 20, 2022

Registering the URL scheme as "standard" fixes the relative-CSS-import case but to be honest it is pretty unusual to encounter it, since normally style-loader would resolve those imports at compile-time.

This switch also has the effect of making the extension ID the "host" of the URL which simplifies some of the parsing and also prevents traversal between extensions.

@rgov
Copy link
Contributor Author

rgov commented May 20, 2022

In the future this URL scheme might also lend itself to injecting <script src="x-foxglove-extension-rsrc://foxglove.studio-extension-turtlesim/dist/extension.js"></script> into the DOM rather than having the renderer request the contents of that file and evaluate it with new Function(). This would probably simplify the extension loading process so that the main process just provides an API listing the installed extensions to the renderer.

In that case Webpack's default publicPath = "auto" mode might just automatically determine how to load relative resources.

@foxymiles
Copy link
Contributor

Thanks very much for putting this together. Did you also have to set up any typescript definitions for it to understand asset imports like import TurtleIcon from "./turtle.svg"?

Do you have a minimal working example of a panel using this new scheme to load resources?

@rgov
Copy link
Contributor Author

rgov commented May 24, 2022

Did you also have to set up any typescript definitions for it to understand asset imports like import TurtleIcon from "./turtle.svg"?

Yes, though I don't fully understand them. I created src/typings/extensions.d.ts that contains

declare module "*.svg" {
    const content: string;
    export default content;
}

I believe this is related to how Webpack translates the import statement into a string constant that contains the URL of the resource for asset/resource.

Do you have a minimal working example of a panel using this new scheme to load resources?

I do not have a minimal example right now, I can create one for you today.

@foxymiles
Copy link
Contributor

Thanks that would be very helpful.

I think your approach here makes sense I just want to make the path for other panel authors to take advantage of this as straightforward as possible.

@rgov
Copy link
Contributor Author

rgov commented May 24, 2022

Here's the example: https://github.com/rgov/studio-extension-resource-test

I think it would be best to have publicPath be set, in the @foxglove/fox base Webpack config, to a magic constant like @FOXGLOVE_EXTENSION_PATH_PREFIX@, and then substitute in the correct extension identifier before executing the plugin.

@foxymiles
Copy link
Contributor

Thanks very much for the example. I'll take a look at this today.

@foxymiles
Copy link
Contributor

So I think your general approach here makes sense but I'd like to give @defunctzombie a chance to weigh in on this and he's on leave for two weeks or so.

Is it ok if we let this wait until he's back and has had a chance to give it a look himself?

@jtbandes
Copy link
Member

Something we'll need to think about is how to support extensions in the web version of Studio. While the web build currently doesn't support them, we have previously experimented with extensions in the web build and I think it would be something we'd like to support in the future. So we may need to provide async resource loading as an extension API rather than using a custom protocol, which we can't hook into in the browser.

@rgov
Copy link
Contributor Author

rgov commented May 24, 2022

I think this still works when loading in a browser and extension resources are being served over HTTP.

Instead of substituting @FOXGLOVE_EXTENSION_PATH_PREFIX@ with x-foxglove-extension-rsrc://foxglove.studio-extension-turtlesim/... at load time, you would just substitute /extensions/foxglove.studio-extension-turtlesim/... and it would simply fetch the files from the web server instead.

I think the Content Security Policy would remain the same, since the extensions would be loaded from the same origin as the app itself.

Again, if you switched to embedding a <script> tag instead of constructing a Function (see comment), then perhaps Webpack's automatic publicPath discovery would work transparently and you wouldn't have to do that substitution trick.

> Registering a scheme as standard allows relative and absolute resources to
  be resolved correctly when served.

If, for example, a CSS file does a relative import, the imported path can be
resolved within the extension bundle.
If an extension is built with a special Webpack publicPath prefix value, the
prefix is substituted with the extension's base URL. The extension resolves
relative paths at runtime according to this base URL.
@rgov
Copy link
Contributor Author

rgov commented Jun 5, 2022

Added the change for configuring the publicPath at load time, and submitted the relevant change to the @foxglove/fox Webpack config in foxglove/create-foxglove-extension#50.

The alternative as discussed in this comment is to let Webpack do automatic publicPath discovery by using a <script> tag or maybe setting import.meta.url when constructing the Function instance.

@defunctzombie
Copy link
Contributor

Thanks for exploring this solution! Here are some thoughts to help us decide on what solution to adopt.

How hard would it be to charge our minds in the future? I.e. should we decide on some other approach to loading assets or even alternative bundlers or strategies for bundling? I think it's important to understand how we can evolve the approach here should we choose to do so and not break existing extensions. This could be as simple as indicating an extension manifest version or some such in the package file.

Does this approach tie us down further to a specific bundler? Right now we do have the somewhat nice property that you could use any bundler so long as you output a single .js file and package manifest for your extension. webpack is not a strict requirement.

As @jtbandes noted, it is important that extensions have some path towards working in a web environment so I'm happy to see there is an approach here with string replacement.

How does this approach work with url(...) items in css files which are then imported? We don't necessarily have to support that but maybe it just works with this approach?

I've personally never been that much a fan of the "import a file as if its source code" approach that webpack has made popular with custom file loaders. I've seen and experienced the confusion with whether the resource is inlined and the import is base64 or the import is a url. The alternative I've had in mind was to expose some loader instance to the extension and the extension could invoke the loader with a string path to the resource: await loader.load('/path/to/foo.svg'). The con here is that the user then has to take the response (a Uint8Array maybe) and use that however they want, the pro is that it works without any extra webpack config and also can support any static file the user wants to bundle sort of like loading a file from their static path.

Here is how vscode handles this: https://code.visualstudio.com/api/extension-guides/webview#loading-local-content which returns a url not unlike the custom url you've proposed the main difference being it happens separate from any bundler.

Here is how the same is done for a chrome extension: https://stackoverflow.com/questions/7644809/get-local-file-inside-the-extension-folder-in-chrome

I think we're on the right path here with a custom url scheme for desktop (and something else for eventual web), but whether that should happen via webpack automagically or whether we should provide a "get url" like function to the extension to resolve a relative path into a full url is what we need to decide.

Read over the two methods I linked above - or maybe see if you can find other prior art besides chrome or vscode - and see what you think about those approaches vs the string replacement with webpack path prefixes and imports.

@rgov
Copy link
Contributor Author

rgov commented Jun 6, 2022

How does this approach work with url(...) items in css files which are then imported? We don't necessarily have to support that but maybe it just works with this approach?

In my testing it works because these are resolved relative to the CSS resource's URL.

How hard would it be to charge our minds in the future?

If you expose the x-foxglove-extension-rsrc:// URL scheme and/or @FOXGLOVE_EXTENSION_PATH_PREFIX@ as API, you would have to support it until you decide to break old extensions.

It does seem to me that at least with the use of the magic value substitution, it gives you flexibility to change the implementation details, for instance the name of the URL scheme or the format of the extension IDs, and to work with both the desktop app and browser, which require different methods.

I am happy to follow the lead of VS Code or Chrome with an API function for this. Both appear give an absolute URL from a relative resource path.

I would at least insist that it to be very simple (read: zero config) when using the @foxglove/fox extension template, because mucking with how assets are loaded is a distraction from implementing extensions.

That raises the question of how to make Webpack invoke this function on each resource path. This isn't supported by publicPath. Let me think about it a little bit.

Does this approach tie us down further to a specific bundler?

I don't think any of the solutions discussed here are specific to Webpack, we just provide the default Webpack configuration to make it transparent for what I assume is the vast majority of extensions that will use the @foxglove/fox template.

The alternative I've had in mind was to expose some loader instance to the extension and the extension could invoke the loader with a string path to the resource: await loader.load('/path/to/foo.svg').

I don't like the idea of getting Uint8Array for a resource's contents because it diverges from the normal behavior of how browsers load resources. Why should loading a PNG from my extension bundle be any different than loading one from the web? It seems it would be difficult to do simple tasks, hurt performance to encode the bytes to Base64, and consume a lot of RAM.

@rgov
Copy link
Contributor Author

rgov commented Jun 6, 2022

You could expose some globally-scoped foxglove.extension context object, defined something like this:

import {join as pathJoin} from "path";

const foxglove = { extension: {} };
foxglove.extension.baseURL = "x-foxglove-extension-rsrc://foxglove.studio-extension-turtlesim/";
foxglove.extension.getURL = (path: string) => pathJoin(foxglove.extension.baseURL, path);

You still have the limitation with Webpack that you cannot provide a function to dynamically initialize publicPath at runtime. You could require that every extension that wants to use Webpack starts with:

declare var __webpack_public_path__: string; 
__webpack_public_path__ = foxglove.extension.baseURL;

But note, you can't inject it at load time because the code has been minified and the variable name is different. You could provide a Webpack plugin to do it (headache).

The magic value substitution trick still feels to me like the least bad option. Realistically, the majority of extensions will use the @foxglove/fox template and its Webpack config; those who don't can use the foxglove.extension API calls.

If a better method with Webpack becomes available in the future, updating the base config should make it a seamless transition for most existing extensions, requiring a rebuild but hopefully no more than that. Studio could detect the presence of the magic value in the code when loading an extension and warn about its deprecation for a transition period before support is removed.

@amacneil
Copy link
Contributor

amacneil commented Aug 5, 2022

Discussed live - this approach will not work with how we load extensions in web.

Current path forward is to tackle the specific bundling issues described in foxglove/community#277

@amacneil amacneil closed this Aug 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants