-
Notifications
You must be signed in to change notification settings - Fork 27.2k
fix(core): sanitize text bindings on SVG <script> elements #68669
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -257,6 +257,24 @@ export function ɵɵsanitizeUrlOrResourceUrl(unsafeUrl: any, tag: string, prop: | |
| return getUrlSanitizer(tag, prop)(unsafeUrl); | ||
| } | ||
|
|
||
| /** | ||
| * Sanitizes a value bound to a property whose security context depends on the host tag — used | ||
| * for host bindings where the host element is unknown at compile time and the property may be a | ||
| * script-source sink on `<script>` or otherwise an HTML/text sink on any other tag. | ||
| * | ||
| * @codeGenApi | ||
| */ | ||
| export function ɵɵsanitizeMaybeScript(unsafeValue: any, tag: string, prop: string): any { | ||
| if (tag.toLowerCase() === 'script') { | ||
| return ɵɵsanitizeScript(unsafeValue); | ||
| } | ||
| const lowerProp = prop.toLowerCase(); | ||
| if (lowerProp === 'innerhtml' || lowerProp === 'outerhtml') { | ||
| return ɵɵsanitizeHtml(unsafeValue); | ||
| } | ||
| return unsafeValue; | ||
| } | ||
|
Comment on lines
+267
to
+276
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need that additional instruction ? Couldn't the compiler determine the right instruction at compile time ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was added for host binding cases where we only know at runtime which tag can be used, the directive/component, as exemplified in the tests.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's also another similar report regarding host binding, but I believe it would be outside the scope of this PR, or I'm unsure if it's supposed to work this way. |
||
|
|
||
| export function validateAgainstEventProperties(name: string) { | ||
| if (name.toLowerCase().startsWith('on')) { | ||
| const errorMessage = | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -118,7 +118,7 @@ describe('sanitization', () => { | |
| [SecurityContext.RESOURCE_URL, ɵɵsanitizeResourceUrl], | ||
| ]); | ||
| Object.entries(schema).forEach(([key, context]) => { | ||
| if (context === SecurityContext.URL || SecurityContext.RESOURCE_URL) { | ||
| if (context === SecurityContext.URL || context === SecurityContext.RESOURCE_URL) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found this while writing the unit tests (apparently it was always evaluated as true if I'm not mistaken) |
||
| const [tag, prop] = key.split('|'); | ||
| const contexts = contextsByProp.get(prop) || new Set<number>(); | ||
| contexts.add(context); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we considered revisiting this? If an element with
scripttag within an SVG-namespaced tree behaves like a script tag—I am actually surprised it works this way, or is Angular just not creating the element with the SVG namespace?—then I'd expect we treat it equally to ascriptelement outside of an SVG structure (or MathML namespaced tree).For defence in depth it may be desirable to keep the new security contexts in place, but I don't see a reason to keep the ingestion pipeline as it is today if that doesn't accurately reflect how this behaves at runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this behavior for SVGScriptElement and how it works according to the HTML spec. I think the Angular compiler follows the same browser behavior, although I may still have a gap in my understanding.
Regarding MathML, when placing content inside a
<script>tag (tested locally), the script is not executed, it is treated as plain text instead. Also, to achieve this, we need to disable schema validation errors, since as I understand it, putting<script>inside MathML is not allowed by default.This change is essentially a mirror of the existing URL sink behavior. Also, if we remove this host binding / binding handling, SVG script bindings will fail.
Test failed removing this
At runtime, I believe there are other issues that I'm not sure are desirable to report here https://issuetracker.google.com/u/1/issues/510537066
Or maybe it always worked this way (I can't find anything in the documentation or code that indicates this is how it should be, but maybe it is correct).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't even know
SVGScriptElementis a thing!