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

Commit

Permalink
fix($compile): sanitize values bound to a[href]
Browse files Browse the repository at this point in the history
  • Loading branch information
@IgorMinar
IgorMinar committed Feb 20, 2013
1 parent 5f5d4fe commit 9532234
Show file tree
Hide file tree
Showing 3 changed files with 197 additions and 9 deletions.
60 changes: 53 additions & 7 deletions src/ng/compile.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,8 @@ function $CompileProvider($provide) {
Suffix = 'Directive',
COMMENT_DIRECTIVE_REGEXP = /^\s*directive\:\s*([\d\w\-_]+)\s+(.*)$/,
CLASS_DIRECTIVE_REGEXP = /(([\d\w\-_]+)(?:\:([^;]+))?;?)/,
MULTI_ROOT_TEMPLATE_ERROR = 'Template must have exactly one root element. was: ';
MULTI_ROOT_TEMPLATE_ERROR = 'Template must have exactly one root element. was: ',
urlSanitizationWhitelist = /^\s*(https?|ftp|mailto):/;


/**
Expand Down Expand Up @@ -209,11 +210,41 @@ function $CompileProvider($provide) {
};


/**
* @ngdoc function
* @name ng.$compileProvider#urlSanitizationWhitelist
* @methodOf ng.$compileProvider
* @function
*
* @description
* Retrieves or overrides the default regular expression that is used for whitelisting of safe
* urls during a[href] sanitization.
*
* The sanitization is a security measure aimed at prevent XSS attacks via html links.
*
* Any url about to be assigned to a[href] via data-binding is first normalized and turned into an
* absolute url. Afterwards the url is matched against the `urlSanitizationWhitelist` regular
* expression. If a match is found the original url is written into the dom. Otherwise the
* absolute url is prefixed with `'unsafe:'` string and only then it is written into the DOM.
*
* @param {RegExp=} regexp New regexp to whitelist urls with.
* @returns {RegExp|ng.$compileProvider} Current RegExp if called without value or self for
* chaining otherwise.
*/
this.urlSanitizationWhitelist = function(regexp) {
if (isDefined(regexp)) {
urlSanitizationWhitelist = regexp;
return this;
}
return urlSanitizationWhitelist;
};


this.$get = [
'$injector', '$interpolate', '$exceptionHandler', '$http', '$templateCache', '$parse',
'$controller', '$rootScope',
'$controller', '$rootScope', '$document',
function($injector, $interpolate, $exceptionHandler, $http, $templateCache, $parse,
$controller, $rootScope) {
$controller, $rootScope, $document) {

var Attributes = function(element, attr) {
this.$$element = element;
Expand All @@ -235,7 +266,8 @@ function $CompileProvider($provide) {
*/
$set: function(key, value, writeAttr, attrName) {
var booleanKey = getBooleanAttrName(this.$$element[0], key),
$$observers = this.$$observers;
$$observers = this.$$observers,
normalizedVal;

if (booleanKey) {
this.$$element.prop(key, value);
Expand All @@ -254,6 +286,19 @@ function $CompileProvider($provide) {
}
}


// sanitize a[href] values
if (nodeName_(this.$$element[0]) === 'A' && key === 'href') {
urlSanitizationNode.setAttribute('href', value);

// href property always returns normalized absolute url, so we can match against that
normalizedVal = urlSanitizationNode.href;
if (!normalizedVal.match(urlSanitizationWhitelist)) {
this[key] = value = 'unsafe:' + normalizedVal;
}
}


if (writeAttr !== false) {
if (value === null || value === undefined) {
this.$$element.removeAttr(attrName);
Expand Down Expand Up @@ -297,7 +342,8 @@ function $CompileProvider($provide) {
}
};

var startSymbol = $interpolate.startSymbol(),
var urlSanitizationNode = $document[0].createElement('a'),
startSymbol = $interpolate.startSymbol(),
endSymbol = $interpolate.endSymbol(),
denormalizeTemplate = (startSymbol == '{{' || endSymbol == '}}')
? identity
Expand Down Expand Up @@ -748,7 +794,7 @@ function $CompileProvider($provide) {
}
break;
}

case '=': {
parentGet = $parse(attrs[attrName]);
parentSet = parentGet.assign || function() {
Expand Down Expand Up @@ -1029,10 +1075,10 @@ function $CompileProvider($provide) {
function addAttrInterpolateDirective(node, directives, value, name) {
var interpolateFn = $interpolate(value, true);


// no interpolation found -> ignore
if (!interpolateFn) return;


directives.push({
priority: 100,
compile: valueFn(function attrInterpolateLinkFn(scope, element, attr) {
Expand Down
5 changes: 3 additions & 2 deletions src/ng/directive/booleanAttrs.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -340,8 +340,9 @@ forEach(['src', 'href'], function(attrName) {

// on IE, if "ng:src" directive declaration is used and "src" attribute doesn't exist
// then calling element.setAttribute('src', 'foo') doesn't do anything, so we need
// to set the property as well to achieve the desired effect
if (msie) element.prop(attrName, value);
// to set the property as well to achieve the desired effect.
// we use attr[attrName] value since $set can sanitize the url.
if (msie) element.prop(attrName, attr[attrName]);
});
}
};
Expand Down
141 changes: 141 additions & 0 deletions test/ng/compileSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2354,7 +2354,148 @@ describe('$compile', function() {
expect(jqLite(element.find('span')[1]).text()).toEqual('T:true');
});
});
});


describe('href sanitization', function() {

it('should sanitize javascript: urls', inject(function($compile, $rootScope) {
element = $compile('<a href="{{testUrl}}"></a>')($rootScope);
$rootScope.testUrl = "javascript:doEvilStuff()";
$rootScope.$apply();

expect(element.attr('href')).toBe('unsafe:javascript:doEvilStuff()');
}));


it('should sanitize data: urls', inject(function($compile, $rootScope) {
element = $compile('<a href="{{testUrl}}"></a>')($rootScope);
$rootScope.testUrl = "data:evilPayload";
$rootScope.$apply();

expect(element.attr('href')).toBe('unsafe:data:evilPayload');
}));


it('should sanitize obfuscated javascript: urls', inject(function($compile, $rootScope) {
element = $compile('<a href="{{testUrl}}"></a>')($rootScope);

// case-sensitive
$rootScope.testUrl = "JaVaScRiPt:doEvilStuff()";
$rootScope.$apply();
expect(element[0].href).toBe('unsafe:javascript:doEvilStuff()');

// tab in protocol
$rootScope.testUrl = "java\u0009script:doEvilStuff()";
$rootScope.$apply();
expect(element[0].href).toMatch(/(http:\/\/|unsafe:javascript:doEvilStuff\(\))/);

// space before
$rootScope.testUrl = " javascript:doEvilStuff()";
$rootScope.$apply();
expect(element[0].href).toBe('unsafe:javascript:doEvilStuff()');

// ws chars before
$rootScope.testUrl = " \u000e javascript:doEvilStuff()";
$rootScope.$apply();
expect(element[0].href).toMatch(/(http:\/\/|unsafe:javascript:doEvilStuff\(\))/);

// post-fixed with proper url
$rootScope.testUrl = "javascript:doEvilStuff(); http://make.me/look/good";
$rootScope.$apply();
expect(element[0].href).toBeOneOf(
'unsafe:javascript:doEvilStuff(); http://make.me/look/good',
'unsafe:javascript:doEvilStuff();%20http://make.me/look/good'
);
}));


it('should sanitize ngHref bindings as well', inject(function($compile, $rootScope) {
element = $compile('<a ng-href="{{testUrl}}"></a>')($rootScope);
$rootScope.testUrl = "javascript:doEvilStuff()";
$rootScope.$apply();

expect(element[0].href).toBe('unsafe:javascript:doEvilStuff()');
}));


it('should not sanitize valid urls', inject(function($compile, $rootScope) {
element = $compile('<a href="{{testUrl}}"></a>')($rootScope);

$rootScope.testUrl = "foo/bar";
$rootScope.$apply();
expect(element.attr('href')).toBe('foo/bar');

$rootScope.testUrl = "/foo/bar";
$rootScope.$apply();
expect(element.attr('href')).toBe('/foo/bar');

$rootScope.testUrl = "../foo/bar";
$rootScope.$apply();
expect(element.attr('href')).toBe('../foo/bar');

$rootScope.testUrl = "#foo";
$rootScope.$apply();
expect(element.attr('href')).toBe('#foo');

$rootScope.testUrl = "http://foo/bar";
$rootScope.$apply();
expect(element.attr('href')).toBe('http://foo/bar');

$rootScope.testUrl = " http://foo/bar";
$rootScope.$apply();
expect(element.attr('href')).toBe(' http://foo/bar');

$rootScope.testUrl = "https://foo/bar";
$rootScope.$apply();
expect(element.attr('href')).toBe('https://foo/bar');

$rootScope.testUrl = "ftp://foo/bar";
$rootScope.$apply();
expect(element.attr('href')).toBe('ftp://foo/bar');

$rootScope.testUrl = "mailto:foo@bar.com";
$rootScope.$apply();
expect(element.attr('href')).toBe('mailto:foo@bar.com');
}));


it('should not sanitize href on elements other than anchor', inject(function($compile, $rootScope) {
element = $compile('<div href="{{testUrl}}"></div>')($rootScope);
$rootScope.testUrl = "javascript:doEvilStuff()";
$rootScope.$apply();

expect(element.attr('href')).toBe('javascript:doEvilStuff()');
}));


it('should not sanitize attributes other than href', inject(function($compile, $rootScope) {
element = $compile('<a title="{{testUrl}}"></a>')($rootScope);
$rootScope.testUrl = "javascript:doEvilStuff()";
$rootScope.$apply();

expect(element.attr('title')).toBe('javascript:doEvilStuff()');
}));


it('should allow reconfiguration of the href whitelist', function() {
module(function($compileProvider) {
expect($compileProvider.urlSanitizationWhitelist() instanceof RegExp).toBe(true);
var returnVal = $compileProvider.urlSanitizationWhitelist(/javascript:/);
expect(returnVal).toBe($compileProvider);
});

inject(function($compile, $rootScope) {
element = $compile('<a href="{{testUrl}}"></a>')($rootScope);

$rootScope.testUrl = "javascript:doEvilStuff()";
$rootScope.$apply();
expect(element.attr('href')).toBe('javascript:doEvilStuff()');

$rootScope.testUrl = "http://recon/figured";
$rootScope.$apply();
expect(element.attr('href')).toBe('unsafe:http://recon/figured');
});
});
});
});

3 comments on commit 9532234

@vitto32
Copy link

Choose a reason for hiding this comment

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

this prevents local app to work as long as file:// is not allowed. Breaking change!
Even hash urls are sanitized.

@IgorMinar
Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right. I'll get this fixed. in the meantime you can reconfigure the regexp via the provider api.

@IgorMinar
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just merged a fix into the upstream repo.

Please sign in to comment.