perf(core): make sanitization tree-shakable in Ivy mode #31934

mhevery · 2019-07-31T18:17:48Z

In VE the Sanitizer is always available in BrowserModule because the VE retrieves it using injection.

In Ivy the injection is optional and we have instructions instead of component definition arrays. The implication of this is that in Ivy the instructions can pull in the sanitizer only when they are working with a property which is known to be unsafe. Because the Injection is optional this works even if no Sanitizer is present. So in Ivy we first use the sanitizer which is pulled in by the instruction, unless one is available through the Injector then we use that one instead.

This PR does few things:

It makes Sanitizer optional in Ivy.
It makes DomSanitizer tree shakable.
It aligns the semantics of Ivy Sanitizer with that of the Ivy sanitization rules.
It refactors DomSanitizer to use same functions as Ivy sanitization for consistency.

IgorMinar

I need to review this more carefully to have good feedback, so I'm just posting initial observations.

Three more things:

Can you please write up a good commit message describing the change and providing enough context for all the changes you made?
Can you take a look at this older PR that Martin has been trying to land for a while: #12310 and see if it makes sense to incorporate it into this change?
We will likely need Rafael or someone else from the security team to take a look at this as well as the changes you made are nontrivial.

packages/core/src/sanitization/bypass.ts

packages/platform-browser/src/browser.ts

packages/platform-browser/src/private_export.ts

packages/platform-browser/test/browser/bootstrap_spec.ts

IgorMinar

a few more comments / suggestions

IgorMinar · 2019-08-02T16:01:33Z

packages/core/src/sanitization/bypass.ts

 */
-export interface TrustedString extends String { [BRAND]: BypassType; }


why did you remove the branded interface? without it the meaning of these interfaces is lost because due to structural typing you can't ensure type safety: http://www.typescriptlang.org/play/#code/KYDwDg9gTgLgBASwHY2FAZgQwMbDgZU3WADVMAbAVzwG8BfAKAdElkRTS1wKOAAkYAW3JxQqJABMAzj2JkqtRs3DR4yVBhx5CxfDACe5PGOCSZO0hWpx6TFqvYau23vmxQEYeCbOzLCmyUGOEwAIykYKBx4bHJMKXNeeWoASUEwEQR0o0FTGES5K1pguGwIJAioSmwYaAAKMEpQ8gRsUoALTCQAc2RugBV2hCkAIShgTABrKQBBMAzWzBgEcvxgbEoPAwAuOEq+gEobErg4AHoz0XSDEqVTkPDI6Lhu4Bh+-TBgADlMXLqDrt9j0ANxMe61PQeHoA473ODjGCbJBwAAGFmSeEElAicBxeAA2mAoBAvrB9ABdAC8oWQEj6uwAJDQYEMpAA6bCdHp9QbDMYTaZzBbYJYrJBrDZbfR0VFwADUJ3haLgdSkwDw7RgMDA2wu3U5EDOPTO6qlCAMAGIQAkDqiwadGHdYvECvwhOQ0hlRCBxNI-JivZlssBcig3QJhHCXm8Pl9fv8jjQEW9kXAAOQAZnTILgToYLoSfj0hmAQZ9frdgeyiBDYfyxYMRmjr3enx+f2AsOTiLT6YALDm8wwlPY2OhKEgauK8UgAO5RMAYop1ABuRV2y4USZKvagKLXRRCVaKQYOnO5vR6fNG4yms3mLVFy1W602Fv0YKUZQq8BAcCpOAkGAOc-EjT1sjqdMpHTA4wQYCcpxfFFUAiOp0E3Vwm2AHd4T3FF0C-JgLjgVlhlI9suXWaY4HKch9DgUJ1niFxiHAkJJEbUsQnGPZImqJEono0j2jwKROwLcpcS1YRMLYj0ANI4A0JAOCGCAA

I did not remove anything, I just moved it from platform-browser to the core. We could add additional branding if you would like, but that is not what we had in the past.

Oh I see what you are doing... rather than use branded types you have switched to using real classes (e.g. SafeValueImpl and descendants).

But in that case I don't see any value in having both real implementations and interfaces... especially since the interfaces are all semantically identically now...

const x: SafeValue = new SafeHtmlImpl('some string'); const y: SafeScript = x; // No compile time error...

Why not just make them all classes and ditch the interfaces?

