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
Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion integration/_payload-limits.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
"master": {
"uncompressed": {
"runtime": 1440,
"main": 147528,
"main": 136380,
"polyfills": 43567
}
}
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/core_private_export.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,3 +36,4 @@ export {clearOverrides as ɵclearOverrides, initServicesIfNeeded as ɵinitServic
export {NOT_FOUND_CHECK_ONLY_ELEMENT_INJECTOR as ɵNOT_FOUND_CHECK_ONLY_ELEMENT_INJECTOR} from './view/provider';
export {getLocalePluralCase as ɵgetLocalePluralCase, findLocaleData as ɵfindLocaleData} from './i18n/locale_data_api';
export {LOCALE_DATA as ɵLOCALE_DATA, LocaleDataIndex as ɵLocaleDataIndex} from './i18n/locale_data';
export {allowSanitizationBypassAndThrow as ɵallowSanitizationBypassAndThrow, getSanitizationBypassType as ɵgetSanitizationBypassType, BypassType as ɵBypassType, unwrapSafeValue as ɵunwrapSafeValue, SafeHtml as ɵSafeHtml, SafeResourceUrl as ɵSafeResourceUrl, SafeScript as ɵSafeScript, SafeStyle as ɵSafeStyle, SafeUrl as ɵSafeUrl, SafeValue as ɵSafeValue} from './sanitization/bypass';
4 changes: 2 additions & 2 deletions packages/core/src/di/injector_compatibility.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ export function ɵɵinject<T>(token: Type<T>| InjectionToken<T>): T;
export function ɵɵinject<T>(token: Type<T>| InjectionToken<T>, flags?: InjectFlags): T|null;
export function ɵɵinject<T>(token: Type<T>| InjectionToken<T>, flags = InjectFlags.Default): T|
null {
return (_injectImplementation || injectInjectorOnly)(token, flags);
return (_injectImplementation || injectInjectorOnly)(resolveForwardRef(token), flags);
}

/**
Expand Down Expand Up @@ -201,7 +201,7 @@ export class NullInjector implements Injector {
// Intentionally left behind: With dev tools open the debugger will stop here. There is no
// reason why correctly written application should cause this exception.
// TODO(misko): uncomment the next line once `ngDevMode` works with closure.
// if(ngDevMode) debugger;
// if (ngDevMode) debugger;
const error = new Error(`NullInjectorError: No provider for ${stringify(token)}!`);
error.name = 'NullInjectorError';
throw error;
Expand Down
5 changes: 3 additions & 2 deletions packages/core/src/render3/styling_next/bindings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import {SafeValue} from '../../sanitization/bypass';
import {StyleSanitizeFn, StyleSanitizeMode} from '../../sanitization/style_sanitizer';
import {ProceduralRenderer3, RElement, Renderer3, RendererStyleFlags3, isProceduralRenderer} from '../interfaces/renderer';

Expand Down Expand Up @@ -110,7 +111,7 @@ export function updateClassBinding(
*/
export function updateStyleBinding(
context: TStylingContext, data: LStylingData, element: RElement, prop: string | null,
bindingIndex: number, value: String | string | number | null | undefined | StylingMapArray,
bindingIndex: number, value: string | number | SafeValue | null | undefined | StylingMapArray,
sanitizer: StyleSanitizeFn | null, deferRegistration: boolean, forceUpdate: boolean): boolean {
const isMapBased = !prop;
const state = getStylingState(element, stateIsPersisted(context));
Expand Down Expand Up @@ -149,7 +150,7 @@ export function updateStyleBinding(
function updateBindingData(
context: TStylingContext, data: LStylingData, counterIndex: number, prop: string | null,
bindingIndex: number,
value: string | String | number | boolean | null | undefined | StylingMapArray,
value: string | SafeValue | number | boolean | null | undefined | StylingMapArray,
deferRegistration: boolean, forceUpdate: boolean, sanitizationRequired: boolean): boolean {
if (!isContextLocked(context)) {
if (deferRegistration) {
Expand Down
19 changes: 10 additions & 9 deletions packages/core/src/render3/styling_next/instructions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import {SafeValue} from '../../sanitization/bypass';
import {StyleSanitizeFn} from '../../sanitization/style_sanitizer';
import {setInputsForProperty} from '../instructions/shared';
import {AttributeMarker, TAttributes, TNode, TNodeType} from '../interfaces/node';
Expand Down Expand Up @@ -96,12 +97,12 @@ export function ɵɵstyleSanitizer(sanitizer: StyleSanitizeFn | null): void {
* @codeGenApi
*/
export function ɵɵstyleProp(
prop: string, value: string | number | String | null, suffix?: string | null): void {
prop: string, value: string | number | SafeValue | null, suffix?: string | null): void {
stylePropInternal(getSelectedIndex(), prop, value, suffix);
}

export function stylePropInternal(
elementIndex: number, prop: string, value: string | number | String | null,
elementIndex: number, prop: string, value: string | number | SafeValue | null,
suffix?: string | null | undefined) {
const lView = getLView();

Expand Down Expand Up @@ -161,8 +162,8 @@ export function ɵɵclassProp(className: string, value: boolean | null): void {
*/
function _stylingProp(
elementIndex: number, bindingIndex: number, prop: string,
value: boolean | number | String | string | null | undefined | NO_CHANGE, isClassBased: boolean,
defer: boolean): boolean {
value: boolean | number | SafeValue | string | null | undefined | NO_CHANGE,
isClassBased: boolean, defer: boolean): boolean {
const lView = getLView();
const tNode = getTNode(elementIndex, lView);
const native = getNativeByTNode(tNode, lView) as RElement;
Expand All @@ -176,8 +177,8 @@ function _stylingProp(
} else {
const sanitizer = getCurrentStyleSanitizer();
updated = updateStyleBinding(
getStylesContext(tNode), lView, native, prop, bindingIndex, value as string | null,
sanitizer, defer, false);
getStylesContext(tNode), lView, native, prop, bindingIndex,
value as string | SafeValue | null, sanitizer, defer, false);
}
}

Expand Down Expand Up @@ -508,8 +509,8 @@ function getContext(tNode: TNode, isClassBased: boolean) {
}

function resolveStylePropValue(
value: string | number | String | null | NO_CHANGE, suffix: string | null | undefined): string|
null|undefined|NO_CHANGE {
value: string | number | SafeValue | null | NO_CHANGE,
suffix: string | null | undefined): string|SafeValue|null|undefined|NO_CHANGE {
if (value === NO_CHANGE) return value;

let resolvedValue: string|null = null;
Expand All @@ -519,7 +520,7 @@ function resolveStylePropValue(
// sanitization entirely (b/c a new string is created)
resolvedValue = renderStringify(value) + suffix;
} else {
// sanitization happens by dealing with a String value
// sanitization happens by dealing with a string value
// this means that the string value will be passed through
// into the style rendering later (which is where the value
// will be sanitized before it is applied)
Expand Down
157 changes: 99 additions & 58 deletions packages/core/src/sanitization/bypass.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,61 +6,120 @@
* found in the LICENSE file at https://angular.io/license
*/

const BRAND = '__SANITIZER_TRUSTED_BRAND__';
import {assertEqual} from '../util/assert';


export const enum BypassType {
Url = 'Url',
Html = 'Html',
ResourceUrl = 'ResourceUrl',
Url = 'URL',
Html = 'HTML',
ResourceUrl = 'ResourceURL',
mhevery marked this conversation as resolved.
Show resolved Hide resolved
Script = 'Script',
Style = 'Style',
}

/**
* A branded trusted string used with sanitization.
* Marker interface for a value that's safe to use in a particular context.
*
* See: {@link TrustedHtmlString}, {@link TrustedResourceUrlString}, {@link TrustedScriptString},
* {@link TrustedStyleString}, {@link TrustedUrlString}
* @publicApi
*/
export interface TrustedString extends String { [BRAND]: BypassType; }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be a breaking change.

export interface SafeValue {}

/**
* A branded trusted string used with sanitization of `html` strings.
* Marker interface for a value that's safe to use as HTML.
*
* See: {@link bypassSanitizationTrustHtml} and {@link htmlSanitizer}.
* @publicApi
*/
export interface TrustedHtmlString extends TrustedString { [BRAND]: BypassType.Html; }
export interface SafeHtml extends SafeValue {}

/**
* A branded trusted string used with sanitization of `style` strings.
* Marker interface for a value that's safe to use as style (CSS).
*
* See: {@link bypassSanitizationTrustStyle} and {@link styleSanitizer}.
* @publicApi
*/
export interface TrustedStyleString extends TrustedString { [BRAND]: BypassType.Style; }
export interface SafeStyle extends SafeValue {}

/**
* A branded trusted string used with sanitization of `url` strings.
* Marker interface for a value that's safe to use as JavaScript.
*
* See: {@link bypassSanitizationTrustScript} and {@link scriptSanitizer}.
* @publicApi
*/
export interface TrustedScriptString extends TrustedString { [BRAND]: BypassType.Script; }
export interface SafeScript extends SafeValue {}

/**
* A branded trusted string used with sanitization of `url` strings.
* Marker interface for a value that's safe to use as a URL linking to a document.
*
* See: {@link bypassSanitizationTrustUrl} and {@link urlSanitizer}.
* @publicApi
*/
export interface TrustedUrlString extends TrustedString { [BRAND]: BypassType.Url; }
export interface SafeUrl extends SafeValue {}

/**
* A branded trusted string used with sanitization of `resourceUrl` strings.
* Marker interface for a value that's safe to use as a URL to load executable code from.
*
* See: {@link bypassSanitizationTrustResourceUrl} and {@link resourceUrlSanitizer}.
* @publicApi
*/
export interface TrustedResourceUrlString extends TrustedString { [BRAND]: BypassType.ResourceUrl; }
export interface SafeResourceUrl extends SafeValue {}


abstract class SafeValueImpl implements SafeValue {
constructor(public changingThisBreaksApplicationSecurity: string) {
// empty
}

abstract getTypeName(): string;

toString() {
return `SafeValue must use [property]=binding: ${this.changingThisBreaksApplicationSecurity}` +
` (see http://g.co/ng/security#xss)`;
}
}

class SafeHtmlImpl extends SafeValueImpl implements SafeHtml {
getTypeName() { return BypassType.Html; }
}
class SafeStyleImpl extends SafeValueImpl implements SafeStyle {
getTypeName() { return BypassType.Style; }
}
class SafeScriptImpl extends SafeValueImpl implements SafeScript {
getTypeName() { return BypassType.Script; }
}
class SafeUrlImpl extends SafeValueImpl implements SafeUrl {
getTypeName() { return BypassType.Url; }
}
class SafeResourceUrlImpl extends SafeValueImpl implements SafeResourceUrl {
getTypeName() { return BypassType.ResourceUrl; }
}

export function unwrapSafeValue(value: SafeValue) {
ngDevMode &&
assertEqual(value instanceof SafeValueImpl, true, 'Expected instance of SafeValueImpl');
return (value as SafeValueImpl).changingThisBreaksApplicationSecurity;
}


export function allowSanitizationBypass(value: any, type: BypassType): boolean {
return (value instanceof String && (value as TrustedStyleString)[BRAND] === type);
export function allowSanitizationBypassAndThrow(
value: any, type: BypassType.Html): value is SafeHtml;
export function allowSanitizationBypassAndThrow(
value: any, type: BypassType.ResourceUrl): value is SafeResourceUrl;
export function allowSanitizationBypassAndThrow(
value: any, type: BypassType.Script): value is SafeScript;
export function allowSanitizationBypassAndThrow(
value: any, type: BypassType.Style): value is SafeStyle;
export function allowSanitizationBypassAndThrow(value: any, type: BypassType.Url): value is SafeUrl;
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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Allow ResourceURLs in URL contexts, they are strictly more trusted.
if (actualType === BypassType.ResourceUrl && type === BypassType.Url) return true;
throw new Error(
`Required a safe ${type}, got a ${actualType} (see http://g.co/ng/security#xss)`);
}
return actualType === type;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have suggestions as to make it more clear?

}

export function getSanitizationBypassType(value: any): BypassType|null {
return value instanceof SafeValueImpl && (value as SafeValueImpl).getTypeName() as BypassType ||
null;
}

/**
Expand All @@ -70,10 +129,10 @@ export function allowSanitizationBypass(value: any, type: BypassType): boolean {
* recognizable to {@link htmlSanitizer} to be trusted implicitly.
*
* @param trustedHtml `html` string which needs to be implicitly trusted.
* @returns a `html` `String` which has been branded to be implicitly trusted.
* @returns a `html` which has been branded to be implicitly trusted.
*/
export function bypassSanitizationTrustHtml(trustedHtml: string): TrustedHtmlString {
return bypassSanitizationTrustString(trustedHtml, BypassType.Html);
export function bypassSanitizationTrustHtml(trustedHtml: string): SafeHtml {
return new SafeHtmlImpl(trustedHtml);
}
/**
* Mark `style` string as trusted.
Expand All @@ -82,10 +141,10 @@ export function bypassSanitizationTrustHtml(trustedHtml: string): TrustedHtmlStr
* recognizable to {@link styleSanitizer} to be trusted implicitly.
*
* @param trustedStyle `style` string which needs to be implicitly trusted.
* @returns a `style` `String` which has been branded to be implicitly trusted.
* @returns a `style` hich has been branded to be implicitly trusted.
*/
export function bypassSanitizationTrustStyle(trustedStyle: string): TrustedStyleString {
return bypassSanitizationTrustString(trustedStyle, BypassType.Style);
export function bypassSanitizationTrustStyle(trustedStyle: string): SafeStyle {
return new SafeStyleImpl(trustedStyle);
}
/**
* Mark `script` string as trusted.
Expand All @@ -94,10 +153,10 @@ export function bypassSanitizationTrustStyle(trustedStyle: string): TrustedStyle
* recognizable to {@link scriptSanitizer} to be trusted implicitly.
*
* @param trustedScript `script` string which needs to be implicitly trusted.
* @returns a `script` `String` which has been branded to be implicitly trusted.
* @returns a `script` which has been branded to be implicitly trusted.
*/
export function bypassSanitizationTrustScript(trustedScript: string): TrustedScriptString {
return bypassSanitizationTrustString(trustedScript, BypassType.Script);
export function bypassSanitizationTrustScript(trustedScript: string): SafeScript {
return new SafeScriptImpl(trustedScript);
}
/**
* Mark `url` string as trusted.
Expand All @@ -106,10 +165,10 @@ export function bypassSanitizationTrustScript(trustedScript: string): TrustedScr
* recognizable to {@link urlSanitizer} to be trusted implicitly.
*
* @param trustedUrl `url` string which needs to be implicitly trusted.
* @returns a `url` `String` which has been branded to be implicitly trusted.
* @returns a `url` which has been branded to be implicitly trusted.
*/
export function bypassSanitizationTrustUrl(trustedUrl: string): TrustedUrlString {
return bypassSanitizationTrustString(trustedUrl, BypassType.Url);
export function bypassSanitizationTrustUrl(trustedUrl: string): SafeUrl {
return new SafeUrlImpl(trustedUrl);
}
/**
* Mark `url` string as trusted.
Expand All @@ -118,26 +177,8 @@ export function bypassSanitizationTrustUrl(trustedUrl: string): TrustedUrlString
* recognizable to {@link resourceUrlSanitizer} to be trusted implicitly.
*
* @param trustedResourceUrl `url` string which needs to be implicitly trusted.
* @returns a `url` `String` which has been branded to be implicitly trusted.
* @returns a `url` which has been branded to be implicitly trusted.
*/
export function bypassSanitizationTrustResourceUrl(trustedResourceUrl: string):
TrustedResourceUrlString {
return bypassSanitizationTrustString(trustedResourceUrl, BypassType.ResourceUrl);
}


function bypassSanitizationTrustString(
trustedString: string, mode: BypassType.Html): TrustedHtmlString;
function bypassSanitizationTrustString(
trustedString: string, mode: BypassType.Style): TrustedStyleString;
function bypassSanitizationTrustString(
trustedString: string, mode: BypassType.Script): TrustedScriptString;
function bypassSanitizationTrustString(
trustedString: string, mode: BypassType.Url): TrustedUrlString;
function bypassSanitizationTrustString(
trustedString: string, mode: BypassType.ResourceUrl): TrustedResourceUrlString;
function bypassSanitizationTrustString(trustedString: string, mode: BypassType): TrustedString {
const trusted = new String(trustedString) as TrustedString;
trusted[BRAND] = mode;
return trusted;
export function bypassSanitizationTrustResourceUrl(trustedResourceUrl: string): SafeResourceUrl {
return new SafeResourceUrlImpl(trustedResourceUrl);
}
Loading