-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Plotly uses inline CSS #2355
Comments
It's not entirely clear to me If it's just using javascript to create and fill a |
Risks: https://stackoverflow.com/a/31759553/1175508
Being suitable for a strict CSP environment is worthwhile. If not, Plotly often can’t be used in any such security-sensitive/modern app. Any To satisfy the no inline styling requirement, you need to neither apply CSS inline in attributes nor in If library consumers not having to add a stylesheet link for Plotly is a requirement, and the styles themselves are required too, then alternatively Plotly should be able to manipulate the DOM to add a stylesheet |
That's the one I'm worried about - if that means that we can't add
then there's simply no possibility of plotly satisfying this requirement, ever. Many of these style attributes encode data values so it would not be possible to refer to some (static) style sheet to apply this styling. We could get rid of |
AFAIK you aren’t restricted to inline CSS even in SVG objects. See https://www.w3.org/wiki/SVG_Security#SVG_as_document_embedded_with_.3Ciframe.3E.2C_.3Cembed.3E_or_.3Cobject.3E |
I can't tell from that comment what you're proposing we should do? |
I assumed it was clear, the solution would be to use external CSS as a subresource, either in the HTML document or the SVG itself. Added advantage is that optimization on the CSS will be easier (e.g. compression). |
Right, but that's exactly what, as I said above, we cannot do:
I suppose theoretically one could 1) figure out all the styles (colors, line widths, etc - could be many thousands of distinct values) needed for the data in your plot, 2) proxy that through some server to generate a style sheet, 3) apply classes instead of inline styles... but that would be a huge infrastructure cost (we're no longer a pure javascript library but it needs to be connected to a server) and performance cost, and frankly I don't see how that would be more secure - if the concern is that users might find some way to meddle with the javascript to generate malicious styles, this potential system seems like it would offer more, not fewer, opportunities for meddling. |
I recognize the issue is challenging but I think you’re jumping to conclusions here. First of all CSS is theoretically meant be used for presentational details, not the semantics of the image. So it shouldn’t be necessary to apply CSS to get usable plots. But on to a more practical perspective ... The solution you picture is of course not feasible, and it’s not one I propose. I only propose to serve static CSS as subresource, and only as needed. For dynamic styling, those presentational parameters you identified so far can easily be replaced with equivalent SVG element attributes (color, stroke width). |
Ah there we go, a concrete suggestion 👍Yes, if attributes are allowed we can consider changing all styles to attributes; I'm not sure if that would cover everything we need, but it should cover all of the data-linked attributes, ie those that cannot be handled by a static external CSS. This would be a fairly large project; it would also make plotly.js deployment more involved, particularly for behind-the-firewall apps that require there to be no links to non-local resources, we'd need the location of the external CSS resource to be configurable. |
As discussed in plotly/dash-core-components#752 - it's apparently not the case that all inline styles are disallowed with strict CSP; just those set by providing the entire style attribute as a string. I don't know what D3 does, but if it accesses the style as an object as React apparently does, we may not need to convert everything to presentation attributes after all. We'd still need to get rid of This could potentially be a build variant so that you'd only need the separate CSS with the CSP build, other users could continue using the single JS bundle. |
FYI, injecting CSS that violates our CSP is a deal-breaker for our site. |
For me too! What needs to be done to get this fixed? I'm afraid I can't help with the technical part, but is there anything I can do, like financial support (donating/sponsoring)? |
As @alexcjohnson wrote above, this is a fairly major project, and is not on the Plotly team's roadmap at the moment. This means that we'll need either a lot of help from someone in the community, or someone will to financially sponsor this project. I expect this is around a $50k-$70k USD project for us. In terms of how we or someone from the community could proceed:
What Sponsorship includes:
Please include the link to this issue when contacting us to discuss. |
@nicolaskruchten Can you motivate the estimated size of this epic? |
As @alexcjohnson mentioned above, we use inline CSS all over the place, so this is a fairly big lift to do while maintaining backwards-compatibility. If you're interested in sponsorship and would like to discuss a breakdown of tasks/how we might make partial progress, please feel free to contact me directly. |
For reference (since this question is still unanswered in this issue), I'm copying in the answer to this question (taken from plotly/dash-core-components#752 (comment)):
I can btw. really recommend what @nicolaskruchten mentions and sponsoring of issues (either partially or fully). My experience on being involved in the partial sponsorship of #897 recently was great. Good plan suggested by the plotly team wrt. what would be possible within the (partial) sponsorship, good follow up during the work and impressive delivery to the open source project (including adding new automatic tests to ensure the CSP compliance does not degrade again). They also found a good way on doing this bundle-by-bundle, such that the |
Thanks @anders-kiaer :) If it turns out that "identify and replace every offending instance" is actually just a small number of places then we can probably do the work for less than $50k-$70k but right now no one has really dug in to figure out the full extent of the problem. If someone from the community wants to do a bit of work to clearly identify all the changes that we'd need to make, we can provide tighter bounds on the cost estimates. |
One possibility here, from a conversation with @alexcjohnson, is that maybe it's just our |
If someone in the community wants to try out setting style properties in a CSP compliant, they can easily experiment/learn with:
|
Thanks @anders-kiaer ! Is this true of old versions of d3 like version 3.x? This is what we use internally right now. |
Yes, looks like it is supported in <!DOCTYPE html>
<html>
<head>
<meta http-equiv="Content-Security-Policy" content="style-src 'self'">
<script src="https://cdnjs.cloudflare.com/ajax/libs/d3/3.0.0/d3.js"></script>
</head>
<body>
<div id="root">Hello world</div>
<script>d3.select("#root").style("background-color", "green");</script>
</body>
</html> |
Great! Looks like someone should tackle the addStyleRule and then see what remains I guess. Maybe just removing that function or replacing it with a return would be enough to find the next violation? |
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
I hope this is not off-topic, but my issue is somewhat connected. I want to use CSS on Plotly.js, but I find absolutely no resources for this. I'm using: "plotly.js": "^2.18.0",
"react": "^18.2.0", andmy plots are not coming from backend, but rather from React, via: import Plot from "react-plotly.js"; I have a plot whose title looks like: title: `<i><b>${title}</b></i>`, and I wanted it to look like: title: `<span class='boldit'>${title}</span>`, where the style is: .boldit {
font-style: italic;
font-weight: bold;
} The only way I could apply something was to hack into the DevTools and figure out that the title has a style Is there a way I could control all these feature of the title without inlining HTML and styles? |
I was searching a way to add styling to the plots as @ricardo-reis-1970 mentioned. We have a webpage with theming and I have to change a lot of colors if the page is dark. I don't want this information duplicated in the code (js/css). I'm used to zenUML.com and their way of styling all and everything with css and this would be very cool. Perhaps having a mode of "remove-all-inner-styling" would already help. I don't know if the paths for the traces has something like |
if someone wouldn't mind adding this to the code, it's kinda magical.
|
Just another vote for implementing #6239 or something similar. |
Allowing styles based on returned hashes works, example: |
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. Fixes plotly#2355 Plotly uses inline CSS
Which necessitates the
unsafe-inline
Content Security Policy directive, or another workaround. Can we get rid of the inline styling somehow?The text was updated successfully, but these errors were encountered: