feat: Improves SafeMarkdown HTML sanitization (#21895)

This commit is contained in:
Michael S. Molina 2022-11-04 10:34:02 -04:00 committed by GitHub
parent b040211970
commit 7d1df3b78d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
14 changed files with 1991 additions and 512 deletions

View File

@ -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.

View File

@ -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);

File diff suppressed because it is too large Load Diff

View File

@ -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",

View File

@ -2,6 +2,19 @@
"name": "@superset-ui/core",
"version": "0.18.25",
"description": "Superset UI core",
"keywords": [
"superset"
],
"homepage": "https://github.com/apache-superset/superset-ui#readme",
"bugs": {
"url": "https://github.com/apache-superset/superset-ui/issues"
},
"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",
@ -9,28 +22,6 @@
"esm",
"lib"
],
"repository": {
"type": "git",
"url": "git+https://github.com/apache-superset/superset-ui.git"
},
"keywords": [
"superset"
],
"author": "Superset",
"license": "Apache-2.0",
"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"
},
"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"
}
}

View File

@ -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);
function SafeMarkdown({ source }: SafeMarkdownProps) {
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]);
// 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>
);
}

View File

@ -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';

View File

@ -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);

View File

@ -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,26 +112,26 @@ 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
@ -139,7 +139,7 @@ describe('Markdown', () => {
dropdown.prop('onChange')('preview');
wrapper.update();
expect(wrapper.find(ReactMarkdown)).toExist();
expect(wrapper.find(SafeMarkdown)).toExist();
expect(wrapper.find(MarkdownEditor)).not.toExist();
});

View File

@ -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>

View File

@ -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

View File

@ -644,6 +644,28 @@ STORE_CACHE_KEYS_IN_METADATA_DB = False
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,

View File

