Updates astro-island for safer props deserialization #3268
Conversation
🦋 Changeset detectedLatest commit: fa51fe6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
github-actions
bot
added
the
pkg: astro
|
:( This is unfortunate. I'm conflicted here, obviously we want to fix this bug but we also don't want to force |
| export function serializeProps(value: any) { | ||
| return serializeJavaScript(value); | ||
| return JSON.stringify(value); |
There was a problem hiding this comment.
You can use SuperJSON.
Also, this is the only place where serialize-javascript is being used. Maybe it should be completely removed
There was a problem hiding this comment.
Way too big for this use case IMO. We have to deserialize in the browser.
There was a problem hiding this comment.
I'm torn on this one since it removes functionality, but maybe it is the right move to go with JSON.stringify() here
functions, regexps, dates, sets or maps
This is the list of datatypes that serialize-javascript helps with. Functions are the only ones I could see being used in a component, but then maybe that really isn't an ideal use case anyway? 🤔
There was a problem hiding this comment.
Way too big for this use case IMO
Well point. And what about devalue? Sorry for insisting, but not having undefined, Date and may break existing code
There was a problem hiding this comment.
No worries! that's definitely a better option. I still don't feel super great about putting it into a script tag but it's better than having many script tags. Let me try that.
There was a problem hiding this comment.
Ok, I have Tried this out. It turns the inline script from 254 bytes to 2.08kB :(
There was a problem hiding this comment.
It turns the inline script from 254 bytes to 2.08kB :(
How weird. devalue itself shouldn't be included in the output. See XSS Mitigation
There was a problem hiding this comment.
I just mean that devalue adds that many bytes. I don't feel comfortable adding that in a PR to fix a regression. Maybe it's a good tradeoff and maybe not, but I think we should weight the pros and cons in a separate issue. So I'm going to revert the astro-island change and go back to script tags until we have time to figure out what to do here.
|
Closing in favor of #3275 |
Closes #3266
Changes
Props passed to components via
astro-islandare serialized with the serialize-javascript. It isn't 100% compatible withJSON.parse(), for example it will keepundefinedvariables which breaks inJSON.parse()This updates
astro-islandto use theeval()deserialization method recommended byserialize-javascriptTesting
Not sure of the right way to add a test for this, suggestions welcome!
Docs
N/A bug fix only