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

Invalid HTML inside dangerouslySetInnerHTML breaks the page. #14797

Open
lfades opened this issue Jul 2, 2020 · 12 comments
Open

Invalid HTML inside dangerouslySetInnerHTML breaks the page. #14797

lfades opened this issue Jul 2, 2020 · 12 comments
Labels
good first issue Easy to fix issues, good for newcomers
Milestone

Comments

@lfades
Copy link
Member

lfades commented Jul 2, 2020

Bug report

Describe the bug

If invalid HTML is added to dangerouslySetInnerHTML, Next.js will output a blank page without providing any feedback. This can be hard to track when working with a CMS provider or markdown files.

To Reproduce

Steps to reproduce the behavior, please provide code snippets or a repository:

  1. Clone https://github.com/lfades/nextjs-inner-html-bug
  2. Run yarn && yarn dev or npm i && npm run dev
  3. See that pages/index.js is a blank page with no errors

Expected behavior

Invalid HTML inside dangerouslySetInnerHTML should throw and/or let the user know that there's something wrong.

The demo also has an index.html and index.js in the root directory that shows how the same code works in React alone, it doesn't produce an error either, but it shows the content.

@lfades lfades added kind: bug good first issue Easy to fix issues, good for newcomers type: next labels Jul 2, 2020
@TatisLois
Copy link

Hey @lfades is it okay if I take this issue up?

I was able to reproduce the issue locally and noticed that the reason the page goes blank is because dangerouslySetInnerHTML seems to be auto closing the open tag while wrapping any HTML/Scripts etc.. underneath inside of the tag.

This does not cause the page to go blank if that invalid html tag is a or even invalid markup like but will go blank if the invalid html tag is a <style> or <script> because the scripts next.js appends at the end of the page can't be executed inside of <style> or <script>.

I think the right behavior would be to ensure that the next.js scripts don't end up wrapped inside of the invalid tag so that the JS can execute and any valid content can still render on the page.

What are your thoughts, i'll attach screenshots for reference...

@TatisLois
Copy link

Open tag will autoclose with all markup and scripts underneath the open tag wrapped inside of it
Screen Shot 2020-07-02 at 12 30 48 PM

Same issue with <script> tag

Screen Shot 2020-07-02 at 12 43 25 PM

NOT an issue with a open markup tag
Screen Shot 2020-07-02 at 12 44 07 PM

@TatisLois
Copy link

@lfades or anyone else..

I tried to run a forked/cloned version of next locally and have it point to the example app but I keep running into a mismatch react issue.

I tried to work through it but am stuck, any thoughts?

Screen Shot 2020-07-02 at 10 36 45 PM

@lfades
Copy link
Member Author

lfades commented Jul 3, 2020

@TatisLois Follow the steps here to setup the Next.js repo: https://github.com/vercel/next.js/blob/canary/contributing.md

Once you run yarn dev inside the root directory (which compiles the Next.js packages), you can change your package.json and point it to the next package: https://github.com/vercel/next.js/blob/canary/contributing.md#running-your-own-app-with-locally-compiled-version-of-nextjs

@TatisLois
Copy link

TatisLois commented Jul 3, 2020

@TatisLois Follow the steps here to setup the Next.js repo: https://github.com/vercel/next.js/blob/canary/contributing.md

Once you run yarn dev inside the root directory (which compiles the Next.js packages), you can change your package.json and point it to the next package: https://github.com/vercel/next.js/blob/canary/contributing.md#running-your-own-app-with-locally-compiled-version-of-nextjs

I followed those steps and is how I ran into the issue. I’ll wipe the repo clean and try again from scratch, thanks!

got it working - thanks

@sannamar
Copy link

sannamar commented Sep 3, 2020

I'll take a look at this issue as well. I think the responsibility of putting in valid html in dangerouslySetInnerHTML is on the user, but I think I have a solution for outputting an error if the user does so. (Ping @lalmqvist)

@sannamar
Copy link

sannamar commented Sep 3, 2020

I don't have time to continue on this issue, so if anyone else wants to continue - feel free to do so!

My idea is to validate the HTML with perhaps html-validator package and logging the error to the console. I think adding something similiar to this in Main component could work:

export function Main() {
  const { inAmpMode, html, docComponentsRendered } = useContext(
    DocumentComponentContext
  )
+  const [invalidHtmlError, setInvalidHtmlError] = useState()

  docComponentsRendered.Main = true

+  useEffect(() => {
+    checkHtml()
+  })

+  const checkHtml = async () => {
+   try {
+      await htmlValidator(html)
+    } catch (error) {
+      setInvalidHtmlError(error)
+    }
+  }

  if (inAmpMode) return <>{AMP_RENDER_TARGET}</>

+  if (invalidHtmlError) {
+    Log.error(
+      "There's  an error with your HTML",
+      JSON.stringify(invalidHtmlError)
+    )
+  }

  return <div id="__next" dangerouslySetInnerHTML={{ __html: html }} />
}

@Timer Timer added this to the backlog milestone Sep 7, 2020
@augustmuir
Copy link

I also had this problem when allowing user generated HTML. If you wrap the dangerously set HTML code using the sanitize package linked below - it auto-fixes this broken HTML problem for you.

https://www.npmjs.com/package/sanitize-html

<div dangerouslySetInnerHTML={{ __html: (sanitizeHtml(yourHtmlStringHere, {
   allowedTags: ['*'],
   allowedAttributes: {'*': [ '*' ]},
   allowedAttributes: { '*': ["*"]},
   allowedIframeHostnames: ['*']
   })) }} 
/>

@Christopher-Stevers
Copy link

I'll take this one on.

@MarioSerano
Copy link

Hey, is there any progress on the issue?

If there is no one that is not taking this on, I'll be ready to take this on.

cc @Timer @Christopher-Stevers @lfades

@karthiknadar1204
Copy link

If there is no progress on this issue,please assign this issue to me

@abhinaypandey02
Copy link
Contributor

Can't reproduce on latest commit. I see the heading but not the subtitle because of wrong tag. But the page works fine.
Screenshot 2023-09-04 at 3 31 54 AM

@samcx samcx removed the kind: bug label Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Easy to fix issues, good for newcomers
Projects
None yet
Development

No branches or pull requests

10 participants