Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hydration fails with nested props arrays/objects #150

Open
eyelidlessness opened this issue Mar 25, 2021 · 7 comments
Open

Hydration fails with nested props arrays/objects #150

eyelidlessness opened this issue Mar 25, 2021 · 7 comments

Comments

@eyelidlessness
Copy link
Collaborator

Props which reproduce this:

{
  "autoplay": true,
  "playsinline": true,
  "sources": [
    {
      "src": "/home/background-desktop.webm",
      "type": "video/webm"
    },
    {
      "src": "/home/background-desktop.mp4",
      "type": "video/mp4"
    }
  ]
}

These props produce this error:

Uncaught SyntaxError: Unexpected end of JSON input
    at JSON.parse (<anonymous>)
    at p (microsite.js:formatted:93)
    at v (microsite.js:formatted:112)
    at t (microsite.js:formatted:124)

I suspect this would be solved by changing this regex to:

const ATTR_REGEX = /(:?\w+)=(\S+|[{[].*?[}\]])/g;

But I'm hesitant to open a PR because I don't fully grasp what's expected in the hydration markers. That said, I'd also probably recommend not using a regex for this purpose in the first place. A few options I've thought of:

  • Use a special token indicating the beginning/end of a given JSON chunk
  • Separate the markers from the props so they can be treated as plain JSON
  • Use a safer serialization format (I hesitate to mention base 64 because it increases the size, but you get the idea)
@ratorx
Copy link
Contributor

ratorx commented Apr 1, 2021

The only general solution is base64 (or some other safe serialisation). Otherwise e.g. strings that contain HTML will be rendered by the browser rather than treated as part of the JSON regardless of how the JS parses it.

@eyelidlessness
Copy link
Collaborator Author

The other solutions are viable.

The special token solution is used in multipart payloads (eg every time you submit an HTML form with file upload or open an HTML email, so ~every day for regular internet users), and the multiline string solution for most shells and many shell inspired languages.

Separating the markers from the props is just a hash map reference, the marker referencing a key in the hydration props payload. They all have tradeoffs. Base-64 has the most costly tradeoff but it’s the least impact to implement.

@eyelidlessness
Copy link
Collaborator Author

Another solution that’s probably a much bigger lift would be a huge win if possible: don’t mount with props at all. Effectively memoize on the server.

The big cost (for users) here is second render because you need a comparison algorithm that’s efficient, and JS doesn’t have one. The big cost (for devs) is accounting for that. But if you squint a little bit it’s actually not a big deal. Just mount noop on load and render on first lifecycle check and don’t worry about the diff. Or async trigger the second render on idle/visible. It’s the same expense with no payload at all.

And dang it feels good to be a gangsta check my own intuitions when they’re questioned.

@ratorx
Copy link
Contributor

ratorx commented Apr 1, 2021

Sorry I should have been more clear. I agree that the other solutions would work for the JSON parsing case, but my point is that it's worth doing the safe serialisation one because of a different problem (which the other methods can't solve):

The problem is that hydration props like:

{
	"key": "<script>alert()</script>"
}

without safe serialisation will cause the script to be executed by the browser.

EDIT: Memoizing on the server effectively embeds the payload into JS, which also sidesteps this problem, so that's viable too.

@eyelidlessness
Copy link
Collaborator Author

That wouldn’t be a problem if the data is actually JSON treated as such, either:

  • escaped in the way it already is
  • serialized separately as a hashmap
  • serialized with the component for async mount per my new suggestion

@eyelidlessness
Copy link
Collaborator Author

Actually you’re right there is a potential XSS (self own or not), JSON serialization doesn’t account for it. So some change in that regard is definitely warranted as well.

@eyelidlessness
Copy link
Collaborator Author

I think serializing into JS is the common theme here and regardless of the rest of the specifics it’s a clear win.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants