You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ru...@apache.org on 2023/12/07 21:38:01 UTC
(superset) 01/01: Wave 1
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch reduce-innerHTML-use
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 1a20a9812ac5dc7e9b9c13a28d6d3bb2d3938940
Author: Evan Rusackas <ev...@rusackas.com>
AuthorDate: Thu Dec 7 14:37:48 2023 -0700
Wave 1
---
superset-frontend/.eslintrc.js | 1 +
superset-frontend/package-lock.json | 17 +++++++++++
superset-frontend/package.json | 1 +
.../packages/superset-ui-core/src/utils/index.ts | 1 +
.../src/utils/{index.ts => sanitizers.ts} | 35 ++++++++++++----------
.../test/chart/components/reactify.test.tsx | 2 ++
.../src/components/DynamicEditableTitle/index.tsx | 2 +-
.../src/components/DynamicPlugins/index.tsx | 6 +++-
superset-frontend/src/components/Icons/Icon.tsx | 13 ++++++--
.../src/dashboard/util/injectCustomCss.ts | 2 ++
superset-frontend/src/embedded/index.tsx | 2 +-
.../utils/client-ws-app/public/javascripts/app.js | 6 ++--
12 files changed, 64 insertions(+), 24 deletions(-)
diff --git a/superset-frontend/.eslintrc.js b/superset-frontend/.eslintrc.js
index 6eebb0d2df..048f0f1213 100644
--- a/superset-frontend/.eslintrc.js
+++ b/superset-frontend/.eslintrc.js
@@ -42,6 +42,7 @@ module.exports = {
'prettier',
'prettier/react',
'plugin:react-hooks/recommended',
+ 'plugin:no-unsanitized/DOM',
],
parser: '@babel/eslint-parser',
parserOptions: {
diff --git a/superset-frontend/package-lock.json b/superset-frontend/package-lock.json
index c4e771b259..ff4f18189d 100644
--- a/superset-frontend/package-lock.json
+++ b/superset-frontend/package-lock.json
@@ -238,6 +238,7 @@
"eslint-plugin-jest-dom": "^3.6.5",
"eslint-plugin-jsx-a11y": "^6.4.1",
"eslint-plugin-no-only-tests": "^2.4.0",
+ "eslint-plugin-no-unsanitized": "^4.0.2",
"eslint-plugin-prettier": "^4.0.0",
"eslint-plugin-react": "^7.22.0",
"eslint-plugin-react-hooks": "^4.2.0",
@@ -30944,6 +30945,15 @@
"node": ">=4.0.0"
}
},
+ "node_modules/eslint-plugin-no-unsanitized": {
+ "version": "4.0.2",
+ "resolved": "https://registry.npmjs.org/eslint-plugin-no-unsanitized/-/eslint-plugin-no-unsanitized-4.0.2.tgz",
+ "integrity": "sha512-Pry0S9YmHoz8NCEMRQh7N0Yexh2MYCNPIlrV52hTmS7qXnTghWsjXouF08bgsrrZqaW9tt1ZiK3j5NEmPE+EjQ==",
+ "dev": true,
+ "peerDependencies": {
+ "eslint": "^6 || ^7 || ^8"
+ }
+ },
"node_modules/eslint-plugin-prettier": {
"version": "4.0.0",
"resolved": "https://registry.npmjs.org/eslint-plugin-prettier/-/eslint-plugin-prettier-4.0.0.tgz",
@@ -88668,6 +88678,13 @@
"integrity": "sha512-azP9PwQYfGtXJjW273nIxQH9Ygr+5/UyeW2wEjYoDtVYPI+WPKwbj0+qcAKYUXFZLRumq4HKkFaoDBAwBoXImQ==",
"dev": true
},
+ "eslint-plugin-no-unsanitized": {
+ "version": "4.0.2",
+ "resolved": "https://registry.npmjs.org/eslint-plugin-no-unsanitized/-/eslint-plugin-no-unsanitized-4.0.2.tgz",
+ "integrity": "sha512-Pry0S9YmHoz8NCEMRQh7N0Yexh2MYCNPIlrV52hTmS7qXnTghWsjXouF08bgsrrZqaW9tt1ZiK3j5NEmPE+EjQ==",
+ "dev": true,
+ "requires": {}
+ },
"eslint-plugin-prettier": {
"version": "4.0.0",
"resolved": "https://registry.npmjs.org/eslint-plugin-prettier/-/eslint-plugin-prettier-4.0.0.tgz",
diff --git a/superset-frontend/package.json b/superset-frontend/package.json
index 3b6310fa71..0fd65b8608 100644
--- a/superset-frontend/package.json
+++ b/superset-frontend/package.json
@@ -303,6 +303,7 @@
"eslint-plugin-jest-dom": "^3.6.5",
"eslint-plugin-jsx-a11y": "^6.4.1",
"eslint-plugin-no-only-tests": "^2.4.0",
+ "eslint-plugin-no-unsanitized": "^4.0.2",
"eslint-plugin-prettier": "^4.0.0",
"eslint-plugin-react": "^7.22.0",
"eslint-plugin-react-hooks": "^4.2.0",
diff --git a/superset-frontend/packages/superset-ui-core/src/utils/index.ts b/superset-frontend/packages/superset-ui-core/src/utils/index.ts
index 32fa88251e..1ac83b863a 100644
--- a/superset-frontend/packages/superset-ui-core/src/utils/index.ts
+++ b/superset-frontend/packages/superset-ui-core/src/utils/index.ts
@@ -32,3 +32,4 @@ export * from './featureFlags';
export * from './random';
export * from './typedMemo';
export * from './html';
+export * from './sanitizers';
diff --git a/superset-frontend/packages/superset-ui-core/src/utils/index.ts b/superset-frontend/packages/superset-ui-core/src/utils/sanitizers.ts
similarity index 50%
copy from superset-frontend/packages/superset-ui-core/src/utils/index.ts
copy to superset-frontend/packages/superset-ui-core/src/utils/sanitizers.ts
index 32fa88251e..ad73ee8056 100644
--- a/superset-frontend/packages/superset-ui-core/src/utils/index.ts
+++ b/superset-frontend/packages/superset-ui-core/src/utils/sanitizers.ts
@@ -16,19 +16,22 @@
* specific language governing permissions and limitations
* under the License.
*/
-export { default as convertKeysToCamelCase } from './convertKeysToCamelCase';
-export { default as ensureIsArray } from './ensureIsArray';
-export { default as ensureIsInt } from './ensureIsInt';
-export { default as isDefined } from './isDefined';
-export { default as isRequired } from './isRequired';
-export { default as isEqualArray } from './isEqualArray';
-export { default as makeSingleton } from './makeSingleton';
-export { default as promiseTimeout } from './promiseTimeout';
-export { default as logging } from './logging';
-export { default as removeDuplicates } from './removeDuplicates';
-export { lruCache } from './lruCache';
-export { getSelectedText } from './getSelectedText';
-export * from './featureFlags';
-export * from './random';
-export * from './typedMemo';
-export * from './html';
+export function sanitizeFileName(fileName: string) {
+ // Allow only alphanumeric characters, hyphens, and underscores
+ return fileName.replace(/[^a-zA-Z0-9-_]/g, '');
+}
+
+export function sanitizeUrl(url: string) {
+ try {
+ // Attempt to create a URL object. If this fails, the URL is invalid
+ // eslint-disable-next-line no-new
+ new URL(url);
+
+ // The URL is valid if no error was thrown.
+ // Proceed with removing JavaScript: and data: protocols to prevent XSS attacks
+ return url.replace(/(javascript:|data:)/gi, '');
+ } catch (e) {
+ // If URL is invalid, return an empty string or handle as needed
+ return '';
+ }
+}
diff --git a/superset-frontend/packages/superset-ui-core/test/chart/components/reactify.test.tsx b/superset-frontend/packages/superset-ui-core/test/chart/components/reactify.test.tsx
index 3277a6fece..a1086c14f9 100644
--- a/superset-frontend/packages/superset-ui-core/test/chart/components/reactify.test.tsx
+++ b/superset-frontend/packages/superset-ui-core/test/chart/components/reactify.test.tsx
@@ -29,6 +29,8 @@ describe('reactify(renderFn)', () => {
const container = element;
container.innerHTML = '';
const child = document.createElement('b');
+ // TODO: make this safer
+ // eslint-disable-next-line no-unsanitized/property
child.innerHTML = props.content ?? '';
container.append(child);
},
diff --git a/superset-frontend/src/components/DynamicEditableTitle/index.tsx b/superset-frontend/src/components/DynamicEditableTitle/index.tsx
index 86205bebc2..f2810f615d 100644
--- a/superset-frontend/src/components/DynamicEditableTitle/index.tsx
+++ b/superset-frontend/src/components/DynamicEditableTitle/index.tsx
@@ -113,7 +113,7 @@ export const DynamicEditableTitle = ({
// then we can measure the width of that span to resize the input element
useLayoutEffect(() => {
if (sizerRef?.current) {
- sizerRef.current.innerHTML = (currentTitle || placeholder).replace(
+ sizerRef.current.textContent = (currentTitle || placeholder).replace(
/\s/g,
' ',
);
diff --git a/superset-frontend/src/components/DynamicPlugins/index.tsx b/superset-frontend/src/components/DynamicPlugins/index.tsx
index 9134e73fda..3e2c65fa99 100644
--- a/superset-frontend/src/components/DynamicPlugins/index.tsx
+++ b/superset-frontend/src/components/DynamicPlugins/index.tsx
@@ -25,6 +25,7 @@ import {
getChartMetadataRegistry,
logging,
makeApi,
+ sanitizeUrl,
} from '@superset-ui/core';
import { omitBy } from 'lodash';
@@ -189,7 +190,10 @@ export const DynamicPluginProvider: React.FC = ({ children }) => {
plugins.map(async plugin => {
let error: Error | null = null;
try {
- await import(/* webpackIgnore: true */ plugin.bundle_url);
+ const plugin_bundle_url = sanitizeUrl(plugin.bundle_url);
+ // This is sanitized via a custom util function
+ // eslint-disable-next-line no-unsanitized/method
+ await import(/* webpackIgnore: true */ plugin_bundle_url);
} catch (err) {
logging.error(
`Failed to load plugin ${plugin.key} with the following error:`,
diff --git a/superset-frontend/src/components/Icons/Icon.tsx b/superset-frontend/src/components/Icons/Icon.tsx
index 283fd194ca..2bd951e16a 100644
--- a/superset-frontend/src/components/Icons/Icon.tsx
+++ b/superset-frontend/src/components/Icons/Icon.tsx
@@ -19,7 +19,7 @@
import React, { useEffect, useRef, useState } from 'react';
import AntdIcon from '@ant-design/icons';
-import { styled } from '@superset-ui/core';
+import { styled, sanitizeFileName } from '@superset-ui/core';
import TransparentIcon from 'src/assets/images/icons/transparent.svg';
import IconType from './IconType';
@@ -55,9 +55,18 @@ export const Icon = (props: IconProps) => {
useEffect(() => {
let cancelled = false;
async function importIcon(): Promise<void> {
+ // This is sanitized via a custom util function
+ /* eslint-disable no-unsanitized/method */
+
ImportedSVG.current = (
- await import(`!!@svgr/webpack!src/assets/images/icons/${fileName}.svg`)
+ await import(
+ `!!@svgr/webpack!src/assets/images/icons/${sanitizeFileName(
+ fileName,
+ )}.svg`
+ )
).default;
+ /* eslint-enable no-unsanitized/method */
+
if (!cancelled) {
setLoaded(true);
}
diff --git a/superset-frontend/src/dashboard/util/injectCustomCss.ts b/superset-frontend/src/dashboard/util/injectCustomCss.ts
index 43cb66f7d9..8df8daec7d 100644
--- a/superset-frontend/src/dashboard/util/injectCustomCss.ts
+++ b/superset-frontend/src/dashboard/util/injectCustomCss.ts
@@ -42,6 +42,8 @@ export default function injectCustomCss(css: string) {
if ('styleSheet' in style) {
(style as HTMLStyleElement & MysteryStyleElement).styleSheet.cssText = css;
} else {
+ // TODO: find a way to avoid innerHTML here, or make this less scary
+ // eslint-disable-next-line no-unsanitized/property
style.innerHTML = css;
}
diff --git a/superset-frontend/src/embedded/index.tsx b/superset-frontend/src/embedded/index.tsx
index 50c026fba8..084ef90fa4 100644
--- a/superset-frontend/src/embedded/index.tsx
+++ b/superset-frontend/src/embedded/index.tsx
@@ -72,7 +72,7 @@ const appMountPoint = document.getElementById('app')!;
const MESSAGE_TYPE = '__embedded_comms__';
function showFailureMessage(message: string) {
- appMountPoint.innerHTML = message;
+ appMountPoint.textContent = message;
}
if (!window.parent || window.parent === window) {
diff --git a/superset-websocket/utils/client-ws-app/public/javascripts/app.js b/superset-websocket/utils/client-ws-app/public/javascripts/app.js
index 5afc847a18..d248287934 100644
--- a/superset-websocket/utils/client-ws-app/public/javascripts/app.js
+++ b/superset-websocket/utils/client-ws-app/public/javascripts/app.js
@@ -42,7 +42,7 @@ function connect() {
// Connection opened
socket.addEventListener('open', function () {
socketCount++;
- document.getElementById('socket-count').innerHTML = socketCount;
+ document.getElementById('socket-count').textContent = socketCount;
connect();
socket.send('Hello Server!');
@@ -58,6 +58,6 @@ function connect() {
connect();
setInterval(() => {
- document.getElementById('message-count').innerHTML = messageCount;
- document.getElementById('message-debug').innerHTML = lastMessage;
+ document.getElementById('message-count').textContent = messageCount;
+ document.getElementById('message-debug').textContent = lastMessage;
}, 250);