fix(ivy): throw on bindings to unknown properties (#28537)

This commit adds a devMode-only check which will throw if a user
attempts to bind a property that does not match a directive
input or a known HTML property.

Example:
```
<div [unknownProp]="someValue"></div>
```

The above will throw because "unknownProp" is not a known
property of HTMLDivElement.

This check is similar to the check executed in View Engine during
template parsing, but occurs at runtime instead of compile-time.

Note: This change uncovered an existing bug with host binding
inheritance, so some Material tests had to be turned off. They
will be fixed in an upcoming PR.

PR Close #28537
This commit is contained in:
Kara Erickson
2019-02-07 12:37:32 -08:00
committed by Miško Hevery
parent 7660d0d74a
commit 1950e2d9ba
13 changed files with 262 additions and 160 deletions
+17 -4
View File
@@ -10,7 +10,7 @@ import {InjectFlags, InjectionToken, Injector} from '../di';
import {resolveForwardRef} from '../di/forward_ref';
import {ErrorHandler} from '../error_handler';
import {Type} from '../interface/type';
import {validateAttribute, validateProperty} from '../sanitization/sanitization';
import {validateAgainstEventAttributes, validateAgainstEventProperties} from '../sanitization/sanitization';
import {Sanitizer} from '../sanitization/security';
import {StyleSanitizeFn} from '../sanitization/style_sanitizer';
import {assertDataInRange, assertDefined, assertEqual, assertLessThan, assertNotEqual} from '../util/assert';
@@ -39,7 +39,7 @@ import {isNodeMatchingSelectorList, matchingSelectorIndex} from './node_selector
import {decreaseElementDepthCount, enterView, getBindingsEnabled, getCheckNoChangesMode, getContextLView, getCurrentDirectiveDef, getElementDepthCount, getIsParent, getLView, getPreviousOrParentTNode, increaseElementDepthCount, isCreationMode, leaveView, nextContextImpl, resetComponentState, setBindingRoot, setCheckNoChangesMode, setCurrentDirectiveDef, setCurrentQueryIndex, setIsParent, setPreviousOrParentTNode} from './state';
import {getInitialClassNameValue, initializeStaticContext as initializeStaticStylingContext, patchContextWithStaticAttrs, renderInitialStylesAndClasses, renderStyling, updateClassProp as updateElementClassProp, updateContextWithBindings, updateStyleProp as updateElementStyleProp, updateStylingMap} from './styling/class_and_style_bindings';
import {BoundPlayerFactory} from './styling/player_factory';
import {createEmptyStylingContext, getStylingContext, hasClassInput, hasStyling, isAnimationProp} from './styling/util';
import {ANIMATION_PROP_PREFIX, createEmptyStylingContext, getStylingContext, hasClassInput, hasStyling, isAnimationProp} from './styling/util';
import {NO_CHANGE} from './tokens';
import {INTERPOLATION_DELIMITER, findComponentView, getComponentViewByIndex, getNativeByIndex, getNativeByTNode, getRootContext, getRootView, getTNode, isComponent, isComponentDef, isContentQueryHost, loadInternal, readElementValue, readPatchedLView, renderStringify} from './util';
@@ -1099,7 +1099,7 @@ export function elementAttribute(
index: number, name: string, value: any, sanitizer?: SanitizerFn | null,
namespace?: string): void {
if (value !== NO_CHANGE) {
ngDevMode && validateAttribute(name);
ngDevMode && validateAgainstEventAttributes(name);
const lView = getLView();
const renderer = lView[RENDERER];
const element = getNativeByIndex(index, lView);
@@ -1193,7 +1193,8 @@ function elementPropertyInternal<T>(
}
} else if (tNode.type === TNodeType.Element) {
if (ngDevMode) {
validateProperty(propName);
validateAgainstEventProperties(propName);
validateAgainstUnknownProperties(element, propName, tNode);
ngDevMode.rendererSetProperty++;
}
@@ -1212,6 +1213,18 @@ function elementPropertyInternal<T>(
}
}
function validateAgainstUnknownProperties(
element: RElement | RComment, propName: string, tNode: TNode) {
// If prop is not a known property of the HTML element...
if (!(propName in element) &&
// and isn't a synthetic animation property...
propName[0] !== ANIMATION_PROP_PREFIX) {
// ... it is probably a user error and we should throw.
throw new Error(
`Template error: Can't bind to '${propName}' since it isn't a known property of '${tNode.tagName}'.`);
}
}
/**
* Stores debugging data for this property binding on first template pass.
* This enables features like DebugElement.properties.
+1 -1
View File
@@ -20,7 +20,7 @@ import {getTNode} from '../util';
import {CorePlayerHandler} from './core_player_handler';
const ANIMATION_PROP_PREFIX = '@';
export const ANIMATION_PROP_PREFIX = '@';
export function createEmptyStylingContext(
element?: RElement | null, sanitizer?: StyleSanitizeFn | null,
@@ -178,7 +178,7 @@ export const defaultStyleSanitizer = (function(prop: string, value?: string): st
return sanitizeStyle(value);
} as StyleSanitizeFn);
export function validateProperty(name: string) {
export function validateAgainstEventProperties(name: string) {
if (name.toLowerCase().startsWith('on')) {
const msg = `Binding to event property '${name}' is disallowed for security reasons, ` +
`please use (${name.slice(2)})=...` +
@@ -188,7 +188,7 @@ export function validateProperty(name: string) {
}
}
export function validateAttribute(name: string) {
export function validateAgainstEventAttributes(name: string) {
if (name.toLowerCase().startsWith('on')) {
const msg = `Binding to event attribute '${name}' is disallowed for security reasons, ` +
`please use (${name.slice(2)})=...`;
File diff suppressed because it is too large Load Diff
+17 -2
View File
@@ -22,7 +22,7 @@ import {getDOM} from '@angular/platform-browser/src/dom/dom_adapter';
import {DOCUMENT} from '@angular/platform-browser/src/dom/dom_tokens';
import {dispatchEvent, el} from '@angular/platform-browser/testing/src/browser_util';
import {expect} from '@angular/platform-browser/testing/src/matchers';
import {fixmeIvy, modifiedInIvy, obsoleteInIvy} from '@angular/private/testing';
import {modifiedInIvy, obsoleteInIvy, onlyInIvy} from '@angular/private/testing';
import {stringify} from '../../src/util/stringify';
@@ -1592,7 +1592,7 @@ function declareTests(config?: {useJit: boolean}) {
});
describe('Property bindings', () => {
fixmeIvy('FW-721: Bindings to unknown properties are not reported as errors')
modifiedInIvy('Unknown property error thrown during update mode, not creation mode')
.it('should throw on bindings to unknown properties', () => {
TestBed.configureTestingModule({declarations: [MyComp]});
const template = '<div unknown="{{ctxProp}}"></div>';
@@ -1606,6 +1606,21 @@ function declareTests(config?: {useJit: boolean}) {
}
});
onlyInIvy('Unknown property error thrown during update mode, not creation mode')
.it('should throw on bindings to unknown properties', () => {
TestBed.configureTestingModule({declarations: [MyComp]});
const template = '<div unknown="{{ctxProp}}"></div>';
TestBed.overrideComponent(MyComp, {set: {template}});
try {
const fixture = TestBed.createComponent(MyComp);
fixture.detectChanges();
throw 'Should throw';
} catch (e) {
expect(e.message).toMatch(
/Template error: Can't bind to 'unknown' since it isn't a known property of 'div'./);
}
});
it('should not throw for property binding to a non-existing property when there is a matching directive property',
() => {
TestBed.configureTestingModule({declarations: [MyComp, MyDir]});
@@ -239,7 +239,7 @@ function declareTests(config?: {useJit: boolean}) {
});
describe('schemas', () => {
fixmeIvy('FW-819: ngtsc compiler should support schemas')
modifiedInIvy('Unknown property error thrown during update mode, not creation mode')
.it('should error on unknown bound properties on custom elements by default', () => {
@Component({template: '<some-element [someUnknownProp]="true"></some-element>'})
class ComponentUsingInvalidProperty {
@@ -252,19 +252,36 @@ function declareTests(config?: {useJit: boolean}) {
expect(() => createModule(SomeModule)).toThrowError(/Can't bind to 'someUnknownProp'/);
});
it('should not error on unknown bound properties on custom elements when using the CUSTOM_ELEMENTS_SCHEMA',
() => {
@Component({template: '<some-element [someUnknownProp]="true"></some-element>'})
class ComponentUsingInvalidProperty {
}
onlyInIvy('Unknown property error thrown during update mode, not creation mode')
.it('should error on unknown bound properties on custom elements by default', () => {
@Component({template: '<some-element [someUnknownProp]="true"></some-element>'})
class ComponentUsingInvalidProperty {
}
@NgModule(
{schemas: [CUSTOM_ELEMENTS_SCHEMA], declarations: [ComponentUsingInvalidProperty]})
class SomeModule {
}
@NgModule({declarations: [ComponentUsingInvalidProperty]})
class SomeModule {
}
expect(() => createModule(SomeModule)).not.toThrow();
});
const fixture = createComp(ComponentUsingInvalidProperty, SomeModule);
expect(() => fixture.detectChanges()).toThrowError(/Can't bind to 'someUnknownProp'/);
});
fixmeIvy('FW-819: ngtsc compiler should support schemas')
.it('should not error on unknown bound properties on custom elements when using the CUSTOM_ELEMENTS_SCHEMA',
() => {
@Component({template: '<some-element [someUnknownProp]="true"></some-element>'})
class ComponentUsingInvalidProperty {
}
@NgModule({
schemas: [CUSTOM_ELEMENTS_SCHEMA],
declarations: [ComponentUsingInvalidProperty]
})
class SomeModule {
}
expect(() => createModule(SomeModule)).not.toThrow();
});
});
describe('id', () => {
@@ -255,7 +255,7 @@ function declareTests(config?: {useJit: boolean}) {
expect(getDOM().getStyle(e, 'background')).not.toContain('javascript');
});
fixmeIvy('FW-850: Should throw on unsafe SVG attributes')
modifiedInIvy('Unknown property error thrown during update mode, not creation mode')
.it('should escape unsafe SVG attributes', () => {
const template = `<svg:circle [xlink:href]="ctxProp">Text</svg:circle>`;
TestBed.overrideComponent(SecuredComponent, {set: {template}});
@@ -264,6 +264,15 @@ function declareTests(config?: {useJit: boolean}) {
.toThrowError(/Can't bind to 'xlink:href'/);
});
onlyInIvy('Unknown property error thrown during update mode, not creation mode')
.it('should escape unsafe SVG attributes', () => {
const template = `<svg:circle [xlink:href]="ctxProp">Text</svg:circle>`;
TestBed.overrideComponent(SecuredComponent, {set: {template}});
const fixture = TestBed.createComponent(SecuredComponent);
expect(() => fixture.detectChanges()).toThrowError(/Can't bind to 'xlink:href'/);
});
it('should escape unsafe HTML values', () => {
const template = `<div [innerHTML]="ctxProp">Text</div>`;
TestBed.overrideComponent(SecuredComponent, {set: {template}});
@@ -182,11 +182,12 @@ describe('instructions', () => {
});
it('should not stringify non string values', () => {
const t = new TemplateFixture(createDiv, () => {}, 1);
const t = new TemplateFixture(() => { element(0, 'input'); }, () => {}, 1);
t.update(() => elementProperty(0, 'hidden', false));
// The hidden property would be true if `false` was stringified into `"false"`.
expect((t.hostElement as HTMLElement).querySelector('div') !.hidden).toEqual(false);
// Note: don't use 'hidden' here because IE10 does not support the hidden property
t.update(() => elementProperty(0, 'required', false));
// The required property would be true if `false` was stringified into `"false"`.
expect((t.hostElement as HTMLElement).querySelector('input') !.required).toEqual(false);
expect(ngDevMode).toHaveProperties({
firstTemplatePass: 1,
tNode: 2, // 1 for div, 1 for host element
+5 -4
View File
@@ -443,7 +443,8 @@ describe('lifecycles', () => {
factory: () => new Component(), template,
consts: consts,
vars: vars,
directives: directives
directives: directives,
inputs: {val: 'val'}
});
};
}
@@ -2665,10 +2666,10 @@ describe('lifecycles', () => {
element(0, 'div');
}
if (rf & RenderFlags.Update) {
elementProperty(0, 'data-a', bind(ctx.a));
elementProperty(0, 'id', bind(ctx.a));
}
},
selectors: [['mycomp']],
selectors: [['my-comp']],
inputs: {
value: 'value',
},
@@ -2683,7 +2684,7 @@ describe('lifecycles', () => {
const App = createComponent('app', function(rf: RenderFlags, ctx: any) {
if (rf & RenderFlags.Create) {
element(0, 'mycomp');
element(0, 'my-comp');
}
if (rf & RenderFlags.Update) {
elementProperty(0, 'value', bind(1));
+5 -5
View File
@@ -170,12 +170,12 @@ describe('pipe', () => {
elementEnd();
}
if (rf & RenderFlags.Update) {
elementProperty(0, 'someProp', bind(pipeBind1(1, 1, 'Megatron')));
elementProperty(0, 'id', bind(pipeBind1(1, 1, 'Megatron')));
}
}
renderToHtml(Template, person, 2, 3, null, [IdentityPipe], rendererFactory2);
expect(renderLog.log).toEqual(['someProp=Megatron']);
expect(renderLog.log).toEqual(['id=Megatron']);
renderLog.clear();
renderToHtml(Template, person, 2, 3, null, pipes, rendererFactory2);
@@ -255,8 +255,8 @@ describe('pipe', () => {
container(4);
}
if (rf & RenderFlags.Update) {
elementProperty(0, 'someProp', bind(pipeBind1(1, 2, true)));
elementProperty(2, 'someProp', bind(pipeBind1(3, 4, true)));
elementProperty(0, 'id', bind(pipeBind1(1, 2, true)));
elementProperty(2, 'id', bind(pipeBind1(3, 4, true)));
pipeInstances.push(load<CountingImpurePipe>(1), load(3));
containerRefreshStart(4);
{
@@ -269,7 +269,7 @@ describe('pipe', () => {
elementEnd();
}
if (rf1 & RenderFlags.Update) {
elementProperty(0, 'someProp', bind(pipeBind1(1, 1, true)));
elementProperty(0, 'id', bind(pipeBind1(1, 1, true)));
pipeInstances.push(load<CountingImpurePipe>(1));
}
}
@@ -42,12 +42,14 @@ describe('ViewContainerRef', () => {
factory: () => directiveInstance = new DirectiveWithVCRef(
directiveInject(ViewContainerRef as any), injectComponentFactoryResolver()),
inputs: {tplRef: 'tplRef'}
inputs: {tplRef: 'tplRef', name: 'name'}
});
// TODO(issue/24571): remove '!'.
tplRef !: TemplateRef<{}>;
name: string = '';
// injecting a ViewContainerRef to create a dynamic container in which embedded views will be
// created
constructor(public vcref: ViewContainerRef, public cfr: ComponentFactoryResolver) {}
@@ -17,7 +17,7 @@ import {MockBackend, MockConnection} from '@angular/http/testing';
import {BrowserModule, DOCUMENT, Title, TransferState, makeStateKey} from '@angular/platform-browser';
import {getDOM} from '@angular/platform-browser/src/dom/dom_adapter';
import {BEFORE_APP_SERIALIZED, INITIAL_CONFIG, PlatformState, ServerModule, ServerTransferStateModule, platformDynamicServer, renderModule, renderModuleFactory} from '@angular/platform-server';
import {fixmeIvy, ivyEnabled} from '@angular/private/testing';
import {fixmeIvy, ivyEnabled, modifiedInIvy} from '@angular/private/testing';
import {Observable} from 'rxjs';
import {first} from 'rxjs/operators';
@@ -99,7 +99,7 @@ class TitleApp {
class TitleAppModule {
}
@Component({selector: 'app', template: '{{text}}<h1 [innerText]="h1"></h1>'})
@Component({selector: 'app', template: '{{text}}<h1 [textContent]="h1"></h1>'})
class MyAsyncServerApp {
text = '';
h1 = '';
@@ -276,6 +276,19 @@ class MyHostComponent {
class FalseAttributesModule {
}
@Component({selector: 'app', template: '<div [innerText]="foo"></div>'})
class InnerTextComponent {
foo = 'Some text';
}
@NgModule({
declarations: [InnerTextComponent],
bootstrap: [InnerTextComponent],
imports: [ServerModule, BrowserModule.withServerTransition({appId: 'inner-text'})]
})
class InnerTextModule {
}
@Component({selector: 'app', template: '<input [name]="name">'})
class MyInputComponent {
@Input()
@@ -528,7 +541,7 @@ class HiddenModule {
let doc: string;
let called: boolean;
let expectedOutput =
'<html><head></head><body><app ng-version="0.0.0-PLACEHOLDER">Works!<h1 innertext="fine">fine</h1></app></body></html>';
'<html><head></head><body><app ng-version="0.0.0-PLACEHOLDER">Works!<h1 textcontent="fine">fine</h1></app></body></html>';
beforeEach(() => {
// PlatformConfig takes in a parsed document so that it can be cached across requests.
@@ -567,6 +580,15 @@ class HiddenModule {
});
}));
modifiedInIvy('Will not support binding to innerText in Ivy since domino does not')
.it('should support binding to innerText', async(() => {
renderModule(InnerTextModule, {document: doc}).then(output => {
expect(output).toBe(
'<html><head></head><body><app ng-version="0.0.0-PLACEHOLDER"><div innertext="Some text">Some text</div></app></body></html>');
called = true;
});
}));
it('using renderModuleFactory should work',
async(inject([PlatformRef], (defaultPlatform: PlatformRef) => {
const compilerFactory: CompilerFactory =
+52
View File
@@ -953,6 +953,58 @@ window.testBlocklist = {
"error": "Error: Expected undefined to be truthy.",
"notes": "Unknown"
},
"MatButton should apply class based on color attribute": {
"error": "Template error: Can't bind to 'disabled' since it isn't a known property of 'a'",
"notes": "FW-1037: Host bindings for host objects in metadata are inherited"
},
"MatButton should not clear previous defined classes": {
"error": "Template error: Can't bind to 'disabled' since it isn't a known property of 'a'",
"notes": "FW-1037: Host bindings for host objects in metadata are inherited"
},
"MatButton button[mat-fab] should have accent palette by default": {
"error": "Template error: Can't bind to 'disabled' since it isn't a known property of 'a'",
"notes": "FW-1037: Host bindings for host objects in metadata are inherited"
},
"MatButton button[mat-mini-fab] should have accent palette by default": {
"error": "Template error: Can't bind to 'disabled' since it isn't a known property of 'a'",
"notes": "FW-1037: Host bindings for host objects in metadata are inherited"
},
"MatButton button[mat-button] should not increment if disabled": {
"error": "Template error: Can't bind to 'disabled' since it isn't a known property of 'a'",
"notes": "FW-1037: Host bindings for host objects in metadata are inherited"
},
"MatButton button[mat-button] should disable the native button element": {
"error": "Template error: Can't bind to 'disabled' since it isn't a known property of 'a'",
"notes": "FW-1037: Host bindings for host objects in metadata are inherited"
},
"MatButton a[mat-button] should not redirect if disabled": {
"error": "Template error: Can't bind to 'disabled' since it isn't a known property of 'a'",
"notes": "FW-1037: Host bindings for host objects in metadata are inherited"
},
"MatButton a[mat-button] should remove tabindex if disabled": {
"error": "Template error: Can't bind to 'disabled' since it isn't a known property of 'a'",
"notes": "FW-1037: Host bindings for host objects in metadata are inherited"
},
"MatButton a[mat-button] should add aria-disabled attribute if disabled": {
"error": "Template error: Can't bind to 'disabled' since it isn't a known property of 'a'",
"notes": "FW-1037: Host bindings for host objects in metadata are inherited"
},
"MatButton a[mat-button] should not add aria-disabled attribute if disabled is false": {
"error": "Template error: Can't bind to 'disabled' since it isn't a known property of 'a'",
"notes": "FW-1037: Host bindings for host objects in metadata are inherited"
},
"MatButton a[mat-button] should be able to set a custom tabindex": {
"error": "Template error: Can't bind to 'disabled' since it isn't a known property of 'a'",
"notes": "FW-1037: Host bindings for host objects in metadata are inherited"
},
"MatButton button ripples should disable the ripple if matRippleDisabled input is set": {
"error": "Template error: Can't bind to 'disabled' since it isn't a known property of 'a'",
"notes": "FW-1037: Host bindings for host objects in metadata are inherited"
},
"MatButton button ripples should disable the ripple when the button is disabled": {
"error": "Template error: Can't bind to 'disabled' since it isn't a known property of 'a'",
"notes": "FW-1037: Host bindings for host objects in metadata are inherited"
},
"MatTabHeader focusing should initialize to the selected index": {
"error": "TypeError: Cannot read property 'nativeElement' of undefined",
"notes": "FW-1019: Design new API to replace static queries"