That would be a breaking change.

IgorMinar · 2019-08-02T17:30:50Z

packages/core/src/sanitization/bypass.ts

+export function allowSanitizationBypassAndThrow(value: any, type: BypassType): boolean;
+export function allowSanitizationBypassAndThrow(value: any, type: BypassType): boolean {
+  const actualType = getSanitizationBypassType(value);
+  if (actualType != null && actualType !== type) {


Suggested change

if (actualType != null && actualType !== type) {

if (actualType !== null && actualType !== type) {

No, I actually want this to be sensitive to null and undefined

At least in the compiler we have been trying to be more explicit about these things, so if we wanted to compare both null and undefined then we would write it out explicitly using ===. This would ensure people like @IgorMinar don't mistake this sort of comparison as an error. I think the minimizer might be able to collapse such comparisons back to == at bundling time so there is no benefit in using it in the source.

Alternatively, I think a comment explaining why != is being used rather than !==` would help.

packages/core/src/sanitization/bypass.ts

IgorMinar · 2019-08-02T17:57:03Z

packages/core/src/sanitization/sanitization.ts

@@ -38,8 +38,8 @@ export function ɵɵsanitizeHtml(unsafeHtml: any): string {
  if (sanitizer) {
    return sanitizer.sanitize(SecurityContext.HTML, unsafeHtml) || '';
  }
-  if (allowSanitizationBypass(unsafeHtml, BypassType.Html)) {
-    return unsafeHtml.toString();
+  if (allowSanitizationBypassAndThrow(unsafeHtml, BypassType.Html)) {


"andThrow"? what does that mean? you don't seem to be throwing anywhere.

the function throws if the sanitization is not valid. Can you suggest a different name?

Regarding the name, I feel that the problem is that this function is trying to do two different things:

check whether we can bypass the sanitizer for these types

check whether sanitization is allowed at all for these types

It seems to me that point 2) should be a check that the sanitizer should be doing e.g. in _sanitizeHtml and friends.

packages/platform-browser/src/security/dom_sanitization_service.ts

petebacondarwin

I came to review for docs-infra but then found that you had removed all the changes in aio :-)

petebacondarwin · 2019-08-10T11:15:42Z

packages/core/src/sanitization/bypass.ts

 */
-export interface TrustedString extends String { [BRAND]: BypassType; }


Oh I see what you are doing... rather than use branded types you have switched to using real classes (e.g. SafeValueImpl and descendants).

But in that case I don't see any value in having both real implementations and interfaces... especially since the interfaces are all semantically identically now...

const x: SafeValue = new SafeHtmlImpl('some string'); const y: SafeScript = x; // No compile time error...

Why not just make them all classes and ditch the interfaces?

petebacondarwin · 2019-08-10T11:18:27Z

packages/core/src/sanitization/bypass.ts

+export function allowSanitizationBypassAndThrow(value: any, type: BypassType): boolean;
+export function allowSanitizationBypassAndThrow(value: any, type: BypassType): boolean {
+  const actualType = getSanitizationBypassType(value);
+  if (actualType != null && actualType !== type) {


At least in the compiler we have been trying to be more explicit about these things, so if we wanted to compare both null and undefined then we would write it out explicitly using ===. This would ensure people like @IgorMinar don't mistake this sort of comparison as an error. I think the minimizer might be able to collapse such comparisons back to == at bundling time so there is no benefit in using it in the source.

Alternatively, I think a comment explaining why != is being used rather than !==` would help.

petebacondarwin · 2019-08-10T11:44:31Z

packages/core/src/sanitization/bypass.ts

+    throw new Error(
+        `Required a safe ${type}, got a ${actualType} (see http://g.co/ng/security#xss)`);
+  }
+  return actualType === type;


So I think this function will:

return true if

actualType === type

or actualType is ResourceUrl and type is Url

return false: if

actualType === null and type !== null

or actualType === undefined and type !== undefined

throw otherwise

Is that correct? It is not so obvious to glean that from reading the code.

Do you have suggestions as to make it more clear?

petebacondarwin · 2019-08-10T11:45:36Z

packages/core/src/sanitization/sanitization.ts

@@ -38,8 +38,8 @@ export function ɵɵsanitizeHtml(unsafeHtml: any): string {
  if (sanitizer) {
    return sanitizer.sanitize(SecurityContext.HTML, unsafeHtml) || '';
  }
-  if (allowSanitizationBypass(unsafeHtml, BypassType.Html)) {
-    return unsafeHtml.toString();
+  if (allowSanitizationBypassAndThrow(unsafeHtml, BypassType.Html)) {


Regarding the name, I feel that the problem is that this function is trying to do two different things:

check whether we can bypass the sanitizer for these types

check whether sanitization is allowed at all for these types

It seems to me that point 2) should be a check that the sanitizer should be doing e.g. in _sanitizeHtml and friends.

petebacondarwin · 2019-08-10T11:46:58Z

packages/core/test/render3/instructions_spec.ts

-        ɵɵstylingApply();
-      });
-      expect(sanitizerInterceptor.lastValue).toEqual(null);
-    });


Is this test no longer applicable? If it is then should it appear somewhere in the new code?

yes, this test is invalid, and has been removed.

mhevery · 2019-08-13T04:27:43Z

@petebacondarwin The type confusion comes from the fact that I am moving code between packages. There is no change to public API. Yes your suggestions would be a great improvement but it would be breaking change as it would make it more strict and so it should be done in separate PR.

In VE the `Sanitizer` is always available in `BrowserModule` because the VE retrieves it using injection. In Ivy the injection is optional and we have instructions instead of component definition arrays. The implication of this is that in Ivy the instructions can pull in the sanitizer only when they are working with a property which is known to be unsafe. Because the Injection is optional this works even if no Sanitizer is present. So in Ivy we first use the sanitizer which is pulled in by the instruction, unless one is available through the `Injector` then we use that one instead. This PR does few things: 1) It makes `Sanitizer` optional in Ivy. 2) It makes `DomSanitizer` tree shakable. 3) It aligns the semantics of Ivy `Sanitizer` with that of the Ivy sanitization rules. 4) It refactors `DomSanitizer` to use same functions as Ivy sanitization for consistency.

