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

Commit

Permalink
fix($compile): properly sanitize xlink:href attribute interoplation
Browse files Browse the repository at this point in the history
Closes #12524
  • Loading branch information
IgorMinar authored and petebacondarwin committed Sep 18, 2015
1 parent 181fc56 commit f33ce17
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 1 deletion.
2 changes: 1 addition & 1 deletion src/ng/compile.js
Original file line number Diff line number Diff line change
Expand Up @@ -1124,7 +1124,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

nodeName = nodeName_(this.$$element);

if ((nodeName === 'a' && key === 'href') ||
if ((nodeName === 'a' && (key === 'href' || key === 'xlinkHref')) ||
(nodeName === 'img' && key === 'src')) {
// sanitize a[href] and img[src] values
this[key] = value = $$sanitizeUri(value, key === 'src');
Expand Down
48 changes: 48 additions & 0 deletions test/ng/compileSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -7215,6 +7215,54 @@ describe('$compile', function() {
});
});

it('should use $$sanitizeUri when declared via ng-href', function() {
var $$sanitizeUri = jasmine.createSpy('$$sanitizeUri');
module(function($provide) {
$provide.value('$$sanitizeUri', $$sanitizeUri);
});
inject(function($compile, $rootScope) {
element = $compile('<a ng-href="{{testUrl}}"></a>')($rootScope);
$rootScope.testUrl = "someUrl";

$$sanitizeUri.andReturn('someSanitizedUrl');
$rootScope.$apply();
expect(element.attr('href')).toBe('someSanitizedUrl');
expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl, false);
});
});

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) {
element = $compile('<svg><a xlink:href="" ng-href="{{ testUrl }}"></a></svg>')($rootScope);
$rootScope.testUrl = "evilUrl";

$$sanitizeUri.andReturn('someSanitizedUrl');
$rootScope.$apply();
expect(element.find('a').prop('href').baseVal).toBe('someSanitizedUrl');
expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl, false);
});
});


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) {
element = $compile('<svg><a xlink:href="" ng-href="{{ testUrl }}"></a></svg>')($rootScope);
$rootScope.testUrl = "evilUrl";

$$sanitizeUri.andReturn('someSanitizedUrl');
$rootScope.$apply();
expect(element.find('a').prop('href').baseVal).toBe('someSanitizedUrl');
expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl, false);
});
});
});

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

6 comments on commit f33ce17

@markzolotoy
Copy link

Choose a reason for hiding this comment

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

So, how exactly do I update my local copy of angular.min.js with this fix? I am running 1.4.7.

@gkalpak
Copy link
Member

@gkalpak gkalpak commented on f33ce17 Nov 7, 2018

Choose a reason for hiding this comment

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

This fix was introduced in v1.5.0-beta.1, so you would have to upgrade.
(Since this seems to be a trivial change, you could possibly manually patch your local copy, but this approach is more error prone and less future proof, so I wouldn't recommend it.)

@markzolotoy
Copy link

Choose a reason for hiding this comment

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

Yeah, but I cannot upgrade to 1.5 at this moment. Or can I? we are still using 1.4.7. Will the upgrade have any breaking changes?

@gkalpak
Copy link
Member

@gkalpak gkalpak commented on f33ce17 Nov 7, 2018

Choose a reason for hiding this comment

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

Yes, there are breaking changes. You can find more info in the changelog and the Migration Guide.

@markzolotoy
Copy link

Choose a reason for hiding this comment

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

I will definitely take a look but what's the bottom line? Will I have to change any code?

@gkalpak
Copy link
Member

@gkalpak gkalpak commented on f33ce17 Nov 8, 2018

Choose a reason for hiding this comment

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

Depends on your app. If you are using features that where affected by breaking changes, then you will need to change code.

Please sign in to comment.