feat: Improves SafeMarkdown HTML sanitization #21895

michael-s-molina · 2022-10-20T21:26:35Z

SUMMARY

Currently, we are rendering HTML by default and using the following function to sanitize the content:

function isSafeMarkup(node: MarkdownAbstractSyntaxTree) {
  return node.type === 'html' && node.value
    ? !/(href|src)="(javascript|vbscript|file):.*"/gim.test(node.value)
    : true;
}

This only blocks a small subset of potential attack vectors as you can easily see here. To improve the component's security this PR does the following:

Upgrades react-markdown from version 4.3.1 to 8.0.3. This will help with many security issues and also enable the use of some really cool plugins to extend our markdown capabilities.
Installs rehype-raw to parse HTML content.
Installs rehype-sanitize to sanitize the HTML content. By default, it uses the same schema used by GitHub to sanitize HTML, which means a more secure configuration.
Adds the HTML_SANITIZATION configuration option to enable/disable HTML sanitization.
Adds the HTML_SANITIZATION_SCHEMA_EXTENSIONS configuration option to extend the default schema in case someone has a controlled environment and wishes to use some blocked elements/attributes.

TESTING INSTRUCTIONS

Make sure HTML is sanitized or not depending on the value of HTML_SANITIZATION
Make sure HTML sanitization reflects the extensions made in HTML_SANITIZATION_SCHEMA_EXTENSIONS
Make sure the examples that use markdown are correctly rendered

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

codecov · 2022-10-24T23:05:16Z

Codecov Report

Merging #21895 (3af45a1) into master (4cbd70d) will decrease coverage by 0.92%.
The diff coverage is 87.50%.

❗ Current head 3af45a1 differs from pull request most recent head 582f34e. Consider uploading reports for the commit 582f34e to get more accurate results

@@            Coverage Diff             @@
##           master   #21895      +/-   ##
==========================================
- Coverage   67.03%   66.11%   -0.93%     
==========================================
  Files        1813     1813              
  Lines       69424    69426       +2     
  Branches     7448     7448              
==========================================
- Hits        46541    45898     -643     
- Misses      20963    21608     +645     
  Partials     1920     1920

