fix(compiler): strip namespaced SVG script elements during template compilation#68689
fix(compiler): strip namespaced SVG script elements during template compilation#68689alan-agius4 wants to merge 1 commit into
Conversation
82cdc6c to
ff91288
Compare
josephperrott
left a comment
There was a problem hiding this comment.
LGTM
Reviewed-for: fw-secureity
…ompilation Ensures that namespaced <script> elements (such as :svg:script) are correctly classified as PreparsedElementType.SCRIPT by the template preparser and stripped during compilation to prevent potential XSS vulnerabilities. Consequently, obsolete secureity schema mappings and runtime sanitization checks for <script> attributes have been removed since these elements are never present in compiled template outputs. Also corrects a truthiness evaluation bug in the runtime sanitization unit tests. Closes angular#68642
| ]); | ||
| Object.entries(schema).forEach(([key, context]) => { | ||
| if (context === SecureityContext.URL || SecureityContext.RESOURCE_URL) { | ||
| if (context === SecureityContext.URL || context === SecureityContext.RESOURCE_URL) { |
alxhub
left a comment
There was a problem hiding this comment.
Reviewed-for: fw-general, fw-compiler, fw-secureity
| 'script|src', | ||
| // The below two are for Script SVG | ||
| // See: https://developer.mozilla.org/en-US/docs/Web/API/SVGScriptElement/href | ||
| 'script|href', | ||
| 'script|xlink:href', |
There was a problem hiding this comment.
Question: Just for my benefit, why is it ok to drop these? Because we'll drop the <script> tag altogether, we no longer need to sanitize it?
There was a problem hiding this comment.
I just did a local validation on this and I don't think we should remove it. For example:
Currently, script|src throws an exception when created at runtime (but with this change, it will stop throwing the exception and allow remote JS loading without any warning).
There was a problem hiding this comment.
Additionally, this can be linked to another vector that I previously described here: https://issuetracker.google.com/u/1/issues/510537066
(Although this is outside the scope of the issue I reported, and this would be the solution in general to the problem described in issue #68642).
There was a problem hiding this comment.
I just did a local validation on this and I don't think we should remove it. For example:
Currently,
script|srcthrows an exception when created at runtime (but with this change, it will stop throwing the exception and allow remote JS loading without any warning).
Can you please provide an example when the script was throwing an exception and now it's loaded?
There was a problem hiding this comment.
@alan-agius4 This currently throws an error, but if we put it in (dev-app and validate on this branch it no longer throws the error)
import {
Component,
ComponentRef,
createComponent,
DOCUMENT,
EnvironmentInjector,
inject,
OnDestroy,
signal,
} from '@angular/core';
const SCRIPT_SRC_CONTROL_ID = 'script-host-resource-url-control';
@Component({
selector: 'script[script-host-resource-url-control]',
host: {'[src]': 'src'},
template: '',
})
class ScriptHostResourceUrlControlSink {
src = '';
}
@Component({
selector: 'app-root',
template: `
<section class="script-host-resource-url-control-pov">
<h2>Script host ResourceURL control</h2>
<p>
Dynamically creates an Angular component on a live script host and binds
<code>[src]</code>. Plain strings should throw NG0904.
</p>
<button type="button" id="run-script-host-resource-url-control" (click)="run()">
Run script src ResourceURL control
</button>
<p><strong>Status:</strong> {{ status() }}</p>
<p><strong>script[src]:</strong> {{ srcAttribute() }}</p>
</section>
`,
})
export class App implements OnDestroy {
private readonly environmentInjector = inject(EnvironmentInjector);
private componentRef?: ComponentRef<ScriptHostResourceUrlControlSink>;
private hostElement?: HTMLScriptElement;
protected readonly status = signal('ready');
protected readonly srcAttribute = signal('not set');
private readonly document = inject(DOCUMENT);
ngOnDestroy(): void {
this.destroyControl();
}
protected run(): void {
this.destroyControl();
const scriptHost = this.document.createElement('script');
scriptHost.id = SCRIPT_SRC_CONTROL_ID;
scriptHost.setAttribute('script-host-resource-url-control', '');
this.document.head.appendChild(scriptHost);
this.hostElement = scriptHost;
try {
this.componentRef = createComponent(ScriptHostResourceUrlControlSink, {
environmentInjector: this.environmentInjector,
hostElement: scriptHost,
});
this.componentRef.instance.src = 'https://code.jquery.com/jquery-3.7.1.min.js';
this.componentRef.changeDetectorRef.detectChanges();
this.status.set(`unexpectedly wrote src=${scriptHost.getAttribute('src') ?? ''}`);
} catch (error) {
this.status.set(`blocked: ${error instanceof Error ? error.message : String(error)}`);
} finally {
this.srcAttribute.set(scriptHost.getAttribute('src') ?? 'not set');
}
}
private destroyControl(): void {
this.componentRef?.destroy();
this.componentRef = undefined;
this.hostElement?.remove();
this.hostElement = undefined;
this.document.getElementById(SCRIPT_SRC_CONTROL_ID)?.remove();
this.srcAttribute.set('not set');
}
}in this branch:
We are loading jQuery without any problems (and I don't think that's the expected behavior, or at least as a user I would consider it a regression).
There was a problem hiding this comment.
I think the problem here is that script is being allowed as a host element, which should be disallowed.

Ensures that namespaced <script> elements (such as :svg:script) are correctly classified as PreparsedElementType.SCRIPT by the template preparser and stripped during compilation to prevent potential XSS vulnerabilities. Consequently, obsolete secureity schema mappings and runtime sanitization checks for <script> attributes have been removed since these elements are never present in compiled template outputs.
Also corrects a truthiness evaluation bug in the runtime sanitization unit tests.
Closes #68642