__staticRouterHydrationData html tag escape

[zheng-chuang]

fix bug when __staticRouterHydrationData contains </script>

[remix-cla-bot]

Hi @zheng-chuang,

Welcome, and thank you for contributing to React Router!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected].

Thanks!

- The Remix team

[changeset-bot]

🦋 Changeset detected

Latest commit: 54f3fcd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

Name	Type
react-router-dom	Patch
react-router	Patch
react-router-dom-v5-compat	Patch
react-router-native	Patch

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

[remix-cla-bot]

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

[timdorr]

I'm not sure what this is actually fixing. Can you explain why this change is needed?

[zheng-chuang]

I'm not sure what this is actually fixing. Can you explain why this change is needed?

When ssr, if the data contains script tags, __staticRouterHydrationData will become like this

[timdorr]

@brophdawg11 Any thoughts on this? This can probably be simplified to .replaceAll('<', '\\\\u003C').

[brophdawg11]

I'm not sure we want to try to handle every potential edge case when it comes to serializing the context. The built-in serialization is very simple by design and provides an opt-out for these types of use cases via <StaticRouterProvider hydrate={false} />. That way you can serialize/deserialize any way you want and pass hydrationData to createBrowserRouter on the client side.

[zheng-chuang]

I'm not sure we want to try to handle every potential edge case when it comes to serializing the context. The built-in serialization is very simple by design and provides an opt-out for these types of use cases via <StaticRouterProvider hydrate={false} />. That way you can serialize/deserialize any way you want and pass hydrationData to createBrowserRouter on the client side.

Thanks, I'll close this PR. But I really hope you can make it easier with this library "serialize-javascript" which I just found.

[SyMind]

@brophdawg11

That way you can serialize/deserialize any way you want and pass hydrationData to createBrowserRouter on the client side.

I agree this, but I still think this PR is necessary.

React-router stringify the JSON string on server, parse JSON string in <script> tag on browser.

When </script> within JSON string in a <script> tag, error will occur. Because HTML parser will treat everything between <script> and </script> as part of the script.

In this case, the react router will fail to run! This is a bug!

This PR will not change the semantics of JSON string, but can effectively avoid the syntax conflict with HTML!

Some references for this problem in stackoverflow:

1. https://stackoverflow.com/questions/22488830/script-within-a-javascript-string-in-a-script-tag
2. https://stackoverflow.com/questions/66837/when-is-a-cdata-section-necessary-within-a-script-tag
3. https://stackoverflow.com/questions/779959/is-it-necessary-to-escape-character-and-for-javascript-string

[SyMind]

@zheng-chuang Please reopen the PR.

[brophdawg11]

I don't think this is worth adding a 2kb gzipped dependency to react-router when this is solvable in user-land via any number of packages depending on your app needs (serialize-javascript, jsesc, and I'm sure many others).

I could be convinced that at the very least we should auto-escape HTML in the JSON string (<, >, ', ", &), so if you want to change this back to the simplest possible approach using .replace to allow safe serialization of <script> (or other html) tags inside your loaderData using the built-in hydration, I'll happily run it by the team.

Personally, if I needed to dynamically render HTML elements within my components, I would send up the data, not the markup:

function loader() {
  return json({
    scripts: [{ src: '/script.js' }],
  });
}

function component() {
  let data = useLoaderData();
  return data.scripts.map(s => <script src={s.src}></script>);
}

Or if I was loading pre-generated markup from something like a CMS, I'd just escape that in my loader if I knew it would contain HTML.

[brophdawg11]

Also would you mind repointing this to dev since it has code changes?

[zheng-chuang]

I don't think this is worth adding a 2kb gzipped dependency to react-router

But this runs on the server side and will not be packaged and sent to the browser.

[brophdawg11]

It's not just for size reasons - react-router-dom has zero external dependencies at the moment:

  "dependencies": {
    "@remix-run/router": "1.3.2",
    "react-router": "6.8.1"
  },

[brophdawg11]

@zheng-chuang I think you may need to rebase this onto dev as well - looks like it's picking up a few unrelated commits from being branched off main initially

[zheng-chuang]

It's not just for size reasons - react-router-dom has zero external dependencies at the moment:

@brophdawg11 serialize-javascript has been removed. and branch is based on dev.

[timdorr]

Suggested change

	// This code is copy form https://github.com/vercel/next.js
	// License: https://github.com/vercel/next.js/blob/b2e9431bb46563264d450b1707dc0d9cdaf70d26/license.md
	// This utility is based on https://github.com/zertosh/htmlescape
	// License: https://github.com/zertosh/htmlescape/blob/0527ca7156a524d256101bb310a9f970f63078ad/LICENSE

Since this file is already copied by Next.js, let's reference the original code as well.

[brophdawg11]

Thanks @zheng-chuang!