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 allowsaddRelatedStyleRule calls to fail without affecting functionality.
Provide a way to preventaddRelatedStyleRule 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: Thedist/plotly.css file will need to be added to the release files.

Possiblyfixes#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 theplotly.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 byplotly.css.
- Further, this library'saddRelatedStyleRule 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></div>
This will cause theaddRelatedStyleRule 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 theplotly-basic.js file from the v2.34.0 release.
The fix with theplotly-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 theScatterChart.tsx andindex.html files.

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

Followup Questions

The dom functiondeleteRelatedStyleRule is no longer used anywhere else in the code. It was used by only inmodebar.js before this change. Should that function be deleted?
The dom functionaddStyleRule is used in two places, both of which are accounted for in this pull request (insrc/registry.js andbuild/plotcss.js). How should we warn developers in the future who comes across this function? Add comments and/or rename it to something likeaddStyleRuleUnsafelyInline?

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.

martian111 added2 commits

August 18, 2024 05:18

Remove inline styles that break plots in strict CSP setups

46ea43c

Support strict Content Security Policies that don't allow unsafe inlinestyles 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 releasefiles.Fixesplotly#2355 Plotly uses inline CSS

Add draftlogs for Pull Requestplotly#7109

c0503e2

martian111 mentioned this pull request

Aug 18, 2024

Fix CSP Strict Style Compatibility#6239

Closed

gvwilson requested a review fromarchmoj

August 21, 2024 15:33

gvwilson assignedarchmoj

Aug 21, 2024

gvwilson added community

community contribution

considered for next cycle

fixfixes something broken labels

Aug 21, 2024

Copy link

Contributor

birkskyum commentedSep 13, 2024

@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 thedeprecated mapbox traces.

Copy link

Contributor

archmoj commentedSep 16, 2024

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

archmoj requested a review fromemilykl

September 17, 2024 20:36

Copy link

ContributorAuthor

martian111 commentedSep 19, 2024

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

archmoj added this to thev3.0.0 milestone

Oct 2, 2024

martian111 added2 commits

October 20, 2024 02:07

Merge branch 'master' into csp-no-inline-style

3f97ec6

Fix unit tests and bug applying bgcolor with relayout

a2eeaf0

Fixed unit tests by updating the expectations as appropriate to matchthe changed logic in Pull Requestplotly#7109. This revealed a bug wherebackground color changes applied using `Plotly.relayout()` were missed.This bug was also fixed in this change in order to get all unit tests,as updated, to pass.

Copy link

ContributorAuthor

martian111 commentedOct 20, 2024

@birkskyum,@archmoj, sorry for the delay getting back to this. Per your request, I've mergedmaster 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: Usinghovertemplate 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 usinghovertemplate 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 requested a review fromalexcjohnson

October 20, 2024 16:48

Copy link

Contributor

archmoj commentedOct 20, 2024

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

archmoj requested review fromgvwilson andndrezn

October 20, 2024 16:50

Copy link

Contributor

archmoj commentedOct 20, 2024

I thinkmaplibre styles looks OK.
@birkskyum Please check them using

npm run strict

instead of

npm start

Thank you!

Copy link

Contributor

birkskyum commentedOct 20, 2024•
edited
Loading

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

Copy link

Contributor

archmoj commentedOct 20, 2024

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.

archmoj approved these changes

Oct 20, 2024

View reviewed changes

Copy link

ContributorAuthor

martian111 commentedOct 20, 2024•
edited
Loading

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 inMigrate 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 thehovertemplate strict CSP issue I mentioned in my last comment. Thanks again!

Copy link

Contributor

birkskyum commentedOct 20, 2024

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.

Copy link

ContributorAuthor

martian111 commentedOct 20, 2024

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!