Open sanmai-NL opened 6 years ago
It's not entirely clear to me a) what risks this rule is trying to mitigate, and b) exactly which practices of ours would need to change to satisfy this rule. Can you clarify?
If it's just using javascript to create and fill a <style>
tag that's a problem (that's what Lib.addStyleRule
is about) we could presumably stop using this - there's a fairly small set of rules we apply - and just apply these styles directly to the elements that need them.
Risks: https://stackoverflow.com/a/31759553/1175508
Note: Disallowing inline styles and inline scripts is one of the biggest security wins CSP provides.
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 unsafe
CSP rules to work around Plotly’s behavior have to be applied to the HTTP response for a view of the app (HTML document) and not e.g. to the response for the Plotly library. So such a workaround adds risk when any untrusted input may be used to generate that HTML. So this bug and #897 are important to solve for e.g. enterprise web apps using Plotly.
To satisfy the no inline styling requirement, you need to neither apply CSS inline in attributes nor in style
tags. The consumer of Plotly needs to load the CSS as a subresource (e.g. via a link
tag). (Perhaps there’s another way as well?)
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 link
to the CSS on the same URL of the JavaScript library, with a css
file name extension as postfix, to accommodate non-CDN consumers. But that would assume a certain CSP policy (script-src 'self'
), so letting consumers add the stylesheet link to their documents themselves is better.
neither apply CSS inline in attributes
That's the one I'm worried about - if that means that we can't add style
attributes to our elements, like:
<path d="M143.61,620V325.5H169.72V620Z"
style="vector-effect: non-scaling-stroke; opacity: 1; stroke-width: 0px; fill: rgb(31, 119, 180); fill-opacity: 1;"
></path>
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 Lib.addStyleRule
but if that just pushes the error down to element style attributes, there's no point.
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
AFAIK you aren’t restricted to inline CSS even in SVG objects
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).
I assumed it was clear, the solution would be to use external CSS as a subresource
Right, but that's exactly what, as I said above, we cannot do:
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.
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).
For dynamic styling, those presentational parameters you identified so far can easily be replaced with equivalent SVG element attributes
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 https://github.com/plotly/dash-core-components/issues/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 Lib.addStyleRule
and require the CSS to be loaded separately.
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.
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
Lib.addStyleRule
and require the CSS to be loaded separately.
For reference (since this question is still unanswered in this issue), I'm copying in the answer to this question (taken from https://github.com/plotly/dash-core-components/issues/752#issuecomment-584236156):
The
selection.style(...)
method ond3
selections is also CSP compatible since, looking at thed3
source code, the statementd3.select("#some_id").style("background-color", "green")
is ~equivalent to
document.getElementById("some_id").style.setProperty("background-color", "green")
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 unsafe-eval
CSP setting is officially not needed using certain bundles, and then one of the next remaining bundles could also be made CSP compatible on next (partial) sponsorship coming in.
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 addStyleRule
function that's problematic here, and that it could "just" be replaced by some judicious usage of document.getElementById("some_id").style.setProperty("background-color", "green")
(if, as @anders-kiaer has suggested, such usage is CSP-compliant!)... If someone from the community wants to try that to see if it clears up these errors, we would surely help you get that PR merged in.
...if, as @anders-kiaer has suggested, such usage is CSP-compliant!
If someone in the community wants to try out setting style properties in a CSP compliant, they can easily experiment/learn with:
style_csp_test.html
:
<!DOCTYPE html>
<html>
<head>
<meta http-equiv="Content-Security-Policy" content="style-src 'self'">
</head>
<body>
<div id="root">Hello world</div>
</body>
</html>
style_csp_test.html
in a browser (you don't need to start a localhost server), and open the browser console.document.getElementById("root").setAttribute("style", "background-color: green");
and you will see that it is blocked by CSP since setting the style
attribute directly is ~equivalent to allowing inline style (if you want to allow for this you would need to change 'self'
in the html
snippet above to 'self' 'unsafe-inline'
.
document.getElementById("root").style.setProperty("background-color", "green");
you will see that the browser will pass it through the CSP checks (d3
and react >= 15
libraries use this form). 🙂
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 d3 (v3)
. I get 🟢 using this snippet (using inline script for testing we are not using inline style 🙈):
<!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?
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 gtitle
, but this has a bit of inline style that always supersedes my CSS, like font-weight: normal;
.
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 id="trace-0"
and similar for other parts of the plot. This would be very cool. A CSS template for this case (no styling in the svg would result in a transparent svg?) would be good.
if someone wouldn't mind adding this to the code, it's kinda magical.
function l(t, e, r) {
var n = "plotly.js-style-" + t
, a = document.getElementById(n);
a || ((a = document.createElement("style")).setAttribute("id", n),
>>> a.setAttribute("nonce", document.querySelector("script[nonce]")?.nonce || ""),
a.appendChild(document.createTextNode("")),
document.head.appendChild(a));
var o = a.sheet;
o.insertRule ? o.insertRule(e + "{" + r + "}", 0) : o.addRule ? o.addRule(e, r, 0) : i.warn("addStyleRule failed")
}
Just another vote for implementing https://github.com/plotly/plotly.js/pull/6239 or something similar. The organization I work for has infrastructural security restrictions that simply won't allow unsafe-inline in responses. This is a blocker for us and I suspect many large security conscious organizations.
Allowing styles based on returned hashes works, example: style-src 'sha-hash1' 'sha-hash2' 'sha-...'
. Though I don't know whether those are the same for all chart types, might be too much work.
Which necessitates the
unsafe-inline
Content Security Policy directive, or another workaround. Can we get rid of the inline styling somehow?