Flag	Coverage Δ
hive	`?`
mysql	`?`
postgres	`?`
presto	`?`
python	`79.61% <100.00%> (-1.93%)`	⬇️
sqlite	`76.90% <100.00%> (+<0.01%)`	⬆️
unit	`51.18% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/views/base.py	`75.70% <ø> (ø)`
...s/superset-ui-core/src/components/SafeMarkdown.tsx	`66.66% <66.66%> (ø)`
...c/dashboard/components/gridComponents/Markdown.jsx	`72.36% <100.00%> (ø)`
...rset-frontend/src/explore/components/SaveModal.tsx	`40.00% <100.00%> (ø)`
superset/config.py	`91.82% <100.00%> (+0.05%)`	⬆️
superset/db_engines/hive.py	`0.00% <0.00%> (-85.19%)`	⬇️
superset/datasets/commands/create.py	`30.61% <0.00%> (-69.39%)`	⬇️
superset/datasets/commands/update.py	`25.00% <0.00%> (-69.05%)`	⬇️
superset/datasets/commands/bulk_delete.py	`33.33% <0.00%> (-53.34%)`	⬇️
superset/datasets/columns/commands/delete.py	`44.11% <0.00%> (-52.95%)`	⬇️
... and 40 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

geido

LGTM!

villebro

LGTM and tested the schema overrides to work as expected. Thanks for doing this, this is a really important enhancement.

It would be nice to follow-up with a PR that adds test coverage (I'm surprised the CI coverage test for superset-ui/core didn't complain about this; @zhaoyongjie do you know?)

villebro · 2022-11-04T13:15:12Z

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

+function SafeMarkdown({
+  source,
+  htmlSanitization = true,
+  htmlSchemaOverrides = {},
+}: SafeMarkdownProps) {


It'd be nice to have RTL tests that test something along the following lines:

the default use case where source is rendering correctly

sanitization is disabled -> how do things change

a schema override before and after

I'll work on this in a follow-up 👍🏼

@villebro superset-ui/core only guarantee 100% coverage on the js/ts file.

@villebro superset-ui/core only guarantee 100% coverage on the js/ts file.

Oh right! Thanks for following up @zhaoyongjie ! 👍

villebro · 2022-11-04T13:22:57Z

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

+  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]);


Kind of unrelated comment, but just thinking out loud: as the feature flags here are constant (they're populated from bootstrap data at load time), displayHtml and escapeHtml are both constants, hence won't really change and are kind of redundant in the dep array. But it would be cool to make something like

const displayHtml = useFeatureFlag(FeatureFlag.DISPLAY_MARKDOWN_HTML);

which could change dynamically, in which case they would also be more relevant in the dep array.

Agreed. I was thinking the same thing here 😄

Hi @villebro @michael-s-molina, I thought the dynamic feature flag might not be a good approach. The reason is that

There are some FF not only on the frontend but also on the backend. for example, ENABLE_TEMPLATE_PROCESSING. so it's hard to set a FF by dynamic.

Some FF is designed for security, for example, ENABLE_EXPLORE_JSON_CSRF_PROTECTION,
ENABLE_TEMPLATE_PROCESSING, and ENABLE_TEMPLATE_REMOVE_FILTERS. The system administrator does not allow users to modify these config dynamically.

I think all of these problems can be addressed in a future configuration module. We can choose what properties we expose to the module and also add the necessary logic to change backend flags.

Good point, I think we should create a new concept to handle user settings.

This reverts commit 7d1df3b.

…#21895)"" This reverts commit d2ae204.

pull-request-size bot added the size/L label Oct 20, 2022

michael-s-molina force-pushed the improve-safe-markdown branch from 17080a2 to 1180ec3 Compare October 24, 2022 22:15

michael-s-molina requested review from dpgaspar, nytai, rusackas, villebro and bkyryliuk October 24, 2022 22:18

michael-s-molina marked this pull request as ready for review October 24, 2022 22:18

michael-s-molina requested review from jinghua-qa and geido as code owners October 25, 2022 14:19

michael-s-molina added 6 commits November 3, 2022 09:32

feat: Improves SafeMarkdown HTML sanitization

43db279

Adds configuration to disable HTML sanitization

20fc043

Adds instructions in UPDATING.md

7d8f377

Changes the feature flag name

dbba9ef

Fixes lint errors

732d669

Fixes E2E test

582f34e

michael-s-molina force-pushed the improve-safe-markdown branch from e01fee2 to 582f34e Compare November 3, 2022 13:33

dpgaspar approved these changes Nov 4, 2022

View reviewed changes

geido approved these changes Nov 4, 2022

View reviewed changes

villebro approved these changes Nov 4, 2022

View reviewed changes

michael-s-molina merged commit 7d1df3b into apache:master Nov 4, 2022

eschutho added v2.0 v2.0.1 labels Nov 8, 2022

AAfghahi pushed a commit that referenced this pull request Nov 30, 2022

feat: Improves SafeMarkdown HTML sanitization (#21895)

1349262

AAfghahi pushed a commit that referenced this pull request Nov 30, 2022

feat: Improves SafeMarkdown HTML sanitization (#21895)

c40b3a7

AAfghahi pushed a commit that referenced this pull request Dec 7, 2022

feat: Improves SafeMarkdown HTML sanitization (#21895)

e02aa10

john-bodley added a commit to airbnb/superset-fork that referenced this pull request Dec 8, 2022

Revert "feat: Improves SafeMarkdown HTML sanitization (apache#21895)"

3fccb49

This reverts commit 7d1df3b.

michael-s-molina added the v1.5 label Jan 3, 2023

michael-s-molina added a commit that referenced this pull request Jan 5, 2023

feat: Improves SafeMarkdown HTML sanitization (#21895)

0296c74

john-bodley added a commit to airbnb/superset-fork that referenced this pull request Jan 17, 2023

Revert "feat: Improves SafeMarkdown HTML sanitization (apache#21895)"

d2ae204

This reverts commit 7d1df3b.

justinpark added a commit to airbnb/superset-fork that referenced this pull request Mar 3, 2023

Revert "Revert "feat: Improves SafeMarkdown HTML sanitization (apache…

fedaab5

…#21895)"" This reverts commit d2ae204.

justinpark mentioned this pull request Mar 11, 2023

fix(dashboard): deepmerge htmlSchemaOverrides #23329

Merged

9 tasks

justinpark mentioned this pull request Mar 27, 2023

fix(dashboard): Add remark plugin on markdown #23502

Merged

9 tasks

mistercrunch added 🍒 1.5.3 🚢 2.1.3 🚢 2.1.0 and removed 🍒 1.5.3 🚢 2.1.3 🚢 2.1.0 labels Feb 18, 2024

mistercrunch added 🍒 1.5.3 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Improves SafeMarkdown HTML sanitization #21895

feat: Improves SafeMarkdown HTML sanitization #21895

michael-s-molina commented Oct 20, 2022 •

edited

Loading

codecov bot commented Oct 24, 2022 •

edited

Loading

geido left a comment

villebro left a comment

villebro Nov 4, 2022

michael-s-molina Nov 4, 2022

zhaoyongjie Nov 4, 2022

villebro Nov 4, 2022

villebro Nov 4, 2022

michael-s-molina Nov 4, 2022

zhaoyongjie Nov 4, 2022

michael-s-molina Nov 4, 2022

zhaoyongjie Nov 4, 2022

feat: Improves SafeMarkdown HTML sanitization #21895

feat: Improves SafeMarkdown HTML sanitization #21895

Conversation

michael-s-molina commented Oct 20, 2022 • edited Loading

SUMMARY

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Oct 24, 2022 • edited Loading

Codecov Report

geido left a comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-s-molina commented Oct 20, 2022 •

edited

Loading

codecov bot commented Oct 24, 2022 •

edited

Loading