Allow JSDOMRunner to load external resources from specified URLs

[deej-io]

For a bit of fun, I'm doing some experimentation with Server-Side rendering technologies to serve and update MML documents - specifically using Go and HTMX . I have a repo with a scrappy, WIP demo here: https://github.com/deej-io/mml-htmx-demo.

The first thing I had to do was update the JSDOMRunner code to allow it to load external resources. Of course this should be disabled by default, as we can't allow people to run arbitrary JavaScript on the MML server, but I thought it might good to have a way of specify allowed resource URLs that can be fetched the runner (in the above demo's case the HTMX library from unpkg).

This is just a WIP to get the discussion going as I'm not happy with the API as it currently stands as we have to thread the URLs through a number of constructors. I am perhaps wondering if a RunnerFactoryFactory approach would be a bit nicer.

Regardless, I wanted to raise this to get your thoughts and make sure there is appetite for this change before I put any more effort into it.

---

What kind of changes does your PR introduce? (check at least one)

• Bugfix
• Feature
• Refactor
• Tests
• Other, please describe:

Does your PR introduce a breaking change? (check one)

• Yes
• No

If yes, please describe its impact and migration path for existing applications:

Does your PR fulfill the following requirements?

• All tests are passing

[MarcusLongmuir]

Hey. HTMX + MML is a cool idea, and making this configurable makes sense anyway.

The resource loading is disabled because when we were doing initial explorations of doing live editing and the JSDOM instance was being reloaded rapidly it would cause the referenced <scripts> to be repeated retrieved without caching which could be substantial.

The ability to run a remote <script src="https://..."> vs include the same JavaScript directly isn't really a security concern as you could always just fetch the script contents and eval it anyway.

As a result I think making it configurable is a good idea. It can only really apply to the JSDOMRunner class though so I'd propose that rather than drilling the options all the way through the EditableNetworkedDOM instead if non-default configuration of the JSDOMRunner is required (to pass this config to allow resources) then the factory functions to use for ObservableDOM and DOMRunnerFactory are inlined e.g.:

const document = new EditableNetworkedDOM(
  `<script src="http://example.com/script.js"/>`,
  (
    observableDOMParameters: ObservableDOMParameters,
    callback: (message: ObservableDOMMessage, observableDOM: ObservableDOMInterface) => void,
  ): ObservableDOMInterface => {
    return new ObservableDOM(
      observableDOMParameters,
      callback,
      (
        htmlPath: string,
        htmlContents: string,
        params: object,
        callback: (mutationList: DOMRunnerMessage) => void,
      ): DOMRunnerInterface => {
        return new JSDOMRunner(htmlPath, htmlContents, params, callback, {
          // Configure the JSDOMRunner using this optional config
          allowResourceLoading: true, // Or a an array of allowed URLs
        });
      },
    );
  },
);

I appreciate that this is relatively verbose, but it's keeping the configuration true as this is a config for only one of the potential runners.

[deej-io]

Thank you for the feedback @MarcusLongmuir - that all makes complete sense.

I've push those changes and checked it is all still working in my demo app 👍 .

I'm loving HTMX right now and it seems to work quite well with MML, but unfortunately it requires a fix to JSDOM, which may not be merged for a while: jsdom/jsdom#3719.

However, it should be as simple as overriding the dependency in package.json of the consuming project, if you did want to use it:

{
  "dependencies": {
    "@mml-io/observable-dom": "*",
  },
  "overrides": {
    "@mml-io/observable-dom": {
      "jsdom": "deej-io/jsdom",
    }
  }
}

[MarcusLongmuir]

Suggested change

	const allow = this.urls.every(allowedURL => {
	const allow = this.urls.some(allowedURL => {

I think this is meant to be "if any of the rules / addresses allow this resource", right?

[deej-io]

Ah yes, thanks.

That's on me only testing with one resource URL 🤦‍♂️.

I'll add some unit tests for it.

[MarcusLongmuir]

Thanks for this, @deej-io 👍