Mitigate risk of using dangerouslySetInnerHTML

[joebuono]

Description of changes

Potential Security Risk

Within the React package of the Amplify UI codebase, we are using React’s dangerouslySetInnerHTML to insert CSS into a <style> tag and then inject it into the DOM:

<style
  id={`amplify-theme-${name}`}
  dangerouslySetInnerHTML={{ __html: cssText }}
/>

We are doing this because we need to dynamically insert CSS into the page.

The reason this is potential security risk is because a developer could dynamically set a theme based on user input. This could allow an attacker to execute malicious code on the webpage since React’s dangerouslySetInnerHTML does not sanitize the HTML.

However, as mentioned in the internal T Corp ticket: “the risk of an unknown attacker being able to exploit this condition would be highly unlikely because the React DOM escapes everything in JSX before rendering, making XSS almost impossible. The CSS that's being set is in control of the Amplify UI team.”

Additionally, the internal createTheme function escapes all input to strings and does not execute any of the passed-in code.

Solution

To mitigate this risk, we are using the DOMPurify to sanitize the HTML and prevent XSS attacks. DOMPurify is the library that OWASP recommends. Specifically, we are using isomorphic-dompurify in order to get the e2e and docs tests to pass (by rendering on both the client and server side).

import DOMPurify from 'isomorphic-dompurify';

<style
  id={`amplify-theme-${name}`}
  dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(cssText) }}
/>

Tradeoffs to using DOMPurify

• Adding DOMPurify as a dependency means that we will have to regularly patch it since browsers change functionality and bypasses are being discovered regularly (see OWASP)
• It will also slightly increase our bundle size by up to 19.6kB

*** Note: I am still writing tests to confirm that the sanitization works as expected.

Issue #, if available

Description of how you validated changes

Checklist

• PR description included
• yarn test passes
• Tests are updated

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[changeset-bot]

🦋 Changeset detected

Latest commit: ee76a6f

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

This PR includes changesets to release 1 package

Name	Type
@aws-amplify/ui-react	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

[joebuono]

This is necessary in order to use isomorphic dompurify: kkomelin/isomorphic-dompurify#91 (comment)

[reesscot]

please put this comment in the actual jest.setup file

[joebuono]

This import fixed the following rollup error: rollup/rollup-plugin-commonjs#361

[reesscot]

This should be in devDependencies since it's not a runtime dependency, just a build-time dependency.

[joebuono]

Importing the isomorphic version of dompurify in order to get the e2e and docs tests to pass. Isomorphic dompurify is basically a wrapper around dompurify that allows it to run on the server side as well as the client.

[reesscot]

I'm nervous about depending on this package which hasn't been battle tested as much as the original. I'd prefer that we create our own universal isomorphic wrapper around DOMPurify.

[joebuono]

Closing this PR because we've found a better solution: #1622

[calebpollman]

Out for JD

[calebpollman]

Taking an extended weekend ? POC for JS Issues Ashish Nanda : ***@***.******@***.***> POC for UI Issues Milan Shah : ***@***.******@***.***>