Skip to content

Commit e43eb96

Browse files
splincodethePunderWoman
splincode
authored and
thePunderWoman
committed
fix(core): warn when style property bindings receive invalid values
Report unsupported style property binding values in development mode while preserving existing binding behavior. Unwrap trusted style values before appending unit suffixes and link NG0318 warnings to the corresponding error guide.
1 parent d068fc1 commit e43eb96

7 files changed

Lines changed: 283 additions & 6 deletions

File tree

‎adev/src/content/reference/errors/NG0318.md‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
# Invalid style property binding value
2+
3+
Angular detected a value with an unsupported type in a style property binding. This can occur in
4+
an individual binding such as `[style.width]` or in a property of a `[style]` map.
5+
6+
Style property values accept strings, numbers, style `SafeValue` objects, `null`, and `undefined`.
7+
Other values, including `SafeValue` objects created for a non-style security context, produce this
8+
warning in development mode.
9+
10+
For example, the following binding passes a boolean instead of a CSS value:
11+
12+
```angular-html
13+
<div [style.display]="isVisible"></div>
14+
```
15+
16+
## Debugging the error
17+
18+
Use the property name and value in the error message to locate the binding. Convert the value to a
19+
valid CSS string or number, or use `null` or `undefined` to remove the style.
20+
21+
When using a unit suffix such as `[style.width.px]`, bind the numeric portion of the value:
22+
23+
```angular-html
24+
<div [style.width.px]="width"></div>
25+
```
26+
27+
Do not pass a `SafeValue` created for another security context, such as trusted HTML, to a style
28+
property binding.

