From 7fe5c804b4853b2eada5d49f076429ef1869c7d0 Mon Sep 17 00:00:00 2001 From: Mark Lagendijk Date: Wed, 22 Apr 2015 10:28:38 +0100 Subject: [PATCH] feat(sanitization): refactored, fixed and extended sanitization #993 - Added $translateSanitization - Removed sanitization logic from interpolators, and implemented usage of $translateSanitization - Changed $translate to use $translateSanitizationProvider for setting the strategy - Added support for using multiple sanitization strategies, which will be executed as a chain. - Throw an error when an unknown strategy name is specified. Failing silently would be a security issue. - Added methods for adding / removing strategies to $translateSanitizationProvider. --- Gruntfile.js | 1 + bower.json | 3 +- karma.unit.conf.js | 1 + src/service/default-interpolation.js | 49 ++---- src/service/messageformat-interpolation.js | 51 ++---- src/service/sanitization.js | 172 +++++++++++++++++++ src/service/translate.js | 14 +- test/unit/service/sanitization.spec.js | 186 +++++++++++++++++++++ test_scopes/angular_1.2.x/bower.json | 3 +- test_scopes/angular_1.3.x/bower.json | 3 +- test_scopes/angular_1.4.x/bower.json | 3 +- 11 files changed, 399 insertions(+), 87 deletions(-) create mode 100644 src/service/sanitization.js create mode 100644 test/unit/service/sanitization.spec.js diff --git a/Gruntfile.js b/Gruntfile.js index 5199a4766..339127420 100644 --- a/Gruntfile.js +++ b/Gruntfile.js @@ -51,6 +51,7 @@ module.exports = function (grunt) { 'src/service/translate.js', 'src/service/default-interpolation.js', 'src/service/storage-key.js', + 'src/service/sanitization.js', 'src/directive/translate.js', 'src/directive/translate-cloak.js', 'src/filter/translate.js' diff --git a/bower.json b/bower.json index 23df894ce..0e06c8d3f 100644 --- a/bower.json +++ b/bower.json @@ -15,7 +15,8 @@ "angular": "~1.2.26", "angular-translate-interpolation-messageformat": "2.6.1", "angular-mocks": "~1.2.26", - "angular-cookies": "~1.2.26" + "angular-cookies": "~1.2.26", + "angular-sanitize": "~1.2.26" }, "resolutions": { "angular": "~1.2.26" diff --git a/karma.unit.conf.js b/karma.unit.conf.js index 8bce918a4..e4f333167 100644 --- a/karma.unit.conf.js +++ b/karma.unit.conf.js @@ -17,6 +17,7 @@ module.exports = function (config) { shared.injectByScope(scope, 'messageformat/messageformat.js'), shared.injectByScope(scope, 'angular/angular.js'), shared.injectByScope(scope, 'angular-cookies/angular-cookies.js'), + shared.injectByScope(scope, 'angular-sanitize/angular-sanitize.js'), shared.injectByScope(scope, 'angular-mocks/angular-mocks.js'), 'src/translate.js', 'src/**/*.js', diff --git a/src/service/default-interpolation.js b/src/service/default-interpolation.js index c7482f5ac..51b9ac8a2 100644 --- a/src/service/default-interpolation.js +++ b/src/service/default-interpolation.js @@ -10,38 +10,11 @@ */ angular.module('pascalprecht.translate').factory('$translateDefaultInterpolation', $translateDefaultInterpolation); -function $translateDefaultInterpolation ($interpolate) { +function $translateDefaultInterpolation ($interpolate, $translateSanitization) { var $translateInterpolator = {}, $locale, - $identifier = 'default', - $sanitizeValueStrategy = null, - // map of all sanitize strategies - sanitizeValueStrategies = { - escaped: function (params) { - var result = {}; - for (var key in params) { - if (Object.prototype.hasOwnProperty.call(params, key)) { - if (angular.isNumber(params[key])) { - result[key] = params[key]; - } else { - result[key] = angular.element('
').text(params[key]).html(); - } - } - } - return result; - } - }; - - var sanitizeParams = function (params) { - var result; - if (angular.isFunction(sanitizeValueStrategies[$sanitizeValueStrategy])) { - result = sanitizeValueStrategies[$sanitizeValueStrategy](params); - } else { - result = params; - } - return result; - }; + $identifier = 'default'; /** * @ngdoc function @@ -71,8 +44,11 @@ function $translateDefaultInterpolation ($interpolate) { return $identifier; }; + /** + * @deprecated ToDo: remove in 3.0 + */ $translateInterpolator.useSanitizeValueStrategy = function (value) { - $sanitizeValueStrategy = value; + $translateSanitization.useStrategy(value); return this; }; @@ -87,11 +63,14 @@ function $translateDefaultInterpolation ($interpolate) { * * @returns {string} interpolated string. */ - $translateInterpolator.interpolate = function (string, interpolateParams) { - if ($sanitizeValueStrategy) { - interpolateParams = sanitizeParams(interpolateParams); - } - return $interpolate(string)(interpolateParams || {}); + $translateInterpolator.interpolate = function (string, interpolationParams) { + interpolationParams = interpolationParams || {}; + interpolationParams = $translateSanitization.sanitize(interpolationParams, 'params'); + + var interpolatedText = $interpolate(string)(interpolationParams); + interpolatedText = $translateSanitization.sanitize(interpolatedText, 'text'); + + return interpolatedText; }; return $translateInterpolator; diff --git a/src/service/messageformat-interpolation.js b/src/service/messageformat-interpolation.js index 951bd1d20..d9e968c20 100644 --- a/src/service/messageformat-interpolation.js +++ b/src/service/messageformat-interpolation.js @@ -14,36 +14,13 @@ angular.module('pascalprecht.translate') */ .factory('$translateMessageFormatInterpolation', $translateMessageFormatInterpolation); -function $translateMessageFormatInterpolation($cacheFactory, TRANSLATE_MF_INTERPOLATION_CACHE) { +function $translateMessageFormatInterpolation($translateSanitization, $cacheFactory, TRANSLATE_MF_INTERPOLATION_CACHE) { var $translateInterpolator = {}, $cache = $cacheFactory.get(TRANSLATE_MF_INTERPOLATION_CACHE), // instantiate with default locale (which is 'en') $mf = new MessageFormat('en'), - $identifier = 'messageformat', - $sanitizeValueStrategy = null, - // map of all sanitize strategies - sanitizeValueStrategies = { - escaped: function (params) { - var result = {}; - for (var key in params) { - if (Object.prototype.hasOwnProperty.call(params, key)) { - result[key] = angular.element('
').text(params[key]).html(); - } - } - return result; - } - }; - - var sanitizeParams = function (params) { - var result; - if (angular.isFunction(sanitizeValueStrategies[$sanitizeValueStrategy])) { - result = sanitizeValueStrategies[$sanitizeValueStrategy](params); - } else { - result = params; - } - return result; - }; + $identifier = 'messageformat'; if (!$cache) { // create cache if it doesn't exist already @@ -84,8 +61,11 @@ function $translateMessageFormatInterpolation($cacheFactory, TRANSLATE_MF_INTERP return $identifier; }; + /** + * @deprecated ToDo: remove in 3.0 + */ $translateInterpolator.useSanitizeValueStrategy = function (value) { - $sanitizeValueStrategy = value; + $translateSanitization.useStrategy(value); return this; }; @@ -99,21 +79,20 @@ function $translateMessageFormatInterpolation($cacheFactory, TRANSLATE_MF_INTERP * * @returns {string} interpolated string. */ - $translateInterpolator.interpolate = function (string, interpolateParams) { + $translateInterpolator.interpolate = function (string, interpolationParams) { + interpolationParams = interpolationParams || {}; + interpolationParams = $translateSanitization.sanitize(interpolationParams, 'params'); - interpolateParams = interpolateParams || {}; - - if ($sanitizeValueStrategy) { - interpolateParams = sanitizeParams(interpolateParams); - } - - var interpolatedText = $cache.get(string + angular.toJson(interpolateParams)); + var interpolatedText = $cache.get(string + angular.toJson(interpolationParams)); // if given string wasn't interpolated yet, we do so now and never have to do it again if (!interpolatedText) { - interpolatedText = $mf.compile(string)(interpolateParams); - $cache.put(string + angular.toJson(interpolateParams), interpolatedText); + interpolatedText = $mf.compile(string)(interpolationParams); + interpolatedText = $translateSanitization.sanitize(interpolatedText, 'text'); + + $cache.put(string + angular.toJson(interpolationParams), interpolatedText); } + return interpolatedText; }; diff --git a/src/service/sanitization.js b/src/service/sanitization.js new file mode 100644 index 000000000..879575e3d --- /dev/null +++ b/src/service/sanitization.js @@ -0,0 +1,172 @@ +/** + * @ngdoc object + * @name pascalprecht.translate.$translateSanitization + * + * @description + * Sanitizes interpolation parameters and translated texts. + * + * @return {object} $translateInterpolator Interpolator service + */ +angular.module('pascalprecht.translate').provider('$translateSanitization', $translateSanitizationProvider); + +function $translateSanitizationProvider(){ + + var provider = this, + $sanitize, + currentStrategy = null,// ToDo: change to either 'sanitize', 'escape' or ['sanitize', 'escapeParameters'] in 3.0. + hasConfiguredStrategy = false, + hasShownNoStrategyConfiguredWarning = false; + + provider.strategies = { + /** + * Sanitizes HTML in the translation text using $sanitize. + */ + sanitize: function(value, mode){ + if(mode === 'text'){ + value = htmlSanitizeValue(value); + } + return value; + }, + /** + * Escapes HTML in the translation. + */ + escape: function(value, mode){ + if(mode === 'text'){ + value = htmlEscapeValue(value); + } + return value; + }, + /** + * Sanitizes HTML in the values of the interpolation parameters using $sanitize. + */ + sanitizeParameters: function(value, mode){ + if(mode === 'params'){ + value = mapInterpolationParameters(value, htmlSanitizeValue); + } + return value; + }, + /** + * Escapes HTML in the values of the interpolation parameters. + */ + escapeParameters: function(value, mode){ + if(mode === 'params'){ + value = mapInterpolationParameters(value, htmlEscapeValue); + } + return value; + } + }; + // Support legacy strategy name 'escaped' for backwards compatability. ToDo: should be removed in 3.0. + provider.strategies.escaped = provider.strategies.escapeParameters; + + /** + * Adds a sanitization strategy to the list of known strategies. + * @param {string} strategyName + * @param {Function} strategyFunction + */ + provider.addStrategy = function(strategyName, strategyFunction){ + provider.strategies[strategyName] = strategyFunction; + }; + + /** + * Removes a sanitization strategy from the list of known strategies. + * @param {string} strategyName + */ + provider.removeStrategy = function(strategyName){ + delete provider.strategies[strategyName]; + }; + + /** + * Selects a sanitization strategy. When an array is provided the strategies will be executed in order. + * @param {string|Function|Array} strategy The sanitization strategy / strategies which should be used. Either a name of an existing strategy, a custom strategy function, or an array consisting of multiple names and / or custom functions. + * @returns {$translateSanitizationProvider} + */ + provider.useStrategy = function(strategy){ + hasConfiguredStrategy = true; + currentStrategy = strategy; + return this; + }; + + provider.$get = function($injector, $log){ + + var applyStrategies = function(value, mode, strategies){ + angular.forEach(strategies, function(strategy){ + if(angular.isFunction(strategy)){ + value = strategy(value, mode); + } + else if(angular.isFunction(provider.strategies[strategy])){ + value = provider.strategies[strategy](value, mode); + } + else{ + throw new Error('pascalprecht.translate.$translateSanitization: Unknown sanitization strategy: \'' + strategy + '\''); + } + }); + return value; + }; + + // ToDo: should be removed in 3.0 + var showNoStrategyConfiguredWarning = function(){ + if(!hasConfiguredStrategy && !hasShownNoStrategyConfiguredWarning){ + $log.warn('pascalprecht.translate.$translateSanitization: No sanitization strategy has been configured. This can have serious security implications. See http://angular-translate.github.io/docs/#/guide/19_security for details.'); + hasShownNoStrategyConfiguredWarning = true; + } + }; + + if($injector.has('$sanitize')){ + $sanitize = $injector.get('$sanitize'); + } + + return { + /** + * Selects a sanitization strategy. When an array is provided the strategies will be executed in order. + * @param {string|Function|Array} strategy The sanitization strategy / strategies which should be used. Either a name of an existing strategy, a custom strategy function, or an array consisting of multiple names and / or custom functions. + * @returns {$translateSanitizationProvider} + */ + useStrategy: provider.useStrategy, + /** + * Sanitizes a value. + * @param {*} value The value which should be sanitized. + * @param {string} mode The current sanitization mode, either 'params' or 'text'. + * @param {string|Function|Array} [strategy] Optional custom strategy which should be used instead of the currently selected strategy. + * @returns {*} + */ + sanitize: function(value, mode, strategy){ + if(!strategy && !currentStrategy){ + showNoStrategyConfiguredWarning(); + return value; + } + + strategy = strategy || currentStrategy; + var strategies = angular.isArray(strategy) ? strategy : [strategy]; + + return applyStrategies(value, mode, strategies); + } + }; + }; + + var htmlEscapeValue = function(value){ + return angular.element('
').text(value).html(); + }; + + var htmlSanitizeValue = function(value){ + if(!$sanitize){ + throw new Error('pascalprecht.translate.$translateSanitization: Error cannot find $sanitize service. Either include the ngSanitize module (https://docs.angularjs.org/api/ngSanitize) or use a sanitization strategy which does not depend on $sanitize, such as \'escape\'.'); + } + return $sanitize(value); + }; + + var mapInterpolationParameters = function(value, iteratee){ + if(angular.isObject(value)){ + var result = angular.isArray(value) ? [] : {}; + angular.forEach(value, function(propertyValue, propertyKey){ + result[propertyKey] = mapInterpolationParameters(propertyValue, iteratee); + }); + return result; + } + else if(angular.isNumber(value)){ + return value; + } + else{ + return iteratee(value); + } + }; +} diff --git a/src/service/translate.js b/src/service/translate.js index e1f665e73..ad00ee07b 100644 --- a/src/service/translate.js +++ b/src/service/translate.js @@ -10,7 +10,7 @@ angular.module('pascalprecht.translate') .provider('$translate', $translate); -function $translate($STORAGE_KEY, $windowProvider) { +function $translate($STORAGE_KEY, $windowProvider, $translateSanitizationProvider) { var $translationTable = {}, $preferredLanguage, @@ -26,7 +26,6 @@ function $translate($STORAGE_KEY, $windowProvider) { $missingTranslationHandlerFactory, $interpolationFactory, $interpolatorFactories = [], - $interpolationSanitizationStrategy = false, $loaderFactory, $cloakClassName = 'translate-cloak', $loaderOptions, @@ -325,7 +324,7 @@ function $translate($STORAGE_KEY, $windowProvider) { * @param {string} value Strategy type. */ this.useSanitizeValueStrategy = function (value) { - $interpolationSanitizationStrategy = value; + $translateSanitizationProvider.useStrategy(value); return this; }; @@ -1074,11 +1073,6 @@ function $translate($STORAGE_KEY, $windowProvider) { } } - // apply additional settings - if (angular.isFunction(defaultInterpolator.useSanitizeValueStrategy)) { - defaultInterpolator.useSanitizeValueStrategy($interpolationSanitizationStrategy); - } - // if we have additional interpolations that were added via // $translateProvider.addInterpolation(), we have to map'em if ($interpolatorFactories.length) { @@ -1086,10 +1080,6 @@ function $translate($STORAGE_KEY, $windowProvider) { var interpolator = $injector.get(interpolatorFactory); // setting initial locale for each interpolation service interpolator.setLocale($preferredLanguage || $uses); - // apply additional settings - if (angular.isFunction(interpolator.useSanitizeValueStrategy)) { - interpolator.useSanitizeValueStrategy($interpolationSanitizationStrategy); - } // make'em recognizable through id interpolatorHashMap[interpolator.getInterpolationIdentifier()] = interpolator; }; diff --git a/test/unit/service/sanitization.spec.js b/test/unit/service/sanitization.spec.js new file mode 100644 index 000000000..cc6deb80d --- /dev/null +++ b/test/unit/service/sanitization.spec.js @@ -0,0 +1,186 @@ +describe('pascalprecht.translate', function(){ + describe('$translateSanitization', function(){ + var $translateSanitization; + + beforeEach(module('pascalprecht.translate', 'ngSanitize')); + beforeEach(inject(function(_$translateSanitization_){ + $translateSanitization = _$translateSanitization_; + })); + + it('should be defined', function(){ + expect($translateSanitization).toBeDefined(); + }); + + it('should be an object ', function(){ + expect(typeof $translateSanitization).toBe('object'); + }); + + it('should have a useStrategy() method', function(){ + expect($translateSanitization.useStrategy).toBeDefined(); + }); + + it('should have a sanitize() method', function(){ + expect($translateSanitization.sanitize).toBeDefined(); + }); + + describe('#sanitize', function(){ + var parameters = { + array: [ + { value: 'This is only an example with a xss attack!' } + ] + }, + text = 'This is only an example with a xss attack!', + expectedParameters, + expectedText; + + describe('with the default strategy', function(){ + it('should return params unchanged', function(){ + expectedParameters = angular.copy(parameters); + expect($translateSanitization.sanitize(parameters, 'params')).toEqual(expectedParameters); + }); + + it('should return text unchanged', function(){ + expectedText = text; + expect($translateSanitization.sanitize(text, 'text')).toEqual(expectedText); + }); + + it('should warn that no strategy has been configured', inject(function($log){ + spyOn($log, 'warn'); + $translateSanitization.sanitize(text, 'text'); + expect($log.warn).toHaveBeenCalled(); + })); + }); + + describe('with the sanitize strategy', function(){ + beforeEach(function(){ + $translateSanitization.useStrategy('sanitize'); + }); + + it('should return params unchanged', function(){ + expectedParameters = angular.copy(parameters); + expect($translateSanitization.sanitize(parameters, 'params')).toEqual(expectedParameters); + }); + + it('should $sanitize the text', function(){ + expectedText = 'This is only an example with a xss attack!'; + expect($translateSanitization.sanitize(text, 'text')).toEqual(expectedText); + }); + }); + + describe('with the escape strategy', function(){ + beforeEach(function(){ + $translateSanitization.useStrategy('escape'); + }); + + it('should return params unchanged', function(){ + expectedParameters = angular.copy(parameters); + expect($translateSanitization.sanitize(parameters, 'params')).toEqual(expectedParameters); + }); + + it('should htmlEscape the text', function(){ + expectedText = 'This is <b>only an example with a <span onclick="alert(\'XSS\')">xss attack</span>!</b>'; + expect($translateSanitization.sanitize(text, 'text')).toEqual(expectedText); + }); + }); + + describe('with the sanitizeParameters strategy', function(){ + beforeEach(function(){ + $translateSanitization.useStrategy('sanitizeParameters'); + }); + + it('should $sanitize params', function(){ + expectedParameters = { + array: [ + { value: 'This is only an example with a xss attack!' } + ] + }; + expect($translateSanitization.sanitize(parameters, 'params')).toEqual(expectedParameters); + }); + + it('should return text unchanged', function(){ + expectedText = text; + expect($translateSanitization.sanitize(text, 'text')).toEqual(expectedText); + }); + }); + + describe('with the escapeParameters strategy', function(){ + beforeEach(function(){ + $translateSanitization.useStrategy('escapeParameters'); + }); + + it('should htmlEscape params', function(){ + expectedParameters = { + array: [ + { value: 'This is <b>only an example with a <span onclick="alert(\'XSS\')">xss attack</span>!</b>' } + ] + }; + expect($translateSanitization.sanitize(parameters, 'params')).toEqual(expectedParameters); + }); + + it('should return text unchanged', function(){ + expectedText = text; + expect($translateSanitization.sanitize(text, 'text')).toEqual(expectedText); + }); + }); + + describe('with the chained [sanitize, escapeParameters] strategy', function(){ + beforeEach(function(){ + $translateSanitization.useStrategy(['sanitize', 'escapeParameters']); + }); + + it('should htmlEscape params', function(){ + expectedParameters = { + array: [ + { value: 'This is <b>only an example with a <span onclick="alert(\'XSS\')">xss attack</span>!</b>' } + ] + }; + expect($translateSanitization.sanitize(parameters, 'params')).toEqual(expectedParameters); + }); + + it('should $sanitize the text', function(){ + expectedText = 'This is only an example with a xss attack!'; + expect($translateSanitization.sanitize(text, 'text')).toEqual(expectedText); + }); + }); + + it('with a custom strategy function should call the strategy function with value and mode', function(){ + $translateSanitization.useStrategy(function(value, mode){ + if(mode === 'text'){ + value = value.toLowerCase(); + } + return value; + }); + expect($translateSanitization.sanitize('THIS IS A TEST', 'text')).toBe('this is a test'); + }); + + it('should allow specifying a different strategy', function(){ + $translateSanitization.useStrategy('escape'); + expectedText = 'This is only an example with a xss attack!'; + expect($translateSanitization.sanitize(text, 'text', 'sanitize')).toEqual(expectedText); + }); + }); + }); + + describe('$translateSanitization#sanitize without ngSanitize', function(){ + var $translateSanitization; + + beforeEach(module('pascalprecht.translate')); + beforeEach(inject(function(_$translateSanitization_){ + $translateSanitization = _$translateSanitization_; + })); + + describe('with the escape strategy', function(){ + it('should work normally', function(){ + $translateSanitization.useStrategy('escape'); + expect($translateSanitization.sanitize('test', 'text')).toEqual('<span>test</span>'); + }); + }); + + describe('with the sanitize strategy', function(){ + it('should throw an error', function(){ + $translateSanitization.useStrategy('sanitize'); + expect($translateSanitization.sanitize.bind($translateSanitization, 'test', 'text')).toThrow(); + }); + }); + }); +}); diff --git a/test_scopes/angular_1.2.x/bower.json b/test_scopes/angular_1.2.x/bower.json index 94797800c..ec344850b 100644 --- a/test_scopes/angular_1.2.x/bower.json +++ b/test_scopes/angular_1.2.x/bower.json @@ -14,7 +14,8 @@ "angular": "~1.2.0", "angular-translate-interpolation-messageformat": "2.4.1", "angular-mocks": "~1.2.0", - "angular-cookies": "~1.2.0" + "angular-cookies": "~1.2.0", + "angular-sanitize": "~1.2.0" }, "resolutions": { "angular": "~1.2.0" diff --git a/test_scopes/angular_1.3.x/bower.json b/test_scopes/angular_1.3.x/bower.json index 4940f771d..4a9f4a02d 100644 --- a/test_scopes/angular_1.3.x/bower.json +++ b/test_scopes/angular_1.3.x/bower.json @@ -14,7 +14,8 @@ "angular": "~1.3.0", "angular-translate-interpolation-messageformat": "2.4.1", "angular-mocks": "~1.3.0", - "angular-cookies": "~1.3.0" + "angular-cookies": "~1.3.0", + "angular-sanitize": "~1.3.0" }, "resolutions": { "angular": "~1.3.0" diff --git a/test_scopes/angular_1.4.x/bower.json b/test_scopes/angular_1.4.x/bower.json index f9168536b..7fcc010ee 100644 --- a/test_scopes/angular_1.4.x/bower.json +++ b/test_scopes/angular_1.4.x/bower.json @@ -14,7 +14,8 @@ "angular": "1.4.0-rc.0", "angular-translate-interpolation-messageformat": "2.4.1", "angular-mocks": "1.4.0-rc.0", - "angular-cookies": "1.4.0-rc.0" + "angular-cookies": "1.4.0-rc.0", + "angular-sanitize": "1.4.0-rc.0" }, "resolutions": { "angular": "1.4.0-rc.0"