XSS Vulnerability

[mattyork]

The StyleSheet and Style components usedangerouslySetInnerHTML on input that comes from outside the library. This is a vulnerability. Ideally the input is sanitized or the css is inserted in an xss-proof way (maybe using insertRule()?), but it should at least be documented that input needs to be trusted.

dangeouslySetInnerHTML should not be used for any input that comes from outside Radium.

[goldensunliu]

are you saying the library should not trust the user? Radium is the tool, your source code is the user. I am pretty sure it is your responsibility to protect yourself.
for example, should exec disallow rm just because it might do something dangerous?

const { exec } from 'child_process'
exec('rm test.out', function callback(error, stdout, stderr){
    // result
});

[kumarharsh]

@mattyork dangerouslySetInnerHTML should not be used for any input that comes from outside your control. Radium is dumb in that respsect, and should remain so. It's the developer's responsibility to sanitize user-inputted styles, not Radium's

[mattyork]

@goldensunliu I'm saying Radium should follow React's safety philosophy:

Our design philosophy is that it should be “easy” to make things safe, and developers should explicitly state their intent when performing “unsafe” operations. The prop name dangerouslySetInnerHTML is intentionally chosen to be frightening, and the prop value (an object instead of a string) can be used to indicate sanitized data.

So when writing non-Radium React components, programmers do not have to worry about sanitizing input from the end-users except when a programmer explicitly chooses to perform an unsafe operation. Radium's use of dangerouslySetInnerHTML goes against that philosophy because there is no indication that the operation is unsafe: no indication in the usage of the library, nor in the documentation. The only way to know it's unsafe is by looking through the source code.

@kumarharsh in this case Radium is in control of dangerouslySetInnerHTML and Radium does not sanitize the input nor tell the programmer that they need to do so. This goes against React programmers' expectations.

So again, two options:

1. Align Radium with React's philosophy and make it safe by default
2. Explicitly state in the documentation every API endpoint that needs sanitized input
   1. Bonus points: a top level comment justifying why Radium strays from React's safe-by-default philosophy
   2. Bonus points: Radium comes with a sanitize tool

[tptee]

While you can't inject <script> tags into a <style> tag, there's other attack vectors through dangerous CSS rules. See some examples here: https://www.owasp.org/index.php/XSS_(Cross_Site_Scripting)_Prevention_Cheat_Sheet#RULE_.234_-_CSS_Escape_And_Strictly_Validate_Before_Inserting_Untrusted_Data_into_HTML_Style_Property_Values

I vote for "safety first." Radium should be as safe to use as any other React component. insertRule() is not enough to ensure safety. I'll PR with sanitization of style rules.

[ianobermiller]

Great discussion, thanks everyone. Looking forward to the PR @tptee!

[tptee]

Turns out there's a pretty exhaustive list of vulnerabilities involved with injecting CSS: http://heideri.ch/jso/#css

Thankfully, user input in Radium is limited to plain objects we control. A whitelist of CSS properties would be the safest option, but could balloon up the bundle size. I'll see what I can do.

[tptee]

I could be reading this wrong, but it looks like React itself doesn't protect against dangerous CSS:
https://github.com/facebook/react/blob/3b96650e39ddda5ba49245713ef16dbc52d25e9e/src/renderers/dom/shared/dangerousStyleValue.js#L33

@sebmarkbage explains it here (they tried a whitelist/validator and it ended up too expensive): facebook/react#3473 (comment)

Considering that, a warning about unsafe user input in style objects is probably our best bet, and would be consistent with how React handles inline styles at this moment.