fix(ivy): ensure sanitizer is not used when direct class application occurs (#33154)

Prior to this patch, if a map-class binding is applied directly then
that value will be incorrectly provided a sanitizer even if there is no
sanitization present for an element.

PR Close #33154
This commit is contained in:
Matias Niemelä 2019-10-14 13:45:51 -07:00 committed by Miško Hevery
parent a86893c10f
commit 1cda80eb3a
5 changed files with 86 additions and 7 deletions

View File

@ -13,7 +13,7 @@ import {RElement} from '../interfaces/renderer';
import {StylingMapArray, StylingMapArrayIndex, TStylingConfig, TStylingContext} from '../interfaces/styling';
import {isDirectiveHost} from '../interfaces/type_checks';
import {BINDING_INDEX, LView, RENDERER} from '../interfaces/view';
import {getActiveDirectiveId, getCurrentStyleSanitizer, getLView, getSelectedIndex, setCurrentStyleSanitizer, setElementExitFn} from '../state';
import {getActiveDirectiveId, getCurrentStyleSanitizer, getLView, getSelectedIndex, resetCurrentStyleSanitizer, setCurrentStyleSanitizer, setElementExitFn} from '../state';
import {applyStylingMapDirectly, applyStylingValueDirectly, flushStyling, setClass, setStyle, updateClassViaContext, updateStyleViaContext} from '../styling/bindings';
import {activateStylingMapFeature} from '../styling/map_based_bindings';
import {attachStylingDebugObject} from '../styling/styling_debug';
@ -174,10 +174,18 @@ function stylingProp(
// Direct Apply Case: bypass context resolution and apply the
// style/class value directly to the element
if (allowDirectStyling(context, hostBindingsMode)) {
const sanitizerToUse = isClassBased ? null : sanitizer;
const renderer = getRenderer(tNode, lView);
updated = applyStylingValueDirectly(
renderer, context, native, lView, bindingIndex, prop, value, isClassBased,
isClassBased ? setClass : setStyle, sanitizer);
isClassBased ? setClass : setStyle, sanitizerToUse);
if (sanitizerToUse) {
// it's important we remove the current style sanitizer once the
// element exits, otherwise it will be used by the next styling
// instructions for the next element.
setElementExitFn(resetCurrentStyleSanitizer);
}
} else {
// Context Resolution (or first update) Case: save the value
// and defer to the context to flush and apply the style/class binding
@ -337,10 +345,17 @@ function _stylingMap(
// Direct Apply Case: bypass context resolution and apply the
// style/class map values directly to the element
if (allowDirectStyling(context, hostBindingsMode)) {
const sanitizerToUse = isClassBased ? null : sanitizer;
const renderer = getRenderer(tNode, lView);
updated = applyStylingMapDirectly(
renderer, context, native, lView, bindingIndex, stylingMapArr as StylingMapArray,
isClassBased, isClassBased ? setClass : setStyle, sanitizer, valueHasChanged);
isClassBased, isClassBased ? setClass : setStyle, sanitizerToUse, valueHasChanged);
if (sanitizerToUse) {
// it's important we remove the current style sanitizer once the
// element exits, otherwise it will be used by the next styling
// instructions for the next element.
setElementExitFn(resetCurrentStyleSanitizer);
}
} else {
updated = valueHasChanged;
activateStylingMapFeature();
@ -442,7 +457,7 @@ function stylingApply(): void {
const classesContext = isStylingContext(tNode.classes) ? tNode.classes as TStylingContext : null;
const stylesContext = isStylingContext(tNode.styles) ? tNode.styles as TStylingContext : null;
flushStyling(renderer, lView, classesContext, stylesContext, native, directiveIndex, sanitizer);
setCurrentStyleSanitizer(null);
resetCurrentStyleSanitizer();
}
function getRenderer(tNode: TNode, lView: LView) {

View File

@ -500,6 +500,10 @@ export function setCurrentStyleSanitizer(sanitizer: StyleSanitizeFn | null) {
_currentSanitizer = sanitizer;
}
export function resetCurrentStyleSanitizer() {
setCurrentStyleSanitizer(null);
}
export function getCurrentStyleSanitizer() {
return _currentSanitizer;
}

View File

@ -756,7 +756,7 @@ function applyStylingValue(
let valueToApply: string|null = unwrapSafeValue(value);
if (isStylingValueDefined(valueToApply)) {
valueToApply =
sanitizer ? sanitizer(prop, value, StyleSanitizeMode.SanitizeOnly) : valueToApply;
sanitizer ? sanitizer(prop, value, StyleSanitizeMode.ValidateAndSanitize) : valueToApply;
applyFn(renderer, element, prop, valueToApply, bindingIndex);
return true;
}
@ -771,8 +771,9 @@ function findAndApplyMapValue(
const p = getMapProp(map, i);
if (p === prop) {
let valueToApply = getMapValue(map, i);
valueToApply =
sanitizer ? sanitizer(prop, valueToApply, StyleSanitizeMode.SanitizeOnly) : valueToApply;
valueToApply = sanitizer ?
sanitizer(prop, valueToApply, StyleSanitizeMode.ValidateAndSanitize) :
valueToApply;
applyFn(renderer, element, prop, valueToApply, bindingIndex);
return true;
}

View File

@ -2117,6 +2117,62 @@ describe('styling', () => {
expect(div.nativeElement.style['width']).toEqual('200px');
}
});
it('should not set classes when falsy value is passed while a sanitizer is present', () => {
@Component({
// Note that we use `background` here because it needs to be sanitized.
template: `
<span class="container" [ngClass]="{disabled: isDisabled}"></span>
<div [style.background]="background"></div>
`,
})
class AppComponent {
isDisabled = false;
background = 'orange';
}
TestBed.configureTestingModule({declarations: [AppComponent]});
const fixture = TestBed.createComponent(AppComponent);
fixture.detectChanges();
const span = fixture.nativeElement.querySelector('span');
expect(span.classList).not.toContain('disabled');
// The issue we're testing for happens after the second change detection.
fixture.detectChanges();
expect(span.classList).not.toContain('disabled');
});
it('should not set classes when falsy value is passed while a sanitizer from host bindings is present',
() => {
@Directive({selector: '[blockStyles]'})
class StylesDirective {
@HostBinding('style.border')
border = '1px solid red';
@HostBinding('style.background')
background = 'white';
}
@Component({
template: `<div class="container" [ngClass]="{disabled: isDisabled}" blockStyles></div>`,
})
class AppComponent {
isDisabled = false;
}
TestBed.configureTestingModule({declarations: [AppComponent, StylesDirective]});
const fixture = TestBed.createComponent(AppComponent);
fixture.detectChanges();
const div = fixture.nativeElement.querySelector('div');
expect(div.classList.contains('disabled')).toBe(false);
// The issue we're testing for happens after the second change detection.
fixture.detectChanges();
expect(div.classList.contains('disabled')).toBe(false);
});
});
function assertStyleCounters(countForSet: number, countForRemove: number) {

View File

@ -1154,6 +1154,9 @@
{
"name": "resetComponentState"
},
{
"name": "resetCurrentStyleSanitizer"
},
{
"name": "resetPreOrderHookFlags"
},