Skip to content

Commit

Permalink
fix(compiler): report errors for missing binding names (#34595)
Browse files Browse the repository at this point in the history 
Currently, would-be binding attributes that are missing binding names
are not parsed as bindings, and fall through as regular attributes. In
some cases, this can lead to a runtime error; trying to assign `#` as a
DOM attribute in an element like in `<div #></div>` fails because `#` is
not a valid attribute name.

Attributes composed of binding prefixes but not defining a binding
should be considered invalid, as this almost certainly indicates an
unintentional elision of a binding by the developer. This commit
introduces error reporting for attributes with a binding name prefix but
no actual binding name.

Closes angular/vscode-ng-language-service#293.

PR Close #34595
  • Loading branch information
@ayazhafiz @kara
ayazhafiz authored and kara committed Feb 11, 2020
1 parent 91a2fd5 commit d13cab7
Show file tree
Hide file tree
Showing 5 changed files with 92 additions and 2 deletions.
7 changes: 6 additions & 1 deletion packages/compiler/src/render3/r3_template_transform.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import * as t from './r3_ast';
import {I18N_ICU_VAR_PREFIX, isI18nRootNode} from './view/i18n/util';

const BIND_NAME_REGEXP =
/^(?:(?:(?:(bind-)|(let-)|(ref-|#)|(on-)|(bindon-)|(@))(.+))|\[\(([^\)]+)\)\]|\[([^\]]+)\]|\(([^\)]+)\))$/;
/^(?:(?:(?:(bind-)|(let-)|(ref-|#)|(on-)|(bindon-)|(@))(.*))|\[\(([^\)]+)\)\]|\[([^\]]+)\]|\(([^\)]+)\))$/;

// Group 1 = "bind-"
const KW_BIND_IDX = 1;
Expand Down Expand Up @@ -399,7 +399,10 @@ class HtmlAstToIvyAst implements html.Visitor {
valueSpan: ParseSourceSpan|undefined, variables: t.Variable[]) {
if (identifier.indexOf('-') > -1) {
this.reportError(`"-" is not allowed in variable names`, sourceSpan);
} else if (identifier.length === 0) {
this.reportError(`Variable does not have a name`, sourceSpan);
}

variables.push(new t.Variable(identifier, value, sourceSpan, valueSpan));
}

Expand All @@ -408,6 +411,8 @@ class HtmlAstToIvyAst implements html.Visitor {
valueSpan: ParseSourceSpan|undefined, references: t.Reference[]) {
if (identifier.indexOf('-') > -1) {
this.reportError(`"-" is not allowed in reference names`, sourceSpan);
} else if (identifier.length === 0) {
this.reportError(`Reference does not have a name`, sourceSpan);
}

references.push(new t.Reference(identifier, value, sourceSpan, valueSpan));
Expand Down
12 changes: 12 additions & 0 deletions packages/compiler/src/template_parser/binding_parser.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,10 @@ export class BindingParser {
name: string, expression: string, isHost: boolean, sourceSpan: ParseSourceSpan,
absoluteOffset: number, valueSpan: ParseSourceSpan|undefined,
targetMatchableAttrs: string[][], targetProps: ParsedProperty[]) {
if (name.length === 0) {
this._reportError(`Property name is missing in binding`, sourceSpan);
}

let isAnimationProp = false;
if (name.startsWith(ANIMATE_PROP_PREFIX)) {
isAnimationProp = true;
Expand Down Expand Up @@ -248,6 +252,10 @@ export class BindingParser {
name: string, expression: string|null, sourceSpan: ParseSourceSpan, absoluteOffset: number,
valueSpan: ParseSourceSpan|undefined, targetMatchableAttrs: string[][],
targetProps: ParsedProperty[]) {
if (name.length === 0) {
this._reportError('Animation trigger is missing', sourceSpan);
}

// This will occur when a @trigger is not paired with an expression.
// For animations it is valid to not have an expression since */void
// states will be applied by angular when the element is attached/detached
Expand Down Expand Up @@ -343,6 +351,10 @@ export class BindingParser {
parseEvent(
name: string, expression: string, sourceSpan: ParseSourceSpan, handlerSpan: ParseSourceSpan,
targetMatchableAttrs: string[][], targetEvents: ParsedEvent[]) {
if (name.length === 0) {
this._reportError(`Event name is missing in binding`, sourceSpan);
}

if (isAnimationLabel(name)) {
name = name.substr(1);
this._parseAnimationEvent(name, expression, sourceSpan, handlerSpan, targetEvents);
Expand Down
6 changes: 5 additions & 1 deletion packages/compiler/src/template_parser/template_parser.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import * as t from './template_ast';
import {PreparsedElementType, preparseElement} from './template_preparser';

const BIND_NAME_REGEXP =
/^(?:(?:(?:(bind-)|(let-)|(ref-|#)|(on-)|(bindon-)|(@))(.+))|\[\(([^\)]+)\)\]|\[([^\]]+)\]|\(([^\)]+)\))$/;
/^(?:(?:(?:(bind-)|(let-)|(ref-|#)|(on-)|(bindon-)|(@))(.*))|\[\(([^\)]+)\)\]|\[([^\]]+)\]|\(([^\)]+)\))$/;

// Group 1 = "bind-"
const KW_BIND_IDX = 1;
Expand Down Expand Up @@ -501,6 +501,8 @@ class TemplateParseVisitor implements html.Visitor {
identifier: string, value: string, sourceSpan: ParseSourceSpan, targetVars: t.VariableAst[]) {
if (identifier.indexOf('-') > -1) {
this._reportError(`"-" is not allowed in variable names`, sourceSpan);
} else if (identifier.length === 0) {
this._reportError(`Variable does not have a name`, sourceSpan);
}

targetVars.push(new t.VariableAst(identifier, value, sourceSpan));
Expand All @@ -511,6 +513,8 @@ class TemplateParseVisitor implements html.Visitor {
targetRefs: ElementOrDirectiveRef[]) {
if (identifier.indexOf('-') > -1) {
this._reportError(`"-" is not allowed in reference names`, sourceSpan);
} else if (identifier.length === 0) {
this._reportError(`Reference does not have a name`, sourceSpan);
}

targetRefs.push(new ElementOrDirectiveRef(identifier, value, sourceSpan));
Expand Down
39 changes: 39 additions & 0 deletions packages/compiler/test/render3/r3_template_transform_spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,10 @@ describe('R3 template transform', () => {
]);
});

it('should report missing property names in bind- syntax', () => {
expect(() => parse('<div bind-></div>')).toThrowError(/Property name is missing in binding/);
});

it('should parse bound properties via {{...}}', () => {
expectFromHtml('<div prop="{{v}}"></div>').toEqual([
['Element', 'div'],
Expand Down Expand Up @@ -339,6 +343,10 @@ describe('R3 template transform', () => {
]);
});

it('should report missing event names in on- syntax', () => {
expect(() => parse('<div on-></div>')).toThrowError(/Event name is missing in binding/);
});

it('should parse bound events and properties via [(...)]', () => {
expectFromHtml('<div [(prop)]="v"></div>').toEqual([
['Element', 'div'],
Expand All @@ -355,12 +363,29 @@ describe('R3 template transform', () => {
]);
});

it('should report missing property names in bindon- syntax', () => {
expect(() => parse('<div bindon-></div>'))
.toThrowError(/Property name is missing in binding/);
});

it('should report an error on empty expression', () => {
expect(() => parse('<div (event)="">')).toThrowError(/Empty expressions are not allowed/);
expect(() => parse('<div (event)=" ">')).toThrowError(/Empty expressions are not allowed/);
});
});

describe('variables', () => {
it('should report variables not on template elements', () => {
expect(() => parse('<div let-a-name="b"></div>'))
.toThrowError(/"let-" is only supported on ng-template elements./);
});

it('should report missing variable names', () => {
expect(() => parse('<ng-template let-><ng-template>'))
.toThrowError(/Variable does not have a name/);
});
});

describe('references', () => {
it('should parse references via #...', () => {
expectFromHtml('<div #a></div>').toEqual([
Expand All @@ -382,6 +407,20 @@ describe('R3 template transform', () => {
['Reference', 'someA', ''],
]);
});

it('should report invalid reference names', () => {
expect(() => parse('<div #a-b></div>')).toThrowError(/"-" is not allowed in reference names/);
});

it('should report missing reference names', () => {
expect(() => parse('<div #></div>')).toThrowError(/Reference does not have a name/);
});
});

describe('literal attribute', () => {
it('should report missing animation trigger in @ syntax', () => {
expect(() => parse('<div @></div>')).toThrowError(/Animation trigger is missing/);
});
});

describe('ng-content', () => {
Expand Down
30 changes: 30 additions & 0 deletions packages/compiler/test/template_parser/template_parser_spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -658,6 +658,11 @@ Binding to attribute 'onEvent' is disallowed for security reasons ("<my-componen
]);
});

it('should report missing property names in bind- syntax', () => {
expect(() => parse('<div bind-></div>', [])).toThrowError(`Template parse errors:
Property name is missing in binding ("<div [ERROR ->]bind-></div>"): TestComp@0:5`);
});

it('should parse bound properties via {{...}} and not report them as attributes', () => {
expect(humanizeTplAst(parse('<div prop="{{v}}">', []))).toEqual([
[ElementAst, 'div'],
Expand All @@ -684,6 +689,11 @@ Binding to attribute 'onEvent' is disallowed for security reasons ("<my-componen
/Assigning animation triggers via @prop="exp" attributes with an expression is invalid. Use property bindings \(e.g. \[@prop\]="exp"\) or use an attribute without a value \(e.g. @prop\) instead. \("<div \[ERROR ->\]@someAnimation="value2">"\): TestComp@0:5/);
});

it('should report missing animation trigger in @ syntax', () => {
expect(() => parse('<div @></div>', [])).toThrowError(`Template parse errors:
Animation trigger is missing ("<div [ERROR ->]@></div>"): TestComp@0:5`);
});

it('should not issue a warning when host attributes contain a valid property-bound animation trigger',
() => {
const animationEntries = ['prop'];
Expand Down Expand Up @@ -804,6 +814,11 @@ Binding to attribute 'onEvent' is disallowed for security reasons ("<my-componen
]))).toEqual([[ElementAst, 'div'], [BoundEventAst, 'event', null, 'v']]);
});

it('should report missing event names in on- syntax', () => {
expect(() => parse('<div on-></div>', []))
.toThrowError(/Event name is missing in binding/);
});

it('should allow events on explicit embedded templates that are emitted by a directive',
() => {
const dirA =
Expand Down Expand Up @@ -840,6 +855,10 @@ Binding to attribute 'onEvent' is disallowed for security reasons ("<my-componen
]);
});

it('should report missing property names in bindon- syntax', () => {
expect(() => parse('<div bindon-></div>', []))
.toThrowError(/Property name is missing in binding/);
});
});

describe('directives', () => {
Expand Down Expand Up @@ -1372,11 +1391,22 @@ There is no directive with "exportAs" set to "dirA" ("<div [ERROR ->]#a="dirA"><
"-" is not allowed in reference names ("<div [ERROR ->]#a-b></div>"): TestComp@0:5`);
});

it('should report missing reference names', () => {
expect(() => parse('<div #></div>', [])).toThrowError(`Template parse errors:
Reference does not have a name ("<div [ERROR ->]#></div>"): TestComp@0:5`);
});

it('should report variables as errors', () => {
expect(() => parse('<div let-a></div>', [])).toThrowError(`Template parse errors:
"let-" is only supported on ng-template elements. ("<div [ERROR ->]let-a></div>"): TestComp@0:5`);
});

it('should report missing variable names', () => {
expect(() => parse('<ng-template let-></ng-template>', []))
.toThrowError(`Template parse errors:
Variable does not have a name ("<ng-template [ERROR ->]let-></ng-template>"): TestComp@0:13`);
});

it('should report duplicate reference names', () => {
expect(() => parse('<div #a></div><div #a></div>', []))
.toThrowError(`Template parse errors:
Expand Down

0 comments on commit d13cab7

Please sign in to comment.