Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Commit

Permalink
feat($compile): lower the xlink:href security context for SVG's a
Browse files Browse the repository at this point in the history
… and `image` elements

Previously, `xlink:href` on SVG's `<a>` and `<image>` elements, was
`$sce.RESOURCE_URL`. While this makes sense for other `xlink:href` usecases, it
was an overkill for these elements.
This commit lowers the `xlink:href` security context for these specific
elements, treating it in the same way as `a[href]` or `img[src]` respectively.
The `xlink:href` security context for other elements is not affected.

BREAKING CHANGE:

In the unlikely case that an app relied on RESOURCE_URL whitelisting for the
purpose of binding to the `xlink:href` property of SVG's `<a>` or `<image>`
elements and if the values do not pass the regular URL sanitization, they will
break.

To fix this you need to ensure that the values used for binding to the affected
`xlink:href` contexts are considered safe URLs, e.g. by whitelisting them in
`$compileProvider`'s `aHrefSanitizationWhitelist` (for `<a>` elements) or
`imgSrcSanitizationWhitelist` (for `<image>` elements).

Closes #15736
  • Loading branch information
rjamet authored and gkalpak committed Mar 4, 2017
1 parent cc793a1 commit 6ccbfa6
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 8 deletions.
11 changes: 7 additions & 4 deletions src/ng/compile.js
Expand Up @@ -1673,9 +1673,10 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
nodeName = nodeName_(this.$$element);

if ((nodeName === 'a' && (key === 'href' || key === 'xlinkHref')) ||
(nodeName === 'img' && key === 'src')) {
(nodeName === 'img' && key === 'src') ||
(nodeName === 'image' && key === 'xlinkHref')) {
// sanitize a[href] and img[src] values
this[key] = value = $$sanitizeUri(value, key === 'src');
this[key] = value = $$sanitizeUri(value, nodeName === 'img' || nodeName === 'image');
} else if (nodeName === 'img' && key === 'srcset' && isDefined(value)) {
// sanitize img[srcset] values
var result = '';
Expand Down Expand Up @@ -3256,8 +3257,10 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
if (['img', 'video', 'audio', 'source', 'track'].indexOf(tag) === -1) {
return $sce.RESOURCE_URL;
}
// maction[xlink:href] can source SVG. It's not limited to <maction>.
} else if (attrNormalizedName === 'xlinkHref' ||
} else if (
// Some xlink:href are okay, most aren't
(attrNormalizedName === 'xlinkHref' && (tag !== 'image' && tag !== 'a')) ||
// Formaction
(tag === 'form' && attrNormalizedName === 'action') ||
// If relative URLs can go where they are not expected to, then
// all sorts of trust issues can arise.
Expand Down
43 changes: 39 additions & 4 deletions test/ng/compileSpec.js
Expand Up @@ -11122,23 +11122,47 @@ describe('$compile', function() {
});

it('should use $$sanitizeUri when working with svg and xlink:href', function() {
var $$sanitizeUri = jasmine.createSpy('$$sanitizeUri');
module(function($provide) {
$provide.value('$$sanitizeUri', $$sanitizeUri);
});
inject(function($compile, $rootScope) {
var elementA = $compile('<svg><a xlink:href="{{ testUrl + \'aTag\' }}"></a></svg>')($rootScope);
var elementImage = $compile('<svg><image xlink:href="{{ testUrl + \'imageTag\' }}"></image></svg>')($rootScope);

//both of these fail the RESOURCE_URL test, that shouldn't be run
$rootScope.testUrl = 'https://bad.example.org';
$$sanitizeUri.and.returnValue('https://clean.example.org');

$rootScope.$apply();
expect(elementA.find('a').attr('xlink:href')).toBe('https://clean.example.org');
expect(elementImage.find('image').attr('xlink:href')).toBe('https://clean.example.org');
// <a> is navigational, so the second argument should be false to reach the aHref whitelist
expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl + 'aTag' , false);
// <image> is media inclusion, it should use the imgSrc whitelist
expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl + 'imageTag', true);
});
});

it('should use $$sanitizeUri when working with svg and xlink:href through ng-href', function() {
var $$sanitizeUri = jasmine.createSpy('$$sanitizeUri');
module(function($provide) {
$provide.value('$$sanitizeUri', $$sanitizeUri);
});
inject(function($compile, $rootScope) {
element = $compile('<svg><a xlink:href="" ng-href="{{ testUrl }}"></a></svg>')($rootScope);
$rootScope.testUrl = 'evilUrl';
//both of these fail the RESOURCE_URL test, that shouldn't be run
$rootScope.testUrl = 'https://bad.example.org';
$$sanitizeUri.and.returnValue('https://clean.example.org');

$$sanitizeUri.and.returnValue('someSanitizedUrl');
$rootScope.$apply();
expect(element.find('a').prop('href').baseVal).toBe('someSanitizedUrl');
expect(element.find('a').prop('href').baseVal).toBe('https://clean.example.org');
expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl, false);
});
});


it('should use $$sanitizeUri when working with svg and xlink:href', function() {
it('should use $$sanitizeUri when working with svg and xlink:href through ng-href', function() {
var $$sanitizeUri = jasmine.createSpy('$$sanitizeUri');
module(function($provide) {
$provide.value('$$sanitizeUri', $$sanitizeUri);
Expand All @@ -11153,6 +11177,17 @@ describe('$compile', function() {
expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl, false);
});
});

it('should have a RESOURCE_URL context for xlink:href by default', function() {
inject(function($compile, $rootScope) {
element = $compile('<svg><whatever xlink:href="{{ testUrl }}"></whatever></svg>')($rootScope);
$rootScope.testUrl = 'https://bad.example.org';

expect(function() {
$rootScope.$apply();
}).toThrowError(/\$sce:insecurl/);
});
});
});

describe('interpolation on HTML DOM event handler attributes onclick, onXYZ, formaction', function() {
Expand Down

1 comment on commit 6ccbfa6

@g-patel
Copy link

@g-patel g-patel commented on 6ccbfa6 Jan 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rjamet @gkalpak does this impact svg element?

Please sign in to comment.