Visitar URL original
fix(removeScriptElement): backport security hardening to v3 (#2269) · svg/svgo@994a9f0 · GitHub
Skip to content

Commit 994a9f0

Browse files
TrySoundTrySound
authored
fix(removeScriptElement): backport security hardening to v3 (#2269)
## Summary Backport the v4 `removeScripts` security hardening to v3's `removeScriptElement` plugin: - reject executable `data:` URLs and legacy `vbscript:` URLs while preserving inert data URLs - sanitize executable content inside SVG `foreignObject` elements - recognize namespace-prefixed SVG anchors - normalize ASCII tabs and newlines before checking URL schemes - add v3 regression coverage and update the plugin documentation This backports the fixes from #2263, #2264, and #2268 and addresses GHSA-4vpr-x523-8j87 and GHSA-w27v-7q3p-w38r for the v3 release line. ## Validation - `yarn test` — 425 passed, 3 skipped - `yarn lint` - `yarn typecheck` - `git diff --check`
1 parent 72a2388 commit 994a9f0

7 files changed

Lines changed: 245 additions & 36 deletions

File tree

‎docs/03-plugins/remove-scripts.mdx‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,9 @@ This **will** break interactive SVGs that rely on JavaScript.
1717
This plugin performs the following operations:
1818

1919
- Removes [`<script>`](https://developer.mozilla.org/docs/Web/SVG/Element/script) elements.
20+
- Sanitizes HTML inside [`<foreignObject>`](https://developer.mozilla.org/docs/Web/SVG/Element/foreignObject) elements by removing event attributes, `srcdoc`, and executable URLs from HTML URL attributes such as `action`, `data`, `formaction`, `href`, and `src`. The elements and other visual content are preserved.
2021
- Removes [SVG event attributes](https://developer.mozilla.org/docs/Web/SVG/Attribute/Events), such as `onload`, `onclick`, and `oninput`, preserving the element itself.
21-
- Collapses [`<a>`](https://developer.mozilla.org/docs/Web/SVG/Element/a) elements, moving children up to the parent element.
22+
- Collapses [`<a>`](https://developer.mozilla.org/docs/Web/SVG/Element/a) elements whose links use `javascript:`, legacy `vbscript:`, or an executable `data:` media type (`text/html`, `application/xhtml+xml`, or `image/svg+xml`), moving children up to the parent element.
2223

2324
## Usage
2425

‎lib/svgo/tools.js‎

Lines changed: 53 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,56 @@ const removeLeadingZero = (value) => {
145145
};
146146
exports.removeLeadingZero = removeLeadingZero;
147147

148+
const executableDataMediaTypes = new Set([
149+
'application/xhtml+xml',
150+
'image/svg+xml',
151+
'text/html',
152+
]);
153+
154+
/**
155+
* Check whether a URL can contain executable content in a browser.
156+
*
157+
* @param {string} value
158+
* @returns {boolean}
159+
*/
160+
const isExecutableUrl = (value) => {
161+
// Browsers remove ASCII tabs and newlines from URLs before parsing them.
162+
// Normalize them here too so they cannot be embedded in a scheme name.
163+
const normalizedValue = value
164+
.replace(/[\t\n\r]/g, '')
165+
.trimStart()
166+
.toLowerCase();
167+
168+
// `javascript:` is executable in modern browsers. `vbscript:` was only
169+
// implemented by legacy Internet Explorer, but rejecting it is inexpensive,
170+
// provides defense in depth for old environments, and keeps this security-
171+
// oriented plugin's scheme denylist explicit and complete.
172+
if (
173+
normalizedValue.startsWith('javascript:') ||
174+
normalizedValue.startsWith('vbscript:')
175+
) {
176+
return true;
177+
}
178+
179+
// A `data:` URL is not inherently executable: images and other inert media
180+
// are common in SVGs and should remain intact. Only reject media types that
181+
// browsers parse as active HTML, XHTML, or SVG documents.
182+
if (!normalizedValue.startsWith('data:')) {
183+
return false;
184+
}
185+
186+
// Data URLs separate the media type from parameters with `;` and from the
187+
// payload with `,`. A URL without either separator has no valid media type.
188+
const mediaTypeEnd = normalizedValue.slice(5).search(/[;,]/);
189+
if (mediaTypeEnd === -1) {
190+
return false;
191+
}
192+
193+
const mediaType = normalizedValue.slice(5, mediaTypeEnd + 5).trim();
194+
return executableDataMediaTypes.has(mediaType);
195+
};
196+
exports.isExecutableUrl = isExecutableUrl;
197+
148198
/**
149199
* If the current node contains any scripts. This does not check parents or
150200
* children of the node, only the properties and attributes of the node itself.
@@ -158,14 +208,14 @@ const hasScripts = (node) => {
158208
}
159209

160210
if (node.name === 'a') {
161-
const hasJsLinks = Object.entries(node.attributes).some(
211+
const hasExecutableLinks = Object.entries(node.attributes).some(
162212
([attrKey, attrValue]) =>
163213
(attrKey === 'href' || attrKey.endsWith(':href')) &&
164214
attrValue != null &&
165-
attrValue.trimStart().toLowerCase().startsWith('javascript:'),
215+
isExecutableUrl(attrValue),
166216
);
167217

168-
if (hasJsLinks) {
218+
if (hasExecutableLinks) {
169219
return true;
170220
}
171221
}

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
{
22
"packageManager": "yarn@2.4.3",
33
"name": "svgo",
4-
"version": "3.3.4",
4+
"version": "3.3.5",
55
"description": "Nodejs-based tool for optimizing SVG vector graphics files",
66
"license": "MIT",
77
"keywords": [

‎plugins/removeScriptElement.js‎

Lines changed: 75 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
'use strict';
22

33
const { detachNodeFromParent } = require('../lib/xast.js');
4+
const { isExecutableUrl } = require('../lib/svgo/tools.js');
45
const { attrsGroups } = require('./_collections.js');
56

67
exports.name = 'removeScriptElement';
@@ -15,11 +16,19 @@ const eventAttrs = [
1516
...attrsGroups.graphicalEvent,
1617
];
1718

19+
const SVG_NAMESPACE = 'http://www.w3.org/2000/svg';
20+
21+
/** Namespaces that support SVG <foreignObject> elements. */
22+
const FOREIGN_OBJECT_NAMESPACES = [SVG_NAMESPACE];
23+
24+
/** Namespaces that support SVG <a> elements. */
25+
const ANCHOR_NAMESPACES = [SVG_NAMESPACE];
26+
1827
/** Namespaces that support executable <script> elements. */
19-
const SCRIPT_NAMESPACES = [
20-
'http://www.w3.org/2000/svg',
21-
'http://www.w3.org/1999/xhtml',
22-
];
28+
const SCRIPT_NAMESPACES = [SVG_NAMESPACE, 'http://www.w3.org/1999/xhtml'];
29+
30+
/** Attributes that can load or navigate to executable documents in HTML. */
31+
const HTML_URL_ATTRS = new Set(['action', 'data', 'formaction', 'href', 'src']);
2332

2433
/**
2534
* @param {string} elem
@@ -62,6 +71,7 @@ exports.fn = () => {
6271
*
6372
* @type {Map<string, string[]>} */
6473
const prefixes = new Map();
74+
let foreignObjectDepth = 0;
6575

6676
return {
6777
element: {
@@ -80,20 +90,55 @@ exports.fn = () => {
8090
}
8191
}
8292

93+
if (
94+
isNamespaceAwareElem(
95+
node.name,
96+
'foreignObject',
97+
prefixes,
98+
FOREIGN_OBJECT_NAMESPACES,
99+
)
100+
) {
101+
foreignObjectDepth += 1;
102+
}
103+
83104
if (
84105
isNamespaceAwareElem(node.name, 'script', prefixes, SCRIPT_NAMESPACES)
85106
) {
86107
detachNodeFromParent(node, parentNode);
87108
return;
88109
}
89110

90-
for (const attr of eventAttrs) {
91-
if (node.attributes[attr] != null) {
111+
for (const [attr, value] of Object.entries(node.attributes)) {
112+
const localAttr = attr.slice(attr.lastIndexOf(':') + 1).toLowerCase();
113+
const isEventAttr =
114+
eventAttrs.includes(attr) ||
115+
(foreignObjectDepth > 0 && localAttr.startsWith('on'));
116+
const isEmbeddedDocumentAttr =
117+
foreignObjectDepth > 0 && localAttr === 'srcdoc';
118+
const isExecutableHtmlUrl =
119+
foreignObjectDepth > 0 &&
120+
HTML_URL_ATTRS.has(localAttr) &&
121+
isExecutableUrl(value);
122+
123+
if (isEventAttr || isEmbeddedDocumentAttr || isExecutableHtmlUrl) {
92124
delete node.attributes[attr];
93125
}
94126
}
95127
},
96128
exit: (node, parentNode) => {
129+
const isForeignObject = isNamespaceAwareElem(
130+
node.name,
131+
'foreignObject',
132+
prefixes,
133+
FOREIGN_OBJECT_NAMESPACES,
134+
);
135+
const isAnchor = isNamespaceAwareElem(
136+
node.name,
137+
'a',
138+
prefixes,
139+
ANCHOR_NAMESPACES,
140+
);
141+
97142
for (const k of Object.keys(node.attributes)) {
98143
if (!k.startsWith('xmlns:')) {
99144
continue;
@@ -103,34 +148,33 @@ exports.fn = () => {
103148
/** @type {string[]} */ (prefixes.get(prefix)).pop();
104149
}
105150

106-
if (node.name !== 'a') {
107-
return;
108-
}
109-
110-
for (const attr of Object.keys(node.attributes)) {
111-
if (attr === 'href' || attr.endsWith(':href')) {
112-
if (
113-
node.attributes[attr] == null ||
114-
!node.attributes[attr]
115-
.trimStart()
116-
.toLowerCase()
117-
.startsWith('javascript:')
118-
) {
119-
continue;
120-
}
121-
122-
const index = parentNode.children.indexOf(node);
123-
parentNode.children.splice(index, 1, ...node.children);
124-
125-
// TODO remove legacy parentNode in v4
126-
for (const child of node.children) {
127-
Object.defineProperty(child, 'parentNode', {
128-
writable: true,
129-
value: parentNode,
130-
});
151+
if (isAnchor) {
152+
for (const attr of Object.keys(node.attributes)) {
153+
if (attr === 'href' || attr.endsWith(':href')) {
154+
if (
155+
node.attributes[attr] == null ||
156+
!isExecutableUrl(node.attributes[attr])
157+
) {
158+
continue;
159+
}
160+
161+
const index = parentNode.children.indexOf(node);
162+
parentNode.children.splice(index, 1, ...node.children);
163+
164+
// TODO remove legacy parentNode in v4
165+
for (const child of node.children) {
166+
Object.defineProperty(child, 'parentNode', {
167+
writable: true,
168+
value: parentNode,
169+
});
170+
}
131171
}
132172
}
133173
}
174+
175+
if (isForeignObject) {
176+
foreignObjectDepth -= 1;
177+
}
134178
},
135179
},
136180
};

‎test/plugins/removeScriptElement.09.svg‎

Lines changed: 34 additions & 0 deletions
Loading

‎test/plugins/removeScriptElement.10.svg‎

Lines changed: 42 additions & 0 deletions
Loading

‎test/plugins/removeScriptElement.11.svg‎

Lines changed: 38 additions & 0 deletions
Loading

0 commit comments

Comments
 (0)