Skip to content

Commit 5bb6f12

Browse files
nolanlawsonwjhsf
andauthored
perf(engine-server): faster <style> validation (#4201)
Co-authored-by: Will Harney <62956339+wjhsf@users.noreply.github.com>
1 parent af03411 commit 5bb6f12

File tree

3 files changed

+68
-28
lines changed

3 files changed

+68
-28
lines changed
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
/*
2+
* Copyright (c) 2024, Salesforce, Inc.
3+
* All rights reserved.
4+
* SPDX-License-Identifier: MIT
5+
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
6+
*/
7+
import { validateStyleTextContents } from '../utils/validate-style-text-contents';
8+
9+
// See https://html.spec.whatwg.org/multipage/syntax.html#cdata-rcdata-restrictions
10+
describe('validateStyleTextContents', () => {
11+
it('throws an error for invalid style text content', () => {
12+
const invalidStrings = [
13+
'</style\t',
14+
'</style\n',
15+
'</style\f',
16+
'</style\r',
17+
'</style ',
18+
'</style>',
19+
'</style/',
20+
];
21+
22+
for (const invalidString of invalidStrings) {
23+
expect(() => validateStyleTextContents(invalidString)).toThrow(
24+
/CSS contains unsafe characters/
25+
);
26+
expect(() => validateStyleTextContents(invalidString.toUpperCase())).toThrow(
27+
/CSS contains unsafe characters/
28+
);
29+
}
30+
});
31+
32+
it('does not throw for valid text content', () => {
33+
const validStrings = ['</style', '</ style>', `data-foo="<>'&"] {}`, `data-foo='"'] {}`];
34+
35+
for (const validString of validStrings) {
36+
expect(() => validateStyleTextContents(validString)).not.toThrow();
37+
expect(() => validateStyleTextContents(validString.toUpperCase())).not.toThrow();
38+
}
39+
});
40+
});

packages/@lwc/engine-server/src/serializer.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,6 @@ function serializeTextContent(contents: string, tagName?: string) {
108108
if (tagName === 'style') {
109109
// Special validation for <style> tags since their content must be served unescaped, and we need to validate
110110
// that the contents are safe to serialize unescaped.
111-
// TODO [#3454]: move this validation to compilation
112111
validateStyleTextContents(contents);
113112
// If we haven't thrown an error during validation, then the content is safe to serialize unescaped
114113
return contents;

packages/@lwc/engine-server/src/utils/validate-style-text-contents.ts

Lines changed: 28 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -5,44 +5,45 @@
55
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
66
*/
77

8-
import * as parse5 from 'parse5';
9-
import { DocumentFragment } from '@parse5/tools';
10-
11-
function isSingleStyleNodeContainingSingleTextNode(node: DocumentFragment) {
12-
if (node.childNodes.length !== 1) {
13-
return false;
14-
}
15-
16-
const style = node.childNodes[0];
17-
18-
if (style.nodeName !== 'style' || style.childNodes.length !== 1) {
19-
return false;
20-
}
21-
22-
const textNode = style.childNodes[0];
23-
24-
return textNode.nodeName === '#text';
25-
}
8+
/**
9+
* Per the HTML spec on restrictions for "raw text elements" like `<style>`:
10+
*
11+
* > The text in raw text and escapable raw text elements must not contain any occurrences of the string
12+
* > "</" (U+003C LESS-THAN SIGN, U+002F SOLIDUS) followed by characters that case-insensitively match the tag name of
13+
* > the element followed by one of:
14+
* > - U+0009 CHARACTER TABULATION (tab)
15+
* > - U+000A LINE FEED (LF)
16+
* > - U+000C FORM FEED (FF)
17+
* > - U+000D CARRIAGE RETURN (CR)
18+
* > - U+0020 SPACE
19+
* > - U+003E GREATER-THAN SIGN (>), or
20+
* > - U+002F SOLIDUS (/)
21+
* @see https://html.spec.whatwg.org/multipage/syntax.html#cdata-rcdata-restrictions
22+
*/
23+
const INVALID_STYLE_CONTENT = /<\/style[\t\n\f\r >/]/i;
2624

2725
/**
28-
* The text content inside `<style>` is a special case. It is _only_ rendered by the LWC engine itself; <style> tags
29-
* are disallowed inside of templates. Also, we want to avoid over-escaping, since CSS containing strings like
30-
* `&amp;` and `&quot;` is not valid CSS (even when inside a `<style>` element).
26+
* The text content inside `<style>` is a special case. It is _only_ rendered by the LWC engine itself; `<style>` tags
27+
* are disallowed inside of HTML templates.
28+
*
29+
* The `<style>` tag is unusual in how it's defined in HTML. Like `<script>`, it is considered a "raw text element,"
30+
* which means that it is parsed as raw text, but certain character sequences are disallowed, namely to avoid XSS
31+
* attacks like `</style><script>alert("pwned")</script>`.
32+
*
33+
* This also means that we cannot use "normal" HTML escaping inside `<style>` tags, e.g. we cannot use `&lt;`,
34+
* `&gt;`, etc., because these are treated as-is by the HTML parser.
35+
*
3136
*
32-
* However, to avoid XSS attacks, we still need to check for things like `</style><script>alert("pwned")</script>`,
33-
* since a user could use that inside of a *.css file to break out of a <style> element.
3437
* @param contents CSS source to validate
3538
* @throws Throws if the contents provided are not valid.
39+
* @see https://html.spec.whatwg.org/multipage/syntax.html#raw-text-elements
3640
* @see https://github.com/salesforce/lwc/issues/3439
3741
* @example
3842
* validateStyleTextContents('div { color: red }') // Ok
3943
* validateStyleTextContents('</style><script>alert("pwned")</script>') // Throws
4044
*/
4145
export function validateStyleTextContents(contents: string): void {
42-
// If parse5 parses this as more than one `<style>` tag, then it is unsafe to be rendered as-is
43-
const fragment = parse5.parseFragment(`<style>${contents}</style>`);
44-
45-
if (!isSingleStyleNodeContainingSingleTextNode(fragment)) {
46+
if (INVALID_STYLE_CONTENT.test(contents)) {
4647
throw new Error(
4748
'CSS contains unsafe characters and cannot be serialized inside a style element'
4849
);

0 commit comments

Comments
 (0)