Skip to content

fix(icon): remove svgSrc, only allow trusted urls #1933

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
Dec 21, 2016
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
7 changes: 1 addition & 6 deletions src/demo-app/icon/icon-demo.html
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,6 @@
These are some icons.
</p>

<p>
From URL:
<md-icon svgSrc="/icon/assets/search-icon.svg"></md-icon>
</p>

<p>
By name registered with MdIconProvider:
<md-icon svgIcon="thumb-up" class="green"></md-icon>
Expand Down Expand Up @@ -37,4 +32,4 @@
Custom icon font and CSS:
<md-icon fontSet="fontawesome" fontIcon="fa-birthday-cake"></md-icon>
</p>
</div>
</div>
9 changes: 6 additions & 3 deletions src/demo-app/icon/icon-demo.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {Component, ViewEncapsulation} from '@angular/core';
import {DomSanitizer} from '@angular/platform-browser';
import {MdIconRegistry} from '@angular/material';

@Component({
Expand All @@ -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');
}
}
35 changes: 22 additions & 13 deletions src/lib/icon/icon-registry.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -15,7 +17,7 @@ import 'rxjs/add/operator/catch';
/** Exception thrown when attempting to load an icon with a name that cannot be found. */
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}"`);
}
}

Expand All @@ -25,14 +27,14 @@ export class MdIconNameNotFoundError extends MdError {
*/
export class MdIconSvgTagNotFoundError extends MdError {
constructor() {
super('<svg> tag not found');
super('<svg> tag not found');
}
}

/** Configuration for an icon, including the URL and possibly the cached SVG element. */
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. */
Expand Down Expand Up @@ -74,27 +76,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);
Expand Down Expand Up @@ -145,7 +147,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<SVGElement> {
getSvgIconFromUrl(safeUrl: SafeResourceUrl): Observable<SVGElement> {
let url = this._sanitizer.sanitize(SecurityContext.RESOURCE_URL, safeUrl);

if (this._cachedIconsByUrl.has(url)) {
return Observable.of(cloneSvg(this._cachedIconsByUrl.get(url)));
}
Expand Down Expand Up @@ -214,9 +218,12 @@ export class MdIconRegistry {
.map(iconSetConfig =>
this._loadSvgIconSetFromConfig(iconSetConfig)
.catch((err: any, caught: Observable<SVGElement>): Observable<SVGElement> => {
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 => {
Expand Down Expand Up @@ -273,7 +280,7 @@ export class MdIconRegistry {
private _loadSvgIconSetFromConfig(config: SvgIconConfig): Observable<SVGElement> {
// 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));
}

/**
Expand Down Expand Up @@ -346,7 +353,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<string> {
private _fetchUrl(safeUrl: SafeResourceUrl): Observable<string> {
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.
Expand Down
Loading