mhevery · 2019-08-13T16:27:38Z

presubmit

IgorMinar

Lgtm, but please resolve Pete's comments.

mhevery · 2019-08-13T21:46:16Z

presubmit

mhevery · 2019-08-14T03:40:00Z

MERGE-ASSISTANCE: Unrelated failure (Unable to force green state)

AndrewKushnir · 2019-08-14T05:07:18Z

The changes in this PR break some g3 targets (Guitar tests) with Ivy, adding "Blocked" label for now, so we can investigate problems before merging this PR.

AndrewKushnir · 2019-08-14T05:09:08Z

Also started a Global TAP Presubmit to see if there are other affected targets.

AndrewKushnir · 2019-08-14T23:51:03Z

…TIZATION_PROVIDERS__POST_R3__` The JSDoc tag was introduced in angular#31934 and was not intentional according to [this discussion on Slack][1]. [1]: https://angular-team.slack.com/archives/CHB51S90D/p1566322373094100?thread_ts=1566292123.093500&cid=CHB51S90D

…TIZATION_PROVIDERS__POST_R3__` (#32314) The JSDoc tag was introduced in #31934 and was not intentional according to [this discussion on Slack][1]. [1]: https://angular-team.slack.com/archives/CHB51S90D/p1566322373094100?thread_ts=1566292123.093500&cid=CHB51S90D PR Close #32314

In VE the `Sanitizer` is always available in `BrowserModule` because the VE retrieves it using injection. In Ivy the injection is optional and we have instructions instead of component definition arrays. The implication of this is that in Ivy the instructions can pull in the sanitizer only when they are working with a property which is known to be unsafe. Because the Injection is optional this works even if no Sanitizer is present. So in Ivy we first use the sanitizer which is pulled in by the instruction, unless one is available through the `Injector` then we use that one instead. This PR does few things: 1) It makes `Sanitizer` optional in Ivy. 2) It makes `DomSanitizer` tree shakable. 3) It aligns the semantics of Ivy `Sanitizer` with that of the Ivy sanitization rules. 4) It refactors `DomSanitizer` to use same functions as Ivy sanitization for consistency. PR Close angular#31934

