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

Commit 0b43909

Browse files
committed
feat($compile): Lower the security context of SVG's a and image xlink:href attributes.
They should go through regular sanitization, RESOURCE_URL is overkill there. This does not change the context for the rest of xlink:href attributes. This is a breaking change in very unlikely cases. If someone whitelisted RESOURCE_URL for the purpose of binding into xlink:href, and that these can't pass the regular URL sanitization, they'll get broken: the fix is to whitelist in the $compileProvider's URL sanitization.
1 parent b7ee5ee commit 0b43909

File tree

2 files changed

+41
-7
lines changed

2 files changed

+41
-7
lines changed

src/ng/compile.js

+6-3
Original file line numberDiff line numberDiff line change
@@ -1671,7 +1671,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
16711671
nodeName = nodeName_(this.$$element);
16721672

16731673
if ((nodeName === 'a' && (key === 'href' || key === 'xlinkHref')) ||
1674-
(nodeName === 'img' && key === 'src')) {
1674+
(nodeName === 'img' && key === 'src') ||
1675+
(nodeName === 'image' && key === 'xlinkHref')) {
16751676
// sanitize a[href] and img[src] values
16761677
this[key] = value = $$sanitizeUri(value, key === 'src');
16771678
} else if (nodeName === 'img' && key === 'srcset' && isDefined(value)) {
@@ -3254,8 +3255,10 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
32543255
if (['img', 'video', 'audio', 'source', 'track'].indexOf(tag) === -1) {
32553256
return $sce.RESOURCE_URL;
32563257
}
3257-
// maction[xlink:href] can source SVG. It's not limited to <maction>.
3258-
} else if (attrNormalizedName === 'xlinkHref' ||
3258+
} else if (
3259+
// Some xlink:href are okay, most aren't
3260+
(attrNormalizedName === 'xlinkHref' && (tag !== 'image' && tag !== 'a')) ||
3261+
// Formaction
32593262
(tag === 'form' && attrNormalizedName === 'action') ||
32603263
// If relative URLs can go where they are not expected to, then
32613264
// all sorts of trust issues can arise.

test/ng/compileSpec.js

+35-4
Original file line numberDiff line numberDiff line change
@@ -11122,23 +11122,42 @@ describe('$compile', function() {
1112211122
});
1112311123

1112411124
it('should use $$sanitizeUri when working with svg and xlink:href', function() {
11125+
var $$sanitizeUri = jasmine.createSpy('$$sanitizeUri');
11126+
module(function($provide) {
11127+
$provide.value('$$sanitizeUri', $$sanitizeUri);
11128+
});
11129+
inject(function($compile, $rootScope) {
11130+
element = $compile('<svg><a xlink:href="{{ testUrl }}"></a></svg>')($rootScope);
11131+
11132+
//both of these fail the RESOURCE_URL test, that shouldn't be run
11133+
$rootScope.testUrl = 'https://bad.example.org';
11134+
$$sanitizeUri.and.returnValue('https://clean.example.org');
11135+
11136+
$rootScope.$apply();
11137+
expect(element.find('a').attr('xlink:href')).toBe('https://clean.example.org');
11138+
expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl, false);
11139+
});
11140+
});
11141+
11142+
it('should use $$sanitizeUri when working with svg and xlink:href through ng-href', function() {
1112511143
var $$sanitizeUri = jasmine.createSpy('$$sanitizeUri');
1112611144
module(function($provide) {
1112711145
$provide.value('$$sanitizeUri', $$sanitizeUri);
1112811146
});
1112911147
inject(function($compile, $rootScope) {
1113011148
element = $compile('<svg><a xlink:href="" ng-href="{{ testUrl }}"></a></svg>')($rootScope);
11131-
$rootScope.testUrl = 'evilUrl';
11149+
//both of these fail the RESOURCE_URL test, that shouldn't be run
11150+
$rootScope.testUrl = 'https://bad.example.org';
11151+
$$sanitizeUri.and.returnValue('https://clean.example.org');
1113211152

11133-
$$sanitizeUri.and.returnValue('someSanitizedUrl');
1113411153
$rootScope.$apply();
11135-
expect(element.find('a').prop('href').baseVal).toBe('someSanitizedUrl');
11154+
expect(element.find('a').prop('href').baseVal).toBe('https://clean.example.org');
1113611155
expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl, false);
1113711156
});
1113811157
});
1113911158

1114011159

11141-
it('should use $$sanitizeUri when working with svg and xlink:href', function() {
11160+
it('should use $$sanitizeUri when working with svg and xlink:href through ng-href', function() {
1114211161
var $$sanitizeUri = jasmine.createSpy('$$sanitizeUri');
1114311162
module(function($provide) {
1114411163
$provide.value('$$sanitizeUri', $$sanitizeUri);
@@ -11153,6 +11172,18 @@ describe('$compile', function() {
1115311172
expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl, false);
1115411173
});
1115511174
});
11175+
11176+
11177+
it('should have a RESOURCE_URL context for xlink:href by default', function() {
11178+
inject(function($compile, $rootScope) {
11179+
element = $compile('<svg><whatever xlink:href="{{ testUrl }}"></whatever></svg>')($rootScope);
11180+
$rootScope.testUrl = 'https://bad.example.org';
11181+
11182+
expect(function() {
11183+
$rootScope.$apply();
11184+
}).toThrowError(/\$sce:insecurl/);
11185+
});
11186+
});
1115611187
});
1115711188

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

0 commit comments

Comments
 (0)