Mark fragment-parsed SVG <script> elements as already started [chromium/src : main]
Dan Clark (Gerrit)
Dan Clark added 3 comments![]( "Open in Gerrit")
if (is_parsing_fragment_ && namespace_uri == svg_names::kNamespaceURI &&Out of an abundance of caution let's gate this change behind a runtime_enabled_features.json5 flag, set to `status: "stable"`.
if (is_parsing_fragment_ && namespace_uri == svg_names::kNamespaceURI &&Not blocking for this CL, but it might be worth investigating whether there's a variant of this issue when the script is created by the XML parser for an XHTML document. That'd bypass this fix.
parser_content_policy_ !=How about refactoring these conditions into a helper that can be shared with the HTMLScriptElement version in `HTMLConstructionSite::InsertScriptElement`?
```c++
bool HTMLConstructionSite::ShouldMarkScriptAlreadyStarted() const {
return is_parsing_fragment_ &&
parser_content_policy_ !=
kAllowScriptingContentAndDoNotMarkAlreadyStarted &&
parser_content_policy_ !=
kAllowScriptingContentAndMarkAsParserInserted;
}
```
Related details
- Leo Lee
- Noam Rosenthal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 1 comment![]( "Open in Gerrit")
parser_content_policy_ !=How about refactoring these conditions into a helper that can be shared with the HTMLScriptElement version in `HTMLConstructionSite::InsertScriptElement`?
```c++
bool HTMLConstructionSite::ShouldMarkScriptAlreadyStarted() const {
return is_parsing_fragment_ &&
parser_content_policy_ !=
kAllowScriptingContentAndDoNotMarkAlreadyStarted &&
parser_content_policy_ !=
kAllowScriptingContentAndMarkAsParserInserted;
}
```
Seems like this is mostly/partially duplicated from https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/parser/html_construction_site.cc;l=1162?
If so, can it be a helper method that's used both by SVG script and HTML script elements?
Related details
- Leo Lee
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leo Lee (Gerrit)
Leo Lee added 4 comments![]( "Open in Gerrit")
Added the runtime flag (stable) as a kill switch, and extracted ShouldMarkScriptAlreadyStarted() shared by the SVG and HTML script paths. The XML/XHTML parser variant is tracked separately in crbug.com/540021703 assigned to me. Thanks Dan and Noam.
if (is_parsing_fragment_ && namespace_uri == svg_names::kNamespaceURI &&Out of an abundance of caution let's gate this change behind a runtime_enabled_features.json5 flag, set to `status: "stable"`.
Done
if (is_parsing_fragment_ && namespace_uri == svg_names::kNamespaceURI &&Not blocking for this CL, but it might be worth investigating whether there's a variant of this issue when the script is created by the XML parser for an XHTML document. That'd bypass this fix.
Confirmed and filed as a security follow-up: crbug.com/540021703.
Noam RosenthalHow about refactoring these conditions into a helper that can be shared with the HTMLScriptElement version in `HTMLConstructionSite::InsertScriptElement`?
```c++
bool HTMLConstructionSite::ShouldMarkScriptAlreadyStarted() const {
return is_parsing_fragment_ &&
parser_content_policy_ !=
kAllowScriptingContentAndDoNotMarkAlreadyStarted &&
parser_content_policy_ !=
kAllowScriptingContentAndMarkAsParserInserted;
}
```
Seems like this is mostly/partially duplicated from https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/parser/html_construction_site.cc;l=1162?
If so, can it be a helper method that's used both by SVG script and HTML script elements?
Extracted ShouldMarkScriptAlreadyStarted() and it's now used by both the SVG <script> path here and InsertScriptElement().
Related details
- Dan Clark
- Noam Rosenthal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal voted and added 2 comments![]( "Open in Gerrit")
Votes added by Noam Rosenthal
| Code-Review | +1 |
2 comments
LGTM % nit
namespace_uri == svg_names::kNamespaceURI &&Maybe take this (and below) from tag_name?
Related details
- Dan Clark
- Leo Lee
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leo Lee (Gerrit)
Leo Lee added 1 comment![]( "Open in Gerrit")
Maybe take this (and below) from tag_name?
- Dan Clark
- Noam Rosenthal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dan Clark (Gerrit)
Dan Clark voted Code-Review+1![]( "Open in Gerrit")
- Leo Lee
- Noam Rosenthal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Leo Lee
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/61629.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
- Leo Lee
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leo Lee (Gerrit)
Leo Lee voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Mark fragment-parsed SVG <script> elements as already started
The HTML fragment parsing algorithm marks parser-created script elements
"already started" so they never execute. Blink applied this only to HTML
scripts in InsertScriptElement(); foreign-content scripts (e.g. SVG
<script>) are created in CreateElement() and were left unmarked. As a
result an SVG <script> injected via innerHTML stayed inert but executed
when the option subtree was later cloned into the document (e.g. by
<selectedcontent>), which was an XSS vector.
Mark fragment-parsed SVG script elements already started, using the same
policy as the HTML script path, and add a regression WPT.
- M third_party/blink/renderer/core/html/parser/html_construction_site.cc
- M third_party/blink/renderer/core/html/parser/html_construction_site.h
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- A third_party/blink/web_tests/external/wpt/html/semantics/scripting-1/the-script-element/svg-script-fragment-parsed-not-executed-on-clone.html
Code-Review: +1 by Dan Clark, +1 by Noam Rosenthal
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/61629
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |