search

airbnb / hypernova

A service for server-side rendering your JavaScript views
MIT License
5.82k stars 208 forks source link

Encode closing Tag #165

Open jburghardt opened 4 years ago

jburghardt commented 4 years ago

Currently encoding in the index.js only includes

const ENCODE = [
  ['&', '&'],
  ['>', '>'],
];

If a component is being rendered SSR and includes a property with a closing script tag, the script tag in the SSrendered HTML will close the hypernova script.

<script type="application/json" data-hypernova-key="App" data-hypernova-id="....">
   <!-- {"props": ..., "title":"</script "}

which will throw an error in the JSON.parse method of the payload.

is there a reason closing tags are not encoded here ? Following changes would suffice:

var ENCODE = [
['&', '&amp;'],
['>', '&gt;'],
['<', '&lt;']
];
ljharb commented 4 years ago

</script shouldn't close anything? you'd need </script>, and the > is escaped.

jburghardt commented 4 years ago

<script with a blank after the t does close the hypernova script

ljharb commented 4 years ago

It seems like indeed </ specifically should be escaped.

jburghardt commented 4 years ago

This is what could happen

<html>
   <head></head>
   <body>
   
   <script type="application/json" id="hypernova-app"><!-- {"props": {"message": "Evil user comment containing </script ", "foo": "bar"}} --></script>
    
   <script type="text/javascript">
  document.addEventListener('DOMContentLoaded', function () {
   window.alert(document.getElementById('hypernova-app').innerHTML);
   });
   </script>
  
 </body>
</html>
jburghardt commented 4 years ago

It seems like indeed </ specifically should be escaped.

escaping just < should be enough.

ljharb commented 4 years ago

That will cause a lot more escaping, of all html tags, unnecessarily. We should only escape the pair.

jburghardt commented 4 years ago

Yep you are right, i updated the pull request.

csharplus commented 2 years ago

@duoertai could you please take a look at this issue?