Skip to content

Commit

Permalink
Prevent binding to the text nodes emitted from a "text" binding (fixe…
Browse files Browse the repository at this point in the history
…s a potential XSS issue)
  • Loading branch information
SteveSanderson committed Jul 1, 2013
1 parent fb579af commit 0f6e3c9
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 0 deletions.
34 changes: 34 additions & 0 deletions spec/defaultBindings/textBehaviors.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,38 @@ describe('Binding: Text', function() {
expect(testNode).toContainText("xxx ");
expect(testNode).toContainHtml("xxx <!--ko text: undefined--><!--/ko-->");
});

it('Should not attempt data binding on the generated text node', function() {
// Since custom binding providers can regard text nodes as bindable, it would be a
// security risk to bind against user-supplied text (XSS).

// First replace the binding provider with one that's hardcoded to replace all text
// content with a special message, via a binding handler that operates on text nodes
var originalBindingProvider = ko.bindingProvider.instance;
ko.bindingProvider.instance = {
nodeHasBindings: function(node, bindingContext) {
return true;
},
getBindingAccessors: function(node, bindingContext) {
if (node.nodeType === 3) {
return {
replaceTextNodeContent: function() { return "should not see this value in the output"; }
};
} else {
return originalBindingProvider.getBindingAccessors(node, bindingContext);
}
}
};
ko.bindingHandlers.replaceTextNodeContent = {
update: function(textNode, valueAccessor) { textNode.data = valueAccessor(); }
};

// Now check that, after applying the "text" binding, the emitted text node does *not*
// get replaced by the special message.
testNode.innerHTML = "<span data-bind='text: sometext'></span>";
ko.applyBindings({ sometext: 'hello' }, testNode);
expect("textContent" in testNode ? testNode.textContent : testNode.innerText).toEqual('hello');

ko.bindingProvider.instance = originalBindingProvider;
});
});
5 changes: 5 additions & 0 deletions src/binding/defaultBindings/text.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
ko.bindingHandlers['text'] = {
'init': function() {
// Prevent binding on the dynamically-injected text node (as developers are unlikely to expect that, and it has security implications).
// It should also make things faster, as we no longer have to consider whether the text node might be bindable.
return { 'controlsDescendantBindings': true };
},
'update': function (element, valueAccessor) {
ko.utils.setTextContent(element, valueAccessor());
}
Expand Down

0 comments on commit 0f6e3c9

Please sign in to comment.