From 4571561dd51b350bbe58e3af6c17ef0e53cd3dd7 Mon Sep 17 00:00:00 2001 From: Jeremy Elbourn Date: Tue, 20 Dec 2016 22:05:19 -0800 Subject: [PATCH] fix(icon): remove svgSrc, only allow trusted urls (#1933) * fix(icon): remove svgSrc, only allow trusted urls * rxjs --- src/demo-app/icon/icon-demo.html | 7 +- src/demo-app/icon/icon-demo.ts | 9 +- src/lib/icon/icon-registry.ts | 35 +++--- src/lib/icon/icon.spec.ts | 177 +++++++++++-------------------- src/lib/icon/icon.ts | 7 +- tools/gulp/tasks/components.ts | 1 + 6 files changed, 90 insertions(+), 146 deletions(-) diff --git a/src/demo-app/icon/icon-demo.html b/src/demo-app/icon/icon-demo.html index 9cf76fd440e8..338c4717ff9f 100644 --- a/src/demo-app/icon/icon-demo.html +++ b/src/demo-app/icon/icon-demo.html @@ -3,11 +3,6 @@ These are some icons.

-

- From URL: - -

-

By name registered with MdIconProvider: @@ -37,4 +32,4 @@ Custom icon font and CSS:

- \ No newline at end of file + diff --git a/src/demo-app/icon/icon-demo.ts b/src/demo-app/icon/icon-demo.ts index 8f6a1dc9f9b2..fac610e52c41 100644 --- a/src/demo-app/icon/icon-demo.ts +++ b/src/demo-app/icon/icon-demo.ts @@ -1,4 +1,5 @@ import {Component, ViewEncapsulation} from '@angular/core'; +import {DomSanitizer} from '@angular/platform-browser'; import {MdIconRegistry} from '@angular/material'; @Component({ @@ -10,10 +11,12 @@ import {MdIconRegistry} from '@angular/material'; encapsulation: ViewEncapsulation.None, }) export class IconDemo { - constructor(mdIconRegistry: MdIconRegistry) { + constructor(mdIconRegistry: MdIconRegistry, sanitizer: DomSanitizer) { mdIconRegistry - .addSvgIcon('thumb-up', '/icon/assets/thumbup-icon.svg') - .addSvgIconSetInNamespace('core', '/icon/assets/core-icon-set.svg') + .addSvgIcon('thumb-up', + sanitizer.bypassSecurityTrustResourceUrl('/icon/assets/thumbup-icon.svg')) + .addSvgIconSetInNamespace('core', + sanitizer.bypassSecurityTrustResourceUrl('/icon/assets/core-icon-set.svg')) .registerFontClassAlias('fontawesome', 'fa'); } } diff --git a/src/lib/icon/icon-registry.ts b/src/lib/icon/icon-registry.ts index dcabdabcd3d7..1c9e449ea814 100644 --- a/src/lib/icon/icon-registry.ts +++ b/src/lib/icon/icon-registry.ts @@ -1,9 +1,11 @@ -import {Injectable} from '@angular/core'; +import {Injectable, SecurityContext} from '@angular/core'; +import {SafeResourceUrl, DomSanitizer} from '@angular/platform-browser'; import {Http} from '@angular/http'; import {MdError} from '../core'; import {Observable} from 'rxjs/Observable'; import 'rxjs/add/observable/forkJoin'; import 'rxjs/add/observable/of'; +import 'rxjs/add/observable/throw'; import 'rxjs/add/operator/map'; import 'rxjs/add/operator/filter'; import 'rxjs/add/operator/do'; @@ -18,7 +20,7 @@ import 'rxjs/add/operator/catch'; */ export class MdIconNameNotFoundError extends MdError { constructor(iconName: string) { - super(`Unable to find icon with the name "${iconName}"`); + super(`Unable to find icon with the name "${iconName}"`); } } @@ -29,7 +31,7 @@ export class MdIconNameNotFoundError extends MdError { */ export class MdIconSvgTagNotFoundError extends MdError { constructor() { - super(' tag not found'); + super(' tag not found'); } } @@ -39,7 +41,7 @@ export class MdIconSvgTagNotFoundError extends MdError { */ class SvgIconConfig { svgElement: SVGElement = null; - constructor(public url: string) { } + constructor(public url: SafeResourceUrl) { } } /** Returns the cache key to use for an icon namespace and name. */ @@ -81,27 +83,27 @@ export class MdIconRegistry { */ private _defaultFontSetClass = 'material-icons'; - constructor(private _http: Http) {} + constructor(private _http: Http, private _sanitizer: DomSanitizer) {} /** Registers an icon by URL in the default namespace. */ - addSvgIcon(iconName: string, url: string): this { + addSvgIcon(iconName: string, url: SafeResourceUrl): this { return this.addSvgIconInNamespace('', iconName, url); } /** Registers an icon by URL in the specified namespace. */ - addSvgIconInNamespace(namespace: string, iconName: string, url: string): this { + addSvgIconInNamespace(namespace: string, iconName: string, url: SafeResourceUrl): this { const key = iconKey(namespace, iconName); this._svgIconConfigs.set(key, new SvgIconConfig(url)); return this; } /** Registers an icon set by URL in the default namespace. */ - addSvgIconSet(url: string): this { + addSvgIconSet(url: SafeResourceUrl): this { return this.addSvgIconSetInNamespace('', url); } /** Registers an icon set by URL in the specified namespace. */ - addSvgIconSetInNamespace(namespace: string, url: string): this { + addSvgIconSetInNamespace(namespace: string, url: SafeResourceUrl): this { const config = new SvgIconConfig(url); if (this._iconSetConfigs.has(namespace)) { this._iconSetConfigs.get(namespace).push(config); @@ -152,7 +154,9 @@ export class MdIconRegistry { * the produced element will always be a new copy of the originally fetched icon. (That is, * it will not contain any modifications made to elements previously returned). */ - getSvgIconFromUrl(url: string): Observable { + getSvgIconFromUrl(safeUrl: SafeResourceUrl): Observable { + let url = this._sanitizer.sanitize(SecurityContext.RESOURCE_URL, safeUrl); + if (this._cachedIconsByUrl.has(url)) { return Observable.of(cloneSvg(this._cachedIconsByUrl.get(url))); } @@ -221,9 +225,12 @@ export class MdIconRegistry { .map(iconSetConfig => this._loadSvgIconSetFromConfig(iconSetConfig) .catch((err: any, caught: Observable): Observable => { + let url = + this._sanitizer.sanitize(SecurityContext.RESOURCE_URL, iconSetConfig.url); + // Swallow errors fetching individual URLs so the combined Observable won't // necessarily fail. - console.log(`Loading icon set URL: ${iconSetConfig.url} failed: ${err}`); + console.log(`Loading icon set URL: ${url} failed: ${err}`); return Observable.of(null); }) .do(svg => { @@ -280,7 +287,7 @@ export class MdIconRegistry { private _loadSvgIconSetFromConfig(config: SvgIconConfig): Observable { // TODO: Document that icons should only be loaded from trusted sources. return this._fetchUrl(config.url) - .map((svgText) => this._svgElementFromString(svgText)); + .map(svgText => this._svgElementFromString(svgText)); } /** @@ -353,7 +360,9 @@ export class MdIconRegistry { * Returns an Observable which produces the string contents of the given URL. Results may be * cached, so future calls with the same URL may not cause another HTTP request. */ - private _fetchUrl(url: string): Observable { + private _fetchUrl(safeUrl: SafeResourceUrl): Observable { + let url = this._sanitizer.sanitize(SecurityContext.RESOURCE_URL, safeUrl); + // Store in-progress fetches to avoid sending a duplicate request for a URL when there is // already a request in progress for that URL. It's necessary to call share() on the // Observable returned by http.get() so that multiple subscribers don't cause multiple XHRs. diff --git a/src/lib/icon/icon.spec.ts b/src/lib/icon/icon.spec.ts index 604770074555..202f6738eb2d 100644 --- a/src/lib/icon/icon.spec.ts +++ b/src/lib/icon/icon.spec.ts @@ -1,8 +1,5 @@ -import { - inject, - async, - TestBed, -} from '@angular/core/testing'; +import {inject, async, TestBed} from '@angular/core/testing'; +import {SafeResourceUrl, DomSanitizer} from '@angular/platform-browser'; import {XHRBackend} from '@angular/http'; import {MockBackend} from '@angular/http/testing'; import {Component} from '@angular/core'; @@ -45,7 +42,6 @@ describe('MdIcon', () => { MdIconLigatureTestApp, MdIconLigatureWithAriaBindingTestApp, MdIconCustomFontCssTestApp, - MdIconFromSvgUrlTestApp, MdIconFromSvgNameTestApp, ], providers: [ @@ -58,11 +54,13 @@ describe('MdIcon', () => { })); let mdIconRegistry: MdIconRegistry; + let sanitizer: DomSanitizer; let httpRequestUrls: string[]; - let deps = [MdIconRegistry, MockBackend]; - beforeEach(inject(deps, (mir: MdIconRegistry, mockBackend: MockBackend) => { + let deps = [MdIconRegistry, MockBackend, DomSanitizer]; + beforeEach(inject(deps, (mir: MdIconRegistry, mockBackend: MockBackend, ds: DomSanitizer) => { mdIconRegistry = mir; + sanitizer = ds; // Keep track of requests so we can verify caching behavior. // Return responses for the SVGs defined in fake-svgs.ts. httpRequestUrls = []; @@ -76,7 +74,7 @@ describe('MdIcon', () => { it('should apply class based on color attribute', () => { let fixture = TestBed.createComponent(MdIconColorTestApp); - const testComponent = fixture.debugElement.componentInstance; + const testComponent = fixture.componentInstance; const mdIconElement = fixture.debugElement.nativeElement.querySelector('md-icon'); testComponent.iconName = 'home'; testComponent.iconColor = 'primary'; @@ -88,7 +86,7 @@ describe('MdIcon', () => { it('should add material-icons class by default', () => { let fixture = TestBed.createComponent(MdIconLigatureTestApp); - const testComponent = fixture.debugElement.componentInstance; + const testComponent = fixture.componentInstance; const mdIconElement = fixture.debugElement.nativeElement.querySelector('md-icon'); testComponent.iconName = 'home'; fixture.detectChanges(); @@ -100,7 +98,7 @@ describe('MdIcon', () => { let fixture = TestBed.createComponent(MdIconLigatureTestApp); - const testComponent = fixture.debugElement.componentInstance; + const testComponent = fixture.componentInstance; const mdIconElement = fixture.debugElement.nativeElement.querySelector('md-icon'); testComponent.iconName = 'home'; fixture.detectChanges(); @@ -109,44 +107,12 @@ describe('MdIcon', () => { }); describe('Icons from URLs', () => { - it('should fetch SVG icon from URL and inline the content', () => { - let fixture = TestBed.createComponent(MdIconFromSvgUrlTestApp); - - const testComponent = fixture.debugElement.componentInstance; - const mdIconElement = fixture.debugElement.nativeElement.querySelector('md-icon'); - let svgElement: any; - - testComponent.iconUrl = 'cat.svg'; - fixture.detectChanges(); - // An element should have been added as a child of . - svgElement = verifyAndGetSingleSvgChild(mdIconElement); - // Default attributes should be set. - expect(svgElement.getAttribute('height')).toBe('100%'); - expect(svgElement.getAttribute('height')).toBe('100%'); - // Make sure SVG content is taken from response. - verifyPathChildElement(svgElement, 'meow'); - - // Change the icon, and the SVG element should be replaced. - testComponent.iconUrl = 'dog.svg'; - fixture.detectChanges(); - svgElement = verifyAndGetSingleSvgChild(mdIconElement); - verifyPathChildElement(svgElement, 'woof'); - - expect(httpRequestUrls).toEqual(['cat.svg', 'dog.svg']); - // Using an icon from a previously loaded URL should not cause another HTTP request. - testComponent.iconUrl = 'cat.svg'; - fixture.detectChanges(); - svgElement = verifyAndGetSingleSvgChild(mdIconElement); - verifyPathChildElement(svgElement, 'meow'); - expect(httpRequestUrls).toEqual(['cat.svg', 'dog.svg']); - }); - it('should register icon URLs by name', () => { - mdIconRegistry.addSvgIcon('fluffy', 'cat.svg'); - mdIconRegistry.addSvgIcon('fido', 'dog.svg'); + mdIconRegistry.addSvgIcon('fluffy', trust('cat.svg')); + mdIconRegistry.addSvgIcon('fido', trust('dog.svg')); let fixture = TestBed.createComponent(MdIconFromSvgNameTestApp); - const testComponent = fixture.debugElement.componentInstance; + const testComponent = fixture.componentInstance; const mdIconElement = fixture.debugElement.nativeElement.querySelector('md-icon'); let svgElement: SVGElement; @@ -173,12 +139,32 @@ describe('MdIcon', () => { expect(httpRequestUrls).toEqual(['dog.svg', 'cat.svg']); }); - it('should extract icon from SVG icon set', () => { + it('should throw an error when using an untrusted icon url', () => { + mdIconRegistry.addSvgIcon('fluffy', 'farm-set-1.svg'); + + expect(() => { + let fixture = TestBed.createComponent(MdIconFromSvgNameTestApp); + fixture.componentInstance.iconName = 'fluffy'; + fixture.detectChanges(); + }).toThrowError(/unsafe value used in a resource URL context/); + }); + + it('should throw an error when using an untrusted icon set url', () => { mdIconRegistry.addSvgIconSetInNamespace('farm', 'farm-set-1.svg'); + expect(() => { + let fixture = TestBed.createComponent(MdIconFromSvgNameTestApp); + fixture.componentInstance.iconName = 'farm:pig'; + fixture.detectChanges(); + }).toThrowError(/unsafe value used in a resource URL context/); + }); + + it('should extract icon from SVG icon set', () => { + mdIconRegistry.addSvgIconSetInNamespace('farm', trust('farm-set-1.svg')); + let fixture = TestBed.createComponent(MdIconFromSvgNameTestApp); - const testComponent = fixture.debugElement.componentInstance; + const testComponent = fixture.componentInstance; const mdIconElement = fixture.debugElement.nativeElement.querySelector('md-icon'); let svgElement: any; let svgChild: any; @@ -210,13 +196,13 @@ describe('MdIcon', () => { }); it('should allow multiple icon sets in a namespace', () => { - mdIconRegistry.addSvgIconSetInNamespace('farm', 'farm-set-1.svg'); - mdIconRegistry.addSvgIconSetInNamespace('farm', 'farm-set-2.svg'); - mdIconRegistry.addSvgIconSetInNamespace('arrows', 'arrow-set.svg'); + mdIconRegistry.addSvgIconSetInNamespace('farm', trust('farm-set-1.svg')); + mdIconRegistry.addSvgIconSetInNamespace('farm', trust('farm-set-2.svg')); + mdIconRegistry.addSvgIconSetInNamespace('arrows', trust('arrow-set.svg')); let fixture = TestBed.createComponent(MdIconFromSvgNameTestApp); - const testComponent = fixture.debugElement.componentInstance; + const testComponent = fixture.componentInstance; const mdIconElement = fixture.debugElement.nativeElement.querySelector('md-icon'); let svgElement: any; let svgChild: any; @@ -254,11 +240,11 @@ describe('MdIcon', () => { }); it('should not wrap elements in icon sets in another svg tag', () => { - mdIconRegistry.addSvgIconSet('arrow-set.svg'); + mdIconRegistry.addSvgIconSet(trust('arrow-set.svg')); let fixture = TestBed.createComponent(MdIconFromSvgNameTestApp); - const testComponent = fixture.debugElement.componentInstance; + const testComponent = fixture.componentInstance; const mdIconElement = fixture.debugElement.nativeElement.querySelector('md-icon'); let svgElement: any; @@ -271,40 +257,12 @@ describe('MdIcon', () => { expect(mdIconElement.getAttribute('aria-label')).toBe('left-arrow'); }); - it('should return unmodified copies of icons from URLs', () => { - let fixture = TestBed.createComponent(MdIconFromSvgUrlTestApp); - - const testComponent = fixture.debugElement.componentInstance; - const mdIconElement = fixture.debugElement.nativeElement.querySelector('md-icon'); - let svgElement: any; - - testComponent.iconUrl = 'cat.svg'; - fixture.detectChanges(); - svgElement = verifyAndGetSingleSvgChild(mdIconElement); - verifyPathChildElement(svgElement, 'meow'); - // Modify the SVG element by setting a viewBox attribute. - svgElement.setAttribute('viewBox', '0 0 100 100'); - - // Switch to a different icon. - testComponent.iconUrl = 'dog.svg'; - fixture.detectChanges(); - svgElement = verifyAndGetSingleSvgChild(mdIconElement); - verifyPathChildElement(svgElement, 'woof'); - - // Switch back to the first icon. The viewBox attribute should not be present. - testComponent.iconUrl = 'cat.svg'; - fixture.detectChanges(); - svgElement = verifyAndGetSingleSvgChild(mdIconElement); - verifyPathChildElement(svgElement, 'meow'); - expect(svgElement.getAttribute('viewBox')).toBeFalsy(); - }); - it('should return unmodified copies of icons from icon sets', () => { - mdIconRegistry.addSvgIconSet('arrow-set.svg'); + mdIconRegistry.addSvgIconSet(trust('arrow-set.svg')); let fixture = TestBed.createComponent(MdIconFromSvgNameTestApp); - const testComponent = fixture.debugElement.componentInstance; + const testComponent = fixture.componentInstance; const mdIconElement = fixture.debugElement.nativeElement.querySelector('md-icon'); let svgElement: any; @@ -337,7 +295,7 @@ describe('MdIcon', () => { let fixture = TestBed.createComponent(MdIconCustomFontCssTestApp); - const testComponent = fixture.debugElement.componentInstance; + const testComponent = fixture.componentInstance; const mdIconElement = fixture.debugElement.nativeElement.querySelector('md-icon'); testComponent.fontSet = 'f1'; testComponent.fontIcon = 'house'; @@ -363,7 +321,7 @@ describe('MdIcon', () => { it('should set aria label from text content if not specified', () => { let fixture = TestBed.createComponent(MdIconLigatureTestApp); - const testComponent = fixture.debugElement.componentInstance; + const testComponent = fixture.componentInstance; const mdIconElement = fixture.debugElement.nativeElement.querySelector('md-icon'); testComponent.iconName = 'home'; @@ -378,7 +336,7 @@ describe('MdIcon', () => { it('should use alt tag if aria label is not specified', () => { let fixture = TestBed.createComponent(MdIconLigatureWithAriaBindingTestApp); - const testComponent = fixture.debugElement.componentInstance; + const testComponent = fixture.componentInstance; const mdIconElement = fixture.debugElement.nativeElement.querySelector('md-icon'); testComponent.iconName = 'home'; testComponent.altText = 'castle'; @@ -393,7 +351,7 @@ describe('MdIcon', () => { it('should use provided aria label rather than icon name', () => { let fixture = TestBed.createComponent(MdIconLigatureWithAriaBindingTestApp); - const testComponent = fixture.debugElement.componentInstance; + const testComponent = fixture.componentInstance; const mdIconElement = fixture.debugElement.nativeElement.querySelector('md-icon'); testComponent.iconName = 'home'; testComponent.ariaLabel = 'house'; @@ -404,7 +362,7 @@ describe('MdIcon', () => { it('should use provided aria label rather than font icon', () => { let fixture = TestBed.createComponent(MdIconCustomFontCssTestApp); - const testComponent = fixture.debugElement.componentInstance; + const testComponent = fixture.componentInstance; const mdIconElement = fixture.debugElement.nativeElement.querySelector('md-icon'); testComponent.fontSet = 'f1'; testComponent.fontIcon = 'house'; @@ -413,42 +371,37 @@ describe('MdIcon', () => { expect(mdIconElement.getAttribute('aria-label')).toBe('home'); }); }); + + /** Marks an svg icon url as explicitly trusted. */ + function trust(iconUrl: string): SafeResourceUrl { + return sanitizer.bypassSecurityTrustResourceUrl(iconUrl); + } }); + /** Test components that contain an MdIcon. */ -@Component({ - selector: 'test-app', - template: `{{iconName}}`, -}) +@Component({template: `{{iconName}}`}) class MdIconLigatureTestApp { ariaLabel: string = null; iconName = ''; } -@Component({ - selector: 'test-app', - template: `{{iconName}}`, -}) +@Component({template: `{{iconName}}`}) class MdIconColorTestApp { ariaLabel: string = null; iconName = ''; iconColor = 'primary'; } -@Component({ - selector: 'test-app', - template: `{{iconName}}`, -}) +@Component({template: `{{iconName}}`}) class MdIconLigatureWithAriaBindingTestApp { + altText: string = ''; ariaLabel: string = null; iconName = ''; } @Component({ - selector: 'test-app', - template: ` - - `, + template: `` }) class MdIconCustomFontCssTestApp { ariaLabel: string = null; @@ -456,19 +409,7 @@ class MdIconCustomFontCssTestApp { fontIcon = ''; } -@Component({ - selector: 'test-app', - template: ``, -}) -class MdIconFromSvgUrlTestApp { - ariaLabel: string = null; - iconUrl = ''; -} - -@Component({ - selector: 'test-app', - template: ``, -}) +@Component({template: ``}) class MdIconFromSvgNameTestApp { ariaLabel: string = null; iconName = ''; diff --git a/src/lib/icon/icon.ts b/src/lib/icon/icon.ts index 7a933a1c8284..39c2b4225850 100644 --- a/src/lib/icon/icon.ts +++ b/src/lib/icon/icon.ts @@ -71,7 +71,6 @@ export class MdIconInvalidNameError extends MdError { export class MdIcon implements OnChanges, OnInit, AfterViewChecked { private _color: string; - @Input() svgSrc: string; @Input() svgIcon: string; @Input() fontSet: string; @Input() fontIcon: string; @@ -146,10 +145,6 @@ export class MdIcon implements OnChanges, OnInit, AfterViewChecked { this._mdIconRegistry.getNamedSvgIcon(iconName, namespace).first().subscribe( svg => this._setSvgElement(svg), (err: any) => console.log(`Error retrieving icon: ${err}`)); - } else if (this.svgSrc) { - this._mdIconRegistry.getSvgIconFromUrl(this.svgSrc).first().subscribe( - svg => this._setSvgElement(svg), - (err: any) => console.log(`Error retrieving icon: ${err}`)); } } if (this._usingFontIcon()) { @@ -203,7 +198,7 @@ export class MdIcon implements OnChanges, OnInit, AfterViewChecked { } private _usingFontIcon(): boolean { - return !(this.svgIcon || this.svgSrc); + return !this.svgIcon; } private _setSvgElement(svg: SVGElement) { diff --git a/tools/gulp/tasks/components.ts b/tools/gulp/tasks/components.ts index 995fff674728..1cefa6cc8341 100644 --- a/tools/gulp/tasks/components.ts +++ b/tools/gulp/tasks/components.ts @@ -72,6 +72,7 @@ task(':build:components:rollup', () => { 'rxjs/add/observable/fromEvent': 'Rx.Observable', 'rxjs/add/observable/forkJoin': 'Rx.Observable', 'rxjs/add/observable/of': 'Rx.Observable', + 'rxjs/add/observable/throw': 'Rx.Observable', 'rxjs/add/operator/toPromise': 'Rx.Observable.prototype', 'rxjs/add/operator/map': 'Rx.Observable.prototype', 'rxjs/add/operator/filter': 'Rx.Observable.prototype',