server-islands: only encode ETAGO delimiter + opening HTML comment syntax #11513
Conversation
🦋 Changeset detectedLatest commit: a0d1205 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
ed1f3ec to
87aa1f6
Compare
|
Thanks @kurtextrem! Would you mind adding a test for this? You can add one here: https://github.com/withastro/astro/blob/main/packages/astro/test/server-islands.test.js (see the fixtures being used). The last PR was a hot-fix so a test didn't get added for urgency but now since the problem is fixed it would be good to have a test to prevent regressions. |
|
@kurtextrem are you still interested in this PR? It would just need a test to move forward |
|
@florian-lefebvre Sorry, yeah - I am. The only unclear thing is how extensive we want to test this. |
|
@kurtextrem it doesn't need to be that extensive, we just want to make sure that scripts can't be injected mostly. |
|
@kurtextrem, are you still interested in finishing the PR? |
CodSpeed Performance ReportMerging #11513 will not alter performanceComparing Summary
|
Co-authored-by: Matt Kane <m@mk.gg>
| @@ -1,12 +1,14 @@ | |||
| --- | |||
| import Island from '../components/Island.astro'; | |||
|
|
|||
| const xssMe ="</script><script>alert('xss')</script>" | |||
There was a problem hiding this comment.
It would be good to have a couple more examples in the test, particularly one that uses a comment
There was a problem hiding this comment.
I'm not really familiar with what could through, examples welcome
There was a problem hiding this comment.
Probably something like '"<!--"</script>'. Thank you for taking this over.
Changes
As per https://mathiasbynens.be/notes/etago, to make JSON safe inside a
<script>literal, you need to only encode end-open tag (ETAGO) delimiters and<!--. Thus, we can avoid some work. I've also hoisted the regexps so that we only initialize them once.Regarding the removal of 0x2029 and 0x2028 added in #11508, to me it does not seem security related to escape those, jsesc notes:
Which does not seem relevant in the server-islands case. Did I miss anything here?
Testing
I used the following snippet by @ascorbic:
Docs
Only a small perf related change, so no further docs needed.