XSS on all examples

[mrj0]

Hi, I think it is challenging to use string interpolation for these template files because there's no way to autoescape unsafe input. At the very least, the examples could set a good example and show how to escape variables.

The only mention of XSS I could find was a PR to remove a library: enhance-dev/enhance.dev#30

To illustrate what I mean, here's one of the example elements in the sample project. I am sorry if I didn't find documentation on how to do this better. If it exists, it would be good to make it much more prominent.

Also, the React cookbook translation is inaccurate because the text would be escaped for HTML automatically (not sure if that's JSX or React). React won't render XSS attacks without very purposefully working around it.

export default function MyHeading ({ html }) {
  const unsafeHTML = '<script>alert("Hello World!")</script>'

  return html`
    <h1>${unsafeHTML}</h1>
  `
}

[brianleroux]

hey @mrj0 / its ok to use string interpolation with js values you control. you'll want to escape values in user input from a form which could be hostile. xss is a great lib for doing that.

[mrj0]

Everything should be escaped by default to prevent mistakes and vulnerabilities. This has been the standard for a long time because it's too cumbersome to reliably remember to escape, especially when data is passed through layers. This really should be addressed somehow, maybe attrs could escape by default?

Curiously, I see that the MyHeader example https://enhance.dev/docs/conventions/elements when used like this:

<my-heading heading="Test & One <script>alert('hi')</script>" />

Renders from the server:

<header>
      <h1>
        Test &amp; One <script>alert('hi')</script>
      </h1>
      <p>
        A default description
      </p>
</header>

Why is the ampersand encoded but not the rest?

[brianleroux]

once again, to be clear, the idea of escaping is for potentially harmful end user input not for all strings in all cases. especially not for strings you author as the developer of the application with full rights to do whatever you want. enhance/ssr runs in a backend context that is trusted. if your backend is compromised there will be no need to cross site script anything. you can read more about xss attack vectors here.

the escaping behaviour you see is likely downstream parse5 which is the html parser Enhance uses to expand custom elements. you can often be surprised by what a standards compliant html parser does!

[mrj0]

You absolutely do not get security by trusting.

I should be your core demographic and this is a real blocker for me. It's sorta strange to have to advocate for this when Django came out 20 years ago and autoescaped even back then. I think safety is a core expectation of any web framework these days. My best advice is to make a new major version to provide autoescaping. Incorporating JSX would help a lot.

[brianleroux]

Sigh. You are misunderstanding. I am saying the backend context is trusted compute. Its a computer you control. You are the administrator. We can't know how you are going to use the templating but you are absolutely free to add XSS when it make sense. We aren't stopping you from doing that for user input from an untrusted compute like a clientside form input fields which could be interpolated into markup. I don't know how many different ways to say the same thing. If you want JSX you are free to do that too. Or by all means send us a PR and open a discussion on our Discord if you are still confused.

[jessehattabaugh]

I agree with @mrj0; if Enhance offered a safeHTML template tag, I'd use it.

[brianleroux]

there's no agree or disagree just basic rudimentary best practice.

you too can run npm i xss and use that. as mentioned many times in many different ways now the renderer runs in a trusted process. as we can't know your use case for escaping and we can't escape 'everything' so this would need to be a helper of some kind. rather than making you learn a new thing and having to maintain and document a new thing using a well known and easy to install module is a great solution.

all this being said folks should also make sure they sanitize user input on the way in, as opposed to escaping content on the way out.

[jessehattabaugh]

If you're rendering HTML, and I imported safeHTML you can know that I want HTML special characters encoded. Sure, I could do that every time I render something, but that's super tedious.

I'm not going to sanitize on the way in because I don't know how where the data is going to be used so I can't escape everything and I don't want to have to deal with unescaping it later.

[brianleroux]

nah

[mrj0]

Well, if you want to be the "we just print strings to html" framework, that's certainly a.. choice. But people are going to expect more. This is not bike shedding, this is core to people's needs and it doesn't sound like this project is ready for how people are actually going to use it, or even the attitude to accept people might have different needs than you do.

[jessehattabaugh]

I'm just gonna leave this here @brianleroux

[brianleroux]

🙄 use that then