Skip to content

Commit 7135071

Browse files
authored
fix(trace-viewer): reject unsafe snapshot attribute names (#43911)
1 parent 36e4544 commit 7135071

3 files changed

Lines changed: 57 additions & 18 deletions

File tree

‎packages/isomorphic/stringUtils.ts‎

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -155,13 +155,52 @@ export function escapeRegExp(s: string) {
155155
}
156156

157157
const escaped = { '&': '&amp;', '<': '&lt;', '>': '&gt;', '"': '&quot;', '\'': '&#39;' };
158-
export function escapeHTMLAttribute(s: string): string {
158+
function escapeHTMLAttribute(s: string): string {
159159
return s.replace(/[&<>"']/ug, char => (escaped as any)[char]);
160160
}
161161
export function escapeHTML(s: string): string {
162162
return s.replace(/[&<]/ug, char => (escaped as any)[char]);
163163
}
164164

165+
// All trace-controlled data goes through this class, so escaping and name checks live in one place.
166+
export class HtmlBuilder {
167+
private _chunks: string[] = [];
168+
169+
text(text: string) {
170+
this._chunks.push(escapeHTML(text));
171+
}
172+
173+
openTag(name: string): boolean {
174+
if (!isSafeName(name))
175+
return false;
176+
this._chunks.push('<', name);
177+
return true;
178+
}
179+
180+
attribute(name: string, value: string) {
181+
if (!isSafeName(name))
182+
return;
183+
this._chunks.push(' ', name, '="', escapeHTMLAttribute(value), '"');
184+
}
185+
186+
endOpenTag() {
187+
this._chunks.push('>');
188+
}
189+
190+
closeTag(name: string) {
191+
this._chunks.push('</', name, '>');
192+
}
193+
194+
toString() {
195+
return this._chunks.join('');
196+
}
197+
}
198+
199+
// Reject names that would end the tag or attribute name early. Allow Unicode custom elements such as math-α.
200+
function isSafeName(name: string): boolean {
201+
return !!name && !/[\s=<>\/"']/.test(name);
202+
}
203+
165204
export function longestCommonSubstring(s1: string, s2: string): string {
166205
const n = s1.length;
167206
const m = s2.length;

‎packages/isomorphic/trace/snapshotRenderer.ts‎

Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
* limitations under the License.
1515
*/
1616

17-
import { escapeHTMLAttribute, escapeHTML } from '../stringUtils';
17+
import { HtmlBuilder } from '../stringUtils';
1818

1919
import type { FrameSnapshot, NodeNameAttributesChildNodesSnapshot, NodeSnapshot, ResourceSnapshot, SubtreeReferenceSnapshot } from './trace';
2020
import type { PageEntry } from './entries';
@@ -91,11 +91,11 @@ export class SnapshotRenderer {
9191
}
9292

9393
render(): RenderedFrameSnapshot {
94-
const result: string[] = [];
94+
const result = new HtmlBuilder();
9595
const visit = (n: NodeSnapshot, snapshotIndex: number, parentTag: string | undefined, parentAttrs: [string, string][] | undefined) => {
9696
// Text node.
9797
if (typeof n === 'string') {
98-
result.push(escapeHTML(n));
98+
result.text(n);
9999
return;
100100
}
101101

@@ -114,17 +114,14 @@ export class SnapshotRenderer {
114114
// crafted trace file could include them to achieve XSS.
115115
if (name.toUpperCase() === 'SCRIPT')
116116
return;
117-
// Reject names that would emit extra attributes (space, =, <, >, /).
118-
// Allow Unicode custom elements such as math-α.
119-
if (/[\s=<>\/]/.test(name))
120-
return;
121117
// Element node.
122118
// Note that <noscript> will not be rendered by default in the trace viewer, because
123119
// JS is enabled. So rename it to <x-noscript>.
124120
const upperName = name.toUpperCase();
125121
const nodeName = upperName === 'NOSCRIPT' ? 'X-NOSCRIPT' : name;
126122
const attrs = Object.entries(nodeAttrs || {});
127-
result.push('<', nodeName);
123+
if (!result.openTag(nodeName))
124+
return;
128125
const kCurrentSrcAttribute = '__playwright_current_src__';
129126
const isFrame = upperName === 'IFRAME' || upperName === 'FRAME';
130127
const isAnchor = upperName === 'A';
@@ -176,20 +173,21 @@ export class SnapshotRenderer {
176173
let attrValue = value;
177174
if (!isAnchor && (attr.toLowerCase() === 'href' || attr.toLowerCase() === 'src' || attr === kCurrentSrcAttribute))
178175
attrValue = rewriteURLForCustomProtocol(value);
179-
result.push(' ', attrName, '="', escapeHTMLAttribute(attrValue), '"');
176+
result.attribute(attrName, attrValue);
180177
}
181178
if (upperName === 'STYLE') {
182179
// Style has always exactly one child which is a text node.
183180
const styleContent = typeof children[0] === 'string' ? children[0] : '';
184-
result.push(' ', '__playwright_style_content__', '="', escapeHTMLAttribute(rewriteURLsInStyleSheetForCustomProtocol(styleContent)), '"');
185-
result.push('></', nodeName, '>');
181+
result.attribute('__playwright_style_content__', rewriteURLsInStyleSheetForCustomProtocol(styleContent));
182+
result.endOpenTag();
183+
result.closeTag(nodeName);
186184
return;
187185
}
188-
result.push('>');
186+
result.endOpenTag();
189187
for (const child of children)
190188
visit(child, snapshotIndex, nodeName, attrs);
191189
if (!autoClosing.has(nodeName))
192-
result.push('</', nodeName, '>');
190+
result.closeTag(nodeName);
193191
return;
194192
} else {
195193
// Why are we here? Let's not throw, just in case.
@@ -211,7 +209,7 @@ export class SnapshotRenderer {
211209
// Hide the document in order to prevent flickering. We will unhide once script has processed shadow.
212210
'<style>*,*::before,*::after { visibility: hidden }</style>',
213211
`<script nonce="${scriptNonce}">${snapshotScript(this.viewport(), this._callId, this.snapshotName)}</script>`
214-
].join('') + result.join('');
212+
].join('') + result.toString();
215213
return { value: { html, scriptNonce }, size: html.length };
216214
});
217215

‎tests/library/snapshot-renderer.spec.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -91,11 +91,13 @@ test('TV-124 snapshot renderer nonces the bootstrap script', () => {
9191

9292
test('TV-125 snapshot renderer strips event handler attributes', () => {
9393
const renderer = new SnapshotRenderer(new LRUCache(1_000_000), [], [makeSnapshot({
94-
html: ['HTML', {}, ['BODY', {}, ['IMG', { 'onerror': 'alert(1)', 'src': 'x' }]]],
94+
html: ['HTML', {}, ['BODY', {}, ['IMG', { 'onerror': 'alert(1)', ' onerror': 'alert(2)', 'x onmouseover=alert(3)': 'y', 'src': 'x' }]]],
9595
})], [], 0);
9696
const { html } = renderer.render();
97-
expect(html).not.toContain('onerror="alert(1)"');
98-
expect(html).toContain('src="x"');
97+
expect(html).not.toContain('alert(1)');
98+
expect(html).not.toContain('alert(2)');
99+
expect(html).not.toContain('alert(3)');
100+
expect(html).toContain('<IMG src="x">');
99101
});
100102

101103
test('TV-126 snapshot renderer strips onclick attributes', () => {

0 commit comments

Comments
 (0)