‎adev/src/content/reference/errors/overview.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
| `NG0300` | [Selector Collision](errors/NG0300) |
1616
| `NG0301` | [Export Not Found](errors/NG0301) |
1717
| `NG0302` | [Pipe Not Found](errors/NG0302) |
18+
| `NG0318` | [Invalid style property binding value](errors/NG0318) |
1819
| `NG0401` | [Missing platform](errors/NG0401) |
1920
| `NG0403` | [Bootstrapped NgModule doesn't specify which component to initialize](errors/NG0403) |
2021
| `NG0500` | [Hydration Node Mismatch](errors/NG0500) |

‎goldens/public-api/core/errors.api.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,8 @@ export const enum RuntimeErrorCode {
100100
// (undocumented)
101101
INVALID_SKIP_HYDRATION_HOST = -504,
102102
// (undocumented)
103+
INVALID_STYLE_PROP_VALUE = -318,
104+
// (undocumented)
103105
LOOP_TRACK_DUPLICATE_KEYS = -955,
104106
// (undocumented)
105107
LOOP_TRACK_RECREATE = -956,

‎packages/core/src/errors.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@ export const enum RuntimeErrorCode {
6666
NO_BINDING_TARGET = 315,
6767
INVALID_BINDING_TARGET = 316,
6868
INVALID_SET_INPUT_CALL = 317,
69+
INVALID_STYLE_PROP_VALUE = -318,
6970

7071
// Bootstrap Errors
7172
MULTIPLE_PLATFORMS = 400,

‎packages/core/src/render3/instructions/styling.ts‎

Lines changed: 43 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,13 @@
66
* found in the LICENSE file at https://angular.dev/license
77
*/
88

9-
import {SafeValue, unwrapSafeValue} from '../../sanitization/bypass';
9+
import {formatRuntimeError, RuntimeErrorCode} from '../../errors';
10+
import {
11+
BypassType,
12+
getSanitizationBypassType,
13+
SafeValue,
14+
unwrapSafeValue,
15+
} from '../../sanitization/bypass';
1016
import {KeyValueArray, keyValueArrayGet, keyValueArraySet} from '../../util/array_utils';
1117
import {
1218
assertDefined,
@@ -216,6 +222,9 @@ export function checkStylingProperty(
216222
stylingFirstUpdatePass(tView, prop, bindingIndex, isClassBased);
217223
}
218224
if (value !== NO_CHANGE && bindingUpdated(lView, bindingIndex, value)) {
225+
if (ngDevMode && !isClassBased) {
226+
warnInvalidStylePropValue(prop, value);
227+
}
219228
const tNode = tView.data[getSelectedIndex()] as TNode;
220229
updateStyling(
221230
tView,
@@ -230,6 +239,36 @@ export function checkStylingProperty(
230239
}
231240
}
232241

242+
function warnInvalidStylePropValue(prop: string, value: unknown): void {
243+
if (
244+
value == null ||
245+
typeof value === 'string' ||
246+
typeof value === 'number' ||
247+
getSanitizationBypassType(value) === BypassType.Style
248+
) {
249+
return;
250+
}
251+
252+
console.warn(
253+
formatRuntimeError(
254+
RuntimeErrorCode.INVALID_STYLE_PROP_VALUE,
255+
`\`[style.${prop}]\` was bound to an invalid value. ` +
256+
`Expected a string, number, SafeValue, null, or undefined, but received ` +
257+
`\`${typeof value}\` (\`${stringifyInvalidStylePropValue(value)}\`).`,
258+
),
259+
);
260+
}
261+
262+
function stringifyInvalidStylePropValue(value: unknown): string {
263+
try {
264+
return stringify(value);
265+
} catch {
266+
// Invalid values may have throwing accessors or conversion methods. A development diagnostic
267+
// must not interrupt the binding update when the value cannot be represented in the warning.
268+
return '[unstringifiable value]';
269+
}
270+
}
271+
233272
/**
234273
* Common code between `ɵɵclassMap` and `ɵɵstyleMap`.
235274
*
@@ -687,10 +726,7 @@ export function toStylingKeyValueArray(
687726
if (value == null /*|| value === undefined */ || value === '') return EMPTY_ARRAY as any;
688727
const styleKeyValueArray: KeyValueArray<any> = [] as any;
689728
const unwrappedValue = unwrapSafeValue(value) as
690-
| string
691-
| string[]
692-
| Set<string>
693-
| {[key: string]: any};
729+
string | string[] | Set<string> | {[key: string]: any};
694730
if (Array.isArray(unwrappedValue)) {
695731
for (let i = 0; i < unwrappedValue.length; i++) {
696732
keyValueArraySet(styleKeyValueArray, unwrappedValue[i], true);
@@ -726,6 +762,7 @@ export function toStylingKeyValueArray(
726762
* @param value The value to set.
727763
*/
728764
export function styleKeyValueArraySet(keyValueArray: KeyValueArray<any>, key: string, value: any) {
765+
ngDevMode && warnInvalidStylePropValue(key, value);
729766
keyValueArraySet(keyValueArray, key, unwrapSafeValue(value));
730767
}
731768

@@ -997,7 +1034,7 @@ function normalizeSuffix(
9971034
// As it produce invalid CSS, which the browsers will automatically omit but Domino will not.
9981035
// Example: `"left": "px;"` instead of `"left": ""`.
9991036
} else if (typeof suffix === 'string') {
1000-
value = value + suffix;
1037+
value = unwrapSafeValue(value) + suffix;
10011038
} else if (typeof value === 'object') {
10021039
value = stringify(unwrapSafeValue(value));
10031040
}

‎packages/core/test/render3/instructions/styling_spec.ts‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import {AttributeMarker, DirectiveDef} from '../../../src/render3';
1010
import {ɵɵdefineDirective} from '../../../src/render3/definition';
1111
import {
1212
classStringParser,
13+
styleKeyValueArraySet,
1314
styleStringParser,
1415
toStylingKeyValueArray,
1516
ɵɵclassProp,
@@ -32,6 +33,10 @@ import {
3233
import {HEADER_OFFSET, TVIEW} from '../../../src/render3/interfaces/view';
3334
import {getLView, leaveView, setBindingRootForHostBindings} from '../../../src/render3/state';
3435
import {getNativeByIndex} from '../../../src/render3/util/view_utils';
36+
import {
37+
bypassSanitizationTrustHtml,
38+
bypassSanitizationTrustStyle,
39+
} from '../../../src/sanitization/bypass';
3540
import {keyValueArraySet} from '../../../src/util/array_utils';
3641
import {getElementClasses, getElementStyles} from '../../../testing/src/styling';
3742

@@ -423,6 +428,46 @@ describe('styling', () => {
423428
'x',
424429
] as any);
425430
});
431+
432+
it('should accept supported style values', () => {
433+
const warnSpy = spyOn(console, 'warn');
434+
435+
expect(
436+
toStylingKeyValueArray(styleKeyValueArraySet, null!, {
437+
color: 'red',
438+
width: 10,
439+
display: bypassSanitizationTrustStyle('block'),
440+
opacity: null,
441+
}),
442+
).toEqual(['color', 'red', 'display', 'block', 'opacity', null, 'width', 10] as any);
443+
expect(warnSpy).not.toHaveBeenCalled();
444+
});
445+
446+
it('should warn about unsupported style values', () => {
447+
const warnSpy = spyOn(console, 'warn');
448+
449+
expect(
450+
toStylingKeyValueArray(styleKeyValueArraySet, null!, {
451+
display: true,
452+
color: bypassSanitizationTrustHtml('red'),
453+
}),
454+
).toEqual(['color', 'red', 'display', true] as any);
455+
expect(warnSpy).toHaveBeenCalledTimes(2);
456+
expect(warnSpy).toHaveBeenCalledWith(
457+
'NG0318: `[style.display]` was bound to an invalid value. ' +
458+
'Expected a string, number, SafeValue, null, or undefined, but received ' +
459+
'`boolean` (`true`). ' +
460+
'Find more at https://next.angular.dev/errors/NG0318',
461+
);
462+
expect(warnSpy).toHaveBeenCalledWith(
463+
'NG0318: `[style.color]` was bound to an invalid value. ' +
464+
'Expected a string, number, SafeValue, null, or undefined, but received `object` ' +
465+
'(`SafeValue must use [property]=binding: red ' +
466+
'(see https://angular.dev/best-practices/security#preventing-cross-site-scripting-xss)`). ' +
467+
'Find more at https://next.angular.dev/errors/NG0318',
468+
);
469+
});
470+
426471
it('should parse objects with null prototype', () => {
427472
const nullProtoObj = Object.assign(Object.create(null), {X: 'x', A: 'a'});
428473
expect(toStylingKeyValueArray(keyValueArraySet, null!, nullProtoObj)).toEqual([

‎packages/core/test/render3/instructions_spec.ts‎

Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,150 @@ describe('instructions', () => {
197197
});
198198

199199
describe('styleProp', () => {
200+
it('should warn when a style property is bound to a boolean', () => {
201+
const warnSpy = spyOn(console, 'warn');
202+
const t = new ViewFixture({
203+
create: createDiv,
204+
update: () => ɵɵstyleProp('display', true),
205+
decls: 1,
206+
vars: 2,
207+
});
208+
209+
t.update();
210+
211+
expect(warnSpy).toHaveBeenCalledOnceWith(
212+
'NG0318: `[style.display]` was bound to an invalid value. ' +
213+
'Expected a string, number, SafeValue, null, or undefined, but received `boolean` (`true`). ' +
214+
'Find more at https://next.angular.dev/errors/NG0318',
215+
);
216+
});
217+
218+
it('should warn when a style property is bound to an object', () => {
219+
const warnSpy = spyOn(console, 'warn');
220+
const t = new ViewFixture({
221+
create: createDiv,
222+
update: () => ɵɵstyleProp('color', {r: 255, g: 0, b: 0}),
223+
decls: 1,
224+
vars: 2,
225+
});
226+
227+
t.update();
228+
229+
expect(warnSpy).toHaveBeenCalledOnceWith(
230+
'NG0318: `[style.color]` was bound to an invalid value. ' +
231+
'Expected a string, number, SafeValue, null, or undefined, but received ' +
232+
'`object` (`[object Object]`). ' +
233+
'Find more at https://next.angular.dev/errors/NG0318',
234+
);
235+
});
236+
237+
it('should warn when a style property is bound to a non-style SafeValue', () => {
238+
const warnSpy = spyOn(console, 'warn');
239+
const t = new ViewFixture({
240+
create: createDiv,
241+
update: () => ɵɵstyleProp('color', bypassSanitizationTrustHtml('red')),
242+
decls: 1,
243+
vars: 2,
244+
});
245+
246+
t.update();
247+
248+
expect(warnSpy).toHaveBeenCalledOnceWith(
249+
'NG0318: `[style.color]` was bound to an invalid value. ' +
250+
'Expected a string, number, SafeValue, null, or undefined, but received `object` ' +
251+
'(`SafeValue must use [property]=binding: red ' +
252+
'(see https://angular.dev/best-practices/security#preventing-cross-site-scripting-xss)`). ' +
253+
'Find more at https://next.angular.dev/errors/NG0318',
254+
);
255+
});
256+
257+
it('should unwrap a style SafeValue before appending a suffix', () => {
258+
const warnSpy = spyOn(console, 'warn');
259+
const t = new ViewFixture({
260+
create: createDiv,
261+
update: () => ɵɵstyleProp('width', bypassSanitizationTrustStyle('100'), 'px'),
262+
decls: 1,
263+
vars: 2,
264+
});
265+
266+
t.update();
267+
268+
expect((t.host.firstChild as HTMLElement).style.width).toBe('100px');
269+
expect(warnSpy).not.toHaveBeenCalled();
270+
});
271+
272+
it('should warn when a style property with a suffix is bound to a non-style SafeValue', () => {
273+
const warnSpy = spyOn(console, 'warn');
274+
const t = new ViewFixture({
275+
create: createDiv,
276+
update: () => ɵɵstyleProp('width', bypassSanitizationTrustHtml('100'), 'px'),
277+
decls: 1,
278+
vars: 2,
279+
});
280+
281+
t.update();
282+
283+
expect(warnSpy).toHaveBeenCalledOnceWith(
284+
'NG0318: `[style.width]` was bound to an invalid value. ' +
285+
'Expected a string, number, SafeValue, null, or undefined, but received `object` ' +
286+
'(`SafeValue must use [property]=binding: 100 ' +
287+
'(see https://angular.dev/best-practices/security#preventing-cross-site-scripting-xss)`). ' +
288+
'Find more at https://next.angular.dev/errors/NG0318',
289+
);
290+
});
291+
292+
it('should continue binding when an invalid value has a throwing getter', () => {
293+
const warnSpy = spyOn(console, 'warn');
294+
const value = Object.assign(() => {}, {toString: () => 'applied-value'});
295+
Object.defineProperty(value, 'overriddenName', {
296+
get: () => {
297+
throw new Error('Unexpected read of overriddenName');
298+
},
299+
});
300+
const t = new ViewFixture({
301+
create: createDiv,
302+
update: () => ɵɵstyleProp('--test-value', value),
303+
decls: 1,
304+
vars: 2,
305+
});
306+
307+
t.update();
308+
309+
expect((t.host.firstChild as HTMLElement).style.getPropertyValue('--test-value')).toBe(
310+
'applied-value',
311+
);
312+
expect(warnSpy).toHaveBeenCalledOnceWith(
313+
'NG0318: `[style.--test-value]` was bound to an invalid value. ' +
314+
'Expected a string, number, SafeValue, null, or undefined, but received ' +
315+
'`function` (`[unstringifiable value]`). ' +
316+
'Find more at https://next.angular.dev/errors/NG0318',
317+
);
318+
});
319+
320+
it('should not warn when a style property is bound to a supported value', () => {
321+
const warnSpy = spyOn(console, 'warn');
322+
let value: string | number | SafeValue | null | undefined = 'block';
323+
const t = new ViewFixture({
324+
create: createDiv,
325+
update: () => ɵɵstyleProp('display', value),
326+
decls: 1,
327+
vars: 2,
328+
});
329+
330+
for (const supportedValue of [
331+
'inline',
332+
1,
333+
bypassSanitizationTrustStyle('block'),
334+
null,
335+
undefined,
336+
]) {
337+
value = supportedValue;
338+
t.update();
339+
}
340+
341+
expect(warnSpy).not.toHaveBeenCalled();
342+
});
343+
200344
it('should allow values even if a bypass operation is applied', () => {
201345
let backgroundImage: string | SafeValue = 'url("http://server")';
202346
const t = new ViewFixture({
@@ -244,6 +388,25 @@ describe('instructions', () => {
244388
fixture.update();
245389
expect(fixture.html).toEqual('<div style="background-color: red; height: 10px;"></div>');
246390
});
391+
392+
it('should warn when a style map property is bound to a boolean', () => {
393+
const warnSpy = spyOn(console, 'warn');
394+
const fixture = new ViewFixture({
395+
create: createDivWithStyle,
396+
update: () => ɵɵstyleMap({display: true}),
397+
decls: 1,
398+
vars: 2,
399+
consts: attrs,
400+
});
401+
402+
fixture.update();
403+
404+
expect(warnSpy).toHaveBeenCalledOnceWith(
405+
'NG0318: `[style.display]` was bound to an invalid value. ' +
406+
'Expected a string, number, SafeValue, null, or undefined, but received `boolean` (`true`). ' +
407+
'Find more at https://next.angular.dev/errors/NG0318',
408+
);
409+
});
247410
});
248411

249412
describe('elementClass', () => {

0 commit comments

Comments
 (0)