Revert "Remove flight chunk" #1278
Conversation
This reverts commit c188865.
| function escapeScriptContent(scriptText: string) { | ||
| return ('' + scriptText).replace(scriptRegex, scriptReplacer); | ||
| } | ||
| const scriptRegex = /(<\/|<)(s)(cript)/gi; | ||
| const scriptReplacer = ( | ||
| match: string, | ||
| prefix: string, | ||
| s: string, | ||
| suffix: string | ||
| ) => `${prefix}${s === 's' ? '\\u0073' : '\\u0053'}${suffix}`; |
There was a problem hiding this comment.
Interesting. It just replaces s with a unicode version
There was a problem hiding this comment.
To summarize my understanding:
- We are not using template literals for the value that gets passed to
push()anymore and instead have replaced that withJSON.stringify()so the case of${alert(1)}is eliminated - We are preventing script tags being injected by unicode-encoding the
sin the script tags to avoid payloads like> <script> alert(1) </script>
Since potential user provided data is being loaded inside a script tag, this solution makes sense. However, IMO a more general and holistic approach would be to avoid such block-listings and instead loading hydrating data within a meta div tag through the data field which then allows for consistently html-encoding all of the user provided data. This way, the global object can be initialized by reading the meta tag, and then the new hydrating data can be appended to it if needed.
However, I'm not clear whether this solution is possible within this context or whether there would be some performance issues so please let me know if that doesn't work. 👍🏻
| } | ||
|
|
||
| if (chunk) { | ||
| script += `__flight.push(${JSON.stringify(escapeScriptContent(chunk))})`; |
There was a problem hiding this comment.
So just to clarify, JSON.stringify()
eliminates the need for using backticks for the string that is passed to push() i.e. for
`__flight.push(\`${normalizedChunk}\`)`Correct? This way the case where this would be abused by passing a payload like ${alert(1)} would be no longer be a concern.
| function escapeScriptContent(scriptText: string) { | ||
| return ('' + scriptText).replace(scriptRegex, scriptReplacer); | ||
| } | ||
| const scriptRegex = /(<\/|<)(s)(cript)/gi; |
There was a problem hiding this comment.
I'm not sure if this is a concern here, but since this is a regex with a global flag, it's stateful and therefore calling RegExp.prototype.test() or RegExp.prototype.exec() on it for two different inputs would be problematic. Do you know if String.prototype.replace() has the same issue or not? I was not able to find anything. 🤔
There was a problem hiding this comment.
It seems to me that only RegExp methods are stateful, so something like String.match() or String.replace() should be fine 👍 https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/exec
|
Closing in favor of #1293 |
Reverts Shopify/hydrogen#1272
This PR adds logic to escape script tags which would put the embedded flight script chunks in SSR payloads in a bad or vulnerable state.
Note: This simply prevents the script tag from being terminated too early. Developers who accept untrusted data via inputs should still sanitize the data before sending it to the client.
Inspired by facebook/react#24385