fix: improve html escaping of element attributes (#11411)

escape `<` because there's an edge case scenario where a script inside an attribute of a noscript is parsed differently

.changeset/young-ads-roll.md

@@ -0,0 +1,5 @@
+---
+"svelte": patch
+---
+
+fix: improve html escaping of element attributes

packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js

@@ -7,12 +7,7 @@ import {
 	unwrap_optional
 } from '../../../../utils/ast.js';
 import { binding_properties } from '../../../bindings.js';
-import {
-	clean_nodes,
-	determine_namespace_for_children,
-	escape_html,
-	infer_namespace
-} from '../../utils.js';
+import { clean_nodes, determine_namespace_for_children, infer_namespace } from '../../utils.js';
 import { DOMProperties, PassiveEvents, VoidElements } from '../../../constants.js';
 import { is_custom_element_node, is_element_node } from '../../../nodes.js';
 import * as b from '../../../../utils/builders.js';
@@ -38,6 +33,7 @@ import {
 	TRANSITION_IN,
 	TRANSITION_OUT
 } from '../../../../../constants.js';
+import { escape_html } from '../../../../../escaping.js';
 import { regex_is_valid_identifier } from '../../../patterns.js';
 import { javascript_visitors_runes } from './javascript-runes.js';
 import { sanitize_template_string } from '../../../../utils/sanitize_template_string.js';
@@ -1981,7 +1977,7 @@ export const template_visitors = {
 							` ${attribute.name}${
 								DOMBooleanAttributes.includes(name) && literal_value === true
 									? ''
-									: `="${literal_value === true ? '' : escape_html(String(literal_value), true)}"`
+									: `="${literal_value === true ? '' : escape_html(literal_value, true)}"`
 							}`
 						);
 						continue;

packages/svelte/src/compiler/phases/3-transform/server/transform-server.js

@@ -17,19 +17,14 @@ import {
 import {
 	clean_nodes,
 	determine_namespace_for_children,
-	escape_html,
 	infer_namespace,
 	transform_inspect_rune
 } from '../utils.js';
 import { create_attribute, is_custom_element_node, is_element_node } from '../../nodes.js';
 import { binding_properties } from '../../bindings.js';
 import { regex_starts_with_newline, regex_whitespaces_strict } from '../../patterns.js';
-import {
-	DOMBooleanAttributes,
-	HYDRATION_END,
-	HYDRATION_END_ELSE,
-	HYDRATION_START
-} from '../../../../constants.js';
+import { DOMBooleanAttributes, HYDRATION_END, HYDRATION_START } from '../../../../constants.js';
+import { escape_html } from '../../../../escaping.js';
 import { sanitize_template_string } from '../../../utils/sanitize_template_string.js';
 import { BLOCK_CLOSE, BLOCK_CLOSE_ELSE } from '../../../../internal/server/hydration.js';
 
@@ -1747,7 +1742,7 @@ const template_visitors = {
 			if (attribute.type === 'SpreadAttribute') {
 				spreads.push(/** @type {import('estree').Expression} */ (context.visit(attribute)));
 			} else if (attribute.type === 'Attribute') {
-				const value = serialize_attribute_value(attribute.value, context);
+				const value = serialize_attribute_value(attribute.value, context, false, true);
 				if (attribute.name === 'name') {
 					expression = b.member(b.member_id('$$props.$$slots'), value, true, true);
 				} else if (attribute.name !== 'slot') {

packages/svelte/src/compiler/phases/3-transform/utils.js

@@ -6,55 +6,6 @@ import {
 import * as b from '../../utils/builders.js';
 import { walk } from 'zimmerframe';
 
-/**
- * @param {string} s
- * @param {boolean} [attr]
- */
-export function escape_html(s, attr) {
-	if (typeof s !== 'string') return s;
-	const delimiter = attr ? '"' : '<';
-	const escaped_delimiter = attr ? '&quot;' : '&lt;';
-	let i_delimiter = s.indexOf(delimiter);
-	let i_ampersand = s.indexOf('&');
-
-	if (i_delimiter < 0 && i_ampersand < 0) return s;
-
-	let left = 0,
-		out = '';
-
-	while (i_delimiter >= 0 && i_ampersand >= 0) {
-		if (i_delimiter < i_ampersand) {
-			if (left < i_delimiter) out += s.substring(left, i_delimiter);
-			out += escaped_delimiter;
-			left = i_delimiter + 1;
-			i_delimiter = s.indexOf(delimiter, left);
-		} else {
-			if (left < i_ampersand) out += s.substring(left, i_ampersand);
-			out += '&amp;';
-			left = i_ampersand + 1;
-			i_ampersand = s.indexOf('&', left);
-		}
-	}
-
-	if (i_delimiter >= 0) {
-		do {
-			if (left < i_delimiter) out += s.substring(left, i_delimiter);
-			out += escaped_delimiter;
-			left = i_delimiter + 1;
-			i_delimiter = s.indexOf(delimiter, left);
-		} while (i_delimiter >= 0);
-	} else if (!attr) {
-		while (i_ampersand >= 0) {
-			if (left < i_ampersand) out += s.substring(left, i_ampersand);
-			out += '&amp;';
-			left = i_ampersand + 1;
-			i_ampersand = s.indexOf('&', left);
-		}
-	}
-
-	return left < s.length ? out + s.substring(left) : out;
-}
-
 /**
  * @param {import('estree').Node} node
  * @returns {boolean}

packages/svelte/src/escaping.js

@@ -0,0 +1,26 @@
+const ATTR_REGEX = /[&"<]/g;
+const CONTENT_REGEX = /[&<]/g;
+
+/**
+ * @template V
+ * @param {V} value
+ * @param {boolean} [is_attr]
+ */
+export function escape_html(value, is_attr) {
+	const str = String(value ?? '');
+
+	const pattern = is_attr ? ATTR_REGEX : CONTENT_REGEX;
+	pattern.lastIndex = 0;
+
+	let escaped = '';
+	let last = 0;
+
+	while (pattern.test(str)) {
+		const i = pattern.lastIndex - 1;
+		const ch = str[i];
+		escaped += str.substring(last, i) + (ch === '&' ? '&amp;' : ch === '"' ? '&quot;' : '&lt;');
+		last = i + 1;
+	}
+
+	return escaped + str.substring(last);
+}

packages/svelte/src/internal/server/index.js

@@ -8,6 +8,7 @@ import {
 	interactive_elements,
 	is_tag_valid_with_parent
 } from '../../constants.js';
+import { escape_html } from '../../escaping.js';
 import { DEV } from 'esm-env';
 import { current_component, pop, push } from './context.js';
 import { BLOCK_CLOSE, BLOCK_OPEN } from './hydration.js';
@@ -39,8 +40,6 @@ import { validate_store } from '../shared/validate.js';
  * }} Payload
  */
 
-const ATTR_REGEX = /[&"]/g;
-const CONTENT_REGEX = /[&<]/g;
 // https://html.spec.whatwg.org/multipage/syntax.html#attributes-2
 // https://infra.spec.whatwg.org/#noncharacter
 const INVALID_ATTR_NAME_CHAR_REGEX =
@@ -214,31 +213,6 @@ export function render(component, options) {
 	};
 }
 
-/**
- * @template V
- * @param {V} value
- * @param {any} is_attr
- * @returns {string}
- */
-export function escape(value, is_attr = false) {
-	const str = String(value ?? '');
-
-	const pattern = is_attr ? ATTR_REGEX : CONTENT_REGEX;
-	pattern.lastIndex = 0;
-
-	let escaped = '';
-	let last = 0;
-
-	while (pattern.test(str)) {
-		const i = pattern.lastIndex - 1;
-		const ch = str[i];
-		escaped += str.substring(last, i) + (ch === '&' ? '&amp;' : ch === '"' ? '&quot;' : '&lt;');
-		last = i + 1;
-	}
-
-	return escaped + str.substring(last);
-}
-
 /**
  * @param {Payload} payload
  * @param {(head_payload: Payload['head']) => void} fn
@@ -260,7 +234,7 @@ export function head(payload, fn) {
  */
 export function attr(name, value, boolean) {
 	if (value == null || (!value && boolean) || (value === '' && name === 'class')) return '';
-	const assignment = boolean ? '' : `="${escape(value, true)}"`;
+	const assignment = boolean ? '' : `="${escape_html(value, true)}"`;
 	return ` ${name}${assignment}`;
 }
 
@@ -381,7 +355,7 @@ export function stringify(value) {
 function style_object_to_string(style_object) {
 	return Object.keys(style_object)
 		.filter(/** @param {any} key */ (key) => style_object[key])
-		.map(/** @param {any} key */ (key) => `${key}: ${escape(style_object[key], true)};`)
+		.map(/** @param {any} key */ (key) => `${key}: ${escape_html(style_object[key], true)};`)
 		.join(' ');
 }
 
@@ -654,3 +628,5 @@ export {
 	validate_snippet,
 	validate_void_dynamic_element
 } from '../shared/validate.js';
+
+export { escape_html as escape };

packages/svelte/tests/runtime-legacy/samples/escaped-attr-2/_config.js

@@ -0,0 +1,7 @@
+import { test } from '../../test';
+
+export default test({
+	test({ assert, logs }) {
+		assert.deepEqual(logs, []);
+	}
+});

packages/svelte/tests/runtime-legacy/samples/escaped-attr-2/main.svelte

@@ -0,0 +1,8 @@
+<script>
+	const x = `</noscript><script>console.log('should not run')<` + `/script>`
+</script>
+
+<noscript>
+	<a href={x}>test</a>
+</noscript>
+

packages/svelte/tests/runtime-legacy/samples/escaped-attr-3/_config.js

@@ -0,0 +1,7 @@
+import { test } from '../../test';
+
+export default test({
+	test({ assert, target }) {
+		assert.htmlEqual(target.innerHTML, '<div title="&amp;<">blah</div>');
+	}
+});

packages/svelte/tests/runtime-legacy/samples/escaped-attr-3/main.svelte

@@ -0,0 +1 @@
+<div title="&amp;&lt;">blah</div>

packages/svelte/tests/runtime-legacy/samples/escaped-attr/_config.js

@@ -0,0 +1,7 @@
+import { test } from '../../test';
+
+export default test({
+	test({ assert, logs }) {
+		assert.deepEqual(logs, []);
+	}
+});

packages/svelte/tests/runtime-legacy/samples/escaped-attr/main.svelte

@@ -0,0 +1,4 @@
+<noscript>
+	<a href="</noscript><script>console.log('should not run')</script>">test</a>
+</noscript>
+

playgrounds/sandbox/run.js

@@ -50,7 +50,7 @@ for (const generate of ['client', 'server']) {
 
 			try {
 				const migrated = migrate(source);
-				fs.writeFileSync(`${cwd}/output/${file}.migrated.svelte`, migrated);
+				fs.writeFileSync(`${cwd}/output/${file}.migrated.svelte`, migrated.code);
 			} catch (e) {
 				console.warn(`Error migrating ${file}`, e);
 			}