From e8768acacc84deb5fab8e6a66d019c48ac11da58 Mon Sep 17 00:00:00 2001 From: Jason Aden Date: Thu, 28 Mar 2019 15:00:51 -0700 Subject: [PATCH] fix(common): escape query selector used when anchor scrolling (#29577) When an anchor scroll happens, we run document.querySelector. This value can be taken directly from the user. Therefore it's possible to throw an error on scrolling, which can cause the application to fail. This PR escapes the selector before using it. Related to #28193 [Internal discussion](https://groups.google.com/a/google.com/forum/#!topic/angular-users/d82GHfmRKLc) PR Close #29577 --- packages/common/src/viewport_scroller.ts | 39 ++++++++++++------ .../common/test/viewport_scroller_spec.ts | 40 +++++++++++++++++++ 2 files changed, 67 insertions(+), 12 deletions(-) create mode 100644 packages/common/test/viewport_scroller_spec.ts diff --git a/packages/common/src/viewport_scroller.ts b/packages/common/src/viewport_scroller.ts index d4b8b5f128..d1b7d400fd 100644 --- a/packages/common/src/viewport_scroller.ts +++ b/packages/common/src/viewport_scroller.ts @@ -6,10 +6,12 @@ * found in the LICENSE file at https://angular.io/license */ -import {defineInjectable, inject} from '@angular/core'; +import {ErrorHandler, defineInjectable, inject} from '@angular/core'; import {DOCUMENT} from './dom_tokens'; + + /** * Defines a scroll position manager. Implemented by `BrowserViewportScroller`. * @@ -19,8 +21,10 @@ export abstract class ViewportScroller { // De-sugared tree-shakable injection // See #23917 /** @nocollapse */ - static ngInjectableDef = defineInjectable( - {providedIn: 'root', factory: () => new BrowserViewportScroller(inject(DOCUMENT), window)}); + static ngInjectableDef = defineInjectable({ + providedIn: 'root', + factory: () => new BrowserViewportScroller(inject(DOCUMENT), window, inject(ErrorHandler)) + }); /** * Configures the top offset used when scrolling to an anchor. @@ -62,7 +66,7 @@ export abstract class ViewportScroller { export class BrowserViewportScroller implements ViewportScroller { private offset: () => [number, number] = () => [0, 0]; - constructor(private document: any, private window: any) {} + constructor(private document: any, private window: any, private errorHandler: ErrorHandler) {} /** * Configures the top offset used when scrolling to an anchor. @@ -106,15 +110,26 @@ export class BrowserViewportScroller implements ViewportScroller { */ scrollToAnchor(anchor: string): void { if (this.supportScrollRestoration()) { - const elSelectedById = this.document.querySelector(`#${anchor}`); - if (elSelectedById) { - this.scrollToElement(elSelectedById); - return; + // Escape anything passed to `querySelector` as it can throw errors and stop the application + // from working if invalid values are passed. + if (this.window.CSS && this.window.CSS.escape) { + anchor = this.window.CSS.escape(anchor); + } else { + anchor = anchor.replace(/(\"|\'\ |:|\.|\[|\]|,|=)/g, '\\$1'); } - const elSelectedByName = this.document.querySelector(`[name='${anchor}']`); - if (elSelectedByName) { - this.scrollToElement(elSelectedByName); - return; + try { + const elSelectedById = this.document.querySelector(`#${anchor}`); + if (elSelectedById) { + this.scrollToElement(elSelectedById); + return; + } + const elSelectedByName = this.document.querySelector(`[name='${anchor}']`); + if (elSelectedByName) { + this.scrollToElement(elSelectedByName); + return; + } + } catch (e) { + this.errorHandler.handleError(e); } } } diff --git a/packages/common/test/viewport_scroller_spec.ts b/packages/common/test/viewport_scroller_spec.ts new file mode 100644 index 0000000000..281ef96d37 --- /dev/null +++ b/packages/common/test/viewport_scroller_spec.ts @@ -0,0 +1,40 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + + + +/** +* @license +* Copyright Google Inc. All Rights Reserved. +* +* Use of this source code is governed by an MIT-style license that can be +* found in the LICENSE file at https://angular.io/license +*/ + +import {describe, expect, it} from '@angular/core/testing/src/testing_internal'; +import {BrowserViewportScroller, ViewportScroller} from '../src/viewport_scroller'; + +{ + describe('BrowserViewportScroller', () => { + let scroller: ViewportScroller; + let documentSpy: any; + beforeEach(() => { + documentSpy = jasmine.createSpyObj('document', ['querySelector']); + scroller = new BrowserViewportScroller(documentSpy, {scrollTo: 1}, null !); + }); + it('escapes invalid characters selectors', () => { + const invalidSelectorChars = `"' :.[],=`; + // Double escaped to make sure we match the actual value passed to `querySelector` + const escapedInvalids = `\\"\\' \\:\\.\\[\\]\\,\\=`; + scroller.scrollToAnchor(`specials=${invalidSelectorChars}`); + expect(documentSpy.querySelector).toHaveBeenCalledWith(`#specials\\=${escapedInvalids}`); + expect(documentSpy.querySelector) + .toHaveBeenCalledWith(`[name='specials\\=${escapedInvalids}']`); + }); + }); +}