fix(trace-viewer): reject unsafe snapshot tag names - #42712
Devin Rousso (dcrousso) merged 4 commits into
Conversation
Crafted traces could put markup in the element name so the renderer emitted attributes such as onerror. Drop names that are not a single HTML token. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
Allow WHATWG-valid names such as my-widget_v2 in snapshot HTML. Attribute-smuggling payloads still fail the tag-name check. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Reject only names that smuggle attributes (whitespace, =, <, >, /). Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
| return; | ||
| // Reject names that would emit extra attributes (space, =, <, >, /). | ||
| // Allow Unicode custom elements such as math-α. | ||
| if (!name || /[\s=<>\/]/.test(name)) |
There was a problem hiding this comment.
do we need the !name? everything else seems to assume it's non-empty so maybe that's a guarantee at this point?
There was a problem hiding this comment.
do we need the
!name? everything else seems to assume it's non-empty so maybe that's a guarantee at this point?
No. Capture never writes an empty nodeName, and '' only emits <>. Dropped the !name check in 169e8e4eb. The smuggle regex stays.
|
|
||
| test('snapshot renderer drops unsafe tag names that smuggle attributes', () => { | ||
| const renderer = new SnapshotRenderer(new LRUCache(1_000_000), [], [makeSnapshot({ | ||
| html: ['HTML', {}, ['BODY', {}, ['img src=x onerror=alert(1)', {}], 'safe']], |
There was a problem hiding this comment.
can you share more info about how this could actually happen? i thought we already did a lot of sanitization that would prevent this so im concerned we're adding additional protection for something that cant actually happen
There was a problem hiding this comment.
can you share more info about how this could actually happen? i thought we already did a lot of sanitization that would prevent this
A recorded page cannot produce this. Capture uses live node.nodeName, and document.createElement('iframe src=...') throws.
A zip opened with show-trace can. Load is JSON.parse then addFrameSnapshot with no name check (traceModernizer.ts).
The existing filters only match exact names (toUpperCase() === 'IFRAME'|'META'|'IMG'). They never see iframe src=https://evil.example, so the iframe src rewrite does not run. Unfixed render of ['iframe src=https://evil.example', {}] is <iframe src=https://evil.example>.
Same crafted-zip model as the SCRIPT / on* / srcdoc / object / embed / doctype guards already in this file.
!name is not needed. Empty name only emits <>. Dropped that check in 169e8e4eb.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Capture never produces an empty nodeName. Keep the smuggle regex. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
Test results for "tests 1"2 failed 7 flaky51987 passed, 1247 skipped Merge workflow run. |
Test results for "MCP"8590 passed, 1446 skipped Merge workflow run. |
Devin Rousso (dcrousso)
left a comment
Devin Rousso (dcrousso)
left a comment
There was a problem hiding this comment.
thanks for the fix!
3cd1025
into
microsoft:main
2 participants
Summary
space,=,<,>,/).math-α.Problem
SnapshotRendererconcatenatesnameinto HTML. Filters foron*, iframesrc, andmeta http-equivonly walk the attrs object. A name such asiframe src=https://evil.examplebecomes real markup those filters never see.script-src 'nonce-…'already blocks inline handlers such asonerror=alert(1). This change is the attribute-smuggle case, not script XSS.Public path:
npx playwright show-traceon a crafted zip.Change
Reject empty names and names matching
/[\s=<>\/]/. Ordinary tags and Unicode custom elements still render.Validation
snapshot renderer drops unsafe tag names that smuggle attributessnapshot renderer still renders ordinary custom elementsincludesmath-αandmy-élémentRelated