Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

fix(typeahead): Typeahead uses ngSanitize when present #3463

Closed
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
1 change: 1 addition & 0 deletions karma.conf.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ module.exports = function(config) {
'misc/test-lib/jquery-1.8.2.min.js',
'node_modules/angular/angular.js',
'node_modules/angular-mocks/angular-mocks.js',
'node_modules/angular-sanitize/angular-sanitize.js',
'misc/test-lib/helpers.js',
'src/**/*.js',
'template/**/*.js'
Expand Down
2 changes: 1 addition & 1 deletion misc/demo/assets/app.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* global FastClick, smoothScroll */
angular.module('ui.bootstrap.demo', ['ui.bootstrap', 'plunker', 'ngTouch', 'ngAnimate'], function($httpProvider){
angular.module('ui.bootstrap.demo', ['ui.bootstrap', 'plunker', 'ngTouch', 'ngAnimate', 'ngSanitize'], function($httpProvider){
FastClick.attach(document.body);
delete $httpProvider.defaults.headers.common['X-Requested-With'];
}).run(['$location', function($location){
Expand Down
1 change: 1 addition & 0 deletions misc/demo/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
<script src="//ajax.googleapis.com/ajax/libs/angularjs/<%= ngversion %>/angular.min.js"></script>
<script src="//ajax.googleapis.com/ajax/libs/angularjs/<%= ngversion %>/angular-animate.min.js"></script>
<script src="//ajax.googleapis.com/ajax/libs/angularjs/<%= ngversion %>/angular-touch.min.js"></script>
<script src="//ajax.googleapis.com/ajax/libs/angularjs/<%= ngversion %>/angular-sanitize.js"></script>
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 missed adding ngSanitize into the demo/app.js. Is this change needed for the demo to work? If not, we can just leave out this change for now and add it later when needed.

<script src="ui-bootstrap-tpls-<%= pkg.version%>.min.js"></script>
<script src="assets/plunker.js"></script>
<script src="assets/app.js"></script>
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
"devDependencies": {
"angular": "^1.4.3",
"angular-mocks": "^1.4.3",
"angular-sanitize": "^1.4.3",
"grunt": "^0.4.5",
"grunt-contrib-concat": "^0.5.1",
"grunt-contrib-copy": "^0.8.0",
Expand Down
17 changes: 17 additions & 0 deletions src/typeahead/test/typeahead-highlight-ngsanitize.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
describe('Security concerns', function() {
var highlightFilter, $sanitize, logSpy;

beforeEach(module('ui.bootstrap.typeahead', 'ngSanitize'));

beforeEach(inject(function (typeaheadHighlightFilter, _$sanitize_, $log) {
highlightFilter = typeaheadHighlightFilter;
$sanitize = _$sanitize_;
logSpy = spyOn($log, 'warn');
}));

it('should not call the $log service when ngSanitize is present', function() {
highlightFilter('before <script src="">match</script> after', 'match');
expect(logSpy).not.toHaveBeenCalled();
});

});
52 changes: 33 additions & 19 deletions src/typeahead/test/typeahead-highlight.spec.js
Original file line number Diff line number Diff line change
@@ -1,38 +1,52 @@
describe('typeaheadHighlight', function() {
var highlightFilter;
describe('typeaheadHighlight', function () {

var highlightFilter, $log, $sce, logSpy;

beforeEach(module('ui.bootstrap.typeahead'));
beforeEach(inject(function(typeaheadHighlightFilter) {

beforeEach(inject(function(_$log_, _$sce_) {
$log = _$log_;
$sce = _$sce_;
logSpy = spyOn($log, 'warn');
}));

beforeEach(inject(function (typeaheadHighlightFilter) {
highlightFilter = typeaheadHighlightFilter;
}));

it('should higlight a match', function() {
expect(highlightFilter('before match after', 'match')).toEqual('before <strong>match</strong> after');
it('should higlight a match', function () {
expect($sce.getTrustedHtml(highlightFilter('before match after', 'match'))).toEqual('before <strong>match</strong> after');
});

it('should higlight a match with mixed case', function() {
expect(highlightFilter('before MaTch after', 'match')).toEqual('before <strong>MaTch</strong> after');
it('should higlight a match with mixed case', function () {
expect($sce.getTrustedHtml(highlightFilter('before MaTch after', 'match'))).toEqual('before <strong>MaTch</strong> after');
});

it('should higlight all matches', function() {
expect(highlightFilter('before MaTch after match', 'match')).toEqual('before <strong>MaTch</strong> after <strong>match</strong>');
it('should higlight all matches', function () {
expect($sce.getTrustedHtml(highlightFilter('before MaTch after match', 'match'))).toEqual('before <strong>MaTch</strong> after <strong>match</strong>');
});

it('should do nothing if no match', function() {
expect(highlightFilter('before match after', 'nomatch')).toEqual('before match after');
it('should do nothing if no match', function () {
expect($sce.getTrustedHtml(highlightFilter('before match after', 'nomatch'))).toEqual('before match after');
});

it('should do nothing if no or empty query', function() {
expect(highlightFilter('before match after', '')).toEqual('before match after');
expect(highlightFilter('before match after', null)).toEqual('before match after');
expect(highlightFilter('before match after', undefined)).toEqual('before match after');
it('should do nothing if no or empty query', function () {
expect($sce.getTrustedHtml(highlightFilter('before match after', ''))).toEqual('before match after');
expect($sce.getTrustedHtml(highlightFilter('before match after', null))).toEqual('before match after');
expect($sce.getTrustedHtml(highlightFilter('before match after', undefined))).toEqual('before match after');
});

it('issue 316 - should work correctly for regexp reserved words', function() {
expect(highlightFilter('before (match after', '(match')).toEqual('before <strong>(match</strong> after');
it('issue 316 - should work correctly for regexp reserved words', function () {
expect($sce.getTrustedHtml(highlightFilter('before (match after', '(match'))).toEqual('before <strong>(match</strong> after');
});

it('issue 1777 - should work correctly with numeric values', function() {
expect(highlightFilter(123, '2')).toEqual('1<strong>2</strong>3');
it('issue 1777 - should work correctly with numeric values', function () {
expect($sce.getTrustedHtml(highlightFilter(123, '2'))).toEqual('1<strong>2</strong>3');
});

it('should show a warning when this component is being used unsafely', function() {
highlightFilter('<i>before</i> match after', 'match');
expect(logSpy).toHaveBeenCalled();
});

});
19 changes: 10 additions & 9 deletions src/typeahead/test/typeahead.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ describe('typeahead tests', function() {
var changeInputValueTo;

beforeEach(module('ui.bootstrap.typeahead'));
beforeEach(module('ngSanitize'));
beforeEach(module('template/typeahead/typeahead-popup.html'));
beforeEach(module('template/typeahead/typeahead-match.html'));
beforeEach(module(function($compileProvider) {
Expand Down Expand Up @@ -470,53 +471,53 @@ describe('typeahead tests', function() {
expect($scope.isNoResults).toBeFalsy();
}));
});

describe('select on exact match', function() {
it('should select on an exact match when set', function() {
$scope.onSelect = jasmine.createSpy('onSelect');
var element = prepareInputEl('<div><input ng-model="result" typeahead-editable="false" typeahead-on-select="onSelect()" typeahead="item for item in source | filter:$viewValue" typeahead-select-on-exact="true"></div>');
var inputEl = findInput(element);

changeInputValueTo(element, 'bar');

expect($scope.result).toEqual('bar');
expect(inputEl.val()).toEqual('bar');
expect(element).toBeClosed();
expect($scope.onSelect).toHaveBeenCalled();
});

it('should not select on an exact match by default', function() {
$scope.onSelect = jasmine.createSpy('onSelect');
var element = prepareInputEl('<div><input ng-model="result" typeahead-editable="false" typeahead-on-select="onSelect()" typeahead="item for item in source | filter:$viewValue"></div>');
var inputEl = findInput(element);

changeInputValueTo(element, 'bar');

expect($scope.result).toBeUndefined();
expect(inputEl.val()).toEqual('bar');
expect($scope.onSelect.calls.any()).toBe(false);
});

it('should not be case sensitive when select on an exact match', function() {
$scope.onSelect = jasmine.createSpy('onSelect');
var element = prepareInputEl('<div><input ng-model="result" typeahead-editable="false" typeahead-on-select="onSelect()" typeahead="item for item in source | filter:$viewValue" typeahead-select-on-exact="true"></div>');
var inputEl = findInput(element);

changeInputValueTo(element, 'BaR');

expect($scope.result).toEqual('bar');
expect(inputEl.val()).toEqual('bar');
expect(element).toBeClosed();
expect($scope.onSelect).toHaveBeenCalled();
});

it('should not auto select when not a match with one potential result left', function() {
$scope.onSelect = jasmine.createSpy('onSelect');
var element = prepareInputEl('<div><input ng-model="result" typeahead-editable="false" typeahead-on-select="onSelect()" typeahead="item for item in source | filter:$viewValue" typeahead-select-on-exact="true"></div>');
var inputEl = findInput(element);

changeInputValueTo(element, 'fo');

expect($scope.result).toBeUndefined();
expect(inputEl.val()).toEqual('fo');
expect($scope.onSelect.calls.any()).toBe(false);
Expand Down
25 changes: 21 additions & 4 deletions src/typeahead/typeahead.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
angular.module('ui.bootstrap.typeahead', ['ui.bootstrap.position', 'ui.bootstrap.bindHtml'])
angular.module('ui.bootstrap.typeahead', ['ui.bootstrap.position'])

/**
* A helper service that can parse typeahead's syntax (string provided by users)
Expand Down Expand Up @@ -481,12 +481,29 @@ angular.module('ui.bootstrap.typeahead', ['ui.bootstrap.position', 'ui.bootstrap
};
}])

.filter('typeaheadHighlight', function() {
.filter('typeaheadHighlight', ['$sce', '$injector', '$log', function($sce, $injector, $log) {

var isSanitizePresent;
isSanitizePresent = $injector.has('$sanitize');

function escapeRegexp(queryToEscape) {
// Regex: capture the whole query string and replace it with the string that will be used to match
// the results, for example if the capture is "a" the result will be \a
return queryToEscape.replace(/([.?*+^$[\]\\(){}|-])/g, '\\$1');
}

function containsHtml(matchItem) {
return /<.*>/g.test(matchItem);
}

return function(matchItem, query) {
return query ? ('' + matchItem).replace(new RegExp(escapeRegexp(query), 'gi'), '<strong>$&</strong>') : matchItem;
if(!isSanitizePresent && containsHtml(matchItem)) {
$log.warn('Unsafe use of typeahead please use ngSanitize'); // Warn the user about the danger
}
matchItem = query? ('' + matchItem).replace(new RegExp(escapeRegexp(query), 'gi'), '<strong>$&</strong>') : matchItem; // Replaces the capture string with a the same string inside of a "strong" tag
Copy link
Contributor

Choose a reason for hiding this comment

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

The output from the replace isn't sanitized. Perhaps do this before calling $sanitize on the item.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, on further thought, I realized your initial logic to check for html tags should be used here like:

if (containsHtml(matchItem) && !$sanitize) {
    $log.warn('Unsafe use of typeahead please use ngSanitize');
}

This way, the user will only be warned when using typeahead with labels that contain HTML.

if(!isSanitizePresent) {
matchItem = $sce.trustAsHtml(matchItem); // If $sanitize is not present we pack the string in a $sce object for the ng-bind-html directive
}
return matchItem;
};
});
}]);
2 changes: 1 addition & 1 deletion template/typeahead/typeahead-match.html
Original file line number Diff line number Diff line change
@@ -1 +1 @@
<a href tabindex="-1" bind-html-unsafe="match.label | typeaheadHighlight:query"></a>
<a href tabindex="-1" ng-bind-html="match.label | typeaheadHighlight:query"></a>