Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: adding feature flags to escape/hide html in markdown #11340

Merged
merged 9 commits into from
Oct 21, 2020
Merged

feat: adding feature flags to escape/hide html in markdown #11340

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4148fc2
utilizing feature flag for html escapement
rusackas Oct 20, 2020
5e1233b
use src alias
rusackas Oct 20, 2020
1b3871c
Feature flag to allow hiding of HTML tags
rusackas Oct 20, 2020
c4aa5d5
strips js attr
rusackas Oct 20, 2020
8f3cb60
better feature flag naming
rusackas Oct 20, 2020
54cfbed
simplifying
rusackas Oct 20, 2020
e10f848
Adding comments to new feature flags
rusackas Oct 21, 2020
1f25ec1
shorter comment, because pylint,
rusackas Oct 21, 2020
3e7b5ec
pedantic change to get black to format a file
rusackas Oct 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 20 additions & 10 deletions superset-frontend/src/dashboard/components/gridComponents/Markdown.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,23 +19,26 @@
import React from 'react';
import PropTypes from 'prop-types';
import ReactMarkdown from 'react-markdown';
import htmlParser from 'react-markdown/plugins/html-parser';

import cx from 'classnames';
import { t } from '@superset-ui/core';
import { Logger, LOG_ACTIONS_RENDER_CHART } from 'src/logger/LogUtils';
import { MarkdownEditor } from 'src/components/AsyncAceEditor';

import DeleteComponentButton from '../DeleteComponentButton';
import DragDroppable from '../dnd/DragDroppable';
import ResizableContainer from '../resizable/ResizableContainer';
import MarkdownModeDropdown from '../menu/MarkdownModeDropdown';
import WithPopoverMenu from '../menu/WithPopoverMenu';
import { componentShape } from '../../util/propShapes';
import { ROW_TYPE, COLUMN_TYPE } from '../../util/componentTypes';
import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags';

import DeleteComponentButton from 'src/dashboard/components/DeleteComponentButton';
import DragDroppable from 'src/dashboard/components/dnd/DragDroppable';
import ResizableContainer from 'src/dashboard/components/resizable/ResizableContainer';
import MarkdownModeDropdown from 'src/dashboard/components/menu/MarkdownModeDropdown';
import WithPopoverMenu from 'src/dashboard/components/menu/WithPopoverMenu';
import { componentShape } from 'src/dashboard/util/propShapes';
import { ROW_TYPE, COLUMN_TYPE } from 'src/dashboard/util/componentTypes';
import {
GRID_MIN_COLUMN_COUNT,
GRID_MIN_ROW_UNITS,
GRID_BASE_UNIT,
} from '../../util/constants';
} from 'src/dashboard/util/constants';

const propTypes = {
id: PropTypes.string.isRequired,
Expand Down Expand Up @@ -84,6 +87,7 @@ function isSafeMarkup(node) {

return true;
}

class Markdown extends React.PureComponent {
constructor(props) {
super(props);
Expand Down Expand Up @@ -259,8 +263,14 @@ class Markdown extends React.PureComponent {
? MARKDOWN_ERROR_MESSAGE
: this.state.markdownSource || MARKDOWN_PLACE_HOLDER
}
escapeHtml={false}
escapeHtml={isFeatureEnabled(FeatureFlag.ESCAPE_MARKDOWN_HTML)}
skipHtml={!isFeatureEnabled(FeatureFlag.DISPLAY_MARKDOWN_HTML)}
allowNode={isSafeMarkup}
astPlugins={[
htmlParser({
isValidNode: node => node.type !== 'script',
}),
]}
/>
);
}
Expand Down
2 changes: 2 additions & 0 deletions superset-frontend/src/featureFlags.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ export enum FeatureFlag {
SQLLAB_BACKEND_PERSISTENCE = 'SQLLAB_BACKEND_PERSISTENCE',
THUMBNAILS = 'THUMBNAILS',
LISTVIEWS_DEFAULT_CARD_VIEW = 'LISTVIEWS_DEFAULT_CARD_VIEW',
DISPLAY_MARKDOWN_HTML = 'DISPLAY_MARKDOWN_HTML',
ESCAPE_MARKDOWN_HTML = 'ESCAPE_MARKDOWN_HTML',
}

export type FeatureFlagMap = {
Expand Down
2 changes: 2 additions & 0 deletions superset/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,8 @@ def _try_json_readsha( # pylint: disable=unused-argument
"TAGGING_SYSTEM": False,
"SQLLAB_BACKEND_PERSISTENCE": False,
"LISTVIEWS_DEFAULT_CARD_VIEW": False,
"DISPLAY_MARKDOWN_HTML": True,
rusackas marked this conversation as resolved.
Show resolved Hide resolved
"ESCAPE_MARKDOWN_HTML": False,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add comments here that explain how these affect the behaviour? Also, it could be a good idea to add a comment in UPDATING.md in case someone is currently relying on this functionality.

Copy link
Member Author

@rusackas rusackas Oct 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default settings in this PR mimic the current default behavior, so the feature shouldn't require any action when updating.

That said, the comments do seem valuable. I'll add 'em!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some comments... let me know if they don't make sense.

rusackas marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security-first default settings would be my preferred approach. Is there a reason why the default here can't be secure out of the box?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe there are any security issues that exist in this implementation.

The reason these flags aren't flipped here is that it would cause the example dashboards (and perhaps customer/user dashboards) to suddenly look very broken, with a bunch of noisy HTML displayed, or (in the examples) nothing displayed at all.

}

# Set the default view to card/grid view if thumbnail support is enabled.
Expand Down