From 904e30a0e2add577426a46cac7f21e697fd17fe3 Mon Sep 17 00:00:00 2001 From: "jeremy@jermolene.com" Date: Thu, 12 May 2022 16:26:33 +0100 Subject: [PATCH] Detect recursion by tracking widget tree depth The old recursion marker approach was very slow, and didn't catch test cases like editions/test/tiddlers/tests/data/transclude/Recursion.tid --- core/modules/widgets/error.js | 63 +++++++++++++++++++ core/modules/widgets/transclude.js | 40 +----------- core/modules/widgets/widget.js | 56 ++++++++++++----- .../tests/data/transclude/Recursion.tid | 17 +++++ editions/test/tiddlers/tests/test-widget.js | 4 +- 5 files changed, 126 insertions(+), 54 deletions(-) create mode 100644 core/modules/widgets/error.js create mode 100644 editions/test/tiddlers/tests/data/transclude/Recursion.tid diff --git a/core/modules/widgets/error.js b/core/modules/widgets/error.js new file mode 100644 index 000000000..6a4a607f1 --- /dev/null +++ b/core/modules/widgets/error.js @@ -0,0 +1,63 @@ +/*\ +title: $:/core/modules/widgets/error.js +type: application/javascript +module-type: widget + +Error widget + +\*/ +(function(){ + +/*jslint node: true, browser: true */ +/*global $tw: false */ +"use strict"; + +var Widget = require("$:/core/modules/widgets/widget.js").widget; + +var ErrorWidget = function(parseTreeNode,options) { + this.initialise(parseTreeNode,options); +}; + +/* +Inherit from the base widget class +*/ +ErrorWidget.prototype = new Widget(); + +/* +Render this widget into the DOM +*/ +ErrorWidget.prototype.render = function(parent,nextSibling) { + this.parentDomNode = parent; + this.computeAttributes(); + this.execute(); + var message = this.getAttribute("$message","Unknown error"), + domNode = this.document.createElement("span"); + domNode.appendChild(this.document.createTextNode(message)); + domNode.className = "tc-error"; + parent.insertBefore(domNode,nextSibling); + this.domNodes.push(domNode); +}; + +/* +Compute the internal state of the widget +*/ +ErrorWidget.prototype.execute = function() { + // Nothing to do for a text node +}; + +/* +Selectively refreshes the widget if needed. Returns true if the widget or any of its children needed re-rendering +*/ +ErrorWidget.prototype.refresh = function(changedTiddlers) { + var changedAttributes = this.computeAttributes(); + if(changedAttributes["$message"]) { + this.refreshSelf(); + return true; + } else { + return false; + } +}; + +exports.error = ErrorWidget; + +})(); diff --git a/core/modules/widgets/transclude.js b/core/modules/widgets/transclude.js index 27d380c54..940ce814c 100755 --- a/core/modules/widgets/transclude.js +++ b/core/modules/widgets/transclude.js @@ -62,25 +62,10 @@ TranscludeWidget.prototype.execute = function() { parseTreeNodes = [{type: "text", text: plainText}]; break; } - // Set context variables for recursion detection - var recursionMarker = this.makeRecursionMarker(); - if(this.recursionMarker === "yes") { - this.setVariable("$transclusion",recursionMarker); - } // Set the legacy transclusion context variables only if we're not transcluding a variable if(!this.transcludeVariable) { - var legacyRecursionMarker = this.makeLegacyRecursionMarker(); - this.setVariable("transclusion",legacyRecursionMarker); - } - // Check for recursion - if(target.parser) { - if(this.parentWidget && this.parentWidget.hasVariable("$transclusion",recursionMarker)) { - parseTreeNodes = [{type: "element", tag: "span", attributes: { - "class": {type: "string", value: "tc-error"} - }, children: [ - {type: "text", text: $tw.language.getString("Error/RecursiveTransclusion")} - ]}]; - } + var recursionMarker = this.makeRecursionMarker(); + this.setVariable("transclusion",recursionMarker); } // Construct the child widgets this.makeChildWidgets(parseTreeNodes); @@ -335,29 +320,10 @@ TranscludeWidget.prototype.getTransclusionSlotValue = function(name,defaultParse } }; -/* -Compose a string comprising the attributes and variables to identify this transclusion for recursion detection -*/ -TranscludeWidget.prototype.makeRecursionMarker = function() { - var marker = { - attributes: {}, - variables: {} - } - $tw.utils.each(this.attributes,function(value,name) { - marker.attributes[name] = value; - }); - for(var name in this.variables) { - if(name !== "$transclusion") { - marker.variables[name] = this.getVariable(name); - } - }; - return JSON.stringify(marker); -}; - /* Compose a string comprising the title, field and/or index to identify this transclusion for recursion detection */ -TranscludeWidget.prototype.makeLegacyRecursionMarker = function() { +TranscludeWidget.prototype.makeRecursionMarker = function() { var output = []; output.push("{"); output.push(this.getVariable("currentTiddler",{defaultValue: ""})); diff --git a/core/modules/widgets/widget.js b/core/modules/widgets/widget.js index fc90a7dc2..e0ee547c3 100755 --- a/core/modules/widgets/widget.js +++ b/core/modules/widgets/widget.js @@ -12,6 +12,9 @@ Widget base class /*global $tw: false */ "use strict"; +/* Maximum permitted depth of the widget tree for recursion detection */ +var MAX_WIDGET_TREE_DEPTH = 1000; + /* Create a widget object for a parse tree node parseTreeNode: reference to the parse tree node to be rendered @@ -375,6 +378,20 @@ Widget.prototype.assignAttributes = function(domNode,options) { } }; +/* +Get the number of ancestor widgets for this widget +*/ +Widget.prototype.getAncestorCount = function() { + if(this.ancestorCount === undefined) { + if(this.parentWidget) { + this.ancestorCount = this.parentWidget.getAncestorCount() + 1; + } else { + this.ancestorCount = 0; + } + } + return this.ancestorCount; +}; + /* Make child widgets correspondng to specified parseTreeNodes */ @@ -382,21 +399,30 @@ Widget.prototype.makeChildWidgets = function(parseTreeNodes,options) { options = options || {}; this.children = []; var self = this; - // Create set variable widgets for each variable - $tw.utils.each(options.variables,function(value,name) { - var setVariableWidget = { - type: "set", - attributes: { - name: {type: "string", value: name}, - value: {type: "string", value: value} - }, - children: parseTreeNodes - }; - parseTreeNodes = [setVariableWidget]; - }); - $tw.utils.each(parseTreeNodes || (this.parseTreeNode && this.parseTreeNode.children),function(childNode) { - self.children.push(self.makeChildWidget(childNode)); - }); + // Check for too much recursion + if(this.getAncestorCount() > MAX_WIDGET_TREE_DEPTH) { + // Error message needs special permission not to cause a recursive error loop + this.children.push(this.makeChildWidget({type: "error", attributes: { + "$message": {type: "string", value: $tw.language.getString("Error/RecursiveTransclusion")} + }})); + } else { + // Create set variable widgets for each variable + $tw.utils.each(options.variables,function(value,name) { + var setVariableWidget = { + type: "set", + attributes: { + name: {type: "string", value: name}, + value: {type: "string", value: value} + }, + children: parseTreeNodes + }; + parseTreeNodes = [setVariableWidget]; + }); + // Create the child widgets + $tw.utils.each(parseTreeNodes || (this.parseTreeNode && this.parseTreeNode.children),function(childNode) { + self.children.push(self.makeChildWidget(childNode)); + }); + } }; /* diff --git a/editions/test/tiddlers/tests/data/transclude/Recursion.tid b/editions/test/tiddlers/tests/data/transclude/Recursion.tid new file mode 100644 index 000000000..23ec97b13 --- /dev/null +++ b/editions/test/tiddlers/tests/data/transclude/Recursion.tid @@ -0,0 +1,17 @@ +title: Transclude/Recursion +description: Transclusion recursion detection +type: text/vnd.tiddlywiki-multiple +tags: [[$:/tags/wiki-test-spec]] + +title: Output + +\whitespace trim +\procedure recurse(a:0) +<$transclude $variable="recurse" a={{{ [add[1]] }}}/> +\end + +<> ++ +title: ExpectedResult + +

Recursive transclusion error in transclude widget

\ No newline at end of file diff --git a/editions/test/tiddlers/tests/test-widget.js b/editions/test/tiddlers/tests/test-widget.js index 10c45317e..51f5dfbd1 100755 --- a/editions/test/tiddlers/tests/test-widget.js +++ b/editions/test/tiddlers/tests/test-widget.js @@ -143,7 +143,7 @@ describe("Widget module", function() { var wiki = new $tw.Wiki(); // Add a tiddler wiki.addTiddlers([ - {title: "TiddlerOne", text: "<$transclude tiddler='TiddlerTwo'/>\n"}, + {title: "TiddlerOne", text: "<$transclude tiddler='TiddlerTwo'/>"}, {title: "TiddlerTwo", text: "<$transclude tiddler='TiddlerOne'/>"} ]); // Test parse tree @@ -157,7 +157,7 @@ describe("Widget module", function() { // Render the widget node to the DOM var wrapper = renderWidgetNode(widgetNode); // Test the rendering - expect(wrapper.innerHTML).toBe("Recursive transclusion error in transclude widget\n\n"); + expect(wrapper.innerHTML).toBe("Recursive transclusion error in transclude widget"); }); it("should deal with SVG elements", function() {