Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix CSP Strict Style Compatibility #6239

Conversation

shaurya-sisodia
Copy link

Fixes #2355 Plotly uses inline CSS

  • Only piece of code violating the CSP strict style rule is the addStyleRule function which is appending a dynamic stylesheet to the head. In all other cases, the plotly.js is setting up a dynamic style using CSP-compliant methods.
  • In the case of a CSP compliant build, we will create a static CSS stylesheet in a CSS file that applications can include on their end.
  • Providing switch in the build process to enable CSP strict style build and providing a path for static CSS stylesheet relative to dist folder.
  • Creating a static CSS file at command line provided path defaulting to build folder with name plot-csp.css

Fixes plotly#2355 Plotly uses inline CSS

* Providing switch in build process to enable CSP strict style build
* Creating a static CSS file at command line provided path defaulting to build folder with name plot-csp.css
@archmoj archmoj added community community contribution status: reviewable labels Jun 22, 2022
@alexcjohnson
Copy link
Collaborator

@shaurya-sisodia thanks for the investigation and the PR! It's great to know that only addStyleRule is problematic for strict CSP - from the earlier discussion I was worried that d3 selection.style was also a problem, which would have made this MUCH harder to resolve.

So given that, I wonder if we can just inline all the styles we need? There aren't that many, and you've already figured out one of the most annoying parts, the hover pseudo-selector. That would avoid having two separate builds (which is more to test & maintain) as well as requiring strict users to remember the stylesheet - easy to forget as it doesn't do all that much until you start interacting with the graph, and it seems like including an external stylesheet brings its own headaches in a strict CSP environment.

@shaurya-sisodia
Copy link
Author

Hi @alexcjohnson

Thanks for the reply.

If we do not want to go by two builds approach. We can instead use adoptedStyleSheets and 'replaceSync' to update styling in csp compliant way for the whole document. Rather than creating a static stylesheet for a special build.

For browers i.e. IE and Safari that don't support replaceSync we can update styles directly on the HTMLElements like we are doing for modebar's dynamic styles in current fix.

@sesa529917
Copy link

Lack of support for strict CSP support is a blocker for the organization I work for.
Our corporate security systems simply won't allow us to run with unsafe-inline.
This PR resolves the issue. Please consider merging it or implementing similar strict CSP support in the library.

@archmoj
Copy link
Contributor

archmoj commented May 27, 2024

@shaurya-sisodia Thanks very much for the PR.
Would you please fetch upstream/master and merge it into this branch?
Also I think we don't want to have a separate bundle for this.
Instead I think this should be fixed in the main bundle.

@archmoj archmoj added the bug something broken label May 27, 2024
@archmoj archmoj mentioned this pull request Jul 4, 2024
@archmoj archmoj added this to the v3.0.0 milestone Jul 8, 2024
@gvwilson gvwilson assigned gvwilson and unassigned gvwilson Jul 26, 2024
@gvwilson gvwilson changed the title Fixed CSP Strict Style Compatibility Fix CSP Strict Style Compatibility Aug 8, 2024
@gvwilson gvwilson added fix fixes something broken P2 considered for next cycle and removed bug something broken labels Aug 8, 2024
@martian111
Copy link
Contributor

Hi @archmoj , I created Pull Request #7109 to address the issue I found above and to fix a few other things I found while testing this pull request. I also incorporated the fix into the main build and branched off of the latest master per your comments above. Seeing no activity from the submitter's account of this pull request for a long time now, I submitted the new pull request in hopes that it will be useful for everyone else.

@gvwilson
Copy link
Contributor

closing in favor of #7109 - thank you @shaurya-sisodia and please accept our apologies for taking so long to get to this.

@gvwilson gvwilson closed this Aug 21, 2024
@archmoj archmoj removed this from the v3.0.0 milestone Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community community contribution fix fixes something broken P2 considered for next cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plotly uses inline CSS
6 participants