Disallow use of "dangerouslySetInnerHTML" on React components (#17759)

Disallows use of "dangerouslySetInnerHTML" on React components, except where explicitly whitelisted
ashwin-pc · Apr 20, 2018 · d9d9fb2 · d9d9fb2
1 parent 6068dce
commit d9d9fb2
Show file tree

Hide file tree

Showing 4 changed files with 27 additions and 3 deletions.
diff --git a/packages/eslint-config-kibana/.eslintrc.js b/packages/eslint-config-kibana/.eslintrc.js
@@ -107,6 +107,7 @@ module.exports = {
     'react/jsx-indent-props': ['error', 2],
     'react/jsx-max-props-per-line': ['error', { maximum: 1, when: 'multiline' }],
     'react/jsx-no-duplicate-props': ['error', { ignoreCase: true }],
+    'react/no-danger': 'error',
     'react/self-closing-comp': 'error',
     'react/jsx-wrap-multilines': ['error', {
       declaration: true,

diff --git a/src/core_plugins/metric_vis/public/components/metric_vis_value.js b/src/core_plugins/metric_vis/public/components/metric_vis_value.js
@@ -37,7 +37,15 @@ class MetricVisValue extends Component {
         <div
           className="metric-value"
           style={metricValueStyle}
-          dangerouslySetInnerHTML={{ __html: metric.value }}
+          /*
+           * Justification for dangerouslySetInnerHTML:
+           * This is one of the visualizations which makes use of the HTML field formatters.
+           * Since these formatters produce raw HTML, this visualization needs to be able to render them as-is, relying
+           * on the field formatter to only produce safe HTML.
+           * `metric.value` is set by the MetricVisComponent, so this component must make sure this value never contains
+           * any unsafe HTML (e.g. by bypassing the field formatter).
+           */
+          dangerouslySetInnerHTML={{ __html: metric.value }} //eslint-disable-line react/no-danger
         />
         { showLabel &&
           <div>{metric.label}</div>

diff --git a/src/ui/public/markdown/markdown.js b/src/ui/public/markdown/markdown.js
@@ -12,6 +12,10 @@ import MarkdownIt from 'markdown-it';
  */
 export function markdownFactory(whiteListedRules, openLinksInNewTab = false) {
   let markdownIt;
+
+  // It is imperitive that the html config property be set to false, to mitigate XSS: the output of markdown-it is
+  // fed directly to the DOM via React's dangerouslySetInnerHTML below.
+
   if (whiteListedRules && whiteListedRules.length > 0) {
     markdownIt = new MarkdownIt('zero', { html: false, linkify: true });
     markdownIt.enable(whiteListedRules);
@@ -89,7 +93,12 @@ export class Markdown extends Component {
       <div
         className={classes}
         {...rest}
-        dangerouslySetInnerHTML={this.state.renderedMarkdown}
+        /*
+         * Justification for dangerouslySetInnerHTML:
+         * The Markdown Visulization is, believe it or not, responsible for rendering Markdown.
+         * This relies on `markdown-it` to produce safe and correct HTML.
+         */
+        dangerouslySetInnerHTML={this.state.renderedMarkdown} //eslint-disable-line react/no-danger
       />
     );
   }

diff --git a/src/ui/public/notify/notifier.js b/src/ui/public/notify/notifier.js
@@ -349,7 +349,13 @@ Notifier.prototype.banner = function (content = '') {
       title="Attention"
       iconType="help"
     >
-      <div dangerouslySetInnerHTML={{ __html: markdownIt.render(content) }} />
+      <div
+        /*
+         * Justification for dangerouslySetInnerHTML:
+         * The notifier relies on `markdown-it` to produce safe and correct HTML.
+         */
+        dangerouslySetInnerHTML={{ __html: markdownIt.render(content) }} //eslint-disable-line react/no-danger
+      />
 
       <EuiButton type="primary" size="s" onClick={dismissBanner}>
         Dismiss