Ignore meta content attribute escaping to prevent invalid URLs

[ihmpavel]

Summary

React automatically escapes HTML. Unfortunately for <meta /> tags, escaping characters like & to & could break URLs. There are a lot of issues and there is no easy solution (at least for raw Next.js hosted on Vercel). I know, escaping HTML is important, but it should not break URLs, which developers are not able to fix easily.

This change is ignoring escaping content of <meta /> attribute.

That means this tag
<meta property='og:image' content='https://example.com?query1=one&query2=two' /> will not become <meta property='og:image' content='https://example.com?query1=one&amp;query2=two' />, but stays the same. (Encoding the URL will make it fail)

Also it prevents duplicating escaping <meta property='og:image' content='https://example.com?query1=one&amp;query2=two' /> to become <meta property='og:image' content='https://example.com?query1=one&amp;amp;query2=two' />.

Altough we have dangerouslySetInnerHTML, there is no such way to change it for attributes (eg content in <meta />), if it is not supported by <React.Fragment /> for inserting innerHTML.

Some related issues

#13838
vercel/next.js#2006
garmeeh/next-seo#678

Test Plan

[facebook-github-bot]

Hi @ihmpavel!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[sizebot]

Comparing: 2edf449...501df50

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	127.58 kB	127.58 kB	=	40.72 kB	40.72 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	130.40 kB	130.40 kB	=	41.65 kB	41.65 kB
facebook-www/ReactDOM-prod.classic.js	=	406.44 kB	406.44 kB	=	75.25 kB	75.25 kB
facebook-www/ReactDOM-prod.modern.js	=	394.80 kB	394.80 kB	=	73.52 kB	73.52 kB
facebook-www/ReactDOMForked-prod.classic.js	=	406.44 kB	406.44 kB	=	75.25 kB	75.25 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
facebook-www/ReactDOMServer-prod.classic.js	+0.58%	47.61 kB	47.88 kB	+0.43%	11.16 kB	11.21 kB

Generated by 🚫 dangerJS against 501df50

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[gaearon]

I might be missing something, but isn't this essentially introducing a security hole?

let post = await db.getPost()

function App() {
  return (
    <...>
     <meta property='og:image' content={post.imageUrl} />
   </...>
  )
}

renderToString(<App />)

Now let's say post.imageUrl is actually

"'><script>alert("boom")</script><div class='"

wouldn't this let it through?

[gaearon]

this looks like an XSS hole to me

[ihmpavel]

Hi, thank you for the review. I have rechecked the implementation and it is XSS for sure.

I was looking in the source code, but I could not find the solution I will be happy with. I do not want to introduce a security hole, but I want to fix the current problem. Sanitizing HTML is important, but sometimes it is not working as we would like to (vercel/next.js#2006 or #13838). I was playing with some URL query parsers/servers and a lot of them could not get right params from sanitized url. (?a=1&b=2 is usually {'a: 1, 'amp;b': 2}).

Maybe escapeHtml function could accept as second parameter a boolean, which will decide not to encode & for <meta content={} /> attribute? Or something like dangerouslySetInnerHTML for attributes? (Next.JS has it's own <Head /> and we are not able to dangerouslySetInnerHTML in fragments; <head dangerouslySetInnerHTML={...<meta content={} />...} /> is impossible to use.)

Let me know if you have any other idea which will not be something hacky as I have on my mind.

[gaearon]

Yeah (I think) I understand the high-level problem, but this solution isn’t something that would work. Let’s discuss on the linked issue.