Remove inline styles that break plots in strict CSP setups

[martian111]

trafficstars

Support strict Content Security Policies that don't allow unsafe inline styles by:

• Removing add/deleteRelatedStyleRule from modebar and use event listeners to emulate the on hover behavior by setting style properties directly on the element, which is allowed in strict CSP environments.
• Output the main/library-wide CSS rules that are inlined when the library loads into a static CSS file so that users can include it within their applications in an acceptable manner. This allows addRelatedStyleRule calls to fail without affecting functionality.
• Provide a way to prevent addRelatedStyleRule from running when the static CSS file is already included in the app to prevent superfluous errors in console output.
• Replace inline styles from "newplotlylogo" with attribute to set the fill color directly on the elements.

Note: The dist/plotly.css file will need to be added to the release files.

Possibly fixes #2355 Plotly uses inline CSS

This pull request is the result of reviewing and testing Pull Request #6239, which did not work properly when I tested it against the v2.34.0 release tag (the most recent release at the time of testing and over 2 years since that pull request was created). It also incorporate the comments within that pull request to improve on this one.

Expected Results

• Applications using this library within an environment with strict CSP (no unsafe inline styles allowed) should now work out-of-the-box if they include/incorporate the plotly.css into the application in a way allowed by their CSP. A separate build is not required to get this fix.
  • The browser will output an error indicating that it "Refused to apply inline style because it violates the following Content Security Policy directive" followed by the site's CSP. This can be ignored since these styles should already be included by plotly.css.
  • Further, this library's addRelatedStyleRule will also output a warning message: "Cannot addRelatedStyleRule, probably due to strict CSP..."
• To prevent the errors and warnings in the console, the following can be added in a way that is accessible in the DOM by the time this library loads: <div id="plotly.js-style-global" class="no-inline-styles"></div> This will cause the addRelatedStyleRule function to not attempt to add the inline styles.
• Users without strict CSP's should not see any change in functionality nor see the aforementioned error/warning messages.

Testing

Testing this is a bit tricky to setup, but I do have an basic sandbox created that seems to demonstrate:

• The error before this fix using the plotly-basic.js file from the v2.34.0 release.
• The fix with the plotly-basic.js patched with this pull request.

CSB has some special quirks, so the CSP was relaxed to allow things to work due to CSB specifics. However, it was not set up to permit unsafe inline styles nor does it contain the sha256 hash of styles inlined by Plotly.

See comments in the ScatterChart.tsx and index.html files.

https://kl75h2.csb.app/ https://codesandbox.io/s/kl75h2

Followup Questions

1. The dom function deleteRelatedStyleRule is no longer used anywhere else in the code. It was used by only in modebar.js before this change. Should that function be deleted?
2. The dom function addStyleRule is used in two places, both of which are accounted for in this pull request (in src/registry.js and build/plotcss.js). How should we warn developers in the future who comes across this function? Add comments and/or rename it to something like addStyleRuleUnsafelyInline?

Limitations

I don't much experience with Plotly.js and have been using it only for a simple scatter chart (like the one in the CSB above). I don't have enough experience to understand if more changes are needed beyond my simple test case and the work done in Pull Request #6239. Hopefully this is still helpful to bring this library closer to supporting strict CSP's.

[birkskyum]

@martian111 , are you able to rebase this? I'm interested in knowing how the maplibre-gl.css fares in your CSP setup. The mapbox gl css issues you resolve here will if not earlier likely be resolve at the time of v3 with the removal of the deprecated mapbox traces.

[archmoj]

@martian111 Please fetch upstream/master and merge it into this branch. Thank you!

[martian111]

Yes, I'll try to rebase or merge from master when I get some time after work (possibly in the weekend).

[martian111]

@birkskyum, @archmoj, sorry for the delay getting back to this. Per your request, I've merged master into this branch. I also fixed the unit tests that had some expectation changes as a result of this pull request, as well as a bug revealed by those tests.

@birkskyum , I haven't used Plotly enough to understand the impact of mapbox vs maplibre. I did retest everything after the merge and fixes using my simple scatter chart example in the CodeSandbox referenced above.

Also, as an aside, I did find another feature of Plotly that does not work with strict CSP: Using hovertemplate to format the hover text. I have not had a chance to look into that too much since the impact of it was much less serious than the issues addressed in this pull request. I was using hovertemplate to make some hover text bold, but with strict CSP, that style would not apply. Would you agree to address that in a separate pull request if I manage to find a solution (not sure yet)?

Thanks!

[archmoj]

Thanks for the updates. This is looking great to me.

[archmoj]

I think maplibre styles looks OK. @birkskyum Please check them using

npm run strict

instead of

npm start

Thank you!

[birkskyum]

Yes, maplibre appears to be alright, it's only the mapbox traces that are affected by this PR.

[archmoj]

Yes, maplibre appears to be alright, it's only the mapbox traces that are affected by this PR.

Thanks @birkskyum for testing. @martian111 FYI - we don't have to try fixing mapbox cases as they are deprecated and users could switch to maplibre variants.

This PR is looking good in my eyes. Let's wait for others to review.

[martian111]

Yes, maplibre appears to be alright, it's only the mapbox traces that are affected by this PR.

Thanks @birkskyum for testing. @martian111 FYI - we don't have to try fixing mapbox cases as they are deprecated and users could switch to maplibre variants.

This PR is looking good in my eyes. Let's wait for others to review.

Thanks @archmoj and @birkskyum ! I am a bit confused on what you mean by "only the mapbox traces that are affected by this PR". Based on what I understand, the bulk of the changes impact the Modebar, which seems to be applicable to all (or most) Plotly trace types and not only the map trace types listed in Migrate to Maplibre in JavaScript. For example, I've been testing with the basic "scatter" trace type in the linked CodeSandbox. Can you please clarify?

I'm trying to understand this library more to see if I even need to look into the hovertemplate strict CSP issue I mentioned in my last comment. Thanks again!

[birkskyum]

Thanks @archmoj and @birkskyum ! I am a bit confused on what you mean by "only the mapbox traces that are affected by this PR". Based on what I understand, the bulk of the changes impact the Modebar, which seems to be applicable to all (or most)

I meant, when considering just mapbox and maplibre traces, it's only mapbox ones - but as you point out there are some generic changes too.

[martian111]

Thanks @archmoj and @birkskyum ! I am a bit confused on what you mean by "only the mapbox traces that are affected by this PR". Based on what I understand, the bulk of the changes impact the Modebar, which seems to be applicable to all (or most)

I meant, when considering just mapbox and maplibre traces, it's only mapbox ones - but as you point out there are some generic changes too.

I see, thanks!

[maryla-uc]

Note: The dist/plotly.css file will need to be added to the release files.

I don't see any plotly.css file anywhere. Is this expected? Should it be added to the dist/ directory?

This feature should also be documented somewhere. Right now I think this PR is the only way to learn about it.

[maryla-uc]

And note that providing the css file would also help with https://github.com/plotly/plotly.js/issues/1433 (see the last comment for example).