@ -30,9 +30,9 @@ position:
uuid: d44e416d-1647-44e4-b442-6da34b44adc4
width: 3
parents:
- ROOT_ID
- GRID_ID
- ROW-PgcMF2PpB
- ROOT_ID
- GRID_ID
- ROW-PgcMF2PpB
type: CHART
CHART-FwpJA_o1-n:
children: []
@ -44,9 +44,9 @@ position:
uuid: 92e1d712-bcf9-4d7e-9b94-26cffe502908
width: 2
parents:
- ROOT_ID
- GRID_ID
- ROW-aseZBdP1v
- ROOT_ID
- GRID_ID
- ROW-aseZBdP1v
type: CHART
CHART-JhE-Y0xxgi:
children: []
@ -59,9 +59,9 @@ position:
uuid: 62b7242e-decc-2d1b-7f80-c62776939d1e
width: 4
parents:
- ROOT_ID
- GRID_ID
- ROW-aseZBdP1v
- ROOT_ID
- GRID_ID
- ROW-aseZBdP1v
type: CHART
CHART-RbxP2Dl7Ad:
children: []
@ -73,9 +73,9 @@ position:
uuid: abe2c022-ceee-a60a-e601-ab93f7ee52b1
width: 2
parents:
- ROOT_ID
- GRID_ID
- ROW-aseZBdP1v
- ROOT_ID
- GRID_ID
- ROW-aseZBdP1v
type: CHART
CHART-cj9KtCNRq3:
children: []
@ -87,10 +87,10 @@ position:
uuid: 7dad983b-e9f6-d2e8-91da-c2262d4e84e8
width: 3
parents:
- ROOT_ID
- GRID_ID
- ROW-aseZBdP1v
- COLUMN-4bvhV9jxDI
- ROOT_ID
- GRID_ID
- ROW-aseZBdP1v
- COLUMN-4bvhV9jxDI
type: CHART
CHART-ej0FpkKxzj:
children: []
@ -102,9 +102,9 @@ position:
uuid: 6cb43397-5c62-4f32-bde2-95344c412b5a
width: 5
parents:
- ROOT_ID
- GRID_ID
- ROW-PgcMF2PpB
- ROOT_ID
- GRID_ID
- ROW-PgcMF2PpB
type: CHART
CHART-f42-kMWQPd:
children: []
@ -116,9 +116,9 @@ position:
uuid: f2a8731b-3d8c-4d86-9d33-7c0a3e64d21c
width: 4
parents:
- ROOT_ID
- GRID_ID
- ROW-PgcMF2PpB
- ROOT_ID
- GRID_ID
- ROW-PgcMF2PpB
type: CHART
CHART-tMLHDtzb67:
children: []
@ -130,9 +130,9 @@ position:
uuid: b0f11bdf-793f-473f-b7d5-b9265e657896
width: 12
parents:
- ROOT_ID
- GRID_ID
- ROW-y62Rf2K3m
- ROOT_ID
- GRID_ID
- ROW-y62Rf2K3m
type: CHART
CHART-vIvrauAMxV:
children: []
@ -144,32 +144,32 @@ position:
uuid: 9f742bdd-cac1-468c-3a37-35c9b3cfd5bb
width: 2
parents:
- ROOT_ID
- GRID_ID
- ROW-aseZBdP1v
- ROOT_ID
- GRID_ID
- ROW-aseZBdP1v
type: CHART
COLUMN-4bvhV9jxDI:
children:
- MARKDOWN--8u3tfVF49
- CHART-cj9KtCNRq3
- MARKDOWN--8u3tfVF49
- CHART-cj9KtCNRq3
id: COLUMN-4bvhV9jxDI
meta:
background: BACKGROUND_TRANSPARENT
width: 2
parents:
- ROOT_ID
- GRID_ID
- ROW-aseZBdP1v
- ROOT_ID
- GRID_ID
- ROW-aseZBdP1v
type: COLUMN
DASHBOARD_VERSION_KEY: v2
GRID_ID:
children:
- ROW-aseZBdP1v
- ROW-y62Rf2K3m
- ROW-PgcMF2PpB
- ROW-aseZBdP1v
- ROW-y62Rf2K3m
- ROW-PgcMF2PpB
id: GRID_ID
parents:
- ROOT_ID
- ROOT_ID
type: GRID
HEADER_ID:
id: HEADER_ID
@ -180,96 +180,95 @@ position:
children: []
id: MARKDOWN--8u3tfVF49
meta:
code: <iframe src="https://cdn.brandfolder.io/5H442O3W/at/pl546j-7le8zk-838dm2/Slack_RGB.svg"
class="iframe" scrolling="no"></iframe>
code: <img src="https://cdn.brandfolder.io/5H442O3W/at/pl546j-7le8zk-838dm2/Slack_RGB.svg">
height: 23
width: 3
parents:
- ROOT_ID
- GRID_ID
- ROW-aseZBdP1v
- COLUMN-4bvhV9jxDI
- ROOT_ID
- GRID_ID
- ROW-aseZBdP1v
- COLUMN-4bvhV9jxDI
type: MARKDOWN
ROOT_ID:
children:
- GRID_ID
- GRID_ID
id: ROOT_ID
type: ROOT
ROW-PgcMF2PpB:
children:
- CHART-EYIBwyUiHc
- CHART-f42-kMWQPd
- CHART-ej0FpkKxzj
- CHART-EYIBwyUiHc
- CHART-f42-kMWQPd
- CHART-ej0FpkKxzj
id: ROW-PgcMF2PpB
meta:
'0': ROOT_ID
"0": ROOT_ID
background: BACKGROUND_TRANSPARENT
parents:
- ROOT_ID
- GRID_ID
- ROOT_ID
- GRID_ID
type: ROW
ROW-aseZBdP1v:
children:
- COLUMN-4bvhV9jxDI
- CHART-vIvrauAMxV
- CHART-RbxP2Dl7Ad
- CHART-FwpJA_o1-n
- CHART-JhE-Y0xxgi
- COLUMN-4bvhV9jxDI
- CHART-vIvrauAMxV
- CHART-RbxP2Dl7Ad
- CHART-FwpJA_o1-n
- CHART-JhE-Y0xxgi
id: ROW-aseZBdP1v
meta:
background: BACKGROUND_TRANSPARENT
parents:
- ROOT_ID
- GRID_ID
- ROOT_ID
- GRID_ID
type: ROW
ROW-y62Rf2K3m:
children:
- CHART-tMLHDtzb67
- CHART-tMLHDtzb67
id: ROW-y62Rf2K3m
meta:
'0': ROOT_ID
"0": ROOT_ID
background: BACKGROUND_TRANSPARENT
parents:
- ROOT_ID
- GRID_ID
- ROOT_ID
- GRID_ID
type: ROW
metadata:
timed_refresh_immune_slices: []
expanded_slices: {}
refresh_frequency: 0
default_filters: '{}'
default_filters: "{}"
color_scheme: supersetColors
label_colors:
'0': '#1FA8C9'
'1': '#454E7C'
introductions: '#5AC189'
jobs: '#FF7F44'
apache-releases: '#666666'
commits: '#E04355'
dashboard-filters: '#FCC700'
announcements: '#A868B7'
general: '#3CCCCB'
superset_stage_alerts: '#A38F79'
contributing: '#8FD3E4'
graduation: '#A1A6BD'
embedd-dashboards: '#ACE1C4'
helm-k8-deployment: '#FEC0A1'
visualization_plugins: '#B2B2B2'
community-feedback: '#EFA1AA'
cypress-tests: '#FDE380'
product_feedback: '#D3B3DA'
developers: '#9EE5E5'
dashboard-level-access: '#D1C6BC'
design: '#1FA8C9'
feature-requests: '#454E7C'
localization: '#5AC189'
newsletter: '#FF7F44'
beginners: '#666666'
github-notifications: '#E04355'
superset-champions: '#FCC700'
superset_prod_reports: '#A868B7'
dashboards: '#3CCCCB'
pull-requests: '#A38F79'
support: '#8FD3E4'
globalnav_search: '#A1A6BD'
"0": "#1FA8C9"
"1": "#454E7C"
introductions: "#5AC189"
jobs: "#FF7F44"
apache-releases: "#666666"
commits: "#E04355"
dashboard-filters: "#FCC700"
announcements: "#A868B7"
general: "#3CCCCB"
superset_stage_alerts: "#A38F79"
contributing: "#8FD3E4"
graduation: "#A1A6BD"
embedd-dashboards: "#ACE1C4"
helm-k8-deployment: "#FEC0A1"
visualization_plugins: "#B2B2B2"
community-feedback: "#EFA1AA"
cypress-tests: "#FDE380"
product_feedback: "#D3B3DA"
developers: "#9EE5E5"
dashboard-level-access: "#D1C6BC"
design: "#1FA8C9"
feature-requests: "#454E7C"
localization: "#5AC189"
newsletter: "#FF7F44"
beginners: "#666666"
github-notifications: "#E04355"
superset-champions: "#FCC700"
superset_prod_reports: "#A868B7"
dashboards: "#3CCCCB"
pull-requests: "#A38F79"
support: "#8FD3E4"
globalnav_search: "#A1A6BD"
version: 1.0.0

View File

@ -112,6 +112,8 @@ FRONTEND_CONF_KEYS = (
"ALLOWED_EXTENSIONS",
"SAMPLES_ROW_LIMIT",
"DEFAULT_TIME_FILTER",
"HTML_SANITIZATION",
"HTML_SANITIZATION_SCHEMA_EXTENSIONS",
)
logger = logging.getLogger(__name__)