Skip to content
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

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

Closed
wants to merge 13 commits into from

Conversation

@mhevery
Copy link
Member

commented Jul 31, 2019

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 mhevery requested review from angular/fw-core as code owners Jul 31, 2019

@googlebot googlebot added the cla: yes label Jul 31, 2019

@IgorMinar
Copy link
Member

left a comment

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 Show resolved Hide resolved
packages/platform-browser/src/browser.ts Outdated Show resolved Hide resolved
packages/platform-browser/src/browser.ts Outdated Show resolved Hide resolved
packages/platform-browser/src/private_export.ts Outdated Show resolved Hide resolved

@mhevery mhevery force-pushed the mhevery:angular_size branch from 8e88ea0 to 35bcff3 Jul 31, 2019

@mhevery mhevery requested a review from angular/fw-public-api as a code owner Aug 1, 2019

@mhevery mhevery force-pushed the mhevery:angular_size branch 2 times, most recently from d7c631c to 8494709 Aug 1, 2019

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

@mhevery mhevery force-pushed the mhevery:angular_size branch 2 times, most recently from 9eebece to b9f0211 Aug 8, 2019

@IgorMinar
Copy link
Member

left a comment

a few more comments / suggestions

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

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 9, 2019

Author Member

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.

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Aug 10, 2019

Member

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?

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Aug 13, 2019

Member

That would be a breaking change.

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) {

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Aug 9, 2019

Member
Suggested change
if (actualType != null && actualType !== type) {
if (actualType !== null && actualType !== type) {

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 9, 2019

Author Member

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

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Aug 10, 2019

Member

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 Outdated Show resolved Hide resolved
@@ -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)) {

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Aug 9, 2019

Member

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

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 9, 2019

Author Member

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

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Aug 10, 2019

Member

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

  1. check whether we can bypass the sanitizer for these types
  2. 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.

@mhevery mhevery force-pushed the mhevery:angular_size branch from 270d0dd to f2ef8ef Aug 9, 2019

@mhevery mhevery requested a review from angular/docs-infra as a code owner Aug 9, 2019

@mhevery mhevery force-pushed the mhevery:angular_size branch from 080d9d7 to 03e14a1 Aug 9, 2019

@mhevery mhevery requested a review from angular/fw-integration as a code owner Aug 10, 2019

@petebacondarwin
Copy link
Member

left a comment

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

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

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Aug 10, 2019

Member

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?

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) {

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Aug 10, 2019

Member

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.

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

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Aug 10, 2019

Member

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.

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 13, 2019

Author Member

Do you have suggestions as to make it more clear?

@@ -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)) {

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Aug 10, 2019

Member

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

  1. check whether we can bypass the sanitizer for these types
  2. 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.

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

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Aug 10, 2019

Member

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

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 13, 2019

Author Member

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

@mhevery

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

@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.

mhevery added 5 commits Jul 31, 2019
perf(core): make sanitization tree-shakable in Ivy mode
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 mhevery force-pushed the mhevery:angular_size branch from dc20440 to 1cb3475 Aug 13, 2019

@mhevery

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

@IgorMinar
Copy link
Member

left a comment

Lgtm, but please resolve Pete's comments.

@mhevery

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

@mhevery

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

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

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

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

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

gkalpak added a commit to gkalpak/angular that referenced this pull request Aug 26, 2019
docs(platform-browser): remove redundant JSDoc tag from `BROWSER_SANI…
…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
mhevery added a commit that referenced this pull request Aug 29, 2019
docs(platform-browser): remove redundant JSDoc tag from `BROWSER_SANI…
…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
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
perf(core): make sanitization tree-shakable in Ivy mode (angular#31934)
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
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
docs(platform-browser): remove redundant JSDoc tag from `BROWSER_SANI…
…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

This comment has been minimized.

Copy link

commented Sep 15, 2019

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.

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.