fix(material/icon): make icon-registry compatible with Trusted Types #23140
Conversation
google-cla
bot
added
the
cla: yes
bjarkler
force-pushed
the
icon-tt
branch
2 times, most recently
from
9419464 to
35cfa62
Compare
| * that will be interpreted as HTML by a browser, e.g. when assigning to | ||
| * element.innerHTML. | ||
| */ | ||
| export function trustedHTMLFromString(html: string): TrustedHTML { |
There was a problem hiding this comment.
small comment: Should this be named so that its unsafeness is clear from the function name?
(Sorry, I'm not the most familiar with Angular naming conventions, so please excuse me if that's not feasible here.)
There was a problem hiding this comment.
We discussed this with the Angular team while preparing angular/angular#39207. The issue is that the code minifier that they use doesn't change function names, so a longer more clear name would likely impact code size. This was the name that we landed on as a compromise.
src/material/icon/trusted-types.ts
Outdated
| if (typeof window !== 'undefined') { | ||
| const ttWindow = window as unknown as {trustedTypes?: TrustedTypePolicyFactory}; | ||
| if (ttWindow.trustedTypes !== undefined) { | ||
| policy = ttWindow.trustedTypes.createPolicy('material#icon', { |
There was a problem hiding this comment.
I think that it would be better to create a single policy for all of @angular/material (and even @angular/cdk), but that would require us to figure out how to securely share it between all of the entry points (e.g. @angular/material/icon).
I don't believe that we have such sharing mechanism today. We do have a concept of private exports in other packages, but this only obfuscates the export name with a hard to type prefix — ɵ (example) which is not good enough for a trusted type policy.
I wonder if there is another way to do this.
The main concern with entry-point level policy names is that there could be too many of them added over time, which would make it a nightmare to manage the policy names within the server configuration.
There was a problem hiding this comment.
aside: the name of the policy could also be changed to be more consistent with our existing angular policies, for example angular#material or angular#components.
There was a problem hiding this comment.
+1, the name of the policy should include "angular" in the name.
As for sharing, I don't know if doing something cross-library possible? I don't know the trusted types system super well, but it seems like the policy object needs to be inaccessible to userspace, yes? If that's the case, there's no mechanism such that another material entry-point could access it while userspace couldn't.
That said, we don't use innerHTML anywhere else in material or cdk- the SVG handling here for icons is special since SVGs are assets that also need to be inlined into the DOM. I would like to move this svg handling code into the cdk in the future, but we can cross that bridge when we get to it.
There was a problem hiding this comment.
@jelbourn what policy name would you prefer? angular#material, angular#cdk, or angular#components?
+1 for keeping the code here for now, as long as we don't foresee a need to do other security-sensitive DOM operations elsewhere in material or cdk.
There was a problem hiding this comment.
I changed the name to angular#components to allow us to use it both for material and cdk in the future. Would be happy to change this if you have other preferences.
When Angular Material is used in an environment that enforces Trusted Types, the icon registry raises a Trusted Types violation due to its use of element.innerHTML when initializing SVG icons. To make the icon registry compatible with Trusted Types, SvgIconConfig.svgText is changed to a TrustedHTML, and its users updated to either produce TrustedHTML (making sure to only do so in cases where its security can be readily assessed) or pass such values along. To facilitate this, add a module that provides a Trusted Types policy, 'angular#components'. The policy is created lazily and stored in a module-local variable. This is the same as the approach taken by Angular proper in https://github.com/angular/angular/blob/master/packages/core/src/util/security/trusted_types.ts
jelbourn
added
target: minor
|
global presubmit looks good, and that ngcc_compatibility failure looks like a flake. Can we merge this? :) |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
When Angular Material is used in an environment that enforces Trusted Types, the icon registry raises a Trusted Types violation due to its use of
element.innerHTMLwhen initializing SVG icons.To make the icon registry compatible with Trusted Types,
SvgIconConfig.svgTextis changed to aTrustedHTML, and its users updated to either produceTrustedHTML(making sure to only do so in cases where its security can be readily assessed) or pass such values along.To facilitate this, add a module that provides a Trusted Types policy,
angular#components, for use internally by Angular Material. The policy is created lazily and stored in a module-local variable. This is the same as the approach taken by Angular proper inhttps://github.com/angular/angular/blob/master/packages/core/src/util/security/trusted_types.ts
Helper functions for unsafely converting a string to a
TrustedHTMLis also introduced, with documentation to make it clear that its use requires a security review. When Trusted Types are not available, the helper function falls back to returning strings, facilitating backwards-compatibility with environments that do not support Trusted Types.