…TIZATION_PROVIDERS__POST_R3__` (angular#32314) The JSDoc tag was introduced in angular#31934 and was not intentional according to [this discussion on Slack][1]. [1]: https://angular-team.slack.com/archives/CHB51S90D/p1566322373094100?thread_ts=1566292123.093500&cid=CHB51S90D PR Close angular#32314

angular-automatic-lock-bot · 2019-09-15T10:21:43Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

mhevery requested review from a team as code owners July 31, 2019 18:17

googlebot added the cla: yes label Jul 31, 2019

IgorMinar reviewed Jul 31, 2019

View reviewed changes

mhevery force-pushed the angular_size branch from 8e88ea0 to 35bcff3 Compare July 31, 2019 20:08

mhevery requested a review from a team as a code owner August 1, 2019 04:11

mhevery force-pushed the angular_size branch 2 times, most recently from d7c631c to 8494709 Compare August 1, 2019 23:34

mhevery added target: major This PR is targeted for the next major release area: core Issues related to the framework runtime labels Aug 3, 2019

ngbot bot added this to the needsTriage milestone Aug 3, 2019

mhevery force-pushed the angular_size branch 2 times, most recently from 9eebece to b9f0211 Compare August 8, 2019 22:23

IgorMinar reviewed Aug 9, 2019

View reviewed changes

mhevery force-pushed the angular_size branch from 270d0dd to f2ef8ef Compare August 9, 2019 18:55

mhevery requested a review from a team as a code owner August 9, 2019 18:55

mhevery force-pushed the angular_size branch from 080d9d7 to 03e14a1 Compare August 9, 2019 22:54

mhevery requested a review from a team as a code owner August 10, 2019 00:15

petebacondarwin reviewed Aug 10, 2019

View reviewed changes

mhevery added 5 commits August 12, 2019 22:40

fixup! perf(core): make sanitization tree-shakable in Ivy mode

f665f03

fixup! perf(core): make sanitization tree-shakable in Ivy mode

3f4d642

fixup! perf(core): make sanitization tree-shakable in Ivy mode

17bfa25

fixup! perf(core): make sanitization tree-shakable in Ivy mode

1cb3475

mhevery force-pushed the angular_size branch from dc20440 to 1cb3475 Compare August 13, 2019 05:59

fixup! perf(core): make sanitization tree-shakable in Ivy mode

c1ef23b

IgorMinar approved these changes Aug 13, 2019

View reviewed changes

fixup! perf(core): make sanitization tree-shakable in Ivy mode

037ec02

mhevery added 4 commits August 13, 2019 14:47

fixup! perf(core): make sanitization tree-shakable in Ivy mode

cb09fe1

fixup! perf(core): make sanitization tree-shakable in Ivy mode

687049f

fixup! perf(core): make sanitization tree-shakable in Ivy mode

f0e8457

fixup! perf(core): make sanitization tree-shakable in Ivy mode

5877a1d

mhevery added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Aug 14, 2019

AndrewKushnir added the state: blocked label Aug 14, 2019

mhevery added 2 commits August 14, 2019 15:16

fixup! perf(core): make sanitization tree-shakable in Ivy mode

51728b0

fixup! perf(core): make sanitization tree-shakable in Ivy mode

027f415

AndrewKushnir closed this in 2e4d17f Aug 15, 2019

gkalpak mentioned this pull request Aug 26, 2019

docs(platform-browser): remove redundant JSDoc tag from BROWSER_SANITIZATION_PROVIDERS__POST_R3__ #32314

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 15, 2019

		*/
		export interface TrustedString extends String { [BRAND]: BypassType; }

	if (actualType != null && actualType !== type) {
	if (actualType !== null && actualType !== type) {

perf(core): make sanitization tree-shakable in Ivy mode #31934

perf(core): make sanitization tree-shakable in Ivy mode #31934

Uh oh!

Conversation

mhevery commented Jul 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IgorMinar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IgorMinar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhevery commented Aug 13, 2019

Uh oh!

mhevery commented Aug 13, 2019

Uh oh!

IgorMinar left a comment

Choose a reason for hiding this comment

Uh oh!

mhevery commented Aug 13, 2019

Uh oh!

mhevery commented Aug 14, 2019

Uh oh!

AndrewKushnir commented Aug 14, 2019

Uh oh!

AndrewKushnir commented Aug 14, 2019

Uh oh!

AndrewKushnir commented Aug 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

angular-automatic-lock-bot bot commented Sep 15, 2019

Uh oh!

Uh oh!

mhevery commented Jul 31, 2019 •

edited

Loading

AndrewKushnir commented Aug 14, 2019 •

edited

Loading