search

nicojs / typed-html

TypeSafe HTML templates using TypeScript. No need to learn a template library.
333 stars 51 forks source link

escaping of element content #5

Closed mark-buer closed 6 years ago

mark-buer commented 6 years ago

Perhaps I'm missing something, but it appears that this library doesn't escape string content within elements?

This is somewhat unexpected since regular old React does perform escaping.

It is probably a security concern since outside content within a template can effectively inject any element, including script elements, into the generated html.

nicojs commented 6 years ago

@mark-buer thanks for reporting this issue!

I've created this project without the use case in mind of allowing user content to be rendered, and thus have not thought about the security aspect of it.

What do you mean exactly? Could you illustrate with an example?

Take this piece of code for example:

const script = <script>alert('hacked!')</script> ;
const body = <body>{script}</body>;

this works by design, because we cannot differentiate this from this piece of code:

const body = <body><script>alert('hacked!')</script> </body>;

We cannot differentiate, because the result of any element is just a string. Is this what you mean?

mark-buer commented 6 years ago

For the two line example you gave, the result is as expected.

But, making a subtle change to the same example:

const script = "<script>alert('hacked!')</script>"; // This is now a string, from the DB, a query parameter, etc
const body = <body>{script}</body>;

actual result:

<body><script>alert('hacked!')</script></body>

expected result:

<body>&lt;script&gt;alert('hacked!')&lt;/script&gt;</body>

Within React, escaping of strings is visible in this JSFiddle.

I'm not sure how you'd go about fixing this issue given the current API shape. If you don't fix it, it'd probably be best to mention the limitation in the README?

For my use case (where I need to handle content that will contain < and other entities), I found that React can render to a string. Despite the extra baggage it will bring, I think I'll go with that for the time being.

Thanks.

nicojs commented 6 years ago

Thanks for the clarification :ok_hand:

I'm not sure how you'd go about fixing this issue given the current API shape. If you don't fix it, it'd probably be best to mention the limitation in the README?

Indeed, the API now provides strings by design. I want to keep that behavior because I think it is a clean way of working. Yes a remark in the readme would definitely be appropriate. I haven't developed a react app yet (it's on my wish list, don't worry), but I can understand that for people from that community it might come as a surprise.

Do you want to add it in a PR? Or shall I? If you know of a good sanitation library, you can also add that as a remark, so people know what to do.

mark-buer commented 6 years ago

I'm a React noob, but have a lot of experience in other web frameworks and languages. Most html templating libraries I've encountered do sanitise html element content. Looks like React is the same.

If you know of a good sanitation library

Part of my reason for assessing this library in the first place was to avoid needing to manually sanitise anything, thus I unfortunately can not recommend any particular library as it is something I avoid doing where ever possible.

Might be best if you make the change to the README, I'm just "passing through".

nicojs commented 6 years ago

@mark-buer could you take a look at my PR? Do you think the message is clear?

mark-buer commented 6 years ago

Looks good to me 👍

Thanks!