Merge pull request #1152 from pimterry/spot-extra-closing-comments

Throw errors if virtual tree traversal finds an unmatched ko closing comment
knockout · Oct 1, 2017 · 7c61332 · 7c61332
2 parents 0729145 + b670e94
commit 7c61332
Show file tree

Hide file tree

Showing 2 changed files with 69 additions and 7 deletions.
diff --git a/spec/bindingAttributeBehaviors.js b/spec/bindingAttributeBehaviors.js
@@ -323,6 +323,48 @@ describe('Binding attribute syntax', function() {
         expect(testNode).toContainText('Hello Bert, Goodbye');
     });
 
+    it('Should reject closing virtual bindings without matching open, when found as a sibling', function() {
+        testNode.innerHTML = "<div></div><!-- /ko -->";
+        expect(function() {
+            ko.applyBindings(null, testNode);
+        }).toThrow();
+    });
+
+    it('Should reject closing virtual bindings without matching open, when found as a a first child', function() {
+        testNode.innerHTML = "<div><!-- /ko --></div>";
+        expect(function() {
+            ko.applyBindings(null, testNode);
+        }).toThrow();
+    });
+
+    it('Should reject closing virtual bindings, when found as first child at the top level', function() {
+        testNode.innerHTML = "<!-- /ko -->";
+        expect(function() {
+            ko.applyBindings(null, testNode);
+        }).toThrow();
+    });
+
+    it('Should reject duplicated closing virtual bindings', function() {
+        testNode.innerHTML = "<!-- ko if: true --><div></div><!-- /ko --><!-- /ko -->";
+        expect(function() {
+            ko.applyBindings(null, testNode);
+        }).toThrow();
+    });
+
+    it('Should reject opening virtual bindings that are not closed', function() {
+        testNode.innerHTML = "<!-- ko if: true -->";
+        expect(function() {
+            ko.applyBindings(null, testNode);
+        }).toThrow();
+    });
+
+    it('Should reject virtual bindings that are nested incorrectly', function() {
+        testNode.innerHTML = "<!-- ko if: true --><div><!-- /ko --></div>";
+        expect(function() {
+            ko.applyBindings(null, testNode);
+        }).toThrow();
+    });
+
     it('Should be able to access virtual children in custom containerless binding', function() {
         var countNodes = 0;
         ko.bindingHandlers.test = {

diff --git a/src/virtualElements.js b/src/virtualElements.js
@@ -24,12 +24,19 @@
         return (node.nodeType == 8) && endCommentRegex.test(commentNodesHaveTextProperty ? node.text : node.nodeValue);
     }
 
+    function isUnmatchedEndComment(node) {
+        return isEndComment(node) && !(ko.utils.domData.get(node, matchedEndCommentDataKey));
+    }
+
+    var matchedEndCommentDataKey = "__ko_matchedEndComment__"
+
     function getVirtualChildren(startComment, allowUnbalanced) {
         var currentNode = startComment;
         var depth = 1;
         var children = [];
         while (currentNode = currentNode.nextSibling) {
             if (isEndComment(currentNode)) {
+                ko.utils.domData.set(currentNode, matchedEndCommentDataKey, true);
                 depth--;
                 if (depth === 0)
                     return children;
@@ -133,19 +140,32 @@
         },
 
         firstChild: function(node) {
-            if (!isStartComment(node))
+            if (!isStartComment(node)) {
+                if (node.firstChild && isEndComment(node.firstChild)) {
+                    throw new Error("Found invalid end comment, as the first child of " + node);
+                }
                 return node.firstChild;
-            if (!node.nextSibling || isEndComment(node.nextSibling))
+            } else if (!node.nextSibling || isEndComment(node.nextSibling)) {
                 return null;
-            return node.nextSibling;
+            } else {
+                return node.nextSibling;
+            }
         },
 
         nextSibling: function(node) {
-            if (isStartComment(node))
+            if (isStartComment(node)) {
                 node = getMatchingEndComment(node);
-            if (node.nextSibling && isEndComment(node.nextSibling))
-                return null;
-            return node.nextSibling;
+            }
+
+            if (node.nextSibling && isEndComment(node.nextSibling)) {
+                if (isUnmatchedEndComment(node.nextSibling)) {
+                    throw Error("Found end comment without a matching opening comment, as child of " + node);
+                } else {
+                    return null;
+                }
+            } else {
+                return node.nextSibling;
+            }
         },
 
         hasBindingValue: isStartComment,