From 2965129b58ba6392f007e1a490942063edc49815 Mon Sep 17 00:00:00 2001 From: Caitlin Potter Date: Mon, 19 May 2014 10:00:58 -0400 Subject: [PATCH 1/2] feat($interpolate): enable escaping interpolated expressions Previously, Angular would offer no proper mechanism to reveal attempted script injection attacks when users would add expressions which may be compiled by angular. This CL enables web servers to escape escaped expressions by replacing interpolation start and end markers with escpaed values (which by default are `{{{{` and `}}}}`, respectively). This also allows the application to render the content of the expression without rendering just the result of the expression. Closes #5601 --- src/ng/interpolate.js | 123 ++++++++++++++++++++++++++++--------- test/ng/interpolateSpec.js | 40 ++++++++++++ 2 files changed, 133 insertions(+), 30 deletions(-) diff --git a/src/ng/interpolate.js b/src/ng/interpolate.js index f3209a9800a0..98f89d9eb599 100644 --- a/src/ng/interpolate.js +++ b/src/ng/interpolate.js @@ -41,6 +41,8 @@ var $interpolateMinErr = minErr('$interpolate'); function $InterpolateProvider() { var startSymbol = '{{'; var endSymbol = '}}'; + var escapedStartSymbol = '{{{{'; + var escapedEndSymbol = '}}}}'; /** * @ngdoc method @@ -49,11 +51,15 @@ function $InterpolateProvider() { * Symbol to denote start of expression in the interpolated string. Defaults to `{{`. * * @param {string=} value new value to set the starting symbol to. + * @param {string=} escaped new value to set the escaped starting symbol to. * @returns {string|self} Returns the symbol when used as getter and self if used as setter. */ - this.startSymbol = function(value){ + this.startSymbol = function(value, escaped){ if (value) { startSymbol = value; + if (escaped) { + escapedStartSymbol = escaped; + } return this; } else { return startSymbol; @@ -69,9 +75,12 @@ function $InterpolateProvider() { * @param {string=} value new value to set the ending symbol to. * @returns {string|self} Returns the symbol when used as getter and self if used as setter. */ - this.endSymbol = function(value){ + this.endSymbol = function(value, escaped){ if (value) { endSymbol = value; + if (escaped) { + escapedEndSymbol = escaped; + } return this; } else { return endSymbol; @@ -81,7 +90,32 @@ function $InterpolateProvider() { this.$get = ['$parse', '$exceptionHandler', '$sce', function($parse, $exceptionHandler, $sce) { var startSymbolLength = startSymbol.length, - endSymbolLength = endSymbol.length; + endSymbolLength = endSymbol.length, + escapedStartLength = escapedStartSymbol.length, + escapedEndLength = escapedEndSymbol.length, + escapedLength = escapedStartLength + escapedEndLength, + ESCAPED_EXPR_REGEXP = new RegExp(lit(escapedStartSymbol) + '((?:.|\n)*?)' + lit(escapedEndSymbol), 'm'), + EXPR_REGEXP = new RegExp(lit(startSymbol) + '((?:.|\n)*?)' + lit(endSymbol), 'm'); + + function lit(str) { + return str.replace(/([\(\)\[\]\{\}\+\\\^\$\.\!\?\*\=\:\|\-])/g, function(op) { + return '\\' + op; + }); + } + + function Piece(text, isExpr) { + this.text = text; + this.isExpr = isExpr; + } + + function addPiece(text, isExpr, pieces) { + var lastPiece = pieces.length ? pieces[pieces.length - 1] : null; + if (!isExpr && lastPiece && !lastPiece.isExpr) { + lastPiece.text += text; + } else { + pieces.push(new Piece(text, isExpr)); + } + } /** * @ngdoc service @@ -152,27 +186,58 @@ function $InterpolateProvider() { textLength = text.length, hasInterpolation = false, hasText = false, - exp, - concat = [], - lastValuesCache = { values: {}, results: {}}; - - while(index < textLength) { - if ( ((startIndex = text.indexOf(startSymbol, index)) != -1) && - ((endIndex = text.indexOf(endSymbol, startIndex + startSymbolLength)) != -1) ) { - if (index !== startIndex) hasText = true; - separators.push(text.substring(index, startIndex)); - exp = text.substring(startIndex + startSymbolLength, endIndex); - expressions.push(exp); - parseFns.push($parse(exp)); - index = endIndex + endSymbolLength; + exp = text, + concat, + lastValuesCache = { values: {}, results: {}}, + pieces = [], + piece, + indexes = []; + + while (text.length) { + var expr = EXPR_REGEXP.exec(text); + var escaped = ESCAPED_EXPR_REGEXP.exec(text); + var until = text.length; + var chunk; + var from = 0; + + if (expr) { + until = expr.index; + } + + if (escaped && escaped.index <= until) { + from = escaped.index; + until = escaped.index + escaped[0].length; + escaped = startSymbol + escaped[1] + endSymbol; + expr = null; + } + + if (until > 0) { + chunk = isString(escaped) ? text.substring(0, from) + escaped : text.substring(0, until); + text = text.slice(until); + addPiece(chunk, false, pieces); + separators.push(chunk); + hasText = true; + } else { + separators.push(''); + } + + if (expr) { + text = text.slice(expr[0].length); + addPiece(expr[1], true, pieces); + expr = null; hasInterpolation = true; + } + } + + concat = new Array(pieces.length); + for (index = 0; index < pieces.length; ++index) { + piece = pieces[index]; + if (piece.isExpr) { + parseFns.push($parse(piece.text)); + expressions.push(piece.text); + indexes.push(index); } else { - // we did not find an interpolation, so we have to add the remainder to the separators array - if (index !== textLength) { - hasText = true; - separators.push(text.substring(index)); - } - break; + concat[index] = piece.text; } } @@ -180,6 +245,8 @@ function $InterpolateProvider() { separators.push(''); } + pieces = null; + // Concatenating expressions makes it hard to reason about whether some combination of // concatenated values are unsafe to use and could easily lead to XSS. By requiring that a // single expression be used for iframe[src], object[src], etc., we ensure that the value @@ -190,18 +257,14 @@ function $InterpolateProvider() { throw $interpolateMinErr('noconcat', "Error while interpolating: {0}\nStrict Contextual Escaping disallows " + "interpolations that concatenate multiple expressions when a trusted value is " + - "required. See http://docs.angularjs.org/api/ng.$sce", text); + "required. See http://docs.angularjs.org/api/ng.$sce", exp); } if (!mustHaveExpression || hasInterpolation) { - concat.length = separators.length + expressions.length; - var compute = function(values) { for(var i = 0, ii = expressions.length; i < ii; i++) { - concat[2*i] = separators[i]; - concat[(2*i)+1] = values[i]; + concat[indexes[i]] = values[i]; } - concat[2*ii] = separators[ii]; return concat.join(''); }; @@ -278,7 +341,7 @@ function $InterpolateProvider() { lastValuesCache.results[scopeId] = lastResult = compute(values); } } catch(err) { - var newErr = $interpolateMinErr('interr', "Can't interpolate: {0}\n{1}", text, + var newErr = $interpolateMinErr('interr', "Can't interpolate: {0}\n{1}", exp, err.toString()); $exceptionHandler(newErr); } @@ -286,7 +349,7 @@ function $InterpolateProvider() { return lastResult; }, { // all of these properties are undocumented for now - exp: text, //just for compatibility with regular watchers created via $watch + exp: exp, //just for compatibility with regular watchers created via $watch separators: separators, expressions: expressions }); diff --git a/test/ng/interpolateSpec.js b/test/ng/interpolateSpec.js index 6dd49d6bdaae..f91b31880786 100644 --- a/test/ng/interpolateSpec.js +++ b/test/ng/interpolateSpec.js @@ -61,6 +61,46 @@ describe('$interpolate', function() { })); + describe('interpolation escaping', function() { + var obj; + + beforeEach(function() { + obj = {foo: 'Hello', bar: 'World'}; + }); + + it('should support escaping interpolation signs', inject(function($interpolate) { + expect($interpolate('{{foo}} {{{{bar}}}}')(obj)).toBe('Hello {{bar}}'); + expect($interpolate('{{{{foo}}}} {{bar}}')(obj)).toBe('{{foo}} World'); + })); + + + it('should unescape multiple expressions', inject(function($interpolate) { + expect($interpolate('{{{{foo}}}}{{{{bar}}}} {{foo}}')(obj)).toBe('{{foo}}{{bar}} Hello'); + })); + + + it('should support customizing escape signs', function() { + module(function($interpolateProvider) { + $interpolateProvider.startSymbol('{{', '[['); + $interpolateProvider.endSymbol('}}', ']]'); + }); + inject(function($interpolate) { + expect($interpolate('{{foo}} [[bar]]')(obj)).toBe('Hello {{bar}}'); + }); + }); + + it('should support customizing escape signs which contain interpolation signs', function() { + module(function($interpolateProvider) { + $interpolateProvider.startSymbol('{{', '-{{-'); + $interpolateProvider.endSymbol('}}', '-}}-'); + }); + inject(function($interpolate) { + expect($interpolate('{{foo}} -{{-bar-}}-')(obj)).toBe('Hello {{bar}}'); + }); + }); + }); + + describe('interpolating in a trusted context', function() { var sce; beforeEach(function() { From 3fd78365fd0fed9bb337350bfbf4589076cc5c2f Mon Sep 17 00:00:00 2001 From: Caitlin Potter Date: Mon, 19 May 2014 19:57:11 -0400 Subject: [PATCH 2/2] fix($interpolate): don't unescape incomplete interpolation escape markers This is a hack to prevent a potential issue brought up by @shahata. Its merit is up for discussion. --- src/ng/interpolate.js | 28 +++++++++++++++++++++++----- test/ng/interpolateSpec.js | 8 ++++++++ 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/src/ng/interpolate.js b/src/ng/interpolate.js index 98f89d9eb599..9d5022752d84 100644 --- a/src/ng/interpolate.js +++ b/src/ng/interpolate.js @@ -115,6 +115,7 @@ function $InterpolateProvider() { } else { pieces.push(new Piece(text, isExpr)); } + return pieces[pieces.length-1]; } /** @@ -191,7 +192,8 @@ function $InterpolateProvider() { lastValuesCache = { values: {}, results: {}}, pieces = [], piece, - indexes = []; + indexes = [], + last; while (text.length) { var expr = EXPR_REGEXP.exec(text); @@ -199,6 +201,16 @@ function $InterpolateProvider() { var until = text.length; var chunk; var from = 0; + var start; + + if (!escaped && expr) { + index = 0; + from = expr.index; + while ((start = text.indexOf(escapedStartSymbol, index)) >= 0 && start <= from) { + index = until = start + escapedStartLength; + expr = null; + } + } if (expr) { until = expr.index; @@ -214,8 +226,14 @@ function $InterpolateProvider() { if (until > 0) { chunk = isString(escaped) ? text.substring(0, from) + escaped : text.substring(0, until); text = text.slice(until); - addPiece(chunk, false, pieces); - separators.push(chunk); + var newChunk = addPiece(chunk, false, pieces); + if (last && !last.isExpr && separators.length) { + separators[separators.length - 1] += chunk; + } else { + separators.push(chunk); + } + last = newChunk; + newChunk = null; hasText = true; } else { separators.push(''); @@ -223,7 +241,7 @@ function $InterpolateProvider() { if (expr) { text = text.slice(expr[0].length); - addPiece(expr[1], true, pieces); + last = addPiece(expr[1], true, pieces); expr = null; hasInterpolation = true; } @@ -245,7 +263,7 @@ function $InterpolateProvider() { separators.push(''); } - pieces = null; + last = pieces = null; // Concatenating expressions makes it hard to reason about whether some combination of // concatenated values are unsafe to use and could easily lead to XSS. By requiring that a diff --git a/test/ng/interpolateSpec.js b/test/ng/interpolateSpec.js index f91b31880786..44f12ac7853e 100644 --- a/test/ng/interpolateSpec.js +++ b/test/ng/interpolateSpec.js @@ -98,6 +98,14 @@ describe('$interpolate', function() { expect($interpolate('{{foo}} -{{-bar-}}-')(obj)).toBe('Hello {{bar}}'); }); }); + + + it('should not unescape incomplete escape expressions', inject(function($interpolate) { + expect($interpolate('{{{{foo{{foo}}')(obj)).toBe('{{{{fooHello'); + expect($interpolate('}}}}foo{{foo}}')(obj)).toBe('}}}}fooHello'); + expect($interpolate('foo{{foo}}{{{{')(obj)).toBe('fooHello{{{{'); + expect($interpolate('foo{{foo}}}}}}')(obj)).toBe('fooHello}}}}'); + })); });