Skip to content
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

XSS on all examples #73

Closed
mrj0 opened this issue Sep 14, 2024 · 12 comments
Closed

XSS on all examples #73

mrj0 opened this issue Sep 14, 2024 · 12 comments

Comments

@mrj0
Copy link

mrj0 commented Sep 14, 2024

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>
  `
}

image

@brianleroux
Copy link
Contributor

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
Copy link
Author

mrj0 commented Sep 14, 2024

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?

image

@brianleroux
Copy link
Contributor

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
Copy link
Author

mrj0 commented Sep 14, 2024

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
Copy link
Contributor

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
Copy link

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

@brianleroux
Copy link
Contributor

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
Copy link

jessehattabaugh commented Sep 15, 2024

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
Copy link
Contributor

nah

@mrj0
Copy link
Author

mrj0 commented Sep 16, 2024

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
Copy link

jessehattabaugh commented Sep 17, 2024

@brianleroux
Copy link
Contributor

🙄 use that then

@enhance-dev enhance-dev locked as resolved and limited conversation to collaborators Sep 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants