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

feat($interpolate): escaped interpolation expressions #7517

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
50 changes: 49 additions & 1 deletion src/ng/interpolate.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,13 @@ function $InterpolateProvider() {

this.$get = ['$parse', '$exceptionHandler', '$sce', function($parse, $exceptionHandler, $sce) {
var startSymbolLength = startSymbol.length,
endSymbolLength = endSymbol.length;
endSymbolLength = endSymbol.length,
escapedStartRegexp = new RegExp(startSymbol.replace(/./g, escape), 'g'),
escapedEndRegexp = new RegExp(endSymbol.replace(/./g, escape), 'g');
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 end up with really ugly looking escape markers this way, which is fine for computers, but it's not very pleasant to look at. The search/replace style escaping can't have custom escape markers for aesthetic niceness, because they would be potentially exploitable.


function escape(ch) {
return '\\\\\\' + ch;
}

/**
* @ngdoc service
Expand Down Expand Up @@ -126,6 +132,42 @@ function $InterpolateProvider() {
*
* `allOrNothing` is useful for interpolating URLs. `ngSrc` and `ngSrcset` use this behavior.
*
* ####Escaped Interpolation
* $interpolate provides a mechanism for escaping interpolation markers. Start and end markers
* can be escaped by preceding each of their characters with a REVERSE SOLIDUS U+005C (backslash).
* It will be rendered as a regular start/end marker, and will not be interpreted as an expression
* or binding.
*
* This enables web-servers to prevent script injection attacks and defacing attacks, to some
* degree, while also enabling code examples to work without relying on the
* {@link ng.directive:ngNonBindable ngNonBindable} directive.
*
* **For security purposes, it is strongly encouraged that web servers escape user-supplied data,
* replacing angle brackets (<, >) with < and > respectively, and replacing all
* interpolation start/end markers with their escaped counterparts.**
*
* Escaped interpolation markers are only replaced with the actual interpolation markers in rendered
* output when the $interpolate service processes the text. So, for HTML elements interpolated
* by {@link ng.$compile $compile}, or otherwise interpolated with the `mustHaveExpression` parameter
* set to `true`, the interpolated text must contain an unescaped interpolation expression. As such,
* this is typically useful only when user-data is used in rendering a template from the server, or
* when otherwise untrusted data is used by a directive.
*
* <example>
* <file name="index.html">
* <div ng-init="username='A user'">
* <p ng-init="apptitle='Escaping demo'">{{apptitle}}: \{\{ username = "some jerk"; \}\}
* </p>
* <p><strong>{{username}}</strong> attempts to inject code which will deface the
* application, but fails to accomplish their task, because the server has correctly
* escaped the interpolation start/end markers with REVERSE SOLIDUS U+005C (backslash)
* characters.</p>
* <p>Instead, the result of the attempted script injection is visible, and can be removed
* from the database by an administrator.</p>
* </div>
* </file>
* </example>
*
* @param {string} text The text with markup to interpolate.
* @param {boolean=} mustHaveExpression if set to true then the interpolation string must have
* embedded expression in order to return an interpolation function. Strings with no
Expand Down Expand Up @@ -176,6 +218,12 @@ function $InterpolateProvider() {
}
}

forEach(separators, function(key, i) {
separators[i] = separators[i].
replace(escapedStartRegexp, startSymbol).
replace(escapedEndRegexp, endSymbol);
});

if (separators.length === expressions.length) {
separators.push('');
}
Expand Down
60 changes: 60 additions & 0 deletions test/ng/interpolateSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,66 @@ 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');
expect($interpolate('{{foo}}\\{\\{foo\\}\\}\\{\\{bar\\}\\}')(obj)).toBe('Hello{{foo}}{{bar}}');
expect($interpolate('\\{\\{foo\\}\\}{{foo}}\\{\\{bar\\}\\}')(obj)).toBe('{{foo}}Hello{{bar}}');
expect($interpolate('{{foo}}\\{\\{foo\\}\\}{{bar}}\\{\\{bar\\}\\}{{foo}}')(obj)).toBe('Hello{{foo}}World{{bar}}Hello');
}));


it('should support escaping custom interpolation start/end symbols', function() {
module(function($interpolateProvider) {
$interpolateProvider.startSymbol('[[');
$interpolateProvider.endSymbol(']]');
});
inject(function($interpolate) {
expect($interpolate('[[foo]] \\[\\[bar\\]\\]')(obj)).toBe('Hello [[bar]]');
});
});


it('should unescape incomplete escaped 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}}');
}));


it('should not unescape markers within expressions', inject(function($interpolate) {
expect($interpolate('{{"\\\\{\\\\{Hello, world!\\\\}\\\\}"}}')(obj)).toBe('\\{\\{Hello, world!\\}\\}');
expect($interpolate('{{"\\{\\{Hello, world!\\}\\}"}}')(obj)).toBe('{{Hello, world!}}');
expect(function() {
$interpolate('{{\\{\\{foo\\}\\}}}')(obj);
}).toThrowMinErr('$parse', 'lexerr',
'Lexer Error: Unexpected next character at columns 0-0 [\\] in expression [\\{\\{foo\\}\\]');
}));


// This test demonstrates that the web-server is responsible for escaping every single instance
// of interpolation start/end markers in an expression which they do not wish to evaluate,
// because AngularJS will not protect them from being evaluated (due to the added complexity
// and maintenance burden of context-sensitive escaping)
it('should evaluate expressions between escaped start/end symbols', inject(function($interpolate) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what I really don't like about this version. (There are other things too, but this is the most problematic). If people are ok with this (and certainly many will be), then I'm all for it. But I see this as bad.

In my view, escaped expression markers should be fully protected from having their contents evaluated in any fashion

Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a comment for this test that states that the server is responsible for ensuring that all bindings are properly escaped, if that doesn't happen, we won't go out of our way to ignore bindings in improperly escaped string.

expect($interpolate('\\{\\{Hello, {{bar}}!\\}\\}')(obj)).toBe('{{Hello, World!}}');
}));
});


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