Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@linode/manager": Tech Stories
---

Replace sanitize-html with dompurify ([#10378](https://github.com/linode/manager/pull/10378))
4 changes: 2 additions & 2 deletions packages/manager/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
"chart.js": "~2.9.4",
"copy-to-clipboard": "^3.0.8",
"country-region-data": "^1.4.5",
"dompurify": "^3.1.0",
"flag-icons": "^6.6.5",
"font-logos": "^0.18.0",
"formik": "~2.1.3",
Expand Down Expand Up @@ -70,7 +71,6 @@
"redux": "^4.0.4",
"redux-thunk": "^2.3.0",
"reselect": "^4.0.0",
"sanitize-html": "^2.12.1",
"search-string": "^3.1.0",
"throttle-debounce": "^2.0.0",
"tss-react": "^4.8.2",
Expand Down Expand Up @@ -132,6 +132,7 @@
"@types/chai-string": "^1.4.5",
"@types/chart.js": "^2.9.21",
"@types/css-mediaquery": "^0.1.1",
"@types/dompurify": "^3.0.5",
"@types/he": "^1.1.0",
"@types/highlight.js": "~10.1.0",
"@types/jsdom": "^21.1.4",
Expand All @@ -153,7 +154,6 @@
"@types/react-select": "^3.0.11",
"@types/recompose": "^0.30.0",
"@types/redux-mock-store": "^1.0.1",
"@types/sanitize-html": "^2.9.1",
"@types/throttle-debounce": "^1.0.0",
"@types/uuid": "^3.4.3",
"@types/yup": "^0.29.13",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { unsafe_MarkdownIt } from 'src/utilities/markdown';
import { sanitizeHTML } from 'src/utilities/sanitizeHTML';
import { useColorMode } from 'src/utilities/theme';

import type { IOptions } from 'sanitize-html';
import type { SanitizeOptions } from 'src/utilities/sanitizeHTML';

hljs.registerLanguage('apache', apache);
hljs.registerLanguage('bash', bash);
Expand All @@ -35,7 +35,7 @@ export type SupportedLanguage =
export interface HighlightedMarkdownProps {
className?: string;
language?: SupportedLanguage;
sanitizeOptions?: IOptions;
sanitizeOptions?: SanitizeOptions;
textOrMarkdown: string;
}

Expand Down Expand Up @@ -88,8 +88,7 @@ export const HighlightedMarkdown = (props: HighlightedMarkdownProps) => {
const unsafe_parsedMarkdown = unsafe_MarkdownIt.render(textOrMarkdown);

const sanitizedHtml = sanitizeHTML({
// eslint-disable-next-line xss/no-mixed-html
options: sanitizeOptions,
sanitizeOptions,
sanitizingTier: 'flexible',
text: unsafe_parsedMarkdown,
});
Expand Down
9 changes: 8 additions & 1 deletion packages/manager/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,14 @@ export const allowedHTMLTagsFlexible: string[] = [
'tr',
];

export const allowedHTMLAttr = ['href', 'lang', 'title', 'align'];
export const allowedHTMLAttr = [
'href',
'lang',
'title',
'align',
'class',
'rel',

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We allow them, but then we strip them all unless it is for <span class="version" />

This will be all cleaned up once we get rid of out eventMessageGenerator transformers, which is part of this epic

];

/**
* MBps rate for intra DC migrations (AKA Mutations)
Expand Down
2 changes: 1 addition & 1 deletion packages/manager/src/features/Events/EventRow.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ export const Row = (props: RowProps) => {
<TableCell data-qa-event-message-cell parentColumn="Event">
<HighlightedMarkdown
sanitizeOptions={{
allowedTags: ['a'],
ALLOWED_TAGS: ['a'],
disallowedTagsMode: 'discard',
}}
textOrMarkdown={message}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { useTheme } from '@mui/material';
import { useSnackbar } from 'notistack';
import * as React from 'react';
import { useParams } from 'react-router-dom';
import sanitize from 'sanitize-html';

import { ActionsPanel } from 'src/components/ActionsPanel/ActionsPanel';
import { Drawer } from 'src/components/Drawer';
Expand All @@ -19,6 +18,7 @@ import {
import { useGrants, useProfile } from 'src/queries/profile';
import { getAPIErrorOrDefault } from 'src/utilities/errorUtils';
import { getEntityIdsByPermission } from 'src/utilities/grants';
import { sanitizeHTML } from 'src/utilities/sanitizeHTML';

interface Props {
helperText: string;
Expand Down Expand Up @@ -94,10 +94,14 @@ export const AddLinodeDrawer = (props: Props) => {
};

const errorNotice = () => {
let errorMsg = sanitize(localError || '', {
allowedAttributes: {},
allowedTags: [], // Disallow all HTML tags,
});
let errorMsg = sanitizeHTML({
sanitizeOptions: {
ALLOWED_ATTR: [],
ALLOWED_TAGS: [], // Disallow all HTML tags,
},
sanitizingTier: 'strict',
text: localError || '',
}).toString();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we need to satisfy the types here with toString()

would love to confirm those changes are good with people familiar with this particular Firewall files. Maybe @carrillo-erik ?

// match something like: Linode <linode_label> (ID <linode_id>)

const linode = /Linode (.+?) \(ID ([^\)]+)\)/i.exec(errorMsg);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import { NodeBalancer } from '@linode/api-v4';
import { useTheme } from '@mui/material';
import { useQueryClient } from '@tanstack/react-query';
import { useSnackbar } from 'notistack';
import * as React from 'react';
import { useQueryClient } from '@tanstack/react-query';
import { useParams } from 'react-router-dom';
import sanitize from 'sanitize-html';

import { ActionsPanel } from 'src/components/ActionsPanel/ActionsPanel';
import { Drawer } from 'src/components/Drawer';
Expand All @@ -22,6 +21,7 @@ import { queryKey } from 'src/queries/nodebalancers';
import { useGrants, useProfile } from 'src/queries/profile';
import { getAPIErrorOrDefault } from 'src/utilities/errorUtils';
import { getEntityIdsByPermission } from 'src/utilities/grants';
import { sanitizeHTML } from 'src/utilities/sanitizeHTML';

interface Props {
helperText: string;
Expand Down Expand Up @@ -103,10 +103,14 @@ export const AddNodebalancerDrawer = (props: Props) => {
};

const errorNotice = () => {
let errorMsg = sanitize(localError || '', {
allowedAttributes: {},
allowedTags: [], // Disallow all HTML tags,
});
let errorMsg = sanitizeHTML({
sanitizeOptions: {
ALLOWED_ATTR: [],
ALLOWED_TAGS: [], // Disallow all HTML tags,
},
sanitizingTier: 'strict',
text: localError || '',
}).toString();
// match something like: NodeBalancer <nodebalancer_label> (ID <nodebalancer_id>)

const nodebalancer = /NodeBalancer (.+?) \(ID ([^\)]+)\)/i.exec(errorMsg);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export const RenderEvent = React.memo((props: RenderEventProps) => {
<div className={unseenEventClass}>
<HighlightedMarkdown
sanitizeOptions={{
allowedTags: getAllowedHTMLTags('strict'),
ALLOWED_TAGS: getAllowedHTMLTags('strict'),
disallowedTagsMode: 'discard',
}}
textOrMarkdown={message}
Expand Down
114 changes: 59 additions & 55 deletions packages/manager/src/utilities/sanitizeHTML.ts
Original file line number Diff line number Diff line change
@@ -1,77 +1,81 @@
import sanitize from 'sanitize-html';
import DOMPurify from 'dompurify';

import { allowedHTMLAttr } from 'src/constants';

import { getAllowedHTMLTags, isURLValid } from './sanitizeHTML.utils';

import type { AllowedHTMLTagsTier } from './sanitizeHTML.utils';
import type { IOptions } from 'sanitize-html';
import type { Config } from 'dompurify';

type DisallowedTagsMode = 'discard' | 'escape';

export interface SanitizeOptions extends Config {
disallowedTagsMode?: DisallowedTagsMode;
}

interface SanitizeHTMLOptions {
allowMoreTags?: string[];
disallowedTagsMode?: IOptions['disallowedTagsMode'];
options?: IOptions;
disallowedTagsMode?: DisallowedTagsMode;
sanitizeOptions?: Config;
sanitizingTier: AllowedHTMLTagsTier;
text: string;
}

export const sanitizeHTML = ({
allowMoreTags,
disallowedTagsMode = 'escape',
options = {},
sanitizeOptions: options = {},
sanitizingTier,
text,
}: SanitizeHTMLOptions) =>
sanitize(text, {
allowedAttributes: {
'*': allowedHTMLAttr,
// "target" and "rel" are allowed because they are handled in the
// transformTags map below.
a: [...allowedHTMLAttr, 'target', 'rel'],
},
allowedClasses: {
span: ['version'],
},
allowedTags: getAllowedHTMLTags(sanitizingTier, allowMoreTags),
disallowedTagsMode,
transformTags: {
// This transformation function does the following to anchor tags:
// 1. Turns the <a /> into a <span /> if the "href" is invalid
// 2. Adds `rel="noopener noreferrer" if _target is "blank" (for security)
// 3. Removes "target" attribute if it's anything other than "_blank"
// 4. Removes custom "rel" attributes
}: SanitizeHTMLOptions) => {
DOMPurify.setConfig({
ALLOWED_ATTR: allowedHTMLAttr,
ALLOWED_TAGS: getAllowedHTMLTags(sanitizingTier, allowMoreTags),
KEEP_CONTENT: disallowedTagsMode === 'discard' ? false : true,
RETURN_DOM: false,
RETURN_DOM_FRAGMENT: false,
RETURN_TRUSTED_TYPE: false,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

...options,
});

a: (tagName, attribs) => {
// If the URL is invalid, transform to a span.
const href = attribs.href ?? '';
if (href && !isURLValid(href)) {
return {
attribs: {},
tagName: 'span',
};
}
// Define transform function for anchor tags
const anchorHandler = (node: HTMLAnchorElement) => {
const href = node.getAttribute('href') ?? '';

// If this link opens a new tab, add "noopener noreferrer" for security.
const target = attribs.target ?? '';
if (target && target === '_blank') {
return {
attribs: {
...attribs,
rel: 'noopener noreferrer',
},
tagName,
};
}
// If the URL is invalid, transform to a span.
if (href && !isURLValid(href)) {
const span = document.createElement('span');
span.setAttribute('class', node.getAttribute('class') || '');

// Otherwise we don't want to allow the "rel" or "target" attributes.
delete attribs.rel;
delete attribs.target;
if (node.parentNode) {
node.parentNode.replaceChild(span, node);
}
} else {
// If this link opens a new tab, add "noopener noreferrer" for security.
const target = node.getAttribute('target') || '';
if (target === '_blank') {
node.setAttribute('rel', 'noopener noreferrer');
} else {
node.removeAttribute('rel');
node.removeAttribute('target');
}
}
};

return {
attribs,
tagName,
};
},
},
...options,
}).trim();
// Register hooks for DOMPurify
DOMPurify.addHook('uponSanitizeElement', (node, data) => {
if (data.tagName === 'a') {
anchorHandler(node as HTMLAnchorElement);
} else if (data.tagName === 'span') {
// Allow class attribute only for span elements
const classAttr = node.getAttribute('class');
if (classAttr && classAttr.trim() !== 'version') {
node.removeAttribute('class');
}
}
});

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

legacy handlers which I hope to get rid of soon once we refactor event generators


// Perform sanitization
const output = DOMPurify.sanitize(text);
return output.trim();
};
Loading