Skip to content

virtual-scroll: Avoid using bypassSecurityTrustStyle which is banned in google3 #12181

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

Merged
merged 2 commits into from
Jul 17, 2018
Merged
Show file tree
Hide file tree
Changes from all 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
3 changes: 1 addition & 2 deletions src/cdk-experimental/scrolling/virtual-scroll-viewport.html
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@
Wrap the rendered content in an element that will be used to offset it based on the scroll
position.
-->
<div #contentWrapper class="cdk-virtual-scroll-content-wrapper"
[style.transform]="_renderedContentTransform">
<div #contentWrapper class="cdk-virtual-scroll-content-wrapper">
<ng-content></ng-content>
</div>
<!--
Expand Down
12 changes: 0 additions & 12 deletions src/cdk-experimental/scrolling/virtual-scroll-viewport.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,6 @@ describe('CdkVirtualScrollViewport', () => {
viewport = testComponent.viewport;
});

it('should sanitize transform inputs', fakeAsync(() => {
finishInit(fixture);
viewport.orientation = 'arbitrary string as orientation' as any;
viewport.setRenderedContentOffset(
'arbitrary string as offset' as any, 'arbitrary string as to' as any);
fixture.detectChanges();
flush();

expect((viewport._renderedContentTransform as any).changingThisBreaksApplicationSecurity)
.toBe('translateY(NaNpx)');
}));

it('should render initial state', fakeAsync(() => {
finishInit(fixture);

Expand Down
29 changes: 15 additions & 14 deletions src/cdk-experimental/scrolling/virtual-scroll-viewport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import {
ViewChild,
ViewEncapsulation,
} from '@angular/core';
import {DomSanitizer, SafeStyle} from '@angular/platform-browser';
import {animationFrameScheduler, fromEvent, Observable, Subject} from 'rxjs';
import {sampleTime, take, takeUntil} from 'rxjs/operators';
import {CdkVirtualForOf} from './virtual-for-of';
Expand Down Expand Up @@ -68,11 +67,8 @@ export class CdkVirtualScrollViewport implements OnInit, OnDestroy {
*/
_totalContentSize = 0;

/** The transform used to offset the rendered content wrapper element. */
_renderedContentTransform: SafeStyle;

/** The raw string version of the rendered content transform. */
private _rawRenderedContentTransform: string;
/** The the rendered content transform. */
private _renderedContentTransform: string;

/** The currently rendered range of indices. */
private _renderedRange: ListRange = {start: 0, end: 0};
Expand Down Expand Up @@ -107,8 +103,9 @@ export class CdkVirtualScrollViewport implements OnInit, OnDestroy {
/** A list of functions to run after the next change detection cycle. */
private _runAfterChangeDetection: Function[] = [];

constructor(public elementRef: ElementRef, private _changeDetectorRef: ChangeDetectorRef,
private _ngZone: NgZone, private _sanitizer: DomSanitizer,
constructor(public elementRef: ElementRef,
private _changeDetectorRef: ChangeDetectorRef,
private _ngZone: NgZone,
@Inject(VIRTUAL_SCROLL_STRATEGY) private _scrollStrategy: VirtualScrollStrategy) {}

ngOnInit() {
Expand Down Expand Up @@ -223,17 +220,16 @@ export class CdkVirtualScrollViewport implements OnInit, OnDestroy {
let transform = `translate${axis}(${Number(offset)}px)`;
this._renderedContentOffset = offset;
if (to === 'to-end') {
// TODO(mmalerba): The viewport should rewrite this as a `to-start` offset on the next render
// cycle. Otherwise elements will appear to expand in the wrong direction (e.g.
// `mat-expansion-panel` would expand upward).
transform += ` translate${axis}(-100%)`;
// The viewport should rewrite this as a `to-start` offset on the next render cycle. Otherwise
// elements will appear to expand in the wrong direction (e.g. `mat-expansion-panel` would
// expand upward).
this._renderedContentOffsetNeedsRewrite = true;
}
if (this._rawRenderedContentTransform != transform) {
if (this._renderedContentTransform != transform) {
// We know this value is safe because we parse `offset` with `Number()` before passing it
// into the string.
this._rawRenderedContentTransform = transform;
this._renderedContentTransform = this._sanitizer.bypassSecurityTrustStyle(transform);
this._renderedContentTransform = transform;
this._markChangeDetectionNeeded(() => {
if (this._renderedContentOffsetNeedsRewrite) {
this._renderedContentOffset -= this.measureRenderedContentSize();
Expand Down Expand Up @@ -317,6 +313,11 @@ export class CdkVirtualScrollViewport implements OnInit, OnDestroy {

// Apply changes to Angular bindings.
this._ngZone.run(() => this._changeDetectorRef.detectChanges());
// Apply the content transform. The transform can't be set via an Angular binding because
// bypassSecurityTrustStyle is banned in Google. However the value is safe, it's composed of
// string literals, a variable that can only be 'X' or 'Y', and user input that is run through
// the `Number` function first to coerce it to a numeric value.
this._contentWrapper.nativeElement.style.transform = this._renderedContentTransform;
// Apply the pending scroll offset separately, since it can't be set up as an Angular binding.
if (this._pendingScrollOffset != null) {
if (this.orientation === 'horizontal') {
Expand Down