feat: Improves SafeMarkdown HTML sanitization (#21895)

UPDATING.md

@@ -24,6 +24,7 @@ assists people when migrating to a new version.
 
 ## Next
 
+- [21895](https://github.com/apache/superset/pull/21895): Markdown components had their security increased by adhering to the same sanitization process enforced by Github. This means that some HTML elements found in markdowns are not allowed anymore due to the security risks they impose. If you're deploying Superset in a trusted environment and wish to use some of the blocked elements, then you can use the HTML_SANITIZATION_SCHEMA_EXTENSIONS configuration to extend the default sanitization schema. There's also the option to disable HTML sanitization using the HTML_SANITIZATION configuration but we do not recommend this approach because of the security risks. Given the provided configurations, we don't view the improved sanitization as a breaking change but as a security patch.
 - [20606](https://github.com/apache/superset/pull/20606): When user clicks on chart title or "Edit chart" button in Dashboard page, Explore opens in the same tab. Clicking while holding cmd/ctrl opens Explore in a new tab. To bring back the old behaviour (always opening Explore in a new tab), flip feature flag `DASHBOARD_EDIT_CHART_IN_NEW_TAB` to `True`.
 - [20799](https://github.com/apache/superset/pull/20799): Presto and Trino engine will now display tracking URL for running queries in SQL Lab. If for some reason you don't want to show the tracking URL (for example, when your data warehouse hasn't enable access for to Presto or Trino UI), update `TRACKING_URL_TRANSFORMER` in `config.py` to return `None`.
 - [21002](https://github.com/apache/superset/pull/21002): Support Python 3.10 and bump pandas 1.4 and pyarrow 6.

superset-frontend/cypress-base/cypress/integration/dashboard/editmode.test.ts

@@ -773,7 +773,7 @@ describe('Dashboard edit', () => {
       cy.getBySel('dashboard-markdown-editor')
         .should(
           'have.text',
-          '✨Header 1✨Header 2✨Header 3Click here to learn more about markdown formatting',
+          '✨Header 1\n✨Header 2\n✨Header 3\n\nClick here to learn more about markdown formatting',
         )
         .click(10, 10);
 

superset-frontend/package.json

@@ -175,7 +175,6 @@
     "react-jsonschema-form": "^1.2.0",
     "react-lines-ellipsis": "^0.15.0",
     "react-loadable": "^5.5.0",
-    "react-markdown": "^4.3.1",
     "react-query": "^3.39.2",
     "react-redux": "^7.2.0",
     "react-resize-detector": "^6.7.6",

superset-frontend/packages/superset-ui-core/package.json

@@ -2,35 +2,26 @@
   "name": "@superset-ui/core",
   "version": "0.18.25",
   "description": "Superset UI core",
-  "sideEffects": false,
-  "main": "lib/index.js",
-  "module": "esm/index.js",
-  "files": [
-    "esm",
-    "lib"
-  ],
-  "repository": {
-    "type": "git",
-    "url": "git+https://github.com/apache-superset/superset-ui.git"
-  },
   "keywords": [
     "superset"
   ],
-  "author": "Superset",
-  "license": "Apache-2.0",
+  "homepage": "https://github.com/apache-superset/superset-ui#readme",
   "bugs": {
     "url": "https://github.com/apache-superset/superset-ui/issues"
   },
-  "homepage": "https://github.com/apache-superset/superset-ui#readme",
-  "publishConfig": {
-    "access": "public"
-  },
-  "devDependencies": {
-    "@emotion/styled": "^11.3.0",
-    "fetch-mock": "^6.5.2",
-    "jest-mock-console": "^1.0.0",
-    "resize-observer-polyfill": "1.5.1"
+  "repository": {
+    "type": "git",
+    "url": "git+https://github.com/apache-superset/superset-ui.git"
   },
+  "license": "Apache-2.0",
+  "author": "Superset",
+  "sideEffects": false,
+  "main": "lib/index.js",
+  "module": "esm/index.js",
+  "files": [
+    "esm",
+    "lib"
+  ],
   "dependencies": {
     "@babel/runtime": "^7.1.2",
     "@types/d3-format": "^1.3.0",
@@ -60,12 +51,20 @@
     "math-expression-evaluator": "^1.3.8",
     "pretty-ms": "^7.0.0",
     "react-error-boundary": "^1.2.5",
-    "react-markdown": "^4.3.1",
+    "react-markdown": "^8.0.3",
+    "rehype-raw": "^6.1.1",
+    "rehype-sanitize": "^5.0.1",
     "reselect": "^4.0.0",
     "rison": "^0.1.1",
     "seedrandom": "^3.0.5",
     "whatwg-fetch": "^3.0.0"
   },
+  "devDependencies": {
+    "@emotion/styled": "^11.3.0",
+    "fetch-mock": "^6.5.2",
+    "jest-mock-console": "^1.0.0",
+    "resize-observer-polyfill": "1.5.1"
+  },
   "peerDependencies": {
     "@emotion/cache": "^11.4.0",
     "@emotion/react": "^11.4.1",
@@ -76,5 +75,8 @@
     "react": "^16.13.1",
     "react-loadable": "^5.5.0",
     "tinycolor2": "*"
+  },
+  "publishConfig": {
+    "access": "public"
   }
 }

superset-frontend/packages/superset-ui-core/src/components/SafeMarkdown.tsx

@@ -16,38 +16,44 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-
-import React from 'react';
-import ReactMarkdown, { MarkdownAbstractSyntaxTree } from 'react-markdown';
-// @ts-ignore no types available
-import htmlParser from 'react-markdown/plugins/html-parser';
-
+import React, { useMemo } from 'react';
+import ReactMarkdown from 'react-markdown';
+import rehypeSanitize, { defaultSchema } from 'rehype-sanitize';
+import rehypeRaw from 'rehype-raw';
+import { merge } from 'lodash';
 import { FeatureFlag, isFeatureEnabled } from '../utils';
 
 interface SafeMarkdownProps {
   source: string;
+  htmlSanitization?: boolean;
+  htmlSchemaOverrides?: typeof defaultSchema;
 }
 
-function isSafeMarkup(node: MarkdownAbstractSyntaxTree) {
-  return node.type === 'html' && node.value
-    ? !/(href|src)="(javascript|vbscript|file):.*"/gim.test(node.value)
-    : true;
-}
+function SafeMarkdown({
+  source,
+  htmlSanitization = true,
+  htmlSchemaOverrides = {},
+}: SafeMarkdownProps) {
+  const displayHtml = isFeatureEnabled(FeatureFlag.DISPLAY_MARKDOWN_HTML);
+  const escapeHtml = isFeatureEnabled(FeatureFlag.ESCAPE_MARKDOWN_HTML);
+
+  const rehypePlugins = useMemo(() => {
+    const rehypePlugins: any = [];
+    if (displayHtml && !escapeHtml) {
+      rehypePlugins.push(rehypeRaw);
+      if (htmlSanitization) {
+        const schema = merge(defaultSchema, htmlSchemaOverrides);
+        rehypePlugins.push([rehypeSanitize, schema]);
+      }
+    }
+    return rehypePlugins;
+  }, [displayHtml, escapeHtml, htmlSanitization, htmlSchemaOverrides]);
 
-function SafeMarkdown({ source }: SafeMarkdownProps) {
+  // React Markdown escapes HTML by default
   return (
-    <ReactMarkdown
-      source={source}
-      escapeHtml={isFeatureEnabled(FeatureFlag.ESCAPE_MARKDOWN_HTML)}
-      skipHtml={!isFeatureEnabled(FeatureFlag.DISPLAY_MARKDOWN_HTML)}
-      allowNode={isSafeMarkup}
-      astPlugins={[
-        htmlParser({
-          isValidNode: (node: MarkdownAbstractSyntaxTree) =>
-            node.type !== 'script',
-        }),
-      ]}
-    />
+    <ReactMarkdown rehypePlugins={rehypePlugins} skipHtml={!displayHtml}>
+      {source}
+    </ReactMarkdown>
   );
 }
 

superset-frontend/spec/helpers/shim.ts → superset-frontend/spec/helpers/shim.tsx

@@ -16,6 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
+import React from 'react';
 import 'core-js/stable';
 import 'regenerator-runtime/runtime';
 import 'abortcontroller-polyfill/dist/abortcontroller-polyfill-only';
@@ -83,4 +84,9 @@ jest.mock('src/hooks/useTabId', () => ({
   useTabId: () => 1,
 }));
 
+// Check https://github.com/remarkjs/react-markdown/issues/635
+jest.mock('react-markdown', () => (props: any) => <>{props.children}</>);
+jest.mock('rehype-sanitize', () => () => jest.fn());
+jest.mock('rehype-raw', () => () => jest.fn());
+
 process.env.WEBPACK_MODE = 'test';

superset-frontend/src/dashboard/components/gridComponents/Markdown.jsx

@@ -65,6 +65,10 @@ const propTypes = {
   deleteComponent: PropTypes.func.isRequired,
   handleComponentDrop: PropTypes.func.isRequired,
   updateComponents: PropTypes.func.isRequired,
+
+  // HTML sanitization
+  htmlSanitization: PropTypes.bool,
+  htmlSchemaOverrides: PropTypes.object,
 };
 
 const defaultProps = {};
@@ -265,6 +269,8 @@ class Markdown extends React.PureComponent {
             ? MARKDOWN_ERROR_MESSAGE
             : this.state.markdownSource || MARKDOWN_PLACE_HOLDER
         }
+        htmlSanitization={this.props.htmlSanitization}
+        htmlSchemaOverrides={this.props.htmlSchemaOverrides}
       />
     );
   }
@@ -373,6 +379,8 @@ function mapStateToProps(state) {
   return {
     undoLength: state.dashboardLayout.past.length,
     redoLength: state.dashboardLayout.future.length,
+    htmlSanitization: state.common.conf.HTML_SANITIZATION,
+    htmlSchemaOverrides: state.common.conf.HTML_SANITIZATION_SCHEMA_EXTENSIONS,
   };
 }
 export default connect(mapStateToProps)(Markdown);

superset-frontend/src/dashboard/components/gridComponents/Markdown.test.jsx

@@ -20,9 +20,9 @@ import { Provider } from 'react-redux';
 import React from 'react';
 import { styledMount as mount } from 'spec/helpers/theming';
 import sinon from 'sinon';
-import ReactMarkdown from 'react-markdown';
 import { DndProvider } from 'react-dnd';
 import { HTML5Backend } from 'react-dnd-html5-backend';
+import { SafeMarkdown } from '@superset-ui/core';
 
 import { act } from 'react-dom/test-utils';
 import { MarkdownEditor } from 'src/components/AsyncAceEditor';
@@ -112,34 +112,34 @@ describe('Markdown', () => {
   it('should render an Markdown when NOT focused', () => {
     const wrapper = setup();
     expect(wrapper.find(MarkdownEditor)).not.toExist();
-    expect(wrapper.find(ReactMarkdown)).toExist();
+    expect(wrapper.find(SafeMarkdown)).toExist();
   });
 
   it('should render an AceEditor when focused and editMode=true and editorMode=edit', async () => {
     const wrapper = setup({ editMode: true });
     expect(wrapper.find(MarkdownEditor)).not.toExist();
-    expect(wrapper.find(ReactMarkdown)).toExist();
+    expect(wrapper.find(SafeMarkdown)).toExist();
     act(() => {
       wrapper.find(WithPopoverMenu).simulate('click'); // focus + edit
     });
     await waitForComponentToPaint(wrapper);
     expect(wrapper.find(MarkdownEditor)).toExist();
-    expect(wrapper.find(ReactMarkdown)).not.toExist();
+    expect(wrapper.find(SafeMarkdown)).not.toExist();
   });
 
   it('should render a ReactMarkdown when focused and editMode=true and editorMode=preview', () => {
     const wrapper = setup({ editMode: true });
     wrapper.find(WithPopoverMenu).simulate('click'); // focus + edit
     expect(wrapper.find(MarkdownEditor)).toExist();
-    expect(wrapper.find(ReactMarkdown)).not.toExist();
+    expect(wrapper.find(SafeMarkdown)).not.toExist();
 
     // we can't call setState on Markdown bc it's not the root component, so call
     // the mode dropdown onchange instead
     const dropdown = wrapper.find(MarkdownModeDropdown);
     dropdown.prop('onChange')('preview');
     wrapper.update();
 
-    expect(wrapper.find(ReactMarkdown)).toExist();
+    expect(wrapper.find(SafeMarkdown)).toExist();
     expect(wrapper.find(MarkdownEditor)).not.toExist();
   });
 

superset-frontend/src/explore/components/SaveModal.tsx

@@ -21,7 +21,6 @@ import React from 'react';
 import { Dispatch } from 'redux';
 import { SelectValue } from 'antd/lib/select';
 import { connect } from 'react-redux';
-import ReactMarkdown from 'react-markdown';
 import { withRouter, RouteComponentProps } from 'react-router-dom';
 import { InfoTooltipWithTrigger } from '@superset-ui/chart-controls';
 import {
@@ -45,7 +44,6 @@ import { SaveActionType } from 'src/explore/types';
 
 // Session storage key for recent dashboard
 const SK_DASHBOARD_ID = 'save_chart_recent_dashboard';
-const SELECT_PLACEHOLDER = t('**Select** a dashboard OR **create** a new one');
 
 interface SaveModalProps extends RouteComponentProps {
   addDangerToast: (msg: string) => void;
@@ -351,11 +349,12 @@ class SaveModal extends React.Component<SaveModalProps, SaveModalState> {
             onChange={this.onDashboardSelectChange}
             value={dashboardSelectValue || undefined}
             placeholder={
-              // Using markdown to allow for good i18n
-              <ReactMarkdown
-                source={SELECT_PLACEHOLDER}
-                renderers={{ paragraph: 'span' }}
-              />
+              <div>
+                <b>{t('Select')}</b>
+                {t(' a dashboard OR ')}
+                <b>{t('create')}</b>
+                {t(' a new one')}
+              </div>
             }
           />
         </FormItem>

superset-frontend/webpack.config.js

@@ -75,7 +75,7 @@ if (!isDevMode) {
 
 const plugins = [
   new webpack.ProvidePlugin({
-    process: 'process/browser',
+    process: 'process/browser.js',
   }),
 
   // creates a manifest.json mapping of name to hashed output used in template files

superset/config.py

@@ -644,6 +644,28 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]:
 ENABLE_CORS = False
 CORS_OPTIONS: Dict[Any, Any] = {}
 
+# Sanitizes the HTML content used in markdowns to allow its rendering in a safe manner.
+# Disabling this option is not recommended for security reasons. If you wish to allow
+# valid safe elements that are not included in the default sanitization schema, use the
+# HTML_SANITIZATION_SCHEMA_EXTENSIONS configuration.
+HTML_SANITIZATION = True
+
+# Use this configuration to extend the HTML sanitization schema.
+# By default we use the Gihtub schema defined in
+# https://github.com/syntax-tree/hast-util-sanitize/blob/main/lib/schema.js
+# For example, the following configuration would allow the rendering of the
+# style attribute for div elements and the ftp protocol in hrefs:
+# HTML_SANITIZATION_SCHEMA_EXTENSIONS = {
+#   "attributes": {
+#     "div": ["style"],
+#   },
+#   "protocols": {
+#     "href": ["ftp"],
+#   }
+# }
+# Be careful when extending the default schema to avoid XSS attacks.
+HTML_SANITIZATION_SCHEMA_EXTENSIONS: Dict[str, Any] = {}
+
 # Chrome allows up to 6 open connections per domain at a time. When there are more
 # than 6 slices in dashboard, a lot of time fetch requests are queued up and wait for
 # next available socket. PR #5039 is trying to allow domain sharding for Superset,