Fix Graphviz WASM module not loading for graph viewer

[koesie10]

It seems that when we added the CSP policy to the webview, we did not take into account that d3-graphviz uses @hpcc-js/wasm to load Graphviz as a WASM module. This commit adds 'wasm-unsafe-eval' to the CSP policy to allow this.

This also fixes a warning emitted by the module because we're not telling it we're not using WebWorkers.

Closes #2077

Checklist

• CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
• Issues have been created for any UI or other user-facing changes made by this pull request.
• [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

[adityasharad]

Two questions:

• Does wasm-eval work? I see that option is designed for Chrome apps and extensions so it may not work in Electron, but it's good to document why we can't use it.
• Is there a way to avoid wasm-unsafe-eval the same way we've avoided JS unsafe-eval, by only allowing WebAssembly that has the same nonce? https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/script-src suggests no such option but perhaps you have found something else in your investigation.

[koesie10]

Two questions:

• Does wasm-eval work? I see that option is designed for Chrome apps and extensions so it may not work in Electron, but it's good to document why we can't use it.

I tried using 'wasm-eval', but it doesn't seem to work. I found the 'wasm-unsafe-eval' here, and it seems like Chrome did change the name of the directive (see the Electron update and the Chromium commit). The spec also only lists the 'wasm-unsafe-eval' as a valid option.

• Is there a way to avoid wasm-unsafe-eval the same way we've avoided JS unsafe-eval, by only allowing WebAssembly that has the same nonce? https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/script-src suggests no such option but perhaps you have found something else in your investigation.

Unfortunately, the spec doesn't list this as a possibility:

If source-list is non-null, and does not contain a source expression which is an ASCII case-insensitive match for the string "'unsafe-eval'", and does not contain a source expression which is an ASCII case-insensitive match for the string "'wasm-unsafe-eval'", then:

1. Let violation be the result of executing § 2.4.1 Create a violation object for global, policy, and directive on global, policy, and "script-src".
2. Set violation’s resource to "wasm-eval".
   ...

There's nothing about checking the nonce, so I don't think it's possible to require a specific nonce.

[elenatanasoiu]

I've read through the links you've added (thanks for documenting it 👍 ). It's not clear to me what level of risk this change has. Would we be able to make a single exception for d3-graphviz instead of changing our CSP?

[koesie10]

I've read through the links you've added (thanks for documenting it 👍 ). It's not clear to me what level of risk this change has. Would we be able to make a single exception for d3-graphviz instead of changing our CSP?

I don't think we're able to do that, there doesn't seem to be a possibility to specify a nonce in the recommendation (see also https://github.com/WebAssembly/content-security-policy/issues/37). The recommendation does list the risks associated with enabling this option. I believe the main risk would be executing untrusted WASM modules, but for this it is necessary for some JavaScript to already be compromised. As long as all JavaScript we're loading is trusted, we shouldn't be able to load untrusted WebAssembly.

[elenatanasoiu]

As long as all JavaScript we're loading is trusted, we shouldn't be able to load untrusted WebAssembly.

Hmm, if we trusted that all our JS packages are safe, I think our particular department would be a bit obsolete.

Would you be open to considering other JS packs that don't require a CSP change?

[adityasharad]

Agree with Elena's suggestions. Or could we use a different CSP just for the graph viewer, e.g. by having a boolean allowWasmUnsafeEval parameter when constructing each CSP?

[koesie10]

Would you be open to considering other JS packs that don't require a CSP change?

The graph viewer is an unsupported part of the extension that seems to have been created as part of a hackathon (see #1111). I don't think we should invest any more time than necessary into the graph viewer. I'm not sure there are many other packages that allow us to do the same thing without rewriting large parts of the graph viewer itself.

Or could we use a different CSP just for the graph viewer, e.g. by having a boolean allowWasmUnsafeEval parameter when constructing each CSP?

I have now implemented this. Unfortunately the graph viewer is part of the results view, so we need to set this parameter on the complete results view and can't just limit it to the results view.

I think removing the graph viewer entirely would be another possibility. We explicitly state that it's unsupported and might not even reimplement it when we unify the MRVA and local results view unless we explicitly plan time for it. I don't think we have any data on how much it's being used, but we can probably ask the language teams.

[aeisenberg]

The graph viewer is an unsupported part of the extension that seems to have been created as part of a hackathon (see #1111). I don't think we should invest any more time than necessary into the graph viewer. I'm not sure there are many other packages that allow us to do the same thing without rewriting large parts of the graph viewer itself.

This is correct and I agree. This feature is not officially supported and if it is too much of a support burden, we can turn it off.

There are internal users who rely on this feature. And it would be worth understanding how often and in what capacity they use it.

[aeisenberg]

I haven't actually tried this out. The code looks good. A couple of comment suggestions and after that, I think it is fine. Thanks for working on this.

[aeisenberg]

Suggested change

	* script-src: allow the given script, using the nonce. also allow loading WebAssembly modules.
	* script-src: allow the given script, using the nonce.
	* wasm-unsafe-eval: possibly allow loading WebAssembly modules

[koesie10]

The 'wasm-unsafe-eval' string is actually part of script-src, so I've split it broken up the script-src into two parts: one for the script and one for wasm eval.