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

feat($interpolate): escape interpolated expressions #5628

Closed
wants to merge 2 commits into from
Closed

feat($interpolate): escape interpolated expressions #5628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f73bc73
feat($interpolate): escape interpolated expressions
caitp May 14, 2014
502d411
WIP: improve robustness of escaped interpolation
caitp May 14, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 71 additions & 8 deletions src/ng/interpolate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ var $interpolateMinErr = minErr('$interpolate');
function $InterpolateProvider() {
var startSymbol = '{{';
var endSymbol = '}}';
var escapedStartSymbol = '{{{{';
var escapedEndSymbol = '}}}}';

/**
* @ngdoc method
Expand All @@ -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 the 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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

might want to log a warning or throw or something, if value === escaped or === the current escaped start symbol, and ditto for the end symbol. Or maybe not, I dunno.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it is necessary. I don't even think you should test if this is the case. There are many cases in which this feature can be used improperly, value === escaped is just one of them and I see no reason to single it out...

startSymbol = value;
if (escaped && escaped !== value) {
escapedStartSymbol = '' + escaped;
}
return this;
} else {
return startSymbol;
Expand All @@ -67,11 +73,15 @@ function $InterpolateProvider() {
* Symbol to denote the end of expression in the interpolated string. Defaults to `}}`.
*
* @param {string=} value new value to set the ending symbol to.
* @param {string=} escaped the new value to set the escaped 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){
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should add unit tests for setting startSymbol and endSymbol with escaped

if (value) {
endSymbol = value;
if (escaped && escaped !== value) {
escapedEndSymbol = '' + escaped;
}
return this;
} else {
return endSymbol;
Expand All @@ -81,7 +91,10 @@ 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,
similarStartSymbols = escapedStartSymbol.indexOf(startSymbol) === 0;

/**
* @ngdoc service
Expand Down Expand Up @@ -154,23 +167,55 @@ function $InterpolateProvider() {
hasText = false,
exp,
concat = [],
lastValuesCache = { values: {}, results: {}};
lastValuesCache = { values: {}, results: {}},
escapedStart = -1,
escapedEnd = -1,
escaped = true,
i = -1;

while(index < textLength) {
if ( ((startIndex = text.indexOf(startSymbol, index)) != -1) &&
((endIndex = text.indexOf(endSymbol, startIndex + startSymbolLength)) != -1) ) {
if (i < 0) {
i = index;
}

if (escaped) {
if ((escapedStart = text.indexOf(escapedStartSymbol, i)) != -1) {
if ((escapedEnd = text.indexOf(escapedEndSymbol, escapedStart + escapedStartLength))
!= -1) {
escapedEnd += escapedEndLength;
} else {
escaped = false;
}
} else {
escaped = false;
}
}

if (((startIndex = text.indexOf(startSymbol, i)) != -1) &&
((endIndex = text.indexOf(endSymbol, startIndex + startSymbolLength)) != -1) &&
(!escaped || escapedEnd < startIndex || escapedStart > endIndex + endSymbolLength) ) {
if (escapedEnd < 0 && similarStartSymbols) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this here only to solve issues like {{{{def}}}? Do you have a use case where this is important? I would remove it, I think it solves these cases only partially (not sure, but I think it will work only when the separator is exactly half of the escaped like in the default {{ and {{{{) and I'm not sure it is even desirable to solve these cases...

Copy link
Contributor

Choose a reason for hiding this comment

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

In general, I like the idea of taking only the expression within the most internal braces. I mean the expression in {{{def}}} should be def and not {def and it should be parsed to {world} or something, but I think it is out of scope for this PR.

while (escapedStart >= 0 && startIndex === escapedStart &&
text.indexOf(startSymbol, startIndex) === startIndex) {
startIndex = escapedStart + startSymbolLength;
escapedStart = text.indexOf(escapedStartSymbol, startIndex + startSymbolLength);
}
}
if (index !== startIndex) hasText = true;
separators.push(text.substring(index, startIndex));
separators.push(unescape(text.substring(index, startIndex)));
exp = text.substring(startIndex + startSymbolLength, endIndex);
expressions.push(exp);
parseFns.push($parse(exp));
index = endIndex + endSymbolLength;
hasInterpolation = true;
i = -1;
} else if (escaped && startIndex >= 0 && escapedStart < (endIndex + endSymbolLength)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit confusing. Could be just else if you move the above (!escaped || escapedEnd < startIndex || escapedStart > endIndex + endSymbolLength) into a nested condition.

i = escapedEnd;
} 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));
separators.push(unescape(text.substring(index)));
}
break;
}
Expand Down Expand Up @@ -316,6 +361,24 @@ function $InterpolateProvider() {
return endSymbol;
};

function unescape(text) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would this need to be documented? As an internal function, given the name its use seems pretty clear

var i = 0, start, end, unescaped = "";
while (i < text.length) {
if (((start = text.indexOf(escapedStartSymbol, i)) != -1 &&
((end = text.indexOf(escapedEndSymbol, start + escapedStartLength)) != -1))) {
end = end + escapedEndLength;
unescaped += text.substring(i, end).
replace(escapedStartSymbol, startSymbol).
replace(escapedEndSymbol, endSymbol);
i = end;
} else {
unescaped += text.substring(i);
break;
}
}
return unescaped;
}

return $interpolate;
}];
}
Expand Down
30 changes: 30 additions & 0 deletions test/ng/interpolateSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,36 @@ describe('$interpolate', function() {
}));


it('should not parse escaped interpolation expressions', inject(function($interpolate, $rootScope) {
$rootScope.foo = 'World';
expect($interpolate('Hello, {{{{foo}}}}!')($rootScope)).toBe('Hello, {{foo}}!');
}));


it('should parse expressions before escaped expressions', inject(function($interpolate, $rootScope) {
$rootScope.foo = 'World';
expect($interpolate('Hello, {{foo}}! ({{{{foo}}}})')($rootScope)).toBe('Hello, World! ({{foo}})');
}));


it('should parse expressions after escaped expressions', inject(function($interpolate, $rootScope) {
$rootScope.foo = 'World';
expect($interpolate('Hello, {{{{foo}}}} ({{foo}}!)')($rootScope)).toBe('Hello, {{foo}} (World!)');
}));


it('should not parse naively escaped expressions', inject(function($interpolate, $rootScope) {
$rootScope.abc = 'Hello';
$rootScope.def = 'world';
expect($interpolate('{{{{abc}}}}}}{{{{def}}}}')($rootScope)).toBe('{{abc}}}}{{def}}');
expect($interpolate('{{{{{{abc}}}}{{{{def}}}}')($rootScope)).toBe('{{{{abc}}{{def}}');
expect($interpolate('{{{{abc}}}}{{{{def}}}}}}')($rootScope)).toBe('{{abc}}{{def}}}}');
expect($interpolate('{{{{abc}}}}{{{{{{def}}}}')($rootScope)).toBe('{{abc}}{{{{def}}');
expect($interpolate('}}}}abc{{abc}}}}{{{{def}}}')($rootScope)).toBe('}}}}abcHello}}{{world}');
expect($interpolate('{{{{{{abc{{def}}}}}}')($rootScope)).toBe('{{{{abc{{def}}}}');
}));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the three obvious test cases I could think of, but there may be other valuable tests as well.

The configuration options don't have any error handling or tests, but if they are given incorrect values they're pretty much expected to screw things up, and that would need to be mentioned in docs. Unless a better approach could be used.



describe('interpolating in a trusted context', function() {
var sce;
beforeEach(function() {
Expand Down