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

Proposal: escape interpolated strings by default #1105

Open
jamesknelson opened this Issue Aug 23, 2017 · 8 comments

Comments

Projects
None yet
6 participants
@jamesknelson
Copy link

jamesknelson commented Aug 23, 2017

styled-components currently requires developers to manually sanitize interpolated strings if they want to avoid CSS injection exploits. And manual sanitization works in theory, it appears that very few people in the ecosystem are actually doing so.

For example, it is possible to inject global styles by setting the text prop on a rebass Tooltip (see rebass issue and codesandbox demo)

<Tooltip text='hello";} html:not(&) {background-color: blue;}  .test'>
  <Text>
    Hover Me
  </Text>
</Tooltip>

There was some discussion in #721 about adding functions to help sanitize, but this was rejected as not immediately needed/not fitting the API. But with this said, I think security is pretty important for a major library like this -- there are a lot of people using styled-components who don't understand the possible dangers of CSS injection attacks.

Perhaps it would make sense to follow React's example and escape all interpolated props by default? You could have a function to mark strings as safe:

// Not needed in this example as a color would be ok after escaping
const Content = styled.div`
  border: 2px solid ${props => styled.dangerouslyInterpolateRawCSS(props.color)};
  border-radius: 5px;
`;

The styled.dangerouslyInterpolateRawCSS function would return the string wrapped in an object. Then, in stringifyRules, you could convert everything other than alphanumerics, %, and a whitelist of other characters into escape codes like \000042, while passing through strings wrapped in objects.

Interested to hear thoughts on this.

@kitten

This comment has been minimized.

Copy link
Member

kitten commented Aug 23, 2017

styled.dangerouslyInterpolateRawCSS

Regarding that proposal, we do not introduce helper functions or new additions to the API very often, if at all.

styled-components currently requires developers to manually sanitize interpolated strings if they want to avoid CSS injection exploits. And manual sanitization works in theory, it appears that very few people in the ecosystem are actually doing so.

The "manual" part is really important. This is because we don't expect a particular string to come in. A string containing more rules can very well be valid additions to the styling.

We do also have a section on warning about this: https://www.styled-components.com/docs/advanced#security

In v3 we'll add invariants around what kind of CSS can be put where for interpolations. But this still breaks down if the user decides to use some mixins i.e. interpolations that allow arbitrary input. We can't really take any action in that case.

(Btw for the next time, this is technically a duplicate of #721 albeit being slightly different 😉)

@mxstbr

This comment has been minimized.

Copy link
Member

mxstbr commented Aug 28, 2017

I wouldn't mind shipping automatic escaping to be honest, and then having some sort of escape hatch if you really don't want to escape. How much code would it be to sanitise the input?

@kitten

This comment has been minimized.

Copy link
Member

kitten commented Aug 28, 2017

@mxstbr we really can’t do this right now as we can’t interpret what is reasonable input from the user and what isn’t. At that point we’d have to enforce the css helper for mixins which is a breaking change and still can’t tell whether the user is interpolating: a value, a property, or a selector

I think this will need to wait for v3 but is totally doable then

@kitten

This comment has been minimized.

Copy link
Member

kitten commented Aug 31, 2017

So while we can't escape interpolations automatically for now, we should make an effort educating our users until then. There'll likely still be attack vectors even when we automatically escape interpolations, since for mixins we can only enforce the css helper, but then need to provide an alternative.

Anyway, @jamesknelson has made some excellent points and his new example is super to show what is possible. We should integrate his example into our documentation. (https://twitter.com/james_k_nelson/status/903246291487956992)

Since this is not enough, we need to answer the simple question of: How can I escape input so that an interpolation is safe?

For this there's a new utility in JS which is in the CSSOM proposal: CSS.escape (try it out! it's in Chrome today afaik) https://drafts.csswg.org/cssom/#the-css.escape%28%29-method

There's a perfect polyfill for this method that we can use in the future by default, but for now we need to urge users to put it in their code themselves whenever they accept/interpolate user input for styled-components. https://www.npmjs.com/package/css.escape

Edit: Opened an issue on the docs styled-components/styled-components-website#102

@geelen

This comment has been minimized.

Copy link
Member

geelen commented Sep 1, 2017

Yeah, I think with v3 we can have a much higher level of guarantees around interpolations. Meaning that an interpolation like color: ${ props => props.userInputLol }; can be checked as being a valid CSS value, right? So it couldn't add extra rules, or selectors.

The problem is this:

styled.div`
  ${ props => props.yolo };
`

injectGlobal`
  ${ localStorage.getItem('super yolo') }
`

The html:not(&) trick in your example @jamesknelson is a really good one. I can't think of any way to disallow that without disallowing useful CSS selectors.

It's also worth pointing out that for these attack vectors to work, you need to be displaying one user's malicious CSS to another user e.g. a customised homepage or something other. And also, that the same exploitations would be possible if you were just inlining it into a CSS file from the server, or sending a custom.css file. There's nothing specific to CSS-in-JS with these exploitations, they're just potentially more likely to occur (since FE devs may be less aware of the issue)?

@jeffsee55

This comment has been minimized.

Copy link

jeffsee55 commented Jun 19, 2018

Came across this issue when doing some research, wondering if now that v3 is out this issue can be addressed? Are there still objections to @jamesknelson's suggestion?

@probablyup

This comment has been minimized.

Copy link
Contributor

probablyup commented Jun 19, 2018

After using s-c for about a year and a half in a large code base, I'm dubious that this would be worth the runtime performance hit.

@jeffsee55

This comment has been minimized.

Copy link

jeffsee55 commented Jun 20, 2018

Fair enough @probablyup, I don't know how impactful this would be for performance. Might be worthwhile to do some profiling and put this to bed one way or another since it seemed like it was desirable by everyone once v3 hit. If there aren't any performance indicators I may try to tackle that this weekend if I get a chance, any tips would be appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment