feat(core): create internal Trusted Types module#39207
Conversation
f79b382 to
e912eaf
Compare
| "author": "angular", | ||
| "license": "MIT", | ||
| "dependencies": { | ||
| "@types/trusted-types": "^1.0.6", |
There was a problem hiding this comment.
Is this really necessary? Seems redundant 😕
There was a problem hiding this comment.
Good question, and I would definitely appreciate insights others might have on this. As far as my limited knowledge on npm package dependencies, this seemed to be necessary. Before adding this, the issue was that @types/trusted-types would not be available as a dependency in an Angular application (even after doing an ng update, it wouldn't appear in yarn.lock), since none of Angular's packages had a dependency on it. When I then ran ng build the application would not compile due to missing type definitions for Trusted Types. So without this (and unless there's a different way), existing Angular applications will break unless they explicitly add a dependency on @types/trusted-types themselves.
There was a problem hiding this comment.
Why would the app fail? Can you give an example of how to reproduce this?
An app would consume the published Angular packages, which are compiled to JS. At that point, any types would have been stripped of (they are a compile-time only thing).
There was a problem hiding this comment.
I share @gkalpak's concerns.
I discussed this with @bjarkler and we decided not to expose any of the trusted types as part of Angular's public api for now.
Once microsoft/TypeScript#30024 is fixed (introduces TT into lib.dom.d.ts) then we can expose TT through our core apis, but until then we should avoid it.
One alternative for still shipping TrustedSanitizer would be to ship it as a new package that has a peer dependency on @types/trusted-types. But this is something we can consider once Angular is compatible with TT, which should be the top priority.
There was a problem hiding this comment.
The issue seems to be that #39219 and potentially other changes make Trusted Types a part of Angular's public API, which is why dependent applications will also need the type definitions to compile. We're looking into ways to circumvent that, at least until microsoft/TypeScript#30024 is fixed.
There was a problem hiding this comment.
The problem is that later PRs introduce Trusted Types into Angular's public api surface, which then causes the types to be present in our d.ts files. In order for tsc to typecheck our d.ts files when compiling the angular app (without skipLibCheck), tsc will need the definitions.
As described in the other comment, we decided to change our rollout strategy and not expose TT in Angular's public api surface for now.
There was a problem hiding this comment.
ahh.. @bjarkler also explained it :) sorry about the noise.
a666a48 to
35631b1
Compare
35631b1 to
455362c
Compare
To facilitate the upcoming Trusted Types support being added to Angular, add the TypeScript type definitions for the Trusted Types browser API as a dependency in the root package.json and types.d.ts since they're needed for compiling the Angular packages.
455362c to
80dee55
Compare
Add a module that provides a Trusted Types policy for use internally by Angular. The policy is created lazily and stored in a module-local variable. For now the module does not allow configuring custom policies or policy names, and instead creates its own policy with 'angular' as a fixed policy name. This is to more easily support tree-shakability. Helper functions for unsafely converting strings to each of the three Trusted Types are also introduced, with documentation that make it clear that their use requires a security review. When Trusted Types are not available, these helper functions fall back to returning strings.
80dee55 to
c22fec5
Compare
IgorMinar
left a comment
There was a problem hiding this comment.
discussed offline, we'll remove the dep on @types/trusted-types. See other comments for details.
| * Returns the Trusted Types policy, or null if Trusted Types are not | ||
| * enabled/supported. The first call to this function will create the policy. | ||
| */ | ||
| function getPolicy(): TrustedTypePolicy|null { |
There was a problem hiding this comment.
I see that you changed all of these from methods to functions. that's great for minification with Terser! Thank you!
There was a problem hiding this comment.
Thanks to @engelsdamien for pointing that out :)
IgorMinar
left a comment
There was a problem hiding this comment.
LGTM now!
Reviewed-for: global-approvers
|
I'm marking this as "target: minor" since we are not making any breaking change here or introducing a risky change. |
Add a module that provides a Trusted Types policy for use internally by Angular. The policy is created lazily and stored in a module-local variable. For now the module does not allow configuring custom policies or policy names, and instead creates its own policy with 'angular' as a fixed policy name. This is to more easily support tree-shakability. Helper functions for unsafely converting strings to each of the three Trusted Types are also introduced, with documentation that make it clear that their use requires a security review. When Trusted Types are not available, these helper functions fall back to returning strings. PR Close #39207
|
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. |
Add a module that provides a Trusted Types policy for use internally by
Angular. The policy is created lazily and stored in a module-local
variable. For now the module does not allow configuring custom policies
or policy names, and instead creates its own policy with 'angular' as a
fixed policy name. This is to more easily support tree-shakability.
Helper functions for unsafely converting strings to each of the three
Trusted Types are also introduced, with names that make it clear that
their use requires a security review. When Trusted Types are not
available, these helper functions fall back to returning strings.
The TypeScript type definitions for Trusted Types is also added as
a dependency. See the individual commits for more details.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Does this PR introduce a breaking change?
Other information
This is part of an ongoing effort to add support for Trusted Types to Angular.